Accept https redirect URIs for dynamically registered clients - #3080
Accept https redirect URIs for dynamically registered clients#3080jeremy wants to merge 4 commits into
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".
Hosted MCP connectors register over DCR with https callback URLs, which the loopback-only policy rejected. Registration now accepts redirect URIs that are either http loopback (unchanged, with port-flexible matching) or https on a non-loopback host (exact matching, no fragments). Plain http on public hosts, custom schemes, and https loopback stay rejected. Operator-provisioned clients are unaffected; the trusted flag remains manual provisioning only.
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.
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".
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.
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".
| LOOPBACK_HOSTS = %w[ 127.0.0.1 localhost ::1 [::1] ] | ||
|
|
||
| def self.loopback_host?(host) | ||
| LOOPBACK_HOSTS.include?(URI.decode_www_form_component(host.to_s).downcase) |
There was a problem hiding this comment.
When an unauthenticated DCR request supplies a syntactically parseable host such as https://%FF/callback, URI.decode_www_form_component returns an invalidly encoded string and downcase raises ArgumentError: input string invalid. The controller only rescues URI::InvalidURIError, so this malformed registration returns a 500 instead of the expected OAuth invalid_redirect_uri response; validate the decoded encoding or handle this exception as an invalid host.
Useful? React with 👍 / 👎.
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.