Skip to content

Add CIMD (Client ID Metadata Document) support for OAuth#2871

Merged
jlowin merged 25 commits intomainfrom
oauth-client-api-refactor
Feb 6, 2026
Merged

Add CIMD (Client ID Metadata Document) support for OAuth#2871
jlowin merged 25 commits intomainfrom
oauth-client-api-refactor

Conversation

@jlowin
Copy link
Copy Markdown
Member

@jlowin jlowin commented Jan 13, 2026

CIMD (Client ID Metadata Documents) lets OAuth clients use a hosted JSON document URL as their client_id instead of Dynamic Client Registration. The server fetches the document, validates it, and uses it to configure the client — identity is verified through domain ownership rather than shared secrets.

from fastmcp import Client
from fastmcp.client.auth import OAuth

async with Client(
    "https://mcp-server.example.com/mcp",
    auth=OAuth(client_metadata_url="https://myapp.example.com/oauth/client.json"),
) as client:
    await client.ping()

The server side delegates all untrusted-URL fetching to a shared ssrf_safe_fetch utility that does DNS resolution, IP validation, DNS pinning, size limits, and timeout enforcement. CIMD documents are cached with a simple TTL. Redirect URI validation is dual-layered: a URI must match both the CIMD document's redirect_uris patterns and the proxy's allowed_client_redirect_uris.

For clients that need stronger auth than public-client, private_key_jwt (RFC 7523) is supported with JTI replay prevention and JWKS key caching.

The OAuth helper now supports deferred binding — mcp_url is optional when using Client(auth=...) since the transport provides it. CLI commands (fastmcp auth cimd create/validate) help with document creation and validation.

@marvin-context-protocol marvin-context-protocol Bot added enhancement Improvement to existing functionality. For issues and smaller PR improvements. breaking change Breaks backward compatibility. Requires minor version bump. Critical for maintainer attention. client Related to the FastMCP client SDK or client-side functionality. auth Related to authentication (Bearer, JWT, OAuth, WorkOS) for client or server. labels Jan 13, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 63af0c8a19

ℹ️ 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".

Comment thread src/fastmcp/server/auth/cimd.py Outdated
Comment thread src/fastmcp/server/auth/oauth_proxy.py Outdated
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 13, 2026

Walkthrough

Adds end-to-end CIMD (Client ID Metadata Document) support: a CLI (fastmcp auth cimd create / fastmcp auth cimd validate); a server-side CIMD subsystem (Pydantic CIMD model, CIMDFetcher with SSRF-safe fetching, caching, jwks/assertion validation, and a CIMDClientManager); SSRF utilities and ssrf-safe JWKS fetching; integration into OAuth/OIDC proxies and auth flows (private_key_jwt handling, CIMD-aware redirect validation, routing/metadata endpoints); client-side OAuth changes for deferred binding via client_metadata_url; and documentation, examples, and CLI wiring.

Possibly related PRs

  • PR 3066: Modifies ProxyDCRClient.validate_redirect_uri — overlaps directly with the CIMD-aware redirect URI validation changes introduced here.
  • PR 2935: Refactors OAuth proxy components — touches the same oauth_proxy routing, client lookup, and caching areas modified for CIMD integration.
  • PR 2577: Adjusts OAuth client binding and server-URL handling — relates to the deferred binding and client_metadata_url changes in the OAuth client.
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.39% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: adding CIMD (Client ID Metadata Document) support for OAuth, which is the primary objective of this pull request.
Description check ✅ Passed The PR description provides a clear explanation of CIMD, includes a working code example, describes server-side implementation details (SSRF safety, caching, dual-layer redirect validation, private_key_jwt support), mentions deferred binding, and lists CLI additions. However, the contributor checklist items are not checked off and no issue number is referenced.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch oauth-client-api-refactor

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
src/fastmcp/server/auth/cimd.py (2)

218-223: Bare except catches all exceptions including KeyboardInterrupt.

The static analysis correctly flags this. While catching all exceptions here prevents crashes on malformed URLs, it's overly broad.

🔧 Suggested fix
         try:
             parsed = urlparse(client_id)
             return parsed.scheme == "https" and parsed.path not in ("", "/")
-        except Exception:
+        except (ValueError, AttributeError):
             return False

406-436: Import inside function body - consider moving to module level.

The fnmatch import on line 431 is inside the function body. While this works, moving it to the module level is more conventional and avoids repeated import overhead on each call.

🔧 Suggested fix

Add at the top of the file with other imports:

import fnmatch

Then remove line 431.

src/fastmcp/cli/cimd.py (1)

30-131: Create command is well-designed with helpful user guidance.

The command:

  • Uses a clear placeholder for client_id that users must update
  • Outputs to file or stdout appropriately
  • Provides important guidance about updating the client_id to match the hosting URL
  • Correctly separates instructions to stderr when outputting JSON to stdout (line 127-130)

One minor observation: the redirect_uri parameter uses name="--redirect-uri" but the example in the docstring uses -r. Consider adding the short form to the parameter definition for consistency.

🔧 Add short form for --redirect-uri
     redirect_uri: Annotated[
         list[str],
         cyclopts.Parameter(
-            name="--redirect-uri",
+            name=["--redirect-uri", "-r"],
             help="Allowed redirect URIs (can specify multiple)",
         ),
     ],
src/fastmcp/client/transports.py (1)

158-177: Consider extracting the OAuth→_OAuthSession conversion logic.

The _set_auth implementations in SSETransport and StreamableHttpTransport (lines 158-177 and 267-286) are nearly identical. This duplication could lead to maintenance issues if the conversion logic needs to change.

♻️ Suggested refactor: Extract to helper function or mixin
def _convert_oauth_to_session(
    url: str,
    auth: OAuth,
    fallback_factory: McpHttpClientFactory | None,
) -> _OAuthSession:
    """Convert OAuth config to an active session with the given URL."""
    return _OAuthSession(
        url,
        scopes=auth.scopes,
        client_name=auth.client_name,
        client_metadata_url=auth.client_metadata_url,
        token_storage=auth.token_storage,
        additional_client_metadata=auth.additional_client_metadata,
        callback_port=auth.callback_port,
        httpx_client_factory=auth.httpx_client_factory or fallback_factory,
    )

Then both transports can call:

if isinstance(auth, OAuth):
    auth = _convert_oauth_to_session(self.url, auth, self.httpx_client_factory)
src/fastmcp/server/auth/oauth_proxy.py (1)

1091-1103: Consider a more explicit approach for storing CIMD metadata.

The dynamic attribute assignment client._cimd_document = cimd_doc (line 1103) works but bypasses Pydantic's model validation and type checking. While the # type: ignore suppresses the type error, this pattern can be fragile.

Consider one of these alternatives:

  1. Add the field to ProxyDCRClient (if CIMD metadata is common enough):
class ProxyDCRClient(OAuthClientInformationFull):
    allowed_redirect_uri_patterns: list[str] | None = Field(default=None)
    client_name: str | None = Field(default=None)
    cimd_document: CIMDDocument | None = Field(default=None, exclude=True)
  1. Use a separate mapping to track CIMD documents by client_id instead of attaching to the model instance.
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7ee3bec and 63af0c8.

⛔ Files ignored due to path filters (4)
  • loq.toml is excluded by none and included by none
  • tests/client/auth/test_oauth_client.py is excluded by none and included by none
  • tests/client/transports/test_transports.py is excluded by none and included by none
  • tests/server/auth/test_cimd.py is excluded by none and included by none
📒 Files selected for processing (8)
  • src/fastmcp/cli/cimd.py
  • src/fastmcp/cli/cli.py
  • src/fastmcp/client/auth/oauth.py
  • src/fastmcp/client/client.py
  • src/fastmcp/client/transports.py
  • src/fastmcp/server/auth/cimd.py
  • src/fastmcp/server/auth/oauth_proxy.py
  • src/fastmcp/utilities/tests.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Python ≥3.10 with full type annotations required for all code
Never use bare except - be specific with exception types in Python code

Files:

  • src/fastmcp/client/client.py
  • src/fastmcp/utilities/tests.py
  • src/fastmcp/cli/cimd.py
  • src/fastmcp/client/auth/oauth.py
  • src/fastmcp/client/transports.py
  • src/fastmcp/server/auth/cimd.py
  • src/fastmcp/cli/cli.py
  • src/fastmcp/server/auth/oauth_proxy.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: jlowin
Repo: jlowin/fastmcp PR: 0
File: :0-0
Timestamp: 2025-12-01T15:48:05.095Z
Learning: PR #2505 in fastmcp adds NEW functionality to get_access_token(): it now first checks request.scope["user"] for the token (which never existed before), then falls back to _sdk_get_access_token() (the only thing the original code did). This is not a reversal of order but entirely new functionality to fix stale token issues.
📚 Learning: 2025-12-01T15:48:05.095Z
Learnt from: jlowin
Repo: jlowin/fastmcp PR: 0
File: :0-0
Timestamp: 2025-12-01T15:48:05.095Z
Learning: PR #2505 in fastmcp adds NEW functionality to get_access_token(): it now first checks request.scope["user"] for the token (which never existed before), then falls back to _sdk_get_access_token() (the only thing the original code did). This is not a reversal of order but entirely new functionality to fix stale token issues.

Applied to files:

  • src/fastmcp/client/auth/oauth.py
🧬 Code graph analysis (5)
src/fastmcp/client/client.py (1)
src/fastmcp/client/auth/oauth.py (1)
  • OAuth (139-208)
src/fastmcp/utilities/tests.py (1)
src/fastmcp/client/auth/oauth.py (2)
  • _OAuthSession (211-408)
  • redirect_handler (312-331)
src/fastmcp/cli/cimd.py (1)
src/fastmcp/server/auth/cimd.py (5)
  • CIMDFetcher (176-455)
  • CIMDFetchError (172-173)
  • CIMDValidationError (168-169)
  • is_cimd_client_id (211-222)
  • fetch (334-404)
src/fastmcp/client/transports.py (1)
src/fastmcp/client/auth/oauth.py (2)
  • OAuth (139-208)
  • _OAuthSession (211-408)
src/fastmcp/server/auth/cimd.py (2)
src/fastmcp/utilities/logging.py (1)
  • get_logger (14-26)
src/fastmcp/server/auth/oauth_proxy.py (1)
  • validate_redirect_uri (236-254)
🪛 Ruff (0.14.11)
src/fastmcp/client/auth/oauth.py

197-200: Avoid specifying long messages outside the exception class

(TRY003)

src/fastmcp/server/auth/cimd.py

118-121: Avoid specifying long messages outside the exception class

(TRY003)


220-220: Consider moving this statement to an else block

(TRY300)


221-221: Do not catch blind exception: Exception

(BLE001)


228-228: Consider moving this statement to an else block

(TRY300)


236-236: Possible binding to all interfaces

(S104)


275-275: Avoid specifying long messages outside the exception class

(TRY003)


278-278: Avoid specifying long messages outside the exception class

(TRY003)


281-281: Avoid specifying long messages outside the exception class

(TRY003)


284-286: Avoid specifying long messages outside the exception class

(TRY003)


290-292: Avoid specifying long messages outside the exception class

(TRY003)


295-295: Avoid specifying long messages outside the exception class

(TRY003)


364-366: Avoid specifying long messages outside the exception class

(TRY003)


368-368: Avoid specifying long messages outside the exception class

(TRY003)


371-373: Avoid specifying long messages outside the exception class

(TRY003)


379-379: Avoid specifying long messages outside the exception class

(TRY003)


385-385: Avoid specifying long messages outside the exception class

(TRY003)


389-392: Avoid specifying long messages outside the exception class

(TRY003)


442-442: Consider moving this statement to an else block

(TRY300)


443-443: Do not catch blind exception: Exception

(BLE001)

src/fastmcp/server/auth/oauth_proxy.py

1097-1097: Possible hardcoded password assigned to argument: "token_endpoint_auth_method"

(S106)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: label-issue-or-pr
🔇 Additional comments (29)
src/fastmcp/server/auth/cimd.py (7)

1-34: LGTM on module setup and default trusted domains.

The module docstring clearly explains the purpose. The default trusted domains list is a reasonable starting point for known CIMD-supporting clients.


36-123: Well-structured CIMD document model with proper validation.

The CIMDDocument model correctly enforces the CIMD specification constraints, particularly the token_endpoint_auth_method validator that rejects shared-secret auth methods. The type annotation Literal["none", "private_key_jwt"] combined with the validator provides defense-in-depth.


151-165: Consider subdomain spoofing in trust/block checks.

The is_trusted and is_blocked methods check if a domain ends with "." + trusted/blocked, which correctly handles subdomains. However, this pattern could allow evil-anthropic.com to be blocked if anthropic.com is blocked (correct), but won't match evilanthropiccom (also correct). The logic looks sound.


266-297: URL validation logic is correct and comprehensive.

The method properly validates HTTPS scheme, host presence, non-root path, private IP blocking, and blocked domain checks. Error messages are informative.


310-332: Cache TTL parsing could be more robust.

The max-age parsing assumes the value after = is a simple digit string, but Cache-Control can have quoted values or additional parameters. This is a minor edge case.


334-404: Solid fetch implementation with proper error handling and security checks.

The fetch method correctly:

  • Checks cache first
  • Validates URL before fetching
  • Uses timeouts
  • Validates response status
  • Parses and validates the document
  • Ensures client_id matches the URL (critical security check)
  • Respects Cache-Control headers

The trailing slash normalization on line 388 is a good touch for URL comparison.


438-455: LGTM on utility methods.

get_domain and clear_cache are straightforward and correctly implemented.

src/fastmcp/cli/cli.py (2)

22-22: LGTM - Import follows existing pattern.

The import of cimd_app follows the same pattern as other CLI subcommand imports (install_app, tasks_app).


956-958: LGTM - CIMD subcommand registration is consistent.

The CIMD app registration follows the established pattern for other subcommand groups like install_app and tasks_app.

src/fastmcp/utilities/tests.py (2)

17-17: Import updated to reflect OAuth architecture refactor.

The import change from OAuth to _OAuthSession aligns with the PR's refactoring where OAuth is now a configuration object and _OAuthSession is the actual provider implementation.


240-258: HeadlessOAuth correctly adapted to new _OAuthSession base class.

The changes properly:

  1. Inherit from _OAuthSession (the actual provider, not the config object)
  2. Pass mcp_url as a positional argument and forward **kwargs to the base class
  3. Use self.httpx_client_factory() in redirect_handler, which is consistent with the base class's approach (see src/fastmcp/client/auth/oauth.py lines 304-305)

This maintains the headless testing capability while aligning with the refactored OAuth architecture.

src/fastmcp/client/client.py (2)

37-37: LGTM - OAuth import added for type hinting.

The import of OAuth from fastmcp.client.auth is needed for the updated type annotation on the auth parameter.


262-268: Type hint correctly expanded to accept OAuth configuration objects.

The auth parameter now accepts OAuth in addition to httpx.Auth, Literal["oauth"], str, and None. This aligns with the PR's goal of simplifying the OAuth API by allowing users to pass an OAuth configuration object directly.

The transport's _set_auth method (line 268) is responsible for converting the OAuth config into the actual _OAuthSession provider, which is the correct separation of concerns.

src/fastmcp/cli/cimd.py (2)

1-28: LGTM - Clean module setup with appropriate imports.

The module correctly imports the CIMD components from the auth module and sets up the CLI app following the project's patterns.


133-208: Validate command provides comprehensive validation with clear feedback.

The command:

  • Checks URL format before attempting fetch
  • Provides clear error messages for different failure modes
  • Displays document details on success for verification
  • Correctly exits with non-zero status on failure

The use of asyncio.run() on line 206 is appropriate for a CLI command that needs to run async code.

src/fastmcp/client/transports.py (3)

35-35: LGTM!

The import correctly brings in both the public OAuth configuration class and the internal _OAuthSession implementation needed for the transport to convert config into an active auth provider.


125-127: LGTM!

The base class _set_auth signature is correctly updated to accept OAuth in addition to existing auth types, maintaining backward compatibility while enabling the new OAuth config pattern.


267-286: LGTM on functionality.

The OAuth conversion logic correctly:

  1. Converts "oauth" literal to OAuth() default config
  2. Builds _OAuthSession with transport's URL and OAuth config values
  3. Falls back to transport's httpx_client_factory if OAuth config doesn't specify one
  4. Preserves BearerAuth for string tokens

This ensures users no longer need to specify the MCP URL twice.

src/fastmcp/client/auth/oauth.py (5)

139-164: LGTM!

The class docstring clearly explains the two registration modes (DCR vs CIMD) with practical examples. The separation of OAuth as a configuration object that the transport converts into an _OAuthSession is a clean design that eliminates the redundant URL specification issue described in the PR objectives.


193-200: Validation looks correct; static analysis hint is a false positive.

The is_valid_client_metadata_url validation at configuration time is appropriate to fail fast on invalid URLs. The long message in the exception is acceptable here as it provides clear user guidance—TRY003 is a style preference rather than a correctness issue.


211-220: LGTM on internal class design.

The docstring correctly identifies _OAuthSession as an internal implementation detail that users should not instantiate directly. The underscore prefix convention reinforces this. The class now receives the MCP URL as a constructor argument rather than requiring users to specify it, which is the core fix for the redundant URL specification issue.


372-407: LGTM on the retry logic for stale credentials.

The async_auth_flow correctly handles ClientNotFoundError by:

  1. Clearing the initialized flag and token storage
  2. Retrying once with fresh registration

This is aligned with the learning about PR #2505 regarding stale token handling. The pattern ensures that cached credentials that become invalid (e.g., client was deleted on server) don't permanently break authentication.


275-283: Stacklevel=4 produces incorrect warning location for direct _OAuthSession instantiation.

The stacklevel is correct for the documented production path (users calling Client(..., auth=OAuth()) through transports), but the codebase contains multiple direct instantiations of _OAuthSession in tests that will produce misleading warnings. For direct instantiation, the warning would point above the test function rather than the instantiation line. While the docstring states users should not instantiate directly, test code does instantiate it directly, and the test utility HeadlessOAuth can also be instantiated directly or through transports.

Consider making stacklevel conditional based on the call stack, moving the warning to a higher-level API, or accepting this as a documentation limitation for non-standard usage.

src/fastmcp/server/auth/oauth_proxy.py (6)

59-64: LGTM!

The CIMD-related imports are well-organized and bring in the necessary components for CIMD client resolution and trust policy handling.


275-277: LGTM on consent UI enhancements.

The verification badge provides clear visual feedback to users about whether a client's identity can be verified via CIMD. The "Verified" state with domain display and the "Unverified client" fallback appropriately communicate trust levels.

Also applies to: 300-315


719-721: LGTM on CIMD configuration.

The enable_cimd flag (defaulting to True) and cimd_trust_policy initialization are well-designed:

  • CIMD is enabled by default for broad compatibility
  • Trust policy falls back to a sensible default (CIMDTrustPolicy())
  • Debug logging provides visibility into CIMD configuration

Also applies to: 859-871


1095-1098: Static analysis S106 is a false positive—ignore safely.

The value "none" for token_endpoint_auth_method is a valid OAuth 2.0 token endpoint authentication method (per RFC 6749 / OAuth 2.1), not a hardcoded password. CIMD clients use public client authentication, which correctly specifies "none".


2286-2300: LGTM on auto-approval logic for trusted CIMD domains.

The auto-approval check correctly:

  1. Only triggers when CIMD is enabled
  2. Extracts the domain from the client_id URL
  3. Checks both auto_approve_trusted flag and is_trusted() policy
  4. Logs the auto-approval for auditability

This provides a streamlined UX for known-good clients (like claude.ai) while maintaining security for unknown clients.


2317-2327: LGTM on CIMD detection for consent page.

The detection logic properly checks for the presence of _cimd_document before treating the client as a CIMD client, and only extracts the domain when applicable. This ensures the verification badge displays accurate information.

Comment thread src/fastmcp/server/auth/cimd.py Outdated
@jlowin jlowin marked this pull request as draft January 14, 2026 00:15
@jlowin jlowin changed the title Refactor OAuth client API to eliminate redundant URL specification Refactor OAuth client API and improve CIMD implementation Jan 14, 2026
@jlowin jlowin changed the title Refactor OAuth client API and improve CIMD implementation Add CIMD (Client ID Metadata Document) support for OAuth Jan 14, 2026
@jlowin jlowin added feature Major new functionality. Reserved for 2-4 significant PRs per release. Not for issues. and removed enhancement Improvement to existing functionality. For issues and smaller PR improvements. breaking change Breaks backward compatibility. Requires minor version bump. Critical for maintainer attention. labels Jan 14, 2026
@marvin-context-protocol
Copy link
Copy Markdown
Contributor

Test Failure Analysis

Summary: The CI static analysis job failed due to code formatting violations caught by (import ordering and line length).

Root Cause: The file has two formatting issues:

  1. Import statements are not in alphabetical order (line 23: should come before )
  2. Several lines exceed readability limits and need wrapping (lines 200, 224, 234-235)

Suggested Solution:

Run the formatting tools locally to auto-fix these issues:

uv run prek run --all-files

This will automatically fix the import order and reformat the long lines. Then commit the changes:

git add tests/server/auth/test_cimd_ssrf_protection.py
git commit -m "Fix formatting in CIMD SSRF protection tests"
git push
Detailed Analysis

The prek pre-commit tool ran ruff-check and ruff-format which caught these violations:

Import order issue:

- from fastmcp.server.auth.cimd import (
-     CIMDClientManager,
-     CIMDDocument,
-     CIMDFetchError,
-     CIMDFetcher,
+ from fastmcp.server.auth.cimd import (
+     CIMDClientManager,
+     CIMDDocument,
+     CIMDFetcher,
+     CIMDFetchError,

Line wrapping:
Lines 200, 224, and 234-235 need to be broken into multiple lines for better readability. The auto-formatter will handle this.

Related Files
  • tests/server/auth/test_cimd_ssrf_protection.py - Contains the formatting violations that need to be fixed

@jlowin jlowin marked this pull request as ready for review January 16, 2026 15:22
@jlowin jlowin added the v3 Targeted for FastMCP 3 label Jan 16, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 02a89eda13

ℹ️ 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".

Comment thread src/fastmcp/server/auth/cimd.py Outdated
Comment thread src/fastmcp/server/auth/cimd.py Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
docs/clients/auth/oauth.mdx (1)

39-40: Fix OAuth example to remove invalid mcp_url parameter.

OAuth does not accept an mcp_url parameter. It is a configuration object; the transport creates the auth session. The example at lines 120–123 will raise TypeError at runtime. The description claiming OAuth "implements the full httpx.Auth interface" is also inaccurate—OAuth is configuration only.

Suggested change
oauth = OAuth(
-    mcp_url="https://fastmcp.cloud/mcp",
     token_storage=encrypted_storage
)
- To fully configure the OAuth flow, use the `OAuth` helper and pass it to the `auth` parameter of the `Client` or transport instance. `OAuth` manages the complexities of the OAuth 2.1 Authorization Code Grant with PKCE (Proof Key for Code Exchange) for enhanced security, and implements the full `httpx.Auth` interface.
+ To fully configure the OAuth flow, use the `OAuth` helper and pass it to the `auth` parameter of the `Client` or transport instance. `OAuth` manages the complexities of the OAuth 2.1 Authorization Code Grant with PKCE (Proof Key for Code Exchange) for enhanced security, and acts as a configuration object; the transport creates the auth session.
src/fastmcp/server/auth/oauth_proxy.py (1)

199-229: Remove exclude=True from cimd_document field to preserve CIMD document during storage and retrieval.

The exclude=True parameter prevents cimd_document from being serialized, causing CIMD clients to lose their document after being stored and retrieved from persistent storage. This breaks private_key_jwt validation (which requires client.cimd_document) and the CIMD verification badge in the consent screen.

Suggested change
-    cimd_document: CIMDDocument | None = Field(default=None, exclude=True)
+    cimd_document: CIMDDocument | None = Field(default=None)
♻️ Duplicate comments (1)
src/fastmcp/server/auth/cimd.py (1)

173-214: SSRF guard still allows link‑local and DNS‑rebind targets.

_is_private_ip only inspects literals/hostname patterns. It doesn’t resolve DNS or block IPv6 link‑local ranges (fe80::/10), so DNS rebinding and link‑local fetches remain possible. Resolve/pin IPs and explicitly reject is_link_local (and multicast) to close the bypass.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 63af0c8 and 02a89ed.

⛔ Files ignored due to path filters (8)
  • loq.toml is excluded by none and included by none
  • tests/server/auth/test_cimd.py is excluded by none and included by none
  • tests/server/auth/test_cimd_ssrf_protection.py is excluded by none and included by none
  • tests/server/auth/test_oauth_proxy.py is excluded by none and included by none
  • tests/server/auth/test_oauth_proxy_redirect_validation.py is excluded by none and included by none
  • tests/server/auth/test_oauth_proxy_storage.py is excluded by none and included by none
  • tests/server/auth/test_oidc_proxy.py is excluded by none and included by none
  • tests/utilities/openapi/test_models.py is excluded by none and included by none
📒 Files selected for processing (12)
  • docs/clients/auth/oauth.mdx
  • docs/development/v3-notes/v3-features.mdx
  • docs/patterns/cli.mdx
  • docs/servers/auth/oauth-proxy.mdx
  • docs/servers/auth/oidc-proxy.mdx
  • src/fastmcp/cli/auth.py
  • src/fastmcp/cli/cli.py
  • src/fastmcp/client/client.py
  • src/fastmcp/client/transports.py
  • src/fastmcp/server/auth/cimd.py
  • src/fastmcp/server/auth/oauth_proxy.py
  • src/fastmcp/server/auth/oidc_proxy.py
✅ Files skipped from review due to trivial changes (1)
  • docs/servers/auth/oauth-proxy.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/fastmcp/cli/cli.py
🧰 Additional context used
📓 Path-based instructions (2)
docs/**/*.mdx

📄 CodeRabbit inference engine (docs/.cursor/rules/mintlify.mdc)

docs/**/*.mdx: Use clear, direct language appropriate for technical audiences
Write in second person ('you') for instructions and procedures in MDX documentation
Use active voice over passive voice in MDX technical documentation
Employ present tense for current states and future tense for outcomes in MDX documentation
Maintain consistent terminology throughout all MDX documentation
Keep sentences concise while providing necessary context in MDX documentation
Use parallel structure in lists, headings, and procedures in MDX documentation
Lead with the most important information using inverted pyramid structure in MDX documentation
Use progressive disclosure in MDX documentation: present basic concepts before advanced ones
Break complex procedures into numbered steps in MDX documentation
Include prerequisites and context before instructions in MDX documentation
Provide expected outcomes for each major step in MDX documentation
End sections with next steps or related information in MDX documentation
Use descriptive, keyword-rich headings for navigation and SEO in MDX documentation
Focus on user goals and outcomes rather than system features in MDX documentation
Anticipate common questions and address them proactively in MDX documentation
Include troubleshooting for likely failure points in MDX documentation
Provide multiple pathways (beginner vs advanced) but offer an opinionated path to avoid overwhelming users in MDX documentation
Always include complete, runnable code examples that users can copy and execute in MDX documentation
Show proper error handling and edge case management in MDX code examples
Use realistic data instead of placeholder values in MDX code examples
Include expected outputs and results for verification in MDX code examples
Test all code examples thoroughly before publishing in MDX documentation
Specify language and include filename when relevant in MDX code examples
Add explanatory comments for complex logic in MDX code examples
Document all API...

Files:

  • docs/servers/auth/oidc-proxy.mdx
  • docs/patterns/cli.mdx
  • docs/clients/auth/oauth.mdx
  • docs/development/v3-notes/v3-features.mdx
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Python ≥3.10 with full type annotations required for all code
Never use bare except - be specific with exception types in Python code

Files:

  • src/fastmcp/server/auth/oidc_proxy.py
  • src/fastmcp/client/client.py
  • src/fastmcp/cli/auth.py
  • src/fastmcp/client/transports.py
  • src/fastmcp/server/auth/cimd.py
  • src/fastmcp/server/auth/oauth_proxy.py
🧠 Learnings (2)
📚 Learning: 2025-11-26T21:52:08.947Z
Learnt from: CR
Repo: jlowin/fastmcp PR: 0
File: docs/.cursor/rules/mintlify.mdc:0-0
Timestamp: 2025-11-26T21:52:08.947Z
Learning: Applies to docs/**/*.mdx : Provide authentication examples showing proper format in MDX API documentation

Applied to files:

  • docs/servers/auth/oidc-proxy.mdx
  • docs/patterns/cli.mdx
📚 Learning: 2025-12-01T15:48:05.095Z
Learnt from: jlowin
Repo: jlowin/fastmcp PR: 0
File: :0-0
Timestamp: 2025-12-01T15:48:05.095Z
Learning: PR `#2505` in fastmcp adds NEW functionality to get_access_token(): it now first checks request.scope["user"] for the token (which never existed before), then falls back to _sdk_get_access_token() (the only thing the original code did). This is not a reversal of order but entirely new functionality to fix stale token issues.

Applied to files:

  • docs/clients/auth/oauth.mdx
🧬 Code graph analysis (3)
src/fastmcp/client/client.py (1)
src/fastmcp/client/auth/oauth.py (1)
  • OAuth (139-208)
src/fastmcp/client/transports.py (1)
src/fastmcp/client/auth/oauth.py (2)
  • OAuth (139-208)
  • _OAuthSession (211-408)
src/fastmcp/server/auth/oauth_proxy.py (1)
src/fastmcp/server/auth/cimd.py (5)
  • CIMDDocument (29-115)
  • CIMDClientManager (514-634)
  • is_cimd_client_id (156-171)
  • is_cimd_client_id (549-558)
  • get_client (560-604)
🪛 LanguageTool
docs/clients/auth/oauth.mdx

[style] ~53-~53: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...]`, optional): OAuth scopes to request. Can be space-separated string or list of st...

(MISSING_IT_THERE)

🪛 Ruff (0.14.11)
src/fastmcp/server/auth/cimd.py

111-114: Avoid specifying long messages outside the exception class

(TRY003)


141-141: Unused method argument: cache_ttl

(ARG002)


142-142: Unused method argument: min_cache_ttl

(ARG002)


143-143: Unused method argument: max_cache_ttl

(ARG002)


170-170: Do not catch blind exception: Exception

(BLE001)


177-177: Consider moving this statement to an else block

(TRY300)


185-185: Possible binding to all interfaces

(S104)


240-240: Avoid specifying long messages outside the exception class

(TRY003)


243-243: Avoid specifying long messages outside the exception class

(TRY003)


246-246: Avoid specifying long messages outside the exception class

(TRY003)


249-251: Avoid specifying long messages outside the exception class

(TRY003)


258-260: Avoid specifying long messages outside the exception class

(TRY003)


280-282: Avoid specifying long messages outside the exception class

(TRY003)


290-292: Avoid specifying long messages outside the exception class

(TRY003)


302-304: Avoid specifying long messages outside the exception class

(TRY003)


310-312: Avoid specifying long messages outside the exception class

(TRY003)


314-314: Avoid specifying long messages outside the exception class

(TRY003)


320-320: Avoid specifying long messages outside the exception class

(TRY003)


326-326: Avoid specifying long messages outside the exception class

(TRY003)


330-333: Avoid specifying long messages outside the exception class

(TRY003)


425-427: Avoid specifying long messages outside the exception class

(TRY003)


432-432: Avoid specifying long messages outside the exception class

(TRY003)


438-438: Avoid specifying long messages outside the exception class

(TRY003)


443-443: Avoid specifying long messages outside the exception class

(TRY003)


445-445: Avoid specifying long messages outside the exception class

(TRY003)


483-483: Avoid specifying long messages outside the exception class

(TRY003)


488-488: Avoid specifying long messages outside the exception class

(TRY003)


504-504: Avoid specifying long messages outside the exception class

(TRY003)


511-511: Avoid specifying long messages outside the exception class

(TRY003)


626-626: Avoid specifying long messages outside the exception class

(TRY003)


629-629: Possible hardcoded password assigned to: "token_endpoint_auth_method"

(S105)


630-630: Avoid specifying long messages outside the exception class

(TRY003)

src/fastmcp/server/auth/oauth_proxy.py

1085-1085: Avoid specifying long messages outside the exception class

(TRY003)


2314-2315: try-except-pass detected, consider logging the exception

(S110)


2314-2314: Do not catch blind exception: Exception

(BLE001)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Run tests: Python 3.10 on windows-latest
  • GitHub Check: Run tests: Python 3.10 on ubuntu-latest
🔇 Additional comments (14)
docs/development/v3-notes/v3-features.mdx (1)

389-444: LGTM! Well-structured CIMD documentation.

The CIMD Support section is comprehensive and well-organized:

  • Clear explanation of how CIMD works with a step-by-step flow
  • Security considerations (SSRF protections) prominently documented
  • Practical code examples with realistic configuration
  • Benefits over DCR clearly articulated

The content follows documentation best practices with progressive disclosure and proper code examples.

docs/servers/auth/oidc-proxy.mdx (1)

236-250: LGTM! Clean CIMD documentation section.

The section appropriately:

  • Uses version badge to indicate the feature availability
  • Provides a concise summary with cross-reference to the main CIMD documentation
  • Documents the enable_cimd parameter with proper type and default value using the ParamField component
docs/patterns/cli.mdx (2)

26-26: LGTM!

Command overview table entry correctly describes the auth cimd commands.


570-649: LGTM! Comprehensive CLI documentation for CIMD commands.

The documentation covers:

  • Clear command structure with create and validate subcommands
  • Complete options tables with required flags properly marked
  • Practical examples showing both stdout and file output workflows
  • Helpful note about updating the placeholder client_id before deployment
  • Detailed validation checks list explaining what the validator verifies

The expected output example for validate provides good verification guidance.

src/fastmcp/client/client.py (2)

37-37: LGTM!

Clean import of the OAuth configuration class from the auth module.


264-264: LGTM! Type signature correctly expanded to accept OAuth objects.

The auth parameter now accepts OAuth configuration objects directly, enabling cleaner client configuration for CIMD-based authentication. The _set_auth method in transports (SSETransport and StreamableHttpTransport) properly handles OAuth objects by converting the string "oauth" to an OAuth instance and extracting OAuth attributes to create an _OAuthSession with proper token storage and callback handling.

src/fastmcp/server/auth/oidc_proxy.py (3)

231-233: LGTM!

The enable_cimd parameter is correctly added with an appropriate default of True (enabled by default), consistent with the PR's goal of making CIMD readily available.


283-286: LGTM!

Clear docstring documenting the enable_cimd parameter with its purpose and default behavior.


359-359: LGTM!

The enable_cimd parameter is correctly propagated to the parent OAuthProxy class through init_kwargs. The parameter is properly defined in OAuthProxy.__init__ with a default value of True and is used to initialize the CIMDClientManager.

docs/clients/auth/oauth.mdx (1)

45-75: CIMD additions are clear and well‑scoped.

The new parameter list, flow step, and CIMD walkthrough are concise and actionable.

Also applies to: 134-217

src/fastmcp/server/auth/cimd.py (1)

29-115: CIMDDocument schema + auth‑method validator look solid.

src/fastmcp/server/auth/oauth_proxy.py (2)

269-449: Consent verification badge is a helpful UX improvement.


1036-1071: Storage‑first CIMD resolution path looks good.

src/fastmcp/cli/auth.py (1)

1-13: Clean CLI namespacing for auth utilities.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment thread src/fastmcp/client/transports.py Outdated
Comment thread src/fastmcp/server/auth/cimd.py
Comment thread src/fastmcp/server/auth/cimd.py
Comment thread src/fastmcp/server/auth/oauth_proxy.py Outdated
Comment on lines +1866 to +1915
# Wrap metadata endpoint to add CIMD support field
elif (
isinstance(route, Route)
and route.path.startswith("/.well-known/oauth-authorization-server")
and route.methods is not None
and "GET" in route.methods
):
# Wrap the original endpoint to add CIMD metadata
# Note: endpoint might be wrapped in CORS middleware

# Create wrapper function with explicit closure to avoid loop binding issues
def make_metadata_wrapper(endpoint):
async def metadata_with_cimd(request):
import json

# Call the original endpoint (might be CORS wrapped)
# We need to handle it as ASGI app
from starlette.responses import JSONResponse

body_parts = []

async def receive():
return {"type": "http.request", "body": b""}

async def send(message):
if message["type"] == "http.response.body":
body_parts.append(message.get("body", b""))

await endpoint(request.scope, receive, send)

# Combine body parts
body = b"".join(body_parts)
metadata = json.loads(body)

# Add CIMD field
metadata["client_id_metadata_document_supported"] = (
self._cimd.enabled
)

return JSONResponse(metadata)

return metadata_with_cimd

custom_routes.append(
Route(
path=route.path,
endpoint=make_metadata_wrapper(route.endpoint),
methods=route.methods,
)
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n src/fastmcp/server/auth/oauth_proxy.py | sed -n '1860,1920p'

Repository: jlowin/fastmcp

Length of output: 2937


🏁 Script executed:

# Check how JSONResponse is imported and used in the codebase
rg "from starlette.responses import" src/ -A 2

# Also check if raw_headers is used elsewhere
rg "raw_headers" src/

Repository: jlowin/fastmcp

Length of output: 2765


🏁 Script executed:

# Check if there are any examples of setting headers on JSONResponse
rg "JSONResponse.*headers" src/

# Also check the Starlette version
rg "starlette" pyproject.toml setup.py requirements.txt 2>/dev/null | head -20

Repository: jlowin/fastmcp

Length of output: 40


🏁 Script executed:

# Check for starlette version requirements
fd "pyproject.toml|setup.py|requirements.txt" -x cat {} \; | grep -A 5 -B 5 starlette

# Check how Response objects handle headers in this codebase
rg "status_code=" src/ -A 2 | head -30

# Look at existing Response usage patterns
rg "JSONResponse\(" src/ -A 3 | head -40

Repository: jlowin/fastmcp

Length of output: 4682


🏁 Script executed:

# Check if there are any examples in the codebase that pass headers to Response objects
rg "headers\s*=" src/ -B 2 -A 1 | grep -A 3 "Response\|JSONResponse" | head -30

# Check the type annotations for the function that needs fixing
sed -n '1878,1906p' src/fastmcp/server/auth/oauth_proxy.py | cat -n

# Search for bare except statements in this file
rg "except:" src/fastmcp/server/auth/oauth_proxy.py

Repository: jlowin/fastmcp

Length of output: 1388


🏁 Script executed:

# Check how Response/JSONResponse headers are typically passed in Starlette
# Look at starlette imports and Response initialization patterns
rg "Response\(" src/fastmcp/utilities/ui.py -B 2 -A 5

# Also check if raw_headers is a valid way to set headers
rg "\.raw_headers" src/

Repository: jlowin/fastmcp

Length of output: 239


🏁 Script executed:

# Check the full context of HTMLResponse usage in ui.py to understand the pattern
sed -n '1,50p' src/fastmcp/utilities/ui.py | cat -n

# Let's also check the type annotations in oauth_proxy.py to see what the file uses
head -50 src/fastmcp/server/auth/oauth_proxy.py | cat -n

# Search for any bare except in the oauth_proxy.py file
rg "except" src/fastmcp/server/auth/oauth_proxy.py -A 1 | head -40

Repository: jlowin/fastmcp

Length of output: 4514


🏁 Script executed:

# Check if JSONResponse accepts headers parameter
rg "JSONResponse\(" src/ -A 3 | grep -E "(headers|status_code)" | head -20

# Check Python version requirement
python3 << 'EOF'
import ast
import sys

# Parse the problematic function to check type annotations
code = """
async def metadata_with_cimd(request):
    import json
    from starlette.responses import JSONResponse
    body_parts = []
    async def receive():
        return {"type": "http.request", "body": b""}
    async def send(message):
        if message["type"] == "http.response.body":
            body_parts.append(message.get("body", b""))
    await endpoint(request.scope, receive, send)
    body = b"".join(body_parts)
    metadata = json.loads(body)
    metadata["client_id_metadata_document_supported"] = self._cimd.enabled
    return JSONResponse(metadata)
"""

tree = ast.parse(code)
func = tree.body[0]
print(f"Function has return annotation: {func.returns is not None}")
print(f"Parameters: {[arg.arg for arg in func.args.args]}")
for param in func.args.args:
    print(f"  {param.arg}: annotation={param.annotation}")
EOF

# Check if there's a specific pattern for Response with headers in starlette usage
rg "headers\s*=" src/ -B 5 | grep -A 10 "Response" | head -30

Repository: jlowin/fastmcp

Length of output: 144


🏁 Script executed:

# Simple check: verify the issue - does JSONResponse accept headers parameter?
# by looking at how it's used elsewhere in the codebase
rg "JSONResponse\(" src/fastmcp/utilities/ -A 2

# Check the specific lines again to verify the function signature
sed -n '1878,1905p' src/fastmcp/server/auth/oauth_proxy.py

Repository: jlowin/fastmcp

Length of output: 1184


🏁 Script executed:

# Let's check the exact way Response objects in Starlette handle headers
# by looking at broader context of how responses are created in this file
sed -n '1830,1920p' src/fastmcp/server/auth/oauth_proxy.py | grep -A 5 -B 5 "Response"

# Check the function signature and its type annotations
sed -n '1877,1880p' src/fastmcp/server/auth/oauth_proxy.py

Repository: jlowin/fastmcp

Length of output: 1030


🏁 Script executed:

# Check how headers are properly passed to JSONResponse in Starlette
# Look for examples in the codebase
rg "Response\(" src/fastmcp -B 2 -A 2 | grep -E "(status_code|headers)" -A 1 -B 1

# Check the full function signature for type annotations
sed -n '1877,1879p' src/fastmcp/server/auth/oauth_proxy.py

# Check if metadata_with_cimd has a return type annotation
sed -n '1877,1910p' src/fastmcp/server/auth/oauth_proxy.py | grep -E "async def|->|:"

Repository: jlowin/fastmcp

Length of output: 3371


🏁 Script executed:

# Check type annotation requirements in the file - look at other async functions
sed -n '1700,1750p' src/fastmcp/server/auth/oauth_proxy.py | grep -E "async def|->|:" | head -10

# See how Request type is used/imported
rg "from starlette" src/fastmcp/server/auth/oauth_proxy.py

# Look for proper type hints on similar nested functions
rg "def make_" src/fastmcp/server/auth/oauth_proxy.py -A 5 | head -30

Repository: jlowin/fastmcp

Length of output: 917


🏁 Script executed:

# Check if functions in this file have parameter and return type annotations
sed -n '1400,1450p' src/fastmcp/server/auth/oauth_proxy.py | grep -E "async def|def" -A 1

# Also check the Request import and usage
rg "Request" src/fastmcp/server/auth/oauth_proxy.py | head -10

# Verify the context: what is the metadata_with_cimd supposed to return?
# Look at Starlette endpoint types
rg "endpoint.*=" src/fastmcp/server/auth/oauth_proxy.py -B 2 -A 2 | head -30

Repository: jlowin/fastmcp

Length of output: 1588


🏁 Script executed:

# Quick verification: check if Response objects in Starlette support raw_headers
# by looking at how cookies/headers are set on responses in this codebase
rg "\.headers\[" src/fastmcp -A 1 -B 1

# Also verify the function should have type annotations
sed -n '1877,1882p' src/fastmcp/server/auth/oauth_proxy.py

Repository: jlowin/fastmcp

Length of output: 1410


🏁 Script executed:

# Check how headers are set on responses in the codebase - look for response.headers pattern
rg "response\.headers\[" src/fastmcp/server/auth -B 3 -A 3

# Check if there are any examples of setting multiple headers at once
rg "headers\s*=" src/fastmcp/server/auth -B 2 -A 2 | head -40

Repository: jlowin/fastmcp

Length of output: 3820


🏁 Script executed:

# Let's confirm how to properly set headers when creating a JSONResponse
# Check if JSONResponse constructor accepts headers parameter
rg "JSONResponse\(" src/ -A 3 | grep -E "headers|status" -B 1 -A 1 | head -30

# Also verify the exact types needed for status and headers
sed -n '1885,1905p' src/fastmcp/server/auth/oauth_proxy.py

Repository: jlowin/fastmcp

Length of output: 1363


Preserve status/headers when wrapping the metadata endpoint.

The wrapper only collects the body and always returns a fresh JSONResponse, dropping the original status code and headers (including CORS/cache headers). Capture http.response.start and forward headers/status to the response.

Additionally, add type annotations to body_parts and the function's return type to comply with the Python ≥3.10 requirement.

Suggested change
-                        body_parts = []
+                        body_parts: list[bytes] = []
+                        status_code = 200
+                        headers: dict[str, str] = {}
@@
-                        async def send(message):
-                            if message["type"] == "http.response.body":
-                                body_parts.append(message.get("body", b""))
+                        async def send(message):
+                            nonlocal status_code, headers
+                            if message["type"] == "http.response.start":
+                                status_code = message.get("status", 200)
+                                raw_headers = message.get("headers", [])
+                                headers = {name.decode(): value.decode() for name, value in raw_headers}
+                            elif message["type"] == "http.response.body":
+                                body_parts.append(message.get("body", b""))
@@
-                        return JSONResponse(metadata)
+                        return JSONResponse(metadata, status_code=status_code, headers=headers)

Comment thread src/fastmcp/server/auth/oauth_proxy.py Outdated
@jlowin jlowin force-pushed the oauth-client-api-refactor branch from 4b7a121 to 515b580 Compare January 16, 2026 19:13
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/fastmcp/server/auth/providers/jwt.py (1)

277-289: Detect multiple JWKS keys without kid instead of silently overwriting.

Keys lacking kid are all stored under _default, so multiple no‑kid entries collapse into one and bypass the “multiple keys” guard. This can mask misconfigured JWKS and lead to ambiguous verification.

🐛 Proposed fix
-                if key_kid:
-                    self._jwks_cache[key_kid] = public_key
-                else:
-                    # Key without kid - use a default identifier
-                    self._jwks_cache["_default"] = public_key
+                if key_kid:
+                    self._jwks_cache[key_kid] = public_key
+                else:
+                    # Key without kid - allow only a single entry
+                    if "_default" in self._jwks_cache:
+                        raise ValueError(
+                            "Multiple JWKS keys without 'kid' are not supported"
+                        )
+                    self._jwks_cache["_default"] = public_key
♻️ Duplicate comments (3)
src/fastmcp/server/auth/cimd.py (2)

781-854: Add type annotations to comply with coding guidelines.

Per coding guidelines, Python ≥3.10 requires full type annotations. Use TYPE_CHECKING with forward references to avoid circular imports while maintaining type safety.

Suggested fix at module level and in methods

Add to imports section:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from fastmcp.server.auth.oauth_proxy import OAuthProxyClient

Then update method signatures:

-    async def get_client(self, client_id_url: str):
+    async def get_client(self, client_id_url: str) -> "OAuthProxyClient | None":
-        client,  # OAuthProxyClient, untyped to avoid circular import
+        client: "OAuthProxyClient",

698-704: Fix base64 padding calculation.

The current formula 4 - len(header_b64) % 4 incorrectly adds 4 padding characters when the string is already aligned (length % 4 == 0), causing base64.urlsafe_b64decode to fail with "Incorrect padding". This was flagged in a prior review but appears unaddressed.

Suggested fix
-            header_b64 += "=" * (4 - len(header_b64) % 4)  # Add padding
+            header_b64 += "=" * (-len(header_b64) % 4)  # Add padding
src/fastmcp/server/auth/oauth_proxy.py (1)

2401-2409: Avoid bare except Exception: pass when parsing CIMD domain.

Per coding guidelines, avoid catching blind exceptions. urlparse can raise ValueError or TypeError for invalid inputs. Catch specific exceptions and log at debug level for troubleshooting.

Suggested fix
         cimd_domain = None
         if is_cimd_client:
             try:
                 from urllib.parse import urlparse

                 parsed = urlparse(txn["client_id"])
                 cimd_domain = parsed.hostname
-            except Exception:
-                pass
+            except (TypeError, ValueError) as e:
+                logger.debug("Failed to parse CIMD client_id for consent badge: %s", e)
🧹 Nitpick comments (3)
src/fastmcp/server/auth/cimd.py (3)

100-108: Use asyncio.get_running_loop() instead of deprecated get_event_loop().

asyncio.get_event_loop() is deprecated in Python 3.10+ and can raise DeprecationWarning. Since this code runs in an async context, use asyncio.get_running_loop() which is the recommended approach.

Suggested fix
     loop = asyncio.get_event_loop()
+    loop = asyncio.get_running_loop()

698-704: Catch specific exceptions instead of bare Exception.

Per coding guidelines, avoid bare except Exception. The operations here can raise specific exceptions: IndexError (missing "."), binascii.Error (invalid base64), and json.JSONDecodeError.

Suggested fix
+        import binascii
+
         try:
             header_b64 = token.split(".")[0]
-            header_b64 += "=" * (4 - len(header_b64) % 4)  # Add padding
+            header_b64 += "=" * (-len(header_b64) % 4)  # Add padding
             header = json.loads(base64.urlsafe_b64decode(header_b64))
             kid = header.get("kid")
-        except Exception as e:
+        except (IndexError, binascii.Error, json.JSONDecodeError, UnicodeDecodeError) as e:
             raise ValueError(f"Failed to extract key ID from token: {e}") from e

456-459: Catch pydantic.ValidationError specifically.

CIMDDocument.model_validate() raises pydantic.ValidationError on validation failure. Catching bare Exception is overly broad.

Suggested fix
+        from pydantic import ValidationError
+
         try:
             doc = CIMDDocument.model_validate(data)
-        except Exception as e:
+        except ValidationError as e:
             raise CIMDValidationError(f"Invalid CIMD document: {e}") from e

Comment on lines 239 to 244
# Extract kid from token header for JWKS lookup
try:
import base64
import json

header_b64 = token.split(".")[0]
header_b64 += "=" * (4 - len(header_b64) % 4) # Add padding
header = json.loads(base64.urlsafe_b64decode(header_b64))
kid = header.get("kid")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix base64url padding to avoid rejecting valid JWTs.

The current padding logic adds 4 "=" characters when the length is already aligned, which can trigger “Incorrect padding” for valid tokens. Use modulo-based padding (and guard the impossible remainder) so aligned headers are untouched.

🐛 Proposed fix
-            header_b64 += "=" * (4 - len(header_b64) % 4)  # Add padding
+            rem = len(header_b64) % 4
+            if rem == 1:
+                raise ValueError("Invalid base64url header length")
+            if rem:
+                header_b64 += "=" * (4 - rem)  # Add required padding

jlowin added 10 commits February 5, 2026 11:03
Core CIMD (Client ID Metadata Documents) infrastructure per SEP-991.
Includes CIMDDocument model, CIMDFetcher with caching/SSRF protection,
and CIMDTrustPolicy for configurable consent handling.
- Add enable_cimd and cimd_trust_policy parameters to OAuthProxy
- Modify get_client to detect CIMD URLs and fetch/validate documents
- Add metadata endpoint wrapper to advertise client_id_metadata_document_supported
- CIMD clients are created as synthetic ProxyDCRClient instances
- Add verified/unverified badges to consent screen
- Verified badge shows domain for CIMD clients
- Unverified badge shows warning for DCR clients
- Add auto-approval for trusted CIMD domains (claude.ai, etc.)
- fastmcp cimd create: Generate CIMD documents for hosting
- fastmcp cimd validate: Fetch and validate hosted CIMD documents
Tests cover:
- CIMDDocument model validation
- CIMDTrustPolicy domain matching
- CIMDFetcher URL validation and SSRF protection
- HTTP fetching with caching
- Change redirect_uris from list[AnyUrl] to list[str] to support wildcards
- Use allowed_redirect_uri_patterns for wildcard matching in CIMD clients
- Remove custom metadata handler that was breaking SDK routes
- Update loq.toml limits for modified files
OAuth is now a configuration object that doesn't require the MCP URL.
The transport builds the real auth provider internally, using the URL
it already knows.

- Split OAuth into config class and _OAuthSession provider
- Transport splats config values explicitly (no mutation)
- Transport's httpx_client_factory used as fallback
- Added OAuth to Client and transport type hints
- HeadlessOAuth now uses configured httpx_client_factory
- Store CIMD clients in persistent storage (no caching)
- Leverage JWTVerifier for private_key_jwt assertion validation
- Add OAuth metadata endpoint support (client_id_metadata_document_supported)
- Add 17 tests for CIMDAssertionValidator and CIMDClientManager
- Fix documentation and type narrowing
- Document CIMD support in OAuth/OIDC proxy server auth
- Add client-side CIMD authentication guide
- Document fastmcp auth cimd CLI commands (create/validate)
- Update all CIMD version badges to 3.0.0
- Add clarifying comments about client_id URL usage
Security improvements:
- Fix critical memory exhaustion vulnerability by streaming responses with 5KB limit
- Remove complex trust policy system (CIMDTrustPolicy) - now uses proxy's allowed_client_redirect_uris
- Simplify SSRF protection to industry-standard approach (no DNS pinning)
- Add comprehensive SSRF protection test suite (16 new tests)

Changes:
- Response streaming prevents DoS attacks (check size incrementally, not after buffering)
- CIMD clients now validate redirect URIs against proxy config, not document metadata
- Private IP/hostname blocking, HTTPS-only, redirects disabled, aggressive timeouts
- Non-standard ports allowed but logged for visibility
- Delete trusted_cimd_domains.py (unused default trust list)

All 47 CIMD tests passing. Implementation follows IETF CIMD spec and OWASP SSRF guidelines.
- Replace bare except with specific exceptions (ValueError, AttributeError)
- Add -r short form for --redirect-uri CLI parameter
- Auto-format test files (import order, line length)
- Add DNS resolution and IP validation before HTTP requests
- Use ipaddress module for robust IP classification
- Add overall timeout tracking for fetch operations
- Improve JTI cache with TTL-based expiration
- Validate jwks_uri with same checks as client_id URLs
- Update tests with proper mocking for DNS resolution
- Add format_ip_for_url helper to bracket IPv6 addresses in URLs
- Fix ssrf_safe_fetch to stream responses and enforce size limits during download
- Preserve original status/headers in OAuth metadata wrapper
- Handle non-JSON responses gracefully in metadata wrapper
- Allow client_secret_basic auth by delegating to SDK when client_id missing from form
- Move json/base64 imports to module level in jwt.py
- Narrow except Exception clauses to specific types
- Remove cimd_document exclusion to persist CIMD docs with clients
- Consolidate format_ip_for_url in ssrf.py, import in cimd.py
SSRF protection improvements, DNS pinning, redirect URI validation
hardening, and CIMD client TTL groundwork. Not yet fully reviewed.
Resolve conflicts from the oauth_proxy.py → oauth_proxy/ package
split on main. Restore main's proxy.py, port CIMD fields to
ProxyDCRClient in models.py, fix imports throughout.

One test remains failing: CIMD redirect URI enforcement needs
the dual-validation logic re-ported to ProxyDCRClient.
The OAuth API refactor (OAuth as config object, _OAuthSession as
provider) is a separate concern from CIMD security hardening.
Reverting to main's OAuth implementation to keep this branch
focused on CIMD/SSRF changes only.
Wire up the existing CIMD infrastructure (client detection, document
fetching, SSRF protection, private_key_jwt auth) into the OAuth proxy
package. OAuthProxy now accepts enable_cimd to activate URL-based client
IDs with automatic lookup, cache-with-TTL refresh for JWKS rotation,
dual redirect URI validation, private_key_jwt token authentication, and
a verified domain badge on the consent screen.
@jlowin jlowin force-pushed the oauth-client-api-refactor branch from 66cf6a3 to 6a73423 Compare February 5, 2026 17:08
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
docs/clients/auth/oauth.mdx (1)

134-216: Convert CIMD setup into Steps and add expected validation output.

The CIMD setup and validation flow reads like a procedure but isn’t structured as Steps, and the validation command lacks expected output or failure examples. Consider converting the “Creating,” “Hosting,” and “Validating” sections into a <Steps> sequence and add success/error output for verification.

✏️ Add expected validation output
 ```bash
 fastmcp auth cimd validate https://myapp.example.com/oauth/client.json

+Expected output:
+text +✅ CIMD document valid (client_id matches URL) +
+
+If validation fails, you see an error like:
+text +❌ client_id mismatch: document has https://myapp.example.com/other.json +

</details>

As per coding guidelines, "Use Steps component for procedures, tutorials, setup guides, and sequential instructions in MDX documentation" and "Include expected outputs and results for verification in MDX code examples."

</blockquote></details>
<details>
<summary>docs/servers/auth/oidc-proxy.mdx (1)</summary><blockquote>

`235-268`: **Make the CIMD example runnable and include a verifiable outcome.**

The new snippet uses `...` placeholders, so it isn’t a copy‑paste runnable example. Please expand it with full initialization (imports and minimal `FastMCP` wiring) and add an expected outcome note so readers can verify success.  


As per coding guidelines, "Always include complete, runnable code examples that users can copy and execute in MDX documentation" and "Include expected outputs and results for verification in MDX code examples."

</blockquote></details>
<details>
<summary>docs/servers/auth/oauth-proxy.mdx (1)</summary><blockquote>

`527-639`: **Replace placeholder snippets with runnable CIMD examples and add verification cues.**

The new CIMD section includes multiple Python snippets with `...`, which makes them non-executable. Please provide complete examples (imports, full `OAuthProxy` initialization, and minimal `FastMCP` wiring) and add a short success or failure cue so readers can verify the behavior.  


As per coding guidelines, "Always include complete, runnable code examples that users can copy and execute in MDX documentation" and "Include expected outputs and results for verification in MDX code examples."

</blockquote></details>

</blockquote></details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Comment on lines 52 to +58
- **`scopes`** (`str | list[str]`, optional): OAuth scopes to request. Can be space-separated string or list of strings
- **`client_name`** (`str`, optional): Client name for dynamic registration. Defaults to `"FastMCP Client"`
- **`client_metadata_url`** (`str`, optional): URL-based client identity (CIMD). See [CIMD Authentication](#cimd-authentication) below
- **`token_storage`** (`AsyncKeyValue`, optional): Storage backend for persisting OAuth tokens. Defaults to in-memory storage (tokens lost on restart). See [Token Storage](#token-storage) for encrypted storage options
- **`additional_client_metadata`** (`dict[str, Any]`, optional): Extra metadata for client registration
- **`callback_port`** (`int`, optional): Fixed port for OAuth callback server. If not specified, uses a random available port
- **`httpx_client_factory`** (`McpHttpClientFactory`, optional): Factory for creating httpx clients
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for OAuth class definition and factory type
rg -n "class OAuth" -g '*.py' -A 20 | head -100

Repository: jlowin/fastmcp

Length of output: 7631


🏁 Script executed:

#!/bin/bash
# Find HttpClientFactory type definition
rg -n "HttpClientFactory|McpHttpClientFactory" -g '*.py' -B 2 -A 2

Repository: jlowin/fastmcp

Length of output: 2612


🏁 Script executed:

#!/bin/bash
# Check the actual oauth.mdx file to see the example and parameter list
cat -n docs/clients/auth/oauth.mdx | sed -n '40,70p'

Repository: jlowin/fastmcp

Length of output: 2011


🏁 Script executed:

#!/bin/bash
# Look for the complete OAuth.__init__ signature
ast-grep --pattern 'class OAuth { $$$__init__($$$) { $$$ }$$$}'

Repository: jlowin/fastmcp

Length of output: 40


🏁 Script executed:

#!/bin/bash
# More targeted search for OAuth __init__ method
rg -n "def __init__" -B 5 -A 15 -g '*.py' | grep -A 15 -B 5 "class OAuth\|OAuth.*__init__"

Repository: jlowin/fastmcp

Length of output: 40


Update OAuth parameter documentation to match the actual constructor signature.

The parameter list is missing mcp_url (a required first parameter shown in the example), and incorrectly includes client_metadata_url which does not exist in the actual OAuth.__init__ signature. The factory type McpHttpClientFactory is correctly cased. Update lines 52–58 to document mcp_url as the first required parameter and remove client_metadata_url from the list.

🧰 Tools
🪛 LanguageTool

[style] ~52-~52: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...]`, optional): OAuth scopes to request. Can be space-separated string or list of st...

(MISSING_IT_THERE)

Client-side: OAuth now accepts client_metadata_url for CIMD identity,
with deferred binding so mcp_url is optional when used with transports.
CLI: cimd create supports --client-id and relative paths. Server-side:
SSRF-hardened fetcher, dual redirect validation, private_key_jwt support.
Standalone CIMD docs page with cross-references from OAuth docs.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (11)
src/fastmcp/server/auth/ssrf.py (3)

114-114: Prefer asyncio.get_running_loop() over asyncio.get_event_loop().

Since this is an async function, a running loop is guaranteed. get_running_loop() is the modern idiom and avoids the deprecation warning path in contexts where no loop is set on the current thread's policy.

♻️ Suggested fix
-    loop = asyncio.get_event_loop()
+    loop = asyncio.get_running_loop()

314-403: Yielding a response-backed iterator from inside async with is correct but fragile — the caller must fully consume chunks before advancing the outer generator.

The checked_chunks() closure captures response, which is only valid while the outer generator is suspended at yield. If a caller stores the chunk iterator and later tries to read from it after closing the outer generator, it will fail silently or error. This is an inherent limitation of this pattern, but it should be documented, or you could consider an async context manager API that makes the resource lifecycle explicit.

💡 Alternative: async context manager approach
from contextlib import asynccontextmanager

`@asynccontextmanager`
async def ssrf_safe_stream(url: str, ...) -> AsyncIterator[tuple[int, AsyncIterator[bytes]]]:
    # ... validate, build pinned URL ...
    async with httpx.AsyncClient(...) as client:
        async with client.stream("GET", pinned_url, ...) as response:
            yield response.status_code, checked_chunks()

This makes it clear the caller must use async with:

async with ssrf_safe_stream(url) as (status, chunks):
    async for chunk in chunks:
        ...

246-264: Consider extracting shared httpx client setup and retry loop into a helper.

Both ssrf_safe_fetch and ssrf_safe_stream duplicate the timeout calculation, pinned URL construction, httpx client configuration, and error-handling tail. A shared helper (e.g., for building the client/timeout and iterating pinned IPs) would reduce the ~50 lines of duplication.

Also applies to: 358-374

docs/clients/auth/oauth.mdx (1)

120-130: Token storage example still passes mcp_url explicitly — consider updating for consistency.

The token storage example at Line 124 still constructs OAuth(mcp_url=..., token_storage=...), while the primary example at Line 44 and the note at Lines 50–52 emphasize that mcp_url is no longer needed when using Client(auth=...). Consider removing the explicit mcp_url to keep the examples consistent and avoid confusion.

Suggested update
 oauth = OAuth(
-    mcp_url="https://your-server.fastmcp.app/mcp",
     token_storage=encrypted_storage
 )
examples/auth/github_oauth/client.py (1)

18-23: Example hardcodes a personal CIMD URL.

The client_metadata_url points to https://www.jlowin.dev/mcp-client.json — a real external URL specific to the repo owner. Other developers cloning this example will need to replace it with their own CIMD document. Consider adding a comment noting this, or using a more obviously-placeholder URL like https://your-app.example.com/oauth/client.json.

src/fastmcp/client/transports/http.py (1)

78-89: Consider extracting shared _set_auth logic to the base class.

The _set_auth implementation is duplicated verbatim between StreamableHttpTransport and SSETransport. Since both inherit from ClientTransport, this could live in the base class to avoid divergence.

src/fastmcp/server/auth/oauth_proxy/models.py (1)

194-219: Verify validate_redirect_uri return type when CIMD redirect_uris is present but empty.

While the CIMDDocument validator guarantees redirect_uris is non-empty, the truthiness check at Line 199 (if cimd_redirect_uris:) would silently skip CIMD validation on an empty list, falling through to pattern-only validation at Line 221. This is safe today due to the model validator, but worth noting for defensive coding.

src/fastmcp/server/auth/oauth_proxy/proxy.py (1)

596-606: Broad except Exception for CIMD refresh is acceptable as resilience, but consider logging the exception type.

The fallback-to-cached pattern is correct for a best-effort refresh. However, except Exception without logging the exception itself (only a generic message on line 604) could hide unexpected errors like MemoryError or KeyboardInterrupt (though the latter isn't caught by Exception). The debug log on line 604 could include the exception for easier troubleshooting.

Suggested improvement
                 except Exception:
-                    logger.debug(
-                        "CIMD refresh failed for %s, using cached client", client_id
+                    logger.debug(
+                        "CIMD refresh failed for %s, using cached client: %s",
+                        client_id,
+                        e,
+                        exc_info=True,
                     )

Note: you'd need to capture the exception as e (except Exception as e:).

src/fastmcp/cli/cimd.py (1)

98-106: Consider using CIMDDocument model for document construction to ensure validity.

The document is built as a raw dict, bypassing the CIMDDocument Pydantic model's validators. If model constraints evolve (e.g., new required fields), the CLI could produce invalid documents. Using CIMDDocument for construction and then serializing would catch this at creation time.

Also, token_endpoint_auth_method is hardcoded to "none" (line 103), but the CIMD spec also supports "private_key_jwt". Consider adding a --auth-method parameter for users who need private_key_jwt.

src/fastmcp/server/auth/cimd.py (2)

268-284: Unused constructor parameters in new code should be removed or genuinely deprecated.

cache_ttl, min_cache_ttl, and max_cache_ttl are documented as "kept for backwards compatibility" but this is a brand-new file. If these existed in a prior iteration of the PR, they can be safely removed now. Keeping dead parameters adds confusion.


735-851: JTI replay cache is in-memory — not effective for horizontally scaled deployments.

The _jti_cache dict on line 713 provides JTI replay protection only within a single process. If the OAuth proxy is deployed across multiple instances (which the proxy's docstring explicitly supports via "pluggable storage enabling horizontal scaling"), a replayed JWT could succeed against a different instance. Consider documenting this limitation or using the existing _client_storage backend for JTI tracking.

Comment thread src/fastmcp/server/auth/cimd.py Outdated
Comment thread src/fastmcp/server/auth/cimd.py
Comment on lines +1515 to +1542
elif (
self._cimd_manager is not None
and isinstance(route, Route)
and route.path.startswith("/.well-known/oauth-authorization-server")
):
client_registration_options = (
self.client_registration_options or ClientRegistrationOptions()
)
revocation_options = self.revocation_options or RevocationOptions()
metadata = build_metadata(
self.base_url, # ty: ignore[invalid-argument-type]
self.service_documentation_url,
client_registration_options,
revocation_options,
)
metadata.client_id_metadata_document_supported = True
handler = MetadataHandler(metadata)
methods = route.methods or ["GET", "OPTIONS"]

custom_routes.append(
Route(
path=route.path,
endpoint=cors_middleware(handler.handle, ["GET", "OPTIONS"]),
methods=methods,
name=route.name,
include_in_schema=route.include_in_schema,
)
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find RevocationOptions class definition and check its defaults
rg -n "class RevocationOptions" -A 15 --type=py

Repository: jlowin/fastmcp

Length of output: 40


🏁 Script executed:

# Find build_metadata function to understand metadata construction
rg -n "def build_metadata" -A 20 --type=py

Repository: jlowin/fastmcp

Length of output: 40


🏁 Script executed:

# Check the parent class and its get_routes method to understand metadata building
rg -n "class.*OAuthProxy" --type=py -A 5

Repository: jlowin/fastmcp

Length of output: 9607


🏁 Script executed:

# Look for where revocation_options is used to understand the context better
rg -n "revocation_options" -B 2 -A 2 --type=py | head -100

Repository: jlowin/fastmcp

Length of output: 5132


🏁 Script executed:

# Search for RevocationOptions in imports and definitions
rg -n "RevocationOptions" --type=py | head -30

Repository: jlowin/fastmcp

Length of output: 902


🏁 Script executed:

# Look for the imports in the proxy file to find where RevocationOptions comes from
head -50 src/fastmcp/server/auth/oauth_proxy/proxy.py | grep -E "^import|^from"

Repository: jlowin/fastmcp

Length of output: 923


🏁 Script executed:

# Find build_metadata function implementation to understand how metadata is constructed
fd "settings.py" --type=f --path="*/auth/*"

Repository: jlowin/fastmcp

Length of output: 291


🏁 Script executed:

# Find OAuthProvider parent class and its metadata building methods
rg -n "class OAuthProvider" -A 30 --type=py

Repository: jlowin/fastmcp

Length of output: 2397


🏁 Script executed:

# Find RevocationOptions import source - it's from mcp.server.auth.settings
# Check if it's defined in the codebase or if it's from the mcp library
find . -name "*.py" -type f -exec grep -l "^class RevocationOptions" {} \;

Repository: jlowin/fastmcp

Length of output: 40


🏁 Script executed:

# Check the imports more carefully in proxy.py to see the full import statement
sed -n '35,70p' src/fastmcp/server/auth/oauth_proxy/proxy.py

Repository: jlowin/fastmcp

Length of output: 1228


🏁 Script executed:

# Look for build_metadata function
rg -n "def build_metadata" --type=py -B 2 -A 15

Repository: jlowin/fastmcp

Length of output: 40


🏁 Script executed:

# Look at the complete get_routes method to understand parent metadata handling
rg -n "def get_routes" src/fastmcp/server/auth/oauth_proxy/proxy.py -A 100 | head -150

Repository: jlowin/fastmcp

Length of output: 5008


🏁 Script executed:

# Check the MCP library RevocationOptions - search for how it's used in tests to infer default
rg -n "RevocationOptions\(\)" --type=py -B 2 -A 2

Repository: jlowin/fastmcp

Length of output: 608


🏁 Script executed:

# Check the parent OAuthProvider's get_routes to see if it also builds metadata
rg -n "def get_routes" src/fastmcp/server/auth/auth.py -A 80 | head -100

Repository: jlowin/fastmcp

Length of output: 4149


🏁 Script executed:

# Look at what the parent returns and if metadata is built there
rg -n "build_metadata\|metadata" src/fastmcp/server/auth/auth.py -B 1 -A 1 | head -80

Repository: jlowin/fastmcp

Length of output: 40


🏁 Script executed:

# Check if there are any utility functions that handle metadata reuse or if metadata objects are typically rebuilt
rg -n "metadata = " src/fastmcp/server/auth/oauth_proxy/proxy.py --type=py

Repository: jlowin/fastmcp

Length of output: 181


🏁 Script executed:

# Check how metadata is handled in tests to understand expected behavior
rg -n "\.well-known/oauth-authorization-server" tests/ -B 3 -A 3 --type=py | head -40

Repository: jlowin/fastmcp

Length of output: 3358


🏁 Script executed:

# See if there's any comment or pattern about why metadata is rebuilt in CIMD case
sed -n '1515,1542p' src/fastmcp/server/auth/oauth_proxy/proxy.py

Repository: jlowin/fastmcp

Length of output: 1342


🏁 Script executed:

# Check if there's a pattern where RevocationOptions is always explicitly set with enabled parameter
rg -n "RevocationOptions\(enabled" --type=py

Repository: jlowin/fastmcp

Length of output: 294


🏁 Script executed:

# Look at what the parent route looks like before CIMD modification
rg -n "class.*Handler" src/fastmcp/server/auth/handlers/ --type=py | grep -i metadata

Repository: jlowin/fastmcp

Length of output: 40


🏁 Script executed:

# See if metadata object is ever reused or modified instead of rebuilt
rg -n "MetadataHandler" src/fastmcp/server/auth/ -B 2 -A 2 --type=py | head -50

Repository: jlowin/fastmcp

Length of output: 991


🏁 Script executed:

# Check the pattern in other tests or code to understand if RevocationOptions() without args is ever correct
rg -n "RevocationOptions\(" --type=py -B 1 -A 1

Repository: jlowin/fastmcp

Length of output: 835


🏁 Script executed:

# Check how metadata is obtained from parent routes - is the parent route's metadata accessible?
sed -n '1450,1470p' src/fastmcp/server/auth/oauth_proxy/proxy.py

Repository: jlowin/fastmcp

Length of output: 923


🏁 Script executed:

# See if there are other examples of modifying parent routes vs rebuilding
rg -n "custom_routes.append" src/fastmcp/server/auth/oauth_proxy/proxy.py -B 5 | head -80

Repository: jlowin/fastmcp

Length of output: 1519


🏁 Script executed:

# Check the complete flow - does parent routes already have MetadataHandler for .well-known?
rg -n "\.well-known/oauth-authorization-server" src/fastmcp/server/auth/ --type=py

Repository: jlowin/fastmcp

Length of output: 2242


🏁 Script executed:

# Look at the parent's path-aware logic for well-known routes more carefully
sed -n '650,670p' src/fastmcp/server/auth/auth.py

Repository: jlowin/fastmcp

Length of output: 985


🏁 Script executed:

# Check if the parent OAuthAuthorizationServerProvider (from MCP lib) creates the initial metadata route
rg -n "OAuthAuthorizationServerProvider" src/fastmcp/server/auth/oauth_proxy/proxy.py -A 5

Repository: jlowin/fastmcp

Length of output: 40


🏁 Script executed:

# Check if the parent route's endpoint could be extracted and reused
rg -n "endpoint=.*metadata" src/fastmcp/server/auth/ --type=py

Repository: jlowin/fastmcp

Length of output: 502


🏁 Script executed:

# Look for how the parent creates the metadata route to understand if we could wrap it instead of rebuild
rg -n "MetadataHandler\|build_metadata" src/fastmcp/server/auth/auth.py --type=py -B 3 -A 3

Repository: jlowin/fastmcp

Length of output: 40


🏁 Script executed:

# Check line 1530 more carefully - is setting client_id_metadata_document_supported the ONLY CIMD-specific change needed?
sed -n '1515,1545p' src/fastmcp/server/auth/oauth_proxy/proxy.py

Repository: jlowin/fastmcp

Length of output: 1469


Fix RevocationOptions initialization and metadata handling for consistency.

Lines 1520-1529 rebuild the OAuth metadata from scratch using build_metadata instead of reusing the parent route's metadata object. This creates a maintenance risk: if the parent's build_metadata behavior evolves (e.g., adding new fields or changing defaults), the CIMD branch could silently miss those changes since it independently reconstructs metadata.

Additionally, line 1523 uses bare RevocationOptions() instead of following the pattern established elsewhere (line 328: RevocationOptions(enabled=True)). Use RevocationOptions(enabled=False) explicitly to match the established pattern and avoid relying on implicit defaults from the external MCP library.

Cache JWTVerifier instances per jwks_uri in CIMDAssertionValidator so
JWKS keys aren't re-fetched on every token exchange. Make SSRF
enforcement opt-in on JWTVerifier (ssrf_safe=True) so operator-configured
localhost JWKS URIs still work — only CIMD's untrusted-input path enables
it. Validate redirect_uri structure in CIMDDocument (require scheme and
host). Also: asyncio.get_running_loop, capture exception in proxy CIMD
fallback, remove dead CIMDFetcher constructor params.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/fastmcp/server/auth/providers/jwt.py (1)

317-334: Non-SSRF JWKS fetch has no request timeout.

The ssrf_safe path correctly uses explicit timeouts, but the standard path creates an httpx.AsyncClient with default timeouts. While httpx defaults to 5 seconds, it's worth being explicit here for consistency and to protect against unresponsive JWKS endpoints in operator-configured setups.

♻️ Suggested improvement
         else:
-            async with httpx.AsyncClient() as client:
+            async with httpx.AsyncClient(
+                timeout=httpx.Timeout(10.0),
+            ) as client:
                 response = await client.get(self.jwks_uri)
                 response.raise_for_status()
                 return response.json()
src/fastmcp/server/auth/cimd.py (2)

43-127: _is_ip_allowed and _resolve_hostname duplicate ssrf.py — DRY violation.

These functions are near-identical copies of is_ip_allowed and resolve_hostname in src/fastmcp/server/auth/ssrf.py. The only difference is the exception type raised (CIMDValidationError vs SSRFError). Since ssrf.py was introduced in this same PR to centralize SSRF protection, cimd.py should reuse it.

♻️ Suggested approach
-from fastmcp.server.auth.ssrf import format_ip_for_url
+from fastmcp.server.auth.ssrf import (
+    SSRFError,
+    format_ip_for_url,
+    is_ip_allowed,
+    resolve_hostname,
+    validate_url,
+)

Then in _validate_url_and_resolve, wrap the call:

     async def _validate_url_and_resolve(self, url: str) -> tuple[str, int, list[str]]:
-        # ... 50+ lines of duplicate validation ...
+        try:
+            validated = await validate_url(url, require_path=True)
+        except SSRFError as e:
+            raise CIMDValidationError(str(e)) from e
+
+        if validated.port != 443:
+            logger.warning("CIMD URL uses non-standard port %d: %s", validated.port, url)
+
+        return validated.hostname, validated.port, validated.resolved_ips

This removes ~80 lines of duplication while preserving the CIMD-specific exception type and non-standard port warning.


701-864: JTI replay protection is solid with proper TTL cleanup.

The two-tier approach (periodic cleanup + emergency size limit) prevents unbounded growth while maintaining security. Caching JWTVerifier per jwks_uri avoids repeated JWKS fetches. The RFC 7523 validation sequence (signature → lifetime → sub → jti) is thorough.

One observation: the _verifier_cache (line 724) grows unboundedly. If many distinct jwks_uri|client_id|token_endpoint combinations appear, this could leak memory. Unlike _jti_cache, there's no eviction or size limit.

Consider adding a size bound or TTL to _verifier_cache to prevent unbounded growth in multi-tenant deployments where many CIMD clients with distinct JWKS URIs authenticate.

Comment thread src/fastmcp/server/auth/ssrf.py Outdated
CIMDFetcher.fetch() was a near-duplicate of ssrf_safe_fetch (DNS pinning,
streaming, size limits, timeouts). Replace with a direct call + simple TTL
cache, cutting ~280 lines. HTTP cache revalidation (ETag/304/Cache-Control)
removed in favor of a 1-hour TTL—these are static config documents.

Delete test_cimd_ssrf_protection.py (643 lines) since CIMD no longer has
its own SSRF implementation to test. Remove broken ssrf_safe_stream. Add
explicit timeout to non-SSRF JWKS fetch path. Bound verifier cache size.
@jlowin jlowin merged commit 880d835 into main Feb 6, 2026
13 checks passed
@jlowin jlowin deleted the oauth-client-api-refactor branch February 6, 2026 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auth Related to authentication (Bearer, JWT, OAuth, WorkOS) for client or server. client Related to the FastMCP client SDK or client-side functionality. feature Major new functionality. Reserved for 2-4 significant PRs per release. Not for issues. v3 Targeted for FastMCP 3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant