fix(security): name the rejected OAuth endpoint in the banner (#7578)

[CrysisDeu]

Summary

The MCP OAuth rejection banner said only "URL contained credential or exfiltration pattern" — no URL, no host — so a user connecting a remote MCP server whose authorization endpoint is outside the hardcoded allowlist (_OAUTH_AUTHORIZATION_ENDPOINTS) could not tell which URL tripped the scanner or what to write into the documented remedy file (oauth_endpoints.json).

Refs #7578

What changed

• security.sanitized_oauth_endpoint(url) (new): returns the lowercase host + path of a rejected authorization URL, safe to surface. 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, and the _EXFIL_PATTERNS set — private-key headers, SSH keys, token shapes). A credential-bearing or budget-exhausting path self-redacts to the shared [REDACTED: credential] tag; a credential-bearing hostname makes the helper return None (a host is an identity — a redacted host would name nothing); a non-ASCII host surfaces in IDNA A-label form (defuses homoglyph spoofing). Length caps on both. The boolean oauth_url_contains_credential API is untouched.
• Rejection banner (chat_runner.py): names the sanitized endpoint in the banner content — which also spells the {"additional_authorization_endpoints": [{"host": ..., "path": ...}]} entry shape — and inside the error meta field, which is the field the dashboard's failed banner actually renders (verified against McpOAuthBanner.tsx). No new meta keys: no shipped surface reads any (per First Principles review, an earlier revision's rejected_host/rejected_path/remedy_shape keys were removed as zero-consumer riders).
• Miro endpoint: ("mcp.miro.com", "/authorize") added to the builtin set — maintainer-verified via RFC 8414 metadata at mcp.miro.com/.well-known/oauth-authorization-server, matching the reporter's independent read (issue item 4's one-line addition; no auto-discovery built).
• Docs: docs/system-specs/modules/security.md OAuth section updated in the same commit.

Scope

Issue items 1–2 (plus the one-line item-4 endpoint addition). Remaining open:

• Item 3 (end-to-end "connect an unlisted OAuth MCP server" docs) — tracked separately in Docs: no end-to-end guide for installing/authenticating a remote (HTTP/OAuth) MCP server #7579.
• Item 4's larger half (RFC 8414 auto-discovery) — open design question, deliberately not built here.

Tests

• New TestSanitizedOAuthEndpoint (test_security.py): query/PKCE values never echoed; single-, double-, past-budget-encoded, and plus-delimited credential paths all redact; credential inside a DNS label returns None; IDNA A-label surfacing; benign-long path truncation and host cap; None fallbacks for empty/invalid/host-less URLs.
• Banner tests (test_mcp_oauth_banner.py): endpoint named in content AND in meta["error"]; entry shape named in content; no extra meta keys; no query value anywhere in the serialized message; credential-bearing path shows the redaction tag, never the credential; unparseable URL falls back to the unnamed banner.
• Miro consent URL added to the legit-OAuth corpus (contract: must pass the gate) + an allowlist unit test.

Manual verification

N/A — unit coverage sufficient: the banner emit path and the sanitizer are pure functions over the URL, and the tests assert the exact rendered fields (content, meta["error"]).

Pattern harvest

Rule candidate: when a security gate rejects input with an opaque message, the rejection surface must name the sanitized identity of what was rejected (here host+path, never values) — and the naming helper must scan with the SAME pattern families and decode depth as the gate itself, or it can echo the very bytes the gate refused. Also: put user-facing detail in the field the frontend actually renders (meta["error"]), not just the message content.

Why no screenshot: backend-only change; the banner is rendered from the same error meta field as before — text content differs only in the appended endpoint sentence, no component/layout/style change.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 21a1e514becd8f74403ebc01c8a3d8bf5f0a3271 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Sound shape: names identity only, reuses the gate's own decode budget and pattern primitives, fails closed, and deliberately defers auto-discovery.

Suggestions

• _oauth_component_is_unsafe hand-enumerates the gate's pattern families (_contains_fixed_credential / _text_contains_bare_secret / _EXFIL_PATTERNS); a family later added to the rejection gate desyncs silently and the sanitizer echoes bytes the gate refused — extract one shared scanner tuple both consume so the invariant is structural, not conventional.

[DESIGN-REVIEWED] 21a1e51

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 21a1e514becd8f74403ebc01c8a3d8bf5f0a3271 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is gathered. The change decomposes cleanly, every item is declared, and the one depth risk is a counted sibling rejection surface. Final review:

First-Principles-Verdict: CONCERNS

The banner fix is cause-level and earns its place, but the connections mint path rejects the same URLs with a bare reason code — an unfixed, undeclared sibling.

What this change ships

Intent: let a user see which OAuth endpoint the scanner rejected so they can write the oauth_endpoints.json remedy — a FIX (#7578).

1. Rejection banner text now names the sanitized host+path — justified (reported defect).
2. meta["error"] carries the endpoint, the field the dashboard actually renders — justified.
3. Banner spells the additional_authorization_endpoints entry shape — justified (remedy needs the exact shape).
4. New public security.sanitized_oauth_endpoint() — 1 consumer (chat_runner.py:1692), the fix's mechanism; not generalized, no knobs.
5. Miro endpoint allowlisted — rides along, but declared, derived from issue item 4, one line.
6. security.md OAuth section updated same-commit — mandated by AGENTS.md.

Watch

• Point patch on one of two rejection surfaces. The gate oauth_url_contains_credential has 3 non-banner consumers (grepped: mint.py:707, warm.py:1929, chat_utils.py:1437); mint.py:726 terminates a Connections-card mint with only "reason": "mint_url_rejected" — same root cause, no endpoint named, and the new helper is exactly what it would call. The description's "Remaining open" list names items 3 and 4 but not this sibling. Accepted-and-deferred if intended; say so or wire it.
• Not a duplicate: diagnose_oauth_url_credential (security.py:12508) deliberately never returns host/URL, so the new helper is meaningfully different, verified.

[FIRST-PRINCIPLES-REVIEWED] 21a1e51

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 21a1e514becd8f74403ebc01c8a3d8bf5f0a3271 — this comment is updated in place on each push.

Review details

Everything verifies: Any is imported, _REDACTED_CREDENTIAL_TAG is a module global resolved at call time, the sanitizer scans one decode layer more than the gate (_MAX_URL_DECODE_PASSES + 1) and fails closed on budget exhaustion, so it can never echo bytes the gate refused. Host/path are the only components surfaced; userinfo, query, and fragment are dropped and re-checked. The spec is updated in the same commit and no blocking: true rule is weakened. The candidate list carried no candidates, and I found no groundable defect at 80+ in Step 2.

No findings.

[OPUS-REVIEWED] 21a1e51

Verdict parsed from the review's SHA-scoped output markers for commit 21a1e514becd8f74403ebc01c8a3d8bf5f0a3271.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 21a1e514becd8f74403ebc01c8a3d8bf5f0a3271: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 21a1e514becd8f74403ebc01c8a3d8bf5f0a3271 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/security.py:12807 -- "host = parsed.hostname" lowercases a rejected SG.<lowercase>.<lowercase> key before scanning, allowing it into the dashboard banner -> Fix: scan parsed.netloc before hostname normalization.
[GPT-REVIEWED] 21a1e51

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 21a1e514becd8f74403ebc01c8a3d8bf5f0a3271: <one-sentence reason>

[CrysisDeu]

• Plus-delimited private-key paths bypass sanitization — disposition: fixed in 91c578f. (span=732b722c70fd)

The component scan loop now decodes with unquote_plus (folding + to spaces before matching) and additionally runs _EXFIL_PATTERNS — the same pattern family the rejection itself can fire on (its private-key header branch accepts [\s+%] delimiters) — at every decode layer. A plus-delimited private-key path now redacts to the shared tag; pinned by test_plus_delimited_private_key_in_path_is_redacted. The advisory FINDING about remedy_shape for unallowlistable endpoints is moot on the new head: remedy_shape (and the other zero-consumer meta keys) were removed entirely per the First Principles subtraction; the entry shape is spelled in the banner text only.

[CrysisDeu]

• Three new meta keys (rejected_host, rejected_path, remedy_shape) ride along with zero consumers — disposition: fixed in 91c578f.

Subtraction taken exactly as prescribed: the three keys and their test assertions are deleted; the pre-existing error/rejected_url/remedy keys stay, and the endpoint + oauth_endpoints.json entry shape travel only in the banner content and the rendered meta["error"] field. docs/system-specs/modules/security.md updated to match. The same head also removes the mint.py log rider flagged by Semgrep/CodeQL, so the whole diff now ships only surfaces with shipped readers.

[CrysisDeu]

• Advisory FINDING: the remedy_shape guard checks only rejected_path shape — disposition: fixed (by removal) in 91c578f.

remedy_shape no longer exists on the new head: it was deleted per the First Principles zero-consumer subtraction, so the partially-guarded emission path this finding pointed at is gone rather than patched. The entry shape is named in the banner text only, as documentation for the operator to fill in by hand.

[CrysisDeu]

• The spec documents a mint log line the diff never adds — disposition: fixed (by subtraction) in e4d0fb0.

The stale mint clause in docs/system-specs/modules/security.md is deleted, and "and logs" is dropped from the commit subject and PR title — the diff and its description now agree: the fix ships in the banner content and the rendered error field only. Context: an earlier revision did add the mint log line, but Semgrep and CodeQL both flagged it (keyword/taint on the logger call), so the rider was removed rather than annotated; the prose just hadn't caught up. On the Watch note (mint.py / warm.py sibling surfaces): accepted-and-deferred — the helper now exists and each is one line, but each also needs scanner-annotation decisions this PR shouldn't carry; deferred to #7765 rather than re-adding a surface two scanners rejected in round 1.

[CrysisDeu]

• Percent-encoded userinfo is surfaced as the host — disposition: fixed in c604803. (span=732b722c70fd)

Exactly as prescribed: sanitized_oauth_endpoint now refuses to name any userinfo-bearing authority — "@" in parsed.netloc or "@" in unquote(parsed.netloc) returns None, mirroring the rejection gate's own check, so user%3Apass%40idp.example can never appear in the banner or persisted meta. The raw-userinfo test was tightened to the same None contract and a percent-encoded pin case added (test_userinfo_authority_returns_none).

[CrysisDeu]

• IDNA normalization creates an unscanned credential — disposition: fixed in 54d0665. (span=732b722c70fd)

Exactly as prescribed: _oauth_component_is_unsafe(host) re-runs after the IDNA conversion and returns None if unsafe; pinned by test_fullwidth_host_normalizing_into_a_credential_returns_none (fullwidth xoxb-… host nameprep-folds into a Slack-token shape and is now refused).

This is the third blocking finding in this span (decode depth + host scan → percent-encoded userinfo → post-IDNA), so the fix asserts the class-closing invariant rather than another point patch: the exact bytes surfaced must be the bytes that passed the scan, re-checked after every transform. Coverage of every transform in the helper:

1. urlparse.hostname + lower() — scanned AFTER, at every percent-decode layer up to the gate's budget.
2. Userinfo — any @ in netloc, raw or percent-decoded, bails to None before naming anything (round-3 fix).
3. IDNA encode — re-scanned AFTER the transform (this fix); UnicodeError bails to None.
4. Truncation (host cap / path cap + …) — removal-only plus a non-alphabet suffix: cannot CREATE a substring match, and the scan ran on the pre-truncation superset.
5. Path — scanned at every unquote_plus layer against fixed-credential, bare-secret-run, and _EXFIL_PATTERNS families; unsafe or budget-exhausting paths become the constant redaction tag.
6. Composition (host + path in the banner/error string) — no pattern spans the boundary: the path always begins / (breaks every fixed-credential shape) and the host is lowercase-folded before display (defeats the entropy heuristic for boundary-spanning runs).

[CrysisDeu]

• Double-encoded userinfo bypasses unquote(parsed.netloc) — disposition: fixed in 7e96bc3. (span=732b722c70fd)

Exactly as prescribed: the "@" check now decodes parsed.netloc through the bounded _MAX_URL_DECODE_PASSES loop, returning None on any layer containing "@" and failing closed when the authority is still decodable at budget exhaustion — the userinfo check now honors the same every-layer invariant as the rest of the scan. Pinned by the user%253Apass%2540idp.example case in test_userinfo_authority_returns_none.

[CrysisDeu]

• Names the endpoint on 1 of 3 user-facing rejection surfaces without declaring the other two as deferred — disposition: accepted-and-deferred to Name the rejected OAuth endpoint in the connections mint/warm rejection surfaces #7765.

The mint (mint.py:700) and warm (warm.py) sibling surfaces are tracked in #7765, filed during this PR's own review cycle: the mint variant WAS attempted in round 1 and was flagged by both Semgrep (python-logger-credential-disclosure) and CodeQL (py/clear-text-logging-sensitive-data), so threading the endpoint there needs a per-site scanner-treatment decision (inline suppression with justification, a CodeQL barrier model, or a non-log surfacing channel) that this PR should not carry. #7765's body records that context so the pick-up is not a blind re-walk.

[bolichen97]

sanitized_oauth_endpoint names only the host and path and reuses the gate's own decode budget (_MAX_URL_DECODE_PASSES) with unquote_plus plus the same three pattern families the rejection fires on, so it cannot echo the bytes the gate refused: an unsafe path collapses to the shared redaction tag, an unsafe or percent-encoded-userinfo authority returns None, a still-decodable component at the budget fails closed, and the post-IDNA re-scan closes the nameprep-normalizes-into-a-credential case. Putting the endpoint in meta["error"] is the correct field given McpOAuthBanner renders that one, and dropping the zero-consumer meta keys keeps the payload honest.