fix(papyrus): keep the located Tectonic binary inside the unpacked tree

[leonlaiyc]

Problem / Motivation

tectonic._locate_binary picks the executable out of an extracted archive and
_provision_once installs it onto the path papyrus later executes. The walk
filtered candidates with candidate.is_file() and not candidate.is_symlink(), so
refusing a link-mediated escape was already the site's own intent — but the filter
cannot see the escape that matters.

Path.rglob descends a directory link, and the executable on the far side is an
ordinary file: is_file() true, is_symlink() false. It passed the filter unchanged
while living outside the tree. On Windows the link is a junction, which
os.path.islink does not report at all — measured on an unelevated box:

junction is_symlink        : False
os.path.islink(junction)   : False
DirEntry.is_symlink        : False
rglob('tectonic') from tree: ['...\\unpacked\\vendor\\tectonic']   <- descended
through-junction is_file   : True
through-junction resolve   : ...\outside\tectonic                  <- outside

The tree / wanted fast path was weaker still: it took the candidate on is_file()
alone, which follows a link, so it had no filter at all.

A resolve-and-compare is not enough, and the first commit here only did that.
Reviewers held it, correctly: the check resolves a path and then _install_binary
opens the same path again. The inode that was checked and the inode that is
installed are two separate lookups, and a writer who retargets the unpack directory
in the window between them wins. Measured against the base commit:

by-name containment check at validation time: True  (accepts it)
installed bytes: b'\x7fELFplanted!'
VULNERABLE: True

Why it matters

The located path is not read — it is installed. _install_binary moves it, applies
_BINARY_MODE (0o755) and os.replaces it onto binary_path(), and
binary_installed re-checks only size and the exec bit, so a plausibly-sized planted
file passes. An arbitrary local file is promoted to an executed compiler with no
recovery once it is in place.

Stated plainly and not inflated: the asset is pinned and fetched over TLS, and
neither tar nor zip can create a Windows junction, so the archive cannot plant this.
It needs a local writer racing the per-process .provision.<pid> unpack directory.
That is the containment layer this walk was already reaching for — a defence that
was inert on the platform its own link shape belongs to, not a remote-archive escape.

What changed (motivation → approach → change)

Root cause: the check and the use are two lookups of one name, and the by-name
check is additionally blind to a junction.

Approach: resolve once, open once, address everything downstream through the
descriptor. That is the discipline pinned_fs already exists for in this repo, and
this change consumes its fd_real_path rather than adding a second mechanism. The
heavier stage_tree_pinned was not used: it is gated on supports_pinned_walk(),
which is false on Windows — the platform this defect lives on.

• _open_inside(candidate, root_real) — opens with O_NOFOLLOW where it exists,
  rejects a non-regular file on the descriptor's own fstat, then asks the kernel
  where the open descriptor actually is (/proc/self/fd, F_GETPATH, or
  GetFinalPathNameByHandleW) and compares that against the root. A descriptor
  cannot be re-pointed, so the answer stays true while it is held. Every failure
  route — unreadable, non-regular, unknowable real path — returns None, never a
  fallback to the pathname.
• _install_binary takes a descriptor, not a path, and copies from it. The
  atomic-publish shape is unchanged: staged in the target directory, mode applied
  before the rename, os.replace onto the final path. The cost is a copy instead of
  a rename, once, for an artifact that was just fetched over the network.
• The root is captured in _provision_once, immediately after unpacked.mkdir()
  and before the archive is written into it. Resolving it inside the locator would
  reopen the same window one level up, since the directory whose containment is
  being asserted is exactly the one an attacker would swap.
• The by-name filter stays as an early refusal that names the right file in the
  error message, and its docstring now says it is not the containment witness.
• The size floor is read from os.fstat(fd) rather than binary.stat(), so the size
  that gates the install is the size of the file that gets installed.

Tests

TestTheInstalledBytesAreTheValidatedBytes — the property is not "the check is
stricter" but "the check and the use address one object":

• test_retargeting_the_link_after_validation_cannot_change_what_is_installed — the
  regression for the race. The link points inside the tree at validation, so a
  by-name check accepts it; it is retargeted outside before the install. Asserts the
  installed bytes are the validated ones. Deterministic: the swap is placed where a
  racing writer would land rather than run concurrently and hoped for.
• test_a_file_reached_through_a_directory_link_gets_no_descriptor — guards itself
  by asserting the lexical path is inside the tree, so a name-only check would
  have accepted it.
• test_a_real_file_inside_the_tree_gets_a_descriptor_on_its_own_bytes — positive
  control, and the one that catches an over-tight comparison: fd_real_path and
  realpath reach the same name by different kernel routes.
• test_containment_fails_closed_when_the_real_path_is_unknowable,
  test_a_directory_never_yields_a_descriptor — the refusal routes.

TestLocateBinaryStaysInsideTheUnpackedTree keeps the by-name filter's own coverage,
with two corrections a reviewer was right about:

• The fast-path case built a directory link, so is_file() was false and it
  silently exercised the walk instead. It now uses a file symlink — the only
  shape that reaches that branch — and is skipped where one cannot be created.
• The walk-descent case asserted rglob descends the link. That is true for a
  Windows junction and false for a POSIX directory symlink: pathlib's **
  deliberately does not descend one, and the Linux shard failed on exactly that
  guard-the-guard assertion (rglob never descended the link, so nothing was under test). Measured on both platforms; the test is now marked for the platform whose
  shape it describes, and keeps the guard-the-guard so a behaviour change fails
  loudly rather than passing vacuously.

Negative control run against the base commit (output quoted above): the unpatched
install writes the planted bytes, the patched install writes the genuine ones.

.github/black-baseline.txt: tectonic.py became black-clean as a result of this
change, and the gate fails on a graduated entry that is still listed, so its row is
pruned. No file was reformatted.

Manual verification

N/A — unit coverage sufficient: the whole defect is the relationship between the
validation and the install, and the tests drive both against a real junction on the
affected platform rather than a mock.

Related Issues

None — found by inspection while auditing is_symlink-based containment guards for
the Windows junction blind spot, the same family as #7881.

Pattern harvest

Rule candidate: review-prompt
Pattern: a containment guard that validates a path and then reopens it. Two
distinct failures ride together — the guard tests the candidate's own node type
(is_symlink/islink) rather than where it resolves, and a Windows junction is
invisible to both islink and DirEntry.is_symlink, so a recursive walk descends
it. Where the walk is rglob/os.walk and the result is later written, moved or
executed, resolve()-then-reopen is not a fix: open once and address the descriptor.

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

🤖 Generated with Claude Code

[github-actions]

Design Review (Fable 5.1, fork) — ✅ PASS

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

Design-Verdict: PASS

Canonical TOCTOU fix: containment is witnessed on a held descriptor and the install copies from that fd, so the checked inode and the installed inode are the same — fails closed on every unknowable-path route.

[DESIGN-REVIEWED] 776a080

[github-actions]

Opus 4.8 Review (fork) — 🔴 changes requested (blocking)

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

BLOCKING — the destination staging write now follows a planted symlink, an arbitrary-file overwrite the old rename did not have.

[BLOCK-MERGE] 776a080

BLOCKING — src/kiro_crew/apps/builtins/papyrus/backend/tectonic.py (_install_binary, the staging write)

staging = target.parent / f".{target.name}.{os.getpid()}.tmp"
...
with open(staging, "wb") as out:
    shutil.copyfileobj(os.fdopen(os.dup(source_fd), "rb", closefd=True), out)
platform_compat.chmod_safe(staging, _BINARY_MODE)

The PR's own accepted actor — a local writer in the vendor directory — plants a symlink at the predictable staging name .tectonic.<pid>.tmp (pid observable, same pid used for the .provision.<pid> tree this change already defends) → open(staging, "wb") opens with O_CREAT|O_TRUNC and no O_NOFOLLOW, following the symlink and truncating/writing ~50MB of Tectonic bytes into the symlink's target, then chmod_safe follows it and sets that file 0o755. The prior shutil.move → os.rename replaces the symlink entry rather than following it, so this is a symlink-follow arbitrary-file overwrite introduced by this hunk — the gateway's privileges write a file the attacker may not otherwise reach.
Fix: create staging with os.open(staging, os.O_WRONLY | os.O_CREAT | os.O_EXCL | getattr(os, "O_NOFOLLOW", 0), 0o600) and write/fchmod through that descriptor, so a planted name is refused rather than followed (fully in-diff).

[OPUS-REVIEWED] 776a080

[github-actions]

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

Premise-level review of 776a0802a92305ad8d9afee36c2f05fce1e9eb2d 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.

This is a well-scoped security fix. Every changed symbol is consumed only within tectonic.py (_provision_once is the sole caller), the change reuses the existing pinned_fs.fd_real_path mechanism rather than adding one, and the referenced test helpers exist. I have what I need.

First-Principles-Verdict: PASS

Closes a local-writer race where the located Tectonic binary was validated by name then reopened for install; every item serves that one fix.

What this change ships

Intent: stop an arbitrary local file from being promoted to the executed Tectonic compiler via a junction/symlink swapped into the per-process unpack tree between validation and install. This is a FIX.

1. _locate_binary now rejects a candidate that resolves outside the tree — justified (closes the rglob-descends-a-link / junction blind spot at name level).
2. New _open_inside descriptor containment witness — justified (1 consumer, _provision_once; that singularity is the point, not premature generalization).
3. _install_binary takes a descriptor and copies instead of shutil.move — justified (makes checked inode == installed inode).
4. Size floor read from os.fstat(fd) not binary.stat() — justified (same check-vs-use discipline).
5. root_real captured right after unpacked.mkdir() in _provision_once — justified (resolving later reopens the window).
6. black-baseline.txt row for tectonic.py pruned — rides along, but mandated (the gate fails on a graduated-but-still-listed entry).

Root cause ("check and use are two lookups of one name") is named and fixed at cause level, reusing pinned_fs.fd_real_path rather than a second mechanism. The one open thread — sibling is_symlink-based containment guards elsewhere (author cites the #7881 family) — is a genuinely larger audit, accepted-and-deferred, not a demand on this change, which targets the highest-harm instance (an executed binary).

[FIRST-PRINCIPLES-REVIEWED] 776a080

[github-actions]

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

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

1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/apps/builtins/papyrus/backend/tectonic.py:833 -- Predictable staging path follows planted links
with open(staging, "wb") as out:
Agent plants .tectonic.<gateway-pid>.tmp symlink -> provisioning follows it -> arbitrary host file is truncated and overwritten.
Anchor: residual/security
Fix: Create a unique staging file with O_CREAT | O_EXCL | O_NOFOLLOW and copy/chmod through its descriptor.
[BLOCK-MERGE] 776a080
[GPT-REVIEWED] 776a080

Adjudication (Fable 5.1) — is blocking on each finding proportionate?

API Error: 400 Claude Code 2.1.240 does not support this model; version 2.1.255 or newer is required. Run 'claude update', or update the Claude desktop app, then try again.

[leonlaiyc]

Addressing the exact-head [BLOCK-MERGE] on ac0aa3d28, and the finding beside it, at 3b4bcaf12.

F1 — TOCTOU at tectonic.py:562. Accepted as stated, not argued down. The fenced verdict is a repository-writer decision and this PR does not ask for an override; the race is closed instead. Validation now happens on an open descriptor (_open_inside → pinned_fs.fd_real_path) and _install_binary copies from that descriptor rather than reopening the name. The unpacked root is captured in _provision_once before the archive is written into it, so the directory whose containment is asserted cannot be swapped underneath the assertion either.

Reproduced against the base commit before fixing it:

by-name containment check at validation time: True  (accepts it)
installed bytes: b'\x7fELFplanted!'    VULNERABLE: True     <- unpatched
installed bytes: b'\x7fELFgenuine'     VULNERABLE: False    <- patched

test_retargeting_the_link_after_validation_cannot_change_what_is_installed pins it deterministically.

FINDING — test_papyrus_tectonic.py:1131. Correct, and thank you: make_dir_link made direct a directory, direct.is_file() was false, and the case silently exercised rglob instead of the fast path. It now uses a file symlink — the only shape that reaches that branch — with @requires_symlinks, and carries a guard-the-guard asserting is_file() so it cannot regress into a vacuous pass.

Also branch-caused, and fixed here: Backend Tests (3.12, 3) failed on this PR's own new test with rglob never descended the link, so nothing was under test. That guard was doing its job. pathlib's ** does not descend a POSIX directory symlink; a Windows junction carries a different reparse tag and the same walk goes through it. Measured on both. The walk-descent test is now marked for the platform whose shape it describes and keeps the guard-the-guard, rather than manufacturing a Linux equivalent for a premise that does not hold there.

Backend Tests (Windows) (2) failed separately with an xdist worker crash in test_dashboard_state_ws.py::TestSlotsBroadcastCarriesFolders, which has no import path to this diff. Recorded as UNATTRIBUTED rather than guessed at as a flake, and not rerun.

[leonlaiyc]

On the exact-head [BLOCK-MERGE] for 3b4bcaf12 — the finding is accepted, and I am not shipping a third pathname check. Parking this for a maintainer decision instead.

The finding is right. root_real = os.path.realpath(unpacked) is a string. The candidate file is pinned to an inode; the extraction root is not. A same-pathname real-directory replacement produces an identical realpath, commonpath accepts it, and attacker bytes are installed. That is the same racing local writer this PR already defends against, using a different technique.

The prescribed fix is "hold a descriptor/handle for unpacked and open candidates relative to that pinned directory." Measured on Windows — the platform this whole PR exists for, because the blind spot is a junction:

supports_pinned_walk()        : False
os.open in os.supports_dir_fd : False
hasattr(os, "O_DIRECTORY")    : False
hasattr(os, "O_NOFOLLOW")     : False
os.open(<a directory>)        : PermissionError 13

So there is no directory descriptor to hold. pinned_fs.open_verified_chain is exactly the right primitive — it admits a component only as the (st_dev, st_ino) a prior scan recorded, which is precisely "same directory object, not same name" — but it is built on dir_fd= and is POSIX-only by construction. Implementing the prescribed fix on Windows means adding a directory-handle layer to pinned_fs (CreateFileW + FILE_FLAG_BACKUP_SEMANTICS, handle identity via GetFileInformationByHandle, and a relative-open path). That is a new cross-platform filesystem primitive, not a fix to this PR.

And this module already has a stated posture for exactly this situation, which I do not think I get to choose on my own. snapshot._staging_is_pinned refuses outright when supports_pinned_tree_walk() is false, and says why: "a by-name walk is not a slightly weaker version of a pinned walk, it is the mechanism whose failure closed two pull requests", with --allow-unpinned-staging as the only way through. Applying that rule here means managed Tectonic provisioning refuses to run on Windows — and there is no operator opt-in seam, because this is a background provisioning thread behind a dashboard status surface, not a CLI command.

So the two honest routes both belong to a repository writer:

1. Scope a Windows directory-pinning primitive in pinned_fs, then rebuild this containment on it. Larger than this PR and it should not be designed inside it.
2. Accept the residual with a recorded override, on the same reasoning the first adjudication of this PR gave and then had fenced: reaching this race needs local write access to .provision.<pid> inside vendor_dir, which already lets the same attacker write straight to binary_path() with no race at all. I am not asserting that as a clearance — a fenced finding is a repository-writer call and this comment is not an override request.

What is already fixed on this head stands on its own and is not affected by the above: the file-level check-to-use race is closed (open once, pinned_fs.fd_real_path, install from that descriptor), with a measured control showing the unpatched install writing planted bytes and the patched one writing the genuine bytes; and the two tests that were passing for the wrong reason are corrected.

Not touching the SHA, not rerunning, not rebasing. Happy to implement either route on a maintainer's word.

[leonlaiyc]

Two CI corrections at 776a0802a. Neither touches the fenced blocker above — this PR stays parked on that.

Branch-caused, mine, fixed. test_retargeting_the_link_after_validation_cannot_change_what_is_installed failed on Backend Tests (3.12, 3):

NotADirectoryError: [Errno 20] Not a directory: '.../unpacked/vendor'

Detaching the link is not the same call on both platforms and I only checked one. A POSIX directory symlink is a link entry, so rmdir refuses it and only unlink removes it; a Windows junction is a real directory entry, which unlink refuses. is_symlink() separates them — and it is False for a junction, which is the same property the code under test is about. The swap now goes through a _detach_dir_link helper that branches on it, so the regression actually runs on Linux instead of erroring before it reaches the assertion. Same bug and same fix in #9070.

Not branch-caused: test_security.py. Both failing shards also carry

FAILED test/test_security.py::TestIsSensitiveBashCommand::test_chained_cd_expansions_do_not_blow_up_the_gate
AttributeError: module 'kiro_crew.security' has no attribute '_dir_holds_sensitive_leaf'

The run merged this branch into main@aba8d79c4, where #9089 had already removed that helper from security.py while test_security.py:5168 still mock.patch.object-ed it. Repaired upstream by cbdd4a569 (#9182), which is not an ancestor of that base but is an ancestor of current main. Nothing to do on this branch; Coverage Gate is derivative of those two shards.

One thing worth a maintainer's eye: Fork workflow-change guard is failing and I cannot satisfy both gates. This PR made tectonic.py black-clean, and scripts/check_black_formatting.py then fails with "1 baselined file(s) are now black-clean. Remove them so the baseline keeps shrinking". Removing that row edits .github/black-baseline.txt, and the fork guard blocks any fork PR touching .github/** without the allow-fork-workflow-change label. So a fork contributor whose change happens to graduate a baselined file is forced into a red square either way — black gate red if they leave the row, fork guard red if they prune it. I kept the prune, because that is what the black gate's own error text instructs, and flagged it here rather than picking the quieter red silently.

[bolichen97]

@leonlaiyc Thanks for this one. I audited #9057 against main and against every other open PR, and here is where it stands.

Nothing of this fix has landed yet. On main, src/kiro_crew/apps/builtins/papyrus/backend/tectonic.py still filters only on candidate.is_file() and not candidate.is_symlink(), still installs with shutil.move, and still takes its size floor from binary.stat(). No merged commit after your merge base touches that file, and no other open PR touches it, so this PR is the only thing carrying the fix. One small correction: #7881 does not modify tectonic.py in any line, and the is_symlink() filter you strengthen came from #1077, so the lineage in the body is a little loose. Nothing in the diff depends on it.

What is still needed before this can merge:

1. The staging write is a regression. _install_binary now does open(staging, "wb") on the predictable name .tectonic.<pid>.tmp, so it follows a planted symlink, truncates an arbitrary host file, and then chmod_safe marks it 0o755. shutil.move did not do that. Both blocking lanes on head 776a0802a prescribe the same small fix: os.open(..., O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW, 0o600), then write and fchmod through that descriptor.
2. Please rebase. The branch is 131 commits behind main.
3. fix(portability): keep the export archive inside the crew directory #9070 already landed the same technique in src/kiro_crew/portability.py. Consider reusing that _open_inside and the _detach_dir_link helper in test/test_portability.py instead of keeping a second copy.

I will add allow-fork-workflow-change so the fork guard stops blocking the required .github/black-baseline.txt row prune. The separate root_real-is-a-pathname finding stays parked for a follow-up, since the directory-handle fix needs a new Windows pinning primitive in pinned_fs.

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