From 849077ecb57a2232cfbace2ba6c0d320dc1c607b Mon Sep 17 00:00:00 2001 From: subrih Date: Fri, 13 Mar 2026 23:14:36 -0700 Subject: [PATCH] fix(access-policy): resolveArgv0 -S= and -SVAL forms, depth cap, bwrap reset export, npmrc comment --- src/infra/access-policy.test.ts | 33 ++++++++++++++++++++++ src/infra/access-policy.ts | 42 ++++++++++++++++++++-------- src/infra/exec-sandbox-bwrap.test.ts | 18 ++++++++++++ src/infra/exec-sandbox-bwrap.ts | 4 +++ 4 files changed, 86 insertions(+), 11 deletions(-) diff --git a/src/infra/access-policy.test.ts b/src/infra/access-policy.test.ts index 0fb47be1b90..e8876bbfebf 100644 --- a/src/infra/access-policy.test.ts +++ b/src/infra/access-policy.test.ts @@ -911,6 +911,39 @@ describe("resolveArgv0", () => { expect(result).not.toBeNull(); expect(result).toMatch(/sh$/); }); + + itUnix("recurses into env --split-string=VALUE equals form", () => { + // --split-string=CMD (equals form) was previously not handled — resolveArgv0 + // returned null, causing the fallback to treat "env" as argv0 and silently + // bypass tool-layer hash/policy checks for the embedded script. + const result = resolveArgv0("env --split-string='/bin/sh -c echo'"); + expect(result).not.toBeNull(); + expect(result).toMatch(/sh$/); + }); + + itUnix("recurses into env -S=VALUE equals form (short flag with equals)", () => { + const result = resolveArgv0("env -S='/bin/sh -c echo'"); + expect(result).not.toBeNull(); + expect(result).toMatch(/sh$/); + }); + + itUnix("recurses into env -SVALUE compact form (no space, no equals)", () => { + const result = resolveArgv0("env -S'/bin/sh -c echo'"); + expect(result).not.toBeNull(); + expect(result).toMatch(/sh$/); + }); + + it("returns null for deeply nested env -S to prevent stack overflow", () => { + // Build a deeply nested "env -S 'env -S ...' " string beyond the depth cap (8). + let cmd = "/bin/sh"; + for (let i = 0; i < 10; i++) { + cmd = `env -S '${cmd}'`; + } + // Should not throw; depth cap returns null before stack overflow. + expect(() => resolveArgv0(cmd)).not.toThrow(); + // Result may be null (cap hit) or a resolved path — either is acceptable. + // The important invariant is: no RangeError. + }); }); // --------------------------------------------------------------------------- diff --git a/src/infra/access-policy.ts b/src/infra/access-policy.ts index 888244bafa1..c4ddcde9a28 100644 --- a/src/infra/access-policy.ts +++ b/src/infra/access-policy.ts @@ -168,7 +168,9 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] .split(/[/\\]/) .pop() ?? ""; // Looks like a file when the last segment has a non-leading dot followed by a - // letter-containing extension — covers "secrets.key", "config.json", ".npmrc". + // letter-containing extension — covers "secrets.key", "config.json". + // Note: pure dotnames like ".npmrc", ".env", ".ssh" do NOT match this regex + // (they have no non-leading dot) and are therefore expanded to /** below. // Digit-only suffixes (v1.0, app-2.3) are treated as versioned directory names. // Bare dotnames without a secondary extension (.ssh, .aws, .env, .gnupg) are // NOT treated as file-like: they are expanded to /** so the subtree is protected @@ -406,7 +408,12 @@ function findOnPath(name: string, pathOverride?: string): string | null { * * Returns null when the command is empty or the path cannot be determined. */ -export function resolveArgv0(command: string, cwd?: string): string | null { +export function resolveArgv0(command: string, cwd?: string, _depth = 0): string | null { + // Guard against deeply nested env -S "env -S '...'" constructs that would + // otherwise overflow the call stack. 8 levels is far more than any real usage. + if (_depth > 8) { + return null; + } const trimmed = command.trim(); if (!trimmed) { return null; @@ -509,23 +516,36 @@ export function resolveArgv0(command: string, cwd?: string): string | null { // NAME=value pairs are handled naturally when we recurse into resolveArgv0. // --block-signal, --default-signal, --ignore-signal use [=SIG] syntax (never space-separated). const envOptWithArgRe = /^(-[uC]|--(unset|chdir))\s+/; - const envSplitStringRe = /^(-S|--(split-string))\s+/; while (afterEnv) { if (afterEnv === "--" || afterEnv.startsWith("-- ")) { afterEnv = afterEnv.slice(2).trimStart(); break; // -- terminates env options; what follows is the command } - if (envSplitStringRe.test(afterEnv)) { - // -S/--split-string: the argument is itself a command string — recurse into it. - afterEnv = afterEnv.replace(/^\S+\s+/, ""); // strip "-S " or "--split-string " + // -S/--split-string: the argument is itself a command string — recurse into it. + // Handle all three forms GNU env accepts: + // space: -S CMD / --split-string CMD + // equals: -S=CMD / --split-string=CMD + // compact: -SCMD (short flag only, value starts immediately after -S) + const splitEqM = afterEnv.match(/^(?:-S|--(split-string))=([\s\S]*)/); + const splitSpM = afterEnv.match(/^(?:-S|--(split-string))\s+([\s\S]*)/); + const splitCmM = afterEnv.match(/^-S([^\s=][\s\S]*)/); + const splitArg = splitEqM + ? splitEqM[splitEqM.length - 1] + : splitSpM + ? splitSpM[splitSpM.length - 1] + : splitCmM + ? splitCmM[1] + : null; + if (splitArg !== null) { + let inner = splitArg.trim(); // Strip surrounding quotes that the shell added around the embedded command. if ( - (afterEnv.startsWith('"') && afterEnv.endsWith('"')) || - (afterEnv.startsWith("'") && afterEnv.endsWith("'")) + (inner.startsWith('"') && inner.endsWith('"')) || + (inner.startsWith("'") && inner.endsWith("'")) ) { - afterEnv = afterEnv.slice(1, -1); + inner = inner.slice(1, -1); } - return afterEnv ? resolveArgv0(afterEnv, cwd) : null; + return inner ? resolveArgv0(inner, cwd, _depth + 1) : null; } if (envOptWithArgRe.test(afterEnv)) { // Strip option + its argument; handle quoted values with spaces. @@ -536,7 +556,7 @@ export function resolveArgv0(command: string, cwd?: string): string | null { break; // first non-option token — may still be NAME=value, handled by recursion } } - return afterEnv ? resolveArgv0(afterEnv, cwd) : null; + return afterEnv ? resolveArgv0(afterEnv, cwd, _depth + 1) : null; } return token; diff --git a/src/infra/exec-sandbox-bwrap.test.ts b/src/infra/exec-sandbox-bwrap.test.ts index cddd53e0835..d44cf2447d0 100644 --- a/src/infra/exec-sandbox-bwrap.test.ts +++ b/src/infra/exec-sandbox-bwrap.test.ts @@ -2,6 +2,7 @@ import os from "node:os"; import { describe, expect, it, vi } from "vitest"; import type { AccessPolicyConfig } from "../config/types.tools.js"; import { + _resetBwrapFileDenyWarnedPathsForTest, _warnBwrapFileDenyOnce, generateBwrapArgs, wrapCommandWithBwrap, @@ -411,3 +412,20 @@ describe("wrapCommandWithBwrap", () => { expect(result).toContain("cat /etc/hosts"); }); }); + +describe("_resetBwrapFileDenyWarnedPathsForTest", () => { + it("clears the warned-paths set so the same path can warn again", () => { + const spy = vi.spyOn(console, "error").mockImplementation(() => {}); + // First warning — set is empty, should fire. + _warnBwrapFileDenyOnce("/tmp/secret.txt"); + expect(spy).toHaveBeenCalledTimes(1); + // Second call with same path — already warned, should NOT fire again. + _warnBwrapFileDenyOnce("/tmp/secret.txt"); + expect(spy).toHaveBeenCalledTimes(1); + // After reset the warning should fire again. + _resetBwrapFileDenyWarnedPathsForTest(); + _warnBwrapFileDenyOnce("/tmp/secret.txt"); + expect(spy).toHaveBeenCalledTimes(2); + spy.mockRestore(); + }); +}); diff --git a/src/infra/exec-sandbox-bwrap.ts b/src/infra/exec-sandbox-bwrap.ts index 7f2aa197ee6..a041d84b5d9 100644 --- a/src/infra/exec-sandbox-bwrap.ts +++ b/src/infra/exec-sandbox-bwrap.ts @@ -33,6 +33,10 @@ let bwrapAvailableCache: boolean | undefined; // the file directly inside the sandbox are not blocked at the syscall level. // See docs/tools/access-policy.md — "File-specific deny[] entries on Linux". const _bwrapFileDenyWarnedPaths = new Set(); +/** Reset the one-time file-deny warning set. Only for use in tests. */ +export function _resetBwrapFileDenyWarnedPathsForTest(): void { + _bwrapFileDenyWarnedPaths.clear(); +} export function _warnBwrapFileDenyOnce(filePath: string): void { if (_bwrapFileDenyWarnedPaths.has(filePath)) { return;