Repository navigation
Conversation
…rt only (#445) The removal primitives of the pinned module exist only with the test-only feature and have no caller. On Unix an object is moved into a quarantine directory with RENAME_NOREPLACE, the open object is identified there and only then unlinked, with the link count checked afterwards; a swap of the last name or a manipulation inside the quarantine directory is moved back and never unlinked. No exclusivity of that directory is assumed: the one window no Unix primitive closes is forced in a test and reported as Disturbed. On Windows the object is opened through the handle-relative lookup with delete access and read-only sharing, so no writer can change the authenticated bytes, and the POSIX delete disposition is set on that same handle. There is no FILE_FLAG_DELETE_ON_CLOSE and no fallback. Leftovers of a crash are finished only after authenticating them again. A scan of the sources and a compile of a dependent with and without the feature keep the primitives out of every other build.
…on (#445) The first draft of the removal was one pull request of 2,172 meaningful lines, over the 1,200-line hard-advisory tier. This commit keeps what has to land first: the API behind test-support, the proofs that it exists in no other build (a scan of the sources and a compile of a dependent with and without the feature), PinnedDir::sync, and the contract with the limit no Unix primitive can remove. Every removal answers Unsupported here; the Unix file removal, the Unix directory removal with the leftovers, and the Windows delete follow as stacked pull requests, each with its own tests.
…the open object (#445) The authenticated file is moved into a quarantine directory with RENAME_NOREPLACE, the open object is identified there and only then unlinked. Anything else is put back, never unlinked. Every failure after the move is handled at one point: the entry goes back, or the outcome is Stranded and says where it is. The parent is synced whenever it changed. The quarantine directory is not assumed to be exclusive. The one window no Unix primitive closes is forced in a test and reported as Disturbed: detected after the fact by the link count of the authenticated object, not prevented.
… its own (#445) CodeScene counted the table as part of a 73-line test; the table is data, the test is the loop over it.
…unix-file-removal # Conflicts: # CHANGELOG.md # crates/worldscript-secure-storage/src/pinned/tests/mod.rs # crates/worldscript-secure-storage/src/pinned/tests/remove.rs # crates/worldscript-secure-storage/src/pinned/unix/removal.rs # docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md
…tory that was made (#445) Review findings on the Unix file removal: - A failure after the move that put the entry back, or left it in the quarantine directory, returned without syncing the parent. The sync is now attempted after both (best effort, so it does not replace the failure being reported) and the BeforeSync stage is reached, which a test observes. - The quarantine directory was removed by its name alone, so a directory that an actor put at that name after renaming the held one away could be removed. The cleanup now checks that the name still leads to the directory that was created; a test forces the swap. - The permission-based Stranded test failed for root. It now skips with a message for root, and the same logic is covered without permissions by a case that blocks the put-back by taking the name again.
…creation race (#445) More review findings on the Unix file removal: - The path where the name vanished before the move created and removed a quarantine directory and reported no change; the parent is now synced. - An entry that stays in the quarantine directory (put-back blocked, or stranded) is retained with a best-effort sync of that directory as well as of the parent. - That the directory behind the quarantine name is the one mkdirat made cannot be proved, since mkdirat returns no descriptor. The documentation says so and why it does not widen the window the contract already states.
…ove, refuse an unopenable one (#445) Further review findings on the Unix file removal: - An entry that stays in the quarantine directory is only reported as being there if the recorded name still leads to that directory. If an actor renamed it away the outcome is Disturbed, which now also covers an entry that cannot be found by name. A test forces the rename. - A failure to move the entry at all (renameat2 unsupported or failing) created and removed a quarantine directory without syncing the parent; it now goes through the same best-effort sync as a rollback. - A umask that takes the owner's read access away leaves a quarantine directory that cannot be opened. The removal is refused (fail closed) and a test pins that, instead of the documentation promising more.
…own part (#445) The Unix file removal had grown to 1,306 meaningful lines through review waves, over the 1,200-line tier. Its hardening against a same-user actor in the quarantine directory and the durability of the failure paths (sync after a rollback or a stranded entry, an identity-checked cleanup of the quarantine directory, a quarantine that was renamed away, an unopenable one, the creation race stated) is a separate proof boundary and is re-applied as the next part on top of this one. What stays here is the removal itself with its single rollback point, the Stranded outcome, the sync of changes that are made, the forced swap tests and the link-count window. The root-skip of the permission case stays.
… the quarantine directory (#445) The parent is synced after a rollback, a stranded entry, a put-back and a vanished name, and an entry that stays in the quarantine directory is retained with a best-effort sync of that directory too. The quarantine directory is removed by its name only if the name still leads to the directory that was created. An entry is reported as retained only if the recorded name still leads to the directory that holds it; otherwise the outcome is Disturbed. A quarantine directory that cannot be opened is refused. Tests force each case, including the quarantine renamed away and the put-back blocked, so they also run for root.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedEnable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Next included review available in 25 minutes. View limit details
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: 🔵 Low · up to
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
Reviewer's GuideThis test-support-only change hardens Unix authenticated file removal by tracking quarantine identity, avoiding deletion of replacement directories, reporting renamed-away entries as Disturbed, failing closed on owner-masked quarantines, and synchronizing affected directories on failure paths; comprehensive forced tests and durability documentation cover the new behavior. Sequence diagram for hardened Unix quarantine removalsequenceDiagram
participant Caller
participant Removal
participant Quarantine
participant Parent
participant Observer
Caller->>Removal: remove_authenticated_file
Removal->>Quarantine: mkdirat
Quarantine-->>Removal: identity
Removal->>Quarantine: adopt
Quarantine-->>Removal: entry moved
Removal->>Removal: judge
alt removal succeeds
Removal->>Quarantine: unlinkat
Removal->>Observer: BeforeSync
Removal->>Parent: sync
else rollback succeeds
Removal->>Quarantine: restore
Removal->>Observer: BeforeSync
Removal->>Parent: sync
Removal-->>Caller: error
else put-back fails and quarantine remains named
Removal->>Quarantine: retained
Quarantine->>Quarantine: sync_all
Removal->>Observer: BeforeSync
Removal->>Parent: sync
Removal-->>Caller: Stranded
else quarantine was renamed away
Removal->>Quarantine: retained
Removal-->>Caller: Disturbed
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
codeant-ai
Bot
added
the
size:L
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30882e5012
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
CodeAnt PR Risk: Medium Risk
Assessed commit: |
…unix-hardening # Conflicts: # CHANGELOG.md # crates/worldscript-secure-storage/src/pinned/tests/remove_unix.rs # crates/worldscript-secure-storage/src/pinned/unix/removal.rs # docs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
[check-pr-size] PR size is over the hard tier (normal profile): 8 files, 932 meaningful lines, 15 commits — limit ≤20 files / ≤1200 lines / ≤10 commits. Consider splitting into smaller, independently reviewable PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b42f3fbd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…eld directory, narrow the cleanup claim (#445) Review findings on the hardening: - retained() checked the name of the quarantine directory but not that the entry is still inside; an actor that removed it and blocked the put-back got Stranded. A proven absence is now Disturbed (an error while looking is not a proof of absence). - When the held quarantine directory was renamed away, the removal goes on in it and now syncs it as well as the parent before reporting Removed. - A failure while constructing the quarantine directory synced nothing after removing it; it syncs the parent best effort. - The identity check before the name-based rmdir narrows the race and does not close it; code and documentation say so. - The permission-based tests skip for root only where the permission is the subject; the renamed-away and removed-entry cases run for every user.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe7158ea30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: qnbs/WorldScript-Studio/.coderabbit.yaml
- Review profile: CHILL
- Plan: Essentials
- Run ID:
ccfa2d0f-f20d-4b32-9846-83631fdef3c3
📒 Files selected for processing: 5
CHANGELOG.mdcrates/worldscript-secure-storage/src/pinned/removal.rscrates/worldscript-secure-storage/src/pinned/tests/remove_unix.rscrates/worldscript-secure-storage/src/pinned/unix/removal.rsdocs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.
…antine directory, document the disturbed cases (#445) Review findings on the hardening: - retained() accepted any object at the recorded entry name, so an actor that moved the authenticated entry away and put another object there while blocking the put-back got Stranded. The entry must now still be the authenticated object (for a swapped-in object: the identity it was seen with); a proven absence or another object is Disturbed, and an error while looking proves neither. - The held quarantine directory is synced after the unlink in every case (removed afterwards, renamed away, or kept alive by an entry an actor put into it); a sync that fails makes the removal Removed(NotConfirmed) instead of confirmed. - The documentation of Disturbed names the removed and the replaced entry.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…a failed sync as itself (#445) The cross-directory rename that puts an entry back, or moves it aside, changes the quarantine directory as well as the parent. Only the parent was synced, so after a put-back or a kept entry a crash could undo the change in the quarantine directory while the parent already showed the name (CodeAnt risk note on e39e837). The quarantine directory is now synced once after the rename was tried, in every path that moved an entry through it. The rename alone decides `restored` and the sync is a separate result, so a sync that fails after a rename that worked is not taken for a put-back that did not happen: no false `Disturbed` or `Stranded`, which a `sync_all()?` inside the rename step would have caused. - Swapped (put back, or kept aside): a failed sync is an error reported after the parent was synced, like a failing parent sync, and not another outcome. - RolledBack and Stranded: best effort, the reported cause is not replaced. - Disturbed (tampering) outranks a failed sync. - Removed: unchanged, `Removed(NotConfirmed)` when the sync fails. - Where nothing moved, nothing is synced. An fsync error on a directory cannot be provoked portably, so the failing cases use a small cfg(test) seam in `Quarantine::sync`; the cases without a failure run the real sync and count how often it was asked for. The seam also closes the gap that the Removed(NotConfirmed) path had no end-to-end test. Mutation-checked: leaving out the sync after a put-back, after a rollback, dropping a failed sync, a `sync_all()?` inside `restore()` and confirming a removal despite a failed sync each fail at least one of the new cases. test-support only, no caller, no new unsafe, no new package. Issue #359.
codeant-ai
Bot
removed
the
size:L
|
Disposition of the CodeAnt "PR Risk: Medium" note on Valid. Fixed in The finding holds on the code at What changed
What pins it (
Limits, stated
Local evidence only, hosted CI pending on this head: the removal unit tests ( |
codeant-ai
Bot
added
the
size:XL
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Sync the quarantine directory when x vanishes after adoption. · removal.rs:364
crates/worldscript-secure-storage/src/pinned/unix/removal.rs:364
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSync the quarantine directory when
xvanishes after adoption.If
adoptmoves an entry and another actor removesxbeforeinspect, this branch returnsVanished. It skipsQuarantine::sync, although the move changed that directory. Sync it before settlement, and report a failed sync after the parent sync. The updated durability contract indocs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.mdrequires a quarantine-directory sync after a move.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: qnbs/WorldScript-Studio/.coderabbit.yaml
- Review profile: CHILL
- Plan: Essentials
- Run ID:
4bb3cf30-9051-43da-aad2-2b4939e6ede9
📒 Files selected for processing: 7
CHANGELOG.mdcrates/worldscript-secure-storage/src/pinned/removal.rscrates/worldscript-secure-storage/src/pinned/tests/mod.rscrates/worldscript-secure-storage/src/pinned/tests/remove_unix.rscrates/worldscript-secure-storage/src/pinned/tests/remove_unix_sync.rscrates/worldscript-secure-storage/src/pinned/unix/removal.rsdocs/native/r15/GATE4D-JOURNAL-DURABLE-EVIDENCE.md
🚧 Files skipped from review as they are similar to previous changes: 1
- CHANGELOG.md
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.
There was a problem hiding this comment.
💡 Codex Review
Sync the quarantine when inspection finds the entry absent
When a same-user actor removes x before this inspection and leaves another entry in the quarantine so discard cannot remove the directory, this branch returns Vanished after syncing only the parent. The held quarantine was changed by the move and removal, so a crash can restore x there despite the operation having returned; route this path through a quarantine sync just like the other post-move outcomes, and report a sync failure rather than claiming completion.
AGENTS.md reference: AGENTS.md:L31-L33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ck the identity when a failed setup is cleaned up (#445) Two findings on aff1678, both valid. An entry that was moved aside and then found gone ("Vanished" at the inspection, or an ENOENT at the unlink) had changed the quarantine directory too, and the previous commit did not sync it for that outcome (Codex P2 and CodeRabbit, independently): the earlier claim "every path that moved an entry through it" was too broad. Both paths now go through Step::Moved like a put-back: the directory is synced after the parent, and a sync that fails is the error instead of a completed removal. The unlink path used to run the sync and drop its result for this outcome. Quarantine::create removed a directory by its name after a failed setup without the identity check that discard() has (CodeAnt). The identity is now taken right after mkdirat, which returns no descriptor, and the cleanup removes the name only if it still leads to that directory; without an identity nothing is removed. discard() and the cleanup share remove_if_named(). rmdir takes a name, not a descriptor, so this narrows the race and does not close it: what a lost race can remove is at most an empty directory. Tests: two scenes (entry gone before the inspection, gone before the unlink) in the sync cases, with and without a failing sync; the removal by name is exercised on its own (the directory that was made goes; one renamed away and replaced, a non-empty one and a name without an identity stay). The setup failure that reaches the cleanup in create() needs a changed process umask and is not forced. Mutation-checked: skipping the sync for an entry gone at the inspection, dropping it for one gone at the unlink, and removing the identity comparison each fail at least one case. CHANGELOG and the evidence document say what the tests pin. test-support only, no caller, no new unsafe, no new package. Issue #359.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
Second wave, on Correction first. My disposition above says the quarantine directory is synced "in every path that moved an entry through it". That was too broad. One path was missing, and Codex (P2) and CodeRabbit (Minor, outside the diff) found it independently. 1. Entry found gone after it was moved aside (Codex P2, CodeRabbit, 2. Cleanup after a failed setup removed the directory by name without an identity check (CodeAnt, What pins it
Limits, stated. The call in Budget, plainly. This is commit 15 of 15. The budget tool reports |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae96312f19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ## [Unreleased] | ||
|
|
||
| - **R-15 Gate 4:** hardening of the Unix file removal against a same-user actor in the quarantine directory, `test-support` only (slice H1a-2, part 2b of 5). The parent is synced after a rollback, a stranded entry, a put-back and a vanished name, and the quarantine directory an entry moved through is synced too (a sync of it that fails is an error after a put-back, a kept entry or an entry an actor removed again, and makes a removal `NotConfirmed`, never another outcome); the quarantine directory, also when its setup fails, is removed by its name only if the name still leads to the directory that was created; an entry is reported as retained only if the recorded name still leads to its directory, otherwise `Disturbed`; a quarantine directory that cannot be opened is refused. Tests force each case, including the quarantine renamed away and the put-back blocked. No new `unsafe`, no new package. Issue #359. PR #1072. |
There was a problem hiding this comment.
When a same-user actor replaces the checked quarantine with an empty directory between identity_at and the name-based unlinkat, remove_if_named can remove that replacement, so this release note's “only if” statement overstates the preservation guarantee. Fresh evidence after the earlier finding is that the final changelog still makes the absolute claim even though the code and evidence document now acknowledge that the identity check only narrows the race; qualify this summary the same way.
AGENTS.md reference: AGENTS.md:L29-L30
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, wording only. remove_if_named checks the identity and then calls unlinkat by name, so a swap between the two can still remove an empty replacement directory. The code comment and the evidence document say the check narrows the race and does not close it; this CHANGELOG line still says "only if".
Not fixed on this head: the PR is at 15 of 15 commits (budget SATURATED), and a 16th commit is not allowed. The thread is left unresolved on purpose. The wording belongs in the next change that touches this entry (or in a clean successor), which is the maintainer's call; I will not describe it as fixed before a commit exists.
| Ok(Step::Changed(RemovalOutcome::Swapped { restored })) | ||
| } | ||
| Inside::Object(_, found) => self.swapped(Some(found)), | ||
| Inside::Foreign => self.swapped(None), |
There was a problem hiding this comment.
When the swapped-in entry is classified as Foreign (for example a symlink, directory, FIFO, or inaccessible file), this arm passes no expected identity, so if a same-user actor replaces x after inspection and blocks the restore, holds(None) accepts any replacement and the operation returns Swapped { restored: false } even though the observed entry is no longer at the recorded recovery location. Fresh evidence beyond the prior retained-object finding is this distinct Inside::Foreign arm explicitly bypassing the identity check now supplied by the Object arm; retain the no-follow stat identity for foreign entries and report Disturbed on a mismatch.
AGENTS.md reference: AGENTS.md:L29-L33
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid. Inside::Foreign is passed as swapped(None), so holds(None) accepts any object at x: if a same-user actor replaces a foreign entry (link, directory, FIFO, inaccessible file) after the inspection and also blocks the put-back, the outcome is Swapped { restored: false } although the entry is no longer where it says. e39e8377 closed this for the Object arm (the identity it was seen with); Foreign is the remaining gap in "reported as retained only if the entry is where the outcome says".
The fix is small: carry the no-follow stat identity of the foreign entry in Inside::Foreign and report Disturbed on a mismatch. It also needs a test seam, because no observer stage sits between the inspection and the retained check, so the replacement cannot be forced deterministically today.
Not fixed on this head: the PR is at 15 of 15 commits (budget SATURATED), and a 16th commit is not allowed. The thread is left unresolved on purpose. Whether this is fixed in a clean successor or in the next change to this file is the maintainer's decision; until a commit exists I do not call it fixed.
Operator control checkpoint — 2026-10-15, exact head
|
Maintainer decision already recorded — execute clean successor, do not wait for another choice (2026-10-15)The latest replies at 18:54 UTC correctly acknowledge both Codex P2s but again describe the corrective route as a "maintainer's call." That call has already been made and published: canonical Linear Gate4–7 ledger, QNB-168, and the earlier PR control comment. FINAL DECISION FOR THIS BRANCHING QUESTION: Carry forward the reviewed net semantic diff of #1072 and preserve As of the last audit, #1072 exact head Unattended execution rule: Re-read AGENTS.md and repository safety/merge rules. Distinguish a new required owner decision from routine engineering decisions already delegated. Do not turn an existing authorization, a CI wait or a reviewer quota into an unnecessary idle stop. Continue bounded safe investigation/preparation during waits; refuse only the action genuinely blocked by a security or merge gate. Report verifiable heads, links, gates and progress succinctly; keep QNB-168 and #359 up to date without duplicated narrative. No instruction here authorizes bypassing deterministic gates. |
|
Successor opened: #1073. This PR stays frozen and must not be merged. #1072 is at 15 of 15 commits (budget This PR is closed as superseded, not merged, once #1073 is integrated. CI and review results on |
…ity proof (#445) (#1073) * feat(core): harden the Unix file removal against a same-user actor in the quarantine directory (#445) Clean successor of #1072 (head ae96312, 15 of 15 commits, budget SATURATED): the reviewed net state of that PR plus the two findings that were still open on it. test-support only, no caller, no new unsafe, no new package. Carried from #1072: the parent is synced after a rollback, a stranded entry, a put-back and a vanished name; the quarantine directory an entry moved through is synced too, and a sync of it that fails is an error after a put-back, a kept entry or an entry an actor removed again and makes a removal NotConfirmed, never another outcome; the rename alone decides `restored`; the quarantine directory is removed by its name only after an identity check, also when its setup fails; an entry is reported as retained only if the recorded name still leads to its directory, otherwise Disturbed; a quarantine directory that cannot be opened is refused. Fixed on top of it: - A foreign entry (link, directory, FIFO, inaccessible file) carries the identity it was seen with (a look that does not follow a link), as an opened object does, so an actor that replaces it after the inspection while the put-back is blocked gets Disturbed instead of Swapped { restored: false }. An identity that cannot be taken is a failed inspection and not a retained entry, so an unobservable state earns no unproven success. The test replaces the entry at the sync of the quarantine directory, the one point between the inspection and the check that a test can reach, and a control leaves the same entry untouched (it is still kept). - The changelog no longer says the name-based cleanup removes "only if" the name leads to the directory that was created: the check narrows the race and does not close it, as the code and the evidence document already said. Mutation-checked, each caught: passing no identity for a foreign entry, plus the mutations recorded on #1072 for the sync, the identity comparison and the vanished-entry paths. The failing-sync cases use a small cfg(test) seam that models the error; no crash is simulated. Issue #359. The PR number in the changelog entry is to be set to the number of this PR when it is opened. * docs(changelog): set the PR number of the Unix quarantine hardening entry (#445) * fix(core): treat a failed look at a retained entry as no proof, state the limit of a foreign identity (#445) Two findings on 8f8c988, both valid. A look at the entry that failed (a search permission lost after the move) was counted as "the entry is there", so a removal whose put-back was blocked reported Stranded, and a kept foreign entry reported Swapped (restored false), without having seen the entry (Codex P2). An outcome that says where an entry is needs a proof, and a failed look proves nothing: it is now Disturbed. The identity that is compared is no longer optional (the "any object counts" path had no caller left), so there is no lenient path to fall into. The Stranded case that is proved, with the name taken again, is unchanged and runs for root as well; the permission case now expects Disturbed and still checks that the entry is in fact the authenticated inode, which is why this is conservative and not a claim that the directory was tampered with. The identity of a foreign entry is a stat identity without a held handle (Codex P2): a link, a FIFO or a device node is not opened, because opening has effects, and no handle can be taken for every kind without them. A file system may give the identity of a removed object to its replacement, which the check cannot tell apart. This is stated where the outcome is described (the variant, the Disturbed contract, the evidence document and the changelog) rather than covered by a mechanism that would hold for Linux only; it is part of the same-user residual that stays owner-gated before any production use. Mutation-checked: counting a failed look as present fails the permission case. test-support only, no caller, no new unsafe, no new package. Issue #359.
|
Superseded by #1073, not merged. #1073 was integrated as This PR was never merged, no history was rewritten and the branch is kept. Checks and reviews on |
This branch was successfully deployed
1 participant
User description
Part of #359 (R-15 Gate 4, slice H1a-2), part 2b of 5, stacked on #1069 (base = its branch).
test-supportonly: no production caller, no production deletion release.Why a part of its own: the Unix file removal reached 1,306 meaningful lines through its review waves (tier 1,200). The hardening against a same-user actor in the quarantine directory and the durability of the failure paths is a separate proof boundary; it is cut out of #1069 (one revert commit there, no force-push) and re-applied here, so the removal itself stays reviewable at 1,109 lines and this part at 263.
What this adds
BeforeSyncstage is reached, which tests observe.discardremoves the name only if it still leads to that directory (at most an empty directory can be removed by a name-basedrmdir).Swapped { restored: false },Stranded) only if the recorded name still leads to its directory; otherwise the outcome isDisturbed(the entry cannot be found by name).Disturbeddocumentation widened accordingly.PermissionDenied, fail closed), pinned by a test.mkdiratmade cannot be proved (no descriptor is returned); the quarantine is not exclusive anyway.Proof
Forced cases: failure at each stage with the sync stage observed after a move and not before it; the put-back blocked by taking the name again (
Stranded, whoever runs it, also for root); a quarantine directory renamed away and replaced by an empty one (cleanup leaves it alone); a quarantine renamed away while the put-back is blocked (Disturbed, the entry intact in the renamed directory); a vanished name (sync reached); the permission-basedStrandedcase skips for root with a message. Local: fmt, clippy-D warningson Linux / Windows / macOS targets, lib and isolation tests.Answers the Codex / CodeRabbit / CodeAnt findings on #1069 raised after its first head (sync after rollback, replacement quarantine directory, root, creation race, quarantine sync, vanish sync, owner-masking umask, adopt-error sync, renamed quarantine).
Status:
H1A_2_UNIX_HARDENING=IMPLEMENTED_TEST_SUPPORT_ONLY,H1_PROVED=NO,R1_PRODUCTION_RECLAMATION=NOT_ADMITTED,GATE4_TERMINAL=NO.Summary by Sourcery
Harden the test-only Unix file removal flow against quarantine tampering while preserving durable and truthful failure outcomes.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by cubic
Hardens the Unix file removal in
test-supportagainst a same-user actor manipulating the quarantine directory, making failure paths durable and reporting tampering clearly. No production caller touches this code.Removed(NotConfirmed).discardremoves the name only if it still leads to that directory, and the same identity check applies when a failed setup is cleaned up. An entry is reported as retained only if the recorded name still leads there and the entry is still the authenticated object (or the identity a swapped-in object was seen with); a proven absence or a replacement isDisturbed.PermissionDenied, fail closed).unsafeor package.Written for commit ae96312. Summary will update on new commits.
Summary by CodeRabbit
CodeAnt-AI Description
Harden test-only Unix file removal against quarantine tampering and sync failures
What Changed
Impact
✅ Test-only removal leaves replacement quarantine directories untouched✅ Tampered or missing quarantine entries are reported as disturbed✅ Failed quarantine syncs do not report removals as confirmed💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.