From 21a1e514becd8f74403ebc01c8a3d8bf5f0a3271 Mon Sep 17 00:00:00 2001 From: Zezhen Xu Date: Tue, 1 Sep 2026 21:35:30 +0000 Subject: [PATCH] fix(security): name the rejected OAuth endpoint in the banner (#7578) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MCP OAuth rejection message "URL contained credential or exfiltration pattern" named no URL and no host, so a user hitting an unlisted identity provider could not tell which endpoint tripped the scanner or what to write into oauth_endpoints.json. - security.py: new `sanitized_oauth_endpoint(url)` helper returns the lowercase host + path of a rejected authorization URL. Query, fragment, port, and userinfo are never included. Both components are scanned at every percent-decode layer up to the gate's own _MAX_URL_DECODE_PASSES budget, with unquote_plus (plus-delimited form encoding folds to spaces before matching) and against the same pattern families the rejection can fire on (fixed credentials, bare-secret runs, the _EXFIL_PATTERNS set). A credential-bearing or budget-exhausting path self-redacts to the shared tag; a credential-bearing hostname makes the helper return None; a non-ASCII host is surfaced in IDNA A-label (punycode) form. Both components are length-capped; unparseable URLs return None. The boolean `oauth_url_contains_credential` API is unchanged. - chat_runner.py: the rejection banner names the sanitized host+path in its content — which also spells the expected {"additional_authorization_endpoints": [{"host": ..., "path": ...}]} entry shape — and inside the `error` meta field, the field the dashboard's failed banner actually renders. No new meta keys: no shipped surface reads any. - security.py: add Miro's MCP authorization endpoint (mcp.miro.com, /authorize) to _OAUTH_AUTHORIZATION_ENDPOINTS — maintainer-verified via RFC 8414 metadata, matching the reporter's independent read. - docs/system-specs/modules/security.md updated in the same commit. - Tests: sanitization invariants (query/PKCE values never echoed; single-, double-, past-budget-encoded, and plus-delimited credential paths all redact; credential in a DNS label returns None; IDNA A-label surfacing; truncation; None fallbacks), banner content/error/meta assertions, and a Miro corpus entry pinning the endpoint as approved. Scope: issue items 1-2 (+ the one-line item 4 endpoint addition). Item 3 (end-to-end docs) is tracked in #7579; RFC 8414 auto-discovery remains an open design question. Refs #7578 --- docs/system-specs/modules/security.md | 2 +- src/kiro_crew/dashboard/chat_runner.py | 49 ++++-- src/kiro_crew/security.py | 152 ++++++++++++++++++ test/oauth_url_corpus.py | 13 ++ test/test_mcp_oauth_banner.py | 69 +++++++++ test/test_security.py | 203 +++++++++++++++++++++++++ 6 files changed, 476 insertions(+), 12 deletions(-) diff --git a/docs/system-specs/modules/security.md b/docs/system-specs/modules/security.md index 3bf17c461c5..d69833b181f 100644 --- a/docs/system-specs/modules/security.md +++ b/docs/system-specs/modules/security.md @@ -171,7 +171,7 @@ under `(allow default)`, never an edition-resolved or user-writable executable. - **Browsing has no keystone capability gate, deliberately (see [browser.md](browser.md)).** Presence of the `playwright-cli` binary on `PATH` makes the capability available, so there is no flag file to protect: the CLI exposes no capability gating to subset once an approved shell turn runs it. Presence is NOT an approval signal. Every dashboard invocation follows the ordinary shell approval ladder, and only an explicit trusted pattern, session trust, or auto-approve grant may skip the prompt. This prevents an unrelated existing install — or a planted launcher in an agent-writable PATH directory — from manufacturing its own grant. Uninstalling revokes availability. Because browsing is a shell command, it is governed on the `commands` plane and an `mcp`-scope deny does not reach it. -- **Operator OAuth consent-endpoint extension (keystone leaf `oauth_endpoints.json`)** — the security module's OAuth banner-safety contract (`security.oauth_url_contains_credential`, and `_exfil_url_warning` under `allow_oauth_entropy=True`) exempts standard front-channel params (`state`, PKCE, …) from the base64-blob/query-length heuristics only at an exact-match `(host, path)` in the code-owned `_OAUTH_AUTHORIZATION_ENDPOINTS`. `~/.kiro/crew/oauth_endpoints.json` (`{"additional_authorization_endpoints": [{"host", "path"}]}`) is the operator's escape hatch for identity providers outside that launch set (Okta orgs, Auth0, self-hosted OIDC, tenant-scoped Entra paths): `security._load_operator_oauth_endpoints()` unions strictly validated entries with the builtin set at check time (`_approved_oauth_authorization_endpoint`, memoized on the file's stat so a hand-edit takes effect on the next check without a restart). **Enforcement point:** the dashboard's live MCP OAuth banner validates URLs with this same gate — `_emit_mcp_oauth_request` in `chat_runner.py` calls `security.oauth_url_contains_credential` directly — so an operator endpoint entry governs the banner path as well as every other consumer wired to the contract gate. Each entry widens a trust boundary, so the file is on `_CREW_SECRET_LEAVES` (full read+write keystone block on both the tool path and every shell form) — an agent must not be able to author its own exemption — and there is deliberately no dashboard writer; the operator hand-edits it out-of-band. Every read fails soft to the EMPTY set (missing/unreadable/corrupt/non-object file, mirroring `computer_use.enable_state.load_state`), invalid entries are skipped individually with a warning (no wildcards, schemes, ports, userinfo, percent-escapes, IP literals, `..`, whitespace, or backslashes; hosts are lowercase-normalized DNS names with a letter TLD, paths exact and case-sensitive), and the entry list is truncated at 50 before validation so a mangled file cannot amplify. HTTPS-only / no-explicit-port / exact-match stay enforced by the gate logic and are NOT relaxable via the file, and the exemption grants exactly what the builtin set grants — fixed-credential patterns, heavy percent-encoding, userinfo, fragments, backslashes, and unknown-param heuristics remain unconditional. The markerless bare-secret entropy heuristic follows the same exact endpoint/parameter scope instead of scanning entropy-bearing recognized parameter values first; parameter names, unknown parameters, and non-query components remain in its scan target. That exemption is additionally bounded to the shapes the protocol itself can emit (`_oauth_entropy_value_is_protocol_shaped`, judged on EVERY decoded form: it percent-decodes until the text stops changing, bounded by `_MAX_URL_DECODE_PASSES`, and refuses a value still decodable at the bound, so `%252F` cannot launder the standard alphabet past a single decode): base64url emits `-`/`_` and never `+`/`/`, and an S256 `code_challenge` is base64url of a 32-byte digest, so it is exactly 43 characters. A base64-standard-alphabet run — the shape of an AWS secret key — therefore cannot ride `state`, `nonce`, or `code_challenge` into the blanked set. The residual is narrower but real: a markerless 40-character credential that happens to be alphanumeric is indistinguishable from ordinary base64url state entropy and is accepted only at this boundary; general output redactors retain the heuristic. An approval that came from an operator entry (not the builtin set) emits a best-effort `oauth_endpoint_extension_used` SEL event, deduped per process per endpoint. +- **Operator OAuth consent-endpoint extension (keystone leaf `oauth_endpoints.json`)** — the security module's OAuth banner-safety contract (`security.oauth_url_contains_credential`, and `_exfil_url_warning` under `allow_oauth_entropy=True`) exempts standard front-channel params (`state`, PKCE, …) from the base64-blob/query-length heuristics only at an exact-match `(host, path)` in the code-owned `_OAUTH_AUTHORIZATION_ENDPOINTS`. `~/.kiro/crew/oauth_endpoints.json` (`{"additional_authorization_endpoints": [{"host", "path"}]}`) is the operator's escape hatch for identity providers outside that launch set (Okta orgs, Auth0, self-hosted OIDC, tenant-scoped Entra paths): `security._load_operator_oauth_endpoints()` unions strictly validated entries with the builtin set at check time (`_approved_oauth_authorization_endpoint`, memoized on the file's stat so a hand-edit takes effect on the next check without a restart). **Enforcement point:** the dashboard's live MCP OAuth banner validates URLs with this same gate — `_emit_mcp_oauth_request` in `chat_runner.py` calls `security.oauth_url_contains_credential` directly — so an operator endpoint entry governs the banner path as well as every other consumer wired to the contract gate. Each entry widens a trust boundary, so the file is on `_CREW_SECRET_LEAVES` (full read+write keystone block on both the tool path and every shell form) — an agent must not be able to author its own exemption — and there is deliberately no dashboard writer; the operator hand-edits it out-of-band. Every read fails soft to the EMPTY set (missing/unreadable/corrupt/non-object file, mirroring `computer_use.enable_state.load_state`), invalid entries are skipped individually with a warning (no wildcards, schemes, ports, userinfo, percent-escapes, IP literals, `..`, whitespace, or backslashes; hosts are lowercase-normalized DNS names with a letter TLD, paths exact and case-sensitive), and the entry list is truncated at 50 before validation so a mangled file cannot amplify. HTTPS-only / no-explicit-port / exact-match stay enforced by the gate logic and are NOT relaxable via the file, and the exemption grants exactly what the builtin set grants — fixed-credential patterns, heavy percent-encoding, userinfo, fragments, backslashes, and unknown-param heuristics remain unconditional. The markerless bare-secret entropy heuristic follows the same exact endpoint/parameter scope instead of scanning entropy-bearing recognized parameter values first; parameter names, unknown parameters, and non-query components remain in its scan target. That exemption is additionally bounded to the shapes the protocol itself can emit (`_oauth_entropy_value_is_protocol_shaped`, judged on EVERY decoded form: it percent-decodes until the text stops changing, bounded by `_MAX_URL_DECODE_PASSES`, and refuses a value still decodable at the bound, so `%252F` cannot launder the standard alphabet past a single decode): base64url emits `-`/`_` and never `+`/`/`, and an S256 `code_challenge` is base64url of a 32-byte digest, so it is exactly 43 characters. A base64-standard-alphabet run — the shape of an AWS secret key — therefore cannot ride `state`, `nonce`, or `code_challenge` into the blanked set. The residual is narrower but real: a markerless 40-character credential that happens to be alphanumeric is indistinguishable from ordinary base64url state entropy and is accepted only at this boundary; general output redactors retain the heuristic. An approval that came from an operator entry (not the builtin set) emits a best-effort `oauth_endpoint_extension_used` SEL event, deduped per process per endpoint. **Rejections name the endpoint, never the values:** `security.sanitized_oauth_endpoint(url)` returns the lowercase host + path of a rejected authorization URL (query/fragment/port/userinfo are never included; both components are scanned at every percent-decode layer up to the gate's own `_MAX_URL_DECODE_PASSES` budget — a credential-bearing or budget-exhausting path self-redacts to the shared tag, a credential-bearing host makes the helper return `None`, a non-ASCII host is surfaced in IDNA A-label form; both components are length-capped; unparseable URLs return `None`). The banner path (`_emit_mcp_oauth_request`) surfaces that pair in the rejection text — which also spells the `{"additional_authorization_endpoints": [{"host", "path"}]}` entry shape — and inside the `error` meta field the dashboard's failed banner actually renders; no additional meta keys are emitted because no shipped surface reads any. So the user can tell WHICH endpoint tripped the scanner and what to write into `oauth_endpoints.json`, without the rejection ever echoing state/PKCE material (#7578). **Privacy-safe OAuth rejection diagnostics.** `security.diagnose_oauth_url_credential()` returns `None` for an accepted URL or an `OAuthUrlCredentialDiagnostic` for the first rejecting sub-check. The record carries only a stable `rule`, a URL-component category, an optional code-owned standard query-parameter name, and a shape profile: total length plus counts of ASCII uppercase, ASCII lowercase, digits, percent signs, URL punctuation, and all other characters. `oauth_url_contains_credential()` retains its boolean caller contract and logs that same bounded signature when it rejects. The diagnostic path does not change a rule, add a bypass, retry, or retain a URL. The URL and parameter value are never returned, logged, persisted, hashed, sampled, or represented by a prefix/suffix; malformed, credential-shaped, and unrecognized parameter names are omitted rather than echoed. This is sufficient for a controlled mint loop to distinguish standard OAuth entropy false positives (for example, `credential_scan_bare_secret_raw` on `state` versus `exfil_query_length`) without creating a second credential-bearing sink. An entropy-bearing recognized parameter value at an approved endpoint produces no diagnostic on entropy alone, while the same shape in an unknown parameter retains the stable `credential_scan_bare_secret_raw` rejection signature. diff --git a/src/kiro_crew/dashboard/chat_runner.py b/src/kiro_crew/dashboard/chat_runner.py index 0d74757fb34..5393bfb9d8e 100644 --- a/src/kiro_crew/dashboard/chat_runner.py +++ b/src/kiro_crew/dashboard/chat_runner.py @@ -250,6 +250,7 @@ redact_and_truncate, redact_credentials, redact_exfiltration_urls, + sanitized_oauth_endpoint, ) from kiro_crew.sel import sel from kiro_crew.session import SessionClosingError, SpeculativeResumeRefused @@ -1683,21 +1684,47 @@ def _emit_mcp_oauth_request( "ACP: rejecting MCP OAuth URL with credential/exfil pattern for %s", server_name or "(unknown)", ) + # Name the endpoint so case 2 is actionable: without the host+path the + # user cannot know what to write into oauth_endpoints.json. The helper + # returns host and path ONLY (query/PKCE material is never echoed) and + # self-redacts a credential-bearing path, so surfacing it does not + # weaken the rejection. + endpoint = sanitized_oauth_endpoint(oauth_url) + rejected_meta: dict[str, Any] = { + "server_name": safe_name, + "failed": True, + "rejected_url": True, + "error": "URL contained credential or exfiltration pattern", + "remedy": "oauth_endpoints.json", + } + endpoint_detail = "" + if endpoint is not None: + rejected_host, rejected_path = endpoint + # The dashboard's failed-banner renderer (McpOAuthBanner) displays + # meta["error"], not the content string — the endpoint must ride in + # the error field to actually reach the user's screen. The banner + # content below additionally spells the oauth_endpoints.json entry + # shape, so text + error together carry the whole remedy; no extra + # meta keys are emitted because no surface reads them. + rejected_meta["error"] = ( + "URL contained credential or exfiltration pattern " + f"(endpoint: {rejected_host}{rejected_path})" + ) + endpoint_detail = ( + f" The rejected authorization endpoint was " + f"{rejected_host}{rejected_path} (query values withheld)." + ) slot.append( "mcp_oauth", f"🚫 {label} sent an authentication URL containing a credential " - "pattern (rejected). If this is a self-hosted or otherwise " - "unlisted identity provider, its authorization endpoint may need " - "adding to oauth_endpoints.json in the Kiro Crew data home; " - "otherwise ask the server owner to fix the URL.", + f"pattern (rejected).{endpoint_detail} If this is a self-hosted " + "or otherwise unlisted identity provider, its authorization " + "endpoint may need adding to oauth_endpoints.json in the Kiro " + 'Crew data home, shaped {"additional_authorization_endpoints": ' + '[{"host": ..., "path": ...}]}; otherwise ask the server owner ' + "to fix the URL.", "msg msg-warn", - meta={ - "server_name": safe_name, - "failed": True, - "rejected_url": True, - "error": "URL contained credential or exfiltration pattern", - "remedy": "oauth_endpoints.json", - }, + meta=rejected_meta, ) return # A new authorize request for this server means kiro-cli started a FRESH diff --git a/src/kiro_crew/security.py b/src/kiro_crew/security.py index a94ad34631d..27e81e1384a 100644 --- a/src/kiro_crew/security.py +++ b/src/kiro_crew/security.py @@ -17,6 +17,7 @@ import sys import threading import time +import unicodedata import uuid from collections import Counter from concurrent.futures import Future @@ -10840,6 +10841,13 @@ def _oauth_url_payload_diagnostic( ("gitlab.com", "/oauth/authorize"), ("mcp.auth.mail.superhuman.com", "/oauth2/authorize"), ("mcp.linear.app", "/authorize"), + # Maintainer-verified 2026-09-01 via RFC 8414 metadata at + # https://mcp.miro.com/.well-known/oauth-authorization-server + # (authorization_endpoint: https://mcp.miro.com/authorize), matching the + # reporter's independent RFC 8414 read in issue #7578. Not (yet) a + # Connections registry entry; the fail-closed banner blocked every + # attempt to connect the Miro remote MCP server. + ("mcp.miro.com", "/authorize"), ("mcp.notion.com", "/authorize"), ("vercel.com", "/oauth/authorize"), } @@ -12702,6 +12710,150 @@ def oauth_url_contains_credential(url: str) -> bool: return True +# Longest path echoed back by ``sanitized_oauth_endpoint``. Real authorization +# endpoint paths are short (the longest builtin is 31 chars); anything past this +# bound is noise at best and smuggled payload at worst, so it is truncated with +# an ellipsis rather than surfaced whole. +_SANITIZED_OAUTH_PATH_MAX_LEN = 200 + +# DNS caps a full hostname at 253 octets; a longer "host" is not a hostname. +_SANITIZED_OAUTH_HOST_MAX_LEN = 253 + + +def _contains_format_characters(text: str) -> bool: + """True when *text* carries Unicode format characters (category Cf). + + Zero-width and directional format characters (U+200B ZERO WIDTH SPACE, + U+200D ZWJ, U+2060 WORD JOINER, RTL/LTR marks, ...) are invisible in a + rendered banner: a credential split by them fails every substring pattern + here yet visually reassembles in the browser. Real authorization-endpoint + components are plain ASCII, so their mere presence is disqualifying. + """ + return any(unicodedata.category(ch) == "Cf" for ch in text) + + +def _oauth_component_is_unsafe(text: str) -> bool: + """True when a URL component carries credential-like material at ANY decode layer. + + Mirrors the rejection gate's decode budget (``_MAX_URL_DECODE_PASSES``): the + gate rejects a double-encoded credential on a DEEPER decode pass, so a + sanitizer that scanned only one layer would echo the very bytes the gate + refused. Fail-closed like the gate: a component still percent-decodable when + the budget runs out, or one carrying a heavy percent-encoded run, is unsafe + even when no known pattern matched. + """ + if _EXFIL_PERCENT_RE.search(text): + return True + candidate = text + for _ in range(_MAX_URL_DECODE_PASSES + 1): + # Invisible format characters (category Cf) split a credential so no + # substring pattern below can match it, while the browser renders the + # fragments visually reassembled. No legitimate endpoint component + # contains them, so presence alone is unsafe — checked on every decode + # layer because %E2%80%8B only becomes U+200B after a decode pass. + if _contains_format_characters(candidate): + return True + # _EXFIL_PATTERNS is included because it is a pattern family the + # REJECTION itself can fire on (plus-delimited private-key headers, + # SSH keys, token shapes) — a component must never be echoed when it + # matches what the gate refused. Over-matching only redacts more. + if ( + _contains_fixed_credential(candidate) + or _text_contains_bare_secret(candidate) + or _EXFIL_PATTERNS.search(candidate) + ): + return True + # unquote_plus, not unquote: form-encoded material delimits with "+" + # (e.g. a plus-separated private-key header), which only matches the + # credential patterns once folded to spaces. Display never uses this + # decoded form, so the wider fold cannot distort what is surfaced. + decoded = unquote_plus(candidate) + if decoded == candidate: + return False + candidate = decoded + # Still decodable after the budget — same deliberate fail-closed posture as + # the gate's saturation guard: refuse to echo what cannot be fully scanned. + return True + + +def sanitized_oauth_endpoint(url: str) -> tuple[str, str] | None: + """Best-effort ``(host, path)`` of an OAuth URL, safe to surface to users. + + :func:`oauth_url_contains_credential` answers only a boolean, so its + callers historically could not tell the user WHICH endpoint tripped the + scanner — the remedy (``oauth_endpoints.json``) needs an exact host+path to + be actionable. This sibling names the endpoint without weakening the + rejection: + + * only the lowercase hostname and the path are returned — NEVER the query, + fragment, port, or userinfo, which is where state/PKCE material and + smuggled credentials live; + * both components are scanned at every percent-decode layer up to the + gate's own budget: a credential-bearing path (raw, encoded, or + over-encoded past the budget) is replaced with the shared redaction tag, + and a credential-bearing HOSTNAME makes the whole helper return ``None`` + — a host is an identity, so a redacted host would name nothing; + * both components are length-capped, so a pathological URL cannot bloat a + banner or a log line. + + Returns ``None`` when the URL does not parse to a hostname, so callers fall + back to their existing unnamed message. Deliberately independent of WHY the + URL was rejected: it never re-runs the credential verdict. + """ + if not url: + return None + try: + parsed = urlparse(url) + host = parsed.hostname + except ValueError: + return None + if not host: + return None + # A userinfo-bearing authority is never named. Raw userinfo is stripped by + # parsed.hostname, but PERCENT-ENCODED userinfo (user%3Apass%40host, or the + # double-encoded %2540 form that survives one decode pass) hides inside + # what urlparse reports as the hostname — check for "@" at EVERY decode + # layer up to the gate's budget, and refuse to name an authority that is + # still decodable when the budget runs out. + netloc_candidate = parsed.netloc + for _ in range(_MAX_URL_DECODE_PASSES + 1): + if "@" in netloc_candidate: + return None + decoded_netloc = unquote_plus(netloc_candidate) + if decoded_netloc == netloc_candidate: + break + netloc_candidate = decoded_netloc + else: + return None + # Scan BEFORE truncating (both components): a credential split by a length + # cap must still trigger redaction, not survive in half. + host = host.lower() + if _oauth_component_is_unsafe(host): + return None + if not host.isascii(): + # Surface an internationalized host in A-label (punycode) form: it + # defuses homoglyph spoofing in the banner and matches the ASCII-only + # shape an oauth_endpoints.json entry must take anyway. + try: + host = host.encode("idna").decode("ascii") + except UnicodeError: + return None + # INVARIANT: the exact byte sequence surfaced must have passed the + # scan in its FINAL form. IDNA's nameprep folds fullwidth characters + # to ASCII, so a token-shaped fullwidth host that the pre-IDNA scan + # could not match can NORMALIZE INTO a credential — re-scan the + # transformed form and refuse to name it. + if _oauth_component_is_unsafe(host): + return None + host = host[:_SANITIZED_OAUTH_HOST_MAX_LEN] + path = parsed.path or "/" + if _oauth_component_is_unsafe(path): + path = _REDACTED_CREDENTIAL_TAG + elif len(path) > _SANITIZED_OAUTH_PATH_MAX_LEN: + path = path[:_SANITIZED_OAUTH_PATH_MAX_LEN] + "…" + return host, path + + # Standard replacement tag for a redacted credential. Shared between the batch # redactor (`redact_credentials`) and the streaming fail-closed path # (`StreamRedactor.feed`) so the on-the-wire marker is identical everywhere. diff --git a/test/oauth_url_corpus.py b/test/oauth_url_corpus.py index 23641b292af..d4cc012c3ed 100644 --- a/test/oauth_url_corpus.py +++ b/test/oauth_url_corpus.py @@ -169,6 +169,19 @@ "&code_challenge_method=S256" "&state=" + ("Kp7mQ2xR" * 12), # 96-char opaque state ), + # Miro remote MCP server — authorization endpoint verified via RFC 8414 + # metadata by the reporter of issue #7578; the fail-closed banner blocked + # every attempt to connect it before the endpoint was allowlisted. + ( + "miro-mcp", + "https://mcp.miro.com/authorize" + "?client_id=3458764514956732000" + "&redirect_uri=http%3A%2F%2F127.0.0.1%3A33418%2Fcallback" + "&response_type=code" + "&state=af0ifjsldkj" + "&code_challenge=E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM" + "&code_challenge_method=S256", + ), ] # Consent URLs that the ACP banner-safety gate diff --git a/test/test_mcp_oauth_banner.py b/test/test_mcp_oauth_banner.py index 780d24d2d98..3e94f02ef0b 100644 --- a/test/test_mcp_oauth_banner.py +++ b/test/test_mcp_oauth_banner.py @@ -224,6 +224,75 @@ def test_rejection_banner_names_the_operator_remedy(self): assert "oauth_url" not in m["meta"] assert "AKIAIOSFODNN7EXAMPLE" not in m["content"] + def test_rejection_banner_names_the_rejected_endpoint(self): + """A rejected URL must name the SANITIZED host+path that tripped the + scanner (#7578): without it the user cannot know which endpoint to + write into ``oauth_endpoints.json``, so the failure reads as + unfixable. Query values carry state/PKCE material (and here, the + smuggled credential) and must NEVER be echoed — not in the content, + not anywhere in the meta dict. + """ + slot = _ChatSlot("s1") + state = MagicMock() + _emit_mcp_oauth_request( + state, + slot, + "self-hosted", + "https://Unlisted-IdP.example/realms/dev/authorize" + "?state=topsecretstatevalue&key=AKIAIOSFODNN7EXAMPLE", + ) + m = slot.messages[0] + # Sanitized endpoint in the banner text. + assert "unlisted-idp.example/realms/dev/authorize" in m["content"] + # The dashboard's failed banner renders meta["error"], NOT content — + # the endpoint must ride there to actually reach the user's screen. + assert "unlisted-idp.example/realms/dev/authorize" in m["meta"]["error"] + # The banner names the expected file shape so the user knows what to write. + assert "additional_authorization_endpoints" in m["content"] + # No extra meta keys: no shipped surface reads any, so none are emitted. + assert "rejected_host" not in m["meta"] + assert "rejected_path" not in m["meta"] + assert "remedy_shape" not in m["meta"] + # Query values never leak into any surfaced field. + serialized = json.dumps(m, ensure_ascii=False) + assert "AKIAIOSFODNN7EXAMPLE" not in serialized + assert "topsecretstatevalue" not in serialized + + def test_redacted_path_never_echoes_the_credential(self): + """A credential-bearing path self-redacts to the shared tag before it + reaches the error field or the banner text.""" + slot = _ChatSlot("s1") + state = MagicMock() + token = "ghp_" "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdef12" + _emit_mcp_oauth_request( + state, + slot, + "self-hosted", + f"https://idp.example/{token}/authorize?state=x", + ) + m = slot.messages[0] + assert "idp.example[REDACTED: credential]" in m["meta"]["error"] + assert token not in json.dumps(m, ensure_ascii=False) + + def test_rejection_banner_survives_unparseable_url(self): + """A URL that cannot be parsed to a hostname still rejects with the + original unnamed banner — no endpoint fields, no crash.""" + slot = _ChatSlot("s1") + state = MagicMock() + _emit_mcp_oauth_request( + state, + slot, + "self-hosted", + "https://[bad-ipv6/authorize?key=AKIAIOSFODNN7EXAMPLE", + ) + m = slot.messages[0] + assert m["meta"]["failed"] is True + assert m["meta"]["rejected_url"] is True + assert "rejected_host" not in m["meta"] + assert "rejected_path" not in m["meta"] + assert "remedy_shape" not in m["meta"] + assert "AKIAIOSFODNN7EXAMPLE" not in m["content"] + def test_accepts_real_github_oauth_pkce_url(self): """Regression: a legitimate GitHub OAuth + PKCE consent URL must be rendered, not rejected. These URLs carry high-entropy params diff --git a/test/test_security.py b/test/test_security.py index 1485cc245ce..5a64a68e1b1 100644 --- a/test/test_security.py +++ b/test/test_security.py @@ -31,6 +31,7 @@ redact_and_truncate, redact_credentials, redact_exfiltration_urls, + sanitized_oauth_endpoint, scan_exfiltration_urls, scan_history, should_record_observe_history, @@ -2548,6 +2549,208 @@ def test_heavy_percent_encoding_in_standard_param_fails_closed(self) -> None: assert cleaned != url assert warnings + def test_miro_mcp_authorize_endpoint_is_approved(self) -> None: + """mcp.miro.com/authorize is a reporter-verified RFC 8414 endpoint + (#7578): a real PKCE consent URL there must pass the banner gate.""" + url = self.NOTION_URL.replace( + "https://api.notion.com/v1/oauth/authorize", + "https://mcp.miro.com/authorize", + 1, + ) + assert oauth_url_contains_credential(url) is False + + +class TestSanitizedOAuthEndpoint: + """``sanitized_oauth_endpoint`` names a rejected endpoint without leaking. + + The boolean gate alone leaves the user unable to tell WHICH URL tripped the + scanner (#7578); this helper surfaces host+path only. The invariant under + test: query values, fragments, userinfo, and credential-bearing paths never + appear in the returned tuple. + """ + + GITHUB_TOKEN = "ghp_" "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdef12" + + def test_returns_host_and_path_only(self) -> None: + result = sanitized_oauth_endpoint( + "https://idp.example/realms/dev/authorize" + "?state=topsecretstate&code_challenge=alsosecret" + ) + assert result == ("idp.example", "/realms/dev/authorize") + + def test_query_values_never_echoed(self) -> None: + result = sanitized_oauth_endpoint( + f"https://idp.example/authorize?token={self.GITHUB_TOKEN}" + ) + assert result is not None + assert self.GITHUB_TOKEN not in "".join(result) + + def test_host_is_lowercased(self) -> None: + assert sanitized_oauth_endpoint("https://IdP.Example/Authorize") == ( + "idp.example", + "/Authorize", # paths are case-sensitive, only the host normalizes + ) + + def test_empty_path_defaults_to_root(self) -> None: + assert sanitized_oauth_endpoint("https://idp.example") == ("idp.example", "/") + + def test_userinfo_authority_returns_none(self) -> None: + """A userinfo-bearing authority is never named — raw or percent-encoded + (user%3Apass%40host hides inside what urlparse reports as the + hostname), mirroring the rejection gate's own check (GPT review).""" + assert ( + sanitized_oauth_endpoint(f"https://{self.GITHUB_TOKEN}@idp.example/authorize") is None + ) + assert ( + sanitized_oauth_endpoint("https://user%3Apass%40idp.example/authorize?state=x") is None + ) + # DOUBLE-encoded userinfo (%2540) survives one decode pass; the "@" + # check runs at every decode layer like the rest of the scan. + assert ( + sanitized_oauth_endpoint("https://user%253Apass%2540idp.example/authorize?state=x") + is None + ) + + def test_fragment_never_echoed(self) -> None: + result = sanitized_oauth_endpoint("https://idp.example/authorize#fragmentsecret") + assert result == ("idp.example", "/authorize") + + def test_credential_in_path_is_redacted(self) -> None: + result = sanitized_oauth_endpoint(f"https://idp.example/{self.GITHUB_TOKEN}/authorize") + assert result is not None + host, path = result + assert host == "idp.example" + assert self.GITHUB_TOKEN not in path + assert path == security.REDACTED_CREDENTIAL_TAG + + def test_format_character_split_credential_in_path_is_redacted(self) -> None: + """Invisible format characters (U+200B) split a credential so no + substring pattern matches, yet the browser renders the fragments + visually reassembled — presence of ANY category-Cf character in a + component is disqualifying on its own (GPT review, round 8).""" + split_token = "\u200b".join( + self.GITHUB_TOKEN[i : i + 8] for i in range(0, len(self.GITHUB_TOKEN), 8) + ) + result = sanitized_oauth_endpoint(f"https://idp.example/{split_token}/authorize?x=1") + assert result is not None + host, path = result + assert host == "idp.example" + assert "\u200b" not in path + assert path == security.REDACTED_CREDENTIAL_TAG + + def test_percent_encoded_format_character_in_path_is_redacted(self) -> None: + """%E2%80%8B only becomes U+200B after a decode pass — the format + character check runs on every decode layer like the rest of the scan.""" + encoded_zwsp = "%E2%80%8B" + result = sanitized_oauth_endpoint(f"https://idp.example/auth{encoded_zwsp}orize?state=x") + assert result is not None + host, path = result + assert host == "idp.example" + assert path == security.REDACTED_CREDENTIAL_TAG + + def test_format_character_in_host_returns_none(self) -> None: + """A host carrying an invisible format character is not a nameable + identity — the helper falls back to the unnamed message.""" + assert sanitized_oauth_endpoint("https://idp\u200bevil.example/authorize") is None + + def test_percent_encoded_credential_in_path_is_redacted(self) -> None: + encoded = "%67%68%70%5F" + self.GITHUB_TOKEN.removeprefix("ghp_") + result = sanitized_oauth_endpoint(f"https://idp.example/{encoded}/authorize") + assert result is not None + host, path = result + assert self.GITHUB_TOKEN not in path + assert encoded not in path + assert path == security.REDACTED_CREDENTIAL_TAG + + def test_double_percent_encoded_credential_in_path_is_redacted(self) -> None: + """The rejection gate decodes up to _MAX_URL_DECODE_PASSES, so it + rejects a DOUBLE-encoded credential on a deeper pass — the sanitizer + must not echo bytes the gate refused (Opus review, worked case).""" + double_encoded = "%2567%2568%2570%255F" + self.GITHUB_TOKEN.removeprefix("ghp_") + result = sanitized_oauth_endpoint(f"https://idp.example/{double_encoded}/authorize") + assert result is not None + _, path = result + assert self.GITHUB_TOKEN not in path + assert double_encoded not in path + assert path == security.REDACTED_CREDENTIAL_TAG + + def test_path_still_decodable_past_budget_is_redacted(self) -> None: + """A path that keeps yielding new decode layers past the budget cannot + be fully scanned — fail closed to the tag, mirroring the gate.""" + nested = "%2525252541" # "A" percent-encoded 5 layers deep + result = sanitized_oauth_endpoint(f"https://idp.example/{nested}/authorize") + assert result is not None + _, path = result + assert path == security.REDACTED_CREDENTIAL_TAG + + def test_plus_delimited_private_key_in_path_is_redacted(self) -> None: + """Form-encoded material delimits with "+"; the scan must fold it to + spaces (unquote_plus) or a plus-separated private-key header slips + through every decode layer unmatched (GPT review).""" + result = sanitized_oauth_endpoint("https://idp.example/BEGIN+RSA+PRIVATE+KEY/authorize") + assert result is not None + _, path = result + assert path == security.REDACTED_CREDENTIAL_TAG + + def test_credential_in_hostname_returns_none(self) -> None: + """A credential smuggled into a DNS label (hyphens are DNS-legal, so a + Slack-token-shaped label parses as a hostname) must not be echoed — + a host is an identity, so the whole helper bails (GPT review).""" + url = "https://xoxb-1234567890-AbCdEfGhIjKl.evil.example/authorize?state=x" + assert sanitized_oauth_endpoint(url) is None + + def test_non_ascii_host_is_surfaced_as_idna_alabel(self) -> None: + """An internationalized host surfaces in punycode A-label form: defuses + homoglyph spoofing and matches the ASCII-only oauth_endpoints.json + entry shape.""" + result = sanitized_oauth_endpoint("https://bücher.example/authorize") + assert result is not None + host, path = result + assert host == "xn--bcher-kva.example" + assert host.isascii() + assert path == "/authorize" + + def test_fullwidth_host_normalizing_into_a_credential_returns_none(self) -> None: + """IDNA nameprep folds fullwidth characters to ASCII, so a token-shaped + fullwidth host can NORMALIZE INTO a credential the pre-IDNA scan could + not match — the surfaced form must be re-scanned after every transform + (GPT review).""" + fullwidth = "ｘｏｘｂ－１２３４５６７８９０－ａｂｃｄｅｆｇｈｉｊｋｌ" + assert sanitized_oauth_endpoint(f"https://{fullwidth}.evil.example/authorize") is None + + def test_overlong_path_is_truncated(self) -> None: + # Hyphenated segments: no 40+ run of the base64 alphabet, so the path + # is benign-long rather than entropy-suspicious — it truncates, not + # redacts. + long_path = "/seg-ment" * 40 + result = sanitized_oauth_endpoint(f"https://idp.example{long_path}") + assert result is not None + _, path = result + assert len(path) == security._SANITIZED_OAUTH_PATH_MAX_LEN + 1 + assert path.endswith("…") + + def test_overlong_host_is_capped(self) -> None: + # 30-char labels: below the 40-char bare-run floor, so the host is + # benign-long — it caps, not bails. + long_host = ".".join(["a" * 30] * 9) + ".example" + result = sanitized_oauth_endpoint(f"https://{long_host}/authorize") + assert result is not None + host, _ = result + assert len(host) <= security._SANITIZED_OAUTH_HOST_MAX_LEN + + @pytest.mark.parametrize( + "url", + [ + "", + "https://[bad-ipv6/x", + "not a url at all", + "https:///path-without-host", + ], + ids=["empty", "invalid-ipv6", "not-a-url", "no-host"], + ) + def test_unparseable_urls_return_none(self, url: str) -> None: + assert sanitized_oauth_endpoint(url) is None + class TestOperatorOAuthEndpointExtension: """The keystone ``oauth_endpoints.json`` extends the OAuth endpoint set.