fix: hardening for non-json object reply - #1293
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the HTTP reply pipeline to make Blob response bodies Fastify-compatible by normalizing them to web streams before sending, and updates request-lifecycle hooks and server initialization to align with Fastify’s newer APIs (including a Fastify version bump).
Changes:
- Introduce
toFastifyReplyPayload()to convertBlobbodies toReadableStreampayloads and use it in renderer + routes that may return Blob bodies. - Update hot-path Fastify lifecycle hooks (pprof + DB cleanup) to use the callback (non-async) fast path, and add tests to enforce hook arity/non-async usage.
- Bump Fastify to
^5.11.2and migrate request logging disabling tologController.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/storage/renderer/renderer.ts | Normalizes asset bodies via toFastifyReplyPayload() before reply.send(). |
| src/storage/renderer/image.test.ts | Adds regression coverage for Blob asset bodies being streamed instead of sent as objects. |
| src/start/worker.ts | Switches Fastify logging configuration to logController for the admin worker server. |
| src/start/server.ts | Switches Fastify logging configuration to logController for API and admin servers. |
| src/internal/http/response-body.ts | Adds Blob → web stream conversion helper for Fastify reply payloads. |
| src/internal/http/response-body.test.ts | Adds unit tests for Blob conversion and passthrough behavior. |
| src/internal/http/index.ts | Exports the new response-body helper from the internal HTTP barrel. |
| src/http/routes/s3/router.test.ts | Adds regression coverage for Blob S3 object bodies being streamed correctly. |
| src/http/routes/s3/index.ts | Normalizes S3 route handler response bodies via toFastifyReplyPayload(). |
| src/http/routes/admin/pprof.ts | Uses callback-style onSend hook and normalizes download bodies via toFastifyReplyPayload(). |
| src/http/routes/admin/pprof.test.ts | Adds regression coverage for Blob profile downloads being streamed correctly. |
| src/http/plugins/sync-hooks.test.ts | Strengthens tests to assert callback-arity + non-async hooks for hot lifecycle hooks. |
| src/http/plugins/log-request.test.ts | Updates Fastify initialization to use logController instead of disableRequestLogging. |
| src/http/plugins/db.ts | Refactors DB cleanup hooks to callback-style fast-path hooks via a shared helper. |
| package.json | Bumps Fastify dependency to ^5.11.2. |
| package-lock.json | Updates lockfile for the Fastify bump and transitive dependency changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
582ccb8 to
131f712
Compare
Coverage Report for CI Build 30825934576Coverage increased (+0.03%) to 80.4%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
082c3a6 to
838eef9
Compare
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
838eef9 to
94c5aaf
Compare
|
@claude review |
There was a problem hiding this comment.
No bugs found, but this PR changes request-lifecycle hooks (DB connection cleanup, Blob-to-stream conversion, onSend hook ordering) that run on every request across the public app, admin app, and worker, plus a fastify minor bump — worth a human look given the blast radius before merging.
What was reviewed:
- DB connection cleanup hook conversions (onSend/onTimeout/onRequestAbort) — callback arities match Fastify's sync signatures, confirmed by the new arity checks in sync-hooks.test.ts.
- blob-response plugin registration order in app.ts/admin-app.ts — Blob→stream conversion runs before other onSend hooks (pprof cache-control, headerValidator), verified by the new integration tests.
- The fastify bump's disableRequestLogging -> LogController migration is applied consistently in server.ts (both HTTP and admin servers) and worker.ts.
- Ruled out: Blob-to-stream conversion silently dropping Content-Length for known-size bodies (already flagged as ruled-out by the bug hunter, not a new finding).
Extended reasoning...
Overview
This PR (1) adds a new blob-response Fastify plugin that converts Blob reply payloads into web streams via an onSend hook (registered first, before all other plugins, in both app.ts and admin-app.ts) and logs metadata about any other unsupported payload shape; (2) converts several onSend/onTimeout/onRequestAbort hooks (DB connection cleanup in db.ts, cache-control header in pprof.ts) from async/Promise-returning to Fastify's callback-based fast path; and (3) bumps fastify from 5.8.5 to 5.11.2, which requires migrating disableRequestLogging to the new LogController API in server.ts and worker.ts. Tests were added/updated across all touched surfaces (app/admin-app registration, blob-response unit tests, hook-arity assertions in sync-hooks.test.ts, S3 router, pprof routes, image renderer).
Security risks
No new attack surface — no auth, crypto, or permission logic is touched. The blob-response plugin's error-path logging is careful to redact the query string and never logs the actual payload body, which the unit test explicitly asserts.
Level of scrutiny
This warrants more than the 'skim and approve' bar: the DB cleanup hooks and the new onSend blob-conversion hook sit on the hot path for every single request served by the public app, admin app, and worker admin server. A subtle hook-ordering or arity mistake here (e.g., calling done() twice, or an onSend hook running after DB disposal instead of before) would silently affect every request rather than fail loudly in one code path. The bug-hunting pass found nothing, and my own read of the callback arities and registration order lines up with Fastify's documented hook signatures and execution order, but given the blast radius and the accompanying dependency bump, I'd rather a human sign off.
Other factors
Coverage dropped significantly per the Coveralls comment, though that appears to be largely pre-existing/unrelated regression noise rather than something introduced by this diff's own patch coverage (68.75% patch coverage, which is reasonable for hook-registration code). The author has already been asked by a maintainer ("@claude review") to have this reviewed, reinforcing that a second pair of eyes here is warranted.
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
Non-JSON content-type payload isn't serialized and causes uncaught exception.
What is the new behavior?
Convert blob payload to web streams where applicable.
Make db connection cleanup hooks sync, any escape will be caught by fastify and it's faster as well by not allocating one promise and doing one less microtask execution.
Additional context
Bump fastify due to fastify/fastify#6889