feat: Add support for MSC4484 #2190
No reviewers
Labels
No labels
Abandoned
Blocked
Bug
Changelog
Added
Changelog
Missing
Changelog
None
Cherry-picking
Database
Dependencies
Dependencies/Renovate
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/E2EE
Matrix/Federation
Matrix/Hydra
Matrix/MSC
Matrix/Media
Matrix/T&S
Merge
Merge/Manual
Merge/Squash
Meta
Meta/CI
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Support
bob the builder
No assignees
4 participants
Notifications
Due date
No due date set.
Blocks
#2294 feat: Add support for OAuth client credentials grant
continuwuation/continuwuity
Reference
continuwuation/continuwuity!2190
Loading…
Reference in a new issue
No description provided.
Delete branch "ginger/msc4484"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This pull request adds support for MSC4484, MSC4540, and MSC4363, and starts enforcing OAuth scopes on the CS API.
Pull request checklist:
mainbranch, and the branch is named something other thanmain.myself, if applicable. This includes ensuring code compiles.
4db47214eb998d796f38998d796f3898f1ae4bdaWIP: feat: Add support for MSC4484to feat: Add support for MSC4484feat: Add support for MSC4484to WIP: feat: Add support for MSC4484cba84bc314d9d8b3bb24d9d8b3bb24bbcf09e6c0bbcf09e6c09e3d9d8af0require_auth_for_profile_requestswork again5f528b43ac95b6deb5f3WIP: feat: Add support for MSC4484to feat: Add support for MSC44844ea9e5ecfd686c40f3a6686c40f3a69b33f6bc55I ran it and it didn't explode
non-explosions but maybe bug:
I get:
Error: M_INSUFFICIENT_USER_AUTHENTICATION (HTTP 401): You do not have permission to access this endpoint.when looking up profiles, and sometimes when resolving aliases.works now, no notes
@ -387,0 +330,4 @@scopes};if !required_scopesIf there are no required scopes this will always be true, which means NoAccessToken routes that pass an auth token will always fail I think?
NoAccessToken passes ApiFullAccess as of
432971daac. this array should never be completely empty because all ruma CSAPI routes require ApiFullAccess@ -107,2 +96,3 @@f.write_str(&urn)if let Some(device_id) = &self.device_id {scopes.push(device_id.to_string());You forgor
format!("urn:matrix:client:device:{device_id}")@ -648,9 +686,12 @@ impl Service {assert_eq!(&client_id, &session_info.client_id, "session info client id mismatch");Uh this might allow anyone to just crash things if client_id is client controlled
Same with the assert above
the client ID has to refer to valid client metadata (see
issue_token) so these asserts can only fail if there's a logic bug@ -559,3 +568,3 @@.ok_or_else(|| OAuthError::invalid_grant("No device ID scope supplied"))?;if selflet session_info = if selfSame in this function
not sure what you're referring to?
Forgejo reordered my comments! I think this was
client_id actually matches?@ -106,0 +107,4 @@let RequestedScopes { device_id, mut scopes } =query.scope.to_scopes().map_err(WebError::BadRequest)?;let existing_session_info = if let Some(device_id) = &device_id {Do you check that the session's client_id actually matches here? Might be able to override other devices not connected to the session to escalate privilege.
The client id only identifies the OAuth client's metadata (and is easy to publicly derive).
create_sessioncan't add temporary scopes to another user's device because the device metadata keyspace is keyed by the user who owns the device. It would be weird if a device's session was for a different client ID, but it wouldn't be a security issue.I mean specifically a client that hasn't been granted admin step-up could escalate? idk seems sussy anyway
Whether or not a device can step up to the admin scope depends on if the authorizing user is an administrator, which is checked at the start of
create_session. Even if it were possible to grant scopes to a device owned by another user (it's not), you would just be giving them admin powers, which isn't a privilege escalation issue as you'd already need to be an admin to do that.Any client can ask for any scope, that's not part of the client metadata.
@ -12,1 +11,4 @@user-select: none;svg {vertical-align: center;do you mean
middle?@ -8,0 +14,4 @@Access your homeserver's administrator tools</li>{% when _ %}<li>missingno</li>Unknown permission? I don’t know something more descriptive than this
that case should never trigger unless there's a logic issue, so I want it to be obviously wrong so devs can easily see it and users are more likely to report it if it somehow makes it into production
Then say that!
🗿
432971daac76aab88cea76aab88cea1f5a754433