feat(tools): add allowedRoots config for fs tools#43565
feat(tools): add allowedRoots config for fs tools#43565YJWell wants to merge 10 commits intoopenclaw:mainfrom
Conversation
openclaw-barnacle
bot
added
docker
Greptile SummaryThis PR implements Issues found:
Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-tools.read.ts
Line: 744-753
Comment:
**`mkdir` should use `sandboxResult.resolved` instead of pre-resolved path**
`path.resolve(dir)` is called before `assertSandboxPath`, and then that same pre-resolved string is passed to `fs.mkdir`. However, `assertSandboxPath` internally applies `expandPath` to the input, which normalises Unicode spaces (e.g. `\u00A0` → space) and expands tilde prefixes. This means the path that passes the sandbox check (`sandboxResult.resolved`) may differ from the `resolved` string that is actually passed to `fs.mkdir`.
The `writeFile` operation in the same function avoids this by capturing and using `sandboxResult.resolved`:
```ts
const sandboxResult = await assertSandboxPath({ ... });
const resolvedPath = sandboxResult.resolved;
await fs.mkdir(path.dirname(resolvedPath), { recursive: true });
await fs.writeFile(resolvedPath, content, "utf-8");
```
`mkdir` should follow the same pattern:
```suggestion
mkdir: async (dir: string) => {
const sandboxResult = await assertSandboxPath({
filePath: dir,
cwd: root,
root,
additionalRoots: allowedRoots,
});
await fs.mkdir(sandboxResult.resolved, { recursive: true });
},
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/pi-tools.read.ts
Line: 813-837
Comment:
**`access` should reuse `sandboxResult.resolved` for consistency**
After `assertSandboxPath` succeeds, the code discards the returned `sandboxResult` and instead calls `path.resolve(absolutePath)` independently for the actual `fs.access` call. `path.resolve` does not apply `expandPath`'s Unicode-space normalisation, so the path used for the sandbox boundary check and the path used for the filesystem call can differ if `absolutePath` contains Unicode spaces.
Both `readFile` and `writeFile` in `createHostEditOperations` correctly capture and reuse `sandboxResult.resolved`. This function should do the same:
```suggestion
access: async (absolutePath: string) => {
let sandboxResult: Awaited<ReturnType<typeof assertSandboxPath>>;
try {
sandboxResult = await assertSandboxPath({
filePath: absolutePath,
cwd: root,
root,
additionalRoots: allowedRoots,
});
} catch {
// Don't throw here – the upstream library replaces any `access` error
// with a misleading "File not found" message. By returning silently the
// subsequent `readFile` call will throw through a code-path that
// propagates the original message.
return;
}
const resolved = sandboxResult.resolved;
try {
await fs.access(resolved);
} catch (error) {
if ((error as NodeJS.ErrnoException).code === "ENOENT") {
throw createFsAccessError("ENOENT", absolutePath);
}
throw error;
}
},
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/config/zod-schema.agent-runtime.ts
Line: 433
Comment:
**Redundant `.default([]).optional()` on `allowedRoots`**
Using `.default([])` and `.optional()` together is contradictory: `.default([])` means Zod will always substitute `[]` when the field is absent, so the value is never `undefined` after parsing, making `.optional()` a no-op at runtime. Pick one:
- If the field should be required with a fallback, keep only `.default([])`.
- If it should be truly optional (callers must handle `undefined`), keep only `.optional()` and handle the missing case at call sites.
Given that `resolveToolFsConfig` already uses `?? globalFs?.allowedRoots` (which handles `undefined`), `.optional()` alone is probably the right choice and is consistent with how `workspaceOnly` is declared on the line above.
```suggestion
allowedRoots: z.array(z.string()).optional(),
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: f46a20c |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/agents/pi-tools.read.ts
Outdated
| return { | ||
| mkdir: async (dir: string) => { | ||
| const relative = toRelativeWorkspacePath(root, dir, { allowRoot: true }); | ||
| const resolved = relative ? path.resolve(root, relative) : path.resolve(root); | ||
| await assertSandboxPath({ filePath: resolved, cwd: root, root }); | ||
| const resolved = path.resolve(dir); | ||
| await assertSandboxPath({ | ||
| filePath: resolved, | ||
| cwd: root, | ||
| root, | ||
| additionalRoots: allowedRoots, | ||
| }); | ||
| await fs.mkdir(resolved, { recursive: true }); |
There was a problem hiding this comment.
mkdir should use sandboxResult.resolved instead of pre-resolved path
path.resolve(dir) is called before assertSandboxPath, and then that same pre-resolved string is passed to fs.mkdir. However, assertSandboxPath internally applies expandPath to the input, which normalises Unicode spaces (e.g. \u00A0 → space) and expands tilde prefixes. This means the path that passes the sandbox check (sandboxResult.resolved) may differ from the resolved string that is actually passed to fs.mkdir.
The writeFile operation in the same function avoids this by capturing and using sandboxResult.resolved:
const sandboxResult = await assertSandboxPath({ ... });
const resolvedPath = sandboxResult.resolved;
await fs.mkdir(path.dirname(resolvedPath), { recursive: true });
await fs.writeFile(resolvedPath, content, "utf-8");mkdir should follow the same pattern:
| return { | |
| mkdir: async (dir: string) => { | |
| const relative = toRelativeWorkspacePath(root, dir, { allowRoot: true }); | |
| const resolved = relative ? path.resolve(root, relative) : path.resolve(root); | |
| await assertSandboxPath({ filePath: resolved, cwd: root, root }); | |
| const resolved = path.resolve(dir); | |
| await assertSandboxPath({ | |
| filePath: resolved, | |
| cwd: root, | |
| root, | |
| additionalRoots: allowedRoots, | |
| }); | |
| await fs.mkdir(resolved, { recursive: true }); | |
| mkdir: async (dir: string) => { | |
| const sandboxResult = await assertSandboxPath({ | |
| filePath: dir, | |
| cwd: root, | |
| root, | |
| additionalRoots: allowedRoots, | |
| }); | |
| await fs.mkdir(sandboxResult.resolved, { recursive: true }); | |
| }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.read.ts
Line: 744-753
Comment:
**`mkdir` should use `sandboxResult.resolved` instead of pre-resolved path**
`path.resolve(dir)` is called before `assertSandboxPath`, and then that same pre-resolved string is passed to `fs.mkdir`. However, `assertSandboxPath` internally applies `expandPath` to the input, which normalises Unicode spaces (e.g. `\u00A0` → space) and expands tilde prefixes. This means the path that passes the sandbox check (`sandboxResult.resolved`) may differ from the `resolved` string that is actually passed to `fs.mkdir`.
The `writeFile` operation in the same function avoids this by capturing and using `sandboxResult.resolved`:
```ts
const sandboxResult = await assertSandboxPath({ ... });
const resolvedPath = sandboxResult.resolved;
await fs.mkdir(path.dirname(resolvedPath), { recursive: true });
await fs.writeFile(resolvedPath, content, "utf-8");
```
`mkdir` should follow the same pattern:
```suggestion
mkdir: async (dir: string) => {
const sandboxResult = await assertSandboxPath({
filePath: dir,
cwd: root,
root,
additionalRoots: allowedRoots,
});
await fs.mkdir(sandboxResult.resolved, { recursive: true });
},
```
How can I resolve this? If you propose a fix, please make it concise.| access: async (absolutePath: string) => { | ||
| let relative: string; | ||
| try { | ||
| relative = toRelativeWorkspacePath(root, absolutePath); | ||
| await assertSandboxPath({ | ||
| filePath: absolutePath, | ||
| cwd: root, | ||
| root, | ||
| additionalRoots: allowedRoots, | ||
| }); | ||
| } catch { | ||
| // Path escapes workspace root. Don't throw here – the upstream | ||
| // library replaces any `access` error with a misleading "File not | ||
| // found" message. By returning silently the subsequent `readFile` | ||
| // call will throw the same "Path escapes workspace root" error | ||
| // through a code-path that propagates the original message. | ||
| // Don't throw here – the upstream library replaces any `access` error | ||
| // with a misleading "File not found" message. By returning silently the | ||
| // subsequent `readFile` call will throw through a code-path that | ||
| // propagates the original message. | ||
| return; | ||
| } | ||
| const resolved = path.resolve(absolutePath); | ||
| try { | ||
| const opened = await openFileWithinRoot({ | ||
| rootDir: root, | ||
| relativePath: relative, | ||
| }); | ||
| await opened.handle.close().catch(() => {}); | ||
| await fs.access(resolved); | ||
| } catch (error) { | ||
| if (error instanceof SafeOpenError && error.code === "not-found") { | ||
| if ((error as NodeJS.ErrnoException).code === "ENOENT") { | ||
| throw createFsAccessError("ENOENT", absolutePath); | ||
| } | ||
| if (error instanceof SafeOpenError && error.code === "outside-workspace") { | ||
| // Don't throw here – see the comment above about the upstream | ||
| // library swallowing access errors as "File not found". | ||
| return; | ||
| } | ||
| throw error; | ||
| } | ||
| }, |
There was a problem hiding this comment.
access should reuse sandboxResult.resolved for consistency
After assertSandboxPath succeeds, the code discards the returned sandboxResult and instead calls path.resolve(absolutePath) independently for the actual fs.access call. path.resolve does not apply expandPath's Unicode-space normalisation, so the path used for the sandbox boundary check and the path used for the filesystem call can differ if absolutePath contains Unicode spaces.
Both readFile and writeFile in createHostEditOperations correctly capture and reuse sandboxResult.resolved. This function should do the same:
| access: async (absolutePath: string) => { | |
| let relative: string; | |
| try { | |
| relative = toRelativeWorkspacePath(root, absolutePath); | |
| await assertSandboxPath({ | |
| filePath: absolutePath, | |
| cwd: root, | |
| root, | |
| additionalRoots: allowedRoots, | |
| }); | |
| } catch { | |
| // Path escapes workspace root. Don't throw here – the upstream | |
| // library replaces any `access` error with a misleading "File not | |
| // found" message. By returning silently the subsequent `readFile` | |
| // call will throw the same "Path escapes workspace root" error | |
| // through a code-path that propagates the original message. | |
| // Don't throw here – the upstream library replaces any `access` error | |
| // with a misleading "File not found" message. By returning silently the | |
| // subsequent `readFile` call will throw through a code-path that | |
| // propagates the original message. | |
| return; | |
| } | |
| const resolved = path.resolve(absolutePath); | |
| try { | |
| const opened = await openFileWithinRoot({ | |
| rootDir: root, | |
| relativePath: relative, | |
| }); | |
| await opened.handle.close().catch(() => {}); | |
| await fs.access(resolved); | |
| } catch (error) { | |
| if (error instanceof SafeOpenError && error.code === "not-found") { | |
| if ((error as NodeJS.ErrnoException).code === "ENOENT") { | |
| throw createFsAccessError("ENOENT", absolutePath); | |
| } | |
| if (error instanceof SafeOpenError && error.code === "outside-workspace") { | |
| // Don't throw here – see the comment above about the upstream | |
| // library swallowing access errors as "File not found". | |
| return; | |
| } | |
| throw error; | |
| } | |
| }, | |
| access: async (absolutePath: string) => { | |
| let sandboxResult: Awaited<ReturnType<typeof assertSandboxPath>>; | |
| try { | |
| sandboxResult = await assertSandboxPath({ | |
| filePath: absolutePath, | |
| cwd: root, | |
| root, | |
| additionalRoots: allowedRoots, | |
| }); | |
| } catch { | |
| // Don't throw here – the upstream library replaces any `access` error | |
| // with a misleading "File not found" message. By returning silently the | |
| // subsequent `readFile` call will throw through a code-path that | |
| // propagates the original message. | |
| return; | |
| } | |
| const resolved = sandboxResult.resolved; | |
| try { | |
| await fs.access(resolved); | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code === "ENOENT") { | |
| throw createFsAccessError("ENOENT", absolutePath); | |
| } | |
| throw error; | |
| } | |
| }, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-tools.read.ts
Line: 813-837
Comment:
**`access` should reuse `sandboxResult.resolved` for consistency**
After `assertSandboxPath` succeeds, the code discards the returned `sandboxResult` and instead calls `path.resolve(absolutePath)` independently for the actual `fs.access` call. `path.resolve` does not apply `expandPath`'s Unicode-space normalisation, so the path used for the sandbox boundary check and the path used for the filesystem call can differ if `absolutePath` contains Unicode spaces.
Both `readFile` and `writeFile` in `createHostEditOperations` correctly capture and reuse `sandboxResult.resolved`. This function should do the same:
```suggestion
access: async (absolutePath: string) => {
let sandboxResult: Awaited<ReturnType<typeof assertSandboxPath>>;
try {
sandboxResult = await assertSandboxPath({
filePath: absolutePath,
cwd: root,
root,
additionalRoots: allowedRoots,
});
} catch {
// Don't throw here – the upstream library replaces any `access` error
// with a misleading "File not found" message. By returning silently the
// subsequent `readFile` call will throw through a code-path that
// propagates the original message.
return;
}
const resolved = sandboxResult.resolved;
try {
await fs.access(resolved);
} catch (error) {
if ((error as NodeJS.ErrnoException).code === "ENOENT") {
throw createFsAccessError("ENOENT", absolutePath);
}
throw error;
}
},
```
How can I resolve this? If you propose a fix, please make it concise.| const ToolFsSchema = z | ||
| .object({ | ||
| workspaceOnly: z.boolean().optional(), | ||
| allowedRoots: z.array(z.string()).default([]).optional(), |
There was a problem hiding this comment.
Redundant .default([]).optional() on allowedRoots
Using .default([]) and .optional() together is contradictory: .default([]) means Zod will always substitute [] when the field is absent, so the value is never undefined after parsing, making .optional() a no-op at runtime. Pick one:
- If the field should be required with a fallback, keep only
.default([]). - If it should be truly optional (callers must handle
undefined), keep only.optional()and handle the missing case at call sites.
Given that resolveToolFsConfig already uses ?? globalFs?.allowedRoots (which handles undefined), .optional() alone is probably the right choice and is consistent with how workspaceOnly is declared on the line above.
| allowedRoots: z.array(z.string()).default([]).optional(), | |
| allowedRoots: z.array(z.string()).optional(), |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/zod-schema.agent-runtime.ts
Line: 433
Comment:
**Redundant `.default([]).optional()` on `allowedRoots`**
Using `.default([])` and `.optional()` together is contradictory: `.default([])` means Zod will always substitute `[]` when the field is absent, so the value is never `undefined` after parsing, making `.optional()` a no-op at runtime. Pick one:
- If the field should be required with a fallback, keep only `.default([])`.
- If it should be truly optional (callers must handle `undefined`), keep only `.optional()` and handle the missing case at call sites.
Given that `resolveToolFsConfig` already uses `?? globalFs?.allowedRoots` (which handles `undefined`), `.optional()` alone is probably the right choice and is consistent with how `workspaceOnly` is declared on the line above.
```suggestion
allowedRoots: z.array(z.string()).optional(),
```
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f46a20c78c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/agents/pi-tools.read.ts
Outdated
| const resolvedPath = sandboxResult.resolved; | ||
| await fs.mkdir(path.dirname(resolvedPath), { recursive: true }); | ||
| await fs.writeFile(resolvedPath, content, "utf-8"); |
There was a problem hiding this comment.
This workspace-only write path validates with assertSandboxPath and then performs a separate fs.writeFile on the resolved path, which reintroduces a TOCTOU alias race: if another process can mutate the filesystem between the check and write, the final write can be redirected outside the allowed root via symlink/hardlink swaps. The previous writeFileWithinRoot flow used pinned/verified writes to close this gap, so this is a security regression in environments with concurrent/untrusted local processes (including background tool commands).
Useful? React with 👍 / 👎.
|
love this ! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23bc7642f5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/agents/pi-tools.read.ts
Outdated
| additionalRoots: allowedRoots, | ||
| }); | ||
| return safeRead.buffer; | ||
| return await fs.readFile(sandboxResult.resolved); |
There was a problem hiding this comment.
In workspaceOnly mode this now validates with assertSandboxPath and then performs a separate fs.readFile on the path, which reintroduces a TOCTOU alias race: a concurrent local process can swap the validated path to a symlink/hardlink outside the allowed root between check and read, allowing unintended file disclosure. The previous readFileWithinRoot flow pinned and verified the opened file identity specifically to prevent this class of escape.
Useful? React with 👍 / 👎.
| const ToolFsSchema = z | ||
| .object({ | ||
| workspaceOnly: z.boolean().optional(), | ||
| allowedRoots: z.array(z.string()).default([]), |
There was a problem hiding this comment.
Defining allowedRoots with .default([]) at schema parse time makes every parsed tools.fs object carry an explicit empty array, so an agent-level tools.fs block that omits allowedRoots will override and drop global tools.fs.allowedRoots instead of inheriting it. That conflicts with the merge logic in resolveToolFsConfig (agentFs?.allowedRoots ?? globalFs?.allowedRoots) and breaks configs that only intended to override workspaceOnly.
Useful? React with 👍 / 👎.
src/agents/pi-tools.read.ts
Outdated
| rootDir: root, | ||
| relativePath: relative, | ||
| rootDir: sandboxResult.matchedRoot, | ||
| relativePath: sandboxResult.relative || path.basename(sandboxResult.resolved), |
There was a problem hiding this comment.
When assertSandboxPath returns relative === "" (for example path: "." or path equal to the root), this fallback rewrites the destination to path.basename(resolved) and writes a different file under the root (e.g., /workspace/workspace) instead of rejecting the invalid target. Before this change, root-targeted writes were rejected, so this introduces surprising writes and potential data corruption from ambiguous user input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b557c7bc3c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/agents/pi-tools.read.ts
Outdated
| relativePath: sandboxResult.relative, | ||
| }); | ||
| return safeRead.buffer; | ||
| return safeRead.content.toString("utf-8"); |
There was a problem hiding this comment.
safeRead.buffer
In createHostEditOperations (workspace-only mode), readFileWithinRoot returns a SafeLocalReadResult with a buffer field (src/infra/fs-safe.ts), but this code reads safeRead.content.toString("utf-8"). That makes safeRead.content undefined, so any host edit call with tools.fs.workspaceOnly=true fails at runtime before applying changes (typically Cannot read properties of undefined (reading 'toString')).
Useful? React with 👍 / 👎.
YJWell
force-pushed
the
feat/fs-allowed-roots
branch
2 times, most recently
from
78c5350 to
9c2a04b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c2a04ba13
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| root, | ||
| containerWorkdir: options?.containerWorkdir, | ||
| }); | ||
| await assertSandboxPath({ filePath: sandboxPath, cwd: root, root }); | ||
| await assertSandboxPath({ filePath: sandboxPath, cwd: root, root, additionalRoots }); |
There was a problem hiding this comment.
When workspaceOnly is enabled in a sandbox run, this guard now forwards additionalRoots directly to assertSandboxPath, but sandboxPath is only remapped for the container workdir (/workspace) and not for other container mounts. In a common bind-mount case (for example tool path /agent/file.txt with allowedRoots containing the host source path), the check compares a container path against host roots and rejects it before the bridge can resolve it, so tools.fs.allowedRoots does not work for sandbox bind mounts unless callers use host paths instead of container paths.
Useful? React with 👍 / 👎.
| if (params.additionalRoots?.length) { | ||
| for (const extraRoot of params.additionalRoots) { | ||
| const expandedExtra = expandPath(extraRoot); | ||
| const resolvedExtra = path.resolve(expandedExtra); |
There was a problem hiding this comment.
additionalRoots entries are accepted as arbitrary strings and resolved via path.resolve(expandedExtra), which anchors relative values (including "."/"") to the process working directory instead of the sandbox/workspace root. That makes filesystem policy depend on daemon launch cwd and can unintentionally grant access outside the intended workspace when a relative/empty value slips into config.
Useful? React with 👍 / 👎.
|
Rebased onto latest CI status after rebase:
Remaining failures are pre-existing on
None of these failures touch files modified in this PR ( Ready for review when CI stabilizes on |
YJWell
force-pushed
the
feat/fs-allowed-roots
branch
from
9c2a04b to
bb4b097
Compare
Adds tools.fs.allowedRoots - an array of additional allowed directory paths for read/write/edit tools when workspaceOnly is true. This provides fine-grained filesystem access control: agents can access specific directories outside the workspace without disabling workspaceOnly. Key changes: - Config schema: added tools.fs.allowedRoots: string[] (default: []) - resolveSandboxPath: accepts additionalRoots, returns matchedRoot - assertSandboxPath: uses matched root for symlink escape guard - Host write/edit tools: wired to respect allowedRoots - Tests: integration tests for assertSandboxPath with additionalRoots Addresses review findings from PR openclaw#33708: 1. Tilde expansion via expandPath() for allowedRoots entries 2. Correct root passed to assertNoPathAliasEscape 3. Integration tests including symlink escape scenarios Closes openclaw#33635
…tes, drop redundant .optional() - mkdir: use sandboxResult.resolved instead of pre-resolved path (greptile) - access: reuse sandboxResult.resolved for consistency (greptile) - writeFile: use writeFileWithinRoot for TOCTOU-safe pinned writes (codex) - zod schema: remove redundant .optional() after .default([]) (greptile)
- fix(test): add missing allowedRoots to ToolFsPolicy in pdf/image tests - fix(security): use pinned readFileWithinRoot in workspace-only edit mode (TOCTOU) - fix(security): reject read/write when resolved path equals matched root - fix(config): change allowedRoots to optional() so agent-level config inherits global
YJWell
force-pushed
the
feat/fs-allowed-roots
branch
from
bb4b097 to
c27cdcf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c27cdcfb54
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const fsPolicy = createToolFsPolicy({ | ||
| workspaceOnly: isMemoryFlushRun || fsConfig.workspaceOnly, | ||
| allowedRoots: fsConfig.allowedRoots, |
There was a problem hiding this comment.
When tools.fs.workspaceOnly=true and a user adds an extra root (for example /shared) to allowedRoots, read/write/edit now accept that path, but apply_patch still does not: createApplyPatchTool is still created with only { cwd, workspaceOnly }, and src/agents/apply-patch.ts resolves every patch target against options.cwd/assertSandboxPath(... root: options.cwd) with no additional-root override. In practice, a config that allows read /shared/foo.ts will still reject apply_patch on /shared/foo.ts, even though schema.help.ts describes tools.fs as covering apply_patch too.
Useful? React with 👍 / 👎.
| cfg && params.agentId ? resolveAgentConfig(cfg, params.agentId)?.tools?.fs : undefined; | ||
| return { | ||
| workspaceOnly: agentFs?.workspaceOnly ?? globalFs?.workspaceOnly, | ||
| allowedRoots: agentFs?.allowedRoots ?? globalFs?.allowedRoots, |
There was a problem hiding this comment.
This new ToolFsPolicy.allowedRoots is only consumed by the coding read/write/edit wrappers. The other places that already honor tools.fs.workspaceOnly still read just the boolean half of the policy—e.g. prompt-image loading in src/agents/pi-embedded-runner/run/attempt.ts:1262/2504-2511, plus createImageTool, createPdfTool, and createImageGenerateTool only pass workspaceOnly into their local-path guards. With a config such as allowedRoots: ["/shared/media"], the read tool can open /shared/media/doc.pdf, but image, pdf, generated-image reference inputs, and auto-detected prompt images will still reject the same file, so the new setting only works for part of the local-file surface.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2b893c4d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const allowedRoots = options?.allowedRoots ?? []; | ||
| if (options?.workspaceOnly) { | ||
| return workspaceDir ? [workspaceDir] : []; | ||
| return Array.from(new Set([...(workspaceDir ? [workspaceDir] : []), ...allowedRoots])); | ||
| } | ||
| const roots = getDefaultLocalRoots(); | ||
| if (!workspaceDir) { | ||
| return [...roots]; | ||
| } | ||
| return Array.from(new Set([...roots, workspaceDir])); | ||
| return Array.from(new Set([...roots, ...(workspaceDir ? [workspaceDir] : []), ...allowedRoots])); |
There was a problem hiding this comment.
allowedRoots before passing them to media tools
tools.fs.allowedRoots is documented as supporting ~, and assertSandboxPath expands it, but this helper forwards the raw strings into loadWebMedia's localRoots checks. In extensions/whatsapp/src/media.ts:117-123, those roots are only realpath/path.resolved, so an entry like "~/projects" becomes <cwd>/~/projects instead of $HOME/projects. As a result, image, pdf, image_generate, and non-sandbox prompt-image loading still reject files under home-directory allowlists even though read/write/edit/apply_patch accept the same config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5a466df. resolveMediaToolLocalRoots() now normalizes media allowedRoots with ~ expansion before passing them to loadWebMedia local-root checks, so media tools match the documented filesystem-tool behavior.
| const roots = getDefaultLocalRoots(); | ||
| if (!workspaceDir) { | ||
| return [...roots]; | ||
| } | ||
| return Array.from(new Set([...roots, workspaceDir])); | ||
| return Array.from(new Set([...roots, ...(workspaceDir ? [workspaceDir] : []), ...allowedRoots])); |
There was a problem hiding this comment.
allowedRoots gated on workspaceOnly
The schema/help text says tools.fs.allowedRoots only applies "when workspaceOnly is true", but this branch appends those paths even when workspaceOnly is false. That means simply pre-populating allowedRoots now widens image/pdf/image_generate and prompt-image access immediately, which is a behavior change for deployments that leave tools.fs.workspaceOnly off (for example when read is denied but media tools stay enabled).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5a466df. Media allowedRoots are now only appended when workspaceOnly === true; unrestricted media mode keeps the default roots + workspace and no longer widens access just because allowedRoots is configured.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a466df569
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/agents/apply-patch.ts
Outdated
| await fs.mkdir(path.dirname(sandboxResult.resolved), { recursive: true }); | ||
| await fs.writeFile(sandboxResult.resolved, content, "utf8"); |
There was a problem hiding this comment.
When apply_patch writes an added/updated/moved file under workspaceOnly, it now validates the path with assertSandboxPath() and then performs separate fs.mkdir()/fs.writeFile() calls. That reintroduces the same TOCTOU alias race the rest of the FS stack avoids: a concurrent local process can swap the validated path or one of its parents to a symlink/hardlink between the check and the write, redirecting the patch outside the allowed root. The previous writeFileWithinRoot flow pinned the target specifically to prevent this class of escape.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2c86b2f. Workspace-only apply_patch writes now go back through writeFileWithinRoot(...) (pinned write helper) instead of fs.mkdir() + fs.writeFile(), and the host-side mkdirp path no longer creates directories eagerly before the pinned write runs.
| const expandedExtra = expandPath(extraRoot); | ||
| const resolvedExtra = path.resolve(expandedExtra); |
There was a problem hiding this comment.
~\ allowedRoots before matching on Windows
The new allowedRoots matching reuses expandPath(), but that helper only expands "~/" and leaves Windows-style home prefixes like "~\\shared" untouched. On Windows hosts, a config such as tools.fs.allowedRoots: ["~\\projects"] therefore resolves against the process CWD instead of the user’s home directory, so paths under %USERPROFILE%\projects are still rejected even though the feature is documented as supporting tilde expansion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2c86b2f. expandPath() now expands both ~/... and Windows-style ~\\..., so allowedRoots home prefixes resolve consistently on Windows too.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c86b2f220
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (trimmed.startsWith("~/") || trimmed.startsWith("~\\")) { | ||
| return path.join(os.homedir(), trimmed.slice(2)); | ||
| } | ||
| return path.resolve(trimmed); |
There was a problem hiding this comment.
@ prefixes from media allowedRoots
Filesystem-tool allowlists already go through expandPath() in src/agents/sandbox-paths.ts, which treats @/foo the same as /foo, but this media-specific normalizer falls through to path.resolve(trimmed). With a config like tools.fs.allowedRoots: ["@/Users/me/media"], read/write/edit/apply_patch accept files under /Users/me/media, while image, pdf, image_generate, and prompt-image loading still reject the same paths because they receive <cwd>/@/Users/... as their local root.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 779b951. Media allowedRoots now strips a leading @ before normalization, so paths like @/Users/me/media resolve the same way as the filesystem-tool path policy instead of becoming <cwd>/@/Users/....
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 779b951513
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/agents/pi-tools.read.ts
Outdated
| await assertSandboxPath({ filePath: sandboxPath, cwd: root, root, additionalRoots }); | ||
| } | ||
| return tool.execute(toolCallId, normalized ?? args, signal, onUpdate); |
There was a problem hiding this comment.
~\\ paths before calling the wrapped FS tool
On Windows, this wrapper now validates path with assertSandboxPath(), which expands ~\\... and makes configs like tools.fs.allowedRoots: ["~\\projects"] look supported, but it still forwards the original argument unchanged to tool.execute(). The wrapped read/write/edit tools come from pi-coding-agent, whose path resolver only expands ~/, so a call like read {path: "~\\projects\\note.txt"} passes the guard and then resolves under the workspace instead of %USERPROFILE%\\projects. In practice, the new Windows-style home-prefix support only works if callers manually expand the path first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c71d526. The workspace-root wrapper now normalizes ~, ~/..., ~\\..., and @-prefixed paths before forwarding them to the wrapped read/write/edit tool, so Windows-style home-prefix inputs that pass the guard also resolve correctly inside the underlying FS tool.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c71d526059
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| workspaceOnly: applyPatchWorkspaceOnly, | ||
| allowedRoots, |
There was a problem hiding this comment.
Passing allowedRoots into createApplyPatchTool() here widens apply_patch even when tools.fs.workspaceOnly is still off. The new config help/types (src/config/schema.help.ts:653-654, src/config/types.tools.ts:284-288) say tools.fs.allowedRoots only applies when the FS workspace guard is enabled, but applyPatchWorkspaceOnly is true by default via tools.exec.applyPatch.workspaceOnly, so a config that only sets tools.fs.allowedRoots will suddenly let apply_patch modify those extra directories. That is an undocumented access change for apply_patch-only setups.
Useful? React with 👍 / 👎.
2 participants
Summary
Implements
tools.fs.allowedRoots— an array of additional allowed path prefixes for read/write/edit tools.Closes #33635
Changes
tools.fs.allowedRoots: string[](default:[])resolveSandboxPath: acceptsadditionalRoots, returnsmatchedRootassertSandboxPath: uses matched root for symlink escape guardPrevious attempt
PR #33708 was closed due to bugs found in review. This PR addresses all review findings:
expandPath()(was missing)assertNoPathAliasEscape(was using workspace root for all)assertSandboxPathincluding symlink escape scenariosConfig example
{ "tools": { "fs": { "workspaceOnly": true, "allowedRoots": ["~/projects", "/shared/data"] } } }