fix: suppress response body for HEAD requests#154
Conversation
|
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 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 |
|
|
||
| private endResponse(body: string | Buffer | undefined): void { | ||
| // HEAD responses must have identical headers to GET but no body (RFC 9110 §9.3.2) | ||
| if (this.req?.getMethod() === 'head') { |
There was a problem hiding this comment.
This is wrong UwsRequest has readonly method: string (uppercase 'HEAD'), not a getMethod() function.
| // HEAD responses must have identical headers to GET but no body (RFC 9110 §9.3.2) | ||
| if (this.req?.getMethod() === 'head') { | ||
| if (this.contentLengthTotal !== undefined) { | ||
| this.uwsRes.endWithoutBody(this.contentLengthTotal); | ||
| } else { | ||
| this.uwsRes.end(); | ||
| } | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
No unit test to verify this behaviour
|
@qianmao1989 I have left some inline comments please fix them |
- Use `this.req?.method === 'HEAD'` (uppercase) as UwsRequest has readonly method property, not getMethod() function - Add unit tests for HEAD request body suppression Addresses review comments on PR FOSSFORGE#154
qianmao1989
force-pushed
the
fix/head-response-body
branch
from
5479d9b to
fe8aada
Compare
|
@VikramAditya33 Thanks for the review! I've addressed both comments:
All 137 tests pass. |
|
Please comment under the issue to get assigned |
- Changed `this.req?.getMethod() === 'head'` to `this.req?.method === 'HEAD'` - UwsRequest has `readonly method: string` (uppercase), not a getMethod() function - Added 3 unit tests for HEAD request handling: - HEAD with content-length → calls endWithoutBody() - HEAD without content-length → calls end() without body - GET request → sends body normally (regression check)
|
@VikramAditya33 I've fixed both issues:
All 137 tests pass. Please review again. |
Please proceed with this there were already 2 contributors who worked on this without getting assigned |
2 participants
Summary
HEAD responses must have identical headers to GET but no body (RFC 9110 §9.3.2). uWS is low-level and doesn't strip bodies for HEAD automatically.
Changes
src/http/core/response.ts: InendResponse(), checkthis.req?.getMethod() === 'head'and callendWithoutBody()orend()without body when method is HEAD.Fixes #151