diff --git a/.claude/rules/sveltekit.md b/.claude/rules/sveltekit.md index 6e6ca32bb..c04634b8b 100644 --- a/.claude/rules/sveltekit.md +++ b/.claude/rules/sveltekit.md @@ -25,6 +25,20 @@ All internal navigation must use `resolve()` from `$app/paths`: - **With search/hash**: `resolve(pathname + search + hash)` (concatenate inside, not outside) - **External**: no `resolve()`, use `rel="noreferrer external"` +## HTTP Cache Headers (`setHeaders`) + +**Never set `s-maxage` / `stale-while-revalidate` on a route whose content varies by session.** +Shared caches key on URL + method + the headers named by `Vary` (RFC 9111), so without +`Vary: Cookie` an anonymous response is served to logged-in users (#3862). + +- Setting the header only when logged out controls the write side, not delivery of an + already-cached entry to a request with a session cookie. +- `Vary: Cookie` matches the whole header; with `_ga` present every returning visitor gets a + unique key and the cache barely hits. +- No `s-maxage` means no CDN caching, so `private` / `no-store` are unnecessary (YAGNI). +- Estimate the saved invocations first. At this site's traffic the `/problems` header saved + single-digit dollars a month and cost two weeks of a production bug. + ## Form Data Validation `formData.get()` returns `string | File | null`. Always validate before casting: diff --git a/e2e/problems_cache.spec.ts b/e2e/problems_cache.spec.ts index 1c28c9b4d..6301d3c86 100644 --- a/e2e/problems_cache.spec.ts +++ b/e2e/problems_cache.spec.ts @@ -2,8 +2,13 @@ import { expect, test } from '@playwright/test'; import { loginAsUser } from './helpers/auth'; +// Regression guard for #3862. A shared cache keys on URL + method + the headers named by +// Vary (RFC 9111), so an anonymous response cached without `Vary: Cookie` is also served to +// logged-in users — showing them the signed-out page. /problems must not opt into the shared +// cache at all, for anonymous and logged-in responses alike. + test.describe('anonymous /problems response', () => { - test('is cache-eligible (cache-control set, no set-cookie)', async ({ page }) => { + test('is not shared-cached (no CDN cache-control directive)', async ({ page }) => { const response = await page.goto('/problems'); if (!response) { @@ -12,17 +17,11 @@ test.describe('anonymous /problems response', () => { const headers = response.headers(); - // Local dev server (pnpm preview) is not behind Vercel edge, so s-maxage is visible. - expect(headers['cache-control']).toContain('public'); - expect(headers['cache-control']).toContain('max-age=0'); - expect(headers['cache-control']).toContain('s-maxage=300'); - expect(headers['cache-control']).toContain('stale-while-revalidate=600'); - - // set-cookie makes a response ineligible for CDN caching. - // The auth layer must not attach a session cookie to anonymous requests. - // If this assertion fails, verify the session handling does not set cookies - // on unauthenticated requests. - expect(headers['set-cookie']).toBeUndefined(); + // Local dev server (pnpm preview) is not behind Vercel edge, so s-maxage would be + // visible here if it were re-introduced. + expect(headers['cache-control'] ?? '').not.toContain('public'); + expect(headers['cache-control'] ?? '').not.toContain('s-maxage'); + expect(headers['cache-control'] ?? '').not.toContain('stale-while-revalidate'); }); }); @@ -37,7 +36,6 @@ test.describe('logged-in /problems response', () => { const headers = response.headers(); - // Personalized responses must never be shared-cached. expect(headers['cache-control'] ?? '').not.toContain('public'); expect(headers['cache-control'] ?? '').not.toContain('s-maxage'); }); diff --git a/src/routes/problems/+page.server.ts b/src/routes/problems/+page.server.ts index 7e333f936..1551d2b35 100644 --- a/src/routes/problems/+page.server.ts +++ b/src/routes/problems/+page.server.ts @@ -11,7 +11,7 @@ import { voteAbsoluteGradeSchema } from '$features/votes/zod/schema'; import { BAD_REQUEST } from '$lib/constants/http-response-status-codes'; // 一覧表ページは、ログインしていなくても閲覧できるようにする -export async function load({ locals, url, setHeaders }) { +export async function load({ locals, url }) { const session = await locals.auth.validate(); const params = await url.searchParams; @@ -21,24 +21,13 @@ export async function load({ locals, url, setHeaders }) { // Degrade gracefully if vote stats are unavailable — the problems page must remain accessible. let voteResults = new Map(); - let voteStatsOk = true; try { voteResults = await getVoteGradeStatistics(); } catch (error) { - voteStatsOk = false; console.error('Failed to load vote statistics:', error); } - // Anonymous responses are identical for all users and contain no per-user - // answer state, so they are safe to cache at the CDN. Logged-in responses - // are personalized and must never be shared-cached. - // Skip caching a degraded response (vote stats failed) to avoid pinning a - // broken page at the edge for the full TTL. - if (session === null && voteStatsOk) { - setHeaders({ 'Cache-Control': 'public, max-age=0, s-maxage=300, stale-while-revalidate=600' }); - } - const taskResults = ( tagIds ? await task_crud.getTasksWithTagIds(tagIds, session?.user.userId) diff --git a/src/routes/problems/page_server.test.ts b/src/routes/problems/page_server.test.ts index 28ee6d845..f69682584 100644 --- a/src/routes/problems/page_server.test.ts +++ b/src/routes/problems/page_server.test.ts @@ -60,33 +60,25 @@ beforeEach(() => { }); describe('load() cache-control behaviour', () => { - describe('sets cache-control', () => { - test('anonymous users get a public shared-cache header when vote stats succeed', async () => { + // Shared caches key on URL + method + headers named by Vary only (RFC 9111), so an + // anonymous response cached without `Vary: Cookie` is served to logged-in users too. + describe('never sets a shared-cache header — shared caches ignore cookies without Vary (#3862)', () => { + test('anonymous users', async () => { const event = createMockEvent({ session: null }); await load(event); - expect(event.setHeaders).toHaveBeenCalledOnce(); - const headerArg = event.setHeaders.mock.calls[0][0] as Record; - expect(headerArg['Cache-Control']).toBe( - 'public, max-age=0, s-maxage=300, stale-while-revalidate=600', - ); + expect(event.setHeaders).not.toHaveBeenCalled(); }); - test('anonymous users with tagIds also get a public shared-cache header', async () => { + test('anonymous users with tagIds', async () => { const event = createMockEvent({ session: null, tagIds: 'abc,dp' }); await load(event); - expect(event.setHeaders).toHaveBeenCalledOnce(); - const headerArg = event.setHeaders.mock.calls[0][0] as Record; - expect(headerArg['Cache-Control']).toBe( - 'public, max-age=0, s-maxage=300, stale-while-revalidate=600', - ); + expect(event.setHeaders).not.toHaveBeenCalled(); }); - }); - describe('does not set cache-control', () => { test('logged-in users — personalized response must never be shared-cached', async () => { const event = createMockEvent({ session: LOGGED_IN_SESSION }); @@ -95,7 +87,7 @@ describe('load() cache-control behaviour', () => { expect(event.setHeaders).not.toHaveBeenCalled(); }); - test('degraded response when vote stats fail — avoids pinning a broken page at the CDN', async () => { + test('degraded response when vote stats fail', async () => { mockGetVoteGradeStatistics.mockRejectedValue(new Error('DB timeout')); const event = createMockEvent({ session: null });