fix(atomic): recheck move authorization before rename - #194
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 3:35 PM ET / 19:35 UTC. ClawSweeper reviewWhat this changesThis PR adds an optional synchronous authorization recheck before direct and staged move renames, with documentation, regression tests, and synchronized 0.7.2 package metadata. Merge readiness⛔ Blocked until real behavior proof is added - 3 items remain The implementation appears correct and keeps authorization rejection ahead of every rename publication route, but this external PR lacks an inspectable real-runtime artifact for that security boundary. Priority: P2 Review scores
Verification
How this fits togetherThe atomic move helper prepares or copies a source path, then publishes it through a guarded filesystem rename. The new callback lets a caller revalidate a revocable owner at that final publication boundary. flowchart LR
A[Caller authorization] --> B[Atomic move helper]
C[Source and destination paths] --> B
B --> D[Directory identity guards]
D --> E[Final authorization check]
E --> F[Direct or staged rename]
F --> G[Published move or refusal]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the narrow callback design and add redacted final-effect evidence showing revocation prevents both direct and forced-EXDEV staged renames while preserving the source and prior destination. Do we have a high-confidence way to reproduce the issue? Yes, source provides a high-confidence path: revoke the callback while directory preparation is paused, then exercise direct and forced-EXDEV staged moves on real source and destination paths. The supplied test implements that scenario with mocked timing, but no inspectable live trace accompanies the PR. Is this the best way to solve the issue? Yes, centralizing the callback inside guarded rename covers both direct and staged publication without changing callers that omit it; real final-effect proof is still needed before merge. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 754f920d99af. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
What Problem This Solves
Fixes an issue where consumers moving files or directories under a revocable authorization could publish after that authorization expired during asynchronous preparation. Checking immediately before calling
movePathWithCopyFallbackdid not protect the eventual rename, including the staged-copy path.Why This Change Was Made
Add the optional
assertBeforeRenamecallback, captured when the move starts and called synchronously after the directory guards, immediately before each rename dispatch. A refusal preserves the original error and cannot trigger copy fallback, even forEXDEVorEPERM. Promise, thenable, and other non-undefined returns refuse publication; asynchronous rejections are consumed.Both direct moves and staged-copy publication use the same guarded rename boundary. Existing source-identity checks and cleanup remain in place. This does not cancel an already-dispatched syscall or make an external authorization store atomic with the filesystem. Production source delta is +22 lines; the new public contract and refusal handling require those additions.
User Impact
Consumers can now revalidate the original operation owner at the last synchronous point before publication. Omitting the callback preserves existing behavior. The options type and atomic-operation documentation describe the contract. This PR also prepares the root package, seven native packages, private build workspace, crate, and generated locks for release 0.7.2.
Evidence
All 20 new regression cases fail against the unchanged 0.7.1 source and pass with this fix. They cover direct and staged moves, genuine cross-device fallback, callback capture, refusal identity, same-turn dispatch, and asynchronous return rejection. The focused move/atomic cohort passes 79 tests, with one existing platform skip.
Live real-directory reproduction through the public package export: registry 0.7.1 publishes replacement bytes after cancellation and permits stale rollback to replace a successor directory. Built 0.7.2 refuses both before rename, preserves source/backup bytes and the successor inode, and still permits restoration by the active original owner. Package hashes remain unchanged during the run.
pnpm check: 4,082 tests pass; 2,333 platform/native-specific cases skip in the JavaScript-fallback run. Includes build, boundary lint, documentation examples, package imports, and public API checks.pnpm test:security: 84 tests pass.cargo test --workspace --locked: 65 pass.cargo clippy --workspace --locked -- -D warnings,pnpm docs:site, andgit diff --checkpass.pnpm native:buildandpnpm package:smokepass on macOS arm64: real root-only npm 12.0.2 and pnpm 11.24.0 installs exercise require/auto/off modes, omitted optionals, and missing-binary refusal. Foreign filtering fixtures prove selection only; platform execution is covered separately by CI.Independent Codex review reports no actionable P0–P2 findings. The new test file adds 197 lines.
Tests added or updated when behavior changed
Security and compatibility impact considered
CHANGELOG.mdupdated when release-relevantNo credentials, private paths, private hosts, or sensitive contents included