perf(@angular/ssr): avoid buffering request body when sanitizing headers - #33711
Conversation
Avoid teeing the incoming request body stream with `request.clone()` when removing untrusted `X-Forwarded-*` headers in `sanitizeRequestHeaders`.
Teeing the stream caused the unconsumed branch to buffer the uploaded body in memory. Passing `request` directly to `new Request(request, { headers })` transfers the stream without teeing or buffering.
Additionally, `request.signal` is automatically inherited by the new `Request` instance without needing to pass it explicitly.
Closes angular#33706
angular-robot
Bot
added
area: performance
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the sanitizeRequestHeaders utility in Angular SSR to avoid cloning the incoming request when removing unallowed proxy headers. Instead of calling request.clone(), the original request is passed directly to the Request constructor, which prevents unnecessary teeing of the request body and preserves the abort signal. A corresponding unit test has been added to verify this behavior. There are no review comments, and I have no additional feedback to provide.
alan-agius4
added
action: review
| expect(secured.headers.get('forwarded')).toBe('host=proxy.com;proto=https'); | ||
| }); | ||
|
|
||
| it('should not tee request body and should preserve abort signal when removing unallowed headers', async () => { |
There was a problem hiding this comment.
Question: How exactly does this test that teeing didn't happen? I'm not sure how you'd verify that, and bodyUsed doesn't seem to quite represent that. I wonder if this test should just focus on AbortSignal?
There was a problem hiding this comment.
In the WHATWG Fetch specification (request.clone()), calling request.clone() tees the body stream into two branches without marking bodyUsed as true on the original request (which caused the unconsumed branch to buffer the body in memory).
In contrast, passing request directly to new Request(request, ...) (Step 35 of the Request constructor spec) extracts and transfers the underlying readable stream from the source request without teeing it, which immediately marks the source request as disturbed and sets request.bodyUsed to true. So checking expect(req.bodyUsed).toBeTrue() verifies that the original request was consumed/transferred directly rather than cloned/teed (where bodyUsed would remain false).
That said, I agree checking bodyUsed wasn't obvious on its own, and combining both assertions made the test busy. I've updated the test file to split this into two focused unit tests one for AbortSignal preservation and one for stream transfer without teeing—and added an explanatory comment documenting why bodyUsed is checked.
alan-agius4
added
action: merge
2 participants
Avoid teeing the incoming request body stream with
request.clone()when removing untrustedX-Forwarded-*headers insanitizeRequestHeaders.Teeing the stream caused the unconsumed branch to buffer the uploaded body in memory. Passing
requestdirectly tonew Request(request, { headers })transfers the stream without teeing or buffering.Additionally,
request.signalis automatically inherited by the newRequestinstance without needing to pass it explicitly.Closes #33706