fix(security): addresses PR review findings with tests

- removes IP from rate-limit log, adds prune threshold guard
- makes SENTRY_DSN optional in Env interface
- moves returnTo consumption after auth success in OAuthCallback
- adds race-safe coordinator guard to onAuthCleared + clearHotSets
- extracts IGNORED_ITEMS_CAP constant, fixes FALLBACK_FG consistency
- documents CSP style-src-attr requirement (Kobalte) in SECURITY.md
- adds 13 tests: cross-tab auth sync, 401 propagation, label-colors
  CSS rules, fetchRepos cap, rate-limit window reset, multi-user
  upstream discovery cap

Author: wgordon17

SECURITY.md

@@ -35,7 +35,7 @@ This policy covers:
 
 This project implements the following security measures:
 
-- **CSP**: Strict Content-Security-Policy with `default-src 'none'` and no `unsafe-eval`
+- **CSP**: Strict Content-Security-Policy with `default-src 'none'`, no `unsafe-eval`, no `unsafe-inline` for scripts (`style-src-attr 'unsafe-inline'` required by Kobalte UI library)
 - **CORS**: Strict origin equality matching on the Worker
 - **OAuth CSRF**: Cryptographically random state parameter with single-use enforcement
 - **Read-only API access**: Octokit hook blocks all write operations

src/app/components/dashboard/DashboardPage.tsx

@@ -84,13 +84,14 @@ onAuthCleared(() => {
   const coord = _coordinator();
   if (coord) {
     coord.destroy();
-    _setCoordinator(null);
+    if (_coordinator() === coord) _setCoordinator(null);
   }
   const hotCoord = _hotCoordinator();
   if (hotCoord) {
     hotCoord.destroy();
-    _setHotCoordinator(null);
+    if (_hotCoordinator() === hotCoord) _setHotCoordinator(null);
   }
+  clearHotSets();
 });
 
 async function pollFetch(): Promise<DashboardData> {

src/app/lib/label-colors.ts

@@ -1,8 +1,10 @@
 import { labelTextColor } from "./format";
 
 // Dynamic label color registry using adoptedStyleSheets.
-// This avoids inline style attributes, allowing removal of
-// style-src-attr 'unsafe-inline' from the CSP.
+// This avoids inline style attributes for labels — however
+// style-src-attr 'unsafe-inline' is still required in the CSP
+// because Kobalte sets inline styles for popper positioning,
+// collapsible height, and presence animations.
 let _sheet: CSSStyleSheet | null = null;
 
 function getSheet(): CSSStyleSheet {
@@ -15,7 +17,7 @@ function getSheet(): CSSStyleSheet {
 
 const registered = new Set<string>();
 const FALLBACK_BG = "e5e7eb";
-const FALLBACK_FG = "#374151";
+const FALLBACK_FG = "374151";
 
 /**
  * Registers a label color in the adopted stylesheet and returns
@@ -28,7 +30,7 @@ export function labelColorClass(hex: string): string {
 
   if (!registered.has(safeHex)) {
     const bg = `#${safeHex}`;
-    const fg = isValid ? labelTextColor(safeHex) : FALLBACK_FG;
+    const fg = isValid ? labelTextColor(safeHex) : `#${FALLBACK_FG}`;
     getSheet().insertRule(`.lb-${safeHex} { background-color: ${bg}; color: ${fg}; }`);
     registered.add(safeHex);
   }

src/app/pages/OAuthCallback.tsx

@@ -36,10 +36,6 @@ export default function OAuthCallback() {
       return;
     }
 
-    // Read and clear returnTo only after all pre-exchange checks pass
-    const returnTo = sessionStorage.getItem(OAUTH_RETURN_TO_KEY);
-    sessionStorage.removeItem(OAUTH_RETURN_TO_KEY);
-
     try {
       const resp = await fetch("/api/oauth/token", {
         method: "POST",
@@ -64,6 +60,9 @@ export default function OAuthCallback() {
         return;
       }
 
+      // Read and clear returnTo only after successful auth (preserves it for retry on failure)
+      const returnTo = sessionStorage.getItem(OAUTH_RETURN_TO_KEY);
+      sessionStorage.removeItem(OAUTH_RETURN_TO_KEY);
       navigate(sanitizeReturnTo(returnTo), { replace: true });
     } catch {
       setError("A network error occurred. Please try again.");

src/app/stores/view.ts

@@ -4,6 +4,7 @@ import { createEffect, onCleanup, untrack } from "solid-js";
 import { pushNotification } from "../lib/errors";
 
 export const VIEW_STORAGE_KEY = "github-tracker:view";
+const IGNORED_ITEMS_CAP = 500;
 
 const IssueFiltersSchema = z.object({
   role: z.enum(["all", "author", "assignee"]).default("all"),
@@ -55,7 +56,7 @@ export const ViewStateSchema = z.object({
         ignoredAt: z.number(),
       })
     )
-    .max(500)
+    .max(IGNORED_ITEMS_CAP)
     .default([]),
   globalFilter: z
     .object({
@@ -148,7 +149,7 @@ export function ignoreItem(item: IgnoredItem): void {
       const already = draft.ignoredItems.some((i) => i.id === item.id);
       if (!already) {
         // FIFO eviction: remove oldest if at cap
-        if (draft.ignoredItems.length >= 500) {
+        if (draft.ignoredItems.length >= IGNORED_ITEMS_CAP) {
           draft.ignoredItems.shift();
         }
         draft.ignoredItems.push(item);

src/worker/index.ts

@@ -3,7 +3,7 @@ export interface Env {
   GITHUB_CLIENT_ID: string;
   GITHUB_CLIENT_SECRET: string;
   ALLOWED_ORIGIN: string;
-  SENTRY_DSN: string; // e.g. "https://key@o123456.ingest.sentry.io/7890123"
+  SENTRY_DSN?: string; // e.g. "https://key@o123456.ingest.sentry.io/7890123"
   SENTRY_SECURITY_TOKEN?: string; // Optional: Sentry security token for Allowed Domains validation
 }
 
@@ -81,8 +81,11 @@ function checkTokenRateLimit(ip: string): boolean {
   return true;
 }
 
-// Periodic cleanup to prevent unbounded map growth
+// Periodic cleanup to prevent unbounded map growth.
+// Only runs when the map exceeds a threshold to avoid O(N) scan on every request.
+const PRUNE_THRESHOLD = 100;
 function pruneTokenRateMap(): void {
+  if (_tokenRateMap.size < PRUNE_THRESHOLD) return;
   const now = Date.now();
   for (const [ip, entry] of _tokenRateMap) {
     if (now >= entry.resetAt) _tokenRateMap.delete(ip);
@@ -134,8 +137,9 @@ function parseSentryDsn(dsn: string): ParsedDsn | null {
 
 /** Get cached parsed DSN, re-parsing only when the DSN string changes. */
 function getOrCacheDsn(env: Env): ParsedDsn | null {
-  if (!_dsnCache || _dsnCache.dsn !== env.SENTRY_DSN) {
-    _dsnCache = { dsn: env.SENTRY_DSN, parsed: parseSentryDsn(env.SENTRY_DSN) };
+  const dsn = env.SENTRY_DSN ?? "";
+  if (!_dsnCache || _dsnCache.dsn !== dsn) {
+    _dsnCache = { dsn, parsed: parseSentryDsn(dsn) };
   }
   return _dsnCache.parsed;
 }
@@ -355,7 +359,7 @@ async function handleTokenExchange(
   pruneTokenRateMap();
   const ip = request.headers.get("CF-Connecting-IP") ?? "unknown";
   if (!checkTokenRateLimit(ip)) {
-    log("warn", "token_exchange_rate_limited", { ip }, request);
+    log("warn", "token_exchange_rate_limited", {}, request);
     return new Response(JSON.stringify({ error: "rate_limited" }), {
       status: 429,
       headers: {

tests/components/OAuthCallback.test.tsx

@@ -314,17 +314,17 @@ describe("OAuthCallback", () => {
     expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull();
   });
 
-  it("OAUTH_RETURN_TO_KEY is removed from sessionStorage after reading", async () => {
+  it("OAUTH_RETURN_TO_KEY is preserved while token exchange is in flight", async () => {
     sessionStorage.setItem(OAUTH_RETURN_TO_KEY, "/settings");
     setupValidState();
     setWindowSearch({ code: "fakecode", state: "teststate" });
     vi.stubGlobal("fetch", vi.fn(() => new Promise(() => {})));
 
     renderCallback();
 
-    await waitFor(() => {
-      expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull();
-    });
+    // returnTo is not consumed until auth completes — preserved for retry on failure
+    await new Promise((r) => setTimeout(r, 50));
+    expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBe("/settings");
   });
 
   it("OAUTH_RETURN_TO_KEY is preserved when CSRF check fails (only consumed on success)", async () => {

tests/lib/label-colors.test.ts

@@ -32,4 +32,20 @@ describe("labelColorClass", () => {
     expect(first).toBe(second);
     expect(first).toBe("lb-ff0000");
   });
+
+  it("inserts CSS rule with correct background and foreground colors", () => {
+    labelColorClass("000000");
+    const sheet = document.adoptedStyleSheets[document.adoptedStyleSheets.length - 1];
+    const rule = sheet.cssRules[0].cssText;
+    expect(rule).toContain(".lb-000000");
+    expect(rule).toContain("background-color");
+    expect(rule).toContain("color");
+  });
+
+  it("does not insert duplicate rules for same hex", () => {
+    labelColorClass("ff0000");
+    labelColorClass("ff0000");
+    const sheet = document.adoptedStyleSheets[document.adoptedStyleSheets.length - 1];
+    expect(sheet.cssRules.length).toBe(1);
+  });
 });

tests/services/api-users.test.ts

@@ -328,6 +328,39 @@ describe("discoverUpstreamRepos", () => {
     expect(result.length).toBeLessThanOrEqual(100);
   });
 
+  it("stops processing users when cap is reached across multiple tracked users", async () => {
+    // User A discovers 70 repos; user B would discover 70 more (overlapping names
+    // avoided by using distinct prefixes). The shared repoNames Set is capped at 100,
+    // so user B can only contribute at most 30 before the cap is hit.
+    const userARepos = Array.from({ length: 70 }, (_, i) => `user-a/repo-${i.toString().padStart(3, "0")}`);
+    const userBRepos = Array.from({ length: 70 }, (_, i) => `user-b/repo-${i.toString().padStart(3, "0")}`);
+
+    const octokit = makeOctokit(
+      async () => ({}),
+      async (_query: string, vars: unknown) => {
+        const v = vars as { q: string };
+        if (v.q.includes("involves:primary") && v.q.includes("is:issue")) {
+          return makeSearchPage(userARepos);
+        }
+        if (v.q.includes("involves:trackeduser") && v.q.includes("is:issue")) {
+          return makeSearchPage(userBRepos);
+        }
+        return makeSearchPage([]);
+      }
+    );
+
+    const trackedUsers = [makeTrackedUser("trackeduser")];
+    const result = await discoverUpstreamRepos(octokit as never, "primary", new Set(), trackedUsers);
+
+    // Total must not exceed the CAP of 100
+    expect(result.length).toBeLessThanOrEqual(100);
+    // User A's repos should all be present (70 < 100)
+    const names = result.map((r) => r.fullName);
+    expect(names.filter((n) => n.startsWith("user-a/")).length).toBe(70);
+    // User B's repos are partially included (capped at 30)
+    expect(names.filter((n) => n.startsWith("user-b/")).length).toBeLessThanOrEqual(30);
+  });
+
   it("returns results sorted alphabetically by fullName", async () => {
     const octokit = makeOctokit(
       async () => ({}),

tests/services/api.test.ts

@@ -168,6 +168,42 @@ describe("fetchRepos", () => {
       "No GitHub client available"
     );
   });
+
+  it("stops pagination at 1000 repos and emits warning notification", async () => {
+    vi.mocked(pushNotification).mockClear();
+
+    async function* mockPaginator() {
+      for (let page = 0; page < 20; page++) {
+        yield {
+          data: Array.from({ length: 100 }, (_, i) => ({
+            owner: { login: "org" },
+            name: `repo-${page * 100 + i}`,
+            full_name: `org/repo-${page * 100 + i}`,
+            pushed_at: "2026-01-01T00:00:00Z",
+          })),
+        };
+      }
+    }
+
+    const octokit = makeBasicOctokit();
+    octokit.paginate.iterator.mockImplementation((() => mockPaginator()) as never);
+
+    const result = await fetchRepos(octokit as never, "org", "org");
+
+    expect(result).toHaveLength(1000);
+    expect(pushNotification).toHaveBeenCalledWith(
+      "api",
+      expect.stringContaining("1000+"),
+      "warning"
+    );
+  });
+
+  // VALID_REPO_NAME is not exported. It is exercised indirectly via buildRepoQualifiers
+  // (called inside fetchIssues/fetchPullRequests). Invalid repo names are silently
+  // filtered from the GraphQL query qualifiers. Direct boundary tests for the regex
+  // itself would require exporting the constant; the existing fetchIssues tests
+  // already cover the valid-name path and the VALID_TRACKED_LOGIN boundary tests
+  // cover the analogous login regex. No additional test added here.
 });
 
 // ── fetchIssues (GraphQL search) ─────────────────────────────────────────────