diff --git a/src/main/browser/browser-manager-downloads.test.ts b/src/main/browser/browser-manager-downloads.test.ts index 981002c727b..e79d510ff62 100644 --- a/src/main/browser/browser-manager-downloads.test.ts +++ b/src/main/browser/browser-manager-downloads.test.ts @@ -123,6 +123,16 @@ describe('browserManager', () => { permission: 'media', origin: 'https://example.com' }) + browserManager.notifyPermissionDenied({ + guestWebContentsId: guest.id, + permission: 'geolocation', + rawUrl: '' + }) + expect(rendererSendMock).toHaveBeenCalledWith('browser:permission-denied', { + browserPageId: 'browser-1', + permission: 'geolocation', + origin: 'unknown' + }) expect(rendererSendMock).toHaveBeenCalledWith( 'browser:download-requested', expect.objectContaining({ diff --git a/src/main/browser/browser-session-partition-policies.ts b/src/main/browser/browser-session-partition-policies.ts index 6bd0322ade6..2118c62ed68 100644 --- a/src/main/browser/browser-session-partition-policies.ts +++ b/src/main/browser/browser-session-partition-policies.ts @@ -22,6 +22,21 @@ const handleWillDownload = ( browserManager.handleGuestWillDownload({ guestWebContentsId: webContents.id, item }) } +function resolvePermissionNoticeUrl( + webContents: Electron.WebContents, + details: Electron.PermissionRequest | undefined +): string { + const requestingUrl = details?.requestingUrl + if (!requestingUrl) { + return webContents.getURL() + } + try { + return new URL(requestingUrl).origin === 'null' ? '' : requestingUrl + } catch { + return '' + } +} + export function installBrowserSessionPartitionPolicies(profile: BrowserSessionProfile): void { const { partition } = profile const sess = session.fromPartition(partition) @@ -37,13 +52,10 @@ export function installBrowserSessionPartitionPolicies(profile: BrowserSessionPr setupClientHintsOverride(sess, cleanUA) } sess.setPermissionRequestHandler((webContents, permission, callback, details) => { - // Why: a permission request can come from a cross-origin sub-frame, and getURL() reports the - // TOP-LEVEL document. Attributing a sub-frame's denial to the embedder tells the user the wrong - // site asked. Every PermissionRequest variant carries requestingUrl; fall back to the top-level - // document only when it is missing. - const requestingUrl = (details as Electron.PermissionRequest | undefined)?.requestingUrl // Why: defer media to macOS TCC; denying at the session layer throws NotAllowedError even after the user granted Camera/Mic to the OS. if (permission === 'media') { + // Capture before async handling; opaque frames cannot be attributed to a named site. + const rawUrl = resolvePermissionNoticeUrl(webContents, details) void requestSystemMediaAccess( details as Electron.MediaAccessPermissionRequest | undefined ).then( @@ -52,7 +64,7 @@ export function installBrowserSessionPartitionPolicies(profile: BrowserSessionPr browserManager.notifyPermissionDenied({ guestWebContentsId: webContents.id, permission, - rawUrl: requestingUrl || webContents.getURL() + rawUrl }) } callback(granted) @@ -62,7 +74,7 @@ export function installBrowserSessionPartitionPolicies(profile: BrowserSessionPr browserManager.notifyPermissionDenied({ guestWebContentsId: webContents.id, permission, - rawUrl: requestingUrl || webContents.getURL() + rawUrl }) callback(false) } @@ -71,10 +83,11 @@ export function installBrowserSessionPartitionPolicies(profile: BrowserSessionPr } const allowed = isAutoGrantedBrowserSessionPermission(permission) if (!allowed) { + const rawUrl = resolvePermissionNoticeUrl(webContents, details) browserManager.notifyPermissionDenied({ guestWebContentsId: webContents.id, permission, - rawUrl: requestingUrl || webContents.getURL() + rawUrl }) } callback(allowed) diff --git a/src/main/browser/browser-session-registry.persistence.test.ts b/src/main/browser/browser-session-registry.persistence.test.ts index 023226babdb..dbdd5b1ea49 100644 --- a/src/main/browser/browser-session-registry.persistence.test.ts +++ b/src/main/browser/browser-session-registry.persistence.test.ts @@ -538,8 +538,7 @@ describe('BrowserSessionRegistry persistence', () => { rawUrl: 'https://example.com/account' }) - // Why: a cross-origin sub-frame can request a permission, and getURL() reports the TOP-LEVEL - // document. Attributing the denial to the embedder tells the user the wrong site asked. + // A subframe denial must name the requester, not its top-level embedder. browserManagerNotifyPermissionDeniedMock.mockClear() requestHandler(guestWc, 'geolocation', permissionCallback, { requestingUrl: 'https://widget.example.net/embed', @@ -553,8 +552,7 @@ describe('BrowserSessionRegistry persistence', () => { }) ) - // Why: requestingUrl is absent on some request shapes; the top-level document is the fallback, - // not a crash or an empty origin. + // Missing or empty frame URLs fall back to the visible top-level page. browserManagerNotifyPermissionDeniedMock.mockClear() requestHandler(guestWc, 'geolocation', permissionCallback, { isMainFrame: true }) await vi.waitFor(() => @@ -564,6 +562,33 @@ describe('BrowserSessionRegistry persistence', () => { rawUrl: 'https://example.com/account' }) ) + + browserManagerNotifyPermissionDeniedMock.mockClear() + requestHandler(guestWc, 'geolocation', permissionCallback, { + requestingUrl: '', + isMainFrame: false + }) + await vi.waitFor(() => + expect(browserManagerNotifyPermissionDeniedMock).toHaveBeenCalledWith({ + guestWebContentsId: 401, + permission: 'geolocation', + rawUrl: 'https://example.com/account' + }) + ) + + // Opaque frame URLs have no site Orca can name accurately. + browserManagerNotifyPermissionDeniedMock.mockClear() + requestHandler(guestWc, 'geolocation', permissionCallback, { + requestingUrl: 'about:blank', + isMainFrame: false + }) + await vi.waitFor(() => + expect(browserManagerNotifyPermissionDeniedMock).toHaveBeenCalledWith({ + guestWebContentsId: 401, + permission: 'geolocation', + rawUrl: '' + }) + ) expect( browserManagerNotifyPermissionDeniedMock.mock.calls.map(([args]) => args.permission) ).toEqual(['geolocation']) @@ -735,6 +760,7 @@ describe('BrowserSessionRegistry persistence', () => { const callback = vi.fn() requestHandler(guestWc, 'media', callback, { mediaTypes: ['video'] }) + guestWc.getURL.mockReturnValue('https://example.com/after-navigation') await vi.waitFor(() => expect(callback).toHaveBeenCalledWith(false)) expect(browserManagerNotifyPermissionDeniedMock).toHaveBeenCalledWith({ diff --git a/src/renderer/src/components/browser-pane/navigate/browser-notices.test.ts b/src/renderer/src/components/browser-pane/navigate/browser-notices.test.ts index 3c3e6d1ff14..f812d5fe02f 100644 --- a/src/renderer/src/components/browser-pane/navigate/browser-notices.test.ts +++ b/src/renderer/src/components/browser-pane/navigate/browser-notices.test.ts @@ -19,10 +19,15 @@ describe('browser notice formatting', () => { origin: 'https://example.com' }) ).toBe('https://example.com asked for camera or microphone access, and Orca denied it.') + expect( + formatPermissionNotice({ + browserPageId: 'browser-1', + permission: 'geolocation', + origin: 'unknown' + }) + ).toBe('this page asked for your location, and Orca denied it.') }) - // Why: with ordinary storage-access now granted, top-level-storage-access is the storage denial a - // user can still hit, and it was rendering as its raw Chromium token. it('names the storage permission in words rather than its raw token', () => { const notice = formatPermissionNotice({ browserPageId: 'browser-1', @@ -31,12 +36,36 @@ describe('browser notice formatting', () => { }) expect(notice).not.toContain('top-level-storage-access') expect(notice).toBe( - 'https://example.com asked for access to its own cookies and storage on this site, and Orca denied it.' + 'https://example.com asked for cookie access on behalf of an embedded site, and Orca denied it.' ) }) - // Why: an unrecognised permission must still name itself rather than render as empty or as prose - // invented for it. + it.each([ + ['storage-access', 'access to its own cookies and storage while embedded on this page'], + ['idle-detection', 'permission to detect when you are idle'], + ['display-capture', 'permission to capture your screen'], + ['window-management', 'screen information and multi-screen window placement'], + ['keyboardLock', 'permission to capture keyboard input'], + ['openExternal', 'permission to open a link outside Orca'], + ['fileSystem', 'access to your files or folders'], + ['hid', 'access to a connected human interface device'], + ['usb', 'access to a USB device'], + ['serial', 'access to a serial device'], + ['midi', 'access to your MIDI devices'], + ['midiSysex', 'access to system-exclusive MIDI messages'], + ['mediaKeySystem', 'access to protected media playback'], + ['speaker-selection', 'permission to choose an audio output device'] + ])('formats the %s permission as readable copy', (permission, description) => { + expect( + formatPermissionNotice({ + browserPageId: 'browser-1', + permission, + origin: 'https://example.com' + }) + ).toBe(`https://example.com asked for ${description}, and Orca denied it.`) + }) + + // Pin the raw-token fallback for permissions Chromium adds later. it('falls back to the raw permission name for anything unmapped', () => { expect( formatPermissionNotice({ diff --git a/src/renderer/src/components/browser-pane/navigate/browser-notices.ts b/src/renderer/src/components/browser-pane/navigate/browser-notices.ts index 15d0da95d2f..ca5e6aa209a 100644 --- a/src/renderer/src/components/browser-pane/navigate/browser-notices.ts +++ b/src/renderer/src/components/browser-pane/navigate/browser-notices.ts @@ -15,10 +15,7 @@ export type LoadFailureMeta = { type BrowserLoadErrorLike = BrowserLoadError | null -// Why: the raw Chromium permission name is what the user sees in the denial notice, and most of -// them do not read as English - "top-level-storage-access" is the one users hit most now that -// ordinary storage-access is granted. The default still returns the raw token, because inventing -// prose for a permission nobody has seen yet is worse than showing its real name. +// Unknown Chromium permissions keep their raw name instead of disappearing behind invented copy. function humanizePermission(permission: string): string { switch (permission) { case 'media': @@ -26,33 +23,37 @@ function humanizePermission(permission: string): string { case 'pointerLock': return 'pointer lock' case 'storage-access': + return 'access to its own cookies and storage while embedded on this page' case 'top-level-storage-access': - return 'access to its own cookies and storage on this site' + return 'cookie access on behalf of an embedded site' case 'geolocation': return 'your location' case 'idle-detection': - return 'to detect when you are idle' + return 'permission to detect when you are idle' case 'display-capture': - return 'to capture your screen' + return 'permission to capture your screen' case 'window-management': - return 'to manage windows on your screen' + return 'screen information and multi-screen window placement' case 'keyboardLock': - return 'to capture all keyboard input' + return 'permission to capture keyboard input' case 'openExternal': - return 'to open an external app' + return 'permission to open a link outside Orca' case 'fileSystem': - return 'access to your files' + return 'access to your files or folders' case 'hid': - return 'access to a USB input device' + return 'access to a connected human interface device' case 'usb': return 'access to a USB device' case 'serial': return 'access to a serial device' case 'midi': - case 'midiSysex': return 'access to your MIDI devices' + case 'midiSysex': + return 'access to system-exclusive MIDI messages' + case 'mediaKeySystem': + return 'access to protected media playback' case 'speaker-selection': - return 'to choose an audio output device' + return 'permission to choose an audio output device' default: return permission }