Skip to content

Commit caa182c

Browse files
committed
fix(desktop): require an explicit accept for every sign-in prompt answer and arm prompts only while visible
1 parent 5975095 commit caa182c

7 files changed

Lines changed: 146 additions & 26 deletions

File tree

‎apps/desktop/e2e/browser-sign-in-prompts.spec.ts‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -504,10 +504,11 @@ createRoot(document.getElementById('root')).render(createElement(Fixture));`,
504504
} })
505505
await make('user-ada', 'ada@example.com')
506506
await make('user-grace', 'grace@example.com')
507-
globalThis.passkeyResult = navigator.credentials.get({ publicKey: {
507+
globalThis.passkeyOutcome = 'pending'
508+
navigator.credentials.get({ publicKey: {
508509
challenge: new Uint8Array(16), rpId: 'localhost', userVerification: 'preferred',
509510
} }).then((credential) => new TextDecoder().decode(credential.response.userHandle),
510-
(error) => 'error:' + error.name)
511+
(error) => 'error:' + error.name).then((outcome) => { globalThis.passkeyOutcome = outcome })
511512
return 'created'
512513
})()`
513514
)
@@ -520,26 +521,31 @@ createRoot(document.getElementById('root')).render(createElement(Fixture));`,
520521
expect(
521522
prompt?.kind === 'passkey-account' && prompt.accounts.map((account) => account.name).sort()
522523
).toEqual(['ada@example.com', 'grace@example.com'])
523-
// A stray Enter must never send an identity: nothing is preselected, and Enter refuses.
524+
// A stray Enter must never send an identity: nothing is preselected and Enter does not
525+
// continue. It dismisses when the prompt has keyboard focus and is ignored when it does
526+
// not (as on a CI runner without window focus); either way, refuse and ask again.
527+
const passkeyOutcome = () => pageAt<string>(passkeyPage, 'globalThis.passkeyOutcome')
524528
await shell.keyboard.press('Enter')
525-
await expect
526-
.poll(() => pageAt<string>(passkeyPage, 'globalThis.passkeyResult'))
527-
.toBe('error:NotAllowedError')
529+
await sleep(QUIET_MS)
530+
expect(['pending', 'error:NotAllowedError']).toContain(await passkeyOutcome())
531+
if ((await passkeyOutcome()) === 'pending') {
532+
await shell.getByRole('button', { name: 'Cancel', exact: true }).click()
533+
}
534+
await expect.poll(passkeyOutcome).toBe('error:NotAllowedError')
528535
await pageAt(
529536
passkeyPage,
530-
`globalThis.passkeyResult = navigator.credentials.get({ publicKey: {
537+
`globalThis.passkeyOutcome = 'pending'
538+
navigator.credentials.get({ publicKey: {
531539
challenge: new Uint8Array(16), rpId: 'localhost', userVerification: 'preferred',
532540
} }).then((credential) => new TextDecoder().decode(credential.response.userHandle),
533-
(error) => 'error:' + error.name); 'asked'`
541+
(error) => 'error:' + error.name).then((outcome) => { globalThis.passkeyOutcome = outcome }); 'asked'`
534542
)
535543
await expect.poll(pagePrompt).toMatchObject({ kind: 'passkey-account' })
536544
const dialog = shell.getByRole('dialog')
537545
await dialog.getByRole('button').filter({ hasText: 'Choose an account' }).click()
538546
await shell.getByRole('menuitemradio', { name: 'grace@example.com' }).click()
539547
await shell.getByRole('button', { name: 'Continue', exact: true }).click()
540-
await expect
541-
.poll(() => pageAt<string>(passkeyPage, 'globalThis.passkeyResult'))
542-
.toBe('user-grace')
548+
await expect.poll(passkeyOutcome).toBe('user-grace')
543549
expect(await pagePrompt()).toBeNull()
544550
})
545551
passed = true

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

Lines changed: 89 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,12 @@ import type {
1111
WebContents,
1212
WebFrameMain,
1313
} from 'electron'
14-
import { app, shell } from 'electron'
14+
import { app, webContents as electronWebContents, shell } from 'electron'
1515
import {
1616
appLaunchNoticeForContents,
1717
handleBrowserLogin,
1818
handleSelectClientCertificate,
19+
handleSelectPasskeyAccount,
1920
httpAuthChallenge,
2021
noteMainFrameNavigation,
2122
pagePromptForContents,
@@ -309,3 +310,90 @@ describe('app links', () => {
309310
expect(shell.openExternal).toHaveBeenCalledExactlyOnceWith('zoommtg://zoom.us/join?id=1')
310311
})
311312
})
313+
314+
describe('accepting an answer', () => {
315+
const topFrame = { parent: null, frameTreeNodeId: 1, origin: PAGE } as WebFrameMain
316+
317+
/** What a renderer could send over IPC without `allowed: true`, which the gesture gate keys on. */
318+
function withoutAllowed(response: Record<string, unknown>): BrowserPagePromptResponse {
319+
// double-cast-allowed: an IPC payload is untyped; this is the shape a hostile renderer could send
320+
return response as unknown as BrowserPagePromptResponse
321+
}
322+
323+
it.each([
324+
[
325+
'client-certificate',
326+
{ kind: 'client-certificate', choiceId: 'sha256/first' },
327+
(contents: WebContents, callback: () => void) =>
328+
handleSelectClientCertificate(
329+
{ preventDefault: () => {} },
330+
contents,
331+
'https://mtls.corp',
332+
[{ fingerprint: 'sha256/first', subjectName: 'Personal' } as Certificate],
333+
callback
334+
),
335+
],
336+
[
337+
'passkey-account',
338+
{ kind: 'passkey-account', choiceId: 'credential-1' },
339+
(contents: WebContents, callback: () => void) => {
340+
vi.mocked(electronWebContents.fromFrame).mockReturnValue(contents)
341+
handleSelectPasskeyAccount(
342+
{},
343+
{
344+
relyingPartyId: 'meet.example',
345+
accounts: [{ credentialId: 'credential-1', name: 'ada', userHandle: 'u1' }],
346+
frame: topFrame,
347+
},
348+
callback
349+
)
350+
},
351+
],
352+
[
353+
'http-auth',
354+
{ kind: 'http-auth', username: 'ada', password: 'pw' },
355+
(contents: WebContents, callback: () => void) => {
356+
noteMainFrameNavigation(contents, 'https://intranet.example/')
357+
handleBrowserLogin(
358+
{ preventDefault: () => {} },
359+
contents,
360+
{
361+
url: 'https://intranet.example/',
362+
isRequestForNavigation: true,
363+
firstAuthAttempt: true,
364+
} as AuthenticationResponseDetails,
365+
{ isProxy: false, scheme: 'basic', host: 'intranet.example', port: 443 } as AuthInfo,
366+
callback
367+
)
368+
},
369+
],
370+
])('sends nothing for a %s answer without allowed: true', (_kind, response, ask) => {
371+
const contents = agentContents()
372+
const callback = vi.fn()
373+
ask(contents, callback)
374+
answer(contents, withoutAllowed(response))
375+
expect(callback).toHaveBeenCalledExactlyOnceWith()
376+
})
377+
378+
it.each([
379+
['an app prompt', { kind: 'external-app', alwaysAllow: true }, 'prompt'],
380+
['an app notice', { kind: 'app-launch-notice' }, 'notice'],
381+
])('opens nothing for %s answer without allowed: true', async (_label, response, slot) => {
382+
app.getApplicationNameForProtocol = vi.fn(() => 'Zoom')
383+
vi.mocked(shell.openExternal).mockClear()
384+
context.userActivated = slot === 'prompt'
385+
const contents = agentContents()
386+
requestExternalLaunch(contents, 'zoommtg://zoom.us/join', { initiator: topFrame })
387+
const requestId =
388+
slot === 'prompt'
389+
? shownPrompt(contents).requestId
390+
: appLaunchNoticeForContents(contents)?.requestId
391+
respondToPagePrompt('chat', {
392+
action: 'respond-prompt',
393+
requestId,
394+
response: withoutAllowed(response),
395+
})
396+
await vi.advanceTimersByTimeAsync(0)
397+
expect(shell.openExternal).not.toHaveBeenCalled()
398+
})
399+
})

‎apps/desktop/src/main/browser-agent/page-prompts.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,8 @@ export function respondToPagePrompt(scopeId: string, action: BrowserPanelAction)
289289
settle(entry, response)
290290
return
291291
}
292-
settle(entry, response?.kind === entry.kind && context.onScreen ? response : null)
292+
const accepted = response?.allowed === true && response.kind === entry.kind && context.onScreen
293+
settle(entry, accepted ? response : null)
293294
}
294295

295296
/**
@@ -321,6 +322,7 @@ export function httpAuthChallenge(
321322
/**
322323
* `app.on('login')` for agent browser tabs. Other contents keep Electron's
323324
* default, which cancels: the main window's proxy prompt needs a shell form.
325+
* The prompt never takes focus, since a page can trigger it on its own.
324326
*/
325327
export function handleBrowserLogin(
326328
event: { preventDefault(): void },
@@ -357,7 +359,7 @@ export function handleBrowserLogin(
357359
answer()
358360
}
359361
},
360-
{ joinable: true, focus: true }
362+
{ joinable: true }
361363
)
362364
if (held === 'held') {
363365
prompts.push(Date.now())
@@ -468,7 +470,7 @@ export function handleSelectClientCertificate(
468470
return
469471
}
470472
const certificate =
471-
response?.kind === 'client-certificate'
473+
response?.allowed === true && response.kind === 'client-certificate'
472474
? certificates.find((candidate) => candidate.fingerprint === response.choiceId)
473475
: undefined
474476
if (!certificate) {
@@ -574,6 +576,7 @@ export function requestExternalLaunch(
574576
}
575577
launchAppLink(target.url)
576578
},
579+
// The prompt answers the user's own click in the page, so it takes the focus that click had.
577580
{ focus: true }
578581
)
579582
if (held) attempts.push(Date.now())

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4141,7 +4141,10 @@ export async function clearProfileStorage(): Promise<void> {
41414141
await clearBrowserSignIns(ses)
41424142
}
41434143

4144-
/** HTTP credentials Chromium caches, and remembered certificate and app-launch choices. */
4144+
/**
4145+
* Sign-out forgets every HTTP credential Chromium cached for browser tabs, in the
4146+
* borrowed app session too, and the remembered certificate and app-launch choices.
4147+
*/
41454148
async function clearBrowserSignIns(agentSession: Session): Promise<void> {
41464149
resetPagePromptDecisions()
41474150
await agentSession.clearAuthCache()
@@ -4179,7 +4182,7 @@ export async function clearAgentData(kinds: readonly BrowserDataKind[]): Promise
41794182
if (storages.length > 0) {
41804183
await ses.clearStorageData({ storages } as Parameters<Session['clearStorageData']>[0])
41814184
}
4182-
if (kinds.includes('cookies')) await clearBrowserSignIns(ses)
4185+
if (kinds.includes('cookies')) await ses.clearAuthCache()
41834186
if (kinds.includes('cache')) {
41844187
await ses.clearCache()
41854188
// Resolved-host verdicts are a cache too, and a user clearing the cache

‎apps/desktop/src/test/electron-mock.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,11 @@ export const session = {
107107
fromPartition: vi.fn(),
108108
}
109109

110+
/** Resolves a frame to its WebContents; tests return the contents they registered. */
111+
export const webContents = {
112+
fromFrame: vi.fn(),
113+
}
114+
110115
export const protocol = {
111116
registerSchemesAsPrivileged: vi.fn(),
112117
handle: vi.fn(),

‎apps/sim/app/workspace/[workspaceId]/home/components/mothership-view/components/resource-content/components/browser-session/browser-page-prompt.tsx‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,21 @@ const ACTIVATION_DELAY_MS = 500
3232

3333
type RespondToPrompt = (requestId: string, response: BrowserPagePromptResponse) => void
3434

35-
/** False until the prompt has been on screen long enough to be read; remount per request. */
36-
function useActivationDelay(): boolean {
35+
/**
36+
* False until the prompt has been visible long enough to be read. Hiding it
37+
* disarms it again, so a prompt shown after waiting behind another dialog
38+
* starts its delay only once it appears.
39+
*/
40+
function useActivationDelay(visible: boolean): boolean {
3741
const [armed, setArmed] = useState(false)
3842
useEffect(() => {
43+
if (!visible) {
44+
setArmed(false)
45+
return
46+
}
3947
const timer = setTimeout(() => setArmed(true), ACTIVATION_DELAY_MS)
4048
return () => clearTimeout(timer)
41-
}, [])
49+
}, [visible])
4250
return armed
4351
}
4452

@@ -48,7 +56,9 @@ interface PromptModalProps<Prompt extends BrowserPagePrompt> {
4856
onRespond: RespondToPrompt
4957
}
5058

59+
/** Signing in takes a click on Sign in: Enter in a field never submits credentials a keystroke meant for the page. */
5160
function HttpAuthPromptModal({ prompt, open, onRespond }: PromptModalProps<BrowserHttpAuthPrompt>) {
61+
const armed = useActivationDelay(open)
5262
const [username, setUsername] = useState('')
5363
const [password, setPassword] = useState('')
5464
const cancel = () => onRespond(prompt.requestId, { allowed: false })
@@ -83,7 +93,12 @@ function HttpAuthPromptModal({ prompt, open, onRespond }: PromptModalProps<Brows
8393
</ChipModalBody>
8494
<ChipModalFooter
8595
onCancel={cancel}
86-
primaryAction={{ label: 'Sign in', onClick: submit, disabled: !username && !password }}
96+
defaultAction='dismiss'
97+
primaryAction={{
98+
label: 'Sign in',
99+
onClick: submit,
100+
disabled: !armed || (!username && !password),
101+
}}
87102
/>
88103
</ChipModal>
89104
)
@@ -119,7 +134,7 @@ function ChoicePromptModal({
119134
onCancel,
120135
onChoose,
121136
}: ChoicePromptModalProps) {
122-
const armed = useActivationDelay()
137+
const armed = useActivationDelay(open)
123138
const [choiceId, setChoiceId] = useState('')
124139
return (
125140
<ChipModal open={open} onOpenChange={(next) => !next && onCancel()} srTitle={title} size='sm'>
@@ -219,7 +234,7 @@ function ExternalAppPromptModal({
219234
open,
220235
onRespond,
221236
}: PromptModalProps<BrowserExternalAppPrompt>) {
222-
const armed = useActivationDelay()
237+
const armed = useActivationDelay(open)
223238
const [alwaysAllow, setAlwaysAllow] = useState(false)
224239
const checkboxId = `browser-always-open-${prompt.requestId}`
225240
return (
@@ -289,7 +304,7 @@ interface BrowserAppLaunchNoticeBarProps {
289304

290305
/** A page's unprompted request to open another app; nothing opens until the user chooses Open. */
291306
export function BrowserAppLaunchNoticeBar({ notice, onRespond }: BrowserAppLaunchNoticeBarProps) {
292-
const armed = useActivationDelay()
307+
const armed = useActivationDelay(true)
293308
return (
294309
<Banner className='border-[var(--border)] border-t bg-[var(--surface-3)] px-3 py-1.5'>
295310
<div className='flex items-center justify-between gap-3'>

‎packages/browser-protocol/src/index.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -358,7 +358,7 @@ export interface BrowserHttpAuthPrompt {
358358
retry: boolean
359359
}
360360

361-
export interface BrowserPasskeyAccount {
361+
interface BrowserPasskeyAccount {
362362
/** The credential id the shell resolves the choice by. */
363363
id: string
364364
name: string
@@ -373,7 +373,7 @@ export interface BrowserPasskeyPrompt {
373373
accounts: BrowserPasskeyAccount[]
374374
}
375375

376-
export interface BrowserClientCertificate {
376+
interface BrowserClientCertificate {
377377
/** Certificate fingerprint; the shell resolves the choice by it. */
378378
id: string
379379
subject: string

0 commit comments

Comments
 (0)