mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-27 09:21:35 +07:00
fix: isolate session:patch hook payload (#53880) (thanks @graciegould)
* gateway: make session:patch hook typed and non-blocking * gateway(test): add session:patch hook coverage * docs(gateway): clarify session:patch security note * fix: address review feedback on session:patch hook Remove unused createInternalHookEvent import and fix doc example to use inline event.type check matching existing hook examples. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: isolate hook payload to prevent mutation leaking into response Shallow-copy sessionEntry and patch in the session:patch hook event so fire-and-forget handlers cannot mutate objects used by the response path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: isolate session:patch hook payload (#53880) (thanks @graciegould) --------- Co-authored-by: “graciegould” <“graciegould5@gmail.com”> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Ayaan Zaidi <hi@obviy.us>
This commit is contained in:
@@ -275,6 +275,68 @@ Triggered when the gateway starts:
|
||||
|
||||
- **`gateway:startup`**: After channels start and hooks are loaded
|
||||
|
||||
### Session Patch Events
|
||||
|
||||
Triggered when session properties are modified:
|
||||
|
||||
- **`session:patch`**: When a session is updated
|
||||
|
||||
#### Session Event Context
|
||||
|
||||
Session events include rich context about the session and changes:
|
||||
|
||||
```typescript
|
||||
{
|
||||
sessionEntry: SessionEntry, // The complete updated session entry
|
||||
patch: { // The patch object (only changed fields)
|
||||
// Session identity & labeling
|
||||
label?: string | null, // Human-readable session label
|
||||
|
||||
// AI model configuration
|
||||
model?: string | null, // Model override (e.g., "claude-opus-4-5")
|
||||
thinkingLevel?: string | null, // Thinking level ("off"|"low"|"med"|"high")
|
||||
verboseLevel?: string | null, // Verbose output level
|
||||
reasoningLevel?: string | null, // Reasoning mode override
|
||||
elevatedLevel?: string | null, // Elevated mode override
|
||||
responseUsage?: "off" | "tokens" | "full" | null, // Usage display mode
|
||||
|
||||
// Tool execution settings
|
||||
execHost?: string | null, // Exec host (sandbox|gateway|node)
|
||||
execSecurity?: string | null, // Security mode (deny|allowlist|full)
|
||||
execAsk?: string | null, // Approval mode (off|on-miss|always)
|
||||
execNode?: string | null, // Node ID for host=node
|
||||
|
||||
// Subagent coordination
|
||||
spawnedBy?: string | null, // Parent session key (for subagents)
|
||||
spawnDepth?: number | null, // Nesting depth (0 = root)
|
||||
|
||||
// Communication policies
|
||||
sendPolicy?: "allow" | "deny" | null, // Message send policy
|
||||
groupActivation?: "mention" | "always" | null, // Group chat activation
|
||||
},
|
||||
cfg: OpenClawConfig // Current gateway config
|
||||
}
|
||||
```
|
||||
|
||||
**Security note:** Only privileged clients (including the Control UI) can trigger `session:patch` events. Standard WebChat clients are blocked from patching sessions (see PR #20800), so the hook will not fire from those connections.
|
||||
|
||||
See `SessionsPatchParamsSchema` in `src/gateway/protocol/schema/sessions.ts` for the complete type definition.
|
||||
|
||||
#### Example: Session Patch Logger Hook
|
||||
|
||||
```typescript
|
||||
const handler = async (event) => {
|
||||
if (event.type !== "session" || event.action !== "patch") {
|
||||
return;
|
||||
}
|
||||
const { patch } = event.context;
|
||||
console.log(`[session-patch] Session updated: ${event.sessionKey}`);
|
||||
console.log(`[session-patch] Changes:`, patch);
|
||||
};
|
||||
|
||||
export default handler;
|
||||
```
|
||||
|
||||
### Message Events
|
||||
|
||||
Triggered when messages are received or sent:
|
||||
|
||||
@@ -18,6 +18,11 @@ import {
|
||||
type SessionEntry,
|
||||
updateSessionStore,
|
||||
} from "../../config/sessions.js";
|
||||
import {
|
||||
triggerInternalHook,
|
||||
type SessionPatchHookContext,
|
||||
type SessionPatchHookEvent,
|
||||
} from "../../hooks/internal-hooks.js";
|
||||
import {
|
||||
normalizeAgentId,
|
||||
parseAgentSessionKey,
|
||||
@@ -893,6 +898,22 @@ export const sessionsHandlers: GatewayRequestHandlers = {
|
||||
respond(false, undefined, applied.error);
|
||||
return;
|
||||
}
|
||||
|
||||
const hookContext: SessionPatchHookContext = structuredClone({
|
||||
sessionEntry: applied.entry,
|
||||
patch: p,
|
||||
cfg,
|
||||
});
|
||||
const hookEvent: SessionPatchHookEvent = {
|
||||
type: "session",
|
||||
action: "patch",
|
||||
sessionKey: target.canonicalKey ?? key,
|
||||
context: hookContext,
|
||||
timestamp: new Date(),
|
||||
messages: [],
|
||||
};
|
||||
void triggerInternalHook(hookEvent);
|
||||
|
||||
const parsed = parseAgentSessionKey(target.canonicalKey ?? key);
|
||||
const agentId = normalizeAgentId(parsed?.agentId ?? resolveDefaultAgentId(cfg));
|
||||
const resolved = resolveSessionModelRef(cfg, applied.entry, agentId);
|
||||
|
||||
@@ -4,6 +4,8 @@ import path from "node:path";
|
||||
import { afterAll, beforeAll, beforeEach, describe, expect, test, vi } from "vitest";
|
||||
import { WebSocket } from "ws";
|
||||
import { DEFAULT_PROVIDER } from "../agents/defaults.js";
|
||||
import { clearConfigCache, clearRuntimeConfigSnapshot } from "../config/config.js";
|
||||
import { isSessionPatchEvent, type InternalHookEvent } from "../hooks/internal-hooks.js";
|
||||
import { GATEWAY_CLIENT_IDS, GATEWAY_CLIENT_MODES } from "./protocol/client-info.js";
|
||||
import { startGatewayServerHarness, type GatewayServerHarness } from "./server.e2e-ws-harness.js";
|
||||
import { createToolSummaryPreviewTranscriptLines } from "./session-preview.test-helpers.js";
|
||||
@@ -32,7 +34,7 @@ const bootstrapCacheMocks = vi.hoisted(() => ({
|
||||
}));
|
||||
|
||||
const sessionHookMocks = vi.hoisted(() => ({
|
||||
triggerInternalHook: vi.fn(async () => {}),
|
||||
triggerInternalHook: vi.fn(async (_event: unknown) => {}),
|
||||
}));
|
||||
|
||||
const subagentLifecycleHookMocks = vi.hoisted(() => ({
|
||||
@@ -236,8 +238,25 @@ async function getMainPreviewEntry(ws: import("ws").WebSocket) {
|
||||
return entry;
|
||||
}
|
||||
|
||||
function isInternalHookEvent(value: unknown): value is InternalHookEvent {
|
||||
if (!value || typeof value !== "object") {
|
||||
return false;
|
||||
}
|
||||
const candidate = value as Record<string, unknown>;
|
||||
return (
|
||||
typeof candidate.type === "string" &&
|
||||
typeof candidate.action === "string" &&
|
||||
typeof candidate.sessionKey === "string" &&
|
||||
Array.isArray(candidate.messages) &&
|
||||
typeof candidate.context === "object" &&
|
||||
candidate.context !== null
|
||||
);
|
||||
}
|
||||
|
||||
describe("gateway server sessions", () => {
|
||||
beforeEach(() => {
|
||||
clearRuntimeConfigSnapshot();
|
||||
clearConfigCache();
|
||||
sessionCleanupMocks.clearSessionQueues.mockClear();
|
||||
sessionCleanupMocks.stopSubagentsForRequester.mockClear();
|
||||
bootstrapCacheMocks.clearBootstrapSnapshot.mockReset();
|
||||
@@ -1787,6 +1806,182 @@ describe("gateway server sessions", () => {
|
||||
ws.close();
|
||||
});
|
||||
|
||||
test("session:patch hook fires with correct context", async () => {
|
||||
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-sessions-patch-hook-"));
|
||||
const storePath = path.join(dir, "sessions.json");
|
||||
testState.sessionStorePath = storePath;
|
||||
|
||||
await writeSessionStore({
|
||||
entries: {
|
||||
main: {
|
||||
sessionId: "sess-hook-test",
|
||||
updatedAt: Date.now(),
|
||||
label: "original-label",
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
sessionHookMocks.triggerInternalHook.mockClear();
|
||||
|
||||
const { ws } = await openClient();
|
||||
|
||||
const patched = await rpcReq(ws, "sessions.patch", {
|
||||
key: "agent:main:main",
|
||||
label: "updated-label",
|
||||
});
|
||||
|
||||
expect(patched.ok).toBe(true);
|
||||
expect(sessionHookMocks.triggerInternalHook).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
type: "session",
|
||||
action: "patch",
|
||||
sessionKey: expect.stringMatching(/agent:main:main/),
|
||||
context: expect.objectContaining({
|
||||
sessionEntry: expect.objectContaining({
|
||||
sessionId: "sess-hook-test",
|
||||
label: "updated-label",
|
||||
}),
|
||||
patch: expect.objectContaining({
|
||||
label: "updated-label",
|
||||
}),
|
||||
cfg: expect.any(Object),
|
||||
}),
|
||||
}),
|
||||
);
|
||||
|
||||
ws.close();
|
||||
});
|
||||
|
||||
test("session:patch hook does not fire for webchat clients", async () => {
|
||||
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-sessions-webchat-hook-"));
|
||||
const storePath = path.join(dir, "sessions.json");
|
||||
testState.sessionStorePath = storePath;
|
||||
|
||||
await writeSessionStore({
|
||||
entries: {
|
||||
main: {
|
||||
sessionId: "sess-webchat-test",
|
||||
updatedAt: Date.now(),
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
sessionHookMocks.triggerInternalHook.mockClear();
|
||||
|
||||
const ws = new WebSocket(`ws://127.0.0.1:${harness.port}`, {
|
||||
headers: { origin: `http://127.0.0.1:${harness.port}` },
|
||||
});
|
||||
trackConnectChallengeNonce(ws);
|
||||
await new Promise<void>((resolve) => ws.once("open", resolve));
|
||||
await connectOk(ws, {
|
||||
client: {
|
||||
id: GATEWAY_CLIENT_IDS.WEBCHAT_UI,
|
||||
version: "1.0.0",
|
||||
platform: "test",
|
||||
mode: GATEWAY_CLIENT_MODES.UI,
|
||||
},
|
||||
scopes: ["operator.admin"],
|
||||
});
|
||||
|
||||
const patched = await rpcReq(ws, "sessions.patch", {
|
||||
key: "agent:main:main",
|
||||
label: "should-not-trigger-hook",
|
||||
});
|
||||
|
||||
expect(patched.ok).toBe(false);
|
||||
expect(sessionHookMocks.triggerInternalHook).not.toHaveBeenCalled();
|
||||
|
||||
ws.close();
|
||||
});
|
||||
|
||||
test("session:patch hook only fires after successful patch", async () => {
|
||||
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-sessions-success-hook-"));
|
||||
const storePath = path.join(dir, "sessions.json");
|
||||
testState.sessionStorePath = storePath;
|
||||
|
||||
await writeSessionStore({
|
||||
entries: {
|
||||
main: {
|
||||
sessionId: "sess-success-test",
|
||||
updatedAt: Date.now(),
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
const { ws } = await openClient();
|
||||
|
||||
sessionHookMocks.triggerInternalHook.mockClear();
|
||||
|
||||
// Test 1: Invalid patch (missing key) - hook should not fire
|
||||
const invalidPatch = await rpcReq(ws, "sessions.patch", {
|
||||
// Missing required 'key' parameter
|
||||
label: "should-fail",
|
||||
});
|
||||
|
||||
expect(invalidPatch.ok).toBe(false);
|
||||
expect(sessionHookMocks.triggerInternalHook).not.toHaveBeenCalled();
|
||||
|
||||
// Test 2: Valid patch - hook should fire
|
||||
const validPatch = await rpcReq(ws, "sessions.patch", {
|
||||
key: "agent:main:main",
|
||||
label: "should-succeed",
|
||||
});
|
||||
|
||||
expect(validPatch.ok).toBe(true);
|
||||
expect(sessionHookMocks.triggerInternalHook).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
type: "session",
|
||||
action: "patch",
|
||||
}),
|
||||
);
|
||||
|
||||
ws.close();
|
||||
});
|
||||
|
||||
test("session:patch hook mutations cannot change the response path", async () => {
|
||||
await createSessionStoreDir();
|
||||
await writeSessionStore({
|
||||
entries: {
|
||||
main: {
|
||||
sessionId: "sess-cfg-isolation-test",
|
||||
updatedAt: Date.now(),
|
||||
},
|
||||
},
|
||||
});
|
||||
|
||||
sessionHookMocks.triggerInternalHook.mockImplementationOnce(async (event) => {
|
||||
if (!isInternalHookEvent(event) || !isSessionPatchEvent(event)) {
|
||||
return;
|
||||
}
|
||||
event.context.cfg.agents = {
|
||||
...event.context.cfg.agents,
|
||||
defaults: {
|
||||
...event.context.cfg.agents?.defaults,
|
||||
model: "zai/glm-4.6",
|
||||
},
|
||||
};
|
||||
});
|
||||
|
||||
const { ws } = await openClient();
|
||||
const patched = await rpcReq<{
|
||||
entry: { label?: string };
|
||||
key: string;
|
||||
resolved: { modelProvider: string; model: string };
|
||||
}>(ws, "sessions.patch", {
|
||||
key: "agent:main:main",
|
||||
label: "cfg-isolation",
|
||||
});
|
||||
|
||||
expect(patched.ok).toBe(true);
|
||||
expect(patched.payload?.resolved).toEqual({
|
||||
modelProvider: "anthropic",
|
||||
model: "claude-opus-4-6",
|
||||
});
|
||||
expect(patched.payload?.entry.label).toBe("cfg-isolation");
|
||||
|
||||
ws.close();
|
||||
});
|
||||
|
||||
test("control-ui client can delete sessions even in webchat mode", async () => {
|
||||
const dir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-sessions-control-ui-delete-"));
|
||||
const storePath = path.join(dir, "sessions.json");
|
||||
|
||||
@@ -8,6 +8,8 @@
|
||||
import type { WorkspaceBootstrapFile } from "../agents/workspace.js";
|
||||
import type { CliDeps } from "../cli/deps.js";
|
||||
import type { OpenClawConfig } from "../config/config.js";
|
||||
import type { SessionEntry } from "../config/sessions.js";
|
||||
import type { SessionsPatchParams } from "../gateway/protocol/index.js";
|
||||
import { createSubsystemLogger } from "../logging/subsystem.js";
|
||||
import { resolveGlobalSingleton } from "../shared/global-singleton.js";
|
||||
|
||||
@@ -157,6 +159,18 @@ export type MessagePreprocessedHookEvent = InternalHookEvent & {
|
||||
context: MessagePreprocessedHookContext;
|
||||
};
|
||||
|
||||
export type SessionPatchHookContext = {
|
||||
sessionEntry: SessionEntry;
|
||||
patch: SessionsPatchParams;
|
||||
cfg: OpenClawConfig;
|
||||
};
|
||||
|
||||
export type SessionPatchHookEvent = InternalHookEvent & {
|
||||
type: "session";
|
||||
action: "patch";
|
||||
context: SessionPatchHookContext;
|
||||
};
|
||||
|
||||
export interface InternalHookEvent {
|
||||
/** The type of event (command, session, agent, gateway, etc.) */
|
||||
type: InternalHookEventType;
|
||||
@@ -418,3 +432,21 @@ export function isMessagePreprocessedEvent(
|
||||
}
|
||||
return hasStringContextField(context, "channelId");
|
||||
}
|
||||
|
||||
export function isSessionPatchEvent(event: InternalHookEvent): event is SessionPatchHookEvent {
|
||||
if (!isHookEventTypeAndAction(event, "session", "patch")) {
|
||||
return false;
|
||||
}
|
||||
const context = getHookContext<SessionPatchHookContext>(event);
|
||||
if (!context) {
|
||||
return false;
|
||||
}
|
||||
return (
|
||||
typeof context.patch === "object" &&
|
||||
context.patch !== null &&
|
||||
typeof context.cfg === "object" &&
|
||||
context.cfg !== null &&
|
||||
typeof context.sessionEntry === "object" &&
|
||||
context.sessionEntry !== null
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user