mirror of https://github.com/openclaw/openclaw.git
Gateway: keep spawned workspace overrides internal (#43801)
* Gateway: keep spawned workspace overrides internal * Changelog: note GHSA-2rqg agent boundary fix * Gateway: persist spawned workspace inheritance in sessions * Agents: clean failed lineage spawn state * Tests: cover lineage attachment cleanup * Tests: cover lineage thread cleanup
This commit is contained in:
parent
97683071b5
commit
46a332385d
|
|
@ -12,6 +12,7 @@ Docs: https://docs.openclaw.ai
|
|||
- Security/host env: block inherited `GIT_EXEC_PATH` from sanitized host exec environments so Git helper resolution cannot be steered by host environment state. (`GHSA-jf5v-pqgw-gm5m`)(#43685) Thanks @zpbrent and @vincentkoc.
|
||||
- Security/session_status: enforce sandbox session-tree visibility and shared agent-to-agent access guards before reading or mutating target session state, so sandboxed subagents can no longer inspect parent session metadata or write parent model overrides via `session_status`. (`GHSA-wcxr-59v9-rxr8`)(#43754) Thanks @tdjackey and @vincentkoc.
|
||||
- Models/secrets: enforce source-managed SecretRef markers in generated `models.json` so runtime-resolved provider secrets are not persisted when runtime projection is skipped. (#43759) Thanks @joshavant.
|
||||
- Security/agent: reject public spawned-run lineage fields and keep workspace inheritance on the internal spawned-session path so external `agent` callers can no longer override the gateway workspace boundary. (`GHSA-2rqg-gjgv-84jm`)(#43801) Thanks @tdjackey and @vincentkoc.
|
||||
- Security/exec allowlist: preserve POSIX case sensitivity and keep `?` within a single path segment so exact-looking allowlist patterns no longer overmatch executables across case or directory boundaries. (`GHSA-f8r2-vg7x-gh8m`)(#43798) Thanks @zpbrent and @vincentkoc.
|
||||
|
||||
### Changes
|
||||
|
|
|
|||
|
|
@ -85,7 +85,10 @@ describe("sessions_spawn depth + child limits", () => {
|
|||
});
|
||||
|
||||
it("rejects spawning when caller depth reaches maxSpawnDepth", async () => {
|
||||
const tool = createSessionsSpawnTool({ agentSessionKey: "agent:main:subagent:parent" });
|
||||
const tool = createSessionsSpawnTool({
|
||||
agentSessionKey: "agent:main:subagent:parent",
|
||||
workspaceDir: "/parent/workspace",
|
||||
});
|
||||
const result = await tool.execute("call-depth-reject", { task: "hello" });
|
||||
|
||||
expect(result.details).toMatchObject({
|
||||
|
|
@ -109,8 +112,13 @@ describe("sessions_spawn depth + child limits", () => {
|
|||
const calls = callGatewayMock.mock.calls.map(
|
||||
(call) => call[0] as { method?: string; params?: Record<string, unknown> },
|
||||
);
|
||||
const agentCall = calls.find((entry) => entry.method === "agent");
|
||||
expect(agentCall?.params?.spawnedBy).toBe("agent:main:subagent:parent");
|
||||
const spawnedByPatch = calls.find(
|
||||
(entry) =>
|
||||
entry.method === "sessions.patch" &&
|
||||
entry.params?.spawnedBy === "agent:main:subagent:parent",
|
||||
);
|
||||
expect(spawnedByPatch?.params?.key).toMatch(/^agent:main:subagent:/);
|
||||
expect(typeof spawnedByPatch?.params?.spawnedWorkspaceDir).toBe("string");
|
||||
|
||||
const spawnDepthPatch = calls.find(
|
||||
(entry) => entry.method === "sessions.patch" && entry.params?.spawnDepth === 2,
|
||||
|
|
|
|||
|
|
@ -380,4 +380,36 @@ describe("sessions_spawn subagent lifecycle hooks", () => {
|
|||
emitLifecycleHooks: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("cleans up the provisional session when lineage patching fails after thread binding", async () => {
|
||||
const callGatewayMock = getCallGatewayMock();
|
||||
callGatewayMock.mockImplementation(async (opts: unknown) => {
|
||||
const request = opts as { method?: string; params?: Record<string, unknown> };
|
||||
if (request.method === "sessions.patch" && typeof request.params?.spawnedBy === "string") {
|
||||
throw new Error("lineage patch failed");
|
||||
}
|
||||
if (request.method === "sessions.delete") {
|
||||
return { ok: true };
|
||||
}
|
||||
return {};
|
||||
});
|
||||
|
||||
const result = await executeDiscordThreadSessionSpawn("call9");
|
||||
|
||||
expect(result.details).toMatchObject({
|
||||
status: "error",
|
||||
error: "lineage patch failed",
|
||||
});
|
||||
expect(hookRunnerMocks.runSubagentSpawned).not.toHaveBeenCalled();
|
||||
expect(hookRunnerMocks.runSubagentEnded).not.toHaveBeenCalled();
|
||||
const methods = getGatewayMethods();
|
||||
expect(methods).toContain("sessions.delete");
|
||||
expect(methods).not.toContain("agent");
|
||||
const deleteCall = findGatewayRequest("sessions.delete");
|
||||
expect(deleteCall?.params).toMatchObject({
|
||||
key: (result.details as { childSessionKey?: string }).childSessionKey,
|
||||
deleteTranscript: true,
|
||||
emitLifecycleHooks: true,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { resetSubagentRegistryForTests } from "./subagent-registry.js";
|
||||
import { decodeStrictBase64, spawnSubagentDirect } from "./subagent-spawn.js";
|
||||
|
||||
|
|
@ -31,6 +32,7 @@ let configOverride: Record<string, unknown> = {
|
|||
},
|
||||
},
|
||||
};
|
||||
let workspaceDirOverride = "";
|
||||
|
||||
vi.mock("../config/config.js", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("../config/config.js")>();
|
||||
|
|
@ -61,7 +63,7 @@ vi.mock("./agent-scope.js", async (importOriginal) => {
|
|||
const actual = await importOriginal<typeof import("./agent-scope.js")>();
|
||||
return {
|
||||
...actual,
|
||||
resolveAgentWorkspaceDir: () => path.join(os.tmpdir(), "agent-workspace"),
|
||||
resolveAgentWorkspaceDir: () => workspaceDirOverride,
|
||||
};
|
||||
});
|
||||
|
||||
|
|
@ -145,6 +147,16 @@ describe("spawnSubagentDirect filename validation", () => {
|
|||
resetSubagentRegistryForTests();
|
||||
callGatewayMock.mockClear();
|
||||
setupGatewayMock();
|
||||
workspaceDirOverride = fs.mkdtempSync(
|
||||
path.join(os.tmpdir(), `openclaw-subagent-attachments-${process.pid}-${Date.now()}-`),
|
||||
);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
if (workspaceDirOverride) {
|
||||
fs.rmSync(workspaceDirOverride, { recursive: true, force: true });
|
||||
workspaceDirOverride = "";
|
||||
}
|
||||
});
|
||||
|
||||
const ctx = {
|
||||
|
|
@ -210,4 +222,43 @@ describe("spawnSubagentDirect filename validation", () => {
|
|||
expect(result.status).toBe("error");
|
||||
expect(result.error).toMatch(/attachments_invalid_name/);
|
||||
});
|
||||
|
||||
it("removes materialized attachments when lineage patching fails", async () => {
|
||||
const calls: Array<{ method?: string; params?: Record<string, unknown> }> = [];
|
||||
callGatewayMock.mockImplementation(async (opts: unknown) => {
|
||||
const request = opts as { method?: string; params?: Record<string, unknown> };
|
||||
calls.push(request);
|
||||
if (request.method === "sessions.patch" && typeof request.params?.spawnedBy === "string") {
|
||||
throw new Error("lineage patch failed");
|
||||
}
|
||||
if (request.method === "sessions.delete") {
|
||||
return { ok: true };
|
||||
}
|
||||
return {};
|
||||
});
|
||||
|
||||
const result = await spawnSubagentDirect(
|
||||
{
|
||||
task: "test",
|
||||
attachments: [{ name: "file.txt", content: validContent, encoding: "base64" }],
|
||||
},
|
||||
ctx,
|
||||
);
|
||||
|
||||
expect(result).toMatchObject({
|
||||
status: "error",
|
||||
error: "lineage patch failed",
|
||||
});
|
||||
const attachmentsRoot = path.join(workspaceDirOverride, ".openclaw", "attachments");
|
||||
const retainedDirs = fs.existsSync(attachmentsRoot)
|
||||
? fs.readdirSync(attachmentsRoot).filter((entry) => !entry.startsWith("."))
|
||||
: [];
|
||||
expect(retainedDirs).toHaveLength(0);
|
||||
const deleteCall = calls.find((entry) => entry.method === "sessions.delete");
|
||||
expect(deleteCall?.params).toMatchObject({
|
||||
key: expect.stringMatching(/^agent:main:subagent:/),
|
||||
deleteTranscript: true,
|
||||
emitLifecycleHooks: false,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -153,6 +153,25 @@ async function cleanupProvisionalSession(
|
|||
}
|
||||
}
|
||||
|
||||
async function cleanupFailedSpawnBeforeAgentStart(params: {
|
||||
childSessionKey: string;
|
||||
attachmentAbsDir?: string;
|
||||
emitLifecycleHooks?: boolean;
|
||||
deleteTranscript?: boolean;
|
||||
}): Promise<void> {
|
||||
if (params.attachmentAbsDir) {
|
||||
try {
|
||||
await fs.rm(params.attachmentAbsDir, { recursive: true, force: true });
|
||||
} catch {
|
||||
// Best-effort cleanup only.
|
||||
}
|
||||
}
|
||||
await cleanupProvisionalSession(params.childSessionKey, {
|
||||
emitLifecycleHooks: params.emitLifecycleHooks,
|
||||
deleteTranscript: params.deleteTranscript,
|
||||
});
|
||||
}
|
||||
|
||||
function resolveSpawnMode(params: {
|
||||
requestedMode?: SpawnSubagentMode;
|
||||
threadRequested: boolean;
|
||||
|
|
@ -561,10 +580,32 @@ export async function spawnSubagentDirect(
|
|||
explicitWorkspaceDir: toolSpawnMetadata.workspaceDir,
|
||||
}),
|
||||
});
|
||||
const spawnLineagePatchError = await patchChildSession({
|
||||
spawnedBy: spawnedByKey,
|
||||
...(spawnedMetadata.workspaceDir ? { spawnedWorkspaceDir: spawnedMetadata.workspaceDir } : {}),
|
||||
});
|
||||
if (spawnLineagePatchError) {
|
||||
await cleanupFailedSpawnBeforeAgentStart({
|
||||
childSessionKey,
|
||||
attachmentAbsDir,
|
||||
emitLifecycleHooks: threadBindingReady,
|
||||
deleteTranscript: true,
|
||||
});
|
||||
return {
|
||||
status: "error",
|
||||
error: spawnLineagePatchError,
|
||||
childSessionKey,
|
||||
};
|
||||
}
|
||||
|
||||
const childIdem = crypto.randomUUID();
|
||||
let childRunId: string = childIdem;
|
||||
try {
|
||||
const {
|
||||
spawnedBy: _spawnedBy,
|
||||
workspaceDir: _workspaceDir,
|
||||
...publicSpawnedMetadata
|
||||
} = spawnedMetadata;
|
||||
const response = await callGateway<{ runId: string }>({
|
||||
method: "agent",
|
||||
params: {
|
||||
|
|
@ -581,7 +622,7 @@ export async function spawnSubagentDirect(
|
|||
thinking: thinkingOverride,
|
||||
timeout: runTimeoutSeconds,
|
||||
label: label || undefined,
|
||||
...spawnedMetadata,
|
||||
...publicSpawnedMetadata,
|
||||
},
|
||||
timeoutMs: 10_000,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -78,6 +78,8 @@ export type SessionEntry = {
|
|||
sessionFile?: string;
|
||||
/** Parent session key that spawned this session (used for sandbox session-tool scoping). */
|
||||
spawnedBy?: string;
|
||||
/** Workspace inherited by spawned sessions and reused on later turns for the same child session. */
|
||||
spawnedWorkspaceDir?: string;
|
||||
/** True after a thread/topic session has been forked from its parent transcript once. */
|
||||
forkedFromParent?: boolean;
|
||||
/** Subagent spawn depth (0 = main, 1 = sub-agent, 2 = sub-sub-agent). */
|
||||
|
|
|
|||
|
|
@ -110,8 +110,6 @@ export const AgentParamsSchema = Type.Object(
|
|||
),
|
||||
idempotencyKey: NonEmptyString,
|
||||
label: Type.Optional(SessionLabelString),
|
||||
spawnedBy: Type.Optional(Type.String()),
|
||||
workspaceDir: Type.Optional(Type.String()),
|
||||
},
|
||||
{ additionalProperties: false },
|
||||
);
|
||||
|
|
|
|||
|
|
@ -71,6 +71,7 @@ export const SessionsPatchParamsSchema = Type.Object(
|
|||
execNode: Type.Optional(Type.Union([NonEmptyString, Type.Null()])),
|
||||
model: Type.Optional(Type.Union([NonEmptyString, Type.Null()])),
|
||||
spawnedBy: Type.Optional(Type.Union([NonEmptyString, Type.Null()])),
|
||||
spawnedWorkspaceDir: Type.Optional(Type.Union([NonEmptyString, Type.Null()])),
|
||||
spawnDepth: Type.Optional(Type.Union([Type.Integer({ minimum: 0 }), Type.Null()])),
|
||||
subagentRole: Type.Optional(
|
||||
Type.Union([Type.Literal("orchestrator"), Type.Literal("leaf"), Type.Null()]),
|
||||
|
|
|
|||
|
|
@ -405,30 +405,53 @@ describe("gateway agent handler", () => {
|
|||
expect(callArgs.bestEffortDeliver).toBe(false);
|
||||
});
|
||||
|
||||
it("only forwards workspaceDir for spawned subagent runs", async () => {
|
||||
it("rejects public spawned-run metadata fields", async () => {
|
||||
primeMainAgentRun();
|
||||
mocks.agentCommand.mockClear();
|
||||
|
||||
await invokeAgent(
|
||||
{
|
||||
message: "normal run",
|
||||
sessionKey: "agent:main:main",
|
||||
workspaceDir: "/tmp/ignored",
|
||||
idempotencyKey: "workspace-ignored",
|
||||
},
|
||||
{ reqId: "workspace-ignored-1" },
|
||||
);
|
||||
await vi.waitFor(() => expect(mocks.agentCommand).toHaveBeenCalled());
|
||||
const normalCall = mocks.agentCommand.mock.calls.at(-1)?.[0] as { workspaceDir?: string };
|
||||
expect(normalCall.workspaceDir).toBeUndefined();
|
||||
mocks.agentCommand.mockClear();
|
||||
const respond = vi.fn();
|
||||
|
||||
await invokeAgent(
|
||||
{
|
||||
message: "spawned run",
|
||||
sessionKey: "agent:main:main",
|
||||
spawnedBy: "agent:main:subagent:parent",
|
||||
workspaceDir: "/tmp/inherited",
|
||||
workspaceDir: "/tmp/injected",
|
||||
idempotencyKey: "workspace-rejected",
|
||||
} as AgentParams,
|
||||
{ reqId: "workspace-rejected-1", respond },
|
||||
);
|
||||
|
||||
expect(mocks.agentCommand).not.toHaveBeenCalled();
|
||||
expect(respond).toHaveBeenCalledWith(
|
||||
false,
|
||||
undefined,
|
||||
expect.objectContaining({
|
||||
message: expect.stringContaining("invalid agent params"),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("only forwards workspaceDir for spawned sessions with stored workspace inheritance", async () => {
|
||||
primeMainAgentRun();
|
||||
mockMainSessionEntry({
|
||||
spawnedBy: "agent:main:subagent:parent",
|
||||
spawnedWorkspaceDir: "/tmp/inherited",
|
||||
});
|
||||
mocks.updateSessionStore.mockImplementation(async (_path, updater) => {
|
||||
const store: Record<string, unknown> = {
|
||||
"agent:main:main": buildExistingMainStoreEntry({
|
||||
spawnedBy: "agent:main:subagent:parent",
|
||||
spawnedWorkspaceDir: "/tmp/inherited",
|
||||
}),
|
||||
};
|
||||
return await updater(store);
|
||||
});
|
||||
mocks.agentCommand.mockClear();
|
||||
|
||||
await invokeAgent(
|
||||
{
|
||||
message: "spawned run",
|
||||
sessionKey: "agent:main:main",
|
||||
idempotencyKey: "workspace-forwarded",
|
||||
},
|
||||
{ reqId: "workspace-forwarded-1" },
|
||||
|
|
|
|||
|
|
@ -190,24 +190,20 @@ export const agentHandlers: GatewayRequestHandlers = {
|
|||
timeout?: number;
|
||||
bestEffortDeliver?: boolean;
|
||||
label?: string;
|
||||
spawnedBy?: string;
|
||||
inputProvenance?: InputProvenance;
|
||||
workspaceDir?: string;
|
||||
};
|
||||
const senderIsOwner = resolveSenderIsOwnerFromClient(client);
|
||||
const cfg = loadConfig();
|
||||
const idem = request.idempotencyKey;
|
||||
const normalizedSpawned = normalizeSpawnedRunMetadata({
|
||||
spawnedBy: request.spawnedBy,
|
||||
groupId: request.groupId,
|
||||
groupChannel: request.groupChannel,
|
||||
groupSpace: request.groupSpace,
|
||||
workspaceDir: request.workspaceDir,
|
||||
});
|
||||
let resolvedGroupId: string | undefined = normalizedSpawned.groupId;
|
||||
let resolvedGroupChannel: string | undefined = normalizedSpawned.groupChannel;
|
||||
let resolvedGroupSpace: string | undefined = normalizedSpawned.groupSpace;
|
||||
let spawnedByValue = normalizedSpawned.spawnedBy;
|
||||
let spawnedByValue: string | undefined;
|
||||
const inputProvenance = normalizeInputProvenance(request.inputProvenance);
|
||||
const cached = context.dedupe.get(`agent:${idem}`);
|
||||
if (cached) {
|
||||
|
|
@ -359,11 +355,7 @@ export const agentHandlers: GatewayRequestHandlers = {
|
|||
const sessionId = entry?.sessionId ?? randomUUID();
|
||||
const labelValue = request.label?.trim() || entry?.label;
|
||||
const sessionAgent = resolveAgentIdFromSessionKey(canonicalKey);
|
||||
spawnedByValue = canonicalizeSpawnedByForAgent(
|
||||
cfg,
|
||||
sessionAgent,
|
||||
spawnedByValue || entry?.spawnedBy,
|
||||
);
|
||||
spawnedByValue = canonicalizeSpawnedByForAgent(cfg, sessionAgent, entry?.spawnedBy);
|
||||
let inheritedGroup:
|
||||
| { groupId?: string; groupChannel?: string; groupSpace?: string }
|
||||
| undefined;
|
||||
|
|
@ -400,6 +392,7 @@ export const agentHandlers: GatewayRequestHandlers = {
|
|||
providerOverride: entry?.providerOverride,
|
||||
label: labelValue,
|
||||
spawnedBy: spawnedByValue,
|
||||
spawnedWorkspaceDir: entry?.spawnedWorkspaceDir,
|
||||
spawnDepth: entry?.spawnDepth,
|
||||
channel: entry?.channel ?? request.channel?.trim(),
|
||||
groupId: resolvedGroupId ?? entry?.groupId,
|
||||
|
|
@ -628,7 +621,7 @@ export const agentHandlers: GatewayRequestHandlers = {
|
|||
// Internal-only: allow workspace override for spawned subagent runs.
|
||||
workspaceDir: resolveIngressWorkspaceOverrideForSpawnedRun({
|
||||
spawnedBy: spawnedByValue,
|
||||
workspaceDir: request.workspaceDir,
|
||||
workspaceDir: sessionEntry?.spawnedWorkspaceDir,
|
||||
}),
|
||||
senderIsOwner,
|
||||
},
|
||||
|
|
|
|||
|
|
@ -265,6 +265,19 @@ describe("gateway sessions patch", () => {
|
|||
expect(entry.spawnedBy).toBe("agent:main:main");
|
||||
});
|
||||
|
||||
test("sets spawnedWorkspaceDir for subagent sessions", async () => {
|
||||
const entry = expectPatchOk(
|
||||
await runPatch({
|
||||
storeKey: "agent:main:subagent:child",
|
||||
patch: {
|
||||
key: "agent:main:subagent:child",
|
||||
spawnedWorkspaceDir: "/tmp/subagent-workspace",
|
||||
},
|
||||
}),
|
||||
);
|
||||
expect(entry.spawnedWorkspaceDir).toBe("/tmp/subagent-workspace");
|
||||
});
|
||||
|
||||
test("sets spawnDepth for ACP sessions", async () => {
|
||||
const entry = expectPatchOk(
|
||||
await runPatch({
|
||||
|
|
@ -282,6 +295,13 @@ describe("gateway sessions patch", () => {
|
|||
expectPatchError(result, "spawnDepth is only supported");
|
||||
});
|
||||
|
||||
test("rejects spawnedWorkspaceDir on non-subagent sessions", async () => {
|
||||
const result = await runPatch({
|
||||
patch: { key: MAIN_SESSION_KEY, spawnedWorkspaceDir: "/tmp/nope" },
|
||||
});
|
||||
expectPatchError(result, "spawnedWorkspaceDir is only supported");
|
||||
});
|
||||
|
||||
test("normalizes exec/send/group patches", async () => {
|
||||
const entry = expectPatchOk(
|
||||
await runPatch({
|
||||
|
|
|
|||
|
|
@ -128,6 +128,27 @@ export async function applySessionsPatchToStore(params: {
|
|||
}
|
||||
}
|
||||
|
||||
if ("spawnedWorkspaceDir" in patch) {
|
||||
const raw = patch.spawnedWorkspaceDir;
|
||||
if (raw === null) {
|
||||
if (existing?.spawnedWorkspaceDir) {
|
||||
return invalid("spawnedWorkspaceDir cannot be cleared once set");
|
||||
}
|
||||
} else if (raw !== undefined) {
|
||||
if (!supportsSpawnLineage(storeKey)) {
|
||||
return invalid("spawnedWorkspaceDir is only supported for subagent:* or acp:* sessions");
|
||||
}
|
||||
const trimmed = String(raw).trim();
|
||||
if (!trimmed) {
|
||||
return invalid("invalid spawnedWorkspaceDir: empty");
|
||||
}
|
||||
if (existing?.spawnedWorkspaceDir && existing.spawnedWorkspaceDir !== trimmed) {
|
||||
return invalid("spawnedWorkspaceDir cannot be changed once set");
|
||||
}
|
||||
next.spawnedWorkspaceDir = trimmed;
|
||||
}
|
||||
}
|
||||
|
||||
if ("spawnDepth" in patch) {
|
||||
const raw = patch.spawnDepth;
|
||||
if (raw === null) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue