mirror of
https://github.com/stablyai/orca.git
synced 2026-10-18 00:03:20 +00:00
fix: elide the start of long popup origins so the registrable domain stays visible
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 <help@stably.ai>
This commit is contained in:
@@ -16,6 +16,7 @@ type FakeWebContents = {
|
||||
const { fakeElectron } = vi.hoisted(() => {
|
||||
function createFakeWebContents(): FakeWebContents {
|
||||
const handlers = new Map<string, Handler[]>()
|
||||
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('<bdi id="origin">')
|
||||
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', () => {
|
||||
|
||||
@@ -67,12 +67,16 @@ const ORIGIN_BAR_HTML = `<!doctype html><html><head><meta charset="utf-8"><style
|
||||
}
|
||||
#insecure { display: none; color: #e40014; font-weight: 600; }
|
||||
body.insecure #insecure { display: inline; }
|
||||
#origin { overflow: hidden; text-overflow: ellipsis; }
|
||||
/* Elide the START of long origins so the registrable domain stays visible
|
||||
(Chrome elides the same side): rtl container puts clip+ellipsis on the
|
||||
left; the bdi keeps the ASCII origin itself rendering LTR. */
|
||||
#origin-clip { overflow: hidden; text-overflow: ellipsis; direction: rtl; }
|
||||
#origin { unicode-bidi: isolate; direction: ltr; }
|
||||
@media (prefers-color-scheme: dark) {
|
||||
body { background: #0a0a0a; color: #fafafa; border-bottom-color: rgb(255 255 255 / 0.07); }
|
||||
#insecure { color: #ff6568; }
|
||||
}
|
||||
</style></head><body><span id="insecure">Not secure</span><span id="origin"></span></body></html>`
|
||||
</style></head><body><span id="insecure">Not secure</span><span id="origin-clip"><bdi id="origin"></bdi></span></body></html>`
|
||||
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user