fix!: authenticate confidential clients at OAuth2 token revocation - #29208
fix!: authenticate confidential clients at OAuth2 token revocation#29208BobbyHo wants to merge 8 commits into
Conversation
RevokeToken read client_secret into its request struct and never looked at it again. Anyone holding a token value and the app's public client_id could revoke that token, a denial of service against one integration. RFC 7009 section 2.1 has the server validate the client's credentials first and only then check that the token was issued to it. The handler now calls authenticateClient for confidential clients before the token is classified, answering 401 invalid_client on failure so an unauthenticated caller learns nothing about the token. The ownership checks in both revoke branches are unchanged, so an authenticated client still receives the RFC 7009 section 2.2 silent 200 for a token it does not own. Public clients are unchanged. The revocation parser did not accept HTTP Basic, the method RFC 6749 section 2.3.1 requires servers to support and the one Coder's own MCP client uses when it revokes. It now folds Basic credentials in through mergeBasicClientAuth exactly as the token endpoint does, including the 400 for credentials that conflict between header and body. codersdk.RevokeOAuth2Token gains a clientSecret parameter; public clients pass an empty string. Its comment claimed the middleware authenticated the client, which was the belief behind this defect. BREAKING CHANGE: a confidential client revoking without its client_secret, or with a wrong one, now receives 401 invalid_client and nothing is revoked. Sending the secret, in the form or as HTTP Basic, succeeds. Go callers of codersdk.RevokeOAuth2Token must pass the new clientSecret argument. The RFC 7009 endpoint is documented for the first time. Closes PLAT-507 (CDR-59) and the revocation half of SEC-348.
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
…-revoke-client-auth
…-revoke-client-auth
…-revoke-client-auth
47ffe36 to
6106a24
Compare
…-revoke-client-auth
…-revoke-client-auth
|
/coder-agents-review |
|
Chat: Review in progress (18/18 reviewers complete) | View chat deep-review v0.9.0 | Round 1 | Last posted: Round 1, 11 findings (2 P3, 2 Nit, 7 Note), COMMENT. Review Finding inventoryFinding inventory — PR #29208Findings
Round logRound 1Netero-only first pass: 1 Note (error-mapping duplication), clean floor, panel proceeded. Panel of 17 (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Kurapika, Razor, Knov, Kite, Ging-go, Ryosuke, Chopper, Gon, Leorio, Komugi, Meruem, Zoro) plus Netero. No P0-P2. 2 P3, 2 Nit, 7 Note posted; 1 Note dropped (benign). Reviewed against 0cdf1b8..ab0e2ac. Ging-go, Komugi, Mafu-san, and several others reported no findings; the change is a clean, well-tested closure of the last unauthenticated OAuth2 secret-reading endpoint (SEC-348). About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a clean, tightly-scoped close of the last OAuth2 endpoint that read a client secret without checking it. Client authentication runs before the token is classified, so an unauthenticated confidential caller gets a uniform 401 whether the token is valid, foreign, or unknown, which shuts the token-existence oracle RFC 7009 §2.2 would otherwise open; MissingSecretUnknownToken proves it. The change reuses the authenticateClient/mergeBasicClientAuth helpers from #29206 instead of reimplementing auth, preserves the ownership check by making the cross-app test authenticate the other app as itself, keeps public clients bound by client_id alone, and pairs every refusal test with a works() probe so a 401-that-still-revokes could not pass. IsPublic() fails closed on an unrecognized client type. Five reviewers, including the Go-idiom, determinism, and process passes, returned no findings.
No P0-P2. Severity count: 2 P3, 2 Nit, 7 Note.
Two items worth a decision. The stale RevokeToken doc comment (CRF-2) still tells the next reader that revocation authenticates on token validity alone, which is the exact false belief the PR description says caused this bug; you corrected it on the SDK method but left it standing on the handler. The per-request warn on a keyless, unrate-limited route (CRF-3) lets anyone who knows a confidential client_id flood the log stream. It is the same class the #29207 panel raised as P3 on the token endpoint and is not a new capability, but keeping Warn vs dropping to Debug plus an aggregate metric is a human call, not something to leave implicit. Everything else is Note/Nit polish.
As Bisky put it after trying to break each of the nine subtests: "These stones cut glass."
coderd/oauth2provider/revoke.go:61
P3 [CRF-2] The RevokeToken doc comment still describes the pre-PR auth model and never mentions the client authentication this PR adds. (Leorio)
The comment reads: "Authentication is unique for this endpoint in that it does not use the standard token authentication middleware. Instead, it expects the token that is being revoked to be valid." That was true before this PR. [...] The next developer reading this comment learns that revocation authenticates on token validity alone. That is the exact false belief the commit says caused the original bug.
You fixed this belief in the SDK method's comment but left it standing on the handler that implements the fix. On a security-critical path the record should name both halves: authenticate the confidential client by client_secret/Basic per RFC 7009 §2.1, then verify the token was issued to that client, with public clients bound by client_id alone.
🤖
coderd/oauth2_test.go:80
Nit [CRF-7] The new //nolint comments copy the OAauth2 misspelling. (Gon)
The three
nolintlines this PR adds readOAauth2 app management requires owner permission. It should beOAuth2.
The typo already appears many times in this file, so the PR propagates an existing class rather than introducing it; fixing the new instances (and optionally the siblings) in one pass removes it. Low impact, but it is a copied error in nolint justifications a reader will read.
🤖
🤖 This review was automatically generated with Coder Agents.
… test comments Log client_id and app_name on the authentication refusal to match the other lines in RevokeToken. Say in the docs that revocation authenticates as a refresh does and show where the form-credential example lives. Trim two test comments to the behavior under test.
…-revoke-client-auth
1 participant
TL;DR
Third of three for SEC-348. The revocation endpoint read
client_secretand never checked it. Anyone holding a token value and the app's publicclient_idcould revoke that token, ending another client's session. RFC 7009 section 2.1 has the server validate the client's credentials first and only then check that the token belongs to it. The ownership check exists; this is the authentication half. Closes PLAT-507 (CDR-59).codersdk.RevokeOAuth2Tokengains aclientSecretparameter, so Go callers must pass one (empty for public clients).Implementation Details
The check
RevokeTokencallsauthenticateClientfor confidential clients after the form is parsed and before the token is classified. Failure answers 401invalid_clientwithWWW-Authenticate, and is logged at warn with the app id and nothing from the body.HTTP Basic
client_id, so Basic appeared to work because the password was never examined.mergeBasicClientAuthfrom refactor(coderd/oauth2provider): extract OAuth2 client authentication helpers #29206. A header and body that disagree answer 400, with the token endpoint's wording.The SDK
RevokeOAuth2Tokensent onlyclient_id. Its comment said the middleware handled client authentication, which is the belief behind this bug. It now takes a secret and the comment says what the handler expects.Docs
Tests
TestOAuth2ProviderRevokeClientAuthentication: missing secret, wrong secret, another app's valid secret, missing secret with an unknown token (401, not 200), correct secret, Basic, Basic with a wrong secret, Basic and body in conflict. Each refusal also checks the session still works.TestOAuth2ProviderRevokeCrossAppandTestOAuth2CoderClientnow pass the secret. The cross-app case authenticates the other app as itself, so the ownership check is still what refuses it.TestOAuth2NoStoreHeaders/Revokechecks the new 401 carriesCache-Control: no-storebefore checking the 200 does.TestOAuth2ProviderPublicClientTokenLifecycle, and the management API'sTestOAuth2ProviderRevoke, which is a different endpoint.Stack: #28752, #29206, #29207, this PR.