Skip to content

Commit a40f60a

Browse files
waleedlatif1waleedlatif1
committed
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
1 parent 09ed990 commit a40f60a

5 files changed

Lines changed: 305 additions & 37 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/session.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -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
})

0 commit comments

Comments
 (0)