diff --git a/.github/workflows/deploy-cloudflare.yml b/.github/workflows/deploy-cloudflare.yml index 02097ab3..6f071e40 100644 --- a/.github/workflows/deploy-cloudflare.yml +++ b/.github/workflows/deploy-cloudflare.yml @@ -160,9 +160,10 @@ jobs: # Workers Cache stores `/skills/trending` (see routeRules in # nuxt.config.ts). This proves a repeat visit is a hit, and that no stored # response sets a cookie, hides Markdown from an agent, or carries a - # session. It fails the job without a rollback: the rollback smoke - # re-checks the previous version, and a version from before the rule - # could never pass these checks. + # session. It also proves a stored page answers its ETag with a 304, and + # that a 304 never replaces the stored page. It fails the job without a + # rollback: the rollback smoke re-checks the previous version, and a + # version from before the rule could never pass these checks. - name: Check the trending edge cache run: pnpm production:edge-cache diff --git a/docs/arch/README.md b/docs/arch/README.md index 9f2fd304..ecce68d9 100644 --- a/docs/arch/README.md +++ b/docs/arch/README.md @@ -73,6 +73,7 @@ See [ADR-0007](../adr/0007-contract-in-the-cli-repository.md). - **Public pages render signed out**: the server never reads the session for a public page. The server render must not branch on User-Agent either, because `Vary` does not name it. Only then can one stored copy serve every visitor. A page that calls `useBotDetection()` on the server needs that call removed before it gets an edge cache rule. The browser loads the session after hydration. `useAuth().state` is `pending`, `anonymous`, or `signed-in`, and an auth-dependent control renders a same-size placeholder while it is `pending`. A page that needs the session on the server uses the `session` or `auth` route middleware, which also marks the response `private, no-store`. - **Session reads**: server code reads the session with `readUserSession()`. It opens a session only when the request carries one, because h3 sets a new session cookie on every other read. ESLint bans a direct `getUserSession()`. - **Markdown negotiation**: `shared/content-negotiation.ts` decides HTML or a 307 to the `.md` URL from `Accept` and `Sec-Fetch-Dest` only, and every page response names both in `Vary`. +- **HTML validators**: `shared/html-validators.ts` gives a page under an `edgeCache` rule a weak ETag: the build id plus a hash of the HTML. A request that sends it back gets an empty 304. A request with a cookie or an `Authorization` header gets no ETag. No page sends `Last-Modified`, because no one date bounds what a page renders. ## URL canonicals diff --git a/scripts/lib/edge-cache-check.ts b/scripts/lib/edge-cache-check.ts index 397275ef..194ee4d9 100644 --- a/scripts/lib/edge-cache-check.ts +++ b/scripts/lib/edge-cache-check.ts @@ -16,6 +16,9 @@ * 6. Every board is stored under its own key: boards that differ only by query * string render a different title or canonical URL. The bare path and the * default range are one board, so they may share both. + * 7. A stored page carries an ETag, and a request that sends it back gets a + * 304. A 304 must never be stored as the page: after a conditional request + * on a cold entry, the next plain request still gets the whole page. */ import { varyKeysEdgeCache } from '../../shared/content-negotiation' @@ -46,7 +49,7 @@ const SERVED_FROM_CACHE = new Set(['HIT', 'STALE', 'UPDATING']) export type EdgeCacheFetch = (input: string, init: RequestInit) => Promise -export type EdgeCacheRequest = 'anonymous' | 'markdown' | 'session-cookie' +export type EdgeCacheRequest = 'anonymous' | 'markdown' | 'session-cookie' | 'revalidation' export type EdgeCacheFailure = | { _tag: 'network-error', path: string, request: EdgeCacheRequest, message: string } @@ -58,6 +61,9 @@ export type EdgeCacheFailure | { _tag: 'rendered-for-session', path: string, reason: string } | { _tag: 'www-redirect', path: string, status: number | null, location: string | null, message?: string } | { _tag: 'shared-cache-entry', paths: string[], identity: string | null } + | { _tag: 'no-etag', path: string } + | { _tag: 'not-revalidated', path: string, status: number } + | { _tag: 'stored-not-modified', path: string, status: number } export type EdgeCacheCheckResult = | { _tag: 'passed', checks: Array<{ path: string, cacheStatuses: string[] }> } @@ -77,6 +83,8 @@ export interface EdgeCacheCheckDependencies { */ hitAttempts?: number retryDelayMs?: number + /** Makes the cold cache entry for the revalidation check. Defaults to a random UUID. */ + nonce?: string } type Fetched @@ -227,6 +235,42 @@ async function checkSessionCookie(path: string, url: string, fetch: EdgeCacheFet return null } +/** + * Runs after the anonymous check, so the entry is warm. Workers Cache stores + * one variant per `Accept` value, so an `Accept` that names the nonce reaches + * a cold entry that still renders HTML. + */ +async function checkRevalidation(path: string, url: string, fetch: EdgeCacheFetch, nonce: string): Promise { + const networkError = (message: string): EdgeCacheFailure => ({ _tag: 'network-error', path, request: 'revalidation', message }) + + const stored = await request(fetch, url, BROWSER_HEADERS) + if (stored._tag === 'error') + return networkError(stored.message) + if (stored.response.status !== 200) + return { _tag: 'status', path, request: 'revalidation', expected: 200, actual: stored.response.status } + const etag = stored.response.headers.get('etag') + if (!etag) + return { _tag: 'no-etag', path } + + const conditional = await request(fetch, url, { ...BROWSER_HEADERS, 'if-none-match': etag }) + if (conditional._tag === 'error') + return networkError(conditional.message) + if (conditional.response.status !== 304) + return { _tag: 'not-revalidated', path, status: conditional.response.status } + + // Either a 200 or a 304 is right here. The plain request after it is the check. + const cold = { ...BROWSER_HEADERS, accept: `${BROWSER_HEADERS.accept},application/x-edge-check;q=0.1;n=${nonce}` } + const coldConditional = await request(fetch, url, { ...cold, 'if-none-match': etag }) + if (coldConditional._tag === 'error') + return networkError(coldConditional.message) + const next = await request(fetch, url, cold) + if (next._tag === 'error') + return networkError(next.message) + if (next.response.status !== 200 || !next.body.includes('')) + return { _tag: 'stored-not-modified', path, status: next.response.status } + return null +} + function defaultWwwBaseUrl(baseUrl: string): string | null { const { protocol, hostname } = new URL(baseUrl) return hostname === 'skilld.dev' ? `${protocol}//www.${hostname}` : null @@ -251,6 +295,7 @@ export async function checkEdgeCache(dependencies: EdgeCacheCheckDependencies): const retryDelayMs = Math.max(0, Math.floor(dependencies.retryDelayMs ?? 5_000)) const paths = dependencies.paths ?? EDGE_CACHED_PATHS const wwwBaseUrl = dependencies.wwwBaseUrl === undefined ? defaultWwwBaseUrl(dependencies.baseUrl) : dependencies.wwwBaseUrl + const nonce = dependencies.nonce ?? crypto.randomUUID() const failures: EdgeCacheFailure[] = [] const checks: Array<{ path: string, cacheStatuses: string[] }> = [] @@ -264,6 +309,9 @@ export async function checkEdgeCache(dependencies: EdgeCacheCheckDependencies): else { checks.push({ path, cacheStatuses: anonymous.cacheStatuses }) identities.set(anonymous.identity, [...identities.get(anonymous.identity) ?? [], path]) + const revalidation = await checkRevalidation(path, url, fetch, nonce) + if (revalidation) + failures.push(revalidation) } // After the anonymous loop, so the apex entry is warm. diff --git a/server/plugins/html-validators.ts b/server/plugins/html-validators.ts new file mode 100644 index 00000000..ddff691b --- /dev/null +++ b/server/plugins/html-validators.ts @@ -0,0 +1,34 @@ +import { getRequestHeader, getResponseHeader, setResponseHeader } from 'h3' +import { useRuntimeConfig } from 'nitropack/runtime' +import { decideHtmlValidators } from '#shared/html-validators' + +/** + * Give an edge cached page an ETag, and answer a request that already holds + * the page with an empty 304. `shared/html-validators.ts` decides both. + * + * The 304 keeps the route's cache headers, so a revalidating Workers Cache + * entry stays fresh for the route's lifetime again. + */ +export default defineNitroPlugin((nitroApp) => { + nitroApp.hooks.hook('render:response', async (response, { event }) => { + const header = (value: string | number | string[] | undefined): string | null => + value === undefined ? null : Array.isArray(value) ? value.join(', ') : String(value) + const decision = await decideHtmlValidators({ + buildId: useRuntimeConfig().app.buildId, + method: event.method, + status: response.statusCode ?? 200, + body: response.body, + request: { get: name => header(getRequestHeader(event, name)) }, + response: { get: name => header(getResponseHeader(event, name)) }, + }) + if (decision._tag === 'skip') + return + + setResponseHeader(event, 'etag', decision.etag) + if (decision._tag === 'not-modified') { + response.statusCode = 304 + response.statusMessage = 'Not Modified' + response.body = '' + } + }) +}) diff --git a/shared/html-validators.ts b/shared/html-validators.ts new file mode 100644 index 00000000..02cea7eb --- /dev/null +++ b/shared/html-validators.ts @@ -0,0 +1,103 @@ +/** + * ETags for the HTML pages Workers Cache stores, and the 304 that answers one. + * + * Without a validator, every recrawl downloads every page again, and Workers + * Cache replaces a stale page with a full render. With one, a crawler that + * sends `If-None-Match` gets a 304, and a revalidating cache keeps its body. + * + * The ETag is weak: a weak ETag promises the same page, not the same bytes. + * Cloudflare compresses responses and may rewrite a strong ETag. Each render + * also carries a fresh Sentry trace id, which is not page content. + * + * There is no `Last-Modified`. A Skill page shows stars, ages and related + * Skills, so no single date bounds what it renders. A date that misses a + * change makes a cache answer `If-Modified-Since` with a wrong 304. A request + * that sends only `If-Modified-Since` gets the full page. + */ + +export interface HeaderReader { + get: (name: string) => string | null +} + +export interface RenderedPage { + /** The Nuxt build id. A deploy changes it, so no ETag survives a deploy. */ + buildId: string + method: string + status: number + /** A string for a whole rendered document. */ + body: unknown + request: HeaderReader + response: HeaderReader +} + +export type HtmlValidatorSkipReason = 'method' | 'status' | 'not-html' | 'credentials' | 'sets-cookie' | 'not-shared' + +export type HtmlValidatorDecision + = | { _tag: 'skip', reason: HtmlValidatorSkipReason } + | { _tag: 'send', etag: string } + | { _tag: 'not-modified', etag: string } + +/** + * The request headers `@harlan-zw/nuxt-cloudflare` treats as credentials. + * A page rendered for a request with one is never stored, so it gets no ETag. + */ +const CREDENTIAL_HEADERS = ['cookie', 'authorization', 'proxy-authorization', 'cf-access-jwt-assertion', 'cf-access-client-id', 'x-api-key'] + +const HTML_TYPE_RE = /^(?:text\/html|application\/xhtml\+xml)\s*(?:;|$)/i + +/** Sentry writes these into every render with a new trace id. */ +const PER_RENDER_META_RE = //g + +const ENTITY_TAG_RE = /(?:W\/)?"[^"]*"/g + +export async function decideHtmlValidators(page: RenderedPage): Promise { + if (page.method !== 'GET' && page.method !== 'HEAD') + return { _tag: 'skip', reason: 'method' } + if (page.status !== 200) + return { _tag: 'skip', reason: 'status' } + if (typeof page.body !== 'string' || !HTML_TYPE_RE.test(page.response.get('content-type') ?? '')) + return { _tag: 'skip', reason: 'not-html' } + if (CREDENTIAL_HEADERS.some(name => page.request.get(name))) + return { _tag: 'skip', reason: 'credentials' } + if (page.response.get('set-cookie')) + return { _tag: 'skip', reason: 'sets-cookie' } + if (!sharedCacheMayStore(page.response.get('cloudflare-cdn-cache-control'))) + return { _tag: 'skip', reason: 'not-shared' } + + const etag = `W/"${page.buildId}-${await contentHash(page.body.replace(PER_RENDER_META_RE, ''))}"` + return ifNoneMatchHits(page.request.get('if-none-match'), etag) + ? { _tag: 'not-modified', etag } + : { _tag: 'send', etag } +} + +/** + * True when the edge policy lets Workers Cache store the page for one visitor + * and serve it to the next. Only such a page is the same for everyone. + */ +function sharedCacheMayStore(cacheControl: string | null): boolean { + let lifetime = 0 + for (const directive of (cacheControl ?? '').toLowerCase().split(',')) { + const [name = '', value] = directive.trim().split('=', 2) + // `private="set-cookie"` keeps one header private, never the page. + if (name === 'no-store' || (name === 'private' && value === undefined)) + return false + if (name === 'max-age' || name === 's-maxage') + lifetime = Math.max(lifetime, Number(value) || 0) + } + return lifetime > 0 +} + +/** Weak comparison (RFC 9110 section 8.8.3.2), which `If-None-Match` uses. */ +function ifNoneMatchHits(header: string | null, etag: string): boolean { + if (!header) + return false + if (header.trim() === '*') + return true + const opaque = etag.replace(/^W\//, '') + return (header.match(ENTITY_TAG_RE) ?? []).some(tag => tag.replace(/^W\//, '') === opaque) +} + +async function contentHash(html: string): Promise { + const digest = await crypto.subtle.digest('SHA-256', new TextEncoder().encode(html)) + return Array.from(new Uint8Array(digest).slice(0, 16), byte => byte.toString(16).padStart(2, '0')).join('') +} diff --git a/test/unit/edge-cache-check.test.ts b/test/unit/edge-cache-check.test.ts index 4f31a9ee..995458fe 100644 --- a/test/unit/edge-cache-check.test.ts +++ b/test/unit/edge-cache-check.test.ts @@ -23,22 +23,28 @@ interface Answer { body?: string } +const STORED_ETAG = 'W/"build-1-stored"' + function response(answer: Answer, path = '/skills/trending'): Response { const headers = new Headers({ 'content-type': 'text/html;charset=utf-8', 'vary': 'Accept, Sec-Fetch-Dest, Host', + ...((answer.status ?? 200) === 200 ? { etag: STORED_ETAG } : {}), ...answer.headers, }) for (const cookie of answer.cookies ?? []) headers.append('set-cookie', cookie) - return new Response(answer.body ?? page(null, false, path), { status: answer.status ?? 200, headers }) + const status = answer.status ?? 200 + // A 304 has no body, and the Response constructor refuses one. + return new Response(status === 304 ? null : answer.body ?? page(null, false, path), { status, headers }) } /** * A fake edge. Every anonymous request after the first to a path is a hit, - * unless a test overrides one kind of request. + * and a request that sends the stored ETag back gets a 304, unless a test + * overrides one kind of request. */ -function edge(overrides: Partial Answer>> = {}): EdgeCacheFetch { +function edge(overrides: Partial Answer>> = {}): EdgeCacheFetch { const seen = new Map() return async (url, init) => { const { pathname, search } = new URL(url) @@ -63,6 +69,13 @@ function edge(overrides: Partial { }) }) + it('sends the stored page\'s ETag back and expects a 304', async () => { + const conditional: Array = [] + const inner = edge() + const result = await run(async (url, init) => { + const headers = new Headers(init.headers) + if (headers.get('if-none-match')) + conditional.push(headers.get('if-none-match')) + return inner(url, init) + }) + + expect(result._tag).toBe('passed') + expect(conditional).toEqual([STORED_ETAG, STORED_ETAG]) + }) + + it('fails when a stored page carries no ETag', async () => { + const result = await run(edge({ + anonymous: (_path, count) => ({ headers: { 'cf-cache-status': count === 1 ? 'MISS' : 'HIT', 'etag': '' } }), + })) + + expect(result).toMatchObject({ _tag: 'failed', failures: [{ _tag: 'no-etag', path: '/skills/trending' }] }) + }) + + it('fails when the edge answers a matching ETag with the whole page', async () => { + const result = await run(edge({ conditional: () => ({ headers: { 'cf-cache-status': 'HIT' } }) })) + + expect(result).toMatchObject({ _tag: 'failed', failures: [{ _tag: 'not-revalidated', path: '/skills/trending', status: 200 }] }) + }) + + it('fails when a conditional request on a cold entry leaves a 304 for the next visitor', async () => { + const inner = edge() + const storedNotModified = new Set() + const result = await run(async (url, init) => { + const headers = new Headers(init.headers) + const variant = headers.get('accept') ?? '' + if (headers.get('if-none-match') && variant.includes('x-edge-check')) + storedNotModified.add(variant) + if (!headers.get('if-none-match') && storedNotModified.has(variant)) + return response({ status: 304, body: '' }) + return inner(url, init) + }) + + expect(result).toMatchObject({ _tag: 'failed', failures: [{ _tag: 'stored-not-modified', path: '/skills/trending', status: 304 }] }) + }) + it('reports a network error instead of throwing', async () => { const result = await run(async () => { throw new Error('connect ECONNREFUSED') diff --git a/test/unit/html-validators.test.ts b/test/unit/html-validators.test.ts new file mode 100644 index 00000000..6a077734 --- /dev/null +++ b/test/unit/html-validators.test.ts @@ -0,0 +1,169 @@ +import type { HtmlValidatorDecision, RenderedPage } from '../../shared/html-validators' +import { IncomingMessage, ServerResponse } from 'node:http' +import { Socket } from 'node:net' +import { createEvent, setResponseHeader } from 'h3' +import { describe, expect, it, vi } from 'vitest' +import registerPlugin from '../../server/plugins/html-validators' +import { decideHtmlValidators } from '../../shared/html-validators' + +vi.hoisted(() => { + vi.stubGlobal('defineNitroPlugin', (plugin: unknown) => plugin) +}) +vi.mock('nitropack/runtime', () => ({ useRuntimeConfig: () => ({ app: { buildId: 'build-1' } }) })) + +const EDGE = 'public, max-age=300, stale-while-revalidate=3600' +const HTML = 'skill-creator

skill-creator

' + +interface PageInput { + buildId?: string + method?: string + status?: number + body?: unknown + request?: Record + response?: Record +} + +function page(input: PageInput = {}): RenderedPage { + return { + buildId: input.buildId ?? 'build-1', + method: input.method ?? 'GET', + status: input.status ?? 200, + body: 'body' in input ? input.body : HTML, + request: new Headers(input.request), + response: new Headers({ 'content-type': 'text/html;charset=utf-8', 'cloudflare-cdn-cache-control': EDGE, ...input.response }), + } +} + +function etagOf(decision: HtmlValidatorDecision): string { + if (decision._tag === 'skip') + throw new Error(`Expected an ETag, got skip: ${decision.reason}`) + return decision.etag +} + +function withTrace(traceId: string): string { + return HTML.replace('', ``) +} + +describe('decideHtmlValidators', () => { + it('answers a repeat request for an edge cached page with not-modified', async () => { + const first = await decideHtmlValidators(page()) + expect(first).toEqual({ _tag: 'send', etag: expect.stringMatching(/^W\/"build-1-[0-9a-f]{32}"$/) }) + + const repeat = await decideHtmlValidators(page({ request: { 'if-none-match': etagOf(first) } })) + expect(repeat).toEqual({ _tag: 'not-modified', etag: etagOf(first) }) + }) + + it.each([ + ['a list that holds it', (etag: string) => `W/"stale", ${etag}`], + ['its strong form', (etag: string) => etag.slice(2)], + ['a wildcard', () => '*'], + ])('treats %s as a match', async (_label, header) => { + const etag = etagOf(await decideHtmlValidators(page())) + + expect(await decideHtmlValidators(page({ request: { 'if-none-match': header(etag) } }))) + .toEqual({ _tag: 'not-modified', etag }) + }) + + it('sends the page again when its content changed', async () => { + const before = etagOf(await decideHtmlValidators(page())) + + const after = await decideHtmlValidators(page({ body: HTML.replace('

', '

Updated '), request: { 'if-none-match': before } })) + + expect(after._tag).toBe('send') + expect(etagOf(after)).not.toBe(before) + }) + + it('sends the page again after a deploy, even when the HTML is identical', async () => { + const before = etagOf(await decideHtmlValidators(page({ buildId: 'build-1' }))) + + const after = await decideHtmlValidators(page({ buildId: 'build-2', request: { 'if-none-match': before } })) + + expect(after).toEqual({ _tag: 'send', etag: expect.stringMatching(/^W\/"build-2-/) }) + }) + + it('gives two renders that differ only in their Sentry trace one ETag', async () => { + const first = etagOf(await decideHtmlValidators(page({ body: withTrace('2edc18a9059f4c47a66214500bfcd5bc') }))) + const second = etagOf(await decideHtmlValidators(page({ body: withTrace('13c5824de8824afeb5d2ab8e2117c5e9') }))) + + expect(second).toBe(first) + }) + + it.each<[string, PageInput, string]>([ + ['a request with a cookie', { request: { cookie: 'theme=dark' } }, 'credentials'], + ['a request with an authorization header', { request: { authorization: 'Bearer token' } }, 'credentials'], + ['a page that sets a cookie', { response: { 'set-cookie': 'nuxt-session=abc; Path=/' } }, 'sets-cookie'], + ['a page the edge must not store', { response: { 'cloudflare-cdn-cache-control': 'private, no-store' } }, 'not-shared'], + ['a page with no edge policy', { response: { 'cloudflare-cdn-cache-control': '' } }, 'not-shared'], + ['a page with a zero edge lifetime', { response: { 'cloudflare-cdn-cache-control': 'public, max-age=0' } }, 'not-shared'], + ['an error page', { status: 404 }, 'status'], + ['a form post', { method: 'POST' }, 'method'], + ['a JSON payload', { response: { 'content-type': 'application/json' } }, 'not-html'], + ['a streamed body', { body: new ReadableStream() }, 'not-html'], + ])('sends no validator for %s', async (_label, input, reason) => { + expect(await decideHtmlValidators(page({ ...input, request: { 'if-none-match': '*', ...input.request } }))) + .toEqual({ _tag: 'skip', reason }) + }) + + it('keeps validators on a page whose cookie rule only names Set-Cookie', async () => { + const decision = await decideHtmlValidators(page({ response: { 'cloudflare-cdn-cache-control': `${EDGE}, private="set-cookie"` } })) + + expect(decision._tag).toBe('send') + }) +}) + +describe('html validators plugin', () => { + interface RenderResponse { body: unknown, statusCode?: number, statusMessage?: string } + type RenderHook = (response: RenderResponse, context: { event: ReturnType }) => Promise + + function renderHook(): RenderHook { + let hook: RenderHook | undefined + registerPlugin({ hooks: { hook: (name: string, callback: RenderHook) => { + if (name === 'render:response') + hook = callback + } } } as unknown as Parameters[0]) + if (!hook) + throw new Error('The plugin registered no render:response hook') + return hook + } + + function event(headers: Record = {}) { + const req = new IncomingMessage(new Socket()) + req.method = 'GET' + req.url = '/gh/anthropics/skills/skill-creator' + req.headers = { host: 'skilld.dev', ...headers } + const res = new ServerResponse(req) + const h3Event = createEvent(req, res) + setResponseHeader(h3Event, 'content-type', 'text/html;charset=utf-8') + setResponseHeader(h3Event, 'cloudflare-cdn-cache-control', EDGE) + return { event: h3Event, res } + } + + it('tags the rendered page, then answers its ETag with an empty 304', async () => { + const hook = renderHook() + const first = event() + const rendered: RenderResponse = { body: HTML, statusCode: 200 } + await hook(rendered, { event: first.event }) + const etag = String(first.res.getHeader('etag')) + + expect(etag).toMatch(/^W\/"build-1-/) + expect(rendered).toEqual({ body: HTML, statusCode: 200 }) + + const repeat = event({ 'if-none-match': etag }) + const revalidated: RenderResponse = { body: HTML, statusCode: 200 } + await hook(revalidated, { event: repeat.event }) + + expect(repeat.res.getHeader('etag')).toBe(etag) + expect(revalidated).toEqual({ body: '', statusCode: 304, statusMessage: 'Not Modified' }) + expect(repeat.res.getHeader('cloudflare-cdn-cache-control')).toBe(EDGE) + }) + + it('leaves a signed-in request without an ETag', async () => { + const hook = renderHook() + const signedIn = event({ 'cookie': 'nuxt-session=abc', 'if-none-match': '*' }) + const rendered: RenderResponse = { body: HTML, statusCode: 200 } + await hook(rendered, { event: signedIn.event }) + + expect(signedIn.res.getHeader('etag')).toBeUndefined() + expect(rendered).toEqual({ body: HTML, statusCode: 200 }) + }) +})