fix: verify confined file identities by lzehrung · Pull Request #263 · lzehrung/codegraph

lzehrung · 2026-08-16T07:18:22Z

Summary

Bind confined reads to the opened descriptor identity.
Reject leaf, parent-directory, and alias-target swaps across the open-verify boundary.
Use the Windows birth-time fallback when lstat cannot provide a device identifier.

Verification

Focused security regressions and npm run check passed.
Final security review approved the TOCTOU defenses and documented hard-link provenance limit.

Copilot

Pull request overview

This PR hardens project-root file confinement against TOCTOU races by binding reads to a verified descriptor identity, and expands test coverage to exercise symlink/hard-link/junction swap scenarios and Windows identity fallbacks.

Changes:

Introduces openConfinedReadableFile(...) to open a file only after confinement, then verify the opened descriptor (fstat) matches pre-open identities.
Updates agent file-view and rename-preview flows to read from the verified FileHandle instead of re-resolving paths.
Adds focused TOCTOU regression tests (leaf swap, parent swap, hard-link replacement, alias swap, and lstat dev fallback) and updates existing expectations around metadata-only access.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
tests/file-view.test.ts	Adds TOCTOU regression tests and updates assertions to reflect descriptor-based metadata checks.
src/util/confinedFile.ts	Implements descriptor-bound confined opens with post-open identity verification and a test hook seam.
src/agent/renamePreview.ts	Switches rename preview file loading to the confined descriptor open path and adjusts error classification.
src/agent/fileView.ts	Switches file view to read via confined descriptors and ensures sensitive redaction remains metadata-only.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

      const message = errorMessage(error);
      let reason: RenameUnsafeSite["reason"] = "unresolved_reference";
-      if (/outside project root/i.test(message)) reason = "outside_root";
+      if (/outside project root|changed between verification and open/i.test(message)) reason = "outside_root";


Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/util/confinedFile.ts:183

rewriteNoFollowOpenError rewrites EINVAL into a TOCTOU/race message unconditionally. EINVAL can also indicate an invalid path/flag combination on platforms where O_NOFOLLOW is unavailable/unused, which makes the thrown error misleading and harder to debug. Consider only rewriting EINVAL when O_NOFOLLOW is actually supported/enabled (keep rewriting ELOOP/EMLINK as today).

  if (
    error instanceof Error &&
    "code" in error &&
    (error.code === "ELOOP" || error.code === "EMLINK" || error.code === "EINVAL")
  ) {

Open the confined path, verify the descriptor (POSIX O_NOFOLLOW; win32 ino identity), and read only through that handle so a symlink swap after realpath cannot exfiltrate outside-root content.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (2)

tests/file-view.test.ts:234

This comment claims the test hook runs "before lstat/open", but openConfinedReadableFile invokes the hook after resolveReadableFile has already completed lstat on the resolved realPath. Updating the comment avoids confusion about what race is being simulated.

    // Deterministic interleaving: setAfterConfinedPathVerifiedForTests runs after realpath
    // confinement succeeds and before lstat/open, swapping the verified path for an outside symlink.

src/util/confinedFile.ts:169

The error text in assertRegularFileStat hard-codes "File view target...", but this utility is now also used by rename preview via openConfinedReadableFile; a non-file input will surface a misleading "File view" message in rename preview provenance.

function assertRegularFileStat(stat: Stats, filePath: string): void {
  if (stat.isFile()) return;
  throw new Error(`File view target is not a file: ${normalizePath(filePath)}`);
}

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/util/confinedFile.ts:163

These Stats fields are JavaScript numbers, but inode identifiers are 64-bit on supported filesystems. Above Number.MAX_SAFE_INTEGER, distinct inode values can round to the same number; with the same device, this check then accepts a swapped file and reads it. Capture and compare BigIntStats ({ bigint: true }) at every pre/post-open stat boundary, converting only non-identity fields such as the returned size after validation.

function sameFileIdentity(preStat: Stats, postStat: Stats): boolean {
  if (preStat.ino !== postStat.ino) return false;
  if (preStat.dev !== 0 && postStat.dev !== 0) return preStat.dev === postStat.dev;
  return preStat.birthtimeMs === postStat.birthtimeMs;

+  realPath: string,
+  expectedStats: readonly Stats[],
+): Promise<{ handle: FileHandle; size: number }> {
+  const openFlags = fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0);


- sameFileIdentity now compares BigIntStats (ino/dev/birthtimeMs) instead of Number-backed Stats, so inode values above Number.MAX_SAFE_INTEGER can no longer collide and let a swapped file pass identity verification. - openVerifiedRegularFile opens with O_NONBLOCK in addition to O_NOFOLLOW so a verified regular file swapped for a FIFO before the open cannot hang the caller; POSIX ignores O_NONBLOCK for regular files, so normal reads are unaffected. - Add a POSIX-only regression test that swaps a verified file for a FIFO via the test hook and asserts the open rejects instead of hanging.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

tests/file-view.test.ts:374

This regression never reaches the birth-time fallback it claims to test. The mock ignores the { bigint: true } call and returns numeric Stats, so its numeric ino is unequal to the descriptor's bigint ino; the assertion therefore fails at the inode check even if the birth-time fallback is removed. Return BigIntStats and use bigint overrides so the matching inode reaches the intended fallback comparison.

      const stat = await originalLstat(candidate);
      if (path.resolve(String(candidate)) !== path.resolve(victimPath)) return stat;
      return Object.assign(Object.create(stat), {
        birthtimeMs: stat.birthtimeMs + 1,
        dev: 0,

The mock intercepted fs.lstat but called the original without forwarding the { bigint: true } options, so it returned Number-backed Stats while production now always requests BigIntStats. The mismatched numeric ino never matched the descriptor's bigint ino, so the test failed at the inode check before reaching the intended birth-time fallback comparison. Forward options through to the real lstat and use bigint literals for the injected overrides.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

tests/file-view.test.ts:361

This exercises only the rejecting half of the new zero-device fallback. The Windows PR job does not run tests/file-view.test.ts, so an implementation that rejects every lstat.dev === 0n case would still pass CI. Add a mocked zero-device case with an unchanged birth time that successfully reads the file.

  it("rejects a mismatched fallback identity when lstat does not expose a device", async () => {

tests/file-view.test.ts:479

Spying on FileHandle.read does not detect content consumed through FileHandle.readFile, while the fs.readFile spy only covers the path-based API. Consequently this metadata-only regression can pass even if the redacted branch reads the entire descriptor. Spy on and assert both handle methods.

    const handleReadSpy = vi.spyOn(Object.getPrototypeOf(probe), "read");

+  if (preStat.dev !== 0n && postStat.dev !== 0n) return preStat.dev === postStat.dev;
+  return preStat.birthtimeMs === postStat.birthtimeMs;


…d spots - sameFileIdentity now always requires nanosecond-precision birthtimeNs equality in addition to ino, not just as a fallback when dev is unavailable. Without an open descriptor pinning identity across the pre-open checks, an unlink+recreate on the same filesystem can reuse the same inode; dev+ino equality alone cannot prove it is the same file. - Split the zero-device fallback test into a reject case (mismatched birthtimeNs) and a new accept case (matched birthtimeNs) so an implementation that rejects every dev-unavailable read would fail CI. - Spy on FileHandle.readFile in the sensitive-symlink metadata-only test, not just FileHandle.read and fs.readFile, so a future redacted-branch regression that reads via handle.readFile() would be caught.

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

lzehrung requested a lite review from Copilot August 16, 2026 14:56

Copilot started reviewing on behalf of lzehrung August 16, 2026 14:56 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

lzehrung requested a lite review from Copilot August 16, 2026 16:12

Copilot started reviewing on behalf of lzehrung August 16, 2026 16:13 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

lzehrung added 4 commits August 16, 2026 13:13

fix: close confined-file TOCTOU with open-then-verify

a4db192

Open the confined path, verify the descriptor (POSIX O_NOFOLLOW; win32 ino identity), and read only through that handle so a symlink swap after realpath cannot exfiltrate outside-root content.

fix: bind confined reads to verified identity

bf7b0b4

fix: verify all confined read identities

f5ee839

fix: classify rename file races correctly

d9ea174

lzehrung force-pushed the fix/confined-file-open-verify branch from cac7d2a to d9ea174 Compare August 16, 2026 17:14

lzehrung requested a lite review from Copilot August 16, 2026 20:21

Copilot started reviewing on behalf of lzehrung August 16, 2026 20:21 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

fix: generalize confined-file error message and TOCTOU test comment

91df095

lzehrung marked this pull request as ready for review August 16, 2026 20:31

lzehrung requested a balanced review from Copilot August 16, 2026 21:12

Copilot started reviewing on behalf of lzehrung August 16, 2026 21:12 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

Comment thread src/util/confinedFile.ts Outdated

realPath: string,

expectedStats: readonly Stats[],

): Promise<{ handle: FileHandle; size: number }> {

const openFlags = fsConstants.O_RDONLY | (fsConstants.O_NOFOLLOW ?? 0);

lzehrung requested a balanced review from Copilot August 16, 2026 21:43

Copilot started reviewing on behalf of lzehrung August 16, 2026 21:44 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

lzehrung requested a balanced review from Copilot August 16, 2026 21:57

Copilot started reviewing on behalf of lzehrung August 16, 2026 21:58 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

Comment thread src/util/confinedFile.ts Outdated

Comment on lines +166 to +167

if (preStat.dev !== 0n && postStat.dev !== 0n) return preStat.dev === postStat.dev;

return preStat.birthtimeMs === postStat.birthtimeMs;

lzehrung requested a balanced review from Copilot August 16, 2026 22:09

Copilot started reviewing on behalf of lzehrung August 16, 2026 22:09 View session

Copilot AI reviewed Aug 16, 2026

View reviewed changes

lzehrung merged commit 2012780 into main Aug 16, 2026
3 checks passed

Navigation Menu

fix: verify confined file identities - #263

fix: verify confined file identities#263
lzehrung merged 8 commits into
mainfrom
fix/confined-file-open-verify

lzehrung commented Aug 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if (preStat.dev !== 0n && postStat.dev !== 0n) return preStat.dev === postStat.dev;
		return preStat.birthtimeMs === postStat.birthtimeMs;

Conversation

lzehrung commented Aug 16, 2026

Summary

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants