Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions e2e-tests/directory.s3.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,10 @@ for (const path of ['/dist/', '/docs/']) {
await res.text();

expect(res.status).toBe(200);
// Directory listings change as files are added, so they're cached mutably.
expect(res.headers.get('cache-control')).toStrictEqual(
CACHE_HEADERS.mutable
);
});
}

Expand Down
2 changes: 1 addition & 1 deletion e2e-tests/fallback.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ test('grabs file from fallback server if r2 request fails', async () => {
);

expect(res.status).toBe(200);
expect(res.headers.get('cache-control')).toStrictEqual(CACHE_HEADERS.success);
expect(res.headers.get('cache-control')).toStrictEqual(CACHE_HEADERS.mutable);
expect(originCalled).toBeTruthy();
expect(await res.text()).toStrictEqual(originResponse);
});
Expand Down
86 changes: 83 additions & 3 deletions e2e-tests/file.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { env, createExecutionContext } from 'cloudflare:test';
import { test, beforeAll, expect } from 'vitest';
import { test, beforeAll, afterEach, expect, vi } from 'vitest';
import { populateR2WithDevBucket } from './util';
import worker from '../src/worker';
import type { Env } from '../src/env';
Expand All @@ -16,6 +16,86 @@ beforeAll(async () => {
await populateR2WithDevBucket();
});

afterEach(() => {
vi.restoreAllMocks();
});

test('GET a versioned asset is cached immutably', async () => {
const ctx = createExecutionContext();

const res = await worker.fetch(
new Request('https://localhost/dist/v20.0.0/SHASUMS256.txt'),
mockedEnv,
ctx
);

// Consume the body promise
await res.text();

expect(res.status).toBe(200);
expect(res.headers.get('cache-control')).toStrictEqual(
CACHE_HEADERS.immutable
);
});

test('HEAD a versioned asset is cached immutably', async () => {
const ctx = createExecutionContext();

const res = await worker.fetch(
new Request('https://localhost/dist/v20.0.0/SHASUMS256.txt', {
method: 'HEAD',
}),
mockedEnv,
ctx
);

// Consume the body promise
await res.text();

expect(res.status).toBe(200);
expect(res.headers.get('cache-control')).toStrictEqual(
CACHE_HEADERS.immutable
);
});

test('GET an invalid object name returns 400 with the failure cache policy', async () => {
vi.spyOn(env.R2_BUCKET, 'get').mockImplementation(() => {
// R2 error 10020: object name not valid
throw new Error('10020: The specified key does not exist.');
});

const ctx = createExecutionContext();

const res = await worker.fetch(
new Request('https://localhost/dist/v20.0.0/SHASUMS256.txt'),
mockedEnv,
ctx
);

expect(res.status).toBe(400);
expect(res.headers.get('cache-control')).toStrictEqual(CACHE_HEADERS.failure);
});

test('GET an out-of-bounds range returns 416 with the failure cache policy', async () => {
vi.spyOn(env.R2_BUCKET, 'get').mockImplementation(() => {
// R2 error 10039: range not satisfiable
throw new Error('10039: The requested range is not satisfiable.');
});

const ctx = createExecutionContext();

const res = await worker.fetch(
new Request('https://localhost/dist/v20.0.0/SHASUMS256.txt', {
headers: { range: 'bytes=999999-1000000' },
}),
mockedEnv,
ctx
);

expect(res.status).toBe(416);
expect(res.headers.get('cache-control')).toStrictEqual(CACHE_HEADERS.failure);
});

test('GET `/dist/index.json` returns 200', async () => {
const ctx = createExecutionContext();

Expand All @@ -29,7 +109,7 @@ test('GET `/dist/index.json` returns 200', async () => {
await res.text();

expect(res.status).toBe(200);
expect(res.headers.get('cache-control')).toStrictEqual(CACHE_HEADERS.success);
expect(res.headers.get('cache-control')).toStrictEqual(CACHE_HEADERS.mutable);
});

test('GET `/dist/asd123.json` returns 404', async () => {
Expand Down Expand Up @@ -223,7 +303,7 @@ test('`if-match` header', async () => {

expect(res.status).toBe(200);
expect(res.headers.get('cache-control')).toStrictEqual(
CACHE_HEADERS.success
CACHE_HEADERS.mutable
);
}
});
Expand Down
3 changes: 2 additions & 1 deletion src/constants/cache.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
export const CACHE_HEADERS = {
success: 'public, max-age=3600, s-maxage=14400',
immutable: 'public, immutable, max-age=31536000, s-maxage=31536000',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In light of https://github.com/nodejs/web-team/blob/main/incidents/2026-03-04.md, debating if we really would want it to be cached this long, especially since what if it happens again and there is a threat actor behind it, wdyt @nodejs/web-infra.

I definitely think it should be cached for longer, but unsure on exactly how long

mutable: 'public, max-age=86400, s-maxage=86400, must-revalidate',
failure: 'private, no-cache, no-store, max-age=0, must-revalidate',
};
8 changes: 6 additions & 2 deletions src/middleware/originMiddleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { CACHE_HEADERS } from '../constants/cache';
import type { Context } from '../context';
import type { Request } from '../routes/request';
import { isDirectoryPath } from '../utils/path';
import { cacheControlFor } from '../utils/cache';
import { getOriginalUrl } from '../utils/request';
import type { Middleware } from './middleware';

/**
Expand All @@ -19,6 +21,8 @@ export class OriginMiddleware implements Middleware {
message: 'hit',
});

const originPathname = getOriginalUrl(request).pathname;

const res = await fetch(
`${ctx.env.ORIGIN_HOST}${request.urlObj.pathname}`,
{
Expand Down Expand Up @@ -53,9 +57,9 @@ export class OriginMiddleware implements Middleware {
// Don't cache this response on the client if it's a directory listing,
// since our listing response might end up differently from nginx's at
// some point
'cache-control': isDirectoryPath(request.urlObj.pathname)
'cache-control': isDirectoryPath(originPathname)
? CACHE_HEADERS.failure
: CACHE_HEADERS.success,
: cacheControlFor(originPathname, res.status),
},
});
}
Expand Down
39 changes: 29 additions & 10 deletions src/middleware/r2Middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,16 @@ import * as Sentry from '@sentry/cloudflare';
import { CACHE_HEADERS } from '../constants/cache';
import docsDirectory from '../constants/docsDirectory.json' assert { type: 'json' };
import type { Context } from '../context';
import type { GetFileResult } from '../providers/provider';
import type { GetFileResult, HeadFileResult } from '../providers/provider';
import { R2Provider } from '../providers/r2Provider';
import responses from '../responses';
import { hasTrailingSlash, isDirectoryPath } from '../utils/path';
import { cacheControlFor } from '../utils/cache';
import type { Middleware } from './middleware';
import latestVersions from '../constants/latestVersions.json' assert { type: 'json' };
import type { Request } from '../routes/request';
import { renderDirectoryListing } from '../utils/directoryListing';
import { parseConditionalHeaders } from '../utils/request';
import { getOriginalUrl, parseConditionalHeaders } from '../utils/request';
import { once } from '../utils/memo';

const getProvider = once((ctx: Context) => new R2Provider({ ctx }));
Expand Down Expand Up @@ -41,7 +42,7 @@ async function handleDirectory(
): Promise<Response> {
if (!hasTrailingSlash(request.urlObj.pathname)) {
// We always want directory listing requests to have a trailing slash
const url = request.unsubstitutedUrl ?? request.urlObj;
const url = getOriginalUrl(request);
return Response.redirect(`${url}/`, 301);
}
Comment thread
flakey5 marked this conversation as resolved.

Expand All @@ -58,21 +59,29 @@ async function handleDirectory(

let responseBody;
if (request.method === 'GET') {
responseBody = renderDirectoryListing(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directory index mutable cache

Low Severity

When a directory request is satisfied by serving index.html instead of an HTML listing, setCacheControl still uses the trailing-slash pathname. isImmutablePath then treats the response as a mutable directory URL, so versioned static index files can miss the long-lived immutable cache policy they would get at the same path with index.html in the URL.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 82a6785. Configure here.

request.unsubstitutedUrl ?? request.urlObj,
result
);
responseBody = renderDirectoryListing(getOriginalUrl(request), result);
}

return new Response(responseBody, {
headers: {
'last-modified': result.lastModified.toUTCString(),
'content-type': 'text/html',
'cache-control': CACHE_HEADERS.success,
// Directory listings change as files are added.
'cache-control': CACHE_HEADERS.mutable,
},
});
}

function setCacheControl(
result: GetFileResult | HeadFileResult,
request: Request
): void {
result.httpHeaders['cache-control'] = cacheControlFor(
getOriginalUrl(request).pathname,
result.httpStatusCode
);
}

function handleFile(
request: Request,
r2Path: string,
Expand All @@ -99,6 +108,8 @@ async function headFile(
return responses.fileNotFound(request.method);
}

setCacheControl(result, request);

return new Response(undefined, {
status: result.httpStatusCode,
headers: result.httpHeaders,
Expand All @@ -121,10 +132,16 @@ async function getFile(
if (err instanceof Error) {
if (err.message.includes('10020')) {
// Object name not valid, url probably has some weirdness in it
return new Response(undefined, { status: 400 });
return new Response(undefined, {
status: 400,
headers: { 'cache-control': CACHE_HEADERS.failure },
});
} else if (err.message.includes('10039')) {
// Range not compatible, probably out of bounds
return new Response(undefined, { status: 416 });
return new Response(undefined, {
status: 416,
headers: { 'cache-control': CACHE_HEADERS.failure },
});
}
}

Expand All @@ -135,6 +152,8 @@ async function getFile(
return responses.fileNotFound(request.method);
}

setCacheControl(result, request);

return new Response(result.contents, {
status: result.httpStatusCode,
headers: result.httpHeaders,
Expand Down
15 changes: 6 additions & 9 deletions src/providers/r2Provider.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { CACHE_HEADERS } from '../constants/cache';
import { R2_RETRY_LIMIT } from '../../lib/limits.mjs';
import CACHED_DIRECTORIES from '../constants/cachedDirectories.json' assert { type: 'json' };
import { CONTENT_TYPE_OVERRIDES } from '../constants/contentTypeOverrides';
Expand Down Expand Up @@ -58,7 +57,7 @@ export class R2Provider implements Provider {

return {
httpStatusCode: 200,
httpHeaders: r2MetadataToHeaders(object, 200),
httpHeaders: r2MetadataToHeaders(object),
};
}

Expand Down Expand Up @@ -97,7 +96,7 @@ export class R2Provider implements Provider {
return {
contents: hasBody ? (object as R2ObjectBody).body : undefined,
httpStatusCode,
httpHeaders: r2MetadataToHeaders(object, httpStatusCode),
httpHeaders: r2MetadataToHeaders(object),
};
}

Expand Down Expand Up @@ -143,10 +142,7 @@ export class R2Provider implements Provider {
}
}

function r2MetadataToHeaders(
object: R2Object,
httpStatusCode: number
): HttpResponseHeaders {
function r2MetadataToHeaders(object: R2Object): HttpResponseHeaders {
const { httpMetadata } = object;

const fileExtension = object.key.substring(object.key.lastIndexOf('.') + 1);
Expand All @@ -164,8 +160,9 @@ function r2MetadataToHeaders(
'accept-range': 'bytes',
// https://github.com/nodejs/build/blob/e3df25d6a23f033db317a53ab1e904c953ba1f00/ansible/www-standalone/resources/config/nodejs.org?plain=1#L194-L196
'access-control-allow-origin': object.key.endsWith('.json') ? '*' : '',
'cache-control':
httpStatusCode === 200 ? CACHE_HEADERS.success : CACHE_HEADERS.failure,
// Set by R2Middleware, which has access to the original request URL needed
// to decide between immutable/mutable/failure cache policies.
'cache-control': '',
expires: httpMetadata?.cacheExpiry?.toUTCString() ?? '',
'last-modified': getLastModified(object),
'content-language': httpMetadata?.contentLanguage ?? '',
Expand Down
67 changes: 67 additions & 0 deletions src/utils/cache.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { describe, test, expect } from 'vitest';
import { isImmutablePath, cacheControlFor } from './cache';
import { CACHE_HEADERS } from '../constants/cache';

describe('isImmutablePath', () => {
test.each([
'/dist/v20.0.0/node-v20.0.0-linux-x64.tar.gz',
'/download/release/v20.0.0/node-v20.0.0.pkg',
'/docs/v18.0.0/api/fs.html',
'/metrics/processed/total-csv.csv',
])('returns true for immutable asset `%s`', path => {
expect(isImmutablePath(path)).toEqual(true);
});

test.each([
// Directory listings
'/dist/',
'/dist/v20.0.0/',
// Index/metadata files
'/dist/index.json',
'/dist/index.tab',
'/llms.txt',
'/node-config-schema.json',
// `/api/*` always serves the latest version's docs
'/api',
'/api/fs.html',
// `latest` aliases point at moving content
'/dist/latest/node-v20.0.0-linux-x64.tar.gz',
'/docs/latest/api/fs.html',
'/download/release/latest/win-x64/node_pdb.7z',
'/dist/latest-v20.x/node-v20.0.0.tar.gz',
// Legacy `latest-v0.X.x` aliases
'/dist/latest-v0.10.x/node-v0.10.48.tar.gz',
'/dist/latest-v0.12.x/node-v0.12.18.tar.gz',
// Codename aliases
'/dist/latest-argon/node-v4.9.1.tar.gz',
'/download/release/latest-iron/win-x64/node_pdb.7z',
'/docs/latest-hydrogen/api/fs.html',
// `node-latest.tar.gz` is a moving alias too
'/dist/node-latest.tar.gz',
])('returns false for mutable path `%s`', path => {
expect(isImmutablePath(path)).toEqual(false);
});
});

describe('cacheControlFor', () => {
test('immutable asset with 200 gets the immutable policy', () => {
expect(
cacheControlFor('/dist/v20.0.0/node-v20.0.0-linux-x64.tar.gz', 200)
).toEqual(CACHE_HEADERS.immutable);
});

test('mutable resource with 200 gets the mutable policy', () => {
expect(cacheControlFor('/dist/index.json', 200)).toEqual(
CACHE_HEADERS.mutable
);
});

test.each([206, 304, 412, 404, 500])(
'status %i always gets the failure policy',
status => {
expect(
cacheControlFor('/dist/v20.0.0/node-v20.0.0.tar.gz', status)
).toEqual(CACHE_HEADERS.failure);
}
);
});
Loading