http: edit _storeHeader to check for Trailer header #12990

arturgvieira-zz · 2017-05-12T07:28:50Z

Test non-chunked message does not have trailer header set,
message will be 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 or 'trailers'.

Ref: #2842

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

http

mscdex · 2017-05-12T09:39:10Z

lib/_http_server.js

+  // 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.
+  if (this.statusCode === 304 && headers && headers.hasOwnProperty('Trailer')) {


Please avoid hasOwnProperty() for performance reasons.

What would be a better choice?

Just try accessing the property and check if it is set to anything.

Thank you, will make the changes shortly.

mscdex · 2017-05-12T09:40:42Z

lib/_http_server.js

+  // (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.


This comment doesn't really describe the code below it well. The code doesn't even check for HEAD requests or 1xx/204 responses.

I will revise, thanks for the review.

arturgvieira-zz · 2017-05-12T10:19:21Z

@mscdex I made edits to the comment, it is more specific to the actual code.

mscdex · 2017-05-12T10:55:28Z

lib/_http_server.js

+  // 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 or 'trailers' octets.
+  if (statusCode === 304 && headers && headers['Trailer'] !== undefined)


Actually, now that I think more about it, just checking for 'Trailer' won't be enough because the letters could be in any case. I wonder if a better solution would be to just check for statusCode === 304 && state.trailer inside _storeHeader() in _http_outgoing.js? Perhaps it could be merged with the existing 204/304 check that's already being done in there.

I also don't know if we should throw or just discard the trailer header...
Thoughts @nodejs/http ?

All good insights. #2842 had a mention about throwing. If it is preferred to discard I'll make the change.

arturgvieira-zz · 2017-05-12T12:45:25Z

@mscdex All done. I updated the location of the check, thank you for pointing out _storeHeader.

cjihrig · 2017-05-12T13:45:49Z

test/parallel/test-http-server-de-chunked-trailer.js

+
+const server = http.createServer(common.mustCall(function(req, res) {
+  assert.throws(() => res.writeHead(304, { 'Trailer': 'baz' }),
+                /^Error: Trailer header is invalid with current status code?/);


Should the ? be a $?

Yes, I'll correct it. Thank you

cjihrig · 2017-05-12T13:46:54Z

test/parallel/test-http-server-de-chunked-trailer.js

+server.listen(0, () => {
+  http.get({
+    port: server.address().port
+  }, common.mustCall((res) => {


common.mustCall() is not needed here.

I will update it. Thanks

arturgvieira-zz · 2017-05-12T13:51:58Z

@cjihrig All done, I corrected the errors. Thanks for the review.

arturgvieira-zz · 2017-05-17T17:07:51Z

@cjihrig Hi, if you have time, could you check if I need to make any further changes?

cjihrig

LGTM. I'd like feedback from @nodejs/http though.

arturgvieira-zz · 2017-05-17T17:20:33Z

Thank you.

mscdex · 2017-05-17T18:18:58Z

I've read RFC 7230 a bit more on this and my understanding/interpretation is that a Trailer header should be fine for 304 as it could indicate that a trailer would have been sent if the request was an unconditional GET (the RFC gives a similar example/explanation for a Transfer-Encoding header sent in a 304). Node already doesn't send the actual trailer for such status codes.

jasnell · 2017-05-17T19:21:10Z

That's my understanding as well @mscdex but the spec is rather vague on the topic.

arturgvieira-zz · 2017-05-17T19:27:47Z

I was actually interested in closing the issue. I read over the RFC as well and I am fine with whatever you guys decide. The Bluemix story may be a concern? Thoughts.

mscdex · 2017-05-17T20:40:35Z

I think the original linked issue seems to be suggesting that any Trailer header should be removed when Transfer-Encoding: chunked is not set since Trailer is a component of a chunk encoded response, regardless of status code. I guess that makes sense to me, but it'd definitely be a semver-major change IMHO.

gibfahn · 2017-05-20T10:58:32Z

The Bluemix story may be a concern? Thoughts.

~~@arturgvieira could you elaborate? Not sure what the Bluemix story is.~~

Ahh, the linked issue (#2842), got it!

refack · 2017-05-25T22:24:58Z

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/831/
CI: https://ci.nodejs.org/job/node-test-commit/10164/

refack · 2017-05-25T22:26:23Z

I think the original linked issue seems to be suggesting that any Trailer header should be removed when Transfer-Encoding: chunked is not set since Trailer is a component of a chunk encoded response, regardless of status code. I guess that makes sense to me, but it'd definitely be a semver-major change IMHO.

@mscdex @jasnell Just to be clear, this case (304 only) is considered semver-patch?

mscdex · 2017-05-25T22:39:48Z

@refack I think I'd rather just have one change for this particular issue and currently, discarding the trailer on a non-chunked response seems like the best option to me (and that would be semver-major).

refack · 2017-05-25T22:40:56Z

@refack I think I'd rather just have one change for this particular issue and currently, discarding the trailer on a non-chunked response seems like the best option to me (and that would be semver-major).

So I'm blocking this.

refack · 2017-05-25T22:44:12Z

@arturgvieira go for the gold... i.e. discard the trailer for all non-chunked responses.
P.S. since headers can be added in any order I'm not sure you can do the trick you did here...

mscdex · 2017-05-25T22:54:21Z

Sorry, I meant the Trailer header, actual trailer content should already be discarded for non-chunked responses.

arturgvieira-zz · 2017-05-25T23:23:38Z

I'm checking for lint errors.

arturgvieira-zz · 2017-05-25T23:32:52Z

All lint errors have been corrected.

refack · 2017-05-25T23:41:47Z

@arturgvieira we don't have consensus on this fix. @mscdex and I think that you should solve the general case #12990 (comment).
That is:

move the test to around L434(https://github.com/nodejs/node/pull/12990/files#diff-286202fdbdd74ede6f5f5334b6176b5cR434)
Check for this.chunkedEncoding !== true && state.trailer
Evaluate if response.addTrailers need modifications
Document in https://github.com/nodejs/node/blob/master/doc/api/http.md#responsesetheadername-value and response.addTrailers.
Expand the test.

mscdex · 2017-05-26T00:13:20Z

I don't think addTrailers() needs any changes, since that content is already being ignored properly. All we should care about is skipping the Trailer header when chunked encoding is not being used. I don't think there is going to be a straight-forward change because a default chunked encoding header could be appended after the header is already generated in _storeHeader().

arturgvieira-zz · 2017-06-11T21:07:19Z

I made the requested corrections.

refack

Good fix 🥇

refack · 2017-06-11T23:18:31Z

test/parallel/test-http-server-de-chunked-trailer.js

+const server = http.createServer(common.mustCall(function(req, res) {
+  res.setHeader('Trailer', 'baz');
+  assert.throws(() => res.writeHead(200, {'Content-Length': '24'}),
+                common.expectsError({ code: 'ERR_HTTP_TRAILER_INVALID' }));


nit: Could you add message: 'Trailers are invalid with this transfer encoding', type: Error

arturgvieira-zz · 2017-06-11T23:32:07Z

All done, I added the additional information for the error as requested.

refack

💯

refack · 2017-06-11T23:57:05Z

CI: https://ci.nodejs.org/job/node-test-commit/10506/

refack · 2017-06-12T20:55:02Z

/cc @nodejs/ctc

mcollina

LGTM

mscdex · 2017-06-12T23:02:29Z

lib/_http_outgoing.js

+  // header fields, regardless of the header fields present in the
+  // message, and thus cannot contain a message body or 'trailers'.
+  if (this.chunkedEncoding !== true && state.trailer) {
+    const trailerInvalidError = new errors.Error('ERR_HTTP_TRAILER_INVALID');


I think this can be simplified to just throw new errors.Error(...)?

mscdex · 2017-06-12T23:08:45Z

test/parallel/test-http-server-de-chunked-trailer.js

+const server = http.createServer(common.mustCall(function(req, res) {
+  res.setHeader('Trailer', 'baz');
+  assert.throws(() => res.writeHead(200, {'Content-Length': '24'}),
+                common.expectsError({ code: 'ERR_HTTP_TRAILER_INVALID',


Perhaps move the object literal into a separate variable and pass that to common.expectsError() to make this more readable?:

const trailerInvalidErr = { code: 'ERR_HTTP_TRAILER_INVALID', message: 'Trailers are invalid with this transfer encoding', type: Error }; assert.throws(() => res.writeHead(200, {'Content-Length': '24'}), common.expectsError(trailerInvalidErr));

mscdex · 2017-06-12T23:09:44Z

test/parallel/test-http-server-de-chunked-trailer.js

+'use strict';
+const common = require('../common');
+
+// This test ensures that when a status code of 304 is set a Trailer header


This description does not agree with the code below.

arturgvieira-zz · 2017-06-13T00:10:04Z

All done, made the changes requested

mscdex · 2017-06-13T00:51:47Z

test/parallel/test-http-server-de-chunked-trailer.js

+}));
+server.listen(0, common.mustCall(() => {
+  http.get({ port: server.address().port }, common.mustCall((res) => {
+    server.close();


Minor nit: you might add a little extra code here to verify that the response is what you expect (both statusCode and body content). In doing so, you'd need to make sure to adjust the Content-Length value to 2.

Ok, will do.

arturgvieira-zz · 2017-06-13T01:33:51Z

Made the requested changes. Thanks @mscdex

mscdex · 2017-06-13T01:45:52Z

test/parallel/test-http-server-de-chunked-trailer.js

+}));
+server.listen(0, common.mustCall(() => {
+  http.get({ port: server.address().port }, common.mustCall((res) => {
+    assert.deepStrictEqual(res.statusCode, 200);


This should just be .strictEqual().

mscdex · 2017-06-13T01:47:00Z

test/parallel/test-http-server-de-chunked-trailer.js

+server.listen(0, common.mustCall(() => {
+  http.get({ port: server.address().port }, common.mustCall((res) => {
+    assert.deepStrictEqual(res.statusCode, 200);
+    res.on('data', common.mustCall((content) => {


Technically it would be better to just buffer data in here (without common.mustCall()) and then assert.strictEqual(buffer, 'ok') in a common.mustCall()-wrapped 'end' event handler:

var buf = ''; res.on('data', (chunk) => { buf += chunk; }).on('end', common.mustCall(() => { assert.strictEqual(buf, 'ok'); }));

The reason being in general you shouldn't assume anything about the number of 'data' events for a stream.

arturgvieira-zz · 2017-06-13T01:57:20Z

@mscdex Take a look, I think this is what you meant.

mscdex · 2017-06-13T02:12:01Z

test/parallel/test-http-server-de-chunked-trailer.js

+    assert.strictEqual(res.statusCode, 200);
+    let buffer;
+    res.on('data', (content) => {
+      buffer ? buffer += content : buffer = content;


IMO this is less readable, I think it's best to just always append to a variable that is initially an empty string. Also you will be able to drop the explicit buffer.toString().

Ok, I just saw your edit. Changing now.

Test non-chunked message does not have trailer header set, message will be 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 or 'trailers'. Ref: #2842

arturgvieira-zz · 2017-06-13T02:15:37Z

@mscdex Thank you for the review.

mscdex · 2017-06-13T02:25:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8620/

mscdex · 2017-06-13T03:12:11Z

LGTM

refack · 2017-06-13T17:23:50Z

@jasnell PTAL (even though we have 3 CTC LGTMs)

Test non-chunked message does not have trailer header set, message will be 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 or 'trailers'. PR-URL: #12990 Ref: #2842 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Brian White <[email protected]>

jasnell · 2017-06-13T20:09:09Z

Landed in 80c9ef0

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label May 12, 2017

arturgvieira-zz changed the title ~~Server header branch~~ http: edit writeHead to check for 304 and Trailer May 12, 2017

mscdex reviewed May 12, 2017

View reviewed changes

arturgvieira-zz changed the title ~~http: edit writeHead to check for 304 and Trailer~~ http: edit _storeHeader to check for 304 and Trailer May 12, 2017

cjihrig requested changes May 12, 2017

View reviewed changes

cjihrig approved these changes May 17, 2017

View reviewed changes

refack self-assigned this May 25, 2017

refack added the blocked PRs that are blocked by other issues or PRs. label May 25, 2017

refack approved these changes Jun 11, 2017

View reviewed changes

mcollina approved these changes Jun 12, 2017

View reviewed changes

refack removed the wip Issues and PRs that are still a work in progress. label Jun 12, 2017

mscdex reviewed Jun 12, 2017

View reviewed changes

mscdex reviewed Jun 13, 2017

View reviewed changes

hiroppy approved these changes Jun 13, 2017

View reviewed changes

jasnell approved these changes Jun 13, 2017

View reviewed changes

jasnell closed this Jun 13, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

refack removed their assignment Oct 20, 2018

http: edit _storeHeader to check for Trailer header #12990

http: edit _storeHeader to check for Trailer header #12990

Conversation

arturgvieira-zz commented May 12, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturgvieira-zz commented May 12, 2017

mscdex May 12, 2017 • edited Loading

Choose a reason for hiding this comment

arturgvieira-zz May 12, 2017 • edited Loading

Choose a reason for hiding this comment

arturgvieira-zz commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturgvieira-zz commented May 12, 2017 • edited Loading

arturgvieira-zz commented May 17, 2017 • edited Loading

cjihrig left a comment

Choose a reason for hiding this comment

arturgvieira-zz commented May 17, 2017

mscdex commented May 17, 2017 • edited Loading

jasnell commented May 17, 2017

arturgvieira-zz commented May 17, 2017

mscdex commented May 17, 2017 • edited Loading

gibfahn commented May 20, 2017 • edited Loading

refack commented May 25, 2017 • edited Loading

refack commented May 25, 2017

mscdex commented May 25, 2017

refack commented May 25, 2017

refack commented May 25, 2017

mscdex commented May 25, 2017

arturgvieira-zz commented May 25, 2017

arturgvieira-zz commented May 25, 2017

refack commented May 25, 2017 • edited Loading

mscdex commented May 26, 2017 • edited Loading

arturgvieira-zz commented Jun 11, 2017

refack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturgvieira-zz commented Jun 11, 2017

refack left a comment

Choose a reason for hiding this comment

refack commented Jun 11, 2017

refack commented Jun 12, 2017

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturgvieira-zz commented Jun 13, 2017

Choose a reason for hiding this comment

arturgvieira-zz Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

arturgvieira-zz commented Jun 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

arturgvieira-zz commented Jun 13, 2017

mscdex Jun 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturgvieira-zz commented Jun 13, 2017

mscdex commented Jun 13, 2017

mscdex commented Jun 13, 2017

refack commented Jun 13, 2017

jasnell commented Jun 13, 2017

arturgvieira-zz commented May 12, 2017 •

edited

Loading

mscdex May 12, 2017 •

edited

Loading

arturgvieira-zz May 12, 2017 •

edited

Loading

arturgvieira-zz commented May 12, 2017 •

edited

Loading

arturgvieira-zz commented May 17, 2017 •

edited

Loading

mscdex commented May 17, 2017 •

edited

Loading

mscdex commented May 17, 2017 •

edited

Loading

gibfahn commented May 20, 2017 •

edited

Loading

refack commented May 25, 2017 •

edited

Loading

refack commented May 25, 2017 •

edited

Loading

mscdex commented May 26, 2017 •

edited

Loading

arturgvieira-zz Jun 13, 2017 •

edited

Loading

mscdex Jun 13, 2017 •

edited

Loading

mscdex Jun 13, 2017 •

edited

Loading