fix(responseInterceptor): remove disallowed headers when response headers contains trailer#830
fix(responseInterceptor): remove disallowed headers when response headers contains trailer#830ziboilihua wants to merge 3 commits intochimurai:masterfrom
Conversation
| res: http.ServerResponse | ||
| ) => Promise<Buffer | string>; | ||
|
|
||
| const TrailerDisallowHeaders: string[] = [ |
There was a problem hiding this comment.
Could you add a link to verify the completeness of these headers?
There was a problem hiding this comment.
…ins trailer Signed-off-by: zibo <ziboilihua@gmail.com>
Signed-off-by: zibo <ziboilihua@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughUpdated response interceptor to omit certain headers when the upstream response includes a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
chimurai
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/handlers/response-interceptor.ts`:
- Around line 69-76: The trailer branch currently strips headers via
TrailerDisallowHeaders but fails to update or remove the stale content-length
copied earlier (see copyHeaders) even though the interceptor mutates
interceptedBuffer; modify the branch guarded by proxyRes.headers.trailer to
explicitly remove or recalculate the content-length header on res (remove
content-length or set it to Buffer.byteLength(interceptedBuffer,'utf8')) and
ensure TrailerDisallowHeaders handling still runs; update tests to exercise an
upstream response that includes both Trailer and Content-Length so this branch
behavior is validated (refer to TrailerDisallowHeaders, copyHeaders,
interceptedBuffer, and proxyRes.headers.trailer).
- Around line 16-31: Remove the incorrect TrailerDisallowHeaders constant and
change the logic that strips headers when an upstream trailer is present so it
only removes the content-length header; specifically, delete
TrailerDisallowHeaders and update the code block that iterates/removes headers
(the section that currently references TrailerDisallowHeaders) to only delete
'content-length' from the response headers when trailers exist, leaving
content-type, set-cookie, cache-control, authorization, content-range, etc.
untouched.
In `@test/e2e/response-interceptor.spec.ts`:
- Line 69: Rename the test description string in the it(...) call in
response-interceptor.spec.ts to a grammatically correct, concise message:
replace "should not contains disallow headers to trailer in response headers
http://httpbin.org/response-headers" with something like "should not include
trailer-disallowed headers in response" (remove the URL and fix grammar); update
the it(...) title where it appears so test output and logs reflect the new
description.
- Around line 69-74: The test currently doesn't hit the trailer code path
because targetServer only mocks /json and /image; add a mock handler on
targetServer (e.g., targetServer.forGet('/response-headers') or '/with-trailer')
that replies 200 with headers "Trailer: X-Stream-Error" and "Content-Length: 1"
and a simple body, then call agent.get(...) against that path and assert the
proxied response completes with status 200 and a non-empty body; this exercises
the TrailerDisallowHeaders logic (refer to TrailerDisallowHeaders) and ensures
the proxy doesn't crash when upstream sends both Trailer and Content-Length —
update the test in response-interceptor.spec.ts to register the mock on
targetServer (or within the test) and replace the weak host header assertion
with status/body assertions using agent and targetServer.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf4d4004-7b3b-4c52-9fa7-0bdab8e3d4b8
📒 Files selected for processing (2)
src/handlers/response-interceptor.tstest/e2e/response-interceptor.spec.ts
| /** | ||
| * Disallow headers when response contains trailer | ||
| * source: https://developer.mozilla.org/docs/Web/HTTP/Headers/Trailer | ||
| */ | ||
| const TrailerDisallowHeaders: string[] = [ | ||
| 'content-length', | ||
| 'host', | ||
| 'content-type', | ||
| 'authorization', | ||
| 'cache-control', | ||
| 'max-forwards', | ||
| 'te', | ||
| 'set-cookie', | ||
| 'content-encoding', | ||
| 'content-range', | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MDN Trailer header disallowed headers meaning declared as trailer vs response headers
💡 Result:
The Trailer header indicates which HTTP fields (headers) the sender plans to include in the trailer section at the end of a chunked message body. It is sent in the header section (initial headers block) of the HTTP message. Disallowed headers in trailers: Certain headers cannot be used as trailer fields because they must be processed before the body (e.g., for routing, framing, authentication). MDN lists: Content-Encoding, Content-Type, Content-Range, Trailer; authentication headers (e.g., Authorization, Set-Cookie); message framing (Transfer-Encoding, Content-Length); routing (Host); request modifiers (Cache-Control, Max-Forwards, TE). This matches RFC 9110 §6.5.1 limitations on trailers. "Declared as trailer" means listed in the Trailer header (e.g., Trailer: Server-Timing), signaling those fields will appear in the trailers section, not the initial response headers. Trailer fields are processed/stored separately from header fields (RFC 9110 §6.5). They cannot override or duplicate critical headers needed pre-body. Example: HTTP/1.1 200 OK Transfer-Encoding: chunked Trailer: Server-Timing [body] Server-Timing: custom-metric;dur=123.4 Server-Timing is declared via Trailer header and appears only in trailers, not response headers.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Trailer
- 2: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Trailer
- 3: https://rfcinfo.com/rfc-9110/5
- 4: https://tools.ietf.org/html/rfc9112
- 5: https://rfc-editor.org/rfc/rfc9112.html
- 6: https://rfc-editor.org/rfc/rfc9110.html
🏁 Script executed:
cat -n src/handlers/response-interceptor.ts | head -50Repository: chimurai/http-proxy-middleware
Length of output: 1872
🏁 Script executed:
cat -n src/handlers/response-interceptor.ts | sed -n '40,150p'Repository: chimurai/http-proxy-middleware
Length of output: 4550
The TrailerDisallowHeaders list is misapplied—these headers cannot be declared as trailers, not stripped from responses.
Per MDN and RFC 9110, the disallowed headers list specifies which header names cannot appear in the trailer section at the end of a chunked response. The current code incorrectly strips these headers from the response itself whenever an upstream trailer is present (lines 73–75).
This breaks legitimate responses: content-type, set-cookie, cache-control, authorization, and content-range are removed from the response headers, causing cookies to be lost, content-type to be missing, and auth to fail in downstream clients.
The actual issue is that content-length is incompatible with trailers (which require chunked transfer encoding). The fix should only remove content-length when a trailer is present, not the entire list of headers.
Suggested fix
- // some headers are disallowed when response headers contains trailer
- if (proxyRes.headers.trailer === undefined) {
- res.setHeader('content-length', Buffer.byteLength(interceptedBuffer, 'utf8'));
- } else {
- TrailerDisallowHeaders.forEach((value) => {
- res.removeHeader(value);
- });
- }
+ // content-length is incompatible with trailers (which require chunked
+ // transfer encoding), so only set it when no trailer is declared.
+ if (proxyRes.headers.trailer === undefined) {
+ res.setHeader('content-length', Buffer.byteLength(interceptedBuffer, 'utf8'));
+ } else {
+ res.removeHeader('content-length');
+ }Remove the TrailerDisallowHeaders constant (lines 20–31) as it is no longer needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handlers/response-interceptor.ts` around lines 16 - 31, Remove the
incorrect TrailerDisallowHeaders constant and change the logic that strips
headers when an upstream trailer is present so it only removes the
content-length header; specifically, delete TrailerDisallowHeaders and update
the code block that iterates/removes headers (the section that currently
references TrailerDisallowHeaders) to only delete 'content-length' from the
response headers when trailers exist, leaving content-type, set-cookie,
cache-control, authorization, content-range, etc. untouched.
| // some headers are disallowed when response headers contains trailer | ||
| if (proxyRes.headers.trailer === undefined) { | ||
| res.setHeader('content-length', Buffer.byteLength(interceptedBuffer, 'utf8')); | ||
| } else { | ||
| TrailerDisallowHeaders.forEach((value) => { | ||
| res.removeHeader(value); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Also note: the interceptor mutates the response buffer, so the upstream content-length copied in copyHeaders is stale when a trailer is present.
In the trailer branch, neither content-length is updated nor removed under the narrower fix unless explicitly handled. If you keep the broader header-stripping approach, make sure this branch is exercised by a test that actually triggers a response carrying both Trailer and Content-Length from the upstream — the new E2E test (see test/e2e/response-interceptor.spec.ts) does not do this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/handlers/response-interceptor.ts` around lines 69 - 76, The trailer
branch currently strips headers via TrailerDisallowHeaders but fails to update
or remove the stale content-length copied earlier (see copyHeaders) even though
the interceptor mutates interceptedBuffer; modify the branch guarded by
proxyRes.headers.trailer to explicitly remove or recalculate the content-length
header on res (remove content-length or set it to
Buffer.byteLength(interceptedBuffer,'utf8')) and ensure TrailerDisallowHeaders
handling still runs; update tests to exercise an upstream response that includes
both Trailer and Content-Length so this branch behavior is validated (refer to
TrailerDisallowHeaders, copyHeaders, interceptedBuffer, and
proxyRes.headers.trailer).
| expect(response.body.favorite).toEqual('叉燒包'); | ||
| }); | ||
|
|
||
| it('should not contains disallow headers to trailer in response headers http://httpbin.org/response-headers', async () => { |
There was a problem hiding this comment.
Minor: test name grammar.
"should not contains disallow headers to trailer in response headers" → e.g., "should not include trailer-disallowed headers in response". Also remove the trailing http://httpbin.org/response-headers URL from the test name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/response-interceptor.spec.ts` at line 69, Rename the test
description string in the it(...) call in response-interceptor.spec.ts to a
grammatically correct, concise message: replace "should not contains disallow
headers to trailer in response headers http://httpbin.org/response-headers" with
something like "should not include trailer-disallowed headers in response"
(remove the URL and fix grammar); update the it(...) title where it appears so
test output and logs reflect the new description.
| it('should not contains disallow headers to trailer in response headers http://httpbin.org/response-headers', async () => { | ||
| const response = await agent | ||
| .get('/response-headers?Trailer=X-Stream-Error&Host=localhost') | ||
| .expect(200); | ||
| expect(response.header['host']).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
This test does not actually exercise the trailer code path.
Two problems:
- The
targetServerconfigured in thisbeforeEachonly mocks/jsonand/image(lines 26-32). A request to/response-headers?Trailer=...&Host=...will not match any mockttp handler and will not return 200 — the.expect(200)should fail. The path/response-headerslooks borrowed from httpbin.org, but mockttp won't emulate it. - Even if it returned 200, the assertion
expect(response.header['host']).toBeUndefined()is non-discriminating — HTTP responses virtually never carry aHostheader, so this passes regardless of whether the newTrailerDisallowHeaderslogic runs.
To meaningfully cover the bug (process exit when upstream sends both Trailer and Content-Length), mock an upstream response that includes a trailer header and assert the proxied request completes successfully. For example:
Suggested test
- it('should not contains disallow headers to trailer in response headers http://httpbin.org/response-headers', async () => {
- const response = await agent
- .get('/response-headers?Trailer=X-Stream-Error&Host=localhost')
- .expect(200);
- expect(response.header['host']).toBeUndefined();
- });
+ it('should not crash when upstream response contains a Trailer header', async () => {
+ await targetServer.forGet('/with-trailer').thenReply(200, JSON.stringify({ ok: true }), {
+ 'content-type': 'application/json; charset=utf-8',
+ trailer: 'X-Stream-Error',
+ });
+
+ const response = await agent.get('/with-trailer').expect(200);
+ expect(response.body.foo).toEqual('bar');
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/response-interceptor.spec.ts` around lines 69 - 74, The test
currently doesn't hit the trailer code path because targetServer only mocks
/json and /image; add a mock handler on targetServer (e.g.,
targetServer.forGet('/response-headers') or '/with-trailer') that replies 200
with headers "Trailer: X-Stream-Error" and "Content-Length: 1" and a simple
body, then call agent.get(...) against that path and assert the proxied response
completes with status 200 and a non-empty body; this exercises the
TrailerDisallowHeaders logic (refer to TrailerDisallowHeaders) and ensures the
proxy doesn't crash when upstream sends both Trailer and Content-Length — update
the test in response-interceptor.spec.ts to register the mock on targetServer
(or within the test) and replace the weak host header assertion with status/body
assertions using agent and targetServer.
2 participants
Description
Process will exit when proxy response header both contains
trailerand disallow headers eg.content-lengthMotivation and Context
How has this been tested?
I've created e2e test for this case, and all unit tests passed.
Types of changes
Checklist:
Summary by CodeRabbit