Harden agent SVG rendering, secret storage, and provider probes

.changeset/harden-provider-boundaries.md

@@ -0,0 +1,5 @@
+---
+"@open-codesign/desktop": patch
+---
+
+Harden desktop provider setup by sanitizing agent-supplied SVG choice icons, storing new API keys with Electron safeStorage when available, redacting encrypted secret rows from diagnostics, and requiring explicit opt-in before testing local or private-network provider URLs.

.github/workflows/ci.yml

@@ -32,7 +32,25 @@ jobs:
           cache: pnpm
 
       - name: Install
-        run: pnpm install --frozen-lockfile
+        run: env -u ELECTRON_SKIP_BINARY_DOWNLOAD -u ELECTRON_SKIP_DOWNLOAD pnpm install --frozen-lockfile --ignore-scripts=false
+
+      - name: Install Electron binary
+        run: |
+          env -u ELECTRON_SKIP_BINARY_DOWNLOAD -u ELECTRON_SKIP_DOWNLOAD pnpm -C apps/desktop exec node -e "
+          const { execFileSync } = require('node:child_process');
+          const path = require('node:path');
+          const electronDir = path.dirname(require.resolve('electron/package.json'));
+          execFileSync(process.execPath, [path.join(electronDir, 'install.js')], { stdio: 'inherit' });
+          "
+
+      - name: Verify Electron binary
+        run: |
+          env -u ELECTRON_SKIP_BINARY_DOWNLOAD -u ELECTRON_SKIP_DOWNLOAD pnpm -C apps/desktop exec node -e "
+          const fs = require('node:fs');
+          const electronPath = require('electron');
+          fs.accessSync(electronPath, fs.constants.X_OK);
+          console.log(electronPath);
+          "
 
       - name: Lint
         run: pnpm lint

apps/desktop/src/main/connection-ipc.test.ts

@@ -12,6 +12,7 @@ import {
   buildAuthHeadersForWire,
   CONNECTION_FETCH_TIMEOUT_MS,
   classifyHttpError,
+  classifyNetworkTarget,
   extractIds,
   extractModelIds,
   fetchWithTimeout,
@@ -1202,6 +1203,81 @@ describe('config:v1:test-endpoint response parsing', () => {
     }
   });
 
+  it('classifies private and metadata network targets', () => {
+    expect(classifyNetworkTarget('https://provider.example/v1')).toBe('public');
+    expect(classifyNetworkTarget('http://localhost:8317')).toBe('loopback');
+    expect(classifyNetworkTarget('http://127.0.0.1:8317')).toBe('loopback');
+    expect(classifyNetworkTarget('http://[::1]:8317')).toBe('loopback');
+    expect(classifyNetworkTarget('http://10.0.0.5:8080')).toBe('private');
+    expect(classifyNetworkTarget('http://172.16.4.5:8080')).toBe('private');
+    expect(classifyNetworkTarget('http://192.168.1.50:8080')).toBe('private');
+    expect(classifyNetworkTarget('http://169.254.1.10:8080')).toBe('link-local');
+    expect(classifyNetworkTarget('http://169.254.169.254/latest')).toBe('metadata');
+    expect(classifyNetworkTarget('http://metadata.google.internal')).toBe('metadata');
+  });
+
+  it('requires explicit confirmation for private endpoint probes', async () => {
+    const { restore } = installFakeFetch(() => {
+      throw new Error('fetch should not be called');
+    });
+    try {
+      await expect(
+        handleConfigV1TestEndpoint({
+          wire: 'openai-chat',
+          baseUrl: 'http://127.0.0.1:8317/v1',
+          apiKey: 'sk-test',
+        }),
+      ).resolves.toEqual({
+        ok: false,
+        error: 'private-network-confirmation-required',
+        message: 'Private or local network provider URLs require explicit confirmation.',
+      });
+    } finally {
+      restore();
+    }
+  });
+
+  it('allows private endpoint probes after explicit confirmation', async () => {
+    const { restore } = installFakeFetch(() => ({
+      status: 200,
+      body: { data: [{ id: 'local' }] },
+    }));
+    try {
+      await expect(
+        handleConfigV1TestEndpoint({
+          wire: 'openai-chat',
+          baseUrl: 'http://127.0.0.1:8317/v1',
+          apiKey: 'sk-test',
+          allowPrivateNetwork: true,
+        }),
+      ).resolves.toEqual({ ok: true, modelCount: 1, models: ['local'] });
+    } finally {
+      restore();
+    }
+  });
+
+  it('blocks metadata endpoint probes even with private-network confirmation', async () => {
+    const { restore } = installFakeFetch(() => {
+      throw new Error('fetch should not be called');
+    });
+    try {
+      await expect(
+        handleConfigV1TestEndpoint({
+          wire: 'openai-chat',
+          baseUrl: 'http://169.254.169.254/latest',
+          apiKey: 'sk-test',
+          allowPrivateNetwork: true,
+        }),
+      ).resolves.toEqual({
+        ok: false,
+        error: 'blocked-network-target',
+        message: 'Metadata service endpoints cannot be used as model provider base URLs.',
+      });
+    } finally {
+      restore();
+    }
+  });
+
   it('rejects malformed baseUrl before attempting fetch', async () => {
     const { restore } = installFakeFetch(() => {
       throw new Error('fetch should not be called');

apps/desktop/src/main/connection-ipc.ts

@@ -1,4 +1,5 @@
 import { createHash } from 'node:crypto';
+import { isIP } from 'node:net';
 import {
   BUILTIN_PROVIDERS,
   CodesignError,
@@ -41,7 +42,13 @@ interface ModelsListPayloadV1 {
 
 const CONNECTION_TEST_FIELDS = ['provider', 'apiKey', 'baseUrl'] as const;
 const MODELS_LIST_FIELDS = ['provider', 'apiKey', 'baseUrl'] as const;
-const TEST_ENDPOINT_FIELDS = ['wire', 'baseUrl', 'apiKey', 'httpHeaders'] as const;
+const TEST_ENDPOINT_FIELDS = [
+  'wire',
+  'baseUrl',
+  'apiKey',
+  'httpHeaders',
+  'allowPrivateNetwork',
+] as const;
 
 function assertKnownFields(
   record: Record<string, unknown>,
@@ -177,6 +184,62 @@ function parseHttpBaseUrl(value: unknown, field: string): string {
   return trimmed;
 }
 
+export type NetworkTargetClass = 'public' | 'loopback' | 'private' | 'link-local' | 'metadata';
+
+function ipv4ToNumber(ip: string): number | null {
+  const parts = ip.split('.').map((part) => Number(part));
+  if (
+    parts.length !== 4 ||
+    parts.some((part) => !Number.isInteger(part) || part < 0 || part > 255)
+  ) {
+    return null;
+  }
+  const [a, b, c, d] = parts as [number, number, number, number];
+  return ((a << 24) >>> 0) + (b << 16) + (c << 8) + d;
+}
+
+function inIpv4Range(ip: string, base: string, bits: number): boolean {
+  const value = ipv4ToNumber(ip);
+  const baseValue = ipv4ToNumber(base);
+  if (value === null || baseValue === null) return false;
+  const mask = bits === 0 ? 0 : (0xffffffff << (32 - bits)) >>> 0;
+  return (value & mask) === (baseValue & mask);
+}
+
+export function classifyNetworkTarget(rawBaseUrl: string): NetworkTargetClass {
+  let parsed: URL;
+  try {
+    parsed = new URL(rawBaseUrl);
+  } catch {
+    return 'public';
+  }
+  const host = parsed.hostname.replace(/^\[(.*)\]$/, '$1').toLowerCase();
+  if (
+    host === 'metadata.google.internal' ||
+    host === 'metadata' ||
+    host === '169.254.169.254' ||
+    host === 'fd00:ec2::254'
+  ) {
+    return 'metadata';
+  }
+  if (host === 'localhost') return 'loopback';
+  const family = isIP(host);
+  if (family === 4) {
+    if (inIpv4Range(host, '127.0.0.0', 8)) return 'loopback';
+    if (inIpv4Range(host, '10.0.0.0', 8)) return 'private';
+    if (inIpv4Range(host, '172.16.0.0', 12)) return 'private';
+    if (inIpv4Range(host, '192.168.0.0', 16)) return 'private';
+    if (inIpv4Range(host, '169.254.0.0', 16)) return 'link-local';
+    return 'public';
+  }
+  if (family === 6) {
+    if (host === '::1') return 'loopback';
+    if (host.startsWith('fe80:')) return 'link-local';
+    if (host.startsWith('fc') || host.startsWith('fd')) return 'private';
+  }
+  return 'public';
+}
+
 // ---------------------------------------------------------------------------
 // Models endpoint construction
 // ---------------------------------------------------------------------------
@@ -978,6 +1041,22 @@ export async function handleConfigV1TestEndpoint(raw: unknown): Promise<TestEndp
     };
   }
 
+  const targetClass = classifyNetworkTarget(payload.baseUrl);
+  if (targetClass === 'metadata') {
+    return {
+      ok: false,
+      error: 'blocked-network-target',
+      message: 'Metadata service endpoints cannot be used as model provider base URLs.',
+    };
+  }
+  if (targetClass !== 'public' && payload.allowPrivateNetwork !== true) {
+    return {
+      ok: false,
+      error: 'private-network-confirmation-required',
+      message: 'Private or local network provider URLs require explicit confirmation.',
+    };
+  }
+
   const { url } = buildEndpointForWire(payload.wire, payload.baseUrl);
   const headers = buildAuthHeadersForWire(
     payload.wire,
@@ -1142,6 +1221,7 @@ interface TestEndpointPayload {
   baseUrl: string;
   apiKey: string;
   httpHeaders?: Record<string, string>;
+  allowPrivateNetwork?: boolean;
 }
 
 export type TestEndpointResponse =
@@ -1172,6 +1252,12 @@ function parseTestEndpointPayload(raw: unknown): TestEndpointPayload {
     baseUrl: parseHttpBaseUrl(baseUrl, 'baseUrl'),
     apiKey: trimmedApiKey,
   };
+  if (r['allowPrivateNetwork'] !== undefined) {
+    if (typeof r['allowPrivateNetwork'] !== 'boolean') {
+      throw new CodesignError('allowPrivateNetwork must be a boolean', ERROR_CODES.IPC_BAD_INPUT);
+    }
+    out.allowPrivateNetwork = r['allowPrivateNetwork'];
+  }
   const headers = parseTestEndpointHttpHeaders(r['httpHeaders']);
   if (headers !== undefined) out.httpHeaders = headers;
   return out;

apps/desktop/src/main/diagnostics-ipc.test.ts

@@ -672,9 +672,9 @@ describe('redactSensitiveTomlFields', () => {
   it('masks the ciphertext field used by this codebase to persist secrets', () => {
     // Reproduces the real bundle leak a user reported on 2026-04-22: the
     // `[secrets.*] ciphertext = "..."` field was slipping through because
-    // it wasn't on the field allowlist. "plain:<value>" is the dev-mode
-    // pass-through encoding (see keychain.ts), so the raw token is right
-    // there in the exported zip.
+    // it wasn't on the field allowlist. Modern rows use safeStorage-backed
+    // `safe:<base64>`, but fallback and legacy config rows can still contain
+    // `plain:<value>`, so the raw token must never appear in the exported zip.
     const input = [
       '[secrets.claude-code-imported]',
       'ciphertext = "plain:another-your-anthropic-auth-token"',

apps/desktop/src/main/diagnostics/redact.ts

@@ -66,10 +66,10 @@ export function redactForIssueUrl(
 /** Mask the VALUE of any TOML line whose key looks sensitive, regardless of
  *  the value's format. Google (AIzaSy...), Azure base64, DeepSeek, and future
  *  bearer tokens all slip past format-based regexes. The `ciphertext` field
- *  is this codebase's specific storage slot for persisted secrets (safeStorage
- *  ciphertext, or in migrated/dev paths a literal plaintext token prefixed
- *  `plain:`) — redact unconditionally. `mask` is the user-visible display
- *  form and already pre-obscured, so it's intentionally NOT on this list. */
+ *  is this codebase's specific storage slot for persisted secrets (`safe:`
+ *  safeStorage ciphertext, legacy safeStorage rows, or fallback `plain:`
+ *  tokens) — redact unconditionally. `mask` is the user-visible display form
+ *  and already pre-obscured, so it's intentionally NOT on this list. */
 export function redactSensitiveTomlFields(s: string): string {
   return s.replace(
     /^(\s*(?:api_?key|token|bearer|secret|access_?token|refresh_?token|password|ciphertext|auth_?token|credential)\s*=\s*)"[^"]*"/gim,

apps/desktop/src/main/keychain.test.ts

@@ -1,23 +1,26 @@
 import { CodesignError, ERROR_CODES, hydrateConfig } from '@open-codesign/shared';
 import { describe, expect, it, vi } from 'vitest';
 
+const loggerMock = vi.hoisted(() => ({
+  warn: vi.fn(),
+  info: vi.fn(),
+  error: vi.fn(),
+}));
+
 vi.mock('./electron-runtime', () => ({
   safeStorage: {
     isEncryptionAvailable: vi.fn(() => true),
+    encryptString: vi.fn((s: string) => Buffer.from(`encrypted:${s}`, 'utf8')),
     decryptString: vi.fn(() => ''),
   },
 }));
 
 vi.mock('./logger', () => ({
-  getLogger: () => ({
-    warn: vi.fn(),
-    info: vi.fn(),
-    error: vi.fn(),
-  }),
+  getLogger: () => loggerMock,
 }));
 
 import { safeStorage } from './electron-runtime';
-import { decryptSecret, migrateSecrets } from './keychain';
+import { decryptSecret, encryptSecret, migrateSecrets } from './keychain';
 
 function expectKeychainEmpty(fn: () => unknown): void {
   try {
@@ -31,6 +34,29 @@ function expectKeychainEmpty(fn: () => unknown): void {
 }
 
 describe('decryptSecret', () => {
+  it('encrypts new secrets with safeStorage when available', () => {
+    const stored = encryptSecret('sk-test-secret');
+    expect(stored).toBe(`safe:${Buffer.from('encrypted:sk-test-secret').toString('base64')}`);
+  });
+
+  it('falls back to plaintext rows when encryption is unavailable', () => {
+    vi.mocked(safeStorage.isEncryptionAvailable).mockReturnValueOnce(false);
+    expect(encryptSecret('sk-test-secret')).toBe('plain:sk-test-secret');
+    expect(loggerMock.warn).toHaveBeenCalledWith(
+      'keychain.safeStorage.unavailable_plaintext_fallback',
+    );
+  });
+
+  it('reads existing plaintext secret rows', () => {
+    expect(decryptSecret('plain:sk-test-secret')).toBe('sk-test-secret');
+  });
+
+  it('reads new encrypted secret rows', () => {
+    vi.mocked(safeStorage.decryptString).mockReturnValueOnce('sk-test-secret');
+    const stored = `safe:${Buffer.from('ciphertext').toString('base64')}`;
+    expect(decryptSecret(stored)).toBe('sk-test-secret');
+  });
+
   it('rejects plaintext secret rows that decrypt to an empty string', () => {
     expectKeychainEmpty(() => decryptSecret('plain:'));
   });
@@ -41,6 +67,57 @@ describe('decryptSecret', () => {
 });
 
 describe('migrateSecrets', () => {
+  it('migrates plaintext rows to encrypted rows when safeStorage is available', () => {
+    const cfg = hydrateConfig({
+      version: 3,
+      activeProvider: 'openai',
+      activeModel: 'gpt-5.4',
+      providers: {
+        openai: {
+          id: 'openai',
+          name: 'OpenAI',
+          builtin: true,
+          wire: 'openai-chat',
+          baseUrl: 'https://api.openai.com/v1',
+          defaultModel: 'gpt-5.4',
+        },
+      },
+      secrets: { openai: { ciphertext: 'plain:sk-test-secret', mask: '' } },
+    });
+
+    const migrated = migrateSecrets(cfg);
+    expect(migrated.changed).toBe(true);
+    expect(migrated.config.secrets['openai']?.ciphertext).toBe(
+      `safe:${Buffer.from('encrypted:sk-test-secret').toString('base64')}`,
+    );
+    expect(migrated.config.secrets['openai']?.mask).toBe('sk-***cret');
+  });
+
+  it('keeps plaintext rows when safeStorage is unavailable', () => {
+    vi.mocked(safeStorage.isEncryptionAvailable).mockReturnValue(false);
+    const cfg = hydrateConfig({
+      version: 3,
+      activeProvider: 'openai',
+      activeModel: 'gpt-5.4',
+      providers: {
+        openai: {
+          id: 'openai',
+          name: 'OpenAI',
+          builtin: true,
+          wire: 'openai-chat',
+          baseUrl: 'https://api.openai.com/v1',
+          defaultModel: 'gpt-5.4',
+        },
+      },
+      secrets: { openai: { ciphertext: 'plain:sk-test-secret', mask: '' } },
+    });
+
+    const migrated = migrateSecrets(cfg);
+    expect(migrated.changed).toBe(true);
+    expect(migrated.config.secrets['openai']?.ciphertext).toBe('plain:sk-test-secret');
+    vi.mocked(safeStorage.isEncryptionAvailable).mockReturnValue(true);
+  });
+
   it('rejects legacy secret rows that decrypt to an empty string', () => {
     vi.mocked(safeStorage.decryptString).mockReturnValueOnce('');
     const cfg = hydrateConfig({