fix(secret): preserve admitted directory permissions and identity - #222
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: blocked before merge. Reviewed September 3, 2026, 11:47 PM ET / September 4, 2026, 03:47 UTC. ClawSweeper reviewWhat this changesThe PR stops secret and private JSON writes from chmod-repairing existing directories, retains exact directory identity through writes and locks, and adds packaged cross-platform regression proof. Regression provenancePossible regression — probable (reviewed change; reproduction). No predecessor PR is attributed. Merge readiness⛔ Blocked before merge - 4 items remain Keep open for an explicit repository-maintainer decision: the patch is a well-tested security repair, but it deliberately changes established write paths to fail closed when directory modes differ or a new directory cannot be safely pinned. Priority: P1 Review scores
Verification
How this fits togetherSecret-file writes and private FileStore JSON updates create or use directories before publishing credentials or state. This change admits those directories with exact identity and mode checks, then carries that admission into pinned writes and sidecar locking. flowchart LR
A[Caller secret or private JSON write] --> B[Directory admission]
B --> C{Existing mode and identity valid?}
C -->|No| D[Fail closed]
C -->|Yes| E[Retained directory guard]
E --> F[Pinned secret write or sidecar lock]
F --> G[Published secret or JSON update]
Decision needed
Why: The implementation restores the documented security contract and has strong proof, but it can stop previously successful caller write paths; a contributor-authored assertion of approval is not verified maintainer authorization. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: A repository maintainer should explicitly accept the documented fail-closed upgrade behavior, then land the repair with its identity and packaged-consumer regressions intact. Do we have a high-confidence way to reproduce the issue? Yes. Current-main source unconditionally chmods selected private directories despite the documented non-repair contract, and the supplied packaged baseline/proof describes the resulting mode-mutation cases. Is this the best way to solve the issue? Yes, subject to explicit maintainer acceptance of the upgrade behavior. Retaining descriptor-bound admission rather than pathname chmod repair is the narrowest solution consistent with the existing contract and security boundary. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against b4322efc07e2. LabelsLabel 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
HistoryReview history (8 earlier review cycles)
|
Keep secret guards and bound lock roots lossless across fallback and native writes. Normalize Root-backed lock targets without recreating a deleted admitted parent, preserving explicit sidecars for external target keys.
|
@clawsweeper re-review Updated head: The pre-lock deletion finding is reproduced and repaired. The old installed package recreated a deleted admitted parent at The same revision also repairs the actual Windows CI bigint/numeric identity mismatch by retaining lossless identities through secret admission, private lock roots, and native/fallback writes. Strict identity comparisons and Windows unknown-component bounds remain intact. Fresh packed macOS proof passed 186 in-scope observations across exact Node 22.0.0/22.23.2/24.20.0 and off/require modes, including separate-process replacement and deletion. Security and package checks passed; complete-scope Codex autoreview is scoped-clean at P0. Full local parallel/serial failures are recorded in the updated body, not hidden. New exact-head CI/coverage and the fresh Linux syscall run are still pending, so this is a review request, not an automerge request. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
Exercise a stale supplied Root against a fresh held lock without requiring directory rename while a child is open. Retain real replacement witnesses, bound acquisition, and dispose unexpected successful handles without masking body failures.
Exercise real directory identities, separate-process replacements and deletion through public package APIs in off/require modes. Record the actual checkout and loaded binary in existing npm/pnpm smoke artifacts. Let explicit proof output paths override the package-script default so prior artifacts remain untouched.
Use native realpath for Windows loader namespace paths. Mark unavailable Git metadata explicitly for minimal build containers while retaining artifact and behavior checks. Cover missing Git, source archives, and loaded-binary identity controls.
|
@clawsweeper re-review The requested real Windows public-API proof is now complete for reviewed head Windows packaged-consumer receipt · successful CI run The job built the Windows binding, packed the root and host packages, and installed root-only consumers using both npm and pnpm. Fresh off/require processes ran 36 public-API observations on actual Windows x64 / Node 24.19.0. Returned filesystem stats were unchanged; replacements and deletion were performed by separate Node processes. Stable writes/creates/private JSON updates succeeded. Root/parent replacements and deleted-parent lock admission rejected The receipt records actual checkout commit
Selected npm/require observations, using real Windows identities: [
{"operation":"write","scenario":"stable","dev":"3606225537","parentIno":"562949954764910","published":true},
{"operation":"write","scenario":"replace-parent","beforeIno":"562949954764918","afterIno":"562949954764920","error":"path-mismatch","openDispatches":0,"published":false},
{"operation":"private-json","scenario":"deletion","beforeIno":"562949954764943","after":null,"error":"path-mismatch","callbacks":0,"openDispatches":0}
]These real IDs are below the safe-integer boundary; the >53-bit collision regression remains a separately identified synthetic test, not a claim about these observed IDs. The same root-only proof passed 36 cases each on macOS arm64, Linux glibc x64, and Linux musl x64: 144 hosted observations total. macOS/glibc receipts also prove the clean matching checkout tree. The minimal musl container explicitly records Git as unavailable; it still records the collected artifact integrity and actual loaded binary. No missing source metadata is promoted into a verified Git receipt. The direct Windows VM attempt executed no tests and both of its leases were released; it is not used as proof. The latest complete local serial suite passed 7,235 tests / 80 skipped. Earlier parallel/timeouts and the two proof-recorder portability failures remain recorded in the PR body. No library runtime changed after the identity/normalization repair; subsequent changes are tests and proof tooling. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Maintainer decision for this repair: accept the documented compatibility boundaries called out by the final review. Rejecting an existing directory whose mode differs from The final review of |
|
Merged as The landed tree is Main's collected root tarball integrity is Pre-merge proof remains: 7,235 tests / 80 skipped in the final complete serial suite; 144 hosted package observations on Windows, macOS, Linux glibc, and Linux musl; actual non-root Linux syscall proof; green CI, merged coverage, benchmarks, and CodeQL. The final review accepted the Windows authority-chain proof and found no actionable code/security findings. Documented compatibility boundaries were accepted before merge. All failed attempts and platform limitations remain qualified in the body. Two CLI merge attempts returned rate-limit errors despite healthy same-account API observations. The standard GitHub REST merge endpoint succeeded through bare Post-merge main CI and coverage are being tracked separately; their pending results are not counted above. No release, version bump, tag, or registry publication was performed. The explicit file-mode parity finding remains the next separate campaign repair. |
Summary
Make secret-directory admission match the documented non-repair policy. Existing roots and parents, including
EEXISTwinners, must already have the exact requested POSIX mode; the writers no longer chmod those pathnames. Mismatches reportinsecure-permissionswithout writing the secret. New-directory initialization uses guarded descriptor authority tied to the admitted identity and effective UID, with ancestry checks and full07777mode verification.Retain lossless bigint directory identities through admission, private lock roots, and native/fallback writes. This corrects the Windows failures on the first PR revision without rounding identities or loosening swap checks. Private JSON locking prepares directories before sidecar acquisition, binds the lock to that admitted parent, and now normalizes Root-backed target keys without creating directories. A deleted or replaced parent is rejected rather than recreated.
Current head:
cf2b9b1be99c8b59c91cae22da55e08c64fb22b6; tree:d053dc472dda638a5a588bee00d6931207765f17. Runtimesrc/, native sources, package manifests, and dependency pins are unchanged since614ff9d; subsequent commits correct test ownership and add durable packaged-consumer proof. The earlier pnpm artifact6591b090ed1c501114f6b9fd4264c85a281a9c4ae2b871f64ab7cbc9c4385de5was rebuilt byte-identically at the test-only5f02fc2revision. Current CI receipts separately record the collected root tarball integrity, compiled module hashes, actual checkout commit/tree, and loaded host binary; the new changelog/proof-tooling revision is not mislabeled as that earlier tarball.Boundaries and compatibility
02750retain their special bits. Invalid mode values fail withinvalid-pathbefore creation; changed initialization ownership reportsnot-owned.000directory underumask(0777); actualO_SEARCHand SDK-verifiedO_EVTONLYprobes failed. That case fails closed without repair or secret publication. Search-preserving masks remain usable; Linux retains guardedO_PATH/procfs initialization.Root.create. External arbitration targets with explicit in-root sidecars, parent aliases, leaf-symlink key semantics, lock-option precedence, and stale recovery remain supported. Non-Root and synchronous acquisition policies are not redesigned.0777. Those observations are not counted as fixed here.Executed defect and regression proof
The original physical non-root baseline exercised 24 cases: both writers silently changed existing
0750/02750roots or parents to0700in 8 documented-contract violations. Four additional late-replacement regressions showed why the admitted guard chain must survive traversal and handoff instead of being reconstructed.The first PR head,
b1247bcb, failed actual Windows Node 22/24 CI and Windows coverage. Two synthetic high-inode tests reproduced stable-directory refusal at the mixed bigint/numeric comparison. The corrected tests cover exact admission and private Root identities, two distinct inode values with the same numeric projection, real writes through fallback/native and simulated Windows branches, and bounded unknown-identity handling. These synthetic identity projections are not presented as naturally occurring macOS inode values or actual Windows-host proof.ClawSweeper also identified the pre-lock deletion race. A new real-filesystem regression and installed-package probe confirmed it: a separate process deleted an admitted parent; the old package recreated it at 0755, then rejected
path-mismatchwithout running the JSON callback. The corrected source leaves the parent absent. Compatibility tests cover external target keys, explicit in-root locks, aliased missing parents, target-leaf symlinks, and stale-Root rejection before reentrant reuse.Existing adversarial fixtures now start with valid private modes so their attacks reach the intended boundary. Swap witnesses remain mandatory. No attack assertion, workload, timeout, or source-size budget was weakened.
Current-head verification
git diff --checkpassed.614ff9dCI and coverage failed in a new held-root fixture, not the earlier bigint comparisons. Its unconditional cleanup masked the original Windows failure. The test now physically replaces the Root before opening a fresh holder, then proves that a stale supplied Root cannot reuse that held entry. It retains real identity witnesses and closes any unexpected second handle; no Windows skip or weaker assertion was added.5f02fc2, passed CI and coverage, including actual Windows Node 22/24 and native coverage. Its complete serial suite passed 7,228 tests / 80 skipped. The corresponding parallel attempt still failed in five timing-sensitive tests.--outputoverrides the package-script default without overwriting earlier artifacts.5f6a442, all Node checks and coverage passed, and Linux/macOS packaged traces each passed 36 cases. Their actual checkout was merge ref8b83e1f27892c5d43125d36027dd781c2ded42a6, with a clean tree exactly matching reviewed tree55d207084f2212898c35c16d4fdc1de8d2efc236. Windows completed its require-mode API assertions but its native-load recorder failed on a loader namespace path; Alpine's recorder failed because Git was absent. Neither failed job is counted as passing.realpathfor loader paths and records Git metadata as explicitly unavailable in minimal containers, rather than making Git a new runtime requirement. It passed 13 focused tooling tests, 36 local packaged cases, final package checks, and a complete serial suite of 7,235 tests / 80 skipped. Latest parallel validation still failed in 3 timeouts, with 7,232 passed / 80 skipped. The earlier 32-failure attempt remains retained. Complete-scope P0 autoreview is scoped-clean.19d3563a8d1427ae41a8b849cde12f42a1b658da(CI merge ref) and treed053dc472dda638a5a588bee00d6931207765f17, exactly matching the reviewed head. macOS/glibc receipts prove the same tree. The musl container explicitly marks Git metadata unavailable; artifact and native-load proof remain valid, but no Git receipt is invented.sha512-dGc1+UXv5wuAwatBGEjj73tGuKyNLxaOg/TY0NQS0yIFqxXA1mgywwTqo1cZNwC30oT4w7D3r9CtbAokD4yO2A==; loaded binary SHA-256:228e5de7651d3ffe9ea7ae5cd9054ed105677b07c833f117f6c22c5b2189e76f. Unix root tarball integrity:sha512-AcIxm4f2fvXvqx9FjQ5N236PUQPepClcmFbCPH8PODjC/kU4cSY55daxO1UVkh5ltCFZr/kfawEPizGHL5+j7A==. Distinct platform artifacts are retained rather than assumed byte-identical.3606225537, parent inode562949954764918replaced by562949954764920; the writer rejectedpath-mismatchwith zero observed open dispatches and no publication. Deleted-parent JSON admission left the parent absent and invoked no callback. These real IDs are below2^53; large-integer collision tests remain separately labeled synthetic evidence. The final ClawSweeper review, stamped 2026-09-04 03:37:03.978 UTC for this exact head, found no actionable code/security findings and accepted the Windows proof. Its remaining compatibility-acceptance flag is accepted for this repair because the non-repair behavior restores the existing documented contract and the fail-closed initialization limit is explicitly documented.The direct AWS Windows attempt (
run_a5d9bc53313b) never brought up SSH: the first lease hit its 20-minute bootstrap deadline, then the CLI allocated a second. The job was stopped, and both leases were explicitly released. No Windows test command ran there; its original held-directory rename behavior remains undiagnosed. The existing hosted Windows package job is the proof route instead.Linux evidence provenance
The fresh final artifact completed AWS run
run_deeba84b91e5, leasecbx_44de4fb4bbef, on Linux x64 / exact Node 24.20.0 as UID 1000. All tarball/probe hashes were verified before execution. Native bindings were omitted; realstrace -f -yyAPI markers separated library mutations from fixture setup/cleanup.O_PATH/ authenticated procfd initializations0775in the held PR artifact)The run covered 24 policy observations per main/final revision (two file-mode observations per revision remain deferred), four private-JSON cases, four final-candidate late replacements, and the deletion case in both the held PR artifact and final candidate. All final replacement/deletion cases rejected
path-mismatch, ran no forbidden JSON callback, and published no secret. Both Linuxumask(0777)initializations succeeded through descriptor authority. Explicit lease cleanup confirmed release after the broker initially reported cleanup pending. This is syscall/behavior proof, not a power-loss claim.Earlier
b1247bcbLinux evidence from runrun_e3b05dac38bfremains recorded separately; it is not substituted for this final-artifact run.No release, version bump, tag, registry publication, dependency addition, or generated-output commit is included.