feat(connections): provider registry and consent-URL security - #2285
Conversation
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound narrow carve-out, but it lands as a second, stricter OAuth gate beside the live permissive one — the weaker gate stays enforced until a future slice. Watch
Suggestions
[DESIGN-REVIEWED] 55a886e |
Carry the Connections provider registry and the consent-URL safety core. Both land dark: the connections_ui flag is off, and nothing here is reachable from the UI on main. Registry (src/kiro_crew/connections/): - Seven entries (notion, github, linear, atlassian, stripe, vercel, gitlab). GitHub stays launch-gated (launch_gate_passed false) pending its app registration, so only six are visible. - Revoke links are now a dated safety promise: revoke_verified_on / revoke_verified_note record when and how each link was checked, and a test fails the build once an attestation ages past 180 days. Notion, Stripe and Vercel links are corrected, and revoke_manual_path carries the in-app navigation for providers whose settings page is a single-page app that re-routes after sign-in and cannot be deep-linked. - l0_expectations pins each provider's advertised authorization-server origin plus DCR/PKCE support, for the account-free probe that consumes it later. - Optional client_id supports the one non-DCR provider without loosening the required-field contract for the rest. Consent-URL security (src/kiro_crew/security.py): - oauth_url_contains_credential is the single gate for validating a provider consent URL, and the only path allowed to exempt standard OAuth entropy (state, PKCE) from the generic URL heuristics. - The exemption is keyed on an exact, code-owned (host, path) allowlist that includes each launch provider's MCP authorization server. These differ from the classic web-OAuth endpoints, and a missing pair fails closed. - Fail-closed everywhere else: fixed credential signatures, multi-pass percent-decoding, heavy percent-encoding, userinfo, path params and fragments are all rejected. Only a structurally valid S256 PKCE challenge at an approved endpoint is subtracted before scanning. - The percent-decode loop is bounded so an over-encoded URL cannot spin it, and saturation fails closed: a payload still decodable when the budget runs out is treated as credential-bearing. Otherwise the bound was an escape hatch — a credential wrapped in one more layer than the cap was never seen in plaintext, and the intermediate forms match neither the literal credential patterns nor the consecutive-octet heavy-encoding detector. - scan_exfiltration_urls and redact_exfiltration_urls keep the strict behaviour for arbitrary agent and model text; they do not inherit the carve-out.
Opus 5 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
3f46a8e to
55a886e
Compare
|
Dispositions for the findings reported against
|
|
Answering the Design Review (advisory CONCERNS) item by item:
|
) The 0.2.0 section was written in #2305, the commit that became v0.2.0-rc.4. Seventy-one commits have landed on main since, nineteen of them feat:, and the section was never revisited. It therefore both omitted shipped features and described one that no longer exists as written. The wrong entry mattered most: the Webhooks bullet told the reader to manage inbound automation "from Settings", but #2343 moved that page behind a per-device Preview pages toggle under Developer and hides it by default. A 0.2.0 user following the release notes would have gone looking for a page that is not there. Added, all from the rc.6 range: opt-in Slack setup and the multi-channel repositioning (#2340), Telegram multi-account (#2203) and inbound attachments (#2201), sub-agent completions reaching non-Slack parents (#2352), Discord reply continuation (#2326), Slack OPTIONS as a control (#1467), the Agent Templates two-pane inspector, project-local agent discovery (#2167), send-a-copy-to-another-instance, Jira and setting link chips (#2019, #1907), CJK emphasis rendering, the MCP Apps switch (#2293, #2337), the Connections provider registry (#2285), GitHub Enterprise Server support in Code Review Sage (#2154), operator notes on user deny patterns (#2341), the locked git-publish floor rules (#2369), the persist-or-refuse boot guard (#2279), and the turn-ceiling bounds on the approval and stall windows (#2372, #2373). Scope is exactly 5fe4bd5..ab20b4e, the range v0.2.0-rc.6 ships. The three commits main carries beyond rc.6, including meeting deletion (#2268), belong to the next release and are deliberately not described here.
…ance probe The provider registry has carried `l0_expectations` since #2285 with no consumer: nothing checked that Notion still uses DCR, that Atlassian's authorization server is still what we think it is, or that any of it was ever true. This is the consumer. A scheduled job fetches each provider's PUBLIC OAuth discovery documents -- protected-resource metadata at its well-known path (RFC 9728) and the authorization-server metadata that names (RFC 8414) -- and diffs what the provider advertises against the committed baseline. Every request is an unauthenticated GET of a static document, so the job needs no repository secret and no provider account. Scope is named in the module docstring, the workflow name and the report itself: this is STATIC METADATA conformance. It does not check challenge shape (does an unauthenticated request answer 401 with a well-formed `resource_metadata` parameter), and a green run says nothing about it -- two of the seven providers fail that today while serving good metadata, so it needs its own baseline of known exceptions and is deferred to L0b. Recut of the kernel of #1329, re-homed onto current main. ## The issuer is an identity, compared code point for code point The baseline field was `authorization_server_origin` and was compared by origin. RFC 8414 §2 makes the issuer an identity compared by simple string comparison, so an origin-only check accepts a tenant or realm substitution -- `/tenant-a` answering for `/tenant-b`. Worse, three of the seven baselines could not even be expressed: stripe https://access.stripe.com/mcp github https://github.com/login/oauth atlassian https://auth.atlassian.com/VCeDsk8ZHncYF1g234fKtc4lNipbBhu3 All three carry a path the old field silently dropped, and the well-known URL at their bare origin returns 404. The field is now `authorization_server` and holds the full issuer, compared VERBATIM. Nothing is normalized away: not host case, not an explicit `:443`, not a trailing slash. Each of those would make two distinct issuer strings compare equal, which is precisely the substitution the check exists to catch -- an authorization server answering for `https://Auth.Example.com` when we committed `https://auth.example.com` has told us something worth surfacing. The one transformation in the module is the single trailing slash removed when CONSTRUCTING the well-known URL, which never touches a compared value. All seven committed baselines were verified to survive exact comparison against both the live advertised value and each issuer's own self-declared `issuer` field. ## The probe never dereferences a provider-supplied URL One invariant holds in both modes: the only authorization-server URL the probe fetches is the one COMMITTED in the registry. Record mode previously dropped the expected-origin guard and then fetched `authorization_servers[0]` -- so a compromised provider could aim the runner at an arbitrary or internal host and get that origin stamped into the registry. Now an advertised issuer is compared, never followed. In probe mode a mismatch fails; in record mode it is reported as NEEDS APPROVAL and the recorder refuses to write it -- including its date, so an unapproved move cannot buy itself another window of silence. That approval path is how the three corrections above were made: the first record run refused all three and printed what to review. Host vetting runs on the host that will actually be DIALLED, not on the raw string. YARL (under aiohttp) IDNA-encodes before connecting, so `https://127。0。0。1` -- with U+3002 IDEOGRAPHIC FULL STOP -- has a raw hostname that is neither an IP literal nor even dotted, yet resolves to `127.0.0.1`. The canonical host is therefore DERIVED FROM YARL rather than re-derived: build a URL and read back `raw_host`, which is the field aiohttp hands its resolver (connector.py). Vetted bytes and dialled bytes are then the same bytes by construction. Re-implementing the encoding was not merely redundant, it was wrong: the stdlib `idna` codec is IDNA2003 while yarl prefers IDNA2008/UTS-46, and they disagree on deviation characters in both directions -- `faß.de` reads as `fass.de` to the stdlib while yarl dials `xn--fa-hia.de`, a Greek final sigma maps to a different A-label entirely, and a ZWJ label the stdlib happily encodes makes yarl refuse the URL outright. A host yarl will not build is refused, and so is one carrying authority punctuation, since yarl would otherwise reinterpret `user@evil.com` as userinfo plus host. Both the registry validator and the probe share that one predicate. Canonicalization is used for vetting ONLY and never touches a compared value: `faß.de` and `xn--fa-hia.de` dial the same host but remain different issuer identifiers, and a test pins that they do not compare equal. ## A stale baseline is not a repo-wide outage A single 90-day threshold over all providers would have failed every PR in the repo on a timer nobody could reset: the nightly cannot refresh the stamps (it is read-only by design) and CI has no network to run the remedy. Three changes: - the PR suite WARNS from day 60 and never fails on age for a dark provider - it hard-fails only past day 90 and only for providers visible to users - the NIGHTLY reports staleness and goes red on it (`--fail-on-stale`, passed only there), so the reminder lands where someone can act ## A fatal error is evidence about the probe, not the providers A crash before the provider loop used to become seven ordinary per-provider failures -- exit 0 for two nights, then a red build blaming all seven. It now exits non-zero immediately and leaves the streak state untouched. Per-provider failures keep the streak semantics they had. ## Transient vs drift, and a lost streak that says so Each provider carries a streak of consecutive failing runs; the job goes red only at three in a row, with night one's failure still in the report. State travels as a build artifact rather than a committed file, so a job making outbound requests never needs `contents: write`. Losing that state is fail-safe but never silent. The document is bound to its contents by a digest, and a state file that is malformed or inconsistent is reported by reason. An ABSENT file is the subtler case, because it means two different things: nothing has ever run, or a previous run's artifact expired. The workflow already knows which -- it looks for a prior run before trying the download -- and now passes that through, so an expected-but-missing artifact is reported as `artifact_missing` while a genuine first run stays null. Otherwise a two-night streak resets with nothing in the report to show it. ## Dates are UTC on both sides `--record` stamps the UTC date, but the future-date guard compared against the LOCAL date, so a user west of UTC could not load a baseline the recorder had just written until local midnight caught up. Both sides now go through one `utc_today()`. Rewriting is style-preserving on purpose. `registry.json` is one compact object per line, which is what keeps a provider edit to a one-line diff a reviewer can read; reformatting would rewrite every line of a security-relevant file. The rewrite goes through the shared `atomic_write` helper rather than `write_text`. An in-place write truncates first, so a full disk or an interrupt partway through leaves a half-written registry -- and this file is parsed at import time, which makes a corrupt one a startup failure for the whole package rather than a failed command. The helper writes a temp file in the same directory and renames, so a reader sees either the old registry or the new one. ## Also Stamp churn is documented as intended: a confirming capture on a later day does move `verified_on`, because the field records when a baseline was last re-derived, not when it last changed. And the stamp is described as a refresh marker rather than a provenance guarantee -- what actually guarantees the baseline is the nightly probe re-deriving it, not a self-attested date. GitLab sends `resource` as an array rather than the string RFC 9728 specifies; the probe tolerates that on a field it only uses to confirm the document describes the right host. The workflow carries a runbook for every deliberate failure mode: refused redirects, refused compression, needs-approval, artifact_missing, and staleness.
…ance probe The provider registry has carried `l0_expectations` since #2285 with no consumer: nothing checked that Notion still uses DCR, that Atlassian's authorization server is still what we think it is, or that any of it was ever true. This is the consumer. A scheduled job fetches each provider's PUBLIC OAuth discovery documents -- protected-resource metadata at its well-known path (RFC 9728) and the authorization-server metadata that names (RFC 8414) -- and diffs what the provider advertises against the committed baseline. Every request is an unauthenticated GET of a static document, so the job needs no repository secret and no provider account. Scope is named in the module docstring, the workflow name and the report itself: this is STATIC METADATA conformance. It does not check challenge shape (does an unauthenticated request answer 401 with a well-formed `resource_metadata` parameter), and a green run says nothing about it -- two of the seven providers fail that today while serving good metadata, so it needs its own baseline of known exceptions and is deferred to L0b. Recut of the kernel of #1329, re-homed onto current main. ## The issuer is an identity, compared code point for code point The baseline field was `authorization_server_origin` and was compared by origin. RFC 8414 §2 makes the issuer an identity compared by simple string comparison, so an origin-only check accepts a tenant or realm substitution -- `/tenant-a` answering for `/tenant-b`. Worse, three of the seven baselines could not even be expressed: stripe https://access.stripe.com/mcp github https://github.com/login/oauth atlassian https://auth.atlassian.com/VCeDsk8ZHncYF1g234fKtc4lNipbBhu3 All three carry a path the old field silently dropped, and the well-known URL at their bare origin returns 404. The field is now `authorization_server` and holds the full issuer, compared VERBATIM. Nothing is normalized away: not host case, not an explicit `:443`, not a trailing slash. Each of those would make two distinct issuer strings compare equal, which is precisely the substitution the check exists to catch -- an authorization server answering for `https://Auth.Example.com` when we committed `https://auth.example.com` has told us something worth surfacing. The one transformation in the module is the single trailing slash removed when CONSTRUCTING the well-known URL, which never touches a compared value. All seven committed baselines were verified to survive exact comparison against both the live advertised value and each issuer's own self-declared `issuer` field. ## The probe never dereferences a provider-supplied URL One invariant holds in both modes: the only authorization-server URL the probe fetches is the one COMMITTED in the registry. Record mode previously dropped the expected-origin guard and then fetched `authorization_servers[0]` -- so a compromised provider could aim the runner at an arbitrary or internal host and get that origin stamped into the registry. Now an advertised issuer is compared, never followed. In probe mode a mismatch fails; in record mode it is reported as NEEDS APPROVAL and the recorder refuses to write it -- including its date, so an unapproved move cannot buy itself another window of silence. That approval path is how the three corrections above were made: the first record run refused all three and printed what to review. Host vetting runs on the host that will actually be DIALLED, not on the raw string. YARL (under aiohttp) IDNA-encodes before connecting, so `https://127。0。0。1` -- with U+3002 IDEOGRAPHIC FULL STOP -- has a raw hostname that is neither an IP literal nor even dotted, yet resolves to `127.0.0.1`. The canonical host is therefore DERIVED FROM YARL rather than re-derived: build a URL and read back `raw_host`, which is the field aiohttp hands its resolver (connector.py). Vetted bytes and dialled bytes are then the same bytes by construction. Re-implementing the encoding was not merely redundant, it was wrong: the stdlib `idna` codec is IDNA2003 while yarl prefers IDNA2008/UTS-46, and they disagree on deviation characters in both directions -- `faß.de` reads as `fass.de` to the stdlib while yarl dials `xn--fa-hia.de`, a Greek final sigma maps to a different A-label entirely, and a ZWJ label the stdlib happily encodes makes yarl refuse the URL outright. A host yarl will not build is refused, and so is one carrying authority punctuation, since yarl would otherwise reinterpret `user@evil.com` as userinfo plus host. Both the registry validator and the probe share that one predicate. Canonicalization is used for vetting ONLY and never touches a compared value: `faß.de` and `xn--fa-hia.de` dial the same host but remain different issuer identifiers, and a test pins that they do not compare equal. ## A stale baseline is not a repo-wide outage A single 90-day threshold over all providers would have failed every PR in the repo on a timer nobody could reset: the nightly cannot refresh the stamps (it is read-only by design) and CI has no network to run the remedy. Three changes: - the PR suite WARNS from day 60 and never fails on age for a dark provider - it hard-fails only past day 90 and only for providers visible to users - the NIGHTLY reports staleness and goes red on it (`--fail-on-stale`, passed only there), so the reminder lands where someone can act ## A fatal error is evidence about the probe, not the providers A crash before the provider loop used to become seven ordinary per-provider failures -- exit 0 for two nights, then a red build blaming all seven. It now exits non-zero immediately and leaves the streak state untouched. Per-provider failures keep the streak semantics they had. ## Transient vs drift, and a lost streak that says so Each provider carries a streak of consecutive failing runs; the job goes red only at three in a row, with night one's failure still in the report. State travels as a build artifact rather than a committed file, so a job making outbound requests never needs `contents: write`. Losing that state is fail-safe but never silent. The document is bound to its contents by a digest, and a state file that is malformed or inconsistent is reported by reason. An ABSENT file is the subtler case, because it means two different things: nothing has ever run, or a previous run's artifact expired. The workflow already knows which -- it looks for a prior run before trying the download -- and now passes that through, so an expected-but-missing artifact is reported as `artifact_missing` while a genuine first run stays null. Otherwise a two-night streak resets with nothing in the report to show it. ## Dates are UTC on both sides `--record` stamps the UTC date, but the future-date guard compared against the LOCAL date, so a user west of UTC could not load a baseline the recorder had just written until local midnight caught up. Both sides now go through one `utc_today()`. Rewriting is style-preserving on purpose. `registry.json` is one compact object per line, which is what keeps a provider edit to a one-line diff a reviewer can read; reformatting would rewrite every line of a security-relevant file. The rewrite goes through the shared `atomic_write` helper rather than `write_text`. An in-place write truncates first, so a full disk or an interrupt partway through leaves a half-written registry -- and this file is parsed at import time, which makes a corrupt one a startup failure for the whole package rather than a failed command. The helper writes a temp file in the same directory and renames, so a reader sees either the old registry or the new one. ## Also Stamp churn is documented as intended: a confirming capture on a later day does move `verified_on`, because the field records when a baseline was last re-derived, not when it last changed. And the stamp is described as a refresh marker rather than a provenance guarantee -- what actually guarantees the baseline is the nightly probe re-deriving it, not a self-attested date. GitLab sends `resource` as an array rather than the string RFC 9728 specifies; the probe tolerates that on a field it only uses to confirm the document describes the right host. The workflow carries a runbook for every deliberate failure mode: refused redirects, refused compression, needs-approval, artifact_missing, and staleness.
…ance probe (#3261) The provider registry has carried `l0_expectations` since #2285 with no consumer: nothing checked that Notion still uses DCR, that Atlassian's authorization server is still what we think it is, or that any of it was ever true. This is the consumer. A scheduled job fetches each provider's PUBLIC OAuth discovery documents -- protected-resource metadata at its well-known path (RFC 9728) and the authorization-server metadata that names (RFC 8414) -- and diffs what the provider advertises against the committed baseline. Every request is an unauthenticated GET of a static document, so the job needs no repository secret and no provider account. Scope is named in the module docstring, the workflow name and the report itself: this is STATIC METADATA conformance. It does not check challenge shape (does an unauthenticated request answer 401 with a well-formed `resource_metadata` parameter), and a green run says nothing about it -- two of the seven providers fail that today while serving good metadata, so it needs its own baseline of known exceptions and is deferred to L0b. Recut of the kernel of #1329, re-homed onto current main. ## The issuer is an identity, compared code point for code point The baseline field was `authorization_server_origin` and was compared by origin. RFC 8414 §2 makes the issuer an identity compared by simple string comparison, so an origin-only check accepts a tenant or realm substitution -- `/tenant-a` answering for `/tenant-b`. Worse, three of the seven baselines could not even be expressed: stripe https://access.stripe.com/mcp github https://github.com/login/oauth atlassian https://auth.atlassian.com/VCeDsk8ZHncYF1g234fKtc4lNipbBhu3 All three carry a path the old field silently dropped, and the well-known URL at their bare origin returns 404. The field is now `authorization_server` and holds the full issuer, compared VERBATIM. Nothing is normalized away: not host case, not an explicit `:443`, not a trailing slash. Each of those would make two distinct issuer strings compare equal, which is precisely the substitution the check exists to catch -- an authorization server answering for `https://Auth.Example.com` when we committed `https://auth.example.com` has told us something worth surfacing. The one transformation in the module is the single trailing slash removed when CONSTRUCTING the well-known URL, which never touches a compared value. All seven committed baselines were verified to survive exact comparison against both the live advertised value and each issuer's own self-declared `issuer` field. ## The probe never dereferences a provider-supplied URL One invariant holds in both modes: the only authorization-server URL the probe fetches is the one COMMITTED in the registry. Record mode previously dropped the expected-origin guard and then fetched `authorization_servers[0]` -- so a compromised provider could aim the runner at an arbitrary or internal host and get that origin stamped into the registry. Now an advertised issuer is compared, never followed. In probe mode a mismatch fails; in record mode it is reported as NEEDS APPROVAL and the recorder refuses to write it -- including its date, so an unapproved move cannot buy itself another window of silence. That approval path is how the three corrections above were made: the first record run refused all three and printed what to review. Host vetting runs on the host that will actually be DIALLED, not on the raw string. YARL (under aiohttp) IDNA-encodes before connecting, so `https://127。0。0。1` -- with U+3002 IDEOGRAPHIC FULL STOP -- has a raw hostname that is neither an IP literal nor even dotted, yet resolves to `127.0.0.1`. The canonical host is therefore DERIVED FROM YARL rather than re-derived: build a URL and read back `raw_host`, which is the field aiohttp hands its resolver (connector.py). Vetted bytes and dialled bytes are then the same bytes by construction. Re-implementing the encoding was not merely redundant, it was wrong: the stdlib `idna` codec is IDNA2003 while yarl prefers IDNA2008/UTS-46, and they disagree on deviation characters in both directions -- `faß.de` reads as `fass.de` to the stdlib while yarl dials `xn--fa-hia.de`, a Greek final sigma maps to a different A-label entirely, and a ZWJ label the stdlib happily encodes makes yarl refuse the URL outright. A host yarl will not build is refused, and so is one carrying authority punctuation, since yarl would otherwise reinterpret `user@evil.com` as userinfo plus host. Both the registry validator and the probe share that one predicate. Canonicalization is used for vetting ONLY and never touches a compared value: `faß.de` and `xn--fa-hia.de` dial the same host but remain different issuer identifiers, and a test pins that they do not compare equal. ## A stale baseline is not a repo-wide outage A single 90-day threshold over all providers would have failed every PR in the repo on a timer nobody could reset: the nightly cannot refresh the stamps (it is read-only by design) and CI has no network to run the remedy. Three changes: - the PR suite WARNS from day 60 and never fails on age for a dark provider - it hard-fails only past day 90 and only for providers visible to users - the NIGHTLY reports staleness and goes red on it (`--fail-on-stale`, passed only there), so the reminder lands where someone can act ## A fatal error is evidence about the probe, not the providers A crash before the provider loop used to become seven ordinary per-provider failures -- exit 0 for two nights, then a red build blaming all seven. It now exits non-zero immediately and leaves the streak state untouched. Per-provider failures keep the streak semantics they had. ## Transient vs drift, and a lost streak that says so Each provider carries a streak of consecutive failing runs; the job goes red only at three in a row, with night one's failure still in the report. State travels as a build artifact rather than a committed file, so a job making outbound requests never needs `contents: write`. Losing that state is fail-safe but never silent. The document is bound to its contents by a digest, and a state file that is malformed or inconsistent is reported by reason. An ABSENT file is the subtler case, because it means two different things: nothing has ever run, or a previous run's artifact expired. The workflow already knows which -- it looks for a prior run before trying the download -- and now passes that through, so an expected-but-missing artifact is reported as `artifact_missing` while a genuine first run stays null. Otherwise a two-night streak resets with nothing in the report to show it. ## Dates are UTC on both sides `--record` stamps the UTC date, but the future-date guard compared against the LOCAL date, so a user west of UTC could not load a baseline the recorder had just written until local midnight caught up. Both sides now go through one `utc_today()`. Rewriting is style-preserving on purpose. `registry.json` is one compact object per line, which is what keeps a provider edit to a one-line diff a reviewer can read; reformatting would rewrite every line of a security-relevant file. The rewrite goes through the shared `atomic_write` helper rather than `write_text`. An in-place write truncates first, so a full disk or an interrupt partway through leaves a half-written registry -- and this file is parsed at import time, which makes a corrupt one a startup failure for the whole package rather than a failed command. The helper writes a temp file in the same directory and renames, so a reader sees either the old registry or the new one. ## Also Stamp churn is documented as intended: a confirming capture on a later day does move `verified_on`, because the field records when a baseline was last re-derived, not when it last changed. And the stamp is described as a refresh marker rather than a provenance guarantee -- what actually guarantees the baseline is the nightly probe re-deriving it, not a self-attested date. GitLab sends `resource` as an array rather than the string RFC 9728 specifies; the probe tolerates that on a field it only uses to confirm the document describes the right host. The workflow carries a runbook for every deliberate failure mode: refused redirects, refused compression, needs-approval, artifact_missing, and staleness.
…rodotdev#2412) The 0.2.0 section was written in kirodotdev#2305, the commit that became v0.2.0-rc.4. Seventy-one commits have landed on main since, nineteen of them feat:, and the section was never revisited. It therefore both omitted shipped features and described one that no longer exists as written. The wrong entry mattered most: the Webhooks bullet told the reader to manage inbound automation "from Settings", but kirodotdev#2343 moved that page behind a per-device Preview pages toggle under Developer and hides it by default. A 0.2.0 user following the release notes would have gone looking for a page that is not there. Added, all from the rc.6 range: opt-in Slack setup and the multi-channel repositioning (kirodotdev#2340), Telegram multi-account (kirodotdev#2203) and inbound attachments (kirodotdev#2201), sub-agent completions reaching non-Slack parents (kirodotdev#2352), Discord reply continuation (kirodotdev#2326), Slack OPTIONS as a control (kirodotdev#1467), the Agent Templates two-pane inspector, project-local agent discovery (kirodotdev#2167), send-a-copy-to-another-instance, Jira and setting link chips (kirodotdev#2019, kirodotdev#1907), CJK emphasis rendering, the MCP Apps switch (kirodotdev#2293, kirodotdev#2337), the Connections provider registry (kirodotdev#2285), GitHub Enterprise Server support in Code Review Sage (kirodotdev#2154), operator notes on user deny patterns (kirodotdev#2341), the locked git-publish floor rules (kirodotdev#2369), the persist-or-refuse boot guard (kirodotdev#2279), and the turn-ceiling bounds on the approval and stall windows (kirodotdev#2372, kirodotdev#2373). Scope is exactly 5fe4bd5..ab20b4e, the range v0.2.0-rc.6 ships. The three commits main carries beyond rc.6, including meeting deletion (kirodotdev#2268), belong to the next release and are deliberately not described here.
…ance probe (kirodotdev#3261) The provider registry has carried `l0_expectations` since kirodotdev#2285 with no consumer: nothing checked that Notion still uses DCR, that Atlassian's authorization server is still what we think it is, or that any of it was ever true. This is the consumer. A scheduled job fetches each provider's PUBLIC OAuth discovery documents -- protected-resource metadata at its well-known path (RFC 9728) and the authorization-server metadata that names (RFC 8414) -- and diffs what the provider advertises against the committed baseline. Every request is an unauthenticated GET of a static document, so the job needs no repository secret and no provider account. Scope is named in the module docstring, the workflow name and the report itself: this is STATIC METADATA conformance. It does not check challenge shape (does an unauthenticated request answer 401 with a well-formed `resource_metadata` parameter), and a green run says nothing about it -- two of the seven providers fail that today while serving good metadata, so it needs its own baseline of known exceptions and is deferred to L0b. Recut of the kernel of kirodotdev#1329, re-homed onto current main. ## The issuer is an identity, compared code point for code point The baseline field was `authorization_server_origin` and was compared by origin. RFC 8414 §2 makes the issuer an identity compared by simple string comparison, so an origin-only check accepts a tenant or realm substitution -- `/tenant-a` answering for `/tenant-b`. Worse, three of the seven baselines could not even be expressed: stripe https://access.stripe.com/mcp github https://github.com/login/oauth atlassian https://auth.atlassian.com/VCeDsk8ZHncYF1g234fKtc4lNipbBhu3 All three carry a path the old field silently dropped, and the well-known URL at their bare origin returns 404. The field is now `authorization_server` and holds the full issuer, compared VERBATIM. Nothing is normalized away: not host case, not an explicit `:443`, not a trailing slash. Each of those would make two distinct issuer strings compare equal, which is precisely the substitution the check exists to catch -- an authorization server answering for `https://Auth.Example.com` when we committed `https://auth.example.com` has told us something worth surfacing. The one transformation in the module is the single trailing slash removed when CONSTRUCTING the well-known URL, which never touches a compared value. All seven committed baselines were verified to survive exact comparison against both the live advertised value and each issuer's own self-declared `issuer` field. ## The probe never dereferences a provider-supplied URL One invariant holds in both modes: the only authorization-server URL the probe fetches is the one COMMITTED in the registry. Record mode previously dropped the expected-origin guard and then fetched `authorization_servers[0]` -- so a compromised provider could aim the runner at an arbitrary or internal host and get that origin stamped into the registry. Now an advertised issuer is compared, never followed. In probe mode a mismatch fails; in record mode it is reported as NEEDS APPROVAL and the recorder refuses to write it -- including its date, so an unapproved move cannot buy itself another window of silence. That approval path is how the three corrections above were made: the first record run refused all three and printed what to review. Host vetting runs on the host that will actually be DIALLED, not on the raw string. YARL (under aiohttp) IDNA-encodes before connecting, so `https://127。0。0。1` -- with U+3002 IDEOGRAPHIC FULL STOP -- has a raw hostname that is neither an IP literal nor even dotted, yet resolves to `127.0.0.1`. The canonical host is therefore DERIVED FROM YARL rather than re-derived: build a URL and read back `raw_host`, which is the field aiohttp hands its resolver (connector.py). Vetted bytes and dialled bytes are then the same bytes by construction. Re-implementing the encoding was not merely redundant, it was wrong: the stdlib `idna` codec is IDNA2003 while yarl prefers IDNA2008/UTS-46, and they disagree on deviation characters in both directions -- `faß.de` reads as `fass.de` to the stdlib while yarl dials `xn--fa-hia.de`, a Greek final sigma maps to a different A-label entirely, and a ZWJ label the stdlib happily encodes makes yarl refuse the URL outright. A host yarl will not build is refused, and so is one carrying authority punctuation, since yarl would otherwise reinterpret `user@evil.com` as userinfo plus host. Both the registry validator and the probe share that one predicate. Canonicalization is used for vetting ONLY and never touches a compared value: `faß.de` and `xn--fa-hia.de` dial the same host but remain different issuer identifiers, and a test pins that they do not compare equal. ## A stale baseline is not a repo-wide outage A single 90-day threshold over all providers would have failed every PR in the repo on a timer nobody could reset: the nightly cannot refresh the stamps (it is read-only by design) and CI has no network to run the remedy. Three changes: - the PR suite WARNS from day 60 and never fails on age for a dark provider - it hard-fails only past day 90 and only for providers visible to users - the NIGHTLY reports staleness and goes red on it (`--fail-on-stale`, passed only there), so the reminder lands where someone can act ## A fatal error is evidence about the probe, not the providers A crash before the provider loop used to become seven ordinary per-provider failures -- exit 0 for two nights, then a red build blaming all seven. It now exits non-zero immediately and leaves the streak state untouched. Per-provider failures keep the streak semantics they had. ## Transient vs drift, and a lost streak that says so Each provider carries a streak of consecutive failing runs; the job goes red only at three in a row, with night one's failure still in the report. State travels as a build artifact rather than a committed file, so a job making outbound requests never needs `contents: write`. Losing that state is fail-safe but never silent. The document is bound to its contents by a digest, and a state file that is malformed or inconsistent is reported by reason. An ABSENT file is the subtler case, because it means two different things: nothing has ever run, or a previous run's artifact expired. The workflow already knows which -- it looks for a prior run before trying the download -- and now passes that through, so an expected-but-missing artifact is reported as `artifact_missing` while a genuine first run stays null. Otherwise a two-night streak resets with nothing in the report to show it. ## Dates are UTC on both sides `--record` stamps the UTC date, but the future-date guard compared against the LOCAL date, so a user west of UTC could not load a baseline the recorder had just written until local midnight caught up. Both sides now go through one `utc_today()`. Rewriting is style-preserving on purpose. `registry.json` is one compact object per line, which is what keeps a provider edit to a one-line diff a reviewer can read; reformatting would rewrite every line of a security-relevant file. The rewrite goes through the shared `atomic_write` helper rather than `write_text`. An in-place write truncates first, so a full disk or an interrupt partway through leaves a half-written registry -- and this file is parsed at import time, which makes a corrupt one a startup failure for the whole package rather than a failed command. The helper writes a temp file in the same directory and renames, so a reader sees either the old registry or the new one. ## Also Stamp churn is documented as intended: a confirming capture on a later day does move `verified_on`, because the field records when a baseline was last re-derived, not when it last changed. And the stamp is described as a refresh marker rather than a provenance guarantee -- what actually guarantees the baseline is the nightly probe re-deriving it, not a self-attested date. GitLab sends `resource` as an array rather than the string RFC 9728 specifies; the probe tolerates that on a field it only uses to confirm the document describes the right host. The workflow carries a runbook for every deliberate failure mode: refused redirects, refused compression, needs-approval, artifact_missing, and staleness.
Connections: provider registry + consent-URL security
First slice of the Connections integration branch. It carries the two pieces
every later slice depends on — the provider registry and the consent-URL
safety gate — and nothing else. It lands dark:
connections_uiis off onmain, and this PR adds no user-reachable surface.
Problem
Two independent problems, both of which block the Connections launch set from
working at all.
The registry's revoke links were unverified and several were wrong. A card's
"Revoke at <provider>" link is a safety promise: it is the answer to "I want
this grant gone." Three of them did not deliver on it. Notion's link pointed at
www.notion.so/profile/integrations, which does not list MCP clients at all;Stripe's and Vercel's pointed at pages the grant does not appear on. Nothing in
the repo recorded when any link had last been checked, so a link that rotted
after a provider redesign would keep shipping indefinitely and silently.
Every launch provider's consent URL was rejected by our own safety gate. An
OAuth authorization URL legitimately carries high-entropy opaque values —
state, a PKCEcode_challenge, a DCR-issuedclient_id. The genericURL-exfiltration heuristics read those as credential-like payloads, so the
banner failed closed and the user saw
authentication failed: URL contained credential or exfiltration patternwhere the Authorize link should have been.Why it matters
A revoke link that goes to the wrong page is worse than no link: the user
believes they have revoked a grant that is still live. That failure is silent
and only surfaces when it matters.
The consent-URL rejection is a hard blocker — no provider in the launch set can
be connected while it stands. It also has a sharp edge in the other direction:
the obvious fix (exempt OAuth-shaped parameters generally) would punch a
credential-shaped hole through the redactors that protect arbitrary agent and
model output. The exemption has to be narrow enough that it cannot be reached
from any path except consent-URL validation.
Fix
Registry: dated revoke attestations
Symptom — three revoke links sent users to pages that do not list the grant.
Root cause — the links were written from documentation, never click-verified,
and the registry had no field in which a verification could be recorded, so
there was nothing for a test to check or expire.
Change — every entry now carries
revoke_verified_on(an ISO date, validatedas a real date, so
2026-02-31is rejected) andrevoke_verified_note(how thecheck was made — a logged-out HTTP audit cannot see which surface actually lists
a grant, and the note says so). A test fails the build once an attestation ages
past
REVOKE_VERIFICATION_MAX_AGE_DAYS(180), which converts a one-time claiminto a recurring obligation.
The three wrong links are corrected. Notion's case needed more than a new URL:
its grants live on a settings-modal page (
Settings → Notion MCP → All MCP clients) that has no addressable URL, so the entry points at the workspace homeand adds
revoke_manual_pathwith the in-app navigation. That optional fieldexists for exactly this shape — a provider whose settings page is a single-page
app that re-routes after sign-in, where no URL can be made reliable from our
side.
Also in the registry: GitLab is added as the seventh entry (single
mcpscope,no read-only variant — the
gotcha_copydiscloses that the grant can write),l0_expectationspins each provider's advertised authorization-server originplus DCR/PKCE support for the probe that consumes it in a later slice, and an
optional
client_idsupports the one non-DCR provider without loosening therequired-field contract for the rest. GitHub stays launch-gated
(
launch_gate_passed: false) pending its app registration, so six of the sevenentries are visible.
Consent-URL security: one narrow, code-owned carve-out
Symptom — every launch provider's authorize URL was flagged as an
exfiltration attempt.
Root cause — the generic heuristics classify by payload shape. An opaque
statevalue and a PKCE challenge are indistinguishable, by shape, from anexfiltrated secret.
Change —
oauth_url_contains_credentialis a dedicated gate and the onlypath permitted to exempt standard OAuth entropy. Its exemption is keyed on an
exact
(host, path)allowlist that is code-owned, not configurable, and neversuffix-matched — so neither an agent-owned setting nor
api.notion.com.attacker.examplecan lower the ceiling. Explicit ports andplain HTTP are not exempted.
The allowlist includes each launch provider's MCP authorization server,
which is a different host and path from the provider's classic web-OAuth
endpoint. Those pairs are what actually blocked the launch set; each was taken
from the provider's own advertised
authorization_endpoint(RFC 8414 metadata,reached by RFC 9728 protected-resource discovery from the registry's
mcp_url)and corroborated against an authorize URL the runtime actually minted.
Everything else stays fail-closed. Fixed credential signatures are checked
against the raw URL and against a bounded multi-pass percent-decode, so a
double-encoded payload cannot survive. Heavy percent-encoding, userinfo in the
authority, path parameters and fragments are all rejected outright. Only a
structurally valid S256 PKCE challenge at an approved endpoint is subtracted
before scanning, and only after it is itself checked for credential content.
scan_exfiltration_urlsandredact_exfiltration_urlskeep their strictbehaviour for arbitrary agent and model text — they do not inherit the
carve-out, and a test pins that asymmetry.
Known tradeoff, accepted deliberately. Two rules here are imprecise in the
safe direction. The heavy percent-encoding detector runs on the whole path, so a
URL carrying a long non-ASCII filename (CJK, for instance) can be redacted even
though it holds no credential. Decode saturation is refused rather than passed
through, so a pathologically over-encoded URL is refused on structure alone.
Both are false positives in a redactor whose failure modes are asymmetric: a
missed credential leaks a secret into agent-visible text permanently, while an
over-redacted URL costs a reader one link. Neither rule is on the consent path,
so the launch set is unaffected. Tightening either one means classifying by
payload semantics rather than shape, which is the same reasoning that produced
the escape hatch this PR closes — so it is not attempted here.
Tests
test/test_connections_registry.py— registry contract: the launch set isexactly the seven agreed slugs, only ungated entries are visible, every
required field is present and correctly typed,
revoke_verified_onmust be areal ISO date, and a dedicated staleness test fails once any attestation
passes 180 days. Notion's
revoke_manual_pathis pinned to the click-verifiedvalue.
test/test_security.py— newTestOAuthAuthorizationUrlRedaction. Positive:an exact approved authorize URL passes the banner gate. Negative, and this is
the substance of the class: an unapproved host, a suffix-extended host, an
extended path, an explicit port, and a downgrade to HTTP each fail closed. A
planted AWS secret, bare AWS secret run, or GitHub token fails closed even
inside an otherwise-approved URL, in raw and encoded form. The same class
asserts the generic redactors still strip a URL the banner gate accepts,
pinning that the carve-out did not leak into general text handling.
test_credential_surviving_the_decode_budget_fails_closedcovers thedecode-saturation boundary: a credential wrapped in one more percent-encoding
layer than the decode budget allows must still be flagged and redacted. It is
parameterized on
_MAX_URL_DECODE_PASSES, so raising the cap does not satisfyit — only the fail-closed behaviour does. Verified to fail against the
pre-fix code (
scan_exfiltration_urlsreturned no warnings, i.e. the URLpassed through clean).
test_a_benign_singly_encoded_url_is_left_aloneis its positive control: anordinary singly-encoded URL reaches a stable payload in one pass, so the
saturation guard stays silent and the URL is returned unmodified. A
fail-closed rule that fired on normal traffic would be over-redaction.
test/test_security_posture.py(redactorcall-site registration) and
test/test_spawn_audit.py.Local gates: isort, flake8,
mypy src/kiro_crew/, and the full backend pytestsuite. No frontend files are touched, so
tsc/vitestare not applicable.Manual verification
The registry's corrected revoke links and Notion's manual navigation path were
click-verified in a logged-in session against the live providers, which is what
the
revoke_verified_notevalues record — a logged-out HTTP audit cannot seewhich surface lists a grant, and the notes distinguish the two strengths of
check rather than overstating them.
The MCP authorization-server
(host, path)pairs were each confirmed twice: readfrom the provider's own advertised
authorization_endpointvia RFC 8414/9728discovery, and cross-checked against an authorize URL the runtime minted live.
End-to-end consent flows are not exercisable from this PR alone — the gate has
no caller on main until the chat-banner slice lands, which is why the coverage
here is unit-level and adversarial rather than a flow walkthrough.
Screenshots
N/A — lands dark behind
connections_ui, no reachable UI on main.Registry behavior change: Atlassian requests no scopes
recommended_scopesfor Atlassian drops to[]deliberately: no valid mixed Jira+Confluence scope list exists (classic Jira scopes and granular Confluence scopes are incompatible vocabularies), and transmitting any list makes Atlassian reject the consent outright. Requesting nothing falls back to provider defaults — the only consent shape Atlassian accepts for a combined Jira+Confluence grant. The default grant includes write access, which the card'sgotcha_copydiscloses to the user.