Greptile Summary

This PR removes the external trash command invocation from movePathToTrash() (both in extensions/browser/src/browser/trash.ts and src/plugin-sdk/browser-maintenance.ts), replacing it with direct fs.renameSync + EXDEV-aware cpSync/rmSync fallback, and adds workspace-derived PATH filtering in service-env.ts and daemon-install-helpers.ts to prevent install-time env vars from persisting attacker-controlled bin directories into the service PATH. Previously flagged issues (EXDEV cross-device handling, timestamp reuse, and path.posix vs native path inconsistency) have all been addressed in the follow-up commit.

Confidence Score: 5/5

This PR is safe to merge — the security fixes are correct, prior review findings have been addressed, and the new implementation is well-tested.

No P0 or P1 issues remain. The workspace-derived PATH filtering logic correctly handles normalization, /proc/self/cwd prefix matching, symlink resolution via realpathSync.native, and the cwd === HOME exception. The movePathToDestination helper correctly handles EXDEV cross-device errors, TOCTOU collision codes, and the retry loop preserves the timestamp. All previously flagged issues were addressed in the follow-up commit.

No files require special attention.

_{Reviews (2): Last reviewed commit: "fix: address review-pr skill feedback" | Re-trigger Greptile}

@greptile review

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep this PR open. The cleanup policy blocks automated closure because the provided GitHub context lists the author association as MEMBER and the PR has the protected maintainer label. Current main also still contains the security-sensitive behavior the PR is trying to replace, so the PR is not obsolete or implemented on main.

Best possible solution:

Keep this PR open for explicit maintainer/security review. If accepted, land this PR or an equivalent focused patch that removes PATH-resolved trash execution from both cleanup helpers, filters workspace-derived service PATH entries with proc/cwd/realpath coverage, reconciles the remaining filesystem-safety review points, and keeps changelog/docs aligned with the final behavior.

What I checked:

• Protected maintainer PR: Provided GitHub context shows this PR is open, unmerged, authored by a MEMBER, and labeled maintainer; repository cleanup instructions require explicit maintainer handling for either protected condition. (e5e3a275feea)
• Current main checked: Local checkout HEAD matches the provided current main SHA. (b79e617ad12c)
• Browser trash helper still executes PATH-resolved command: movePathToTrash imports runExec and calls runExec("trash", [targetPath], { timeoutMs: 10_000 }) before falling back to ~/.Trash. (extensions/browser/src/browser/trash.ts:5, b79e617ad12c)
• SDK maintenance helper still executes PATH-resolved command: The SDK helper lazily loads the exec runtime and calls runExec("trash", [targetPath], { timeoutMs: 10_000 }) inside movePathToTrash. (src/plugin-sdk/browser-maintenance.ts:62, b79e617ad12c)
• Agent deletion reaches trash cleanup: Agent deletion calls moveToTrashBestEffort for workspace, agent, and sessions directories; that helper calls movePathToTrash(pathname). (src/gateway/server-methods/agents.ts:654, b79e617ad12c)
• Service env still includes env-configured toolchain roots: Current service PATH construction adds PNPM_HOME, NPM_CONFIG_PREFIX/bin, BUN_INSTALL/bin, VOLTA_HOME/bin, ASDF_DATA_DIR/shims, and NIX_PROFILES bins without the PR's cwd/proc/realpath workspace-derived filtering. (src/daemon/service-env.ts:105, b79e617ad12c)

Remaining risk / open question:

• Closing this PR would bypass the explicit cleanup protections for maintainer-owned and maintainer-labeled work.
• Closing this PR now would leave current main with PATH-resolved trash execution reachable from maintenance cleanup paths.
• The PR changes security-sensitive filesystem cleanup and service PATH persistence; it needs normal maintainer/security review before merge or rejection.

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

Addressed in 86ce4dd43b908b5f48f2fda609d54e227e21c17c.

Quoted comment from @aisle-research-bot:

🔒 Aisle Security Analysis

We found 3 potential security issue(s) in this PR:

#	Severity	Title
1	🟠 High	Trash destination path escape via absolute basename (root path)
2	🟠 High	Trash destination path escape via absolute basename (root path)
3	🟡 Medium	TOCTOU and potential overwrite via fs.renameSync when moving paths into ~/.Trash

1. 🟠 Trash destination path escape via absolute basename (root path)

Property	Value
Severity	High
CWE	CWE-22
Location	extensions/browser/src/browser/trash.ts:51-55

Description

movePathToTrash() builds the trash destination using path.join(trashDir, ${base}-${timestamp}) where base = path.basename(targetPath).

On POSIX, path.basename("/") === "/" (and for other root-like paths, base can start with a path separator). When the second argument to path.join() is absolute, Node ignores the first argument, so the computed destination can escape trashDir and become an absolute path (e.g. "/-123").

Impact:

• If a caller ever passes a root path (e.g. "/") or another value that yields an absolute base, the function may renameSync/cpSync/rmSync using a destination outside of ~/.Trash.
• This defeats the intended containment of cleanup operations to the user trash directory and can lead to unintended writes/removals in arbitrary filesystem locations (subject to process permissions).

Vulnerable code:

const base = path.basename(targetPath);
const timestamp = Date.now();
const baseDest = path.join(trashDir, `${base}-${timestamp}`);

Recommendation

Harden destination construction to guarantee it stays within trashDir.

Recommended approach:

1. Reject root paths / root basenames.
2. Resolve the computed destination and enforce it is inside trashDir.

Example:

const trashDir = path.join(os.homedir(), ".Trash");
fs.mkdirSync(trashDir, { recursive: true });

const parsed = path.parse(targetPath);
if (targetPath === parsed.root) {
  throw new Error(`Refusing to trash root path: ${targetPath}`);
}

const base = path.basename(targetPath);
​// Defensive: prevent absolute/drive-root-like basename from escaping join
const safeLeaf = base.replace(/[\\/]+/g, "");
if (!safeLeaf) {
  throw new Error(`Unable to derive safe trash basename for: ${targetPath}`);
}

const dest = path.resolve(trashDir, `${safeLeaf}-${Date.now()}`);
const trashRoot = path.resolve(trashDir) + path.sep;
if (!dest.startsWith(trashRoot)) {
  throw new Error(`Trash destination escaped trashDir: ${dest}`);
}

Apply the same check to all retry destinations as well.

2. 🟠 Trash destination path escape via absolute basename (root path)

Property	Value
Severity	High
CWE	CWE-22
Location	src/plugin-sdk/browser-maintenance.ts:99-104

Description

movePathToTrash() in the plugin SDK constructs the destination with path.join(trashDir, ${base}-${timestamp}) and base = path.basename(targetPath).

If targetPath is a filesystem root (or otherwise yields an absolute base), the joined path can escape trashDir because path.join(a, absoluteB) returns absoluteB.

This can cause trash operations to write outside of the intended ~/.Trash directory and then remove the original path via rmSync (copy+remove fallback), subject to process permissions.

Vulnerable code:

const base = path.basename(targetPath);
const timestamp = Date.now();
const baseDest = path.join(trashDir, `${base}-${timestamp}`);

Recommendation

Ensure the computed trash destination cannot become absolute / escape trashDir.

For example, reject root targets and validate containment:

const parsed = path.parse(targetPath);
if (targetPath === parsed.root) {
  throw new Error(`Refusing to trash root path: ${targetPath}`);
}

const base = path.basename(targetPath).replace(/[\\/]+/g, "");
if (!base) throw new Error(`Unable to derive safe basename for: ${targetPath}`);

const dest = path.resolve(trashDir, `${base}-${Date.now()}`);
if (!dest.startsWith(path.resolve(trashDir) + path.sep)) {
  throw new Error(`Trash destination escaped trashDir: ${dest}`);
}

Apply the same validation to all retry destinations.

3. 🟡 TOCTOU and potential overwrite via fs.renameSync when moving paths into ~/.Trash

Property	Value
Severity	Medium
CWE	CWE-367
Location	src/plugin-sdk/browser-maintenance.ts:53-56

Description

movePathToTrash attempts to avoid destination collisions by checking fs.existsSync(dest) and retrying with a random suffix. However, the actual move uses fs.renameSync(targetPath, dest).

On POSIX platforms, rename(2) (and Node’s fs.renameSync) can replace an existing destination file atomically, rather than failing with EEXIST. This means a concurrently-created dest can still be overwritten after the existsSync check, defeating the collision/uniqueness logic.

Impact depends on runtime context, but if the process has elevated privileges (or runs under a different user than the one controlling $HOME/.Trash), this becomes a destructive overwrite primitive:

• TOCTOU window: existsSync(dest) check occurs before renameSync.
• Overwrite sink: renameSync may overwrite an attacker-created file at dest.
• Symlink risk: trashDir is derived from os.homedir() and created with mkdirSync without verifying it is not a symlink; if ~/.Trash is a symlink to another directory, moves can be redirected.

Vulnerable code:

const baseDest = path.join(trashDir, `${base}-${timestamp}`);
if (!fs.existsSync(baseDest) && movePathToDestination(targetPath, baseDest)) {
  return baseDest;
}

​// ...

fs.renameSync(targetPath, dest);

The same pattern exists in both the browser extension and plugin-sdk implementations.

Recommendation

Avoid relying on existsSync + rename for uniqueness/collision avoidance.

Use an O_EXCL-style reservation for the destination name (or use a uniquely created directory) so that the destination cannot be swapped/created between check and move:

Option A (recommended): create a unique directory with mkdtemp and move into it:

const trashDir = path.join(os.homedir(), ".Trash");
fs.mkdirSync(trashDir, { recursive: true });

const container = fs.mkdtempSync(path.join(trashDir, `${base}-`));
const dest = path.join(container, base);
fs.renameSync(targetPath, dest);

Option B: reserve the exact destination path using openSync(dest, 'wx') (or equivalent) to ensure exclusivity, then move into it (note: still be careful with directories and symlinks).

Additionally, harden trashDir handling:

• lstatSync(trashDir) and reject if it is a symlink
• Compare realpathSync(trashDir) to an expected location under realpathSync(os.homedir()) to prevent redirection

---

Analyzed PR: #73264 at commit 71e356f

_{Last updated on: 2026-04-28T10:57:57Z}

Addressed in 86ce4dd43b908b5f48f2fda609d54e227e21c17c.

Quoted comment from @clawsweeper:

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep open. This PR is protected by both author association and the maintainer label, and current origin/main still contains the behavior the PR is trying to harden: both trash helpers call a bare trash command via runExec, and service PATH construction still preserves env-derived/toolchain PATH entries without the PR's workspace-derived filtering.

Best possible solution:

Keep the PR open for explicit maintainer/security review. If accepted, land this PR or an equivalent focused patch that removes PATH-resolved trash execution from both cleanup helpers, filters workspace-derived service PATH entries with realpath-aware coverage, and drops unrelated UI churn.

What I checked:

• Protected maintainer item: The provided GitHub context shows the PR author association is MEMBER and labels include maintainer; the cleanup policy requires explicit maintainer judgment for either condition.
• Browser plugin still executes PATH-resolved trash: Current main imports runExec and calls runExec("trash", [targetPath], { timeoutMs: 10_000 }) before falling back to ~/.Trash. (extensions/browser/src/browser/trash.ts:5, 2a0af6754e2b)
• SDK maintenance helper still executes PATH-resolved trash: Current main dynamically loads the exec runtime and calls runExec("trash", ...) inside movePathToTrash; agent deletion reaches this helper through moveToTrashBestEffort. (src/plugin-sdk/browser-maintenance.ts:62, 2a0af6754e2b)
• Agent deletion uses the SDK trash helper: The gateway server method calls movePathToTrash(pathname) during best-effort cleanup, matching the PR body's admin-maintenance trigger path. (src/gateway/server-methods/agents.ts:265, 2a0af6754e2b)
• Service env still includes env-configured toolchain bins: Current main adds PNPM_HOME, NPM_CONFIG_PREFIX/bin, BUN_INSTALL/bin, VOLTA_HOME/bin, ASDF_DATA_DIR/shims, and NIX_PROFILES profile bins without the PR's cwd/workspace-derived exclusion. (src/daemon/service-env.ts:105, 2a0af6754e2b)
• Existing PATH merge lacks workspace filtering: mergeServicePath on current main filters only relative paths and paths under temp dirs; it has no /proc/self/cwd, install-cwd, or realpath workspace exclusion like the PR proposes. (src/commands/daemon-install-helpers.ts:171, 2a0af6754e2b)

Remaining risk / open question:

• Closing this PR would leave the reported hardening unmerged on current main: PATH-resolved trash execution and permissive service PATH preservation are still present.
• Because the PR is security-sensitive and maintainer-labeled, the remaining review should explicitly reconcile the bot-raised canonicalization, symlink, realpath, and TOCTOU points before landing.
• The PR includes a small unrelated UI import-order change; it should be removed or justified during normal review so the security patch stays focused.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 2a0af6754e2b.

Addressed in e330db40ff3cf639401f80e7d3e450fb8f30729b.

Quoted comment from @aisle-research-bot:

🔒 Aisle Security Analysis

We found 5 potential security issue(s) in this PR:

#	Severity	Title
1	🟠 High	Workspace-derived PATH filtering bypass via /proc//cwd paths when target directory does not exist
2	🟠 High	Incomplete filtering of proc-based CWD symlinks allows untrusted PATH segments to persist
3	🟠 High	Arbitrary directory deletion via movePathToTrash() EXDEV fallback (cpSync + rmSync) when targetPath is attacker-controlled
4	🟡 Medium	TOCTOU symlink race when creating/using per-user trash directory
5	🟡 Medium	TOCTOU symlink race when creating/using per-user trash directory (plugin-sdk)

1. 🟠 Workspace-derived PATH filtering bypass via /proc//cwd paths when target directory does not exist

Property	Value
Severity	High
CWE	CWE-428
Location	src/daemon/service-env.ts:92-115

Description

isWorkspaceDerivedPath() is intended to prevent install-time, workspace-derived toolchain paths from being persisted into the daemon's minimal PATH. However, it only string-matches the literal prefix /proc/self/cwd and otherwise relies on realpathSync of the full candidate directory.

On Linux, an attacker (or a poisoned install environment) can set env-configured toolchain roots (e.g., NPM_CONFIG_PREFIX, PNPM_HOME, BUN_INSTALL, NIX_PROFILES, etc.) to a workspace-derived path via another procfs cwd symlink such as /proc/<installer_pid>/cwd/....

Because realpathServicePathDir() uses fs.realpathSync.native(dir) on the full path, it fails (returns undefined) if the final directory does not exist at install time. In that case, the function returns false and the path is not filtered, but it is still persisted into the daemon environment. The directory can then be created later inside the workspace, allowing workspace binaries to be found/executed by the daemon.

Vulnerable code:

if (isSameOrChildPath(dir, "/proc/self/cwd")) {
  return true;
}
...
const realDir = realpathServicePathDir(dir);
const realCwd = realpathServicePathDir(cwd);
...
return Boolean(realDir && realCwd && ... && isSameOrChildPath(realDir, realCwd));

This leaves a gap for procfs cwd symlink variants (e.g., /proc/12345/cwd) and for non-existent target subdirectories where realpathSync cannot resolve the path.

Recommendation

Harden workspace-derived path detection so it cannot be bypassed by procfs symlink variants or by paths that don't exist yet.

Suggested changes:

1. Treat any procfs cwd symlink as workspace-derived (at least /proc/self/cwd and /proc/<pid>/cwd):

const procCwdRe = /^\/proc\/(?:self|\d+)\/cwd(?:\/|$)/;
if (procCwdRe.test(dir)) return true;

2. Avoid relying on realpathSync of the full candidate path. If you want symlink-aware checks even when the final directory doesn't exist, resolve the existing parent (or use fs.realpathSync on the longest existing prefix) and then compare that against the real CWD.

3. Consider rejecting any env-configured bin roots that are not verifiably outside the install workspace (fail-closed for these specific toolchain env vars), or require existence checks before persisting them.

These changes prevent durable service PATH entries from pointing into the install workspace via non-existent or procfs-derived paths.

2. 🟠 Incomplete filtering of proc-based CWD symlinks allows untrusted PATH segments to persist

Property	Value
Severity	High
CWE	CWE-426
Location	src/commands/daemon-install-helpers.ts:192-215

Description

mergeServicePath() attempts to prevent PATH poisoning by dropping preserved PATH segments that reference the install working directory (CWD) or /proc/self/cwd.

However, the protection is incomplete:

• Only the literal prefix /proc/self/cwd is blocked (/proc/<pid>/cwd, /proc/thread-self/cwd, or /proc/self/root/... are not).
• The additional symlink-to-CWD protection relies on realpathSync.native(). If it throws (e.g., /proc not mounted/accessible at install time, or the path temporarily missing), the code falls back to preserving the segment.
• A preserved proc-based path (e.g., /proc/thread-self/cwd/evil-bin) can later resolve to the daemon’s working directory at runtime, allowing attacker-controlled executables to be found earlier on PATH.

Vulnerable code:

const procSelfCwd = path.normalize("/proc/self/cwd");
if (isSameOrChildPath(normalized, procSelfCwd)) {
  return undefined;
}
...
try {
  const realSegment = path.normalize(fs.realpathSync.native(normalized));
  const realCwd = path.normalize(fs.realpathSync.native(cwd));
  if (isSameOrChildPath(realSegment, realCwd)) {
    return undefined;
  }
} catch {
  ​// Legacy PATH entries may no longer exist; keep filtering best-effort.
}
return normalized;

Recommendation

Treat proc-based paths (and other pseudo-filesystem symlink patterns) as unsafe without relying on realpath() success.

Suggested hardening:

1. Reject any preserved PATH segment under /proc entirely (Linux-only), or at least reject known cwd symlink forms:

   • /proc/self/cwd
   • /proc/thread-self/cwd
   • /proc/<digits>/cwd
   • /proc/self/root (and descendants)

2. If realpathSync.native() throws for a preserved segment, conservatively drop it when it is within /proc or contains other known indirections.

Example:

const isProcPath = (p: string) => path.normalize(p).startsWith(`${path.sep}proc${path.sep}`);

const normalizePreservedPathSegment = (segment: string): string | undefined => {
  if (!path.isAbsolute(segment)) return undefined;
  const normalized = path.normalize(segment);

  if (isProcPath(normalized)) return undefined; // conservative

  const cwd = path.resolve(process.cwd());
  if (isSameOrChildPath(normalized, cwd)) return undefined;

  try {
    const realSegment = path.normalize(fs.realpathSync.native(normalized));
    const realCwd = path.normalize(fs.realpathSync.native(cwd));
    if (isSameOrChildPath(realSegment, realCwd)) return undefined;
  } catch {
    ​// for preserved segments, fail closed when uncertain
    return undefined;
  }

  return normalized;
};

This prevents runtime-only resolution from reintroducing workspace-controlled PATH entries (CWE-426).

3. 🟠 Arbitrary directory deletion via movePathToTrash() EXDEV fallback (cpSync + rmSync) when targetPath is attacker-controlled

Property	Value
Severity	High
CWE	CWE-73
Location	src/plugin-sdk/browser-maintenance.ts:120-123

Description

movePathToTrash() now falls back to a cross-device implementation that recursively copies the source to ~/.Trash and then recursively deletes the original path.

If a caller can influence targetPath, this becomes an arbitrary directory deletion primitive under the service’s privileges:

• fs.cpSync(targetPath, dest, { recursive: true, ... }) copies attacker-chosen content
• fs.rmSync(targetPath, { recursive: true, force: true }) then deletes the attacker-chosen path
• Only filesystem roots (e.g. /) are blocked; other sensitive directories (/etc, /home, application state dirs, mounted volumes, etc.) are not

This is reachable from server-side request handlers such as agents.delete, which resolves paths from configuration (workspace, agentDir, etc.) without constraining them to an application-owned directory before calling movePathToTrash() best-effort.

Vulnerable code (sink):

fs.cpSync(targetPath, dest, { recursive: true, force: false, errorOnExist: true });
fs.rmSync(targetPath, { recursive: true, force: true });

Recommendation

Constrain targetPath to a known-safe root (application-owned directories) before invoking deletion/move operations.

Recommended defense-in-depth:

1. Add an allowlist root check in movePathToTrash() (or in all call sites) to ensure the path is inside an expected directory (e.g. the OpenClaw state dir / workspace root).
2. Refuse to operate on common sensitive directories (e.g. /etc, /home, /usr, Windows drive roots) and on paths that resolve outside the allowlisted root after realpath.
3. Avoid force:true for recursive deletion unless strictly necessary; surface errors instead.

Example (call-site guard):

import path from "node:path";
import fs from "node:fs";

function assertInsideRoot(targetPath: string, rootDir: string) {
  const rootReal = fs.realpathSync.native(rootDir);
  const targetReal = fs.realpathSync.native(targetPath);
  const rootResolved = path.resolve(rootReal);
  const targetResolved = path.resolve(targetReal);
  if (targetResolved === rootResolved || !targetResolved.startsWith(rootResolved + path.sep)) {
    throw new Error(`Refusing to trash path outside allowed root: ${targetPath}`);
  }
}

assertInsideRoot(targetPath, allowedWorkspaceRoot);
await movePathToTrash(targetPath);

This ensures only application-owned data can be removed, preventing arbitrary deletion if an attacker can affect configuration or request parameters.

4. 🟡 TOCTOU symlink race when creating/using per-user trash directory

Property	Value
Severity	Medium
CWE	CWE-367
Location	extensions/browser/src/browser/trash.ts:25-70

Description

The per-user trash implementation performs a one-time symlink check on ~/.Trash, but then later creates temporary directories and moves/copies files by path, allowing a local attacker who can modify the filesystem to swap ~/.Trash (or its parent components) to a symlink between the check and later filesystem operations.

In resolveTrashDir():

• fs.mkdirSync(trashDir, { recursive: true }) creates/ensures the directory
• fs.lstatSync(trashDir).isSymbolicLink() checks for symlink only at that moment
• later, reserveTrashDestination() calls fs.mkdtempSync(containerPrefix) where containerPrefix is under trashDir

Because mkdtempSync, renameSync, and cpSync follow symlinks in path components, a race can redirect operations to an attacker-chosen location (e.g., if ~/.Trash is replaced with a symlink to another directory). Impact depends on the process privileges; if this code runs in a service/daemon context with higher privileges than the attacker, it can enable writing/moving data outside the intended home trash location.

Vulnerable flow:

• input: filesystem state (attacker can replace ~/.Trash with symlink)
• check: lstatSync at one time
• sink: mkdtempSync/renameSync/cpSync later use trashDir again by path

Recommendation

Make all subsequent operations use a stable, symlink-free real path and (ideally) enforce safe permissions.

Minimum hardening:

1. After creating and validating trashDir, resolve it once with realpathSync and return that real path.
2. Use the returned real path for all later operations (mkdtempSync, renameSync, cpSync).
3. Optionally verify ownership and permissions (e.g., mode 0700) and reject if group/world writable.

Example:

function resolveTrashDir(): string {
  const homeDir = os.homedir();
  const trashDir = path.join(homeDir, ".Trash");

  fs.mkdirSync(trashDir, { recursive: true, mode: 0o700 });
  const lst = fs.lstatSync(trashDir);
  if (!lst.isDirectory() || lst.isSymbolicLink()) {
    throw new Error(`Refusing to use non-directory/symlink trash directory: ${trashDir}`);
  }

  const realHome = path.resolve(fs.realpathSync.native(homeDir));
  const realTrashDir = path.resolve(fs.realpathSync.native(trashDir));
  if (realTrashDir === realHome || !realTrashDir.startsWith(realHome + path.sep)) {
    throw new Error(`Trash directory escaped home directory: ${trashDir}`);
  }

  return realTrashDir; // use stable path going forward
}

For stronger guarantees on Unix, consider directory-fd based operations (open with O_DIRECTORY|O_NOFOLLOW plus *at() syscalls) where available to fully eliminate TOCTOU.

5. 🟡 TOCTOU symlink race when creating/using per-user trash directory (plugin-sdk)

Property	Value
Severity	Medium
CWE	CWE-367
Location	src/plugin-sdk/browser-maintenance.ts:50-110

Description

movePathToTrash() in the plugin SDK has the same TOCTOU/symlink race pattern as the browser extension trash implementation.

resolveTrashDir() ensures and checks ~/.Trash once, but later uses trashDir again in reserveTrashDestination() to create a temporary directory via fs.mkdtempSync() and then moves/copies into it. A local attacker who can modify the filesystem can race-replace ~/.Trash with a symlink after the lstatSync check but before mkdtempSync/renameSync/cpSync, redirecting the trash destination.

This is more concerning here because the function is in src/plugin-sdk/ and is used by server-side code (e.g., gateway handlers) where runtime privileges may differ from the attacker’s.

Vulnerable behavior:

• check: lstatSync(trashDir).isSymbolicLink()
• later sinks: mkdtempSync(containerPrefix), renameSync(targetPath, dest), and fallback cpSync(...); rmSync(...) all follow symlinks in path components.

Recommendation

Resolve and use a stable real path for the trash directory for the remainder of the operation, and consider enforcing safe permissions.

function resolveTrashDir(): string {
  const homeDir = os.homedir();
  const trashDir = path.join(homeDir, ".Trash");
  fs.mkdirSync(trashDir, { recursive: true, mode: 0o700 });

  const lst = fs.lstatSync(trashDir);
  if (!lst.isDirectory() || lst.isSymbolicLink()) {
    throw new Error(`Refusing to use symlinked/non-directory trash directory: ${trashDir}`);
  }

  const realHome = path.resolve(fs.realpathSync.native(homeDir));
  const realTrashDir = path.resolve(fs.realpathSync.native(trashDir));
  if (realTrashDir === realHome || !realTrashDir.startsWith(realHome + path.sep)) {
    throw new Error(`Trash directory escaped home directory: ${trashDir}`);
  }

  return realTrashDir;
}

Then ensure reserveTrashDestination() and all subsequent operations use the returned realTrashDir. For best-in-class protection on Unix, use directory-fd based APIs (open with O_NOFOLLOW + *at syscalls) to eliminate TOCTOU windows entirely.

---

Analyzed PR: #73264 at commit 6c74ae5

_{Last updated on: 2026-04-28T12:09:40Z}

Addressed in e330db40ff3cf639401f80e7d3e450fb8f30729b.

Quoted comment from @clawsweeper:

Codex review: keeping this open for maintainer follow-up; there is still a little grit to resolve.

Keep open. This PR is protected by both the maintainer label and a MEMBER author association, and current main still contains the PATH-resolved trash execution plus permissive service PATH preservation the PR is trying to harden. It remains a live security-sensitive implementation candidate, not obsolete cleanup.

Best possible solution:

Keep this PR open for explicit maintainer/security review. If accepted, land this PR or an equivalent focused patch that removes PATH-resolved trash execution from both cleanup helpers, filters workspace-derived service PATH entries with realpath-aware coverage, addresses the remaining filesystem-safety review points, trims unrelated UI churn, and updates the Nix service PATH docs if the final behavior changes.

What I checked:

• Protected PR metadata: The provided GitHub context lists authorAssociation MEMBER and labels including maintainer; repository cleanup policy requires explicit maintainer judgment for either condition.
• Browser trash helper still resolves a bare command on main: Current main imports runExec and calls runExec("trash", [targetPath], { timeoutMs: 10_000 }) before falling back to ~/.Trash. (extensions/browser/src/browser/trash.ts:5, e4ff7c162044)
• SDK trash helper still resolves a bare command on main: The SDK maintenance helper lazily loads the exec runtime, then calls runExec("trash", ...) inside movePathToTrash. (src/plugin-sdk/browser-maintenance.ts:62, e4ff7c162044)
• Agent deletion reaches the trash helper: Gateway agent deletion resolves workspace, agent, and session directories, then calls moveToTrashBestEffort, which calls movePathToTrash; this matches the PR body's admin-maintenance trigger path. (src/gateway/server-methods/agents.ts:654, e4ff7c162044)
• Service env still preserves env-configured toolchain roots: Current service PATH construction adds PNPM_HOME, NPM_CONFIG_PREFIX/bin, BUN_INSTALL/bin, VOLTA_HOME/bin, ASDF_DATA_DIR/shims, and NIX_PROFILES profile bins without a cwd, /proc/self/cwd, or realpath workspace-derived exclusion. (src/daemon/service-env.ts:105, e4ff7c162044)
• Existing PATH merge lacks workspace-derived filtering: mergeServicePath only rejects non-absolute preserved segments and paths under tmp roots; it does not reject /proc/self/cwd, the install cwd, or symlinks resolving into the workspace. (src/commands/daemon-install-helpers.ts:171, e4ff7c162044)

Remaining risk / open question:

• Closing this PR now would leave current main with PATH-resolved trash execution reachable from maintenance cleanup paths.
• The proposed patch changes security-sensitive filesystem behavior, including recursive copy/remove fallback paths, so maintainer/security review should reconcile the bot-raised containment and TOCTOU concerns before merge.
• If workspace-derived NIX_PROFILES entries are filtered on landing, docs that currently say every NIX_PROFILES entry is added to the service PATH may need a small clarification.
• The one-line ui/src/ui/app-view-state.ts import-order change appears unrelated to the security fix and should be removed or justified before landing.

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