fix(dashboard): redact known-path listings segment-wise so deep paths survive

[jeeshofone]

Problem / Motivation

redact()'s bare-secret matcher (_BARE_SECRET_RUN_RE) has / inside its payload character class (it is a base64 alphabet character), so a deep slash-separated path whose directory chain carries no ./-/_ scans as one candidate run of 40+ characters. redact_credentials pass 3's whole-run amplification — correct for a real secret glued to adjacent base64 characters — then replaces the entire path with a single [REDACTED: credential] placeholder.

Two genuinely different files (.../libeay32.dll and .../ssleay32.dll under a stock vendored Binaries/ThirdParty/OpenSSL/Win64/VS2015/ chain) collapse to a byte-identical key. #7678's de-duplication then resolves the collision by dropping the later entries — so post-#7678 the failure mode is no longer a loud render crash but the workspace tree silently omitting real files (the reporter measured 237 mangled paths → 158 duplicate keys → ~79 files invisible on one large vendored tree). Fixes #8042.

Why it matters

Files that exist on disk vanish from the file picker with no error, no log line, and no visual hint. The threshold behaviour (exactly one extra directory level flips a path from intact to destroyed) makes it look intermittent and unreportable to most users.

What changed (motivation → approach → change)

• Motivation: the whole-run amplification must stay for unknown-provenance text (it is what stops a 41-char key leaking verbatim), so the general matcher cannot simply drop / from its class — splitting a real slash-carrying base64 secret into sub-40-char pieces would leak it. The fix has to live where the value is provably a path.
• Approach: the reporter's suggested direction 1 — the listing endpoints (api_project_tree, api_project_git_status) know they hold project-relative POSIX path lists, so a separator is a hard boundary there, and only there.
• Change (src/kiro_crew/dashboard/handlers/files.py): new module helper _redact_path_display(path) — fast path returns after a single redact() call when the value is unchanged (the overwhelmingly common case, so the 10k-entry tree does not pay per-segment scanning); slow path redacts each /-separated segment independently. Both endpoints' path mutations now route through it. The general redact() and _BARE_SECRET_RUN_RE are untouched.

Tests

• test_deep_paths_are_not_collapsed_into_one_key — reproduces the redact() treats a deep path as one secret run, collapsing distinct paths to one key #8042 shape end-to-end through GET /api/project/tree: two vendored-binary paths under a deep slash-only chain both survive intact and distinct. Includes a premise assert that the general redact() still mangles the raw path, so the test provably exercises the slow path (and flags fixture rot if the matcher ever changes).
• test_redact_path_display_still_redacts_a_secret_segment — coverage-hole guard: a credential-shaped single segment still redacts; the ordinary neighbouring segment survives.
• Full test/test_project_tree.py suite: 10 passed (including fix(dashboard): de-duplicate redacted path listings so a collision can't crash the tree (#7671) #7678's existing collision-dedup test, which still passes — an in-segment credential collision still collapses and dedups exactly as before).
• Lint floor: black gate, isort, flake8, mypy all clean on both changed files.

Manual verification

Ran the reporter's repro semantics via the new premise assert: redact_via_context("Binaries/ThirdParty/OpenSSL/Win64/VS2015/libeay32.dll") mangles the path on this branch's base (confirming the live bug), while _redact_path_display returns both example paths intact and distinct.

Screenshots / video

Not applicable — backend response content; the observable effect is files no longer disappearing from the tree, covered by the endpoint-level test.

Related Issues

Fixes #8042. Context: #7671 (original render crash), #7678 (crash fix via de-dup; explicitly scoped away from this matcher), #6350 (prior separator-related fence issue in the same module, different function).

Pattern harvest

Rule candidate: a redaction/sanitization heuristic tuned for unknown-provenance text should get a provenance-aware wrapper at call sites that KNOW the value's type (here: paths), rather than weakening the general matcher — the type knowledge is what makes a separator a safe hard boundary. Second occurrence of this shape in security.py-adjacent code (#7912's shell-vs-source-body context split is the same lesson).

Checklist

• Tests added/updated and passing locally
• Lint/format gates run on all changed files
• No unrelated changes bundled
• PR body follows the template

Contribution License Agreement

By submitting this pull request, I confirm that my contribution is made under the terms of the project's contribution license agreement.

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of fe20f96ff121e354a0c3360c6e9e215bc530a4ee via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Provenance-aware segment redaction at the call sites that prove path-ness is the right layer; the general matcher's whole-run amplification correctly stays intact.

Suggestions

• _redact_path_display is stranded as a private helper in files.py while the same bug class exists in sibling path-egress sites (e.g. artifacts.py:831 session-doc paths, where a deep slash-only prefix collapses and makes an innocent doc unmatchable for /materialize) — hoisting it next to redact() would let those adopt it in a follow-up instead of re-deriving the provenance argument.

[DESIGN-REVIEWED] fe20f96

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of fe20f96ff121e354a0c3360c6e9e215bc530a4ee via the fork AI-review pipeline — 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 checks complete. The premise is verified in the base (security.py:11831 has / in the 40-char run class), no existing path-aware display redactor exists anywhere in src/ (grepped redact_path|segment.*redact — only a rejection gate in pptx_maker/decks.py:154, a different job), and all 7 filesystem-path display sites in the dashboard are converted by this diff. Final review follows.

First-Principles-Verdict: PASS

A measured defect (~79 files silently missing) fixed at its cause — provenance-aware boundary at path-typed call sites — covering every sibling display site, adding only one private helper.

What this change ships

Intent: stop the file tree silently omitting real files whose deep slash-only paths the credential redactor swallows whole (#8042) — a FIX.

1. Deep vendored paths in /api/project/tree no longer vanish — justified (the fix, measured harm).
2. Git-status file paths survive the same collapse — justified, same cause.
3. repoRoot/root display values render intact, not as a placeholder — undeclared (test cites a review advisory; PR body names only "both endpoints"), same cause, justified.
4. api_project_git's echoed path (success and 400 arms) renders intact — undeclared, same cause, justified.
5. Branch names deliberately keep whole-string redaction — justified (free-form text can be a slash-bearing secret).
6. New private _redact_path_display with a fast path — justified: 8 real consumers in this diff, zero public surface; no existing mechanism (grepped redact_path|segment.*redact across src/: only a rejection gate in pptx_maker/decks.py:154, a different job).
7. Three tests including a premise assert that pins the live matcher behavior — justified.

The general matcher is untouched, which is the correct level: weakening _BARE_SECRET_RUN_RE (verified at security.py:11831 — / is in its payload class) would leak real slash-bearing secrets, so the invariant gap was routing provably-path values through an unknown-provenance redactor.

Watch

• One counted unfixed sibling of the root cause: files.py:206 rejects a file_send whose user-supplied raw_path is a deep slash-only chain as "sensitive" (same whole-run match). Provenance there is client input, so leaving it fail-closed is defensible — accepted-and-deferred, not a demand.

[FIRST-PRINCIPLES-REVIEWED] fe20f96

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed fe20f96ff121e354a0c3360c6e9e215bc530a4ee via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] fe20f96

[jeeshofone]

Advisory round — head 1d5f518c43a6142cf38318c7159fa0370abadcdc (was 05ef24ccf)

Design suggestion + First Principles CONCERNS (sibling known-path redact() calls): TAKEN for the display values, DECLINED-with-reason for the screens

Both lanes point at the same sibling class, so one disposition:

TAKEN — the four single-path DISPLAY values now route through _redact_path_display: repoRoot and branch in api_project_git_status, and both root returns in api_project_tree (the non-repo early return included). branch is not strictly a filesystem path but slash-separated branch names (feature/deep/nested/name) carry the identical collapse class, and the helper degrades to a single redact() call for plain values. New test test_redact_path_display_keeps_a_deep_root_value (with a premise assert that the general matcher does mangle the shape).

DECLINED for the remaining whole-string redact() calls First Principles counted — those are fail-closed SCREENS, not display transforms: redact(x) != x comparisons that REFUSE an operation when the value trips any matcher (filename gate, upload/download content screens, media probe). Making a screen segment-wise would loosen what it refuses — the conservative false-positive is the point of a screen, and changing refusal behaviour is out of this PR's scope. If maintainers want path-aware screening it deserves its own change with its own tests.

11 tests green (was 10); full lint floor clean.

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed fe20f96ff121e354a0c3360c6e9e215bc530a4ee via the fork AI-review pipeline; updated in place on each push.

BLOCKING -- src/kiro_crew/dashboard/handlers/files.py:4609 -- Segment recombination exposes slash-bearing credentials
return "/".join(redact(segment) for segment in path.split("/"))
40-character AWS secret path -> project listing -> short segments remain unchanged -> dashboard receives the raw credential.
Anchor: backend-security-controls
Fix: Return red when whole-path redaction changes instead of recombining segments.
FINDING -- src/kiro_crew/dashboard/handlers/files.py:3542 -- "_redact_path_display(root)" also changes the undeclared /api/project/git redaction surface -> Fix: revert the three /api/project/git substitutions. (origin: validation)
[BLOCK-MERGE] fe20f96
[GPT-REVIEWED] fe20f96

[jeeshofone]

GPT round — head 487a4246989a6282f4e35be57c4a7049f6080400 (was 1d5f518c4)

BLOCKING (files.py:4811 — branch through the path helper): FIXED — finding accepted

Correct catch. A branch NAME is free-form text, not a provable path: it can itself BE a slash-bearing 40-character secret, and segment-wise splitting would break it into sub-threshold pieces that render raw. The provenance argument that justifies segment-wise redaction holds only for values derived from the filesystem/git listing — which branch is not.

branch is back on whole-string redact() with a comment recording exactly why it must stay there (so the next advisory pass doesn't "fix" it forward again). repoRoot and the two root returns keep the helper — those are caller-derived paths. 11 tests green, full lint floor clean.

This also sharpens the PR's own rule: the wrapper applies where the caller PROVES the value is a path, and a branch name failing that test is the boundary case that proves the test matters.

[jeeshofone]

GPT round on 487a4246989 — mechanism CONCEDED, literal fix contested: maintainer pick requested

The mechanism is real. redact()'s bare-run matcher includes / in its payload class, so a ≥40-char alnum secret spanning segments matches whole-string but can fail every per-segment check — the fallback re-exposes what redact() caught. Conceded without reservation.

Why I have not applied the literal fix ("preserve red"). Returning red on the slow path re-collapses every deep slash-only path to one byte-identical placeholder — that is exactly the #8042 defect this PR exists to fix (the premise test test_deep_paths_are_not_collapsed_into_one_key pins that the filer's benign repro takes this same slow path), and it breaks the Design/FP advisory requirement accepted two rounds ago that deep root/repoRoot values render intact (test_redact_path_display_keeps_a_deep_root_value). The two lanes now pin exact inverses on one axis.

The inputs cannot be told apart. Verified against the live regex: the filer's benign Binaries/ThirdParty/OpenSSL/Win64/VS2015 (40 chars) and a cooperatively-encoded spanning secret both match _BARE_SECRET_RUN_RE identically. No function can satisfy both requirements on this input class — any fix chooses which accepted requirement to break.

The spanning coverage was never adversarially sound. Also verified: a single . (or -/_) in any segment defeats the bare-run matcher on main today (wJalrXUtnFEMI/K7MDENG.x/bPxRfiCYEXAMPLEKEY → no match). An adversary who controls directory names — the premise of the finding's "agent-created path" — already bypasses it; the matcher only ever caught cooperative encodings. The marginal exposure this round names also has an unusual threat shape: it requires an actor with workspace write access encoding a secret it can already read, displayed to the dashboard viewer who owns both.

Three resolutions, maintainer's pick — I will implement any within one round:

1. Keep per-segment for proven paths (current head; Design/FP-endorsed; filer's direction 1). Residual: cooperatively-encoded spanning secrets in path listings render raw — the same class main's dot-bypass already leaks.
2. Fail-closed: return red (this round's literal fix). Cost: regresses redact() treats a deep path as one secret run, collapsing distinct paths to one key #8042 to unfixed (distinct files collapse and vanish from the tree) and reverts the Design/FP root-intact requirement.
3. Distinct-but-closed: return red suffixed with a short salted, non-reversible per-path fingerprint. Keeps GPT's invariant and fixes the file-hiding half of redact() treats a deep path as one secret run, collapsing distinct paths to one key #8042 (distinct keys, no dedup swallowing), but paths/roots render as placeholders — illegible for the UI use the Design lane required.

Round trajectory for the record: advisory round (display extensions taken), GPT r1 (branch reverted to whole-string — accepted because a branch name is not layout-proven), GPT r2 = this, demanding the inverse of the reviewed design for the proven-path case. Holding code still until the pick lands.

[chenmingwei23]

You asked for a maintainer pick, so here is an independent measurement against your head 487a4246989. I am not a maintainer and I am deliberately not recommending a shape -- this is evidence for the pick you asked for, nothing more.

Your direction is the right one

Worth saying first, because it is the part the review lanes have not told you.

The issue's wording does not settle whether the collapse causes over-redaction (distinct non-secrets share a mask, information lost) or under-redaction (a real secret missed because a sibling path already claimed the key). Those need different fixes. I settled it on main:

• Both stock vendored paths collapse to one byte-identical string, reproduced.
• The threshold you documented holds exactly: a 40-character run gives 1 window and survives; a 49-character run gives 10 windows and is destroyed. One directory level is the difference.
• On a 4,000-path synthetic vendored tree: 72 paths mangled, and 71 distinct paths lost to key collision. Your silent-omission account is right, and it scales.

Under-redaction does not reproduce on main, and I pushed on it rather than assuming:

• 400 synthetic tokens that the module's own _looks_like_secret_key accepts: 400/400 redacted standalone, and 400/400 still redacted when embedded as a segment inside a slash chain. Zero leaks.
• The reason is structural. The two run-level fast-path gates (_has_all_three_char_classes, _HEX_ONLY_RE) are closed under substring, so they are verdict-neutral by construction. The only run-level exclusion that is not closed under substring, _decodes_to_printable_text, fired 0 times in 14,000 forced glue combinations.

So / in the payload class never hides a secret. It only destroys non-secrets. The defect is over-redaction only, which is exactly what you targeted -- and on your head the reported symptom is genuinely gone: the helper returns both example paths intact and distinct.

The hole, measured against the shipped helper

These rows come from importing _redact_path_display from your head, not from a re-implementation of its slow path. Corpus is 600 synthetic tokens the module itself classifies as bare secret keys, generated from a seeded PRNG (seed 8042) so you can reproduce the counts; 283 of the 600 (47.2%) happen to contain a /.

case	base catches	your helper catches	reached slow path
secret is one whole segment, contains /	283/283	0/283	283/283
secret is one whole segment, no /	317/317	317/317	317/317
secret straddles a separator (20/20)	300/300	0/300	300/300
secret straddles a separator (10/30)	300/300	0/300	300/300
secret glued to an adjacent base64 char, no separator present	300/300	157/300	300/300

Three things about this table:

1. It is live, not hypothetical. Every leaking case reached the slow path -- the fast path does not short-circuit them, because a value containing a secret is by definition one redact() modifies.
2. It is a clean partition, not sampling noise. The misses are exactly the slash-bearing set: 0/283 with a /, 317/317 without.
3. The last row has no path separator in it at all. That isolates the mechanism: once the slow path is entered, split("/") cuts the secret, not just the path. A base64 secret contains / about half the time.

Why your own guard cannot see this

This is the part I think is genuinely hard to get from a review lane, and it is an easy mistake rather than an oversight.

test_redact_path_display_still_redacts_a_secret_segment uses the AKIA-prefixed documented example ID. That value is a 20-character segment containing no /, and it is caught by the labelled AKIA branch of _CREDENTIAL_PATTERNS. Segmentation cannot break a labelled short credential: it has no separator to split on and it is nowhere near the 40-character floor.

The class at risk is the 40-character bare secret that _BARE_SECRET_RUN_RE exists for -- the one with no prefix and no label, which is the entire subject of #8042. So the guard exercises the one credential class that structurally cannot exhibit the hole, which is very likely why the surface reads as closed.

A same-class site the diff leaves behind

Separate item, small. files.py:3540, inside _project_git_branch() (whose result is returned through api_project_git):

out: dict = {"repo": True, "repoRoot": redact(root)}

Still plain redact(), while the identical repoRoot value in api_project_git_status was converted at line 4809. root there is filesystem-derived and echoed to the dashboard, and the file's own comment at that site says so. Your test_redact_path_display_keeps_a_deep_root_value asserts that a deep root must render intact, so by your own stated standard this site is a miss: a deep slash-only project root still renders as a placeholder here.

Why this is a pick and not a review nit

A splitter cannot distinguish a / inside a base64 secret from a / that separates two path segments. They are the same byte and the value carries no marker to tell them apart. That is a property of the segment-wise approach itself, not a bug in your implementation -- there is no version of split("/") that gets this right, so it is not fixable by tightening the helper.

Which is what makes it a genuine security-sensitivity trade rather than something a reviewer can just tell you to correct: the approach buys exact path fidelity and pays in bare-secret coverage on the values it special-cases. Whether that trade is the right one is the call you asked for, and it is not mine to make. I am offering no alternative shape here on purpose.

Measurement notes: every token above is synthetic, generated locally from a seeded PRNG, never a real credential, and no token value appears in this comment or in any fixture. The main figures are from a9769ebb1; the helper figures are from your 487a4246989. Nothing was pushed to this branch.

[jeeshofone]

Acknowledging the measurement — arbitration stance updated, head 3dba6fdaac65ad7db4fd21fa275416dfc79b7efa

@chenmingwei23 thank you — this is exactly the evidence the pick needed, and two of your findings correct my earlier framing rather than just inform it. Stating both plainly:

1. My "single-segment secrets still redact" guard tested the wrong credential class. test_redact_path_display_still_redacts_a_secret_segment uses the AKIA documented example — a 20-char LABELLED credential with no /, caught by the prefixed branch, structurally incapable of exhibiting the hole. It proved nothing about the bare ≥40-char class that is the entire subject here. Conceded.
2. The residual in the current head is measured, live, and large — not the corner I framed. 0/283 slash-bearing bare secrets caught (~47% of the bare-base64 class), and 0/600 when the secret straddles a separator, all reaching the slow path. My earlier disposition weighted the residual by the dot-bypass argument (an adversary already evades the matcher on main); your table shows the cooperative/accidental class the matcher DOES catch on main is roughly half of all bare base64 secrets. The maintainer should weigh option 1 with your numbers, not my framing.

Your over-redaction measurements equally confirm the defect direction (71 distinct paths lost to collision on a 4k tree on main; zero under-redaction leaks in 14k forced combinations), and your structural conclusion — the same byte carries no marker, so no splitter can get this right — is precisely why this is an arbitration and not a fix round.

Taken now (same direction as the accepted Design/FP advisory, does not prejudge the pick): the missed _project_git_branch() site — its repoRoot was still plain redact() while the identical display value in api_project_git_status was converted; both now route through the helper, so whatever shape the arbitration picks applies uniformly. 11 tests green, floor clean.

The three options in my previous comment stand, now with corrected weights. Still holding the contested axis until the pick.

[jeeshofone]

Design PASS suggestion taken — head fe20f96ff121e354a0c3360c6e9e215bc530a4ee

Design Review's remaining same-provenance site is converted: api_project_git's echoed path (both the success body and the not-a-directory 400 arm — the sites its comment describes; they sit at files.py:3635/3651 on the reviewed head) now routes through _redact_path_display, matching the other allow-listed-path display values. Every known-path display echo in this handler family is now uniform, so whichever shape the open GPT arbitration picks applies to all of them at once. 11 tests green, floor clean. Contested axis still held.

[jeeshofone]

Record-keeping: GPT round 3 re-asserts the arbitration axis, now on both surfaces

No new mechanism this round — noting it for the arbitration record:

1. The BLOCKING finding is the same segment-recombination axis held since round 2, with the same literal fix ("return red") whose cost — re-collapsing distinct deep paths, the redact() treats a deep path as one secret run, collapsing distinct paths to one key #8042 defect itself — is already on the table with @chenmingwei23's measurements.
2. The new non-blocking FINDING asks to revert the three /api/project/git conversions — the exact sites the Design lane's PASS suggested converting last round. The two lanes now demand inverses on BOTH surfaces of this PR (the listing fallback and the display echoes), which is precisely the shape the open maintainer-pick request describes.

Code stays held on the contested axis. The three options (per-segment / fail-closed red / distinct-but-closed fingerprint) stand with the corrected weights; whichever is picked now applies uniformly to every site, since the advisory rounds made the helper's application consistent.

[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 #3987 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 #3987: REBASE. Same coupling as PR #6905 on the same line; the set of path-shaped fields this PR introduces should be settled together with whichever path-display redactor is adopted. Files: src/kiro_crew/dashboard/handlers/files.py.
• PR #6905 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 #6905: MERGE_DISCUSSION. The two PRs edit the same six lines with two different provenance-aware wrappers for the same defect class and conflict textually. PR #8055 is the more general mechanism and already covers PR #6905's user-visible symptom, so shipping both wrappers side by side would leave two competing redaction policies in one module. A maintainer decision is needed on which helper is canonical; the likely best outcome keeps PR #8055's generic segment-wise helper and salvages PR #6905's boundary test suite, doc correction, and the two restored endpoint tests. Closing PR #6905 outright would discard work PR #8055 does not replace. Files: src/kiro_crew/dashboard/handlers/files.py.

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