diff --git a/src/config/sessions/session-accessor.lifecycle.ts b/src/config/sessions/session-accessor.lifecycle.ts index 2a7d79655dd5..a992845fc76f 100644 --- a/src/config/sessions/session-accessor.lifecycle.ts +++ b/src/config/sessions/session-accessor.lifecycle.ts @@ -208,6 +208,8 @@ export async function applySessionPatchProjection< TFailure extends SessionPatchProjectionFailure, >(params: { agentId?: string; + /** Revalidates request-scoped authorization after the writer slot is held. */ + assertCurrent?: () => void; storePath: string; resolveTarget: (snapshot: SessionPatchProjectionSnapshot) => SessionPatchProjectionTarget; project: ( @@ -239,7 +241,15 @@ export async function applySessionPatchProjection< removals: candidateKeys .filter((sessionKey) => sessionKey !== target.primaryKey) .map((sessionKey) => ({ sessionKey })), - upserts: [{ sessionKey: target.primaryKey, entry: projected.entry }], + upserts: [ + { + sessionKey: target.primaryKey, + buildEntry: () => { + params.assertCurrent?.(); + return projected.entry; + }, + }, + ], skipMaintenance: true, }); return { ...projected, entry: structuredClone(projected.entry) }; diff --git a/src/gateway/server-methods.authorization.test.ts b/src/gateway/server-methods.authorization.test.ts index 9cb5898e4923..2bad9074100d 100644 --- a/src/gateway/server-methods.authorization.test.ts +++ b/src/gateway/server-methods.authorization.test.ts @@ -1,11 +1,18 @@ import { afterEach, describe, expect, it, vi } from "vitest"; +import { + loadSessionEntry, + patchSessionEntry, + upsertSessionEntry, +} from "../config/sessions/session-accessor.js"; import { createEmptyPluginRegistry } from "../plugins/registry-empty.js"; import { setActivePluginRegistry } from "../plugins/runtime.js"; +import { withOpenClawTestState } from "../test-utils/openclaw-test-state.js"; import { createGatewayMethodRegistry, createPluginGatewayMethodDescriptor, } from "./methods/registry.js"; import { handleGatewayRequest } from "./server-methods.js"; +import { sessionMutationHandlers } from "./server-methods/sessions-mutations.js"; import type { GatewayRequestHandler } from "./server-methods/types.js"; const METHOD = "workboard.cards.dispatch"; @@ -231,4 +238,103 @@ describe("gateway method authorization", () => { }), ).toHaveBeenCalledWith(true, { profile }); }); + + it("rejects a mutation when its authorized session instance is replaced before commit", async () => { + await withOpenClawTestState({ scenario: "minimal" }, async () => { + const sessionKey = "agent:main:commit-bound-authorization"; + await upsertSessionEntry( + { agentId: "main", sessionKey }, + { + sessionId: "session-shared", + updatedAt: 1, + visibility: "shared", + }, + ); + + let continueHandler = () => {}; + const handlerCanContinue = new Promise((resolve) => { + continueHandler = resolve; + }); + let markHandlerStarted = () => {}; + const handlerStarted = new Promise((resolve) => { + markHandlerStarted = resolve; + }); + const patchHandler = sessionMutationHandlers["sessions.patch"]; + if (!patchHandler) { + throw new Error("sessions.patch handler is not registered"); + } + const respond = vi.fn(); + const request = handleGatewayRequest({ + req: { + type: "req", + id: "req-session-commit-bound", + method: "sessions.patch", + params: { key: sessionKey, label: "stale mutation" }, + }, + respond, + client: { + connId: "conn-session-commit-bound", + authenticatedUserId: "member@example.com", + authenticatedUserProfile: { + profileId: "member", + displayName: "Member", + hasAvatar: false, + updatedAt: 1, + }, + connect: { + role: "operator", + scopes: ["operator.write"], + client: { id: "test", version: "1", platform: "test", mode: "test" }, + minProtocol: 1, + maxProtocol: 1, + }, + } as Parameters[0]["client"], + isWebchatConnect: () => false, + context: { + getRuntimeConfig: () => ({}), + logGateway: { warn: vi.fn() }, + broadcast: vi.fn(), + broadcastToConnIds: vi.fn(), + getSessionEventSubscriberConnIds: () => new Set(), + chatAbortControllers: new Map(), + } as unknown as Parameters[0]["context"], + extraHandlers: { + "sessions.patch": async (options) => { + markHandlerStarted(); + await handlerCanContinue; + await patchHandler(options); + }, + }, + }); + + await handlerStarted; + await upsertSessionEntry( + { agentId: "main", sessionKey }, + { + sessionId: "session-draft-replacement", + updatedAt: 2, + visibility: "draft", + createdActor: { type: "human", id: "owner" }, + }, + ); + await patchSessionEntry({ agentId: "main", sessionKey }, () => ({ + visibility: "draft", + })); + continueHandler(); + await request; + + expect(respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ + details: expect.objectContaining({ code: "SESSION_MUTATION_AUTHORIZATION_CHANGED" }), + }), + ); + expect(loadSessionEntry({ agentId: "main", sessionKey })).toMatchObject({ + sessionId: "session-draft-replacement", + visibility: "draft", + }); + expect(loadSessionEntry({ agentId: "main", sessionKey })).not.toHaveProperty("label"); + }); + }); }); diff --git a/src/gateway/server-methods.ts b/src/gateway/server-methods.ts index 84cbf2a0cf31..33a99b7a176b 100644 --- a/src/gateway/server-methods.ts +++ b/src/gateway/server-methods.ts @@ -44,7 +44,10 @@ import type { GatewayRequestHandlers, GatewayRequestOptions, } from "./server-methods/types.js"; -import { authorizeSessionMutation } from "./session-sharing.js"; +import { + resolveSessionMutationAuthorization, + SessionMutationAuthorizationChangedError, +} from "./session-sharing.js"; const loadAgentHandlers = lazyHandlerModule( () => import("./server-methods/agent.js"), @@ -935,20 +938,14 @@ export async function handleGatewayRequest( respond(false, undefined, authError); return; } - // Best-effort pre-dispatch participation gate. Session ownership/visibility are - // usability features, not a security boundary (docs/concepts/multi-user.md, - // SECURITY.md), so this check is intentionally not commit-bound to the resolved - // instance: a concurrent reset/recreate can still race an in-flight mutation. - // Sharing-membership writes re-verify the instance in their own transaction; - // real isolation is separate agents/hosts. - const sessionMutationError = authorizeSessionMutation({ + const sessionMutation = resolveSessionMutationAuthorization({ client: client ?? null, method: req.method, requestParams: req.params, context, }); - if (sessionMutationError) { - respond(false, undefined, sessionMutationError); + if (sessionMutation.error) { + respond(false, undefined, sessionMutation.error); return; } if ( @@ -1075,16 +1072,28 @@ export async function handleGatewayRequest( isWebchatConnect, respond, context, + ...(sessionMutation.authorization + ? { sessionMutationAuthorization: sessionMutation.authorization } + : {}), }); // All handlers run inside a request scope so that plugin runtime // subagent methods (e.g. context engine tools spawning sub-agents // during tool execution) can dispatch back into the gateway. // The scope also carries caller identity into plugin-owned gateway methods. - const invokeWithRequestScope = async () => - await withPluginRuntimeGatewayRequestScope( - { context, client, isWebchatConnect }, - invokeHandler, - ); + const invokeWithRequestScope = async () => { + try { + await withPluginRuntimeGatewayRequestScope( + { context, client, isWebchatConnect }, + invokeHandler, + ); + } catch (error) { + if (error instanceof SessionMutationAuthorizationChangedError) { + respond(false, undefined, error.error); + return; + } + throw error; + } + }; if (!rootWorkAdmission) { await invokeWithRequestScope(); return; diff --git a/src/gateway/server-methods/sessions-delete.ts b/src/gateway/server-methods/sessions-delete.ts index 4969332847f9..6e684140a0a2 100644 --- a/src/gateway/server-methods/sessions-delete.ts +++ b/src/gateway/server-methods/sessions-delete.ts @@ -42,7 +42,15 @@ import type { GatewayRequestHandlers } from "./types.js"; import { assertValidParams } from "./validation.js"; export const sessionDeleteHandlers: GatewayRequestHandlers = { - "sessions.delete": async ({ req, params, respond, client, isWebchatConnect, context }) => { + "sessions.delete": async ({ + req, + params, + respond, + client, + isWebchatConnect, + context, + sessionMutationAuthorization, + }) => { if (!assertValidParams(params, validateSessionsDeleteParams, "sessions.delete", respond)) { return; } @@ -192,6 +200,7 @@ export const sessionDeleteHandlers: GatewayRequestHandlers = { if (!chatAbort) { throw new Error("chat.abort handler is not registered"); } + sessionMutationAuthorization?.assertCurrent(); await chatAbort({ req, params: { @@ -204,6 +213,7 @@ export const sessionDeleteHandlers: GatewayRequestHandlers = { context, client, isWebchatConnect, + ...(sessionMutationAuthorization ? { sessionMutationAuthorization } : {}), }); if (abortResult?.ok === false) { respond(false, undefined, abortResult.error); @@ -223,6 +233,7 @@ export const sessionDeleteHandlers: GatewayRequestHandlers = { scope: storePath, identities: deleteLifecycleIdentities, prepare: async () => { + sessionMutationAuthorization?.assertCurrent(); const preparedEntry = loadSessionEntry(key, { agentId: requestedAgentId }).entry; deleteBlockedByModelLock = rejectModelSelectionLockedDelete( preparedEntry, @@ -268,6 +279,7 @@ export const sessionDeleteHandlers: GatewayRequestHandlers = { ); return undefined; } + sessionMutationAuthorization?.assertCurrent(); const { entry, legacyKey, canonicalKey } = loadSessionEntry(key, { agentId: requestedAgentId, }); @@ -319,6 +331,7 @@ export const sessionDeleteHandlers: GatewayRequestHandlers = { ...(requestedAgentId ? { agentId: requestedAgentId } : {}), }); const postCleanupEntry = postCleanupTarget.entry; + sessionMutationAuthorization?.assertCurrent(); if ( !expectedLifecycleRevisionMatches(postCleanupEntry) || !expectedSessionIdMatches(postCleanupEntry) diff --git a/src/gateway/server-methods/sessions-dispatch.ts b/src/gateway/server-methods/sessions-dispatch.ts index b771ff5a1400..fd14b1fcb49e 100644 --- a/src/gateway/server-methods/sessions-dispatch.ts +++ b/src/gateway/server-methods/sessions-dispatch.ts @@ -9,6 +9,7 @@ import { import { managedWorktrees } from "../../agents/worktrees/service.js"; import { formatErrorMessage } from "../../infra/errors.js"; import { resolveRequestedSessionAgentId as resolveRequestedGlobalAgentId } from "../session-create-service.js"; +import { SessionMutationAuthorizationChangedError } from "../session-sharing.js"; import { projectWorkerSessionPlacement } from "../worker-environments/placement-projector.js"; import { isWorkerPlacementSessionRuntimeSupported, @@ -23,7 +24,7 @@ import type { GatewayRequestHandlers } from "./types.js"; import { assertValidParams } from "./validation.js"; export const sessionDispatchHandlers: GatewayRequestHandlers = { - "sessions.dispatch": async ({ params, respond, context }) => { + "sessions.dispatch": async ({ params, respond, context, sessionMutationAuthorization }) => { if (!assertValidParams(params, validateSessionsDispatchParams, "sessions.dispatch", respond)) { return; } @@ -132,6 +133,9 @@ export const sessionDispatchHandlers: GatewayRequestHandlers = { return; } try { + // Dispatch is session-id addressed after this point; reject a replacement before handing + // the captured instance to the asynchronous worker service. + sessionMutationAuthorization?.assertCurrent(); const placement = await dispatchService.dispatch({ sessionId, sessionKey: target.canonicalKey, @@ -149,6 +153,9 @@ export const sessionDispatchHandlers: GatewayRequestHandlers = { undefined, ); } catch (error) { + if (error instanceof SessionMutationAuthorizationChangedError) { + throw error; + } respond( false, undefined, @@ -159,7 +166,7 @@ export const sessionDispatchHandlers: GatewayRequestHandlers = { ); } }, - "sessions.reclaim": async ({ params, respond, context }) => { + "sessions.reclaim": async ({ params, respond, context, sessionMutationAuthorization }) => { if (!assertValidParams(params, validateSessionsReclaimParams, "sessions.reclaim", respond)) { return; } @@ -240,6 +247,7 @@ export const sessionDispatchHandlers: GatewayRequestHandlers = { return; } try { + sessionMutationAuthorization?.assertCurrent(); const placement = await placementService.reclaim({ sessionId, sessionKey: target.canonicalKey, @@ -256,6 +264,9 @@ export const sessionDispatchHandlers: GatewayRequestHandlers = { undefined, ); } catch (error) { + if (error instanceof SessionMutationAuthorizationChangedError) { + throw error; + } respond( false, undefined, diff --git a/src/gateway/server-methods/sessions-files.ts b/src/gateway/server-methods/sessions-files.ts index a1f01c45830b..2b153d95a614 100644 --- a/src/gateway/server-methods/sessions-files.ts +++ b/src/gateway/server-methods/sessions-files.ts @@ -711,7 +711,7 @@ export const sessionsFilesHandlers: GatewayRequestHandlers = { ...result, }); }, - "sessions.files.set": async ({ params, respond }) => { + "sessions.files.set": async ({ params, respond, sessionMutationAuthorization }) => { if (!assertValidParams(params, validateSessionsFilesSetParams, "sessions.files.set", respond)) { return; } @@ -766,6 +766,9 @@ export const sessionsFilesHandlers: GatewayRequestHandlers = { return; } let update: WorkspaceFileUpdateResult; + // The resolved root belongs to the authorized instance. Recheck after all async path + // discovery so a replacement cannot redirect this write to its workspace. + sessionMutationAuthorization?.assertCurrent(); try { update = await updateWorkspaceFile( loaded.root, diff --git a/src/gateway/server-methods/sessions-groups.ts b/src/gateway/server-methods/sessions-groups.ts index d5bf0ffb2be1..2d224bb4568e 100644 --- a/src/gateway/server-methods/sessions-groups.ts +++ b/src/gateway/server-methods/sessions-groups.ts @@ -14,6 +14,7 @@ import { putSessionGroups, renameSessionGroup, } from "../session-groups.js"; +import { SessionMutationAuthorizationChangedError } from "../session-sharing.js"; import { emitSessionsChanged } from "./session-change-event.js"; import type { GatewayRequestHandlers } from "./types.js"; import { assertValidParams } from "./validation.js"; @@ -37,7 +38,7 @@ export const sessionGroupHandlers: GatewayRequestHandlers = { // Catalog-only changes still need to reach other open clients. emitSessionsChanged(context, { reason: "groups" }); }, - "sessions.groups.rename": async ({ params, respond, context }) => { + "sessions.groups.rename": async ({ params, respond, context, sessionMutationAuthorization }) => { if ( !assertValidParams( params, @@ -53,14 +54,19 @@ export const sessionGroupHandlers: GatewayRequestHandlers = { cfg: context.getRuntimeConfig(), name: params.name, to: params.to, + assertCurrent: sessionMutationAuthorization?.assertCurrent, + assertTargetCurrent: sessionMutationAuthorization?.assertTargetCurrent, }); respond(true, { ok: true, ...result }, undefined); emitSessionsChanged(context, { reason: "groups" }); } catch (error) { + if (error instanceof SessionMutationAuthorizationChangedError) { + throw error; + } respond(false, undefined, errorShape(ErrorCodes.UNAVAILABLE, formatErrorMessage(error))); } }, - "sessions.groups.delete": async ({ params, respond, context }) => { + "sessions.groups.delete": async ({ params, respond, context, sessionMutationAuthorization }) => { if ( !assertValidParams( params, @@ -75,10 +81,15 @@ export const sessionGroupHandlers: GatewayRequestHandlers = { const result = await deleteSessionGroup({ cfg: context.getRuntimeConfig(), name: params.name, + assertCurrent: sessionMutationAuthorization?.assertCurrent, + assertTargetCurrent: sessionMutationAuthorization?.assertTargetCurrent, }); respond(true, { ok: true, ...result }, undefined); emitSessionsChanged(context, { reason: "groups" }); } catch (error) { + if (error instanceof SessionMutationAuthorizationChangedError) { + throw error; + } respond(false, undefined, errorShape(ErrorCodes.UNAVAILABLE, formatErrorMessage(error))); } }, diff --git a/src/gateway/server-methods/sessions-mutations.ts b/src/gateway/server-methods/sessions-mutations.ts index 3571a7c80321..3fc52f818a91 100644 --- a/src/gateway/server-methods/sessions-mutations.ts +++ b/src/gateway/server-methods/sessions-mutations.ts @@ -49,7 +49,7 @@ import type { GatewayRequestHandlers } from "./types.js"; import { assertValidParams } from "./validation.js"; export const sessionMutationHandlers: GatewayRequestHandlers = { - "sessions.patch": async ({ params, respond, context, client }) => { + "sessions.patch": async ({ params, respond, context, client, sessionMutationAuthorization }) => { if (!assertValidParams(params, validateSessionsPatchParams, "sessions.patch", respond)) { return; } @@ -161,6 +161,7 @@ export const sessionMutationHandlers: GatewayRequestHandlers = { } return await applySessionPatchProjection({ agentId: target.agentId, + assertCurrent: sessionMutationAuthorization?.assertCurrent, storePath, resolveTarget: ({ entries }) => { const store = Object.fromEntries( @@ -311,7 +312,13 @@ export const sessionMutationHandlers: GatewayRequestHandlers = { reason: "patch", }); }, - "sessions.pluginPatch": async ({ params, respond, context, client }) => { + "sessions.pluginPatch": async ({ + params, + respond, + context, + client, + sessionMutationAuthorization, + }) => { if ( !assertValidParams(params, validateSessionsPluginPatchParams, "sessions.pluginPatch", respond) ) { @@ -372,6 +379,7 @@ export const sessionMutationHandlers: GatewayRequestHandlers = { namespace, value: params.value, unset: params.unset === true, + assertCurrent: sessionMutationAuthorization?.assertCurrent, }); if (!patched.ok) { respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, patched.error)); @@ -383,7 +391,7 @@ export const sessionMutationHandlers: GatewayRequestHandlers = { reason: "plugin-patch", }); }, - "sessions.reset": async ({ params, respond, context, client }) => { + "sessions.reset": async ({ params, respond, context, client, sessionMutationAuthorization }) => { if (!assertValidParams(params, validateSessionsResetParams, "sessions.reset", respond)) { return; } @@ -401,6 +409,7 @@ export const sessionMutationHandlers: GatewayRequestHandlers = { reason, commandSource: "gateway:sessions.reset", creation: resolveOperatorSessionCreation(client), + assertAuthorizedInstance: sessionMutationAuthorization?.assertCurrent, }); if (!result.ok) { respond(false, undefined, result.error); diff --git a/src/gateway/server-methods/sessions-sharing.test.ts b/src/gateway/server-methods/sessions-sharing.test.ts index 52ac331530f3..427c1ceede05 100644 --- a/src/gateway/server-methods/sessions-sharing.test.ts +++ b/src/gateway/server-methods/sessions-sharing.test.ts @@ -21,7 +21,7 @@ import { withOpenClawTestState } from "../../test-utils/openclaw-test-state.js"; import { createBoardViewTicket } from "../board-view-ticket.js"; import { authorizeResolvedSessionMutation, - authorizeSessionMutation, + resolveSessionMutationAuthorization, canReceiveSessionEvent, filterDraftSessionsForClient, invalidateSessionSharingSnapshot, @@ -427,12 +427,12 @@ describe("session sharing handlers", () => { const requestContext = context(vi.fn(), cfg); expect( - authorizeSessionMutation({ + resolveSessionMutationAuthorization({ client: memberClient, method: "board.action", requestParams: { ticket, agentId: "work" }, context: requestContext, - }), + }).error, ).toMatchObject({ details: { code: "SESSION_PARTICIPATION_REQUIRED" } }); const { ticket: unscopedTicket } = createBoardViewTicket({ @@ -442,12 +442,12 @@ describe("session sharing handlers", () => { viewGeneration: "b".repeat(32), }); expect( - authorizeSessionMutation({ + resolveSessionMutationAuthorization({ client: memberClient, method: "board.action", requestParams: { ticket: unscopedTicket, agentId: "work" }, context: requestContext, - }), + }).error, ).toMatchObject({ details: { code: "SESSION_MUTATION_TARGET_REQUIRED" } }); }); }); @@ -488,12 +488,12 @@ describe("session sharing handlers", () => { ]; const expectAccess = (allowed: boolean) => { for (const [method, requestParams] of mutations) { - const error = authorizeSessionMutation({ + const error = resolveSessionMutationAuthorization({ client: memberClient, method, requestParams, context: requestContext, - }); + }).error; if (allowed) { expect(error, method).toBeNull(); } else { @@ -599,12 +599,12 @@ describe("session sharing handlers", () => { }, ); expect( - authorizeSessionMutation({ + resolveSessionMutationAuthorization({ client: identifiedClient("viewer"), method: "sessions.groups.delete", requestParams: { name: "Projects" }, context: requestContext, - }), + }).error, ).toMatchObject({ details: { code: "SESSION_PARTICIPATION_REQUIRED" } }); expect( await call("session.members.list", { sessionKey: restrictedKey }, requestContext, { diff --git a/src/gateway/server-methods/shared-types.ts b/src/gateway/server-methods/shared-types.ts index af8275173128..d86e3ea6eafa 100644 --- a/src/gateway/server-methods/shared-types.ts +++ b/src/gateway/server-methods/shared-types.ts @@ -324,6 +324,12 @@ export type GatewayRequestOptions = { methodRegistry?: GatewayMethodRegistryView; }; +/** Commit-time guard captured by the pre-dispatch session participation check. */ +export type SessionMutationAuthorization = { + assertCurrent: () => void; + assertTargetCurrent: (target: { sessionKey: string; agentId?: string }) => void; +}; + /** Normalized method invocation options passed to registered handlers. */ export type GatewayRequestHandlerOptions = { req: RequestFrame; @@ -332,6 +338,7 @@ export type GatewayRequestHandlerOptions = { isWebchatConnect: (params: ConnectParams | null | undefined) => boolean; respond: RespondFn; context: GatewayRequestContext; + sessionMutationAuthorization?: SessionMutationAuthorization; }; /** Single gateway method implementation. */ diff --git a/src/gateway/session-groups.ts b/src/gateway/session-groups.ts index 2477bc0f6b71..304b6a00e828 100644 --- a/src/gateway/session-groups.ts +++ b/src/gateway/session-groups.ts @@ -12,6 +12,7 @@ import { openOpenClawStateDatabase, runOpenClawStateWriteTransaction, } from "../state/openclaw-state-db.js"; +import { SessionMutationAuthorizationChangedError } from "./session-sharing.js"; // Write transactions must run on the same env-scoped handle as their // statements; a bare transaction would open the default state DB while the @@ -167,6 +168,7 @@ async function updateMemberCategories( from: string, to: string | undefined, env: NodeJS.ProcessEnv, + assertTargetCurrent?: (target: { agentId: string; sessionKey: string }) => void, ): Promise { let updated = 0; for (const target of resolveAllAgentSessionStoreTargetsSync(cfg, { env })) { @@ -177,6 +179,16 @@ async function updateMemberCategories( if (entry.category?.trim() !== from) { return []; } + try { + assertTargetCurrent?.({ agentId: target.agentId, sessionKey }); + } catch (error) { + if (error instanceof SessionMutationAuthorizationChangedError) { + // Group membership spans separate agent databases. Once the catalog commit starts, + // skip a concurrently replaced target instead of failing after earlier stores wrote. + return []; + } + throw error; + } const next = { ...entry }; if (to === undefined) { delete next.category; @@ -197,6 +209,8 @@ export async function renameSessionGroup(params: { name: string; to: string; env?: NodeJS.ProcessEnv; + assertCurrent?: () => void; + assertTargetCurrent?: (target: { agentId: string; sessionKey: string }) => void; }): Promise<{ groups: SessionGroupRecord[]; updatedSessions: number }> { const env = params.env ?? process.env; const from = normalizeOptionalString(params.name); @@ -205,9 +219,13 @@ export async function renameSessionGroup(params: { throw new Error("group rename requires non-empty names"); } if (from !== to) { + params.assertCurrent?.(); renameCatalogEntry(from, to, env); } - const updatedSessions = from === to ? 0 : await updateMemberCategories(params.cfg, from, to, env); + const updatedSessions = + from === to + ? 0 + : await updateMemberCategories(params.cfg, from, to, env, params.assertTargetCurrent); return { groups: listSessionGroups(env), updatedSessions }; } @@ -215,12 +233,15 @@ export async function deleteSessionGroup(params: { cfg: OpenClawConfig; name: string; env?: NodeJS.ProcessEnv; + assertCurrent?: () => void; + assertTargetCurrent?: (target: { agentId: string; sessionKey: string }) => void; }): Promise<{ groups: SessionGroupRecord[]; updatedSessions: number }> { const env = params.env ?? process.env; const name = normalizeOptionalString(params.name); if (!name) { throw new Error("group delete requires a non-empty name"); } + params.assertCurrent?.(); runOpenClawStateWriteTransaction( ({ db }) => { executeSqliteQuerySync( @@ -230,6 +251,12 @@ export async function deleteSessionGroup(params: { }, { env }, ); - const updatedSessions = await updateMemberCategories(params.cfg, name, undefined, env); + const updatedSessions = await updateMemberCategories( + params.cfg, + name, + undefined, + env, + params.assertTargetCurrent, + ); return { groups: listSessionGroups(env), updatedSessions }; } diff --git a/src/gateway/session-reset-service.ts b/src/gateway/session-reset-service.ts index 33cc9bd33a02..f7dd8713f352 100644 --- a/src/gateway/session-reset-service.ts +++ b/src/gateway/session-reset-service.ts @@ -926,6 +926,7 @@ export async function performGatewaySessionReset(params: { /** Trusted provenance for a reset that materializes a previously missing row. */ creation?: { via: SessionCreatedVia; actor?: SessionCreatedActor }; assertCurrent?: () => void; + assertAuthorizedInstance?: () => void; onCommitted?: (commit: { key: string; sessionId: string }) => void; }): Promise< | { @@ -1040,6 +1041,7 @@ export async function performGatewaySessionReset(params: { // lets aborted runs finish admission cleanup without deadlocking reset. prepare: async () => { params.assertCurrent?.(); + params.assertAuthorizedInstance?.(); admittedWorkReleased = await interruptSessionWorkAdmissions({ scope: resetTarget.storePath, identities: resetLifecycleIdentities, @@ -1113,8 +1115,10 @@ export async function performGatewaySessionReset(params: { }, ); params.assertCurrent?.(); + params.assertAuthorizedInstance?.(); await triggerInternalHook(hookEvent); params.assertCurrent?.(); + params.assertAuthorizedInstance?.(); // Cleanup below is destructive. Once it starts, finish rotating the same // session even if gateway ownership changes; otherwise runtime state can be // reset while the persisted session still points at the old conversation. @@ -1180,6 +1184,7 @@ export async function performGatewaySessionReset(params: { : undefined; let createdNewEntry = false; + params.assertAuthorizedInstance?.(); const boundaryEntry = loadSessionEntry( params.key, requestedAgentId ? { agentId: requestedAgentId } : undefined, @@ -1220,6 +1225,7 @@ export async function performGatewaySessionReset(params: { ], }, buildNextEntry: ({ currentEntry, primaryKey }) => { + params.assertAuthorizedInstance?.(); createdNewEntry = currentEntry === undefined; if (currentEntry?.sessionId !== boundaryEntry?.sessionId) { if (currentEntry) { diff --git a/src/gateway/session-sharing.test.ts b/src/gateway/session-sharing.test.ts index f68d0eff5659..22d17334d77e 100644 --- a/src/gateway/session-sharing.test.ts +++ b/src/gateway/session-sharing.test.ts @@ -5,7 +5,7 @@ import { withOpenClawTestState } from "../test-utils/openclaw-test-state.js"; import type { GatewayClient } from "./server-methods/types.js"; import { allowedSessionVisibilities, - authorizeSessionMutation, + resolveSessionMutationAuthorization, canReceiveSessionEvent, filterDraftSessionsForClient, resolveSessionSharingRole, @@ -169,16 +169,17 @@ describe("session sharing policy", () => { ["exec.approval.resolve", { id: "approval-1" }], ] as const) { expect( - authorizeSessionMutation({ client: outsider, method, requestParams, context }), + resolveSessionMutationAuthorization({ client: outsider, method, requestParams, context }) + .error, ).toMatchObject({ details: { code: "SESSION_PARTICIPATION_REQUIRED" } }); } expect( - authorizeSessionMutation({ + resolveSessionMutationAuthorization({ client: client({}), method: "chat.send", requestParams: { sessionKey: "agent:main:solo-draft" }, context, - }), + }).error, ).toBeNull(); }); }); @@ -186,20 +187,20 @@ describe("session sharing policy", () => { it("fails closed when a required session mutation has no target", () => { const context = { chatAbortControllers: new Map(), getRuntimeConfig: () => ({}) } as never; expect( - authorizeSessionMutation({ + resolveSessionMutationAuthorization({ client: client({}), method: "sessions.reset", requestParams: {}, context, - }), + }).error, ).toMatchObject({ details: { code: "SESSION_MUTATION_TARGET_REQUIRED" } }); expect( - authorizeSessionMutation({ + resolveSessionMutationAuthorization({ client: client({ scopes: ["operator.admin"] }), method: "sessions.reset", requestParams: {}, context, - }), + }).error, ).toBeNull(); }); diff --git a/src/gateway/session-sharing.ts b/src/gateway/session-sharing.ts index 9666dc401d8f..16bc5b72d605 100644 --- a/src/gateway/session-sharing.ts +++ b/src/gateway/session-sharing.ts @@ -15,7 +15,11 @@ import { listSessionEntries } from "../config/sessions/session-accessor.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; import { verifyBoardViewTicket } from "./board-view-ticket.js"; import { gatewayClientSessionCreator } from "./server-methods/gateway-client-identity.js"; -import type { GatewayClient, GatewayRequestContext } from "./server-methods/types.js"; +import type { + GatewayClient, + GatewayRequestContext, + SessionMutationAuthorization, +} from "./server-methods/types.js"; import type { GatewayWsClient } from "./server/ws-types.js"; import { resolveFreshestSessionStoreMatchFromStoreKeys, @@ -43,6 +47,24 @@ type SessionMutationTarget = { agentId?: string; }; +type AuthorizedSessionMutationTarget = SessionMutationTarget & { + resolved: Pick< + SessionSharingTarget, + "agentId" | "canonicalKey" | "storeKey" | "storePath" + > | null; + sessionId: string | null; +}; + +export class SessionMutationAuthorizationChangedError extends Error { + readonly error: ErrorShape; + + constructor(error: ErrorShape) { + super(error.message); + this.name = "SessionMutationAuthorizationChangedError"; + this.error = error; + } +} + const sharingSnapshotCache = new Map(); const sharingSnapshotAliases = new Map(); @@ -163,14 +185,21 @@ export function authorizeResolvedSessionMutation(params: { if (!target) { return null; } - const visibility = resolveSessionVisibility(target.entry); - const role = resolveSessionSharingRole({ client: params.client, target }); + return authorizeSessionSharingTarget({ client: params.client, target }); +} + +function authorizeSessionSharingTarget(params: { + client: GatewayClient | null; + target: SessionSharingTarget; +}): ErrorShape | null { + const visibility = resolveSessionVisibility(params.target.entry); + const role = resolveSessionSharingRole({ client: params.client, target: params.target }); return canMutateSession({ role, visibility }) ? null : errorShape(ErrorCodes.INVALID_REQUEST, `session is ${visibility} for this connection`, { details: { code: "SESSION_PARTICIPATION_REQUIRED", - sessionKey: params.sessionKey, + sessionKey: params.target.canonicalKey, visibility, }, }); @@ -354,14 +383,14 @@ function resolveSessionMutationTargets(params: { : undefined; } -export function authorizeSessionMutation(params: { +export function resolveSessionMutationAuthorization(params: { client: GatewayClient | null; method: string; requestParams: unknown; context: GatewayRequestContext; -}): ErrorShape | null { +}): { authorization?: SessionMutationAuthorization; error: ErrorShape | null } { if (isGatewayAdmin(params.client)) { - return null; + return { error: null }; } // Resolve runtime config at most once per request and only when a path needs it. The context // getter reloads/resolves gateway config, so non-session requests (the vast majority) must not @@ -377,25 +406,102 @@ export function authorizeSessionMutation(params: { }); if (!targetRefs) { if (REQUIRED_SESSION_TARGET_METHODS.has(params.method)) { - return errorShape(ErrorCodes.INVALID_REQUEST, "session mutation target is unavailable", { - details: { code: "SESSION_MUTATION_TARGET_REQUIRED", method: params.method }, - }); + return { + error: errorShape(ErrorCodes.INVALID_REQUEST, "session mutation target is unavailable", { + details: { code: "SESSION_MUTATION_TARGET_REQUIRED", method: params.method }, + }), + }; } - return null; + return { error: null }; } const cfg = getCfg(); + const authorizedTargets: AuthorizedSessionMutationTarget[] = []; for (const targetRef of targetRefs) { - const error = authorizeResolvedSessionMutation({ + const target = resolveSessionSharingTarget({ cfg, - client: params.client, sessionKey: targetRef.sessionKey, agentId: targetRef.agentId, }); + const error = target ? authorizeSessionSharingTarget({ client: params.client, target }) : null; if (error) { - return error; + return { error }; } + authorizedTargets.push({ + ...targetRef, + resolved: target + ? { + agentId: target.agentId, + canonicalKey: target.canonicalKey, + storeKey: target.storeKey, + storePath: target.storePath, + } + : null, + sessionId: target?.entry.sessionId?.trim() || null, + }); } - return null; + return { + error: null, + authorization: (() => { + const assertTargetCurrent = ( + targetRef: SessionMutationTarget, + expected: AuthorizedSessionMutationTarget | undefined, + currentCfg: OpenClawConfig, + ) => { + const current = resolveSessionSharingTarget({ + cfg: currentCfg, + sessionKey: targetRef.sessionKey, + agentId: targetRef.agentId, + }); + const sameResolvedTarget = + expected === undefined || + (current === null + ? expected.resolved === null + : expected.resolved !== null && + current.agentId === expected.resolved.agentId && + current.canonicalKey === expected.resolved.canonicalKey && + current.storeKey === expected.resolved.storeKey && + current.storePath === expected.resolved.storePath && + (current.entry.sessionId?.trim() || null) === expected.sessionId); + if (!sameResolvedTarget) { + throw new SessionMutationAuthorizationChangedError( + errorShape( + ErrorCodes.INVALID_REQUEST, + `session changed before ${params.method}; retry the request`, + { + details: { + code: "SESSION_MUTATION_AUTHORIZATION_CHANGED", + method: params.method, + sessionKey: targetRef.sessionKey, + }, + }, + ), + ); + } + if (!current) { + return; + } + const error = authorizeSessionSharingTarget({ client: params.client, target: current }); + if (error) { + throw new SessionMutationAuthorizationChangedError(error); + } + }; + return { + assertCurrent: () => { + const currentCfg = params.context.getRuntimeConfig(); + for (const authorized of authorizedTargets) { + assertTargetCurrent(authorized, authorized, currentCfg); + } + }, + assertTargetCurrent: (targetRef: SessionMutationTarget) => { + const expected = authorizedTargets.find( + (target) => + target.sessionKey === targetRef.sessionKey && target.agentId === targetRef.agentId, + ); + assertTargetCurrent(targetRef, expected, params.context.getRuntimeConfig()); + }, + }; + })(), + }; } function sharingSnapshotKey(sessionKey: string, agentId?: string): string { diff --git a/src/plugins/host-hook-state.ts b/src/plugins/host-hook-state.ts index 6d69108d71ab..f9d220694f26 100644 --- a/src/plugins/host-hook-state.ts +++ b/src/plugins/host-hook-state.ts @@ -280,6 +280,7 @@ export async function patchPluginSessionExtension(params: { namespace: string; value?: PluginJsonValue; unset?: boolean; + assertCurrent?: () => void; }): Promise<{ ok: true; key: string; value?: PluginJsonValue } | { ok: false; error: string }> { const namespace = normalizeNamespace(params.namespace); const pluginId = params.pluginId.trim(); @@ -318,6 +319,7 @@ export async function patchPluginSessionExtension(params: { const updated = await updateResolvedSessionEntry( { cfg: params.cfg, sessionKey: params.sessionKey }, (entry, context) => { + params.assertCurrent?.(); const entryRecord = entry as Record; const pluginExtensions = { ...entry.pluginExtensions }; const pluginState = { ...pluginExtensions[pluginId] };