Skip to content

Commit 03b80f4

Browse files
authored
fix(desktop): keep sign-in popups working in the agent browser (#9506)
* fix(desktop): keep sign-in popups working in the agent browser - Adopt window.open('') / about:blank popups as tabs that keep window.opener (MSAL-style sign-in and payment flows got null) - Read an adopted popup's session from its contents, not its URL - Refuse a form POST into a new tab rather than resending it as a GET; forward the referrer on reissued links - Closing a popup returns to its opener unless the user switched tabs since (Chrome's rule) - mailto: links in the app window open the mail client, with attach/extra headers refused * fix(desktop): keep deferred popups in their opener's session - A background-tab popup with no Chromium-supplied contents now opens in the opener's session rather than the agent partition - Driver tests expect the referrer options loadURL now receives
1 parent 3dfcaa0 commit 03b80f4

6 files changed

Lines changed: 341 additions & 48 deletions

File tree

‎apps/desktop/e2e/browser-tools.spec.ts‎

Lines changed: 161 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { mkdtempSync } from 'node:fs'
2-
import { createServer, type Server } from 'node:http'
2+
import { createServer, type IncomingMessage, type Server } from 'node:http'
33
import { tmpdir } from 'node:os'
44
import { join } from 'node:path'
55
import { fileURLToPath } from 'node:url'
@@ -62,6 +62,36 @@ const CAPABILITIES_FIXTURE = `<!doctype html><title>Capabilities fixture</title>
6262
const POPUP_FIXTURE = `<!doctype html><title>Authorize fixture</title>
6363
<button onclick="window.opener.postMessage('granted', '*'); window.close()">Allow</button>`
6464

65+
/** Sign-in and payment popup patterns: blank-then-navigate, and form posts into a new tab. */
66+
const popupsFixture = (
67+
appOrigin: string,
68+
popupOrigin: string
69+
) => `<!doctype html><title>Popups fixture</title>
70+
<form id="same-session" method="post" action="${popupOrigin}/echo" target="_blank"><input name="note" value="hello"></form>
71+
<form id="cross-session" method="post" action="${appOrigin}/echo" target="_blank"><input name="note" value="hello"></form>
72+
<script>
73+
let popup = null;
74+
function signIn() {
75+
popup = window.open('', 'signin', 'width=400,height=400');
76+
document.body.dataset.handle = String(popup !== null);
77+
popup?.document.write('<script>opener.postMessage("ready", "*")<\\/script>');
78+
}
79+
addEventListener('message', (event) => {
80+
if (event.data !== 'ready') return void (document.body.dataset.connected = event.data);
81+
document.body.dataset.blank = 'ready';
82+
popup.location.href = new URLSearchParams(location.search).get('next') || '${popupOrigin}/authorize';
83+
});
84+
</script>`
85+
86+
async function readBody(request: IncomingMessage): Promise<string> {
87+
let body = ''
88+
for await (const chunk of request) body += chunk.toString()
89+
return body
90+
}
91+
92+
const echoPage = (method: string | undefined, body: string) =>
93+
`<!doctype html><title>${method} ${body}</title>`
94+
6595
const UPLOAD_TARGET_FIXTURE = `<!doctype html><title>Upload target fixture</title>
6696
<div id="surface"></div>
6797
<script>
@@ -105,6 +135,7 @@ test.describe('browser tools', () => {
105135
let window: Page
106136
let callCount = 0
107137
let beforeUploadResponse: (() => Promise<void>) | undefined
138+
const appEchoes: string[] = []
108139

109140
test.beforeAll(async () => {
110141
server = createServer(async (request, response) => {
@@ -153,8 +184,7 @@ test.describe('browser tools', () => {
153184
return
154185
}
155186
if (path === '/api/desktop/tool/file') {
156-
let body = ''
157-
for await (const chunk of request) body += chunk.toString()
187+
const body = await readBody(request)
158188
const { toolCallId, index } = JSON.parse(body)
159189
const reference = claimed.get(toolCallId)?.args.paths
160190
const found = Array.isArray(reference) && reference[index] === 'files/receipt.txt'
@@ -192,9 +222,25 @@ test.describe('browser tools', () => {
192222
)
193223
return
194224
}
225+
if (path === '/echo') {
226+
const body = await readBody(request)
227+
appEchoes.push(`${request.method} ${body}`)
228+
response.writeHead(200, { 'Content-Type': 'text/html' })
229+
response.end(echoPage(request.method, body))
230+
return
231+
}
232+
if (path === '/authorize') {
233+
response.writeHead(200, { 'Content-Type': 'text/html' })
234+
response.end(POPUP_FIXTURE)
235+
return
236+
}
237+
if (path === '/popups') {
238+
response.writeHead(200, { 'Content-Type': 'text/html' })
239+
response.end(popupsFixture(origin, popupOrigin))
240+
return
241+
}
195242
if (path === '/api/desktop/tool/authorize') {
196-
let body = ''
197-
for await (const chunk of request) body += chunk.toString()
243+
const body = await readBody(request)
198244
const authorization = calls.get(JSON.parse(body).toolCallId)
199245
calls.delete(JSON.parse(body).toolCallId)
200246
if (authorization) claimed.set(JSON.parse(body).toolCallId, authorization)
@@ -224,9 +270,11 @@ test.describe('browser tools', () => {
224270
origin = `http://127.0.0.1:${address.port}`
225271
/** Pages outside the app origin browse in the agent partition, like any third-party site. */
226272
site = origin.replace('127.0.0.1', 'localhost')
227-
popupServer = createServer((_request, response) => {
273+
popupServer = createServer(async (request, response) => {
228274
response.writeHead(200, { 'Content-Type': 'text/html' })
229-
response.end(POPUP_FIXTURE)
275+
response.end(
276+
request.url === '/echo' ? echoPage(request.method, await readBody(request)) : POPUP_FIXTURE
277+
)
230278
})
231279
await new Promise<void>((resolve) => popupServer.listen(0, '127.0.0.1', resolve))
232280
const popupAddress = popupServer.address()
@@ -276,6 +324,7 @@ test.describe('browser tools', () => {
276324

277325
test.afterEach(async () => {
278326
beforeUploadResponse = undefined
327+
appEchoes.length = 0
279328
await app?.close()
280329
calls.clear()
281330
claimed.clear()
@@ -626,6 +675,111 @@ test.describe('browser tools', () => {
626675
.toHaveLength(1)
627676
})
628677

678+
const tabsState = () =>
679+
window.evaluate(
680+
(scope) =>
681+
(
682+
globalThis as typeof globalThis & { simDesktop: SimDesktopApi }
683+
).simDesktop.browserAgent.activateScope(scope),
684+
SCOPE
685+
)
686+
const tabUrls = async () => (await tabsState()).tabs.map((tab) => tab.url)
687+
/** Runs script in a page with a user gesture, as the user's own click would. */
688+
const inPage = (url: string, code: string) =>
689+
app.evaluate(
690+
({ webContents }, { url, code }) =>
691+
webContents
692+
.getAllWebContents()
693+
.find((contents) => contents.getURL() === url)
694+
?.executeJavaScript(code, true),
695+
{ url, code }
696+
)
697+
const pageDataset = (url: string) => inPage(url, '({ ...document.body.dataset })')
698+
699+
test('adopts a blank popup that keeps its opener and returns there when it closes', async () => {
700+
const openerUrl = `${site}/popups`
701+
expect((await execute('browser_open_url', { url: openerUrl })).ok).toBe(true)
702+
// A second tab makes the strip neighbour differ from the opener.
703+
expect((await execute('browser_open_tab', { url: `${site}/other` })).ok).toBe(true)
704+
const openerTabId = (await tabsState()).tabs.find((tab) => tab.url === openerUrl)?.tabId
705+
expect(openerTabId).toBeTruthy()
706+
707+
await inPage(openerUrl, 'signIn()')
708+
await expect
709+
.poll(() => pageDataset(openerUrl))
710+
.toMatchObject({ handle: 'true', blank: 'ready' })
711+
await expect.poll(tabUrls).toContain(`${popupOrigin}/authorize`)
712+
const popupTab = (await tabsState()).tabs.find((tab) => tab.url === `${popupOrigin}/authorize`)
713+
expect(popupTab?.active).toBe(true)
714+
715+
await inPage(`${popupOrigin}/authorize`, "document.querySelector('button').click()")
716+
await expect.poll(() => pageDataset(openerUrl)).toMatchObject({ connected: 'granted' })
717+
await expect.poll(async () => (await tabsState()).tabs.length).toBe(2)
718+
expect((await tabsState()).activeTabId).toBe(openerTabId)
719+
})
720+
721+
test('forgets the opener once the user switches tabs, closing the popup to its neighbour', async () => {
722+
const openerUrl = `${site}/popups`
723+
expect((await execute('browser_open_url', { url: openerUrl })).ok).toBe(true)
724+
expect((await execute('browser_open_tab', { url: `${site}/other` })).ok).toBe(true)
725+
const otherTabId = (await tabsState()).tabs.find((tab) => tab.url === `${site}/other`)?.tabId
726+
await inPage(openerUrl, 'signIn()')
727+
await expect.poll(tabUrls).toContain(`${popupOrigin}/authorize`)
728+
const popupTabId = (await tabsState()).tabs.find(
729+
(tab) => tab.url === `${popupOrigin}/authorize`
730+
)?.tabId
731+
for (const tabId of [otherTabId, popupTabId]) {
732+
await window.evaluate(
733+
({ tabId, scope }) =>
734+
(
735+
globalThis as typeof globalThis & { simDesktop: SimDesktopApi }
736+
).simDesktop.browserAgent.panelAction({ action: 'switch-tab', tabId }, scope),
737+
{ tabId, scope: SCOPE }
738+
)
739+
await expect.poll(async () => (await tabsState()).activeTabId).toBe(tabId)
740+
}
741+
742+
await inPage(`${popupOrigin}/authorize`, "document.querySelector('button').click()")
743+
await expect.poll(() => pageDataset(openerUrl)).toMatchObject({ connected: 'granted' })
744+
await expect.poll(async () => (await tabsState()).tabs.length).toBe(2)
745+
expect((await tabsState()).activeTabId).toBe(otherTabId)
746+
})
747+
748+
test('keeps data: popups denied', async () => {
749+
const openerUrl = `${site}/popups`
750+
expect((await execute('browser_open_url', { url: openerUrl })).ok).toBe(true)
751+
expect(await inPage(openerUrl, "String(window.open('data:text/html,x') !== null)")).toBe(
752+
'false'
753+
)
754+
expect((await tabsState()).tabs).toHaveLength(1)
755+
})
756+
757+
test('posts a same-session form popup and refuses to reissue a cross-session one', async () => {
758+
const openerUrl = `${site}/popups`
759+
expect((await execute('browser_open_url', { url: openerUrl })).ok).toBe(true)
760+
await inPage(openerUrl, "document.getElementById('same-session').requestSubmit()")
761+
await expect.poll(() => inPage(`${popupOrigin}/echo`, 'document.title')).toBe('POST note=hello')
762+
763+
const before = (await tabsState()).tabs.length
764+
await inPage(openerUrl, "document.getElementById('cross-session').requestSubmit()")
765+
// The handler decides popups in order, so once this one is listed the post was decided.
766+
await inPage(openerUrl, `void window.open('${site}/sentinel')`)
767+
await expect.poll(tabUrls).toContain(`${site}/sentinel`)
768+
expect((await tabsState()).tabs).toHaveLength(before + 1)
769+
expect(appEchoes).toEqual([])
770+
})
771+
772+
test('keeps the opener of a Sim page blank popup that navigates within Sim', async () => {
773+
const openerUrl = `${origin}/popups?next=${encodeURIComponent(`${origin}/authorize`)}`
774+
expect((await execute('browser_open_url', { url: openerUrl })).ok).toBe(true)
775+
await inPage(openerUrl, 'signIn()')
776+
await expect.poll(tabUrls).toContain(`${origin}/authorize`)
777+
await inPage(`${origin}/authorize`, "document.querySelector('button').click()")
778+
await expect
779+
.poll(() => pageDataset(openerUrl))
780+
.toMatchObject({ handle: 'true', connected: 'granted' })
781+
})
782+
629783
test('renders PDFs in the built-in viewer', async () => {
630784
const response = await execute('browser_open_url', { url: `${site}/doc.pdf` })
631785
expect(response.ok, response.error).toBe(true)

‎apps/desktop/src/main/browser-agent/driver.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -804,7 +804,7 @@ describe('executeTool', () => {
804804
const popup = session.activeTab()?.view.webContents
805805
if (!popup) throw new Error('Expected user popup tab')
806806
expect(session.automationTab()?.view.webContents).toBe(source)
807-
expect(popup.loadURL).toHaveBeenCalledWith('https://user-popup.example/')
807+
expect(popup.loadURL).toHaveBeenCalledWith('https://user-popup.example/', expect.anything())
808808
})
809809

810810
it('keeps agent-opened target-blank tabs agent-owned after dispatch ends', async () => {
@@ -823,7 +823,7 @@ describe('executeTool', () => {
823823
expect(session.activeTab()?.view.webContents).toBe(source)
824824
session.setAutomationActive(false)
825825
expect(session.automationTab()?.view.webContents).toBe(popup)
826-
expect(popup.loadURL).toHaveBeenCalledWith('https://agent-popup.example/')
826+
expect(popup.loadURL).toHaveBeenCalledWith('https://agent-popup.example/', expect.anything())
827827
})
828828

829829
it('does not report a timed-out restored tab as ready to the model', async () => {

‎apps/desktop/src/main/browser-agent/session.test.ts‎

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ function _setPlatform(platform: NodeJS.Platform): void {
3232
Object.defineProperty(process, 'platform', { configurable: true, value: platform })
3333
}
3434

35-
type PopupHandler = (details: { url: string }) => {
35+
type PopupHandler = (details: { url: string; disposition?: string }) => {
3636
action: string
3737
outlivesOpener?: boolean
3838
createWindow?: (options: Record<string, unknown>) => unknown
@@ -1259,7 +1259,7 @@ describe('browser-agent session', () => {
12591259
openWindow({ url: 'https://example.com/later' }).createWindow?.({})
12601260
const popup = (session.activeTab()?.view as unknown as MockView).webContents
12611261
expect(popup).not.toBe(source)
1262-
expect(popup.loadURL).toHaveBeenCalledWith('https://example.com/later')
1262+
expect(popup.loadURL).toHaveBeenCalledWith('https://example.com/later', expect.anything())
12631263
expect(openWindow({ url: 'javascript:alert(1)' })).toEqual({ action: 'deny' })
12641264
expect(session.listTabs()).toHaveLength(2)
12651265
})
@@ -2321,13 +2321,28 @@ describe('importAgentCookies', () => {
23212321

23222322
describe('first-party browser sessions', () => {
23232323
const origin = 'https://www.dev.sim.ai'
2324+
const appSession = new WebContentsView().webContents.session
23242325
function initialize(persistence?: sessionModule.BrowserSessionPersistence) {
23252326
return freshSession(null, {}, persistence, undefined, {
23262327
origin,
2327-
session: new WebContentsView().webContents.session,
2328+
session: appSession,
23282329
})
23292330
}
23302331

2332+
it("opens a deferred blank popup in its opener's session", () => {
2333+
const session = initialize()
2334+
const opener = session.addAutomationTab(`${origin}/home`).view.webContents
2335+
Object.assign(opener, { session: appSession })
2336+
const openWindow = vi.mocked(opener.setWindowOpenHandler).mock.calls[0]?.[0] as PopupHandler
2337+
2338+
const popup = openWindow({ url: 'about:blank', disposition: 'background-tab' }).createWindow?.(
2339+
{}
2340+
) as WebContents
2341+
expect(popup).not.toBe(opener)
2342+
expect(agentAppOrigin(popup)).toBe(origin)
2343+
expect(popup.loadURL).not.toHaveBeenCalled()
2344+
})
2345+
23312346
it('keeps a populated tab and its history when crossing the session boundary', () => {
23322347
const session = initialize()
23332348
const tab = session.addAutomationTab(`${origin}/home`)

0 commit comments

Comments
 (0)