mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-25 11:55:47 -06:00
fix(sessions): reject stale mutations after session replacement (#113023)
* fix(sessions): bind mutation auth to session instance * fix(sessions): preserve reset lifecycle ownership
This commit is contained in:
committed by
GitHub
parent
41404f669a
commit
ee99c6eb18
@@ -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) };
|
||||
|
||||
@@ -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<void>((resolve) => {
|
||||
continueHandler = resolve;
|
||||
});
|
||||
let markHandlerStarted = () => {};
|
||||
const handlerStarted = new Promise<void>((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<typeof handleGatewayRequest>[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<typeof handleGatewayRequest>[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");
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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)));
|
||||
}
|
||||
},
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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, {
|
||||
|
||||
@@ -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. */
|
||||
|
||||
@@ -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<number> {
|
||||
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 };
|
||||
}
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
|
||||
|
||||
+121
-15
@@ -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<string, SessionSharingSnapshot>();
|
||||
const sharingSnapshotAliases = new Map<string, string>();
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
const pluginExtensions = { ...entry.pluginExtensions };
|
||||
const pluginState = { ...pluginExtensions[pluginId] };
|
||||
|
||||
Reference in New Issue
Block a user