Exclude the Trailer header from 304 responses?

[ghost]

Including the Trailer header field with responses that don't have Transfer-Encoding: chunked causes some (overly strict?) proxies to drop the response e.g. IBM Bluemix sends back a 502 top the client in this scenario, even though Node/Express responds with a 304.

I brought up the issue with the Express team and it was suggested that since Node strips out the body for a 304, it might also make sense to strip out the Trailer header from Node. See: expressjs/express#2749

Interested what you all think about it.

[mnot]

Yes, that makes sense:

http://httpwg.github.io/specs/rfc7230.html#message.body.length --

Any response to a HEAD request and any response with a 1xx (Informational), 204 (No Content), or 304 (Not Modified) status code is always terminated by the first empty line after the header fields, regardless of the header fields present in the message, and thus cannot contain a message body.

Note that the response is terminated. Also, the trailers are syntactically part of the chunked body, see
http://httpwg.github.io/specs/rfc7230.html#chunked.encoding

All that said, a receiving implementation could choose to recover from the error of seeing a trailer there, but it's probably not a good idea, since this is about message framing, and there are security implications.

[ghost]

Do you consider it an error to have the Trailer header at all with those responses, or only if there are trailers sent with responses that should have no body? This issue that's being seen with IBM Bluemix is the former, i.e. the Trailer header is in the response, but no actual trailers are sent in the body.

[mnot]

It's not a great thing to do, but there's nothing in the spec that prohibits a sender from generating a Trailer header without actually sending those trailers.

[Trott]

/cc @nodejs/http

(Trying to get some old issues un-stalled or closed if they are close-able...)

[jasnell]

I am available as a mentor for this issue if anyone wants to pick it up.

[ghost]

I wouldn't mind taking a stab at it…

[ALJCepeda]

@jasnell @wprl I'd like to give this a shot if no works been done on it. If I understand correctly the task is to strip the Trailer header from responses that don't have Transfer-Encoding: chunked including 304s since they aren't chunked.

However, tunniclm from Express suggests this is a valid response and should be accepted by the proxy and the user can prevent this issue by not adding the header to responses that aren't chunked

So is this really an issue that needs to be addressed?

[gibfahn]

cc/ @tunniclm

[kaicataldo]

Looking for a good first contribution. Is this something that should still be implemented (wasn't sure after @ALJCepeda's comment). Thanks!

[jasnell]

I believe this is something that should be addressed given the issues. RFC 7230 is very clear that the Trailer header only carries semantics when chunked-encoding is used and is completely silent on whether the Trailer header is valid when chunked-encoding is not. That puts it into a bit of a grey area. Simply omitting the Trailer header when not using chunked-encoding makes sense with the rather large caveat that doing so would likely need to be handled as a semver-major change.

[sebbers]

@jasnell Are you still open to mentoring on this issue?

[jasnell]

Yes, absolutely. This shouldn't be too difficult to do.

[sebbers]

Great! What first steps would you recommend?

[jasnell]

Take a look at the writeHead function here https://github.com/nodejs/node/blob/master/lib/_http_server.js#L163.

Specifically, look right about here: https://github.com/nodejs/node/blob/master/lib/_http_server.js#L208

You should be able to add a check that will report an error if statusCode === 304 and the Trailer header is set.

[sebbers]

ac62fc7

[arturgvieira-zz]

@jasnell Hi, I would like to help with this one. I just read over the writeHead function in the file you mentioned.

[arturgvieira-zz]

Hi, I made the change, please let me know if its good to go. Thanks

[Trott]

Is this still an issue in Node.js 8.3.0?

[nikshepsvn]

Is this still open, would love to give it a go @jasnell

[jasnell]

I believe it is still an issue but it's possible I could be missing something. It's a straightforward test to check. Let me know @nikshepsvn if you need pointers on which part of the code to start poking.

[apapirovski]

@jasnell is this actually still an issue? I thought that 80c9ef0 addressed this?

[apapirovski]

ping @jasnell — is this still an issue? See my comment above re: the commit that might've addressed it. That said, I could be missing something deeper.

[pjmolina]

Hi. I created a simple test for reproduce it when we found the bug, two years ago.

At that time, it worked when deployed on Heroku, but failed when deployed on IBM Bluemix. The difference: Go HTTP middleware in Bluemix (very stricts with trailers). It should be trivial to deploy this sample on Bluemix with the latest version of Node and check if it is solved.

[arturgvieira-zz]

Hi, @pjmolina, how did the test go? I am available to work on this if you would like help.

[pjmolina]

Hi @arturgvieira I need to find the time to repeat it again on Bluemix and will back here to share the results.

[pjmolina]

Hi, @arturgvieira et al.
I just re-deployed the minimal repro-test and these are the results:

Environment	Platform	NodeJS vesion	Test	Url
A0	Heroku	6.11.1	OK	http://h304.herokuapp.com
B1	Bluemix	6.12	Fail	http://h304.mybluemix.net
B2	Bluemix	9.2.1	Fail	http://h304b.mybluemix.net

Repro steps

1. Execute:

curl -i http://<server>/r1

In all environments, resource r1 should work on GET as trailers are not involved and return a 200 OK response.

2. Execute

curl -i http://<server>/r2

This will return a 200 OK on (A0) but a 502 Bad gateway on (B1 and B2). The difference between r1 and r2 are ETAG trailer injected by Express.

3. Execute:

curl -i http://<server>/r2 -H 'If-None-Match: "f-nQIm94ZGyY5czC9sMuZyIC+aXps"'

This will return a 304 Not modified on (A0) but a 502 Bad gateway on (B1 and B2).

It will work on (A0) but will fail on (B1 and B2).

4. Test /r3 adds Transfer-Encoding = chunked with similar results.

The main hypothesis here is: some part of the trailers specs are open to interpretation (in the spec) and are implemented with subtle differences between Go and NodeJS core implementations. This difference causes the interop problem shown here, as IBM Bluemix uses a router as middleware based on Go.

If you run the test locally, everything will work as curl (as client) will accept the trailers as they come (not very strict).

I will let the sample environments running some days just in case someone wants to test and reproduce it.

More info: original thread.

Corollary: this bug prevents having ETags and a proper HTTP cache handling on NodeJS backends hosted at least on Bluemix (other cloud hosts could be affected too, for example, CloudFoundry based ones).

[apapirovski]

@pjmolina can you reproduce this with plain Node.js code rather than Express? Express monkey patches lots of code so it wouldn't surprise me if it could still be broken there while working in Node.js.

Unless I'm severely misunderstanding the issue, the following code in Node.js should make sure that we don't allow Trailer without transfer-encoding: chunked: 80c9ef0

[pjmolina]

I see your point @apapirovski
Agreed to remove Express and try it again.
Do you have a minimal working sample I can use to test it on Bluemix?

[dougwilson]

Express doesn't monkey patch anything -- it only adds new methods. Not sure what you are referring to, exactly. Can you point to where this monkey patching is?

[arturgvieira-zz]

I wrote a test when I first developed the PR for this Issue. The test itself should be a good starting point for an all NodeJS solution.

[apapirovski]

@dougwilson Sorry, I should've been clearer. There are middleware that are included within express itself (or at least referenced within the test suite, at the very least) that will, for example, monkey patch writeHead to set default headers. I haven't looked through the entirety of the source code but that's the example I had in mind because I specifically ran into it while working on http2 compatibility mode.

[dougwilson]

On that note, were you ever going to provide the code you used for testing at some point :) ?

[apapirovski]

@dougwilson do you want me to just make a git repo with my copy of express & node_modules? 😆I mean, I can do that... it'll be a mess to work with but I suppose it might be better than nothing.

I've been trying to carve out some time to get PRs for the actual packages (supertest, superagent, etc.) used but it's a bit more work than just temporarily monkey-patching them.

[apapirovski]

@pjmolina Are you sure you tested with Node v8 & v9? I'm getting the expected error: Error [ERR_HTTP_TRAILER_INVALID]: Trailers are invalid with this transfer encoding. It's possible your 502 above might be because Node is throwing an error, as it should.

[apapirovski]

Based on my testing, this has been addressed. Closing.

[arturgvieira-zz]

Thanks for all the help, I had a lot of fun. I learned quite a bit of web standards, and techniques in NodeJS, which I am very thankful for.