fix(usage): log the transcripts the UNC gate refuses instead of a silent zero - #7285
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS A deliberately scoped observability fix — the silent zero gets a named cause, and the harder UNC-admission root fix is correctly deferred with its hazard documented. [DESIGN-REVIEWED] 2a31082 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All checks are done. The diff does exactly one thing, the description declares its symptom-level placement with a derived security reason for the deferral, First-Principles-Verdict: PASS One aggregated warning where a diagnosable silence was; the deferred cause (#8079) is named with a derived security reason, and nothing rides along. What this change shipsIntent: let an operator on a UNC roaming-profile home tell a refusal-zero on the usage page from a genuinely idle account — a FIX (of the silence half of #6733, with the wrong number explicitly deferred).
The harm is a reported defect, so the requirement is derived. Zero option: the silence continues and the operator has no thread to pull. No duplicate: Watch
[FIRST-PRINCIPLES-REVIEWED] 2a31082 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
4c156e0 to
bd515d4
Compare
bd515d4 to
b502e57
Compare
buluoray
left a comment
There was a problem hiding this comment.
Reviewed at b502e5763. 1 blocking / 0 yellow / 1 blue — comment only. The blocker is one stale sentence in the PR body, not a code or security problem; correct it and this is approvable as-is.
The security substance is sound
I reviewed this as the highest-risk change in the batch, since it widens a trusted root. All four axes came back clean:
- No keystone exposure. The new root
<kiro home>/sessions/clioverlaps nothing insecurity._SENSITIVE_HOME_DIRSor_CREW_SECRET_LEAVES, andunc_probe_allowedis not the read/write authorization gate —is_sensitive_pathstill runs post-resolution, sosecurity_policy.json,profiles/,admission_policy.jsonandcomputer_use.jsonstay unreadable and unwritable.settings/mcp.jsonstays refused, with a pinned test. Admission is the transcript dir exactly, not the kiro home. - The root match is not bypassable by a crafted path. It is
normcase(normpath(...)), which handles case folding, mixed separators and\\?\prefixes, and it refuses by default — so a crafted variant either lexically sits under the root or is rejected. Critically it is separator-boundary-anchored (cand == rootn or cand.startswith(rootn.rstrip("\\/") + os.sep)) rather than a bare prefix compare, so a sibling like<root>-evildoes not match. I mutated it to a barestartswithand bothneighbour_directory_*tests reddened, so that anchoring is load-bearing rather than incidental. - Junctions are handled. The new leaf-level
is_reparse_pointcheck uses islink plusst_reparse_tag, which is the partpath.is_symlink()alone would miss on Windows. - No deny coverage changed.
security.pyis untouched, so~/.kiroand the legacy~/.kirocrewcoverage is exactly as before.
All five mutations I ran reddened their target test and none proved inert: separator-anchor → bare startswith, dropping the sessions accessor, defeating the memo (3 == 1), removing the reparse guard, disabling the counter.
BLOCKING — PR body §3 describes a payload field the code deliberately does not add
The body currently says:
... and a
refused_transcriptscount in the payload, matching the machine-readablecode: sessions_dir_unreadablethis function already returns for the unreadable-dir case. The path stays server-side in the log; the payload carries only the count.
That is not what head does. refused_transcripts appears in usage.py only as a local (1772), an increment (1810), and two log arguments (1853, 1862) — it never reaches the return dict at 1895. The count is log-only, and both the code comment and test/test_usage.py:~112 state explicitly that it is not an API field, attributing that to the First Principles review on this PR.
So the body is stale from a pre-review revision. The shipped behavior is the more conservative of the two, which is why this costs nothing to fix: it is a one-sentence edit, not a code change. I am flagging it as blocking only because anyone reading the body — a reviewer, an operator, or someone writing a usage-page renderer — is being told the API exposes a field that does not exist.
Blue
hooks.py:1984 (validate_file_path) — the UNC gate is lexical on the raw path and realpath runs after admission, so a symlink or junction at a directory component of any trusted root (not just the transcript leaf) is still resolved through. This is pre-existing, applies equally to the data-home, temp and agents roots, is disclosed in docs/system-specs/modules/security.md, and tracked separately. The new leaf-level check is a net improvement, so this is context rather than a request.
Fix the §3 sentence and I will approve.
b502e57 to
b7d221d
Compare
Disposition for head
|
| trusted root | reparse check before validate_file_path at base |
|---|---|
| crew data home | none |
| temp directory | none |
| kiro agents dir | none |
| kiro-cli transcript dir | admitted by this PR, and this PR adds the check |
So the primitive GPT describes - plant a link under a trusted root, have
realpath follow it, gateway opens SMB to the attacker's host - is available at
base on three roots with no race to win. The attacker capability required to
exploit the window here (a same-user writer with Windows symlink privilege) is
strictly sufficient to exploit those three unconditionally. Admitting the fourth
root therefore grants no capability that base did not already grant more cheaply.
2. The prescribed fix subtracts protection. "Remove the sessions-root
admission" would revert the #6733 repair - on a roaming (UNC) home the usage page
counts zero sessions and reports a confident zero - while leaving the three
unchecked roots exactly as they are. The net effect is fewer working features and
the same exposure.
3. The correct fix is one change in pinned_fs, not a subtraction in one
consumer. Closing the race needs a no-follow open with the read taken from
the handle (CreateFileW + FILE_FLAG_OPEN_REPARSE_POINT on Windows; Python's
os.open exposes O_NOFOLLOW on POSIX only). Doing that once for every consumer
is the right shape; doing it inside this one consumer would leave the other three
roots unfixed and add a second spelling of the same mechanism.
Filed as #7505, which covers both halves - the unconditional gap on three
roots (the larger exposure, reachable without timing) and this TOCTOU refinement -
and records the ordering argument. The spec section this PR writes already said
the property "holds for the data home, the temp dir and the agents dir too ...
tracked separately rather than folded in here"; #7505 is that tracker, which
previously did not exist. That was a fair gap in the PR and it is now closed.
To be explicit about what this disposition is and is not: it is not a
deferral. I am not asking for a security defect to be accepted and fixed later
inside this diff's scope. I am arguing the finding is not attributable to this
diff - the diff's own root is the best-protected of the four, and the finding's
consequence chain runs identically at base through roots this diff does not
touch. If a maintainer reads that differently, the alternative I would take is
adding the no-follow open to pinned_fs in a separate PR and rebasing this one
on top of it, rather than dropping the transcript root.
Also on this head, unrelated to the above: Backend Tests shard 3 and
Frontend Lint & Type Check are red for main-owned reasons, filed as #7504
(log-site census has slack for dashboard/handlers/files.py, reproduced on a
frontend-only PR) and #7499 (main sits at 660 eslint warnings against a 659
ratchet, measured 660 with and without a branch diff applied). Neither is
actionable here. Coverage Gate and PR Readiness are downstream aggregates of
those two plus the GPT verdict.
First Principles CONCERNS on this head is still open and will get its own
disposition once the main-owned reds clear and the lanes settle on a stable head -
answering it against a head that is about to be rebased again would just be churn.
First Principles CONCERNS on
|
b7d221d to
c00d456
Compare
Handoff on the GPT block for head
|
c00d456 to
869b771
Compare
Update for head
|
| finding | raised on |
|---|---|
A -- hooks.py, global UNC allowlist exposes an SMB credential probe |
b7d221d3d, 869b7713e |
B -- usage.py, link check can be raced before UNC resolution |
b7d221d3d, c00d4568b |
Two distinct findings, two heads each. Neither is lane noise. The live block on this
head is A:
BLOCKING -- src/kiro_crew/hooks.py:1995 -- Global UNC allowlist exposes an SMB credential probe
> `_config_paths.kiro_sessions_dir,`
Agent-planted image symlink under the transcript root + channel message containing its
path -> build_prompt_blocks -> Path.is_file() follows the UNC target -> outbound SMB
authentication.
Fix: Remove the sessions directory from the global UNC roots.
What matters for the decision: both findings prescribe the same remedy. A says
"remove the sessions directory from the global UNC roots"; B said the same "until
transcript resolution refuses links atomically". Four review rolls, two independent
framings, one remedy. So the two paths already on the table are unchanged, and the
choice between them is now better specified:
- (a) Land an atomic no-follow open in
pinned_fsas a prerequisite, then rebase
this PR onto it -- keeps the transcript-root admission and closes the class for all
four roots. Satisfies A and B's shared "until it refuses links atomically" clause. - (b) Split this PR: drop the
kiro_sessions_diradmission, ship only the
non-silent-refusal logging. That is literally what both findings ask for, and it
still fixes half of Windows UNC-home: usage page silently counts zero sessions (validate_file_path refuses ~/.kiro/sessions/cli transcripts) #6733 -- the usage page stops reporting a confident zero with
nothing logged. The admission then lands after (a).
Base parity re-verified at the current base 556f08f93 (not restated from the earlier
note): this diff does not touch src/kiro_crew/acp/prompt_blocks.py,
is_reparse_point guards none of the data-home / temp-dir / agents-dir roots, and
validate_file_path contains no reparse check. The class is still unconditional at
base on three sibling roots, which is what #7505 tracks -- it argues for fixing the
class, not for admitting a fourth root while it is open.
Everything else on this head is clean: mergeable=true, one commit, Design and
Opus clean, zero unresolved review threads, and local gates green (black, flake8,
isort, mypy, plus the two touched test files at 188 passed / 1 skipped). The only reds
are the GPT lane and the PR Readiness status that aggregates it.
…ent zero On a Windows roaming-profile home the home directory is itself a UNC share, so `unc_probe_allowed` gates every path under it. It admits the crew data home, the temp directory and the kiro agents dir but not `<kiro home>/sessions/cli`, so `usage._parse_sessions` has every kiro-cli transcript refused by `validate_file_path` and `continue`s on `None`. The usage page reported a confident zero with nothing anywhere to say why. Count those refusals and emit one aggregated warning naming the directory. It is aggregated after the loop rather than logged per file because that failure mode refuses all of them at once. The count is deliberately not a response field while no renderer reads it, and the record is server-side only because `%s` of a `Path` is a filesystem path the returned payload never carries. This does NOT make the count correct on a UNC home. The page still reports zero there; it just stops reporting it silently, which was the second half of the reported defect. Admitting the transcript directory to the UNC gate is what would restore the count, and that is deferred to #8079 with the prerequisite recorded: the gate is lexical on the RAW path, so admitting the root admits any link planted under it, and the window is `check -> realpath` rather than `check -> open` -- `validate_file_path` resolves immediately after admitting and that resolution is itself the outbound probe. A no-follow open therefore cannot close it, and `getattr(os, "O_NOFOLLOW", 0)` is 0 on Windows, the only platform where UNC applies. It needs an atomic no-follow resolution in `pinned_fs` first. Refs #6733 Refs #8079
869b771 to
2a31082
Compare
iamwhatever
left a comment
There was a problem hiding this comment.
Tier 1 auto-approve: fix (2 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: counts transcripts refused by path validation in _parse_sessions and emits one aggregated server-side warning, so a UNC/roaming-profile home no longer renders an indistinguishable confident zero; no payload field added, no change to the validator itself.
Problem / Motivation
On a Windows roaming profile the home directory is itself a UNC share, so
hooks.unc_probe_allowedgates every path under it. It admits the crew data home,the temp directory and the kiro agents dir, but not
<kiro home>/sessions/cli.So
dashboard.handlers.usage._parse_sessionshas every kiro-cli transcript refusedby
validate_file_path,continues on theNone, and the usage page renders aconfident zero with nothing anywhere -- log, payload or UI -- to say why.
That is two harms: a wrong number, and silence about it. This PR fixes only the
silence. The scope was reduced during review; see "What is deliberately not here".
Why it matters
A zero on the usage page is indistinguishable from an idle account. An operator on a
roaming profile has no way to tell that the page is reporting a refusal rather than
a fact, and no thread to pull -- nothing is logged, so there is nothing to search
for. After this change the same zero is accompanied by a server-side warning naming
the directory and the number of transcripts excluded, which is enough to recognise
the platform problem and find #8079.
What changed
One counter and one log record in
_parse_sessions:validate_file_pathare counted instead of silently skipped.logger.warningreports the count andthe directory.
Three deliberate details:
so a per-file record would emit one line per session file for a single cause.
ship an unused API surface; the test pins that it stays absent.
%sof aPathis a filesystem path, and the returnedpayload deliberately never carries one (same reason as the existing
iterdirerror handler just above).
Tests
test/test_usage.py, run withpytest test/test_usage.py -n 2-- 91 passed.test_refused_transcripts_are_reported_not_swallowed-- refusals are counted andone aggregated warning is emitted; also pins that the count is not a payload
field.
test_no_refusal_log_when_every_transcript_validates-- the negative case, so thelog cannot become unconditional noise.
Mutation-verified rather than assumed: replacing
if refused_transcripts:withif False:turns the first test red (assert 0 == 1) and leaves the second green,which is the discrimination you want from the pair. Also clean:
flake8,isort,mypy, andscripts/check_black_formatting.py.What is deliberately not here
The UNC-gate admission for the transcript directory is not in this PR. The page
still reports zero on a roaming-profile home. Tracked in #8079, with the reason it
cannot be a small change recorded there:
unc_probe_allowedis lexical on the RAW path -- it never touches the filesystem,because touching a UNC path is the outbound SMB probe the gate exists to prevent.
So admitting the root admits any link planted under it, and the reader then resolves
that link. The window is
check -> realpath, notcheck -> open:validate_file_pathcallsos.path.realpathimmediately after the gate admits, andthat call is the probe. A no-follow open is too late, and
getattr(os, "O_NOFOLLOW", 0)is 0 on Windows -- the only platform where UNCapplies. Closing it needs an atomic no-follow resolution in
pinned_fs(
CreateFileW+FILE_FLAG_OPEN_REPARSE_POINT), added once so everyvalidate_file_pathconsumer benefits.An earlier revision of this PR admitted the root and guarded the reader with an
lstat-based reparse check. Review found that shape reachable from two directionsand both findings prescribed the same remedy -- remove the sessions directory from
the global UNC roots until resolution refuses links atomically. That is what this
revision does, so neither finding's span exists in the diff any more: the roots
tuple is untouched and the pre-validator check is gone.
hooks.py,docs/system-specs/modules/security.mdandtest/test_acp_prompt_blocks.pyareback to their base contents and no longer appear in the diff at all.
Pattern harvest
Rule candidate: review-prompt
Pattern: a gate that signals refusal by returning a sentinel, consumed by a bare
continueinside an aggregation loop -- so a total refusal renders as alegitimate empty result.
This defect was not a typo; it is that shape.
validate_file_pathreturnsNoneto mean refused, the loop did
if resolved_str is None: continue, and the functionreturns a count. One refused file is noise worth skipping; every file refused is a
platform misconfiguration, and both compile to the same rendered
0. The tell isstructural and reviewable: a sentinel-returning validator whose
Nonebranch has nocounter, no log and no distinct return, inside a loop whose product is an aggregate.
Not proposed as semgrep, deliberately: the rule needs to know which callee is a
gate rather than an ordinary lookup that may legitimately miss, and that is
project knowledge rather than syntax. A review prompt can ask the question ("this
loop skips on a sentinel -- what does the caller see when EVERY item skips?") where
a syntactic matcher would mostly fire on benign
dict.getmisses.Refs #6733
Refs #7505
Refs #8079