romtuck/openclaw
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(browser-tool): disallow close without targetId to avoid unsafe tab ownership

Browse Source 
The close action previously allowed falling back to the active tab when
no targetId was provided. This implicit behavior makes tab ownership
ambiguous and unsafe in the multi-tool cooperative (concurrent) scenarios.

Without an explicit targetId, a close action may unintentionally close
a tab not created or owned by the caller.

This change enforces an explicit targetId for closing tabs and fails fast
otherwise, making destructive browser actions deterministic and ownership-safe.
This commit is contained in:
Jayden Liang 2026-01-28 20:49:26 -08:00
parent a7534dc223
commit 5906b5ab6e
No known key found for this signature in database
GPG Key ID: F22F8D6E50278D3D
2 changed files with 39 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

32
src/agents/tools/browser-tool.test.ts
View File

@ -289,3 +289,35 @@ describe("browser tool snapshot labels", () => {
expect(result?.content?.[1]).toMatchObject({ type: "image" });
});
});
describe("browser tool tab open and close behaviors", () => {
afterEach(() => {
vi.clearAllMocks();
configMocks.loadConfig.mockReturnValue({ browser: {} });
});
it("close a tab by targetId should be allowed in profile = 'clawd'", async () => {
const tool = createBrowserTool({ sandboxBridgeUrl: "http://127.0.0.1:9999" });
await tool.execute?.("", { action: "close", targetId: "a real target id", profile: "clawd" });
expect(browserClientMocks.browserCloseTab).toHaveBeenCalledWith(
"http://127.0.0.1:9999",
"a real target id",
{ profile: "clawd" },
);
});
it("close a tab by targetId should be allowed in profile = 'chrome'", async () => {
const tool = createBrowserTool({ sandboxBridgeUrl: "http://127.0.0.1:9999" });
await tool.execute?.("", { action: "close", targetId: "a real target id", profile: "clawd" });
expect(browserClientMocks.browserCloseTab).toHaveBeenCalledWith(
"http://127.0.0.1:9999",
"a real target id",
{ profile: "clawd" },
);
});
it("close a tab without targetId should throw error", async () => {
const tool = createBrowserTool({ sandboxBridgeUrl: "http://127.0.0.1:9999" });
await expect(tool.execute?.("", { action: "close" })).rejects.toThrow();
});
});

8
src/agents/tools/browser-tool.ts
View File

@ -407,7 +407,13 @@ export function createBrowserTool(opts?: {
return jsonResult(result);
}
if (targetId) await browserCloseTab(baseUrl, targetId, { profile });
else await browserAct(baseUrl, { kind: "close" }, { profile });
else {
// Known issue: The 'close' action does not verify the ownership of the current tab before atempting to close it, leading to potential action conflicts in concurrent scenarios. For now, enforce a strict close action with targetId for proxyRequest: path = "/act". The following line is commented out to prevent accidental usage. Instead, we throw an error to inform the user.
// await browserAct(baseUrl, { kind: "close" }, { profile });
throw new Error(
`Closing a tab without providing a targetId is not allowed. (But it was somehow allowed to do so in a previous version!). To avoid closing a tab not belonging to you, ensure you maintain a set of tabs by their targetId and provide a targetId when closing.`,
);
}
return jsonResult({ ok: true });
}
case "snapshot": {