Skip to content

Support Client ID Metadata Documents (CIMD) as a registration mode - #191

Open
aterga wants to merge 30 commits into
mainfrom
claude/cimd-client-id-metadata-documents
Open

Support Client ID Metadata Documents (CIMD) as a registration mode#191
aterga wants to merge 30 commits into
mainfrom
claude/cimd-client-id-metadata-documents

Conversation

@aterga

@aterga aterga commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Both directories steer servers to Client ID Metadata Documents over DCR. Anthropic's connector auth doc recommends CIMD for directory listings ("DCR causes Claude to register a new client on every fresh connection"); OpenAI's Apps SDK auth doc says ChatGPT prioritises it. Each selects CIMD when our AS metadata advertises client_id_metadata_document_supported: true alongside "none" in token_endpoint_auth_methods_supported. We already had the second; this PR adds the first and the implementation behind it. Advertising the flag alone would have been an outage: Claude would send client_id=https://claude.ai/oauth/…, validate_client would find nothing in the DCR store, and every Claude connection would fail with invalid_client.

With CIMD the client_id is an https URL, and the RFC 7591-shaped JSON at that URL is the client's registration. Nothing is stored per client, so a directory client connecting thousands of times mints no registrations. Both vendors' live documents are the parsing test's fixtures:

Client client_id redirect_uris auth method
ChatGPT https://chatgpt.com/oauth/client.json ["https://chatgpt.com/connector_platform_oauth_redirect"] prefers private_key_jwt, lists none
Claude Code https://claude.ai/oauth/claude-code-client-metadata ["http://localhost/callback", "http://127.0.0.1/callback"] none

ChatGPT's document declares exactly the redirect #189 pinned Exact, which is a nice confirmation of that call.

Design basis: the scoping in PR #143

This follows the design PR #143 lays out (docs/scoping-cimd.md there; unmerged, so referenced rather than linked), which I had not read when the PR was first opened. Where it stands against that scoping:

  • Trust-policy-gated (§3.1, §3.6) — a URL client_id is fetched only when its origin is a vetted vendor's: a host on or under a domain of DEFAULT_ALLOWED_REDIRECTS (plus OAUTH_ALLOWED_REDIRECT_PREFIXES entries), on the default https port (cimd_origin_trusted). One source of truth for "who is a vetted vendor", as §8.2 recommends. An explicit :8443 is refused before any lookup, as §3.1 asks. The new outbound-fetch surface on the unauthenticated /oauth/authorize is therefore a finite set of vetted hosts, not any URL.
  • Reject, don't fall back (§8.1) — a URL client_id off the policy gets the same treatment as a hosted redirect off the allow-list: 403 invalid_client naming the contact, or the "not approved" page for a browser. Nothing is fetched, nothing about the URL is reflected.
  • Fetcher (§3.3, Phase 0) — the discovery module's SSRF guard (its classifiers tightened along the way to default-deny: the IPv6 side refuses every native address outside the allocated global-unicast 2000::/3, default-denies the IETF protocol-assignment block 2001::/23 within it admitting only the IANA registry's six reachable exceptions by name, and carves out documentation, 6to4 and 3fff::/20; the IPv4 side also refuses the deprecated 6to4 relay block 192.88.99.0/24 bar its reachable 192.88.99.2), exposed as imcp2_core::public_fetch, with the strict reader §3.3 asks for (only a 200 OK is the document — a 206 fragment or any other 2xx is refused like a 3xx — and an over-cap, cut-off, or non-UTF-8 body is an error, never a truncated document), Accept: application/json and a required JSON media type, redirects disabled entirely (the option §3.3 recommends), and no proxy from the environment (through a proxy the address pin would bind nothing). Tighter than proposed where the real documents allow it: 8 KiB cap (they are under 1 KB), one 5 s deadline including DNS (Claude gives our authorize endpoint 10 s).
  • Document-intrinsic vs per-request (§3.4, §3.5) — the split is implemented exactly. Document-intrinsic failures (404, a redirect, not JSON, client_id ≠ URL as a plain string match, too large, over MAX_REDIRECT_URIS or MAX_REDIRECT_URI_LEN, a flow this server does not run, no redirect a DCR registration could have registered) are negative-cached for 60 s; the redirect membership and allow-list checks run per request against the positively cached document and never produce a negative entry, so a bad-redirect probe cannot lock out a real client. Transient failures (a resolver that did not answer, deadline, connection, 5xx, the 4xx a client may retry — 408, 421, 425, 429 — an exhausted rate budget) are not cached; the SSRF guard reports a resolver failure apart from its refusals (ResolveError) so the two cannot be confused.
  • Outbound-DoS control (§5, Phase 1 acceptance criterion)both caps §5 asks for: concurrency (2 per host, 8 overall in flight) and rate (a token bucket of 60 fetches a minute per process and 30 per vetted domain, keyed by the allow-list domain the host matched so the key set is finite, and spent only on a fetch that actually goes out — a request refused for congestion drains no budget), an excess request told to retry rather than queued, plus the negative cache. All of it is one per process (CimdState::shared), shared by every store the binary mounts (/mcp, /mcp-beta), so the limits hold as documented rather than multiplying with the mounts; hosts are keyed by one spelling (lower-case, no trailing dot) so no spelling buys a second per-host quota; a client_id is bounded at 2 KiB before it becomes a cache key, so the cache's memory is bounded too; and a request dropped mid-fetch gives back its host slot and permit, while the flight it was in stays for its waiters (the last one out retires it), so cancelled connections neither grow the map nor split one document's requests over two fetches. Concurrent misses for one document share one fetch. Logging is bounded the same way: the per-request diagnostics on the unauthenticated path are debug-level, and the invalid/unavailable outcomes are logged at warn only where a fetch actually happened.
  • Display fields never trusted (§4) — nothing from the document is displayed; the section comment records that a future consent screen may show the client_id host only.
  • Divergence: no TTL floor (§3.5 asks for one). A floor overrides an origin's no-store / max-age=0 and can keep a withdrawn redirect authorized; review round 1 flagged exactly that. There is a 24 h ceiling and a 10 min default (less the age the response already has); the in-flight and rate caps and the negative cache are what bound the fetch rate, not a floor. ETag revalidation is not implemented; the documents are tiny and the cache is in-memory (§8.3: a miss just re-fetches).

How it works

  • Detectioncimd_client_id: an https URL (the scheme in any case) of at most 2 KiB with a host, a path beyond /, no fragment or userinfo — the last two checked on the raw string as well, since the WHATWG parser erases an empty @, strips tab/newline/CR, trims leading or trailing controls and spaces, and reads a backslash as a slash, none of which a raw identifier may carry. It is taken as given — the string its document must repeat byte for byte, and the cache key — with only the host normalised for the trust policy and the per-host quota, so https://ChatGPT.com/… or an explicit :443 is a client like any other provided its document says the same. Anything else is an ordinary DCR id; that path is unchanged.
  • Trust policycimd_origin_trusted, before anything else (above).
  • Allow-list before fetch — the requested redirect_uri must pass the hosted-redirect allow-list before the document is asked for, so even a vetted host is not fetched on behalf of a redirect that could never be used.
  • Fetchfetch_public_document (above), once a host slot and a permit are held and a rate token taken. Freshness follows HTTP for the SHARED cache this is: Vary: * means no reuse at all; every Cache-Control line is combined (a line that cannot be decoded counts as forbidding reuse) and split into directives only at commas outside a quoted-string, private forbids reuse like no-store, s-maxage takes precedence over max-age, a directive given more than once is honoured at its most restrictive value, one given without a valid number is stale rather than the default lifetime, Expires (relative to Date) decides where Cache-Control grants no freshness, and the response's current age — the larger of Age (every line counted, the greatest winning, an unparseable one counting as the greatest) and the time since Date — is subtracted, and reported alongside so the caller's own default lifetime is net of it too. Failures are typed (FetchError::{Refused, Unreachable, Answered{status}, TooLarge, NotUtf8}) so the caller can make the §3.4 split.
  • Validationparse_client_metadata, per the draft and Anthropic's reference server: client_id equals the URL exactly; no client secret; can authenticate as a public client (none — absent means none, a non-string is malformed — or none listed in token_endpoint_auth_methods_supported, which is ChatGPT's case); can run this server's one flow (grant_types absent or including authorization_code, response_types absent or including code, as DCR requires of a registration); redirect_uris within what a DCR registration may send (16 entries of at most 2 KiB each); and of those only the ones a DCR registration could have registered (redirect_uri_permitted: loopback, or https on an allow-listed host and pinned path, never with query or fragment) that are loopback or same-origin with the document URL are kept, so a self-asserted document cannot point the code at another party nor slip in a redirect DCR would refuse.
  • Redirect check — exactly what a DCR registration gets: redirect_allowed over the document's URIs (loopback port-agnostically, per RFC 8252 §7.3 — Claude Code needs localhost as well as 127.0.0.1) and the hosted-redirect allow-list.
  • Single-flight — concurrent misses for one document share the one fetch and its outcome (document, uncacheable document, invalid, or unavailable), so nobody re-fetches serially behind a slow origin. The fetcher retires the flight before publishing, still under the flight's lock, so no later request can join it to reuse an outcome the origin said not to reuse; every request in the flight holds a FlightGuard, and for a flight whose fetcher was cancelled before publishing the last holder out retires it, so a waiter takes over the fetch and nothing is left behind.
  • Errors — a transient fetch failure is temporarily_unavailable (retry, not "re-add the connector"); an invalid document is invalid_client; an off-policy origin is invalid_client with the contact. None reflects the caller-supplied URL to the browser; the cause is logged where the fetch happened.
  • Token endpoint — unchanged: it already compares the request's client_id with the one bound into the grant, and a URL binds fine.

Opt-in, and rollout

CIMD is off unless the deployment sets OAUTH_CIMD_ENABLED=1: unset, the metadata does not advertise it and a URL client_id is an unknown client, so a deploy of this PR changes no behaviour by itself. The variable is wired through the checked-in deployment path — deploy/native/imcp2.servicedeploy.shdeploy-native.yml, which takes vars.OAUTH_CIMD_ENABLED from the GitHub Environment (see deploy/native/README.md). Set it to 1 on the staging Environment and deploy, connect from Claude web and ChatGPT while watching for client metadata document unavailable / client metadata document is invalid at warn in the logs (emitted once per fetch, never per request), then production. To roll back, unset the variable and redeploy (workflow_dispatch with the same ref is enough; no rebuild): the value is rendered into the systemd unit at deploy time and read once at start-up, so changing the variable alone changes nothing on the host. Once the process restarts without it, Claude's discovery cache (~5 minutes) has clients back on DCR within minutes. The status dashboard's as-metadata check reports CIMD=on|off.

What this PR could not verify: hosted Claude's own document URL is not published (the doc names only Claude Code's, and guesses at it returned 403), so its shape could not be checked the way the other two were; Anthropic's reference server enforces the same rules this PR does, so their client should pass them, but "should" is not "verified". (An earlier revision of this description reported ChatGPT's document as 404: that was this sandbox's egress, not the document, which is served fine elsewhere.)

What this does not do

There is still no consent screen: /oauth/authorize hands the browser straight to Internet Identity, for CIMD clients exactly as for DCR ones. Phase 2 of #143 (branding keyed on the verified domain, coordinated with II) and Phase 3 (opening CIMD beyond the trust policy) are not here. Nor does it serve a stale document when its origin starts failing (stale-if-error). The discovery crawl's own site_client still takes a proxy from the environment as reqwest does by default; that is pre-existing and left for a follow-up.

Related issues

Follows #189. Design per #143 (unmerged scoping). Rebased onto #190, which checked in rustfmt.toml and made the formatting check part of CI. Both submission docs are updated (docs/anthropic-directory-submission.md had CIMD down as a follow-up "if usage grows").

Changes

  • crates/imcp2-core/src/public_fetch.rs (new) — fetch_public_document with typed FetchError; the strict SSRF-guarded, proxy-free GET, split into sending and accept so the acceptance rules are testable on synthetic responses; PublicDocument carries the remaining freshness and the response's current age; freshness (Vary: *, combined Cache-Control with undecodable lines read as no reuse, Expires fallback) and current_age (every Age line, conservatively, and Date), cache_directives (quoted-string-aware splitting) and cache_max_age (shared-cache semantics: private, s-maxage, most-restrictive duplicates, malformed values stale, directives matched by name). discover.rs gains read_capped_bytes (the lossy read_capped_inner is now built on it), a typed ResolveError for resolve_public_url (the crawl keeps its string errors via From), makes both pub(crate), and makes the classifiers default-deny (ipv6_is_global: nothing native outside 2000::/3, 2001::/23 denied with ietf_protocol_assignment_is_global for the registry's exceptions, documentation and 6to4 carved out; ipv4_is_global: the 6to4 relay block bar its reachable exception); lib.rs exports the module (additive public API on the published crate — no version bump here, that's yours to schedule); Cargo.toml adds tokio's time feature, httpdate (already in the lockfile through hyper) as a dependency, and http as a dev-dependency (two edges in Cargo.lock, no new crate).
  • src/auth.rs — the CIMD section: constants, cimd_enabled_by_env / cimd_enabled_by (the opt-in), ClientMetadata, cimd_client_id (reusing raw_authority_has_userinfo), cimd_origin_trusted / allow_listed_domain / vetted_domain / host_key (the trust policy), parse_client_metadata, is_json_media_type, cimd_ttl (the origin's remaining freshness, or the default less the response's age), CimdState (the process-wide cache, single-flight map, in-flight bounds and rate buckets; retire_flight), TokenBucket / Rates, HostSlot and FlightGuard (the guards that give a slot back and retire an unpublished flight, however the request ends), fetch_and_validate_client_metadata, classify_fetch_error (5xx and the retryable 4xx — 408, 421, 425, 429 — are the moment; every other answer is the URL), fetch_client_metadata_document with a #[cfg(test)] fixture registry (answering with a document or any status, aged, failing, or hanging). AuthStore gains cimd: Arc<CimdState> and cimd_enabled; validate_client returns a ClientCheck verdict (Allowed / Refused / MetadataUnavailable / UntrustedClientOrigin); client_metadata_for / fetch_and_cache_client_metadata / remember_client_metadata do single-flight, bounds, fetch, validate, cache, keyed by the identifier as given, with warn-level logging only where a fetch happened; /oauth/authorize maps MetadataUnavailable to a retry and UntrustedClientOrigin to the not-approved page or its JSON; the metadata advertises the flag per cimd_enabled.
  • deploy/native/imcp2.service, deploy/native/deploy.sh, .github/workflows/deploy-native.yml, deploy/native/README.mdOAUTH_CIMD_ENABLED wired from the GitHub Environment variable to the unit, with the rollback (unset and redeploy) spelled out.
  • monitoring/mcp-status/checks.js — the as-metadata detail line reports CIMD=on|off (reported, not required); its test fixture and assertion updated.
  • Tests — cimd_client_id_shape (including non-canonical spellings and an upper-case scheme accepted, the exact length cap, and what the parser would alter or trim — an empty userinfo, tab/newline/CR, edge controls, backslashes — refused), cimd_client_id_is_taken_as_given, client_metadata_parsing (both vendors' real documents, every refusal, the redirect_uris count and length bounds, a non-string auth method, the grant and response types, a loopback entry with a fragment and an unpinned own-origin path dropped), cimd_fetch_error_classification (5xx, 408, 421, 425 and 429 the moment; redirects and the other 4xx the URL), cimd_host_key_is_one_spelling_per_host, cimd_opt_in_values, cimd_origin_trust_policy (real identifiers and subdomains trusted; a stranger, a look-alike, a vetted name under a stranger, and a non-default port refused with no fetch; CIMD off makes a URL id an unknown client), cimd_cache_ttl_is_bounded (including the default net of the response's age), cimd_media_type, cimd_rate_bucket, cimd_fetch_rate_is_bounded (a vendor's share, then the process's budget, refused before any fetch, holding nothing), cimd_client_authorization (the authorize path end to end without network: allow-list before fetch, caching, no-store not cached, a day-old answer with no cache hint not cached, cross-origin refusal, wrong media type, port-agnostic loopback, transient failure retried and not remembered, invalid and 404 remembered, a per-request failure not poisoning the positive cache), cimd_fetches_are_coalesced_and_bounded_per_host (one fetch for three concurrent misses, one shared failure for three concurrent misses, a third document on one host refused with no rate token spent and every slot released), cimd_flight_retirement_rules (published → retired at once however many hold it; unpublished → kept for the waiter, retired by the last holder; a newer flight never touched), cimd_cancelled_fetch_leaves_nothing_behind (a lone fetcher aborted mid-fetch: no flight entry, host slot or permit left behind, and the next request succeeds), cimd_cancelled_fetcher_hands_over_to_a_waiter (a fetcher aborted with a waiter in the flight: the flight survives, the waiter fetches once, a newcomer fetches nothing, the map is empty afterwards), cimd_state_is_shared_by_every_store, as_metadata_advertises_cimd_only_where_enabled, authorize_points_an_unvetted_cimd_origin_at_the_contact, authorize_tells_a_cimd_client_to_retry_when_its_document_is_unavailable (the retry response at the endpoint itself, for an unreachable origin and for a 425 answer: 503 temporarily_unavailable to a programmatic caller, the sign-in error page to a browser, never invalid_client, neither body reflecting the URL or the cause, and nothing remembered so the next request fetches again); public_fetch: guard refusals (loopback, private, link-local, site-local, unique-local, discard-only, unallocated IPv6 space, the IETF protocol-assignment block, documentation, SRv6, the 6to4 relay block, metadata, IPv4-mapped), an unresolvable host as Unreachable, the single deadline, refusal of redirects, 4xx, 5xx and every non-200 2xx as typed errors, exact cap, UTF-8, Age (including overflowing, non-numeric, non-ASCII, and several lines) and the current age on its own, stale Date, Age versus apparent age either way round, clock skew, Expires with and without Date, past and invalid Expires, Cache-Control precedence over Expires, Vary: *, multi-line Cache-Control, an undecodable Cache-Control or Vary line, duplicate max-age, malformed max-age, private, s-maxage, argued no-cache, commas and escapes inside quoted arguments, an unterminated quoted-string; discover.rs's classifier test refuses the special-purpose ranges, unassigned 2001::/23 space and everything outside 2000::/3, while keeping the registry's reachable exceptions global. The pre-existing LRU-stamp test is updated for the verdict type.
  • README.md, docs/anthropic-directory-submission.md, docs/openai-directory-submission.md — CIMD documented as supported: trust policy, opt-in and rollback, same-origin rule, public-client rule, media-type rule, bounds (in flight and in rate), negative cache.

Testing

  • cargo build --locked --workspace --all-targets
  • cargo test --locked --workspace --all-targets — 297 tests, 0 failures
  • cargo fmt --all -- --check — clean under the rustfmt.toml Check in rustfmt.toml and enforce formatting in CI #190 checked in
  • cargo clippy --locked --workspace --all-targets — the 9 warnings are the pre-existing ones in imcp2-core (calls.rs, discover.rs, tools.rs, management.rs); none in auth.rs or public_fetch.rs, and this change adds none
  • npm test --prefix monitoring/mcp-status — 71 tests, 0 failures
  • .github/scripts/scan-internal-identifiers.sh origin/main...HEAD — clean, commit messages included

Negative controls, each restored afterwards: removing the allow-list-before-fetch check flips the rogue-redirect case from Refused to MetadataUnavailable("must not be fetched"), proving the test observes whether a fetch happened; removing the metadata flag fails the metadata test; putting 421/425 back among the URL failures fails both the classification test and the endpoint test (the 425 client gets 403, not 503). The two vendor documents in the fixtures are byte-faithful to what chatgpt.com and claude.ai served on 2026-09-03.

Review rounds (Copilot): round 1 found same-origin redirects accepted, DNS outside the deadline, a cache floor overriding no-store, and a usize::MAX overflow; round 2 found Age ignored and only one Cache-Control line read, lossy UTF-8 decoding, no media-type check, no single-flight, one host able to take every permit, and redirect handling untested; round 3 found CIMD defaulting on with nothing in the deploy path to turn it off, and single-flight sharing only cache hits rather than outcomes; round 4 found a resolver outage classified as a URL refusal (and so negative-cached), the environment's proxy bypassing the address pin, the first of several max-age values winning, redirect_uris bounded in count but not length, and the in-flight limits being per store rather than per process; round 5 (suppressed comments, no threads) found private and s-maxage ignored by what is a shared cache, a non-string token_endpoint_auth_method read as absent, and HTTP 408 treated as a permanent failure; round 6 found the client_id unbounded in length before becoming a cache key, a trailing-dot host buying a second per-host quota, and the README narrowing the public-client rule; round 7 found grant_types / response_types ignored, so a document declaring only another flow was accepted; round 8 found a request cancelled mid-fetch leaving its single-flight entry behind; round 9 found the shared IPv6 classifier not refusing the deprecated site-local fec0::/10; round 10 found the discard-only 100::/64 likewise, a loopback redirect with a fragment retained (and matched fragment-free), no rate cap behind the concurrency cap, and a cancelled fetcher's waiters and newcomers landing on two flights; round 11 found the benchmarking and ORCHID IPv6 ranges likewise, freshness ignoring the apparent age a Date header implies, and rate tokens spent on requests refused for congestion; round 12 found the documentation and SRv6 IPv6 ranges likewise, and a published flight joinable until its last holder left, prolonging a no-store outcome; round 13 (suppressed, no thread) found the runbook implying that unsetting the variable alone rolls CIMD back, when a redeploy is needed; round 14 found unassigned 2001::/23 space still classified public (fixed by default-denying the block), a malformed max-age falling through to the default lifetime, and a gap between publishing an outcome and retiring its flight; round 15 found Cache-Control split at commas inside quoted-strings, the canonical-form requirement refusing client_id spellings the draft allows, and 2001:1::3 (DNS-SD anycast, reachable) wrongly refused; round 16 found an upper-case HTTPS:// scheme refused before parsing, a test doc comment describing the removed canonical-form contract, and Expires ignored where Cache-Control grants no freshness; round 17 found the deprecated 6to4 relay block 192.88.99.0/24 classified public, Vary: * responses cached, and a raw client_id the parser would silently alter (empty userinfo, tab/newline/CR) accepted; round 18 (suppressed, no threads) found an overflowing Age read as zero, and per-request warn logs on the unauthenticated path that a flood could make unbounded; rounds 19 and 20 (suppressed, no threads) found three doc comments still describing earlier contracts; round 21 found only the first Age line read (a non-ASCII one counting as zero), and a client_id with leading or trailing controls or spaces accepted though the parser trims them; round 22 (suppressed, no thread) found the IPv6 classifier still default-allow outside its named exclusions (fixed by refusing everything outside 2000::/3); round 23 found a 206 Partial Content accepted as the document, and an undecodable Cache-Control line silently dropped; round 24 found the default cache lifetime granted in full to an answer that was already a day old; round 25 (summary only, no comment) found a client_id with backslashes, which the parser reads as slashes, accepted; round 26 (summary only, no comment) found 421 Misdirected Request and 425 Too Early — both defined as answers the client may retry — classified as failures of the URL and so negative-cached, and the endpoint's retry response covered only through the verdict it maps rather than directly. All sixty-two fixed; see the commits and the threads. One earlier test failure was a racy deadline test of mine (two competing timeouts), fixed by making the outer deadline the only one; another was a transport blip in the pre-existing live svault.tech test, not this PR's (comment on the PR).

Not tested here: a live authorization from Claude or ChatGPT against a deployed build — this needs the deploy, and see "Opt-in, and rollout" above.

Checklist

  • I have read the Contributing guidelines.
  • Docs (README / comments) updated for any user-visible change.
  • No secrets, credentials, or internal-only information are included.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj

Both directories steer servers to CIMD over DCR: Anthropic recommends it
for directory listings, and ChatGPT prioritises it. Each selects CIMD when
the AS metadata advertises `client_id_metadata_document_supported: true`
alongside `none` in `token_endpoint_auth_methods_supported` — so the flag
must never be advertised ahead of the implementation, or every Claude
connection fails with `invalid_client`.

With CIMD the `client_id` IS an https URL, and the RFC 7591-shaped JSON at
that URL is the client's registration. Nothing is stored per client, so a
directory client that connects thousands of times no longer mints a DCR
registration each time.

The document is fetched under the discovery module's SSRF guard (https
only, public addresses only, pinned against rebinding, redirect hops
re-checked), now exposed as `imcp2_core::public_fetch::fetch_public_document`
— strict where the crawl is opportunistic: a body over the cap (8 KiB), a
transfer cut off mid-body, or an answer from a redirect target is an error,
never a shorter document. 5 s timeout, at most 8 fetches in flight (an
excess request is told to retry, not queued), and a bounded cache honouring
the origin's `max-age` clamped to 1 min–24 h, 10 min by default. Failures
are never cached.

Validation follows the draft and Anthropic's reference server: the
document's `client_id` must equal the URL exactly; it may carry no secret;
it must be able to authenticate as a public client (ChatGPT's document
prefers `private_key_jwt` but lists `none`, which is what it uses here);
and of its `redirect_uris` only loopback ones and those same-origin with
the document URL are kept, so a self-asserted document cannot point the
code at another party. The requested redirect then gets EXACTLY the checks
a DCR registration gets — a match against those URIs (loopback
port-agnostically) AND the hosted-redirect allow-list — and the allow-list
is checked BEFORE any fetch, so a redirect this server would refuse anyway
never costs an outbound request. A fetch failure is `temporarily_unavailable`
(retry), an invalid document `invalid_client`; neither reflects the
caller-supplied URL to the browser.

`OAUTH_CIMD_DISABLED=1` withdraws the advertisement and the mechanism at
deploy time without a rebuild, because hosted Claude's own document URL is
not published and could not be verified here; clients re-read the metadata
within minutes and fall back to DCR.

Tests use ChatGPT's and Claude Code's real documents (as served
2026-09-03) as fixtures, and a process-global stand-in for the web so the
authorize path is exercised end to end without network: allow-list before
fetch, caching, cross-origin refusal, port-agnostic loopback, kill switch.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
@aterga
aterga force-pushed the claude/cimd-client-id-metadata-documents branch from 68d7426 to a5cf0ec Compare September 3, 2026 13:35
aterga and others added 2 commits September 3, 2026 13:40
The as-metadata check's detail line now ends in `CIMD=on|off`, read from
`client_id_metadata_document_supported`, alongside the issuer and PKCE it
already reports. CIMD is the registration mode both directories prefer, and
`OAUTH_CIMD_DISABLED` can withdraw it at deploy time, so the dashboard is
where an operator confirms which mode the production instance is actually
offering — and where a regression that dropped the flag would show. Reported,
not required: the switch being off is a state to see, not an outage.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
The guard test probed a private 10/8 address that is not one of the example
values the internal-identifier scan strips before matching, so the scan
flagged it. Use the canonical example address the scan allows and that
discover.rs's own guard tests use. Same test, same refusal, no suppression
marker needed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
@aterga
aterga force-pushed the claude/cimd-client-id-metadata-documents branch from 99cebc5 to 6d6124a Compare September 3, 2026 13:45

aterga commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

CI test failed on 6d6124a in discoverability::tests::authorizes_a_declared_canister_and_refuses_an_undeclared_one, and I'm treating it as not this PR's:

  • The panic is a transport error fetching a live third-party site: could not read /.well-known/ic-architecture at https://svault.tech: error sending request for url.
  • This PR does not touch discoverability.rs (or that code path at all), and the same job passed on the previous head c776135, whose only difference is one string in a public_fetch test.
  • The test passes locally right now, and https://svault.tech/.well-known/ic-architecture answers 200 in under a second, so the site is up — the runner hit a blip.

The flake has a mechanism worth fixing separately: the test guards its first fetch_declared_manifest with an early return if svault.tech is unreachable ("skips if it stops publishing, rather than failing CI on someone else's deploy"), but authorize_call re-fetches the manifest at discoverability.rs:202, and that second call is .expected. A blip between the two fetches panics instead of skipping. Two ways to close the gap, either outside this PR's scope: have the test tolerate a transport error from authorize_call the way it tolerates one from the first fetch, or fetch the manifest once in the test and drive the decision (decide_*) on constructed input, as the unit tests above it already do.

Re-running the failed job once.


Generated by Claude Code

@aterga
aterga requested a balanced review from Copilot September 3, 2026 14:20
@aterga
aterga marked this pull request as ready for review September 3, 2026 14:20
@aterga
aterga requested a review from a team September 3, 2026 14:20

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Redirect handling, DNS timeout coverage, and cache-control behavior undermine the promised strict fetch guarantees.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds Client ID Metadata Documents as a stateless OAuth registration mode alongside DCR.

Changes:

  • Implements CIMD detection, validation, caching, authorization, and kill switch.
  • Adds an SSRF-guarded public document fetcher.
  • Updates monitoring, tests, and directory documentation.
File summaries
File Description
src/auth.rs Implements CIMD authorization and caching.
crates/imcp2-core/src/public_fetch.rs Adds guarded document fetching.
crates/imcp2-core/src/discover.rs Exposes shared SSRF helpers internally.
crates/imcp2-core/src/lib.rs Exports the public-fetch module.
monitoring/mcp-status/checks.js Reports CIMD status.
monitoring/mcp-status/checks.test.js Tests CIMD status reporting.
README.md Documents CIMD behavior.
docs/openai-directory-submission.md Records OpenAI CIMD readiness.
docs/anthropic-directory-submission.md Records Anthropic CIMD readiness.
Review details

Suppressed comments (2)

crates/imcp2-core/src/public_fetch.rs:53

  • This policy follows same-host and public-IP redirects even though this strict fetch promises that any redirect target is rejected. The later check compares only origins, so https://host/client.json -> https://host/other is accepted and parsed as the original client's document. Disable redirects here; the existing non-success check will then reject the 3xx response.
        .redirect(ssrf_redirect_policy())

crates/imcp2-core/src/public_fetch.rs:81

  • This public API accepts any usize, so max_bytes + 1 overflows for usize::MAX (panic in checked builds, wrap to zero otherwise), potentially returning an empty document as a successful fetch. Use saturating addition or reject that input explicitly.
    let body = match read_capped_inner(resp, max_bytes + 1).await {
  • Files reviewed: 9/9 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/auth.rs Outdated
Comment thread crates/imcp2-core/src/public_fetch.rs Outdated
Four findings from review, each a real gap between what the code promised
and what it did:

Redirects. `public_fetch` followed same-host and public-IP hops under the
crawl's redirect guard and then compared origins, so a same-origin redirect
to another path put a different document behind the client_id URL. Now no
redirect is followed at all: a 3xx is a non-success answer and is refused,
which is what the module doc had claimed.

Deadline. The caller's timeout started after `resolve_public_url`, leaving
DNS resolution unbounded — in the CIMD path, a slow resolver could hold one
of the eight in-flight permits past the five seconds the authorize budget
allows. One `tokio::time::timeout` now covers resolution, connect, response
and body. imcp2-core gains tokio's `time` feature for it.

Cache floor. `no-store`, `no-cache` and `max-age=0` were clamped up to a
minute and the document reused meanwhile, defeating an origin's explicit
instruction and keeping a withdrawn redirect authorized. The floor is gone:
a zero lifetime means the document is not cached, and a positive `max-age`
is honoured as given up to the 24 h ceiling. The floor's DoS rationale did
not hold — an invalid document is never cached either, so a stranger could
always force a fetch per request; the in-flight bound is what contains that.

Overflow. `max_bytes + 1` wrapped for `usize::MAX`; it saturates now.

Tests: a zero timeout expires during resolution of a public name and is
reported as the deadline, not the guard; an uncapped read is accepted; a
`no-store` document authorizes once and is refetched, not reused; the TTL
test pins "no floor, ceiling kept, zero means don't cache".

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 14:33
CI caught `deadline_covers_resolution` racing: on a runner whose resolver
answers before tokio's timer tick, the fetch got past DNS and reqwest's own
per-request `.timeout(ZERO)` failed it with a request error, not the
deadline's. Two timeouts over one operation is the flaw. The client now sets
none of its own; the outer `tokio::time::timeout` is the single deadline,
dropping the future on expiry aborts the connection, and the caller sees the
same error wherever the time ran out. The test asserts exactly that and is
deterministic for it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Metadata validation, HTTP caching, decoding, and fetch admission contain unresolved correctness and reliability issues.

Review details

Suppressed comments (5)

Previously missed (5) — in code that hasn't changed since the last review.

crates/imcp2-core/src/public_fetch.rs:88

  • The cache hint ignores the response's current age. For a CDN response with Cache-Control: max-age=86400 and Age: 86399, client_metadata_for starts a fresh 24-hour TTL, so a withdrawn redirect can remain authorized almost a day beyond the origin's freshness lifetime. Return or compute the remaining freshness lifetime using the HTTP Age/Date semantics rather than forwarding the raw max-age value as a new TTL.
    crates/imcp2-core/src/public_fetch.rs:88
  • HeaderMap::get observes only one Cache-Control field, but repeated Cache-Control fields are semantically combined. A response containing separate max-age=86400 and no-store fields can therefore be cached for a day depending on field order, violating the explicit no-store instruction. Combine all field values before parsing directives.
    crates/imcp2-core/src/public_fetch.rs:92
  • This strict fetch delegates decoding to read_capped_inner, which uses String::from_utf8_lossy. Invalid UTF-8 bytes inside a JSON string are therefore replaced with U+FFFD and the resulting document can pass serde_json validation, even though CIMD JSON must be UTF-8. Preserve the existing fail-soft discovery behavior, but make this strict path reject decoding errors rather than normalizing them.
    src/auth.rs:1263
  • The global fail-fast semaphore can be monopolized with very little unauthenticated traffic: eight slow distinct URLs occupy every permit for up to five seconds, failures are not cached, and subsequent legitimate cold/expired CIMD requests all receive 503. Concurrent misses for one popular client also issue duplicate fetches and can consume all eight slots. Add per-key single-flight coalescing and admission control that one source/origin cannot exhaust before enabling this on the public authorize endpoint.
    src/auth.rs:1271
  • The fetched media type is discarded here, so a 200 response with a missing or text/html Content-Type is accepted whenever its body parses as the expected JSON. CIMD metadata documents are required to be served as application/json; validate the case-insensitive media-type essence (while allowing parameters) and classify a mismatch as an invalid document before parsing it.
  • Files reviewed: 10/10 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Copilot AI review requested due to automatic review settings September 3, 2026 14:39

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Content-type validation, HTTP cache freshness, and cold-cache request coordination remain incomplete.

Review details

Suppressed comments (4)

Previously missed (4) — in code that hasn't changed since the last review.

crates/imcp2-core/src/public_fetch.rs:92

  • This does not fully honor HTTP freshness semantics: HeaderMap::get reads only one of potentially several legal Cache-Control field lines, and the returned max-age is reused from receipt without subtracting the response's Age. For example, a CDN response with max-age=86400, Age: 86399 is cached locally for another day, and a second Cache-Control: no-store line can be missed. Combine all Cache-Control values and reduce the lifetime by Age before exposing it to callers.
    src/auth.rs:1263
  • Cold-cache requests for the same client_id are not coalesced: all eight can consume permits fetching the identical document, while the ninth legitimate authorization is immediately rejected. This creates a thundering herd for popular directory clients and also lets a very low request rate monopolize the process-wide pool with slow URLs. Add per-key single-flight coordination with a cache recheck so one fetch serves concurrent requests; retain a separate global bound for distinct URLs.
    src/auth.rs:1269
  • The fetched response's Content-Type is recorded but never validated before accepting the metadata. The CIMD requirements call for application/json; as written, a document served as HTML or plain text is accepted whenever its bytes happen to parse as JSON. Validate the media type (allowing normal parameters such as charset) and classify a mismatch as CimdError::Invalid.
    crates/imcp2-core/src/public_fetch.rs:74
  • No test exercises a 3xx response, so the redirect behavior that was corrected during the prior review is not protected against regression; the current deadline test never receives an HTTP response. Add a deterministic fetch test (or extract a response-policy seam) proving that same-origin and cross-origin redirects are returned as errors without requesting their targets.
  • Files reviewed: 10/10 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Six findings, each a gap between what the code claimed and what it did:

Freshness. `Cache-Control` was read from one header line and its `max-age`
reused as a fresh lifetime. HTTP combines all lines (a `no-store` on the
second counts) and freshness is `max-age` less the response's `Age`, so a
CDN answer one second from expiry gave us a new day. `public_fetch` now
reports the remaining lifetime from the combined fields, `Age` subtracted.

Decoding. The body went through the crawl's lossy UTF-8 read, so a byte
that was not UTF-8 became U+FFFD and the document still parsed. The strict
path now reads bytes (`read_capped_bytes`, which the lossy read is built on)
and refuses invalid UTF-8; the document parsed is the one served.

Media type. A 200 with any `Content-Type` was parsed as JSON. A metadata
document must be served as `application/json`; anything else, by essence
(parameters and case aside), is now `invalid_client` before parsing.

Thundering herd. Concurrent misses for one document each fetched it and
each spent a permit. A per-`client_id` single-flight lock now lets the first
fetch and the rest read the cache after it.

Monopolisation. Eight slow distinct URLs on one host could hold every
permit. A per-host cap of two now bounds any one host; the global bound of
eight stays. Neither queues: an excess request is told to retry.

Redirects were untested. `accept` is split from the sending so the
acceptance rules run against synthetic responses with no network: every 3xx
is refused as "not followed", non-2xx refused, the cap exact, non-UTF-8
refused, `Age` and multi-line `Cache-Control` honoured. imcp2-core gains
`http` as a dev-dependency for them (Cargo.lock: one edge).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 14:59

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The deployment enables a currently failing integration by default, and failed or uncacheable fetches are not correctly coalesced.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 10/11 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread src/auth.rs Outdated
Comment thread src/auth.rs Outdated
Align the Client ID Metadata Document support with the scoping in
PR #143 and the third review round.

Trust policy: a URL client_id is fetched only when its origin is a
vetted vendor's — a host on or under a domain of the hosted-redirect
allow-list, on the default https port. Anything else is refused before
any request goes out and, like a hosted redirect off the allow-list,
pointed at the allow-listing contact (403 invalid_client, or the
not-approved page for a browser). The one vendor list decides both
where a code may land and whose document this server will GET.

Opt-in: CIMD is advertised and URL client_ids accepted only where the
deployment sets OAUTH_CIMD_ENABLED=1. The deploy template takes the
variable from the GitHub Environment, so a routine deploy never
switches the directory clients over by itself; unsetting it is the
rollback.

Negative cache: a failure that is about the URL itself (404, a
redirect, not JSON, about another URL, too large, not UTF-8) is
remembered for a minute so a repeat costs no fetch. A transient one
(deadline, connection, 5xx, 429) is not, and a per-request failure (a
redirect the document does not list) never is, so a probe cannot lock
out a real client. The fetcher's errors are typed to make that split.

Single-flight shares the outcome: concurrent misses for one document
share the one fetch's result — failure and uncacheable document
included — instead of re-fetching serially behind it, and the flight
entry is retired only by the flight that made it.

A document may list no more redirect_uris than a DCR registration.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 15:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

SSRF proxy handling, transient DNS classification, cache freshness, redirect bounds, and process-wide concurrency limits need correction.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 14/15 changed files
  • Comments generated: 5
  • Review effort level: Balanced

Comment thread crates/imcp2-core/src/public_fetch.rs
Comment thread crates/imcp2-core/src/public_fetch.rs Outdated
Comment thread crates/imcp2-core/src/public_fetch.rs Outdated
Comment thread src/auth.rs
Comment thread src/auth.rs Outdated
- A resolver failure is a failure of the moment, not of the URL: the
  SSRF guard now reports it apart from its refusals (ResolveError), the
  fetcher maps it to Unreachable, and the client-metadata cache no longer
  remembers a DNS outage as "no document there" for a minute.
- The guarded fetch takes no proxy from the environment: a proxy would
  resolve the host itself and the address pin would bind nothing.
- A max-age given more than once is honoured at its most restrictive
  value, so a duplicate can never extend freshness.
- A document's redirect_uris are bounded in length as well as count,
  exactly as a DCR registration's are, so a document admits no redirect
  DCR would refuse.
- The cache, single-flight map and in-flight bounds are one per process,
  shared by every store the binary mounts, so the documented limits hold
  per process rather than multiplying with the mounts.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 15:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The security-sensitive OAuth, SSRF, caching, and concurrency changes require final human review and live vendor validation.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/imcp2-core/src/public_fetch.rs:60

  • The public field documentation no longer matches the value returned by freshness: it also uses s-maxage/Expires, subtracts apparent age from Date, and returns zero for private or Vary: *. Documenting only max-age/Age can mislead downstream users of this newly exported API about when None and Some(0) occur.
  • Files reviewed: 15/16 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

The twentieth review round: the public field's doc still named only
max-age and Age. It now describes the value as computed — s-maxage or
max-age from every Cache-Control line, else Expires less Date, minus the
current age — and says when Some(0) and None occur.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 17:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

URL preprocessing and malformed Age handling can violate the documented validation and cache-safety behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/auth.rs:966

  • Url::parse also silently strips leading/trailing C0 controls and spaces. Consequently a raw identifier such as " https://chatgpt.com/oauth/client.json " reaches the CIMD path and is fetched as the normalized URL, despite the contract here requiring the raw identifier itself to be the HTTPS URL. Extend the raw precheck (and the shape regression test) to reject those trimmed bytes too.
    if client_id.contains(['\t', '\n', '\r']) || raw_authority_has_userinfo(client_id) {
        return None;
  • Files reviewed: 15/16 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread crates/imcp2-core/src/public_fetch.rs Outdated
…ld trim

The twenty-first review round, two findings:

- Every `Age` line counts and the greatest wins, and one that is not
  even ASCII is the greatest age like any other unparseable one — only
  the first line was read, and a non-ASCII value counted as zero.
- The WHATWG parser also trims leading and trailing C0 controls and
  spaces from a URL, so a raw client_id beginning or ending with one is
  refused, as tab/newline/CR and an empty userinfo already were.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 18:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The IPv6 SSRF classifier still accepts reserved addresses outside the globally allocated unicast range.

Review details

Suppressed comments (1)

crates/imcp2-core/src/discover.rs:1384

  • The IPv6 guard is still default-allow: any address not in these individual exclusions is treated as public. For example, 4000::1 is outside the currently allocated global-unicast 2000::/3, yet this function returns true, so a vetted hostname could route the new unauthenticated fetch to internally routed reserved space. Require 2000::/3 for native IPv6 addresses (the mapped IPv4 path already returns above) and add a reserved-address regression case.
        || (seg[0] & 0xffc0) == 0xfec0       // fec0::/10 site-local (deprecated, RFC 3879)
        || (seg[0] == 0x0100 && seg[1] == 0 && seg[2] == 0 && seg[3] == 0) // 100::/64 discard-only (RFC 6666)
        || (seg[0] == 0x3fff && (seg[1] & 0xf000) == 0)          // 3fff::/20 documentation (RFC 9637)
        || seg[0] == 0x5f00                                      // 5f00::/16 SRv6 SIDs, not globally reachable (RFC 9602)
  • Files reviewed: 15/16 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

The twenty-second review round: the IPv6 classifier was default-allow —
any address not on its list of exclusions was public — so an address in
space IANA has not allocated for global unicast (4000::1, say) passed.
Global unicast is allocated only from 2000::/3, so a native address
outside it is now refused without being named, which also covers the
loopback, discard, NAT64, SRv6, unique-local, link-local, site-local and
multicast ranges the list used to enumerate; within 2000::/3 the
2001::/23 default-deny and the documentation and 6to4 carve-outs remain.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 18:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The fetcher can accept partial representations and can over-cache when a valid non-ASCII Cache-Control line is discarded.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/imcp2-core/src/public_fetch.rs:224

  • filter_map silently discards any Cache-Control field line that HeaderValue::to_str() cannot decode. HTTP quoted strings may legally contain obs-text, so a response can have Cache-Control: max-age=86400 plus a valid byte-valued line such as foo="<obs-text>", max-age=0; the restrictive line is dropped and the document is reused for a day. Treat an undecodable line as zero freshness rather than ignoring it.
  • Files reviewed: 15/16 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread crates/imcp2-core/src/public_fetch.rs Outdated
… as no reuse

The twenty-third review round, two findings:

- Only a `200 OK` is the document. Any other 2xx was accepted before — a
  `206 Partial Content` fragment that happens to parse as JSON would have
  been validated as the complete metadata document, against the strict
  reader's completeness guarantee — and is now refused like a 3xx or 4xx.
- A `Cache-Control` or `Vary` line the header cannot decode (a quoted
  argument may carry obs-text) is read as forbidding reuse rather than
  skipped, or an undecodable `max-age=0` beside a decodable `max-age=86400`
  would be dropped and the day honoured.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 18:25

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The default cache lifetime currently ignores an already-aged response, allowing stale client metadata to be reused.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 15/16 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread crates/imcp2-core/src/public_fetch.rs
The twenty-fourth review round: where the origin sent no freshness
information, the ten-minute default was granted in full, so a document
some cache along the way had already held for a day (Age: 86400, or an
old Date) got ten fresh minutes here. The fetched document now reports
its current age alongside the remaining freshness, and the default
lifetime is that default less the age; an origin's own value is already
net of it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 18:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

CIMD validation still accepts backslash-normalized identifiers that violate its raw URL and userinfo constraints.

Review details
  • Files reviewed: 14/16 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

The twenty-fifth review round: the WHATWG parser reads a backslash as a
slash in an https URL — `https:\\host\path` parses as `https://host/path`,
and one in the authority ends it before an `@` the raw scan expects
there — so a raw identifier with a backslash was not the URL that was
parsed and fetched. Any backslash is refused on the raw string now,
alongside tab/newline/CR, trimmed controls and an empty userinfo.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 18:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Retryable HTTP 421/425 responses are treated as invalid clients, and the transient endpoint response lacks direct coverage.

Review details
  • Files reviewed: 14/16 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

The twenty-sixth review round. A document fetch answered 421 Misdirected
Request or 425 Too Early was classified as a failure of the URL, so the
client was refused as unknown and the refusal remembered for the negative
TTL, though both statuses are defined as ones the client may retry: 421
is about the connection the request arrived on, 425 about the moment.
Both join 408 and 429 as failures of the moment, told to retry and never
remembered.

The endpoint's retry response — 503 temporarily_unavailable to a
programmatic caller, the sign-in error page to a browser — was covered
only through the verdict it maps. It is now exercised directly, for an
unreachable origin and for a 425 answer: the status and error code, that
neither body reflects the client_id URL or the cause, and that nothing
is remembered so the next request fetches again. The test fixture gains
an `answer(url, status)` for any non-200 status; `not_found` is built on
it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Xd7VT72Qt16qiAJynu9EKj
Copilot AI review requested due to automatic review settings September 3, 2026 18:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The authentication and SSRF-sensitive changes require final human review and staged live-client validation.

Review details
  • Files reviewed: 14/16 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants