From cd85bdc19647ee31ab3942a1ada7adfaf7e0a17c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 4 Apr 2026 23:37:36 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20CSO=20security=20fixes=20=E2=80=94=20tok?= =?UTF-8?q?en=20leak,=20domain=20bypass,=20input=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Remove root token from /health endpoint entirely (CSO #1 CRITICAL). Origin header is spoofable. Extension reads from ~/.gstack/.auth.json. 2. Add domain check for newtab URL (CSO #5). Previously only goto was checked, allowing domain-restricted agents to bypass via newtab. 3. Validate scope values, rateLimit, expiresSeconds in createToken() (CSO #4). Rejects invalid scopes and negative values. Co-Authored-By: Claude Opus 4.6 (1M context) --- browse/src/server.ts | 19 ++++++++--- browse/src/token-registry.ts | 12 +++++++ browse/test/server-auth.test.ts | 27 +++++++++++---- browse/test/token-registry.test.ts | 55 ++++++++++++++++++++++++++++-- 4 files changed, 100 insertions(+), 13 deletions(-) diff --git a/browse/src/server.ts b/browse/src/server.ts index 00130f00..cb2688f6 100644 --- a/browse/src/server.ts +++ b/browse/src/server.ts @@ -893,6 +893,16 @@ async function handleCommand(body: any, tokenInfo?: TokenInfo | null): Promise= 0'); + if (expiresSeconds !== null && expiresSeconds !== undefined && expiresSeconds < 0) { + throw new Error('expiresSeconds must be >= 0 or null'); + } + const token = generateToken('gsk_sess_'); const now = new Date(); const expiresAt = expiresSeconds === null diff --git a/browse/test/server-auth.test.ts b/browse/test/server-auth.test.ts index 1bb45550..63495965 100644 --- a/browse/test/server-auth.test.ts +++ b/browse/test/server-auth.test.ts @@ -21,13 +21,28 @@ function sliceBetween(source: string, startMarker: string, endMarker: string): s } describe('Server auth security', () => { - // Test 1: /health serves auth token gated on chrome-extension:// Origin - // to prevent leaking when the server is tunneled to the internet. - test('/health serves auth token only for chrome extension origin', () => { + // Test 1: /health must NOT serve the auth token (CSO finding #1 — spoofable Origin) + // Extension reads token from ~/.gstack/.auth.json instead. + test('/health does NOT serve auth token', () => { const healthBlock = sliceBetween(SERVER_SRC, "url.pathname === '/health'", "url.pathname === '/connect'"); - expect(healthBlock).toContain('AUTH_TOKEN'); - // Must be gated on chrome-extension Origin - expect(healthBlock).toContain('chrome-extension://'); + // Token must not appear in the health response construction + expect(healthBlock).not.toContain('token: AUTH_TOKEN'); + expect(healthBlock).not.toContain('token: AUTH'); + // Should have a comment explaining why + expect(healthBlock).toContain('NOT served here'); + }); + + // Test 1b: /health must not use chrome-extension Origin gating (spoofable) + test('/health does not use spoofable Origin header for token gating', () => { + const healthBlock = sliceBetween(SERVER_SRC, "url.pathname === '/health'", "url.pathname === '/connect'"); + expect(healthBlock).not.toContain("chrome-extension://') ? { token"); + }); + + // Test 1c: newtab must check domain restrictions (CSO finding #5) + test('newtab enforces domain restrictions', () => { + const newtabBlock = sliceBetween(SERVER_SRC, "newtab with ownership for scoped tokens", "Block mutation commands while watching"); + expect(newtabBlock).toContain('checkDomain'); + expect(newtabBlock).toContain('Domain not allowed'); }); // Test 2: /refs endpoint requires auth via validateAuth diff --git a/browse/test/token-registry.test.ts b/browse/test/token-registry.test.ts index 7e004bc6..e272ea18 100644 --- a/browse/test/token-registry.test.ts +++ b/browse/test/token-registry.test.ts @@ -139,8 +139,11 @@ describe('token-registry', () => { expect(validateToken('gsk_sess_unknown')).toBeNull(); }); - it('rejects expired token', () => { - const created = createToken({ clientId: 'expiring', expiresSeconds: -1 }); + it('rejects expired token', async () => { + // expiresSeconds: 0 creates a token that expires at creation time + const created = createToken({ clientId: 'expiring', expiresSeconds: 0 }); + // Wait 1ms so the expiry is definitively in the past + await new Promise(r => setTimeout(r, 2)); expect(validateToken(created.token)).toBeNull(); }); }); @@ -345,4 +348,52 @@ describe('token-registry', () => { expect(SCOPE_ADMIN.has('screenshot')).toBe(false); }); }); + + // ─── CSO Fix #4: Input validation ────────────────────────────── + describe('Input validation (CSO finding #4)', () => { + it('rejects invalid scope values', () => { + expect(() => createToken({ + clientId: 'test-invalid-scope', + scopes: ['read', 'bogus' as any], + })).toThrow('Invalid scope: bogus'); + }); + + it('rejects negative rateLimit', () => { + expect(() => createToken({ + clientId: 'test-neg-rate', + rateLimit: -1, + })).toThrow('rateLimit must be >= 0'); + }); + + it('rejects negative expiresSeconds', () => { + expect(() => createToken({ + clientId: 'test-neg-expire', + expiresSeconds: -100, + })).toThrow('expiresSeconds must be >= 0 or null'); + }); + + it('accepts null expiresSeconds (indefinite)', () => { + const token = createToken({ + clientId: 'test-indefinite', + expiresSeconds: null, + }); + expect(token.expiresAt).toBeNull(); + }); + + it('accepts zero rateLimit (unlimited)', () => { + const token = createToken({ + clientId: 'test-unlimited-rate', + rateLimit: 0, + }); + expect(token.rateLimit).toBe(0); + }); + + it('accepts valid scopes', () => { + const token = createToken({ + clientId: 'test-valid-scopes', + scopes: ['read', 'write', 'admin', 'meta'], + }); + expect(token.scopes).toEqual(['read', 'write', 'admin', 'meta']); + }); + }); });