[codex] Fix OAuth resource indicator selection#1987
[codex] Fix OAuth resource indicator selection#1987chelojimenez wants to merge 1 commit intomainfrom
Conversation
chelojimenez
temporarily deployed
to
preview-pr-1987
GitHub Actions
Inactive
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Internal previewPreview URL: https://mcp-inspector-pr-1987.up.railway.app |
dosubot
Bot
added
size:L
WalkthroughThis change refactors OAuth resource parameter handling across the SDK. Rather than deriving the resource solely from Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 6/8 reviews remaining, refill in 10 minutes and 56 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/tests/oauth/state-machine-regressions.test.ts (1)
124-178: ⚡ Quick winAdd one rejection-path regression alongside this happy-path case.
This locks down the security boundary you just introduced. A cross-origin value, a sibling path, or a fragment-only
resourceshould fall back instead of propagating into the authorization URL and token request body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tests/oauth/state-machine-regressions.test.ts` around lines 124 - 178, Add a second test in the same suite that mirrors the happy-path but uses a rejected resource value (e.g., cross-origin URL, sibling path, or fragment-only string) in state.resourceMetadata.resource and asserts the state machine does not propagate it into the authorization URL or token request body; specifically, create a state like the existing one (using createOAuthStateMachine, currentStep "received_client_credentials", protocolVersion "2025-11-25"), call machine.proceedToNextStep() twice to reach "authorization_request" and assert that new URL(state.authorizationUrl).searchParams.get("resource") is null/empty, then simulate receiving an authorization code (set currentStep to "received_authorization_code" and authorizationCode), call proceedToNextStep() and assert that (state.lastRequest?.body as Record<string,string>).resource is undefined/absent. Ensure the test covers at least one of: cross-origin host, sibling path, or fragment-only resource values to validate the fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sdk/tests/oauth/state-machine-regressions.test.ts`:
- Around line 124-178: Add a second test in the same suite that mirrors the
happy-path but uses a rejected resource value (e.g., cross-origin URL, sibling
path, or fragment-only string) in state.resourceMetadata.resource and asserts
the state machine does not propagate it into the authorization URL or token
request body; specifically, create a state like the existing one (using
createOAuthStateMachine, currentStep "received_client_credentials",
protocolVersion "2025-11-25"), call machine.proceedToNextStep() twice to reach
"authorization_request" and assert that new
URL(state.authorizationUrl).searchParams.get("resource") is null/empty, then
simulate receiving an authorization code (set currentStep to
"received_authorization_code" and authorizationCode), call proceedToNextStep()
and assert that (state.lastRequest?.body as Record<string,string>).resource is
undefined/absent. Ensure the test covers at least one of: cross-origin host,
sibling path, or fragment-only resource values to validate the fallback
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0baad53d-473e-4481-99a7-1b746d624996
📒 Files selected for processing (7)
sdk/src/oauth-conformance/checks/oauth-negative.tssdk/src/oauth-conformance/runner.tssdk/src/oauth-login.tssdk/src/oauth/state-machines/debug-oauth-2025-06-18.tssdk/src/oauth/state-machines/debug-oauth-2025-11-25.tssdk/src/oauth/state-machines/shared/urls.tssdk/tests/oauth/state-machine-regressions.test.ts
chelojimenez
force-pushed
the
codex/fix-oauth-resource-indicator
branch
from
6120bd5 to
7f8d685
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/tests/oauth/state-machine-regressions.test.ts (1)
141-156: ⚡ Quick winAssert the executed token POST, not just the staged state.
This test stops at
token_requestand inspects the preview object instate.lastRequest.body. The real form-encoded body is assembled on the next step, so a regression there would still pass. Advance once more with a mocked token response and assert therequestExecutorcall contains the expectedresourcevalue in the encoded body.Also applies to: 172-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/tests/oauth/state-machine-regressions.test.ts` around lines 141 - 156, The test currently stops at the token_request preview and only checks state.lastRequest.body; instead, advance the state machine one more step by simulating a mocked token response so the actual form-encoded token POST is assembled, then assert the mocked requestExecutor was invoked and that its called request body contains the expected resource parameter (check the encoded body string, not the preview object). Update the test around createOAuthStateMachine / token_request to invoke the next transition with a fake token response and replace the existing state.lastRequest.body assertion with an assertion on requestExecutor.mock.calls (or similar) verifying the resource value is present in the encoded body.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sdk/tests/oauth/state-machine-regressions.test.ts`:
- Around line 141-156: The test currently stops at the token_request preview and
only checks state.lastRequest.body; instead, advance the state machine one more
step by simulating a mocked token response so the actual form-encoded token POST
is assembled, then assert the mocked requestExecutor was invoked and that its
called request body contains the expected resource parameter (check the encoded
body string, not the preview object). Update the test around
createOAuthStateMachine / token_request to invoke the next transition with a
fake token response and replace the existing state.lastRequest.body assertion
with an assertion on requestExecutor.mock.calls (or similar) verifying the
resource value is present in the encoded body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a6c7b49-5276-4e00-8d2f-33ce6ee2e11c
📒 Files selected for processing (4)
sdk/src/oauth/state-machines/debug-oauth-2025-06-18.tssdk/src/oauth/state-machines/debug-oauth-2025-11-25.tssdk/src/oauth/state-machines/shared/urls.tssdk/tests/oauth/state-machine-regressions.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/oauth/state-machines/shared/urls.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7f8d685. Configure here.
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Exported function has no external consumers
Low Severity
isOAuthResourceIndicatorAllowed is exported but never imported anywhere outside urls.ts. A grep across the entire sdk directory shows only two references — the definition at line 48 and the internal call at line 97 within resolveOAuthResourceIndicator. The export keyword is unnecessary since the function is only used as a private helper in the same file.
Reviewed by Cursor Bugbot for commit 7f8d685. Configure here.
1 participant
Summary
resourcevalue when the browser OAuth state machine builds authorization and token resource indicators./mcpendpoints whose protected-resource metadata advertises the origin as the OAuth resource.Scope
This is not implemented as a literal guest-only conditional. The failing hosted guest path goes through the browser callback/debug state-machine path (
source: callback), while signed-in hosted workspace callbacks typically complete through the hosted server-side callback path. The fix is scoped to that browser state-machine path rather than to SDK CLI login or conformance client-credentials helpers.Root Cause
The browser OAuth state machine was requesting tokens with
resource=https://.../mcp. Servers like Linear and BART advertise the protected resource as the origin, e.g.https://mcp.linear.apporhttps://bart.vanshaj.dev. The authorization flow could complete, but the resulting access token had the wrong resource/audience for the MCP initialize request, so servers rejected it with401 invalid_token.Validation
npm run test -w @mcpjam/sdk -- --run tests/oauth/state-machine-regressions.test.tsnpm run typecheck -w @mcpjam/sdkgit diff --check HEAD~1..HEADNote
Medium Risk
Changes how the OAuth
resourceparameter is selected for authorization/token requests and JWT audience checks, which can affect login/token issuance behavior across servers if the new validation/fallback logic is wrong.Overview
OAuth flows now prefer the protected-resource metadata
resourcevalue when computing the RFC8707 resource indicator, rather than always using the configured MCP endpoint URL.Adds
resolveOAuthResourceIndicator(with fragment stripping plus same-origin and path-prefix validation) and switches bothdebug-oauth-2025-06-18anddebug-oauth-2025-11-25state machines to use it consistently for diagram display, auth URL construction, token requests, and decoded tokenaudvalidation.Adds a regression test ensuring the 2025-11-25 state machine uses the advertised metadata
resourcefor both authorization and token requests (and updates an expected error string formatting).Reviewed by Cursor Bugbot for commit 7f8d685. Bugbot is set up for automated code reviews on this repo. Configure here.