Skip to content

fix(dashboard): preserve macOS temp project paths - #6905

Open
leonlaiyc wants to merge 1 commit into
kirodotdev:mainfrom
leonlaiyc:fix/project-git-macos-path-redaction
Open

fix(dashboard): preserve macOS temp project paths#6905
leonlaiyc wants to merge 1 commit into
kirodotdev:mainfrom
leonlaiyc:fix/project-git-macos-path-redaction

Conversation

@leonlaiyc

@leonlaiyc leonlaiyc commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

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/git returns repoRoot and path as the literal string [REDACTED: credential], so the activity panel shows a redaction marker where the repository root should be.

redact()'s _BARE_SECRET_RUN_RE deliberately 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:

redact("/private/var/folders/6r/54rts88h7yebke7n6clhoq9d0roaen/T/project")
  -> "[REDACTED: credential]"

The repository has hit this twice before and worked around it in the test layer both times, never in production:

  • test/test_project_git.py deleted 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.
  • Later, fix(test): make the backend suite pass on macOS, not only on Linux #5366 repointed Darwin's pytest temp base to /tmp repo-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

  • A real macOS user still sees the bug. fix(test): make the backend suite pass on macOS, not only on Linux #5366 changed where pytest puts temp directories; it did nothing for a real project directory. Any macOS project whose realpath sits under the per-user temp root — scratch and ephemeral workspaces, and anything created via mkdtemp — is rendered as [REDACTED: credential] in the activity panel. The in-code comment at handlers/files.py claiming "A normal path is unchanged" was false on that platform.
  • A coverage hole outlived its cause. The two deleted tests were the only automated proof that the endpoint resolves a branch on the happy path and walks up from a subdirectory to the repository root. That gap has been safely closeable since fix(test): make the backend suite pass on macOS, not only on Linux #5366 landed and simply was not noticed; while it stood, a regression in repo-root resolution would have shipped silently on all platforms.

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:

  • New _MACOS_TEMP_PROJECT_PREFIX_RE, anchored at \A, matching exactly /private/var/folders/[a-z0-9]{2}/[a-z0-9_]{30}/T followed 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.
  • New _redact_project_path(path). No match → returns redact(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 with redact() applied to that /T plus the entire remaining suffix. See Boundary contract below for why the split sits at exactly that point.
  • Applied to the three absolute-path egress sites in api_project_git (repoRoot, the "Not a directory" 400 body, and the success envelope's path), and to api_project_git_status's repoRoot. 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 bare redact() — 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/tree is 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 to redact() 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. Both root egress sites in api_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 listed paths stay on bare redact(), 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:

head split window that leaked
11b9d5fa match.group(0) + redact(path[match.end():]) every window crossing the boundary
cded733c prefix[:-1] + redact(prefix[-1] + …) the one starting on the / before T
a3b5ad1e prefix[:-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 where

redact(path) != path          # canonical removes it
_redact_project_path(path) == path   # the helper did not

i.e. the helper weakening the canonical output policy rather than narrowing a false positive. Two concrete fixtures, both accepted by _looks_like_secret_key standing alone, now pin it:

T/PtYgjmUhBel31iEl2hpChYgCfrL1spNxnyVmih    # leaked at 11b9d5fa
/T/EqV8ib8HDy88YtDtXbiufMdI8X2Y4rUmer/BH    # leaked at cded733c

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.

corpus prefix[-1] (1 byte) prefix[-2:] (2 bytes) canonical
ordinary project names 0 0 300
pytest temp-dir names 0 0 300
truncated sha-256 digests (38 hex) 0 0 300
truncated sha-256 digests (37 hex) 0 0 300
uuid4 hex 0 0 300
uniformly-random base64, 37 0 506 562
uniformly-random base64, 38 512 518 571

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-character T/ 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.md is updated so the "Project git" entry describes the split accurately instead of claiming all four fields pass through plain redact, and the "Project tree" entry no longer says root and every path alike pass through redact.

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_suffixsecurity negative control: an AKIA… 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 — patches redact and asserts a non-Darwin path reaches it verbatim, exactly once.
  • test_similar_macos_path_with_the_wrong_id_width_is_not_exemptnegative 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, class TestMacosPrefixBoundary — the boundary contract above, stated as properties rather than examples:

  • test_a_credential_spanning_the_prefix_boundary_is_redactedboth blocking findings, pinned. Runs the T/ and /T/ fixtures above: each is asserted to be a credential by _looks_like_secret_key standing 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 against 11b9d5fa's helper: 3 failed, 17 passed. Red-before against cded733c's one-byte helper: 2 failed, 18 passedAssertionError: 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 earlier test_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 is T/ 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_prefixesthe 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_splitthe 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 /T is preserved byte-for-byte, and /T plus the tail equals redact() 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 patching os.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_repo and test_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's tmp_path resolves 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. The endswith(">") 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.py1107 passed, 19 skipped, 0 failed.
  • One earlier run of that same set reported a single failure that did not reproduce on re-run and printed no nodeid to attribute. Both known base-equal flakes on this box live in that set (TestHomeDirTargetsCache::test_second_call_does_not_rebuild, whole-file only, and TestIsSensitiveBashCommand::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.
  • One inherited local error, not a failure and not from this change: test_project_tree.py::test_vanished_directory_response_is_redacted (pre-existing, untouched by this PR) monkeypatches the process-global os.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 reports 1 passed, 1 error there too. The new tree tests deliberately avoid that patch.
  • Previously reported and unchanged: 6 WinError 1314 symlink-privilege failures in test_file_download.py / test_file_raw.py, reproduced identically on a pristine origin/main worktree 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 linux on 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 / video section, 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

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

@leonlaiyc
leonlaiyc requested a review from a team as a code owner August 30, 2026 03:04
@leonlaiyc
leonlaiyc requested a review from CrysisDeu August 30, 2026 03:04
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

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

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

BLOCKING -- src/kiro_crew/dashboard/handlers/files.py:3597 -- Boundary scan omits the preceding slash
return prefix[:-1] + redact(prefix[-1] + path[match.end():])
A detector-accepted 40-byte /T/<37-byte tail> key in a macOS temp project path -> project endpoints -> only 39 bytes are scanned -> credential reaches the dashboard verbatim.
Anchor: backend-security-controls
Fix: Preserve one fewer byte: prefix[:-2] + redact(prefix[-2:] + path[match.end():]).

[BLOCK-MERGE] cded733
[GPT-REVIEWED] cded733

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

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

All six egress sites feed the wrapper realpath'd values (os.path.realpath in _resolve_project_git and the tree/status handlers), so the \A/private/var/... anchor covers macOS's /var/folders symlink spelling — the concern I was probing dies. The design holds up: real, in-tree-documented harm; a fail-closed exemption (a non-matching path falls through to full redaction, never leaks); the user-controlled suffix plus the boundary byte still passes through the canonical redactor; rejected alternatives (redactor-level fixes, full revert) are evidence-backed; docs updated in the same commit.

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

  • _redact_project_path silently depends on callers realpath-ing first (/var/folders/... wouldn't match); state that precondition in the docstring so a future non-canonicalized call site doesn't quietly regress to the old symptom.

[DESIGN-REVIEWED] cded733

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of cded733cb22132f415227b94caeed136ba0b63e2 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 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 ships

Intent: make macOS temp-rooted project paths render in the dashboard instead of [REDACTED: credential] — a FIX.

  1. Activity panel shows the real repo root on macOS temp projects — justified (defect recorded at base, test/test_project_git.py)
  2. Same fix on the git-status panel's repo root — justified (base comment promises parity with /api/project/git)
  3. Same fix on the Files tab's tree root, both response arms — justified, same class
  4. Two happy-path tests deleted at base are restored — justified (the base comment names them as lost coverage)
  5. New boundary test suite pinning what the exemption may and may not cost — justified
  6. Two existing redaction tests loosened from whole-string-wrapped to contains-marker — necessary for the prefix-preserving output; rides along, harmless
  7. Spec paragraph updated in the same commit — mandated by AGENTS.md
  8. New private helper _redact_project_path + prefix regex — mechanism level; 6 real consumers, no public surface

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 redact\(.*path|root across dashboard/handlers: 2 sibling absolute-path egress sites outside files.py. artifacts.py:820 redacts session-doc paths whose comment claims "ordinary paths still round-trip through /materialize" — false under Darwin temp for exactly this PR's reason, so materialize refuses those docs; knowledge.py:1894 (file_path) is the same shape, thinner harm. The helper is private to files.py, so the general fix is genuinely larger — accepted-and-deferred, but a human should see the count.

[FIRST-PRINCIPLES-REVIEWED] cded733

@github-actions

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

I've independently verified the core security property rather than inheriting the discovery narrative.

The change adds _redact_project_path, which withholds only the OS-owned /private/var/folders/[a-z0-9]{2}/[a-z0-9_]{30}/T prefix (minus the trailing T) from the bare-secret scan, handing T + the full user-controlled suffix to canonical redact. Checking the actual classifier in security.py:

  • The withheld region ([a-z0-9]{2}/[a-z0-9_]{30}) can never hide a real credential: _looks_like_secret_key gate 2 requires an uppercase char, and the id contains none; an _ also isn't in _BARE_SECRET_RUN_RE's class so it breaks runs. No 40-char window confined to that region can clear the gates, so dropping it cannot leak a secret — it only kills the false positive.
  • Any user-planted credential lives entirely in the suffix, and every 40-char window over the suffix is present in the scanned T/<suffix> string (the T is deliberately kept in-scan so a window straddling the T/ boundary is still evaluated). Both endpoints stay consistent.
  • The one asymmetry (whole-run _decodes_to_printable_text differing between the full run and T/<suffix>) is not reachable: it needs a suffix that both contains a gate-clearing key window and decodes ≥85% printable only after removing the OS prefix — mutually contradictory, not a concrete input.

The other call sites merely swap redact_redact_project_path for absolute-path values (repoRoot, root, base) while repo-relative paths correctly stay on redact; comments match behavior. Nothing in the candidate list to falsify (it returned clean), and I could not ground any Step 2 finding to the bar.

No findings.

[OPUS-REVIEWED] cded733

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@leonlaiyc

Copy link
Copy Markdown
Contributor Author

Re the BLOCKING finding on _redact_project_path — the mechanism is real and I reproduced it; the stated consequence is not, so the suggested revert is not the right remedy. Detail is in the PR body under Boundary contract; the short version:

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 redact(whole) and preserved by _redact_project_path. So the split does drop windows.

But it drops no detection. _looks_like_secret_key gates on an exactly-40-character window, and the prefix regex ends at T with a lookahead for /, so a non-empty suffix always starts at that separator — a user-controlled run of ≥ 39 characters is still scanned as a full window. Only ≤ 38-character runs lose one, and those are below the classifier's own minimum: the canonical redactor does not flag such a value standing alone either. Measured across run lengths 37–41, and over labelled-credential and base64-chunk placements at four positions under two prefixes — no value that redact() detects on its own becomes visible. The T/-borrowing window in the finding is not a key: two of its forty bytes are a fixed OS path.

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 T/ context re-redacts ~85% of opaque 38-character directory names while catching nothing the canonical policy would catch.

Pinned by TestMacosPrefixBoundary (mutation-checked: widening the exemption fails 14 of its assertions), and recorded in the helper's docstring.

Separately, the Design and First Principles reviews were right about /api/project/tree — the two root sites are folded in rather than left as a follow-up. Six absolute-path egress sites in the file, six fixed.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 30, 2026
@leonlaiyc
leonlaiyc force-pushed the fix/project-git-macos-path-redaction branch from 8e4b326 to 11b9d5f Compare August 30, 2026 13:05
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 30, 2026
leonlaiyc added a commit to leonlaiyc/KiroCrew that referenced this pull request Aug 31, 2026
`_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>
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@github-actions github-actions Bot added the readiness: action required A blocking check or review needs attention label Aug 31, 2026
@leonlaiyc

Copy link
Copy Markdown
Contributor Author

The blocking finding is accepted and the code is changed on cded733c. My earlier reply on this thread was wrong, and it was wrong in a way worth naming.

I argued the dropped window cost no detection because a ≤ 38-character user-controlled run sits below _looks_like_secret_key's exact-40 minimum. That reasoning does not hold: an AWS secret key may itself contain /, so the window the classifier actually evaluates is T/ plus the name, and whether that is a credential is a per-value question — not one settled by the length of the portion the user controls independently. The security invariant is defined over the egress string, and the split violated it:

key   = T/PtYgjmUhBel31iEl2hpChYgCfrL1spNxnyVmih   # _looks_like_secret_key -> True
path  = /private/var/folders/6r/<id>/T/PtYgjmUhBel31iEl2hpChYgCfrL1spNxnyVmih
redact(path)                != path      # canonical removes it
_redact_project_path(path)  == path      # the helper did not

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 inconvenientreturn redact(path) restores the #6905 defect this PR exists to fix, so it trades one output-correctness bug for another. The split moves one byte instead:

prefix = match.group(0)
return prefix[:-1] + redact(prefix[-1] + path[match.end():])

Everything above the trailing T is preserved; T and everything after it go to the canonical redactor. Every 40-character window redact() would evaluate over T/<suffix> is now evaluated. The high-entropy [a-z0-9]{2}/[a-z0-9_]{30} id still never enters the scan — that withholding is the whole exemption, so the false positive cannot return.

I also have to correct a measurement I gave you. I claimed a 2-character T/ boundary "re-redacts ~85% of opaque 38-character directory names (pytest temp dirs, hashes)". That number came from a uniformly-random base64 corpus and was generalised to corpora it had never been run against. Re-measured, 300 samples per corpus, under both a self-flagged and a non-self-flagged prefix:

corpus split at match.end() boundary T canonical redact()
ordinary project names 0 0 0
pytest temp-dir names 0 0 0
truncated sha-256 digests (38 hex) 0 0 297
uuid4 hex 0 0 297
uniformly-random base64, exactly 38 0 258 294
uniformly-random base64, 30–45 106 122 294

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 AssertionError: boundary credential survived: /private/var/folders/6r/zyxvp…. test_a_credential_spanning_the_prefix_boundary_is_redacted pins it, asserting both that the canonical redactor removes the value and that the helper does — so a regression cannot pass by making the canonical side stop firing. Two tests whose premise you overturned were rewritten rather than deleted: test_a_38_char_name_is_kept_and_was_never_classifiable is now test_a_38_char_name_is_judged_on_its_own_boundary_window, and the identity test tracks the moved split point. test_ordinary_project_names_survive_under_both_prefixes and test_the_os_owned_id_never_enters_the_scan pin the two directions against each other.

Green on cded733c: the seven redaction/path suites at 1107 passed, 19 skipped, 0 failed; flake8, isort, black baseline gate and mypy clean. The PR body's Boundary contract section has been rewritten to describe what actually ships, including both corrections above.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
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>
@leonlaiyc
leonlaiyc force-pushed the fix/project-git-macos-path-redaction branch from cded733 to a3b5ad1 Compare August 31, 2026 02:27
@leonlaiyc

Copy link
Copy Markdown
Contributor Author

Right again, and the fix is on a3b5ad1e. I reproduced your /T/-spanning key before changing anything:

/T/EqV8ib8HDy88YtDtXbiufMdI8X2Y4rUmer/BH    # _looks_like_secret_key -> True
canonical redact  : removes it
prefix[-1] split  : LEAKS  (both prefixes)
prefix[-2:] split : removes it

So cded733c had the same defect as 11b9d5fa, one byte further left, and my correction had only moved the boundary instead of fixing the argument behind it.

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 /. That is the reasoning both earlier heads rested on and it was wrong at both positions.

Applying it: the prefix regex ends in the fixed literal /T, and everything to the left of that literal is the OS-generated [a-z0-9]{2}/[a-z0-9_]{30} id. Scanning /T + suffix therefore covers every window composed entirely of fixed or user-controlled bytes — so 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. That is why this is not "one more byte" a third time; the boundary is now the end of the variable region rather than wherever the last review stopped.

I took your suggested prefix[:-2] + redact(prefix[-2:] + …) exactly.

Why still not the full revert. return redact(path) restores the #6905 defect: the id is high-entropy and self-flagging, so redact() removes it on its own account and every macOS temp-rooted project renders as [REDACTED: credential]. That is the 300/600 column below.

Measured cost of the second byte — 300 samples per corpus, both a self-flagged and a non-self-flagged prefix (n=600):

corpus prefix[-1] prefix[-2:] canonical
ordinary project names 0 0 300
pytest temp-dir names 0 0 300
sha-256 digests, 38 hex 0 0 300
sha-256 digests, 37 hex 0 0 300
uuid4 hex 0 0 300
uniformly-random base64, 37 0 506 562
uniformly-random base64, 38 512 518 571

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. test_a_credential_spanning_the_prefix_boundary_is_redacted now runs both fixtures (T/… and /T/…) under both prefixes, asserting each is a credential standing alone, then that canonical and the helper both remove it. Red-before against cded733c's helper: 2 failed, 18 passed. test_the_exemption_is_exactly_prefix_plus_canonical_suffix asserts the split point itself (boundary == "/T"), and the false-positive control gained a 37-character hex name — the length that reaches 40 only via /T/, i.e. exactly what this byte exposes.

Green on a3b5ad1e: the seven redaction/path suites at 1107 passed, 19 skipped, 0 failed; flake8, isort, black baseline gate, mypy clean.

Also squashed to a single commit — PR Hygiene was failing on the commit count (3), not on anything in the diff. The PR body's Boundary contract section is rewritten to the table above and carries both of my earlier corrections on the record.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 31, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

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. Both are correct alone and coupled once both land: the new path-shaped fields must adopt whichever path-aware redactor wins, so ordering and follow-up belong in one discussion. Files: src/kiro_crew/dashboard/handlers/files.py.
  • This PR is OVERLAPPING with PR #7678. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6905: MERGE_DISCUSSION. The merged dedup is what makes PR #6905 stale; the rebase is mechanical -- keep main's dict.fromkeys dedup and apply the wrapper only to result['root']. Files: src/kiro_crew/dashboard/handlers/files.py.
  • This PR is OVERLAPPING with PR #8055. 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.

@dwu96 dwu96 added the needs-pr-triage PR scanner: awaiting automated triage label Sep 7, 2026
@NicholasRBowers NicholasRBowers added drive-to-green PR claimed by drive-to-green pipeline and removed needs-pr-triage PR scanner: awaiting automated triage labels Sep 7, 2026
@NicholasRBowers

Copy link
Copy Markdown
Contributor

🤖 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 a3b5ad1e with regression tests, but the fork review lanes never restamped because CI never went green (stale-base failures in files this diff never touches, plus an infra fetch failure). Plan: rebase onto current main to resolve the conflict, which clears the inherited reds and re-fires the review lanes so GPT can restamp the fixed finding.

If you'd prefer I don't touch this PR, add the pr-no-autofix label.

@bolichen97

Copy link
Copy Markdown
Collaborator

@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 src/kiro_crew/dashboard/handlers/files.py (_project_git_branch repoRoot, api_project_git in both the 200 and 400 arms, api_project_git_status repoRoot, and both api_project_tree root returns), and both add tests to test/test_project_tree.py. The root cause is shared: redact()'s bare-secret run pattern includes /, so a slash-only path scans as one long run. The wrappers are mutually exclusive. Yours exempts the fixed Darwin /private/var/folders/<2>/<30>/T prefix and keeps canonical scanning over the rest, and TestMacosPrefixBoundary pins that a credential spanning a / is still redacted. #8055 splits on every /, which is more general (it also covers the repo-relative listing values in files[].path and paths[] that you leave on bare redact()), but a slash-bearing AWS key would be emitted raw.

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 dirty and 1243 behind, so whichever we take needs a rebase that keeps #7678's dict.fromkeys de-duplication near result['paths']. Also note draft #3987 adds new path-shaped git-status fields on bare redact() and must adopt the winner. Nothing here is superseded: main still carries the defect, and your restored happy-path tests and the docs/system-specs/modules/learn-cron-dashboard.md correction are worth keeping either way.

Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

drive-to-green PR claimed by drive-to-green pipeline fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants