feat(agents): symlink-safe sandbox file access closing TOCTOU window #120

Merged
CoreRasurae merged 1 commit from feature/m1-symlink-safe-file-access into master 2026-08-09 22:06:43 +00:00
Member

Summary

Hardens FileAccessCore (the shared core behind both the built-in file_read/file_write tools and the inline-code read_file/write_file helpers, ADR-2035) against a symlink time-of-check/time-of-use race (CWE-367).

SandboxRootPolicy.resolve validated a path once via os.path.realpath, and FileAccessCore.read_window/write then opened that resolved path with a plain open(...). Because resolution and open() are two separate filesystem operations, an attacker who already has write access to a directory the path passes through could swap a path component — the final one or an intermediate parent — for a symlink after validation but before the open, which a plain open() would silently follow. The same gap existed on the unconfined path the built-in tools use, which never resolved a path internally before opening at all.

A new SymlinkSafeOpener opens the target one path component at a time from the filesystem root, with O_NOFOLLOW set on every os.open() call (O_DIRECTORY for every component but the last). Because O_NOFOLLOW only inspects the trailing component of a single open() call, walking one component per call means every component — not only the final one — is independently re-verified not to be a symlink at the exact moment it is opened. FileAccessCore.read_window/write (and the write helpers prepare_append_content/handle_insert_position) now route every open through this opener, so the fix applies identically to the built-in tools and the inline helpers, confined or not. Legitimate in-root reads and writes — including all file_write modes (w/a/insert) — are unchanged. This is enforcement-only hardening: the set of admitted/rejected paths is unchanged, so no Actor Configuration Standard (docs/index.md) revision accompanies it.

Documented as ADR-2035 D-9 (2026-08-08 revision — see the ADR's own Revision History section for the full design rationale, including why open-then-fstat re-verification was rejected in favor of the component-wise walk).

Testing

  • Behave (unit): features/symlink_safe_open.feature — deterministically simulates the exact race via a monkeypatched resolve/realpath step that swaps the target immediately after validation, for both final- and intermediate-component swaps, across the read path and every write mode (w/a/insert), plus argument-validation and legitimate-access regression scenarios.
  • Robot (integration):
    • robot/symlink_safe_open.robot — races a real, genuinely concurrent background thread against FileAccessCore directly (confine=True) for reads and every write mode, no mocks.
    • robot/inline_code_file_helpers.robot — races the same real thread against the public read_file inline helper end to end through a real ToolAgent.
    • (A symmetric "write" race against the unconfined write_file helper was considered and dropped: safe_mode: false has no containment boundary at all by design (D-5), so following an already-present symlink there is correct, expected behavior, not a security gap — the write side of D-9 is instead exercised against the genuinely-confined FileAccessCore.write(confine=True) surface, which is where a boundary actually applies.)
  • nox -s lint typecheck security_scan dead_code unit_tests coverage_report integration_tests all green. nox -s benchmark_regression -- --quick run informationally (not required for merge); the existing benchmarks/file_access_benchmark.py already exercises the affected hot paths.

Definition of Done

  • All subtasks in #105 completed and checked off.
  • ADR-2035 revised (D-9) following the project's revision-history procedure.
  • Behave + Robot tests added.
  • Coverage remains >= threshold via nox -s coverage_report.
  • nox core gates green.
  • CHANGELOG updated.

Closes #105

## Summary Hardens `FileAccessCore` (the shared core behind both the built-in `file_read`/`file_write` tools and the inline-code `read_file`/`write_file` helpers, ADR-2035) against a symlink time-of-check/time-of-use race (CWE-367). `SandboxRootPolicy.resolve` validated a path once via `os.path.realpath`, and `FileAccessCore.read_window`/`write` then opened that resolved path with a plain `open(...)`. Because resolution and `open()` are two separate filesystem operations, an attacker who already has write access to a directory the path passes through could swap a path component — the final one or an intermediate parent — for a symlink after validation but before the open, which a plain `open()` would silently follow. The same gap existed on the unconfined path the built-in tools use, which never resolved a path internally before opening at all. A new `SymlinkSafeOpener` opens the target one path component at a time from the filesystem root, with `O_NOFOLLOW` set on every `os.open()` call (`O_DIRECTORY` for every component but the last). Because `O_NOFOLLOW` only inspects the *trailing* component of a single `open()` call, walking one component per call means every component — not only the final one — is independently re-verified not to be a symlink at the exact moment it is opened. `FileAccessCore.read_window`/`write` (and the write helpers `prepare_append_content`/`handle_insert_position`) now route every open through this opener, so the fix applies identically to the built-in tools and the inline helpers, confined or not. Legitimate in-root reads and writes — including all `file_write` modes (`w`/`a`/`insert`) — are unchanged. This is enforcement-only hardening: the set of admitted/rejected paths is unchanged, so no Actor Configuration Standard (`docs/index.md`) revision accompanies it. Documented as **ADR-2035 D-9** (2026-08-08 revision — see the ADR's own Revision History section for the full design rationale, including why open-then-`fstat` re-verification was rejected in favor of the component-wise walk). ## Testing - **Behave (unit):** `features/symlink_safe_open.feature` — deterministically simulates the exact race via a monkeypatched `resolve`/`realpath` step that swaps the target immediately after validation, for both final- and intermediate-component swaps, across the read path and every write mode (`w`/`a`/`insert`), plus argument-validation and legitimate-access regression scenarios. - **Robot (integration):** - `robot/symlink_safe_open.robot` — races a real, genuinely concurrent background thread against `FileAccessCore` directly (`confine=True`) for reads and every write mode, no mocks. - `robot/inline_code_file_helpers.robot` — races the same real thread against the public `read_file` inline helper end to end through a real `ToolAgent`. - (A symmetric "write" race against the *unconfined* `write_file` helper was considered and dropped: `safe_mode: false` has no containment boundary at all by design (D-5), so following an already-present symlink there is correct, expected behavior, not a security gap — the write side of D-9 is instead exercised against the genuinely-confined `FileAccessCore.write(confine=True)` surface, which is where a boundary actually applies.) - `nox -s lint typecheck security_scan dead_code unit_tests coverage_report integration_tests` all green. `nox -s benchmark_regression -- --quick` run informationally (not required for merge); the existing `benchmarks/file_access_benchmark.py` already exercises the affected hot paths. ## Definition of Done - [x] All subtasks in #105 completed and checked off. - [x] ADR-2035 revised (D-9) following the project's revision-history procedure. - [x] Behave + Robot tests added. - [x] Coverage remains >= threshold via `nox -s coverage_report`. - [x] `nox` core gates green. - [x] CHANGELOG updated. Closes #105
CoreRasurae added this to the v2.1.0 milestone 2026-08-08 18:22:44 +00:00
hurui200320 left a comment

PR Review: !120 (Ticket #105)

Verdict: Approve

The implementation correctly closes the TOCTOU window described in ADR-2035 D-9. The component-wise O_NOFOLLOW/O_DIRECTORY walk is sound, applies to both confined and unconfined paths through the single shared FileAccessCore, and preserves the existing admission semantics. Tests are comprehensive for the file-open paths (deterministic Behave monkeypatch coverage plus real-concurrency Robot stress). Documentation (ADR revision, CHANGELOG) is in order. No critical or major issues were found.

Critical Issues

None.

Major Issues

None.

Minor Issues

  1. Missing Behave coverage for intermediate-component swap on writes.

    • File: features/symlink_safe_open.feature (around line 57)
    • Problem: The acceptance criteria require that an intermediate parent component swapped to an out-of-root symlink be refused for writes as well as reads. The implementation protects writes (the same SymlinkSafeOpener is used), but only the read path has an explicit intermediate-swap Behave scenario. Write modes are only tested with a final-component swap.
    • Recommendation: Add an intermediate-component swap Scenario Outline for FileAccessCore.write covering w, a, and insert.
  2. Racing swapper in inline integration test can crash on contention.

    • File: robot/InlineFileHelperTestLib.py (lines 115–124)
    • Problem: _start_swapper_thread does not catch OSError while toggling the target between a real file and a symlink. A race against the main thread's read_file/write_file can raise FileNotFoundError or FileExistsError, causing the swapper thread to die and weakening the test's ability to actually exercise the race window. The analogous SymlinkSafeOpenTestLib._start_swapper already wraps its toggle loop in try ... except OSError.
    • Recommendation: Wrap the toggle body in try ... except OSError: continue, matching SymlinkSafeOpenTestLib.
  3. Built-in file_read directory listing still follows symlinks without O_NOFOLLOW.

    • File: src/cleveractors/agents/tool.py (lines 846–859)
    • Problem: The directory-listing branch of _file_read_tool uses plain os.listdir/os.path.isdir/os.path.getsize. A post-validation symlink swap on the path can still leak an outside directory listing. This is pre-existing and outside ADR-2035 D-9's "file open" scope, but it leaves a residual TOCTOU gap on that surface.
    • Recommendation: Either route directory listing through the same component-wise O_NOFOLLOW|O_DIRECTORY walk (os.listdir(dir_fd)) or document it as a known residual gap.

Nits

  1. Return type of _secure_open could be more precise.

    • File: src/cleveractors/agents/file_access.py (line 331)
    • Problem: Returns Any; the value is always a text-mode file object.
    • Recommendation: Use typing.TextIO.
  2. _SYMLINK_SWAP_ERRNOS omits EMLINK on some POSIX platforms.

    • File: src/cleveractors/agents/file_access.py (line 62)
    • Problem: Some systems surface O_NOFOLLOW symlink refusal as EMLINK rather than ELOOP. Linux/Gitea CI likely uses ELOOP, so not blocking.
    • Recommendation: Consider adding errno.EMLINK if the project supports those platforms.
  3. Legitimate-write regression coverage only exercises append mode.

    • File: features/symlink_safe_open.feature (lines 79–84)
    • Problem: The single legitimate-write scenario uses mode "a". The existing inline_code_file_helpers.feature covers w/insert, but a dedicated per-mode regression scenario in this feature would make the hardening's behavior-preservation claim explicit.
    • Recommendation: Convert the scenario to a Scenario Outline for w, a, and insert.

Summary

This is a solid, well-documented defense-in-depth change. The SymlinkSafeOpener design correctly addresses CWE-367 without changing the set of admitted paths, and by routing every FileAccessCore open through it the fix applies uniformly to both the built-in tools and the inline helpers. The Behave + Robot test combination gives both deterministic proof and real concurrency stress. With the minor coverage/robustness items addressed (or explicitly documented as out of scope), this is ready to merge.

## PR Review: !120 (Ticket #105) ### Verdict: Approve The implementation correctly closes the TOCTOU window described in ADR-2035 D-9. The component-wise `O_NOFOLLOW`/`O_DIRECTORY` walk is sound, applies to both confined and unconfined paths through the single shared `FileAccessCore`, and preserves the existing admission semantics. Tests are comprehensive for the file-open paths (deterministic Behave monkeypatch coverage plus real-concurrency Robot stress). Documentation (ADR revision, CHANGELOG) is in order. No critical or major issues were found. ### Critical Issues None. ### Major Issues None. ### Minor Issues 1. **Missing Behave coverage for intermediate-component swap on writes.** - **File:** `features/symlink_safe_open.feature` (around line 57) - **Problem:** The acceptance criteria require that an intermediate parent component swapped to an out-of-root symlink be refused for writes as well as reads. The implementation protects writes (the same `SymlinkSafeOpener` is used), but only the read path has an explicit intermediate-swap Behave scenario. Write modes are only tested with a final-component swap. - **Recommendation:** Add an intermediate-component swap `Scenario Outline` for `FileAccessCore.write` covering `w`, `a`, and `insert`. 2. **Racing swapper in inline integration test can crash on contention.** - **File:** `robot/InlineFileHelperTestLib.py` (lines 115–124) - **Problem:** `_start_swapper_thread` does not catch `OSError` while toggling the target between a real file and a symlink. A race against the main thread's `read_file`/`write_file` can raise `FileNotFoundError` or `FileExistsError`, causing the swapper thread to die and weakening the test's ability to actually exercise the race window. The analogous `SymlinkSafeOpenTestLib._start_swapper` already wraps its toggle loop in `try ... except OSError`. - **Recommendation:** Wrap the toggle body in `try ... except OSError: continue`, matching `SymlinkSafeOpenTestLib`. 3. **Built-in `file_read` directory listing still follows symlinks without `O_NOFOLLOW`.** - **File:** `src/cleveractors/agents/tool.py` (lines 846–859) - **Problem:** The directory-listing branch of `_file_read_tool` uses plain `os.listdir`/`os.path.isdir`/`os.path.getsize`. A post-validation symlink swap on the path can still leak an outside directory listing. This is pre-existing and outside ADR-2035 D-9's "file open" scope, but it leaves a residual TOCTOU gap on that surface. - **Recommendation:** Either route directory listing through the same component-wise `O_NOFOLLOW|O_DIRECTORY` walk (`os.listdir(dir_fd)`) or document it as a known residual gap. ### Nits 1. **Return type of `_secure_open` could be more precise.** - **File:** `src/cleveractors/agents/file_access.py` (line 331) - **Problem:** Returns `Any`; the value is always a text-mode file object. - **Recommendation:** Use `typing.TextIO`. 2. **`_SYMLINK_SWAP_ERRNOS` omits `EMLINK` on some POSIX platforms.** - **File:** `src/cleveractors/agents/file_access.py` (line 62) - **Problem:** Some systems surface `O_NOFOLLOW` symlink refusal as `EMLINK` rather than `ELOOP`. Linux/Gitea CI likely uses `ELOOP`, so not blocking. - **Recommendation:** Consider adding `errno.EMLINK` if the project supports those platforms. 3. **Legitimate-write regression coverage only exercises append mode.** - **File:** `features/symlink_safe_open.feature` (lines 79–84) - **Problem:** The single legitimate-write scenario uses `mode "a"`. The existing `inline_code_file_helpers.feature` covers `w`/`insert`, but a dedicated per-mode regression scenario in this feature would make the hardening's behavior-preservation claim explicit. - **Recommendation:** Convert the scenario to a `Scenario Outline` for `w`, `a`, and `insert`. ### Summary This is a solid, well-documented defense-in-depth change. The `SymlinkSafeOpener` design correctly addresses CWE-367 without changing the set of admitted paths, and by routing every `FileAccessCore` open through it the fix applies uniformly to both the built-in tools and the inline helpers. The Behave + Robot test combination gives both deterministic proof and real concurrency stress. With the minor coverage/robustness items addressed (or explicitly documented as out of scope), this is ready to merge.
@ -0,0 +54,4 @@
Then the core raises ValueError mentioning "sandbox root"
And the secret outside the sandbox is never read
Scenario Outline: FileAccessCore.write refuses a final-component swap that happens inside its own resolve-then-open step for every write mode
Member

Minor: consider adding an intermediate-component swap Scenario Outline for writes, matching the read coverage above.

Minor: consider adding an intermediate-component swap Scenario Outline for writes, matching the read coverage above.
@ -0,0 +76,4 @@
Then the core raises an OSError
And the secret outside the sandbox is never read
Scenario: Legitimate in-root writes still succeed for every write mode after the hardening
Member

Nit: consider a Scenario Outline for w/a/insert to explicitly assert behavior preservation for every write mode.

Nit: consider a Scenario Outline for w/a/insert to explicitly assert behavior preservation for every write mode.
@ -80,0 +112,4 @@
target_path = self._sandbox / self._target_name
stop = threading.Event()
def swap_forever() -> None:
Member

Minor: the toggle loop should catch OSError to avoid the swapper thread crashing on contention (see SymlinkSafeOpenTestLib._start_swapper for the pattern).

Minor: the toggle loop should catch OSError to avoid the swapper thread crashing on contention (see SymlinkSafeOpenTestLib._start_swapper for the pattern).
@ -48,0 +59,4 @@
# errno values a component-wise O_NOFOLLOW walk raises when a path component
# no longer matches what it was when the caller resolved/admitted the path —
# most commonly because it was swapped for a symlink after that decision.
_SYMLINK_SWAP_ERRNOS = (errno.ELOOP, errno.ENOTDIR)
Member

Nit: some POSIX systems surface O_NOFOLLOW symlink refusal as EMLINK; consider adding it.

Nit: some POSIX systems surface O_NOFOLLOW symlink refusal as EMLINK; consider adding it.
@ -227,2 +328,4 @@
return self._root_policy
@staticmethod
def _secure_open(
Member

Nit: return type could be typing.TextIO instead of Any.

Nit: return type could be `typing.TextIO` instead of `Any`.
Member

Minor/pre-existing: the directory-listing branch still follows symlinks, leaving a residual TOCTOU gap not covered by ADR-2035 D-9.

Minor/pre-existing: the directory-listing branch still follows symlinks, leaving a residual TOCTOU gap not covered by ADR-2035 D-9.
CoreRasurae force-pushed feature/m1-symlink-safe-file-access from 4a55dd51a2
Some checks failed
CI / lint (pull_request) Failing after 37s
CI / typecheck (pull_request) Successful in 59s
CI / security (pull_request) Successful in 59s
CI / quality (pull_request) Successful in 41s
CI / integration_tests (pull_request) Successful in 2m5s
CI / build (pull_request) Successful in 44s
CI / unit_tests (pull_request) Successful in 4m4s
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 8s
CI / benchmark (pull_request) Failing after 31m29s
to 794331f84f
Some checks failed
CI / lint (pull_request) Failing after 37s
CI / typecheck (pull_request) Successful in 57s
CI / security (pull_request) Successful in 53s
CI / quality (pull_request) Successful in 33s
CI / build (pull_request) Successful in 1m37s
CI / unit_tests (pull_request) Successful in 3m50s
CI / coverage (pull_request) Has been skipped
CI / integration_tests (pull_request) Successful in 3m13s
CI / status-check (pull_request) Failing after 9s
CI / benchmark (pull_request) Has been cancelled
2026-08-09 21:35:17 +00:00
Compare
CoreRasurae force-pushed feature/m1-symlink-safe-file-access from 794331f84f
Some checks failed
CI / lint (pull_request) Failing after 37s
CI / typecheck (pull_request) Successful in 57s
CI / security (pull_request) Successful in 53s
CI / quality (pull_request) Successful in 33s
CI / build (pull_request) Successful in 1m37s
CI / unit_tests (pull_request) Successful in 3m50s
CI / coverage (pull_request) Has been skipped
CI / integration_tests (pull_request) Successful in 3m13s
CI / status-check (pull_request) Failing after 9s
CI / benchmark (pull_request) Has been cancelled
to 7a07de8377
Some checks failed
CI / lint (pull_request) Failing after 1m39s
CI / security (pull_request) Successful in 2m15s
CI / quality (pull_request) Successful in 2m15s
CI / typecheck (pull_request) Successful in 2m43s
CI / build (pull_request) Successful in 1m13s
CI / integration_tests (pull_request) Successful in 3m50s
CI / unit_tests (pull_request) Successful in 5m34s
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 9s
CI / benchmark (pull_request) Has been cancelled
2026-08-09 21:37:08 +00:00
Compare
CoreRasurae force-pushed feature/m1-symlink-safe-file-access from 7a07de8377
Some checks failed
CI / lint (pull_request) Failing after 1m39s
CI / security (pull_request) Successful in 2m15s
CI / quality (pull_request) Successful in 2m15s
CI / typecheck (pull_request) Successful in 2m43s
CI / build (pull_request) Successful in 1m13s
CI / integration_tests (pull_request) Successful in 3m50s
CI / unit_tests (pull_request) Successful in 5m34s
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 9s
CI / benchmark (pull_request) Has been cancelled
to da81067127
Some checks failed
CI / lint (pull_request) Failing after 18s
CI / typecheck (pull_request) Successful in 1m16s
CI / build (pull_request) Successful in 51s
CI / security (pull_request) Successful in 1m15s
CI / quality (pull_request) Successful in 1m7s
CI / integration_tests (pull_request) Successful in 1m58s
CI / unit_tests (pull_request) Successful in 5m12s
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 6s
CI / benchmark (pull_request) Has been cancelled
2026-08-09 21:40:22 +00:00
Compare
Author
Member

Thanks for the thorough review — replying to each item.

Minor Issues

  1. Missing Behave coverage for intermediate-component swap on writes — Fixed.
    Added a Scenario Outline in features/symlink_safe_open.feature ("FileAccessCore.write refuses an intermediate-component swap that happens inside its own resolve-then-open step for every write mode") covering w/a/insert, mirroring the existing final-component write outline. It reuses the existing I arm an intermediate-component symlink swap on "sub" ... and the secret outside the sandbox is never overwritten steps, so no new step definitions were needed.

  2. Racing swapper can crash on contention — Fixed.
    Wrapped the toggle body in robot/InlineFileHelperTestLib.py::_start_swapper_thread in try/except OSError: continue, matching SymlinkSafeOpenTestLib._start_swapper. Verified with a scoped nox -s integration_tests -- robot/inline_code_file_helpers.robot run — 3/3 pass (the transient [ ERROR ] lines in that run are the race's own expected contention, not failures).

  3. Built-in file_read directory listing still follows symlinks — Not fixed, deliberately.
    Confirmed: _file_read_tool's directory-listing branch (os.listdir/os.path.isdir/os.path.getsize) never routes through FileAccessCore, so it sits entirely outside SymlinkSafeOpener's reach — you're right that it's a real, same-class gap. I initially added a note about it to ADR-2035, but on reflection (and per feedback from a second pass) that's the wrong home for it: the ADR documents the D-9 decision and its consequences, not an inventory of every unhardened call site elsewhere in the codebase — the spec should stay decision-scoped and not accumulate ticket-shaped follow-up notes. It's also a distinct unit of work: hardening it means extending the component-wise walk to directory enumeration (os.listdir(dir_fd)), a different filesystem primitive from open(), not a corollary of this change. Left out of this PR and out of the ADR; worth its own issue if you'd like me to file one.

Nits

  1. _secure_open return type AnyTextIO — Fixed. Added the import and changed the annotation; nox -s typecheck (Pyright strict) stays clean.

  2. _SYMLINK_SWAP_ERRNOS omits EMLINK — Not applied.
    Checked this against POSIX/Linux open(2) semantics: O_NOFOLLOW symlink refusal is documented as ELOOP (already handled), and ENOTDIR covers an intermediate component that stopped being a directory (already handled). EMLINK ("too many links") is a link()/rename() errno, not one open() raises for a swapped symlink on any POSIX platform I could find documented. Since the project's CI target is Linux and I couldn't substantiate a platform where open() + O_NOFOLLOW surfaces EMLINK, adding it would be speculative rather than corrective, so I left it out. Happy to add it if you have a specific platform/reference in mind.

  3. Legitimate-write regression only exercised append mode — Fixed. Added a Scenario Outline ("Legitimate in-root writes still succeed for every write mode after the hardening") covering w/a/insert, plus a new the core write succeeds step. Kept the original append-mode scenario as-is (renamed to "...append writes..." for clarity) since it also asserts on-disk content, not just success.

Verification

nox -s lint and nox -s typecheck clean. nox -s unit_tests scoped to the touched features: 39 scenarios / 170 steps, all passing. nox -s coverage_report scoped to the same features: file_access.py at 92%, no newly-introduced uncovered lines (the gaps are pre-existing paths exercised elsewhere in the full suite, not part of this diff). nox -s integration_tests scoped to robot/inline_code_file_helpers.robot: 3/3 passing.

All pushed to this branch (amended, not a new commit, since the commit hasn't landed on master yet). Re-requesting review — let me know if you'd like the directory-listing gap filed as its own issue.

Thanks for the thorough review — replying to each item. ### Minor Issues 1. **Missing Behave coverage for intermediate-component swap on writes — Fixed.** Added a `Scenario Outline` in `features/symlink_safe_open.feature` ("FileAccessCore.write refuses an intermediate-component swap that happens inside its own resolve-then-open step for every write mode") covering `w`/`a`/`insert`, mirroring the existing final-component write outline. It reuses the existing `I arm an intermediate-component symlink swap on "sub" ...` and `the secret outside the sandbox is never overwritten` steps, so no new step definitions were needed. 2. **Racing swapper can crash on contention — Fixed.** Wrapped the toggle body in `robot/InlineFileHelperTestLib.py::_start_swapper_thread` in `try/except OSError: continue`, matching `SymlinkSafeOpenTestLib._start_swapper`. Verified with a scoped `nox -s integration_tests -- robot/inline_code_file_helpers.robot` run — 3/3 pass (the transient `[ ERROR ]` lines in that run are the race's own expected contention, not failures). 3. **Built-in `file_read` directory listing still follows symlinks — Not fixed, deliberately.** Confirmed: `_file_read_tool`'s directory-listing branch (`os.listdir`/`os.path.isdir`/`os.path.getsize`) never routes through `FileAccessCore`, so it sits entirely outside `SymlinkSafeOpener`'s reach — you're right that it's a real, same-class gap. I initially added a note about it to ADR-2035, but on reflection (and per feedback from a second pass) that's the wrong home for it: the ADR documents the D-9 decision and its consequences, not an inventory of every unhardened call site elsewhere in the codebase — the spec should stay decision-scoped and not accumulate ticket-shaped follow-up notes. It's also a distinct unit of work: hardening it means extending the component-wise walk to directory enumeration (`os.listdir(dir_fd)`), a different filesystem primitive from `open()`, not a corollary of this change. Left out of this PR and out of the ADR; worth its own issue if you'd like me to file one. ### Nits 1. **`_secure_open` return type `Any` → `TextIO` — Fixed.** Added the import and changed the annotation; `nox -s typecheck` (Pyright strict) stays clean. 2. **`_SYMLINK_SWAP_ERRNOS` omits `EMLINK` — Not applied.** Checked this against POSIX/Linux `open(2)` semantics: `O_NOFOLLOW` symlink refusal is documented as `ELOOP` (already handled), and `ENOTDIR` covers an intermediate component that stopped being a directory (already handled). `EMLINK` ("too many links") is a `link()`/`rename()` errno, not one `open()` raises for a swapped symlink on any POSIX platform I could find documented. Since the project's CI target is Linux and I couldn't substantiate a platform where `open()` + `O_NOFOLLOW` surfaces `EMLINK`, adding it would be speculative rather than corrective, so I left it out. Happy to add it if you have a specific platform/reference in mind. 3. **Legitimate-write regression only exercised append mode — Fixed.** Added a `Scenario Outline` ("Legitimate in-root writes still succeed for every write mode after the hardening") covering `w`/`a`/`insert`, plus a new `the core write succeeds` step. Kept the original append-mode scenario as-is (renamed to "...append writes..." for clarity) since it also asserts on-disk content, not just success. ### Verification `nox -s lint` and `nox -s typecheck` clean. `nox -s unit_tests` scoped to the touched features: 39 scenarios / 170 steps, all passing. `nox -s coverage_report` scoped to the same features: `file_access.py` at 92%, no newly-introduced uncovered lines (the gaps are pre-existing paths exercised elsewhere in the full suite, not part of this diff). `nox -s integration_tests` scoped to `robot/inline_code_file_helpers.robot`: 3/3 passing. All pushed to this branch (amended, not a new commit, since the commit hasn't landed on `master` yet). Re-requesting review — let me know if you'd like the directory-listing gap filed as its own issue.
Author
Member

Filed the directory-listing gap (minor issue #3 above) as its own issue: #126, under the same parent Epic (#77) as #105. Not folding it into this PR or into ADR-2035 — different unit of work, same rationale as noted above.

Filed the directory-listing gap (minor issue #3 above) as its own issue: #126, under the same parent Epic (#77) as #105. Not folding it into this PR or into ADR-2035 — different unit of work, same rationale as noted above.
feat(agents): symlink-safe sandbox file access closing TOCTOU window
Some checks failed
CI / lint (pull_request) Successful in 41s
CI / quality (pull_request) Successful in 50s
CI / typecheck (pull_request) Successful in 1m5s
CI / security (pull_request) Successful in 1m20s
CI / build (pull_request) Successful in 59s
CI / integration_tests (pull_request) Successful in 2m50s
CI / unit_tests (pull_request) Successful in 4m3s
CI / coverage (pull_request) Successful in 4m8s
CI / status-check (pull_request) Successful in 7s
CI / benchmark (pull_request) Has been cancelled
CI / lint (push) Successful in 55s
CI / quality (push) Successful in 54s
CI / typecheck (push) Successful in 1m1s
CI / build (push) Successful in 53s
CI / integration_tests (push) Successful in 2m29s
CI / unit_tests (push) Successful in 4m45s
CI / security (push) Failing after 15m11s
CI / coverage (push) Has been skipped
CI / status-check (push) Failing after 6s
CI / benchmark (push) Failing after 34m4s
bb3e75097e
SandboxRootPolicy.resolve validated a path once (realpath), then
FileAccessCore.read_window/write opened it with a plain open(resolved, ...).
Because resolution and open() are two separate filesystem operations, an
attacker with write access to a directory the path passes through could
swap a component - the final one or an intermediate parent - for a symlink
after validation but before the open, which open() would silently follow
(CWE-367). The same gap existed on the unconfined path the built-in tools
use, which never resolved at all before opening.

Adds SymlinkSafeOpener, which opens the target one path component at a time
from the filesystem root, with O_NOFOLLOW set on every os.open() call
(O_DIRECTORY for all but the last component). Because O_NOFOLLOW only
inspects the trailing component of a single open() call, walking one
component per call means every component - not only the final one - is
independently re-verified not to be a symlink at the exact moment it is
opened. FileAccessCore.read_window/write (and the write helpers
prepare_append_content/handle_insert_position) now route every open through
this opener, restoring a resolve step for the previously-unresolved
unconfined path too, so the fix applies identically to the built-in
file_read/file_write tools and the inline read_file/write_file helpers. A
detected post-validation swap surfaces as ValueError for a confined caller
(same vocabulary as an ordinary containment violation) and as the
underlying OSError for an unconfined caller, matching what the built-in
tools already handle. Legitimate in-root reads and writes, including all
file_write modes (w/a/insert), are unchanged.

This is enforcement-only hardening - the set of paths admitted or rejected
is unchanged, so no Actor Configuration Standard revision is needed.
Documented as ADR-2035 D-9 (2026-08-08 revision, see its Revision History).

Tests: Behave scenarios simulate the exact race deterministically via a
monkeypatched resolve/realpath step for final- and intermediate-component
swaps across read and every write mode. Robot integration tests race real
concurrent threads (no mocks) against both the inline read_file helper and
FileAccessCore directly for every write mode, and against the built-in
tools' generic OSError handling.

ISSUES CLOSED: #105
CoreRasurae force-pushed feature/m1-symlink-safe-file-access from da81067127
Some checks failed
CI / lint (pull_request) Failing after 18s
CI / typecheck (pull_request) Successful in 1m16s
CI / build (pull_request) Successful in 51s
CI / security (pull_request) Successful in 1m15s
CI / quality (pull_request) Successful in 1m7s
CI / integration_tests (pull_request) Successful in 1m58s
CI / unit_tests (pull_request) Successful in 5m12s
CI / coverage (pull_request) Has been skipped
CI / status-check (pull_request) Failing after 6s
CI / benchmark (pull_request) Has been cancelled
to bb3e75097e
Some checks failed
CI / lint (pull_request) Successful in 41s
CI / quality (pull_request) Successful in 50s
CI / typecheck (pull_request) Successful in 1m5s
CI / security (pull_request) Successful in 1m20s
CI / build (pull_request) Successful in 59s
CI / integration_tests (pull_request) Successful in 2m50s
CI / unit_tests (pull_request) Successful in 4m3s
CI / coverage (pull_request) Successful in 4m8s
CI / status-check (pull_request) Successful in 7s
CI / benchmark (pull_request) Has been cancelled
CI / lint (push) Successful in 55s
CI / quality (push) Successful in 54s
CI / typecheck (push) Successful in 1m1s
CI / build (push) Successful in 53s
CI / integration_tests (push) Successful in 2m29s
CI / unit_tests (push) Successful in 4m45s
CI / security (push) Failing after 15m11s
CI / coverage (push) Has been skipped
CI / status-check (push) Failing after 6s
CI / benchmark (push) Failing after 34m4s
2026-08-09 21:52:51 +00:00
Compare
CoreRasurae deleted branch feature/m1-symlink-safe-file-access 2026-08-09 22:06:56 +00:00
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Reference
cleveragents/cleveractors-core!120
No description provided.