Repository navigation
Accept https redirect URIs for dynamically registered clients - #3080
Conversation
There was a problem hiding this comment.
Pull request overview
Expands dynamic OAuth client registration to support non-loopback HTTPS redirect URIs while preserving loopback HTTP behavior.
Changes:
- Adds HTTPS redirect validation to the model and DCR endpoint.
- Adds model and integration coverage for accepted and rejected redirects.
- Verifies exact-match authorization for HTTPS callbacks.
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 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
app/models/oauth/client.rb |
Extends redirect validation for HTTPS URIs. |
app/controllers/oauth/clients_controller.rb |
Allows qualifying HTTPS redirects during DCR. |
test/models/oauth/client_test.rb |
Tests the expanded model policy. |
test/integration/oauth_flow_test.rb |
Tests DCR and authorization matching behavior. |
Suppressed comments (2)
app/controllers/oauth/clients_controller.rb:58
- The finite string list does not establish that this is a non-loopback host. For example,
https://127.0.0.2/callback(the 127/8 loopback range) and alternate IPv6 loopback spellings pass this validation, although HTTPS loopback URIs are meant to remain rejected. Use a shared host classifier that recognizes loopback IP ranges/normalized literals (while retaining the intended localhost handling).
parsed.scheme == "https" && parsed.host.present? && !Oauth::LOOPBACK_HOSTS.include?(parsed.host)
app/models/oauth/client.rb:61
- This model check has the same inverted allow-list problem as the DCR endpoint: a real loopback address not spelled exactly like an entry in
LOOPBACK_HOSTS, such as127.0.0.2, is accepted as an HTTPS non-loopback host. That lets direct model creation persist redirect URIs the stated policy rejects. Share a normalized IP-range-aware loopback classifier with the controller.
parsed.scheme == "https" && parsed.host.present? && !parsed.host.in?(Oauth::LOOPBACK_HOSTS)
💡 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: 9f7fa82122
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8710ca34b9
ℹ️ 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".
8710ca3 to
a0405b0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0405b06ac
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d3b27724fa
ℹ️ 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".
d3b2772 to
0e4c5ae
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. |
66d7cd5 to
a883a73
Compare
a883a73 to
3a69a29
Compare
|
Rebased onto #2296's authorization-endpoint fix (CSP form-action, RFC 9207 iss, canonical scope). The only conflicts were tests and helpers that sat next to each other. Content is otherwise unchanged. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a69a2971d
ℹ️ 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".
5e09643 to
1f2263a
Compare
1f2263a to
704b3dc
Compare
704b3dc to
7163917
Compare
|
RFC 9700 §4.11.2 (authorization server as open redirector), re-audited at this head. The fix is already here in 7163917: 60de0c1 adds tests only. Every validation that redirects an error now has a case: Not done: limiting https DCR to vetted hosts, which is bc3's approach. It would undo the point of this PR, so it is a product call. #3081 and #3082 were rebased onto this head with no changes. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60de0c1156
ℹ️ 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".
60de0c1 to
891f9a3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 891f9a3ece
ℹ️ 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".
891f9a3 to
d154d6f
Compare
Port-flexible loopback matching was gated on every registered URI being loopback, so a client registering both a loopback and an https callback lost the flexibility for its loopback URI. Match each presented URI on its own merits instead, which leaves loopback? without callers — gone. Model validation also accepted an empty fragment (https://x/cb#) that the DCR endpoint rejects: URI#fragment returns "" there, which present? misses. Reject any non-nil fragment so both layers agree.
…onsent Hostnames are case-insensitive but the loopback checks weren't, so https://LOCALHOST/callback slipped past the https-loopback rejection. All loopback classification now funnels through Oauth.loopback_host?, which downcases first. The consent screen also labeled every dynamically registered client a local tool. Hosted clients now show the redirect host instead, so the person authorizing can see where access is headed.
https://%6cocalhost slipped past the enumerated loopback list: Ruby preserves the encoding in URI#host while user agents decode it, so the two layers disagreed about where the redirect lands. Decode before comparing. Deeper canonicalization (punycode, alternate IP spellings like 127.1) stays out of scope: a miss classifies the host as non-loopback, which is the stricter branch — exact matching only — and the redirect still targets the user's own machine, granting the registrant nothing.
An unauthenticated DCR request supplying a host like https://%FF/callback made Oauth.loopback_host? decode to an invalidly-encoded string and raise ArgumentError on downcase, which no caller rescued — returning a 500 instead of an invalid_redirect_uri error. Convert the decode failure into URI::InvalidURIError at the single shared chokepoint so every caller (all of which already rescue it) rejects the host cleanly.
Registration rejects fragments, but the loopback port exception matched only scheme, host and path, so it admitted an unregistered redirect carrying one (RFC 6749 §3.1.2).
Validation errors are found before the user has seen the consent screen. Redirecting them to a self-registered https host would let anyone register a URL and use us to bounce a signed-in user to it unprompted (RFC 9700 §4.11.2). Those errors now render locally. Loopback and operator-provisioned redirects still get the error redirect, and Deny still redirects because the consent screen has named the host by then.
…https host Each validation that redirects an error (scope, state, response_type, code_challenge_method) now has a case showing it renders here for an unvetted https client, and an operator-provisioned https client still gets the redirect. Guards RFC 9700 §4.11.2 against a new validation that bypasses redirect_with_error's vetting.
… on consent Codex's last four findings on this PR came from the same root: a self-registered redirect's authority could say one thing and mean another. A percent-encoded host showed raw on consent although the browser decodes it. Userinfo was ignored when a loopback port varied. An out-of-range port registered fine but stranded the user after consent. A nondefault port was left out of the consent warning. Rather than canonicalize the authority everywhere it is shown or compared, registration now accepts only a plain one: no userinfo, a host of letters, digits, hyphens and dots (or a bracketed IPv6 literal), and a port from 1 to 65535 (Oauth.plain_authority?). The model and the DCR endpoint share that rule. Loopback port variation also requires matching userinfo. The consent warning names host:port whenever the port isn't the scheme's default.
http://%5B%3A%3A1%5D:8888/callback decodes to the [::1] loopback, but browsers reject it as a URL. The plain-authority rule already refuses it, and this test keeps it refused.
A mixed client's loopback redirect let an authorization request vary the port to 65536 or 0. Ruby parses those ports, but browsers reject the URL, so the user was stranded after consent. The presented URI must now meet the same plain-authority rule as a registered one, port range included.
Browsers parse a host whose last label is all digits as IPv4, so a redirect like https://999.999.999.999/callback registered fine but browsers reject it. Within the plain-authority rule, such a host must now be four octets, each 0-255. DNS names, including ones with numeric inner labels, are unchanged.
Browsers read a leading zero as octal, so https://0177.0.0.1/callback registered as an outside host but navigates to 127.0.0.1. That slipped past the https-loopback refusal and the consent warning. 08.0.0.1 registered but browsers reject it. A host whose last label is a number, decimal or 0x hex as in WHATWG's ends-in-a-number check, must now be four decimal octets from 0 to 255 with no leading zeroes.
#2296 now accepts a request without state, so the error left to pin for a self-registered https host is a state past 2048 characters.
13b0822 to
5d74b53
Compare
|
Review threads: 1 resolved (0 fixed, 1 declined with the reasoning in the thread). |
* commit '5d74b5309ec2fe6646709d8faa38fa3a31b28525': Pin a too-long state, not a missing one, as a pre-consent error Accept a numeric host only as canonical decimal octets Take a numeric last label as IPv4, and require a valid dotted quad Vary a loopback port only to a presented URI with a plain authority Pin that a percent-encoded IPv6 loopback host doesn't register Register only redirects whose authority is plain, and show their port on consent Pin that a mixed registration never varies an https loopback port (Codex) Pin every pre-consent error against redirecting to a self-registered https host Render pre-consent errors here for self-registered https clients Never vary a loopback port onto a redirect with a fragment Reject redirect hosts whose percent-encoding is invalid UTF-8 Decode percent-encoded hosts before loopback classification Downcase hosts before loopback checks and name the redirect host on consent Match loopback redirects per URI and reject empty fragments Accept https redirect URIs for dynamically registered clients
* origin/oauth: Make the OAuth migrations safe to run again after stopping partway
* origin/oauth: Answer 404 from a dark endpoint however many times it's asked
* tp/oauth: (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: Bring main's tests up to this branch: Ruby 4 and the access token finder
* tp/oauth: 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
* tp/oauth: Filter the code parameter by its whole name, so code_challenge stays readable
Stacked on #2296 (base:
oauth). First of three gap-closers taking the minimal OAuth 2.1 stack to connector grade.Hosted MCP connectors register over DCR with https callback URLs, which the loopback-only policy rejected. 9f7fa82 widens the registration policy:
trustedflag stays manual provisioning only — dynamically registered https clients remain untrusted and go through the consent screen.Model validation and the DCR endpoint enforce the same policy, with model and integration tests for both sides plus exact-match authorization behavior for https redirects.
Next in stack: refresh tokens + expiry, then confidential clients.