Skip to content

Support confidential clients with client_secret_post - #3082

Open
jeremy wants to merge 5 commits into
oauth-refresh-tokensfrom
oauth-confidential-clients
Open

Support confidential clients with client_secret_post#3082
jeremy wants to merge 5 commits into
oauth-refresh-tokensfrom
oauth-confidential-clients

Conversation

@jeremy

@jeremy jeremy commented Aug 28, 2026

Copy link
Copy Markdown
Member

Stacked on the refresh-token PR. Third gap-closer for #2296, matching what beta2's bc3 authorization server advertises.

8080b3f adds confidential client support:

  • Clients carry a token_endpoint_auth_method (none or client_secret_post). Confidential clients get a generated client_secret at creation, returned once in the DCR response (.compact keeps it out of public-client responses).
  • The token endpoint authenticates confidential clients on both grants, answering invalid_client 401 when the posted secret is missing or wrong. Public loopback clients are unchanged.
  • Discovery advertises both auth methods; DCR rejects anything else (e.g. client_secret_basic).
  • Secrets are stored plaintext with constant-time comparison, matching how the stack already stores access tokens.

Revocation stays unauthenticated by design: revoking requires holding the token itself, which is the same capability the credential guards.

Copilot AI balanced review requested due to automatic review settings August 28, 2026 06:16
@jeremy jeremy mentioned this pull request Aug 28, 2026
4 tasks

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread app/controllers/oauth/tokens_controller.rb Outdated
Comment thread app/controllers/oauth/clients_controller.rb
Comment thread app/controllers/oauth/metadata_controller.rb

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread app/controllers/oauth/clients_controller.rb
@jeremy
jeremy force-pushed the oauth-confidential-clients branch 2 times, most recently from e5b198c to 40d32e9 Compare August 28, 2026 06:26

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread db/migrate/20260827110000_add_oauth_client_secrets.rb
Comment thread app/models/oauth/client.rb
Comment thread app/controllers/oauth/metadata_controller.rb
Comment thread app/controllers/oauth/clients_controller.rb
@jeremy
jeremy force-pushed the oauth-confidential-clients branch from 40d32e9 to c1e15ca Compare August 28, 2026 06:40

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread app/controllers/oauth/tokens_controller.rb Outdated
@jeremy
jeremy force-pushed the oauth-confidential-clients branch from 0502fd0 to fe5aab8 Compare August 28, 2026 06:48

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment thread app/controllers/oauth/tokens_controller.rb Outdated
jeremy and others added 5 commits August 31, 2026 12:54
beta2's bc3 authorization server advertises client_secret_post, and
hosted connectors register as confidential clients. Clients now carry a
token_endpoint_auth_method (none or client_secret_post); confidential
clients get a generated client_secret at creation, returned once in the
DCR response.

The token endpoint authenticates confidential clients on both grants,
answering invalid_client 401 when the posted secret is missing or
wrong. Public loopback clients are unchanged. Discovery advertises both
auth methods.

Revocation stays unauthenticated by design: revoking requires holding
the token itself, which is the same capability the credential guards.
A confidential client posting client_secret as an array or hash passed
the presence check and blew up in secure_compare — an unauthenticated
500 where invalid_client belongs. Require a string before comparing.

Token and registration responses carry bearer credentials, so they now
answer Cache-Control: no-store and Pragma: no-cache per RFC 6749 §5.1
and RFC 7591.
client_secret_post authenticates with both client_id and client_secret
in the request body (RFC 6749 §2.3.1); the token endpoint now checks
the posted client_id against the authenticating client on every grant.

Registration responses issuing a client_secret now carry
client_secret_expires_at: 0 (never expires), required by RFC 7591
§3.2.1, and the discovery test pins both advertised auth methods.
Rails merges query-string params, so POST /oauth/token?client_id=…&
client_secret=… authenticated — leaving reusable secrets in proxy and
access logs. Read credentials from request_parameters, so query-string
credentials fail with invalid_client.
When a valid refresh token belonged to a confidential client but the request
omitted client_id, validate_refresh_client ran first and returned invalid_grant,
so the authentication failure never reached authenticate_client's 401
invalid_client. A client can read invalid_grant as a revoked or expired refresh
token and discard a still-valid grant. Run authenticate_client after the token
is loaded but before validate_refresh_client so client-auth failures surface as
invalid_client.
@jeremy
jeremy force-pushed the oauth-confidential-clients branch from fe5aab8 to 92b3eb2 Compare August 31, 2026 19:56
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-31T20:03:26.480077Z 92b3eb2 New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

credentials = request.request_parameters

unless credentials["client_id"] == client.client_id && client.authenticate_secret(credentials["client_secret"])
oauth_error "invalid_client", "Client authentication failed", status: :unauthorized

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Return 400 for client_secret_post failures

When body-based client_secret_post credentials are missing or incorrect, this returns HTTP 401 without a WWW-Authenticate challenge. RFC 6749 §5.2 defaults token errors to 400 and reserves 401 for indicating supported HTTP authentication schemes; HTTP 401 responses also require a challenge. Because this server advertises no header-based client authentication scheme, strict OAuth/HTTP clients can reject or mishandle this response; return :bad_request for these body-credential failures.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants