fix(security): gate the quadratic sensitive-path scan behind a linear pre-filter

[chenmingwei23]

1. What is the problem?

is_sensitive_bash_command() is quadratic in command length, and it runs
synchronously on the asyncio event loop.

All four alternatives of the sensitive-path matcher's verb/redirect-anchored
branch carried a leading .* (_READ_CMDS.*, _WRITE_CMDS.*,
_SCRIPT_OPEN.*, .*[<>|]\s*). The eleven verb-independent branches had theirs
removed for exactly this reason -- the comment there still says "Do NOT
reintroduce .* here" -- but this branch was not touched, so the engine retried
from every start offset. Measured on main, on a clean command (the common
case, and the only one that runs to completion, since a match returns early):

command length	is_sensitive_bash_command
9 KB	1.09s
18 KB	3.99s
37 KB	16.03s
56 KB	36.54s
75 KB	65.11s

4x per doubling across eight points. Profiled, re.search is 94% of the gate and
99.5% of that is this one matcher, so the tokenizing passes are not involved.

A second, independent multiplier compounds it. Pass 1b re-scans one
separator-collapsed variant per spelling, and _SEPARATOR_RUN_RE matches any
doubled separator -- including the // in an ordinary URL. Same command, same
length, single slash instead of double: 4.58s against 16.06s at 37 KB.

2. Why this issue matters to the user

The gate sits on the always-enforced deny path in _resolve_permission, so it is
evaluated on every tool call on every surface, and it is evaluated inline on the
event loop. A single oversized command therefore does not fail one tool call -- it
freezes the whole gateway, and then kills it. LoopStallWatchdog arms
faulthandler.dump_traceback_later(..., exit=True), so a slow-but-finite regex
becomes a process exit and a systemd restart:

• past LOOP_STALL_EXIT_AFTER_DEFAULT (25s) at about 45 KB of command
• past LOOP_STALL_EXIT_AFTER_MANAGED_DEFAULT (90s) at about 87 KB

Reported as a gateway restarting roughly every two hours, the cadence matching two
cron jobs whose agent turns emit long curl pipelines with multi-KB JSON bodies --
i.e. commands that are both long and full of URLs, which is the worst case for both
multipliers at once. Every restart kills the in-flight cron turn, and any dashboard
or Slack session live at the time freezes for the watchdog budget and then dies with
the process.

Command text is agent-authored under the influence of untrusted external content,
so this is reachable deliberately and not only by accident.

3. How our fix solves it

Chaining from the symptom back to the root cause:

gateway exits every ~2h <- watchdog sees the loop silent for its whole budget
<- the gate holds the loop for 60s+ inside one call <- the matcher is retried
from every start offset <- branch (1) carries a leading .\* -- and, on top of
that, the same scan is paid four times because a URL's // produces three
collapsed variants.

Two complementary changes, because they bound different cases and neither alone is
enough.

(a) Remove the redundant .* from the redirect alternative (.*[<>|]\s* ->
[<>|]\s*). This is the same redundancy the earlier round removed from the other
eleven branches, and the same argument: the pattern is only ever used via
.search, which already retries at every offset, so .*[<>|]\s*P matches a
subject exactly when [<>|]\s*P does -- a match of the former from any offset
implies the metacharacter sits at some offset the search also visits.

This one was the dominant term. Unlike the three verb alternatives beside it,
which only enter where a verb actually matches, it entered at every offset
unconditionally. The three verb alternatives keep their .*, since there it spans
a meaningful gap between the verb and the path; they cost O(verb occurrences x n)
and measure negligible even on a subject that is nothing but cat invocations
(0.015s at 53 KB).

(b) Run pass 1 and pass 1b behind a cheap superset pre-filter, the device this
module already uses for credential redaction (_might_contain_credential) and with
the same contract: it may match where the pattern would not, but it must never fail
to match where the pattern would, so only the cost can move, never the verdict.

The pre-filter is not a hand-written literal list. It is the pattern's own branch
list, assembled in the same function from the same locals, with exactly one
substitution: branch (1), verb_or_redirect_anchor + sensitive_path, is replaced by
a bare sensitive_path. Dropping a required prefix can only admit more
subjects, so that alternative matches wherever branch (1) did. Every other
alternative -- the eleven token-anchored tails and the two anchor-free ones -- is
passed through character for character, so it matches exactly where the pattern's
own branch does. No case is left for the pre-filter to miss.

The token anchor is deliberately kept on those eleven, and that detail is
load-bearing. Dropping it would also have been a sound superset by the same
"required prefix" argument -- and the first version of this PR did drop it -- but it
lets the engine enter the WINDOWS tails at every offset, and those tails contain
win_gsep, a starred group. A UNC-style backslash run then backtracks through it:

backslash run	anchors dropped	anchors kept
0.5 KB	0.066s	0.000s
2 KB	1.019s	0.002s
4 KB	4.055s	0.003s
10 KB	25.446s	0.008s

25.4s is past the 25s watchdog, so that version had introduced the very crash this
PR exists to prevent, on a different subject -- the same DoS family already recorded
on win_gsep itself, where admitting a separator run into the patterns measured 33s
on 6,000 backslashes. Found by the GPT review lane; the anchored form is cheaper on
every other subject measured too, so nothing is traded for it.

Both patterns are returned from one _build_sensitive_patterns() call and cached
together by _build_sensitive_cache(), so the pre-filter cannot come to describe a
stale pattern or a stale Path.home(). _names_fenced_path() is the single place
the two are paired and is now the only caller of _get_sensitive_re(), so no call
site can reach the expensive scan without the linear check in front of it. The
pre-filter is applied to each collapsed subject rather than inferred from the
original, since collapsing rewrites the text.

Why both are needed: (b) removes the common case -- a command naming no fenced
path is not scanned even once, which is what the reported crash was made of -- while
(a) bounds the worst case. A subject the pre-filter admits but the pattern
rejects reaches the scan by design (a fenced path glued to a name character, with no
verb or redirect anywhere: abc~/.aws/credentials), and with the .* still present
that path was itself 4x per doubling. Design Review raised exactly this residual;
(a) closes it.

Results, all linear now:

subject	before	after
clean, 37 KB	16.03s	0.385s
clean, 75 KB	65.11s	0.752s
clean, 309 KB	~287s	2.814s
adversarial near-miss, 2.6 / 5.4 / 11 / 23 KB	0.021 / 0.086 / 0.350s (4x)	0.016 / 0.028 / 0.055 / 0.117s (2x)
10 KB backslash run	(0.006s on main)	0.024s

The pre-filter's own cost is 0.021s at 75 KB. Beyond about 49 KB an adversarial
near-miss is refused outright by the pre-existing traversal-analysis budget in pass
4 ("requires more traversal analysis than this gate performs (4096 units)"), which
is fail-closed -- so the window is bounded at both ends.

One thing I deliberately did not do: moving the gate off the event loop
looks like the obvious fix and is not a sufficient one. Measured with a 50 ms async
heartbeat against the real gate on the 37 KB subject: inline, the worst heartbeat gap
is 16.21s; via asyncio.to_thread, 4.08s. CPython's re holds the GIL for long
stretches, so a thread hop is a ~4x mitigation that moves the crash to a larger
command rather than removing it. Worth doing as defence in depth, separately.

4. What tests we did

New tests live in test/test_security_regex_linearity.py rather than a new file,
because that file already covers the earlier round of this same defect -- the change
that removed .* from the other eleven branches. This is that defect one branch
over, so it belongs in the same differential.

• test_prefilter_never_misses_what_the_pattern_matches -- the pre-filter's safety
  contract over a 25-case corpus spanning every branch family: POSIX and
  Windows-native spellings, the %APPDATA% / %LOCALAPPDATA% / !APPDATA!
  aliases, the home-anchored AppData\Roaming form the alias branch does not
  cover, the variable-leaf %USERPROFILE%\.kiro\crew\%F% shape, a quoted C:\Users
  leaf, a UNC anchor, $KIRO_HOME and %KIRO_HOME%, both anchor-free bare leaves,
  an upper-case spelling, and the two shapes only branch (1) matches. Those two
  are what prove replacing branch (1) with its bare tail is safe.
• test_a_backslash_run_does_not_blow_up_the_prefilter -- the pre-filter runs on
  every command, so it has to be safe on hostile input, which the first version of
  it was not. A 10 KB backslash run through the real gate, bounded absolutely (2s
  against a 0.024s fixed path) rather than by a doubling ratio, following the
  existing test_long_nonshell_line_does_not_blow_up beside it. The structural half
  -- that the eleven anchors are still present at all -- is asserted in the anchor
  test, so dropping them again fails loudly instead of showing up as a crash dump.
• test_removing_the_redirect_wildcard_is_match_set_identical -- the differential
  for change (a). It reconstructs the pre-change pattern by putting the .* back
  and asserts the two agree on every case in both corpora plus negatives, rather
  than asking the reader to trust the argument.
• test_redirect_glued_paths_are_still_blocked -- all six shapes that alternative
  exists for and no other branch matches: >~/path, >>, 2>, <~/path,
  |~/path, and a piped tee.
• test_gate_verdicts_unchanged_by_the_prefilter -- the corpus end to end through
  the real gate.
• test_clean_command_does_not_reach_the_quadratic_pattern and
  test_a_url_no_longer_multiplies_the_expensive_scan -- the cost fix, asserted by
  counting scans of the expensive pattern via a proxy: exactly 0 where it used to
  be 4. Deliberately not a wall-clock ratio: ReDoS doubling-ratio test asserts linearity the matcher does not have: is_denied is O(length x occurrences) #3080, flaky: ReDoS linearity test asserts a wall-clock ratio and fails on shared runners #4108, Flaky: ReDoS doubling-ratio test fails on unrelated frontend PRs (3.13x and 3.31x against a 3.0 bound) #3938 and Windows shard 2: the ReDoS doubling-ratio assertion is perturbed by in-process CPU bursts (1 in 145) #2811 are
  four separate flakes caused by timing assertions on this very module, and a count
  is exact on every runner.
• test_a_fenced_command_still_reaches_the_pattern -- the pre-filter admits a real
  candidate instead of deciding for itself.
• test_prefilter_is_built_from_the_same_home_as_the_pattern -- with Path.home()
  patched to a home under neither Users nor home, so only the resolved literal
  can match; this is the case where a pre-filter built from a second reading of the
  constants would disagree.
• test_sensitive_anchor_has_no_leading_wildcard rewritten: it asserted a
  source-text count of the anchor spelling, which this change necessarily breaks
  because the anchor is now written once and applied to a tuple. It now asserts
  against the compiled patterns -- no .* anchor, the anchor present exactly 11
  times in the pattern and 11 times in the pre-filter, 14 pre-filter
  alternatives, the pre-filter free of .*, and the redirect alternative present
  but without its .*. Strictly stronger than what it replaced.

Gate: 2523 passed / 1 skipped across test_security_regex_linearity,
test_security, test_hooks, test_mcp_cron_security,
test_governance_self_protection, test_security_alt_traversal,
test_llm_helpers_tool_input_offload, test_connections_tool_aliases,
test_exfil_gate_opt_out; and 2290 passed across all 20 test files that
exercise this gate. flake8, isort, black and mypy clean on the changed
files. (black reports security.py as needing reformatting on pristine main
too in my environment -- a target-version mismatch, verified as a pre-existing
baseline; its diff does not touch any region this PR changes.)

Separately from the test suite, the superset property was re-verified over 46 cases
plus every separator-collapsed variant of each, driven from the real builder output
rather than a reconstructed pattern.

The spec moved with the code: docs/system-specs/modules/security.md records both
changes, the measurements, the URL multiplier, the residual and its bound, and the
verdict-neutrality guarantee.

5. Any other suggestions on the work

Three review items are already applied in this branch rather than deferred, because
two of them were load-bearing:

• GPT review (blocking) found the pre-filter itself quadratic on a backslash
  run. Reproduced at 25.4s on 10 KB, fixed by keeping the token anchor on the
  eleven anchored tails, as the finding proposed. Measurements above.
• Design Review (concerns) found the pre-filter alone left the adversarial case
  open. Correct, and closed by removing branch (1)'s redundant .* -- which turned
  out to be the better half of the fix, since it bounds the worst case rather than
  just the common one.
• First Principles (pass, one subtraction) asked for _build_sensitive_regex to
  go, since it had no src/ consumers. Deleted; its one caller now uses
  _build_sensitive_patterns()[0].

Two follow-ups remain, neither blocking:

1. The three scanners on the event loop are still on it. Credential redaction + deny scan runs on the event loop on every tool call #3056 named
   is_sensitive_path, is_sensitive_bash_command and is_denied as synchronous
   scanners in this chain and closed after moving only the tool_input scan to a
   thread; _resolve_permission still calls all three inline, a few lines above its
   own await asyncio.to_thread(...). Given the GIL measurement above this is
   defence in depth rather than a fix, but the asymmetry inside one function is
   worth closing.
2. The watchdog could degrade instead of exiting. When the stalled frame is
   inside the permission gate specifically, cancelling the offending task (denying
   the tool call) preserves the loop; dump-then-exit is the right last resort for a
   genuinely dead loop, not for a slow one.

A note on the original report, since it is quoted in the issue: it diagnosed
"catastrophic backtracking" and expected commands of a few KB to cross the 90s
budget. The blowup is quadratic, not exponential, and a few KB is 0.03-0.2s --
the real threshold is tens of KB. That distinction is why this PR removes a
redundant quantifier and gates the rest, rather than reaching for a linear-time
engine or a timeout.

Pattern harvest

Rule candidate: lint over re.compile pattern sources under src/
Pattern: an alternative that begins with .* in a pattern only ever used via
.search

.search already retries at every start offset, so a leading .* on an
alternative matches nothing extra while making the scan quadratic in the longest
line. It is a one-token mistake with no upside and a gateway-killing downside.

This is generalizable, and the evidence that a comment is not enough is this
module's own history: the earlier round removed this construct from eleven branches
and left the warning "Do NOT reintroduce .* here" directly above the twelfth
instance, which survived that round and is what this PR removes. Same file, same
defect, second time.

The check needs no execution: for a compiled pattern reached only through
.search, flag any top-level alternative whose source starts with .*.
test_sensitive_anchor_has_no_leading_wildcard now asserts exactly this on the
compiled patterns for this one matcher -- promoting it to a repo-wide lint over
every re.compile in src/ is the harvest, and would have caught this instance
without anyone measuring a crash first.

Fixes #8338

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root-cause fix with a verdict-neutral pre-filter proven by differential tests, drift-proofed by single-source construction, alternatives measured and rejected on evidence.

[DESIGN-REVIEWED] 501439c

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 501439c351ed3403d5ff091791625c338c0b1782 — 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.

Verification complete. All counted claims check out: _names_fenced_path is the sole production caller of _get_sensitive_re with exactly 2 consumers (security.py:8742, :8773); the pre-filter device has a named in-repo precedent (_might_contain_credential, security.py:11956, same superset contract); the diff's items are all declared in the description, and the fix chain (watchdog exit ← loop wedge ← quadratic scan ← leading .*) terminates at the actual cause. Final review follows.

First-Principles-Verdict: PASS

A reported crash, chased to its cause, fixed with the module's own existing device, verdict-neutrality pinned by differential tests — every item earns its place.

What this change ships

Intent: stop long clean commands from wedging the event loop until the stall watchdog kills the gateway. FIX.

1. Long redirect-free commands no longer scan quadratically — the redundant .* removed at its cause — justified
2. Commands naming no fenced path skip the expensive scan entirely (new pre-filter) — justified, declared, reuses the _might_contain_credential contract
3. Pre-filter and pattern built and cached in one call, so they cannot drift — justified (stale-home hazard, pinned by test)
4. Pattern construction restructured into one anchor applied to a tail tuple — declared, load-bearing for the no-drift guarantee
5. Spec bullet in security.md updated same-commit — mandated by AGENTS.md
6. Safety corpus + scan-count tests replacing wall-clock assertions — justified (four prior flakes cited)

No undeclared items; nothing moved, relabeled, or defaulted differently. Consumer counts: _names_fenced_path 2 (both passes of is_sensitive_bash_command); all new symbols module-private. The earlier round fixed the 11 sibling branches; this is the last one, and the remaining verb .*s are kept for a stated, measured reason.

Subtractions

• Shrink the _build_sensitive_prefilter docstring and the four security.md bullets to present-tense invariants: the before/after timing tables, #8338/#8349, and "Found by the GPT review lane" are the task-log narration AGENTS.md's comment rule forbids, and stale-by-construction.
• Fold _build_sensitive_prefilter(alternatives) into _build_sensitive_patterns returning two compiled patterns — the tuple-of-strings seam has 1 production consumer (_build_sensitive_cache, security.py:7647) and every caller passes the same value.

[FIRST-PRINCIPLES-REVIEWED] 501439c

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've independently verified the load-bearing properties of this change:

1. Prefilter superset holds — branch (1) (verb_or_redirect_anchor + sensitive_path) is replaced by bare sensitive_path (dropping a required prefix admits strictly more); the 11 token-anchored tails and 2 anchor-free tails pass through character-for-character. So _names_fenced_path cannot move a verdict, only cost.
2. .* removal is match-set identical — under .search, if .*[<>|]\s*P matches from any offset, the [<>|] sits at some offset .search also visits, so [<>|]\s*P matches there. No newline concern (no DOTALL).
3. No new ReDoS on the bare unanchored prefilter branch — sensitive_path = home_alts/(?:dirs)path_end; its only quantifier is [^/\s]+ on the generic-home alternatives, a single non-nested + on a character class followed by a required literal /. Linear. The Windows tails (the win_gsep starred group) correctly retain their token anchor, which is the actual backtracking hazard.
4. Single build — _build_sensitive_cache populates both caches from one _build_sensitive_patterns(), so the prefilter can't describe a stale home or pattern.

Both call sites (pass 1, pass 1b) were converted consistently; other _get_sensitive_re() callers still receive the same-verdict pattern. Nothing groundable at 80+.

No findings.

[OPUS-REVIEWED] 501439c

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 501439c351ed3403d5ff091791625c338c0b1782 and found no blocking issues.

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/security.py:7487 -- “needs to strip exactly this prefix” contradicts the implementation, which preserves it on eleven tails -> Fix: say the pre-filter keeps this prefix. (origin: validation)
[GPT-REVIEWED] 501439c

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

[chenmingwei23]

GPT's blocking finding was correct, reproduced, and is fixed in 501439c35. Posting the measurements since the fix changes the shape of the superset argument.

Reproduced. Dropping token_anchor from the eleven tails let the engine enter the WINDOWS tails at every offset, and those tails contain win_gsep -- (?:sep(?:\.|[^\\/\s'"]{1,64}sep\.\.))*sep, a starred group. A UNC-style backslash run backtracks through it, quadratically:

10KB-approaching backslash run	pre-filter, anchors dropped	pre-filter, anchors kept
0.5 KB	0.066s	0.000s
1 KB	0.258s	0.001s
2 KB	1.019s	0.002s
4 KB	4.055s	0.003s
10 KB	25.446s	0.008s

So the severity call was right too: 25.4s is past LOOP_STALL_EXIT_AFTER_DEFAULT, meaning the pre-filter had introduced the very crash it exists to prevent, on a different subject. It is also the same DoS family this module already records one function up, where admitting a separator run into the patterns measured 33s on 6,000 backslashes.

Applied the proposed remedy as given: keep token_anchor on the eleven token-anchored tails, unanchor only sensitive_path plus the two already anchor-free tails. Through the real gate the 10KB run is now 0.024s and linear (0.005s / 0.024s / 0.048s at 2 / 10 / 20 KB), and the clean-command figures are unchanged (0.385s at 37 KB, 0.752s at 75 KB).

It also simplifies the superset argument, which is worth stating because the previous one was doing more work than it needed to. The pre-filter is now the pattern's own branch list with exactly ONE substitution: branch (1), verb_or_redirect_anchor + sensitive_path, becomes a bare sensitive_path. Dropping a required prefix can only admit more, so that alternative matches wherever branch (1) did; every other alternative is passed through character for character and so matches exactly where the pattern's own branch does. No case is left to miss -- and the anchored form is cheaper on every subject measured, so nothing was traded for it.

Re-verified the superset over 46 cases plus every separator-collapsed variant of each, using the real builder output rather than a reconstructed pattern. Five Windows spellings that were only in my scratch corpus are now in the in-repo one: the home-anchored AppData\Roaming form (which the %APPDATA% alias branch does not cover), the variable-leaf %USERPROFILE%\.kiro\crew\%F% shape, a quoted C:\Users leaf, %KIRO_HOME%, and !APPDATA!.

New guard: test_a_backslash_run_does_not_blow_up_the_prefilter, plus a structural assertion in test_sensitive_anchor_has_no_leading_wildcard that the eleven anchors are still present -- so dropping them again fails loudly rather than showing up as a crash dump. The time bound there is absolute and generous (2s against a 0.024s fixed path) rather than a doubling ratio, following the existing test beside it: ratio assertions on this module are the direct cause of four separate CI flakes (#3080, #4108, #3938, #2811).

Two other review items in the same push: Design Review's residual (a subject the pre-filter admits but the pattern rejects still ran the quadratic scan) is closed by removing the redundant .* from branch (1)'s redirect alternative, and First Principles' subtraction is applied (_build_sensitive_regex deleted; it had no src/ consumers).

Unrelated red: Dependency Audit fails with npm audit timed out after 120s for website/package-lock.json, failing closed on a tool timeout. It fails the same way on other open PRs (#5539, #8275) and passed on #7350, and this PR touches no dependency file.

[chenmingwei23]

Closing as superseded by #8282, which was opened about 2.5 hours before I filed #8338 and which I did not find because I searched existing issues but not open PRs.

#8282 covers strictly more of this defect:

• it removes the same redundant .* from the redirect alternative, with the same reasoning
• it goes on to restructure the verb-anchored branch into a linear per-line search, which this PR deliberately deferred
• it adds a fail-closed size ceiling
• it also solves the half this PR never touched: attributing a stall to the cron job that caused it, so doctor can name the right job instead of the operator pausing the wrong one

The one mechanism unique to this PR is the superset pre-filter, and with the verb-anchored branch restructured the quadratic term it was gating is gone, so it would be an optimisation on an already-linear path rather than a fix.

The findings worth keeping are posted on #8282 instead: a measured lead that its 20 KB ceiling may still admit a loop-wedging subject (an unanchored path search over a backslash run measured 91s at 19 KB in a model of that code path), a regression guard for that shape, and the case for linting this defect class repo-wide.

Issue #8338 is closed as a duplicate. No hard feelings toward the work -- the review lanes on this branch were genuinely useful: GPT caught that my pre-filter had itself introduced a 25s stall on a backslash run, which is exactly the lead now sitting on #8282.