From 5dad0c8ba4c199ab314b08fa72244dff0ac6d5cd Mon Sep 17 00:00:00 2001 From: George Moon Date: Fri, 3 Jul 2026 21:37:51 -0400 Subject: [PATCH] fix(security)!: sanitize unmapped S3 errors and stop mid-stream socket leaks Two hardening fixes from a security review of the S3-fronting proxy. 1. Error-detail leakage (src/s3-gateway.ts): mapError() mapped only NoSuchKey/NoSuchBucket->404, AccessDenied->403, InvalidRange->416 and rethrew everything else unchanged. Unmapped S3ServiceExceptions leak sensitive detail (PermanentRedirect -> bucket region/endpoint; SlowDown/InternalError -> requestId and often bucket name), which the examples render straight into the response body. Unmapped errors are now wrapped in a generic S3ProxyError(500); the original is preserved on `cause` for server-side logging only. 2. Mid-stream socket leak (src/index.ts writeBody, examples/http.ts, examples/fastify-basic.ts): plain stream.pipe(res) does not destroy the S3 source stream when the client disconnects mid-transfer, leaking the underlying S3 socket. Switched to stream.pipeline(), which destroys the source on client abort or write error. Rebased onto master: the leak fix moved from examples/express-basic.ts (now the v4.1 middleware() one-liner) into the shared writeBody() path, so pipe()/middleware()/static-site are all covered at the core. BREAKING CHANGE: fetch(), send(), and init() now reject with a sanitized S3ProxyError(500) for previously-unclassified errors instead of the raw S3ServiceException/AWS error. Inspect `err.cause` for the original. The known mappings (S3NotFound/404, S3Forbidden/403, S3InvalidRange/416) are unchanged. Additionally, a mid-stream S3 failure (after headers are sent) now terminates (destroys) the response rather than end()-ing it, so a truncated body surfaces as a broken connection instead of a silent short success. Consumers that branch on raw error types/messages or rely on the old end()-on-mid-stream-error behavior must update. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 5 ++++- examples/fastify-basic.ts | 17 ++++++----------- examples/http.ts | 7 ++++++- src/index.ts | 20 +++++++++++++------- src/s3-gateway.ts | 12 +++++++++--- test/fetch.test.ts | 19 ++++++++++++++----- test/pipe.test.ts | 7 +++++-- test/s3proxy.test.ts | 23 +++++++++++++++++------ 8 files changed, 74 insertions(+), 36 deletions(-) diff --git a/README.md b/README.md index d7671b3..5f381c3 100644 --- a/README.md +++ b/README.md @@ -103,7 +103,10 @@ app.get('/*splat', (req, res) => proxy.pipe(req, res)); ``` Both resolve when the body finishes streaming, and neither rejects on a -classified S3 failure; they write the correct status instead. Use the +classified S3 failure; they write the correct status instead. If S3 fails +mid-stream (after the headers are already sent), the response is terminated +rather than cleanly ended, so a truncated body surfaces as a broken +connection instead of a silently short success. Use the explicit `fetch()` + try/catch form (below) when you need a custom error body (XML/HTML), structured logging, or a non-Express framework. diff --git a/examples/fastify-basic.ts b/examples/fastify-basic.ts index 5548c34..a8ad0bc 100644 --- a/examples/fastify-basic.ts +++ b/examples/fastify-basic.ts @@ -5,6 +5,7 @@ * v4 proxy.fetch() API: pure data fetch, frameworks own the response. */ +import { pipeline } from 'node:stream'; import { XmlNode, XmlText } from '@aws-sdk/xml-builder'; import Fastify from 'fastify'; import { S3Proxy } from '../src/index.js'; @@ -60,18 +61,12 @@ fastify.head('/*', async (request, reply) => { fastify.get('/*', async (request, reply) => { const { stream, status, headers } = await proxy.fetch(request.raw as unknown as HttpRequest); reply.raw.writeHead(status, headers); - stream.on('error', (err: S3Error) => { - const errorXml = createErrorXml(err, request.url); - if (!reply.raw.headersSent) { - reply - .status(err.statusCode || 500) - .type('application/xml') - .send(errorXml); - } else { - reply.raw.end(); - } + // pipeline (not .pipe) destroys the S3 source stream if the client disconnects + // mid-transfer, so the underlying S3 socket isn't leaked. Headers are already + // sent by writeHead above, so on a stream error we can only end the response. + pipeline(stream, reply.raw, (err) => { + if (err && !reply.raw.writableEnded) reply.raw.end(); }); - stream.pipe(reply.raw); return reply.hijack(); }); diff --git a/examples/http.ts b/examples/http.ts index c58f748..d6a5c2b 100644 --- a/examples/http.ts +++ b/examples/http.ts @@ -1,4 +1,5 @@ import { createServer } from 'node:http'; +import { pipeline } from 'node:stream'; import { S3Proxy } from '../src/index.js'; import type { HttpRequest } from '../src/types.js'; @@ -18,7 +19,11 @@ const server = createServer(async (req, res) => { try { const { stream, status, headers } = await proxy.fetch(req as unknown as HttpRequest); res.writeHead(status, headers); - stream.on('error', () => res.end()).pipe(res); + // pipeline (not .pipe) destroys the S3 source stream if the client + // disconnects mid-transfer, so the underlying S3 socket isn't leaked. + pipeline(stream, res, (err) => { + if (err && !res.writableEnded) res.end(); + }); } catch (error) { const statusCode = typeof error === 'object' && diff --git a/src/index.ts b/src/index.ts index a77ad78..ff8f805 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,5 +1,5 @@ import { EventEmitter } from 'node:events'; -import { Readable } from 'node:stream'; +import { pipeline, Readable } from 'node:stream'; import { GetObjectCommand, HeadBucketCommand, HeadObjectCommand } from '@aws-sdk/client-s3'; import { S3Forbidden, S3NotFound, S3ProxyError } from './errors.js'; import { mapHeaderToParam, parseRequest } from './request-parser.js'; @@ -147,6 +147,12 @@ export class S3Proxy extends EventEmitter { * Resolves once the body has finished streaming (or the error response * has been written). Never rejects for classified S3 failures — it * renders them. It only rejects if `res.writeHead` itself throws. + * + * If S3 fails *mid-stream* — after the status and headers have already + * gone out — the response is terminated (destroyed), not cleanly ended: + * the client sees a broken connection instead of a silently truncated + * "successful" body, and the S3 source socket is torn down. (Before v4.2.2 + * the response was `end()`-ed, which could mask a short read as success.) */ public pipe(req: HttpRequest, res: HttpResponse): Promise { return this.fetch(req).then( @@ -170,12 +176,12 @@ export class S3Proxy extends EventEmitter { ): Promise { res.writeHead(status, headers); return new Promise((resolve) => { - stream.on('error', () => { - res.end(); - resolve(); - }); - stream.on('end', resolve); - stream.pipe(res); + // pipeline (not .pipe) destroys the S3 source stream when the client + // disconnects mid-transfer or a write fails, so the underlying S3 socket + // isn't leaked. Headers are already sent, so a mid-stream failure can only + // be handled by ending the response — hence resolve (never reject) in all + // cases; middleware()/pipe() treat a rendered/terminated response as done. + pipeline(stream, res, () => resolve()); }); } diff --git a/src/s3-gateway.ts b/src/s3-gateway.ts index 9665961..4258df1 100644 --- a/src/s3-gateway.ts +++ b/src/s3-gateway.ts @@ -11,7 +11,7 @@ import { S3Client, S3ServiceException, } from '@aws-sdk/client-s3'; -import { S3Forbidden, S3InvalidRange, S3NotFound, type S3ProxyError } from './errors.js'; +import { S3Forbidden, S3InvalidRange, S3NotFound, S3ProxyError } from './errors.js'; import type { S3FetchResponse, S3ProxyOptions } from './types.js'; import { UserException } from './UserException.js'; @@ -75,7 +75,7 @@ function outputToHeaders(output: SupportedOutput): Record { return h; } -function mapError(e: unknown, target: string): S3ProxyError | never { +function mapError(e: unknown, target: string): S3ProxyError { if (e instanceof NoSuchKey || e instanceof NoSuchBucket) { return new S3NotFound(target, { cause: e }); } @@ -83,7 +83,13 @@ function mapError(e: unknown, target: string): S3ProxyError | never { if (e.name === 'AccessDenied') return new S3Forbidden(target, { cause: e }); if (e.name === 'InvalidRange') return new S3InvalidRange(target, { cause: e }); } - throw e; + // Anything else — an unmapped S3ServiceException (e.g. PermanentRedirect, + // SlowDown, InternalError), a network error, or an unexpected throw — is + // wrapped in a generic 500. Raw AWS details (bucket region/endpoint, + // requestId, internal messages) must never reach the client; the original + // is preserved on `cause` for server-side logging only. Consumers that log + // errors should log `err.cause`, never surface it in a response body. + return new S3ProxyError(`S3 request failed: ${target}`, 500, { cause: e }); } /** diff --git a/test/fetch.test.ts b/test/fetch.test.ts index 24b3d4d..dd36cfd 100644 --- a/test/fetch.test.ts +++ b/test/fetch.test.ts @@ -1,6 +1,6 @@ import { GetObjectCommand, S3ServiceException } from '@aws-sdk/client-s3'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; -import { S3Forbidden, S3InvalidRange, S3NotFound, S3Proxy } from '../src/index.js'; +import { S3Forbidden, S3InvalidRange, S3NotFound, S3Proxy, S3ProxyError } from '../src/index.js'; import { s3Mock, setupS3Mocks, teardownS3Mocks } from './helpers/aws-mock.js'; import { catchError, makeReq, readAll } from './helpers/http-mocks.js'; @@ -88,20 +88,29 @@ describe('proxy.fetch', () => { expect((err as S3InvalidRange).cause).toBe(invalidRange); }); - it('rethrows non-AWS errors unchanged', async () => { + it('wraps non-AWS errors in a sanitized 500 (original preserved on cause)', async () => { + // Unmapped errors (network failures, unexpected AWS exceptions like + // PermanentRedirect/SlowDown) must not leak their raw message to the client. const networkError = new Error('Network error'); s3Mock.on(GetObjectCommand, { Bucket: '.test-bucket', Key: 'flaky.txt' }).rejects(networkError); - await expect(proxy.fetch(makeReq('/flaky.txt'))).rejects.toThrow('Network error'); + const err = await catchError(proxy.fetch(makeReq('/flaky.txt'))); + expect(err).toBeInstanceOf(S3ProxyError); + expect((err as S3ProxyError).statusCode).toBe(500); + expect((err as Error).message).not.toContain('Network error'); + expect((err as S3ProxyError).cause).toBe(networkError); }); - it('throws for unrecognized body type', async () => { + it('wraps an unrecognized body type in a sanitized 500 (original on cause)', async () => { s3Mock.on(GetObjectCommand, { Bucket: '.test-bucket', Key: 'weird.txt' }).resolves({ Body: { invalid: 'body' } as never, ContentLength: 100, ContentType: 'text/plain', $metadata: { httpStatusCode: 200, requestId: 'mock' }, }); - await expect(proxy.fetch(makeReq('/weird.txt'))).rejects.toThrow('unrecognized type'); + const err = await catchError(proxy.fetch(makeReq('/weird.txt'))); + expect(err).toBeInstanceOf(S3ProxyError); + expect((err as S3ProxyError).statusCode).toBe(500); + expect(((err as S3ProxyError).cause as Error).message).toBe('unrecognized type'); }); it('does not write to a response (pure)', async () => { diff --git a/test/pipe.test.ts b/test/pipe.test.ts index 75c0c58..33e6989 100644 --- a/test/pipe.test.ts +++ b/test/pipe.test.ts @@ -82,7 +82,7 @@ describe('proxy.pipe / proxy.middleware (convenience adapter)', () => { }); describe('mid-stream failure', () => { - it('ends the response and resolves when the S3 body errors after headers are sent', async () => { + it('terminates the response and resolves when the S3 body errors after headers are sent', async () => { s3Mock.on(GetObjectCommand, { Bucket: '.test-bucket', Key: 'flaky-stream.bin' }).resolves({ Body: new Readable({ read() { @@ -95,7 +95,10 @@ describe('proxy.pipe / proxy.middleware (convenience adapter)', () => { const res = new FakeResponse(); await expect(proxy.pipe(makeReq('/flaky-stream.bin'), res)).resolves.toBeUndefined(); expect(res.status).toBe(200); // headers already went out before the stream broke - expect(res.writableEnded).toBe(true); + // pipeline() destroys the response on a mid-stream source error rather than + // end()-ing it: a truncated body is surfaced as a broken connection, not a + // silently-short "successful" response. destroy() also tears down the S3 source. + expect(res.destroyed).toBe(true); }); }); }); diff --git a/test/s3proxy.test.ts b/test/s3proxy.test.ts index 1ddedb8..4769843 100644 --- a/test/s3proxy.test.ts +++ b/test/s3proxy.test.ts @@ -1,8 +1,9 @@ import { HeadBucketCommand } from '@aws-sdk/client-s3'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { S3NotFound, S3Proxy, UserException } from '../src/index.js'; +import { S3NotFound, S3Proxy, S3ProxyError, UserException } from '../src/index.js'; import type { S3ProxyConfig } from '../src/types.js'; import { s3Mock, setupS3Mocks, teardownS3Mocks } from './helpers/aws-mock.js'; +import { catchError } from './helpers/http-mocks.js'; describe('S3Proxy', () => { beforeEach(() => { @@ -72,8 +73,14 @@ describe('S3Proxy', () => { const errorSpy = vi.fn(); proxy.on('error', errorSpy); - await expect(proxy.init()).rejects.toThrow('Init failed'); - expect(errorSpy).toHaveBeenCalledWith(initError); + // init() rejects with a sanitized S3ProxyError (raw message not exposed); + // the original is preserved on `cause` and the same error is emitted. + const err = await catchError(proxy.init()); + expect(err).toBeInstanceOf(S3ProxyError); + expect((err as S3ProxyError).statusCode).toBe(500); + expect((err as Error).message).not.toContain('Init failed'); + expect((err as S3ProxyError).cause).toBe(initError); + expect(errorSpy).toHaveBeenCalledWith(err); }); }); @@ -99,10 +106,14 @@ describe('S3Proxy', () => { }); describe('verifyOnInit', () => { - it('defaults to true: rejects when the bucket is unreachable', async () => { + it('defaults to true: rejects with a sanitized error when the bucket is unreachable', async () => { const proxy = new S3Proxy({ bucket: '.test-bucket' }); - s3Mock.on(HeadBucketCommand).rejectsOnce(new Error('unreachable')); - await expect(proxy.init()).rejects.toThrow('unreachable'); + const cause = new Error('unreachable'); + s3Mock.on(HeadBucketCommand).rejectsOnce(cause); + const err = await catchError(proxy.init()); + expect(err).toBeInstanceOf(S3ProxyError); + expect((err as Error).message).not.toContain('unreachable'); + expect((err as S3ProxyError).cause).toBe(cause); }); it('false: init() resolves without sending a HeadBucket', async () => {