fix(http): parse body before guards and return parsed body synchronously - #208
fix(http): parse body before guards and return parsed body synchronously#208nicoske wants to merge 2 commits into
Conversation
Guards ran before body parsing, and request.body kept returning a Promise even after the pipeline had parsed the body. Any guard that reads request.body (the usual shape of an auth guard that binds a token to payload fields) saw a pending Promise instead of the payload, and the same happened to schema validators that read request.body without awaiting (e.g. Nestia's @TypedBody). On Express none of this is an issue because body-parser middleware runs ahead of the guards. Parse the body before guard execution and store it on the request, and make the body getter return the parsed value directly once it is available. Awaiting request.body keeps working in both states, and pipes overwrite the stored body as before. Streaming content types (multipart, octet-stream) are untouched. The trade-off is that a request rejected by a guard now pays the body parse cost first, which is the same behavior as Express with global body-parser middleware.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http/routing/route-registry.ts`:
- Around line 497-502: Update the body parsing condition in the route
registration flow to parse whenever the request is not streaming, even when
normalizedContentType is absent. Keep the existing normalizedContentType
requirement for the pipe-handling path, and continue storing the resolved body
through req._setTransformedBody so request.body remains synchronous.
In `@test/http/guards-body-access.e2e.spec.ts`:
- Around line 75-89: Update the beforeAll setup around the port constant and
adapter.listen call to use the shared get-port helper with port 0, then assign
baseUrl using the uWS-assigned port returned through that helper (via
uWS.us_socket_local_port). Remove the fixed 13391 value while preserving the
existing application initialization and error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ec13a28e-42f4-4c64-b6f0-00333cc6ca5a
📒 Files selected for processing (3)
src/http/core/request.tssrc/http/routing/route-registry.tstest/http/guards-body-access.e2e.spec.ts
| if (normalizedContentType && !isStreamingContentType) { | ||
| body = await req.body; | ||
| // Store the parsed body so the request.body getter resolves | ||
| // synchronously from here on | ||
| req._setTransformedBody(body); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse non-streaming bodies even without Content-Type.
The normalizedContentType gate leaves valid body-bearing requests without that header unparsed; guards then receive the buffer() Promise instead of the synchronously stored body. Parse whenever the request is not a streaming type; retain the content-type condition for pipes.
Proposed fix
- if (normalizedContentType && !isStreamingContentType) {
+ if (!isStreamingContentType) {
body = await req.body;
// Store the parsed body so the request.body getter resolves
// synchronously from here on
req._setTransformedBody(body);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (normalizedContentType && !isStreamingContentType) { | |
| body = await req.body; | |
| // Store the parsed body so the request.body getter resolves | |
| // synchronously from here on | |
| req._setTransformedBody(body); | |
| } | |
| if (!isStreamingContentType) { | |
| body = await req.body; | |
| // Store the parsed body so the request.body getter resolves | |
| // synchronously from here on | |
| req._setTransformedBody(body); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/http/routing/route-registry.ts` around lines 497 - 502, Update the body
parsing condition in the route registration flow to parse whenever the request
is not streaming, even when normalizedContentType is absent. Keep the existing
normalizedContentType requirement for the pipe-handling path, and continue
storing the resolved body through req._setTransformedBody so request.body
remains synchronous.
There was a problem hiding this comment.
@nicoske I feel like this is a good suggestion by coderabbit to go with.
|
@VikramAditya33 pushed the fix for the no Content-Type case (parse any non streaming request) with a regression test. the other note (fixed test port 13391) i can swap for a random port in the same PR if you want. |
2 participants
Fixes #206
Moves body parsing ahead of guard execution in the route registry and stores the parsed body on the request right away, not only when pipes run. The
bodygetter returns the parsed value synchronously once it exists; before parsing it still returns a Promise, andawait req.bodybehaves the same in both states. Pipes keep overwriting the stored body as before, and streaming content types (multipart, octet-stream) are still skipped.Motivation: guards commonly authorize against payload fields, and libraries like Nestia's
@TypedBodyreadrequest.bodywithout awaiting. Both get a pending Promise on this platform while working fine on Express.The declared getter type changes from
Promise<unknown>tounknown. The only pattern that could notice is calling.then()on it directly instead of awaiting, which nothing in the code base or docs does.Malformed JSON keeps producing the same response it did before the reorder; the new e2e pins that down.
New e2e suite:
test/http/guards-body-access.e2e.spec.ts(guard reads the body synchronously, allow/deny based on payload, synchronous read in the handler, malformed JSON). Lint, unit (975) and e2e (239) suites are green on Node 24.Summary by CodeRabbit
New Features
Bug Fixes
Tests