From f4e84b4dc911750e5f7baba03a11c86f2490b198 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sun, 16 Aug 2026 09:39:31 -0700 Subject: [PATCH] fix(browse): remove the dead security shield + unfed /health.security (re-derived from #2557) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sidebar's SEC shield has been dead UI since the PTY terminal rewrite: nothing set its data-status, nothing unhid it, and the /health.security field behind it read getStatus() off ~/.gstack/security/session-state.json — a file whose ONLY writer (sidebar-agent.ts) was deleted with the chat path. /health therefore reported a permanent 'inactive', or a stale FALSE-GREEN 'protected' wherever an old state file survived on disk (a single unit-test run was enough to plant one). A green shield sourced from leftover state reads as "no threats detected" when the real state is "not measured" — the same fail-open class as #2026. Removed (dead surfaces only): the shield markup/CSS and the stale sidepanel.js comment; the /health security field and server.ts's getStatus import; getStatus / SecurityStatus / StatusDetail / SessionState / read+writeSessionState (and security.ts's dead child_process import); the session-state + getStatus unit tests — including the round-trip test that wrote real fixture data into ~/.gstack and left /health green forever. (The PR's security-sidepanel-dom.test.ts deletion already happened on main via #2230; its resolveDisconnectCause guard landed via the #2085 typeof fix. Neither re-applied.) Kept, per ENG-OV9 — security.ts has LIVE consumers: the pure combiner (combineVerdict + THRESHOLDS), canary utilities, and extractDomain stay; server.ts's /pty-inject-scan L4 path (isSidecarAvailable + scanWithSidecar) is untouched. browse/test/server-security-surface.test.ts pins BOTH directions: the dead surface stays dead (no /health security field, no getStatus import, no reader of the security session-state file, shield markup gone) and the live half stays live (sidecar wiring in server.ts, combiner/canary exports in security.ts, /health carries no token — the v1.63 regression wall). A future re-feed from LIVE signals must update that test deliberately rather than resurrect the state-file path. F13 (same commit): CLAUDE.md's Sidebar security stack section, ARCHITECTURE.md's prompt-injection Visibility + critical-constraint paragraphs, and BROWSER.md's security section now describe the removed surfaces as history, not live features. Net -166 lines. Tests: server-security-surface + security + security-adversarial(+fixes) + security-integration + server-auth 114 pass; sidepanel-* + extension-token + extension-sender-auth 58 pass / 2 skip. Re-derived from PR #2557 by @frederik-kaster-noygear. Co-authored-by: Frederik Kaster Co-Authored-By: Claude Fable 5 --- ARCHITECTURE.md | 4 +- BROWSER.md | 7 +- CLAUDE.md | 13 ++- browse/src/security.ts | 107 +++----------------- browse/src/server.ts | 12 ++- browse/test/security.test.ts | 64 ++---------- browse/test/server-security-surface.test.ts | 86 ++++++++++++++++ extension/sidepanel.css | 42 +------- extension/sidepanel.html | 10 -- extension/sidepanel.js | 3 - 10 files changed, 134 insertions(+), 214 deletions(-) create mode 100644 browse/test/server-security-surface.test.ts diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1856f0ad7..6c2d6c74f 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -184,11 +184,11 @@ The Chrome sidebar agent has tools (Bash, Read, Glob, Grep, WebFetch) and reads 5. **L6 ensemble combiner (`combineVerdict`).** BLOCK requires agreement from two ML classifiers at >= `WARN` (0.75), not a single confident hit. This is the Stack Overflow instruction-writing false-positive mitigation. On tool-output scans, single-layer high confidence BLOCKs directly — the content wasn't user-authored, so the FP concern doesn't apply. -**Critical constraint:** `security-classifier.ts` runs only in the sidebar-agent process, never in the compiled browse binary. `@huggingface/transformers` v4 requires `onnxruntime-node`, which fails `dlopen` from Bun compile's temp extract directory. Only the pure-string pieces (canary inject/check, verdict combiner, attack log, status) are in `security.ts`, which is safe to import from `server.ts`. +**Critical constraint:** `security-classifier.ts` runs only in the security sidecar subprocess (`security-sidecar-entry.ts`), never in the compiled browse binary. `@huggingface/transformers` v4 requires `onnxruntime-node`, which fails `dlopen` from Bun compile's temp extract directory. Only the pure-string pieces (canary inject/check, verdict combiner) are in `security.ts`, which is safe to import from `server.ts`. (The attack log lives in `tunnel-denial-log.ts`; the session-state/status surface was removed in #2557.) **Env knobs:** `GSTACK_SECURITY_OFF=1` is a real kill switch (skips ML scan, canary still injects). Model cache at `~/.gstack/models/testsavant-small/` (112MB, first run) and `~/.gstack/models/deberta-v3-injection/` (721MB, opt-in only). Attack log at `~/.gstack/security/attempts.jsonl` (salted sha256 + domain, rotates at 10MB, 5 generations). Per-device salt at `~/.gstack/security/device-salt` (0600), cached in-process to survive FS-unwritable environments. -**Visibility.** The sidebar header shows a shield icon (green/amber/red) polled via `/sidebar-chat`. A centered banner appears on canary leak or BLOCK verdict with the exact layer scores. `bin/gstack-security-dashboard` aggregates local attempts; `supabase/functions/community-pulse` aggregates opt-in community telemetry across users. +**Visibility.** A centered banner appears on canary leak or BLOCK verdict with the exact layer scores. `bin/gstack-security-dashboard` aggregates local attempts; `supabase/functions/community-pulse` aggregates opt-in community telemetry across users. (The sidebar header's SEC shield icon and the `/health` `security` field were removed in #2557: their only data source — `~/.gstack/security/session-state.json` — lost its only writer when the chat-path agent was ripped, so the shield reported stale or empty state. The live defenses report through their own call sites.) ## The ref system diff --git a/BROWSER.md b/BROWSER.md index 1ab7a1e60..af6c92611 100644 --- a/BROWSER.md +++ b/BROWSER.md @@ -880,10 +880,11 @@ sidebar chat pipeline that hosted them. **Canary leak always BLOCKs - Attack log: `~/.gstack/security/attempts.jsonl` (salted SHA-256 + domain only, rotates at 10MB, 5 generations). - Per-device salt: `~/.gstack/security/device-salt` (0600). -- Session state: `~/.gstack/security/session-state.json` (cross-process, - atomic). -A shield icon in the sidebar header shows the live status. See +There is no security status indicator in the sidebar and no `security` +field on `/health` (#2557): the session-state file that fed them lost its +only writer when the chat-path agent was removed, so they reported stale or +empty data. The live defenses report through their own call sites. See ARCHITECTURE.md § "Prompt injection defense" for the full threat model. --- diff --git a/CLAUDE.md b/CLAUDE.md index ac661a717..852162cda 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -429,9 +429,16 @@ leak always BLOCKs (deterministic). - Classifier model cache: `~/.gstack/models/testsavant-small/` (112MB, first run only) - Attack log: `~/.gstack/security/attempts.jsonl` — written by `tunnel-denial-log.ts` (tunnel-surface rejections; rotates at 10MB, 5 generations) -- Session state: `~/.gstack/security/session-state.json` (cross-process, atomic; - NOTE: classifierStatus currently has no live writer — shield status derives - from what's on disk) + +History note (#2557): the cross-process session state +(`~/.gstack/security/session-state.json`), `getStatus()`, the `/health` +`security` field, and the sidepanel SEC shield were all removed — the state +file lost its only writer when sidebar-agent.ts was ripped, so the shield +reported a permanent 'inactive' or a stale false-green 'protected' from +leftover disk state. The live defenses (L1-L3 filters, L4 sidecar on the +inject-scan path) report through their own call sites, never through +/health. `browse/test/server-security-surface.test.ts` pins both the +removal and the live L4 wiring. Do not re-document these as live. ## Dev symlink awareness diff --git a/browse/src/security.ts b/browse/src/security.ts index 6475aea73..3da662a00 100644 --- a/browse/src/security.ts +++ b/browse/src/security.ts @@ -23,18 +23,19 @@ * host process): the combiner is pure and tested, and server.ts's * inline L4 path is the consumer of record. * - * Cross-process state lives at ~/.gstack/security/session-state.json. - * classifierStatus in that state has no live writer since the chat-path rip - * (the sidecar reports status over its own NDJSON protocol instead). + * There is no longer any cross-process session state (#2557). + * ~/.gstack/security/session-state.json existed to carry classifier status + * across the server.ts / sidebar-agent.ts boundary; sidebar-agent.ts went + * away with the PTY terminal rewrite, leaving nothing to write the file and + * a /health.security status that reported stale or empty data — a permanent + * 'inactive', or a false-green 'protected' wherever an old state file + * survived on disk. getStatus / SessionState / read+writeSessionState and + * the /health field were removed together. Per-tab decision files under + * ~/.gstack/security/decisions/ are unaffected, and the L4 sidecar reports + * status over its own NDJSON protocol (security-sidecar-client.ts). */ -import { randomBytes, createHash } from 'crypto'; -import { spawn } from 'child_process'; -import * as fs from 'fs'; -import * as path from 'path'; -import * as os from 'os'; -import { restrictFilePermissions, appendSecureFile, mkdirSecure } from './file-permissions'; -import { atomicWriteQuiet } from '../../lib/fs-atomic'; +import { randomBytes } from 'crypto'; // ─── Thresholds + verdict types ────────────────────────────── @@ -83,17 +84,6 @@ export interface SecurityResult { confidence: number; } -export type SecurityStatus = 'protected' | 'degraded' | 'inactive'; - -export interface StatusDetail { - status: SecurityStatus; - layers: { - testsavant: 'ok' | 'degraded' | 'off'; - canary: 'ok' | 'off'; - }; - lastUpdated: string; -} - // ─── Verdict combiner (ensemble rule, label-first for transcript) ──── /** @@ -322,79 +312,8 @@ export function checkCanaryInStructure(value: unknown, canary: string): boolean // attempts.jsonl rotation + telemetry spawn plumbing) lived here until the // chat-path scanner that called it was ripped with sidebar-agent.ts. The // LIVE attempts.jsonl writer is tunnel-denial-log.ts, which owns its own -// rotation. - -const SECURITY_DIR = path.join(os.homedir(), '.gstack', 'security'); - -// ─── Cross-process session state ───────────────────────────── - -const STATE_FILE = path.join(SECURITY_DIR, 'session-state.json'); - -/** - * SessionState is a DISK FORMAT (~/.gstack/security/session-state.json). - * Old files may carry a `transcript` field inside classifierStatus from the - * removed Haiku layer — readSessionState tolerates it (JSON.parse keeps the - * extra key; getStatus ignores it), but we never write it. - */ -export interface SessionState { - sessionId: string; - canary: string; - warnedDomains: string[]; // per-session rate limit for special telemetry - classifierStatus: { - testsavant: 'ok' | 'degraded' | 'off'; - }; - lastUpdated: string; -} - -/** - * Atomic write of session state (via lib/fs-atomic). Writes are safe - * across process boundaries. Swallow-with-log polarity: a failed write - * must never take down the caller (security state is best-effort cache). - */ -export function writeSessionState(state: SessionState): void { - try { mkdirSecure(SECURITY_DIR); } catch { /* write below fails and logs */ } - if (atomicWriteQuiet(STATE_FILE, JSON.stringify(state, null, 2), { mode: 0o600 })) { - // Windows ACL hardening (POSIX chmod is redundant with mode above). - restrictFilePermissions(STATE_FILE); - } else { - console.error('[security] writeSessionState failed'); - } -} - -export function readSessionState(): SessionState | null { - try { - if (!fs.existsSync(STATE_FILE)) return null; - return JSON.parse(fs.readFileSync(STATE_FILE, 'utf8')); - } catch { - return null; - } -} - -// ─── Status reporting (for shield icon via /health) ────────── - -export function getStatus(): StatusDetail { - const state = readSessionState(); - // Read the field explicitly (never spread classifierStatus): old on-disk - // state may carry a stale `transcript` key from the removed Haiku layer, - // and spreading would leak it into the /health payload. - const testsavant = state?.classifierStatus?.testsavant ?? 'off'; - const canary = state?.canary ? 'ok' : 'off'; - - let status: SecurityStatus; - if (testsavant === 'ok' && canary === 'ok') { - status = 'protected'; - } else if (testsavant === 'off' && canary === 'off') { - status = 'inactive'; - } else { - status = 'degraded'; - } - - return { - status, - layers: { testsavant, canary: canary as 'ok' | 'off' }, - lastUpdated: state?.lastUpdated ?? new Date().toISOString(), - }; -} +// rotation. The cross-process session state + getStatus shield feed went +// the same way (#2557) — see the module header. /** * Extract url domain for logging. Never logs path or query string. diff --git a/browse/src/server.ts b/browse/src/server.ts index 241ceab0c..9d42ff71d 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -24,7 +24,6 @@ import { runContentFilters, type ContentFilterResult, markHiddenElements, getCleanTextWithStripping, cleanupHiddenMarkers, } from './content-security'; -import { getStatus as getSecurityStatus } from './security'; import { isSidecarAvailable, scanWithSidecar } from './security-sidecar-client'; import { writeSecureFile, mkdirSecure, appendSecureFile } from './file-permissions'; import { handleSnapshot, SNAPSHOT_FLAGS } from './snapshot'; @@ -1899,10 +1898,13 @@ export function buildFetchHandler(cfg: ServerConfig): ServerHandle { mode: browserManager.getConnectionMode(), uptime: Math.floor((Date.now() - startTime) / 1000), tabs: browserManager.getTabCount(), - // Security module status — drives the shield icon in the sidepanel. - // Returns {status: 'protected'|'degraded'|'inactive', layers: {...}}. - // Fed by the page-content side (testsavant sidecar, canary state). - security: getSecurityStatus(), + // No `security` field (#2557): the only writer of the status it + // reported (sidebar-agent.ts's session-state file) went away with + // the chat path, so it read from a file nothing wrote — reporting + // a permanent 'inactive', or a stale false-green 'protected' + // wherever an old state file survived on disk. The live defenses + // (content-security L1-L3, the L4 sidecar on /pty-inject-scan) + // report through their own call sites, not through /health. // Terminal-agent discovery. ONLY a port number — never a token. // Tokens flow via the /pty-session HttpOnly cookie path. See // `pty-session-cookie.ts` for the rationale (codex outside-voice diff --git a/browse/test/security.test.ts b/browse/test/security.test.ts index ac1642e4f..d49d5ed0b 100644 --- a/browse/test/security.test.ts +++ b/browse/test/security.test.ts @@ -10,18 +10,12 @@ */ import { describe, test, expect } from 'bun:test'; -import * as fs from 'fs'; -import * as os from 'os'; -import * as path from 'path'; import { THRESHOLDS, combineVerdict, generateCanary, injectCanary, checkCanaryInStructure, - writeSessionState, - readSessionState, - getStatus, extractDomain, type LayerSignal, } from '../src/security'; @@ -244,57 +238,13 @@ describe('canary', () => { // ─── Attack log + rotation ─────────────────────────────────── -// ─── Session state (cross-process, atomic) ─────────────────── - -describe('session state', () => { - test('write + read round-trip', () => { - const state = { - sessionId: 'test-session-123', - canary: 'CANARY-TEST', - warnedDomains: ['example.com'], - classifierStatus: { testsavant: 'ok' as const }, - lastUpdated: '2026-04-19T12:34:56Z', - }; - writeSessionState(state); - const got = readSessionState(); - expect(got).not.toBeNull(); - expect(got!.sessionId).toBe('test-session-123'); - expect(got!.canary).toBe('CANARY-TEST'); - expect(got!.warnedDomains).toEqual(['example.com']); - }); - - test('tolerates stale transcript field from pre-rip on-disk state', () => { - // SessionState is a disk format. Files written before the Haiku - // transcript layer was removed carry classifierStatus.transcript — - // getStatus must read them fine, not require transcript for - // 'protected', and never leak the stale key into /health. - const stateFile = path.join(os.homedir(), '.gstack', 'security', 'session-state.json'); - fs.mkdirSync(path.dirname(stateFile), { recursive: true }); - fs.writeFileSync(stateFile, JSON.stringify({ - sessionId: 'legacy-session', - canary: 'CANARY-LEGACY', - warnedDomains: [], - classifierStatus: { testsavant: 'ok', transcript: 'degraded' }, - lastUpdated: '2026-04-19T12:34:56Z', - })); - const s = getStatus(); - expect(s.status).toBe('protected'); - expect('transcript' in s.layers).toBe(false); - }); -}); - -// ─── Status reporting for shield icon ──────────────────────── - -describe('getStatus', () => { - test('returns a valid SecurityStatus shape', () => { - const s = getStatus(); - expect(['protected', 'degraded', 'inactive']).toContain(s.status); - expect(s.layers).toBeDefined(); - expect(['ok', 'degraded', 'off']).toContain(s.layers.testsavant); - expect(['ok', 'off']).toContain(s.layers.canary); - expect(s.lastUpdated).toBeTruthy(); - }); -}); +// NOTE (#2557): the session-state + getStatus tests that lived here wrote +// REAL fixture data into ~/.gstack/security/session-state.json — after which +// /health reported a false-green 'protected' indefinitely. The surfaces they +// covered (SessionState, read/writeSessionState, getStatus, the /health +// security field, the sidepanel SEC shield) were dead since the PTY terminal +// rewrite and are now removed. server-security-surface.test.ts pins the +// removal + the live L4 wiring. // ─── URL domain extraction ─────────────────────────────────── diff --git a/browse/test/server-security-surface.test.ts b/browse/test/server-security-surface.test.ts new file mode 100644 index 000000000..cdfb76c96 --- /dev/null +++ b/browse/test/server-security-surface.test.ts @@ -0,0 +1,86 @@ +/** + * #2557 / ENG-OV9: pins the dead-shield removal AND the live L4 wiring. + * + * The removed surface: /health's `security` field read getStatus(), whose + * only data source (~/.gstack/security/session-state.json) lost its only + * writer when sidebar-agent.ts was ripped — so /health reported a permanent + * 'inactive' or, wherever an old state file survived, a stale FALSE-GREEN + * 'protected' ("no threats detected" when the real state was "not + * measured"). Same fail-open class as #2026. + * + * The kept surface (ENG-OV9): security.ts is NOT dead — server.ts's + * /pty-inject-scan path is the live L4 consumer (sidecar scan + URL + * blocklist + datamark envelope), and security.ts's pure combiner/canary + * exports stay. This test pins both directions so a future "cleanup" can't + * silently take the live half, and a future re-feed of /health.security + * from LIVE signals (isSidecarAvailable, content filters) must update this + * test deliberately rather than resurrect the state-file path. + * + * Source-level, same style as windows-spawn-hide.test.ts. + */ + +import { describe, expect, test } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; + +const SRC = (f: string) => fs.readFileSync(path.join(import.meta.dir, '../src', f), 'utf-8'); + +describe('#2557: dead shield surface stays dead', () => { + test('/health carries no security field and server.ts does not import getStatus', () => { + const server = SRC('server.ts'); + expect(server).not.toMatch(/security:\s*getSecurityStatus\(\)/); + expect(server).not.toMatch(/getStatus as getSecurityStatus/); + // The SECURITY session-state file must not be read anywhere in src/ — + // that file has no writer, so any reader is a false-signal feed. + // (session-persist.ts's per-project /session-state.json is a + // different, live file — only the ~/.gstack/security/ one is dead.) + for (const f of fs.readdirSync(path.join(import.meta.dir, '../src')).filter((x) => x.endsWith('.ts'))) { + const code = SRC(f).replace(/\/\*[\s\S]*?\*\//g, '').replace(/^\s*\/\/.*$/gm, '').replace(/^\s*\*.*$/gm, ''); + const refs = /security[/'",\s][^\n]{0,80}session-state\.json/.test(code); + expect({ file: f, refs }).toEqual({ file: f, refs: false }); + } + }); + + test('security.ts no longer exports the unfed status surface', () => { + const security = SRC('security.ts'); + expect(security).not.toMatch(/export function getStatus/); + expect(security).not.toMatch(/export function (read|write)SessionState/); + expect(security).not.toMatch(/export interface SessionState/); + expect(security).not.toMatch(/export interface StatusDetail/); + }); + + test('the sidepanel shield markup is gone', () => { + const html = fs.readFileSync(path.join(import.meta.dir, '../../extension/sidepanel.html'), 'utf-8'); + const css = fs.readFileSync(path.join(import.meta.dir, '../../extension/sidepanel.css'), 'utf-8'); + expect(html).not.toContain('security-shield'); + expect(css).not.toMatch(/\.security-shield\s*\{/); + }); +}); + +describe('ENG-OV9: the LIVE L4 path is untouched', () => { + test('server.ts still consumes the sidecar on the inject-scan path', () => { + const server = SRC('server.ts'); + expect(server).toContain("from './security-sidecar-client'"); + expect(server).toMatch(/isSidecarAvailable/); + expect(server).toMatch(/scanWithSidecar\(/); + }); + + test('security.ts keeps the pure combiner + canary exports', () => { + const security = SRC('security.ts'); + expect(security).toMatch(/export const THRESHOLDS/); + expect(security).toMatch(/export function combineVerdict/); + expect(security).toMatch(/export function generateCanary/); + expect(security).toMatch(/export function injectCanary/); + expect(security).toMatch(/export function checkCanaryInStructure/); + expect(security).toMatch(/export function extractDomain/); + }); + + test('/health stays liveness-only: no token in any mode (regression wall from v1.63)', () => { + const server = SRC('server.ts'); + // The /health handler block must not interpolate a token. + const healthIdx = server.indexOf("url.pathname === '/health'"); + expect(healthIdx).toBeGreaterThan(0); + const healthBlock = server.slice(healthIdx, healthIdx + 1500); + expect(healthBlock).not.toMatch(/token:\s*[^n]/i); + }); +}); diff --git a/extension/sidepanel.css b/extension/sidepanel.css index 0bc306b25..debabc83b 100644 --- a/extension/sidepanel.css +++ b/extension/sidepanel.css @@ -47,39 +47,6 @@ --radius-full: 9999px; } -/* ─── Security Shield ───────────────────────────────────────────── */ -/* 3 states — green=protected, amber=degraded, red=inactive. - Custom SVG outline + "SEC" label in JetBrains Mono to match the - industrial/CLI aesthetic (design review Pass 7 decision). */ - -.security-shield { - position: absolute; - top: 6px; - right: 8px; - z-index: 10; - display: inline-flex; - align-items: center; - gap: 4px; - padding: 2px 6px; - border-radius: var(--radius-sm, 4px); - font-family: var(--font-mono, 'JetBrains Mono', monospace); - font-size: 10px; - font-weight: 500; - letter-spacing: 0.04em; - background: rgba(255, 255, 255, 0.02); - transition: color 200ms ease-out, background 200ms ease-out; - cursor: default; -} -.security-shield[data-status="protected"] { - color: var(--success, #22C55E); -} -.security-shield[data-status="degraded"] { - color: var(--amber-400, #FBBF24); -} -.security-shield[data-status="inactive"] { - color: var(--error, #EF4444); -} - /* ─── Connection Banner ─────────────────────────────────────────── */ .conn-banner { @@ -129,10 +96,11 @@ .security-banner { position: relative; - /* Sit above the absolutely-positioned security-shield (z-index: 10) so - the banner's close button and controls receive clicks. Without this - the shield at top-right overlaps the banner's close X region and - intercepts pointer events. */ + /* Kept above other absolutely-positioned chrome so the banner's close + button and controls receive clicks. This originally guarded against + the top-right security shield (z-index: 10) overlapping the close X + and intercepting pointer events; that shield has since been removed + (#2557 — dead UI since the PTY terminal rewrite). */ z-index: 20; padding: 20px 16px; text-align: center; diff --git a/extension/sidepanel.html b/extension/sidepanel.html index b2ce8a1b5..105586c52 100644 --- a/extension/sidepanel.html +++ b/extension/sidepanel.html @@ -6,16 +6,6 @@ - - -