From 4a7f6ee7f456efc9255bd8330746a224d9fe8bb0 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 11 Jul 2026 20:18:32 -0700 Subject: [PATCH] fix: elide the start of long popup origins so the registrable domain stays visible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review findings on the origin bar: - The bar right-ellipsized long hostnames, hiding exactly the part that matters: window.open('https://accounts.google.com.<...>.evil.com', '', 'width=360') rendered as 'https://accounts.google.com.signin.s…'. Clip from the left instead (rtl container + isolated ltr bdi so host and port keep normal character order), matching how Chrome elides. - Re-assert the origin on the popup's did-finish-load so a transiently dropped executeJavaScript write cannot leave a stale origin up. - Re-pin view layout on enter/leave-full-screen: verified at runtime that HTML5 fullscreen keeps the bar visible on macOS (resize fires); the explicit events make that hold on every platform. - Test hardening: fake WebContents now flips isDestroyed after 'destroyed' so the double-close guard is actually exercised. Co-authored-by: Orca --- .../browser/popup-origin-bar-window.test.ts | 41 ++++++++++++++++++- src/main/browser/popup-origin-bar-window.ts | 18 +++++++- 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/src/main/browser/popup-origin-bar-window.test.ts b/src/main/browser/popup-origin-bar-window.test.ts index fd92d8b71de..c0161dd94e8 100644 --- a/src/main/browser/popup-origin-bar-window.test.ts +++ b/src/main/browser/popup-origin-bar-window.test.ts @@ -16,6 +16,7 @@ type FakeWebContents = { const { fakeElectron } = vi.hoisted(() => { function createFakeWebContents(): FakeWebContents { const handlers = new Map() + let destroyed = false const add = (event: string, handler: Handler): void => { handlers.set(event, [...(handlers.get(event) ?? []), handler]) } @@ -31,8 +32,13 @@ const { fakeElectron } = vi.hoisted(() => { executeJavaScript: vi.fn(() => Promise.resolve()), loadURL: vi.fn(() => Promise.resolve()), close: vi.fn(), - isDestroyed: vi.fn(() => false), + // Mirrors real Electron: isDestroyed() is already true inside a + // 'destroyed' handler, which is what the double-close guard relies on. + isDestroyed: vi.fn(() => destroyed), emit: (event: string, ...args: unknown[]) => { + if (event === 'destroyed') { + destroyed = true + } // off() replaces the stored array, so iterating the fetched one is safe. for (const handler of handlers.get(event) ?? []) { handler(...args) @@ -270,13 +276,44 @@ describe('openPopupWithOriginBar', () => { expect(lastWindow().setTitle).toHaveBeenLastCalledWith('https://evil.example.net') }) - it('closes the window when the popup content is destroyed', () => { + it('closes the window when the popup content is destroyed, without re-closing the contents', () => { const adopted = createFakeWebContents() openPopupWithOriginBar({ webContents: adopted as never }, 'https://example.com/') adopted.emit('destroyed') expect(lastWindow().close).toHaveBeenCalled() + // The window's closed handler must not call close() on already-destroyed + // contents — that throws in real Electron. + expect(adopted.close).not.toHaveBeenCalled() + }) + + it('re-asserts the origin when the popup finishes loading', () => { + const adopted = createFakeWebContents() + openPopupWithOriginBar({ webContents: adopted as never }, 'https://example.com/login') + const { bar } = lastViews() + bar.webContents.emit('did-finish-load') + bar.webContents.executeJavaScript.mockClear() + + adopted.emit('did-finish-load') + + expect(bar.webContents.executeJavaScript).toHaveBeenCalledTimes(1) + expect(bar.webContents.executeJavaScript.mock.calls[0][0]).toContain('"https://example.com"') + }) + + it('elides the start of long origins so the registrable domain stays visible', () => { + openPopupWithOriginBar( + { webContents: createFakeWebContents() as never }, + 'https://example.com/' + ) + const { bar } = lastViews() + const dataUrl = bar.webContents.loadURL.mock.calls[0][0] as string + const html = decodeURIComponent(dataUrl.replace('data:text/html;charset=utf-8,', '')) + // rtl clip container ellipsizes the left; the isolated ltr bdi keeps the + // origin's own characters (host, port) in normal order. + expect(html).toContain('') + expect(html).toMatch(/#origin-clip\s*{[^}]*direction:\s*rtl/) + expect(html).toMatch(/#origin\s*{[^}]*direction:\s*ltr/) }) it('closes the popup content and notifies listeners when the window closes', () => { diff --git a/src/main/browser/popup-origin-bar-window.ts b/src/main/browser/popup-origin-bar-window.ts index 03eb3c46c9d..504c862a5b8 100644 --- a/src/main/browser/popup-origin-bar-window.ts +++ b/src/main/browser/popup-origin-bar-window.ts @@ -67,12 +67,16 @@ const ORIGIN_BAR_HTML = `Not secure` function clampPopupContentSize(options: PopupChildWindowOptions): { width: number @@ -133,6 +137,11 @@ export function openPopupWithOriginBar( }) } window.on('resize', layoutViews) + // Why: HTML5 fullscreen makes the whole window fullscreen. resize covers + // this on macOS, but re-pin on the explicit events too so the origin bar + // provably stays above fullscreen content on every platform. + window.on('enter-full-screen', layoutViews) + window.on('leave-full-screen', layoutViews) layoutViews() const contentWebContents = contentView.webContents @@ -165,6 +174,10 @@ export function openPopupWithOriginBar( renderOrigin() } contentWebContents.on('did-navigate', handleDidNavigate) + // Why: origin writes fail silently if the bar is mid-load; re-asserting at + // load completion means a dropped write can never leave a stale origin up + // for the lifetime of the page. + contentWebContents.on('did-finish-load', renderOrigin) const handlePageTitleUpdated = (_event: Electron.Event, title: string): void => { if (!window.isDestroyed() && title) { window.setTitle(title) @@ -189,6 +202,7 @@ export function openPopupWithOriginBar( if (!contentWebContents.isDestroyed()) { contentWebContents.off('destroyed', handleContentDestroyed) contentWebContents.off('did-navigate', handleDidNavigate) + contentWebContents.off('did-finish-load', renderOrigin) contentWebContents.off('page-title-updated', handlePageTitleUpdated) // Why: close() (not destroy) so the page's unload handlers run — OAuth // pages often notify the opener from unload.