Skip to content

[codex] centralize filesystem boundary guards#8

Merged
steipete merged 21 commits into
mainfrom
codex/centralize-fs-boundary-guards
May 6, 2026
Merged

[codex] centralize filesystem boundary guards#8
steipete merged 21 commits into
mainfrom
codex/centralize-fs-boundary-guards

Conversation

@steipete
Copy link
Copy Markdown
Contributor

@steipete steipete commented May 6, 2026

Summary

  • Fixes the filesystem race findings with targeted regression tests across root fallbacks, archive merges, store/private writes, JSON fallbacks, temp helpers, durable queue ids, and trash moves.
  • Centralizes the repeated safety patterns into shared safe path segment validation, directory guards, guarded mutation wrappers, and pinned temp-workspace reads.
  • Adds a static pnpm lint:fs-boundary guard so the known raw fallback patterns do not drift back in.

Validation

  • pnpm check
  • pnpm test test/findings-regression.test.ts

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 6, 2026

Codex review: needs real behavior proof before merge.

Summary
The PR centralizes filesystem boundary guard primitives and adds hardening, tests, docs, changelog notes, and a static lint guard for several fs-safe fallback paths.

Reproducibility: yes. from source inspection: force pinned Python mode off, write to a nested missing parent with mkdir enabled, and swap the newly created parent to a symlink after fallback mkdir but before the path-based open. This read-only review did not execute the PR branch.

Real behavior proof
Needs real behavior proof before merge: The PR only lists test commands and CI; please add terminal output, logs, a recording, screenshot, or linked artifact showing after-fix behavior in a real setup, with private paths, endpoints, keys, and similar data redacted. Updating the PR body should trigger a fresh ClawSweeper review; if it does not, ask a maintainer to comment @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, ask a maintainer to comment @clawsweeper re-review.

Next step before merge
The PR is a draft security hardening change with a concrete blocker and missing real behavior proof, so it needs contributor and maintainer follow-up rather than an automated repair lane.

Security
Needs attention: The security hardening leaves a concrete path-escape gap in the pinned-write fallback when mkdir creates a new parent.

Review findings

  • [P1] Guard the created parent before fallback writes — src/pinned-write.ts:202-205
Review details

Best possible solution:

Land the guard centralization only after the fallback code guards the actual created parent directory, adds regression coverage for that race, and includes real behavior proof from the contributor's setup.

Do we have a high-confidence way to reproduce the issue?

Yes, from source inspection: force pinned Python mode off, write to a nested missing parent with mkdir enabled, and swap the newly created parent to a symlink after fallback mkdir but before the path-based open. This read-only review did not execute the PR branch.

Is this the best way to solve the issue?

No; shared guard primitives are the right direction, but this implementation should recreate/assert a guard for parentPath after mkdir before opening, renaming, or cleaning up fallback paths.

Full review comments:

  • [P1] Guard the created parent before fallback writes — src/pinned-write.ts:202-205
    When mkdir creates a missing parent, parentGuard still refers to the nearest existing ancestor. The overwrite=false and temp-file fallback paths then open names under the newly created parent while only checking that ancestor, so a symlink swap of the new parent can redirect fallback writes outside the root; recreate and use a guard for parentPath after mkdir.
    Confidence: 0.82

Overall correctness: patch is incorrect
Overall confidence: 0.82

Security concerns:

  • [high] Fallback writes can escape a newly created parent — src/pinned-write.ts:202
    runPinnedWriteFallback records parentGuard before mkdir, so when parentPath did not exist the later fs.open(targetPath/tempPath) calls are guarded only by an ancestor. A swapped new parent symlink can redirect writes or cleanup outside the intended root.
    Confidence: 0.82

What I checked:

  • Not implemented on current main: Current main still has no lint:fs-boundary script in check and lacks the PR's added guard/lint/test files, so the exact PR change is not already present on main. (package.json:104, 55327c893031)
  • Fallback guard gap: The PR captures parentGuard from the nearest existing ancestor before mkdir, then uses that same guard for writes under parentPath; if parentPath was just created, the actual parent directory is not pinned before path-based opens or renames. (src/pinned-write.ts:202, 261ca3cbc067)
  • Real behavior proof missing: The PR body lists pnpm check and a targeted test command, and CI checks are green, but there are no comments, logs, screenshots, recordings, or linked artifacts showing after-fix behavior in a real setup. (261ca3cbc067)
  • Feature history: The central fs-safe root, pinned-write, store, archive, and trash code paths on current main trace primarily to Peter Steinberger's mainline commits, including the package release import of those files and earlier fs-safe helper work. (src/pinned-write.ts:196, cbe59d156a0d)

Likely related people:

  • steipete: Current-main history shows Peter Steinberger introduced and maintained the central fs-safe implementation paths affected by this PR, including pinned writes, root fallbacks, stores, archive staging, and trash handling. (role: introduced behavior and recent maintainer; confidence: high; commits: cbe59d156a0d, ff2e84aaeaae, e210a26af2c0; files: src/pinned-write.ts, src/root-impl.ts, src/file-store.ts)
  • jesse-merhi: Recently contributed the merged boundary-bypass test rename PR, which is adjacent to this PR's regression-test surface but not the central implementation owner. (role: adjacent test contributor; confidence: low; commits: 9cbee5d1b7d3; files: test/additional-boundary-bypass.test.ts, test/read-boundary-bypass.test.ts, test/write-boundary-bypass.test.ts)

Remaining risk / open question:

  • The PR is draft and security-sensitive, so final maintainer review is still needed after the fallback guard fix.
  • The blocker was found by source inspection rather than by executing the PR branch in this read-only review, so it should be backed by a focused regression test.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 55327c893031.

@steipete steipete marked this pull request as ready for review May 6, 2026 22:55
@steipete steipete merged commit 500243f into main May 6, 2026
5 checks passed
@steipete steipete deleted the codex/centralize-fs-boundary-guards branch May 6, 2026 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant