diff --git a/examples/express-basic.ts b/examples/express-basic.ts index 7c7247b..78c8cb0 100644 --- a/examples/express-basic.ts +++ b/examples/express-basic.ts @@ -8,6 +8,7 @@ Author: George Moon */ +import { pipeline } from 'node:stream'; import { XmlNode, XmlText } from '@aws-sdk/xml-builder'; import bodyParser from 'body-parser'; import express, { type Request, type Response } from 'express'; @@ -108,7 +109,11 @@ app try { const { stream, status, headers } = await proxy.fetch(req as unknown as HttpRequest); res.writeHead(status, headers); - stream.on('error', (err: ErrorWithDetails) => handleError(req, res, err)).pipe(res); + // pipeline (not .pipe) destroys the S3 source stream on client disconnect + // or write error, so the underlying S3 socket isn't leaked. + pipeline(stream, res, (err) => { + if (err) handleError(req, res, err as ErrorWithDetails); + }); } catch (error) { handleError(req, res, error as ErrorWithDetails); } 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/s3-gateway.ts b/src/s3-gateway.ts index 972b481..5e3f46a 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'; @@ -55,7 +55,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 }); } @@ -63,7 +63,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 cfe1253..8ecdb58 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'; @@ -75,20 +75,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/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 () => {