fix(dashboard): preserve macOS temp project paths - #6905
Conversation
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed BLOCKING -- src/kiro_crew/dashboard/handlers/files.py:3597 -- Boundary scan omits the preceding slash |
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of All six egress sites feed the wrapper realpath'd values ( Design-Verdict: PASS Narrow, fail-closed, evidence-backed call-site exemption that fixes the documented macOS false positive without weakening the canonical credential redactor. Suggestions
[DESIGN-REVIEWED] cded733 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All verification is done. Producing the final review. First-Principles-Verdict: CONCERNS The fix implements exactly the call-site sanitizer the base's own test comment specified — but the false-positive class crosses files, and one counted sibling stays broken. What this change shipsIntent: make macOS temp-rooted project paths render in the dashboard instead of
The redactor-level alternative was measured and rejected at base (three fixes, with leak rates, in the deleted-test comment), so the call-site placement is a decision the repository already recorded — not relitigated. Watch"That is the whole class in this file: six absolute-path egress sites, six fixed" — true, but the class isn't file-scoped. Grepped [FIRST-PRINCIPLES-REVIEWED] cded733 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsI've independently verified the core security property rather than inheriting the discovery narrative. The change adds
The other call sites merely swap No findings. [OPUS-REVIEWED] cded733 |
|
Re the BLOCKING finding on Reproduced. On a prefix that is not self-flagged (an ordinary Darwin id carrying an underscore, so the #6905 false positive is out of the picture), a 38-character directory name is redacted by But it drops no detection. Why not revert. That restores the defect the PR exists to fix. I also measured the natural middle ground — giving the scan boundary context from the prefix — and it is worse, because the OS id is itself high-entropy and any overlapping window inherits it: 39 characters of context re-redacts ~50% of ordinary project names, and even a 2-character Pinned by Separately, the Design and First Principles reviews were right about |
8e4b326 to
11b9d5f
Compare
`_redact_project_path` handed the canonical redactor only the text after
the Darwin temp prefix, so a 40-character window that begins at the
prefix's trailing `T` was never evaluated. An AWS secret key may contain
`/`, so `T/` plus 38 user-controlled characters is a well-formed 40-byte
key rather than a window that merely borrows OS bytes -- and that class
produced `redact(path) != path` while the helper returned `path`
unchanged, weakening the canonical output policy instead of narrowing a
false positive.
Move the split one byte earlier: preserve everything above the trailing
`T`, and hand `T` plus the suffix to the canonical redactor. The
high-entropy `[a-z0-9]{2}/[a-z0-9_]{30}` id still never enters the scan,
which is what the exemption exists for, so the kirodotdev#6905 false positive
cannot return.
Measured over 300 samples each of ordinary project names, pytest temp-dir
names, truncated sha-256 digests and uuid hex, under both a self-flagged
and a non-self-flagged prefix: the boundary byte costs zero additional
redactions. The only names it newly redacts are uniformly-random 38-char
base64 runs, which the canonical redactor already redacts on this path.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The blocking finding is accepted and the code is changed on I argued the dropped window cost no detection because a ≤ 38-character user-controlled run sits below So the helper was weakening the canonical output policy, not narrowing a false positive. That is your finding exactly. The remedy is not the full revert, and not because reverting is inconvenient — prefix = match.group(0)
return prefix[:-1] + redact(prefix[-1] + path[match.end():])Everything above the trailing I also have to correct a measurement I gave you. I claimed a 2-character
Zero additional redactions on every realistic corpus, including the two I named. The only class it newly redacts is uniformly-random 38-character base64 — where canonical already removes 294/300 on this same path today, so the boundary is strictly narrower than canonical there rather than a new policy. Red-before, new tests against the previous head's helper: 3 failed, 17 passed, the decisive one reporting Green on |
A project rooted under Darwin's per-user temp dir renders as `[REDACTED: credential]` in the dashboard. The generic bare-secret detector's character class includes `/`, so `/private/var/folders/<2>/<30>/T/...` reads as one long high-entropy token even though both variable components are OS-owned and fixed-width. Give the six absolute-path egress sites in this file a path-aware wrapper that withholds only the OS-generated id from the scan. Off Darwin it is byte-identical to `redact`. BOUNDARY. The split sits at the END of the variable region: the prefix regex ends in the literal `/T`, and that literal is handed to the canonical redactor along with the suffix. An AWS secret key may itself contain `/`, so a window is a real credential candidate whenever every byte in it is fixed or user-controlled -- `/T/` plus 37 user-controlled characters is a well-formed 40-byte key. Scanning `/T` + suffix covers every such window, so there is no further byte to concede: anything reaching further left necessarily contains OS-generated id bytes that no caller can choose. Measured over ordinary project names, pytest temp-dir names, truncated sha-256 digests and uuid hex (300 samples each, under both a self-flagged and a non-self-flagged prefix): the boundary costs zero additional redactions. The only names it newly redacts are uniformly-random 37-38 character base64 runs, which the canonical redactor already redacts on this path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cded733 to
a3b5ad1
Compare
|
Right again, and the fix is on So The argument, restated so it terminates. A 40-byte window is a real credential candidate whenever every byte in it is either a fixed literal or user-controlled — "it borrows OS bytes" is not a disqualifier, because an AWS secret key may itself contain Applying it: the prefix regex ends in the fixed literal I took your suggested Why still not the full revert. Measured cost of the second byte — 300 samples per corpus, both a self-flagged and a non-self-flagged prefix (n=600):
Zero additional redactions on every realistic corpus — identical to the one-byte split. The only class it newly redacts is uniformly-random base64 of 37–38 chars, where canonical already removes 562/600 and 571/600 on this same path, so the boundary stays strictly narrower than canonical. Tests. Green on Also squashed to a single commit — |
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
|
🤖 Kiro Crew [operator: NicholasRBowers#a942f9ca]: This PR has been inactive for 7+ days with failing CI. I've assessed the blockers and they appear resolvable — I'll push fixes directly to this branch as a co-author. Assessment: The substance is done — GPT's boundary-scan finding was already fixed on head If you'd prefer I don't touch this PR, add the |
|
@leonlaiyc thanks for this, and apologies for the slow review. Before it can land we need to reconcile it with #8055 (@jeeshofone), which fixes the same defect in the same place. Both PRs rewrite the same six egress sites in Suggested path: settle the policy first, then land one helper. Our preference is your boundary-safe helper for absolute roots, extended to the listing values #8055 covers, so a single redactor governs the module. #8055 is 674 commits behind and blocked; this branch is Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
Problem / Motivation
The dashboard's project-git endpoints echo an absolute repository path back to the UI through
security.redact(). On macOS that destroys ordinary paths:GET /api/project/gitreturnsrepoRootandpathas the literal string[REDACTED: credential], so the activity panel shows a redaction marker where the repository root should be.redact()'s_BARE_SECRET_RUN_REdeliberately includes/in its character class, because real credentials (base64, OAuth/PKCE material) contain slashes. A POSIX path is therefore scanned as one token. macOS's per-user temporary root has the fixed shape/private/var/folders/<2 chars>/<30 chars>/T/…, which yields a 63-character run whose 40-character window clears every entropy and structural gate.Measured against this branch's base commit, using the production redactor:
The repository has hit this twice before and worked around it in the test layer both times, never in production:
test/test_project_git.pydeleted its two happy-path tests, leaving a comment stating the defect "is still present and is now unobserved" and that the remedy is "a path-specific sanitizer at the call site" — explicitly not a change to the redactor./tmprepo-wide, which removed the symptom for tests only.Neither touched the endpoint. This PR implements the call-site sanitizer that comment specified.
Why it matters
mkdtemp— is rendered as[REDACTED: credential]in the activity panel. The in-code comment athandlers/files.pyclaiming "A normal path is unchanged" was false on that platform.The redactor itself is deliberately left alone. The prior investigation recorded in that comment tried and rejected three redactor-level fixes with evidence — a path-shape guard leaked a real AWS key containing two slashes, splitting the run on
/missed every real key, and a window slash-count threshold leaked 2.555% of otherwise-caught keys over 200k samples. Weakening a credential redactor so a path renders is the wrong trade, so the fix belongs at the call site.What changed (motivation → approach → change)
Symptom → a fixed-shape, OS-owned path prefix is indistinguishable from a high-entropy bare secret to a slash-tolerant detector. Root cause → the endpoint applies the generic bare-secret redactor to a value it already knows is a filesystem path. Change → give those call sites a path-aware wrapper that exempts only the OS-owned prefix and still sends everything an agent can influence through the canonical redactor.
src/kiro_crew/dashboard/handlers/files.py:_MACOS_TEMP_PROJECT_PREFIX_RE, anchored at\A, matching exactly/private/var/folders/[a-z0-9]{2}/[a-z0-9_]{30}/Tfollowed by/or end-of-string. Both variable components are OS-owned and fixed-width. The lookahead is load-bearing: without it an attacker-chosen segment such as…/Tevil/would be swallowed by the exemption._redact_project_path(path). No match → returnsredact(path), so every non-Darwin-shaped path keeps today's behaviour byte-for-byte. On a match → returns the prefix up to but excluding its trailing/T, concatenated withredact()applied to that/Tplus the entire remaining suffix. See Boundary contract below for why the split sits at exactly that point.api_project_git(repoRoot, the"Not a directory"400 body, and the success envelope'spath), and toapi_project_git_status'srepoRoot. The latter is included because its own comment already promises it "goes through the same redaction as api_project_git" — fixing only the first endpoint would have silently falsified that.Deliberately unchanged:
branch,head, and the repo-relative file paths in the status response stay on bareredact()— they are not absolute paths and carry no OS-owned prefix. The SEL audit trail continues to record the real, unredacted path, and a test pins that.Two in-code comments that the change made inaccurate are corrected in the same commit.
/api/project/treeis now included. An earlier revision left it out as a separate endpoint with its own response contract. Two reviewers independently counted it and were right that the reason did not distinguish anything: the wrapper is byte-identical toredact()off-Darwin, so applying it there changes no contract, and a macOS user's Files tab still rendered[REDACTED: credential]where the project root belongs. Bothrootegress sites inapi_project_tree— the not-a-directory early return and the listing response — now use_redact_project_path. That is the whole class in this file: six absolute-path egress sites, six fixed. The listedpathsstay on bareredact(), correctly — they are project-relative and carry no OS-owned prefix.Boundary contract — where the split goes, and why it stops there. The exact-head review raised this twice and was right both times. The history matters because it is what fixes the shape of the argument, not just the byte count:
11b9d5famatch.group(0) + redact(path[match.end():])cded733cprefix[:-1] + redact(prefix[-1] + …)/beforeTa3b5ad1eprefix[:-2] + redact(prefix[-2:] + …)The governing fact is that an AWS secret key may itself contain
/. So a 40-byte window is a real credential candidate whenever every one of its bytes is either a fixed literal or user-controlled — it is not disqualified by "borrowing OS bytes", which is the reasoning both earlier revisions rested on. Both leaks were the same defect: a class wherei.e. the helper weakening the canonical output policy rather than narrowing a false positive. Two concrete fixtures, both accepted by
_looks_like_secret_keystanding alone, now pin it:Why this is not "one more byte" a third time. The prefix regex is
/private/var/folders/[a-z0-9]{2}/[a-z0-9_]{30}/T— it ends in the fixed literal/T, and everything to the left of that literal is the OS-generated id. Scanning/T+ suffix therefore covers every window composed entirely of fixed or user-controlled bytes, and there is no next byte to concede: a window reaching further left necessarily contains id bytes, which the OS generates and no caller can choose. The recursion terminates at a stateable boundary rather than at wherever the last review stopped looking.That boundary is also what the exemption is for. The id is high-entropy and self-flagging —
redact()removes it on its own account — so letting it into the scan is exactly the #6905 false positive this PR exists to fix. The review's suggested full revert (return redact(path)) does precisely that.Measured cost of the second boundary byte — 300 samples per corpus, under both a self-flagged and a non-self-flagged prefix (n=600 each). "canonical" is
redact()on the whole path, i.e. today's pre-PR behaviour.prefix[-1](1 byte)prefix[-2:](2 bytes)The second byte costs zero additional redactions on every realistic corpus — identical to the one-byte split. The only class it newly redacts is uniformly-random base64 of 37–38 characters, where canonical already removes 562/600 and 571/600 on this same path, so the boundary stays strictly narrower than canonical rather than becoming a second policy. The 300s in the canonical column are the defect itself: every self-flagged-prefix path.
Two corrections to my own earlier replies on this thread, since both are now on the record. (1) I argued a ≤ 38-character user-controlled run is below the classifier's exact-40 minimum and so costs no detection — that is the reasoning the
/-in-keys fact defeats, and it was wrong at both byte positions. (2) I claimed a 2-characterT/boundary "re-redacts ~85% of opaque 38-character directory names (pytest temp dirs, hashes)". That figure came from a uniformly-random base64 corpus and was generalised to corpora it had never been run against; re-measured, both are 0 (table above).docs/system-specs/modules/learn-cron-dashboard.mdis updated so the "Project git" entry describes the split accurately instead of claiming all four fields pass through plainredact, and the "Project tree" entry no longer saysrootand every path alike pass throughredact.Tests
New, in
test/test_project_git.py— these are what actually prove the fix, and they use hardcoded macOS-shaped strings, so they exercise the exemption identically on every platform:test_macos_temp_root_is_not_mistaken_for_a_bare_secret— a macOS temp project root survives byte-for-byte.test_macos_temp_root_still_redacts_a_secret_in_the_suffix— security negative control: anAKIA…key in the user-controlled suffix is still redacted. Asserts both that the key is absent and that a redaction marker is present.test_other_paths_still_use_the_canonical_redactor— patchesredactand asserts a non-Darwin path reaches it verbatim, exactly once.test_similar_macos_path_with_the_wrong_id_width_is_not_exempt— negative control on the regex: a 31-character id (one over the fixed width) is not exempted and falls through to the canonical redactor, pinning that the exemption is shape-exact rather than a prefix heuristic.New, in
test/test_project_git.py, classTestMacosPrefixBoundary— the boundary contract above, stated as properties rather than examples:test_a_credential_spanning_the_prefix_boundary_is_redacted— both blocking findings, pinned. Runs theT/and/T/fixtures above: each is asserted to be a credential by_looks_like_secret_keystanding alone, then asserted removed by BOTH the canonical redactor and the helper, under both prefixes. Asserting both halves means a regression cannot pass by making the canonical side stop firing. Red-before against11b9d5fa's helper: 3 failed, 17 passed. Red-before againstcded733c's one-byte helper: 2 failed, 18 passed —AssertionError: boundary credential survived: /private/var/folders/6r/zyxvpxvq6csfxvn_n0000000000000/T/EqV8ib8HDy88YtDtXbiufMdI8X2Y4rUmer/BH.test_the_two_fixture_prefixes_behave_as_documented— establishes the two controls. An all-alphanumeric temp id is self-flagged (the fix(dashboard): preserve macOS temp project paths #6905 defect); an ordinary id carrying an underscore is not, because the underscore falls outside the bare-secret character class. The second control is what lets the boundary property be measured independently of the false positive this PR fixes.test_neither_boundary_name_is_a_secret_standing_alone— both fixture names are under 40 characters and unflagged bare; they differ only in what their own leading separator buys them.test_a_38_char_name_is_judged_on_its_own_boundary_window— replaces the earliertest_a_38_char_name_is_kept_and_was_never_classifiable, whose premise the review overturned. Being under 40 characters is not what decides such a name; the window actually evaluated isT/plus the name. This fixture clears the classifier's gates and is now removed, while the OS-owned id above the boundary is still preserved byte-for-byte.test_ordinary_project_names_survive_under_both_prefixes— the false-positive control, covering the boundary. Seven ordinary names — including a pytest temp-dir shape, a 38-character hex digest, and a 37-character one, the length that reaches 40 only via/T/and is therefore exactly what the second boundary byte exposes — plus the bare prefix with no suffix, under both prefixes. This is the assertion that breaks if the boundary is ever widened into the OS id.test_the_os_owned_id_never_enters_the_scan— why the exemption exists at all: the self-flagged id is removed by the canonical redactor on its own account and must stay out of the scan.test_a_39_char_name_is_still_scanned_with_its_separator— under both prefixes.test_a_real_credential_never_survives_the_split— the security invariant, 12 cases: three real credential shapes × four placements, each asserted detectable standing alone first, then asserted absent under both prefixes.test_the_exemption_is_exactly_prefix_plus_canonical_suffix— the whole contract as an identity: everything above the trailing/Tis preserved byte-for-byte, and/Tplus the tail equalsredact()of the same. The split point itself is asserted (boundary == "/T") rather than assumed, so the exemption cannot drift into a second redaction policy.Mutation-checked: widening the exemption to return the path unchanged fails this class across every credential placement; reverting to the original split fails three of its tests, and reverting just the second boundary byte fails two. No half of the contract is vacuously green.
New, in
test/test_project_tree.py:test_not_a_directory_root_takes_the_path_aware_redactor— the early-return arm. Reached with a real non-directory rather than by patchingos.path.isdir, which is process-global and breaks unrelated lazy imports.test_listing_root_takes_the_path_aware_redactor— the listing arm, pinned on the call because the two redactors agree off-Darwin, plus an assertion that the listing itself still works.New, in
test/test_project_git_status_log.py:test_repo_root_uses_the_path_aware_redactor— pins the parity the neighbouring comment promises. Red-before on this branch with only that one line reverted:AssertionError: assert 0 == 1(the wrapper is never reached).Also restored:
test_returns_branch_for_repoandtest_finds_repo_root_from_subdirectory, deleted when the endpoint defect was first documented. To be precise about what this is and is not — these two pass on macOS because of #5366's temp-base redirect, not because of this production change; pytest'stmp_pathresolves under/private/tmp, which the new regex intentionally does not match. They are restored here because this PR removes the comment that justified deleting them, and they recover the lost coverage of branch labelling and repo-root walk-up. They are not evidence for the fix; the four tests above are.Three pre-existing assertions relaxed from
.startswith("<")to"<" in: with a preserved prefix the redacted value no longer begins at index 0 on a Darwin-shaped path. Theendswith(">")half is retained, so each still proves the tail passed through the patched redactor.Results on head
a3b5ad1e(Python 3.10.6, Windows):test_project_git.py+test_project_tree.py+test_security.py+test_project_git_status_log.py+test_log_redaction.py+test_redaction_mirror_parity.py+test_redact_meta_snapshot.py— 1107 passed, 19 skipped, 0 failed.TestHomeDirTargetsCache::test_second_call_does_not_rebuild, whole-file only, andTestIsSensitiveBashCommand::test_chained_cd_expansions_do_not_blow_up_the_gate, a wall-clock guard). Recorded rather than omitted; it is not being claimed as attributed.test_project_tree.py::test_vanished_directory_response_is_redacted(pre-existing, untouched by this PR) monkeypatches the process-globalos.path.isdir, which breaks a lazy numpy C-extension import during teardown on this Windows box. Verified at this branch's base: that test alone reports1 passed, 1 errorthere too. The new tree tests deliberately avoid that patch.WinError 1314symlink-privilege failures intest_file_download.py/test_file_raw.py, reproduced identically on a pristineorigin/mainworktree at this branch's own base. Inherited local environment limitation; neither file has any path to the diff.Gates green on head
a3b5ad1e:mypy --platform linuxon the changed handler (no issues), black baseline gate (scripts/check_black_formatting.py, 5 files in scope, no new offenders), isort, flake8, docs-lint, lockdown-before-publish, brand-name, changelog-history, focus-cue.Manual verification
N/A — unit coverage sufficient: the defect is a deterministic pure-string transformation of a response field, and the exemption's positive case, secret-bearing suffix, boundary windows, wrong-width shape, attacker-chosen
T…segment, and non-Darwin fallthrough are each pinned by a test that runs identically on every platform.On the
## Screenshots / videosection, deleted above: the rendered surfaces (the activity panel's branch label and the Files tab's project tree) do change for a macOS user —[REDACTED: credential]becomes the real path — so this is not a no-visual-delta change and is not being claimed as one. What changes is one string in a JSON response, with no layout, component, or style change; and the delta is reproducible only on Darwin, which this branch was developed and tested on Windows. Rather than stage a synthetic screenshot that would not be evidence of the platform-specific behaviour, the before/after is given as the redactor's actual output at the base commit and on this head, in Problem / Motivation above. A macOS reviewer can confirm it directly by opening any project whose realpath is under/private/var/folders/…/T/.Related Issues
No filed issue. The defect was documented in-tree by the comment this PR removes from
test/test_project_git.py, which recorded it as "still present and now unobserved" and specified the call-site sanitizer implemented here.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement