fix(core): complete Unix quarantine hardening and foreign-entry identity proof (#445)

Merged
QN
3d ago
+931-83

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

  1. CHANGELOG overstates the cleanup (Codex P2). 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 entry now says the check narrows the race and does not close it (rmdir takes 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.
  2. Foreign entry carried no identity (Codex P2). Inside::Foreign was passed as swapped(None), so holds(None) accepted any object at x. 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, scene ForeignReplaced: 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 stays Swapped { restored: false }.
    • Mutation-checked: passing no identity makes ForeignReplaced return Swapped { restored: false }, with the foreign object still in x.

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 becomes Removed(NotConfirmed). It never turns into a false Disturbed or Stranded.
  • 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 fsync error on a directory cannot be provoked portably; the failing-sync cases use a small cfg(test) seam that models the error. No crash is simulated.
  • The call in Quarantine::create that 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, never Disturbed or Stranded.
  • 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, not Swapped { 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": Disturbed is returned because an outcome that says where an entry is needs a proof, which is why the Stranded case is proved separately without permissions.
  • remove_if_named removes 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 the rmdir race and does not close it.
  • remove_unix_sync.rs and remove_unix_cleanup.rs pin each case, with the failing-sync cases using a cfg(test) seam since an fsync error 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.