fix(security): fence repeated Windows path separators (#6350)

[chenmingwei23]

What is the problem?

Win32 collapses a repeated path separator, so %LOCALAPPDATA%\\kiro-cli and
%LOCALAPPDATA%\kiro-cli are the same directory. The shell-side credential
fence (is_sensitive_bash_command) matches raw command text and accepted
exactly ONE separator per boundary, so a doubled separator anywhere in the
chain named the fenced store while matching no branch of the matcher:

spelling	before	after
type "%LOCALAPPDATA%\kiro-cli\data.sqlite3"	blocked	blocked
type "%LOCALAPPDATA%\\kiro-cli\data.sqlite3"	permitted	blocked
type "%LOCALAPPDATA%\\\kiro-cli\data.sqlite3"	permitted	blocked
type "%LOCALAPPDATA%//kiro-cli/data.sqlite3"	permitted	blocked
type "C:\Users\me\AppData\\Local\kiro-cli\data.sqlite3"	permitted	blocked
cat "C:\Users\me\\.aws\credentials"	permitted	blocked
echo x > "C:\Users\me\.kiro\\agents\evil.json"	permitted	blocked

Two of those rows go beyond what #6350 reported, and both matter for how the
issue is sized:

• .aws and .ssh do leak. The report concluded they stay blocked, but it
  only doubled the separator before the leaf, which the trailing boundary
  already absorbs. Doubled after the anchor they were permitted.
• The ~/.kiro/agents write gate leaks too. That gate is a code-execution
  boundary, not a read fence: a spec planted there becomes a
  KIROCREW_MCP_TARGET_<SERVER> command the gateway execs outside the
  per-session sandbox.

Measured on the base commit with a 34-row matrix: 22 fenced spellings were
permitted, 0 benign paths refused.

Why this issue matters to the user

The fenced directories hold kiro-cli's live SSO bearer token, and this gate is
what stops read-only shell auto-approval and the agent file tools from reaching
them. An agent that reads data.sqlite3 holds that token. The bypass needs no
unusual privilege and no symlink, only an extra backslash in a path the agent
was already going to write, and the doubled spelling is not one a human reviewer
reads as suspicious. For the agents-dir row the consequence is a persistent
unsandboxed command running as the user rather than a credential read.

How our fix solves it

Symptom: a doubled separator at an inter-segment boundary is permitted while
the single-separator spelling of the same file is blocked.

Mechanism: win_gsep, the generalized separator, ended in exactly one
win_sep, and its repeat group accepted only the canonical no-ops \. and
NAME\... A bare extra separator matches neither, so the chain simply failed
one segment in. A doubled separator immediately before the leaf still blocked,
because the trailing boundary (?:{win_sep}|\s|$|['"]) absorbs one -- which is
why the gap looked narrower than it was.

Root cause: the separator was defined as "exactly one" in one shared place.
win_gsep is consumed by the fenced-dirs pattern, the home-anchored path, the
%APPDATA% / %LOCALAPPDATA% remainders, the write-protected prefixes and
leaves, the crew variable-leaf parents and the agents dir, so every store branch
inherited the same hole at once. This is fixed at that definition rather than
per spelling:

• win_seps (one-or-more) replaces the single separator at every join site
  inside a path. The trailing boundary deliberately stays single: it
  only has to observe that the path ended, and the rest of a run is text it
  never consumes.
• The other sites that spelled a separator directly rather than through
  win_gsep are moved over too, or the class re-enters one segment to the side:
  the generic drive-letter home anchor (C:\\Users\\u), the resolved home
  literal, the %APPDATA% / %LOCALAPPDATA% anchor-specific \..\Roaming and
  \..\Local excursions, the crew variable-leaf join, and the anchorless
  relative-traversal matcher with its segment alternation.
• The resolved home literal was re.escape()d whole, so a run inside the anchor
  itself escaped it. It is now rendered through the run. generic_win_home
  already covers the Users / home shapes; a home elsewhere
  (D:\profiles\u) has only this literal to match on.

Over-matching is the safe direction for this gate (naming a fenced path is the
signal), but this change also touches the write-protection gate, where
over-matching would refuse a legitimate write. That is why every run shape is
paired with benign controls in the tests.

On backtracking: the two separator classes are disjoint from the name run
([^\\/\s'\"]{1,64}, which excludes separators) and from ., so admitting
one-or-more introduces no new quantifier ambiguity, and the name run keeps its
length cap. A pathological-input test pins the decision time.

What tests we did

New TestWindowsSeparatorRuns in test/test_security.py (134 cases),
asserted as a matrix -- every anchor x every run shape x every boundary
position -- because closing these one spelling at a time is what produced the
current shape:

• a run right after each anchor (C:\Users\u, %USERPROFILE%,
  $env:USERPROFILE, ~, $HOME) for .aws and .ssh;
• a run at every inter-segment boundary of every multi-segment fenced dir,
  parametrized off _SENSITIVE_HOME_DIRS so the matrix cannot drift from the
  list;
• the %APPDATA% / %LOCALAPPDATA% branches including their own excursions;
• runs composed with the canonical no-ops (\., X\..);
• the anchorless relative-traversal spelling;
• the write gates: agents dir, $env:KIRO_HOME, the crew variable leaf, and
  every entry of _WRITE_PROTECTED_BASH_LEAVES;
• the resolved-home-literal anchor, built through _build_sensitive_regex()
  under a patched non-Users home -- a pure function of the home, so no module
  cache is disturbed;
• benign controls that must stay permitted, including .awsx and agentsx
  (a name that merely starts with a fenced one) and an ordinary project path
  with a doubled separator;
• a decision-time bound on pathological separator runs.

Mutation-verified against the base commit: 131 of the 134 fail without the
security.py change. The 3 that pass on base are the two guards that are
supposed to (the benign controls and the timing bound) plus one leaf
parametrization that a separate anchorless name rule already covered.

Gates run: pytest test/test_security.py (1129 passed, 1 skipped),
test_hooks.py, test_governance_self_protection.py,
test_connections_tool_aliases.py, test_app_sources_write_protection.py,
test_app_registry_source_seams.py (626 passed), test_trust_reads.py,
test_mcp_cron_security.py, test_computer_use_api.py, test_aws_consent.py,
test_workflows_library.py (458 passed). flake8, isort, mypy and black clean on
both touched files. Decision time on the pathological inputs is unchanged or
better than base (two of the four 4KB probes drop from ~750ms to ~0.3ms because
they now match early instead of scanning to the end).

What is NOT verified, stated plainly because this is a security control:
there is no Windows host in this environment. Nothing here observed Win32
actually collapsing a separator run; that equivalence is taken from the platform
contract, and what these tests pin is the matcher's side of it. That is sound
for this change because the fence is a pure text function -- the raw pass
never reads os.name, so every assertion above runs and means the same thing on
the Linux, macOS and Windows CI shards. No filesystem resolution is involved,
which is exactly what distinguishes this class from the 8.3 short-name class in
#5265.

Any other suggestions on the work

• Scope: this PR closes Repeated path separator bypasses the Windows kiro-cli credential fence #6350 only, not Windows path-normalization forms bypass the shell credential fence (trailing dot, 8.3 short name) #5265. They are siblings in the same
  code span and Repeated path separator bypasses the Windows kiro-cli credential fence #6350 suggests handling them together, but Windows path-normalization forms bypass the shell credential fence (trailing dot, 8.3 short name) #5265 already has an
  open PR (fix(security): fence trailing-dot and 8.3 Windows spellings (#5265) #5336) that rewrites the same win_gsep statement for the
  trailing-dot and 8.3 classes. Doing both here would have duplicated live work.
  The two changes are complementary and the textual overlap is one statement, so
  whichever lands second rebases mechanically; fix(security): fence trailing-dot and 8.3 Windows spellings (#5265) #5336's win_gsep still ends in
  exactly one separator, so it does not close this class on its own (verified
  against its head d19f8141).
• The deeper direction both issues point at is direction 1 from Windows path-normalization forms bypass the shell credential fence (trailing dot, 8.3 short name) #5265: extract
  path-shaped tokens and compare the resolved path against the fenced set.
  Worth noting that most of that engine already exists on main --
  _shape_path_token collapses separator runs, strips trailing dots and spaces,
  and nets .. -- but it is only wired into the pass-3 cwd-walk scan. Reusing it
  for the whole fence is the real end state and is an architecture change to a
  security boundary, so it wants its own design ticket rather than riding along
  here.
• The {1,64} cap on the excursion name run is untouched by this PR and is
  defeatable by a 65-character segment. It is pre-existing and is being addressed
  in fix(security): fence trailing-dot and 8.3 Windows spellings (#5265) #5336.

Closes #6350

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound fix (canonicalize the subject once, linearly), but the PR description documents the rejected pattern-widening approach, not the code that shipped.

Watch

• Description↔diff mismatch on the mechanism: the description says the fix is "win_seps (one-or-more) replaces the single separator at every join site" and that "admitting one-or-more introduces no new quantifier ambiguity" — but the diff contains no win_seps; it ships _separator_collapsed_variants + pass 1b, and its own comments state the pattern approach "was tried first and is a denial-of-service on this very gate... 33s against 1.3s... past the gateway's 25s watchdog". Anyone auditing this security fence from the PR record will reconstruct the wrong mechanism and the wrong performance claim. Update the description before merge.
• Pass 1b canonicalizes only the pass-1 matchers; passes 2/3 rely on _shape_path_token already collapsing runs. If pass 2's normalizer path does not (unverified here), a run-spelled path caught only by pass 2 remains a residual gap — worth one confirming test or a note in the follow-up ticket the PR already proposes.

[DESIGN-REVIEWED] b0f2bd0

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of b0f2bd004448ee25c965075b591f3405718c650f — 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 three raw-text matchers are consumed only inside is_sensitive_bash_command, so pass 1b covers every call site — no unfixed sibling consumers. The fix itself is cause-level and minimal; the one real finding is that the PR description narrates a mechanism the diff explicitly abandoned. Final review:

First-Principles-Verdict: CONCERNS

The fix is cause-level and every piece earns its place, but the description narrates a pattern-level fix the diff explicitly rejected as a DoS.

What this change ships

Intent: stop a doubled Windows path separator from slipping past the credential/write fence (#6350). This is a FIX.

1. Repeated-separator spellings of fenced credential paths now refused — justified (Repeated path separator bypasses the Windows kiro-cli credential fence #6350, keystone invariant).
2. Same for the ~/.kiro/agents and crew write gates — justified, declared.
3. Same for relative-traversal spellings (..\\.aws\…) — justified.
4. Archive extraction into the governance root via a doubled separator now refused — undeclared in the description's visible text; part of the fix, not a rider.
5. Commands containing a run cost up to 4 extra matcher passes — justified; zero cost on the common command, and the pattern-level alternative was measured as a watchdog-crossing hang.
6. UNC leading pairs survive collapsing — justified, counterexample pinned in tests.
7. Mixed-separator runs emit both collapse spellings — justified, counterexample pinned.
8. 134-case test matrix parametrized off _SENSITIVE_HOME_DIRS, plus a decision-time bound — justified.

No new public surface: all three added names are module-private with exactly one consumer each (is_sensitive_bash_command, security.py:6983,6933; grepped _separator_collapsed_variants|_SEPARATOR_RUN_RE|_PATH_TOKEN_BOUNDARY, 0 external hits). The named sibling class (#5265, 8.3/trailing-dot) is deferred to open PR #5336 — accepted.

Watch

• Description contradicts the diff on mechanism: it says "win_seps (one-or-more) replaces the single separator at every join site" and "the resolved home literal … is now rendered through the run", while the shipped hunk says "The patterns below deliberately still spell ONE separator, and the run is collapsed in the SUBJECT instead." No win_seps exists in the diff, and its measured claims ("131 of the 134 fail without the change") describe that earlier revision. Two comments (security.py:5818, 6798) still point at _collapse_separator_runs, a helper that doesn't exist — the shipped name is _separator_collapsed_variants. The job is unchanged and genuinely done, so this is a stale narrative, not a false purpose — but a human should re-verify the mutation and timing claims against the collapse implementation before merging.

[FIRST-PRINCIPLES-REVIEWED] b0f2bd0

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No blocking issues — one advisory over-block regression in the new separator-collapse pass.

FINDING — src/kiro_crew/security.py:6883 — because _PATH_TOKEN_BOUNDARY includes :, Pass-1b rewrites a URL scheme's :// into a UNC pair :\\, so curl https://raw.githubusercontent.com/user/dotfiles/main/.npmrc (or any fetch URL containing a /.aws/, /.ssh/, /.netrc/… path segment) collapses to https:\\…/.npmrc, matches unc_prefix → win_gsep → win_dirs → win_path_end in win_sensitive_path, and a legitimate HTTP fetch that reads no local file is refused as "command accesses sensitive credential path" (allowed on base) → Fix: drop : from the keep_leading_pair boundary test (a genuine UNC anchor is only preceded by start-of-string/whitespace/quote, never by a scheme:), rather than reusing the full _PATH_TOKEN_BOUNDARY set for the leading-pair decision.

[OPUS-REVIEWED] b0f2bd0

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] b0f2bd0

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

[bolichen97]

Readiness passed on the current head, all checks green, mergeable against main, and no outstanding change requests from any reviewer. Approving as part of a maintainer sweep of fix-type PRs.

[chenmingwei23]

The rebase onto main brought a new BLOCKING finding, and it is REAL. Fixed in 3a4e31094 by changing the approach, not by reverting.

BLOCKING -- separator runs can crash the gateway. ACCEPTED, REPRODUCED, and the mechanism was mine. I measured it rather than arguing from the earlier round's numbers, because the rebase moved the surrounding code:

bare backslashes in one command	base (main)	rebased head e9013c4
4,000	592 ms	14,822 ms
6,000	1,339 ms	33,054 ms
8,000	2,405 ms	58,816 ms

So 6,000 backslashes crossed the 25s watchdog exactly as reported. My earlier comment claiming the disjoint character classes kept this linear was wrong: disjointness bounds the NAME run, not the separator run. {win_sep}+ appears inside the starred generalized separator AND again after it, so a long run splits between them many ways and the engine consumes the whole run at every start offset.

Two fixes were tried and rejected, both measured:

• Making the run maximal with a lookahead ({win_sep}+(?!{win_sep})) only halved it -- 6,000 fell to 12.3s but 8,000 still took 22.4s, inside the watchdog's margin rather than clear of it.
• Capping the run ({win_sep}{1,64}) was fast (1.3x base) but traded the hang for a BYPASS: measured, a 65-separator spelling of the fenced store came back permitted, and the normalizer passes did not catch it either. Unacceptable in a fence.

Shipped fix: collapse the run in the SUBJECT, not the patterns. _collapse_separator_runs folds each run to its own first character in one linear pass, and pass 1b re-runs the two existing matchers over that copy only when the original missed and only when there was a run to collapse. Consequences:

• Every pattern in this file is left exactly as tight as main already had it. The diff no longer edits win_gsep, generic_win_home, the %APPDATA% / %LOCALAPPDATA% excursions, the crew variable-leaf join, the resolved-home literal, or the relative-traversal matcher. Net deletions dropped from 9 to 3 -- the change is now almost purely additive, which is the right shape for a security fence.
• Complete for any run length. Measured: 1, 2, 64, 65 and 200 separators all BLOCKED, on the alias branch, the home-anchored branch, a forward-slash spelling and an unquoted spelling. The 64-cap residue is gone.
• Cost is back to base. 8,000 backslashes: 3,082 ms against 2,405 ms on main, roughly 1.2x, which is the collapse pass itself.
• The run collapses to its own first character rather than a fixed one because the resolved-home literal is re.escape-d and expects the platform's exact separator -- collapsing \\ to / stopped it matching, caught by an existing test.
• The original command is always matched FIRST, so a spelling that needs the run intact -- a UNC \\server\share anchor -- keeps matching unaffected.

One test moved with the design: test_the_resolved_home_literal_anchor_tolerates_a_run asserted on the compiled pattern, which no longer carries the run, so it now asserts the composition the gate actually evaluates (collapse, then match) and adds a negative control that an unrelated profile is refused collapsed or not.

Verified on 3a4e31094: test_security.py 1180 passed / 1 skipped; test_trust_reads.py, test_hooks.py, test_governance_self_protection.py, test_connections_tool_aliases.py 641 passed; flake8 and mypy clean. Targeted runs only, as required on this host.

Note for the maintainer: the rebase force-push dismissed the earlier approval, so this needs re-approving before it can merge.

[chenmingwei23]

CI note, so the red on this PR is not misread as the fix regressing something.

The failing backend shards are broken on main, not caused by this branch. Reproduced on pristine kirocrew/main at 5fe1d64e4, in isolation, nothing from this PR in the tree:

FAILED test/test_agent_spec_hardened_reads.py::TestResolveMcpServer::test_valid_agent_spec_still_resolves_under_the_same_cap
  AssertionError: assert (('c', 'a'), {}) == ('c', 'a')
1 failed, 110 passed

_resolve_mcp_server returns a call_args-shaped tuple where the test expects the value itself, so either the test asserts the wrong shape or the function's return contract regressed. It sits in shard 1, which is why it reds out Backend Tests (3.10, 1), (3.12, 1) and (Windows) (1) together. This is fleet-wide: any PR whose CI merges with current main inherits it. Recorded locally as f-20260831-04.

The remaining red, Backend Tests (Windows) (3) -- test_resource_limits_schema.py::TestCgroupConsumerUnchanged::test_the_scope_argv_never_carries_a_zero_ceiling, {'max_memory_mb': 0.5, ...} -- passes on Linux main, so it is Windows-only and I cannot reproduce it on this host. It exercises cgroup_scope_argv fractional ceilings; this PR touches neither that function nor anything it calls. I am flagging it as probably base-owned rather than asserting it, since I have no Windows evidence either way.

Correcting my own earlier evidence on this thread: I previously said both files "pass locally on this branch", offering that as proof the failures were not mine. That reasoning was wrong even though the conclusion held. This worktree predates main's addition of TestResolveMcpServer, so the file I ran did not contain the failing test at all. The check that actually settles it is the one above -- running current main's copy of the file, on current main.

No push is being made to chase these: the /ai-review override on this PR is pinned to ced77480940251c123692fd944d30c2677d4c772, and any push voids it and re-blocks the GPT lane. This PR's own two files are green everywhere they run.

[chenmingwei23]

Settled, with the clean comparison I should have reached for first: main's own CI is red with exactly this PR's failure set.

Run 33348798652 on main at 5fe1d64e4, no PR involved, failed:

Backend Tests (3.10, 1)
Backend Tests (3.12, 1)
Backend Tests (Windows) (1)
Backend Tests (Windows) (3)
Coverage Gate

That is the same five checks, same shard numbers, as here. And the Windows shard 3 failure on main (job 99358170959) is the same test and the same assertion:

FAILED test/test_resource_limits_schema.py::TestCgroupConsumerUnchanged::test_the_scope_argv_never_carries_a_zero_ceiling
  AssertionError: {'max_memory_mb': 0.5, 'max_processes': 0.5}

So both failures are base-owned:

• test_agent_spec_hardened_reads.py::TestResolveMcpServer::test_valid_agent_spec_still_resolves_under_the_same_cap -- shard 1 on all three platforms, and reproducible on a pristine main checkout in isolation. Recorded f-20260831-04.
• test_resource_limits_schema.py::TestCgroupConsumerUnchanged::test_the_scope_argv_never_carries_a_zero_ceiling -- Windows only, passes on Linux main. The failing assertion is tasks == [], i.e. cgroup_scope_argv emitted no TasksMax= at all for {max_processes: 0.5, max_memory_mb: 0.5}, so a fractional limit produces an unbounded scope rather than a floored one. Recorded f-20260831-05, and worth someone's attention on its own merits rather than only as CI noise.

Two corrections to my earlier reasoning on this thread, since both were methodologically wrong even where the conclusion held:

1. I compared Backend Tests (Windows) (3) by NAME across other open PRs and read four greens as evidence. That comparison is invalid: pytest-split runs with no durations file ([pytest-split] No test durations found), so it splits evenly by test count and shard membership shifts whenever a PR adds tests. Shard "3" is not the same set of tests on two different PRs.
2. Those PRs' results were stale anyway, and I could have detected that from data I already had: PR fix(safety): say so when a restart drops an auto-approve grant #7098 shows Windows (1) green, which is impossible against a main where the shard-1 test fails deterministically. Their runs predate the breakage, so they could never have been evidence either way.

The check that actually settles ownership is the one above -- main's own CI on main's own commit. No cross-PR inference, no local worktree that might not match main's tree.

Nothing here is pushed: the /ai-review override is pinned to ced77480940251c123692fd944d30c2677d4c772 and any push voids it. This PR's own two files are green on every job that runs them.

[chenmingwei23]

Closed/reopened to recompute the merge ref against healed main (#7200); head unchanged, override remains pinned to ced7748.

[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

• PR #7913 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7913: MERGE_DISCUSSION. The merged PR is the origin of the defect, not coverage of the fix. It only establishes that pass 1b on main is unconditional and must not be dropped outright. Files: src/kiro_crew/security.py.

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