Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: reject control characters in http.request() #8923

Merged
merged 1 commit into from
Oct 13, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions lib/_http_client.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,12 @@ function ClientRequest(options, cb) {
if (self.agent && self.agent.protocol)
expectedProtocol = self.agent.protocol;

if (options.path && / /.test(options.path)) {
if (options.path && /[\u0000-\u0020]/.test(options.path)) {
// The actual regex is more like /[^A-Za-z0-9\-._~!$&'()*+,;=/:@]/
// with an additional rule for ignoring percentage-escaped characters
// but that's a) hard to capture in a regular expression that performs
// well, and b) possibly too restrictive for real-world usage. That's
// why it only scans for spaces because those are guaranteed to create
// an invalid request.
// well, and b) possibly too restrictive for real-world usage.
// Restrict the filter to control characters and spaces.
throw new TypeError('Request path contains unescaped characters');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RangeError?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, but for a different PR because people may want to be careful and consider that semver-major?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think this would be a unecessary breaking change if we consider error types as API. So, LGTM as-is.

} else if (protocol !== expectedProtocol) {
throw new Error('Protocol "' + protocol + '" not supported. ' +
Expand Down
19 changes: 12 additions & 7 deletions test/parallel/test-http-client-unescaped-path.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
'use strict';
var common = require('../common');
var assert = require('assert');
var http = require('http');
const common = require('../common');
const assert = require('assert');
const http = require('http');

assert.throws(function() {
// Path with spaces in it should throw.
http.get({ path: 'bad path' }, common.fail);
}, /contains unescaped characters/);
function* bad() {
for (let i = 0; i <= 32; i += 1)
yield 'bad' + String.fromCharCode(i) + 'path';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it doesn't add any real value but what about using a template literal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially had that but I felt it was less readable than concatenation. Consider:

yield 'bad' + String.fromCharCode(i) + 'path';

Vs.

yield `bad${String.fromCharCode(i)}path`;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah no big deal.

}

for (const path of bad()) {
assert.throws(() => http.get({ path }, common.fail),
/contains unescaped characters/);
}