fix(core): complete Unix quarantine hardening and foreign-entry identity proof (#445)
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 on main (45ce3d0c): the substantive successor commit 8f8c9883 carries the reviewed net tree of #1072 (head ae96312f) plus the fixes for those two findings, and the second commit 99336659 updates 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-support only: the primitives exist only with the test-only feature and have no caller. No new unsafe, no new package. Slice H1a-2, part of #359 (Linear QNB-168).
The two findings open on #1072, and how they are closed here
- CHANGELOG overstates the cleanup (Codex P2).
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. - Foreign entry carried no identity (Codex P2).
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.- Control
ForeignKept: the same entry left untouched staysSwapped { restored: false }. - Mutation-checked: passing no identity makes
ForeignReplacedreturnSwapped { restored: false }, with the foreign object still inx.
What the net state contains (from #1072)
- The quarantine directory is synced after the rename was tried, in every path that moves an entry through it, including an entry an actor removed again. The rename alone decides
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. - A failed setup is cleaned up only after an identity check, shared with
discard(). - Tests:
remove_unix_sync.rs,remove_unix_cleanup.rs, evidence document and CHANGELOG say what the tests pin.
Limits, stated
- An
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. - The call in
Quarantine::createthat reaches the cleanup is not forced end-to-end (it needs a changed process umask); the shared function is tested directly. - Unchanged and not accepted for production: the
unlinkat(name)race against a same-user actor and the link-count heuristic. They stay owner-gated before any production reclamation (H1b), see #359.
Local evidence, hosted CI and review pending
cargo fmt --check; cargo clippy --all-targets -D warnings on the host and compile-only for aarch64-apple-darwin; the pinned:: unit tests on Linux; pnpm run ci:prepush (canonical wrapper) after deps:reconcile; pr:budget against main: CAUTION, OK on 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:
- Detect tampering or unprovable state when entries in the Unix quarantine directory are replaced, removed, or become inaccessible after inspection.
- Prevent cleanup from removing a different quarantine directory that has taken over the original name.
Enhancements:
- Require synchronization of both the parent and quarantine directories before confirming Unix removals, while reporting failed quarantine syncs conservatively.
- Record identities for foreign quarantine entries so retained-entry outcomes are only reported when their location can be verified.
- Fail closed when quarantine directories cannot be inspected and preserve failure or tampering outcomes without masking them with sync errors.
Documentation:
- Update the Unix removal durability and race-handling evidence and changelog to document the strengthened guarantees and remaining limitations.
Tests:
- Add Unix tests covering quarantine synchronization, failed syncs, foreign-entry replacement, cleanup identity checks, inaccessible directories, and tampering scenarios.
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.
- The quarantine directory an entry moved through is now synced and counts toward durability: a removal is
Removed(Confirmed)only when both the parent and that directory synced, and a failed sync is an error, neverDisturbedorStranded. - A foreign entry (link, directory, FIFO, inaccessible file) carries the identity it was seen with, so an actor that replaces it after inspection and blocks the put-back gets
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. - A look at a retained entry that fails (a search permission lost after the move) no longer counts as "the entry is there":
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.
CodeAnt-AI Description
Harden test-support Unix file removal against changes to its temporary holding area
What Changed
- Unix removals sync the temporary holding area as well as the parent directory. If syncing fails, a removal is marked unconfirmed; failures after an entry is put back, kept aside, or found missing are reported instead of treated as successful outcomes.
- If an entry held aside is replaced before it can be restored, removal reports tampering rather than claiming the entry is still safely retained.
- Cleanup leaves a replacement directory untouched when its name no longer points to the directory created for the removal.
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:
@codeant-ai ask: Your question here
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
@codeant-ai ask: Can you suggest a safer alternative to storing this secret?
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:
@codeant-ai: Your feedback here
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
@codeant-ai: Do not flag unused imports.
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
@codeant-ai: review
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
- Unix file removal checks quarantine-directory and entry identities before cleanup, and refuses to proceed if the quarantine directory cannot be opened.
- Cleanup leaves unrelated or nonempty quarantine contents untouched.
-
Reliability
- Removal syncs the relevant directories; failed syncs can result in an unconfirmed outcome rather than confirmed removal.
- Recovery outcomes distinguish entries that are restored, retained, missing, or disturbed when their location cannot be verified.