http: improve invalid character in header error message #9010

evanlucas · 2016-10-10T17:07:38Z

Checklist

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

Affected core subsystem(s)

http

Description of change

This commit includes the header name in the error message when invalid
characters are in the value.

jasnell

LGTM with a nit.

jasnell · 2016-10-10T17:13:12Z

test/parallel/test-http-client-invalid-header.js

+'use strict';
+var common = require('../common');
+var assert = require('assert');
+var http = require('http');


ack, not sure what I was thinking.

cjihrig

LGTM pending @jasnell's nits.

lpinca

LGTM

lpinca · 2016-10-12T16:20:12Z

Tiny nit: commit subject line is a bit too long (55 chars) but I honestly have not idea how to make it shorter.

Fishrock123 · 2016-10-12T16:42:15Z

@ChALkeR could you search for The header content contains invalid characters

jasnell · 2016-10-17T17:26:11Z

Ping ... @evanlucas @Fishrock123 @ChALkeR ... as a semver-major this needs to be landed ASAP if it's going to make it into v7.0.0. We're a week out from the release and I do not want to land a semver-major last minute.

evanlucas · 2016-10-17T22:35:19Z

Commit message title has been updated.

bnoordhuis · 2016-10-18T10:05:45Z

lib/_http_outgoing.js

@@ -355,7 +355,8 @@ OutgoingMessage.prototype.setHeader = function(name, value) {
 if (this._header)
 throw new Error('Can\'t set headers after they are sent.');
 if (common._checkInvalidHeaderChar(value) === true) {
- throw new TypeError('The header content contains invalid characters');
+ throw new TypeError(
+ `The header content for "${name}" contains invalid characters`);


Style: a line continuation should have four spaces of indent.

bnoordhuis · 2016-10-18T10:08:39Z

Anyone have opinions on whether this could be used in information leaks or, if the header name is attacker-controlled, inserting tainted data in logs or terminals? It seems farfetched but it's good to think about such things.

evanlucas · 2016-10-18T11:11:04Z

@bnoordhuis that is a good point. Thanks for bringing it up. We are already throwing if the header name is not valid with a message like 'Header name must be a valid HTTP Token ["' + name + '"]'. We also include the header name in the error message if the value is undefined. Is there an alternative that you suggest that would make this less concerning to you?

addaleax · 2016-10-18T13:40:41Z

How about ${JSON.stringify(name)} instead of "${name}"? That might be a bit simplistic but at least it escapes anything there is to escape.

This commit includes the header name in the error message when invalid characters are in the value.

evanlucas · 2016-10-19T01:52:15Z

Updated with @addaleax's suggestion.

ChALkeR · 2016-10-19T05:09:18Z

@Fishrock123 @evanlucas Sorry for the delay on this one — still catching up and finishing unfinished stuff =).

All matches for header content contains (from August, I have not yet updated the dataset since then):

http-node-1.2.0.tgz/_http_outgoing.js:309:    throw new TypeError('The header content contains invalid characters');
http-node-1.2.0.tgz/_http_outgoing.js:348:    throw new TypeError('The header content contains invalid characters');
http-node-1.2.0.tgz/_http_outgoing.js:522:      throw new TypeError('The header content contains invalid characters');

Note that 7bef1b7 is relatively new, so I might need to sync the dataset today and re-check another time.

jasnell · 2016-10-20T16:27:12Z

@evanlucas ... I will be running the v7.0.0 RC1 build this afternoon. If this should get in to v7, then it needs to land this morning.

Trott · 2016-10-20T17:56:59Z

This was discussed at yesterday's CTC meeting and a resolution was arrived at, so I'm going to remove the ctc-agenda label.

ChALkeR · 2016-10-23T08:44:35Z

Ok, I updated the dataset to 2016-10-22, the results for header content contains are pretty much the same the same as they were in #9010 (comment):

frida-http-1.0.1.tgz/lib/_http_outgoing.js:313:    throw new TypeError('The header content contains invalid characters');
frida-http-1.0.1.tgz/lib/_http_outgoing.js:353:    throw new TypeError('The header content contains invalid characters');
http-node-1.2.0.tgz/_http_outgoing.js:309:    throw new TypeError('The header content contains invalid characters');
http-node-1.2.0.tgz/_http_outgoing.js:348:    throw new TypeError('The header content contains invalid characters');
http-node-1.2.0.tgz/_http_outgoing.js:522:      throw new TypeError('The header content contains invalid characters');
rapx-win-1.4.2.tgz/ruff_modules/http/src/_http_outgoing.js:312:        throw new TypeError('The header content contains invalid characters');
rapx-win-1.4.2.tgz/ruff_modules/http/src/_http_outgoing.js:351:        throw new TypeError('The header content contains invalid characters');

Only copies of _http_outgoing.js that were added to packages for whatever reason contain that string, It doesn't look like anyone currently depends on the exact error message.

Note that delaying this until v8.0 will give the package authors more time to introduce dependencies on the error message, so this would have to be re-checked later.

Upd: I performed another search for content contains to be more sure, as this PR just inserts more words between «content» and «contains». Same results (and some unrelated stuff that I discarded):

> ./search.code.sh 'content contains' | grep -vE '(that|the|text|new|HTML|node|tab|window|loaded|provided) content contains' | grep -vE '(fb|interaction|res|resultObj).content contains' | grep -vE 'content contains (the|HTML|myCoolFramework)'
frida-http-1.0.1.tgz/lib/_http_outgoing.js:313:    throw new TypeError('The header content contains invalid characters');
frida-http-1.0.1.tgz/lib/_http_outgoing.js:353:    throw new TypeError('The header content contains invalid characters');
frida-http-1.0.1.tgz/lib/_http_outgoing.js:527:      throw new TypeError('The trailer content contains invalid characters');
http-node-1.2.0.tgz/_http_outgoing.js:309:    throw new TypeError('The header content contains invalid characters');
http-node-1.2.0.tgz/_http_outgoing.js:348:    throw new TypeError('The header content contains invalid characters');
http-node-1.2.0.tgz/_http_outgoing.js:522:      throw new TypeError('The header content contains invalid characters');
rapx-win-1.4.2.tgz/ruff_modules/http/src/_http_outgoing.js:312:        throw new TypeError('The header content contains invalid characters');
rapx-win-1.4.2.tgz/ruff_modules/http/src/_http_outgoing.js:351:        throw new TypeError('The header content contains invalid characters');
rapx-win-1.4.2.tgz/ruff_modules/http/src/_http_outgoing.js:522:            throw new TypeError('The trailer content contains invalid characters');

ChALkeR · 2016-10-23T08:50:14Z

Btw, what about The trailer content contains invalid characters error? Shouldn't it be also changed?

evanlucas · 2016-10-24T15:49:54Z

@ChALkeR thanks, I'll add one for trailers too.

evanlucas · 2016-11-02T12:50:47Z

Closing as we landed the debug messages instead (#9195)

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Oct 10, 2016

evanlucas added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 10, 2016

jasnell approved these changes Oct 10, 2016

View reviewed changes

cjihrig approved these changes Oct 10, 2016

View reviewed changes

evanlucas force-pushed the hcmoreinfo branch from dd351c3 to 6dbebaa Compare October 10, 2016 17:42

lpinca approved these changes Oct 10, 2016

View reviewed changes

evanlucas added the ctc-agenda label Oct 12, 2016

ChALkeR self-assigned this Oct 12, 2016

Fishrock123 added this to the 7.0.0 milestone Oct 12, 2016

evanlucas force-pushed the hcmoreinfo branch from 6dbebaa to e2a5760 Compare October 17, 2016 22:34

Trott mentioned this pull request Oct 17, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-10-19 #9143

Closed

bnoordhuis requested changes Oct 18, 2016

View reviewed changes

evanlucas force-pushed the hcmoreinfo branch from e2a5760 to 0b0f102 Compare October 18, 2016 10:55

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

http: improve invalid header character error

f1f9f7b

This commit includes the header name in the error message when invalid characters are in the value.

evanlucas force-pushed the hcmoreinfo branch from 0b0f102 to f1f9f7b Compare October 19, 2016 01:51

evanlucas mentioned this pull request Oct 19, 2016

http: add debug message for invalid header value #9195

Closed

2 tasks

Trott removed the ctc-agenda label Oct 20, 2016

evanlucas modified the milestones: 8.0.0, 7.0.0 Oct 20, 2016

ChALkeR removed their assignment Oct 23, 2016

evanlucas closed this Nov 2, 2016

evanlucas deleted the hcmoreinfo branch November 2, 2016 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: improve invalid character in header error message #9010

http: improve invalid character in header error message #9010

evanlucas commented Oct 10, 2016 •

edited

Loading

jasnell left a comment

jasnell Oct 10, 2016

evanlucas Oct 10, 2016

cjihrig left a comment

lpinca left a comment

lpinca commented Oct 12, 2016 •

edited

Loading

Fishrock123 commented Oct 12, 2016

jasnell commented Oct 17, 2016

evanlucas commented Oct 17, 2016

bnoordhuis Oct 18, 2016

bnoordhuis commented Oct 18, 2016

evanlucas commented Oct 18, 2016

addaleax commented Oct 18, 2016

evanlucas commented Oct 19, 2016

ChALkeR commented Oct 19, 2016

jasnell commented Oct 20, 2016

Trott commented Oct 20, 2016

ChALkeR commented Oct 23, 2016 •

edited

Loading

ChALkeR commented Oct 23, 2016

evanlucas commented Oct 24, 2016

evanlucas commented Nov 2, 2016

http: improve invalid character in header error message #9010

http: improve invalid character in header error message #9010

Conversation

evanlucas commented Oct 10, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

jasnell left a comment

Choose a reason for hiding this comment

jasnell Oct 10, 2016

Choose a reason for hiding this comment

evanlucas Oct 10, 2016

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

lpinca left a comment

Choose a reason for hiding this comment

lpinca commented Oct 12, 2016 • edited Loading

Fishrock123 commented Oct 12, 2016

jasnell commented Oct 17, 2016

evanlucas commented Oct 17, 2016

bnoordhuis Oct 18, 2016

Choose a reason for hiding this comment

bnoordhuis commented Oct 18, 2016

evanlucas commented Oct 18, 2016

addaleax commented Oct 18, 2016

evanlucas commented Oct 19, 2016

ChALkeR commented Oct 19, 2016

jasnell commented Oct 20, 2016

Trott commented Oct 20, 2016

ChALkeR commented Oct 23, 2016 • edited Loading

ChALkeR commented Oct 23, 2016

evanlucas commented Oct 24, 2016

evanlucas commented Nov 2, 2016

evanlucas commented Oct 10, 2016 •

edited

Loading

lpinca commented Oct 12, 2016 •

edited

Loading

ChALkeR commented Oct 23, 2016 •

edited

Loading