Open a sidebar HTML preview full-page in the external browser - #1638
Open a sidebar HTML preview full-page in the external browser#1638brsbl wants to merge 1 commit into
Conversation
The inline-vis external icon in a thread opens the visualization in the sidebar. This adds the second step: the sidebar preview header gains an Open in external browser action that opens the rendered page full-page in the user's default browser. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🚨 SLOP COP 🚨 · I am the Slop Cop. I am reviewing this pull request for security, code quality, performance, architecture, and user behavior. |
| "shrink-0 text-muted-foreground hover:bg-state-hover hover:text-foreground", | ||
| )} | ||
| onClick={() => { | ||
| openUrlInExternalBrowser( |
There was a problem hiding this comment.
🚨 slopcop/review — The new action opens workspace HTML as a top-level page on the trusted bb host.
The CSP gives the page an opaque origin, but it still permits scripts and outbound requests. A malicious file can show a false bb login page and send entered secrets elsewhere. The address bar still shows the trusted bb host. Please use a separate untrusted host. A trusted wrapper with a fixed warning and sandboxed iframe is another safe option. Add a test that proves preview content cannot replace or hide the warning.
| "shrink-0 text-muted-foreground hover:bg-state-hover hover:text-foreground", | ||
| )} | ||
| onClick={() => { | ||
| openUrlInExternalBrowser( |
There was a problem hiding this comment.
🚨 slopcop/review — Electron sends this URL to the system browser, which does not share the Electron Connect cookie.
Connect returns a sign-in page when the system browser has no owner cookie. The action then fails to open the preview immediately. Please add a secure browser handoff or another authentication flow that ends at the preview. Add a test for a paired desktop user with no system-browser cookie.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary
This change adds a globe button to rendered HTML previews in the sidebar. The button opens the HTML as a full page in the system browser.
Findings
- Medium — A preview can show a false bb page on the trusted bb host. The server CSP blocks cookie and storage access. However, scripts can still run and send entered data elsewhere. A malicious file can copy a bb login page while the address bar shows the trusted host. Use an untrusted preview host or a trusted wrapper with a fixed warning.
- Medium — The Electron Connect path can show a sign-in page instead of the preview. Electron stores its Connect cookie in its own session. The system browser does not share that cookie. Add a secure handoff or an authentication flow that returns to the preview.
Review details
- The change uses the shared external-browser helper. I found no new code duplication that needs a required refactor.
- The code creates no blob URL, timer, subscription, or global event handler. I found no performance or cleanup issue.
- The full app suite passed 347 test files and 2,768 tests. The app type check and
git diff --checkalso passed. - The dev app loaded in a real browser. Its isolated data had no projects or threads, so the full sidebar click path was unavailable.
- The browser and dev server stopped after the test.
The final GPT-5.6 review gate marked both medium issues as required fixes. I posted this as a comment-only review, as required by this rule.
An inline visualization in a thread (
::inline-vis{file="…"}) has an external icon whose click opens the file in the sidebar. This adds the second step: once it is open there, the sidebar preview header gains an Open in external browser action that opens the visualization full-page in the user's default browser.The first click is unchanged —
plugins/inline-vis/app.tsxis untouched, so the in-thread icon still only ever opens the sidebar.Decisions worth reviewing
Globe, notExternalLink.OpenInEditorButtonalready rendersExternalLinkin this same toolbar; reusing it produced two identical adjacent icons.Globeis the codebase's browser signifier (BrowserTabContent,NewTabFileSearch,ThreadDetailView). The existing "Open in editor" affordance is untouched and still works.apps/desktop/src/main.tsparses withnew URL(payload)and no base — a relative path throws into a barereturn, i.e. a dead button with no error.toAbsolutePreviewUrlis what makes the desktop path work; a test pins it.openUrlInExternalBrowser, deliberately notuseOpenUrlByPreference. The latter honours the in-app-browser preference and would route the visualization to bb's in-app browser panel, which is not "the user's default external browser".htmlandiframestates) and is absent for text/markdown/csv. A negative test pins that.Verification
Gates on this branch, rebased onto current
main(includes #1630 and #1635):tsc --noEmitclean ·FilePreview.test.tsx19/19 ·plugins/inline-vis16/16 unchanged · ESLint 0 errors (4 pre-existingset-state-in-effectwarnings on lines this diff does not touch).Driven through the real UI in an isolated dev instance, three assertions:
The served route returns
200 text/htmlwith nocontent-disposition, so it renders rather than downloads. It carriescontent-security-policy: sandbox allow-scripts, which also applies to the top-level document: scripts run and relative assets load, butlocalStorage/cookies/same-origin fetches are unavailable — identical to what the sidebar iframe already imposes. The CSP is the security boundary for arbitrary worktree HTML and is deliberately left alone.Known gap
The live click-through exercised the browser dispatch (
window.open). The desktop dispatch (shell.openExternal→ OS default browser) is covered by a unit test assertingopenExternalUrlreceives the absolute URL, and the main-process consumer was confirmed by readingmain.ts, but the IPC hop itself was not exercised by clicking inside a packaged Electron build.BB-Thread-ID: thr_zhe3sbcbgx