[stack 2/8] fix(security): harden session and autonomous execution boundaries - #1159
[stack 2/8] fix(security): harden session and autonomous execution boundaries#1159sethkarten wants to merge 9 commits into
Conversation
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| appendFileSync(newSessionFile, `${JSON.stringify(out)}\n`); | ||
| forkedEntries.push(out); | ||
| } | ||
| writePrivateFileAtomic(newSessionFile, `${forkedEntries.map((entry) => JSON.stringify(entry)).join("\n")}\n`); |
There was a problem hiding this comment.
🟠 High core/session-manager.ts:2280
forkFrom builds the entire forked session as one in-memory string via forkedEntries.map(...).join("\n") before writing, so forking a sufficiently large session can exhaust the Node heap and fail. Previously each entry was appended to the file one line at a time, avoiding the full-session allocation. Consider writing entries to a temporary file incrementally and then atomically renaming, instead of constructing the complete JSONL payload in memory.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/core/session-manager.ts around line 2280:
`forkFrom` builds the entire forked session as one in-memory string via `forkedEntries.map(...).join("\n")` before writing, so forking a sufficiently large session can exhaust the Node heap and fail. Previously each entry was appended to the file one line at a time, avoiding the full-session allocation. Consider writing entries to a temporary file incrementally and then atomically renaming, instead of constructing the complete JSONL payload in memory.
| this.persist = persist; | ||
| if (persist && sessionDir && !existsSync(sessionDir)) { | ||
| mkdirSync(sessionDir, { recursive: true }); | ||
| if (persist && sessionDir) { |
There was a problem hiding this comment.
🟠 High core/session-manager.ts:1221
SessionManager.open(path) derives sessionDir from dirname(path), so opening an arbitrary user-selected session file (e.g. --resume /shared/project/session.jsonl) runs ensurePrivateDirectory on /shared/project, chmodding an existing shared directory to 0700 and revoking access for every other user. The constructor now calls ensurePrivateDirectory(sessionDir) unconditionally whenever persist is true, even for directories the caller already owns and intends to remain shared. Only directories newly created by the agent should be chmodded; opening an existing file in an arbitrary location must not mutate its parent directory's permissions.
Also found in 1 other location(s)
packages/coding-agent/src/utils/private-files.ts:75
ensurePrivateDirectoryunconditionally changes an existing directory to mode0700. Callers pass user-selected locations such as--session-dirand the parent of a file supplied to--resume; if that directory is intentionally shared or contains unrelated files, merely opening/creating a session removes access for every other user and can break unrelated workflows. Only directories created by the agent should be chmodded automatically, or the private session storage should live in a dedicated child directory.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/core/session-manager.ts around line 1221:
`SessionManager.open(path)` derives `sessionDir` from `dirname(path)`, so opening an arbitrary user-selected session file (e.g. `--resume /shared/project/session.jsonl`) runs `ensurePrivateDirectory` on `/shared/project`, chmodding an existing shared directory to `0700` and revoking access for every other user. The constructor now calls `ensurePrivateDirectory(sessionDir)` unconditionally whenever `persist` is true, even for directories the caller already owns and intends to remain shared. Only directories newly created by the agent should be chmodded; opening an existing file in an arbitrary location must not mutate its parent directory's permissions.
Also found in 1 other location(s):
- packages/coding-agent/src/utils/private-files.ts:75 -- `ensurePrivateDirectory` unconditionally changes an existing directory to mode `0700`. Callers pass user-selected locations such as `--session-dir` and the parent of a file supplied to `--resume`; if that directory is intentionally shared or contains unrelated files, merely opening/creating a session removes access for every other user and can break unrelated workflows. Only directories created by the agent should be chmodded automatically, or the private session storage should live in a dedicated child directory.
| } | ||
| } | ||
|
|
||
| export function readPrivateFile(path: string, encoding: BufferEncoding): string { |
There was a problem hiding this comment.
🟢 Low utils/private-files.ts:104
readPrivateFile calls setPrivateFileMode before entering the try/finally that closes fd, so if fchmodSync/chmodSync throws (e.g. on a read-only or permission-restricted filesystem), the file descriptor leaks. Callers that catch the error and retry can exhaust file descriptors. Move setPrivateFileMode inside the try block.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/utils/private-files.ts around line 104:
`readPrivateFile` calls `setPrivateFileMode` before entering the `try/finally` that closes `fd`, so if `fchmodSync`/`chmodSync` throws (e.g. on a read-only or permission-restricted filesystem), the file descriptor leaks. Callers that catch the error and retry can exhaust file descriptors. Move `setPrivateFileMode` inside the `try` block.
| path.chmod(mode) | ||
|
|
||
|
|
||
| def _ensure_private_directory(path: Path) -> None: |
There was a problem hiding this comment.
🟠 High rlm/harness.py:102
On Windows, _ensure_private_directory calls path.chmod(0o700) to restrict the harness directory, but Windows chmod() only toggles the read-only flag and ignores Unix permission bits. The directory therefore keeps its inherited/default ACL, so other local users can read or replace the supposedly private harness state. Python only added restrictive ACL handling for mkdir(mode=...) in 3.13, so this is broken on the project's supported 3.10–3.12 range. Consider applying an explicit restrictive Windows ACL (e.g. via win32security or icacls) for both creation and existing-directory verification instead of chmod.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @prime-agent-runtime/src/rlm/harness.py around line 102:
On Windows, `_ensure_private_directory` calls `path.chmod(0o700)` to restrict the harness directory, but Windows `chmod()` only toggles the read-only flag and ignores Unix permission bits. The directory therefore keeps its inherited/default ACL, so other local users can read or replace the supposedly private harness state. Python only added restrictive ACL handling for `mkdir(mode=...)` in 3.13, so this is broken on the project's supported 3.10–3.12 range. Consider applying an explicit restrictive Windows ACL (e.g. via `win32security` or `icacls`) for both creation and existing-directory verification instead of `chmod`.
| const historyPath = getRefinementHistoryPath(harnessStateDir); | ||
| mkdirSync(harnessStateDir, { recursive: true }); | ||
| appendFileSync(historyPath, `${JSON.stringify(result)}\n`, "utf8"); | ||
| appendPrivateFile(historyPath, `${JSON.stringify(result)}\n`); |
There was a problem hiding this comment.
🟠 High refinement/refinement.ts:357
appendGlobalRefinement calls appendPrivateFile, which checks for file absence before opening with O_CREAT | O_EXCL. When two sessions append the first global refinement concurrently, both observe the missing file, one open fails with EEXIST, and that error propagates to the caller. Because the refinement has already been applied by the time this runs, the command reports failure and skips recording both the global rollback-history entry and the session entry, leaving an applied change with no rollback record. The previous appendFileSync did not have this race because it atomically created-and-appended in one call. Consider ensuring the append path tolerates concurrent first-writer creation (e.g., retry on EEXIST or fall back to a non-exclusive open) so the history entry is always recorded after a refinement is applied.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/core/refinement/refinement.ts around line 357:
`appendGlobalRefinement` calls `appendPrivateFile`, which checks for file absence before opening with `O_CREAT | O_EXCL`. When two sessions append the first global refinement concurrently, both observe the missing file, one open fails with `EEXIST`, and that error propagates to the caller. Because the refinement has already been applied by the time this runs, the command reports failure and skips recording both the global rollback-history entry and the session entry, leaving an applied change with no rollback record. The previous `appendFileSync` did not have this race because it atomically created-and-appended in one call. Consider ensuring the append path tolerates concurrent first-writer creation (e.g., retry on `EEXIST` or fall back to a non-exclusive open) so the history entry is always recorded after a refinement is applied.
| @@ -159,7 +179,14 @@ def _prime_agent_restore_state(): | |||
| return | |||
|
|
|||
| try: | |||
There was a problem hiding this comment.
🟠 High kernel/state-snapshot.ts:181
os.lstat validates the path as a regular file, then os.open opens it in a separate call, leaving a TOCTOU window. Between the two calls, a process able to modify the snapshot directory can replace the checked file with a symlink or a different regular file; O_NOFOLLOW does not block the regular-file-replacement case, and _b.getattr(os, "O_NOFOLLOW", 0) silently drops no-follow protection on platforms where the flag is unavailable (e.g. Windows). dill.load then deserializes the replacement payload, executing attacker-controlled code. Consider opening the file first and validating the descriptor with fstat (or os.fstat), and fail closed when O_NOFOLLOW is unavailable rather than defaulting to 0.
Also found in 1 other location(s)
prime-agent-runtime/src/rlm/harness.py:125
_open_private_for_readperforms a pathnamelstat()and then a separateos.open(), whilegetattr(os, "O_NOFOLLOW", 0)silently removes the only atomic no-follow protection on platforms where that extension is unavailable (including Windows). An attacker able to modify the state path can swap the checked regular file for a symlink/reparse point between these calls; the code then opens and reads the link target and_chmod_open_filemay also chmod that unrelated target. This defeats the private-file boundary and can expose or alter another file. The open must use a platform-specific atomic reparse-point-safe mechanism, or fail closed when no such mechanism exists.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/coding-agent/src/core/kernel/state-snapshot.ts around line 181:
`os.lstat` validates the path as a regular file, then `os.open` opens it in a separate call, leaving a TOCTOU window. Between the two calls, a process able to modify the snapshot directory can replace the checked file with a symlink or a different regular file; `O_NOFOLLOW` does not block the regular-file-replacement case, and `_b.getattr(os, "O_NOFOLLOW", 0)` silently drops no-follow protection on platforms where the flag is unavailable (e.g. Windows). `dill.load` then deserializes the replacement payload, executing attacker-controlled code. Consider opening the file first and validating the descriptor with `fstat` (or `os.fstat`), and fail closed when `O_NOFOLLOW` is unavailable rather than defaulting to 0.
Also found in 1 other location(s):
- prime-agent-runtime/src/rlm/harness.py:125 -- `_open_private_for_read` performs a pathname `lstat()` and then a separate `os.open()`, while `getattr(os, "O_NOFOLLOW", 0)` silently removes the only atomic no-follow protection on platforms where that extension is unavailable (including Windows). An attacker able to modify the state path can swap the checked regular file for a symlink/reparse point between these calls; the code then opens and reads the link target and `_chmod_open_file` may also chmod that unrelated target. This defeats the private-file boundary and can expose or alter another file. The open must use a platform-specific atomic reparse-point-safe mechanism, or fail closed when no such mechanism exists.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d07e5a8. Configure here.
| const childSessionManager = SessionManager.create(this._cwd, options.sessionDir); | ||
| const childSessionManager = options.parentSession.sessionManager.isPersisted() | ||
| ? SessionManager.create(this._cwd, options.sessionDir) | ||
| : SessionManager.inMemory(this._cwd, options.sessionDir); |
There was a problem hiding this comment.
Ephemeral children omit RLM depth
Medium Severity
When the parent has no sessionFile (--no-session / in-memory), the whole newSession call is skipped, so child headers never get rlmDepth. newSession already accepts an explicit rlmDepth with a missing parentSession; gating on sessionFile drops that update. Runtime depth still comes from config, but the session header stays at the root default.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d07e5a8. Configure here.
| export function appendGlobalRefinement(harnessStateDir: string, result: RefinementResult): string { | ||
| const historyPath = getRefinementHistoryPath(harnessStateDir); | ||
| mkdirSync(harnessStateDir, { recursive: true }); | ||
| appendFileSync(historyPath, `${JSON.stringify(result)}\n`, "utf8"); |
There was a problem hiding this comment.
History load throws on symlink
Medium Severity
loadGlobalRefinementHistory switched to readPrivateFile without a top-level try/catch. A symlinked history file passes existsSync but then throws from readPrivateFile, instead of degrading to an empty list like loadHarnessState does for the same private-read failure mode.
Reviewed by Cursor Bugbot for commit d07e5a8. Configure here.
| if (!parentExisted) chmodSync(parent, PRIVATE_DIRECTORY_MODE); | ||
| } else { | ||
| ensurePrivateDirectory(parent); | ||
| } |
There was a problem hiding this comment.
Export rejects symlinked parents
Medium Severity
HTML export uses writePrivateFileAtomic with privateParent: false, but that path still refuses when the parent directory is a symlink. User export destinations often live under symlinked trees, so export can fail where plain writeFileSync previously succeeded.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit d07e5a8. Configure here.
|
Thanks for picking this up. As the reporter of the underlying findings (PR #1105, now closed), here is the scope-coverage confirmation for triage: Covered by the stack: session-ID/path containment and the dill resume RCE path (#1159), harness.py private modes (#1159), coding-agent auth-storage atomic writes (#1160), snapshot/export/state-snapshot paths (#1159/#1160). Two findings from the original report are NOT covered by any PR in #1158-#1165:
Separately, our open PR #966 (snapshot-cache invalidation when worker recovery drops operations, daemon-supervisor.ts) overlaps the #1161/#1163 area; we are fine closing it if your stack covers that path. |
2 participants
Stack 2/8 — fix(security): harden session and autonomous execution boundaries
Base:
stack/external-01-verificationReview order: merge only after the preceding stack layer is accepted. This PR is not intended to merge independently out of order.
Stack navigation
Summary
--no-sessiondescendants in memory and document the actual autonomous-execution isolation boundary.Validation
npm run check; focused Vitest 184/184; Python harness 36/36; workflow digest and documentation-link validation.Provenance
upstream/mainusing issue reports and PR descriptions/comments only.Linked-item disposition
Fixed on merge
Independently superseded pull requests
Partial/distinct overlap — remains open
Reviewer notes
main, to avoid cumulative duplicate diffs.