From bea7103cb5d33992c71674ad6270a71699b64f79 Mon Sep 17 00:00:00 2001 From: osama-rizk Date: Mon, 3 Aug 2026 12:11:17 +0200 Subject: [PATCH] fix(bb-file-bucket): harden dev file server against stored XSS and token forgery The local dev file server served attacker-controlled bodies inline under an attacker-controlled Content-Type (stored-XSS primitive), and signed presigned-URL tokens with a hardcoded, source-visible secret (forgeable offline). - GET responses now send X-Content-Type-Options: nosniff and Content-Disposition: attachment so an uploaded text/html/SVG payload can't execute in the app origin. - LOCAL_FILE_SECRET is now a per-process random value instead of a fixed literal. The token-minting mock and the validating dev server share one in-process module instance, so local presigned-URL round-trips still work. Adds regression tests (both verified red on the old code): nosniff+attachment on GET, and rejection of a token forged with the former hardcoded secret. --- .../file-bucket-dev-server-hardening.md | 5 ++ packages/bb-file-bucket/DESIGN.md | 3 +- .../bb-file-bucket/src/file-server.test.ts | 49 +++++++++++++++++++ packages/bb-file-bucket/src/file-server.ts | 17 ++++++- packages/bb-file-bucket/src/tokens.ts | 20 +++++++- 5 files changed, 90 insertions(+), 4 deletions(-) create mode 100644 .changeset/file-bucket-dev-server-hardening.md diff --git a/.changeset/file-bucket-dev-server-hardening.md b/.changeset/file-bucket-dev-server-hardening.md new file mode 100644 index 000000000..12b3f0e6e --- /dev/null +++ b/.changeset/file-bucket-dev-server-hardening.md @@ -0,0 +1,5 @@ +--- +"@aws-blocks/bb-file-bucket": patch +--- + +Harden the local dev file server against stored XSS and token forgery. Downloads are now served with `X-Content-Type-Options: nosniff` and `Content-Disposition: attachment`, so an uploaded `text/html`/SVG payload can no longer execute inline in the app's origin. The HMAC secret used to sign presigned-URL tokens is now a per-process random value instead of a hardcoded, source-visible literal, so tokens can no longer be forged offline. Both the token-minting mock and the validating dev file server share the same in-process value, so local presigned-URL round-trips are unaffected. diff --git a/packages/bb-file-bucket/DESIGN.md b/packages/bb-file-bucket/DESIGN.md index 2878f4afe..42ef85010 100644 --- a/packages/bb-file-bucket/DESIGN.md +++ b/packages/bb-file-bucket/DESIGN.md @@ -70,7 +70,8 @@ Creates a single S3 bucket: - `versions/{key}/{versionId}` (+ `.json` sidecars, `__deleted__` marker) — version history. - Path mapping for both the mock and the dev file-server is centralized in `paths.ts` so they stay in lockstep. - Data persists across dev server restarts. Customers can wipe with `rm -rf .bb-data`. -- Presigned URLs are served by the dev file-server at `/.bb-file-bucket/{scope.fullId}/{path}?token=...`. The path segments are URL-encoded; the server decodes them and validates an HMAC token scoped to method, path, and expiry. +- Presigned URLs are served by the dev file-server at `/.bb-file-bucket/{scope.fullId}/{path}?token=...`. The path segments are URL-encoded; the server decodes them and validates an HMAC token scoped to method, path, and expiry. The HMAC secret (`LOCAL_FILE_SECRET` in `tokens.ts`) is a **per-process random value** — the token-minting mock and the validating dev file-server share the same in-process module instance, so tokens are unforgeable without being a hardcoded, source-visible literal. +- Downloads are served with `X-Content-Type-Options: nosniff` and `Content-Disposition: attachment`. The stored body and its `Content-Type` are caller-controlled, so serving them inline would make the dev file-server a stored-XSS vector (an uploaded `text/html`/SVG payload executing in the app origin). Forcing a download + disabling MIME sniffing keeps local dev no weaker than S3-behind-CloudFront. - `scan()` recursively walks only the `content/` root and yields every file it finds — no marker-based filtering — so user keys are unrestricted. - The dev file-server's PUT handler delegates to the registered `FileBucket` instance (via a process-global registry) so uploads get versioning, key validation, and metadata. There is no direct-write fallback; an unregistered bucket fails loud with a 500. - Key length validated against S3's 1,024-byte limit (warns, does not reject). diff --git a/packages/bb-file-bucket/src/file-server.test.ts b/packages/bb-file-bucket/src/file-server.test.ts index 69696e2f4..bd91e9130 100644 --- a/packages/bb-file-bucket/src/file-server.test.ts +++ b/packages/bb-file-bucket/src/file-server.test.ts @@ -72,6 +72,38 @@ describe('file-server: basic GET/PUT', () => { assert.strictEqual(await getRes.text(), 'hello world'); }); + test('GET hardens against stored XSS: nosniff + attachment disposition', async () => { + // A client uploads an HTML payload under a text/html content type — the + // classic stored-XSS setup. When the dev server serves it back it must + // never let the browser render it inline in the app's origin. + const bucket = new FileBucket(scope, 'fs-xss'); + const putUrl = await bucket.putUrl('payload.html', { contentType: 'text/html' }); + const adjustedPut = putUrl.replace(/localhost:\d+/, `localhost:${port}`); + const putRes = await fetch(adjustedPut, { + method: 'PUT', + body: '', + headers: { 'Content-Type': 'text/html' }, + }); + assert.strictEqual(putRes.status, 200, `PUT failed: ${putRes.status}`); + + const getUrl = await bucket.getUrl('payload.html'); + const adjustedGet = getUrl.replace(/localhost:\d+/, `localhost:${port}`); + const getRes = await fetch(adjustedGet); + assert.strictEqual(getRes.status, 200, `GET failed: ${getRes.status}`); + assert.strictEqual( + getRes.headers.get('x-content-type-options'), + 'nosniff', + 'GET must send X-Content-Type-Options: nosniff', + ); + assert.match( + getRes.headers.get('content-disposition') ?? '', + /^attachment/, + 'GET must force download via Content-Disposition: attachment', + ); + // consume the body so the socket closes cleanly + await getRes.arrayBuffer(); + }); + test('GET non-existent file returns 404', async () => { const bucket = new FileBucket(scope, 'fs-404'); const url = await bucket.getUrl('missing.txt'); @@ -87,6 +119,23 @@ describe('file-server: basic GET/PUT', () => { assert.strictEqual(res.status, 403); }); + test('a token forged with the former hardcoded secret is rejected', async () => { + // The signing secret used to be a fixed, source-visible literal, so anyone + // could mint a valid token offline for any fullId/path without ever calling + // getUrl()/putUrl(). It is now a per-process random secret. A token signed + // with the old literal must no longer validate. + const bucket = new FileBucket(scope, 'fs-forge'); + const putUrl = await bucket.putUrl('secret.txt', { contentType: 'text/plain' }); + const adjustedPut = putUrl.replace(/localhost:\d+/, `localhost:${port}`); + await fetch(adjustedPut, { method: 'PUT', body: 'data', headers: { 'Content-Type': 'text/plain' } }); + + const forged = mintFileToken('fsrv-fs-forge', 'secret.txt', 'GET', 3600, '__blocks_file_bucket_dev_secret__'); + const url = `http://localhost:${port}/.bb-file-bucket/fsrv-fs-forge/secret.txt?token=${forged}`; + const res = await fetch(url); + await res.arrayBuffer(); + assert.strictEqual(res.status, 403, 'a token forged with the old hardcoded secret must be rejected'); + }); + test('PUT for an unregistered bucket fails loud (500), no silent write', async () => { // Mint a structurally valid token for a fullId that has no FileBucket // instance registered. The server must refuse rather than fall back to diff --git a/packages/bb-file-bucket/src/file-server.ts b/packages/bb-file-bucket/src/file-server.ts index 939474972..7df71d2dd 100644 --- a/packages/bb-file-bucket/src/file-server.ts +++ b/packages/bb-file-bucket/src/file-server.ts @@ -141,7 +141,22 @@ export function attach(httpServer: Server) { } const body = readFileSync(readPath); - res.writeHead(200, { 'Content-Type': contentType, 'Content-Length': body.length.toString() }); + // The stored object body and its Content-Type are attacker-controlled + // (any client with a presigned PUT can upload arbitrary bytes under an + // arbitrary content type). Serving that back inline turns the dev file + // server into a stored-XSS vector: an uploaded `text/html` (or sniffed + // HTML/SVG) payload would execute in the origin of the local app. + // `nosniff` stops the browser from MIME-sniffing octet-streams into + // HTML, and `Content-Disposition: attachment` forces a download rather + // than inline rendering — so an uploaded document can never run as a + // page. Real S3 objects served through CloudFront are hardened the same + // way; this keeps local dev from being weaker than production. + res.writeHead(200, { + 'Content-Type': contentType, + 'Content-Length': body.length.toString(), + 'X-Content-Type-Options': 'nosniff', + 'Content-Disposition': 'attachment', + }); res.end(body); } else if (req.method === 'PUT') { const valid = validateFileToken(token, LOCAL_FILE_SECRET, fullId, path, 'PUT'); diff --git a/packages/bb-file-bucket/src/tokens.ts b/packages/bb-file-bucket/src/tokens.ts index 14ca8f500..74804a294 100644 --- a/packages/bb-file-bucket/src/tokens.ts +++ b/packages/bb-file-bucket/src/tokens.ts @@ -1,12 +1,28 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -import { createHmac } from 'node:crypto'; +import { createHmac, randomBytes } from 'node:crypto'; import { constantTimeEquals } from '@aws-blocks/core/bb-utils'; // ── Token helpers ─────────────────────────────────────────────────────────── -export const LOCAL_FILE_SECRET = '__blocks_file_bucket_dev_secret__'; +/** + * Per-process HMAC secret for signing local presigned-URL tokens. + * + * The mock bucket (which mints tokens) and the dev file server (which validates + * them) both import this module and run in the *same* dev-server process, so a + * value generated once at module load is shared between them via the ESM module + * cache — no configuration needed. + * + * This deliberately replaces a previously hardcoded literal. A fixed, source- + * visible secret let anyone forge a valid token for any `fullId`/path/method and + * hit the dev file server without ever calling `getUrl()`/`putUrl()`, defeating + * the point of signing. A random per-process secret makes tokens unforgeable + * while keeping the local round-trip working, since both ends share this value. + * Tokens do not need to survive a dev-server restart (presigned URLs are short- + * lived and re-minted on demand), so per-process randomness is sufficient. + */ +export const LOCAL_FILE_SECRET = randomBytes(32).toString('base64url'); interface FileTokenPayload { fullId: string;