fix(exec): derive agentId from sessionKey for allowlist lookup
When creating exec tools via chat/Discord, agentId was not passed, causing allowlist lookup to use 'default' key instead of 'main'. User's allowlist entries in agents.main were never matched. Now derives agentId from sessionKey if not explicitly provided, ensuring correct allowlist lookup for all exec paths.
This commit is contained in:
committed by
Peter Steinberger
parent
51cd9c7ff4
commit
0c55b1e9ce
@@ -54,6 +54,7 @@ import { callGatewayTool } from "./tools/gateway.js";
|
||||
import { listNodes, resolveNodeIdFromList } from "./tools/nodes-utils.js";
|
||||
import { getShellConfig, sanitizeBinaryOutput } from "./shell-utils.js";
|
||||
import { buildCursorPositionResponse, stripDsrRequests } from "./pty-dsr.js";
|
||||
import { parseAgentSessionKey, resolveAgentIdFromSessionKey } from "../routing/session-key.js";
|
||||
|
||||
const DEFAULT_MAX_OUTPUT = clampNumber(
|
||||
readEnvInt("PI_BASH_MAX_OUTPUT_CHARS"),
|
||||
@@ -659,6 +660,11 @@ export function createExecTool(
|
||||
const notifyOnExit = defaults?.notifyOnExit !== false;
|
||||
const notifySessionKey = defaults?.sessionKey?.trim() || undefined;
|
||||
const approvalRunningNoticeMs = resolveApprovalRunningNoticeMs(defaults?.approvalRunningNoticeMs);
|
||||
// Derive agentId only when sessionKey is an agent session key.
|
||||
const parsedAgentSession = parseAgentSessionKey(defaults?.sessionKey);
|
||||
const agentId =
|
||||
defaults?.agentId ??
|
||||
(parsedAgentSession ? resolveAgentIdFromSessionKey(defaults?.sessionKey) : undefined);
|
||||
|
||||
return {
|
||||
name: "exec",
|
||||
@@ -799,7 +805,7 @@ export function createExecTool(
|
||||
|
||||
if (host === "node") {
|
||||
const approvals = resolveExecApprovals(
|
||||
defaults?.agentId,
|
||||
agentId,
|
||||
host === "node" ? { security: "allowlist" } : undefined,
|
||||
);
|
||||
const hostSecurity = minSecurity(security, approvals.agent.security);
|
||||
@@ -865,7 +871,7 @@ export function createExecTool(
|
||||
cwd: workdir,
|
||||
env: nodeEnv,
|
||||
timeoutMs: typeof params.timeout === "number" ? params.timeout * 1000 : undefined,
|
||||
agentId: defaults?.agentId,
|
||||
agentId,
|
||||
sessionKey: defaults?.sessionKey,
|
||||
approved: approvedByAsk,
|
||||
approvalDecision: approvalDecision ?? undefined,
|
||||
@@ -895,9 +901,9 @@ export function createExecTool(
|
||||
host: "node",
|
||||
security: hostSecurity,
|
||||
ask: hostAsk,
|
||||
agentId: defaults?.agentId,
|
||||
resolvedPath: undefined,
|
||||
sessionKey: defaults?.sessionKey,
|
||||
agentId,
|
||||
resolvedPath: null,
|
||||
sessionKey: defaults?.sessionKey ?? null,
|
||||
timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS,
|
||||
},
|
||||
)) as { decision?: string } | null;
|
||||
@@ -1026,7 +1032,7 @@ export function createExecTool(
|
||||
}
|
||||
|
||||
if (host === "gateway") {
|
||||
const approvals = resolveExecApprovals(defaults?.agentId, { security: "allowlist" });
|
||||
const approvals = resolveExecApprovals(agentId, { security: "allowlist" });
|
||||
const hostSecurity = minSecurity(security, approvals.agent.security);
|
||||
const hostAsk = maxAsk(ask, approvals.agent.ask);
|
||||
const askFallback = approvals.agent.askFallback;
|
||||
@@ -1060,7 +1066,7 @@ export function createExecTool(
|
||||
const approvalSlug = createApprovalSlug(approvalId);
|
||||
const expiresAtMs = Date.now() + DEFAULT_APPROVAL_TIMEOUT_MS;
|
||||
const contextKey = `exec:${approvalId}`;
|
||||
const resolvedPath = analysis.segments[0]?.resolution?.resolvedPath;
|
||||
const resolvedPath = analysis.segments[0]?.resolution?.resolvedPath ?? null;
|
||||
const noticeSeconds = Math.max(1, Math.round(approvalRunningNoticeMs / 1000));
|
||||
const commandText = params.command;
|
||||
const effectiveTimeout =
|
||||
@@ -1080,9 +1086,9 @@ export function createExecTool(
|
||||
host: "gateway",
|
||||
security: hostSecurity,
|
||||
ask: hostAsk,
|
||||
agentId: defaults?.agentId,
|
||||
agentId,
|
||||
resolvedPath,
|
||||
sessionKey: defaults?.sessionKey,
|
||||
sessionKey: defaults?.sessionKey ?? null,
|
||||
timeoutMs: DEFAULT_APPROVAL_TIMEOUT_MS,
|
||||
},
|
||||
)) as { decision?: string } | null;
|
||||
@@ -1123,7 +1129,7 @@ export function createExecTool(
|
||||
for (const segment of analysis.segments) {
|
||||
const pattern = segment.resolution?.resolvedPath ?? "";
|
||||
if (pattern) {
|
||||
addAllowlistEntry(approvals.file, defaults?.agentId, pattern);
|
||||
addAllowlistEntry(approvals.file, agentId, pattern);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1152,7 +1158,7 @@ export function createExecTool(
|
||||
seen.add(match.pattern);
|
||||
recordAllowlistUse(
|
||||
approvals.file,
|
||||
defaults?.agentId,
|
||||
agentId,
|
||||
match,
|
||||
commandText,
|
||||
resolvedPath ?? undefined,
|
||||
@@ -1242,7 +1248,7 @@ export function createExecTool(
|
||||
seen.add(match.pattern);
|
||||
recordAllowlistUse(
|
||||
approvals.file,
|
||||
defaults?.agentId,
|
||||
agentId,
|
||||
match,
|
||||
params.command,
|
||||
analysis.segments[0]?.resolution?.resolvedPath,
|
||||
|
||||
@@ -14,6 +14,7 @@ import {
|
||||
normalizeSafeBins,
|
||||
resolveCommandResolution,
|
||||
resolveExecApprovals,
|
||||
resolveExecApprovalsFromFile,
|
||||
type ExecAllowlistEntry,
|
||||
} from "./exec-approvals.js";
|
||||
|
||||
@@ -227,3 +228,35 @@ describe("exec approvals wildcard agent", () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("exec approvals default agent migration", () => {
|
||||
it("migrates legacy default agent entries to main", () => {
|
||||
const file = {
|
||||
version: 1,
|
||||
agents: {
|
||||
default: { allowlist: [{ pattern: "/bin/legacy" }] },
|
||||
},
|
||||
};
|
||||
const resolved = resolveExecApprovalsFromFile({ file });
|
||||
expect(resolved.allowlist.map((entry) => entry.pattern)).toEqual(["/bin/legacy"]);
|
||||
expect(resolved.file.agents?.default).toBeUndefined();
|
||||
expect(resolved.file.agents?.main?.allowlist?.[0]?.pattern).toBe("/bin/legacy");
|
||||
});
|
||||
|
||||
it("prefers main agent settings when both main and default exist", () => {
|
||||
const file = {
|
||||
version: 1,
|
||||
agents: {
|
||||
main: { ask: "always", allowlist: [{ pattern: "/bin/main" }] },
|
||||
default: { ask: "off", allowlist: [{ pattern: "/bin/legacy" }] },
|
||||
},
|
||||
};
|
||||
const resolved = resolveExecApprovalsFromFile({ file });
|
||||
expect(resolved.agent.ask).toBe("always");
|
||||
expect(resolved.allowlist.map((entry) => entry.pattern)).toEqual([
|
||||
"/bin/main",
|
||||
"/bin/legacy",
|
||||
]);
|
||||
expect(resolved.file.agents?.default).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -4,6 +4,8 @@ import net from "node:net";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
|
||||
import { DEFAULT_AGENT_ID } from "../routing/session-key.js";
|
||||
|
||||
export type ExecHost = "sandbox" | "gateway" | "node";
|
||||
export type ExecSecurity = "deny" | "allowlist" | "full";
|
||||
export type ExecAsk = "off" | "on-miss" | "always";
|
||||
@@ -84,6 +86,32 @@ export function resolveExecApprovalsSocketPath(): string {
|
||||
return expandHome(DEFAULT_SOCKET);
|
||||
}
|
||||
|
||||
function normalizeAllowlistPattern(value: string | undefined): string | null {
|
||||
const trimmed = value?.trim() ?? "";
|
||||
return trimmed ? trimmed.toLowerCase() : null;
|
||||
}
|
||||
|
||||
function mergeLegacyAgent(current: ExecApprovalsAgent, legacy: ExecApprovalsAgent): ExecApprovalsAgent {
|
||||
const allowlist: ExecAllowlistEntry[] = [];
|
||||
const seen = new Set<string>();
|
||||
const pushEntry = (entry: ExecAllowlistEntry) => {
|
||||
const key = normalizeAllowlistPattern(entry.pattern);
|
||||
if (!key || seen.has(key)) return;
|
||||
seen.add(key);
|
||||
allowlist.push(entry);
|
||||
};
|
||||
for (const entry of current.allowlist ?? []) pushEntry(entry);
|
||||
for (const entry of legacy.allowlist ?? []) pushEntry(entry);
|
||||
|
||||
return {
|
||||
security: current.security ?? legacy.security,
|
||||
ask: current.ask ?? legacy.ask,
|
||||
askFallback: current.askFallback ?? legacy.askFallback,
|
||||
autoAllowSkills: current.autoAllowSkills ?? legacy.autoAllowSkills,
|
||||
allowlist: allowlist.length > 0 ? allowlist : undefined,
|
||||
};
|
||||
}
|
||||
|
||||
function ensureDir(filePath: string) {
|
||||
const dir = path.dirname(filePath);
|
||||
fs.mkdirSync(dir, { recursive: true });
|
||||
@@ -92,6 +120,15 @@ function ensureDir(filePath: string) {
|
||||
export function normalizeExecApprovals(file: ExecApprovalsFile): ExecApprovalsFile {
|
||||
const socketPath = file.socket?.path?.trim();
|
||||
const token = file.socket?.token?.trim();
|
||||
const agents = { ...(file.agents ?? {}) };
|
||||
const legacyDefault = agents.default;
|
||||
if (legacyDefault) {
|
||||
const main = agents[DEFAULT_AGENT_ID];
|
||||
agents[DEFAULT_AGENT_ID] = main
|
||||
? mergeLegacyAgent(main, legacyDefault)
|
||||
: legacyDefault;
|
||||
delete agents.default;
|
||||
}
|
||||
const normalized: ExecApprovalsFile = {
|
||||
version: 1,
|
||||
socket: {
|
||||
@@ -104,7 +141,7 @@ export function normalizeExecApprovals(file: ExecApprovalsFile): ExecApprovalsFi
|
||||
askFallback: file.defaults?.askFallback,
|
||||
autoAllowSkills: file.defaults?.autoAllowSkills,
|
||||
},
|
||||
agents: file.agents ?? {},
|
||||
agents,
|
||||
};
|
||||
return normalized;
|
||||
}
|
||||
@@ -231,7 +268,7 @@ export function resolveExecApprovalsFromFile(params: {
|
||||
}): ExecApprovalsResolved {
|
||||
const file = normalizeExecApprovals(params.file);
|
||||
const defaults = file.defaults ?? {};
|
||||
const agentKey = params.agentId ?? "default";
|
||||
const agentKey = params.agentId ?? DEFAULT_AGENT_ID;
|
||||
const agent = file.agents?.[agentKey] ?? {};
|
||||
const wildcard = file.agents?.["*"] ?? {};
|
||||
const fallbackSecurity = params.overrides?.security ?? DEFAULT_SECURITY;
|
||||
|
||||
Reference in New Issue
Block a user