Skip to content

Commit 0a8e813

Browse files
committed
fix: addresses PR #40 review findings
- adds Suspense boundary wrapping Router for lazy chunk loading states - adds /api/csp-report worker endpoint that scrubs OAuth params before forwarding CSP violation reports to Sentry (replaces direct report-uri) - adds Reporting-Endpoints header enabling modern report-to directive - removes unnecessary blob: from CSP img-src - extracts ErrorBoundary fallback to named handleRouteError function - adds .catch() with console.warn to both prefetch paths - adds [app] to Sentry allowed console breadcrumb prefixes - extracts getOrCacheDsn helper to deduplicate DSN cache logic - adds parseSentryDsn publicKey validation - caps CSP report batch fan-out to 20 to prevent amplification - scrubs referrer field in CSP reports (OAuth param leak vector) - adds 18 tests covering CSP report endpoint, ErrorBoundary for all lazy routes, prefetch scheduling, and sentry prefix
1 parent f25c75e commit 0a8e813

File tree

8 files changed

+557
-26
lines changed

8 files changed

+557
-26
lines changed

public/_headers

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
/*
2-
Content-Security-Policy: default-src 'none'; script-src 'self' 'sha256-uEFqyYCMaNy1Su5VmWLZ1hOCRBjkhm4+ieHHxQW6d3Y='; style-src-elem 'self'; style-src-attr 'unsafe-inline'; img-src 'self' blob: data: https://avatars.githubusercontent.com; connect-src 'self' https://api.github.com; font-src 'self'; worker-src 'self'; manifest-src 'self'; frame-ancestors 'none'; base-uri 'self'; form-action 'none'; upgrade-insecure-requests; report-uri https://o284235.ingest.us.sentry.io/api/4511122822922240/security/?sentry_key=4dc4335a9746201c02ff2107c0d20f73
2+
Content-Security-Policy: default-src 'none'; script-src 'self' 'sha256-uEFqyYCMaNy1Su5VmWLZ1hOCRBjkhm4+ieHHxQW6d3Y='; style-src-elem 'self'; style-src-attr 'unsafe-inline'; img-src 'self' data: https://avatars.githubusercontent.com; connect-src 'self' https://api.github.com; font-src 'self'; worker-src 'self'; manifest-src 'self'; frame-ancestors 'none'; base-uri 'self'; form-action 'none'; upgrade-insecure-requests; report-uri /api/csp-report; report-to csp-endpoint
3+
Reporting-Endpoints: csp-endpoint="/api/csp-report"
34
X-Content-Type-Options: nosniff
45
Referrer-Policy: strict-origin-when-cross-origin
56
Permissions-Policy: geolocation=(), microphone=(), camera=()

src/app/App.tsx

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { createSignal, createEffect, onMount, Show, ErrorBoundary, lazy, type JSX } from "solid-js";
1+
import { createSignal, createEffect, onMount, Show, ErrorBoundary, Suspense, lazy, type JSX } from "solid-js";
22
import { Router, Route, Navigate, useNavigate } from "@solidjs/router";
33
import { isAuthenticated, validateToken, AUTH_STORAGE_KEY } from "./stores/auth";
44
import { config, initConfigPersistence, resolveTheme } from "./stores/config";
@@ -13,6 +13,11 @@ const DashboardPage = lazy(() => import("./components/dashboard/DashboardPage"))
1313
const OnboardingWizard = lazy(() => import("./components/onboarding/OnboardingWizard"));
1414
const SettingsPage = lazy(() => import("./components/settings/SettingsPage"));
1515

16+
function handleRouteError(err: unknown) {
17+
console.error("[app] Route render failed:", err);
18+
return <ChunkErrorFallback />;
19+
}
20+
1621
function ChunkErrorFallback() {
1722
return (
1823
<div class="min-h-screen flex items-center justify-center bg-base-200">
@@ -163,21 +168,29 @@ export default function App() {
163168
// Preload dashboard chunk in parallel with token validation to avoid
164169
// a sequential waterfall (validateToken → chunk fetch)
165170
if (localStorage.getItem?.(AUTH_STORAGE_KEY)) {
166-
void import("./components/dashboard/DashboardPage");
171+
import("./components/dashboard/DashboardPage").catch(() => {
172+
console.warn("[app] Dashboard chunk preload failed");
173+
});
167174
}
168175
});
169176

170177
return (
171-
<ErrorBoundary fallback={(err) => { console.error("[app] Route render failed:", err); return <ChunkErrorFallback />; }}>
172-
<Router>
173-
<Route path="/" component={RootRedirect} />
174-
<Route path="/login" component={LoginPage} />
175-
<Route path="/oauth/callback" component={OAuthCallback} />
176-
<Route path="/onboarding" component={() => <AuthGuard><OnboardingWizard /></AuthGuard>} />
177-
<Route path="/dashboard" component={() => <AuthGuard><DashboardPage /></AuthGuard>} />
178-
<Route path="/settings" component={() => <AuthGuard><SettingsPage /></AuthGuard>} />
179-
<Route path="/privacy" component={PrivacyPage} />
180-
</Router>
178+
<ErrorBoundary fallback={handleRouteError}>
179+
<Suspense fallback={
180+
<div class="min-h-screen flex items-center justify-center bg-base-200">
181+
<span class="loading loading-spinner loading-lg" aria-label="Loading" />
182+
</div>
183+
}>
184+
<Router>
185+
<Route path="/" component={RootRedirect} />
186+
<Route path="/login" component={LoginPage} />
187+
<Route path="/oauth/callback" component={OAuthCallback} />
188+
<Route path="/onboarding" component={() => <AuthGuard><OnboardingWizard /></AuthGuard>} />
189+
<Route path="/dashboard" component={() => <AuthGuard><DashboardPage /></AuthGuard>} />
190+
<Route path="/settings" component={() => <AuthGuard><SettingsPage /></AuthGuard>} />
191+
<Route path="/privacy" component={PrivacyPage} />
192+
</Router>
193+
</Suspense>
181194
</ErrorBoundary>
182195
);
183196
}

src/app/pages/LoginPage.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@ export default function LoginPage() {
1414
onMount(() => {
1515
// Speculatively prefetch the dashboard chunk while the user is on the
1616
// login page. By the time they authenticate, the chunk is cached.
17-
const prefetch = () => void import("../components/dashboard/DashboardPage");
17+
const prefetch = () => {
18+
import("../components/dashboard/DashboardPage").catch(() => {
19+
console.warn("[app] Dashboard chunk prefetch failed");
20+
});
21+
};
1822
"requestIdleCallback" in window
1923
? requestIdleCallback(prefetch)
2024
: setTimeout(prefetch, 2000);

src/worker/index.ts

Lines changed: 121 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -80,21 +80,31 @@ function getCorsHeaders(
8080
// The envelope DSN is validated against env.SENTRY_DSN to prevent open proxy abuse.
8181
const SENTRY_ENVELOPE_MAX_BYTES = 256 * 1024; // 256 KB — Sentry rejects >200KB compressed
8282

83-
let _dsnCache: { dsn: string; parsed: { host: string; projectId: string } | null } | undefined;
83+
interface ParsedDsn { host: string; projectId: string; publicKey: string }
8484

85-
/** Parse host and project ID from a Sentry DSN URL. Returns null if invalid. */
86-
function parseSentryDsn(dsn: string): { host: string; projectId: string } | null {
85+
let _dsnCache: { dsn: string; parsed: ParsedDsn | null } | undefined;
86+
87+
/** Parse host, project ID, and public key from a Sentry DSN URL. Returns null if invalid. */
88+
function parseSentryDsn(dsn: string): ParsedDsn | null {
8789
if (!dsn) return null;
8890
try {
8991
const url = new URL(dsn);
9092
const projectId = url.pathname.split("/").filter(Boolean).pop() ?? "";
91-
if (!url.hostname || !projectId) return null;
92-
return { host: url.hostname, projectId };
93+
if (!url.hostname || !projectId || !url.username) return null;
94+
return { host: url.hostname, projectId, publicKey: url.username };
9395
} catch {
9496
return null;
9597
}
9698
}
9799

100+
/** Get cached parsed DSN, re-parsing only when the DSN string changes. */
101+
function getOrCacheDsn(env: Env): ParsedDsn | null {
102+
if (!_dsnCache || _dsnCache.dsn !== env.SENTRY_DSN) {
103+
_dsnCache = { dsn: env.SENTRY_DSN, parsed: parseSentryDsn(env.SENTRY_DSN) };
104+
}
105+
return _dsnCache.parsed;
106+
}
107+
98108
async function handleSentryTunnel(
99109
request: Request,
100110
env: Env,
@@ -103,10 +113,7 @@ async function handleSentryTunnel(
103113
return new Response(null, { status: 405, headers: SECURITY_HEADERS });
104114
}
105115

106-
if (!_dsnCache || _dsnCache.dsn !== env.SENTRY_DSN) {
107-
_dsnCache = { dsn: env.SENTRY_DSN, parsed: parseSentryDsn(env.SENTRY_DSN) };
108-
}
109-
const allowedDsn = _dsnCache.parsed;
116+
const allowedDsn = getOrCacheDsn(env);
110117
if (!allowedDsn) {
111118
log("warn", "sentry_tunnel_not_configured", {}, request);
112119
return new Response(null, { status: 404, headers: SECURITY_HEADERS });
@@ -186,6 +193,106 @@ async function handleSentryTunnel(
186193
}
187194
}
188195

196+
// ── CSP report tunnel ────────────────────────────────────────────────────
197+
// Receives browser CSP violation reports, scrubs OAuth params from URLs,
198+
// then forwards to Sentry's security ingest endpoint.
199+
const CSP_REPORT_MAX_BYTES = 64 * 1024;
200+
const CSP_OAUTH_PARAMS_RE = /([?&])(code|state|access_token)=[^&\s]*/g;
201+
202+
function scrubReportUrl(url: unknown): string | undefined {
203+
if (typeof url !== "string") return undefined;
204+
return url.replace(CSP_OAUTH_PARAMS_RE, "$1$2=[REDACTED]");
205+
}
206+
207+
function scrubCspReportBody(body: Record<string, unknown>): Record<string, unknown> {
208+
const scrubbed = { ...body };
209+
// Legacy report-uri format uses kebab-case keys
210+
for (const key of ["document-uri", "blocked-uri", "source-file", "referrer"]) {
211+
if (typeof scrubbed[key] === "string") scrubbed[key] = scrubReportUrl(scrubbed[key]);
212+
}
213+
// report-to format uses camelCase keys
214+
for (const key of ["documentURL", "blockedURL", "sourceFile", "referrer"]) {
215+
if (typeof scrubbed[key] === "string") scrubbed[key] = scrubReportUrl(scrubbed[key]);
216+
}
217+
return scrubbed;
218+
}
219+
220+
async function handleCspReport(request: Request, env: Env): Promise<Response> {
221+
if (request.method !== "POST") {
222+
return new Response(null, { status: 405, headers: SECURITY_HEADERS });
223+
}
224+
225+
const allowedDsn = getOrCacheDsn(env);
226+
if (!allowedDsn) {
227+
return new Response(null, { status: 404, headers: SECURITY_HEADERS });
228+
}
229+
230+
let bodyText: string;
231+
try {
232+
bodyText = await request.text();
233+
} catch {
234+
return new Response(null, { status: 400, headers: SECURITY_HEADERS });
235+
}
236+
237+
if (bodyText.length > CSP_REPORT_MAX_BYTES) {
238+
log("warn", "csp_report_too_large", { body_length: bodyText.length }, request);
239+
return new Response(null, { status: 413, headers: SECURITY_HEADERS });
240+
}
241+
242+
const contentType = request.headers.get("Content-Type") ?? "";
243+
let scrubbedPayloads: Array<Record<string, unknown>> = [];
244+
245+
try {
246+
if (contentType.includes("application/reports+json")) {
247+
// report-to format: array of report objects
248+
const reports = JSON.parse(bodyText) as Array<{ type?: string; body?: Record<string, unknown> }>;
249+
for (const report of reports) {
250+
if (report.type === "csp-violation" && report.body) {
251+
scrubbedPayloads.push({ "csp-report": scrubCspReportBody(report.body) });
252+
}
253+
}
254+
} else {
255+
// Legacy report-uri format: { "csp-report": { ... } }
256+
const parsed = JSON.parse(bodyText) as { "csp-report"?: Record<string, unknown> };
257+
if (parsed["csp-report"]) {
258+
scrubbedPayloads.push({ "csp-report": scrubCspReportBody(parsed["csp-report"]) });
259+
}
260+
}
261+
} catch {
262+
log("warn", "csp_report_parse_failed", {}, request);
263+
return new Response(null, { status: 400, headers: SECURITY_HEADERS });
264+
}
265+
266+
if (scrubbedPayloads.length === 0) {
267+
return new Response(null, { status: 204, headers: SECURITY_HEADERS });
268+
}
269+
270+
// Cap fan-out to prevent amplification from crafted report-to batches
271+
if (scrubbedPayloads.length > 20) {
272+
scrubbedPayloads = scrubbedPayloads.slice(0, 20);
273+
}
274+
275+
// Sentry security endpoint expects individual csp-report JSON objects
276+
const sentryUrl = `https://${allowedDsn.host}/api/${allowedDsn.projectId}/security/?sentry_key=${allowedDsn.publicKey}`;
277+
278+
const results = await Promise.all(
279+
scrubbedPayloads.map((payload) =>
280+
fetch(sentryUrl, {
281+
method: "POST",
282+
headers: { "Content-Type": "application/csp-report" },
283+
body: JSON.stringify(payload),
284+
}).catch(() => null)
285+
)
286+
);
287+
288+
log("info", "csp_report_forwarded", {
289+
count: scrubbedPayloads.length,
290+
sentry_ok: results.some((r) => r?.ok),
291+
}, request);
292+
293+
return new Response(null, { status: 204, headers: SECURITY_HEADERS });
294+
}
295+
189296
// GitHub OAuth code format validation (SDR-005): alphanumeric, 1-40 chars.
190297
// GitHub's code format is undocumented and has changed historically — validate
191298
// loosely here; GitHub's server validates the actual code.
@@ -350,6 +457,11 @@ export default {
350457
return handleSentryTunnel(request, env);
351458
}
352459

460+
// CSP report tunnel — scrubs OAuth params before forwarding to Sentry
461+
if (url.pathname === "/api/csp-report") {
462+
return handleCspReport(request, env);
463+
}
464+
353465
if (url.pathname === "/api/oauth/token") {
354466
return handleTokenExchange(request, env, cors);
355467
}

tests/components/App.test.tsx

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,17 +52,25 @@ vi.mock("../../src/app/stores/cache", async (importOriginal) => {
5252

5353
// Mock heavy page/component dependencies
5454
let dashboardShouldThrow = false;
55+
let onboardingShouldThrow = false;
56+
let settingsShouldThrow = false;
5557
vi.mock("../../src/app/components/dashboard/DashboardPage", () => ({
5658
default: () => {
5759
if (dashboardShouldThrow) throw new Error("chunk load failed");
5860
return <div data-testid="dashboard-page">Dashboard</div>;
5961
},
6062
}));
6163
vi.mock("../../src/app/components/onboarding/OnboardingWizard", () => ({
62-
default: () => <div data-testid="onboarding-wizard">Onboarding</div>,
64+
default: () => {
65+
if (onboardingShouldThrow) throw new Error("chunk load failed");
66+
return <div data-testid="onboarding-wizard">Onboarding</div>;
67+
},
6368
}));
6469
vi.mock("../../src/app/components/settings/SettingsPage", () => ({
65-
default: () => <div data-testid="settings-page">Settings</div>,
70+
default: () => {
71+
if (settingsShouldThrow) throw new Error("chunk load failed");
72+
return <div data-testid="settings-page">Settings</div>;
73+
},
6674
}));
6775

6876
import * as configStore from "../../src/app/stores/config";
@@ -78,6 +86,8 @@ describe("App", () => {
7886
mockIsAuthenticated = false;
7987
mockValidateToken = async () => false;
8088
dashboardShouldThrow = false;
89+
onboardingShouldThrow = false;
90+
settingsShouldThrow = false;
8191
// Re-apply default mock implementations that are needed across tests
8292
vi.mocked(cacheStore.evictStaleEntries).mockResolvedValue(0);
8393
// Reset config to defaults
@@ -188,4 +198,60 @@ describe("App", () => {
188198

189199
spy.mockRestore();
190200
});
201+
202+
it("shows error fallback when onboarding route throws", async () => {
203+
onboardingShouldThrow = true;
204+
mockIsAuthenticated = true;
205+
configStore.updateConfig({ onboardingComplete: false });
206+
window.history.pushState({}, "", "/onboarding");
207+
208+
const spy = vi.spyOn(console, "error").mockImplementation(() => {});
209+
210+
render(() => <App />);
211+
212+
await waitFor(() => {
213+
screen.getByText("Failed to load page");
214+
});
215+
216+
spy.mockRestore();
217+
});
218+
219+
it("shows error fallback when settings route throws", async () => {
220+
settingsShouldThrow = true;
221+
mockIsAuthenticated = true;
222+
configStore.updateConfig({ onboardingComplete: true });
223+
window.history.pushState({}, "", "/settings");
224+
225+
const spy = vi.spyOn(console, "error").mockImplementation(() => {});
226+
227+
render(() => <App />);
228+
229+
await waitFor(() => {
230+
screen.getByText("Failed to load page");
231+
});
232+
233+
spy.mockRestore();
234+
});
235+
236+
it("preloads dashboard chunk when auth token exists in localStorage", async () => {
237+
// happy-dom's localStorage is a Proxy; use vi.stubGlobal with a mock
238+
const store: Record<string, string> = { "github-tracker:auth-token": "test-token" };
239+
vi.stubGlobal("localStorage", {
240+
getItem: (key: string) => store[key] ?? null,
241+
setItem: (key: string, val: string) => { store[key] = val; },
242+
removeItem: (key: string) => { delete store[key]; },
243+
clear: () => { for (const k of Object.keys(store)) delete store[k]; },
244+
});
245+
246+
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
247+
248+
render(() => <App />);
249+
250+
await waitFor(() => {
251+
expect(vi.mocked(cacheStore.evictStaleEntries)).toHaveBeenCalled();
252+
});
253+
254+
vi.unstubAllGlobals();
255+
warnSpy.mockRestore();
256+
});
191257
});

tests/components/LoginPage.test.tsx

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@ vi.mock("../../src/app/lib/pat", async (importOriginal) => {
1616
};
1717
});
1818

19+
// Mock lazy-loaded component to prevent real imports during prefetch
20+
vi.mock("../../src/app/components/dashboard/DashboardPage", () => ({
21+
default: () => null,
22+
}));
23+
1924
// Full router mock — per project convention (SolidJS useNavigate requires Route context;
2025
// partial mocks of @solidjs/router render empty divs)
2126
const mockNavigate = vi.fn();
@@ -106,6 +111,27 @@ describe("LoginPage — OAuth view (default)", () => {
106111
await user.click(screen.getByText("Sign in with GitHub"));
107112
expect(window.location.href).toContain("https://github.com/login/oauth/authorize");
108113
});
114+
115+
it("schedules dashboard chunk prefetch on mount", () => {
116+
// happy-dom lacks requestIdleCallback, so the setTimeout(prefetch, 2000) fallback fires
117+
vi.useFakeTimers();
118+
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
119+
120+
render(() => <LoginPage />);
121+
122+
// The prefetch is scheduled via setTimeout with 2s delay
123+
const timeoutCalls = vi.getTimerCount();
124+
expect(timeoutCalls).toBeGreaterThan(0);
125+
126+
// Advancing the timer triggers the import — resolves to mock, no error
127+
vi.advanceTimersByTime(2000);
128+
129+
// No console.warn means the .catch() didn't fire (import succeeded via mock)
130+
expect(warnSpy).not.toHaveBeenCalledWith("[app] Dashboard chunk prefetch failed");
131+
132+
vi.useRealTimers();
133+
warnSpy.mockRestore();
134+
});
109135
});
110136

111137
describe("LoginPage — PAT form navigation", () => {

tests/lib/sentry.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ describe("beforeBreadcrumbHandler", () => {
179179
});
180180

181181
it("keeps allowed console breadcrumbs", () => {
182-
const prefixes = ["[auth]", "[api]", "[poll]", "[dashboard]", "[settings]"];
182+
const prefixes = ["[app]", "[auth]", "[api]", "[poll]", "[dashboard]", "[settings]"];
183183
for (const prefix of prefixes) {
184184
const breadcrumb = {
185185
category: "console",

0 commit comments

Comments
 (0)