Skip to content

Commit d405cda

Browse files
ZengWen-DTsteipete
ZengWen-DT
and
steipete
authored
fix(browser): resolve act targetId aliases before mismatch check (#96178)
* fix(browser): resolve act targetId aliases before mismatch check The /act top-level and batch targetId guards compared the caller-supplied targetId against the resolved canonical tab.targetId with raw string equality. Any supported alias form (tabId, label, suggestedTargetId, or a unique id prefix) resolves to a different canonical id, so act requests that followed the documented 'prefer suggestedTargetId/tabId/label' guidance were rejected with 403 ACT_TARGET_ID_MISMATCH even though they named the correct tab. snapshot/open/close/tabs lack this guard and kept working, matching the reported symptom matrix. Resolve the action targetId through the same tab alias resolution the route used and reject only ids that resolve to a different tab. * fix(browser): canonicalize act targetId aliases before Playwright dispatch The /act gate accepted tabId/label/suggested/prefix aliases of the request tab but left them on action.targetId. The managed executor reads action.targetId ?? targetId for an exact page lookup (executeSingleAction -> getPageForTargetId), so an alias missed the lookup and broke the action at runtime whenever more than one page was open (single-page masked it via the pages.length===1 fallback). Canonicalize the action targetId (top-level and nested batch sub-actions) to the resolved tab id before dispatch; reject ids that resolve to a different tab. Replace the mock-masked contract assertions with executor-action assertions plus direct canonicalizer unit tests. * test(browser): brace act-targetId guard clauses for curly lint * docs(changelog): note browser target alias fix * docs(changelog): note browser target alias fix * fix(browser): reject ambiguous batch target aliases * test(browser): type targetless act fixture * docs(changelog): move browser alias fix to unreleased * chore: drop nonessential browser changelog entry --------- Co-authored-by: Peter Steinberger <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>
1 parent a04b6ce commit d405cda

4 files changed

Lines changed: 134 additions & 26 deletions

File tree

‎extensions/browser/src/browser/routes/agent.act.normalize.test.ts‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,67 @@
11
// Browser tests cover agent.act.normalize plugin behavior.
22
import { describe, expect, it } from "vitest";
33
import { MAX_SAFE_TIMEOUT_DELAY_MS } from "../timer-delay.js";
4-
import { normalizeActRequest } from "./agent.act.normalize.js";
4+
import { canonicalizeActTargetIds, normalizeActRequest } from "./agent.act.normalize.js";
5+
6+
describe("canonicalizeActTargetIds", () => {
7+
const canonical = "abcd1234";
8+
const tab = { targetId: canonical, suggestedTargetId: "sg-1", tabId: "tab-7", label: "Inbox" };
9+
10+
it("rewrites every same-tab alias to the canonical targetId before dispatch", () => {
11+
for (const alias of ["abcd", "tab-7", "Inbox", "sg-1", canonical]) {
12+
const action = { kind: "click", ref: "1", targetId: alias } as const;
13+
expect(canonicalizeActTargetIds(action, tab)).toBeNull();
14+
expect(action.targetId).toBe(canonical);
15+
}
16+
});
17+
18+
it("canonicalizes batch sub-action aliases recursively", () => {
19+
const action = {
20+
kind: "batch",
21+
targetId: "abcd",
22+
actions: [
23+
{ kind: "click", ref: "1", targetId: "tab-7" },
24+
{ kind: "batch", actions: [{ kind: "resize", width: 2, height: 2, targetId: "Inbox" }] },
25+
],
26+
} satisfies Parameters<typeof canonicalizeActTargetIds>[0];
27+
expect(canonicalizeActTargetIds(action, tab, [tab])).toBeNull();
28+
expect(action.targetId).toBe(canonical);
29+
const [first, nested] = action.actions;
30+
expect(first?.targetId).toBe(canonical);
31+
if (nested?.kind !== "batch") {
32+
throw new Error("expected nested batch");
33+
}
34+
expect(nested.actions[0]?.targetId).toBe(canonical);
35+
});
36+
37+
it("leaves an absent targetId unset so dispatch falls back to the request tab", () => {
38+
const action: Parameters<typeof canonicalizeActTargetIds>[0] = { kind: "click", ref: "1" };
39+
expect(canonicalizeActTargetIds(action, tab)).toBeNull();
40+
expect(action.targetId).toBeUndefined();
41+
});
42+
43+
it("rejects ids that resolve to a different tab", () => {
44+
expect(canonicalizeActTargetIds({ kind: "click", ref: "1", targetId: "zzzz9999" }, tab)).toBe(
45+
"action targetId must match request targetId",
46+
);
47+
expect(
48+
canonicalizeActTargetIds(
49+
{ kind: "batch", actions: [{ kind: "click", ref: "1", targetId: "zzzz9999" }] },
50+
tab,
51+
),
52+
).toBe("batched action targetId must match request targetId");
53+
});
54+
55+
it("rejects a batched targetId prefix that is ambiguous across tabs", () => {
56+
expect(
57+
canonicalizeActTargetIds(
58+
{ kind: "batch", actions: [{ kind: "click", ref: "1", targetId: "abcd" }] },
59+
tab,
60+
[tab, { targetId: "abcd9999" }],
61+
),
62+
).toBe("batched action targetId must match request targetId");
63+
});
64+
});
565

666
describe("normalizeActRequest numeric fields", () => {
767
it("keeps structured numeric action options", () => {

‎extensions/browser/src/browser/routes/agent.act.normalize.ts‎

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
} from "../act-policy.js";
1414
import type { BrowserActRequest, BrowserFormField } from "../client-actions.types.js";
1515
import { normalizeBrowserFormField } from "../form-fields.js";
16+
import { resolveTargetIdFromTabs } from "../target-id.js";
1617
import {
1718
type ActKind,
1819
isActKind,
@@ -45,19 +46,28 @@ function countBatchActions(actions: BrowserActRequest[]): number {
4546
return count;
4647
}
4748

48-
/** Validate that nested batch actions cannot drift to a different target tab. */
49-
export function validateBatchTargetIds(
50-
actions: BrowserActRequest[],
51-
targetId: string,
49+
/** Keep nested action overrides inside the route-selected tab. */
50+
export function canonicalizeActTargetIds(
51+
action: BrowserActRequest,
52+
tab: { targetId: string; suggestedTargetId?: string; tabId?: string; label?: string },
53+
tabs = [tab],
54+
batched = false,
5255
): string | null {
53-
for (const action of actions) {
54-
if (action.targetId && action.targetId !== targetId) {
55-
return "batched action targetId must match request targetId";
56+
if (action.targetId) {
57+
const resolved = resolveTargetIdFromTabs(action.targetId, batched ? tabs : [tab]);
58+
if (!resolved.ok || resolved.targetId !== tab.targetId) {
59+
return batched
60+
? "batched action targetId must match request targetId"
61+
: "action targetId must match request targetId";
5662
}
57-
if (action.kind === "batch") {
58-
const nestedError = validateBatchTargetIds(action.actions, targetId);
59-
if (nestedError) {
60-
return nestedError;
63+
// The Playwright executor treats action.targetId as an exact override.
64+
action.targetId = tab.targetId;
65+
}
66+
if (action.kind === "batch") {
67+
for (const subAction of action.actions) {
68+
const error = canonicalizeActTargetIds(subAction, tab, tabs, true);
69+
if (error) {
70+
return error;
6171
}
6272
}
6373
}

‎extensions/browser/src/browser/routes/agent.act.ts‎

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ import {
3636
jsonActError,
3737
} from "./agent.act.errors.js";
3838
import { registerBrowserAgentActHookRoutes } from "./agent.act.hooks.js";
39-
import { normalizeActRequest, validateBatchTargetIds } from "./agent.act.normalize.js";
39+
import { canonicalizeActTargetIds, normalizeActRequest } from "./agent.act.normalize.js";
4040
import { type ActKind, isActKind } from "./agent.act.shared.js";
4141
import {
4242
readBody,
@@ -436,13 +436,16 @@ export function registerBrowserAgentActRoutes(
436436
...extra,
437437
});
438438
};
439-
if (action.targetId && action.targetId !== tab.targetId) {
440-
return jsonActError(
441-
res,
442-
403,
443-
ACT_ERROR_CODES.targetIdMismatch,
444-
"action targetId must match request targetId",
445-
);
439+
// Nested batch aliases can differ from the request alias, so prefixes
440+
// must stay unique across the full tab set before canonicalization.
441+
const actionTabs =
442+
action.kind === "batch" && !isExistingSession ? await profileCtx.listTabs() : [tab];
443+
if (!actionTabs.some((candidate) => candidate.targetId === tab.targetId)) {
444+
actionTabs.unshift(tab);
445+
}
446+
const targetIdError = canonicalizeActTargetIds(action, tab, actionTabs);
447+
if (targetIdError) {
448+
return jsonActError(res, 403, ACT_ERROR_CODES.targetIdMismatch, targetIdError);
446449
}
447450
const profileName = profileCtx.profile.name;
448451
if (isExistingSession) {
@@ -656,12 +659,6 @@ export function registerBrowserAgentActRoutes(
656659
if (!pw) {
657660
return;
658661
}
659-
if (action.kind === "batch") {
660-
const targetIdError = validateBatchTargetIds(action.actions, tab.targetId);
661-
if (targetIdError) {
662-
return jsonActError(res, 403, ACT_ERROR_CODES.targetIdMismatch, targetIdError);
663-
}
664-
}
665662
const result = await pw.executeActViaPlaywright({
666663
cdpUrl,
667664
action,

‎extensions/browser/src/browser/server.agent-contract-core.test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,47 @@ describe("browser control server", () => {
196196
slowTimeoutMs,
197197
);
198198

199+
it(
200+
"canonicalizes a unique targetId prefix to the request tab before dispatch",
201+
async () => {
202+
const base = await startServerAndBase();
203+
// "abcd" is a unique prefix of the canonical targetId "abcd1234". The route
204+
// must rewrite the action's targetId to the canonical id, because the
205+
// Playwright executor reads `action.targetId ?? targetId` for an exact page
206+
// lookup; a surviving alias would miss the lookup and break at runtime.
207+
const response = await postJson<{ ok: boolean }>(`${base}/act`, {
208+
kind: "click",
209+
ref: "1",
210+
targetId: "abcd",
211+
});
212+
213+
expect(response.ok).toBe(true);
214+
const execArgs = mockFirstArg(pwMocks.executeActViaPlaywright, 0, "executeAct");
215+
const action = execArgs.action as { targetId?: string };
216+
expect(action.targetId).toBe("abcd1234");
217+
},
218+
slowTimeoutMs,
219+
);
220+
221+
it(
222+
"canonicalizes a batched sub-action targetId alias before dispatch",
223+
async () => {
224+
const base = await startServerAndBase();
225+
const response = await postJson<{ ok: boolean }>(`${base}/act`, {
226+
kind: "batch",
227+
targetId: "abcd1234",
228+
// Sub-action references the same tab via a unique prefix alias.
229+
actions: [{ kind: "click", ref: "1", targetId: "abcd" }],
230+
});
231+
232+
expect(response.ok).toBe(true);
233+
const execArgs = mockFirstArg(pwMocks.executeActViaPlaywright, 0, "executeAct");
234+
const action = execArgs.action as { actions?: Array<{ targetId?: string }> };
235+
expect(action.actions?.[0]?.targetId).toBe("abcd1234");
236+
},
237+
slowTimeoutMs,
238+
);
239+
199240
it(
200241
"returns the replacement targetId after an action-triggered target swap",
201242
async () => {

0 commit comments

Comments
 (0)