Repository navigation
Support confidential clients with client_secret_post - #3082
Conversation
There was a problem hiding this comment.
Pull request overview
Adds OAuth confidential-client support using client_secret_post, including registration, authentication, discovery metadata, persistence, and tests.
Changes:
- Generates and authenticates confidential-client secrets.
- Extends dynamic registration and discovery metadata.
- Adds database fields, fixtures, and OAuth flow coverage.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
app/controllers/oauth/clients_controller.rb |
Registers confidential clients and returns secrets. |
app/controllers/oauth/metadata_controller.rb |
Advertises client_secret_post. |
app/controllers/oauth/tokens_controller.rb |
Authenticates confidential token requests. |
app/models/oauth/client.rb |
Adds secret generation and verification. |
db/migrate/20260827110000_add_oauth_client_secrets.rb |
Adds client authentication columns. |
db/schema.rb |
Updates MySQL schema. |
db/schema_sqlite.rb |
Updates SQLite schema. |
test/fixtures/oauth/clients.yml |
Adds a confidential-client fixture. |
test/integration/oauth_flow_test.rb |
Tests registration and token flows. |
test/models/oauth/client_test.rb |
Tests secret behavior and validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8080b3f2ac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e5b198c to
40d32e9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40d32e9afb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
40d32e9 to
c1e15ca
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0502fd087d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0502fd0 to
fe5aab8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe5aab8efe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fe5aab8 to
92b3eb2
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92b3eb2b2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
92b3eb2 to
f2b19f1
Compare
f2b19f1 to
61d0f7c
Compare
|
Rebased onto #2296's token-endpoint conformance fix (comment) and #3081's follow-up. The client-auth ordering commit now also covers the code grant.
For that reason the presence check for |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61d0f7ccf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review status on head 61d0f7c: CI is green on all four train heads. One thread is still open. It needs a human decision: authenticate confidential clients at revocation. That is RFC 7009 §2.1, which conflicts with the unauthenticated-revocation posture recorded in this PR's body. Codex reviewed this head. It has not re-reviewed the rebased heads of #2296, #3080 or #3081 within about 7 minutes of the push. |
61d0f7c to
465118e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 465118ed00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ache token errors Refresh resolved the token before authenticating the client, so a wrong secret answered invalid_client for a live refresh token and invalid_grant for a dead one: an oracle for whoever holds a token without the secret. The token endpoint now authenticates the client the request names, before any code or refresh token is looked up. A request attempts authentication when it names a confidential client or carries a client_secret, and must then succeed; a request naming a public client attempts none, and a confidential grant presented that way is refused as issued to another client, the same answer as a dead grant. client_id is required up front again (invalid_request), so its absence can't depend on the grant either. prevent_caching ran as an after_action, which a halted callback chain skips, so every error response from the token and registration endpoints went out without Cache-Control: no-store. It is now a before_action.
RFC 6749 §2.3.1: the authorization server MUST support HTTP Basic for clients issued a password. Oauth::ClientAuthentication reads either transport. It base64-decodes the Basic value strictly, splits on the first colon and form-urldecodes each half (Appendix B). A confidential client's secret verifies whichever way it arrives; the registered method is advisory. Basic plus a non-empty body client_secret is two methods at once, refused as invalid_request (§2.3, §5.2). A malformed header, a body client_id naming another client, or Basic naming a public client fails as invalid_client. Every invalid_client is now a 401 carrying a Basic challenge (§5.2). The earlier 400 was chosen because no header scheme existed to name in the challenge, and Basic now is one. The pilot gate keys on the Basic id. Discovery, DCR and the client model accept client_secret_basic. Mirrors basecamp/bc3#13678.
RFC 7009 §2.1: the server first validates the client's credentials, then checks that the token was issued to that client. Before, any holder of a token could revoke the grant without the client's secret. A confidential client authenticates with client_secret_post or client_secret_basic. A public client names its client_id. Each revokes only tokens issued to it. If the token is unknown, belongs to another client or is a personal access token, the endpoint still answers 200 and revokes nothing (§2.2). It also answers 200 when no client is identified, as with a missing or unknown client_id. Only failed authentication is refused, with 401 invalid_client: a known confidential client with a missing or wrong secret, or a Basic header that doesn't authenticate. That answer depends only on the request, never on whether the token exists. Discovery advertises the three methods for revocation. Mirrors basecamp/bc3's Oauth::RevocationsController.
A body client_secret with no client_id, or with an unknown one, is a failed authentication attempt. Before this it got the 200 that means the token is dead, so a client with a mistyped client_id would stop using a grant that was still live. Revocation now uses the same rule as the token endpoint: Basic, a confidential client, or any secret is an attempt, and an attempt that fails gets 401. The rule turns on the request alone, so the refusal says nothing about which clients exist.
A public client holds no password, so a body client_secret beside its client_id is a failed authentication attempt, the same as a Basic header naming it. Before this, the request identified the public client and got 200, even when the token belonged to another client and nothing was revoked.
…eader RFC 6749 §5.2 requires the WWW-Authenticate challenge on invalid_client when the client attempted authentication through the Authorization header. A client that sent its secret in the body never tried the header, so its 401 now carries no challenge, at the token endpoint and at revocation alike. Any Authorization header still earns one, since a 401 must name a scheme that applies and Basic is the only one here. Status codes are unchanged: invalid_client is 401 either way. This matches basecamp/bc3 (Oauth::ClientAuthentication#challenge_client_authentication).
Restricting revocation to a client's own tokens also stopped personal access tokens from being revoked there, since they belong to no client. Revocation should work the same for every kind of token, and personal access tokens may later become OAuth tokens themselves, as they are in Basecamp. A personal access token has no client, so holding it is the credential: whoever presents one revokes it. Client credentials sent along with it still have to authenticate under the usual rules. A failed attempt gets 401 invalid_client and revokes nothing, and Basic beside a body secret is still invalid_request. OAuth tokens are unchanged: a client revokes only its own.
A request naming a public client makes no authentication attempt. Presenting a confidential client's code or refresh token in that request got "Grant was not issued to this client" when the grant was live, and "Invalid refresh token" when it wasn't. The description was an oracle for whether someone else's grant was live. Both cases now give the dead-grant answer.
Revocation now authenticates confidential clients: a wrong secret gets 401, the right one gets 200. Unthrottled, that let anyone who knew a public client_id guess its secret without limit, around the token endpoint's limit of 20 per minute. Revocation now has the same per-IP limit (RFC 6749 §2.3.1, RFC 7009 §5).
A failed Basic authentication stays a 401 challenging Basic: the endpoint evaluated the Authorization header, so the challenge applies. Every other invalid_client is now a 400 with no challenge: a failed client_secret_post, a public client presenting a secret, a secret that authenticates no client at revocation, or a body failure beside an Authorization header of another scheme, which used to draw a Basic challenge for a scheme never tried. - A bare 401 violates RFC 9110 §15.5.2, which requires a challenge. - A Basic challenge for a request that never used Basic names a scheme that doesn't apply to what was tried. - oauth4webapi and openid-client turn a challenged 401 into a WWWAuthenticateChallengeError instead of invalid_client. - A 401 challenging Basic on a form POST can pop a browser's auth dialog. Revocation keeps its stricter rule, refusing a presented secret that authenticates no client, now with 400.
no-store covered the token and registration endpoints. Discovery, consent, its redirects and revocation answered without it, and every response sent the app's default Referrer-Policy. Now each OAuth response, halted callback chains and the sign-in redirect included, carries Cache-Control: no-store and Referrer-Policy: no-referrer.
"Authorization: Basic" with no credentials didn't match the scheme pattern, so it counted as no attempt: a public client's exchange went ahead, and revocation deleted a presented personal access token. Now it fails as invalid_client with a Basic challenge, like any malformed Basic header.
dc2d791 to
274c566
Compare
|
Review threads: 1 resolved (1 fixed: bare Basic scheme). |
* commit '274c56688b1872aff9b09e10a645e03d54bdd826': (21 commits) Read the grant's refresh token before the bare-Basic attempts, not after reloading Fail a bare Basic scheme as client authentication, not ignore it Keep every authorization server response out of caches and Referers Answer invalid_client with 400 unless Basic authentication failed Throttle revocation like the token endpoint Answer a grant issued to another client exactly as a dead one Revoke personal access tokens at the revocation endpoint again Name the Basic scheme only to a client that tried the Authorization header Refuse a client secret that names a public client at revocation Refuse a revocation whose client secret authenticates no client Authenticate clients at revocation and revoke only their own tokens Accept client_secret_basic at the token endpoint Authenticate the named client before resolving the grant, and never cache token errors Pin that confidential clients sit behind the availability switches Authenticate confidential clients before checking the code's verifier and redirect_uri Answer failed client_secret_post authentication with 400, not 401 Authenticate the client before validating the refresh grant Confine client credentials to the request body Require client_id with client_secret_post and declare secret expiry Reject non-string client secrets and mark credential responses no-store ...
…pping partway * origin/oauth-refresh-tokens: Make the OAuth migrations safe to run again after stopping partway
* origin/oauth-refresh-tokens: Answer 404 from a dark endpoint however many times it's asked
* tp/oauth-refresh-tokens: (60 commits) Name v0.2.0 in the pin's comment Pin fizzy-mcp-server v0.2.0 Refuse a join code that was reset while it was being redeemed Filter presigned storage signatures from logs Keep the reviewer's memberships in reach, and print the member audit escaped Import an export that lacks a column this database has only just gained List the reviewer accounts' members when signing the reviewer out Bump thruster from 0.1.26 to 0.1.27 (#3199) Give the asset build a placeholder direct-upload credential Anchor the stubbed storage host pattern Sign direct uploads with a write-only storage credential Lock the reviewer's email address, so a code holder can't move its membership out of revocation's reach Use Ruby's GC defaults and log job memory (#3238) Pin the fizzy_mcp accessory to fizzy-mcp-server v0.1.0 by digest Scope webhook dispatch by account as well as board Test the reviewer audit against the structured log line production writes Revoke on the email alone, and record each use in the request's structured log Remove the reviewer's access tokens and passkeys on sign-out, so a code holder can't keep a way back in Wire the reviewer sign-in into production from Deploy/Fizzy Close the reviewer's live sockets on sign-out, and keep the identity id out of Sentry ...
* tp/oauth-refresh-tokens: Bring main's tests up to this branch: Ruby 4 and the access token finder
* tp/oauth-refresh-tokens: Pin the load balancers to kamal-proxy v0.10.2, and say why configure-lb-mcp.sh still checks Give the load balancer the real client IP
… a secret A confidential client's secret is stored as given, so every literal on oauth_clients is dropped from logs like the other credential tables'.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
* tp/oauth-refresh-tokens: Filter the code parameter by its whole name, so code_challenge stays readable
Stacked on the refresh-token PR. Third gap-closer for #2296, matching what beta2's bc3 authorization server advertises. Two conformance decisions also land here: D3 (client_secret_basic) and D4 (revocation client authentication). This PR owns client authentication, and both decisions reverse lines that were in this body before. D1 and D2 are stacked above it in #3160 and #3161.
1ca8787 adds confidential client support:
token_endpoint_auth_method(none,client_secret_postorclient_secret_basic). Confidential clients get a generatedclient_secretat creation. It is returned once in the DCR response, and.compactkeeps it out of public-client responses.D3: client_secret_basic (d65d011)
RFC 6749 §2.3.1 says the server MUST support HTTP Basic for clients issued a password. This mirrors basecamp/bc3#13678 (
Oauth::ClientAuthentication).Oauth::ClientAuthenticationreads either transport. It base64-decodes the Basic value strictly, splits on the first colon, and form-urldecodes each half (Appendix B), so%3Abecomes:and+becomes a space (bc3). A confidential client's secret verifies whichever way it arrives, because the registered method is advisory.Ambiguity (bc3):
client_secret(even an identical one)invalid_request: more than one method (§2.3, §5.2)client_idnaming the same client, or an emptyclient_secret=client_idnaming another clientinvalid_clientinvalid_clientinvalid_client: a public client holds no passwordinvalid_clientis 401 +WWW-Authenticate: Basic realm="<issuer>"only for a failed Basic attempt, and 400 with no challenge for everything else (b6b8329). "Everything else" is a failedclient_secret_post, a public client presenting a secret, a secret that authenticates no client, and a body failure beside an Authorization header of another scheme (which until now drew a Basic challenge). Why:WWWAuthenticateChallengeErrorinstead ofinvalid_client.This is the policy settled across bc3, Fizzy and HEY after an adversarial review; it supersedes the earlier "every
invalid_clientis 401" (9b27557). Fizzy has no deployed clients, so nothing depends on the old status. Status, body and header remain identical for an unknown client and a wrong secret.The pilot gate keys on the Basic id. Discovery advertises
token_endpoint_auth_methods_supported: [none, client_secret_post, client_secret_basic], and DCR acceptsclient_secret_basic. bc3 pins DCR tonone, but Fizzy's DCR already acceptedclient_secret_post, so Basic joins it.D4: revocation client authentication (e80b5b6)
RFC 7009 §2.1: validate the client's credentials, then check that the token was issued to that client. This mirrors bc3's
Oauth::RevocationsController, and the Basic branch mirrors #13678.invalid_client, no challenge, nothing revokedinvalid_client+WWW-Authenticate: Basic, whatever the tokenclient_id, its own tokenclient_id, or an unknown one, without Basic or a client secretclient_id, an unknown one, or a public client, which holds no password)invalid_client(c9375f1, 29a5d11). bc3 answers 200 here; Fizzy treats any presented secret as an authentication attempt, as its token endpoint doesinvalid_client(401 + Basic if via Basic, else 400), nothing revokedThe row for a missing or unknown
client_idfollows bc3. When no client is identified, the request has failed to identify a client rather than failed authentication, so it gets the non-oracular 200 and notinvalid_client. Personal access tokens stay revocable here (98619ab, Jeremy's decision: revocation is uniform across token kinds, and PATs may later be rebuilt on OAuth, see #3164). A PAT has no client, so holding it is the credential. Client credentials presented alongside it must still pass the rules above. Discovery advertisesrevocation_endpoint_auth_methods_supported: [none, client_secret_post, client_secret_basic].The old line "Revocation stays unauthenticated by design" is withdrawn. Codex's revocation thread asked for exactly this change.
Tests
test/integration/oauth_client_secret_basic_test.rbandtest/integration/oauth_revocation_test.rbare new. I wrote them first: 20 failures and 1 error before the change, and the 3 that passed were guards for behavior that already existed. They cover Basic-only refresh and code exchange,%3A/+decoding, both-methods → 400, header/body id mismatch, five malformed shapes, Basic naming a public client, unknown client vs wrong secret giving identical answers, a post failure → 400 without the challenge (also beside an Authorization header of another scheme), the pilot gate reading the Basic id, a drift test (every advertised secret method authenticates), DCR, the revocation rows above, including the cross-client revoke, and personal-access-token revocation, which is possession-based but still refused on failed client authentication. A revoked PAT then stops authenticating. Existing tests expect 400 for a non-Basicinvalid_client(written first: 17 failures before b6b8329), and revocation requests in them name theirclient_id.Locally:
bin/rails test test/integration test/models test/controllerspasses (1517 tests).Shipped-client matrix for the
invalid_client401 → 400 changeNone of the OAuth endpoints are live yet, since this train is unmerged. No first-party Fizzy client calls
/oauth/token,/oauth/revocationor any other/oauth/*or/.well-known/oauth-*path. I checked every tag and every remote branch./oauth/*v1.0-233, no force-updatesession_tokencookie1.1.3-14, no force-updateLoginApi.kt:31-56@1.1.3-14), then the cookiev4.0.1internal/client/client.go:238,648@v4.0.1)v0.2.4go/pkg/fizzy/auth_strategy.go:31,51); itsAGENTS.md:60says "no OAuth"The PAT and session flows don't touch these endpoints: PATs go only to API routes as Bearer, and sessions go to
/session*. The Fizzy server has no minimum-version gate. The only OAuth-shaped client code was a HEY-copiedCalendarAccessTokenUpdater.swifton the fizzy-iosmulti-identity-setupbranch. It never shipped, was removed in6920f9f, and treated 400 and 401 alike. The planned hosted-MCP exchange client (fizzy-mcp-serverdocs/hosted-plan) should expect a 400invalid_clientfor a body-auth failure.