fix(write): preserve final modes and cleanup ownership - #223
Conversation
Finalize explicit file modes after content, verify complete secret mode bits, and preserve exact ownership through native mode changes and failed-write cleanup. Keep append permission tightening before data while restoring special bits afterward and completing short synchronous writes. Add regressions, native CI selection, docs, and Unreleased notes.
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
P2
|
Codex review: blocked before merge. Reviewed September 4, 2026, 1:37 AM ET / 05:37 UTC. ClawSweeper reviewWhat this changesThis PR preserves requested special file modes after writes, limits failed-write cleanup to verified owned files, and completes short synchronous regular-file appends. Regression provenancePossible regression — probable (reviewed change; reproduction). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 1 item remains Keep open: this is a current, coherent security-hardening PR whose introduced implementation, regression coverage, and supplied installed-consumer evidence support continued review. The previous real-behavior-proof request is satisfied. Priority: P2 Review scores
Verification
How this fits togetherfs-safe accepts application-controlled file targets through guarded Root and secret-file APIs, then writes through native or JavaScript backends. These changes affect final permissions, publication verification, and cleanup after failed writes. flowchart LR
A[Application file request] --> B[Guarded Root or secret writer]
B --> C[Native or JavaScript write backend]
C --> D[Write content]
D --> E[Finalize requested mode]
E --> F[Verify identity and publication]
C --> G[Failed-write cleanup]
G --> H[Preserve unverified replacements]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the narrow mode-finalization and identity-fenced cleanup changes while retaining the documented fail-closed behavior for unverifiable cleanup paths. Do we have a high-confidence way to reproduce the issue? Yes. The base-to-head diff directly identifies the former chmod-before-write ordering and incomplete mode mask, and the supplied runtime receipts exercise the corrected scenarios. Is this the best way to solve the issue? Yes. Finalizing modes through the retained descriptor and refusing cleanup when ownership cannot be verified is the narrowest solution consistent with the package’s filesystem-safety contract. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 128bf1f863cb. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Inspectable installed-consumer proofFor reviewed head The tarball was packed before commit (base128/dirty=true). A fresh build of the clean reviewed head matches all six changed compiled module hashes. npm/pnpm installed the root and host tarballs; the successful pnpm fixture pins the host dependency through pnpm11 workspace overrides. Separate root-only registry smoke is covered by the hosted artifacts, not claimed from this fixture. Captured per-process outcomes[
{
"receipt": "npm-22.0.0-off.json",
"node": "v22.0.0",
"nativeMode": "off",
"nativeLoaded": false,
"passed": 59
},
{
"receipt": "npm-22.0.0-require.json",
"node": "v22.0.0",
"nativeMode": "require",
"nativeLoaded": true,
"passed": 57
},
{
"receipt": "npm-22.23.2-off.json",
"node": "v22.23.2",
"nativeMode": "off",
"nativeLoaded": false,
"passed": 59
},
{
"receipt": "npm-22.23.2-require.json",
"node": "v22.23.2",
"nativeMode": "require",
"nativeLoaded": true,
"passed": 57
},
{
"receipt": "npm-24.20.0-off.json",
"node": "v24.20.0",
"nativeMode": "off",
"nativeLoaded": false,
"passed": 59
},
{
"receipt": "npm-24.20.0-require.json",
"node": "v24.20.0",
"nativeMode": "require",
"nativeLoaded": true,
"passed": 57
},
{
"receipt": "pnpm-22.0.0-off.json",
"node": "v22.0.0",
"nativeMode": "off",
"nativeLoaded": false,
"passed": 59
},
{
"receipt": "pnpm-22.0.0-require.json",
"node": "v22.0.0",
"nativeMode": "require",
"nativeLoaded": true,
"passed": 57
},
{
"receipt": "pnpm-22.23.2-off.json",
"node": "v22.23.2",
"nativeMode": "off",
"nativeLoaded": false,
"passed": 59
},
{
"receipt": "pnpm-22.23.2-require.json",
"node": "v22.23.2",
"nativeMode": "require",
"nativeLoaded": true,
"passed": 57
},
{
"receipt": "pnpm-24.20.0-off.json",
"node": "v24.20.0",
"nativeMode": "off",
"nativeLoaded": false,
"passed": 59
},
{
"receipt": "pnpm-24.20.0-require.json",
"node": "v24.20.0",
"nativeMode": "require",
"nativeLoaded": true,
"passed": 57
}
]Selected filesystem receipt rowsAll rows below check both the full POSIX mode and real bytes. Restrictive-mode readback changes test-owned permissions only after recording the final mode. Cleanup cases inject a chmod error, physically rename the owned entry, and create a different file at the old name; assertions require the original error, preserved original bytes, and preserved replacement bytes. Short-write injection limits each real write to two bytes and requires three calls for six bytes. No inode values are projected in these macOS consumer runs. These are selected examples from the 696 passing observations, not an additional test count. [
{
"nativeMode": "off",
"label": "writeSecretFileAtomic:2600",
"verdict": "pass",
"mode": "0o2600",
"dev": "16777233",
"ino": "1006414210",
"bytes": 26
},
{
"nativeMode": "off",
"label": "writeSecretFileAtomic:7777",
"verdict": "pass",
"mode": "0o7777",
"dev": "16777233",
"ino": "1006414237",
"bytes": 26
},
{
"nativeMode": "off",
"label": "createSecretFileAtomic:4600",
"verdict": "pass",
"mode": "0o4600",
"dev": "16777233",
"ino": "1006414288",
"bytes": 26
},
{
"nativeMode": "off",
"label": "append:sync:empty=false:6600",
"verdict": "pass",
"mode": "0o6600",
"dev": "16777233",
"ino": "1006414502",
"bytes": 33
},
{
"nativeMode": "off",
"label": "append:sync:injected-two-byte-writes",
"verdict": "pass",
"mode": "0o600",
"dev": "16777233",
"ino": "1006414504",
"bytes": 13
},
{
"nativeMode": "off",
"label": "fallback:write:injected-chmod-error-preserves-replacement",
"verdict": "pass",
"mode": "0o600",
"dev": "16777233",
"ino": "1006414507",
"bytes": 11
},
{
"nativeMode": "off",
"label": "fallback:create:injected-chmod-error-preserves-replacement",
"verdict": "pass",
"mode": "0o600",
"dev": "16777233",
"ino": "1006414510",
"bytes": 11
},
{
"nativeMode": "require",
"label": "writeSecretFileAtomic:2600",
"verdict": "pass",
"mode": "0o2600",
"dev": "16777233",
"ino": "1006414526",
"bytes": 26
},
{
"nativeMode": "require",
"label": "writeSecretFileAtomic:7777",
"verdict": "pass",
"mode": "0o7777",
"dev": "16777233",
"ino": "1006414533",
"bytes": 26
},
{
"nativeMode": "require",
"label": "createSecretFileAtomic:4600",
"verdict": "pass",
"mode": "0o4600",
"dev": "16777233",
"ino": "1006414547",
"bytes": 26
},
{
"nativeMode": "require",
"label": "append:sync:empty=false:6600",
"verdict": "pass",
"mode": "0o6600",
"dev": "16777233",
"ino": "1006414641",
"bytes": 33
},
{
"nativeMode": "require",
"label": "append:sync:injected-two-byte-writes",
"verdict": "pass",
"mode": "0o600",
"dev": "16777233",
"ino": "1006414643",
"bytes": 13
}
]Compiled-module bridge to the clean reviewed head{
"pinned-write": "dda092808100414ffd5d7e9fd5f810766fdd304a95d008cdae3ed6c364e22a65",
"root-write-verification": "8d31e316281fc059927778b7a09039ed44adee7580ad93902e849433442f9aba",
"native-pinned-write-windows": "5c714d8ea4cd7cd25e5dc1f46ba1dccfe1bb7683f1b9cb1b65ee9d7a6bd97b13",
"native-operations": "6190390e00b96b13f666164d9eb3ac5bd462c751085e26573f2343d664a3733d",
"replace-file-temp-owner": "d5181978cc3d449b3e7644eb6ae8a42ecf62808c60351262d4fbafdc272f6106",
"regular-file": "c1d34f464150b69516db3ed3bb9663c7e1ca36fd35637fee6c54f15d61f7aff8"
}Node22.0.0 runs already-installed consumers; package-manager execution uses supported Node24. Two earlier pnpm fixtures failed native provenance and are not counted: transitive resolution selected the registry binary, and a first override attempt used the obsolete package.json setting. The corrected fixture's actual-loaded-binary assertion passed for every required-native process. Actual Windows native executionThe hosted Windows native job and its logs built the Windows binding and executed the new tests with The six ownership cases cover both write and create, each with stable publication, replacement before reopening, and replacement during an injected final-chmod failure. They require real replacement bytes to remain, no chmod of the replacement, preservation of the original bytes, and the original refusal/error. The five cleanup cases require removal only for matching authority and preservation when any supplied file/parent identity is unknown. All eleven ran on Windows; they are not among the skips. POSIX mode suites are explicitly skipped there. These Windows mutation tests use public write APIs from the built-source test setup and the actual Windows native mechanism, with deliberately projected colliding file-ID observations. They are not natural-ID-collision evidence or installed-tarball mutation tests. The independent Windows installed-consumer artifact supplies root-only npm/pnpm execution with 36 unprojected directory/admission observations, actual binary-load checks, and source/package receipts: {
"host": "win32-x64-msvc",
"node": "v24.19.0",
"source": {
"commit": "6fe61d3b46e17d1adb350111a483f5e0a9194412",
"tree": "91d94d2060d7959b77899b93c0101b4b78ec7b7d",
"dirty": false
},
"rootIntegrity": "sha512-Ez9Ap/bFy0rbIoWVGnm44T99ZK2LyJcUMr/n4TtP36GfHhFCkcB+Aun44GJxbO9XrEzRjQAgFxAttcJMlTLozw==",
"packagedNativeSha256": "4e9cba23291b02216e37861b4ef307a49837526f679848df2b8ee1a545ffe2f0",
"npm": { "off": { "nativeLoaded": false, "rows": 9 }, "require": { "nativeLoaded": true, "rows": 9 } },
"pnpm": { "off": { "nativeLoaded": false, "rows": 9 }, "require": { "nativeLoaded": true, "rows": 9 } }
}The CI checkout is a merge-ref commit, not the PR head, and its tree exactly equals the reviewed head's tree. The packaged native hash belongs to the package-smoke job; no byte-identity claim is made for the separately built native-check job binary. Equivalent packaged artifacts exist for macOS, glibc Linux, and musl Linux, bringing that separate hosted control to 144 observations. Musl explicitly reports Git metadata unavailable. This evidence does not claim atomic conditional unlink, power-loss durability, POSIX modes on Windows, or absence of every race. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
Maintainer dispositionUnder the standing authorized repair-and-landing scope, retain the documented fail-closed cleanup behavior: unverifiable paths remain for caller-managed cleanup rather than being removed on uncertain authority. Identity checks followed by name-based unlink are not atomic conditional deletion or a rollback guarantee. This repair strengthens those checks; it does not introduce or claim a stronger OS primitive, widen staging permissions, or weaken root/file identity policy. The residual review note therefore requires no further code change. The current-head review, stamped 2026-09-04T05:37:59.850Z for All exact-head CI, coverage, benchmark, and CodeQL gates passed. An additional complete local Proceed with the scoped repair and reverify actual merged main. No release or publication is authorized by this disposition. |
|
Merged as 027f643 and reverified from clean, fast-forwarded The original problem was ordering and ownership: content writes could clear requested special mode bits, secret verification checked only basic permission bits, and some failure cleanup could act on a replacement pathname. The repair finalizes modes through owned descriptors, verifies complete mode bits, and preserves unverified cleanup paths. The adjacent append fix retains permission tightening before data and completes short synchronous writes. Runtime source growth is 48 net lines; cleanup remains best-effort rather than atomic conditional unlink. Fresh actual-main verification passed: the complete CI-bounded check ran 7,347 tests with 80 platform skips, security ran 84 tests, root-only npm/pnpm smoke supplied 36 directory-admission observations, and newly installed root+host tarballs passed 696 mode/append/cleanup observations across Node22.0.0/22.23.2/24.20.0 and off/require. Required-native runs proved the actual fresh binary loaded. Both contention modes also passed 800 contention acquisitions across 32 processes, plus their two release-retry permission cases. The inspectable runtime proof retains the Windows trace and all provenance qualifications. Fresh main receipt: {
"commit": "027f6435122b6f68185754815b7dd9b1eab97c70",
"tree": "91d94d2060d7959b77899b93c0101b4b78ec7b7d",
"dirty": false,
"rootIntegrity": "sha512-ji3Kp11dS+QCM5+r+EO/63tdmNMDHas59VP50OUIn0U90WcPQkrj5jqxCUeGk3+fM0f7RHAu4W3eGg43Ppe4iw==",
"nativeSha256": "6179e193a0d2b696d4afdf2e9b2e021989437439741faa3fdf9b37e9a48da424",
"changedCompiledModuleHashesMatchReviewedProof": 6,
"installedModeObservationsPassed": 696
}Commands run on merged main: pnpm native:buildCI=1 pnpm checkpnpm test:securitypnpm package:smokeThe package-smoke invocation used an explicit fresh output directory; the resulting tarballs were supplied to the separate installed-consumer probe. node scripts/sidecar-contention-proof.mjs offnode scripts/sidecar-contention-proof.mjs requiregit diff --checkAll PR-head CI, coverage, benchmarks, and CodeQL checks passed without a CI rerun. The final review accepted the runtime proof and reported no actionable code/security findings; the documented existing cleanup boundary was retained by maintainer disposition. Post-merge CI and coverage are separately tracked and were still running when this closeout was written. The earlier unconstrained local parallel run remains a failed attempt (eight timeouts and two accompanying cleanup errors); separate serial and CI-bounded checks passed. Two fixture-only pnpm provenance failures are also retained, not relabeled as fresh-native passes. No release or publication was performed. The broader campaign still has outcome-ledger reconciliation and genuinely untested scenarios to resolve. |
1 participant
Summary
Preserve explicitly requested file modes after content writes, and keep final mode changes and failed-write cleanup within the original file's ownership boundary. Content writes can clear set-ID bits: the JavaScript pinned writer previously applied chmod too early, while secret publication verification inspected only
0777and therefore both rejected valid special modes and missed unexpected special bits.The pinned fallback now finalizes mode after content, secret publication compares all
07777bits, and native Windows checks lossless descriptor identity before final chmod. Native failure cleanup compares bigint parent/file receipts. JavaScript fallback cleanup borrows the existing guarded atomic-owner leaf cleanup while retaining its original FileHandle, instead of removing an unverified pathname. Completed publication remains outside failure cleanup.The adjacent regular-file append helpers retain permission tightening before data and restore explicitly requested special bits afterward. Synchronous append now uses the already-open descriptor's complete-write primitive rather than accepting one short write. Raw Root append/writable lifecycles, inherited-mode policy, archive special-bit stripping, public numeric Stats shapes, defaults, and error contracts are unchanged. Runtime source delta: +48 net lines.
Regression evidence
Validation
07777, Root/store buffer/copy/stream facades, async/sync appends, short sync writes, and fallback mode-failure cleanup. All six changed compiled module hashes match a fresh build of clean commit02cfad0bc08c4430fe97cf6e7a8e5f8adbcdd2f7; the artifact itself was packed before commit and records dirty-base provenance.The ordinary parallel
pnpm checkattempt is retained as failed: 7,339 passed, eight test timeouts, 80 skips, plus two accompanying archive cleanup errors. The separate serial result is not a retroactive pass for that attempt.Inspectable after-fix runtime proof includes captured consumer receipt rows, per-process outcomes, compiled-module hashes, the actual Windows native execution trace, and hosted artifact links. The Windows required-native selection passed 52 tests with 403 platform skips, including all six mode-ownership and five cleanup-identity cases. Those built-source/native mutation regressions are distinguished from the separately installed Windows package controls. Hosted root-only package proof covers 144 observations across Windows, macOS, glibc Linux, and musl Linux.
Exact-head CI, coverage, benchmarks, and CodeQL all passed. Optional skipped service/dispatch checks are not counted as test passes.
Boundaries
Cleanup remains best-effort identity checking followed by name-based unlink, not atomic conditional deletion. Unknown or changed authority can leave artifacts for caller-managed cleanup. Windows does not enforce POSIX modes. Append failures may leave appended bytes; no rollback guarantee is added. Documentation and Unreleased changelog entries are included. No version bump, release, or publication.