fix(access-policy): resolveArgv0 -S= and -SVAL forms, depth cap, bwrap reset export, npmrc comment

This commit is contained in:
subrih 2026-03-13 23:14:36 -07:00
parent 4c0ffe0884
commit 849077ecb5
4 changed files with 86 additions and 11 deletions

View File

@ -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.
});
});
// ---------------------------------------------------------------------------

View File

@ -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;

View File

@ -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();
});
});

View File

@ -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<string>();
/** 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;