feat(policy): add ConfinedRoot filesystem confinement primitive - #1415
feat(policy): add ConfinedRoot filesystem confinement primitive#1415khaliqgant wants to merge 2 commits into
Conversation
Binds a write to a directory it cannot escape. Policy deciding "allowed" is not a filesystem guarantee: the path can be a symlink, a hardlink to something outside the root, or replaced a microsecond after the check. Two contracts, deliberately separate: - a security refusal makes NO observable filesystem change - an authorized write is atomic — never partial, never a zero-length window Resolution is descriptor-relative, rooted at a directory descriptor captured once. Each component is opened relative to the previous one, so a component swapped on disk mid-operation cannot redirect the write. On Linux this is true openat semantics via /proc/self/fd; elsewhere it degrades to pinned-path, which detects rather than prevents. The mode is reported on every write so callers assert the guarantee they actually got instead of assuming the stronger one. Originated in the Agent Relay x Ratify design-partner spike, where four distinct defects were found in this code by adversarial testing — three by us, one by Identities AI. Every one was platform- or timing-dependent, and none would have been caught by asserting on the return value alone: - open(fifo, O_WRONLY) blocks forever waiting for a reader - O_TRUNC destroys a hardlinked file before the link check can refuse - a freed inode is recycled, defeating a (dev, ino) identity comparison - a refusal deleted the pre-existing file it was protecting The suite that found them ships alongside: 21 tests including concurrent-swap stress and positive controls, so a "fix" that refuses everything cannot pass. Nothing consumes this yet. Placement in @agent-relay/policy is the least-bad home for a safety primitive and is the reviewer's call to confirm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughAdds ChangesFilesystem confinement
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ConfinedRoot
participant TargetValidation
participant TemporarySiblingFile
participant AtomicRename
Caller->>ConfinedRoot: writeFile(requestPath, contents)
ConfinedRoot->>TargetValidation: resolve and validate path
TargetValidation-->>ConfinedRoot: approve or return ConfinementError
ConfinedRoot->>TemporarySiblingFile: create and write contents
TemporarySiblingFile->>AtomicRename: replace target by rename
AtomicRename-->>Caller: return ConfinedWrite
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (6)
packages/policy/src/fs-confine.ts (3)
396-399: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the temp name unique per write.
The temp name depends only on the target name and the pid. Leftover debris from an earlier crash makes the
O_EXCLopen fail withEEXIST. That error escapes as a rawError, not aConfinementError, and the target then stays unwritable until the debris is removed by hand. A counter or random suffix removes the wedge.♻️ Proposed fix for temp-name uniqueness
- const tempName = `.${finalName}.c5tmp-${process.pid}`; + const tempName = `.${finalName}.c5tmp-${process.pid}-${randomBytes(6).toString('hex')}`;Add the import:
+import { randomBytes } from 'node:crypto';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/policy/src/fs-confine.ts` around lines 396 - 399, Update the private commit method to generate a unique tempName for every write, using a per-write counter or random suffix rather than only finalName and process.pid. Keep the path confinement through this.at unchanged, and ensure repeated writes remain usable when stale temp files exist.
407-424: 🩺 Stability & Availability | 🔵 TrivialConsider
fsyncbefore the rename for crash consistency.The write loop closes the temp descriptor and renames it over the target. Without
fsyncSyncon the temp descriptor, a crash or power loss after the rename can leave the target with the new name and unwritten content. The C2 wording promises no partial content. If durability across host crashes is in scope for this primitive, sync the temp file before the rename, and optionally sync the parent directory descriptor after it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/policy/src/fs-confine.ts` around lines 407 - 424, Update the atomic write flow around tempFd, closeSync, and renameSync to call fsyncSync on the temporary file descriptor after the complete write loop and before renaming, preserving the no-partial-content guarantee across crashes. If directory durability is required by this primitive, also sync the parent directory after renameSync.
158-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
afterValidatehook signature with what is passed.
writeConfinedalways callshooks.afterValidate?.(-1). The probe descriptor is closed at line 349, so no valid descriptor exists at that point. The declared parameterfd: numberand its doc comment describe a descriptor that callers never receive. Change the hook to take no argument, or document the sentinel.♻️ Proposed signature change
- /** Fires after the descriptor has passed validation, before content is written. */ - afterValidate?: (fd: number) => void; + /** Fires after target validation completes, before content is written. */ + afterValidate?: () => void;- hooks.afterValidate?.(-1); + hooks.afterValidate?.();Also applies to: 353-353
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/policy/src/fs-confine.ts` around lines 158 - 159, Update the afterValidate hook declaration and its documentation to reflect that writeConfined invokes it without a valid descriptor, and change the invocation near the post-validation flow to omit the -1 sentinel argument. Preserve the hook’s existing timing and optional behavior.packages/policy/src/fs-confine.test.ts (3)
310-318: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the refusal explicitly in this atomicity test.
The
catchblock swallows every error. The test then passes even if the implementation throws an unexpected non-ConfinementError, for example a rawEEXISTfrom the temp file. Assert the thrown value and its code, asexpectRefusaldoes elsewhere in this file.💚 Proposed fix to assert the refusal
- try { - cr.writeFile('keep.md', 'REPLACEMENT', { - afterValidate: () => linkSync(target, join(s.outside, 'keep-link.md')), - }); - } catch { - /* expected */ - } + let thrown: unknown; + try { + cr.writeFile('keep.md', 'REPLACEMENT', { + afterValidate: () => linkSync(target, join(s.outside, 'keep-link.md')), + }); + } catch (err) { + thrown = err; + } + expect(thrown).toBeInstanceOf(ConfinementError); + expect((thrown as ConfinementError).code).toBe('hardlink');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/policy/src/fs-confine.test.ts` around lines 310 - 318, Update the try/catch in the atomicity test around cr.writeFile to capture the thrown error and explicitly validate it with the existing expectRefusal helper, including the expected refusal code. Do not swallow unexpected errors, while preserving the existing assertion that the target remains ORIGINAL.
140-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the remaining request-path refusal codes.
validateRequestPathproducesempty_path,empty_segment,nul_byte, andbackslash, andcommitproducesshort_write. The suite asserts none of these. The cheap ones are the request-path codes, because they need no filesystem setup. Add them so a change to the validation order or the code names fails a test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/policy/src/fs-confine.test.ts` around lines 140 - 189, Extend the refusal tests near the existing “ConfinedRoot — refusals mutate nothing (C1)” cases to cover validateRequestPath’s empty_path, empty_segment, nul_byte, and backslash refusal codes using filesystem-independent writeFile inputs. Assert each through expectRefusal with the exact code, preserving the existing setup and test style.
181-185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the skip instead of returning silently.
If
mkfifois absent, the test returns early and reports a pass. A reader cannot tell that the FIFO case never ran. Use the Vitest test context to skip, so the result shows as skipped.♻️ Proposed fix for an explicit skip
- it('refuses a fifo rather than blocking on it forever', () => { + it('refuses a fifo rather than blocking on it forever', (ctx) => { ... try { execFileSync('mkfifo', [join(s.root, 'pipe')]); } catch { - return; // platform without mkfifo + ctx.skip(); // platform without mkfifo + return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/policy/src/fs-confine.test.ts` around lines 181 - 185, Update the mkfifo availability handling in the affected test to use the Vitest test context’s skip mechanism instead of returning silently from the catch block. Ensure the FIFO test is reported as skipped when mkfifo is unavailable, while preserving normal execution when the command exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/policy/src/fs-confine.ts`:
- Around line 206-213: Update the root-anchor class around close() and at() to
track whether the anchor has been closed, make close() idempotently mark that
state after releasing rootFd, and reject subsequent write operations before
constructing descriptor-relative paths. Ensure calls after close fail closed
rather than using a reused descriptor.
- Around line 396-405: Update writeConfined and commit to preserve the validated
target permissions: capture the existing mode from the validation stat using
st.mode & 0o7777, pass it into commit, and create the temporary file with that
mode instead of always using 0o644. Retain 0o644 only when no existing target
mode is available.
---
Nitpick comments:
In `@packages/policy/src/fs-confine.test.ts`:
- Around line 310-318: Update the try/catch in the atomicity test around
cr.writeFile to capture the thrown error and explicitly validate it with the
existing expectRefusal helper, including the expected refusal code. Do not
swallow unexpected errors, while preserving the existing assertion that the
target remains ORIGINAL.
- Around line 140-189: Extend the refusal tests near the existing “ConfinedRoot
— refusals mutate nothing (C1)” cases to cover validateRequestPath’s empty_path,
empty_segment, nul_byte, and backslash refusal codes using
filesystem-independent writeFile inputs. Assert each through expectRefusal with
the exact code, preserving the existing setup and test style.
- Around line 181-185: Update the mkfifo availability handling in the affected
test to use the Vitest test context’s skip mechanism instead of returning
silently from the catch block. Ensure the FIFO test is reported as skipped when
mkfifo is unavailable, while preserving normal execution when the command
exists.
In `@packages/policy/src/fs-confine.ts`:
- Around line 396-399: Update the private commit method to generate a unique
tempName for every write, using a per-write counter or random suffix rather than
only finalName and process.pid. Keep the path confinement through this.at
unchanged, and ensure repeated writes remain usable when stale temp files exist.
- Around line 407-424: Update the atomic write flow around tempFd, closeSync,
and renameSync to call fsyncSync on the temporary file descriptor after the
complete write loop and before renaming, preserving the no-partial-content
guarantee across crashes. If directory durability is required by this primitive,
also sync the parent directory after renameSync.
- Around line 158-159: Update the afterValidate hook declaration and its
documentation to reflect that writeConfined invokes it without a valid
descriptor, and change the invocation near the post-validation flow to omit the
-1 sentinel argument. Preserve the hook’s existing timing and optional behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 949edf4a-79c8-4830-b3b5-8f7ed44d10ed
📒 Files selected for processing (4)
CHANGELOG.mdpackages/policy/src/fs-confine.test.tspackages/policy/src/fs-confine.tspackages/policy/src/index.ts
| /** Release the root anchor. The object is unusable afterwards. */ | ||
| close(): void { | ||
| try { | ||
| closeSync(this.rootFd); | ||
| } catch { | ||
| /* already closed */ | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Guard against use after close().
close() releases rootFd but leaves the object callable. The kernel can then assign that descriptor number to an unrelated file. In descriptor-relative mode at() builds /proc/self/fd/${dir.fd}/${name}, so a later writeFile call can resolve under a directory that is no longer the root. The confinement guarantee is lost silently instead of failing closed.
Track the closed state and refuse further writes.
🔒️ Proposed fix to fail closed after `close()`
readonly resolutionMode: ResolutionMode;
+ private closed = false;
...
close(): void {
+ this.closed = true;
try {
closeSync(this.rootFd);
} catch {
/* already closed */
}
} writeFile(requestPath: string, contents: string, hooks: WriteHooks = {}): ConfinedWrite {
+ if (this.closed) fail('root_unresolvable', 'confinement root is closed');
const segments = this.validateRequestPath(requestPath);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/policy/src/fs-confine.ts` around lines 206 - 213, Update the
root-anchor class around close() and at() to track whether the anchor has been
closed, make close() idempotently mark that state after releasing rootFd, and
reject subsequent write operations before constructing descriptor-relative
paths. Ensure calls after close fail closed rather than using a reused
descriptor.
| private commit(dir: PinnedDir, finalName: string, contents: string): number { | ||
| const tempName = `.${finalName}.c5tmp-${process.pid}`; | ||
| const tempPath = this.at(dir, tempName); | ||
| const targetPath = this.at(dir, finalName); | ||
|
|
||
| const tempFd = openSync( | ||
| tempPath, | ||
| constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL | constants.O_NOFOLLOW, | ||
| 0o644 | ||
| ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Preserve the existing target mode instead of forcing 0o644.
commit always creates the temp file with mode 0o644. renameSync replaces the target inode, so the original permissions are discarded. A target created as 0o600 becomes world-readable after a write. This weakens the file's confidentiality and contradicts the C2 wording that the target is "fully replaced or left exactly as it was".
Capture st.mode from the validation probe and apply it to the temp file before the rename.
🔒️ Proposed fix to carry the existing mode
- private commit(dir: PinnedDir, finalName: string, contents: string): number {
+ private commit(dir: PinnedDir, finalName: string, contents: string, mode = 0o644): number {
const tempName = `.${finalName}.c5tmp-${process.pid}`;
const tempPath = this.at(dir, tempName);
const targetPath = this.at(dir, finalName);
const tempFd = openSync(
tempPath,
constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL | constants.O_NOFOLLOW,
- 0o644
+ mode
);In writeConfined, record the mode observed on the validated descriptor and pass it to commit:
// inside the `existing !== null` probe block
existingMode = st.mode & 0o7777;
// ...
bytesWritten: this.commit(dir, finalName, contents, existingMode ?? 0o644),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/policy/src/fs-confine.ts` around lines 396 - 405, Update
writeConfined and commit to preserve the validated target permissions: capture
the existing mode from the validation stat using st.mode & 0o7777, pass it into
commit, and create the temporary file with that mode instead of always using
0o644. Retain 0o644 only when no existing target mode is available.
![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: 34d87dfcee
ℹ️ 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".
| const tempFd = openSync( | ||
| tempPath, | ||
| constants.O_WRONLY | constants.O_CREAT | constants.O_EXCL | constants.O_NOFOLLOW, | ||
| 0o644, | ||
| ); |
There was a problem hiding this comment.
When a same-UID concurrent mutator hardlinks the predictable temporary path outside the root after this open, subsequent writeSync calls write the payload through both names. I reproduced this on Linux in descriptor-relative mode: the call reported success while the outside link received the complete payload and the committed target had nlink === 2. Because target hardlink validation occurs before this temporary inode exists, the advertised confinement is bypassed; the commit needs an anonymous/native temporary-file mechanism that cannot acquire an outside name while being written.
Useful? React with 👍 / 👎.
|
|
||
| ### Added | ||
|
|
||
| - `@agent-relay/policy` exports `ConfinedRoot`: filesystem confinement that binds a write to a directory it cannot escape. Refuses path traversal, symlinked components, hardlinks to files outside the root, non-regular targets, and components swapped mid-operation; writes atomically so an interrupted write never leaves a partial or empty file. A security refusal makes no observable filesystem change. Nothing consumes it yet — it is available for callers that execute agent-directed writes. |
There was a problem hiding this comment.
This is the first pending user-visible change after 11.3.1, and adding a new public package export is a SemVer-minor addition, but the commit leaves the heading as plain [Unreleased]. Change it to [Unreleased - Minor] so the pending release level follows the repository's required monotonic release convention.
AGENTS.md reference: AGENTS.md:L36-L43
Useful? React with 👍 / 👎.
| if (existing === null) { | ||
| mkdirSync(childPath); // one component, no recursion |
There was a problem hiding this comment.
When the requested path contains a missing parent and a concurrent mutator plants an unsafe final component afterward, these mkdirSync calls remain even though writeFile throws a ConfinementError. For example, writing new/target while beforeOpen plants an outside symlink at the target refuses with symlink_target but leaves the newly created new directory, contradicting the documented and changelog-visible guarantee that a security refusal makes no observable filesystem change.
Useful? React with 👍 / 👎.
|
Closing — wrong home, and the reasoning is worth recording.
The real home is relayfile. Relayfile and relayauth already implement the same two-gate split this primitive was built for: relayauth decides whether a VFS path is authorized ( Re-targeting there. The matrix is the transferable asset here — it found four defects that all passed code review. |
1 participant
What
ConfinedRootbinds a write to a directory it cannot escape. Policy deciding allowed is not a filesystem guarantee — the path can be a symlink, a hardlink to something outside the root, or replaced a microsecond after the check.Two contracts, deliberately separate:
Resolution is descriptor-relative, rooted at a directory descriptor captured once. Each component is opened relative to the previous one, so a component swapped on disk mid-operation cannot redirect the write. On Linux that is true
openatsemantics via/proc/self/fd; elsewhere it degrades topinned-path, which detects rather than prevents.resolutionModeis reported on every write so callers assert the guarantee they actually got rather than assuming the stronger one — a nativeopenat2binding withRESOLVE_BENEATHwould be strictly better and is noted in the module.Why the tests are the interesting part
This originated in the Agent Relay × Ratify design-partner spike, where four distinct defects were found in this code by adversarial testing — three by us, one by Identities AI. Every one was platform- or timing-dependent, and none would have been caught by asserting on the return value alone:
open(fifo, O_WRONLY)blocks foreverO_TRUNCdestroys a hardlinked fileopen, before any check can run(dev, ino)comparison — passed on macOS, real escape on Linuxcreatedwas inferred from a pre-swap stat; cleanup ran on a recomputed pathThe suite that found them ships alongside: 21 tests, including a concurrent parent-swap stress loop and positive controls, so a "fix" that refuses everything cannot pass. A confinement layer that blocks legitimate writes is as broken as one that permits escapes.
Verification
packages/policy— 21/21npx turbo build— cleannpx turbo lint— cleandescriptor-relative) and macOS (pinned-path) in the originating spike, on overlayfs, tmpfs, and APFS.Review notes
Nothing consumes this yet. It is a primitive, added so the next caller that executes agent-directed writes has something correct to reach for rather than writing a fourth lexical prefix check.
Placement is the reviewer's call.
@agent-relay/policyis the authorization/safety family, which makes it the least-bad home — policy decides, this enforces. If you would rather it were its own package, or lived closer to whatever will consume it first, say so and I will move it.Known limitation, stated in the module: on non-Linux platforms the guarantee is weaker. If untrusted processes can mutate the tree during a write, the boundary needs to be an OS-level one (mount namespace, jail) rather than this layer.
🤖 Generated with Claude Code