fix(browser): local HTTPS Try HTTPS + cert proceed (#8454)#9104
Conversation
) Bare localhost still defaults to http:// (no silent scheme flip). After a local HTTP failure, offer Try HTTPS. For untrusted loopback HTTPS certs (ERR_CERT_AUTHORITY_INVALID), show certificate-specific copy and an explicit Proceed Anyway (Unsafe) path bound to one WebContents, secure endpoint, leaf SHA-256, and error — in-memory only, with a session request gate so Chromium's cert-cache cannot leak trust across tabs. Also covers desktop webviews and SSH/headless offscreen pages via runtime RPC + browser.certificate-trust.v1. Based on design and implementation from @AmethystLiang in #9070, rebased onto current main with conflict resolution and adversarial hardening: - pin accepted identity at grant time (reject conflicting sibling proceeds) - clear stashed load errors on navigation commit - normalize registerGuest IPC rejection for did-attach/dom-ready retry - advertise certificate-trust whenever a browser backend can host pages - harden e2e local HTTPS server document counting Closes #8454 Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds scoped local HTTPS certificate approval for eligible loopback endpoints, including certificate identity hashing, pending challenges, session request gating, navigation invalidation, and lifecycle cleanup. Exposes structured load and certificate failures through browser APIs, IPC, runtime RPC, and synchronized tab state. Replaces browser failure rendering with a shared recovery overlay supporting HTTPS retry and unsafe certificate proceeding. Adds localized messages, URL helpers, controller and renderer tests, runtime synchronization coverage, and a real local HTTPS end-to-end test. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/main/ipc/browser.ts (1)
243-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse optional chaining for
challengeIdfor consistency.While this code won't crash when
argsis undefined due to the||short-circuiting onargs?.browserPageId, using optional chaining forchallengeIdmakes the code more resilient to future refactoring.💡 Proposed consistency improvement
if ( !isTrustedBrowserRenderer(event.sender) || typeof args?.browserPageId !== 'string' || - typeof args.challengeId !== 'string' + typeof args?.challengeId !== 'string' ) {
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6dd588e6-5c71-4462-80ef-e6efd17facbc
📒 Files selected for processing (55)
docs/reference/browser-local-https-certificate-trust.mdsrc/main/browser/agent-browser-bridge.test.tssrc/main/browser/agent-browser-bridge.tssrc/main/browser/browser-certificate-challenge.tssrc/main/browser/browser-certificate-identity.tssrc/main/browser/browser-certificate-request-guard.tssrc/main/browser/browser-certificate-trust-controller.test.tssrc/main/browser/browser-certificate-trust-controller.tssrc/main/browser/browser-manager-grab.test.tssrc/main/browser/browser-manager.test.tssrc/main/browser/browser-manager.tssrc/main/browser/browser-session-registry.persistence.test.tssrc/main/browser/browser-session-registry.test.tssrc/main/browser/browser-session-registry.tssrc/main/index.tssrc/main/ipc/browser.test.tssrc/main/ipc/browser.tssrc/main/runtime/orca-runtime-browser.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/browser-core.tssrc/main/runtime/rpc/methods/browser-schemas.tssrc/main/runtime/runtime-rpc.test.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/components/browser-pane/BrowserPane.tsxsrc/renderer/src/components/browser-pane/browser-load-failure-overlay.test.tsxsrc/renderer/src/components/browser-pane/browser-load-failure-overlay.tsxsrc/renderer/src/components/browser-pane/browser-notices.test.tssrc/renderer/src/components/browser-pane/browser-notices.tssrc/renderer/src/components/tab-bar/tab-create-entry-classifier.test.tssrc/renderer/src/components/tab-bar/tab-create-entry-url-classification.tssrc/renderer/src/hooks/useIpcEvents.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/runtime/remote-server-parity.test.tssrc/renderer/src/runtime/sync-runtime-graph-browser.test.tssrc/renderer/src/runtime/sync-runtime-graph.tssrc/renderer/src/runtime/web-session-tabs-sync.test.tssrc/renderer/src/runtime/web-session-tabs-sync.tssrc/renderer/src/store/slices/browser.test.tssrc/renderer/src/store/slices/browser.tssrc/renderer/src/web/web-preload-api.tssrc/shared/browser-certificate-errors.tssrc/shared/browser-url.test.tssrc/shared/browser-url.tssrc/shared/protocol-version.tssrc/shared/runtime-types.tssrc/shared/types.tstests/e2e/browser-local-https-certificate-trust.spec.tstests/e2e/helpers/local-https-test-certificate.tstests/e2e/helpers/local-https-test-server.ts
| if (details.resourceType === 'mainFrame' && webContentsId !== undefined) { | ||
| this.reportBlockedMainFrame(webContentsId, details.url, accepted) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Suppress synthetic challenges after certificate rotation.
After a conflicting leaf is rejected and the grant is revoked, the next retry reaches this branch and recreates Proceed Anyway from the old accepted identity. Track the endpoint as rotated/blocked and suppress further challenges until process restart.
Add coverage for: replacement certificate → retry through request gate → no new failure challenge.
| async browserProceedCertificate( | ||
| params: { challengeId: string } & BrowserCommandTargetParams | ||
| ): Promise<BrowserCertificateProceedResult> { | ||
| const target = await this.resolveBrowserCommandTarget(params) | ||
| if (!target.browserPageId) { | ||
| return { ok: false, reason: 'missing' } | ||
| } | ||
| return browserCertificateTrustController.proceed(target.browserPageId, params.challengeId) | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fallback to the active browser tab when --page is omitted.
When a CLI or Agent caller uses a command with BrowserCommandTargetParams and omits the --page parameter, resolveBrowserCommandTarget resolves target.browserPageId to undefined.
Other commands in this file (like browserTabSetProfile and browserTabProfileClone) manually fall back to the bridge's active page ID. Without this fallback, browserProceedCertificate will unexpectedly fail with reason: 'missing' when run without an explicit --page target.
🐛 Proposed fix to enable implicit active-tab routing
async browserProceedCertificate(
params: { challengeId: string } & BrowserCommandTargetParams
): Promise<BrowserCertificateProceedResult> {
const target = await this.resolveBrowserCommandTarget(params)
- if (!target.browserPageId) {
+ const browserPageId = target.browserPageId ?? this.requireAgentBrowserBridge().getActivePageId(target.worktreeId)
+ if (!browserPageId) {
return { ok: false, reason: 'missing' }
}
- return browserCertificateTrustController.proceed(target.browserPageId, params.challengeId)
+ return browserCertificateTrustController.proceed(browserPageId, params.challengeId)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async browserProceedCertificate( | |
| params: { challengeId: string } & BrowserCommandTargetParams | |
| ): Promise<BrowserCertificateProceedResult> { | |
| const target = await this.resolveBrowserCommandTarget(params) | |
| if (!target.browserPageId) { | |
| return { ok: false, reason: 'missing' } | |
| } | |
| return browserCertificateTrustController.proceed(target.browserPageId, params.challengeId) | |
| } | |
| async browserProceedCertificate( | |
| params: { challengeId: string } & BrowserCommandTargetParams | |
| ): Promise<BrowserCertificateProceedResult> { | |
| const target = await this.resolveBrowserCommandTarget(params) | |
| const browserPageId = target.browserPageId ?? this.requireAgentBrowserBridge().getActivePageId(target.worktreeId) | |
| if (!browserPageId) { | |
| return { ok: false, reason: 'missing' } | |
| } | |
| return browserCertificateTrustController.proceed(browserPageId, params.challengeId) | |
| } |
| it('keeps the shared legacy local-dev forms in parity with address-bar normalization', () => { | ||
| for (const input of ['0.0.0.0:3000', '[::1]:3000', '[2001:db8::1]:3000/path']) { | ||
| expect(classifyTabEntryQuery(input, readyFiles([]))).toMatchObject({ | ||
| kind: 'host-url', | ||
| url: expect.stringMatching(/^http:/) | ||
| }) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete normalized URL.
Checking only /^http:/ would miss regressions that corrupt the IPv6 brackets, port, host, or path despite retaining the scheme. Use an input/expected table and assert the exact url.
Proposed test strengthening
- for (const input of ['0.0.0.0:3000', '[::1]:3000', '[2001:db8::1]:3000/path']) {
+ for (const [input, url] of [
+ ['0.0.0.0:3000', 'http://0.0.0.0:3000'],
+ ['[::1]:3000', 'http://[::1]:3000'],
+ ['[2001:db8::1]:3000/path', 'http://[2001:db8::1]:3000/path']
+ ]) {
expect(classifyTabEntryQuery(input, readyFiles([]))).toMatchObject({
kind: 'host-url',
- url: expect.stringMatching(/^http:/)
+ url
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('keeps the shared legacy local-dev forms in parity with address-bar normalization', () => { | |
| for (const input of ['0.0.0.0:3000', '[::1]:3000', '[2001:db8::1]:3000/path']) { | |
| expect(classifyTabEntryQuery(input, readyFiles([]))).toMatchObject({ | |
| kind: 'host-url', | |
| url: expect.stringMatching(/^http:/) | |
| }) | |
| } | |
| }) | |
| it('keeps the shared legacy local-dev forms in parity with address-bar normalization', () => { | |
| for (const [input, url] of [ | |
| ['0.0.0.0:3000', 'http://0.0.0.0:3000'], | |
| ['[::1]:3000', 'http://[::1]:3000'], | |
| ['[2001:db8::1]:3000/path', 'http://[2001:db8::1]:3000/path'] | |
| ]) { | |
| expect(classifyTabEntryQuery(input, readyFiles([]))).toMatchObject({ | |
| kind: 'host-url', | |
| url | |
| }) | |
| } | |
| }) |
| certificateFailure: { | ||
| challengeId: 'challenge-1', | ||
| browserPageId: 'page-1' | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete synchronized certificate failure.
This nested subset still passes if errorCode, error, origin, displayHost, canProceed, or observedAt are dropped. Assert the full fixture so transport-contract regressions are detected.
Proposed assertion
certificateFailure: {
challengeId: 'challenge-1',
- browserPageId: 'page-1'
+ browserPageId: 'page-1',
+ errorCode: -202,
+ error: 'ERR_CERT_AUTHORITY_INVALID',
+ origin: 'https://localhost:3443',
+ displayHost: 'localhost:3443',
+ canProceed: true,
+ observedAt: 123
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| certificateFailure: { | |
| challengeId: 'challenge-1', | |
| browserPageId: 'page-1' | |
| }, | |
| certificateFailure: { | |
| challengeId: 'challenge-1', | |
| browserPageId: 'page-1', | |
| errorCode: -202, | |
| error: 'ERR_CERT_AUTHORITY_INVALID', | |
| origin: 'https://localhost:3443', | |
| displayHost: 'localhost:3443', | |
| canProceed: true, | |
| observedAt: 123 | |
| }, |
| const firstTab = await createBrowserTab(orcaPage, worktreeId, firstServer.schemeLessUrl) | ||
| const firstSlot = browserSlot(orcaPage, firstTab.id) | ||
|
|
||
| await expect(firstSlot.getByRole('button', { name: 'Try HTTPS' })).toBeVisible() | ||
| await firstSlot.getByRole('button', { name: 'Try HTTPS' }).click() | ||
| await expect( | ||
| firstSlot.getByRole('heading', { name: "Connection isn't secure" }) | ||
| ).toBeVisible() | ||
| // The certificate-failure branch keeps its safe recovery actions and the | ||
| // certificate-specific hint, but never the local-server connectivity hint. | ||
| await expect(firstSlot.getByRole('button', { name: 'Copy Address' })).toBeVisible() | ||
| await expect(firstSlot.getByRole('button', { name: 'Retry' })).toBeVisible() | ||
| await expect(firstSlot.getByText(/use a trusted local certificate/i)).toBeVisible() | ||
| await expect(firstSlot.getByText(/make sure the server is running/i)).toHaveCount(0) | ||
| await firstSlot.getByRole('button', { name: 'Proceed Anyway (Unsafe)' }).click() | ||
| await expect | ||
| .poll(() => readBrowserHeading(orcaPage, firstTab.id), { timeout: 10_000 }) | ||
| .toBe('Local HTTPS request 1') | ||
| await expect | ||
| .poll(() => readBrowserState(orcaPage, firstTab.id, '__localTlsState')) | ||
| .toEqual({ asset: true, webSocket: true }) | ||
| expect(firstServer.assetRequestCount()).toBe(1) | ||
| expect(firstServer.webSocketConnectionCount()).toBe(1) | ||
|
|
||
| const secondTab = await createBrowserTab(orcaPage, worktreeId, firstServer.secureUrl) | ||
| const secondSlot = browserSlot(orcaPage, secondTab.id) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the browser page ID for viewport and webview lookups.
These helpers query a viewport keyed by BrowserPage.id, but the calls pass the workspace-tab id. Use .pageId; retain .id only for switchToBrowserTab.
Proposed fix
- const firstSlot = browserSlot(orcaPage, firstTab.id)
+ const firstSlot = browserSlot(orcaPage, firstTab.pageId)
- .poll(() => readBrowserHeading(orcaPage, firstTab.id), { timeout: 10_000 })
+ .poll(() => readBrowserHeading(orcaPage, firstTab.pageId), { timeout: 10_000 })
- .poll(() => readBrowserState(orcaPage, firstTab.id, '__localTlsState'))
+ .poll(() => readBrowserState(orcaPage, firstTab.pageId, '__localTlsState'))
- const secondSlot = browserSlot(orcaPage, secondTab.id)
+ const secondSlot = browserSlot(orcaPage, secondTab.pageId)
- .poll(() => readBrowserState(orcaPage, probeTab.id, '__siblingTlsProbe'))
+ .poll(() => readBrowserState(orcaPage, probeTab.pageId, '__siblingTlsProbe'))
await switchToBrowserTab(orcaPage, worktreeId, firstTab.id)
- await reloadBrowserGuest(orcaPage, firstTab.id)
+ await reloadBrowserGuest(orcaPage, firstTab.pageId)
- .poll(() => readBrowserHeading(orcaPage, firstTab.id), { timeout: 10_000 })
+ .poll(() => readBrowserHeading(orcaPage, firstTab.pageId), { timeout: 10_000 })Also applies to: 171-185
Main's #9104 reads browserCertificateFailuresByPageId in buildMobileBrowserTab but left partial AppState test helpers without the field, breaking PR Checks merge commits. Default the map in fixtures and use optional chaining so partial state cannot throw.
* fix(agents): recognize OpenCode native OC | tab titles OpenCode's native OSC titles use `OC | <task>` without an `opencode` token, so title classifiers left tabs as Claude/unknown. Map the native marker (optional mux prefix) to OpenCode identity in both title classifiers, exclude it from isClaudeAgent, and cover lookalikes plus stale Claude launch reclaim. Builds on and supersedes #8590 (credit @gatsby74). Fixes #8478. * fix(agents): drop renderer import from #8478 shared repro tsconfig.node includes src/shared tests; importing agent-status pulled renderer modules outside the node project and failed typecheck. Assert opencode identity via shared title classifiers only (OpenCode TUI sets "OpenCode" and `OC | ${title}`). * fix(runtime): fill browser cert failure map in mobile snapshot fixtures Main's #9104 reads browserCertificateFailuresByPageId in buildMobileBrowserTab but left partial AppState test helpers without the field, breaking PR Checks merge commits. Default the map in fixtures and use optional chaining so partial state cannot throw.
1 participant
Description
Fixes #8454 — the built-in browser could not reach HTTPS-only local dev servers (e.g.
next dev --experimental-https).Two independent failures:
http://(no silent flip to HTTPS — that would break normal HTTP dev servers). After a local HTTP failure, the load overlay offers Try HTTPS.ERR_CERT_AUTHORITY_INVALIDon eligible loopback hosts only (localhost/*.localhost/127/8/::1).Approval is temporary and narrow: one WebContents × secure endpoint × leaf SHA-256 × error, in-memory only. A session
webRequestgate contains Chromium’s certificate-cache so trust cannot leak to sibling tabs/assets/WSS. Desktop webviews and SSH/headless offscreen pages both supported (browser.certificate.proceed+browser.certificate-trust.v1).Credit: Design + core implementation from @AmethystLiang in #9070. This PR rebases that work onto current
main, resolves conflicts (markup mode, clicked-link frame routing, removedserveSimStateWatcher), and adds adversarial hardening (below). Supersedes #9070.Evidence (of fix — tests/commands before→after)
Before (main / this worktree pre-fix):
After:
Still: scheme-less
localhost:3000→http://…(intentional). New coverage for eligibility, HTTPS recovery URLs, grant isolation, conflicting-leaf proceed rejection, and overlay actions. Real-Electron e2e suite added attests/e2e/browser-local-https-certificate-trust.spec.ts(not run in this agent pass; unit mocks alone do not replace packaged validation).ELI5
Typing
localhost:3000still goes to plain HTTP first (most dev servers are HTTP). If that fails, a button lets you try HTTPS. If the HTTPS cert is self-signed and local, Orca shows a clear warning and a one-time “I understand, open it” button for that tab only — not a global “ignore all certs” switch.User-regression-tradeoffs
Ideally zero for normal workflows:
http://)https://file:/// UNC / absolute pathsAdversarial review findings fixed (vs #9070)
proceed()successfully after the first approvalERR_ABORTEDcannot restore an old overlay over a good pageregisterGuestIPC rejection →falseso did-attach/dom-ready retry is cleanbrowser.certificate-trust.v1whenever a browser backend can host pages (desktop or headless), not only offscreen/as document requests (favicon/probes 404)