fix(portability): keep the export archive inside the crew directory - #9070
Conversation
`create_export_zip` walks `workspace`, `plan_memory` and `skills` with
`rglob("*")` and skips an entry that `is_symlink()`, so that an export never
carries bytes from outside the crew directory. That skip is blind to the escape
that costs the most: `rglob` DESCENDS a directory link, and the file on the far
side is an ordinary one — `is_symlink()` false — so it is written straight into
the archive. On Windows the link is typically a junction, which `is_symlink`
does not report at all, and a junction needs none of the privilege a directory
symlink needs there.
The two filters below the skip do not catch it either. `_is_excluded` and
`is_sensitive_path` both read the LEXICAL path, which runs through the link's
own name and therefore looks like ordinary crew content — so a sensitive file
behind a link passes the sensitivity check that exists to stop exactly this.
Measured on unpatched main with a real junction: `workspace/memory/linked/
not-ours.md` appeared in the export's namelist, sourced from outside the crew
directory. The consequence class is content the user then hands to someone else,
since an export archive is made to be moved off the host.
Fixed by requiring the RESOLVED path to stay under the resolved crew directory —
the repo's existing idiom (`apps/backend.py:651`). Resolving is what covers both
shapes at once: it follows every reparse point on the way down and answers where
the bytes actually live. `mc` is resolved once so a legitimately linked
`$KIROCREW_HOME` does not reject the whole export. The `is_symlink()` skip is
kept as-is; this adds a layer rather than replacing one.
The import side of this module is untouched.
`test_export_skips_symlinks` cannot cover this — it plants a link that IS the
entry, and it `pytest.skip`s where symlinks cannot be created, i.e. on the
platform where the junction spelling lives. The new tests use
`conftest.make_dir_link` and need no privilege.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Design Review (Fable 5.1, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound fix on a real boundary (workspace-writable-by-agent + Windows SMB probe), reusing existing pinned-fd primitives — but the description contradicts the shipped diff on a security-relevant hunk. Watch
Suggestions
[DESIGN-REVIEWED] 82700d5 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsFINDING -- src/kiro_crew/platform_compat.py:3853 -- |
First Principles Review (Fable 5.1, fork) — 🟡 CONCERNSPremise-level review of First-Principles-Verdict: CONCERNS Every item traces to a named harm on a real boundary (export leak, agent-tool write race, Windows UNC auth probe) — but a second defect fix and a one-caller What this change shipsIntent: stop
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 82700d5 |
|
Addressing the exact-head F1 — TOCTOU at Reproduced against the shape Streaming rather than reading whole is deliberate — a workspace file has no size bound here. That is also why FINDING — the One more thing that was green for the wrong reason. The walk-descent test guarded itself with |
3325f5f to
8a74e97
Compare
|
Both exact-head F1 — hardlinked alias. Correct, and it is the sharper version of the point I only half-made. F2 — ZIP64. Confirmed against CPython's Red-before, measured against Both pass on this head; The hardlink test guards itself through four oracles outside the module — the alias is not a symlink, its Design and First Principles also name sibling check-to-open sites elsewhere in this module. They are advisory and I have deliberately left them — widening this PR to silence advisory comments is how a bounded security fix stops being reviewable. Happy to take them as a follow-up. Amended rather than stacked, to stay inside the two-commit rule; the PR body is re-synced to what actually ships. |
8a74e97 to
4f5e14d
Compare
4f5e14d to
be3caf3
Compare
|
The exact-head security The finding, confirmed. The fix, and where it deviates from the prescription. The suggested remedy was "open through a
So the same policy is applied through the same primitives, scoped to this input: Cost, stated exactly. Nothing outside the crew directory is newly dropped — containment already refused that. Content still inside it is either walked under its own name anyway, or, if it lives in a part of the crew directory the export does not walk, is now left out on Windows — which is already the behaviour on POSIX, where Red-before, on the property rather than a proxy. The five surrounding tests passed unchanged in that same run — they are controls, and a control going red would mean the screen was being credited with something it does not do.
What this does not close, so it is not read as more than it is. The screen covers the read. It does not cover the enumeration: The descriptor-pinning regression test moved its swap site to the crew root itself, since the new screen makes a link below the root unreachable on Windows; the root is the one component deliberately outside the screen and therefore exactly where a swap can still land, which keeps that property covered on both platforms instead of on POSIX alone. The PR description is re-synced to what actually ships. 🤖 Generated with Claude Code |
be3caf3 to
5663dd3
Compare
|
The exact-head security The finding was right, and here is the measurement rather than a concession. Same racing writer, same instant (the swap fires from inside the walk immediately after the targeted component is verified and before the next filesystem call), run against both builds: Arm A is the whole finding in three lines: the guard worked — nothing was archived — and the probe went out anyway. The primitive already existed; this reuses it rather than inventing one.
The leaf. Windows has no Three boundaries, stated because getting one wrong is worse than the defect. The walk starts at the resolved root — Coverage is on the property, not the mechanism.
What this still does not close, so it is not read as more than it is. The pins cover the read. They do not cover the enumeration: 🤖 Generated with Claude Code |
5663dd3 to
651241f
Compare
|
The exact-head security The finding, reproduced before anything was changed. Every resolving call was wrapped and an export run over a workspace holding one pre-planted junction: Both builds archive nothing from behind the junction. That is exactly why "the file was not archived" cannot be the test: if the junction names The ordering you prescribed is what shipped.
The measurement that made step 3 possible, since it is the load-bearing one and I did not want to assert it from documentation:
So
The regression asserts on the calls, not the archive.
Everything you listed as preserved is pinned and green: hardlink refusal, ZIP64 streaming, descriptor-bound archive bytes, ancestor-swap protection ( POSIX is not handed Windows semantics. The walk pins on both platforms, but only half the property is real there: The exported set is byte-identical, measured rather than argued: on a tree covering nesting, One pre-existing defect found while measuring that, and deliberately NOT fixed here. Amended rather than stacked, to stay inside the two-commit rule; the PR body is re-synced to what actually ships, and the residual paragraph it used to carry is gone because the residual is. |
651241f to
f1dc774
Compare
|
The cause is not a typo, and it is worth stating precisely because it hid from every local gate: CPython reworded this exception between the versions in play. 3.10 raises The repair is one line plus the reason:
GPT's advisory on Amended rather than stacked, to stay inside the two-commit rule. |
`create_export_zip` walked `workspace/`, `plan_memory/` and `skills/` with
`rglob("*")`, which DESCENDS a directory link while the file on the far side
is an ordinary one, so the walk's own `is_symlink()` skip never fired for it.
Neither by-name filter below that skip is a containment test either:
`_is_excluded` is a rule about the archive name, and `is_sensitive_path` asks
whether a path is a PROTECTED location -- it does resolve links, so a linked
`~/.aws` was caught -- never whether it is inside the crew directory. An
ordinary file of the user's, reached through a link dropped in the workspace,
was sensitive to none of them and went into an archive the user hands on.
A resolve-and-compare would answer that for an instant only: `ZipFile.write`
takes a NAME and opens it again, so the file that was checked and the file
that is read are two separate lookups, and a running gateway gives agent
tools write access to that workspace while an export can be triggered.
`_open_verified` inverts the order -- open first, then ask the kernel where
the open thing is via `pinned_fs.fd_real_path` -- and `_add_from_fd` streams
the entry from that descriptor, so check and use address one object. A
hardlink alias, which no path-based guard can see, is refused on the
descriptor's link count; `force_zip64=True` keeps a source over `ZIP64_LIMIT`
archiving as `ZipFile.write` used to.
That settles what may be ARCHIVED. It does not settle what may be TOUCHED,
and on Windows those are different questions: resolving a path whose
component is a link to a UNC share IS an outbound SMB authentication, so a
refusal computed afterwards has already paid the cost it exists to prevent.
No pathname check fixes it -- every by-name check is a check-to-open window
and an adversary that can plant the link chooses when. The names have to be
held rather than inspected.
The enumeration was the last place that still inspected them. Pinning the
candidate's ancestors closed the swap race, but `rglob` had already descended
a PRE-PLANTED junction, and `is_file()` and `is_sensitive_path()` had already
resolved what it yielded, before any of it ran. Measured on this module,
exporting a workspace holding one planted junction: 14 resolving calls went
out through it -- `os.path.realpath` and `ntpath._getfinalpathname` -- while
the export correctly archived nothing from behind it. Nothing being archived
was never the question.
So the walk descends and verifies in one motion instead. `_walk_pinned` pins
a directory with `platform_compat.pin_directory` before listing it -- the
handle omits `FILE_SHARE_DELETE`, so a held directory can be neither renamed
nor deleted, nor can anything above it, and the open refuses to follow a
reparse point, so a junction at the name fails there instead of being
traversed -- then classifies each child from the listing Windows already
returned, where the attributes and reparse tag arrive inline and cost no
lookup THROUGH the child. A child directory is descended only by pinning it
in turn, so the path the kernel walks to reach any component runs entirely
through components already opened and verified, starting at the crew root
itself. This is the pattern `aws_control/backend/storage.py` already uses to
hold the path a sandboxed CLI writes through. The leaf goes through the new
`platform_compat.open_file_no_reparse`, which opens a reparse point AS ITSELF
rather than following it, so the refusal and the open are one operation;
POSIX keeps the `O_NOFOLLOW` open it already had, named.
`_pin_ancestors` and `_open_inside` are gone rather than kept alongside this:
the walk holds the same chain, for the whole subtree instead of re-walking it
per file, so keeping them would be two mechanisms doing one job.
The walk starts at the RESOLVED root, since `$KIROCREW_HOME` may itself be a
link and `pin_directory` refuses a reparse point at a name -- pinning the
configured spelling would return an empty export on such a host.
The exported set is unchanged: on a tree covering nesting, `EXCLUDE_DIRS`,
`EXPORT_EXCLUDE`, `.pid` and `skills/auto`, the archive is byte-identical to
the one the previous walk produced.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
f1dc774 to
82700d5
Compare
|
The exact-head security The body argued it was a separate defect on a separate path that would make a five-times-blocked security fix unreviewable. The reviewer's answer is the better one: in a change whose entire subject is "stop the export packaging bytes it should not", shipping a known path that puts The defect. The fix is the reviewer's, verbatim: On the archive name specifically: Correction to the body, since it now says something false. It claimed the exported set was "byte-identical" before and after this PR. That was true of the walk rewrite and is no longer true of the PR: on Windows this change now removes files from the archive — every nested Regression, and it is platform-independent. Red-before with only Amended rather than stacked, to stay inside the two-commit rule. |
Problem / Motivation
portability.create_export_zipwalksworkspace/,plan_memory/andskills/withrglob("*")and writes each file into an archive the user downloads and hands on.The walk skipped an entry that is a symlink — but
rglobdescends adirectory link, and the file on the far side is an ordinary one, so that skip never
fired for it. Measured:
workspace/memory/linked/not-ours.md, living outside thecrew directory entirely, appeared in the export's namelist.
Correction to this PR's earlier description. It said
_is_excludedandis_sensitive_path"both read the LEXICAL path". That is wrong about the second one,and a reviewer was right to flag it:
is_sensitive_path→_path_in_home_dirs→_candidate_formsbuilds_resolved_forms_bounded(expanded), so it does resolvelinks — a linked
~/.awsreally was caught. The accurate statement is narrower anddifferent in kind: neither filter is a containment test at all.
_is_excludedisa rule about the archive name;
is_sensitive_pathasks whether a path is aprotected location, never whether it is inside the crew directory. An ordinary
file of the user's — notes, documents, anything under a link somebody dropped in the
workspace — was neither, and went into the archive.
A resolve-and-compare is not enough, and the first commit here only did that.
ZipFile.writetakes a name and opens it itself, so the file that was checked andthe file that is read are two separate lookups. Measured against the shape
mainships:
And a refusal computed after the open is already too late on Windows — the
third finding.
_open_insidecalledos.open(candidate, ...)and only then askedfd_real_pathwhere the descriptor had landed. Opening a path whose ancestor is alink to a UNC share is itself an outbound SMB authentication as this process, so
the containment, sensitivity and hardlink checks were all correct and all ran one
syscall too late. This is the ordering
hooks.validate_file_pathstates as its owncontract — the Windows UNC and linked-ancestor gates run before any resolution,
"because
realpathon a UNC path is itself the outbound SMB probe".And a pathname screen could not fix it — the fourth finding, which is what this
revision answers. The previous head refused a candidate whose ancestor was a
reparse point, checked with
lstat. A reviewer was right that this is stillraceable: any check of a name is a check-to-open window, and an adversary who can
plant the link chooses when. Measured, not conceded — the same racing writer,
run at the same instant, against both builds:
Arm A is the finding in three lines: the guard worked — nothing was archived — and
the probe went out anyway.
And the ENUMERATION was still probing before any of that ran — the fifth finding,
which this revision answers. Pinning the candidate's ancestors closed the swap
race, but a junction already sitting in the workspace when the export starts was
never raced for:
rglobdescended it, andis_file()andis_sensitive_path()resolved what it yielded, before
_open_insidewas ever reached. Measured bywrapping every resolving call and exporting a workspace holding one planted junction:
Both builds archive nothing from behind the junction, which is exactly why the
archive cannot be the oracle. If the junction names
\\host\share, those 14resolutions are the outbound SMB authentication, and
return Noneafterwardscannot recall them. The previous revision disclosed this as an unclosed residual; it
is closed here.
Why it matters
An export is a file the user deliberately hands to someone else — another machine, a
support thread, a backup. Content that was never theirs to share leaves the host with
no recovery once the archive has left.
The race is not theoretical here: a running gateway gives agent tools write access to
the same workspace the export walks, and an export can be triggered while they are
working. Under the race the harm is also strictly worse than under the plain blind
spot, because the sensitivity gate is one of the checks the swap gets to skip — the
name is validated, and something else is read.
The ordering finding is a different kind of harm rather than more of the same: a leaked
NTLM exchange is not something a later
return Nonecan take back, and it costs theattacker nothing but a junction in a directory their tools can already write to.
What changed (motivation → approach → change)
Root cause: the check and the use are two lookups of one name, and no lookup was
asking about containment in the first place.
Approach: resolve once, open once, address everything downstream through the
descriptor — the discipline
pinned_fsalready exists for in this repo. This changeconsumes its
fd_real_pathrather than adding a second mechanism._open_verified(target, root_real)— opens withO_NOFOLLOWwhere itexists, rejects a non-regular file on the descriptor's own
fstat, then asks thekernel where the open descriptor actually is (
/proc/self/fd,F_GETPATH, orGetFinalPathNameByHandleW) and compares that against the crew root.is_sensitive_pathis re-asked about that real path, so the protected-locationrule is not the one check the swap defeats. Every failure route — unreadable,
non-regular, unknowable real path — returns
None, never a fallback to thepathname.
_add_from_fdstreams the entry from that descriptor instead of lettingZipFile.writereopen the name. Streaming rather than reading whole is deliberate:a workspace file has no size bound here and the export used to copy one of any
size. This is also why
hooks.safe_read_file_bytes_nolink— which performs exactlythe right validation — was not reused: its 50 MB cap would silently drop a large
workspace file from the export.
fstat, so hand-building theZipInfodoes not quietly lose whatwriteused to record.os.path.realpath(mc)), since$KIROCREW_HOMEmay itself legitimately be a link and the test must not reject thewhole export because of it.
known to be unwanted, with a comment saying what each one actually decides.
Three things the descriptor alone does not settle, all raised in review and all fixed
here:
target's inode, so there is no symlink for
O_NOFOLLOWto refuse,is_symlink()is False, and
fd_real_pathreports the path the descriptor was opened by — theinnocent workspace alias — not a canonical one.
is_sensitive_pathis then askedabout the alias and answers "not sensitive" while the bytes behind it are a
credential file's. Only the link COUNT, read off the descriptor, sees it, so
st_nlink > 1is refused. Same rule and same reasoning aspinned_fs.refuse_hardlink_aliasandhooks.safe_read_file_bytes_nolink; the costis honest and small — a workspace file that legitimately has a second link is left
out of the export.
force_zip64=Trueon the streamed entry.ZipFile.writestat'd the source andturned ZIP64 on by itself; a streamed entry does not know its size when the header
is written, so without this a source over
zipfile.ZIP64_LIMITraisesRuntimeErrorpart-way through and the export endpoint answers 500. A multi-GiBfile under
workspace/is ordinary — a dataset, a model artifact — and it used toarchive fine, so leaving this off would trade one defect for a regression.
_walk_contained/_walk_pinnedreplacerglob. A directory is PINNED withplatform_compat.pin_directorybefore it is listed, and each child is thenclassified from the listing itself (
_entry_is_link) rather than by a call thatresolves it: on Windows
FindFirstFileWreturns the attributes and the reparse taginline, so a child's type and link-ness cost no lookup THROUGH that child. A child
directory is descended only by pinning it in turn, so the path the kernel walks to
reach component n runs entirely through components already opened and verified —
starting at the crew root itself, which the kernel walks to reach every candidate.
pin_directorysupplies both halves on Windows: its handle omitsFILE_SHARE_DELETE, so a held directory can be neither renamed nor deleted — norcan anything above it — and its open refuses to follow a reparse point, so a
junction sitting at the name fails there instead of being traversed. This is not
a new mechanism:
aws_control/backend/storage.pyalready pins root-then-child forexactly this reason — "a watcher can no longer rename the directory away and plant a
junction at its name between our create and the CLI's open".
_pin_ancestorsand_open_insideare deleted rather than kept beside it. Thewalk holds the same chain, for a whole subtree instead of re-walking it once per
file, so keeping both would be two mechanisms doing one job.
_entry_is_linkrefuses every reparse point rather than only link-shaped ones — which is precisely
what
pin_directoryandopen_file_no_reparsealready refuse, so it classifieswhat the opens would reject anyway: a cheap skip, never the enforcement.
DirEntry.is_symlink()alone would not do: a junction's tag isIO_REPARSE_TAG_MOUNT_POINT, notIO_REPARSE_TAG_SYMLINK, so that method answersFalse for one — which is how
rglobcame to descend it in the first place.is_sensitive_pathis no longer asked about the PATHNAME. That call was theprobe — 12 of the 14 measured resolutions. It is removed, not relocated: the same
question is still asked in
_open_verified, of the descriptor's real path, whichwas always the load-bearing one because a name can be re-pointed and a descriptor
cannot.
platform_compat.open_file_no_reparseopens withFILE_FLAG_OPEN_REPARSE_POINT, soa reparse point at the final name is opened as itself and refused (
ELOOP) ratherthan followed — Windows has no
O_NOFOLLOW, soos.openfollows one. The attributeis read off the descriptor, which makes it a fact about what was opened rather than
a prediction about what a later open will find. It shares the
CreateFileWwiringwith
pin_directoryvia one private helper, so the two do not carry separate copiesof the same security-critical flags. On POSIX it is the
O_NOFOLLOWopen the codealready had, named.
Three deliberate boundaries, each stated because getting one wrong is worse than the
defect:
pin_directoryrefuses a reparse point ata name, so pinning the configured spelling of
$KIROCREW_HOMEwould fail on a hostwhere the crew directory is itself a link — and return an empty archive rather
than a safer one. A silently empty backup is worse than the leak. Resolving first
means the chain that gets pinned is the real one; the crew root's own ancestors are
configuration this export has already read through.
only half the property is real there:
pin_directoryon POSIX isO_RDONLY | O_DIRECTORY | O_NOFOLLOW, whose refusal of a symlinked directory isgenuine and is exactly what
rglobalready did — so the exported set is unchanged —while the anti-rename half has no POSIX equivalent and nothing here claims it. The
Windows-only tests say so in their skip reason. Containment on POSIX rests where it
always did, on
_open_verifiedchecking the descriptor's real path; resolving asymlink there is local and leaks nothing, so there is no outbound probe to prevent.
components the export itself enumerated, so it is UNC-shaped only when the root is —
and refusing on that would break a deliberately configured UNC home for nothing.
What it costs, stated exactly. Nothing outside the crew directory is newly
dropped — containment already refused that. Content still inside it is either walked
under its own name anyway, or, if it lives in a part of the crew directory the export
does not walk, is left out on Windows — which is already the behaviour on POSIX, where
rglobnever descends a directory link. The walk holds one descriptor per directoryfor as long as that directory's subtree is being produced — depth, not breadth, bounds
how many are open at once, and it is strictly fewer opens than the previous revision,
which re-pinned the whole ancestor chain once per file. Each is released in a
finallythat also runs if the consumer abandons the walk; a test pins that they arereleased even when a component mid-chain refuses, because a handle leak here would
exhaust a large export rather than merely look untidy.
The exported set is unchanged by the walk rewrite, measured rather than argued:
on a tree covering nesting,
EXCLUDE_DIRS,EXPORT_EXCLUDE,.pidandskills/auto, the archive namelist and manifest counts were byte-identical to theones the previous walk produced. The exclusion fix below then deliberately CHANGES
that set on Windows — see the next section.
The Windows exclusion bypass — disclosed here first, then fixed here. An earlier
revision of this description scoped this out as a separate defect. A security review
disagreed and was right, so it ships in this PR.
_keep_for_exportasked_is_excluded(PurePosixPath(str(rel))). On Windowsstr(rel)is backslash-separated, soPurePosixPathparses the whole relative pathas a SINGLE component:
.namebecomesworkspace otes\.envand.partshaslength one, so the
EXPORT_EXCLUDEbasename set and theEXCLUDE_DIRSwalk bothstop matching.
workspace/notes/.env— a credential file this export exists to keepout — went into the archive the user downloads and hands on.
Fixed with
PurePosixPath(*rel.parts)for the filter andrel.as_posix()for thearchive name. Measured on the same fixture tree as the parity run above:
So the exported set does change on Windows, and that is the fix: every nested
.env,.local_secret,sel_hmac.key,telemetry_salt,*.pid, and everythingunder
snapshots/,outbox/,uploads/and__pycache__/now stays out. POSIX isunaffected —
str(rel)andpartsalready agreed there.On the archive name:
ZipInforewritesos.septoday, so that half fixes no livebug; it stops the member name depending on that behaviour, next to a filter that must
not.
Tests
TestTheAncestorSwapIsRefusedNotDetected(new — replaces the screen's tests):test_a_racing_writer_cannot_swap_a_pinned_ancestor— the blocking finding's ownscenario, executed rather than described. The swap is performed from inside
pin_directory, immediately after the targeted component has been verified andpinned and before the next filesystem call — the precise instant a real racing
writer would aim for, rather than a thread that has to get lucky. Two assertions,
both load-bearing: the rename must fail, because that is the property (the
export no longer depends on nobody having swapped the directory, but on nobody being
able to); and the descriptor must still come back on the genuine bytes, because a
guard that closed the race by refusing everything would pass the first assertion and
be useless.
test_the_same_rename_succeeds_once_nothing_is_pinned— guards that guard: therefusal above is the pin, not a filesystem that could never have done the rename.
test_a_reparse_point_at_the_leaf_is_refused_without_being_followed—open_file_no_reparserefuses the link, and the guard-the-guard then proves thatexact name is traversable by an ordinary
os.open, so the refusal cannot bemistaken for the link being unopenable.
test_an_ordinary_file_still_opens_through_the_no_follow_open— the descriptor isthe one
_add_from_fdstreams from, and on Windows it is now aCreateFileWhandlewrapped in a CRT descriptor. The test exercises
st_nlink(the hardlink rule readsit off this handle),
lseekanddup+fdopen— exactly the operations a shallowtest would miss and the archive would break on.
test_every_pin_is_released_even_when_a_component_refuses— a component mid-chainis made to refuse and every pinned descriptor must still be closed.
TestNothingIsProbedThroughAPlantedReparsePoint(new — the fifth finding):test_a_pre_planted_junction_is_never_resolved_during_an_export— the assertionis on the CALLS, not on the archive. A junction is planted under
workspace/before the export runs; every resolving call (
os.stat,os.open,os.listdir,os.scandir,os.path.realpath/isfile/isdir/exists/getsize, andntpath._getfinalpathname) is wrapped, and the test fails if any of them names thejunction or anything beneath it.
os.lstatandos.path.islinkare deliberatelynot in that set — they do not follow the final component, so naming the link with
one traverses nothing. An audit hook covers
os.scandir/os.listdiras well,because
pathlibbindsscandirat import time and patchingos.scandirdoes notsee
rglob's own descent — the single most important call to catch. Two negativecontrols sit beside it: the genuine
workspace/keep.mdmust still be archived (aguard that refused the whole tree would otherwise pass), and the loot must still be
absent.
test_the_recorder_actually_catches_a_probe— guards that guard. It performs thethree calls the previous build made on that exact path (
is_file(),realpath(),rglob) and asserts the recorder saw both arealpathand thescandirdescent. An oracle that can never fire proves nothing, and this one isthe whole test.
is_symlink()must be False (it really is a junction, which is whyrglobwalked in),
is_link_or_junctionmust be True, and the far side must really bereadable through the link.
Red-before for these two: with only
portability.pyreverted to5663dd30aandthe new tests kept,
test_a_pre_planted_junction_is_never_resolved_during_an_exportfails with
AssertionError: the export resolved a path through the planted junction (14 calls), listingos.path.realpathandntpath._getfinalpathnameon the junctionpath. The recorder test passes on both builds, as it must — it tests the oracle, not
the fix.
The tests that depend on the rename lock are marked Windows-only with the reason
stated: the walk pins on both platforms, but the anti-rename half is a share-mode
property (
FILE_SHARE_DELETEomitted) that POSIX has no equivalent of.TestALinkedCrewRootStillExports:test_a_crew_root_reached_through_a_link_still_packages_its_workspace— drives thepublic
create_export_zipwith$KIROCREW_HOMEon a real link. This is theregression the resolved-root rule exists to avoid, and it would otherwise be silent.
TestTheArchivedBytesAreTheValidatedBytes(same assertions, retargeted from_open_insideto_open_verifiednow that the pins live in the walk):test_retargeting_the_link_after_validation_cannot_change_what_is_archived(thedescriptor race, with the swap at the crew root — the one component outside the pins),
test_a_sensitive_target_is_refused_on_the_descriptor,test_a_file_reached_through_a_directory_link_gets_no_descriptor,test_a_real_file_inside_the_crew_dir_gets_a_descriptor_on_its_own_bytes,test_a_hardlinked_alias_gets_no_descriptor(+ its single-link control),test_a_streamed_entry_larger_than_the_zip64_limit_still_exports(+test_the_zip64_guard_can_actually_fail),test_containment_fails_closed_when_the_real_path_is_unknowable,test_a_directory_never_yields_a_descriptor.TestExport:test_export_does_not_package_files_reached_through_a_directory_link(Windows-marked, with its guard-the-guard walking from the root the production walk
starts at),
test_export_preserves_the_mtime_of_what_it_packages,test_export_still_packages_a_real_nested_workspace_file.Red-before is the A/B in Problem / Motivation, run against
be3caf39bloadedside-by-side with this build and driven by the identical attacker: on the previous
head the racing rename succeeded, the junction was planted, and
os.openwas stillcalled on the candidate; on this head the rename is refused with
WinError 32and thegenuine bytes come back. The same swap, injected through the shipped test, is
test_a_racing_writer_cannot_swap_a_pinned_ancestor.Gates.
test/test_portability.py78 passed / 4 skipped on Windows against realjunctions.
pin_directory's existing consumers re-run clean:test_platform_compat.py,test_aws_control_storage.py,test_pinned_staging.py=383 passed / 72 skipped, the single failure being
TestFindPythonInterpreterReal::test_version_gate_ignores_a_sitecustomize_decoy_on_pythonpath,reproduced with this branch's production files reverted to
be3caf39b— it is anartefact of the
PYTHONPATHoverride this Windows box needs to run the suite at all,which is the very thing that test is about.
mypy --platform linuxreports 0 errorsin
portability.py(the 4 it finds are pre-existing indashboard/state.pyand oneother, reached by import following).
black --checkclean onportability.py;test/test_portability.pyis in the repository's black baseline (1164 known-unformatted files) and
scripts/check_black_formatting.pypasses, so it isdeliberately not reformatted — doing so would bury this diff under an unrelated
rewrite of its SQL fixtures.
flake8andisort --check-onlyclean on both;scripts/scrub-lint.sh --no-historypasses.Manual verification
N/A — unit coverage sufficient: the whole defect is the relationship between the
containment decision and the archive read, and the tests drive both against a real
junction rather than a mock, including through the public
create_export_zip. Theordering property is asserted on the syscall itself rather than on an observable
side effect, which is what a manual run could not have shown either.
Related Issues
None — found by inspection while auditing
is_symlink-based containment guards forthe Windows junction blind spot, the same family as #7881.
Pattern harvest
Rule candidate:
review-promptPattern: a containment guard that validates a path and then reopens it. Two
distinct failures ride together — a recursive walk descends a directory link and the
leaf beyond it carries no mark (a Windows junction carries none anywhere), and
resolve()-then-open(name)is a check-to-use window. Where a walk feeds somethingthat reads, writes, moves or executes the result, the fix is not a stricter name
check: open once and address the descriptor. A second lesson for review prompts:
"filter X already resolves links" does not mean filter X answers containment — ask
which question each filter actually answers before crediting it with an unrelated one.
Third lesson, and the one this PR was blocked twice to learn: on Windows a correct
refusal computed after the open is still a leak, and no pathname check can fix it.
Touching an untrusted path is the outbound probe, so the sequence must be
open-and-judge in one operation, with the ancestors held rather than inspected —
lstat-then-open andis_symlink()-then-open are the same defect wearing differentclothes. The repository already had the primitive (
platform_compat.pin_directory)and already had the pattern (
aws_control/backend/storage.py); the review promptworth writing is "if this guard checks a name and then opens that name, what holds the
name still in between?".
Fourth: when reusing an ancestor-walking guard, ask what it is scoped to. A walk
that runs to the drive root fails closed on everything when the configured root is
itself a link — which would have turned a leak fix into a silently empty backup.
Fifth, and the one this revision cost: guarding the read does not guard the walk.
Four revisions hardened what the export was allowed to OPEN while the enumeration in
front of it kept resolving untrusted names for free. The review prompt worth writing
is "before the guard runs, what has already touched this path?" — and the test that
catches it must assert on the calls, because every build here archived the right
thing.
rglob,glob,os.walkandPath.iterdirare all guard-free surface. Theenumeration primitive that is safe was already available:
os.scandirhands back achild's type and reparse tag from the directory listing, so a walk can decide about a
child without ever addressing it.
Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)Contribution License Agreement
🤖 Generated with Claude Code