Repository navigation
fix(core): complete Unix quarantine hardening and foreign-entry identity proof (#445) - #1073
Merged
Merged
Conversation
… 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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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 · |
Reviewer's GuideCompletes the Unix quarantine hardening by synchronizing every directory affected by quarantine moves, making removal durability contingent on both directories, and requiring identity evidence for cleanup and retained foreign entries; extensive test-only seams and scenarios verify failure, tampering, and race-detection behavior while documenting the remaining name-based limitations. Sequence diagram for hardened Unix quarantine removalsequenceDiagram
participant Caller
participant ParentDir
participant Quarantine
participant Observer
Caller->>ParentDir: renameat_with(..., RENAME_NOREPLACE)
ParentDir-->>Caller: entry moved to quarantine
Caller->>Quarantine: inspect(kind)
Quarantine-->>Caller: Inside::Object or Inside::Foreign(identity)
alt authenticated object
Caller->>Observer: Observer(RemovalStage::BeforeUnlink)
Caller->>Quarantine: unlinkat(x)
Quarantine-->>Caller: removal result
Caller->>Quarantine: sync()
Caller->>ParentDir: sync()
Caller-->>Caller: Removed(Confirmed or NotConfirmed)
else foreign or swapped entry
Caller->>Quarantine: restore()
Caller->>Quarantine: sync()
Caller->>ParentDir: sync()
Caller-->>Caller: Swapped or Disturbed
end
State diagram for Unix quarantine removal outcomesstateDiagram-v2
[*] --> MovedToQuarantine
MovedToQuarantine --> Inspected
Inspected --> Removed: authenticated object
Inspected --> Restored: foreign identity matches after check
Inspected --> Stranded: restore failed and retained identity is proven
Inspected --> Disturbed: recorded quarantine or entry identity no longer matches
Inspected --> Vanished: entry is gone
Removed --> ParentSynced
Removed --> NotConfirmed: quarantine sync failed
Restored --> ParentSynced
Stranded --> ParentSynced
Vanished --> ParentSynced
ParentSynced --> [*]
Flow diagram for foreign-entry identity proofflowchart TD
A[inspect quarantine entry] --> B{Entry is foreign?}
B -- no --> C[continue authenticated-object checks]
B -- yes --> D["identity_from(stat without following link)"]
D --> E{Identity available?}
E -- no --> F[inspection failure; restore or strand]
E -- yes --> G[restore]
G --> H{Entry still has recorded identity?}
H -- yes --> I[Swapped]
H -- no --> J[Disturbed]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
codeant-ai
Bot
added
the
size:XL
|
[check-pr-size] PR size is over the target tier (normal profile): 8 files, 1014 meaningful lines, 3 commits — limit ≤8 files / ≤400 lines / ≤6 commits. Consider splitting into smaller, independently reviewable PRs. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
![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.
![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: 8f8c98830f
ℹ️ 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 PR Risk: Low Risk
Assessed commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… 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.
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.
Owner
Author
|
@coderabbitai review One request, after the rate-limit window: the last CodeRabbit review of this PR was on |
✅ Action performedReview finished.
|
This was referenced Oct 15, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
What this is
A clean successor of #1072, which is superseded and not merged. #1072 reached 15 of 15 commits (budget
SATURATED) with two valid P2 findings still open, so it cannot take a 16th commit. This PR has two commits onmain(45ce3d0c): the substantive successor commit8f8c9883carries the reviewed net tree of #1072 (headae96312f) plus the fixes for those two findings, and the second commit99336659updates only the CHANGELOG PR reference. Commit-signature verification remains a CI gate. #1072 stays frozen until this PR is integrated; no history was rewritten.test-supportonly: the primitives exist only with the test-only feature and have no caller. No newunsafe, no new package. Slice H1a-2, part of #359 (Linear QNB-168).The two findings open on #1072, and how they are closed here
remove_if_namedchecks the identity and then callsunlinkatby name, so a swap between the two can still remove an empty replacement directory. The entry now says the check narrows the race and does not close it (rmdirtakes a name; a lost race removes at most an empty directory), and that retained-entry detection is a look at a name that detects after the fact.Inside::Foreignwas passed asswapped(None), soholds(None)accepted any object atx. A link, directory, FIFO or inaccessible file now carries the identity it was seen with (a look that does not follow a link). An identity that cannot be taken is a failed inspection, not a retained entry, so an unobservable state earns no unproven success.remove_unix_sync.rs, sceneForeignReplaced: a directory takes the name, the name is taken again so the put-back is blocked, and the entry is replaced after the inspection (run at the sync of the quarantine directory, the one point between inspection and check that a test can reach). Result:Disturbed.ForeignKept: the same entry left untouched staysSwapped { restored: false }.ForeignReplacedreturnSwapped { restored: false }, with the foreign object still inx.What the net state contains (from #1072)
restored; a sync that fails after a rename that worked is an error after the parent was synced, and a removal becomesRemoved(NotConfirmed). It never turns into a falseDisturbedorStranded.discard().remove_unix_sync.rs,remove_unix_cleanup.rs, evidence document and CHANGELOG say what the tests pin.Limits, stated
fsyncerror on a directory cannot be provoked portably; the failing-sync cases use a smallcfg(test)seam that models the error. No crash is simulated.Quarantine::createthat reaches the cleanup is not forced end-to-end (it needs a changed process umask); the shared function is tested directly.unlinkat(name)race against a same-user actor and the link-count heuristic. They stay owner-gated before any production reclamation (H1b), see Desktop fs-data key-rotation migration is not crash-resumable (mixed-key state possible) #359.Local evidence, hosted CI and review pending
cargo fmt --check;cargo clippy --all-targets -D warningson the host and compile-only foraarch64-apple-darwin; thepinned::unit tests on Linux;pnpm run ci:prepush(canonical wrapper) afterdeps:reconcile;pr:budgetagainstmain:CAUTION,OKon the initial substantive commit; GitHub currently reports 2 commits (the second is CHANGELOG-only). Hosted Windows/macOS/Ubuntu evidence, CI, CodeQL and independent review for this head are pending; nothing from #1072 carries over.Part of #359.
Summary by Sourcery
Harden test-support Unix quarantine removal by verifying foreign-entry identity and quarantine durability before reporting outcomes.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Summary by cubic
Hardens the Unix quarantine-based file removal against a same-user actor in the quarantine directory, closing the two open findings on #1072 and two more found on the first commit of this PR. Test-support only, with no caller, no new
unsafe, and no new package.Removed(Confirmed)only when both the parent and that directory synced, and a failed sync is an error, neverDisturbedorStranded.Disturbed, notSwapped { restored: false }. The identity is a stat identity without a held handle since opening such entries has effects; a reused identity is not noticed, and this limit is stated in the docs rather than covered by a Linux-only mechanism.Disturbedis returned because an outcome that says where an entry is needs a proof, which is why theStrandedcase is proved separately without permissions.remove_if_namedremoves the quarantine directory only when the name still leads to the directory that was created, and the changelog no longer overstates the name-based cleanup: the identity check narrows thermdirrace and does not close it.remove_unix_sync.rsandremove_unix_cleanup.rspin each case, with the failing-sync cases using acfg(test)seam since anfsyncerror on a directory cannot be provoked portably.Written for commit 1602d79. Summary will update on new commits.
CodeAnt-AI Description
Harden test-support Unix file removal against changes to its temporary holding area
What Changed
Impact
✅ Unix removals confirmed only after both directories are synced✅ Replaced held entries detected as tampering✅ Replacement directories left untouched💡 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.
Summary by CodeRabbit
Security
Reliability