From 86a645ccf921f91631cb9d1c3e218631ab625d97 Mon Sep 17 00:00:00 2001 From: testvalue Date: Tue, 24 Mar 2026 13:17:11 -0400 Subject: [PATCH 01/22] refactor(worker): removes refresh and logout endpoints --- src/worker/index.ts | 140 +------------------------------------------- 1 file changed, 1 insertion(+), 139 deletions(-) diff --git a/src/worker/index.ts b/src/worker/index.ts index 01be77d3..1ca6b0b8 100644 --- a/src/worker/index.ts +++ b/src/worker/index.ts @@ -45,52 +45,17 @@ function getCorsHeaders( "Access-Control-Allow-Origin": allowedOrigin, "Access-Control-Allow-Methods": "POST", "Access-Control-Allow-Headers": "Content-Type", - "Access-Control-Allow-Credentials": "true", "Vary": "Origin", }; } return {}; } -// ── Refresh token cookie helpers ───────────────────────────────────────────── - -const RT_COOKIE_NAME_PROD = "__Host-github_tracker_rt"; -const RT_COOKIE_NAME_LOCAL = "github_tracker_rt"; -const RT_MAX_AGE = 15_552_000; // ~6 months, matches GitHub refresh token lifetime - -function setRefreshTokenCookie(token: string, env: Env): string { - const isLocal = env.ALLOWED_ORIGIN.startsWith("http://localhost"); - if (isLocal) { - return `${RT_COOKIE_NAME_LOCAL}=${token}; HttpOnly; SameSite=Lax; Path=/; Max-Age=${RT_MAX_AGE}`; - } - return `${RT_COOKIE_NAME_PROD}=${token}; HttpOnly; Secure; SameSite=Strict; Path=/; Max-Age=${RT_MAX_AGE}`; -} - -function clearRefreshTokenCookie(env: Env): string { - const isLocal = env.ALLOWED_ORIGIN.startsWith("http://localhost"); - if (isLocal) { - return `${RT_COOKIE_NAME_LOCAL}=; HttpOnly; SameSite=Lax; Path=/; Max-Age=0`; - } - return `${RT_COOKIE_NAME_PROD}=; HttpOnly; Secure; SameSite=Strict; Path=/; Max-Age=0`; -} - -function getRefreshTokenFromCookie(request: Request, env: Env): string | null { - const cookie = request.headers.get("Cookie"); - if (!cookie) return null; - const isLocal = env.ALLOWED_ORIGIN.startsWith("http://localhost"); - const name = isLocal ? RT_COOKIE_NAME_LOCAL : RT_COOKIE_NAME_PROD; - const match = cookie.match(new RegExp(`${name}=([^;]+)`)); - return match ? match[1] : null; -} - // GitHub OAuth code format validation (SDR-005): alphanumeric, 1-40 chars. // GitHub's code format is undocumented and has changed historically — validate // loosely here; GitHub's server validates the actual code. const VALID_CODE_RE = /^[a-zA-Z0-9_-]{1,40}$/; -// GitHub App refresh token format validation (SEC-003): ghr_ prefix + alphanumeric/underscore -const VALID_REFRESH_TOKEN_RE = /^ghr_[A-Za-z0-9_]{20,255}$/; - async function handleTokenExchange( request: Request, env: Env, @@ -158,111 +123,16 @@ async function handleTokenExchange( } // Return only allowed fields — never forward full GitHub response. - // Refresh token goes into an HttpOnly cookie, NOT the response body. - const refreshToken = typeof githubData["refresh_token"] === "string" - ? githubData["refresh_token"] - : null; - const allowed = { access_token: githubData["access_token"], token_type: githubData["token_type"] ?? "bearer", scope: githubData["scope"] ?? "", - expires_in: githubData["expires_in"] ?? null, - }; - - const headers: Record = { - "Content-Type": "application/json", - ...cors, - ...securityHeaders(), - }; - if (refreshToken) { - headers["Set-Cookie"] = setRefreshTokenCookie(refreshToken, env); - } - - return new Response(JSON.stringify(allowed), { status: 200, headers }); -} - -async function handleRefreshToken( - request: Request, - env: Env, - cors: Record -): Promise { - if (request.method !== "POST") { - return errorResponse("method_not_allowed", 405, cors); - } - - // Read refresh token from HttpOnly cookie (not request body) - const refreshToken = getRefreshTokenFromCookie(request, env); - if (!refreshToken || !VALID_REFRESH_TOKEN_RE.test(refreshToken)) { - return errorResponse("invalid_request", 400, cors); - } - - let githubData: Record; - try { - const githubResp = await fetch( - "https://github.com/login/oauth/access_token", - { - method: "POST", - headers: { - "Content-Type": "application/json", - Accept: "application/json", - }, - body: JSON.stringify({ - client_id: env.GITHUB_CLIENT_ID, - client_secret: env.GITHUB_CLIENT_SECRET, - grant_type: "refresh_token", - refresh_token: refreshToken, - }), - } - ); - githubData = (await githubResp.json()) as Record; - } catch { - return errorResponse("token_exchange_failed", 400, cors); - } - - if ( - typeof githubData["error"] === "string" || - typeof githubData["access_token"] !== "string" - ) { - return errorResponse("token_exchange_failed", 400, cors); - } - - // Rotated refresh token goes into HttpOnly cookie, not response body - const newRefreshToken = typeof githubData["refresh_token"] === "string" - ? githubData["refresh_token"] - : null; - - const allowed = { - access_token: githubData["access_token"], - token_type: githubData["token_type"] ?? "bearer", - expires_in: githubData["expires_in"] ?? null, - }; - - const headers: Record = { - "Content-Type": "application/json", - ...cors, - ...securityHeaders(), }; - if (newRefreshToken) { - headers["Set-Cookie"] = setRefreshTokenCookie(newRefreshToken, env); - } - - return new Response(JSON.stringify(allowed), { status: 200, headers }); -} -async function handleLogout( - request: Request, - env: Env, - cors: Record -): Promise { - if (request.method !== "POST") { - return errorResponse("method_not_allowed", 405, cors); - } - return new Response(JSON.stringify({ ok: true }), { + return new Response(JSON.stringify(allowed), { status: 200, headers: { "Content-Type": "application/json", - "Set-Cookie": clearRefreshTokenCookie(env), ...cors, ...securityHeaders(), }, @@ -287,14 +157,6 @@ export default { return handleTokenExchange(request, env, cors); } - if (url.pathname === "/api/oauth/refresh") { - return handleRefreshToken(request, env, cors); - } - - if (url.pathname === "/api/oauth/logout") { - return handleLogout(request, env, cors); - } - if (url.pathname === "/api/health" && request.method === "GET") { return new Response("OK", { headers: securityHeaders(), From 5be4d48bb6f20ef4351ea1858d2758e602f61660 Mon Sep 17 00:00:00 2001 From: testvalue Date: Tue, 24 Mar 2026 13:29:53 -0400 Subject: [PATCH 02/22] refactor(auth): switches to localStorage token persistence for OAuth App --- .github/workflows/ci.yml | 26 ++ .github/workflows/preview.yml | 101 ----- e2e/settings.spec.ts | 26 +- e2e/smoke.spec.ts | 23 +- src/app/App.tsx | 8 +- .../components/dashboard/DashboardPage.tsx | 10 +- src/app/components/settings/SettingsPage.tsx | 4 +- src/app/pages/LoginPage.tsx | 2 +- src/app/pages/OAuthCallback.tsx | 1 - src/app/pages/PrivacyPage.tsx | 6 +- src/app/services/poll.ts | 2 +- src/app/stores/auth.ts | 81 +--- tests/components/App.test.tsx | 22 +- tests/components/DashboardPage.test.tsx | 33 +- tests/components/LoginPage.test.tsx | 9 + tests/components/OAuthCallback.test.tsx | 3 +- tests/setup.ts | 17 + tests/stores/auth.test.ts | 120 ++---- tests/worker/oauth.test.ts | 355 ++---------------- vitest.config.ts | 1 + wrangler.toml | 1 - 21 files changed, 191 insertions(+), 660 deletions(-) create mode 100644 .github/workflows/ci.yml delete mode 100644 .github/workflows/preview.yml create mode 100644 tests/setup.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000..ab287a40 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,26 @@ +name: CI +on: + pull_request: +permissions: + contents: read +jobs: + ci: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: pnpm/action-setup@v4 + - uses: actions/setup-node@v4 + with: + node-version: 22 + cache: pnpm + - run: pnpm install --frozen-lockfile + - run: pnpm run typecheck + - run: pnpm test + - name: Verify CSP hash + run: node scripts/verify-csp-hash.mjs + - name: Install Playwright browsers + run: npx playwright install chromium --with-deps + - name: Run E2E tests + run: pnpm test:e2e + env: + VITE_GITHUB_CLIENT_ID: ${{ vars.VITE_GITHUB_CLIENT_ID }} diff --git a/.github/workflows/preview.yml b/.github/workflows/preview.yml deleted file mode 100644 index 44e6b4e6..00000000 --- a/.github/workflows/preview.yml +++ /dev/null @@ -1,101 +0,0 @@ -name: Preview -on: - pull_request: -permissions: - contents: read - pull-requests: write -jobs: - ci: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - uses: pnpm/action-setup@v4 - - uses: actions/setup-node@v4 - with: - node-version: 22 - cache: pnpm - - run: pnpm install --frozen-lockfile - - run: pnpm run typecheck - - run: pnpm test - - name: Verify CSP hash - run: node scripts/verify-csp-hash.mjs - - name: Install Playwright browsers - run: npx playwright install chromium --with-deps - - name: Run E2E tests - run: pnpm test:e2e - env: - VITE_GITHUB_CLIENT_ID: ${{ vars.VITE_GITHUB_CLIENT_ID }} - preview: - needs: ci - if: github.event.pull_request.head.repo.full_name == github.repository - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - uses: pnpm/action-setup@v4 - - uses: actions/setup-node@v4 - with: - node-version: 22 - cache: pnpm - - run: pnpm install --frozen-lockfile - - name: Verify CSP hash - run: node scripts/verify-csp-hash.mjs - - run: pnpm run build - env: - VITE_GITHUB_CLIENT_ID: ${{ vars.VITE_GITHUB_CLIENT_ID }} - - name: Slugify branch name - id: slug - env: - BRANCH: ${{ github.head_ref }} - run: echo "alias=$(echo "$BRANCH" | sed 's/[^a-zA-Z0-9]/-/g; s/--*/-/g; s/^-//; s/-$//' | tr 'A-Z' 'a-z' | sed 's/^[^a-z]*//' | cut -c1-48 | sed 's/-$//; s/^$/preview/')" >> "$GITHUB_OUTPUT" - - name: Upload preview version - id: preview - uses: cloudflare/wrangler-action@v3 - with: - apiToken: ${{ secrets.CLOUDFLARE_API_TOKEN }} - accountId: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }} - command: versions upload --preview-alias ${{ steps.slug.outputs.alias }} - - name: Comment preview URL - uses: actions/github-script@v7 - env: - WRANGLER_OUTPUT: ${{ steps.preview.outputs.command-output }} - BRANCH_ALIAS: ${{ steps.slug.outputs.alias }} - BRANCH_NAME: ${{ github.head_ref }} - with: - script: | - const output = process.env.WRANGLER_OUTPUT || ''; - const urlMatch = output.match(/https:\/\/[^\s"'`]+\.workers\.dev[^\s"'`]*/); - const alias = process.env.BRANCH_ALIAS; - const branch = process.env.BRANCH_NAME; - const url = urlMatch ? urlMatch[0] : `Preview alias: ${alias} (check workflow logs for URL)`; - const marker = ''; - const body = [ - '### Preview Deployment', - '', - urlMatch ? `[${url}](${url})` : url, - '', - `Branch: \`${branch}\` | Alias: \`${alias}\``, - '', - marker, - ].join('\n'); - - const comments = await github.paginate(github.rest.issues.listComments, { - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - }); - const existing = comments.find(c => c.body?.includes(marker)); - if (existing) { - await github.rest.issues.updateComment({ - owner: context.repo.owner, - repo: context.repo.repo, - comment_id: existing.id, - body, - }); - } else { - await github.rest.issues.createComment({ - owner: context.repo.owner, - repo: context.repo.repo, - issue_number: context.issue.number, - body, - }); - } diff --git a/e2e/settings.spec.ts b/e2e/settings.spec.ts index 1c9ef115..26954caa 100644 --- a/e2e/settings.spec.ts +++ b/e2e/settings.spec.ts @@ -1,17 +1,11 @@ import { test, expect, type Page } from "@playwright/test"; /** - * Register API route interceptors and inject config BEFORE any navigation. - * The app calls refreshAccessToken() on load, which POSTs to /api/oauth/refresh - * (HttpOnly cookie-based). We intercept that to return a valid access token. + * Register API route interceptors and inject auth + config into localStorage BEFORE navigation. + * OAuth App uses permanent tokens stored in localStorage — no refresh endpoint needed. + * The app calls validateToken() on load, which GETs /user to verify the token. */ async function setupAuth(page: Page) { - await page.route("**/api/oauth/refresh", (route) => - route.fulfill({ - status: 200, - json: { access_token: "ghu_fake", expires_in: 86400 }, - }) - ); await page.route("https://api.github.com/user", (route) => route.fulfill({ status: 200, @@ -36,6 +30,7 @@ async function setupAuth(page: Page) { ); await page.addInitScript(() => { + localStorage.setItem("github-tracker:auth-token", "ghu_fake"); localStorage.setItem( "github-tracker:config", JSON.stringify({ @@ -114,16 +109,17 @@ test("sign out clears auth and redirects to login", async ({ page }) => { const signOutBtn = page.getByRole("button", { name: /^sign out$/i }); await expect(signOutBtn).toBeVisible(); - // Intercept the logout cookie-clearing request - await page.route("**/api/oauth/logout", (route) => - route.fulfill({ status: 200, json: { ok: true } }) - ); - await signOutBtn.click(); - // clearAuth() clears in-memory token and navigates to /login + // clearAuth() removes the localStorage token and navigates to /login await expect(page).toHaveURL(/\/login/); + // Verify auth token was cleared from localStorage + const authToken = await page.evaluate(() => + localStorage.getItem("github-tracker:auth-token") + ); + expect(authToken).toBeNull(); + // Verify config was reset (SDR-016 data leakage prevention). // The persistence effect may re-write defaults, so check that user-specific // data (selectedOrgs, onboardingComplete) was cleared rather than checking null. diff --git a/e2e/smoke.spec.ts b/e2e/smoke.spec.ts index ba35a5ae..9da13289 100644 --- a/e2e/smoke.spec.ts +++ b/e2e/smoke.spec.ts @@ -1,20 +1,12 @@ import { test, expect, type Page } from "@playwright/test"; /** - * Register API route interceptors and inject config BEFORE any navigation. - * The app calls refreshAccessToken() on load, which POSTs to /api/oauth/refresh - * (HttpOnly cookie-based). We intercept that to return a valid access token, - * and intercept GET /user (called by refreshAccessToken to validate the token). + * Register API route interceptors and inject auth + config into localStorage BEFORE navigation. + * OAuth App uses permanent tokens stored in localStorage — no refresh endpoint needed. + * The app calls validateToken() on load, which GETs /user to verify the token. */ async function setupAuth(page: Page) { - // Intercept refresh token exchange — app calls this on page load - await page.route("**/api/oauth/refresh", (route) => - route.fulfill({ - status: 200, - json: { access_token: "ghu_fake", expires_in: 86400 }, - }) - ); - // Intercept /user validation (called by refreshAccessToken after getting token) + // Intercept /user validation (called by validateToken on page load) await page.route("https://api.github.com/user", (route) => route.fulfill({ status: 200, @@ -46,8 +38,9 @@ async function setupAuth(page: Page) { route.fulfill({ status: 200, json: { data: {} } }) ); - // Inject config into localStorage (config is still persisted there) + // Seed localStorage with auth token and config before the page loads await page.addInitScript(() => { + localStorage.setItem("github-tracker:auth-token", "ghu_fake"); localStorage.setItem( "github-tracker:config", JSON.stringify({ @@ -83,8 +76,8 @@ test("OAuth callback flow completes and redirects", async ({ page }) => { status: 200, json: { access_token: "ghu_fake", - refresh_token: "ghr_fake", - expires_in: 86400, + token_type: "bearer", + scope: "repo read:org notifications", }, }) ); diff --git a/src/app/App.tsx b/src/app/App.tsx index d32d1e7c..b9b4347c 100644 --- a/src/app/App.tsx +++ b/src/app/App.tsx @@ -1,6 +1,6 @@ import { createSignal, createEffect, onMount, Show, type JSX } from "solid-js"; import { Router, Route, Navigate, useNavigate } from "@solidjs/router"; -import { isAuthenticated, refreshAccessToken } from "./stores/auth"; +import { isAuthenticated, validateToken } from "./stores/auth"; import { config, initConfigPersistence } from "./stores/config"; import { initViewPersistence } from "./stores/view"; import { evictStaleEntries } from "./stores/cache"; @@ -13,14 +13,14 @@ import SettingsPage from "./components/settings/SettingsPage"; import PrivacyPage from "./pages/PrivacyPage"; // Auth guard: redirects unauthenticated users to /login. -// On page load (no in-memory token), attempts a silent refresh via HttpOnly cookie. +// On page load, validates the localStorage token with GitHub API. function AuthGuard(props: { children: JSX.Element }) { const [validating, setValidating] = createSignal(true); const navigate = useNavigate(); onMount(async () => { if (!isAuthenticated()) { - await refreshAccessToken(); + await validateToken(); } setValidating(false); }); @@ -71,7 +71,7 @@ function RootRedirect() { onMount(async () => { if (!isAuthenticated()) { - await refreshAccessToken(); + await validateToken(); } setValidating(false); }); diff --git a/src/app/components/dashboard/DashboardPage.tsx b/src/app/components/dashboard/DashboardPage.tsx index dc99036d..2c91ffed 100644 --- a/src/app/components/dashboard/DashboardPage.tsx +++ b/src/app/components/dashboard/DashboardPage.tsx @@ -10,7 +10,7 @@ import { config } from "../../stores/config"; import { viewState, updateViewState } from "../../stores/view"; import type { Issue, PullRequest, WorkflowRun, ApiError } from "../../services/api"; import { createPollCoordinator, fetchAllData } from "../../services/poll"; -import { refreshAccessToken, clearAuth, user } from "../../stores/auth"; +import { clearAuth, user } from "../../stores/auth"; import { getErrors, dismissError } from "../../lib/errors"; import ErrorBannerList from "../shared/ErrorBannerList"; @@ -62,11 +62,9 @@ async function pollFetch(): Promise : null; if (status === 401) { - const refreshed = await refreshAccessToken(); - if (!refreshed) { - clearAuth(); - window.location.replace("/login"); - } + // Permanent token is revoked — clear auth and redirect to login + clearAuth(); + window.location.replace("/login"); } setDashboardData("loading", false); throw err; diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index 8741bdfd..c1a36001 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -256,8 +256,8 @@ export default function SettingsPage() { setConfirmReset(true); return; } - // clearAuth handles: token + user signals, localStorage (config/view), - // HttpOnly cookie logout, IndexedDB cache, and onAuthCleared callbacks + // clearAuth handles: token + user signals, localStorage (auth/config/view), + // IndexedDB cache, and onAuthCleared callbacks clearAuth(); window.location.reload(); } diff --git a/src/app/pages/LoginPage.tsx b/src/app/pages/LoginPage.tsx index a9e09449..23b01c1f 100644 --- a/src/app/pages/LoginPage.tsx +++ b/src/app/pages/LoginPage.tsx @@ -16,7 +16,7 @@ export default function LoginPage() { client_id: clientId, redirect_uri: redirectUri, state, - // No scope param — GitHub App uses installation-level permissions (SDR-009) + scope: "repo read:org notifications", }); window.location.href = `https://github.com/login/oauth/authorize?${params.toString()}`; diff --git a/src/app/pages/OAuthCallback.tsx b/src/app/pages/OAuthCallback.tsx index 5bb4d65f..b01932ac 100644 --- a/src/app/pages/OAuthCallback.tsx +++ b/src/app/pages/OAuthCallback.tsx @@ -6,7 +6,6 @@ interface TokenResponse { access_token: string; token_type?: string; scope?: string; - expires_in?: number | null; error?: string; } diff --git a/src/app/pages/PrivacyPage.tsx b/src/app/pages/PrivacyPage.tsx index e161d092..70024941 100644 --- a/src/app/pages/PrivacyPage.tsx +++ b/src/app/pages/PrivacyPage.tsx @@ -32,8 +32,8 @@ export default function PrivacyPage() { reduce GitHub API usage. Cleared on logout.
  • - HttpOnly cookie — a GitHub refresh token managed - by the OAuth proxy worker. Never accessible to JavaScript. + localStorage — your OAuth access token, used to + authenticate GitHub API requests. Cleared on logout.
    • @@ -44,7 +44,7 @@ export default function PrivacyPage() {
  • No analytics or tracking scripts
  • No server-side data storage
  • No third-party data sharing
    • -
  • No cookies beyond the OAuth refresh token
    • +
  • No cookies

    • diff --git a/src/app/services/poll.ts b/src/app/services/poll.ts index 94cb53dc..69d63da3 100644 --- a/src/app/services/poll.ts +++ b/src/app/services/poll.ts @@ -96,7 +96,7 @@ async function hasNotificationChanges(): Promise { (err as { status?: number }).status === 403 ) { console.warn("[poll] Notifications API returned 403 — disabling gate"); - pushError("notifications", "Notifications API returned 403 — polling without notification gate", false); + pushError("notifications", "Notifications API returned 403 — check that the notifications scope is granted", false); _notifGateDisabled = true; } return true; diff --git a/src/app/stores/auth.ts b/src/app/stores/auth.ts index 6e1c19f1..481b05ff 100644 --- a/src/app/stores/auth.ts +++ b/src/app/stores/auth.ts @@ -3,6 +3,8 @@ import { clearCache } from "./cache"; import { CONFIG_STORAGE_KEY, resetConfig } from "./config"; import { VIEW_STORAGE_KEY, resetViewState } from "./view"; +export const AUTH_STORAGE_KEY = "github-tracker:auth-token"; + export interface GitHubUser { login: string; avatar_url: string; @@ -13,15 +15,16 @@ interface TokenExchangeResponse { access_token: string; token_type?: string; scope?: string; - expires_in?: number | null; } // ── Signals ───────────────────────────────────────────────────────────────── -// Access token is kept in-memory only (never persisted to localStorage). -// On page reload, refreshAccessToken() uses the HttpOnly refresh token cookie -// to obtain a fresh access token from the Worker. -const [_token, _setToken] = createSignal(null); +// Access token is persisted to localStorage for permanent OAuth App tokens. +// On page reload, validateToken() reads from localStorage and verifies with GitHub. +const [_token, _setToken] = createSignal( + // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition + typeof localStorage !== "undefined" ? (localStorage.getItem?.(AUTH_STORAGE_KEY) ?? null) : null +); const [user, setUser] = createSignal(null); export const token = _token; @@ -35,8 +38,9 @@ export { user }; // ── Actions ───────────────────────────────────────────────────────────────── export function setAuth(response: TokenExchangeResponse): void { + localStorage.setItem(AUTH_STORAGE_KEY, response.access_token); _setToken(response.access_token); - console.info("[auth] access token set (in-memory)"); + console.info("[auth] access token set (localStorage)"); } const _onClearCallbacks: (() => void)[] = []; @@ -52,12 +56,11 @@ export function clearAuth(): void { resetConfig(); resetViewState(); // Clear localStorage entries (persistence effects will write back defaults) + localStorage.removeItem(AUTH_STORAGE_KEY); localStorage.removeItem(CONFIG_STORAGE_KEY); localStorage.removeItem(VIEW_STORAGE_KEY); _setToken(null); setUser(null); - // Clear HttpOnly refresh token cookie via Worker (fire-and-forget) - fetch("/api/oauth/logout", { method: "POST" }).catch(() => {}); // Clear IndexedDB cache to prevent data leakage between users (SDR-016) clearCache().catch(() => { // Non-fatal — cache clear failure should not block logout @@ -69,61 +72,6 @@ export function clearAuth(): void { console.info("[auth] auth cleared"); } -export async function refreshAccessToken(): Promise { - try { - // Refresh token is in an HttpOnly cookie — the browser sends it automatically - const resp = await fetch("/api/oauth/refresh", { - method: "POST", - }); - - if (!resp.ok) { - console.info("[auth] token refresh failed — clearing auth"); - clearAuth(); - return false; - } - - const data = (await resp.json()) as TokenExchangeResponse; - - if (typeof data.access_token !== "string") { - console.info("[auth] token refresh returned invalid response"); - clearAuth(); - return false; - } - - // Validate the new token before setting it (SDR-013) - const validationResp = await fetch("https://api.github.com/user", { - headers: { - Authorization: `Bearer ${data.access_token}`, - Accept: "application/vnd.github+json", - }, - }); - - if (!validationResp.ok) { - console.info("[auth] new token failed validation — clearing auth"); - clearAuth(); - return false; - } - - // Token is valid — set it in memory - setAuth(data); - - // Populate user signal from validation response - const userData = (await validationResp.json()) as { - login: string; - avatar_url: string; - name: string | null; - }; - setUser(userData); - - console.info("[auth] token refresh succeeded"); - return true; - } catch { - console.info("[auth] token refresh error — clearing auth"); - clearAuth(); - return false; - } -} - export async function validateToken(): Promise { const currentToken = _token(); if (!currentToken) return false; @@ -148,12 +96,15 @@ export async function validateToken(): Promise { } if (resp.status === 401) { - console.info("[auth] access token expired — attempting refresh"); - return refreshAccessToken(); + // Permanent token is revoked — clear auth and redirect to login + console.info("[auth] access token invalid — clearing auth"); + clearAuth(); + return false; } return false; } catch { + // Network error — permanent token survives transient failures return false; } } diff --git a/tests/components/App.test.tsx b/tests/components/App.test.tsx index d035b3e0..fe576b0d 100644 --- a/tests/components/App.test.tsx +++ b/tests/components/App.test.tsx @@ -3,17 +3,17 @@ import { render, screen, waitFor } from "@solidjs/testing-library"; // Module-level variables to control mock return values let mockIsAuthenticated = false; -// refreshAccessToken mock fn — replaced per-test -let mockRefreshAccessToken: () => Promise = async () => false; +// validateToken mock fn — replaced per-test +let mockValidateToken: () => Promise = async () => false; -// isAuthenticated/refreshAccessToken are plain functions (not SolidJS signals) because +// isAuthenticated/validateToken are plain functions (not SolidJS signals) because // RootRedirect and AuthGuard read them in onMount (one-shot), not in createEffect. vi.mock("../../src/app/stores/auth", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, isAuthenticated: () => mockIsAuthenticated, - refreshAccessToken: vi.fn(async () => mockRefreshAccessToken()), + validateToken: vi.fn(async () => mockValidateToken()), }; }); @@ -72,7 +72,7 @@ describe("App", () => { // Reset all mock state (implementations, calls, return values) vi.resetAllMocks(); mockIsAuthenticated = false; - mockRefreshAccessToken = async () => false; + mockValidateToken = async () => false; // Re-apply default mock implementations that are needed across tests vi.mocked(cacheStore.evictStaleEntries).mockResolvedValue(0); // Reset config to defaults @@ -84,17 +84,17 @@ describe("App", () => { } }); - it("shows loading spinner during auth refresh", async () => { + it("shows loading spinner during token validation", async () => { mockIsAuthenticated = false; - mockRefreshAccessToken = () => new Promise(() => {}); // never resolves + mockValidateToken = () => new Promise(() => {}); // never resolves render(() => ); screen.getByLabelText("Loading"); }); - it("redirects to /login when not authenticated and refresh fails", async () => { + it("redirects to /login when not authenticated and validation fails", async () => { mockIsAuthenticated = false; - mockRefreshAccessToken = async () => false; + mockValidateToken = async () => false; render(() => ); @@ -104,7 +104,7 @@ describe("App", () => { }); it("redirects to /onboarding when authenticated but onboarding incomplete", async () => { - // isAuthenticated=true → refreshAccessToken NOT called → immediate routing + // isAuthenticated=true → validateToken NOT called → immediate routing mockIsAuthenticated = true; configStore.updateConfig({ onboardingComplete: false }); @@ -116,7 +116,7 @@ describe("App", () => { }); it("redirects to /dashboard when authenticated and onboarding complete", async () => { - // isAuthenticated=true → refreshAccessToken NOT called → immediate routing + // isAuthenticated=true → validateToken NOT called → immediate routing mockIsAuthenticated = true; configStore.updateConfig({ onboardingComplete: true }); diff --git a/tests/components/DashboardPage.test.tsx b/tests/components/DashboardPage.test.tsx index d172d44f..935168a4 100644 --- a/tests/components/DashboardPage.test.tsx +++ b/tests/components/DashboardPage.test.tsx @@ -23,7 +23,6 @@ vi.mock("@solidjs/router", () => ({ // Mock auth store vi.mock("../../src/app/stores/auth", () => ({ - refreshAccessToken: vi.fn().mockResolvedValue(true), clearAuth: vi.fn(), token: () => "fake-token", user: () => ({ login: "testuser", avatar_url: "", name: "Test User" }), @@ -92,8 +91,6 @@ beforeEach(async () => { mockLocationReplace.mockClear(); capturedFetchAll = null; vi.mocked(authStore.clearAuth).mockClear(); - vi.mocked(authStore.refreshAccessToken).mockClear(); - vi.mocked(authStore.refreshAccessToken).mockResolvedValue(true); vi.mocked(pollService.fetchAllData).mockResolvedValue({ issues: [], pullRequests: [], @@ -270,21 +267,10 @@ describe("DashboardPage — auth error handling", () => { consoleErrorSpy.mockRestore(); }); - it("calls refreshAccessToken on 401 error from fetchAllData", async () => { + it("calls clearAuth and redirects to /login on 401 error (permanent token revoked)", async () => { const err401 = Object.assign(new Error("Unauthorized"), { status: 401 }); vi.mocked(pollService.fetchAllData).mockRejectedValue(err401); - render(() => ); - await waitFor(() => { - expect(authStore.refreshAccessToken).toHaveBeenCalledOnce(); - }); - }); - - it("calls clearAuth and navigates to /login when refresh fails", async () => { - const err401 = Object.assign(new Error("Unauthorized"), { status: 401 }); - vi.mocked(pollService.fetchAllData).mockRejectedValue(err401); - vi.mocked(authStore.refreshAccessToken).mockResolvedValue(false); - render(() => ); await waitFor(() => { expect(authStore.clearAuth).toHaveBeenCalledOnce(); @@ -292,20 +278,7 @@ describe("DashboardPage — auth error handling", () => { }); }); - it("does not call clearAuth when refresh succeeds after 401", async () => { - const err401 = Object.assign(new Error("Unauthorized"), { status: 401 }); - vi.mocked(pollService.fetchAllData).mockRejectedValue(err401); - vi.mocked(authStore.refreshAccessToken).mockResolvedValue(true); - - render(() => ); - await waitFor(() => { - expect(authStore.refreshAccessToken).toHaveBeenCalledOnce(); - }); - expect(authStore.clearAuth).not.toHaveBeenCalled(); - expect(mockLocationReplace).not.toHaveBeenCalledWith("/login"); - }); - - it("does not call refreshAccessToken for non-401 errors", async () => { + it("does not call clearAuth for non-401 errors", async () => { const err500 = Object.assign(new Error("Server Error"), { status: 500 }); vi.mocked(pollService.fetchAllData).mockRejectedValue(err500); @@ -313,7 +286,7 @@ describe("DashboardPage — auth error handling", () => { // Flush all pending microtasks so the rejected promise settles await Promise.resolve(); await Promise.resolve(); - expect(authStore.refreshAccessToken).not.toHaveBeenCalled(); + expect(authStore.clearAuth).not.toHaveBeenCalled(); expect(mockLocationReplace).not.toHaveBeenCalledWith("/login"); }); }); diff --git a/tests/components/LoginPage.test.tsx b/tests/components/LoginPage.test.tsx index 87dc604e..eb7f2c97 100644 --- a/tests/components/LoginPage.test.tsx +++ b/tests/components/LoginPage.test.tsx @@ -94,6 +94,15 @@ describe("LoginPage", () => { expect(url.searchParams.get("redirect_uri")).toContain("/oauth/callback"); }); + it("OAuth URL includes scope parameter with required scopes", async () => { + const user = userEvent.setup(); + render(() => ); + const button = screen.getByText("Sign in with GitHub"); + await user.click(button); + const url = new URL(window.location.href); + expect(url.searchParams.get("scope")).toBe("repo read:org notifications"); + }); + it("each login click generates a unique state", async () => { // Render two separate instances to simulate two clicks const { unmount } = render(() => ); diff --git a/tests/components/OAuthCallback.test.tsx b/tests/components/OAuthCallback.test.tsx index 7fb4b1d0..e8e18ac4 100644 --- a/tests/components/OAuthCallback.test.tsx +++ b/tests/components/OAuthCallback.test.tsx @@ -112,13 +112,12 @@ describe("OAuthCallback", () => { }); }); - it("passes token response (access_token, expires_in) to setAuth", async () => { + it("passes token response (access_token, token_type) to setAuth", async () => { setupValidState(); setWindowSearch({ code: "fakecode", state: "teststate" }); const fullResponse = { access_token: "tok123", - expires_in: 28800, token_type: "bearer", }; vi.stubGlobal( diff --git a/tests/setup.ts b/tests/setup.ts new file mode 100644 index 00000000..90488474 --- /dev/null +++ b/tests/setup.ts @@ -0,0 +1,17 @@ +// Global test setup: ensure localStorage is available before module imports. +// auth.ts reads localStorage at module scope (to initialize the token signal from +// persisted value). happy-dom establishes window globals lazily, so this shim +// ensures localStorage exists even during early module initialization. +if (typeof localStorage === "undefined") { + const store: Record = {}; + Object.defineProperty(globalThis, "localStorage", { + value: { + getItem: (key: string) => store[key] ?? null, + setItem: (key: string, value: string) => { store[key] = value; }, + removeItem: (key: string) => { delete store[key]; }, + clear: () => { Object.keys(store).forEach((k) => delete store[k]); }, + }, + writable: true, + configurable: true, + }); +} diff --git a/tests/stores/auth.test.ts b/tests/stores/auth.test.ts index 0dedc34e..3aa3758f 100644 --- a/tests/stores/auth.test.ts +++ b/tests/stores/auth.test.ts @@ -28,8 +28,9 @@ Object.defineProperty(globalThis, "localStorage", { configurable: true, }); -// auth.ts no longer reads localStorage on import — access token is in-memory only. +// auth.ts reads localStorage on import (to initialize token signal from persisted value). // Each describe block uses vi.resetModules() + dynamic import to get clean signal state. +// Tests that seed localStorage must do so BEFORE the dynamic import. describe("setAuth / token signal", () => { let mod: typeof import("../../src/app/stores/auth"); @@ -49,16 +50,24 @@ describe("setAuth / token signal", () => { expect(mod.token()).toBe("ghs_abc123"); }); - it("does not persist access token to localStorage", () => { + it("persists access token to localStorage", () => { mod.setAuth({ access_token: "ghs_abc123" }); - expect(localStorageMock.getItem("github-tracker:auth")).toBeNull(); + expect(localStorageMock.getItem("github-tracker:auth-token")).toBe("ghs_abc123"); }); - it("token signal starts as null on fresh module load", async () => { + it("token signal starts as null on fresh module load with empty localStorage", async () => { + localStorageMock.clear(); vi.resetModules(); const fresh = await import("../../src/app/stores/auth"); expect(fresh.token()).toBeNull(); }); + + it("token signal initializes from localStorage on module load", async () => { + localStorageMock.setItem("github-tracker:auth-token", "ghs_persisted"); + vi.resetModules(); + const fresh = await import("../../src/app/stores/auth"); + expect(fresh.token()).toBe("ghs_persisted"); + }); }); describe("isAuthenticated", () => { @@ -87,8 +96,6 @@ describe("clearAuth", () => { beforeEach(async () => { localStorageMock.clear(); vi.resetModules(); - // Stub fetch so clearAuth's POST /api/oauth/logout doesn't hit the network - vi.stubGlobal("fetch", vi.fn().mockResolvedValue({ ok: true })); mod = await import("../../src/app/stores/auth"); }); @@ -102,6 +109,12 @@ describe("clearAuth", () => { expect(mod.token()).toBeNull(); }); + it("removes auth token from localStorage", () => { + mod.setAuth({ access_token: "ghs_abc" }); + mod.clearAuth(); + expect(localStorageMock.getItem("github-tracker:auth-token")).toBeNull(); + }); + it("removes config and view keys from localStorage (SDR-016)", () => { localStorageMock.setItem("github-tracker:config", "{}"); localStorageMock.setItem("github-tracker:view", "{}"); @@ -110,10 +123,11 @@ describe("clearAuth", () => { expect(localStorageMock.getItem("github-tracker:view")).toBeNull(); }); - it("sends POST to /api/oauth/logout to clear HttpOnly cookie", () => { + it("does not call /api/oauth/logout (OAuth App uses permanent tokens)", () => { + const fetchMock = vi.fn(); + vi.stubGlobal("fetch", fetchMock); mod.clearAuth(); - const fetchMock = vi.mocked(globalThis.fetch); - expect(fetchMock).toHaveBeenCalledWith("/api/oauth/logout", { method: "POST" }); + expect(fetchMock).not.toHaveBeenCalled(); }); }); @@ -123,8 +137,6 @@ describe("onAuthCleared callbacks", () => { beforeEach(async () => { localStorageMock.clear(); vi.resetModules(); - // Stub fetch so clearAuth's POST /api/oauth/logout doesn't hit the network - vi.stubGlobal("fetch", vi.fn().mockResolvedValue({ ok: true })); mod = await import("../../src/app/stores/auth"); }); @@ -202,19 +214,16 @@ describe("validateToken", () => { expect(mod.user()?.name).toBe("Will"); }); - it("clears auth when 401 and refresh cookie is missing/expired", async () => { + it("clears auth on 401 (permanent token revoked — no refresh fallback)", async () => { vi.stubGlobal("fetch", vi.fn() - // GET /user returns 401 (access token expired) + // GET /user returns 401 (access token revoked) .mockResolvedValueOnce({ ok: false, status: 401, json: () => Promise.resolve({}) }) - // POST /api/oauth/refresh returns 400 (no cookie / invalid) - .mockResolvedValueOnce({ ok: false, status: 400, json: () => Promise.resolve({ error: "invalid_request" }) }) - // POST /api/oauth/logout (fire-and-forget from clearAuth) - .mockResolvedValue({ ok: true, json: () => Promise.resolve({}) }) ); - mod.setAuth({ access_token: "ghs_expired" }); + mod.setAuth({ access_token: "ghs_revoked" }); await mod.validateToken(); expect(mod.token()).toBeNull(); + expect(localStorageMock.getItem("github-tracker:auth-token")).toBeNull(); }); it("returns false and leaves token unchanged on non-200/non-401 response (e.g., 503)", async () => { @@ -244,77 +253,18 @@ describe("validateToken", () => { // Token should remain untouched — network error is not an auth failure expect(mod.token()).toBe("ghs_network"); }); -}); - -describe("refreshAccessToken", () => { - afterEach(() => { - vi.restoreAllMocks(); - vi.unstubAllGlobals(); - }); - - it("POSTs to /api/oauth/refresh (cookie-based) and updates token on success", async () => { - const newTokenResponse = { access_token: "ghs_new", expires_in: 3600 }; - const githubUser = { login: "wgordon", avatar_url: "https://avatar.url", name: "Will" }; - vi.stubGlobal("fetch", vi.fn() - // POST /api/oauth/refresh (no body — refresh token is in HttpOnly cookie) - .mockResolvedValueOnce({ ok: true, status: 200, json: () => Promise.resolve(newTokenResponse) }) - // GET https://api.github.com/user (validation) - .mockResolvedValueOnce({ ok: true, status: 200, json: () => Promise.resolve(githubUser) }) - ); - - localStorageMock.clear(); + it("token survives transient network errors (permanent token not cleared on network failure)", async () => { + // Seed localStorage so token initializes on module load + localStorageMock.setItem("github-tracker:auth-token", "ghs_permanent"); vi.resetModules(); - const mod = await import("../../src/app/stores/auth"); - - const result = await mod.refreshAccessToken(); - expect(result).toBe(true); - expect(mod.token()).toBe("ghs_new"); + const freshMod = await import("../../src/app/stores/auth"); - // Verify the refresh request sent no body (cookie-based) - const fetchMock = vi.mocked(globalThis.fetch); - const [refreshUrl, refreshInit] = fetchMock.mock.calls[0]; - expect(refreshUrl).toBe("/api/oauth/refresh"); - expect((refreshInit as RequestInit).body).toBeUndefined(); - - // Verify access token is NOT in localStorage - expect(localStorageMock.getItem("github-tracker:auth")).toBeNull(); - }); - - it("returns false and calls clearAuth() on non-ok refresh response", async () => { - vi.stubGlobal("fetch", vi.fn() - .mockResolvedValue({ ok: false, status: 401, json: () => Promise.resolve({}) }) - ); - - localStorageMock.clear(); - vi.resetModules(); - const mod = await import("../../src/app/stores/auth"); - - const result = await mod.refreshAccessToken(); - expect(result).toBe(false); - expect(mod.token()).toBeNull(); - }); - - it("clears auth when refresh succeeds but validation fails (SDR-013)", async () => { - vi.stubGlobal("fetch", vi.fn() - .mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve({ - access_token: "ghs_unvalidated", - expires_in: 28800, - }), - }) - .mockResolvedValueOnce({ ok: false, status: 401, json: () => Promise.resolve({}) }) - // POST /api/oauth/logout (fire-and-forget from clearAuth) - .mockResolvedValue({ ok: true, json: () => Promise.resolve({}) }) - ); - - localStorageMock.clear(); - vi.resetModules(); - const mod = await import("../../src/app/stores/auth"); + vi.stubGlobal("fetch", vi.fn().mockRejectedValue(new TypeError("Failed to fetch"))); + const result = await freshMod.validateToken(); - const result = await mod.refreshAccessToken(); expect(result).toBe(false); - expect(mod.token()).toBeNull(); + expect(freshMod.token()).toBe("ghs_permanent"); + expect(localStorageMock.getItem("github-tracker:auth-token")).toBe("ghs_permanent"); }); }); diff --git a/tests/worker/oauth.test.ts b/tests/worker/oauth.test.ts index 7afe8878..025a7611 100644 --- a/tests/worker/oauth.test.ts +++ b/tests/worker/oauth.test.ts @@ -16,7 +16,7 @@ function makeEnv(overrides: Partial = {}): Env { function makeRequest( method: string, path: string, - options: { body?: unknown; origin?: string; contentType?: string; cookie?: string } = {} + options: { body?: unknown; origin?: string; contentType?: string } = {} ): Request { const url = `https://gh.gordoncode.dev${path}`; const headers: Record = {}; @@ -28,9 +28,6 @@ function makeRequest( if (options.body !== undefined) { headers["Content-Type"] = options.contentType ?? "application/json"; } - if (options.cookie) { - headers["Cookie"] = options.cookie; - } return new Request(url, { method, headers, @@ -58,15 +55,13 @@ describe("Worker OAuth endpoint", () => { // ── Token exchange ───────────────────────────────────────────────────────── - it("POST /api/oauth/token with valid code returns allowed fields", async () => { + it("POST /api/oauth/token with valid code returns access_token, token_type, scope", async () => { globalThis.fetch = vi.fn().mockResolvedValue( new Response( JSON.stringify({ access_token: "ghu_access123", token_type: "bearer", - scope: "", - refresh_token: "ghr_refresh456", - expires_in: 28800, + scope: "repo read:org notifications", extra_field: "should_not_be_returned", }), { status: 200 } @@ -74,23 +69,20 @@ describe("Worker OAuth endpoint", () => { ); const req = makeRequest("POST", "/api/oauth/token", { body: { code: VALID_CODE } }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.status).toBe(200); const json = await res.json() as Record; expect(json["access_token"]).toBe("ghu_access123"); expect(json["token_type"]).toBe("bearer"); - expect(json["expires_in"]).toBe(28800); - // Refresh token must NOT be in the response body — it's in an HttpOnly cookie - expect(json["refresh_token"]).toBeUndefined(); + // Must not include expires_in (OAuth App tokens are permanent) + expect(json["expires_in"]).toBeUndefined(); // Must not include extra fields expect(json["extra_field"]).toBeUndefined(); - // Refresh token is set as an HttpOnly cookie - const setCookie = res.headers.get("Set-Cookie") ?? ""; - expect(setCookie).toContain("__Host-github_tracker_rt=ghr_refresh456"); - expect(setCookie).toContain("HttpOnly"); - expect(setCookie).toContain("Secure"); - expect(setCookie).toContain("Path=/"); + // Must not set a cookie (no refresh token for OAuth App) + expect(res.headers.get("Set-Cookie")).toBeNull(); + // Must not include Access-Control-Allow-Credentials + expect(res.headers.get("Access-Control-Allow-Credentials")).toBeNull(); }); it("POST /api/oauth/token forwards client_id and client_secret to GitHub", async () => { @@ -102,7 +94,7 @@ describe("Worker OAuth endpoint", () => { globalThis.fetch = mockFetch; const req = makeRequest("POST", "/api/oauth/token", { body: { code: VALID_CODE } }); - await worker.fetch(req, makeEnv(), ); + await worker.fetch(req, makeEnv()); expect(mockFetch).toHaveBeenCalledOnce(); const [url, init] = mockFetch.mock.calls[0] as [string, RequestInit]; @@ -122,7 +114,7 @@ describe("Worker OAuth endpoint", () => { ); const req = makeRequest("POST", "/api/oauth/token", { body: { code: VALID_CODE } }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.status).toBe(400); const json = await res.json() as Record; @@ -134,7 +126,7 @@ describe("Worker OAuth endpoint", () => { it("POST /api/oauth/token with missing code returns 400", async () => { const req = makeRequest("POST", "/api/oauth/token", { body: {} }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.status).toBe(400); const json = await res.json() as Record; expect(json["error"]).toBe("invalid_request"); @@ -153,7 +145,7 @@ describe("Worker OAuth endpoint", () => { for (const code of cases) { const req = makeRequest("POST", "/api/oauth/token", { body: { code } }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.status, `Expected 400 for code: ${code}`).toBe(400); const json = await res.json() as Record; expect(json["error"]).toBe("invalid_request"); @@ -227,7 +219,7 @@ describe("Worker OAuth endpoint", () => { body: { code: VALID_CODE }, contentType: "text/plain", }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.status).toBe(400); const json = await res.json() as Record; expect(json["error"]).toBe("invalid_request"); @@ -237,7 +229,7 @@ describe("Worker OAuth endpoint", () => { globalThis.fetch = vi.fn().mockRejectedValue(new Error("network error")); const req = makeRequest("POST", "/api/oauth/token", { body: { code: VALID_CODE } }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.status).toBe(400); const json = await res.json() as Record; expect(json["error"]).toBe("token_exchange_failed"); @@ -247,86 +239,28 @@ describe("Worker OAuth endpoint", () => { it("GET /api/oauth/token returns 405", async () => { const req = makeRequest("GET", "/api/oauth/token"); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.status).toBe(405); const json = await res.json() as Record; expect(json["error"]).toBe("method_not_allowed"); }); - // ── Refresh endpoint ──────────────────────────────────────────────────────── - - it("POST /api/oauth/refresh with valid cookie returns new access_token and rotates cookie", async () => { - globalThis.fetch = vi.fn().mockResolvedValue( - new Response( - JSON.stringify({ - access_token: "ghu_new_access", - token_type: "bearer", - refresh_token: "ghr_new_refresh", - expires_in: 28800, - }), - { status: 200 } - ) - ); - - const req = makeRequest("POST", "/api/oauth/refresh", { - cookie: "__Host-github_tracker_rt=ghr_old_refresh_token_value", - }); - const res = await worker.fetch(req, makeEnv()); - expect(res.status).toBe(200); - - const json = await res.json() as Record; - expect(json["access_token"]).toBe("ghu_new_access"); - // Refresh token must NOT be in the response body - expect(json["refresh_token"]).toBeUndefined(); - expect(json["expires_in"]).toBe(28800); - // Rotated refresh token is in the Set-Cookie header - const setCookie = res.headers.get("Set-Cookie") ?? ""; - expect(setCookie).toContain("__Host-github_tracker_rt=ghr_new_refresh"); - expect(setCookie).toContain("HttpOnly"); - }); - - it("POST /api/oauth/refresh reads token from cookie and sends grant_type=refresh_token to GitHub", async () => { - const mockFetch = vi.fn().mockResolvedValue( - new Response(JSON.stringify({ access_token: "ghu_new", token_type: "bearer" }), { - status: 200, - }) - ); - globalThis.fetch = mockFetch; + // ── Removed endpoints return 404 ──────────────────────────────────────────── - const req = makeRequest("POST", "/api/oauth/refresh", { - cookie: "__Host-github_tracker_rt=ghr_old_cookie_token_123", - }); - await worker.fetch(req, makeEnv()); - - const [, init] = mockFetch.mock.calls[0] as [string, RequestInit]; - const body = JSON.parse(init.body as string) as Record; - expect(body["grant_type"]).toBe("refresh_token"); - expect(body["refresh_token"]).toBe("ghr_old_cookie_token_123"); - }); - - it("POST /api/oauth/refresh with GitHub error returns 400 with generic error", async () => { - globalThis.fetch = vi.fn().mockResolvedValue( - new Response( - JSON.stringify({ error: "bad_refresh_token", error_description: "Token is expired" }), - { status: 200 } - ) - ); - - const req = makeRequest("POST", "/api/oauth/refresh", { - cookie: "__Host-github_tracker_rt=ghr_" + "a".repeat(20), - }); + it("POST /api/oauth/refresh returns 404 (endpoint removed for OAuth App)", async () => { + const req = makeRequest("POST", "/api/oauth/refresh"); const res = await worker.fetch(req, makeEnv()); - expect(res.status).toBe(400); + expect(res.status).toBe(404); const json = await res.json() as Record; - expect(json["error"]).toBe("token_exchange_failed"); + expect(json["error"]).toBe("not_found"); }); - it("POST /api/oauth/refresh with no cookie returns 400", async () => { - const req = makeRequest("POST", "/api/oauth/refresh"); + it("POST /api/oauth/logout returns 404 (endpoint removed for OAuth App)", async () => { + const req = makeRequest("POST", "/api/oauth/logout"); const res = await worker.fetch(req, makeEnv()); - expect(res.status).toBe(400); + expect(res.status).toBe(404); const json = await res.json() as Record; - expect(json["error"]).toBe("invalid_request"); + expect(json["error"]).toBe("not_found"); }); // ── CORS ──────────────────────────────────────────────────────────────────── @@ -342,10 +276,12 @@ describe("Worker OAuth endpoint", () => { body: { code: VALID_CODE }, origin: ALLOWED_ORIGIN, }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.headers.get("Access-Control-Allow-Origin")).toBe(ALLOWED_ORIGIN); expect(res.headers.get("Access-Control-Allow-Methods")).toBe("POST"); expect(res.headers.get("Access-Control-Allow-Headers")).toBe("Content-Type"); + // No credentials header for OAuth App (no cookies) + expect(res.headers.get("Access-Control-Allow-Credentials")).toBeNull(); }); it("CORS headers are absent for non-matching origin (SDR-004)", async () => { @@ -359,7 +295,7 @@ describe("Worker OAuth endpoint", () => { body: { code: VALID_CODE }, origin: "https://evil.example.com", }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.headers.get("Access-Control-Allow-Origin")).toBeNull(); }); @@ -375,7 +311,7 @@ describe("Worker OAuth endpoint", () => { body: { code: VALID_CODE }, origin: `https://gh.gordoncode.dev.evil.com`, }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.headers.get("Access-Control-Allow-Origin")).toBeNull(); }); @@ -385,115 +321,23 @@ describe("Worker OAuth endpoint", () => { const req = makeRequest("OPTIONS", "/api/oauth/token", { origin: ALLOWED_ORIGIN, }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.status).toBe(204); expect(res.headers.get("Access-Control-Allow-Origin")).toBe(ALLOWED_ORIGIN); }); - it("OPTIONS /api/oauth/refresh returns 204", async () => { - const req = makeRequest("OPTIONS", "/api/oauth/refresh", { - origin: ALLOWED_ORIGIN, - }); - const res = await worker.fetch(req, makeEnv(), ); - expect(res.status).toBe(204); - }); - - // ── qa-7: Refresh endpoint parity tests ──────────────────────────────────── - - it("GET /api/oauth/refresh returns 405", async () => { - const req = makeRequest("GET", "/api/oauth/refresh"); - const res = await worker.fetch(req, makeEnv()); - expect(res.status).toBe(405); - const json = await res.json() as Record; - expect(json["error"]).toBe("method_not_allowed"); - }); - - it("POST /api/oauth/refresh when GitHub fetch fails returns generic error", async () => { - globalThis.fetch = vi.fn().mockRejectedValue(new Error("network failure")); - - const req = makeRequest("POST", "/api/oauth/refresh", { - cookie: "__Host-github_tracker_rt=ghr_" + "b".repeat(20), - }); - const res = await worker.fetch(req, makeEnv()); - expect(res.status).toBe(400); - const json = await res.json() as Record; - expect(json["error"]).toBe("token_exchange_failed"); - // Stack trace must not be in response (SDR-006) - expect(JSON.stringify(json)).not.toContain("Error"); - }); - - it("POST /api/oauth/refresh with invalid cookie token format returns 400 (too short)", async () => { - const mockFetch = vi.fn(); - globalThis.fetch = mockFetch; - - const req = makeRequest("POST", "/api/oauth/refresh", { - cookie: "__Host-github_tracker_rt=ghr_tooshort", - }); - const res = await worker.fetch(req, makeEnv()); - expect(res.status).toBe(400); - const json = await res.json() as Record; - expect(json["error"]).toBe("invalid_request"); - expect(mockFetch).not.toHaveBeenCalled(); - }); - - it("POST /api/oauth/refresh with invalid cookie token format returns 400 (invalid chars)", async () => { - const mockFetch = vi.fn(); - globalThis.fetch = mockFetch; - - const req = makeRequest("POST", "/api/oauth/refresh", { - cookie: "__Host-github_tracker_rt=ghr_invalid-token-with-dashes!!", - }); - const res = await worker.fetch(req, makeEnv()); - expect(res.status).toBe(400); - const json = await res.json() as Record; - expect(json["error"]).toBe("invalid_request"); - expect(mockFetch).not.toHaveBeenCalled(); - }); - - it("POST /api/oauth/refresh with missing ghr_ prefix in cookie returns 400", async () => { - const mockFetch = vi.fn(); - globalThis.fetch = mockFetch; - - const req = makeRequest("POST", "/api/oauth/refresh", { - cookie: "__Host-github_tracker_rt=ghx_" + "a".repeat(20), - }); - const res = await worker.fetch(req, makeEnv()); - expect(res.status).toBe(400); - const json = await res.json() as Record; - expect(json["error"]).toBe("invalid_request"); - expect(mockFetch).not.toHaveBeenCalled(); - }); - - // ── Logout endpoint ────────────────────────────────────────────────────── - - it("POST /api/oauth/logout clears the refresh token cookie", async () => { - const req = makeRequest("POST", "/api/oauth/logout"); - const res = await worker.fetch(req, makeEnv()); - expect(res.status).toBe(200); - const setCookie = res.headers.get("Set-Cookie") ?? ""; - expect(setCookie).toContain("__Host-github_tracker_rt=;"); - expect(setCookie).toContain("Max-Age=0"); - expect(setCookie).toContain("HttpOnly"); - }); - - it("GET /api/oauth/logout returns 405", async () => { - const req = makeRequest("GET", "/api/oauth/logout"); - const res = await worker.fetch(req, makeEnv()); - expect(res.status).toBe(405); - }); - // ── Health and routing ────────────────────────────────────────────────────── it("GET /api/health returns 200 OK", async () => { const req = makeRequest("GET", "/api/health"); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.status).toBe(200); expect(await res.text()).toBe("OK"); }); it("POST /api/unknown returns 404 with predefined error", async () => { const req = makeRequest("POST", "/api/unknown"); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.status).toBe(404); const json = await res.json() as Record; expect(json["error"]).toBe("not_found"); @@ -509,7 +353,7 @@ describe("Worker OAuth endpoint", () => { ); const req = makeRequest("POST", "/api/oauth/token", { body: { code: VALID_CODE } }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.headers.get("X-Content-Type-Options")).toBe("nosniff"); expect(res.headers.get("Referrer-Policy")).toBe("strict-origin-when-cross-origin"); expect(res.headers.get("X-Frame-Options")).toBe("DENY"); @@ -517,14 +361,14 @@ describe("Worker OAuth endpoint", () => { it("Security headers present on error responses", async () => { const req = makeRequest("POST", "/api/oauth/token", { body: {} }); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.headers.get("X-Content-Type-Options")).toBe("nosniff"); expect(res.headers.get("X-Frame-Options")).toBe("DENY"); }); it("Security headers present on health response", async () => { const req = makeRequest("GET", "/api/health"); - const res = await worker.fetch(req, makeEnv(), ); + const res = await worker.fetch(req, makeEnv()); expect(res.headers.get("X-Content-Type-Options")).toBe("nosniff"); }); @@ -533,131 +377,8 @@ describe("Worker OAuth endpoint", () => { it("Non-API requests are forwarded to ASSETS", async () => { const req = new Request("https://gh.gordoncode.dev/index.html"); const assetFetch = vi.fn().mockResolvedValue(new Response("", { status: 200 })); - const res = await worker.fetch(req, makeEnv({ ASSETS: { fetch: assetFetch } }), ); + const res = await worker.fetch(req, makeEnv({ ASSETS: { fetch: assetFetch } })); expect(assetFetch).toHaveBeenCalledOnce(); expect(res.status).toBe(200); }); }); - -// ── qa-6: localhost cookie path ─────────────────────────────────────────────── - -const LOCAL_ORIGIN = "http://localhost:5173"; - -function makeLocalEnv(overrides: Partial = {}): Env { - return { - ASSETS: { fetch: async () => new Response("asset") }, - GITHUB_CLIENT_ID: "test_client_id", - GITHUB_CLIENT_SECRET: "test_client_secret", - ALLOWED_ORIGIN: LOCAL_ORIGIN, - ...overrides, - }; -} - -function makeLocalRequest( - method: string, - path: string, - options: { body?: unknown; cookie?: string } = {} -): Request { - const url = `http://localhost:5173${path}`; - const headers: Record = { "Origin": LOCAL_ORIGIN }; - if (options.body !== undefined) { - headers["Content-Type"] = "application/json"; - } - if (options.cookie) { - headers["Cookie"] = options.cookie; - } - return new Request(url, { - method, - headers, - body: options.body !== undefined ? JSON.stringify(options.body) : undefined, - }); -} - -describe("Worker OAuth endpoint — localhost cookie branch (qa-6)", () => { - let originalFetch: typeof globalThis.fetch; - - beforeEach(() => { - originalFetch = globalThis.fetch; - }); - - afterEach(() => { - globalThis.fetch = originalFetch; - vi.restoreAllMocks(); - }); - - it("token exchange: Set-Cookie uses plain github_tracker_rt name (no __Host- prefix)", async () => { - globalThis.fetch = vi.fn().mockResolvedValue( - new Response( - JSON.stringify({ - access_token: "ghu_local_access", - token_type: "bearer", - scope: "", - refresh_token: "ghr_" + "a".repeat(20), - expires_in: 28800, - }), - { status: 200 } - ) - ); - - const req = makeLocalRequest("POST", "/api/oauth/token", { body: { code: VALID_CODE } }); - const res = await worker.fetch(req, makeLocalEnv()); - expect(res.status).toBe(200); - - const setCookie = res.headers.get("Set-Cookie") ?? ""; - // Must NOT use __Host- prefix - expect(setCookie).not.toContain("__Host-"); - // Must use the plain cookie name - expect(setCookie).toContain("github_tracker_rt=ghr_"); - // Must NOT contain Secure attribute (localhost is not HTTPS) - expect(setCookie).not.toContain("Secure"); - // Must use SameSite=Lax (not Strict) - expect(setCookie).toContain("SameSite=Lax"); - }); - - it("token exchange: Set-Cookie contains Path=/", async () => { - globalThis.fetch = vi.fn().mockResolvedValue( - new Response( - JSON.stringify({ - access_token: "ghu_local_access", - token_type: "bearer", - scope: "", - refresh_token: "ghr_" + "b".repeat(20), - expires_in: 28800, - }), - { status: 200 } - ) - ); - - const req = makeLocalRequest("POST", "/api/oauth/token", { body: { code: VALID_CODE } }); - const res = await worker.fetch(req, makeLocalEnv()); - const setCookie = res.headers.get("Set-Cookie") ?? ""; - expect(setCookie).toContain("Path=/"); - }); - - it("refresh: reads cookie from plain github_tracker_rt name and rotates it correctly", async () => { - const refreshToken = "ghr_" + "c".repeat(20); - globalThis.fetch = vi.fn().mockResolvedValue( - new Response( - JSON.stringify({ - access_token: "ghu_local_new", - token_type: "bearer", - refresh_token: "ghr_" + "d".repeat(20), - expires_in: 28800, - }), - { status: 200 } - ) - ); - - const req = makeLocalRequest("POST", "/api/oauth/refresh", { - cookie: `github_tracker_rt=${refreshToken}`, - }); - const res = await worker.fetch(req, makeLocalEnv()); - expect(res.status).toBe(200); - - const setCookie = res.headers.get("Set-Cookie") ?? ""; - expect(setCookie).toContain("github_tracker_rt=ghr_"); - expect(setCookie).not.toContain("__Host-"); - expect(setCookie).not.toContain("Secure"); - expect(setCookie).toContain("SameSite=Lax"); - }); -}); diff --git a/vitest.config.ts b/vitest.config.ts index 23385360..5153a393 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -10,6 +10,7 @@ export default defineConfig({ test: { environment: "happy-dom", globals: true, + setupFiles: ["tests/setup.ts"], include: ["tests/**/*.test.ts", "tests/**/*.test.tsx"], exclude: ["tests/worker/**"], passWithNoTests: true, diff --git a/wrangler.toml b/wrangler.toml index 03c3d126..479994c6 100644 --- a/wrangler.toml +++ b/wrangler.toml @@ -2,7 +2,6 @@ name = "github-tracker" main = "src/worker/index.ts" compatibility_date = "2026-03-01" workers_dev = false -preview_urls = true [assets] # The Cloudflare Vite plugin overrides this directory at build time. From fd0f9e72439e09118c1da2c310fa869eac4c316a Mon Sep 17 00:00:00 2001 From: testvalue Date: Tue, 24 Mar 2026 13:31:33 -0400 Subject: [PATCH 03/22] docs: updates documentation for OAuth App migration --- .env.example | 4 +- DEPLOY.md | 134 ++++++++++++++++++++------------------------------- README.md | 11 +++-- 3 files changed, 60 insertions(+), 89 deletions(-) diff --git a/.env.example b/.env.example index 9fb11745..29423e15 100644 --- a/.env.example +++ b/.env.example @@ -1,4 +1,4 @@ -# GitHub App client ID — embedded into client-side bundle at build time by Vite. +# GitHub OAuth App client ID — embedded into client-side bundle at build time by Vite. # This is public information (visible in the OAuth authorize URL). # Set this as a GitHub Actions variable (not a secret) for CI/CD. -VITE_GITHUB_CLIENT_ID=your_github_app_client_id_here +VITE_GITHUB_CLIENT_ID=your_oauth_app_client_id_here diff --git a/DEPLOY.md b/DEPLOY.md index 56782b51..11d63cfa 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -20,63 +20,38 @@ ### Variables (GitHub repo → Settings → Secrets and variables → Actions → Variables) **`VITE_GITHUB_CLIENT_ID`** -- This is the GitHub App Client ID (not a secret — it is embedded in the built JS bundle) +- This is the GitHub OAuth App Client ID (not a secret — it is embedded in the built JS bundle) - Add it as an Actions **variable** (not a secret) -- See GitHub App setup below for how to obtain it +- See OAuth App setup below for how to obtain it -## GitHub App Setup +## GitHub OAuth App Setup -1. Go to GitHub → Settings → Developer settings → GitHub Apps → **New GitHub App** -2. Fill in the basic details: - - **App name**: your app name (e.g. `gh-tracker-yourname`) - - **Description**: `Personal dashboard for tracking GitHub issues, PRs, and Actions runs across repos and orgs.` +1. Go to GitHub → Settings → Developer settings → OAuth Apps → **New OAuth App** +2. Fill in the details: + - **Application name**: your app name (e.g. `gh-tracker-yourname`) - **Homepage URL**: `https://gh.gordoncode.dev` -3. Under **Identifying and authorizing users**: - - **Callback URLs** — register all three: - - `https://gh.gordoncode.dev/oauth/callback` (production) - - `https://github-tracker..workers.dev/oauth/callback` (preview — GitHub's subdomain matching should allow per-branch preview aliases like `alias.github-tracker..workers.dev` to work; verify after first preview deploy) - - `http://localhost:5173/oauth/callback` (local dev) - - ✅ **Expire user authorization tokens** — check this. The app uses short-lived access tokens (8hr) with HttpOnly cookie-based refresh token rotation. - - ✅ **Request user authorization (OAuth) during installation** — check this. Streamlines the install + authorize flow into one step. -4. Under **Post installation**: - - Leave **Setup URL** blank - - Leave **Redirect on update** unchecked -5. Under **Webhook**: - - ❌ Uncheck **Active** — the app polls; it does not use webhooks. -6. Under **Permissions**: - - **Repository permissions** (read-only): - - | Permission | Access | Used for | - |------------|--------|----------| - | **Actions** | Read-only | `GET /repos/{owner}/{repo}/actions/runs` — workflow run list | - | **Checks** | Read-only | `GET /repos/{owner}/{repo}/commits/{ref}/check-runs` — PR check status (REST fallback) | - | **Commit statuses** | Read-only | `GET /repos/{owner}/{repo}/commits/{ref}/status` — legacy commit status (REST fallback) | - | **Issues** | Read-only | `GET /search/issues?q=is:issue` — issue search | - | **Metadata** | Read-only | Automatically granted when any repo permission is set. Required for basic repo info. | - | **Pull requests** | Read-only | `GET /search/issues?q=is:pr`, `GET /repos/{owner}/{repo}/pulls/{pull_number}`, `/reviews` — PR search, detail, and reviews | - - **Organization permissions:** - - | Permission | Access | Used for | - |------------|--------|----------| - | **Members** | Read-only | `GET /user/orgs` — list user's organizations for the org selector | - - **Account permissions:** - - | Permission | Access | Used for | - |------------|--------|----------| - | _(none required)_ | | | - -7. Under **Where can this GitHub App be installed?**: - - **Any account** — the app uses OAuth authorization (not installation tokens), so any GitHub user needs to be able to authorize via the login flow -8. Click **Create GitHub App** -9. Note the **Client ID** — this is your `VITE_GITHUB_CLIENT_ID` -10. Click **Generate a new client secret** and save it for the Worker secrets below - -### Notifications API limitation - -The GitHub Notifications API (`GET /notifications`) does not support GitHub App user access tokens — only classic personal access tokens. The app uses notifications as a polling optimization gate (skip full fetch when nothing changed). When the notifications endpoint returns 403, the gate **auto-disables** and the app falls back to time-based polling. No functionality is lost; polling is just slightly less efficient. + - **Authorization callback URL**: `https://gh.gordoncode.dev/oauth/callback` +3. Click **Register application** +4. Note the **Client ID** — this is your `VITE_GITHUB_CLIENT_ID` +5. Click **Generate a new client secret** and save it for the Worker secrets below + +### Scopes + +The login flow requests `scope=repo read:org notifications`: + +| Scope | Used for | +|-------|----------| +| `repo` | Read issues, PRs, check runs, workflow runs (includes private repos) | +| `read:org` | `GET /user/orgs` — list user's organizations for the org selector | +| `notifications` | `GET /notifications` — polling optimization gate (304 = skip full fetch) | + +**Note:** The `repo` scope grants write access to repositories, but this app never performs write operations (POST/PUT/PATCH/DELETE on repo endpoints). It is read-only by design. + +### Local development OAuth App + +Create a second OAuth App for local development: +- **Authorization callback URL**: `http://localhost:5173/oauth/callback` +- Set its Client ID and Secret in `.dev.vars` (see Local Development below) ## Cloudflare Worker Secrets @@ -91,41 +66,30 @@ wrangler secret put ALLOWED_ORIGIN ``` - `GITHUB_CLIENT_ID`: same value as `VITE_GITHUB_CLIENT_ID` -- `GITHUB_CLIENT_SECRET`: the Client Secret from your GitHub App +- `GITHUB_CLIENT_SECRET`: the Client Secret from your GitHub OAuth App - `ALLOWED_ORIGIN`: `https://gh.gordoncode.dev` -### Preview versions - -Preview deployments use `wrangler versions upload` (not a separate environment), so they inherit production secrets automatically. No additional secret configuration is needed. - -CORS note: Preview URLs are same-origin (SPA and API share the same `*.workers.dev` host), so the `ALLOWED_ORIGIN` strict-equality check is irrelevant — browsers don't enforce CORS on same-origin requests. - -**Migration note:** If you previously deployed with `wrangler deploy --env preview`, an orphaned `github-tracker-preview` worker may still exist. Delete it via `wrangler delete --name github-tracker-preview` or through the Cloudflare dashboard. - ## Worker API Endpoints -| Endpoint | Method | Auth | Purpose | -|----------|--------|------|---------| -| `/api/oauth/token` | POST | none | Exchange OAuth code for access token. Refresh token set as HttpOnly cookie. | -| `/api/oauth/refresh` | POST | cookie | Refresh expired access token. Reads `github_tracker_rt` HttpOnly cookie. Sets rotated cookie. | -| `/api/oauth/logout` | POST | none | Clears the `github_tracker_rt` HttpOnly cookie (`Max-Age=0`). | -| `/api/health` | GET | none | Health check. Returns `OK`. | +| Endpoint | Method | Purpose | +|----------|--------|---------| +| `/api/oauth/token` | POST | Exchange OAuth authorization code for permanent access token. | +| `/api/health` | GET | Health check. Returns `OK`. | -### Refresh Token Security +### Token Storage Security -The refresh token (6-month lifetime) is stored as an **HttpOnly cookie** — never in `localStorage` or the response body. This protects the high-value long-lived credential from XSS: +The OAuth App access token is a permanent credential (no expiry). It is stored in `localStorage` under the key `github-tracker:auth-token`: -- Production cookie: `__Host-github_tracker_rt` with `HttpOnly; Secure; SameSite=Strict; Path=/` -- Local dev: `github_tracker_rt` with `HttpOnly; SameSite=Lax; Path=/` (no `Secure` — localhost is HTTP; no `__Host-` prefix — requires `Secure`) -- The short-lived access token (8hr) is held in-memory only (never persisted to `localStorage`); on page reload, `refreshAccessToken()` obtains a fresh token via the cookie -- On logout, the client calls `POST /api/oauth/logout` to clear the cookie -- GitHub rotates the refresh token on each use; the Worker sets the new value as a cookie +- **CSP protects against XSS token theft**: `script-src 'self'` prevents injection of unauthorized scripts that could read `localStorage` +- On page load, `validateToken()` calls `GET /user` to verify the token is still valid +- On 401, the app immediately clears auth and redirects to login (token is revoked, not expired) +- On logout, the token is removed from `localStorage` and all local state is cleared +- Transient network errors do NOT clear the token (permanent tokens survive connectivity issues) ### CORS - `Access-Control-Allow-Origin`: exact match against `ALLOWED_ORIGIN` (no wildcards) -- `Access-Control-Allow-Credentials: true`: enables cookie-based refresh for cross-origin preview deploys -- Same-origin requests (production, local dev) send cookies automatically without CORS +- No `Access-Control-Allow-Credentials` header (OAuth App uses no cookies) ## Local Development @@ -138,9 +102,15 @@ pnpm run build wrangler deploy ``` -For preview (uploads a version without promoting to production): +## Migration from GitHub App -```sh -pnpm run build -wrangler versions upload --preview-alias my-feature -``` +If you previously deployed with the GitHub App model (HttpOnly cookie refresh tokens), follow these steps: + +1. **Update GitHub Actions variable**: change `VITE_GITHUB_CLIENT_ID` to your OAuth App's Client ID +2. **Update Cloudflare secrets**: re-run `wrangler secret put GITHUB_CLIENT_ID` and `GITHUB_CLIENT_SECRET` with OAuth App values +3. **Update `ALLOWED_ORIGIN`** if it changed (usually unchanged) +4. **Redeploy** the Worker: `pnpm run build && wrangler deploy` +5. **Existing users** will be logged out on next page load (their refresh cookie is no longer valid; they will be prompted to log in again via the new OAuth App flow) +6. **Delete the old GitHub App** (optional): GitHub → Settings → Developer settings → GitHub Apps → your app → Advanced → Delete + +The old `POST /api/oauth/refresh` and `POST /api/oauth/logout` endpoints no longer exist and return 404. diff --git a/README.md b/README.md index 18f56753..c5f9dccb 100644 --- a/README.md +++ b/README.md @@ -52,14 +52,14 @@ src/ github.ts # Octokit client factory with ETag caching and rate limit tracking poll.ts # Poll coordinator with visibility-aware auto-refresh stores/ - auth.ts # OAuth token management with auto-refresh + auth.ts # OAuth token management (localStorage persistence, validateToken) cache.ts # IndexedDB cache with TTL eviction and ETag support config.ts # Zod v4-validated config with localStorage persistence view.ts # View state (tabs, sorting, ignored items, filters) lib/ notifications.ts # Desktop notification permission, detection, and dispatch worker/ - index.ts # OAuth token exchange/refresh endpoint, CORS, security headers + index.ts # OAuth token exchange endpoint, CORS, security headers tests/ fixtures/ # GitHub API response fixtures (orgs, repos, issues, PRs, runs) services/ # API service, Octokit client, and poll coordinator tests @@ -74,10 +74,11 @@ tests/ - Strict CSP: `script-src 'self'` (SHA-256 exception for dark mode script only) - OAuth CSRF protection via `crypto.getRandomValues` state parameter - CORS locked to exact origin (strict equality, no substring matching) -- Access token in-memory only (never persisted); refresh token in `__Host-` HttpOnly cookie -- Auto-refresh on 401 and on page load via HttpOnly cookie +- Access token stored in `localStorage` under app-specific key; CSP prevents XSS token theft +- Token validation on page load via `GET /user`; 401 clears auth immediately (no silent refresh) - All GitHub API strings auto-escaped by SolidJS JSX (no innerHTML) +- `repo` scope granted (required for private repos) — app never performs write operations ## Deployment -See [DEPLOY.md](./DEPLOY.md) for Cloudflare, GitHub App, and CI/CD setup. +See [DEPLOY.md](./DEPLOY.md) for Cloudflare, OAuth App, and CI/CD setup. From 3b6ded309616caae7226b72f0e2cceac9fe8954c Mon Sep 17 00:00:00 2001 From: testvalue Date: Tue, 24 Mar 2026 13:44:02 -0400 Subject: [PATCH 04/22] fix: addresses review findings from security/QA/structural review --- .../components/dashboard/DashboardPage.tsx | 24 ++++++++++++++++--- src/app/components/settings/SettingsPage.tsx | 2 +- src/app/pages/PrivacyPage.tsx | 7 ++---- src/app/services/poll.ts | 9 ++++++- src/worker/index.ts | 4 ++-- tests/components/DashboardPage.test.tsx | 3 +++ 6 files changed, 37 insertions(+), 12 deletions(-) diff --git a/src/app/components/dashboard/DashboardPage.tsx b/src/app/components/dashboard/DashboardPage.tsx index 2c91ffed..1ae93b4e 100644 --- a/src/app/components/dashboard/DashboardPage.tsx +++ b/src/app/components/dashboard/DashboardPage.tsx @@ -10,7 +10,7 @@ import { config } from "../../stores/config"; import { viewState, updateViewState } from "../../stores/view"; import type { Issue, PullRequest, WorkflowRun, ApiError } from "../../services/api"; import { createPollCoordinator, fetchAllData } from "../../services/poll"; -import { clearAuth, user } from "../../stores/auth"; +import { clearAuth, user, onAuthCleared } from "../../stores/auth"; import { getErrors, dismissError } from "../../lib/errors"; import ErrorBannerList from "../shared/ErrorBannerList"; @@ -25,13 +25,30 @@ interface DashboardStore { lastRefreshedAt: Date | null; } -const [dashboardData, setDashboardData] = createStore({ +const initialDashboardState: DashboardStore = { issues: [], pullRequests: [], workflowRuns: [], errors: [], loading: true, lastRefreshedAt: null, +}; + +const [dashboardData, setDashboardData] = createStore({ ...initialDashboardState }); + +function resetDashboardData(): void { + setDashboardData({ ...initialDashboardState }); +} + +// Clear dashboard data on logout so user A's data doesn't show to user B +onAuthCleared(resetDashboardData); + +// Stop polling on logout so the coordinator doesn't keep firing with a null token +onAuthCleared(() => { + if (_coordinator) { + _coordinator.destroy(); + _coordinator = null; + } }); async function pollFetch(): Promise { @@ -62,7 +79,8 @@ async function pollFetch(): Promise : null; if (status === 401) { - // Permanent token is revoked — clear auth and redirect to login + // Hard redirect (not navigate()) forces a full page reload, which clears + // module-level state like _coordinator and dashboardData for the next user. clearAuth(); window.location.replace("/login"); } diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index c1a36001..c06ae5c1 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -251,7 +251,7 @@ export default function SettingsPage() { URL.revokeObjectURL(url); } - async function handleResetAll() { + function handleResetAll() { if (!confirmReset()) { setConfirmReset(true); return; diff --git a/src/app/pages/PrivacyPage.tsx b/src/app/pages/PrivacyPage.tsx index 70024941..92939f02 100644 --- a/src/app/pages/PrivacyPage.tsx +++ b/src/app/pages/PrivacyPage.tsx @@ -25,16 +25,13 @@ export default function PrivacyPage() {
    • localStorage — your settings (selected orgs, - repos, theme, etc.) and view state (tab filters, sort order). + repos, theme, etc.), view state (tab filters, sort order), and + OAuth access token for GitHub API authentication. Cleared on logout.
    • IndexedDB — cached API responses with ETags to reduce GitHub API usage. Cleared on logout.
      • -
    • - localStorage — your OAuth access token, used to - authenticate GitHub API requests. Cleared on logout. -

    diff --git a/src/app/services/poll.ts b/src/app/services/poll.ts index 69d63da3..f3c32854 100644 --- a/src/app/services/poll.ts +++ b/src/app/services/poll.ts @@ -30,6 +30,7 @@ export interface PollCoordinator { isRefreshing: () => boolean; lastRefreshAt: () => Date | null; manualRefresh: () => void; + destroy: () => void; } // ── Notifications gate ─────────────────────────────────────────────────────── @@ -330,5 +331,11 @@ export function createPollCoordinator( } } - return { isRefreshing, lastRefreshAt, manualRefresh }; + function destroy(): void { + destroyed = true; + clearTimer(); + document.removeEventListener("visibilitychange", handleVisibilityChange); + } + + return { isRefreshing, lastRefreshAt, manualRefresh, destroy }; } diff --git a/src/worker/index.ts b/src/worker/index.ts index 1ca6b0b8..04969d50 100644 --- a/src/worker/index.ts +++ b/src/worker/index.ts @@ -145,8 +145,8 @@ export default { const origin = request.headers.get("Origin"); const cors = getCorsHeaders(origin, env.ALLOWED_ORIGIN); - // CORS preflight - if (request.method === "OPTIONS" && url.pathname.startsWith("/api/")) { + // CORS preflight for the token exchange endpoint only + if (request.method === "OPTIONS" && url.pathname === "/api/oauth/token") { return new Response(null, { status: 204, headers: { ...cors, "Access-Control-Max-Age": "86400", ...securityHeaders() }, diff --git a/tests/components/DashboardPage.test.tsx b/tests/components/DashboardPage.test.tsx index 935168a4..0da6fcc1 100644 --- a/tests/components/DashboardPage.test.tsx +++ b/tests/components/DashboardPage.test.tsx @@ -77,6 +77,7 @@ beforeEach(async () => { isRefreshing: () => false, lastRefreshAt: () => null, manualRefresh: vi.fn(), + destroy: vi.fn(), }; } ), @@ -105,6 +106,7 @@ beforeEach(async () => { isRefreshing: () => false, lastRefreshAt: () => null, manualRefresh: vi.fn(), + destroy: vi.fn(), }; } ); @@ -229,6 +231,7 @@ describe("DashboardPage — data flow", () => { isRefreshing: () => true, lastRefreshAt: () => null, manualRefresh: vi.fn(), + destroy: vi.fn(), }); // fetchAllData never resolves vi.mocked(pollService.fetchAllData).mockReturnValue(new Promise(() => {})); From 5e3eb3a92d8bf0a01863eb34a66343208ad7c97e Mon Sep 17 00:00:00 2001 From: testvalue Date: Tue, 24 Mar 2026 13:46:47 -0400 Subject: [PATCH 05/22] fix: adds scope comment and clearAuth reentrancy guard --- src/app/pages/LoginPage.tsx | 1 + src/app/stores/auth.ts | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/app/pages/LoginPage.tsx b/src/app/pages/LoginPage.tsx index 23b01c1f..44fd0e1a 100644 --- a/src/app/pages/LoginPage.tsx +++ b/src/app/pages/LoginPage.tsx @@ -16,6 +16,7 @@ export default function LoginPage() { client_id: clientId, redirect_uri: redirectUri, state, + // repo: read issues/PRs; read:org: list orgs; notifications: gate scope: "repo read:org notifications", }); diff --git a/src/app/stores/auth.ts b/src/app/stores/auth.ts index 481b05ff..3ea66651 100644 --- a/src/app/stores/auth.ts +++ b/src/app/stores/auth.ts @@ -50,7 +50,11 @@ export function onAuthCleared(cb: () => void): void { _onClearCallbacks.push(cb); } +let _clearing = false; + export function clearAuth(): void { + if (_clearing) return; + _clearing = true; // Reset in-memory stores to defaults BEFORE clearing localStorage, // so the persistence effects re-write defaults (not stale user data). resetConfig(); @@ -70,6 +74,7 @@ export function clearAuth(): void { try { cb(); } catch (e) { console.warn("[auth] onAuthCleared callback threw:", e); } } console.info("[auth] auth cleared"); + _clearing = false; } export async function validateToken(): Promise { From f206675cf955925ea8066100b8cc9b0ecdac8798 Mon Sep 17 00:00:00 2001 From: testvalue Date: Tue, 24 Mar 2026 14:24:17 -0400 Subject: [PATCH 06/22] fix(auth): wraps clearAuth reentrancy guard in try/finally --- src/app/stores/auth.ts | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/src/app/stores/auth.ts b/src/app/stores/auth.ts index 3ea66651..588a6529 100644 --- a/src/app/stores/auth.ts +++ b/src/app/stores/auth.ts @@ -55,26 +55,29 @@ let _clearing = false; export function clearAuth(): void { if (_clearing) return; _clearing = true; - // Reset in-memory stores to defaults BEFORE clearing localStorage, - // so the persistence effects re-write defaults (not stale user data). - resetConfig(); - resetViewState(); - // Clear localStorage entries (persistence effects will write back defaults) - localStorage.removeItem(AUTH_STORAGE_KEY); - localStorage.removeItem(CONFIG_STORAGE_KEY); - localStorage.removeItem(VIEW_STORAGE_KEY); - _setToken(null); - setUser(null); - // Clear IndexedDB cache to prevent data leakage between users (SDR-016) - clearCache().catch(() => { - // Non-fatal — cache clear failure should not block logout - }); - // Run registered cleanup callbacks (e.g., poll state reset) - for (const cb of _onClearCallbacks) { - try { cb(); } catch (e) { console.warn("[auth] onAuthCleared callback threw:", e); } + try { + // Reset in-memory stores to defaults BEFORE clearing localStorage, + // so the persistence effects re-write defaults (not stale user data). + resetConfig(); + resetViewState(); + // Clear localStorage entries (persistence effects will write back defaults) + localStorage.removeItem(AUTH_STORAGE_KEY); + localStorage.removeItem(CONFIG_STORAGE_KEY); + localStorage.removeItem(VIEW_STORAGE_KEY); + _setToken(null); + setUser(null); + // Clear IndexedDB cache to prevent data leakage between users (SDR-016) + clearCache().catch(() => { + // Non-fatal — cache clear failure should not block logout + }); + // Run registered cleanup callbacks (e.g., poll state reset) + for (const cb of _onClearCallbacks) { + try { cb(); } catch (e) { console.warn("[auth] onAuthCleared callback threw:", e); } + } + console.info("[auth] auth cleared"); + } finally { + _clearing = false; } - console.info("[auth] auth cleared"); - _clearing = false; } export async function validateToken(): Promise { From 885a783da72c124f7ae81200a88b1755f4a7af15 Mon Sep 17 00:00:00 2001 From: testvalue Date: Tue, 24 Mar 2026 14:31:20 -0400 Subject: [PATCH 07/22] refactor: removes void wrapper, adds try/finally guard --- src/app/components/settings/SettingsPage.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index c06ae5c1..a3b667fe 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -637,7 +637,7 @@ export default function SettingsPage() { Are you sure?