fix(security): sanitize unmapped errors + stop example stream leaks#124
Open
gmoon wants to merge 1 commit into
Open
fix(security): sanitize unmapped errors + stop example stream leaks#124gmoon wants to merge 1 commit into
gmoon wants to merge 1 commit into
Conversation
…ples Two hardening fixes from a security review of the S3-fronting proxy. 1) Error-detail leakage (src/s3-gateway.ts). mapError only wrapped NoSuchKey/NoSuchBucket -> 404, AccessDenied -> 403, InvalidRange -> 416 and rethrew everything else unchanged. Unmapped S3ServiceExceptions carry sensitive detail — PermanentRedirect discloses the bucket's region/endpoint, SlowDown/InternalError carry $metadata.requestId and often the bucket name — and the examples render err.message straight into the response body, so a client could probe to harvest AWS internals. Unmapped errors are now wrapped in a generic S3ProxyError(500); the original is preserved on `cause` for server-side logging only. This applies to both fetch() and healthCheck() (the latter is client-reachable via the examples' /health route). BEHAVIOR CHANGE: send()/init()/fetch() now reject with a sanitized S3ProxyError(500) for previously-unclassified errors instead of the raw error. Inspect `err.cause` for the original. Tests updated to assert the new contract (sanitized message + preserved cause). 2) Stream/socket leak in examples (examples/http.ts, express-basic.ts, fastify-basic.ts). `.pipe(res)` doesn't destroy the S3 source stream when the client disconnects mid-transfer, leaking the underlying S3 socket under abusive clients. Switched to stream.pipeline(), which destroys the source on client abort or write error. Not included (tracked separately): optional keyPrefix confinement, nosniff header control, 403/404 enumeration hardening, init() double-signaling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two hardening fixes from a security review of the S3-fronting proxy. Scoped to the two highest-value findings; validated locally (
lint+type-check+build+ 49/49 unit tests green).1. Error-detail leakage (MEDIUM) —
src/s3-gateway.tsmapErrorwrapped only NoSuchKey/NoSuchBucket→404, AccessDenied→403, InvalidRange→416 and rethrew everything else unchanged. UnmappedS3ServiceExceptions carry sensitive detail:PermanentRedirect→ discloses the bucket's real region/endpointSlowDown/InternalError→ carry$metadata.requestIdand often the bucket name…and the examples render
err.messagestraight into the response body, so a client can force these conditions and harvest AWS internals.Fix: unmapped errors are wrapped in a generic
S3ProxyError(500); the original is preserved oncausefor server-side logging only. Applies to bothfetch()andhealthCheck()— the latter is client-reachable via the examples'/healthroute, so narrowing tofetch()alone would leave/healthleaking.Important
Behavior change:
send()/init()/fetch()now reject with a sanitizedS3ProxyError(500)for previously-unclassified errors instead of the raw error. Inspecterr.causefor the original. The known mappings (404/403/416) are unchanged. Unit tests updated to assert the new contract (sanitized message + preservedcause).2. Stream/socket leak in examples (MEDIUM) —
examples/{http,express-basic,fastify-basic}.ts.pipe(res)does not destroy the S3 source stream when the client disconnects mid-transfer, leaking the underlying S3 socket under abusive clients (open a range request, disconnect, repeat). Switched tostream.pipeline(), which destroys the source on client abort or write error. (hono-basic.tsalready handles this viaReadable.toWebcancellation.)Deliberately not included (tracked separately)
nosniff/content-type control — stored-XSS surface if the bucket accepts untrusted uploadsinit()double-signaling (emit + throw)Test plan
npm run lint/type-check/build/test:unit(49/49) green locally