Repository navigation
fix: Restore MCP OAuth Browser Binding 🪢 - #16634
Conversation
|
@codex review Please review the current remote head c24bd81 of this PR. Confirm that this exact head is the reviewed commit. Focus on browser binding before token exchange or persistence, preservation of valid CSRF/session callbacks, wrong-user and absent-cookie rejection, and chat/SSE binding and resume compatibility. The base-to-head diff removes only the insecure pending-flow authorization fallback; lifecycle protections are preserved. |
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. |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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.
Copilot review overview
🟡 Changes recommended
A stale cross-account session cookie can still prevent the documented legitimate callback recovery path.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Restores browser binding for MCP OAuth callbacks to prevent authorization from an unrelated browser.
Changes:
- Requires a valid flow-CSRF or initiating-user session cookie.
- Adds route and browser regression coverage.
- Removes obsolete insecure-fallback tests.
| File | Description |
|---|---|
api/server/routes/mcp.js |
Removes pending-flow authorization fallback. |
api/server/routes/__tests__/mcp.spec.js |
Tests callback binding and rejection paths. |
e2e/specs/mock/mcp-oauth-resume.spec.ts |
Tests isolated-browser rejection and stream recovery. |
packages/api/src/mcp/__tests__/MCPOAuthCSRFFallback.test.ts |
Removes obsolete fallback tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@codex review Please review the exact current remote head f0706b2. The additional compatibility fix refreshes invalid or cross-account browser session bindings at authenticated initiation/bind routes. A real route cookie-jar regression covers account switch → bind → stale callback clears CSRF → legitimate session-bound callback. All 160 MCP route tests, the isolated-browser resume test, API typecheck, and affected static checks pass. Please confirm the reviewed commit matches this full head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0706b20cf
ℹ️ 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".
|
@codex review Please review the exact current remote head 8af0a09. After two actionable findings I audited the full browser-binding subsystem by invariant: authenticated cookie producers, untrusted cookie-parser values, both validators, success/error callbacks, state/expiry/supersession/cancellation, persistence, chat bind-before-sign-in, reload/resume, and rollout. Both validators now reject non-string values and unequal UTF-8 buffer lengths without throwing. All 166 route tests, API typechecking, and affected static checks pass; regression coverage includes account-switch recovery using the actual bind producer and cookie jar, malformed CSRF/session cookies, and isolated-browser callback rejection. Please assess the full current diff and confirm this exact reviewed commit. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review Retrying the review service failure (Unknown error), with no code changes. Please review the exact current remote head 8af0a09 and confirm this reviewed commit. The full subsystem invariant audit and regression evidence are in the PR description; the two prior actionable findings are fixed and resolved. |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |

Summary
A fresh pending MCP OAuth flow currently lets a callback bypass both browser-binding cookies. Someone who initiates a flow can share its authorization URL, and a different browser's provider grant can then be saved for the initiating account. I restored the requirement for a valid per-flow CSRF cookie or an initiating-user session cookie before any code exchange or credential persistence, closing the regression of GHSA-vf7j-7mrx-hp7g.
How it works
Chat/SSE flows already call the authenticated bind endpoint before enabling sign-in. Settings initiation also sets the binding cookies. No new configuration, cookie format, API contract, credential format, or frontend behavior is introduced.
Change Type
Testing
npx tsc --noEmitinpackages/api.git diff --check.npm run lighthouseand repeated the Lighthouse run on final head8af0a09c14: median LCP 3,488 ms / 4,500 ms, CLS 0.018 / 0.1, TBT 62 ms / 500 ms, with 250 ms injected database latency.Test Configuration
macOS, Node 24; real Express callback and cookie validation, mocked external exchange/storage in route tests; Playwright production client with disposable in-memory MongoDB and local MCP/provider fixtures. Lighthouse injects 250 ms per Mongo query.
Risk / compatibility
Callbacks from a browser that has lost both binding cookies now fail closed. Re-initiate authorization in the originating browser to restore binding; pending server state alone is deliberately insufficient. Existing current clients already bind before sign-in. During a rolling deployment, an old server replica still permits the insecure fallback until every replica is updated. There is no migration or stored-token change.
I checked the binding producers, success/error callbacks, token persistence, retry/supersession, and resume path against source. The base graph matches the exact
devSHA and identifies the MCP and Actions cookie-helper callers; the Actions callback already requires binding and is unchanged.The invariant audit distinguishes untrusted cookie-parser values from authenticated producer identity and cached flow metadata: only string cookies with matching character/byte lengths reach constant-time comparison; missing/invalid bindings cannot authorize callbacks or mutate flows; authenticated producers replace wrong-account bindings; valid current-account cookies retain their lifetime; state/lifecycle checks still gate persistence. The unindexed branch additions were reconciled directly against source, rather than inferred from absent graph edges.
The final Codex review found no major issues on
8af0a09c14fa9fced7ebd8e56d0c07611370b186. Both actionable review threads are fixed with regression coverage and resolved. The first review attempt on this final head failed with a service error; the same-head retry completed successfully without further code changes. CI is still running; no failures have been reported.Checklist