fix: prefer IdP-granted scopes over client-requested scopes in OAuthProxy#3492
fix: prefer IdP-granted scopes over client-requested scopes in OAuthProxy#3492
Conversation
marvin-context-protocol
Bot
added
bug
![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: 609efca236
ℹ️ 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".
| # requested scope). Fall back to requested scopes only when the IdP | ||
| # omits scope, meaning it granted exactly what was requested. | ||
| granted_scopes: list[str] = ( | ||
| idp_tokens["scope"].split() |
There was a problem hiding this comment.
This new split() assumes the IdP returns space-delimited scopes, but some providers return comma-delimited strings (for example repo,gist), so the proxy will treat multiple granted scopes as a single scope token. In that case FastMCP stores/mints incorrect scopes (and the same parsing pattern is used again in refresh handling), which can cause valid scope checks and refresh-scope subset checks to fail because repo is no longer present as an individual scope. Using parse_scopes here would preserve compatibility with non-space scope formats.
Useful? React with 👍 / 👎.
Some providers (e.g. GitHub) return comma-delimited scopes like "repo,gist" rather than the RFC 6749 space-delimited format.
…dp-granted-scopes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d956b62e36
ℹ️ 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".
| http_client = self.httpx_client_factory( | ||
| headers=headers, | ||
| auth=self.auth, | ||
| follow_redirects=True, # type: ignore[call-arg] | ||
| **({"timeout": timeout} if timeout else {}), |
There was a problem hiding this comment.
connect_session() no longer forwards follow_redirects when invoking httpx_client_factory, so factories written to the documented/previous calling convention (headers, auth, follow_redirects, optional timeout) now fail with a TypeError when a session starts. This is a runtime compatibility regression for existing custom factories that required the follow_redirects argument.
Useful? React with 👍 / 👎.
| enabled = [c for c in components if is_enabled(c)] | ||
| token = get_access_token() |
There was a problem hiding this comment.
This helper always runs component auth checks using get_access_token() and has no stdio bypass, unlike the server’s _get_auth_context flow. In stdio sessions, the token is typically None, so auth-protected prompts/resources from non-FastMCP providers are silently filtered out and reported as missing even though stdio transport is intended to skip bearer-token auth checks.
Useful? React with 👍 / 👎.
…dp-granted-scopes
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 976795ec1c
ℹ️ 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".
| for r in await provider.list_resources() | ||
| if is_enabled(r) | ||
| ] |
There was a problem hiding this comment.
In the non-FastMCP branch, this list comprehension filters with is_enabled(...) before apply_session_transforms(...) runs. That ordering breaks session-level visibility overrides: a resource/template disabled by provider transforms but re-enabled for the current session is dropped before the session transform can restore it, so list_resources can still hide components that the session explicitly enabled. FastMCP.list_resources applies session transforms first, then filters, and this fallback should preserve that same behavior.
Useful? React with 👍 / 👎.
| if component.auth is not None: | ||
| token = get_access_token() | ||
| ctx = AuthContext(token=token, component=component) |
There was a problem hiding this comment.
This fallback path always runs component auth checks using get_access_token(), but stdio sessions typically have no bearer token. In the main server flow, stdio explicitly skips auth evaluation (_get_auth_context), so auth-protected resources are accessible there; here they are treated as unauthorized and surfaced as “not found.” This is a transport-specific regression for non-FastMCP providers using ResourcesAsTools and should include the same stdio bypass (including the analogous logic in _filter_authorized).
Useful? React with 👍 / 👎.
jlowin
added
the
security
Co-authored-by: Claude Opus 4.6 <[email protected]> Co-authored-by: Jeremiah Lowin <[email protected]> Co-authored-by: Marvin Context Protocol <41898282+Marvin Context [email protected]> Co-authored-by: voidborne-d <[email protected]> Co-authored-by: marvin-context-protocol[bot] <225465937+marvin-context-protocol[bot]@users.noreply.github.com> Co-authored-by: Claude <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: d 🔹 <[email protected]> Co-authored-by: Jeremiah Lowin <[email protected]> Co-authored-by: nightcityblade <[email protected]> Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]> Co-authored-by: Claude Sonnet 4.6 <[email protected]> Co-authored-by: Bill Easton <[email protected]> Co-authored-by: Sumanshu Nankana <[email protected]> Co-authored-by: Eric Robinson <[email protected]> Co-authored-by: Martim Santos <[email protected]> Co-authored-by: d 🔹 <[email protected]> Co-authored-by: Matthieu B <[email protected]> Co-authored-by: Sascha Buehrle <[email protected]> Co-authored-by: Hakancan <[email protected]> Co-authored-by: nightcityblade <[email protected]> Co-authored-by: Matt Hallowell <[email protected]> Co-authored-by: nate nowack <[email protected]> Co-authored-by: Bill Easton <[email protected]> Co-authored-by: Marcus Shu <[email protected]> Co-authored-by: Rushabh Doshi <[email protected]> Co-authored-by: AIKAWA Shigechika <[email protected]> Co-authored-by: Jeremy Simon <[email protected]> Co-authored-by: Miguel Miranda Dias <[email protected]> Co-authored-by: Anthony James Padavano <[email protected]> Co-authored-by: Mostafa Kamal <[email protected]> Fix auto-close MRE script posting comment without closing (#3386) Fix WorkOS token scope verification bypass 🤖 Generated with Codex (#3407) Fix initialize McpError fallthrough 🤖 Generated with Codex (#3413) Fix transform arg collisions with passthrough params (#3431) Fix get_* returning None when latest version is disabled (#3439) Fix get_* returning None when latest version is disabled (#3421) Fix server lifespan overlap teardown (#3415) Fix $ref output schema object detection regression (#3420) resolved annotations (#3429) Fix async partial callables rejected by iscoroutinefunction (#3438) Fix async partial callables rejected by iscoroutinefunction (#3423) fix: add version to components (#3458) fix: use intent-based flag for OIDC scope patch in load_access_token (#3465) Fixes #3461 fix: normalize Google scope shorthands and surface valid_scopes (#3477) fix: resolve ty 0.0.23 type-checking errors and bump pin (#3481) fix: shield lifespan teardown from cancellation (#3480) fix: forward custom_route endpoints from mounted servers (#3462) fix updates _get_additional_http_routes() to traverse providers, Fixes #3457 fix: remove hardcoded version from CLI help text (#3456) fix: monty 0.0.8 compatibility, drop external_functions from constructor (#3468) fix: task test teardown hanging 5s per test (#3499) Closes #3498 fix: validate workspace path is a directory before cursor install (#3440) Fixes #3426 fix: handle re.error from malformed URI templates in build_regex (#3501) fix: reject empty/OIDC-only required_scopes in AzureProvider (#3503) fix: restrict $ref resolution to local refs only (SSRF/LFI) (#3502) fix warnings and timeouts (#3504) close upgrade check issue when build passes (#3505) Closes #3484 fix: URL-encode path params to prevent SSRF/path traversal (GHSA-vv7q-7jx5-f767) (#3507) fix: prevent path traversal in skill download (#3493) fix: prefer IdP-granted scopes over client-requested scopes in OAuthProxy (#3492) fix: remove unrelated transform and http.py changes from PR scope fix: remove forced follow_redirects from httpx_client_factory calls (#3496) fix: stop passing follow_redirects to httpx_client_factory fix: restore follow_redirects=True for custom httpx client factories Closes #3509 fix: CSRF double-submit cookie check in consent flow (#3519) fix: validate server names in install commands (#3522) fix: use raw strings for regex in pytest.raises match (#3523) fix: reject refresh tokens used as Bearer access tokens (#3524) fix: route ResourcesAsTools/PromptsAsTools through server middleware (#3495) fix: resolve Pyright "Module is not callable" on @tool, @resource, @prompt decorators (#3540) fix: filter warnings by message in KEY_PREFIX test (#3549) fix: suppress output schema for ToolResult subclass annotations (#3548) fix: increase sleep duration in proxy cache tests (#3567) fix: store absolute token expiry to prevent stale expires_in on reload (#3572) fix: preserve tool properties named 'title' during schema compression (#3582) Fix loopback redirect URI port matching per RFC 8252 §7.3 (#3589) Fix app tool routing: visibility check and middleware propagation (#3591) Fix query parameter serialization to respect OpenAPI explode/style settings (#3595) Fix dev apps form: union types, textarea support, JSON parsing (#3597) fix(google): replace deprecated /oauth2/v1/tokeninfo with /oauth2/v3/userinfo (#3603) fix: resolve EntraOBOToken dependency injection through MultiAuth (#3609) fix(docs): correct misleading stateless_http header (#3622) fix: filesystem provider import machinery (#3626) Closes #3625 (issues 2, 3, 6) fix: recover StdioTransport after subprocess exits (#3630) fix(server): preserve mounted tool task metadata (#3632) fix: scope deprecation warning filter to FastMCPDeprecationWarning (#3649) fix imports, add PrefabAppConfig (#3650) fix: resolve CurrentFastMCP/ctx.fastmcp to child server in mounted background tasks (#3651) Fix blocking docs issues: chart imports, Select API, Rx consistency (#3652) closed by default (#3657) Fix prompt caching middleware missing wrap/unwrap round-trip (#3666) fix: serialize object query params per OpenAPI style/explode rules (#3662) Fixes #2857 fix: HTTP request headers not accessible in background task workers (#3631) fix: restore HTTP headers in worker execution path for background tasks (#3681) fix: strip discriminator after dereferencing schemas (#3682) fix: remove stale ty:ignore directives for ty 0.0.26 (#3684) Fix docs gaps in app provider pages (#3690) fix: dev apps log panel UX improvements (#3698) fix dev server empty string args (#3700)
1 participant
When an IdP silently drops or downgrades scopes that a client requested (rather than rejecting the request outright), OAuthProxy was storing the originally requested scopes instead of what the IdP actually granted. Per RFC 6749 §5.1, when the granted scope differs from what was requested, the IdP includes a
scopeparameter in its token response — but the proxy wasn't checking it.This is most relevant in the
verify_id_tokenpath where the JWT verifier correctly skips scope claims (id_tokens don't carry them), but the fallback usedauthorization_code.scopes(the request) rather than the IdP response. The same pattern existed in the refresh token flow.Now both
exchange_authorization_codeandexchange_refresh_tokencheckidp_tokens["scope"]/token_response["scope"]first, falling back to the requested scopes only when the IdP omits the field (meaning it granted exactly what was asked for).