On a closer look, I think there wasn't any security impact, the worst thing that would happen is that some actions done by OAuth tools could be performed on behalf of a temporary account instead of a user who revoked a scope. I think it was like that for years. Once we broke things in dc8b4323f, this started returning errors instead. Please double-check, but I think we can make this public.

Apps using client credentials flow were completely broken since dc8b4323f. I haven't heard any complaints, and we don't have good observability for this… I think we can look at the rate of mwoauth_invalid_authorization_invalid_user errors on this dashboard, and it went up on June 11 when the buggy patch went live, but then it subsided. Maybe whoever was using the client credentials grant shut down their app?

I also filed T433732 while testing this.

Proposed patches for this:

I wrote myself an OAuth 2 app for testing these workflows. It's shoddy (vibe-coded, barely reviewed), but with some work this could be the start of an end-to-end test suite we could run with Selenium (see also T341759). Don't run it locally without adjusting, since it messes with the wiki's LocalSettings.php and database, you'll need to adjust some paths at least. In general I suggest keeping agents and agent-generated code in a virtual machine :)

The steps I followed while testing could become 4 end-to-end tests (expected behavior in parentheses):

1. client_creds_access_token (OK)
2. api_query (OK) —
   1. not affected by patch 1
   2. without patch 2, this step fails
3. api_edit (OK, saved as IP/temp user) —
   1. not affected by patch 1
   2. without patch 2, this step fails

1. auth_code_access_token (OK)
2. api_query (OK)
3. api_edit (OK)
4. refresh_revoke (OK)
5. api_query (OK)
6. api_edit (OK)

1. auth_code_access_token (OK)
2. [onwiki] Use Special:OAuthManageMyGrants/revoke on the newly approved app
3. api_query (fails)
4. api_edit (fails)
5. refresh_revoke (fails)

1. auth_code_access_token (OK)
2. [onwiki] Use Special:OAuthManageMyGrants/update on the newly approved app and remove 'editpage' scope
3. api_query (OK)
4. api_edit (OK) — but maybe it should fail, see T433732
5. refresh_revoke (OK)
6. api_query (OK) —
   1. before patches: fails with mwoauth-invalid-authorization-invalid-user
   2. either patch makes it succeed
7. api_edit (fails with permissiondenied) —
   1. before patches: fails with mwoauth-invalid-authorization-invalid-user
   2. note that patch 2 without patch 1 makes it incorrectly succeed, and edit is saved as IP/temp user

(I haven't tested this with OAuth 1 at all, that needs checking)

That means that we issue an access token with scopes that the user may have revoked in the meantime via OAuthManageMyGrants.

Not even necessarily in the meanwhile, IIUIC. Authorize an app -> revoke a bunch of rights -> get an access token -> get a refresh token. Since the refresh token will just load all the scopes from the client, you have all rights again.
This is wrong even if we remove partial revocations, but add partial authorizations some time in the future.

It would be more reasonable to either inherit the scopes that the access token had (but probably not easily accessible at that point?) or take them from the consumer acceptance record.

In T433556#12176040, @matmarex wrote:

Please double-check, but I think we can make this public.

I think partial revocation issues are in general very low-key, so even if it were truly broken, it would be fine to go through Gerrit. Maybe make the commit summaries a bit vague about the implications.

Apps using client credentials flow were completely broken since dc8b4323f.

That doesn't seem to be the case - here are a bunch of apps that only used the client credentials flow recently, and here are all their Logstash events. They include things like CirrusSearch and slow-parse that should happen well after authentication.

Proposed patches for this:

0001-Filter-out-revoked-scopes-when-issuing-access-tokens.patch1 KBDownload

0002-Restore-support-for-anonymous-client-credentials-app.patch1 KBDownload

CR+1, haven't tested yet.

I wrote myself an OAuth 2 app for testing these workflows.

Very cool :)

In T433556#12176040, @matmarex wrote:

[..]with some work this could be the start of an end-to-end test suite we could run with Selenium (see also T341759).

We talked a bit about this at T415281: [EPIC] OAuth extension critical workflows (for automated tests enhancement), IMO Selenium is not particularly useful here as 1) the UI interactions are very limited, and we basically just want to make sure that a given request ends in a given response, so simulating a browser is unnecessary, 2) they are slow and fragile. We should go with something like BrowserKit (and so a test suite that's CLI based rather than UI-based).

The UI-based test suite would be nice for beta/production (as a powered-up version of the hello-world apps), not sure that's important enough to be worth the effort though.

I've also been working on a small Python app for manual testing (T429528: Create oauth tester library - it's basically done, just dragging my feet on the last 10% of making logging levels configurable and such). That's not useful for automation, but handy for manual testing.

In T433556#12180679, @Tgr wrote:

In T433556#12176040, @matmarex wrote:

Apps using client credentials flow were completely broken since dc8b4323f.

That doesn't seem to be the case - here are a bunch of apps that only used the client credentials flow recently, and here are all their Logstash events. They include things like CirrusSearch and slow-parse that should happen well after authentication.

The second dashboard only shows owner-only apps. They have oarc_oauth2_allowed_grants set to ["client_credentials"], but this doesn't really do anything, and it's set that way here: https://gerrit.wikimedia.org/g/mediawiki/extensions/OAuth/+/8ae014a07926fb8ce53aefc2feeb1b7dd2dc7a1b/src/Frontend/SpecialPages/SpecialMWOAuthConsumerRegistration.php#743 (which is a bit weird – maybe they should have an empty list of grants? but that's a question for another day)

The first dashboard shows that some clients are fetching access tokens via the client credentials grant, but it doesn't show whether they are using them, and if they are, whether or not they are getting errors. Fetching access tokens works, only using them fails. Sorry, "completely broken" was not very clear.

I updated the title to reflect that a) there is no security issue here b) there are two distinct problems (but we only discovered that while investigating the first problem, so it all got jumbled together in one task). I will submit the patches for review to Gerrit.

Change #1324398 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/OAuth@master] Filter out revoked scopes when issuing access tokens for refresh token

https://gerrit.wikimedia.org/r/1324398

Change #1324399 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/OAuth@master] Restore support for (anonymous) client credentials apps

https://gerrit.wikimedia.org/r/1324399

Change #1324398 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Filter out revoked scopes when issuing access tokens for refresh token

https://gerrit.wikimedia.org/r/1324398

Change #1324399 merged by jenkins-bot:

[mediawiki/extensions/OAuth@master] Restore support for (anonymous) client credentials apps

https://gerrit.wikimedia.org/r/1324399