feat(agents): symlink-safe sandbox file access closing TOCTOU window #120
No reviewers
Labels
No labels
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Blocks
#105 Harden sandbox file access against symlink TOCTOU (CWE-367) with symlink-safe open
cleveragents/cleveractors-core
Reference
cleveragents/cleveractors-core!120
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/m1-symlink-safe-file-access"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Hardens
FileAccessCore(the shared core behind both the built-infile_read/file_writetools and the inline-coderead_file/write_filehelpers, ADR-2035) against a symlink time-of-check/time-of-use race (CWE-367).SandboxRootPolicy.resolvevalidated a path once viaos.path.realpath, andFileAccessCore.read_window/writethen opened that resolved path with a plainopen(...). Because resolution andopen()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 plainopen()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
SymlinkSafeOpeneropens the target one path component at a time from the filesystem root, withO_NOFOLLOWset on everyos.open()call (O_DIRECTORYfor every component but the last). BecauseO_NOFOLLOWonly inspects the trailing component of a singleopen()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 helpersprepare_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 allfile_writemodes (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-
fstatre-verification was rejected in favor of the component-wise walk).Testing
features/symlink_safe_open.feature— deterministically simulates the exact race via a monkeypatchedresolve/realpathstep 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/symlink_safe_open.robot— races a real, genuinely concurrent background thread againstFileAccessCoredirectly (confine=True) for reads and every write mode, no mocks.robot/inline_code_file_helpers.robot— races the same real thread against the publicread_fileinline helper end to end through a realToolAgent.write_filehelper was considered and dropped:safe_mode: falsehas 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-confinedFileAccessCore.write(confine=True)surface, which is where a boundary actually applies.)nox -s lint typecheck security_scan dead_code unit_tests coverage_report integration_testsall green.nox -s benchmark_regression -- --quickrun informationally (not required for merge); the existingbenchmarks/file_access_benchmark.pyalready exercises the affected hot paths.Definition of Done
nox -s coverage_report.noxcore gates green.Closes #105
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_DIRECTORYwalk is sound, applies to both confined and unconfined paths through the single sharedFileAccessCore, 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
Missing Behave coverage for intermediate-component swap on writes.
features/symlink_safe_open.feature(around line 57)SymlinkSafeOpeneris used), but only the read path has an explicit intermediate-swap Behave scenario. Write modes are only tested with a final-component swap.Scenario OutlineforFileAccessCore.writecoveringw,a, andinsert.Racing swapper in inline integration test can crash on contention.
robot/InlineFileHelperTestLib.py(lines 115–124)_start_swapper_threaddoes not catchOSErrorwhile toggling the target between a real file and a symlink. A race against the main thread'sread_file/write_filecan raiseFileNotFoundErrororFileExistsError, causing the swapper thread to die and weakening the test's ability to actually exercise the race window. The analogousSymlinkSafeOpenTestLib._start_swapperalready wraps its toggle loop intry ... except OSError.try ... except OSError: continue, matchingSymlinkSafeOpenTestLib.Built-in
file_readdirectory listing still follows symlinks withoutO_NOFOLLOW.src/cleveractors/agents/tool.py(lines 846–859)_file_read_tooluses plainos.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.O_NOFOLLOW|O_DIRECTORYwalk (os.listdir(dir_fd)) or document it as a known residual gap.Nits
Return type of
_secure_opencould be more precise.src/cleveractors/agents/file_access.py(line 331)Any; the value is always a text-mode file object.typing.TextIO._SYMLINK_SWAP_ERRNOSomitsEMLINKon some POSIX platforms.src/cleveractors/agents/file_access.py(line 62)O_NOFOLLOWsymlink refusal asEMLINKrather thanELOOP. Linux/Gitea CI likely usesELOOP, so not blocking.errno.EMLINKif the project supports those platforms.Legitimate-write regression coverage only exercises append mode.
features/symlink_safe_open.feature(lines 79–84)mode "a". The existinginline_code_file_helpers.featurecoversw/insert, but a dedicated per-mode regression scenario in this feature would make the hardening's behavior-preservation claim explicit.Scenario Outlineforw,a, andinsert.Summary
This is a solid, well-documented defense-in-depth change. The
SymlinkSafeOpenerdesign correctly addresses CWE-367 without changing the set of admitted paths, and by routing everyFileAccessCoreopen 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 readScenario Outline: FileAccessCore.write refuses a final-component swap that happens inside its own resolve-then-open step for every write modeMinor: consider adding an intermediate-component swap Scenario Outline for writes, matching the read coverage above.
@ -0,0 +76,4 @@Then the core raises an OSErrorAnd the secret outside the sandbox is never readScenario: Legitimate in-root writes still succeed for every write mode after the hardeningNit: 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_namestop = threading.Event()def swap_forever() -> None: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)Nit: some POSIX systems surface O_NOFOLLOW symlink refusal as EMLINK; consider adding it.
@ -227,2 +328,4 @@return self._root_policy@staticmethoddef _secure_open(Nit: return type could be
typing.TextIOinstead ofAny.Minor/pre-existing: the directory-listing branch still follows symlinks, leaving a residual TOCTOU gap not covered by ADR-2035 D-9.
4a55dd51a2794331f84f794331f84f7a07de83777a07de8377da81067127Thanks for the thorough review — replying to each item.
Minor Issues
Missing Behave coverage for intermediate-component swap on writes — Fixed.
Added a
Scenario Outlineinfeatures/symlink_safe_open.feature("FileAccessCore.write refuses an intermediate-component swap that happens inside its own resolve-then-open step for every write mode") coveringw/a/insert, mirroring the existing final-component write outline. It reuses the existingI arm an intermediate-component symlink swap on "sub" ...andthe secret outside the sandbox is never overwrittensteps, so no new step definitions were needed.Racing swapper can crash on contention — Fixed.
Wrapped the toggle body in
robot/InlineFileHelperTestLib.py::_start_swapper_threadintry/except OSError: continue, matchingSymlinkSafeOpenTestLib._start_swapper. Verified with a scopednox -s integration_tests -- robot/inline_code_file_helpers.robotrun — 3/3 pass (the transient[ ERROR ]lines in that run are the race's own expected contention, not failures).Built-in
file_readdirectory 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 throughFileAccessCore, so it sits entirely outsideSymlinkSafeOpener'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 fromopen(), 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
_secure_openreturn typeAny→TextIO— Fixed. Added the import and changed the annotation;nox -s typecheck(Pyright strict) stays clean._SYMLINK_SWAP_ERRNOSomitsEMLINK— Not applied.Checked this against POSIX/Linux
open(2)semantics:O_NOFOLLOWsymlink refusal is documented asELOOP(already handled), andENOTDIRcovers an intermediate component that stopped being a directory (already handled).EMLINK("too many links") is alink()/rename()errno, not oneopen()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 whereopen()+O_NOFOLLOWsurfacesEMLINK, 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.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") coveringw/a/insert, plus a newthe core write succeedsstep. 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 lintandnox -s typecheckclean.nox -s unit_testsscoped to the touched features: 39 scenarios / 170 steps, all passing.nox -s coverage_reportscoped to the same features:file_access.pyat 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_testsscoped torobot/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
masteryet). Re-requesting review — let me know if you'd like the directory-listing gap filed as its own issue.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.
da81067127bb3e75097e