From 25e53f4b0ca37cad5e17ef3fb94fc1acd8ebfa22 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 19 Oct 2026 04:21:16 -0700 Subject: [PATCH] fix(security): put every trust decision on a host-owned token surface (#30710) * fix(security): put every trust dialog on a host-owned token surface Plugin themes write CSS tokens on the document root, and dialogs portal to body, so any trust surface without plugin-security-chrome inherits them; a theme could hide what is being approved. The orca.yaml hook trust dialog, notebook run confirmation, plugin install, remove and rollback dialogs were missing it. DialogContent gains surface="security", which applies the host-owned tokens, and a scrollable prop for the plugin dialogs' existing viewport-height scrolling; every trust dialog uses the surface and a test lists them. The style guide says when to use each. * fix(security): keep inline trust decisions on the host-owned surface The agent command approval card (Allow/Deny) and the browser certificate override sit outside a dialog, so plugin theme tokens still reached them. A SecuritySurface primitive puts its child on the same host-owned token set as DialogContent surface="security"; both use it, and the security surface test now lists inline trust surfaces as well as dialogs. --- docs/STYLEGUIDE.md | 2 + .../navigate/browser-load-failure-overlay.tsx | 202 ++++++++------- .../src/components/editor/IpynbViewer.tsx | 2 +- .../native-chat/NativeChatApprovalCard.tsx | 244 +++++++++--------- .../settings/PluginConsentDialog.tsx | 4 +- .../settings/PluginInstallDialog.tsx | 2 +- .../PluginMarketplacePreviewDialog.tsx | 2 +- .../PluginMarketplaceSourceDialog.tsx | 4 +- .../settings/PluginRemoveDialog.tsx | 1 + .../settings/PluginRollbackDialog.tsx | 1 + .../sidebar/OrcaYamlTrustDialog.tsx | 2 +- src/renderer/src/components/ui/dialog.tsx | 9 +- .../components/ui/security-surface.test.tsx | 61 +++++ .../src/components/ui/security-surface.tsx | 15 ++ 14 files changed, 327 insertions(+), 224 deletions(-) create mode 100644 src/renderer/src/components/ui/security-surface.test.tsx create mode 100644 src/renderer/src/components/ui/security-surface.tsx diff --git a/docs/STYLEGUIDE.md b/docs/STYLEGUIDE.md index e236389c7d8..9b9a6f5a62f 100644 --- a/docs/STYLEGUIDE.md +++ b/docs/STYLEGUIDE.md @@ -156,6 +156,8 @@ Sizes: `default` (36px), `sm` (32px), `xs` (24px), `lg` (40px), plus `icon`, `ic Browse `src/renderer/src/components/ui/` for the full list. Most wrap a Radix UI primitive — exceptions are `command` (wraps `cmdk`), `sonner` (wraps `sonner`), and the visual-only wrappers (`badge`, `button-group`, `card`, `input`) which apply tokens and Tailwind utilities directly. Never reimplement headless behavior; extend the existing wrapper. +Use `DialogContent surface="security"` for every dialog that asks the user to trust, approve, run or remove code, including plugin provenance and approval, so plugin themes cannot change the trust surface's tokens. Use its `scrollable` prop when the whole dialog scrolls. Wrap a trust decision outside a dialog, such as an agent approval card or a certificate override, in `SecuritySurface`. + ### Picking the right primitive When a control has multiple plausible primitives, use this fork: diff --git a/src/renderer/src/components/browser-pane/navigate/browser-load-failure-overlay.tsx b/src/renderer/src/components/browser-pane/navigate/browser-load-failure-overlay.tsx index eb79eed7d6a..d8163f21a61 100644 --- a/src/renderer/src/components/browser-pane/navigate/browser-load-failure-overlay.tsx +++ b/src/renderer/src/components/browser-pane/navigate/browser-load-failure-overlay.tsx @@ -1,3 +1,4 @@ +import { SecuritySurface } from '@/components/ui/security-surface' import { useEffect, useRef, useState } from 'react' import { Copy, ExternalLink, Globe, Loader2, RefreshCw, ShieldAlert } from 'lucide-react' @@ -261,107 +262,114 @@ export function BrowserLoadFailureOverlay({ ) : null return ( -
-
-
- {certificateError ? : } -
-

- {certificateError - ? translate('browser.loadFailure.connectionNotSecure', "Connection isn't secure") - : guestRecoveryError - ? translate('browser.guestRecovery.title', 'Browser page stopped') - : meta.host - ? translate('browser.loadFailure.cantReachHost', "Can't reach {{value0}}", { - value0: meta.host - }) - : translate('browser.loadFailure.cantLoadPage', "Can't load this page")} -

-

- {formatLoadFailureDescription(presentationLoadError, meta)} -

- {certificateError && meta.isLocalhostLike ? ( -

- {translate( - 'browser.loadFailure.trustedCertificateGuidance', - 'For local development, use a trusted local certificate when possible.' - )} + +

+
+
+ {certificateError ? : } +
+

+ {certificateError + ? translate('browser.loadFailure.connectionNotSecure', "Connection isn't secure") + : guestRecoveryError + ? translate('browser.guestRecovery.title', 'Browser page stopped') + : meta.host + ? translate('browser.loadFailure.cantReachHost', "Can't reach {{value0}}", { + value0: meta.host + }) + : translate('browser.loadFailure.cantLoadPage', "Can't load this page")} +

+

+ {formatLoadFailureDescription(presentationLoadError, meta)}

- ) : null} - {recoveryHint ?

{recoveryHint}

: null} - {sshRoutedHint ? ( -

- {translate( - 'browser.loadFailure.sshRoutedHint', - "This page browses through the workspace's SSH host — the host may be disconnected or unable to reach this site." - )} - {onRecheckSshRoute ? ( + {certificateError && meta.isLocalhostLike ? ( +

+ {translate( + 'browser.loadFailure.trustedCertificateGuidance', + 'For local development, use a trusted local certificate when possible.' + )} +

+ ) : null} + {recoveryHint ? ( +

{recoveryHint}

+ ) : null} + {sshRoutedHint ? ( +

+ {translate( + 'browser.loadFailure.sshRoutedHint', + "This page browses through the workspace's SSH host — the host may be disconnected or unable to reach this site." + )} + {onRecheckSshRoute ? ( + <> + {' '} + + + ) : null} +

+ ) : null} + {activeProceedAttempt?.state === 'failed' && activeProceedAttempt.reason ? ( +

+ {formatCertificateProceedFailure(activeProceedAttempt.reason)} +

+ ) : null} +
+ {certificateError ? ( <> - {' '} - + {externalButton} + {retryButton} + {copyButton} + {matchingCertificateFailure ? ( + + ) : null} - ) : null} -

- ) : null} - {activeProceedAttempt?.state === 'failed' && activeProceedAttempt.reason ? ( -

- {formatCertificateProceedFailure(activeProceedAttempt.reason)} -

- ) : null} -
- {certificateError ? ( - <> - {externalButton} - {retryButton} - {copyButton} - {matchingCertificateFailure ? ( - - ) : null} - - ) : ( - <> - {httpsRecoveryUrl && !guestRecoveryError ? ( - - ) : null} - {retryButton} - {copyButton} - {externalButton} - - )} + ) : ( + <> + {httpsRecoveryUrl && !guestRecoveryError ? ( + + ) : null} + {retryButton} + {copyButton} + {externalButton} + + )} +
-
+ ) } diff --git a/src/renderer/src/components/editor/IpynbViewer.tsx b/src/renderer/src/components/editor/IpynbViewer.tsx index 03beb68b122..e5283b7cb85 100644 --- a/src/renderer/src/components/editor/IpynbViewer.tsx +++ b/src/renderer/src/components/editor/IpynbViewer.tsx @@ -255,7 +255,7 @@ export default function IpynbViewer({ } }} > - + {translate('auto.components.editor.IpynbViewer.9e06ae5d36', 'Run Notebook Code?')} diff --git a/src/renderer/src/components/native-chat/NativeChatApprovalCard.tsx b/src/renderer/src/components/native-chat/NativeChatApprovalCard.tsx index 2918058b790..7dd0c988b09 100644 --- a/src/renderer/src/components/native-chat/NativeChatApprovalCard.tsx +++ b/src/renderer/src/components/native-chat/NativeChatApprovalCard.tsx @@ -1,3 +1,4 @@ +import { SecuritySurface } from '@/components/ui/security-surface' import { useRef } from 'react' import { ShieldQuestion, X } from 'lucide-react' import { cn } from '@/lib/utils' @@ -60,133 +61,136 @@ export function NativeChatApprovalCard({ const escape = onCancel ?? (isSubmitting ? undefined : onCollapse) return ( -
-
-
{ - if (event.key === 'Escape' && !event.nativeEvent.isComposing && escape) { - event.preventDefault() - event.stopPropagation() - escape() - } - }} - className="flex min-h-0 w-full flex-1 flex-col gap-2 overflow-hidden rounded-lg border border-input bg-card px-4 py-3 shadow-xs focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" - > -
- -
-

- {approval.title} -

-
- {onCancel ? ( - - ) : onCollapse ? ( - - ) : null} -
- {hasContext ? ( -
- {approval.description ? ( -

{approval.description}

- ) : null} - {approval.decisionReason ? ( -

- - {translate('components.native-chat.approval.reason', 'Reason')}:{' '} - - {approval.decisionReason} + +

+
+
{ + if (event.key === 'Escape' && !event.nativeEvent.isComposing && escape) { + event.preventDefault() + event.stopPropagation() + escape() + } + }} + className="flex min-h-0 w-full flex-1 flex-col gap-2 overflow-hidden rounded-lg border border-input bg-card px-4 py-3 shadow-xs focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring" + > +
+ +
+

+ {approval.title}

- ) : null} - {neededPath ? ( -

- - {translate('components.native-chat.approval.needsAccess', 'Needs access to')} - :{' '} - - {neededPath} -

- ) : null} - {isPlanApprovalSubject(approval.subject) ? ( -
- - {approval.subject.filePath ? ( -

- - {translate('components.native-chat.approval.plan.file', 'Plan file')}:{' '} - - {approval.subject.filePath} -

- ) : null} -
- ) : approval.detail ? ( -
+ {onCancel ? ( +
- ) : null} - {newerSubject ? ( -

- {translate( - 'components.native-chat.approval.needsNewerOrca', - 'This request needs a newer version of Orca.' - )} -

+ + + ) : onCollapse ? ( + ) : null}
- ) : null} -
- {approval.options.map((opt, i) => ( - - ))} + {approval.description ? ( +

{approval.description}

+ ) : null} + {approval.decisionReason ? ( +

+ + {translate('components.native-chat.approval.reason', 'Reason')}:{' '} + + {approval.decisionReason} +

+ ) : null} + {neededPath ? ( +

+ + {translate('components.native-chat.approval.needsAccess', 'Needs access to')} + :{' '} + + {neededPath} +

+ ) : null} + {isPlanApprovalSubject(approval.subject) ? ( +
+ + {approval.subject.filePath ? ( +

+ + {translate('components.native-chat.approval.plan.file', 'Plan file')} + :{' '} + + {approval.subject.filePath} +

+ ) : null} +
+ ) : approval.detail ? ( +
+ {approval.detail} +
+ ) : null} + {newerSubject ? ( +

+ {translate( + 'components.native-chat.approval.needsNewerOrca', + 'This request needs a newer version of Orca.' + )} +

+ ) : null} +
+ ) : null} +
+ {approval.options.map((opt, i) => ( + + ))} +
-
+ ) } diff --git a/src/renderer/src/components/settings/PluginConsentDialog.tsx b/src/renderer/src/components/settings/PluginConsentDialog.tsx index f0b39d05ba1..7b0eaca54b2 100644 --- a/src/renderer/src/components/settings/PluginConsentDialog.tsx +++ b/src/renderer/src/components/settings/PluginConsentDialog.tsx @@ -134,7 +134,9 @@ export function PluginConsentDialog({ }} > { // Why: dismissal is the safety-preserving path, so it receives initial focus. event.preventDefault() diff --git a/src/renderer/src/components/settings/PluginInstallDialog.tsx b/src/renderer/src/components/settings/PluginInstallDialog.tsx index 1b13bea59e1..b9786db960a 100644 --- a/src/renderer/src/components/settings/PluginInstallDialog.tsx +++ b/src/renderer/src/components/settings/PluginInstallDialog.tsx @@ -79,7 +79,7 @@ export function PluginInstallDialog({ return ( !installing && onOpenChange(nextOpen)}> - + {translate('auto.components.settings.PluginInstallDialog.title', 'Install plugin')} diff --git a/src/renderer/src/components/settings/PluginMarketplacePreviewDialog.tsx b/src/renderer/src/components/settings/PluginMarketplacePreviewDialog.tsx index 6f7e34fa657..cdc3c35bde4 100644 --- a/src/renderer/src/components/settings/PluginMarketplacePreviewDialog.tsx +++ b/src/renderer/src/components/settings/PluginMarketplacePreviewDialog.tsx @@ -152,7 +152,7 @@ export function PluginMarketplacePreviewDialog({ : undefined return ( !open && !busy && onClose()}> - + {preview ? ( <> diff --git a/src/renderer/src/components/settings/PluginMarketplaceSourceDialog.tsx b/src/renderer/src/components/settings/PluginMarketplaceSourceDialog.tsx index 89c901ff2a8..85391956270 100644 --- a/src/renderer/src/components/settings/PluginMarketplaceSourceDialog.tsx +++ b/src/renderer/src/components/settings/PluginMarketplaceSourceDialog.tsx @@ -121,7 +121,9 @@ export function PluginMarketplaceSourceDialog({ return ( !busyAction && onOpenChange(nextOpen)}> { event.preventDefault() urlRef.current?.focus() diff --git a/src/renderer/src/components/settings/PluginRemoveDialog.tsx b/src/renderer/src/components/settings/PluginRemoveDialog.tsx index 3cae9dd9aaf..f9829e66470 100644 --- a/src/renderer/src/components/settings/PluginRemoveDialog.tsx +++ b/src/renderer/src/components/settings/PluginRemoveDialog.tsx @@ -29,6 +29,7 @@ export function PluginRemoveDialog({ return ( !open && !busy && onCancel()}> { event.preventDefault() cancelRef.current?.focus() diff --git a/src/renderer/src/components/settings/PluginRollbackDialog.tsx b/src/renderer/src/components/settings/PluginRollbackDialog.tsx index f8693f4829e..21a03bf12ad 100644 --- a/src/renderer/src/components/settings/PluginRollbackDialog.tsx +++ b/src/renderer/src/components/settings/PluginRollbackDialog.tsx @@ -31,6 +31,7 @@ export function PluginRollbackDialog({ return ( !open && !busy && onCancel()}> { event.preventDefault() cancelRef.current?.focus() diff --git a/src/renderer/src/components/sidebar/OrcaYamlTrustDialog.tsx b/src/renderer/src/components/sidebar/OrcaYamlTrustDialog.tsx index 60ea9bcf90f..a14a8990f81 100644 --- a/src/renderer/src/components/sidebar/OrcaYamlTrustDialog.tsx +++ b/src/renderer/src/components/sidebar/OrcaYamlTrustDialog.tsx @@ -104,7 +104,7 @@ const OrcaYamlTrustDialog = React.memo(function OrcaYamlTrustDialog() { return ( - + {previouslyApproved diff --git a/src/renderer/src/components/ui/dialog.tsx b/src/renderer/src/components/ui/dialog.tsx index b19a1c769dc..2763406ee92 100644 --- a/src/renderer/src/components/ui/dialog.tsx +++ b/src/renderer/src/components/ui/dialog.tsx @@ -1,5 +1,6 @@ 'use client' +import { SECURITY_SURFACE_CLASS } from './security-surface' import * as React from 'react' import { cva, type VariantProps } from 'class-variance-authority' import { Minus, XIcon } from 'lucide-react' @@ -54,7 +55,9 @@ const dialogContentSurfaceVariants = cva('', { 'feature-tip': 'gap-0 bg-[color-mix(in_srgb,var(--foreground)_8%,var(--background))] p-0 dark:bg-[color-mix(in_srgb,var(--foreground)_16%,var(--background))]', /** Edge to edge, for an embedded panel that brings its own header and padding. */ - panel: 'gap-0 p-0' + panel: 'gap-0 p-0', + /** Trust, consent and provenance decisions: host-owned tokens a plugin theme cannot restyle. */ + security: SECURITY_SURFACE_CLASS } }, defaultVariants: { surface: 'default' } @@ -94,6 +97,7 @@ function DialogContent({ closeButtonVariant = 'close', surface, size, + scrollable = false, ...props }: React.ComponentProps & VariantProps & { @@ -102,6 +106,8 @@ function DialogContent({ closeButtonVariant?: 'close' | 'minimize' /** `compact` is a short confirmation's width, like the close prompts. */ size?: 'default' | 'compact' + /** The whole dialog scrolls within the viewport instead of growing past it. */ + scrollable?: boolean }) { return ( @@ -120,6 +126,7 @@ function DialogContent({ className={cn( 'fixed top-[50%] left-[50%] z-50 grid w-full max-w-[calc(100%-2rem)] grid-cols-[minmax(0,1fr)] translate-x-[-50%] translate-y-[-50%] gap-4 rounded-lg border border-black/14 bg-background/96 p-6 text-foreground shadow-[0_20px_60px_rgba(0,0,0,0.28),inset_0_1px_0_rgba(255,255,255,0.08)] backdrop-blur-2xl duration-200 outline-none dark:border-white/14 dark:bg-[rgba(23,23,23,0.96)] dark:shadow-[0_24px_72px_rgba(0,0,0,0.55),inset_0_1px_0_rgba(255,255,255,0.06)] data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=closed]:zoom-out-95 data-[state=open]:animate-in data-[state=open]:fade-in-0 data-[state=open]:zoom-in-95 sm:max-w-lg data-[size=compact]:sm:max-w-sm', dialogContentSurfaceVariants({ surface }), + scrollable && 'max-h-[calc(100vh-3rem)] overflow-y-auto scrollbar-sleek', className )} {...props} diff --git a/src/renderer/src/components/ui/security-surface.test.tsx b/src/renderer/src/components/ui/security-surface.test.tsx new file mode 100644 index 00000000000..9a6e8acc8ee --- /dev/null +++ b/src/renderer/src/components/ui/security-surface.test.tsx @@ -0,0 +1,61 @@ +// @vitest-environment happy-dom +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { cleanup, render } from '@testing-library/react' +import { afterEach, describe, expect, it } from 'vitest' +import { Dialog, DialogContent, DialogDescription, DialogTitle } from './dialog' +import { SecuritySurface } from './security-surface' + +// Every dialog that asks the user to trust, approve, run or remove code. A new one belongs here. +const TRUST_DIALOGS = [ + 'sidebar/OrcaYamlTrustDialog.tsx', + 'editor/IpynbViewer.tsx', + 'settings/PluginConsentDialog.tsx', + 'settings/PluginInstallDialog.tsx', + 'settings/PluginMarketplacePreviewDialog.tsx', + 'settings/PluginMarketplaceSourceDialog.tsx', + 'settings/PluginRemoveDialog.tsx', + 'settings/PluginRollbackDialog.tsx' +] + +// Trust decisions made outside a dialog: agent command approvals and certificate overrides. +const INLINE_TRUST_SURFACES = [ + 'native-chat/NativeChatApprovalCard.tsx', + 'browser-pane/navigate/browser-load-failure-overlay.tsx' +] + +afterEach(cleanup) + +describe('security surfaces', () => { + it('applies the host-owned token set', () => { + render( + + + Trust + Review + + + ) + expect(document.querySelector('[role="dialog"]')?.classList).toContain('plugin-security-chrome') + }) + + it.each(TRUST_DIALOGS)('%s renders on the security surface', (file) => { + const source = readFileSync(join(import.meta.dirname, '..', file), 'utf8') + expect(source).toMatch(/]*surface="security"/) + expect(source).not.toContain('plugin-security-chrome') + }) + + it('applies the host-owned token set outside dialogs', () => { + const { container } = render( + +
Allow
+ + ) + expect(container.firstElementChild?.classList).toContain('plugin-security-chrome') + }) + + it.each(INLINE_TRUST_SURFACES)('%s renders inside a security surface', (file) => { + const source = readFileSync(join(import.meta.dirname, '..', file), 'utf8') + expect(source).toMatch(/]/) + }) +}) diff --git a/src/renderer/src/components/ui/security-surface.tsx b/src/renderer/src/components/ui/security-surface.tsx new file mode 100644 index 00000000000..ccf2a741e09 --- /dev/null +++ b/src/renderer/src/components/ui/security-surface.tsx @@ -0,0 +1,15 @@ +import type React from 'react' +import * as Slot from 'radix-ui/slot' + +/** Host-owned tokens a plugin theme cannot restyle; see `.plugin-security-chrome` in main.css. */ +export const SECURITY_SURFACE_CLASS = 'plugin-security-chrome' + +/** Puts its one child element on the host-owned token surface. For trust decisions outside a + * dialog (an agent approval, a certificate override); dialogs use `surface="security"`. */ +export function SecuritySurface({ children }: { children: React.ReactElement }): React.JSX.Element { + return ( + + {children} + + ) +}