CodeIssuesPull RequestsActionsSecurityInsights
✨ AI
More
Settings

fix(oauth): the consent screen never showed where the tokens go #5590

MergedXSccantynz wants to mergefix/oauth-consent-shows-destinationmainopened 1d ago
ccantynzcommented 1d ago

External security report, 2026-08-30. The core of it is right, and the fix is one screen rather than the one they proposed.

/oauth/register is unauthenticated by design — claude.ai connectors, Cursor and Copilot self-register, and MCP depends on it. So anyone can register a client with a name, a description and a redirect they control. All three are attacker-chosen. The screen asking a user to approve it showed the name, the description and the scopes — and never the destination. A user had no way to tell a real connector from a hostile one.

The fix

Show the one fact on that page the registrant cannot fake: where the code is about to be delivered.

Host only, not the full URI — a path is attacker-controlled text, long enough to push the domain off screen, which is the oldest trick there is. An unparseable URI is shown verbatim rather than hidden: a destination you can't read still beats one you can't see.

Also replaces by unknown. Measured the same day: 38 OAuth clients, all with owner_id NULL, including the real claude.ai connectors. That line was printed for every app and distinguished nothing — noise wearing the costume of provenance. A self-registered client now says so.

Deliberately not changed, and a test records the decision

Open registration stays. The report's first remediation — require an initial access token, reject unauthenticated registration with 401 — would break every MCP connector to fix a consent screen. Registration being open is the feature; the screen was the gap. Rate limiting stays, because open ≠ unlimited.

Two other points don't hold, recorded so nobody re-litigates them from the email alone:

  • PKCE is not a mitigation here. The attacker is the client and controls the verifier.
  • The registration_access_token is not a disclosure. RFC 7592 returns it to the registrant to manage their own client — which is who received it.

Severity, honestly

Not unauthenticated account takeover. It requires a logged-in victim to load a consent screen and approve it.

The two clients the researcher registered were live and unrevoked on production; both are now revoked, and oauth_access_tokens shows zero tokens ever issued to either — no account was reached. Revocation is enforced in all three paths: authorize (oauth.tsx:655), decision/token, and token validation (auth.ts:93).

Tests

Seven on the rendering and the deliberate non-change. 53 existing OAuth tests still pass.

ccantynzAI Reviewcommented 1d ago

AI Triage

(no summary)

Priority: medium Risk area: mixed

Suggested labels: (no label suggestions) Suggested reviewers: (no reviewer suggestions)

Suggestions only — nothing has been applied. The PR author stays in control.

gluecron[bot]🤖 botAI Reviewcommented 1d ago

AI review unavailable

The platform's AI balance is exhausted, so AI generation is temporarily unavailable. Nothing was lost. You can queue this as a repair for the internal agent from the repository's Health page, or try again once the balance is restored. The PR is otherwise unchanged.

Cross-repo impact

See what breaks downstream if this PR merges.

Analyze →
⮌ Merged

This pull request was merged into main.

c comment · e edit title · m merge · a approve · r request changes · ? shortcuts