Fix CIMD redirect allowlist bypass and cache revalidation

[jlowin]

CIMD follow-up review found two redirect validation inconsistencies in the proxy path: allowlist constraints could be bypassed when a CIMD client omitted redirect_uri with a single configured URI, and an explicit empty allowlist did not behave as deny-all for CIMD redirects. The same review also showed CIMD document fetching was still fixed-TTL only, even though the feature now depends on HTTP cache directives to keep metadata fresh without unnecessary refetches.

This update makes CIMD redirect enforcement consistent with proxy policy in both omitted and explicit redirect_uri flows, adds conditional HTTP revalidation support (Cache-Control, ETag, Last-Modified, 304, no-store, no-cache) to CIMDFetcher via SSRF-safe response metadata, and clarifies docs that enable_cimd is default-on while False is an explicit opt-out.

fetcher = CIMDFetcher()
doc = await fetcher.fetch(client_id_url)   # initial 200 caches with response directives
doc = await fetcher.fetch(client_id_url)   # revalidates via If-None-Match/If-Modified-Since when required

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d776b8d571

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

[chatgpt-codex-connector]

Preserve cached cache-control on 304 without headers

When a conditional request gets a 304, you rebuild cache policy solely from the 304 response headers. If the origin omits cache headers on the 304 (common), _parse_cache_policy falls back to the default TTL and clears must_revalidate, overwriting the stricter directives from the cached 200. For example, a cached response with Cache-Control: max-age=0 will be replaced by DEFAULT_CACHE_TTL_SECONDS after a 304 without cache headers, letting clients skip revalidation for ~5 minutes against the origin’s intent. This should reuse the cached policy when the 304 doesn’t provide updated cache headers (RFC allows 304 to only include changes).

Useful? React with 👍 / 👎.

[coderabbitai]

Walkthrough

Reworks CIMD fetching and OAuth proxy redirect handling. Adds SSRFFetchResponse and ssrf_safe_fetch_response (ssrf_safe_fetch now delegates to it). Replaces simple TTL caching with HTTP-cache semantics via internal _CIMDCacheEntry and _CIMDCachePolicy, parses Cache-Control/Expires, supports ETag/Last-Modified, conditional GETs, 304 handling, and no-store eviction. Updates header parsing and error handling. Adjusts oauth-proxy models to resolve a single CIMD-provided redirect_uri and validate it against configured allowed_redirect_uri_patterns. Updates docs to note CIMD enabled by default.

Possibly related PRs

• jlowin/fastmcp PR 2871: Prior work introducing the original CIMDFetcher and ssrf_safe_fetch abstractions that this change extends with ssrf_safe_fetch_response and policy-driven caching.
• jlowin/fastmcp PR 2935: Changes to redirect-URI resolution and validation in oauth_proxy models that overlap the single-CIMD-candidate validation logic updated here.
• jlowin/fastmcp PR 3066: Related adjustments enforcing allowlist/validation for redirect URIs in oauth_proxy components affecting the same validation surface.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the two main changes: fixing a CIMD redirect allowlist bypass vulnerability and implementing proper HTTP cache revalidation for CIMD documents.
Description check	✅ Passed	The PR description comprehensively covers all required template sections, provides clear problem statement and solution details, includes a code example, and is well-structured for reviewer understanding.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ 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 codex/cimd-followup-fixes

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

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

210-259: Cache-Control parsing is solid; consider also matching the literal must-revalidate directive.

Currently only no-cache sets must_revalidate. The HTTP must-revalidate directive (a distinct Cache-Control token) has slightly different semantics — it forbids serving stale responses without revalidation — but in this implementation stale entries always trigger a refetch anyway (line 309 falls through), so the omission is safe today. If that staleness-fallback behavior ever changes, this would silently regress.

Consider:

         no_store = "no-store" in directives
-        must_revalidate = "no-cache" in directives
+        must_revalidate = "no-cache" in directives or "must-revalidate" in directives

---

337-339: Ruff TRY003: inline exception message.

Static analysis flags the long string literal in the CIMDFetchError raise. This is a minor style nit — the message is clear and used in a single place. You could suppress with # noqa: TRY003 or move the message to the exception class if you prefer to stay lint-clean.

[marvin-context-protocol]

Test Failure Analysis

Summary: Windows tests are timing out during WorkOSProvider initialization due to synchronous DiskStore directory creation in OAuthProxy.__init__().

Root Cause: The test_authkit_domain_https_prefix_handling test creates three WorkOSProvider instances without passing client_storage. Each instantiation triggers DiskStore(directory=settings.home / "oauth-proxy") which performs synchronous directory.mkdir(). On Windows CI, this file I/O operation is extremely slow (likely due to antivirus scanning or path resolution), causing the test to timeout before completion.

Suggested Solution: Follow the pattern used in test_oauth_proxy_storage.py by passing explicit storage to provider constructors in tests:

from key_value.aio.stores.memory import MemoryStore

def test_authkit_domain_https_prefix_handling(self):
    storage = MemoryStore()

    provider1 = WorkOSProvider(
        client_id="test_client",
        client_secret="test_secret",
        authkit_domain="test.authkit.app",
        base_url="https://myserver.com",
        jwt_signing_key="test-secret",
        client_storage=storage,  # Add this
    )
    # ... repeat for provider2 and provider3

This eliminates disk I/O entirely during tests and aligns with existing test patterns.

Detailed Analysis

Stack Trace:

File "proxy.py", line 426, in __init__
    key_value=DiskStore(directory=settings.home / "oauth-proxy"),
File "key_value/aio/stores/disk/store.py", line 76, in __init__
    directory.mkdir(parents=True, exist_ok=True)
File "pathlib.py", line 1175, in mkdir
    self._accessor.mkdir(self, mode)
+++++++++++++++++++++++++++++++++++ Timeout +++++++++++++++++++++++++++++++++++

Why This Happens:

• OAuthProxy.__init__() creates a default DiskStore when client_storage=None
• DiskStore.__init__() calls synchronous directory.mkdir(parents=True, exist_ok=True)
• On Windows, file system operations can be very slow
• Creating three providers in one test multiplies the timeout risk

Existing Patterns:

• tests/server/auth/oauth_proxy/test_oauth_proxy_storage.py explicitly passes storage fixtures
• tests/server/auth/oauth_proxy/test_oauth_proxy.py uses temp_storage and memory_storage fixtures
• Most OAuth provider tests pass jwt_signing_key but not client_storage, causing this issue

Related Files

Files Requiring Changes:

• tests/server/auth/providers/test_workos.py:32-71 - Add client_storage=MemoryStore() to all provider instantiations

Reference Implementation:

• tests/server/auth/oauth_proxy/test_oauth_proxy_storage.py:60-68 - Shows correct pattern
• src/fastmcp/server/auth/oauth_proxy/proxy.py:426 - Where DiskStore is created by default

Test Fixtures Available:

• tests/conftest.py:114-115 - memory_storage fixture
• tests/conftest.py:118-123 - temp_storage fixture