diff --git a/src/app/components/settings/SettingsPage.tsx b/src/app/components/settings/SettingsPage.tsx index c9a0affb..3442930b 100644 --- a/src/app/components/settings/SettingsPage.tsx +++ b/src/app/components/settings/SettingsPage.tsx @@ -3,7 +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 { pushNotification } from "../../lib/errors"; +import { buildOrgAccessUrl } from "../../lib/oauth"; +import { isSafeGitHubUrl, openGitHubUrl } from "../../lib/url"; import { fetchOrgs } from "../../services/api"; import { getClient } from "../../services/github"; import OrgSelector from "../onboarding/OrgSelector"; @@ -153,7 +155,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 [merging, setMerging] = createSignal(false); const [notifPermission, setNotifPermission] = createSignal( typeof Notification !== "undefined" ? Notification.permission : "default" ); @@ -161,6 +163,7 @@ export default function SettingsPage() { // Save indicator const [showSaved, setShowSaved] = createSignal(false); let saveTimer: ReturnType | undefined; + let pendingFocusHandler: (() => void) | undefined; function saveWithFeedback(patch: Parameters[0]) { updateConfig(patch); @@ -169,7 +172,12 @@ export default function SettingsPage() { saveTimer = setTimeout(() => setShowSaved(false), 1500); } - onCleanup(() => clearTimeout(saveTimer)); + onCleanup(() => { + clearTimeout(saveTimer); + if (pendingFocusHandler) { + window.removeEventListener("focus", pendingFocusHandler); + } + }); // Local copies for org/repo editing (committed on blur/change) const [localOrgs, setLocalOrgs] = createSignal(config.selectedOrgs); @@ -191,13 +199,6 @@ 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 ────────────────────────────────────────────────────────────── @@ -205,6 +206,7 @@ export default function SettingsPage() { async function mergeNewOrgs() { const client = getClient(); if (!client) return; + setMerging(true); const snapshot = [...config.selectedOrgs]; try { const allOrgs = await fetchOrgs(client); @@ -220,16 +222,27 @@ export default function SettingsPage() { console.info(`[settings] merged ${newOrgs.length} new org(s)`); } } catch { - // Non-fatal — user can manually manage orgs + pushNotification("org-sync", "Failed to sync organizations — try again or manage manually", "warning"); + } finally { + setMerging(false); } } - 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 handleGrantOrgs() { + const url = buildOrgAccessUrl(); + if (!isSafeGitHubUrl(url)) return; + openGitHubUrl(url); + // Remove any prior focus listener before adding a new one (dedup on rapid clicks) + if (pendingFocusHandler) { + window.removeEventListener("focus", pendingFocusHandler); + } + const onFocus = () => { + window.removeEventListener("focus", onFocus); + pendingFocusHandler = undefined; + void mergeNewOrgs(); + }; + pendingFocusHandler = onFocus; + window.addEventListener("focus", onFocus); } function handleOrgsChange(orgs: string[]) { @@ -408,16 +421,16 @@ export default function SettingsPage() { Organization Access

- Re-authorize with GitHub to grant access to additional organizations + Manage organization access on GitHub — new orgs sync automatically when you return

diff --git a/src/app/lib/oauth.ts b/src/app/lib/oauth.ts index decf5175..249c6b71 100644 --- a/src/app/lib/oauth.ts +++ b/src/app/lib/oauth.ts @@ -1,6 +1,5 @@ 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)); @@ -36,3 +35,13 @@ export function buildAuthorizeUrl(options?: { returnTo?: string }): string { }); return `https://github.com/login/oauth/authorize?${params.toString()}`; } + +const VALID_CLIENT_ID_RE = /^[A-Za-z0-9_-]+$/; + +export function buildOrgAccessUrl(): string { + const clientId = import.meta.env.VITE_GITHUB_CLIENT_ID as string; + if (!clientId || !VALID_CLIENT_ID_RE.test(clientId)) { + throw new Error("Invalid VITE_GITHUB_CLIENT_ID"); + } + return `https://github.com/settings/connections/applications/${clientId}`; +} diff --git a/tests/components/settings/SettingsPage.test.tsx b/tests/components/settings/SettingsPage.test.tsx index 728a2fac..f5b34409 100644 --- a/tests/components/settings/SettingsPage.test.tsx +++ b/tests/components/settings/SettingsPage.test.tsx @@ -41,6 +41,15 @@ vi.mock("../../../src/app/services/api", () => ({ fetchRepos: vi.fn().mockResolvedValue([]), })); +vi.mock("../../../src/app/lib/url", () => ({ + isSafeGitHubUrl: vi.fn(() => true), + openGitHubUrl: vi.fn(), +})); + +vi.mock("../../../src/app/lib/errors", () => ({ + pushNotification: vi.fn(), +})); + // ── Imports after mocks ─────────────────────────────────────────────────────── import { render } from "@solidjs/testing-library"; @@ -50,7 +59,8 @@ import * as authStore from "../../../src/app/stores/auth"; import * as cacheStore from "../../../src/app/stores/cache"; import * as apiModule from "../../../src/app/services/api"; import { updateConfig, config } from "../../../src/app/stores/config"; -import { MERGE_ORGS_KEY, OAUTH_STATE_KEY } from "../../../src/app/lib/oauth"; +import { buildOrgAccessUrl } from "../../../src/app/lib/oauth"; +import * as urlModule from "../../../src/app/lib/url"; // ── Helpers ─────────────────────────────────────────────────────────────────── @@ -89,6 +99,9 @@ beforeEach(() => { setupMatchMedia(); vi.clearAllMocks(); + // Restore isSafeGitHubUrl mock (vi.restoreAllMocks strips factory implementations) + vi.mocked(urlModule.isSafeGitHubUrl).mockReturnValue(true); + // Reset config to defaults updateConfig({ refreshInterval: 300, @@ -104,7 +117,6 @@ beforeEach(() => { selectedRepos: [], }); - // Clear sessionStorage — re-auth tests set MERGE_ORGS_KEY and OAUTH_STATE_KEY sessionStorage.clear(); // Mock window.location with both reload and href @@ -521,126 +533,147 @@ describe("SettingsPage — Theme application", () => { }); }); -describe("SettingsPage — Re-auth button", () => { +describe("SettingsPage — Grant more orgs button", () => { + beforeEach(() => { + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + }); + afterEach(() => { + vi.unstubAllEnvs(); + }); + it("renders 'Grant more orgs' button in Organizations & Repositories section", () => { renderSettings(); screen.getByRole("button", { name: "Grant more orgs" }); }); - it("clicking 'Grant more orgs' sets MERGE_ORGS_KEY in sessionStorage", async () => { + it("clicking 'Grant more orgs' opens GitHub app settings via openGitHubUrl", async () => { const user = userEvent.setup(); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); renderSettings(); const btn = screen.getByRole("button", { name: "Grant more orgs" }); await user.click(btn); - expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBe("true"); - vi.unstubAllEnvs(); + expect(urlModule.openGitHubUrl).toHaveBeenCalledWith(buildOrgAccessUrl()); }); - it("clicking 'Grant more orgs' sets window.location.href to GitHub authorize URL", async () => { + it("clicking 'Grant more orgs' registers a focus listener for auto-merge", async () => { const user = userEvent.setup(); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + const addSpy = vi.spyOn(window, "addEventListener"); renderSettings(); const btn = screen.getByRole("button", { name: "Grant more orgs" }); await user.click(btn); - expect(window.location.href).toContain("https://github.com/login/oauth/authorize"); - vi.unstubAllEnvs(); + expect(addSpy).toHaveBeenCalledWith("focus", expect.any(Function)); }); - it("clicking 'Grant more orgs' also sets OAUTH_STATE_KEY in sessionStorage", async () => { + it("shows disabled 'Syncing...' button during merge, reverts after", async () => { const user = userEvent.setup(); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); - renderSettings(); - const btn = screen.getByRole("button", { name: "Grant more orgs" }); - await user.click(btn); - expect(sessionStorage.getItem(OAUTH_STATE_KEY)).toBeTruthy(); - vi.unstubAllEnvs(); - }); - - it("'Grant more orgs' button is disabled after click and re-enables after timeout", async () => { - vi.useFakeTimers(); - const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime }); - vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + updateConfig({ selectedOrgs: [] }); + let resolveFetch!: (v: never[]) => void; + vi.mocked(apiModule.fetchOrgs).mockReturnValue( + new Promise((r) => { resolveFetch = r as (v: never[]) => void; }) + ); renderSettings(); const btn = screen.getByRole("button", { name: "Grant more orgs" }); await user.click(btn); - expect(btn.hasAttribute("disabled")).toBe(true); - vi.advanceTimersByTime(3000); - expect(btn.hasAttribute("disabled")).toBe(false); - vi.unstubAllEnvs(); - vi.useRealTimers(); + window.dispatchEvent(new Event("focus")); + await waitFor(() => { + const syncBtn = screen.getByRole("button", { name: "Syncing..." }); + expect(syncBtn.hasAttribute("disabled")).toBe(true); + }); + resolveFetch([]); + await waitFor(() => { + const restored = screen.getByRole("button", { name: "Grant more orgs" }); + expect(restored.hasAttribute("disabled")).toBe(false); + }); }); -}); -describe("SettingsPage — Auto-merge orgs on mount", () => { - it("calls fetchOrgs and merges new orgs when MERGE_ORGS_KEY is in sessionStorage", async () => { - sessionStorage.setItem(MERGE_ORGS_KEY, "true"); + it("auto-merges new orgs when window regains focus after granting", async () => { + const user = userEvent.setup(); updateConfig({ selectedOrgs: ["existing-org"] }); vi.mocked(apiModule.fetchOrgs).mockResolvedValue([ { login: "existing-org", avatarUrl: "", type: "org" }, { login: "new-org", avatarUrl: "", type: "org" }, ]); renderSettings(); + const btn = screen.getByRole("button", { name: "Grant more orgs" }); + await user.click(btn); + window.dispatchEvent(new Event("focus")); await waitFor(() => { expect(apiModule.fetchOrgs).toHaveBeenCalled(); }); await waitFor(() => { expect(config.selectedOrgs).toContain("new-org"); + expect(config.selectedOrgs).toContain("existing-org"); }); }); - it("preserves existing selectedOrgs when merging", async () => { - sessionStorage.setItem(MERGE_ORGS_KEY, "true"); + it("pushes warning notification on fetchOrgs failure", async () => { + const { pushNotification } = await import("../../../src/app/lib/errors"); + const user = userEvent.setup(); updateConfig({ selectedOrgs: ["existing-org"] }); - vi.mocked(apiModule.fetchOrgs).mockResolvedValue([ - { login: "existing-org", avatarUrl: "", type: "org" }, - { login: "new-org", avatarUrl: "", type: "org" }, - ]); + vi.mocked(apiModule.fetchOrgs).mockRejectedValue(new Error("Network error")); renderSettings(); + const btn = screen.getByRole("button", { name: "Grant more orgs" }); + await user.click(btn); + window.dispatchEvent(new Event("focus")); await waitFor(() => { - expect(config.selectedOrgs).toContain("existing-org"); + expect(apiModule.fetchOrgs).toHaveBeenCalled(); }); - }); - - it("removes MERGE_ORGS_KEY from sessionStorage after processing", async () => { - sessionStorage.setItem(MERGE_ORGS_KEY, "true"); - updateConfig({ selectedOrgs: [] }); - vi.mocked(apiModule.fetchOrgs).mockResolvedValue([ - { login: "new-org", avatarUrl: "", type: "org" }, - ]); - renderSettings(); await waitFor(() => { - expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBeNull(); + expect(pushNotification).toHaveBeenCalledWith( + "org-sync", + expect.stringContaining("Failed to sync"), + "warning", + ); }); + expect(config.selectedOrgs).toEqual(["existing-org"]); }); - it("does not call fetchOrgs when MERGE_ORGS_KEY is not in sessionStorage", () => { - // No MERGE_ORGS_KEY set + it("skips merge on focus when getClient returns null", async () => { + const user = userEvent.setup(); + const github = await import("../../../src/app/services/github"); + vi.mocked(github.getClient).mockReturnValueOnce(null); renderSettings(); + const btn = screen.getByRole("button", { name: "Grant more orgs" }); + await user.click(btn); + window.dispatchEvent(new Event("focus")); + await new Promise((r) => setTimeout(r, 50)); expect(apiModule.fetchOrgs).not.toHaveBeenCalled(); }); - it("silently handles fetchOrgs rejection without breaking", async () => { - sessionStorage.setItem(MERGE_ORGS_KEY, "true"); - updateConfig({ selectedOrgs: ["existing-org"] }); - vi.mocked(apiModule.fetchOrgs).mockRejectedValue(new Error("Network error")); + it("rapid double-click deduplicates focus listeners", async () => { + const user = userEvent.setup(); + const removeSpy = vi.spyOn(window, "removeEventListener"); renderSettings(); - await waitFor(() => { - expect(apiModule.fetchOrgs).toHaveBeenCalled(); - }); - // Config unchanged — error was swallowed - expect(config.selectedOrgs).toEqual(["existing-org"]); + const btn = screen.getByRole("button", { name: "Grant more orgs" }); + await user.click(btn); + await user.click(btn); + const focusRemoves = removeSpy.mock.calls.filter(([evt]) => evt === "focus"); + expect(focusRemoves.length).toBeGreaterThanOrEqual(1); }); - it("skips merge when getClient returns null", async () => { - sessionStorage.setItem(MERGE_ORGS_KEY, "true"); - const github = await import("../../../src/app/services/github"); - vi.mocked(github.getClient).mockReturnValueOnce(null); + it("cleans up pending focus listener on component unmount", async () => { + const user = userEvent.setup(); + const removeSpy = vi.spyOn(window, "removeEventListener"); + const { unmount } = renderSettings(); + const btn = screen.getByRole("button", { name: "Grant more orgs" }); + await user.click(btn); + unmount(); + const focusRemoves = removeSpy.mock.calls.filter(([evt]) => evt === "focus"); + expect(focusRemoves.length).toBeGreaterThanOrEqual(1); + }); + + it("focus listener self-removes — second focus does not re-trigger merge", async () => { + const user = userEvent.setup(); + updateConfig({ selectedOrgs: [] }); + vi.mocked(apiModule.fetchOrgs).mockResolvedValue([]); renderSettings(); - // MERGE_ORGS_KEY still removed synchronously before mergeNewOrgs + const btn = screen.getByRole("button", { name: "Grant more orgs" }); + await user.click(btn); + window.dispatchEvent(new Event("focus")); await waitFor(() => { - expect(sessionStorage.getItem(MERGE_ORGS_KEY)).toBeNull(); + expect(apiModule.fetchOrgs).toHaveBeenCalledTimes(1); }); - expect(apiModule.fetchOrgs).not.toHaveBeenCalled(); + window.dispatchEvent(new Event("focus")); + await new Promise((r) => setTimeout(r, 50)); + expect(apiModule.fetchOrgs).toHaveBeenCalledTimes(1); }); }); diff --git a/tests/lib/oauth.test.ts b/tests/lib/oauth.test.ts index 89c6ebe6..222d2506 100644 --- a/tests/lib/oauth.test.ts +++ b/tests/lib/oauth.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { generateOAuthState, buildAuthorizeUrl, + buildOrgAccessUrl, sanitizeReturnTo, OAUTH_STATE_KEY, OAUTH_RETURN_TO_KEY, @@ -97,6 +98,29 @@ describe("oauth helpers", () => { }); }); + describe("buildOrgAccessUrl", () => { + it("returns GitHub connections URL with client ID", () => { + const url = buildOrgAccessUrl(); + expect(url).toBe( + "https://github.com/settings/connections/applications/test-client-id" + ); + }); + + it("throws for undefined client ID", () => { + vi.stubEnv("VITE_GITHUB_CLIENT_ID", ""); + expect(() => buildOrgAccessUrl()).toThrow("Invalid VITE_GITHUB_CLIENT_ID"); + vi.unstubAllEnvs(); + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + }); + + it("throws for client ID with path traversal characters", () => { + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "../../../evil"); + expect(() => buildOrgAccessUrl()).toThrow("Invalid VITE_GITHUB_CLIENT_ID"); + vi.unstubAllEnvs(); + vi.stubEnv("VITE_GITHUB_CLIENT_ID", "test-client-id"); + }); + }); + describe("sanitizeReturnTo", () => { it("accepts internal paths", () => { expect(sanitizeReturnTo("/settings")).toBe("/settings");