diff --git a/src/app/components/onboarding/OnboardingWizard.tsx b/src/app/components/onboarding/OnboardingWizard.tsx index 2b3fd1fc..96a1022c 100644 --- a/src/app/components/onboarding/OnboardingWizard.tsx +++ b/src/app/components/onboarding/OnboardingWizard.tsx @@ -1,29 +1,57 @@ -import { createSignal, Show } from "solid-js"; +import { + createSignal, + createMemo, + onMount, + Show, + Switch, + Match, +} from "solid-js"; import { config, updateConfig, CONFIG_STORAGE_KEY } from "../../stores/config"; -import { RepoRef } from "../../services/api"; -import OrgSelector from "./OrgSelector"; +import { fetchOrgs, type OrgEntry, type RepoRef } from "../../services/api"; +import { getClient } from "../../services/github"; import RepoSelector from "./RepoSelector"; - -const STEPS = ["Select Organizations", "Select Repositories"] as const; +import LoadingSpinner from "../shared/LoadingSpinner"; export default function OnboardingWizard() { - const [step, setStep] = createSignal(0); - const [selectedOrgs, setSelectedOrgs] = createSignal( - config.selectedOrgs.length > 0 ? [...config.selectedOrgs] : [] - ); const [selectedRepos, setSelectedRepos] = createSignal( config.selectedRepos.length > 0 ? [...config.selectedRepos] : [] ); - function handleNext() { - if (step() === 0) { - updateConfig({ selectedOrgs: selectedOrgs() }); - setStep(1); + const [loading, setLoading] = createSignal(true); + const [error, setError] = createSignal(null); + const [orgEntries, setOrgEntries] = createSignal([]); + + const allOrgLogins = createMemo(() => orgEntries().map((o) => o.login)); + + async function loadOrgs() { + setLoading(true); + setError(null); + try { + const client = getClient(); + if (!client) throw new Error("No GitHub client available"); + const result = await fetchOrgs(client); + setOrgEntries(result); + } catch (err) { + setError( + err instanceof Error ? err.message : "Failed to load organizations" + ); + } finally { + setLoading(false); } } + onMount(() => { + if (config.onboardingComplete) { + window.location.replace("/dashboard"); + return; + } + void loadOrgs(); + }); + function handleFinish() { + const uniqueOrgs = [...new Set(selectedRepos().map((r) => r.owner))]; updateConfig({ + selectedOrgs: uniqueOrgs, selectedRepos: selectedRepos(), onboardingComplete: true, }); @@ -32,15 +60,6 @@ export default function OnboardingWizard() { window.location.replace("/dashboard"); } - function handleBack() { - setStep((s) => Math.max(0, s - 1)); - } - - const canProceed = () => { - if (step() === 0) return selectedOrgs().length > 0; - return selectedRepos().length > 0; - }; - return (
@@ -50,136 +69,66 @@ export default function OnboardingWizard() { GitHub Tracker Setup

- Step {step() + 1} of {STEPS.length} + Select the repositories you want to track.

- {/* Step indicator */} - - - {/* Step content */} + {/* Content */}
- -
-

- Select Organizations -

-

- Choose the GitHub organizations and personal account to track. -

-
- - - - -
-

- Select Repositories -

-

- Choose which repositories to track within your selected - organizations. -

-
- - + + +
+

+ {error()} +

+ +
+ + +
+ +
+ + +
+

+ Select Repositories +

+

+ Choose which repositories to track. +

+
+ + +
- {/* Navigation buttons */} -
- 0} - fallback={
} - > - - - - - Next - - } - > + {/* Navigation buttons — hidden during loading/error to avoid confusion */} + +
- -
+
+
); diff --git a/src/app/components/onboarding/RepoSelector.tsx b/src/app/components/onboarding/RepoSelector.tsx index e9bae88b..c8492606 100644 --- a/src/app/components/onboarding/RepoSelector.tsx +++ b/src/app/components/onboarding/RepoSelector.tsx @@ -2,7 +2,6 @@ import { createSignal, createEffect, createMemo, - For, Show, Index, } from "solid-js"; @@ -14,6 +13,7 @@ import FilterInput from "../shared/FilterInput"; interface RepoSelectorProps { selectedOrgs: string[]; + orgEntries?: OrgEntry[]; // Pre-fetched org entries — skip internal fetchOrgs when provided selected: RepoRef[]; onChange: (selected: RepoRef[]) => void; } @@ -30,10 +30,19 @@ export default function RepoSelector(props: RepoSelectorProps) { const [filter, setFilter] = createSignal(""); const [orgStates, setOrgStates] = createSignal([]); const [loadedCount, setLoadedCount] = createSignal(0); + let effectVersion = 0; // Initialize org states and fetch repos on mount / when selectedOrgs change createEffect(() => { const orgs = props.selectedOrgs; + // Capture orgEntries synchronously so SolidJS tracks it as a reactive + // dependency. Reading it inside the async IIFE below would be fragile — + // it happens to work today (before any await) but would silently break + // if the check were moved after an await. + const preloadedEntries = props.orgEntries; + // Version counter: if selectedOrgs changes while fetches are in-flight, + // stale callbacks check this and bail out instead of writing to state. + const version = ++effectVersion; if (orgs.length === 0) { setOrgStates([]); setLoadedCount(0); @@ -69,15 +78,21 @@ export default function RepoSelector(props: RepoSelectorProps) { // Fetch org type info first, then repos incrementally void (async () => { - let orgEntries: OrgEntry[] = []; - try { - orgEntries = await fetchOrgs(client); - } catch { - // If fetchOrgs fails, treat all as "org" type + let entries: OrgEntry[]; + if (preloadedEntries != null) { + entries = preloadedEntries; + } else { + try { + entries = await fetchOrgs(client); + } catch { + entries = []; + } } + if (version !== effectVersion) return; + const typeMap = new Map( - orgEntries.map((e) => [e.login, e.type]) + entries.map((e) => [e.login, e.type]) ); // Fetch repos for each org independently so results trickle in @@ -85,12 +100,14 @@ export default function RepoSelector(props: RepoSelectorProps) { const type = typeMap.get(org) ?? "org"; try { const repos = await fetchRepos(client, org, type); + if (version !== effectVersion) return; setOrgStates((prev) => prev.map((s) => s.org === org ? { ...s, type, repos, loading: false } : s ) ); } catch (err) { + if (version !== effectVersion) return; const message = err instanceof Error ? err.message : "Failed to load repositories"; setOrgStates((prev) => @@ -101,7 +118,9 @@ export default function RepoSelector(props: RepoSelectorProps) { ) ); } finally { - setLoadedCount((c) => c + 1); + if (version === effectVersion) { + setLoadedCount((c) => c + 1); + } } }); @@ -113,6 +132,8 @@ export default function RepoSelector(props: RepoSelectorProps) { const client = getClient(); if (!client) return; + const version = effectVersion; + setOrgStates((prev) => prev.map((s) => s.org === org ? { ...s, loading: true, error: null } : s @@ -124,6 +145,7 @@ export default function RepoSelector(props: RepoSelectorProps) { void fetchRepos(client, org, type) .then((repos) => { + if (version !== effectVersion) return; setOrgStates((prev) => prev.map((s) => s.org === org ? { ...s, repos, loading: false, error: null } : s @@ -131,6 +153,7 @@ export default function RepoSelector(props: RepoSelectorProps) { ); }) .catch((err) => { + if (version !== effectVersion) return; const message = err instanceof Error ? err.message : "Failed to load repositories"; setOrgStates((prev) => @@ -184,7 +207,7 @@ export default function RepoSelector(props: RepoSelectorProps) { // ── Filtering ────────────────────────────────────────────────────────────── - const q = () => filter().toLowerCase().trim(); + const q = createMemo(() => filter().toLowerCase().trim()); function filteredReposForOrg(state: OrgRepoState): RepoEntry[] { const query = q(); @@ -270,23 +293,24 @@ export default function RepoSelector(props: RepoSelectorProps) { - {/* Per-org repo lists */} - + {/* Per-org repo lists — Index (not For) avoids tearing down every org's + DOM subtree when a single org's state updates via setOrgStates(prev.map(...)) */} + {(state) => { - const visible = createMemo(() => filteredReposForOrg(state)); + const visible = createMemo(() => filteredReposForOrg(state())); return (
{/* Org header */}
- {state.org} + {state().org} - +
{/* Loading state for this org */} - +
{/* Error state for this org */} - +
- {state.error} + {state().error}
); }} - + {/* Total count */} 0}> diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index a3b667fe..c9a0affb 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -3,6 +3,9 @@ import { useNavigate } from "@solidjs/router"; import { config, updateConfig } from "../../stores/config"; import { clearAuth } from "../../stores/auth"; import { clearCache } from "../../stores/cache"; +import { buildAuthorizeUrl, MERGE_ORGS_KEY } from "../../lib/oauth"; +import { fetchOrgs } from "../../services/api"; +import { getClient } from "../../services/github"; import OrgSelector from "../onboarding/OrgSelector"; import RepoSelector from "../onboarding/RepoSelector"; import type { RepoRef } from "../../services/api"; @@ -150,6 +153,7 @@ export default function SettingsPage() { const [confirmClearCache, setConfirmClearCache] = createSignal(false); const [confirmReset, setConfirmReset] = createSignal(false); const [cacheClearing, setCacheClearing] = createSignal(false); + const [reauthing, setReauthing] = createSignal(false); const [notifPermission, setNotifPermission] = createSignal( typeof Notification !== "undefined" ? Notification.permission : "default" ); @@ -187,10 +191,47 @@ export default function SettingsPage() { }; mq.addEventListener("change", handler); onCleanup(() => mq.removeEventListener("change", handler)); + + // Auto-merge newly accessible orgs after re-auth redirect + const shouldMerge = sessionStorage.getItem(MERGE_ORGS_KEY); + if (shouldMerge) { + sessionStorage.removeItem(MERGE_ORGS_KEY); + void mergeNewOrgs(); + } }); // ── Helpers ────────────────────────────────────────────────────────────── + async function mergeNewOrgs() { + const client = getClient(); + if (!client) return; + const snapshot = [...config.selectedOrgs]; + try { + const allOrgs = await fetchOrgs(client); + // Case-insensitive comparison — GitHub logins are case-insensitive + const currentSet = new Set(snapshot.map((o) => o.toLowerCase())); + const newOrgs = allOrgs + .map((o) => o.login) + .filter((login) => !currentSet.has(login.toLowerCase())); + if (newOrgs.length > 0) { + const merged = [...snapshot, ...newOrgs]; + setLocalOrgs(merged); + saveWithFeedback({ selectedOrgs: merged }); + console.info(`[settings] merged ${newOrgs.length} new org(s)`); + } + } catch { + // Non-fatal — user can manually manage orgs + } + } + + function handleReAuth() { + setReauthing(true); + // Reset if navigation doesn't happen (e.g., beforeunload dialog blocks redirect) + setTimeout(() => setReauthing(false), 3000); + sessionStorage.setItem(MERGE_ORGS_KEY, "true"); + window.location.href = buildAuthorizeUrl({ returnTo: "/settings" }); + } + function handleOrgsChange(orgs: string[]) { setLocalOrgs(orgs); saveWithFeedback({ selectedOrgs: orgs }); @@ -360,25 +401,48 @@ export default function SettingsPage() {
-
-
-

Repositories

-

- {localRepos().length} selected -

- 50}> -

- Tracking {localRepos().length} repos will use significant API quota per poll cycle +

+
+
+

+ Organization Access

- +

+ Re-authorize with GitHub to grant access to additional organizations +

+
+ +
+
+ +
+
+
+

Repositories

+

+ {localRepos().length} selected +

+ 50}> +

+ Tracking {localRepos().length} repos will use significant API quota per poll cycle +

+ +
+
-
diff --git a/src/app/lib/oauth.ts b/src/app/lib/oauth.ts new file mode 100644 index 00000000..decf5175 --- /dev/null +++ b/src/app/lib/oauth.ts @@ -0,0 +1,38 @@ +export const OAUTH_STATE_KEY = "github-tracker:oauth-state"; +export const OAUTH_RETURN_TO_KEY = "github-tracker:oauth-return-to"; +export const MERGE_ORGS_KEY = "github-tracker:merge-orgs"; + +export function generateOAuthState(): string { + const stateBytes = crypto.getRandomValues(new Uint8Array(16)); + return btoa(String.fromCharCode(...stateBytes)) + .replace(/\+/g, "-") + .replace(/\//g, "_") + .replace(/=/g, ""); +} + +export function sanitizeReturnTo(returnTo: string | null): string { + return returnTo && returnTo.startsWith("/") && !returnTo.startsWith("//") + ? returnTo + : "/"; +} + +export function buildAuthorizeUrl(options?: { returnTo?: string }): string { + const clientId = import.meta.env.VITE_GITHUB_CLIENT_ID as string; + const state = generateOAuthState(); + sessionStorage.setItem(OAUTH_STATE_KEY, state); + if (options?.returnTo) { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, options.returnTo); + } else { + // Clear any stale returnTo from a previous re-auth attempt + sessionStorage.removeItem(OAUTH_RETURN_TO_KEY); + } + const redirectUri = `${window.location.origin}/oauth/callback`; + const params = new URLSearchParams({ + client_id: clientId, + redirect_uri: redirectUri, + // repo: read issues/PRs; read:org: list orgs; notifications: gate + scope: "repo read:org notifications", + state, + }); + return `https://github.com/login/oauth/authorize?${params.toString()}`; +} diff --git a/src/app/pages/LoginPage.tsx b/src/app/pages/LoginPage.tsx index 44fd0e1a..02135881 100644 --- a/src/app/pages/LoginPage.tsx +++ b/src/app/pages/LoginPage.tsx @@ -1,26 +1,8 @@ +import { buildAuthorizeUrl } from "../lib/oauth"; + export default function LoginPage() { function handleLogin() { - const clientId = import.meta.env.VITE_GITHUB_CLIENT_ID as string; - - // Generate cryptographically random state for CSRF protection (SDR-002) - const stateBytes = crypto.getRandomValues(new Uint8Array(16)); - const state = btoa(String.fromCharCode(...stateBytes)) - .replace(/\+/g, "-") - .replace(/\//g, "_") - .replace(/=/g, ""); - - sessionStorage.setItem("github-tracker:oauth-state", state); - - const redirectUri = `${window.location.origin}/oauth/callback`; - const params = new URLSearchParams({ - client_id: clientId, - redirect_uri: redirectUri, - state, - // repo: read issues/PRs; read:org: list orgs; notifications: gate - scope: "repo read:org notifications", - }); - - window.location.href = `https://github.com/login/oauth/authorize?${params.toString()}`; + window.location.href = buildAuthorizeUrl(); } return ( @@ -38,6 +20,7 @@ export default function LoginPage() {
); diff --git a/tests/components/LoginPage.test.tsx b/tests/components/LoginPage.test.tsx index eb7f2c97..e548fbfe 100644 --- a/tests/components/LoginPage.test.tsx +++ b/tests/components/LoginPage.test.tsx @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { render, screen } from "@solidjs/testing-library"; import userEvent from "@testing-library/user-event"; import LoginPage from "../../src/app/pages/LoginPage"; +import { OAUTH_STATE_KEY } from "../../src/app/lib/oauth"; describe("LoginPage", () => { beforeEach(() => { @@ -70,7 +71,7 @@ describe("LoginPage", () => { render(() => ); const button = screen.getByText("Sign in with GitHub"); await user.click(button); - const stored = sessionStorage.getItem("github-tracker:oauth-state"); + const stored = sessionStorage.getItem(OAUTH_STATE_KEY); expect(stored).toBeTruthy(); }); @@ -81,7 +82,7 @@ describe("LoginPage", () => { await user.click(button); const url = new URL(window.location.href); const urlState = url.searchParams.get("state"); - const storedState = sessionStorage.getItem("github-tracker:oauth-state"); + const storedState = sessionStorage.getItem(OAUTH_STATE_KEY); expect(urlState).toBe(storedState); }); diff --git a/tests/components/OAuthCallback.test.tsx b/tests/components/OAuthCallback.test.tsx index 66442f1a..b6223d6c 100644 --- a/tests/components/OAuthCallback.test.tsx +++ b/tests/components/OAuthCallback.test.tsx @@ -11,6 +11,7 @@ vi.mock("../../src/app/stores/auth", () => ({ import * as authStore from "../../src/app/stores/auth"; import OAuthCallback from "../../src/app/pages/OAuthCallback"; +import { OAUTH_STATE_KEY, OAUTH_RETURN_TO_KEY } from "../../src/app/lib/oauth"; /** Render OAuthCallback inside a proper router context (useNavigate requires a Route). */ function renderCallback() { @@ -55,7 +56,7 @@ describe("OAuthCallback", () => { } function setupValidState() { - sessionStorage.setItem("github-tracker:oauth-state", "teststate"); + sessionStorage.setItem(OAUTH_STATE_KEY, "teststate"); } it("shows loading state while exchanging code", async () => { @@ -186,7 +187,7 @@ describe("OAuthCallback", () => { }); it("shows CSRF error when state param is missing from URL", async () => { - sessionStorage.setItem("github-tracker:oauth-state", "teststate"); + sessionStorage.setItem(OAUTH_STATE_KEY, "teststate"); setWindowSearch({ code: "fakecode" }); // no state param renderCallback(); @@ -197,7 +198,7 @@ describe("OAuthCallback", () => { }); it("shows CSRF error when state param does not match sessionStorage", async () => { - sessionStorage.setItem("github-tracker:oauth-state", "expected-state"); + sessionStorage.setItem(OAUTH_STATE_KEY, "expected-state"); setWindowSearch({ code: "fakecode", state: "wrong-state" }); renderCallback(); @@ -228,7 +229,7 @@ describe("OAuthCallback", () => { // onMount runs asynchronously — wait for the key to be cleared await waitFor(() => { - expect(sessionStorage.getItem("github-tracker:oauth-state")).toBeNull(); + expect(sessionStorage.getItem(OAUTH_STATE_KEY)).toBeNull(); }); }); @@ -241,7 +242,7 @@ describe("OAuthCallback", () => { vi.stubGlobal( "fetch", vi.fn(() => { - expect(sessionStorage.getItem("github-tracker:oauth-state")).toBeNull(); + expect(sessionStorage.getItem(OAUTH_STATE_KEY)).toBeNull(); return new Promise(() => {}); // keep pending }) ); @@ -254,7 +255,7 @@ describe("OAuthCallback", () => { }); it("handles missing code param", async () => { - sessionStorage.setItem("github-tracker:oauth-state", "teststate"); + sessionStorage.setItem(OAUTH_STATE_KEY, "teststate"); setWindowSearch({ state: "teststate" }); renderCallback(); @@ -287,4 +288,91 @@ describe("OAuthCallback", () => { }); }); + // ── returnTo redirect tests ──────────────────────────────────────────────── + + function setupSuccessfulCallback() { + setupValidState(); + setWindowSearch({ code: "fakecode", state: "teststate" }); + vi.stubGlobal( + "fetch", + vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ access_token: "tok123" }), + }) + ); + vi.mocked(authStore.validateToken).mockResolvedValue(true); + } + + it("navigates to returnTo path when OAUTH_RETURN_TO_KEY is set to /settings", async () => { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, "/settings"); + setupSuccessfulCallback(); + renderCallback(); + + await waitFor(() => { + expect(authStore.validateToken).toHaveBeenCalled(); + }); + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + }); + + it("OAUTH_RETURN_TO_KEY is removed from sessionStorage after reading", 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(); + }); + }); + + it("OAUTH_RETURN_TO_KEY is cleared even when CSRF check fails (stale key protection)", async () => { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, "/settings"); + sessionStorage.setItem(OAUTH_STATE_KEY, "expected-state"); + setWindowSearch({ code: "fakecode", state: "wrong-state" }); + + renderCallback(); + + await waitFor(() => { + screen.getByText(/Invalid OAuth state/i); + }); + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + }); + + it("navigates to / when OAUTH_RETURN_TO_KEY is not set", async () => { + setupSuccessfulCallback(); + renderCallback(); + + await waitFor(() => { + expect(authStore.validateToken).toHaveBeenCalled(); + }); + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + }); + + it("rejects absolute URL returnTo (open-redirect protection)", async () => { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, "https://evil.com"); + setupSuccessfulCallback(); + renderCallback(); + + await waitFor(() => { + expect(authStore.validateToken).toHaveBeenCalled(); + }); + // Key consumed regardless + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + }); + + it("rejects protocol-relative URL returnTo (// attack)", async () => { + sessionStorage.setItem(OAUTH_RETURN_TO_KEY, "//evil.com"); + setupSuccessfulCallback(); + renderCallback(); + + await waitFor(() => { + expect(authStore.validateToken).toHaveBeenCalled(); + }); + expect(sessionStorage.getItem(OAUTH_RETURN_TO_KEY)).toBeNull(); + }); + + // returnTo validation is tested via sanitizeReturnTo in tests/lib/oauth.test.ts + }); diff --git a/tests/components/onboarding/OnboardingWizard.test.tsx b/tests/components/onboarding/OnboardingWizard.test.tsx index f25479ee..3264a6ba 100644 --- a/tests/components/onboarding/OnboardingWizard.test.tsx +++ b/tests/components/onboarding/OnboardingWizard.test.tsx @@ -1,28 +1,33 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { render, screen, waitFor } from "@solidjs/testing-library"; import userEvent from "@testing-library/user-event"; -import type { RepoRef } from "../../../src/app/services/api"; - -// Mock OrgSelector and RepoSelector to avoid their internal fetching -vi.mock("../../../src/app/components/onboarding/OrgSelector", () => ({ - default: (props: { selected: string[]; onChange: (s: string[]) => void }) => ( -
- - Selected: {props.selected.join(",")} -
- ), +import type { RepoRef, OrgEntry } from "../../../src/app/services/api"; + +// Mock fetchOrgs and getClient +vi.mock("../../../src/app/services/api", () => ({ + fetchOrgs: vi.fn(), +})); +vi.mock("../../../src/app/services/github", () => ({ + getClient: vi.fn(() => ({})), })); +// Mock RepoSelector to avoid internal fetching vi.mock("../../../src/app/components/onboarding/RepoSelector", () => ({ default: (props: { selectedOrgs: string[]; + orgEntries?: OrgEntry[]; selected: RepoRef[]; onChange: (s: RepoRef[]) => void; }) => (
+ + {props.selectedOrgs.join(",")} +