diff --git a/README.md b/README.md index ea7e326..381c3f5 100644 --- a/README.md +++ b/README.md @@ -355,7 +355,10 @@ Endpoints: - `GET /.well-known/oauth-protected-resource` — points clients at the AS - `POST /oauth/register` — dynamic client registration (RFC 7591); `redirect_uris` are stored and persisted to `OAUTH_CLIENTS_FILE`; requested `grant_types` are - honoured (intersected with `authorization_code`) + honoured (intersected with `authorization_code`). A **hosted** `redirect_uri` is + rejected unless its host is on the allow-list (see the Companion-control note + below); loopback redirects are always accepted. + - `GET /oauth/authorize` — validates the client + redirect, requires PKCE, sets the binding cookie, then redirects to II's handshake (with `registration_key`) - `GET /oauth/connect/callback` — the **pinned callback page**: II navigates here @@ -408,12 +411,21 @@ incl. loopback** (a loopback redirect resolves on the consenter's own machine). `SameSite=Lax` still rides the top-level cross-site GET II uses to navigate back to the callback page. -> **Companion control (not in this change).** The *same-browser* variant — a victim -> socially engineered into running the whole flow toward an attacker-registered -> **hosted** `redirect_uri` — is not closed by this (the victim's browser -> legitimately holds both proofs). It needs **hosted-redirect allow-listing**; -> loopback/native clients are safe either way (the code resolves on the consenter's -> own machine). +> **Companion control: the hosted-redirect allow-list.** The *same-browser* variant +> (a victim socially engineered into running the whole flow toward an +> attacker-registered **hosted** `redirect_uri`) is not closed by Consent-Bound +> Completion alone, since the victim's browser legitimately holds both proofs. It +> **is** closed by a **hosted-redirect allow-list**: dynamic client registration +> accepts only a loopback redirect, or a hosted redirect whose host is (a subdomain +> of) an allow-listed registrable domain, so an attacker cannot register a hosted +> destination it controls. The list is seeded with the known MCP connector vendors +> and widened per deployment with `OAUTH_ALLOWED_REDIRECT_DOMAINS` (additive); +> loopback/native clients are exempt (the code resolves on the consenter's own +> machine). Enforced at both `/oauth/register` and `/oauth/authorize`. A client +> turned away is pointed at a contact address to request approval: +> `/oauth/register` says so in its JSON `error_description`, and a browser that +> reaches `/oauth/authorize` gets an on-brand "not allowed" page (not a raw error) +> naming the same contact. ### The registration-delegation connect handshake @@ -700,8 +712,8 @@ delegation. Omitting `account` uses the default account. handshake binds the connection's session key (a fragment-delivered, canister-signed delegation redeemed via `mcp_register_v2`), with **Consent-Bound Completion** binding `/oauth/connect/redeem` to both the initiator (`sid` cookie) and the consenter (the - fragment delegation); expiring tokens. (The RFC 8628 device grant was dropped. - Same-browser-variant closure needs hosted-redirect allow-listing — see Auth.) + fragment delegation); expiring tokens. The same-browser phishing variant is closed + by a hosted-redirect allow-list (see Auth). (The RFC 8628 device grant was dropped.) - [x] On-demand **domain identities**: the registered session key mints per-app account delegations directly via II canister methods (`call_canister`/`get_app_principal` `derivation_origin`); no per-app browser flow. diff --git a/src/assets/connect-error.html b/src/assets/connect-error.html new file mode 100644 index 0000000..3b1dd78 --- /dev/null +++ b/src/assets/connect-error.html @@ -0,0 +1,13 @@ + + +MCP client not allowed + +
+
+

This MCP client isn't allowed

+

This server only accepts connections from approved MCP clients, and the one you're connecting from is not on the allow-list.

+

To request access, email __CONTACT__ and include your MCP client's redirect URI.

+
+
+ diff --git a/src/assets/connect.css b/src/assets/connect.css index ac158d5..3b31319 100644 --- a/src/assets/connect.css +++ b/src/assets/connect.css @@ -82,7 +82,10 @@ body { color: var(--ink); } .detail { margin: 0; max-width: 32ch; font-size: 1rem; color: var(--secondary); } -.hint { margin: 0; font-size: 0.9rem; color: var(--muted); } +.hint { margin: 0; max-width: 34ch; font-size: 0.9rem; color: var(--muted); } +/* Links appear only on the error page (the callback page has none): keep them on + the Ember accent so they read as links in both themes without a UA blue. */ +.detail a, .hint a { color: var(--accent); } /* Foot-of-page attribution: an eyebrow label + the DFINITY mark in a soft pill. */ .hosted { display: flex; diff --git a/src/auth.rs b/src/auth.rs index f68f743..3319069 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -46,11 +46,16 @@ //! In the confused-deputy path the delegation lands in the honest page in the //! VICTIM's browser, whose cookie does not match the one `X` was bound to, so the //! redeem aborts. This closes the split-browser injection for all transports incl. -//! loopback (a loopback redirect resolves on the consenter's own machine). Not -//! closed here (companion control): the *same-browser* variant where a victim is -//! socially engineered into running the whole flow toward an attacker-registered -//! **hosted** `redirect_uri`; that needs hosted-redirect allow-listing (loopback is -//! safe either way). +//! loopback (a loopback redirect resolves on the consenter's own machine). The +//! *same-browser* variant (a victim socially engineered into running the whole +//! flow toward an attacker-registered **hosted** `redirect_uri`) is closed by a +//! **hosted-redirect allow-list** ([`redirect_uri_permitted`]): open DCR may +//! register only loopback or allow-listed-domain hosted redirects, so an attacker +//! cannot register a hosted destination it controls (loopback is safe either way). +//! A client turned away by the allow-list is pointed at [`ALLOWLIST_CONTACT`] to +//! request approval: `/oauth/register` says so in its JSON `error_description`, +//! and a browser that reaches `/oauth/authorize` gets the on-brand +//! [`not_allowlisted_page`] instead of a raw error. //! //! The wire shapes match the merged II contract (verified against the beta II //! canister's live `.did`, `fgte5-ciaaa-aaaad-aaatq-cai`): the connect link @@ -157,15 +162,98 @@ fn persist_clients(clients: &HashMap) { } } +/// Registrable domains whose hosts (and subdomains) may register a **hosted** +/// (non-loopback) OAuth `redirect_uri`. Dynamic client registration is open, so +/// without this an attacker could register a client with a hosted redirect it +/// controls and phish an authorization code to it (the same-browser variant that +/// Consent-Bound Completion does not close). Loopback redirects are exempt (a +/// loopback code resolves on the consenter's own machine), so native/CLI clients +/// need no entry. Seeded with the MCP connector vendors seen in practice; widen +/// at deploy time with `OAUTH_ALLOWED_REDIRECT_DOMAINS` (comma/space-separated, +/// additive) without a rebuild. +const DEFAULT_ALLOWED_REDIRECT_DOMAINS: &[&str] = &[ + "antigravity.google", // Google Antigravity + "chatgpt.com", // OpenAI ChatGPT connectors + "claude.ai", // Anthropic Claude + "cursor.com", // Cursor + "grok.com", // xAI Grok + "perplexity.ai", // Perplexity (any subdomain) + "perplexity.com", // Perplexity (any subdomain) + "vscode.dev", // Visual Studio Code (any subdomain, e.g. insiders.) +]; + +/// The effective hosted-redirect allow-list: the compiled-in defaults plus any +/// entries in `OAUTH_ALLOWED_REDIRECT_DOMAINS`. Each env entry may be a bare +/// domain or an `https://…` origin (scheme/port/path are stripped to the host). +/// Additive: the shipped binary is safe by default and ops can only widen the +/// set, never accidentally drop a known vendor. +fn allowed_redirect_domains() -> Vec { + let mut domains: Vec = + DEFAULT_ALLOWED_REDIRECT_DOMAINS.iter().map(|d| d.to_string()).collect(); + if let Ok(extra) = std::env::var("OAUTH_ALLOWED_REDIRECT_DOMAINS") { + for raw in extra.split([',', ' ', '\t', '\n']).map(str::trim).filter(|s| !s.is_empty()) { + // Parse as a URL to robustly reduce an entry to its bare host, stripping + // scheme, userinfo, port, and path alike. Bare domains have no scheme, so + // give them one first (`vendor.example` / `vendor.example:8443` -> host + // `vendor.example`), matching what `Url::host_str()` yields at match time. + let candidate = + if raw.contains("://") { raw.to_string() } else { format!("https://{raw}") }; + let host = url::Url::parse(&candidate) + .ok() + .and_then(|u| u.host_str().map(|h| h.trim_end_matches('.').to_ascii_lowercase())) + .filter(|h| !h.is_empty()); + if let Some(host) = host { + domains.push(host); + } + } + } + domains +} + +/// Whether a `redirect_uri` may be registered or receive an authorization code. +/// Loopback (`http://localhost` / `127.0.0.1` / `[::1]`, any port) is always +/// allowed. A hosted redirect must be `https`, carry no userinfo, and its host +/// must equal, or be a subdomain of, an allow-listed registrable domain. The host +/// is read from the PARSED URL, not the raw string, so authority tricks such as +/// `https://claude.ai@evil.com` or `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 already rejects it). +fn redirect_uri_permitted(redirect_uri: &str) -> bool { + if is_loopback_redirect(redirect_uri) { + return true; + } + let Ok(url) = url::Url::parse(redirect_uri) else { + return false; + }; + if url.scheme() != "https" || !url.username().is_empty() || url.password().is_some() { + return false; + } + let Some(host) = url.host_str() else { + return false; + }; + let host = host.trim_end_matches('.').to_ascii_lowercase(); + // host == domain, or host is a dot-boundary subdomain of it. `strip_suffix` + // keeps the same semantics as `ends_with(format!(".{d}"))` without allocating + // a string per candidate on every register/authorize validation. + allowed_redirect_domains() + .iter() + .any(|d| host == *d || host.strip_suffix(d.as_str()).is_some_and(|p| p.ends_with('.'))) +} + /// Acceptance rule for a redirect (OAuth 2.1): the client must be REGISTERED, -/// and the requested redirect must either exactly match a registered URI, or be -/// a loopback URI matching a registered loopback URI on everything but the -/// port. RFC 8252 §7.3 requires the any-port latitude — native clients bind an -/// ephemeral loopback port at runtime, so the exact port can't be registered — -/// but registration itself is still required, so every client that can receive -/// a code is on record (DCR is open, so this is an audit trail, not vetting). +/// the redirect must pass the hosted-redirect allow-list +/// ([`redirect_uri_permitted`]), and it must either exactly match a registered +/// URI or be a loopback URI matching a registered loopback URI on everything but +/// the port. RFC 8252 §7.3 requires the any-port latitude, since native clients +/// bind an ephemeral loopback port at runtime, so the exact port can't be +/// registered. The allow-list is re-checked here (not only at registration) so a +/// pre-existing registration whose domain is no longer allowed can't be used. fn redirect_allowed(reg: Option<&ClientReg>, redirect_uri: &str) -> bool { let Some(reg) = reg else { return false }; + if !redirect_uri_permitted(redirect_uri) { + return false; + } reg.redirect_uris .iter() .any(|u| u == redirect_uri || loopback_match(u, redirect_uri)) @@ -335,6 +423,15 @@ pub async fn authorize(State(store): State, Query(q): Query return oauth_err(StatusCode::BAD_REQUEST, "invalid_request", "response_type=code required"), } if !store.validate_client(&q.client_id, &q.redirect_uri).await { + // A well-formed hosted `redirect_uri` that simply isn't on the allow-list + // is an approval gap, not a malformed request: show the browser a friendly + // page naming the cause and the contact, rather than a raw JSON blob. This + // covers clients that skip DCR or use a stored-but-disallowed registration + // (the standard flow is blocked earlier, at `/oauth/register`). The page is + // static and reflects nothing, so naming the cause leaks nothing. + if !redirect_uri_permitted(&q.redirect_uri) { + return not_allowlisted_page(); + } // `invalid_client` (not `invalid_request`): the request is well-formed, // it's the CLIENT identification that failed — the AS error code the // MCP server guide (and RFC 6749's taxonomy) expects here. No redirect: @@ -675,6 +772,60 @@ fn pinned_callback_page(prefix: &str) -> Response { resp } +/// Where a rejected MCP client is told to request allow-listing. Shown both in +/// the browser-facing `/oauth/authorize` error page ([`not_allowlisted_page`]) +/// and in the `/oauth/register` JSON `error_description`, so a vendor blocked by +/// the hosted-redirect allow-list ([`redirect_uri_permitted`]) has a clear next +/// step rather than an opaque failure. +const ALLOWLIST_CONTACT: &str = "mcp@dfinity.org"; + +/// HTML for the "MCP client not allowed" page, a real `.html` asset (compiled in +/// via `include_str!`, no runtime file I/O). It reuses the pinned callback page's +/// self-contained shell (`assets/connect.css`, the inlined logo) but carries no +/// script. `__NONCE__`/`__CSS__`/`__LOGO__`/`__CONTACT__` are spliced in at render +/// time; none is user-influenced (the contact is a compiled-in constant), so no +/// request value is ever reflected into the markup. +const CONNECT_ERROR_HTML: &str = include_str!("assets/connect-error.html"); + +/// The friendly, browser-facing rejection for a client whose `redirect_uri` is +/// not on the hosted-redirect allow-list ([`redirect_uri_permitted`]). A real +/// browser reaches `/oauth/authorize` by top-level navigation, so a terse JSON +/// `invalid_client` renders as a raw error blob; this serves an on-brand page +/// that names the cause and points the vendor at [`ALLOWLIST_CONTACT`]. Static +/// and non-reflecting: no query or redirect value is interpolated, so it can't be +/// turned into a content-injection or open-redirect surface. `403 Forbidden`: the +/// request is understood but refused by policy. +fn not_allowlisted_page() -> Response { + let nonce = csp_nonce(); + let html = CONNECT_ERROR_HTML + .replace("__NONCE__", &nonce) + .replace("__CSS__", CONNECT_PAGE_CSS) + .replace("__LOGO__", CONNECT_LOGO_SVG) + .replace("__CONTACT__", ALLOWLIST_CONTACT); + // No script on this page, so no `script-src`; the only inline is the nonce'd + // `