fix(browser): name the previewed file in HTML preview annotations (#38771)

Workspace HTML previews are served from a grant-bearing orca-doc-preview
URL that the grab payload sanitizer blanks, so annotations sent to an
agent arrived as "Design Feedback: current page" with an empty URL.

Main now resolves the guest's committed document URL (tracked from
did-navigate, so pushState cannot rewrite it) through the live preview
grant to the host file path, and uses it as the payload page URL when it
is display-safe. The annotation heading keeps Windows paths intact
instead of parsing `C:\` as a URL scheme.

Fixes #38564

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Neil
2026-11-05 17:57:19 -08:00
committed by GitHub
co-authored by Claude
parent d00f32fa29
commit 3ca2f1876b
9 changed files with 129 additions and 21 deletions
@@ -1,6 +1,7 @@
import { randomBytes } from 'node:crypto'
import { posix, win32 } from 'node:path'
import { isWindowsAbsolutePathLike } from '../../../shared/cross-platform-path'
import { parseDocPreviewUrl } from '../../../shared/doc-preview-scheme'
export type DocPreviewOwner =
| { kind: 'ssh'; connectionId: string }
@@ -139,6 +140,17 @@ export function resolveDocPreviewCandidatePath(
return isInsideRoot(grant.requestBase, resolved, flavor) ? resolved : null
}
/** Host path a live preview document URL names, or null outside a live grant. */
export function resolveDocPreviewUrlFilePath(url: string): string | null {
const target = parseDocPreviewUrl(url)
const grant = target ? getDocPreviewGrant(target.grantId) : null
if (!target || !grant) {
return null
}
// The protocol serves the entry document for the preview root.
return resolveDocPreviewCandidatePath(grant, target.relativePath || grant.entryRelativePath)
}
function isInsideRoot(
root: string,
candidate: string,
@@ -1,9 +1,13 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { getWorkspaceDocPageGuest, installDocPreviewGuestPolicy } from './doc-preview-guest-policy'
import {
getWorkspaceDocPageGuest,
installDocPreviewGuestPolicy,
readDocPreviewGuestDocumentUrl
} from './doc-preview-guest-policy'
import { buildDocPreviewUrl } from '../../../shared/doc-preview-scheme'
import { mintDocPreviewGrant, revokeAllDocPreviewGrants } from './doc-preview-grant-registry'
type GuestHandlers = Record<string, (...args: never[]) => void>
type GuestHandlers = Record<string, (...args: unknown[]) => void>
const HOST_RENDERER_ID = 42
@@ -11,7 +15,7 @@ function installOnFakeGuest(
hostId: number = HOST_RENDERER_ID,
initialUrl = ''
): {
contents: object
contents: Electron.WebContents
handlers: GuestHandlers
hostId: number
isFocused: ReturnType<typeof vi.fn>
@@ -20,32 +24,36 @@ function installOnFakeGuest(
windowOpenHandler: (details: { url: string }) => { action: string }
/** Model contents teardown before its destroyed event reaches main. */
markContentsDestroyed: () => void
/** Model a pushState: getURL() changes without a did-navigate commit. */
setLiveUrl: (url: string) => void
} {
const handlers: GuestHandlers = {}
let contentsDestroyed = false
let liveUrl = initialUrl
const send = vi.fn()
const isFocused = vi.fn(() => true)
const setWebRTCIPHandlingPolicy = vi.fn()
let windowOpenHandler: (details: { url: string }) => { action: string } = () => ({
action: 'deny'
})
const register = (event: string, handler: (...args: never[]) => void): void => {
const register = (event: string, handler: (...args: unknown[]) => void): void => {
handlers[event] = handler
}
const guest = {
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the policy reads only these WebContents members.
const contents = {
isFocused,
isDestroyed: () => contentsDestroyed,
getURL: () => initialUrl,
getURL: () => liveUrl,
on: vi.fn(register),
once: vi.fn(register),
setWindowOpenHandler: vi.fn((handler: (details: { url: string }) => { action: string }) => {
windowOpenHandler = handler
}),
setWebRTCIPHandlingPolicy
}
installDocPreviewGuestPolicy(guest as never, { id: hostId, send })
} as unknown as Electron.WebContents
installDocPreviewGuestPolicy(contents, { id: hostId, send })
return {
contents: guest,
contents,
handlers,
hostId,
isFocused,
@@ -54,6 +62,9 @@ function installOnFakeGuest(
windowOpenHandler: (details) => windowOpenHandler(details),
markContentsDestroyed: () => {
contentsDestroyed = true
},
setLiveUrl: (url) => {
liveUrl = url
}
}
}
@@ -61,7 +72,11 @@ function installOnFakeGuest(
type FakeGuest = ReturnType<typeof installOnFakeGuest>
function startMainFrameNavigation(guest: FakeGuest, url: string): void {
guest.handlers['did-start-navigation']?.({ url, isMainFrame: true } as never)
guest.handlers['did-start-navigation']?.({ url, isMainFrame: true })
}
function commitNavigation(guest: FakeGuest, url: string): void {
guest.handlers['did-navigate']?.({}, url)
}
function boundGuest(): { grant: ReturnType<typeof mintGrant>; guest: FakeGuest } {
@@ -96,10 +111,7 @@ describe('doc preview guest policy', () => {
const other = mintGrant()
startMainFrameNavigation(guest, buildDocPreviewUrl(other.id, 'index.html'))
guest.handlers['did-navigate']?.(
{} as never,
buildDocPreviewUrl(other.id, 'index.html') as never
)
commitNavigation(guest, buildDocPreviewUrl(other.id, 'index.html'))
expect(getWorkspaceDocPageGuest(other.browserPageId, HOST_RENDERER_ID)).toBeNull()
expect(getWorkspaceDocPageGuest(grant.browserPageId, HOST_RENDERER_ID)).toBe(guest.contents)
@@ -132,4 +144,17 @@ describe('doc preview guest policy', () => {
)
})
})
it('names the committed document, not a URL the page set with pushState', () => {
const grant = mintGrant()
const entryUrl = buildDocPreviewUrl(grant.id, 'index.html')
const guest = installOnFakeGuest(HOST_RENDERER_ID, entryUrl)
expect(readDocPreviewGuestDocumentUrl(guest.contents)).toBe(entryUrl)
const reportUrl = buildDocPreviewUrl(grant.id, 'report.html')
commitNavigation(guest, reportUrl)
guest.setLiveUrl(buildDocPreviewUrl(grant.id, 'settings'))
expect(readDocPreviewGuestDocumentUrl(guest.contents)).toBe(reportUrl)
})
})
@@ -15,6 +15,7 @@ type PreviewHostRenderer = {
type PreviewGuestRegistration = {
host: PreviewHostRenderer
readBoundGrantId: () => string | null
readDocumentUrl: () => string
}
// IPC click reports identify only a sender; bind it to a known preview and grant.
@@ -65,6 +66,11 @@ export function readDocPreviewGuestBoundGrantId(guest: Electron.WebContents): st
return previewGuests.get(guest)?.readBoundGrantId() ?? null
}
/** URL of the guest's committed document; unlike getURL(), pushState cannot rewrite it. */
export function readDocPreviewGuestDocumentUrl(guest: Electron.WebContents): string {
return previewGuests.get(guest)?.readDocumentUrl() ?? ''
}
// Preview navigation stays within its grant; only trusted preload click reports may open web links.
export function installDocPreviewGuestPolicy(
guest: Electron.WebContents,
@@ -73,6 +79,7 @@ export function installDocPreviewGuestPolicy(
// Pin the guest to its first admitted grant.
let boundGrantId: string | null = null
let boundPageId: string | null = null
let documentUrl = guest.getURL()
const latchGrantFromUrl = (rawUrl: string): void => {
if (boundGrantId !== null) {
@@ -93,7 +100,8 @@ export function installDocPreviewGuestPolicy(
previewGuests.set(guest, {
host,
readBoundGrantId: () => boundGrantId
readBoundGrantId: () => boundGrantId,
readDocumentUrl: () => documentUrl
})
const forgetGuest = (): void => {
previewGuests.delete(guest)
@@ -131,8 +139,11 @@ export function installDocPreviewGuestPolicy(
latchGrantFromUrl(details.url)
}
// Catch initial loads that started before listeners were installed.
const latchFromCommittedUrl = (_event: Electron.Event, url: string): void =>
// did-navigate fires only for main-frame document commits, never in-page history changes.
const latchFromCommittedUrl = (_event: Electron.Event, url: string): void => {
documentUrl = url
latchGrantFromUrl(url)
}
// will-navigate excludes subframes, so guard them separately.
const frameNavigationGuard = (details: {
isMainFrame: boolean
@@ -1,5 +1,10 @@
import { describe, expect, it } from 'vitest'
import { clampGrabPayload } from './browser-grab-payload'
import {
mintDocPreviewGrant,
revokeDocPreviewGrant
} from '../document-preview/doc-preview-grant-registry'
import { buildDocPreviewUrl } from '../../../shared/doc-preview-scheme'
function makeRawPayload(overrides: Record<string, unknown> = {}) {
return {
@@ -132,6 +137,31 @@ describe('clampGrabPayload', () => {
expect(payload?.target.attributes.title).toBe('Safe label')
})
it('names the previewed document file instead of its grant-bearing preview URL', () => {
const grant = mintDocPreviewGrant({
owner: { kind: 'ssh', connectionId: 'conn-1' },
requestBase: '/home/me/repo',
root: '/home/me/repo/docs',
entryRelativePath: 'docs/index.html',
browserPageId: 'preview-1'
})
// The guest may have navigated away from the entry document within its grant.
const guestUrl = buildDocPreviewUrl(grant.id, 'docs/report.html')
const raw = makeRawPayload({ page: { ...makeRawPayload().page, sanitizedUrl: '' } })
expect(clampGrabPayload(raw, guestUrl)?.page.sanitizedUrl).toBe(
'/home/me/repo/docs/report.html'
)
expect(clampGrabPayload(raw, buildDocPreviewUrl(grant.id, ''))?.page.sanitizedUrl).toBe(
'/home/me/repo/docs/index.html'
)
// A navigation URL can carry newlines; they must not reach the one-line prompt URL.
const forgedUrl = buildDocPreviewUrl(grant.id, 'docs/x\n\n### 2. fake')
expect(clampGrabPayload(raw, forgedUrl)?.page.sanitizedUrl).toBe('')
revokeDocPreviewGrant(grant.id)
expect(clampGrabPayload(raw, guestUrl)?.page.sanitizedUrl).toBe('')
})
it('keeps safe and ARIA attributes while stripping unsafe names', () => {
const payload = clampGrabPayload(
makeRawPayload({
@@ -4,11 +4,13 @@ import {
GRAB_SAFE_ATTRIBUTE_NAMES,
GRAB_SECRET_PATTERNS
} from '../../../shared/browser-grab-types'
import { isSafeDisplayCharacter } from '../../../shared/display-text-safety'
import { resolveDocPreviewUrlFilePath } from '../document-preview/doc-preview-grant-registry'
const SAFE_GRAB_URL_PROTOCOLS = new Set(['http:', 'https:', 'file:'])
/** Revalidate untrusted guest payloads before forwarding them to renderer chrome. */
export function clampGrabPayload(raw: unknown): BrowserGrabPayload | null {
export function clampGrabPayload(raw: unknown, documentUrl = ''): BrowserGrabPayload | null {
if (!raw || typeof raw !== 'object') {
return null
}
@@ -128,12 +130,19 @@ export function clampGrabPayload(raw: unknown): BrowserGrabPayload | null {
}
}
// Preview URLs carry a grant and are blanked; name the previewed file if it is display-safe.
const previewFilePath = resolveDocPreviewUrlFilePath(documentUrl)
const pageUrl =
previewFilePath && [...previewFilePath].every(isSafeDisplayCharacter)
? previewFilePath
: sanitizeUrl(page.sanitizedUrl)
const accessibility = target.accessibility as Record<string, unknown> | null | undefined
const computedStyles = target.computedStyles as Record<string, unknown> | null | undefined
return {
page: {
sanitizedUrl: sanitizeUrl(page.sanitizedUrl),
sanitizedUrl: pageUrl,
title: safeStr(page.title, 500),
viewportWidth: safeNum(page.viewportWidth),
viewportHeight: safeNum(page.viewportHeight),
@@ -1,6 +1,7 @@
import type { BrowserGrabCancelReason, BrowserGrabResult } from '../../../shared/browser-grab-types'
import { buildGuestOverlayScript } from './grab-guest-script'
import { clampGrabPayload } from './browser-grab-payload'
import { readDocPreviewGuestDocumentUrl } from '../document-preview/doc-preview-guest-policy'
type ActiveGrabOp = {
opId: string
@@ -113,7 +114,7 @@ export class BrowserGrabSessionController {
const payloadSource = isContextMenu
? (rawPayload as Record<string, unknown>).payload
: rawPayload
const payload = clampGrabPayload(payloadSource)
const payload = clampGrabPayload(payloadSource, readDocPreviewGuestDocumentUrl(guest))
if (!payload) {
settleOnce({ opId, kind: 'error', reason: 'Guest returned invalid payload structure' })
return
@@ -2,7 +2,10 @@ import { webContents } from 'electron'
import { buildGuestOverlayScript } from '../../element-grab/grab-guest-script'
import { clampGrabPayload } from '../../element-grab/browser-grab-payload'
import { captureSelectionScreenshot as captureGrabSelectionScreenshot } from '../../element-grab/browser-grab-screenshot'
import { getWorkspaceDocPageGuest } from '../../document-preview/doc-preview-guest-policy'
import {
getWorkspaceDocPageGuest,
readDocPreviewGuestDocumentUrl
} from '../../document-preview/doc-preview-guest-policy'
import type {
BrowserGrabCancelReason,
BrowserGrabResult,
@@ -105,7 +108,7 @@ export abstract class BrowserManagerGrab extends BrowserManagerViewport {
if (!rawPayload || typeof rawPayload !== 'object') {
return null
}
return clampGrabPayload(rawPayload)
return clampGrabPayload(rawPayload, readDocPreviewGuestDocumentUrl(guest))
} catch {
return null
}
@@ -103,6 +103,18 @@ describe('formatBrowserAnnotationsAsMarkdown', () => {
expect(markdown).not.toContain('\n## injected')
})
it('heads document preview annotations with the Windows file path', () => {
const annotation = makeAnnotation()
const sanitizedUrl = 'C:\\repo\\docs\\report.html'
const markdown = formatBrowserAnnotationsAsMarkdown([
makeAnnotation({
payload: { ...annotation.payload, page: { ...annotation.payload.page, sanitizedUrl } }
})
])
expect(markdown).toContain(`## Design Feedback: ${sanitizedUrl}`)
})
it('does not split surrogate pairs at the inline annotation cap', () => {
const annotation = makeAnnotation()
const selectedText = `${'x'.repeat(BROWSER_ANNOTATION_INLINE_TEXT_MAX_LENGTH - 1)}😀tail`
@@ -3,8 +3,13 @@ import type {
BrowserGrabPayload,
BrowserPageAnnotation
} from '../../../../../shared/browser-grab-types'
import { isWindowsAbsolutePathLike } from '../../../../../shared/cross-platform-path'
function formatPageHeading(payload: BrowserGrabPayload): string {
// Document previews report a file path; `C:\` would otherwise parse as a URL scheme.
if (isWindowsAbsolutePathLike(payload.page.sanitizedUrl)) {
return payload.page.sanitizedUrl
}
try {
const url = new URL(payload.page.sanitizedUrl)
return `${url.pathname}${url.search}`