Skip to content

Commit fff6333

Browse files
steipetesteipete
committed
fix(exec): implement Windows argPattern allowlist flow
1 parent cc5146b commit fff6333

10 files changed

Lines changed: 1247 additions & 111 deletions

‎src/agents/bash-tools.exec-host-gateway.ts‎

Lines changed: 13 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,16 @@
11
import type { AgentToolResult } from "@mariozechner/pi-agent-core";
22
import {
33
addDurableCommandApproval,
4-
addAllowlistEntry,
54
type ExecAsk,
65
resolveExecApprovalAllowedDecisions,
76
type ExecSecurity,
87
buildEnforcedShellCommand,
98
evaluateShellAllowlist,
109
hasDurableExecApproval,
11-
recordAllowlistUse,
10+
persistAllowAlwaysPatterns,
11+
recordAllowlistMatchesUse,
1212
resolveApprovalAuditCandidatePath,
1313
requiresExecApproval,
14-
resolveAllowAlwaysPatterns,
1514
} from "../infra/exec-approvals.js";
1615
import {
1716
describeInterpreterInlineEval,
@@ -144,19 +143,14 @@ export async function processGatewayAllowlist(
144143
logInfo(`exec: obfuscation detected (gateway): ${obfuscation.reasons.join(", ")}`);
145144
params.warnings.push(`⚠️ Obfuscated command detected: ${obfuscation.reasons.join("; ")}`);
146145
}
147-
const recordMatchedAllowlistUse = (resolvedPath?: string) => {
148-
if (allowlistMatches.length === 0) {
149-
return;
150-
}
151-
const seen = new Set<string>();
152-
for (const match of allowlistMatches) {
153-
if (seen.has(match.pattern)) {
154-
continue;
155-
}
156-
seen.add(match.pattern);
157-
recordAllowlistUse(approvals.file, params.agentId, match, params.command, resolvedPath);
158-
}
159-
};
146+
const recordMatchedAllowlistUse = (resolvedPath?: string) =>
147+
recordAllowlistMatchesUse({
148+
approvals: approvals.file,
149+
agentId: params.agentId,
150+
matches: allowlistMatches,
151+
command: params.command,
152+
resolvedPath,
153+
});
160154
const hasHeredocSegment = allowlistEval.segments.some((segment) =>
161155
segment.argv.some((token) => token.startsWith("<<")),
162156
);
@@ -320,20 +314,15 @@ export async function processGatewayAllowlist(
320314
} else if (decision === "allow-always") {
321315
approvedByAsk = true;
322316
if (!requiresInlineEvalApproval) {
323-
const patterns = resolveAllowAlwaysPatterns({
317+
const patterns = persistAllowAlwaysPatterns({
318+
approvals: approvals.file,
319+
agentId: params.agentId,
324320
segments: allowlistEval.segments,
325321
cwd: params.workdir,
326322
env: params.env,
327323
platform: process.platform,
328324
strictInlineEval: params.strictInlineEval === true,
329325
});
330-
for (const pattern of patterns) {
331-
if (pattern) {
332-
addAllowlistEntry(approvals.file, params.agentId, pattern, {
333-
source: "allow-always",
334-
});
335-
}
336-
}
337326
if (patterns.length === 0) {
338327
addDurableCommandApproval(approvals.file, params.agentId, params.command);
339328
}

‎src/gateway/protocol/schema/exec-approvals.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ export const ExecApprovalsAllowlistEntrySchema = Type.Object(
55
{
66
id: Type.Optional(NonEmptyString),
77
pattern: Type.String(),
8+
argPattern: Type.Optional(Type.String()),
89
lastUsedAt: Type.Optional(Type.Integer({ minimum: 0 })),
910
lastUsedCommand: Type.Optional(Type.String()),
1011
lastResolvedPath: Type.Optional(Type.String()),

‎src/infra/exec-approvals-allow-always.test.ts‎

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -146,31 +146,38 @@ describe("resolveAllowAlwaysPatterns", () => {
146146
expect(second.allowlistSatisfied).toBe(false);
147147
}
148148

149-
function expectPositionalArgvCarrierRejected(command: string) {
149+
function expectPositionalArgvCarrierResult(params: {
150+
command: string;
151+
expectPersisted: boolean;
152+
}) {
150153
const dir = makeTempDir();
151154
const touch = makeExecutable(dir, "touch");
152155
const env = { PATH: `${dir}${path.delimiter}${process.env.PATH ?? ""}` };
153156
const safeBins = resolveSafeBins(undefined);
154157
const marker = path.join(dir, "marker");
155-
const expandedCommand = command.replaceAll("{marker}", marker);
158+
const command = params.command.replaceAll("{marker}", marker);
156159

157160
const { persisted } = resolvePersistedPatterns({
158-
command: expandedCommand,
161+
command,
159162
dir,
160163
env,
161164
safeBins,
162165
});
163-
expect(persisted).toEqual([]);
166+
if (params.expectPersisted) {
167+
expect(persisted).toEqual([touch]);
168+
} else {
169+
expect(persisted).toEqual([]);
170+
}
164171

165172
const second = evaluateShellAllowlist({
166-
command: expandedCommand,
173+
command,
167174
allowlist: [{ pattern: touch }],
168175
safeBins,
169176
cwd: dir,
170177
env,
171178
platform: process.platform,
172179
});
173-
expect(second.allowlistSatisfied).toBe(false);
180+
expect(second.allowlistSatisfied).toBe(params.expectPersisted);
174181
}
175182

176183
it("returns direct executable paths for non-shell segments", () => {
@@ -387,29 +394,41 @@ describe("resolveAllowAlwaysPatterns", () => {
387394
if (process.platform === "win32") {
388395
return;
389396
}
390-
expectPositionalArgvCarrierRejected(`sh -lc '$0 "$1"' touch {marker}`);
397+
expectPositionalArgvCarrierResult({
398+
command: `sh -lc '$0 "$1"' touch {marker}`,
399+
expectPersisted: true,
400+
});
391401
});
392402

393403
it("rejects exec positional argv carriers", () => {
394404
if (process.platform === "win32") {
395405
return;
396406
}
397-
expectPositionalArgvCarrierRejected(`sh -lc 'exec -- "$0" "$1"' touch {marker}`);
407+
expectPositionalArgvCarrierResult({
408+
command: `sh -lc 'exec -- "$0" "$1"' touch {marker}`,
409+
expectPersisted: true,
410+
});
398411
});
399412

400413
it("rejects positional argv carriers when $0 is single-quoted", () => {
401414
if (process.platform === "win32") {
402415
return;
403416
}
404-
expectPositionalArgvCarrierRejected(`sh -lc "'$0' "$1"" touch {marker}`);
417+
expectPositionalArgvCarrierResult({
418+
command: `sh -lc "'$0' "$1"" touch {marker}`,
419+
expectPersisted: false,
420+
});
405421
});
406422

407423
it("rejects positional argv carriers when exec is separated from $0 by a newline", () => {
408424
if (process.platform === "win32") {
409425
return;
410426
}
411-
expectPositionalArgvCarrierRejected(`sh -lc "exec
412-
$0 \\"$1\\"" touch {marker}`);
427+
expectPositionalArgvCarrierResult({
428+
command: `sh -lc "exec
429+
$0 \\"$1\\"" touch {marker}`,
430+
expectPersisted: false,
431+
});
413432
});
414433

415434
it("rejects positional argv carriers when inline command contains extra shell operations", () => {
@@ -915,7 +934,7 @@ $0 \\"$1\\"" touch {marker}`);
915934
expect(second.allowlistSatisfied).toBe(false);
916935
});
917936

918-
it("rejects positional carrier when carried executable is an unknown dispatch carrier", () => {
937+
it("allows positional carriers for unknown carried executables when explicitly allowlisted", () => {
919938
if (process.platform === "win32") {
920939
return;
921940
}
@@ -940,6 +959,6 @@ $0 \\"$1\\"" touch {marker}`);
940959
env,
941960
platform: process.platform,
942961
});
943-
expect(second.allowlistSatisfied).toBe(false);
962+
expect(second.allowlistSatisfied).toBe(true);
944963
});
945964
});

0 commit comments

Comments
 (0)