From 7b302354fde8e5b7eb1e0c74a7aae86056595e24 Mon Sep 17 00:00:00 2001 From: Arshavir Ter-Gabrielyan Date: Wed, 23 Sep 2026 23:28:45 +0200 Subject: [PATCH 1/6] Surface vetted-connector branding to Internet Identity (name + logo) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the imcp2 side of the client-branding extension (docs/scoping-client-branding.md, PR #103): let II show WHICH vetted product is asking for a connect, instead of an anonymous "some bridge". - New `branding` module: a server-curated table of vetted connectors (ChatGPT, Claude, Cursor, Grok, Perplexity, Google Antigravity), keyed on the request's already-validated `redirect_uri` vendor — never on the client-supplied `client_name`/`logo_uri`, which open DCR makes attacker-controlled (rendering them would be a consent-phishing gift). A connect that doesn't resolve to a vetted web vendor — including every loopback (native-app) redirect — keeps the status-quo anonymous screen. - `authorize` resolves the connector for the validated redirect and passes its slug on the II connect link as `&connector=` (additive fragment param; an II that doesn't know it ignores it). - Two issuer-rooted GET endpoints II fetches same-origin with the #4091-validated callback: `/branding/{slug}` (name + absolute logo URL + `verified`, `no-store`) and `/branding/{slug}/logo` (SVG, `nosniff`). Both 404 outside the curated set so the path can't probe. The bundled logos are ORIGINAL neutral placeholders (a generic "verified connector" mark), deliberately not reproductions of vendor logos — each vendor's own licensed mark is dropped into its `Connector::logo` later. Actually rendering this needs II's counterpart change (parse the slug, fetch these endpoints, render); until then it is inert and harmless. Co-Authored-By: Claude Opus 4.8 --- crates/imcp2-core/src/iiconnect.rs | 37 ++++- crates/imcp2-local/src/login/mod.rs | 4 + src/auth.rs | 27 +++- src/branding.rs | 208 ++++++++++++++++++++++++++++ src/lib.rs | 12 ++ tests/routers.rs | 42 ++++++ 6 files changed, 321 insertions(+), 9 deletions(-) create mode 100644 src/branding.rs diff --git a/crates/imcp2-core/src/iiconnect.rs b/crates/imcp2-core/src/iiconnect.rs index dd62559..4b9bf0f 100644 --- a/crates/imcp2-core/src/iiconnect.rs +++ b/crates/imcp2-core/src/iiconnect.rs @@ -32,19 +32,33 @@ use serde::Deserialize; /// the fragment; the callback page rendered by [`pinned_callback_page`] is the /// sole fragment reader. No `priv(X)` is ever put in the link — only its /// public half. +/// +/// `connector`, when present, is the server-curated verified-connector slug for +/// the vetted vendor this connect is for (see the embedding server's branding +/// module): II fetches the product's name and logo by it to brand the consent +/// screen. It rides the fragment like the other params and is **additive** — an +/// II that does not know the parameter ignores it — so emitting it is safe +/// before II's side ships. Omitted for an unvetted/loopback connect, which keeps +/// today's anonymous consent screen. pub fn ii_mcp_url( ii_url: &str, callback_url: &str, state: &str, ttl_secs: u64, reg_pubkey_b64: &str, + connector: Option<&str>, ) -> String { - format!( + let mut url = format!( "{ii_url}/mcp#callback={cb}&state={st}&ttl={ttl_secs}®istration_key={rk}", cb = urlencoding::encode(callback_url), st = urlencoding::encode(state), rk = urlencoding::encode(reg_pubkey_b64), - ) + ); + if let Some(slug) = connector { + url.push_str("&connector="); + url.push_str(&urlencoding::encode(slug)); + } + url } // ---- Callback allow-list (II #4091) --------------------------------------- @@ -514,10 +528,29 @@ mod tests { "sess-1", 3600, "AQID", + None, ); assert!(url.starts_with("https://id.ai/mcp#callback=")); assert!(url.contains("callback=http%3A%2F%2F127.0.0.1%3A4361%2Fcallback"), "{url}"); assert!(url.contains("&state=sess-1&ttl=3600®istration_key=AQID"), "{url}"); + // No connector slug → the parameter is absent (unvetted/loopback connect). + assert!(!url.contains("connector="), "{url}"); + } + + // A vetted connect appends `&connector=` (percent-encoded) after the + // #4093 params — the additive branding hint II reads to brand the consent + // screen. + #[test] + fn ii_mcp_url_appends_the_connector_slug() { + let url = super::ii_mcp_url( + "https://id.ai", + "https://mcp.example.com/mcp/oauth/connect/callback", + "sess-1", + 3600, + "AQID", + Some("chatgpt"), + ); + assert!(url.ends_with("&connector=chatgpt"), "{url}"); } // The page's script resolves the redeem answer three ways: `redirect` diff --git a/crates/imcp2-local/src/login/mod.rs b/crates/imcp2-local/src/login/mod.rs index 4ed1b8d..bad2b2e 100644 --- a/crates/imcp2-local/src/login/mod.rs +++ b/crates/imcp2-local/src/login/mod.rs @@ -234,6 +234,10 @@ impl LoginDriver { &session_id, GRANT_TTL_SECS, ®_pubkey, + // No connector branding: a local app authenticates over a loopback + // callback, which carries no vetted-vendor identity (verified-connector + // branding is web-vendor only). + None, ); let shutdown = Arc::new(Notify::new()); diff --git a/src/auth.rs b/src/auth.rs index 817798e..ba74755 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -1624,8 +1624,9 @@ impl AuthStore { /// This instance's AS issuer: `{public_url}{mcp_path}` (an RFC 8414 *path /// issuer* whenever the router is nested below the root). Every OAuth - /// endpoint lives under it, at `{issuer}/oauth/*`. - fn issuer(&self) -> String { + /// endpoint lives under it, at `{issuer}/oauth/*`; the branding endpoints at + /// `{issuer}/branding/*` ([`crate::branding`]) also root here. + pub(crate) fn issuer(&self) -> String { format!("{}{}", self.public_url, self.mcp_path) } @@ -2240,7 +2241,13 @@ pub async fn authorize( store.mcp_path, CONNECT_TTL.as_secs(), ); - let ii_url = ii_mcp_url(&store, &session_id, ®_pubkey); + // Verified-connector branding (see [`crate::branding`]): the request's + // redirect_uri is validated by this point, so its vendor is a curated signal + // of product identity. Resolve the connector slug for a vetted web vendor + // (`None` for loopback / unlisted → status-quo anonymous consent) and let it + // ride the connect link as `&connector=` for II to brand the screen. + let connector = crate::branding::connector_for_redirect(&q.redirect_uri).map(|c| c.slug); + let ii_url = ii_mcp_url(&store, &session_id, ®_pubkey, connector); // Redirect the consenting browser to the II connect link with a real HTTP // 302 (`Location`). The link's params ride in the URL fragment // (`#callback=…&state=…®istration_key=…`); modern browsers preserve a @@ -2309,13 +2316,19 @@ fn build_redirect(redirect_uri: &str, code: &str, client_state: &str, iss: &str) /// delegation in the fragment; that callback page is our sole fragment reader /// ([`connect_callback_page`]). No `priv(X)` is ever put in the link — only its /// public half. -fn ii_mcp_url(store: &AuthStore, session_id: &str, reg_pubkey_b64: &str) -> String { +fn ii_mcp_url( + store: &AuthStore, + session_id: &str, + reg_pubkey_b64: &str, + connector: Option<&str>, +) -> String { iiconnect::ii_mcp_url( &store.instance().ii_url, &connect_callback_url(store), session_id, GRANT_TTL_SECS, reg_pubkey_b64, + connector, ) } // ---- Callback allow-list (II #4091) --------------------------------------- @@ -4599,7 +4612,7 @@ mod tests { #[test] fn v2_link_carries_registration_key() { let store = test_store(); - let url = super::ii_mcp_url(&store, "sess-1", "PUBX"); + let url = super::ii_mcp_url(&store, "sess-1", "PUBX", None); assert!(url.starts_with("https://ii.test/mcp#"), "everything rides the fragment: {url}"); assert!(url.contains("state=sess-1")); assert!(url.contains("registration_key=PUBX")); @@ -4674,8 +4687,8 @@ mod tests { // Each declared entry must equal the callback embedded in that // instance's II link, byte for byte. for (store, link) in [ - (&prod, super::ii_mcp_url(&prod, "s", "K")), - (&beta, super::ii_mcp_url(&beta, "s", "K")), + (&prod, super::ii_mcp_url(&prod, "s", "K", None)), + (&beta, super::ii_mcp_url(&beta, "s", "K", None)), ] { let expected = super::connect_callback_url(store); assert!(declared.contains(&expected), "{expected} must be declared: {declared:?}"); diff --git a/src/branding.rs b/src/branding.rs new file mode 100644 index 0000000..689f5ea --- /dev/null +++ b/src/branding.rs @@ -0,0 +1,208 @@ +//! Verified-connector branding: surface a vetted MCP client's product name and +//! logo to Internet Identity's consent screen, so the user sees WHICH vetted +//! product is requesting the connect rather than granting their II accounts to an +//! anonymous "some bridge". +//! +//! Keyed on the **vetted redirect vendor** — the request's already-validated +//! `redirect_uri` host matched against the hosted-redirect allow-list — never on +//! the client-supplied `client_name`/`logo_uri`. Open DCR (`/oauth/register`) +//! takes all callers, so client-supplied identity is attacker-controlled; +//! rendering it would be a consent-phishing gift (a hostile client registering +//! `client_name: "Internet Identity"` with a lookalike mark). A client can only +//! obtain a vetted `redirect_uri` if it genuinely is that vendor (the path-pinned +//! allow-list, [`crate::auth`]), so the redirect vendor is a sound, server-curated +//! proxy for product identity. A connect that does not resolve to a vetted vendor +//! — including every loopback (native-app) redirect — gets the status-quo +//! anonymous consent screen; nothing regresses, it just gets no name/logo. +//! +//! II reads two GET endpoints from this origin (the SAME origin as the connect +//! callback it already validated via #4091): `/branding/{slug}` (name + logo URL + +//! `verified`) and `/branding/{slug}/logo` (the bundled image). Both 404 for any +//! slug outside the curated set, so the path segment can't be used to probe. The +//! slug rides the connect link as `&connector={slug}` (see +//! `iiconnect::ii_mcp_url`). Design: `docs/scoping-client-branding.md`. +//! +//! II-side coordination is required to actually render this (parse the slug, fetch +//! these endpoints from the validated callback origin, show a "verified connector" +//! treatment). Until it ships, these endpoints and the link param are inert and +//! harmless. + +use axum::{ + extract::{Path, State}, + http::{header, HeaderValue, StatusCode}, + response::{IntoResponse, Response}, + Json, +}; +use serde_json::json; + +use crate::auth::AuthStore; + +/// A vetted connector's server-curated branding. +pub(crate) struct Connector { + /// Stable slug: the `&connector=` value in the connect link and the + /// `/branding/{slug}` path segment. Server-determined, never client-supplied. + pub slug: &'static str, + /// Human display name shown on the consent screen. + pub name: &'static str, + /// Apex domains identifying this vendor, matched against the (already + /// validated) `redirect_uri` host — the host itself or a subdomain of it. + domains: &'static [&'static str], + /// The bundled logo (SVG markup). A NEUTRAL PLACEHOLDER today + /// ([`PLACEHOLDER_LOGO_SVG`]); swap this per-connector for the vendor's own + /// licensed mark. + logo: &'static str, +} + +/// A neutral, generic "verified connector" mark — ORIGINAL artwork (a check in a +/// ring on a parchment tile), deliberately NOT a reproduction of any vendor's +/// logo, which would be a trademark/licensing matter this repo should not decide. +/// It is the placeholder for every connector until each vendor's own licensed +/// mark is dropped into its [`Connector::logo`]. II renders it via a fixed-size +/// `` (an ``-loaded SVG cannot execute script), so it needs no +/// server-side sanitising. +pub(crate) const PLACEHOLDER_LOGO_SVG: &str = r##""##; + +/// The vetted connectors and their curated branding. The domains mirror the +/// vendor set in `DEFAULT_ALLOWED_REDIRECTS`: a product gets branding exactly when +/// it can obtain a vetted redirect. v1 covers self-authenticating WEB connectors +/// only; native/loopback apps stay anonymous (their only identity signal is a +/// spoofable `client_name`). Logos are placeholders — see [`PLACEHOLDER_LOGO_SVG`]. +pub(crate) const CONNECTORS: &[Connector] = &[ + Connector { slug: "chatgpt", name: "ChatGPT", domains: &["chatgpt.com"], logo: PLACEHOLDER_LOGO_SVG }, + Connector { slug: "claude", name: "Claude", domains: &["claude.ai"], logo: PLACEHOLDER_LOGO_SVG }, + Connector { slug: "cursor", name: "Cursor", domains: &["cursor.com"], logo: PLACEHOLDER_LOGO_SVG }, + Connector { slug: "grok", name: "Grok", domains: &["grok.com"], logo: PLACEHOLDER_LOGO_SVG }, + Connector { + slug: "perplexity", + name: "Perplexity", + domains: &["perplexity.ai", "perplexity.com"], + logo: PLACEHOLDER_LOGO_SVG, + }, + Connector { + slug: "antigravity", + name: "Google Antigravity", + domains: &["antigravity.google"], + logo: PLACEHOLDER_LOGO_SVG, + }, +]; + +/// Whether `host` is `domain` or a subdomain of it, matched at a segment boundary +/// so a look-alike apex (`evilchatgpt.com`) does NOT match `chatgpt.com`. +fn host_matches(host: &str, domain: &str) -> bool { + host == domain || host.strip_suffix(domain).is_some_and(|prefix| prefix.ends_with('.')) +} + +/// The vetted connector a request's already-validated `redirect_uri` identifies, +/// if any. Reads only the host (the redirect is validated by the caller). `None` +/// for a loopback or unlisted host → status-quo anonymous consent. +pub(crate) fn connector_for_redirect(redirect_uri: &str) -> Option<&'static Connector> { + let host = url::Url::parse(redirect_uri).ok()?.host_str()?.to_ascii_lowercase(); + CONNECTORS.iter().find(|c| c.domains.iter().any(|d| host_matches(&host, d))) +} + +/// The connector for a curated slug, or `None` for any slug outside the set (so a +/// `/branding/{slug}` path segment can never probe arbitrary keys). +pub(crate) fn connector_by_slug(slug: &str) -> Option<&'static Connector> { + CONNECTORS.iter().find(|c| c.slug == slug) +} + +/// `GET {issuer}/branding/{slug}` — the tiny metadata document II reads to brand +/// the consent screen: the curated name, the absolute logo URL, and `verified`. +/// `no-store` (II fetches it cross-origin under `permissive_cors`; an +/// intermediary must not serve it stale). `404` outside the curated set. +pub(crate) async fn branding_metadata( + State(store): State, + Path(slug): Path, +) -> Response { + let Some(connector) = connector_by_slug(&slug) else { + return branding_not_found(); + }; + let logo = format!("{}/branding/{}/logo", store.issuer(), connector.slug); + let mut resp = + Json(json!({ "name": connector.name, "logo": logo, "verified": true })).into_response(); + resp.headers_mut().insert(header::CACHE_CONTROL, HeaderValue::from_static("no-store")); + resp +} + +/// `GET {issuer}/branding/{slug}/logo` — the bundled connector logo (SVG bytes), +/// with `nosniff` and a cache header. `404` outside the curated set. II MUST +/// render it via a fixed-size ``, never inline the SVG into the consent +/// DOM: an ``-loaded SVG cannot execute script, an inlined one can. +pub(crate) async fn branding_logo(Path(slug): Path) -> Response { + let Some(connector) = connector_by_slug(&slug) else { + return branding_not_found(); + }; + let mut resp = (StatusCode::OK, connector.logo).into_response(); + let headers = resp.headers_mut(); + headers.insert(header::CONTENT_TYPE, HeaderValue::from_static("image/svg+xml")); + headers.insert(header::X_CONTENT_TYPE_OPTIONS, HeaderValue::from_static("nosniff")); + headers.insert(header::CACHE_CONTROL, HeaderValue::from_static("public, max-age=3600")); + resp +} + +/// A slug outside the curated set: 404, so the path can't be used to probe. +fn branding_not_found() -> Response { + (StatusCode::NOT_FOUND, "not a branded connector").into_response() +} + +#[cfg(test)] +mod tests { + use super::{connector_by_slug, connector_for_redirect, CONNECTORS}; + + #[test] + fn resolves_vetted_redirect_vendors_to_their_slug() { + // Each vetted vendor's own callback resolves to its curated slug. + let cases = [ + ("https://chatgpt.com/connector/oauth/abc", "chatgpt"), + ("https://claude.ai/api/mcp/auth_callback", "claude"), + ("https://cursor.com/agents/mcp/oauth/callback", "cursor"), + ("https://grok.com/mcp/callback", "grok"), + ("https://perplexity.ai/rest/connections/oauth_callback", "perplexity"), + ("https://antigravity.google/oauth-callback", "antigravity"), + ]; + for (redirect, slug) in cases { + assert_eq!( + connector_for_redirect(redirect).map(|c| c.slug), + Some(slug), + "redirect {redirect}" + ); + } + // A subdomain of a vetted vendor still resolves (Perplexity uses www/etc.). + assert_eq!( + connector_for_redirect("https://www.perplexity.ai/rest/connections/oauth_callback") + .map(|c| c.slug), + Some("perplexity") + ); + assert_eq!( + connector_for_redirect("https://www.cursor.com/agents/mcp/oauth/callback") + .map(|c| c.slug), + Some("cursor") + ); + } + + #[test] + fn unvetted_and_lookalike_and_loopback_get_no_branding() { + // Loopback (native app) → no branding, by design. + assert!(connector_for_redirect("http://127.0.0.1:5173/cb").is_none()); + assert!(connector_for_redirect("http://[::1]:8080/cb").is_none()); + // An unlisted vendor → none. + assert!(connector_for_redirect("https://attacker.example/cb").is_none()); + // A look-alike apex must NOT match at a non-segment boundary. + assert!(connector_for_redirect("https://evilchatgpt.com/cb").is_none()); + assert!(connector_for_redirect("https://claude.ai.attacker.example/cb").is_none()); + // Not a URL at all → none, never a panic. + assert!(connector_for_redirect("not a url").is_none()); + } + + #[test] + fn slug_lookup_is_closed_to_the_curated_set() { + assert!(connector_by_slug("claude").is_some()); + assert!(connector_by_slug("unknown").is_none()); + assert!(connector_by_slug("../secrets").is_none()); + assert!(connector_by_slug("").is_none()); + // Every connector has a non-empty slug and name. + for c in CONNECTORS { + assert!(!c.slug.is_empty() && !c.name.is_empty()); + } + } +} diff --git a/src/lib.rs b/src/lib.rs index 901a600..e33f0b5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -73,6 +73,7 @@ //! and `II_URL_PROD` / `II_CANISTER_ID_PROD` (the Internet Identity instances). mod auth; +mod branding; // Docs live in the module itself (`//!` in src/metrics.rs): an outer doc // comment here would resolve its intra-doc links in *this* scope rather than // the module's, silently breaking the links to `Registry` and `MatchedPath`. @@ -330,9 +331,20 @@ impl McpServer { .with_state(self.store.clone()) .layer(permissive_cors()); + // Verified-connector branding (see [`branding`]), rooted at the issuer + // (`{public_url}{mcp_path}/branding/{slug}`) so II reaches it same-origin + // with the #4091-validated callback. CORS-open, like the other endpoints + // II fetches cross-origin. + let branding = Router::new() + .route("/branding/{slug}", get(branding::branding_metadata)) + .route("/branding/{slug}/logo", get(branding::branding_logo)) + .with_state(self.store.clone()) + .layer(permissive_cors()); + Router::new() .nest("/oauth", oauth) .merge(oidc_alternate) + .merge(branding) // Everything else is the MCP endpoint: the bare mount path, its // trailing-slash form, and any sub-path (the streamable service // dispatches on method, not path) — same breadth `nest_service` diff --git a/tests/routers.rs b/tests/routers.rs index a5af63c..1023746 100644 --- a/tests/routers.rs +++ b/tests/routers.rs @@ -451,3 +451,45 @@ async fn oauth_endpoints_live_under_each_mount() { ); } } + +/// Verified-connector branding endpoints (II fetches these from the issuer origin +/// to brand the consent screen): a curated slug yields name + logo + verified; a +/// slug outside the curated set 404s so the path can't be used to probe. +#[tokio::test] +async fn branding_endpoints_serve_vetted_connectors_and_404_others() { + // Metadata for a vetted connector: the curated name, the absolute logo URL + // under the instance issuer, `verified`, and `no-store`. + let resp = + app().oneshot(Request::get("/mcp/branding/claude").body(Body::empty()).unwrap()).await.unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!( + resp.headers().get("cache-control").and_then(|v| v.to_str().ok()), + Some("no-store") + ); + let bytes = resp.into_body().collect().await.unwrap().to_bytes(); + let doc: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(doc["name"], "Claude"); + assert_eq!(doc["verified"], true); + assert_eq!(doc["logo"], format!("{PUBLIC_URL}/mcp/branding/claude/logo")); + + // The logo is an SVG with nosniff (II renders it via , never inlined). + let resp = app() + .oneshot(Request::get("/mcp/branding/claude/logo").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!( + resp.headers().get("content-type").and_then(|v| v.to_str().ok()), + Some("image/svg+xml") + ); + assert_eq!( + resp.headers().get("x-content-type-options").and_then(|v| v.to_str().ok()), + Some("nosniff") + ); + + // A slug outside the curated set 404s on both endpoints. + for path in ["/mcp/branding/not-a-connector", "/mcp/branding/not-a-connector/logo"] { + let resp = app().oneshot(Request::get(path).body(Body::empty()).unwrap()).await.unwrap(); + assert_eq!(resp.status(), StatusCode::NOT_FOUND, "GET {path}"); + } +} From 24ab3feef57a8591d81b5b0cb1737a4267f05c0f Mon Sep 17 00:00:00 2001 From: Arshavir Ter-Gabrielyan Date: Thu, 24 Sep 2026 14:04:39 +0200 Subject: [PATCH 2/6] Bind connector branding to the authorization session The first cut let the connect-link fragment carry `&connector=` and served `GET {issuer}/branding/{slug}` unconditionally. The fragment is craftable by any native client that drives the navigation, so a loopback session could have presented itself to Internet Identity as "verified Claude". Branding is now answered per session: II asks `GET {issuer}/branding?state=` with the `state` it already holds, and the server derives the connector from that pending, unexpired connect's validated `redirect_uri`. Nothing in the fragment asserts branding (the connector param is gone from `ii_mcp_url`), and a loopback or unlisted session gets the same 404 as an unknown one. The lookup is read-only, so it cannot disturb the handshake. Branding and redirect validation now share one host rule (`host_key` + `host_is_or_under`), so a trailing-dot `claude.ai.` redirect, which validation accepts, brands as Claude instead of reading as anonymous; the allow-list parser and the CIMD trust match use the same helpers. A redirect validation would refuse never resolves to a connector, and a test holds the curated domains equal to the compiled-in allow-list vendors. The DCR router test now drives the 200 path over HTTP (authorize -> the `state` in II's link -> `/branding?state=`), including `no-store` and both claude.ai spellings. Co-Authored-By: Claude Opus 5.5 --- crates/imcp2-core/src/iiconnect.rs | 37 +---- crates/imcp2-local/src/login/mod.rs | 4 - src/auth.rs | 185 +++++++++++++++++++---- src/branding.rs | 218 +++++++++++++++++++--------- src/lib.rs | 11 +- tests/routers.rs | 156 +++++++++++++++----- 6 files changed, 427 insertions(+), 184 deletions(-) diff --git a/crates/imcp2-core/src/iiconnect.rs b/crates/imcp2-core/src/iiconnect.rs index 4b9bf0f..dd62559 100644 --- a/crates/imcp2-core/src/iiconnect.rs +++ b/crates/imcp2-core/src/iiconnect.rs @@ -32,33 +32,19 @@ use serde::Deserialize; /// the fragment; the callback page rendered by [`pinned_callback_page`] is the /// sole fragment reader. No `priv(X)` is ever put in the link — only its /// public half. -/// -/// `connector`, when present, is the server-curated verified-connector slug for -/// the vetted vendor this connect is for (see the embedding server's branding -/// module): II fetches the product's name and logo by it to brand the consent -/// screen. It rides the fragment like the other params and is **additive** — an -/// II that does not know the parameter ignores it — so emitting it is safe -/// before II's side ships. Omitted for an unvetted/loopback connect, which keeps -/// today's anonymous consent screen. pub fn ii_mcp_url( ii_url: &str, callback_url: &str, state: &str, ttl_secs: u64, reg_pubkey_b64: &str, - connector: Option<&str>, ) -> String { - let mut url = format!( + format!( "{ii_url}/mcp#callback={cb}&state={st}&ttl={ttl_secs}®istration_key={rk}", cb = urlencoding::encode(callback_url), st = urlencoding::encode(state), rk = urlencoding::encode(reg_pubkey_b64), - ); - if let Some(slug) = connector { - url.push_str("&connector="); - url.push_str(&urlencoding::encode(slug)); - } - url + ) } // ---- Callback allow-list (II #4091) --------------------------------------- @@ -528,29 +514,10 @@ mod tests { "sess-1", 3600, "AQID", - None, ); assert!(url.starts_with("https://id.ai/mcp#callback=")); assert!(url.contains("callback=http%3A%2F%2F127.0.0.1%3A4361%2Fcallback"), "{url}"); assert!(url.contains("&state=sess-1&ttl=3600®istration_key=AQID"), "{url}"); - // No connector slug → the parameter is absent (unvetted/loopback connect). - assert!(!url.contains("connector="), "{url}"); - } - - // A vetted connect appends `&connector=` (percent-encoded) after the - // #4093 params — the additive branding hint II reads to brand the consent - // screen. - #[test] - fn ii_mcp_url_appends_the_connector_slug() { - let url = super::ii_mcp_url( - "https://id.ai", - "https://mcp.example.com/mcp/oauth/connect/callback", - "sess-1", - 3600, - "AQID", - Some("chatgpt"), - ); - assert!(url.ends_with("&connector=chatgpt"), "{url}"); } // The page's script resolves the redeem answer three ways: `redirect` diff --git a/crates/imcp2-local/src/login/mod.rs b/crates/imcp2-local/src/login/mod.rs index bad2b2e..4ed1b8d 100644 --- a/crates/imcp2-local/src/login/mod.rs +++ b/crates/imcp2-local/src/login/mod.rs @@ -234,10 +234,6 @@ impl LoginDriver { &session_id, GRANT_TTL_SECS, ®_pubkey, - // No connector branding: a local app authenticates over a loopback - // callback, which carries no vetted-vendor identity (verified-connector - // branding is web-vendor only). - None, ); let shutdown = Arc::new(Notify::new()); diff --git a/src/auth.rs b/src/auth.rs index ba74755..57ec0a5 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -515,6 +515,12 @@ const DEFAULT_ALLOWED_REDIRECTS: &[(&str, &str, PathPin)] = &[ ("perplexity.com", "/rest/connections/oauth_callback", PathPin::Prefix), ]; +/// The compiled-in allow-list's vendor domains, for the branding parity test. +#[cfg(test)] +pub(crate) fn default_redirect_domains() -> std::collections::BTreeSet<&'static str> { + DEFAULT_ALLOWED_REDIRECTS.iter().map(|(domain, _, _)| *domain).collect() +} + /// The effective hosted-redirect allow-list: the compiled-in defaults plus any /// entries in `OAUTH_ALLOWED_REDIRECT_PREFIXES`. Each env entry is a bare /// `https://host/path` value pinning a host + path prefix; an entry that is not @@ -568,7 +574,7 @@ fn parse_redirect_prefix(raw: &str) -> Option<(String, String)> { { return None; } - let host = u.host_str()?.trim_end_matches('.').to_ascii_lowercase(); + let host = host_key(u.host_str()?); let path = u.path().to_string(); (!host.is_empty() && path != "/" && !path.is_empty()).then_some((host, path)) } @@ -601,6 +607,14 @@ fn path_has_percent_encoding(path: &str) -> bool { path.contains('%') } +/// Whether `host` (already [`host_key`]-normalized) is `domain` or a +/// dot-boundary subdomain of it, so a look-alike apex (`evilchatgpt.com`) never +/// matches `chatgpt.com`. The one host rule for redirect validation, the CIMD +/// trust policy, and [`crate::branding`], so none can disagree about a vendor. +pub(crate) fn host_is_or_under(host: &str, domain: &str) -> bool { + host == domain || host.strip_suffix(domain).is_some_and(|p| p.ends_with('.')) +} + /// Whether a `redirect_uri` may be registered or receive an authorization code. /// No redirect (loopback or hosted) may carry a query or fragment component: the /// authorization endpoint appends `?code=…&state=…`, so a pre-existing query would @@ -619,7 +633,7 @@ fn path_has_percent_encoding(path: &str) -> bool { /// `https://claude.ai.evil.com` resolve to their real host and are refused; a bare /// `https://user@claude.ai` is refused too (userinfo serves no purpose in a /// redirect target, only muddies which host is addressed, and loopback rejects it). -fn redirect_uri_permitted(redirect_uri: &str) -> bool { +pub(crate) fn redirect_uri_permitted(redirect_uri: &str) -> bool { let Ok(url) = url::Url::parse(redirect_uri) else { return false; }; @@ -654,7 +668,7 @@ fn redirect_uri_permitted(redirect_uri: &str) -> bool { if url != canonical { return false; } - let host = host.trim_end_matches('.').to_ascii_lowercase(); + let host = host_key(host); let path = url.path(); // url::Url collapses only LITERAL `.`/`..` segments (raw or `%2e`-encoded whole // segments), so a real-slash traversal `/connector/oauth/../../g/evil` arrives @@ -674,7 +688,7 @@ fn redirect_uri_permitted(redirect_uri: &str) -> bool { // third-party/user-content paths (e.g. `/page/…`, `/g/…`) on the same origin; // without it, domain-only matching would let those capture the code. allowed_redirects().iter().any(|(domain, prefix, pin)| { - (host == *domain || host.strip_suffix(domain.as_str()).is_some_and(|p| p.ends_with('.'))) + host_is_or_under(&host, domain) && match pin { PathPin::Exact => path == prefix, PathPin::Prefix => path_within_prefix(path, prefix), @@ -990,15 +1004,17 @@ fn allow_listed_domain(host: &str) -> bool { /// subdomain of, if any: the trust policy's match. fn vetted_domain(host: &str) -> Option<&'static str> { let host = host_key(host); - allowed_redirects().iter().map(|(domain, _, _)| domain.as_str()).find(|domain| { - host == *domain || host.strip_suffix(*domain).is_some_and(|p| p.ends_with('.')) - }) + allowed_redirects() + .iter() + .map(|(domain, _, _)| domain.as_str()) + .find(|domain| host_is_or_under(&host, domain)) } /// One spelling per host — lower-case, no trailing dot — so that whatever is -/// keyed by host (the trust policy's match, the per-host in-flight slots) treats -/// `claude.ai`, `Claude.AI` and `claude.ai.` as the one host they resolve to. -fn host_key(host: &str) -> String { +/// keyed or matched by host (redirect validation, the trust policy's match, the +/// per-host in-flight slots, [`crate::branding`]) treats `claude.ai`, +/// `Claude.AI` and `claude.ai.` as the one host they resolve to. +pub(crate) fn host_key(host: &str) -> String { host.trim_end_matches('.').to_ascii_lowercase() } @@ -1625,7 +1641,7 @@ impl AuthStore { /// This instance's AS issuer: `{public_url}{mcp_path}` (an RFC 8414 *path /// issuer* whenever the router is nested below the root). Every OAuth /// endpoint lives under it, at `{issuer}/oauth/*`; the branding endpoints at - /// `{issuer}/branding/*` ([`crate::branding`]) also root here. + /// `{issuer}/branding*` ([`crate::branding`]) root here too. pub(crate) fn issuer(&self) -> String { format!("{}{}", self.public_url, self.mcp_path) } @@ -1855,6 +1871,20 @@ impl AuthStore { authz.insert(session_id, pending); } + /// The validated `redirect_uri` of the pending, unexpired connect + /// `session_id`, if there is one. Read-only — it neither consumes nor touches + /// the entry — so a lookup cannot interfere with the handshake. Branding + /// ([`crate::branding`]) derives the connector from THIS, the server's record + /// of what `authorize` validated, and never from anything the (craftable) + /// connect-link fragment asserts. + pub(crate) async fn pending_redirect_uri(&self, session_id: &str) -> Option { + let authz = self.authz.read().await; + authz + .get(session_id) + .filter(|pending| !pending.remaining().is_zero()) + .map(|pending| pending.redirect_uri.clone()) + } + /// Drop every expired entry from the short-lived OAuth maps, returning how /// many went from each. The admission bounds ([`make_room`]) cap these maps at /// all times; this is what returns the memory once a burst has passed, so an @@ -2241,13 +2271,7 @@ pub async fn authorize( store.mcp_path, CONNECT_TTL.as_secs(), ); - // Verified-connector branding (see [`crate::branding`]): the request's - // redirect_uri is validated by this point, so its vendor is a curated signal - // of product identity. Resolve the connector slug for a vetted web vendor - // (`None` for loopback / unlisted → status-quo anonymous consent) and let it - // ride the connect link as `&connector=` for II to brand the screen. - let connector = crate::branding::connector_for_redirect(&q.redirect_uri).map(|c| c.slug); - let ii_url = ii_mcp_url(&store, &session_id, ®_pubkey, connector); + let ii_url = ii_mcp_url(&store, &session_id, ®_pubkey); // Redirect the consenting browser to the II connect link with a real HTTP // 302 (`Location`). The link's params ride in the URL fragment // (`#callback=…&state=…®istration_key=…`); modern browsers preserve a @@ -2316,19 +2340,13 @@ fn build_redirect(redirect_uri: &str, code: &str, client_state: &str, iss: &str) /// delegation in the fragment; that callback page is our sole fragment reader /// ([`connect_callback_page`]). No `priv(X)` is ever put in the link — only its /// public half. -fn ii_mcp_url( - store: &AuthStore, - session_id: &str, - reg_pubkey_b64: &str, - connector: Option<&str>, -) -> String { +fn ii_mcp_url(store: &AuthStore, session_id: &str, reg_pubkey_b64: &str) -> String { iiconnect::ii_mcp_url( &store.instance().ii_url, &connect_callback_url(store), session_id, GRANT_TTL_SECS, reg_pubkey_b64, - connector, ) } // ---- Callback allow-list (II #4091) --------------------------------------- @@ -4612,16 +4630,127 @@ mod tests { #[test] fn v2_link_carries_registration_key() { let store = test_store(); - let url = super::ii_mcp_url(&store, "sess-1", "PUBX", None); + let url = super::ii_mcp_url(&store, "sess-1", "PUBX"); assert!(url.starts_with("https://ii.test/mcp#"), "everything rides the fragment: {url}"); assert!(url.contains("state=sess-1")); assert!(url.contains("registration_key=PUBX")); + // The fragment is attacker-craftable, so it must never ASSERT branding: + // II asks the server (`/branding?state=…`), which answers from the + // session's validated redirect (see `branding_is_bound_to_the_session`). + assert!(!url.contains("connector"), "no branding hint in the fragment: {url}"); // The callback lives under the instance's mount ({public_url}{mcp_path}). let encoded = urlencoding::encode("https://mcp.test/mcp/oauth/connect/callback").into_owned(); assert!(url.contains(&format!("callback={encoded}")), "callback under the mount: {url}"); } + /// Record a pending connect for `redirect` the way `authorize` does (through + /// the bounded insert), created at `created`. + async fn seed_pending_redirect( + store: &super::AuthStore, + id: &str, + redirect: &str, + created: std::time::Instant, + ) { + store + .insert_pending( + id.to_string(), + super::AuthzPending { + client_id: "c".into(), + redirect_uri: redirect.into(), + client_state: String::new(), + code_challenge: Some("cc".into()), + cookie: "k".into(), + created, + code: None, + redeeming: false, + }, + ) + .await; + } + + /// GET `/branding?state=` for `state`: the status and (on 200) the JSON body. + async fn branding_for( + store: &super::AuthStore, + state: Option<&str>, + ) -> (axum::http::StatusCode, serde_json::Value) { + use axum::extract::{Query, State}; + let resp = crate::branding::branding_metadata( + State(store.clone()), + Ok(Query(crate::branding::BrandingQuery { state: state.map(str::to_string) })), + ) + .await; + let status = resp.status(); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX).await.expect("body"); + (status, serde_json::from_slice(&bytes).unwrap_or(serde_json::Value::Null)) + } + + // Branding is BOUND to the authorization session: II asks by `state`, and the + // server answers from that pending connect's VALIDATED redirect — never from + // anything the craftable fragment asserts. So a loopback/native client that + // drives navigation cannot make its own (valid) session read as a vetted + // connector: there is no slug to add or swap, and its session's redirect is + // loopback. Unknown, missing, and expired states all get the same 404. + #[tokio::test] + async fn branding_is_bound_to_the_session() { + use axum::http::StatusCode; + let store = test_store(); + let now = std::time::Instant::now(); + seed_pending_redirect( + &store, + "sess-claude", + "https://claude.ai/api/mcp/auth_callback", + now, + ) + .await; + seed_pending_redirect(&store, "sess-native", "http://127.0.0.1:5173/cb", now).await; + // Validation trims a trailing root dot, so branding must too. + seed_pending_redirect(&store, "sess-dot", "https://claude.ai./api/mcp/auth_callback", now) + .await; + + // A vetted session reads as its connector, with the logo under the issuer. + let (status, doc) = branding_for(&store, Some("sess-claude")).await; + assert_eq!(status, StatusCode::OK); + assert_eq!(doc["name"], "Claude"); + assert_eq!(doc["verified"], true); + assert_eq!(doc["logo"], "https://mcp.test/mcp/branding/claude/logo"); + + // The core of the finding: a valid NATIVE session gets no branding. + assert_eq!(branding_for(&store, Some("sess-native")).await.0, StatusCode::NOT_FOUND); + + // The trailing-dot form of a vetted redirect keeps its branding. + let (status, doc) = branding_for(&store, Some("sess-dot")).await; + assert_eq!(status, StatusCode::OK); + assert_eq!(doc["name"], "Claude"); + + // Unknown and missing states are indistinguishable from "no branding". + assert_eq!(branding_for(&store, Some("sess-unknown")).await.0, StatusCode::NOT_FOUND); + assert_eq!(branding_for(&store, None).await.0, StatusCode::NOT_FOUND); + + // An EXPIRED pending connect is gone for branding too (same definition of + // expired as the handshake: `AuthzPending::remaining`). Skipped only on a + // host whose monotonic clock started less than CONNECT_TTL ago. + if let Some(past) = now.checked_sub(super::CONNECT_TTL + std::time::Duration::from_secs(1)) + { + seed_pending_redirect( + &store, + "sess-old", + "https://claude.ai/api/mcp/auth_callback", + past, + ) + .await; + assert_eq!(branding_for(&store, Some("sess-old")).await.0, StatusCode::NOT_FOUND); + } + + // Read-only: the lookups neither consumed nor advanced the pending + // connect, so the handshake proceeds exactly as if II never asked. + let authz = store.authz.read().await; + let pending = authz.get("sess-claude").expect("still pending"); + assert_eq!(pending.redirect_uri, "https://claude.ai/api/mcp/auth_callback"); + assert_eq!(pending.created, now); + assert!(pending.code.is_none() && !pending.redeeming); + } + // The allow-list invariant (II #4091 matches by EXACT string equality): the // /.well-known/ii-auth-callbacks document must declare, verbatim, the same // callback URLs the II links embed — for every instance. Built from one @@ -4687,8 +4816,8 @@ mod tests { // Each declared entry must equal the callback embedded in that // instance's II link, byte for byte. for (store, link) in [ - (&prod, super::ii_mcp_url(&prod, "s", "K", None)), - (&beta, super::ii_mcp_url(&beta, "s", "K", None)), + (&prod, super::ii_mcp_url(&prod, "s", "K")), + (&beta, super::ii_mcp_url(&beta, "s", "K")), ] { let expected = super::connect_callback_url(store); assert!(declared.contains(&expected), "{expected} must be declared: {declared:?}"); diff --git a/src/branding.rs b/src/branding.rs index 689f5ea..aff3ba4 100644 --- a/src/branding.rs +++ b/src/branding.rs @@ -3,49 +3,59 @@ //! product is requesting the connect rather than granting their II accounts to an //! anonymous "some bridge". //! -//! Keyed on the **vetted redirect vendor** — the request's already-validated -//! `redirect_uri` host matched against the hosted-redirect allow-list — never on -//! the client-supplied `client_name`/`logo_uri`. Open DCR (`/oauth/register`) -//! takes all callers, so client-supplied identity is attacker-controlled; -//! rendering it would be a consent-phishing gift (a hostile client registering -//! `client_name: "Internet Identity"` with a lookalike mark). A client can only -//! obtain a vetted `redirect_uri` if it genuinely is that vendor (the path-pinned -//! allow-list, [`crate::auth`]), so the redirect vendor is a sound, server-curated -//! proxy for product identity. A connect that does not resolve to a vetted vendor -//! — including every loopback (native-app) redirect — gets the status-quo -//! anonymous consent screen; nothing regresses, it just gets no name/logo. +//! **The server is the only source of branding, and it answers per session.** II +//! asks `GET {issuer}/branding?state={state}` — the `state` it already holds from +//! the connect link — and the server derives the connector from that pending, +//! unexpired connect's **validated** `redirect_uri` (see +//! [`AuthStore::pending_redirect_uri`]). Nothing in the connect-link fragment +//! asserts branding: the fragment is attacker-craftable (a native client drives +//! the navigation and sees the 302), so a client-supplied slug there would let +//! any valid session claim to be "verified Claude". Binding to the session's +//! validated redirect closes that at the root: a loopback (native-app) session, +//! or one on an unlisted host, reads as anonymous no matter what the fragment +//! says. //! -//! II reads two GET endpoints from this origin (the SAME origin as the connect -//! callback it already validated via #4091): `/branding/{slug}` (name + logo URL + -//! `verified`) and `/branding/{slug}/logo` (the bundled image). Both 404 for any -//! slug outside the curated set, so the path segment can't be used to probe. The -//! slug rides the connect link as `&connector={slug}` (see -//! `iiconnect::ii_mcp_url`). Design: `docs/scoping-client-branding.md`. +//! Keyed on the **vetted redirect vendor**, never on the client-supplied +//! `client_name`/`logo_uri`: open DCR takes all callers, so client-supplied +//! identity is attacker-controlled. Anyone may *register* a vetted +//! `redirect_uri`, but the path-pinned allow-list means the authorization code +//! for it is delivered only to that vendor's own callback — so whoever completes +//! the connect is that vendor, and the redirect vendor is a sound, +//! server-curated proxy for product identity. (That holds for a conforming +//! browser; a user agent the attacker controls, such as an embedded webview, is +//! out of scope for branding as it is for the rest of the flow.) Host matching +//! reuses validation's own rule ([`crate::auth::host_key`], +//! [`crate::auth::host_is_or_under`]), and a redirect validation would refuse +//! never resolves, so branding and validation cannot disagree about a vendor. //! -//! II-side coordination is required to actually render this (parse the slug, fetch -//! these endpoints from the validated callback origin, show a "verified connector" -//! treatment). Until it ships, these endpoints and the link param are inert and -//! harmless. +//! Endpoints, both issuer-rooted (same origin as the #4091-validated callback): +//! `GET /branding?state=…` (session-bound metadata: name, logo URL, `verified`) +//! and `GET /branding/{slug}/logo` (the bundled image — a static per-connector +//! asset, no trust decision). II-side coordination is required to render any of +//! this — including showing the `verified` badge only for imcp2 issuer origins II +//! itself trusts, and using the one `state` it parsed from the link for both the +//! lookup and the callback. Until that ships, the endpoints are inert. use axum::{ - extract::{Path, State}, + extract::{rejection::QueryRejection, Path, Query, State}, http::{header, HeaderValue, StatusCode}, response::{IntoResponse, Response}, Json, }; +use serde::Deserialize; use serde_json::json; -use crate::auth::AuthStore; +use crate::auth::{host_is_or_under, host_key, redirect_uri_permitted, AuthStore}; /// A vetted connector's server-curated branding. pub(crate) struct Connector { - /// Stable slug: the `&connector=` value in the connect link and the - /// `/branding/{slug}` path segment. Server-determined, never client-supplied. + /// Stable slug: the `/branding/{slug}/logo` path segment. Server-determined, + /// never client-supplied. pub slug: &'static str, /// Human display name shown on the consent screen. pub name: &'static str, - /// Apex domains identifying this vendor, matched against the (already - /// validated) `redirect_uri` host — the host itself or a subdomain of it. + /// Apex domains identifying this vendor, matched against the validated + /// `redirect_uri` host — the host itself or a dot-boundary subdomain of it. domains: &'static [&'static str], /// The bundled logo (SVG markup). A NEUTRAL PLACEHOLDER today /// ([`PLACEHOLDER_LOGO_SVG`]); swap this per-connector for the vendor's own @@ -62,15 +72,32 @@ pub(crate) struct Connector { /// server-side sanitising. pub(crate) const PLACEHOLDER_LOGO_SVG: &str = r##""##; -/// The vetted connectors and their curated branding. The domains mirror the -/// vendor set in `DEFAULT_ALLOWED_REDIRECTS`: a product gets branding exactly when -/// it can obtain a vetted redirect. v1 covers self-authenticating WEB connectors -/// only; native/loopback apps stay anonymous (their only identity signal is a -/// spoofable `client_name`). Logos are placeholders — see [`PLACEHOLDER_LOGO_SVG`]. +/// The vetted connectors and their curated branding. The domains are exactly the +/// vendor domains of the compiled-in `DEFAULT_ALLOWED_REDIRECTS` (a test holds +/// the two sets equal); a host ops adds via `OAUTH_ALLOWED_REDIRECT_PREFIXES` is +/// accepted for redirects but stays anonymous until it is curated here. v1 +/// covers self-authenticating WEB connectors only; native/loopback apps stay +/// anonymous (their only identity signal is a spoofable `client_name`). Logos +/// are placeholders — see [`PLACEHOLDER_LOGO_SVG`]. pub(crate) const CONNECTORS: &[Connector] = &[ - Connector { slug: "chatgpt", name: "ChatGPT", domains: &["chatgpt.com"], logo: PLACEHOLDER_LOGO_SVG }, - Connector { slug: "claude", name: "Claude", domains: &["claude.ai"], logo: PLACEHOLDER_LOGO_SVG }, - Connector { slug: "cursor", name: "Cursor", domains: &["cursor.com"], logo: PLACEHOLDER_LOGO_SVG }, + Connector { + slug: "chatgpt", + name: "ChatGPT", + domains: &["chatgpt.com"], + logo: PLACEHOLDER_LOGO_SVG, + }, + Connector { + slug: "claude", + name: "Claude", + domains: &["claude.ai"], + logo: PLACEHOLDER_LOGO_SVG, + }, + Connector { + slug: "cursor", + name: "Cursor", + domains: &["cursor.com"], + logo: PLACEHOLDER_LOGO_SVG, + }, Connector { slug: "grok", name: "Grok", domains: &["grok.com"], logo: PLACEHOLDER_LOGO_SVG }, Connector { slug: "perplexity", @@ -86,35 +113,55 @@ pub(crate) const CONNECTORS: &[Connector] = &[ }, ]; -/// Whether `host` is `domain` or a subdomain of it, matched at a segment boundary -/// so a look-alike apex (`evilchatgpt.com`) does NOT match `chatgpt.com`. -fn host_matches(host: &str, domain: &str) -> bool { - host == domain || host.strip_suffix(domain).is_some_and(|prefix| prefix.ends_with('.')) -} - -/// The vetted connector a request's already-validated `redirect_uri` identifies, -/// if any. Reads only the host (the redirect is validated by the caller). `None` -/// for a loopback or unlisted host → status-quo anonymous consent. +/// The vetted connector a `redirect_uri` belongs to, if any. The redirect must +/// pass validation itself ([`redirect_uri_permitted`]: allow-listed path, no +/// port, userinfo, query, or percent-encoding) and be `https`, so this never +/// depends on its caller having validated; the host then matches exactly as +/// validation does — trailing root dots trimmed, lowercased, dot-boundary +/// subdomains. `None` for loopback or an unlisted host, which read as anonymous. pub(crate) fn connector_for_redirect(redirect_uri: &str) -> Option<&'static Connector> { - let host = url::Url::parse(redirect_uri).ok()?.host_str()?.to_ascii_lowercase(); - CONNECTORS.iter().find(|c| c.domains.iter().any(|d| host_matches(&host, d))) + if !redirect_uri_permitted(redirect_uri) { + return None; + } + let url = url::Url::parse(redirect_uri).ok()?; + if url.scheme() != "https" { + return None; + } + let host = host_key(url.host_str()?); + CONNECTORS.iter().find(|c| c.domains.iter().any(|domain| host_is_or_under(&host, domain))) } /// The connector for a curated slug, or `None` for any slug outside the set (so a -/// `/branding/{slug}` path segment can never probe arbitrary keys). +/// `/branding/{slug}/logo` path segment can never probe arbitrary keys). pub(crate) fn connector_by_slug(slug: &str) -> Option<&'static Connector> { CONNECTORS.iter().find(|c| c.slug == slug) } -/// `GET {issuer}/branding/{slug}` — the tiny metadata document II reads to brand -/// the consent screen: the curated name, the absolute logo URL, and `verified`. -/// `no-store` (II fetches it cross-origin under `permissive_cors`; an -/// intermediary must not serve it stale). `404` outside the curated set. +/// `?state=` of the session-bound metadata request: the connect `state` (= the +/// pending session id) II already holds from the connect link. +#[derive(Deserialize)] +pub(crate) struct BrandingQuery { + pub(crate) state: Option, +} + +/// `GET {issuer}/branding?state={state}` — the connector branding for ONE pending +/// connect: the curated name, the absolute logo URL, and `verified`, derived from +/// that connect's validated redirect. `404` — identically — for a missing, +/// unknown, or expired `state` and for a session whose redirect is not a vetted +/// connector (loopback / unlisted), so the response is no oracle for which +/// sessions exist; a malformed query (e.g. a repeated `state`) gets that same +/// 404 rather than axum's 400. `no-store`: the answer is per-session. pub(crate) async fn branding_metadata( State(store): State, - Path(slug): Path, + query: Result, QueryRejection>, ) -> Response { - let Some(connector) = connector_by_slug(&slug) else { + let Some(state) = query.ok().and_then(|Query(query)| query.state) else { + return branding_not_found(); + }; + let Some(redirect) = store.pending_redirect_uri(&state).await else { + return branding_not_found(); + }; + let Some(connector) = connector_for_redirect(&redirect) else { return branding_not_found(); }; let logo = format!("{}/branding/{}/logo", store.issuer(), connector.slug); @@ -125,9 +172,11 @@ pub(crate) async fn branding_metadata( } /// `GET {issuer}/branding/{slug}/logo` — the bundled connector logo (SVG bytes), -/// with `nosniff` and a cache header. `404` outside the curated set. II MUST -/// render it via a fixed-size ``, never inline the SVG into the consent -/// DOM: an ``-loaded SVG cannot execute script, an inlined one can. +/// with `nosniff` and a cache header. `404` outside the curated set. A static +/// per-connector asset: it makes no claim about any session (that is +/// [`branding_metadata`]'s job, whose `logo` URL points here). II MUST render it +/// via a fixed-size ``, never inline the SVG into the consent DOM: an +/// ``-loaded SVG cannot execute script, an inlined one can. pub(crate) async fn branding_logo(Path(slug): Path) -> Response { let Some(connector) = connector_by_slug(&slug) else { return branding_not_found(); @@ -140,18 +189,19 @@ pub(crate) async fn branding_logo(Path(slug): Path) -> Response { resp } -/// A slug outside the curated set: 404, so the path can't be used to probe. +/// No branding for this request: 404, the same for every reason. fn branding_not_found() -> Response { - (StatusCode::NOT_FOUND, "not a branded connector").into_response() + (StatusCode::NOT_FOUND, "no connector branding").into_response() } #[cfg(test)] mod tests { + use std::collections::BTreeSet; + use super::{connector_by_slug, connector_for_redirect, CONNECTORS}; #[test] fn resolves_vetted_redirect_vendors_to_their_slug() { - // Each vetted vendor's own callback resolves to its curated slug. let cases = [ ("https://chatgpt.com/connector/oauth/abc", "chatgpt"), ("https://claude.ai/api/mcp/auth_callback", "claude"), @@ -159,6 +209,14 @@ mod tests { ("https://grok.com/mcp/callback", "grok"), ("https://perplexity.ai/rest/connections/oauth_callback", "perplexity"), ("https://antigravity.google/oauth-callback", "antigravity"), + // Subdomains (Perplexity uses www/staging/…; Cursor registers www). + ("https://www.perplexity.ai/rest/connections/oauth_callback", "perplexity"), + ("https://www.cursor.com/agents/mcp/oauth/callback", "cursor"), + // A trailing root dot names the same host; validation accepts it, so + // branding must resolve it too. + ("https://claude.ai./api/mcp/auth_callback", "claude"), + // Host case never matters. + ("https://CLAUDE.AI/api/mcp/auth_callback", "claude"), ]; for (redirect, slug) in cases { assert_eq!( @@ -167,17 +225,6 @@ mod tests { "redirect {redirect}" ); } - // A subdomain of a vetted vendor still resolves (Perplexity uses www/etc.). - assert_eq!( - connector_for_redirect("https://www.perplexity.ai/rest/connections/oauth_callback") - .map(|c| c.slug), - Some("perplexity") - ); - assert_eq!( - connector_for_redirect("https://www.cursor.com/agents/mcp/oauth/callback") - .map(|c| c.slug), - Some("cursor") - ); } #[test] @@ -185,22 +232,49 @@ mod tests { // Loopback (native app) → no branding, by design. assert!(connector_for_redirect("http://127.0.0.1:5173/cb").is_none()); assert!(connector_for_redirect("http://[::1]:8080/cb").is_none()); + assert!(connector_for_redirect("http://localhost/cb").is_none()); // An unlisted vendor → none. assert!(connector_for_redirect("https://attacker.example/cb").is_none()); - // A look-alike apex must NOT match at a non-segment boundary. + // Look-alikes must NOT match at a non-segment boundary. assert!(connector_for_redirect("https://evilchatgpt.com/cb").is_none()); assert!(connector_for_redirect("https://claude.ai.attacker.example/cb").is_none()); + // A non-https vetted host never resolves (validation would refuse it). + assert!(connector_for_redirect("http://claude.ai/api/mcp/auth_callback").is_none()); // Not a URL at all → none, never a panic. assert!(connector_for_redirect("not a url").is_none()); } + #[test] + fn a_redirect_validation_refuses_never_resolves() { + // A vetted host, but a path off its pin, a port, userinfo, a query, or + // percent-encoding: validation refuses each, so branding must too. + for redirect in [ + "https://claude.ai/not/the/callback", + "https://claude.ai:8443/api/mcp/auth_callback", + "https://user@claude.ai/api/mcp/auth_callback", + "https://claude.ai/api/mcp/auth_callback?x=1", + "https://claude.ai/api/mcp/auth_callback/%2e%2e", + ] { + assert!(connector_for_redirect(redirect).is_none(), "redirect {redirect}"); + } + } + + #[test] + fn connector_domains_are_the_compiled_in_allow_list_vendors() { + // Every vetted vendor is curated, and nothing is curated that cannot + // obtain a vetted redirect — so a new allow-list entry fails this test + // until someone decides its branding. + let curated: BTreeSet<&str> = + CONNECTORS.iter().flat_map(|c| c.domains.iter().copied()).collect(); + assert_eq!(curated, crate::auth::default_redirect_domains()); + } + #[test] fn slug_lookup_is_closed_to_the_curated_set() { assert!(connector_by_slug("claude").is_some()); assert!(connector_by_slug("unknown").is_none()); assert!(connector_by_slug("../secrets").is_none()); assert!(connector_by_slug("").is_none()); - // Every connector has a non-empty slug and name. for c in CONNECTORS { assert!(!c.slug.is_empty() && !c.name.is_empty()); } diff --git a/src/lib.rs b/src/lib.rs index e33f0b5..d2e0738 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -331,12 +331,13 @@ impl McpServer { .with_state(self.store.clone()) .layer(permissive_cors()); - // Verified-connector branding (see [`branding`]), rooted at the issuer - // (`{public_url}{mcp_path}/branding/{slug}`) so II reaches it same-origin - // with the #4091-validated callback. CORS-open, like the other endpoints - // II fetches cross-origin. + // Verified-connector branding (see [`branding`]), rooted at the issuer so + // II reaches it same-origin with the #4091-validated callback: the + // session-bound metadata (`/branding?state=…`, answered from that pending + // connect's validated redirect) and the static per-connector logo. + // CORS-open, like the other endpoints II fetches cross-origin. let branding = Router::new() - .route("/branding/{slug}", get(branding::branding_metadata)) + .route("/branding", get(branding::branding_metadata)) .route("/branding/{slug}/logo", get(branding::branding_logo)) .with_state(self.store.clone()) .layer(permissive_cors()); diff --git a/tests/routers.rs b/tests/routers.rs index 1023746..e5cb549 100644 --- a/tests/routers.rs +++ b/tests/routers.rs @@ -215,6 +215,11 @@ async fn unauthenticated_mcp_requests_get_the_path_aware_challenge() { /// registration lands in the state directory's `oauth-clients.json` so it /// survives a restart, and a redirect off the hosted allow-list is refused /// before anything is stored. +/// +/// It also drives verified-connector branding end to end over HTTP (authorize → +/// the `state` in II's link → `GET /branding?state=`), because this is the one +/// test that registers a client: a second accepting registration elsewhere would +/// race the persistence poll below on the shared store file. #[tokio::test] async fn dynamic_client_registration_round_trips_and_persists() { // One app, cloned per request, so both calls share the same client store. @@ -237,28 +242,86 @@ async fn dynamic_client_registration_round_trips_and_persists() { } }; - let (status, doc) = register(r#"{"redirect_uris":["http://127.0.0.1:4321/cb"]}"#).await; + // Loopback first (asserted below); the two claude.ai spellings — plain and + // with a trailing root dot, which validation accepts — are for branding. + let (status, doc) = register( + r#"{"redirect_uris":["http://127.0.0.1:4321/cb", + "https://claude.ai/api/mcp/auth_callback", + "https://claude.ai./api/mcp/auth_callback"]}"#, + ) + .await; assert_eq!(status, StatusCode::CREATED); let client_id = doc["client_id"].as_str().expect("a client_id").to_string(); assert_eq!(doc["redirect_uris"][0], "http://127.0.0.1:4321/cb"); // The fresh registration is usable: authorize accepts it and hands the - // browser to Internet Identity (302 + the binding cookie). - let resp = app - .clone() - .oneshot( - Request::get(format!( - "/mcp/oauth/authorize?response_type=code&client_id={client_id}\ - &redirect_uri=http://127.0.0.1:4321/cb&code_challenge=abc\ - &code_challenge_method=S256" - )) - .body(Body::empty()) - .unwrap(), - ) - .await - .unwrap(); - assert_eq!(resp.status(), StatusCode::FOUND, "a registered client can start a sign-in"); - assert!(resp.headers().contains_key("set-cookie"), "the binding cookie must be set"); + // browser to Internet Identity (302 + the binding cookie). Returns the + // connect `state` (the pending session id) from the II link's fragment — + // what II passes back to `/branding`. + let authorize = |redirect: &'static str| { + let app = app.clone(); + let client_id = client_id.clone(); + async move { + let resp = app + .oneshot( + Request::get(format!( + "/mcp/oauth/authorize?response_type=code&client_id={client_id}\ + &redirect_uri={redirect}&code_challenge=abc\ + &code_challenge_method=S256" + )) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::FOUND, "a registered client can start a sign-in"); + assert!(resp.headers().contains_key("set-cookie"), "the binding cookie must be set"); + let location = resp.headers()["location"].to_str().unwrap().to_string(); + let (_, fragment) = location.split_once('#').expect("the II link carries a fragment"); + url::form_urlencoded::parse(fragment.as_bytes()) + .find(|(key, _)| key == "state") + .map(|(_, state)| state.into_owned()) + .expect("the II link carries the connect state") + } + }; + let branding = |state: String| { + let app = app.clone(); + async move { + let resp = app + .oneshot( + Request::get(format!("/mcp/branding?state={state}")) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + let status = resp.status(); + let cache = resp + .headers() + .get("cache-control") + .and_then(|v| v.to_str().ok()) + .map(str::to_owned); + let bytes = resp.into_body().collect().await.unwrap().to_bytes(); + let doc = serde_json::from_slice(&bytes).unwrap_or(serde_json::Value::Null); + (status, cache, doc) + } + }; + + // Branding is answered per session, from that session's validated redirect: + // a loopback (native-app) session stays anonymous ... + let (status, _, _) = branding(authorize("http://127.0.0.1:4321/cb").await).await; + assert_eq!(status, StatusCode::NOT_FOUND, "a loopback session gets no branding"); + // ... and both claude.ai spellings brand as Claude, uncached. + for redirect in + ["https://claude.ai/api/mcp/auth_callback", "https://claude.ai./api/mcp/auth_callback"] + { + let (status, cache, doc) = branding(authorize(redirect).await).await; + assert_eq!(status, StatusCode::OK, "branding for {redirect}"); + assert_eq!(doc["name"], "Claude", "branding for {redirect}"); + assert_eq!(doc["verified"], true, "branding for {redirect}"); + assert_eq!(doc["logo"], format!("{PUBLIC_URL}/mcp/branding/claude/logo")); + assert_eq!(cache.as_deref(), Some("no-store"), "a per-session answer must not be cached"); + } // A hosted redirect that isn't allow-listed is refused (nothing stored). let (status, doc) = register(r#"{"redirect_uris":["https://attacker.example/cb"]}"#).await; @@ -452,25 +515,37 @@ async fn oauth_endpoints_live_under_each_mount() { } } -/// Verified-connector branding endpoints (II fetches these from the issuer origin -/// to brand the consent screen): a curated slug yields name + logo + verified; a -/// slug outside the curated set 404s so the path can't be used to probe. +/// Verified-connector branding routing (II fetches these from the issuer origin to +/// brand the consent screen). The metadata is SESSION-BOUND — answered only for a +/// pending connect's validated redirect (the 200 path over HTTP is driven by +/// `dynamic_client_registration_round_trips_and_persists`, and expiry by +/// `branding_is_bound_to_the_session` in `src/auth.rs`); here: no session → 404, +/// CORS-open for II's cross-origin fetch, the unbound per-slug metadata path is +/// gone, and the static logo serves. #[tokio::test] -async fn branding_endpoints_serve_vetted_connectors_and_404_others() { - // Metadata for a vetted connector: the curated name, the absolute logo URL - // under the instance issuer, `verified`, and `no-store`. - let resp = - app().oneshot(Request::get("/mcp/branding/claude").body(Body::empty()).unwrap()).await.unwrap(); - assert_eq!(resp.status(), StatusCode::OK); - assert_eq!( - resp.headers().get("cache-control").and_then(|v| v.to_str().ok()), - Some("no-store") - ); - let bytes = resp.into_body().collect().await.unwrap().to_bytes(); - let doc: serde_json::Value = serde_json::from_slice(&bytes).unwrap(); - assert_eq!(doc["name"], "Claude"); - assert_eq!(doc["verified"], true); - assert_eq!(doc["logo"], format!("{PUBLIC_URL}/mcp/branding/claude/logo")); +async fn branding_endpoints_are_session_bound_and_serve_logos() { + // No such session, and no `state` at all: the same 404. + for path in ["/mcp/branding?state=sess-does-not-exist", "/mcp/branding"] { + let resp = app() + .oneshot( + Request::get(path).header("origin", "https://id.ai").body(Body::empty()).unwrap(), + ) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::NOT_FOUND, "GET {path}"); + assert!( + resp.headers().contains_key("access-control-allow-origin"), + "GET {path}: II fetches this cross-origin, so it must be CORS-open" + ); + } + + // The old UNBOUND per-slug metadata endpoint (it returned `verified: true` for + // any catalog slug) no longer exists: it must not answer with branding. + let resp = app() + .oneshot(Request::get("/mcp/branding/claude").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_ne!(resp.status(), StatusCode::OK, "no slug-keyed branding metadata"); // The logo is an SVG with nosniff (II renders it via , never inlined). let resp = app() @@ -487,9 +562,10 @@ async fn branding_endpoints_serve_vetted_connectors_and_404_others() { Some("nosniff") ); - // A slug outside the curated set 404s on both endpoints. - for path in ["/mcp/branding/not-a-connector", "/mcp/branding/not-a-connector/logo"] { - let resp = app().oneshot(Request::get(path).body(Body::empty()).unwrap()).await.unwrap(); - assert_eq!(resp.status(), StatusCode::NOT_FOUND, "GET {path}"); - } + // A slug outside the curated set 404s, so the path can't probe. + let resp = app() + .oneshot(Request::get("/mcp/branding/not-a-connector/logo").body(Body::empty()).unwrap()) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::NOT_FOUND); } From 3a760c9bbc359ffc85c9909902e2835851d847a7 Mon Sep 17 00:00:00 2001 From: Arshavir Ter-Gabrielyan Date: Thu, 24 Sep 2026 14:09:50 +0200 Subject: [PATCH 3/6] Branding docs: spell out the validation invariant Co-Authored-By: Claude Opus 5.5 --- src/branding.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/branding.rs b/src/branding.rs index aff3ba4..2893f0b 100644 --- a/src/branding.rs +++ b/src/branding.rs @@ -25,8 +25,9 @@ //! browser; a user agent the attacker controls, such as an embedded webview, is //! out of scope for branding as it is for the rest of the flow.) Host matching //! reuses validation's own rule ([`crate::auth::host_key`], -//! [`crate::auth::host_is_or_under`]), and a redirect validation would refuse -//! never resolves, so branding and validation cannot disagree about a vendor. +//! [`crate::auth::host_is_or_under`]), and a redirect that validation would +//! refuse never resolves to a connector, so branding and validation cannot +//! disagree about a vendor. //! //! Endpoints, both issuer-rooted (same origin as the #4091-validated callback): //! `GET /branding?state=…` (session-bound metadata: name, logo URL, `verified`) @@ -245,7 +246,7 @@ mod tests { } #[test] - fn a_redirect_validation_refuses_never_resolves() { + fn a_redirect_that_validation_refuses_never_resolves() { // A vetted host, but a path off its pin, a port, userinfo, a query, or // percent-encoding: validation refuses each, so branding must too. for redirect in [ From dfde9eaec1bf8302b5c3d289432892de1f9ec398 Mon Sep 17 00:00:00 2001 From: Arshavir Ter-Gabrielyan Date: Thu, 24 Sep 2026 15:13:35 +0200 Subject: [PATCH 4/6] Document the branding routes on mcp_router Co-Authored-By: Claude Opus 5.5 --- src/lib.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index d2e0738..fd10831 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -237,9 +237,14 @@ impl McpServer { /// * `/.well-known/oauth-authorization-server` — the OIDC-style /// alternate location of the AS metadata (some clients derive /// `/.well-known/…` instead of RFC 8414 path insertion); - /// * everything else — the MCP streamable-HTTP endpoint (the router + /// * `/branding?state=…` and `/branding/{slug}/logo` — verified-connector + /// branding for Internet Identity's consent screen, CORS-open: the + /// first answers for one pending connect (from its validated + /// `redirect_uri`, `no-store`, `404` otherwise), the second serves a + /// curated connector's static logo; + /// * every other path — the MCP streamable-HTTP endpoint (the router /// fallback, so the bare mount path, its trailing-slash form, and - /// sub-paths all reach it), bearer-token gated, with the CORS + /// unmatched sub-paths all reach it), bearer-token gated, with the CORS /// preflight answered before authentication and `WWW-Authenticate` /// exposed cross-origin. /// From 36522d5bc2517b9d3face11cce175a7fad13bec9 Mon Sep 17 00:00:00 2001 From: Arshavir Ter-Gabrielyan Date: Thu, 24 Sep 2026 16:53:21 +0200 Subject: [PATCH 5/6] Brand only compiled-in callbacks; harden the branding responses connector_for_redirect accepted anything the effective allow-list admits, so an OAUTH_ALLOWED_REDIRECT_PREFIXES entry on a vendor's domain (another path on claude.ai, say) was shown as that vendor, contrary to the docs. Branding now requires a redirect admitted by a compiled-in DEFAULT_ALLOWED_REDIRECTS entry, under validation's own rules (redirect_uri_on_default_allow_list, sharing hosted_redirect_admitted with redirect_uri_permitted), so a vendor's name only ever rests on the reviewed callback paths in this repo. The branding 404 is now no-store like the 200 it stands in for, and the logo carries a sandboxing CSP: opened top-level it is an SVG document on the issuer origin, where isolation does not apply. A test keeps every bundled logo free of script, event handlers, embedded HTML, and external references, for when licensed vendor marks replace the placeholder. Co-Authored-By: Claude Opus 5.5 --- src/auth.rs | 65 +++++++++++++++++++++++++++++- src/branding.rs | 103 ++++++++++++++++++++++++++++++++++++----------- tests/routers.rs | 8 ++++ 3 files changed, 151 insertions(+), 25 deletions(-) diff --git a/src/auth.rs b/src/auth.rs index 57ec0a5..4e553f0 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -652,6 +652,40 @@ pub(crate) fn redirect_uri_permitted(redirect_uri: &str) -> bool { if is_loopback_url(&url) { return true; } + hosted_redirect_admitted( + &url, + allowed_redirects() + .iter() + .map(|(domain, path, pin)| (domain.as_str(), path.as_str(), *pin)), + ) +} + +/// Whether `redirect_uri` is a hosted redirect admitted by a **compiled-in** +/// [`DEFAULT_ALLOWED_REDIRECTS`] entry, under the same rules as +/// [`redirect_uri_permitted`] (no query or fragment, canonical `https` shape, no +/// percent-encoding, pinned path). Operator entries from +/// `OAUTH_ALLOWED_REDIRECT_PREFIXES` do not count, and neither does loopback. +/// Connector branding ([`crate::branding`]) rests on this, so a vendor's name is +/// only ever shown for the reviewed callback paths in this repo, never for a path +/// a deployment added on the vendor's domain. +pub(crate) fn redirect_uri_on_default_allow_list(redirect_uri: &str) -> bool { + let Ok(url) = url::Url::parse(redirect_uri) else { + return false; + }; + if url.query().is_some() || url.fragment().is_some() { + return false; + } + hosted_redirect_admitted(&url, DEFAULT_ALLOWED_REDIRECTS.iter().copied()) +} + +/// The hosted half of [`redirect_uri_permitted`], over an explicit set of +/// `(domain, path, pin)` entries: `url` (already free of query and fragment) must +/// have the canonical `https://` shape, a path with no +/// percent-encoding, and a host and path that some entry admits. +fn hosted_redirect_admitted<'a>( + url: &url::Url, + entries: impl IntoIterator, +) -> bool { let Some(host) = url.host_str() else { return false; }; @@ -665,7 +699,7 @@ pub(crate) fn redirect_uri_permitted(redirect_uri: &str) -> bool { let Ok(canonical) = url::Url::parse(&format!("https://{host}{}", url.path())) else { return false; }; - if url != canonical { + if url != &canonical { return false; } let host = host_key(host); @@ -687,7 +721,7 @@ pub(crate) fn redirect_uri_permitted(redirect_uri: &str) -> bool { // or descendants too). The path pin is what keeps a registration off // third-party/user-content paths (e.g. `/page/…`, `/g/…`) on the same origin; // without it, domain-only matching would let those capture the code. - allowed_redirects().iter().any(|(domain, prefix, pin)| { + entries.into_iter().any(|(domain, prefix, pin)| { host_is_or_under(&host, domain) && match pin { PathPin::Exact => path == prefix, @@ -3243,6 +3277,33 @@ mod tests { /// pinned callback path; loopback always passes; everything else (a /// user-content path on an allow-listed origin, a wrong path, an unlisted /// domain, or an authority-trick look-alike) is refused. + // Branding's vetting ignores operator entries: a path a deployment adds on a + // vendor's domain is a valid redirect but never earns that vendor's name. + #[test] + fn default_allow_list_vetting_ignores_operator_entries() { + use super::{ + hosted_redirect_admitted, redirect_uri_on_default_allow_list, PathPin, + DEFAULT_ALLOWED_REDIRECTS, + }; + let ops_added = "https://claude.ai/ops/added/cb"; + let with_operator_entry = DEFAULT_ALLOWED_REDIRECTS.iter().copied().chain([( + "claude.ai", + "/ops/added", + PathPin::Prefix, + )]); + let url = url::Url::parse(ops_added).unwrap(); + assert!( + hosted_redirect_admitted(&url, with_operator_entry), + "the operator entry admits it" + ); + assert!(!redirect_uri_on_default_allow_list(ops_added), "but it is not a vetted callback"); + // The compiled-in callbacks are vetted, with validation's own rules. + assert!(redirect_uri_on_default_allow_list("https://claude.ai/api/mcp/auth_callback")); + assert!(redirect_uri_on_default_allow_list("https://claude.ai./api/mcp/auth_callback")); + assert!(!redirect_uri_on_default_allow_list("https://claude.ai/api/mcp/auth_callback?x=1")); + assert!(!redirect_uri_on_default_allow_list("http://127.0.0.1:5173/cb")); + } + #[test] fn hosted_redirect_allow_list() { // Allow-listed vendor domains/subdomains UNDER their pinned callback path. diff --git a/src/branding.rs b/src/branding.rs index 2893f0b..1fb4ceb 100644 --- a/src/branding.rs +++ b/src/branding.rs @@ -23,11 +23,13 @@ //! the connect is that vendor, and the redirect vendor is a sound, //! server-curated proxy for product identity. (That holds for a conforming //! browser; a user agent the attacker controls, such as an embedded webview, is -//! out of scope for branding as it is for the rest of the flow.) Host matching +//! out of scope for branding as it is for the rest of the flow.) Only a redirect +//! admitted by a **compiled-in** allow-list entry resolves +//! ([`crate::auth::redirect_uri_on_default_allow_list`]), so a vendor's name is +//! never shown for a path an operator added on the vendor's domain. Host matching //! reuses validation's own rule ([`crate::auth::host_key`], -//! [`crate::auth::host_is_or_under`]), and a redirect that validation would -//! refuse never resolves to a connector, so branding and validation cannot -//! disagree about a vendor. +//! [`crate::auth::host_is_or_under`]), so branding and validation cannot disagree +//! about a vendor. //! //! Endpoints, both issuer-rooted (same origin as the #4091-validated callback): //! `GET /branding?state=…` (session-bound metadata: name, logo URL, `verified`) @@ -46,7 +48,7 @@ use axum::{ use serde::Deserialize; use serde_json::json; -use crate::auth::{host_is_or_under, host_key, redirect_uri_permitted, AuthStore}; +use crate::auth::{host_is_or_under, host_key, redirect_uri_on_default_allow_list, AuthStore}; /// A vetted connector's server-curated branding. pub(crate) struct Connector { @@ -69,14 +71,16 @@ pub(crate) struct Connector { /// logo, which would be a trademark/licensing matter this repo should not decide. /// It is the placeholder for every connector until each vendor's own licensed /// mark is dropped into its [`Connector::logo`]. II renders it via a fixed-size -/// `` (an ``-loaded SVG cannot execute script), so it needs no -/// server-side sanitising. +/// `` (an ``-loaded SVG cannot execute script), but the logo URL can +/// also be opened top-level on the issuer origin, so every bundled logo must be +/// static and script-free (a test checks) and is served under a sandboxing CSP. pub(crate) const PLACEHOLDER_LOGO_SVG: &str = r##""##; /// The vetted connectors and their curated branding. The domains are exactly the /// vendor domains of the compiled-in `DEFAULT_ALLOWED_REDIRECTS` (a test holds -/// the two sets equal); a host ops adds via `OAUTH_ALLOWED_REDIRECT_PREFIXES` is -/// accepted for redirects but stays anonymous until it is curated here. v1 +/// the two sets equal). An `OAUTH_ALLOWED_REDIRECT_PREFIXES` entry is accepted for +/// redirects but never branded, even on a vendor's domain, because resolution +/// requires a compiled-in entry ([`connector_for_redirect`]). v1 /// covers self-authenticating WEB connectors only; native/loopback apps stay /// anonymous (their only identity signal is a spoofable `client_name`). Logos /// are placeholders — see [`PLACEHOLDER_LOGO_SVG`]. @@ -114,20 +118,19 @@ pub(crate) const CONNECTORS: &[Connector] = &[ }, ]; -/// The vetted connector a `redirect_uri` belongs to, if any. The redirect must -/// pass validation itself ([`redirect_uri_permitted`]: allow-listed path, no -/// port, userinfo, query, or percent-encoding) and be `https`, so this never -/// depends on its caller having validated; the host then matches exactly as -/// validation does — trailing root dots trimmed, lowercased, dot-boundary -/// subdomains. `None` for loopback or an unlisted host, which read as anonymous. +/// The vetted connector a `redirect_uri` belongs to, if any. The redirect must be +/// admitted by a compiled-in allow-list entry under validation's own rules +/// ([`redirect_uri_on_default_allow_list`]: canonical `https`, pinned path, no +/// port, userinfo, query, fragment, or percent-encoding), so this never depends +/// on its caller having validated, and an operator-added entry never brands. The +/// host then matches exactly as validation does — trailing root dots trimmed, +/// lowercased, dot-boundary subdomains. `None` for loopback, an unlisted host, or +/// an operator entry, which read as anonymous. pub(crate) fn connector_for_redirect(redirect_uri: &str) -> Option<&'static Connector> { - if !redirect_uri_permitted(redirect_uri) { + if !redirect_uri_on_default_allow_list(redirect_uri) { return None; } let url = url::Url::parse(redirect_uri).ok()?; - if url.scheme() != "https" { - return None; - } let host = host_key(url.host_str()?); CONNECTORS.iter().find(|c| c.domains.iter().any(|domain| host_is_or_under(&host, domain))) } @@ -151,7 +154,8 @@ pub(crate) struct BrandingQuery { /// unknown, or expired `state` and for a session whose redirect is not a vetted /// connector (loopback / unlisted), so the response is no oracle for which /// sessions exist; a malformed query (e.g. a repeated `state`) gets that same -/// 404 rather than axum's 400. `no-store`: the answer is per-session. +/// 404 rather than axum's 400. Every response is `no-store`: the answer is +/// per-session. pub(crate) async fn branding_metadata( State(store): State, query: Result, QueryRejection>, @@ -177,7 +181,9 @@ pub(crate) async fn branding_metadata( /// per-connector asset: it makes no claim about any session (that is /// [`branding_metadata`]'s job, whose `logo` URL points here). II MUST render it /// via a fixed-size ``, never inline the SVG into the consent DOM: an -/// ``-loaded SVG cannot execute script, an inlined one can. +/// ``-loaded SVG cannot execute script, an inlined one can. Opened top-level +/// it would render as a document on the issuer origin, so it also carries a +/// sandboxing CSP ([`LOGO_CSP`]); an `` ignores that header. pub(crate) async fn branding_logo(Path(slug): Path) -> Response { let Some(connector) = connector_by_slug(&slug) else { return branding_not_found(); @@ -187,12 +193,21 @@ pub(crate) async fn branding_logo(Path(slug): Path) -> Response { headers.insert(header::CONTENT_TYPE, HeaderValue::from_static("image/svg+xml")); headers.insert(header::X_CONTENT_TYPE_OPTIONS, HeaderValue::from_static("nosniff")); headers.insert(header::CACHE_CONTROL, HeaderValue::from_static("public, max-age=3600")); + headers.insert(header::CONTENT_SECURITY_POLICY, HeaderValue::from_static(LOGO_CSP)); resp } -/// No branding for this request: 404, the same for every reason. +/// The logo's `Content-Security-Policy`: no script, no subresources, a sandboxed +/// document — inert even when the SVG is opened top-level on the issuer origin. +/// Inline styles stay allowed, since SVG presentation may use them. +const LOGO_CSP: &str = "default-src 'none'; style-src 'unsafe-inline'; sandbox"; + +/// No branding for this request: 404, the same for every reason, and `no-store` +/// like the answer it stands in for. fn branding_not_found() -> Response { - (StatusCode::NOT_FOUND, "no connector branding").into_response() + let mut resp = (StatusCode::NOT_FOUND, "no connector branding").into_response(); + resp.headers_mut().insert(header::CACHE_CONTROL, HeaderValue::from_static("no-store")); + resp } #[cfg(test)] @@ -270,6 +285,48 @@ mod tests { assert_eq!(curated, crate::auth::default_redirect_domains()); } + #[test] + fn bundled_logos_are_static_svg() { + // A logo URL can be opened top-level on the issuer origin, where an SVG is a + // document: every bundled mark must be inert on its own. + // Internal references (`href="#id"`, `url(#gradient)`) are fine; script, + // event handlers, embedded HTML, and anything external are not. + for c in CONNECTORS { + // Namespace declarations are names, not references: drop the standard ones. + let svg = c + .logo + .to_ascii_lowercase() + .replace(char::is_whitespace, " ") + .replace(r#"xmlns="http://www.w3.org/2000/svg""#, "") + .replace(r#"xmlns:xlink="http://www.w3.org/1999/xlink""#, ""); + assert!(svg.starts_with(" Date: Thu, 24 Sep 2026 17:03:44 +0200 Subject: [PATCH 6/6] Tighten the bundled-logo guard to internal references only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The logo test banned a handful of literal substrings, so a spaced `href = "https://…"` or a quoted `url('https://…')` got through. It now strips whitespace first and requires every href, src, and url() target to be an internal `#…` reference, and also rejects XHTML, animation, and DTDs. It checks itself against a set of unsafe SVGs, spaced and quoted spellings included, so the guard can't quietly weaken when licensed vendor marks replace the placeholder. The module doc now says what branding vouches for: where the authorization code is delivered, not who started the connect. Co-Authored-By: Claude Opus 5.5 --- src/branding.rs | 114 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 76 insertions(+), 38 deletions(-) diff --git a/src/branding.rs b/src/branding.rs index 1fb4ceb..5779d82 100644 --- a/src/branding.rs +++ b/src/branding.rs @@ -1,7 +1,9 @@ //! Verified-connector branding: surface a vetted MCP client's product name and //! logo to Internet Identity's consent screen, so the user sees WHICH vetted -//! product is requesting the connect rather than granting their II accounts to an -//! anonymous "some bridge". +//! product will receive the access a connect grants, rather than granting their +//! II accounts on behalf of an unnamed client. It names where the authorization +//! code is delivered — not who started the connect, nor which account at that +//! product ends up holding the grant. //! //! **The server is the only source of branding, and it answers per session.** II //! asks `GET {issuer}/branding?state={state}` — the `state` it already holds from @@ -285,45 +287,81 @@ mod tests { assert_eq!(curated, crate::auth::default_redirect_domains()); } + /// What makes `svg` unfit to bundle as a logo, if anything. A logo URL can be + /// opened top-level on the issuer origin, where an SVG is a document, so every + /// bundled mark must be inert on its own: no script, event handlers, embedded + /// HTML, animation, or DTD, and no reference that leaves the document — every + /// `href`, `src`, and `url()` must be an internal `#…` reference. A guard for + /// hand-reviewed assets, not a general SVG sanitizer. + fn logo_problems(svg: &str) -> Vec { + let lower = svg.to_ascii_lowercase(); + let mut problems = Vec::new(); + if !lower.trim_start().starts_with(" 0 && rest[name..].trim_start().starts_with('=') + }); + if handler { + problems.push("event-handler attribute".to_string()); + } + problems + } + #[test] fn bundled_logos_are_static_svg() { - // A logo URL can be opened top-level on the issuer origin, where an SVG is a - // document: every bundled mark must be inert on its own. - // Internal references (`href="#id"`, `url(#gradient)`) are fine; script, - // event handlers, embedded HTML, and anything external are not. for c in CONNECTORS { - // Namespace declarations are names, not references: drop the standard ones. - let svg = c - .logo - .to_ascii_lowercase() - .replace(char::is_whitespace, " ") - .replace(r#"xmlns="http://www.w3.org/2000/svg""#, "") - .replace(r#"xmlns:xlink="http://www.w3.org/1999/xlink""#, ""); - assert!(svg.starts_with("::new(), "{}: bundled logo", c.slug); + } + // Internal references are fine. + let internal = r##""##; + assert!(logo_problems(internal).is_empty()); + // The guard catches each unsafe form, including spaced and quoted spellings. + for bad in [ + r#""#, + r#""#, + r#""#, + r#""#, + r#""#, + r#""#, + r#""#, + r#""#, + r#"
"#, + r#""#, + r#""#, + r#""#, + r#"]>&e;"#, + ] { + assert!(!logo_problems(bad).is_empty(), "the guard missed: {bad}"); } }