fix(ui): reject stale session delete confirmations (#124097)

* fix(ui): bind session deletes to listed identity

* fix(gateway): authorize guarded session deletes

* test(ui): expect guarded batch delete identity
This commit is contained in:
Peter Steinberger
2026-08-15 21:43:10 -07:00
committed by GitHub
parent f61ec66249
commit 0e65272e21
17 changed files with 230 additions and 84 deletions
+15
View File
@@ -621,6 +621,21 @@ describe("method scope resolution", () => {
archivedOnly: true,
expectedSessionId: "sess-1",
}),
).toEqual({ allowed: true });
expect(
resolveLeastPrivilegeOperatorScopesForMethod("sessions.delete", {
key: "agent:main:old",
archivedOnly: true,
expectedSessionId: "sess-1",
}),
).toEqual(["operator.write"]);
expect(
authorizeOperatorScopesForMethod("sessions.delete", ["operator.write"], {
key: "agent:main:old",
archivedOnly: true,
expectedSessionId: "sess-1",
futureField: true,
}),
).toEqual({ allowed: false, missingScope: "operator.admin" });
expect(
resolveLeastPrivilegeOperatorScopesForMethod("sessions.delete", {
+21 -10
View File
@@ -124,21 +124,32 @@ describe("resolveDynamicSessionMutationRequiredScope", () => {
key: "agent:main:archived",
deleteTranscript: true,
archivedOnly: true,
expectedSessionId: "session-1",
}),
).toBe("operator.write");
expect(
resolveDynamicSessionMutationRequiredScope("sessions.delete", {
key: "agent:main:active",
deleteTranscript: true,
}),
).toBe("operator.admin");
expect(
resolveDynamicSessionMutationRequiredScope("sessions.delete", {
for (const params of [
undefined,
null,
[],
{ key: "agent:main:active", deleteTranscript: true },
{ key: "agent:main:archived", archivedOnly: "yes" },
{
key: "agent:main:archived",
archivedOnly: true,
expectedSessionId: "session-1",
emitLifecycleHooks: false,
}),
).toBe("operator.admin");
},
{
key: "agent:main:archived",
archivedOnly: true,
expectedSessionId: "session-1",
futureField: true,
},
]) {
expect(resolveDynamicSessionMutationRequiredScope("sessions.delete", params)).toBe(
"operator.admin",
);
}
});
it("does not duplicate static method policy from the core descriptor table", () => {
+1
View File
@@ -25,6 +25,7 @@ const SESSIONS_DELETE_WRITE_SCOPE_FIELDS: ReadonlySet<string> = new Set([
"key",
"agentId",
"deleteTranscript",
"expectedSessionId",
"archivedOnly",
]);
+1 -1
View File
@@ -771,7 +771,7 @@ export class SessionDataController implements ReactiveController, SessionCatalog
publishSessionMutationError(scope: SidebarSessionMutationScope, error: unknown): void {
if (this.isSessionMutationScopeCurrent(scope)) {
this.sessionMutationError = String(error);
this.sessionMutationError = error instanceof Error ? error.message : String(error);
this.notify();
}
}
+5 -1
View File
@@ -63,7 +63,11 @@ export function sessionMenuReasons(params: {
.map((row) =>
reason({
method: "sessions.delete",
params: { key: row.key, ...(row.archived ? { archivedOnly: true } : {}) },
params: {
key: row.key,
...(row.sessionId ? { expectedSessionId: row.sessionId } : {}),
...(row.archived ? { archivedOnly: true } : {}),
},
}),
)
.find((value): value is string => Boolean(value));
@@ -10,6 +10,7 @@ import type { ApplicationGatewaySnapshot } from "../app/gateway.ts";
import { loadSettings, patchSettings } from "../app/settings.ts";
import { t } from "../i18n/index.ts";
import type { SessionCapability } from "../lib/sessions/index.ts";
import type { SessionDeleteBatchResult } from "../lib/sessions/session-capability.ts";
import { showToast } from "../lib/toast.ts";
import {
answerConfirmDialog,
@@ -95,7 +96,13 @@ function createHarness(
} as ApplicationGatewaySnapshot;
const refreshReplacement = vi.fn(async () => undefined);
const refreshTheme = vi.fn();
const deleteMany = vi.fn(async () => ({ deleted: [], errors: [], preservedWorktrees: [] }));
const deleteMany = vi.fn(
async (): Promise<SessionDeleteBatchResult> => ({
deleted: [],
errors: [],
preservedWorktrees: [],
}),
);
const deleteOne = vi.fn(async () => ({ deleted: true }));
const groupsDelete = vi.fn(async () => "completed" as const);
const scope = {
@@ -472,6 +479,12 @@ describe("session organizer destructive confirmations", () => {
it("renders the localized batch-delete copy in-app and deletes once accepted", async () => {
const harness = createHarness(destructiveHarness);
const rows = [sessionRow(0), sessionRow(1)];
const retryError = `Session ${rows[0]!.key} changed before deletion. Retry.`;
harness.deleteMany.mockResolvedValueOnce({
deleted: [rows[1]!.key],
errors: [retryError],
preservedWorktrees: [],
});
const pending = deleteSessionsBatch(harness.host, rows, harness.scope);
const actions = await waitForConfirmDialogActions();
@@ -482,9 +495,21 @@ describe("session organizer destructive confirmations", () => {
await pending;
expect(harness.deleteMany).toHaveBeenCalledWith([
{ key: rows[0]!.key, agentId: "main", deleteTranscript: true },
{ key: rows[1]!.key, agentId: "main", deleteTranscript: true },
{
key: rows[0]!.key,
agentId: "main",
deleteTranscript: true,
expectedSessionId: rows[0]!.sessionId,
},
{
key: rows[1]!.key,
agentId: "main",
deleteTranscript: true,
expectedSessionId: rows[1]!.sessionId,
},
]);
expect(harness.publishSessionMutationError).toHaveBeenCalledWith(harness.scope, retryError);
expect(retryError).not.toContain("GatewayRequestError");
});
it.each(destructiveOperations)("sends no $name request when cancelled", async (operation) => {
@@ -565,7 +590,11 @@ describe("session organizer destructive confirmations", () => {
await deleteSession(harness.host, sessionRow(0), harness.scope, { offerSkip: true });
expect(document.body.querySelector("openclaw-modal-dialog")).toBeNull();
expect(harness.deleteOne).toHaveBeenCalledOnce();
expect(harness.deleteOne).toHaveBeenCalledWith(sessionRow(0).key, {
agentId: "main",
deleteTranscript: true,
expectedSessionId: sessionRow(0).sessionId,
});
});
it("asks again after the preference is reset", async () => {
@@ -294,6 +294,7 @@ export async function deleteSessionsBatch(
key: row.key,
agentId: parseAgentSessionKey(row.key)?.agentId ?? scope.selectedAgentId,
deleteTranscript: true,
...(row.sessionId ? { expectedSessionId: row.sessionId } : {}),
...(row.archived === true ? { archivedOnly: true } : {}),
}));
for (const params of requests) {
@@ -578,6 +579,7 @@ export async function deleteSession(
const deleteParams = {
agentId,
deleteTranscript: true,
...(session.sessionId ? { expectedSessionId: session.sessionId } : {}),
...(session.archived === true ? { archivedOnly: true } : {}),
};
if (
@@ -847,6 +847,7 @@ suite.define(() => {
expect(requireRecord(request.params)).toMatchObject({
archivedOnly: true,
deleteTranscript: true,
expectedSessionId: `session:${key}`,
key,
});
await row.waitFor({ state: "visible" });
@@ -0,0 +1,96 @@
import path from "node:path";
import { expect, it } from "vitest";
import { expectRequestCountStable } from "./chat-flow.test-support.ts";
import {
captureUiProof,
captureUiProofEnabled,
createSessionManagementE2eSuite,
installMockGateway,
sessionRow,
sessionsListResponse,
uiProofArtifactDir,
waitForConfirmModal,
} from "./session-management.test-support.ts";
const suite = createSessionManagementE2eSuite();
suite.define(() => {
it("rejects deleting a same-key replacement after the in-app confirm", async () => {
const key = "agent:main:research";
const context = await suite.browser.newContext({
locale: "en-US",
serviceWorkers: "block",
viewport: { height: 900, width: 1280 },
recordVideo: captureUiProofEnabled
? { dir: uiProofArtifactDir, size: { height: 900, width: 1280 } }
: undefined,
});
const page = await context.newPage();
const proofVideo = page.video();
// Playwright auto-dismisses native dialogs, which is exactly how a
// bridge-less WebView behaves. Deleting must not depend on one.
const nativeDialogs: string[] = [];
page.on("dialog", (dialog) => {
nativeDialogs.push(dialog.message());
void dialog.dismiss();
});
const gateway = await installMockGateway(page, {
methodResponses: {
"sessions.delete": { ok: true, deleted: true },
"sessions.list": sessionsListResponse([
sessionRow("agent:main:main", "Main", Date.parse("2026-07-01T16:00:00.000Z")),
sessionRow(key, "Research notes", Date.parse("2026-07-01T15:00:00.000Z")),
]),
},
sessionKey: "agent:main:main",
});
try {
await page.goto(`${suite.server.baseUrl}chat`);
const row = page.locator(`.sidebar-recent-session[data-session-key="${key}"]`);
await row.waitFor({ state: "visible", timeout: 10_000 });
await row.hover();
await row.getByRole("button", { name: "Open session menu" }).click();
await page
.locator("openclaw-session-menu")
.getByRole("menuitem", { name: "Delete…" })
.click();
const confirmModal = await waitForConfirmModal(page);
await captureUiProof(page, "sidebar-delete-session-confirm.png");
await gateway.deferNext("sessions.delete");
await confirmModal.getByRole("button", { name: "Delete", exact: true }).evaluate((button) => {
if (!(button instanceof HTMLButtonElement)) {
throw new Error("expected delete confirmation button");
}
button.click();
button.click();
});
const request = await gateway.waitForRequest("sessions.delete");
expect(request).toMatchObject({
params: { deleteTranscript: true, expectedSessionId: `session:${key}`, key },
});
await expectRequestCountStable(gateway, "sessions.delete", 1);
await gateway.rejectDeferred("sessions.delete", {
code: "INVALID_REQUEST",
message: `Session ${key} changed before deletion. Retry.`,
});
const visibleError = page.locator("[data-sidebar-session-error]");
await expect
.poll(() => visibleError.textContent())
.toContain("changed before deletion. Retry.");
expect(await visibleError.textContent()).not.toContain("GatewayRequestError");
await row.waitFor({ state: "visible" });
await captureUiProof(page, "sidebar-delete-session-replaced-error.png");
expect(nativeDialogs).toEqual([]);
} finally {
await context.close();
if (proofVideo) {
await proofVideo.saveAs(
path.join(uiProofArtifactDir, "sidebar-delete-session-replaced.webm"),
);
}
}
});
});
@@ -19,7 +19,6 @@ import {
sessionsListResponse,
trimmedTextContents,
uiProofArtifactDir,
waitForConfirmModal,
waitForPatch,
} from "./session-management.test-support.ts";
@@ -1003,53 +1002,4 @@ suite.define(() => {
await context.close();
}
});
it("deletes a sidebar session through the in-app confirm", async () => {
const key = "agent:main:research";
const context = await suite.browser.newContext({
locale: "en-US",
serviceWorkers: "block",
viewport: { height: 900, width: 1280 },
});
const page = await context.newPage();
// Playwright auto-dismisses native dialogs, which is exactly how a
// bridge-less WebView behaves. Deleting must not depend on one.
const nativeDialogs: string[] = [];
page.on("dialog", (dialog) => {
nativeDialogs.push(dialog.message());
void dialog.dismiss();
});
const gateway = await installMockGateway(page, {
methodResponses: {
"sessions.delete": { ok: true, deleted: true },
"sessions.list": sessionsListResponse([
sessionRow("agent:main:main", "Main", Date.parse("2026-07-01T16:00:00.000Z")),
sessionRow(key, "Research notes", Date.parse("2026-07-01T15:00:00.000Z")),
]),
},
sessionKey: "agent:main:main",
});
try {
await page.goto(`${suite.server.baseUrl}chat`);
const row = page.locator(`.sidebar-recent-session[data-session-key="${key}"]`);
await row.waitFor({ state: "visible", timeout: 10_000 });
await row.hover();
await row.getByRole("button", { name: "Open session menu" }).click();
await page
.locator("openclaw-session-menu")
.getByRole("menuitem", { name: "Delete…" })
.click();
const confirmModal = await waitForConfirmModal(page);
await captureUiProof(page, "sidebar-delete-session-confirm.png");
await confirmModal.getByRole("button", { name: "Delete", exact: true }).click();
await expect(gateway.waitForRequest("sessions.delete")).resolves.toMatchObject({
params: { deleteTranscript: true, key },
});
expect(nativeDialogs).toEqual([]);
} finally {
await context.close();
}
});
});
+26 -5
View File
@@ -364,18 +364,31 @@ describe("createSessionCapability", () => {
const { gateway } = createGatewayHarness(client);
const sessions = createSessionCapability(gateway);
await expect(sessions.delete(key)).resolves.toEqual({ deleted: false });
await expect(
sessions.delete(key, { expectedSessionId: "session-before-replacement" }),
).resolves.toEqual({ deleted: false });
expect(sessions.state.deletedSessions).toEqual([]);
expect(request).toHaveBeenCalledTimes(1);
expect(request).toHaveBeenCalledWith("sessions.delete", {
key,
deleteTranscript: true,
expectedSessionId: "session-before-replacement",
});
sessions.dispose();
});
it("excludes lifecycle no-ops from batch deletion results", async () => {
const rejectedKey = "agent:main:rejected";
const keptKey = "agent:main:kept";
const deletedKey = "agent:main:deleted";
const request = vi.fn(async (method: string, params?: unknown) => {
if (method === "sessions.delete") {
const key = (params as { key?: string } | undefined)?.key;
if (key === rejectedKey) {
throw new GatewayRequestError({
code: "INVALID_REQUEST",
message: `Session ${key} changed before deletion. Retry.`,
});
}
return { ok: true, deleted: key === deletedKey };
}
if (method === "sessions.list") {
@@ -392,11 +405,19 @@ describe("createSessionCapability", () => {
});
await expect(
sessions.deleteMany([{ key: keptKey }, { key: deletedKey, archivedOnly: true }]),
).resolves.toEqual({ deleted: [deletedKey], errors: [], preservedWorktrees: [] });
sessions.deleteMany([
{ key: rejectedKey },
{ key: keptKey },
{ key: deletedKey, archivedOnly: true },
]),
).resolves.toEqual({
deleted: [deletedKey],
errors: [`Session ${rejectedKey} changed before deletion. Retry.`],
preservedWorktrees: [],
});
expect(deletedSnapshots.some((keys) => keys.includes(deletedKey))).toBe(true);
expect(deletedSnapshots.some((keys) => keys.includes(keptKey))).toBe(false);
expect(request).toHaveBeenCalledTimes(3);
expect(request).toHaveBeenCalledTimes(4);
expect(request).toHaveBeenCalledWith("sessions.delete", {
key: deletedKey,
deleteTranscript: true,
+2 -4
View File
@@ -77,14 +77,12 @@ export type SessionListSnapshot = Pick<SessionState, "result" | "agentId" | "loa
export type SessionDeleteOptions = {
agentId?: string;
deleteTranscript?: boolean;
expectedSessionId?: string;
archivedOnly?: boolean;
};
export type SessionDeleteTarget = {
export type SessionDeleteTarget = SessionDeleteOptions & {
key: string;
agentId?: string;
deleteTranscript?: boolean;
archivedOnly?: boolean;
};
/** Dirty/unpushed checkouts survive session deletion; callers surface them. */
+3 -2
View File
@@ -3,6 +3,7 @@ import type {
SessionsListResult,
SessionsPatchResult,
} from "../../api/types.ts";
import { formatUiError } from "../format-error.ts";
import {
requestSessionCreate,
resolveSessionCreateParams,
@@ -421,7 +422,7 @@ export function createSessionMutations(host: SessionMutationsHost) {
if (!host.connection.isCurrent(scope)) {
return { deleted: false };
}
host.publish({ ...host.readState(), error: String(error) }, "operation");
host.publish({ ...host.readState(), error: formatUiError(error) }, "operation");
throw error;
}
};
@@ -452,7 +453,7 @@ export function createSessionMutations(host: SessionMutationsHost) {
}
}
} catch (error) {
errors.push(String(error));
errors.push(formatUiError(error));
}
}
if (deleted.length > 0 && host.connection.isCurrent(scope)) {
+1
View File
@@ -157,6 +157,7 @@ export function requestSessionDelete(
return client.request<SessionDeleteResponse>("sessions.delete", {
...buildSessionRequestParams(key, options.agentId),
deleteTranscript: options.deleteTranscript ?? true,
...(options.expectedSessionId ? { expectedSessionId: options.expectedSessionId } : {}),
...(options.archivedOnly === true ? { archivedOnly: true } : {}),
});
}
+9 -4
View File
@@ -631,19 +631,22 @@ describe("sessions page lifecycle", () => {
it("retargets the Gateway after deleting the current session", async () => {
const key = "agent:writer:work";
const sessionId = "session-writer-work";
const sessions = createSessions({
deleteMany: vi.fn(async () => ({ deleted: [key], errors: [], preservedWorktrees: [] })),
});
const mutableGateway = createGateway({} as GatewayBrowserClient);
mutableGateway.emit({ sessionKey: key });
const page = await createPage(createContext(mutableGateway.gateway, sessions));
page.result = { count: 1, sessions: [{ key }] } as SessionsListResult;
page.result = { count: 1, sessions: [{ key, sessionId }] } as SessionsListResult;
page.selectedKeys = new Set([key]);
vi.mocked(showConfirmDialog).mockResolvedValue(true);
await page.deleteSelected();
expect(sessions.deleteMany).toHaveBeenCalledWith([{ key, agentId: undefined }]);
expect(sessions.deleteMany).toHaveBeenCalledWith([
{ key, agentId: undefined, expectedSessionId: sessionId },
]);
expect(mutableGateway.setSessionKey).toHaveBeenCalledWith("agent:writer:main");
expect(page.result?.sessions).toEqual([]);
expect(page.selectedKeys).toEqual(new Set());
@@ -717,10 +720,11 @@ describe("sessions page lifecycle", () => {
const activeKey = "agent:main:active";
const archivedKey = "agent:main:archived";
const unknownKey = "agent:main:unknown";
const retryError = `Session ${archivedKey} changed before deletion. Retry.`;
const sessions = createSessions({
deleteMany: vi.fn(async () => ({
deleted: [archivedKey],
errors: ["active denied", "unknown denied"],
errors: [retryError],
preservedWorktrees: [],
})),
});
@@ -748,7 +752,8 @@ describe("sessions page lifecycle", () => {
sessions: [{ key: activeKey, archived: false }],
});
expect(page.selectedKeys).toEqual(new Set([activeKey, unknownKey]));
expect(page.error).toBe("active denied; unknown denied");
expect(page.error).toBe(retryError);
expect(page.error).not.toContain("GatewayRequestError");
});
it("stops an active cloud worker and refreshes the session roster", async () => {
+2 -1
View File
@@ -100,7 +100,7 @@ type SessionsPageMutationResult = "completed" | "failed" | "stale";
/** Type-only, so the dialog itself stays behind its lazy boundary. */
type InputDialogOpener = (typeof import("../../components/input-dialog.ts"))["showInputDialog"];
type SessionDeleteRow = Pick<GatewaySessionRow, "key" | "archived">;
type SessionDeleteRow = Pick<GatewaySessionRow, "key" | "archived" | "sessionId">;
class SessionsPage extends OpenClawLightDomElement {
@consume({ context: applicationContext, subscribe: true })
@@ -780,6 +780,7 @@ class SessionsPage extends OpenClawLightDomElement {
key: row.key,
agentId: this.sessionAgentId(row.key, scope.context),
...options,
...(row.sessionId ? { expectedSessionId: row.sessionId } : {}),
...(row.archived === true ? { archivedOnly: true } : {}),
}));
for (const params of requests) {
@@ -286,8 +286,18 @@ describe("AppSidebar multi-select", () => {
await waitForFast(() => expect(harness.deleteMany).toHaveBeenCalledOnce());
expect(harness.deleteMany).toHaveBeenCalledWith([
{ key: "agent:main:a", agentId: "main", deleteTranscript: true },
{ key: "agent:main:b", agentId: "main", deleteTranscript: true },
{
key: "agent:main:a",
agentId: "main",
deleteTranscript: true,
expectedSessionId: "session:agent:main:a",
},
{
key: "agent:main:b",
agentId: "main",
deleteTranscript: true,
expectedSessionId: "session:agent:main:b",
},
]);
} finally {
restoreDialogPolyfill();