fix: reject retargeted synchronous secret reads

[steipete]

Summary

This audits every explicit platform branch, Windows path conditional, native/fallback selection, and hand-maintained sync/async pair in src/ against the documented and exported contracts.

One deterministic security divergence had a clear contract and is fixed: readSecretFileSync() previewed one file, then trusted the identity selected by a later realpathSync() call. If the path was retargeted between those steps, the sync reader returned replacement bytes while readSecretFile() rejected the same race with path-mismatch. The sync reader now compares the preview identity with the pinned descriptor before reading.

The other confirmed divergences below need public error-code or portability decisions, so this PR deliberately leaves them unchanged.

Bug and repro

• src/secret-file.ts:127: retarget a secret symlink from file A to file B inside the realpathSync seam; before the fix, readSecretFileSync(link) returned B while readSecretFile(link) threw path-mismatch. The new regression failed on unmodified origin/main with “expected function to throw an error, but it didn't.”

Inventory

Seam examined	Files / public operations	Verdict	Notes
Windows/POSIX containment and normalization	path.ts, root-path.ts, root-path-existing.ts, root-path-symlink.ts; isPathInside, resolveRootPath, resolveRootPathSync	agreed	Sync/async traversal, overlong input, separators, drive-relative input, whitespace, and symlink hops agree in host proofs and shared logic.
Case-sensitive Windows directories	path.ts, root-path.ts, deny-mutations.ts; Root containment and deny policies	unverifiable-from-here	Windows comparison lowercases paths. A per-directory case-sensitive Windows volume could make differently cased siblings distinct while lexical containment treats them as equal. Needs a real Windows case-sensitive-directory proof before changing public comparison behavior.
Unsafe devices, reserved names, file URLs, and filename sanitation	device-path.ts, local-file-access.ts, filename.ts, safe-path-segment.ts; all read/open entry points	agreed	Platform-injected tests cover POSIX devices/fd namespaces, Windows devices, UNC/extended paths, drive-relative segments, trailing dot/space aliases, and sanitized output.
Portable archive entry names	archive-entry.ts; validateArchiveEntryPath, extraction/read paths	divergent-and-escalated	NUL, dir/CON.txt, payload.txt:stream, trailing-dot/space names, and Windows-invalid characters are accepted on POSIX but alias, fail, or address ADS/device semantics on Windows. Recommend a portable segment policy, but rejecting currently accepted POSIX archives is a compatibility decision.
Archive native/JavaScript policy and modes	archive.ts, archive-native.ts, archive-read.ts, archive-input.ts, archive-staging.ts, archive-kind.ts	agreed	Shared TypeScript policy owns paths, filters, collision checks, limits, and clamped modes; targeted native/JS equivalence passed. Windows intentionally does not claim POSIX mode enforcement.
Root read/open surface	root-impl.ts, root-context.ts, opened-realpath.ts; open, read*, stat, list, exists, walk	agreed	Error normalization, device checks, symlink/hardlink policy, and writable-directory not-file behavior align. Windows opened-handle path discovery remains best-effort as documented.
Root mutation surface	root-impl.ts, root-errors.ts; write, create, copyIn, append, openWritable, mkdir, remove, move	agreed	Native/guarded selection, fallback error normalization, deny policies, missing parents, overwrite, directory targets, and cleanup ownership were traced for every verb.
Native pinned writes versus guarded fallback	pinned-write.ts, native-pinned-write.ts, native-operations.ts	agreed	Create-only collision timing, stream limits, modes, identity fences, cleanup, and rename policies agree in native-backed tests.
Remaining native OS error mappings	native/src/windows.rs, native/src/unix.rs, native-operations.ts	unverifiable-from-here	The known public cases (EPERM, ENOENT, EEXIST, ENAMETOOLONG, directory targets) are aligned. Unmapped Windows/NT and POSIX errors collapse to EIO; a live cross-OS fault matrix is needed to prove less common codes.
Native loader/config target selection	native.ts, native-config.ts	agreed	Seven published target selections, musl detection, auto/off/require behavior, and helper-unavailable paths are covered by deterministic loader injection.
Directory durability	directory-durability.ts, fsync.ts; async/sync strict and best-effort APIs	agreed	Windows unsupported open/sync families are explicit, access errors remain strict, and sync/async identity revalidation matches.
Atomic replace and copy fallback	replace-file.ts, replace-file-descriptor.ts, replace-file-copy-fallback.ts; async/sync twins	agreed	Descriptor modes, Windows no-mode boundary, hardlinks, symlinks, retry/copy policy, restoration, and sync ordering agree.
Cross-device/Windows move fallback	move-path.ts; movePathWithCopyFallback	agreed	Only EXDEV and Windows EPERM enter copy fallback; source identity, hardlinks, modes, and cleanup are fenced consistently.
Permission and ownership semantics	secure-file.ts, permissions.ts, permissions-windows.ts, windows-owner.ts, owner-dacl.ts, private-directory.ts	unverifiable-from-here	Injected native/fallback ACL facts agree and POSIX mode ownership is clear. Live Windows ACL inheritance, remote-volume locality, and owner-command behavior cannot be fully verified on macOS.
Secret read path identity	secret-file.ts, secret-read-async.ts; strict/optional sync and async readers	divergent-and-fixed	Sync now compares the preview identity with the opened descriptor, matching async path-mismatch behavior.
Secret read I/O error selection	secret-file.ts:147, secret-read-async.ts:82	divergent-and-escalated	A post-validation read EIO becomes invalid-path in sync and path-mismatch in async. Recommend preserving the underlying I/O error or defining one common operational code; neither current code accurately describes it, so this PR does not choose.
Secret writes and modes	secret-file.ts; writeSecretFileAtomic, createSecretFileAtomic	agreed	POSIX enforces exact descriptor modes; Windows explicitly skips POSIX mode enforcement while retaining identity and type checks.
Regular-file sync/async twins	regular-file.ts; stat/read/append pairs	agreed	Missing, type, hardlink, symlink, bounded-growth, identity, encoding, and mode behavior match.
JSON sync/async families	json.ts; strict/lenient reads and writes	agreed	Differences in retry and legacy sync-write options are documented; strict/lenient read result contracts and bounded errors align.
FileStore sync/async validation errors	file-store.ts:132, file-store.ts:339, file-store.ts:444	divergent-and-escalated	One-line repro: read a stable directory key. Async returns not-file; sync returns path-mismatch. Stable hardlinks similarly become hardlink async versus path-mismatch sync. Recommend semantic codes in sync, but that is a public code change.
Temp workspace sync/async reads	private-temp-workspace.ts:256	divergent-and-escalated	One-line repro: create a directory at a workspace leaf and call read. Async returns not-file; sync fabricates raw ENOENT. Recommend matching the async Root codes, pending compatibility approval.
Directory guards and symlink-parent sync/async twins	directory-guard.ts, guarded-mutation.ts, symlink-parents.ts	divergent-and-escalated	With default allowMissing, an existing file ancestor plus /child returns success async because ENOTDIR is treated as missing, while sync throws raw ENOTDIR. Recommend defining whether this helper is a pure symlink predicate or also validates usable ancestry before changing either side.
Standalone and Root walking	walk.ts, root-walk.ts	agreed	Budgets, depth, filtering, failed-directory reporting, symlink policies, and cycle detection match between sync/async standalone walkers and the documented Root variant.
Sidecar/file-lock sync/async paths	sidecar-lock*.ts, file-lock.ts, file-lock-sync.ts	agreed	Ownership tokens, bounded snapshots, reclaim, reentrancy, and cleanup match. The Windows transient EPERM retry is intentionally async-only and documented as such.
Hash native/fallback	file-hash.ts, native/src/fast_file.rs; sha256File	agreed	Results, offsets, file type, and path identity agree; native acceleration preserves the caller-owned handle contract.
Exclusive publication native/fallback	publish-file.ts, publish-file-failure.ts	agreed	Hardlink/copy/rename selection, collision behavior, hashes, cleanup receipts, and directory sync agree in targeted equivalence tests.
Local roots, home paths, and secure temp roots	local-roots.ts, home-dir.ts, secure-temp-dir.ts, temp-target.ts	agreed	URL/platform adapters, canonical roots, missing paths, Windows fallback joining, UID/mode availability, and sync/async temp semantics are explicit and covered.
JSON durable queue platform branches	json-durable-queue.ts	agreed	Darwin system aliases, Windows root comparison, no-follow availability, mode best-effort behavior, and entry identity checks are internally consistent.
Repeated no-follow/platform fallbacks	output-sibling.ts, sidecar-lock-reclaim.ts, file-lock-sync.ts, file-store-boundary.ts, archive-read.ts, archive-input.ts, archive-staging.ts, sidecar-lock.ts	agreed	Each omits unavailable Windows POSIX flags but retains lstat/fstat/identity checks or an explicitly best-effort mode boundary.

Verification

• Confirmed the new regression fails on unmodified source: sync returned replacement bytes instead of throwing.
• pnpm install --frozen-lockfile
• pnpm native:build
• Targeted native integration/publication/archive/secret suites: 74 passed, 5 platform skips
• pnpm native:test: 16 passed
• pnpm test:coverage: 1,004 passed, 59 platform skips; 90.05% line coverage
• pnpm check: 1,004 passed, 59 platform skips
• pnpm test:security: 70 passed
• git diff --check
• Codex autoreview (gpt-5.6-sol, high): clean, no accepted/actionable findings

Generated coverage/, dist/, native binary, and Cargo target artifacts were removed before commit.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed August 3, 2026, 2:50 AM ET / 06:50 UTC.

ClawSweeper review

What this changes

The branch makes synchronous secret-file reads reject a symlink path retargeted after preview validation, matching the asynchronous reader’s path-mismatch behavior.

Merge readiness

⚠️ Ready for maintainer review - 1 item remains

Keep this active PR open for normal merge review. Current main still has the synchronous preview-to-open identity gap, while this focused patch adds the missing fail-closed comparison and a direct regression; the remaining Windows checks should complete before merge.

Priority: P1
Reviewed head: e5cb2227f4a949e30c8db55c8904b0054970774f

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A narrow, policy-aligned security repair with direct regression coverage and strong reported validation; final confidence depends only on the in-progress Windows checks.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (live_output): The PR body provides a concrete before-fix failure observation, the expected post-fix typed failure, and reported targeted plus full security/cross-platform validation; terminal-style proof is appropriate for this non-visual filesystem race.
Patch quality	🦞 diamond lobster (5/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (live_output): The PR body provides a concrete before-fix failure observation, the expected post-fix typed failure, and reported targeted plus full security/cross-platform validation; terminal-style proof is appropriate for this non-visual filesystem race.
Evidence reviewed	7 items	Current-main gap: The synchronous reader previews the target and opens a pinned descriptor, but current main reads from that descriptor immediately afterward without comparing previewStat to the opened descriptor identity. Existing async contract: The asynchronous reader already rejects when the previewed identity differs from the opened descriptor, returning path-mismatch before reading bytes. Focused repair: The proposed patch compares previewStat with opened.stat and returns path-mismatch before the bounded descriptor read, closing the identified synchronous race.
Findings	None	None.
Security	None	None.

How this fits together

The secret-file API reads credential material from a caller-provided path after validating file identity and permissions. It previews metadata, opens a pinned descriptor, validates the descriptor, and returns either bounded secret bytes or a typed failure that callers can handle safely.

flowchart TD
  A[Caller secret path] --> B[Preview metadata]
  B --> C[Resolve and open pinned descriptor]
  C --> D[Compare preview and descriptor identities]
  D --> E[Bounded descriptor read]
  E --> F[Secret value or typed error]

Loading

Before merge

• Resolve merge risk (P2) - This changes a secret-file security boundary; merge only after the remaining Windows package/native checks confirm the platform fallback path remains healthy.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Patch scope	production +8, tests +30, changelog +4	The patch is tightly bounded to one secret-read validation check, one deterministic regression, and the required release note.

Merge-risk options

Maintainer options:

1. Complete cross-platform validation (recommended)
   Allow the remaining Windows package and native checks to finish, then merge the focused identity-fence repair if they remain green.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Require all remaining Windows package and native checks to succeed before merging this secret-read identity-fence repair.

Technical review

Best possible solution:

Merge the narrow preview-versus-descriptor identity fence with its regression test so both secret readers fail closed when a path is swapped during validation.

Do we have a high-confidence way to reproduce the issue?

Yes. Current source shows the sync path previews one identity and later reads another descriptor without comparing them, while the added deterministic symlink-retarget test directly exercises that seam.

Is this the best way to solve the issue?

Yes. Reusing the existing sameFileIdentity guard to compare the preview with the pinned descriptor is the smallest maintainable fix and matches the established asynchronous contract.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against b8738eb59b29.

Labels

Label changes:

• add P1: Without the added check, a secret-read call can return bytes from a replacement file after a pathname retarget instead of failing closed.
• add merge-risk: 🚨 security-boundary: The changed code governs identity validation immediately before secret bytes are read from a pinned descriptor.
• add proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a concrete before-fix failure observation, the expected post-fix typed failure, and reported targeted plus full security/cross-platform validation; terminal-style proof is appropriate for this non-visual filesystem race.
• add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body provides a concrete before-fix failure observation, the expected post-fix typed failure, and reported targeted plus full security/cross-platform validation; terminal-style proof is appropriate for this non-visual filesystem race.

Label justifications:

• P1: Without the added check, a secret-read call can return bytes from a replacement file after a pathname retarget instead of failing closed.
• merge-risk: 🚨 security-boundary: The changed code governs identity validation immediately before secret bytes are read from a pinned descriptor.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body provides a concrete before-fix failure observation, the expected post-fix typed failure, and reported targeted plus full security/cross-platform validation; terminal-style proof is appropriate for this non-visual filesystem race.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides a concrete before-fix failure observation, the expected post-fix typed failure, and reported targeted plus full security/cross-platform validation; terminal-style proof is appropriate for this non-visual filesystem race.

Evidence

What I checked:

• Current-main gap: The synchronous reader previews the target and opens a pinned descriptor, but current main reads from that descriptor immediately afterward without comparing previewStat to the opened descriptor identity. (src/secret-file.ts:110, b8738eb59b29)
• Existing async contract: The asynchronous reader already rejects when the previewed identity differs from the opened descriptor, returning path-mismatch before reading bytes. (src/secret-read-async.ts:69, b8738eb59b29)
• Focused repair: The proposed patch compares previewStat with opened.stat and returns path-mismatch before the bounded descriptor read, closing the identified synchronous race. (src/secret-file.ts:127, e5cb2227f4a9)
• Regression coverage: The added POSIX test retargets separate sync and async symlinks inside their realpath seams and asserts that both public readers throw path-mismatch. (test/secret-file-failure.test.ts:131, e5cb2227f4a9)
• Provenance and release state: The repair commit is directly based on current main, is not an ancestor of the v0.5.2 release tag, and therefore is neither merged nor released yet. (CHANGELOG.md:3, e5cb2227f4a9)
• Area provenance: Current-main blame attributes the surrounding synchronous secret-read flow to the v0.5.2 release commit by Peter Steinberger, and the current repair is authored by the same contributor identity. (src/secret-file.ts:110, 32759e8923a1)

Likely related people:

• steipete: Current-main blame attributes the surrounding synchronous reader to Peter Steinberger’s v0.5.2 release commit, and steipete authored the focused follow-up repair from that exact main revision. (role: recent secret-file contributor; confidence: high; commits: 32759e8923a1, e5cb2227f4a9; files: src/secret-file.ts, src/secret-read-async.ts, test/secret-file-failure.test.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Wait for the remaining Windows package and native checks to complete successfully before merge.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.