Repository navigation
fix(oauth): forbid framing of the consent and setup pages and show where approval sends access - #747
aliasunder wants to merge 18 commits into
Conversation
Both Express apps now mount one middleware ahead of everything else. It sets X-Frame-Options: DENY, Content-Security-Policy: frame-ancestors 'none', X-Content-Type-Options: nosniff and Referrer-Policy: same-origin, and both apps stop sending X-Powered-By. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every /oauth/decide response, including the form parser's errors, and every setup-mode response now sends Cache-Control: no-store, as does the full server's /setup page. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the vault scope The consent page now shows the destination of Approve (the redirect address's scheme and host, or the address as written when it has no host) and a fixed description of the grant, in place of the scope list. New sign-ins always get the vault scope; refresh grants keep their stored scope. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and the response headers Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![umm-actually[bot]](https://avatars.githubusercontent.com/in/4233716?s=60&v=4){kind=small}
|
umm-actually re-reviewed at No new findings (4 tracked finding(s) across all runs). Context notes
umm-actually · z-ai/glm-5.3-flash |
…curately The server's error handler answers a form parser error with a 500, so the comment no longer names a 413. Ship-Check: pr-review · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ments - Scope the "mounted first" comments in both apps to what mounting first does, and name the consent and setup pages in the content-policy comment. - Say why the consent page always shows the scheme, why it does not use URL.origin, and that the SDK passes only addresses URL.parse can read. - State that `vault` is the server's only scope, and that any consent action but Approve denies. - Point setup-server at the scripts that set SETUP_REASON and derive PUBLIC_URL, and say why it keeps its own error middleware. - SECURITY.md: name the redirect address plainly, say which 404 page sets its own content policy, and list every page that sends no-store. - ARCHITECTURE.md: say where a refresh grant with another scope comes from. Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the reveal toggle's variables The page shows the client ID when a client has no name; both render paths passed the same fallback before. The Show/Hide toggle's inline script on the consent and setup pages uses descriptive names. The Referrer-Policy comment says what a null Origin would hide. Ship-Check: triage · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…and pin the setup page's reveal toggle - The consent decision's no-store checks move out of the approve, reject and 400 tests into their own cases, so a missing header fails tests named for caching. - The 404 page's framing check moves from the root-redirect test into the security headers block. - "keeps the scope a request names" becomes "grants vault when the request names vault", matching the fixed grant. - The setup page's Show/Hide handler gets the pin the consent page's has. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…andles The page replaces the content policy for a known path called with an unanswered method too, such as GET /oauth/decide, not only for an unknown path. Ship-Check: bug-check · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ship-Check: triage · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e paragraph Every new sign-in now gets this scope whatever it asks for, so the DEFAULT_SCOPE name no longer described it. Ship-Check: triage · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n oversized form The consent page's no-store comes from the MCP SDK's authorize handler; the integration test now asserts it, so the SECURITY.md claim stays true if the SDK changes. The oversized-form case notes that only the bare router answers 413. The vault-scope test group drops its old default-scope title. Ship-Check: pr-monitor · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
CSP header on consent page overwrites the frame-ancestors-only policy
The oauth-routes.ts response sets Content-Security-Policy to 'default-src none' inside the same send callback that follows the global middleware's frame-ancestors-only CSP; Express replaces the earlier header value rather than adding a second one, so the consent page is served with only default-src 'none' and loses the frame-ancestors 'none' directive that ARCHITECTURE.md and SECURITY.md state every response carries. Failure scenario: Load the consent page and inspect response headers: Content-Security-Policy is 'default-src none' with no frame-ancestors directive, so a page can embed the consent screen in an iframe despite the documented protection. Suggested fixInclude frame-ancestors 'none' in the CSP value set in the /authorize response callback, e.g. "default-src 'none'; frame-ancestors 'none'", or verify res.append is used instead of res.set so both policies are emitted.umm-actually · z-ai/glm-5.3-flash |
|
Re: umm-actually comment 6713588389 ("CSP header on consent page overwrites the frame-ancestors-only policy"): This is intentional, and the described header does not occur. The only place in 🔍 ship-check · pr-monitor · claude-opus-5-5 |
…e consent page its own paragraph Ship-Check: code-quality · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cacheable The router-only test answers an oversized form with the parser's 413. On the full server the error middleware answers the same error with a 500, and the response must still carry no-store. Ship-Check: test-audit · claude-opus-5-5 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
The OAuth consent page and the setup pages can no longer be shown inside another site's frame, and the consent page now shows where Approve sends access and what it grants.
:remoteimage's setup mode:X-Frame-Options: DENY,Content-Security-Policy: frame-ancestors 'none',X-Content-Type-Options: nosniffandReferrer-Policy: same-origin.X-Powered-Byis no longer sent. One middleware insrc/utils/security-headers.ts, mounted first in both apps.X-Frame-Optionsbut notframe-ancestors; it still cannot be framed./oauth/decide, set ahead of the form parser so parser errors carry it too) or of any setup-mode response, plus the configured server'sGET /setuppage.https://claude.ai,http://127.0.0.1:19876), or the whole address when it has no host (claude.ai:callback). Only a web address can start withhttps://.vault, the server's only advertised scope, whatever scope the client asks for. The token response'sscopesays so. Refresh grants already stored keep their scope; nothing reads a scope, so access is unchanged.Docs: ARCHITECTURE.md (Auth) and a new SECURITY.md "Response headers" section.
Test plan
npm test: all pass except load timeouts in two shell-script test files this change does not touch, which pass when run alonenpm run build:server,npm run lint,npm run knipx-powered-byswitch, eachno-storeline, or restoring the requested-scope grant fails the matching tests🤖 Generated with Claude Code