feat(wave4): trust — tests for high-risk paths, relay hardening, credential hygiene

SECURITY.md

@@ -26,7 +26,7 @@ devcompass is a browser-only single-page app. There is no backend that we operat
 | Surface | What is stored | Where |
 | --- | --- | --- |
 | Login | Raw PAT, ASCII-sanitized | `localStorage["devcompass.pat"]` |
-| Cache panel | Token reference key only | IndexedDB `devcompass.tokens` (id `current`) |
+| Cache panel | Token-keyed cache rows (masked in the UI) | IndexedDB `devcompass.prefs` |
 | Network | `Authorization: bearer <token>` | Request header to `api.github.com`, never to a server we operate |
 
 You can wipe everything from **Settings → Storage → Clear all cache**, or by deleting the `devcompass.pat` key and `devcompass` IndexedDB database from your browser.

api/_relay.test.ts

@@ -0,0 +1,81 @@
+import { afterEach, describe, expect, it, vi } from 'vitest'
+import { isAllowedTarget, relay } from './_relay'
+
+function jsonResponse(body: unknown, init?: { status?: number; headers?: Record<string, string> }) {
+  return new Response(JSON.stringify(body), {
+    status: init?.status ?? 200,
+    headers: { 'content-type': 'application/json', ...init?.headers },
+  })
+}
+
+describe('isAllowedTarget', () => {
+  it('allows sentry.io and regional hosts over https', () => {
+    expect(isAllowedTarget('https://sentry.io/api/0/organizations/')).not.toBeNull()
+    expect(isAllowedTarget('https://us.sentry.io/api/0/issues/1/')).not.toBeNull()
+    expect(isAllowedTarget('https://de.sentry.io/api/0/projects/')).not.toBeNull()
+  })
+
+  it('rejects non-allowlisted hosts, lookalikes and http', () => {
+    expect(isAllowedTarget('https://evil.com/api')).toBeNull()
+    expect(isAllowedTarget('https://sentry.io.evil.com/api')).toBeNull()
+    expect(isAllowedTarget('https://notsentry.io/api')).toBeNull()
+    expect(isAllowedTarget('http://sentry.io/api')).toBeNull()
+    expect(isAllowedTarget('not a url')).toBeNull()
+  })
+})
+
+describe('relay', () => {
+  afterEach(() => vi.unstubAllGlobals())
+
+  it('rejects non-allowlisted targets without calling fetch', async () => {
+    const fetchSpy = vi.fn()
+    vi.stubGlobal('fetch', fetchSpy)
+    const res = await relay({ method: 'GET', targetUrl: 'https://evil.com/x', headers: {} })
+    expect(res.status).toBe(400)
+    expect(fetchSpy).not.toHaveBeenCalled()
+  })
+
+  it('rejects disallowed methods', async () => {
+    const fetchSpy = vi.fn()
+    vi.stubGlobal('fetch', fetchSpy)
+    const res = await relay({ method: 'TRACE', targetUrl: 'https://sentry.io/api/0/', headers: {} })
+    expect(res.status).toBe(405)
+    expect(fetchSpy).not.toHaveBeenCalled()
+  })
+
+  it('forwards only the auth/content/accept headers', async () => {
+    const fetchSpy = vi.fn(async () => jsonResponse({ ok: true }))
+    vi.stubGlobal('fetch', fetchSpy)
+    await relay({
+      method: 'GET',
+      targetUrl: 'https://sentry.io/api/0/organizations/',
+      headers: { authorization: 'Bearer tok', cookie: 'session=1', 'x-forwarded-for': '1.2.3.4', accept: 'application/json' },
+    })
+    const sent = fetchSpy.mock.calls[0]![1]!.headers as Headers
+    expect(sent.get('authorization')).toBe('Bearer tok')
+    expect(sent.get('accept')).toBe('application/json')
+    expect(sent.get('cookie')).toBeNull()
+    expect(sent.get('x-forwarded-for')).toBeNull()
+  })
+
+  it('strips set-cookie from the relayed response and exposes link', async () => {
+    vi.stubGlobal('fetch', vi.fn(async () => jsonResponse([], {
+      headers: { 'set-cookie': 'sid=abc', link: '<https://sentry.io/x>; rel="next"; results="true"; cursor="0:100:0"' },
+    })))
+    const res = await relay({ method: 'GET', targetUrl: 'https://sentry.io/api/0/issues/', headers: {} })
+    expect(res.status).toBe(200)
+    const keys = Object.keys(res.headers).map((k) => k.toLowerCase())
+    expect(keys).not.toContain('set-cookie')
+    expect(res.headers['link']).toContain('cursor="0:100:0"')
+    expect(res.headers['access-control-expose-headers']).toContain('link')
+  })
+
+  it('refuses oversized responses (declared content-length)', async () => {
+    vi.stubGlobal('fetch', vi.fn(async () => new Response('x', {
+      status: 200,
+      headers: { 'content-length': String(100 * 1024 * 1024) },
+    })))
+    const res = await relay({ method: 'GET', targetUrl: 'https://sentry.io/api/0/issues/', headers: {} })
+    expect(res.status).toBe(502)
+  })
+})

api/_relay.ts

@@ -69,9 +69,18 @@ function errorResult(status: number, message: string): RelayResult {
   }
 }
 
+/** Methods the connectors actually use — everything else is rejected. */
+const ALLOWED_METHODS = new Set(['GET', 'POST', 'PUT', 'PATCH', 'DELETE'])
+
+/** Refuse to relay bodies past this size — protects the browser from a hostile upstream. */
+const MAX_RESPONSE_BYTES = 20 * 1024 * 1024 // 20 MB
+
 export async function relay(input: RelayInput): Promise<RelayResult> {
   const url = isAllowedTarget(input.targetUrl)
   if (!url) return errorResult(400, 'target not allowed (must be https + allowlisted host)')
+  if (!ALLOWED_METHODS.has(input.method.toUpperCase())) {
+    return errorResult(405, `method ${input.method} not allowed`)
+  }
 
   const fwd = new Headers()
   for (const h of FORWARD_REQUEST_HEADERS) {
@@ -101,5 +110,15 @@ export async function relay(input: RelayInput): Promise<RelayResult> {
   // browser fetch can read the cursor.
   headers['access-control-expose-headers'] = 'link, x-hits, x-max-hits'
 
-  return { status: upstream.status, headers, body: await upstream.arrayBuffer() }
+  // Size cap: trust content-length when present; otherwise verify after reading.
+  const declared = parseInt(upstream.headers.get('content-length') ?? '', 10)
+  if (Number.isFinite(declared) && declared > MAX_RESPONSE_BYTES) {
+    return errorResult(502, `upstream response too large (${declared} bytes)`)
+  }
+  const body = await upstream.arrayBuffer()
+  if (body.byteLength > MAX_RESPONSE_BYTES) {
+    return errorResult(502, `upstream response too large (${body.byteLength} bytes)`)
+  }
+
+  return { status: upstream.status, headers, body }
 }

docs/TASK_TODO.md

@@ -26,14 +26,14 @@ Wave numbering comes from the 2026-06 product audit (architecture + UX + perf/se
 - [x] **Skeleton states** — shared ScopeSkeleton replaces raw "Loading…" in Issues/Notifications.
 - [x] **Text filter for long lists** — title/repo filter input in Issues + Notifications (shown past 8 items).
 
-## Pending — Wave 4 "trust before scale" (robustness)
+## In progress — Wave 4 "trust before scale" (robustness)
 
-- [ ] **Tests for high-risk modules** — api/_relay allowlist, sentry client (cursor/retry/env-normalize), useUnifiedIssues merge, utils/time boundaries, agentPrompt builders.
-- [ ] **Sanitize Sentry token on store** — sentryConfig should run sanitizeToken like the PAT does.
-- [ ] **db.tokens stores the token string** — keep only metadata (scopes/expiry).
-- [ ] **Relay hardening round 2** — response size cap, HTTP method restriction.
-- [ ] **Timer cleanup** — 4 setTimeout without unmount cleanup (SinceScope, DetailModalHeader, Checks, DetailModal status).
-- [ ] **API boundary validation** — gql/rest/sentryFetch all cast `as T` with no runtime shape check.
+- [x] **Tests for high-risk modules** — 29 tests across 7 files: relay allowlist/forwarding/size-cap, sentry client (proxy routing, cursor parse, fail-fast 4xx), normEnvironment, relativeTime boundaries (incl. the 0y regression), agentPrompt builders. (useUnifiedIssues merge needs renderHook → future with @testing-library.)
+- [x] **Sanitize Sentry token on store** — sentryConfig.update sanitizes at the storage boundary (getAuth already sanitized reads).
+- [x] **db.tokens dropped** — the table stored the raw token but had zero readers (saveTokenMeta/getTokenMeta/isTokenExpiringSoon were dead); Dexie v4 removes it; SECURITY.md/Settings copy updated.
+- [x] **Relay hardening round 2** — method allowlist (GET/POST/PUT/PATCH/DELETE → 405 otherwise) + 20MB response size cap (declared and actual).
+- [x] **Timer cleanup** — shared useFlash hook (clears on re-trigger + unmount) replaces the raw setTimeout in SinceScope, DetailModalHeader, Checks. (DetailModal's status effect already had cleanup.)
+- [x] **API boundary guards (cheap)** — gql throws on 200-with-no-data; fetchWorkflowRunJobs null-safe. Full schema validation (zod) stays longer-term.
 
 ## Pending — longer term
 

src/api/github/client.ts

@@ -28,6 +28,10 @@ export async function gql<T>(token: string, query: string, variables: Record<str
       if (json.errors) {
         throw new Error(json.errors.map((e: { message: string }) => e.message).join('; '))
       }
+      // Cheap boundary check: a 200 with no data is a malformed/proxy response,
+      // not a usable payload — fail loudly instead of letting `undefined as T`
+      // propagate into the UI.
+      if (json.data == null) throw new Error('GitHub API returned no data')
       return json.data as T
     } catch (e) {
       lastError = e instanceof Error ? e : new Error(String(e))

src/api/github/prs.ts

@@ -261,8 +261,8 @@ export async function mergePullRequest(
 /** Lists jobs in a workflow run. Used to map a CheckRun.name to a job_id so we can fetch its logs. */
 export async function fetchWorkflowRunJobs(token: string, owner: string, name: string, runId: number): Promise<WorkflowJob[]> {
   if (token === DEMO_TOKEN) return []
-  const data = await rest(token, 'GET', `/repos/${owner}/${name}/actions/runs/${runId}/jobs?per_page=100`) as { jobs: WorkflowJob[] }
-  return data.jobs ?? []
+  const data = await rest(token, 'GET', `/repos/${owner}/${name}/actions/runs/${runId}/jobs?per_page=100`) as { jobs?: WorkflowJob[] } | null
+  return data?.jobs ?? []
 }
 
 /**

src/api/sentry/client.test.ts

@@ -0,0 +1,58 @@
+import { afterEach, describe, expect, it, vi } from 'vitest'
+import { parseNextCursor, sentryFetch, type SentryAuth } from './client'
+
+const AUTH: SentryAuth = { token: 'sntryu_test', region: '', proxyBase: '/api/proxy' }
+
+describe('parseNextCursor', () => {
+  it('extracts the cursor only when rel="next" has results', () => {
+    const link = '<https://sentry.io/p>; rel="previous"; results="false"; cursor="0:0:1", <https://sentry.io/n>; rel="next"; results="true"; cursor="0:100:0"'
+    expect(parseNextCursor(link)).toBe('0:100:0')
+  })
+
+  it('returns null when next has no more results, or no header', () => {
+    expect(parseNextCursor('<u>; rel="next"; results="false"; cursor="0:100:0"')).toBeNull()
+    expect(parseNextCursor(null)).toBeNull()
+    expect(parseNextCursor('')).toBeNull()
+  })
+})
+
+describe('sentryFetch', () => {
+  afterEach(() => vi.unstubAllGlobals())
+
+  it('routes through the proxy with the upstream url encoded and bearer auth', async () => {
+    const fetchSpy = vi.fn(async (..._args: unknown[]) => new Response('[]', { status: 200 }))
+    vi.stubGlobal('fetch', fetchSpy)
+    await sentryFetch('/organizations/acme/issues/', AUTH, { query: 'is:unresolved', empty: undefined })
+    const [url, init] = fetchSpy.mock.calls[0]! as unknown as [string, RequestInit]
+    expect(url.startsWith('/api/proxy?url=')).toBe(true)
+    const upstream = decodeURIComponent(url.slice('/api/proxy?url='.length))
+    expect(upstream).toContain('https://sentry.io/api/0/organizations/acme/issues/')
+    expect(upstream).toContain('query=is%3Aunresolved')
+    expect(upstream).not.toContain('empty=')
+    expect((init.headers as Record<string, string>).Authorization).toBe('Bearer sntryu_test')
+  })
+
+  it('uses the regional host when region is set', async () => {
+    const fetchSpy = vi.fn(async (..._args: unknown[]) => new Response('[]', { status: 200 }))
+    vi.stubGlobal('fetch', fetchSpy)
+    await sentryFetch('/organizations/', { ...AUTH, region: 'de' })
+    expect(decodeURIComponent(fetchSpy.mock.calls[0]![0] as string)).toContain('https://de.sentry.io/api/0/')
+  })
+
+  it('returns data + nextCursor from the Link header', async () => {
+    vi.stubGlobal('fetch', vi.fn(async () => new Response('[{"id":"1"}]', {
+      status: 200,
+      headers: { link: '<u>; rel="next"; results="true"; cursor="0:100:0"' },
+    })))
+    const page = await sentryFetch<{ id: string }[]>('/x/', AUTH)
+    expect(page.data).toEqual([{ id: '1' }])
+    expect(page.nextCursor).toBe('0:100:0')
+  })
+
+  it('fails fast on 4xx without retrying', async () => {
+    const fetchSpy = vi.fn(async () => new Response('{"detail":"nope"}', { status: 404 }))
+    vi.stubGlobal('fetch', fetchSpy)
+    await expect(sentryFetch('/missing/', AUTH)).rejects.toThrow(/Sentry 404/)
+    expect(fetchSpy).toHaveBeenCalledTimes(1)
+  })
+})

src/api/sentry/client.ts

@@ -61,7 +61,7 @@ export async function sentryFetch<T>(
 
 // Sentry cursor pagination (RFC5988): `<…>; rel="next"; results="true"; cursor="0:100:0"`.
 // Only a next link with results="true" actually has more.
-function parseNextCursor(link: string | null): string | null {
+export function parseNextCursor(link: string | null): string | null {
   if (!link) return null
   for (const part of link.split(',')) {
     if (part.includes('rel="next"') && part.includes('results="true"')) {

src/api/sentry/issues.test.ts

@@ -0,0 +1,17 @@
+import { describe, expect, it } from 'vitest'
+import { normEnvironment } from './issues'
+
+describe('normEnvironment', () => {
+  it('treats empty and the "all" sentinel as no filter', () => {
+    expect(normEnvironment(undefined)).toBeUndefined()
+    expect(normEnvironment('')).toBeUndefined()
+    expect(normEnvironment('  ')).toBeUndefined()
+    expect(normEnvironment('all')).toBeUndefined()
+    expect(normEnvironment('ALL')).toBeUndefined()
+  })
+
+  it('passes real environments through trimmed', () => {
+    expect(normEnvironment('production')).toBe('production')
+    expect(normEnvironment(' staging ')).toBe('staging')
+  })
+})

src/api/sentry/issues.ts

@@ -43,7 +43,7 @@ export type IssueQuery = {
 
 // Sentry validates `environment` against real environments and 404s on an
 // unknown one. Empty or the sentinel "all" means "no filter" → omit the param.
-function normEnvironment(env?: string): string | undefined {
+export function normEnvironment(env?: string): string | undefined {
   const e = env?.trim()
   return !e || e.toLowerCase() === 'all' ? undefined : e
 }

src/components/SettingsTab.tsx

@@ -141,7 +141,7 @@ export function SettingsTab({ panel, onForceResync }: Props) {
             <li>
               <code>IndexedDB "devcompass"</code>
               <span className="muted">
-                Repos, orgs, pinned items, snoozes, token meta, and the per-API
+                Repos, orgs, pinned items, snoozes, and the per-API
                 response cache (viewer / tokenInfo / userOrgs / PR detail / branches /
                 since-last-visit snapshot). All scoped to this origin.
               </span>

src/components/home/DetailModalHeader.tsx

@@ -1,5 +1,6 @@
 import { useMemo, useState } from 'react'
 import { FaCodeBranch } from 'react-icons/fa'
+import { useFlash } from '../../hooks/useFlash'
 import type { PRDetail } from '../../api/github'
 import { ConfirmDialog } from '../ConfirmDialog'
 import { OrgChip } from './OrgChip'
@@ -79,7 +80,7 @@ function HeaderMeta({ detail, item }: { detail: PRDetail; item: AttentionItem })
 }
 
 function LinkActions({ item }: { item: AttentionItem }) {
-  const [copied, setCopied] = useState(false)
+  const [copied, flashCopied] = useFlash(1800)
   // When the Clipboard API fails (insecure context, denied permission, etc.)
   // we surface the URL in a styled dialog so the user can manually select +
   // copy. Replaces the old window.prompt fallback.
@@ -88,8 +89,7 @@ function LinkActions({ item }: { item: AttentionItem }) {
     const url = item.url || `${window.location.origin}${window.location.pathname}?pr=${item.org}/${item.repo}/${item.number}`
     try {
       await navigator.clipboard.writeText(url)
-      setCopied(true)
-      setTimeout(() => setCopied(false), 1800)
+      flashCopied()
     } catch {
       setFallbackUrl(url)
     }

src/components/home/detail/Checks.tsx

@@ -6,6 +6,7 @@ import {
   type PRDetail,
   type WorkflowJob
 } from '../../../api/github'
+import { useFlash } from '../../../hooks/useFlash'
 
 const MAX_LOG_LINES = 500
 
@@ -144,14 +145,13 @@ function CheckItem({ row, token, owner, repo }: { row: CheckRow; token: string;
   const [loading, setLoading] = useState(false)
   const [err, setErr] = useState<string | null>(null)
   const [matchedJob, setMatchedJob] = useState<WorkflowJob | null>(null)
-  const [copied, setCopied] = useState(false)
+  const [copied, flashCopied] = useFlash(1500)
 
   async function copyLog() {
     if (!log) return
     try {
       await navigator.clipboard.writeText(log)
-      setCopied(true)
-      setTimeout(() => setCopied(false), 1500)
+      flashCopied()
     } catch {
       // Fallback: select the <pre> text so the user can Cmd/Ctrl+C manually.
       const pre = document.querySelector<HTMLPreElement>('.hs-log-pre')

src/components/home/scopes/SinceScope.tsx

@@ -1,19 +1,18 @@
-import { useState } from 'react'
 import { OrgChip } from '../OrgChip'
 import { useSinceLastVisit, type SinceEvent } from '../useSinceLastVisit'
+import { useFlash } from '../../../hooks/useFlash'
 import type { AttentionItem } from '../types'
 import { relativeTime } from '../../../utils/time'
 import { type ScopeProps } from './common'
 
 export function SinceScope({ repos, onOpenItem, onOpenRepo }: ScopeProps) {
   const { events, isFirstRun, snapshot, markSeen } = useSinceLastVisit(repos)
-  const [seen, setSeen] = useState(false)
+  const [seen, flashSeen] = useFlash(1500)
   const showEvents = !seen ? events : []
 
   async function handleMarkSeen() {
     await markSeen()
-    setSeen(true)
-    setTimeout(() => setSeen(false), 1500)
+    flashSeen()
   }
 
   function handleEvent(ev: SinceEvent) {

src/hooks/useFlash.ts

@@ -0,0 +1,20 @@
+import { useEffect, useRef, useState } from 'react'
+
+/**
+ * Transient boolean that auto-resets after `ms` — for "✓ Copied"-style
+ * confirmations. The timer is cleared on re-trigger and on unmount, so rapid
+ * clicks don't stack timers and unmounted components don't set state.
+ */
+export function useFlash(ms: number): [boolean, () => void] {
+  const [on, setOn] = useState(false)
+  const timer = useRef<number | null>(null)
+
+  useEffect(() => () => { if (timer.current) clearTimeout(timer.current) }, [])
+
+  function trigger() {
+    setOn(true)
+    if (timer.current) clearTimeout(timer.current)
+    timer.current = window.setTimeout(() => setOn(false), ms)
+  }
+  return [on, trigger]
+}