Skip to content

Commit aec9809

Browse files
steipetesteipete
committed
fix: require admin for HTTP session kills
1 parent 26b9736 commit aec9809

2 files changed

Lines changed: 30 additions & 145 deletions

File tree

‎src/gateway/session-kill-http.test.ts‎

Lines changed: 21 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,14 @@ const REQUESTER_WRITE_HEADERS = {
1515
"x-openclaw-scopes": "operator.write",
1616
"x-openclaw-requester-session-key": "agent:main:main",
1717
};
18+
const REQUESTER_ADMIN_HEADERS = {
19+
"x-openclaw-scopes": "operator.admin",
20+
"x-openclaw-requester-session-key": "agent:other:main",
21+
};
1822

1923
let cfg: Record<string, unknown> = {};
2024
const authMock = vi.fn(async (): Promise<GatewayAuthResult> => ({ ok: true }));
21-
const isLocalDirectRequestMock = vi.fn(() => true);
2225
const loadSessionEntryMock = vi.fn();
23-
const getLatestSubagentRunByChildSessionKeyMock = vi.fn();
24-
const resolveSubagentControllerMock = vi.fn();
25-
const killControlledSubagentRunMock = vi.fn();
2626
const killSubagentRunAdminMock = vi.fn();
2727

2828
vi.mock("../config/config.js", () => ({
@@ -35,21 +35,14 @@ vi.mock("../config/io.js", () => ({
3535

3636
vi.mock("./auth.js", () => ({
3737
authorizeHttpGatewayConnect: authMock,
38-
isLocalDirectRequest: isLocalDirectRequestMock,
3938
}));
4039

4140
vi.mock("./session-utils.js", () => ({
4241
loadSessionEntry: loadSessionEntryMock,
4342
}));
4443

45-
vi.mock("../agents/subagent-registry.js", () => ({
46-
getLatestSubagentRunByChildSessionKey: getLatestSubagentRunByChildSessionKeyMock,
47-
}));
48-
4944
vi.mock("../agents/subagent-control.js", () => ({
50-
killControlledSubagentRun: killControlledSubagentRunMock,
5145
killSubagentRunAdmin: killSubagentRunAdminMock,
52-
resolveSubagentController: resolveSubagentControllerMock,
5346
}));
5447

5548
const { handleSessionKillHttpRequest } = await import("./session-kill-http.js");
@@ -93,13 +86,7 @@ beforeEach(() => {
9386
cfg = {};
9487
authMock.mockReset();
9588
authMock.mockResolvedValue({ ok: true, method: "token" });
96-
isLocalDirectRequestMock.mockReset();
97-
isLocalDirectRequestMock.mockReturnValue(true);
9889
loadSessionEntryMock.mockReset();
99-
getLatestSubagentRunByChildSessionKeyMock.mockReset();
100-
resolveSubagentControllerMock.mockReset();
101-
resolveSubagentControllerMock.mockReturnValue({ controllerSessionKey: "agent:main:main" });
102-
killControlledSubagentRunMock.mockReset();
10390
killSubagentRunAdminMock.mockReset();
10491
});
10592

@@ -134,12 +121,6 @@ function mockWorkerSession() {
134121
});
135122
}
136123

137-
function allowRemoteRequesterKill() {
138-
isLocalDirectRequestMock.mockReturnValue(false);
139-
allowTrustedProxyAuth();
140-
mockWorkerSession();
141-
}
142-
143124
async function expectForbiddenMissingScope(response: Response, message: string) {
144125
expect(response.status).toBe(403);
145126
expectErrorResponse(await response.json(), {
@@ -148,11 +129,6 @@ async function expectForbiddenMissingScope(response: Response, message: string)
148129
});
149130
}
150131

151-
async function expectRequesterKillResponse(response: Response, killed: boolean) {
152-
expect(response.status).toBe(200);
153-
await expect(response.json()).resolves.toEqual({ ok: true, killed });
154-
}
155-
156132
function expectErrorResponse(body: unknown, expected: { type: string; message?: string }) {
157133
const response = body as {
158134
ok?: unknown;
@@ -210,7 +186,6 @@ describe("POST /sessions/:sessionKey/kill", () => {
210186
expect(authMock).not.toHaveBeenCalled();
211187
expect(loadSessionEntryMock).not.toHaveBeenCalled();
212188
expect(killSubagentRunAdminMock).not.toHaveBeenCalled();
213-
expect(killControlledSubagentRunMock).not.toHaveBeenCalled();
214189
},
215190
);
216191

@@ -261,8 +236,7 @@ describe("POST /sessions/:sessionKey/kill", () => {
261236
expect(killSubagentRunAdminMock).not.toHaveBeenCalled();
262237
});
263238

264-
it("rejects remote bearer-auth kills without requester ownership", async () => {
265-
isLocalDirectRequestMock.mockReturnValue(false);
239+
it("rejects bearer-auth kills without a trusted admin scope surface", async () => {
266240
mockWorkerSession();
267241

268242
const response = await postWorkerKill();
@@ -271,63 +245,29 @@ describe("POST /sessions/:sessionKey/kill", () => {
271245
expect(killSubagentRunAdminMock).not.toHaveBeenCalled();
272246
});
273247

274-
it("rejects remote kills without requester ownership or an authorized token", async () => {
275-
isLocalDirectRequestMock.mockReturnValue(false);
276-
authMock.mockResolvedValueOnce({ ok: true });
277-
mockWorkerSession();
278-
279-
const response = await postWorkerKill("", {
280-
authorization: "",
281-
});
282-
expect(response.status).toBe(403);
248+
it("rejects trusted-proxy requester-session kills without admin scope", async () => {
249+
allowTrustedProxyAuth();
250+
const response = await postWorkerKill("", REQUESTER_WRITE_HEADERS);
251+
await expectForbiddenMissingScope(response, "missing scope: operator.admin");
252+
expect(loadSessionEntryMock).not.toHaveBeenCalled();
283253
expect(killSubagentRunAdminMock).not.toHaveBeenCalled();
284254
});
285255

286-
it("uses requester ownership checks when a requester session header is provided without admin bypass", async () => {
287-
allowRemoteRequesterKill();
288-
getLatestSubagentRunByChildSessionKeyMock.mockReturnValue({
289-
runId: "run-1",
290-
childSessionKey: WORKER_SESSION_KEY,
291-
});
292-
killControlledSubagentRunMock.mockResolvedValue({ status: "ok" });
256+
it("uses the admin kill path even when the requester session header is present", async () => {
257+
allowTrustedProxyAuth();
258+
mockWorkerSession();
259+
killSubagentRunAdminMock.mockResolvedValue({ found: true, killed: true });
293260

294-
const response = await postWorkerKill("", REQUESTER_WRITE_HEADERS);
295-
await expectRequesterKillResponse(response, true);
296-
expect(resolveSubagentControllerMock).toHaveBeenCalledWith({
261+
const response = await postWorkerKill("", REQUESTER_ADMIN_HEADERS);
262+
expect(response.status).toBe(200);
263+
await expect(response.json()).resolves.toEqual({ ok: true, killed: true });
264+
expect(killSubagentRunAdminMock).toHaveBeenCalledWith({
297265
cfg,
298-
agentSessionKey: "agent:main:main",
299-
});
300-
expect(getLatestSubagentRunByChildSessionKeyMock).toHaveBeenCalledWith(WORKER_SESSION_KEY);
301-
expect(killSubagentRunAdminMock).not.toHaveBeenCalled();
302-
});
303-
304-
it("uses the newest child-session row for requester-owned kills when stale rows still exist", async () => {
305-
allowRemoteRequesterKill();
306-
getLatestSubagentRunByChildSessionKeyMock.mockReturnValue({
307-
runId: "run-current-ended",
308-
childSessionKey: WORKER_SESSION_KEY,
309-
endedAt: Date.now() - 1,
266+
sessionKey: WORKER_SESSION_KEY,
310267
});
311-
killControlledSubagentRunMock.mockResolvedValue({ status: "done" });
312-
313-
const response = await postWorkerKill("", REQUESTER_WRITE_HEADERS);
314-
await expectRequesterKillResponse(response, false);
315-
expect(killControlledSubagentRunMock).toHaveBeenCalledTimes(1);
316-
const killCall = killControlledSubagentRunMock.mock.calls.at(0)?.[0] as
317-
| {
318-
cfg?: unknown;
319-
controller?: { controllerSessionKey?: string };
320-
entry?: { runId?: string; childSessionKey?: string };
321-
}
322-
| undefined;
323-
expect(killCall?.cfg).toBe(cfg);
324-
expect(killCall?.controller?.controllerSessionKey).toBe("agent:main:main");
325-
expect(killCall?.entry?.runId).toBe("run-current-ended");
326-
expect(killCall?.entry?.childSessionKey).toBe(WORKER_SESSION_KEY);
327268
});
328269

329-
it("rejects bearer-auth requester kills without a trusted write scope surface", async () => {
330-
isLocalDirectRequestMock.mockReturnValue(false);
270+
it("rejects bearer-auth requester kills without a trusted admin scope surface", async () => {
331271
const response = await post(
332272
"/sessions/agent%3Amain%3Asubagent%3Aworker/kill",
333273
TEST_GATEWAY_TOKEN,
@@ -336,10 +276,9 @@ describe("POST /sessions/:sessionKey/kill", () => {
336276
expect(response.status).toBe(403);
337277
expectErrorResponse(await response.json(), {
338278
type: "forbidden",
339-
message: "missing scope: operator.write",
279+
message: "missing scope: operator.admin",
340280
});
341281
expect(loadSessionEntryMock).not.toHaveBeenCalled();
342282
expect(killSubagentRunAdminMock).not.toHaveBeenCalled();
343-
expect(killControlledSubagentRunMock).not.toHaveBeenCalled();
344283
});
345284
});

‎src/gateway/session-kill-http.ts‎

Lines changed: 9 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,10 @@
11
// Gateway HTTP session kill handler.
2-
// Allows local admins or owning parent sessions to stop subagent runs.
2+
// Stops subagent runs through the admin-scoped HTTP control surface.
33
import type { IncomingMessage, ServerResponse } from "node:http";
4-
import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce";
5-
import {
6-
killControlledSubagentRun,
7-
killSubagentRunAdmin,
8-
resolveSubagentController,
9-
} from "../agents/subagent-control.js";
10-
import { getLatestSubagentRunByChildSessionKey } from "../agents/subagent-registry.js";
4+
import { killSubagentRunAdmin } from "../agents/subagent-control.js";
115
import { getRuntimeConfig } from "../config/io.js";
126
import type { AuthRateLimiter } from "./auth-rate-limit.js";
13-
import { isLocalDirectRequest, type ResolvedGatewayAuth } from "./auth.js";
7+
import type { ResolvedGatewayAuth } from "./auth.js";
148
import {
159
sendInvalidRequest,
1610
sendJson,
@@ -24,8 +18,6 @@ import {
2418
import { authorizeOperatorScopesForMethod } from "./method-scopes.js";
2519
import { loadSessionEntry } from "./session-utils.js";
2620

27-
const REQUESTER_SESSION_KEY_HEADER = "x-openclaw-requester-session-key";
28-
2921
type SessionKeyPathResolution =
3022
| { matched: false }
3123
| { matched: true; sessionKey: string }
@@ -86,30 +78,8 @@ export async function handleSessionKillHttpRequest(
8678
return true;
8779
}
8880

89-
const trustedProxies = opts.trustedProxies ?? cfg.gateway?.trustedProxies;
90-
const allowRealIpFallback = opts.allowRealIpFallback ?? cfg.gateway?.allowRealIpFallback;
91-
const requesterSessionKey = normalizeOptionalString(
92-
req.headers[REQUESTER_SESSION_KEY_HEADER]?.toString(),
93-
);
94-
const allowLocalAdminKill = isLocalDirectRequest(req, trustedProxies, allowRealIpFallback);
9581
const requestedScopes = resolveTrustedHttpOperatorScopes(req, requestAuth);
96-
97-
// Remote browser requests must prove parent-session ownership; local direct
98-
// operator requests can perform the stronger admin kill path.
99-
if (!requesterSessionKey && !allowLocalAdminKill) {
100-
sendJson(res, 403, {
101-
ok: false,
102-
error: {
103-
type: "forbidden",
104-
message: "Session kills require a local admin request or requester session ownership.",
105-
},
106-
});
107-
return true;
108-
}
109-
110-
const requiredOperatorMethod =
111-
requesterSessionKey && !allowLocalAdminKill ? "sessions.abort" : "sessions.delete";
112-
const scopeAuth = authorizeOperatorScopesForMethod(requiredOperatorMethod, requestedScopes);
82+
const scopeAuth = authorizeOperatorScopesForMethod("sessions.delete", requestedScopes);
11383
if (!scopeAuth.allowed) {
11484
sendMissingScopeForbidden(res, scopeAuth.missingScope);
11585
return true;
@@ -127,38 +97,14 @@ export async function handleSessionKillHttpRequest(
12797
return true;
12898
}
12999

130-
let killed = false;
131-
if (!allowLocalAdminKill && requesterSessionKey) {
132-
const runEntry = getLatestSubagentRunByChildSessionKey(canonicalKey);
133-
if (runEntry) {
134-
const result = await killControlledSubagentRun({
135-
cfg,
136-
controller: resolveSubagentController({ cfg, agentSessionKey: requesterSessionKey }),
137-
entry: runEntry,
138-
});
139-
if (result.status === "forbidden") {
140-
sendJson(res, 403, {
141-
ok: false,
142-
error: {
143-
type: "forbidden",
144-
message: result.error,
145-
},
146-
});
147-
return true;
148-
}
149-
killed = result.status === "ok";
150-
}
151-
} else {
152-
const result = await killSubagentRunAdmin({
153-
cfg,
154-
sessionKey: canonicalKey,
155-
});
156-
killed = result.killed;
157-
}
100+
const result = await killSubagentRunAdmin({
101+
cfg,
102+
sessionKey: canonicalKey,
103+
});
158104

159105
sendJson(res, 200, {
160106
ok: true,
161-
killed,
107+
killed: result.killed,
162108
});
163109
return true;
164110
}

0 commit comments

Comments
 (0)