From 14619971c93dd7f9604b310edab761288c56020f Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Fri, 20 Apr 2018 22:48:02 -0700 Subject: [PATCH 1/3] url: make urlToOptions tamper-safe --- lib/internal/safe_globals.js | 1 + lib/internal/url.js | 33 ++++++++++++++++++++- test/parallel/test-whatwg-url-properties.js | 19 ++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/lib/internal/safe_globals.js b/lib/internal/safe_globals.js index ad58fa662b53ef..f8795dc5be4804 100644 --- a/lib/internal/safe_globals.js +++ b/lib/internal/safe_globals.js @@ -21,6 +21,7 @@ const makeSafe = (unsafe, safe) => { return safe; }; +exports.makeSafe = makeSafe; exports.SafeMap = makeSafe(Map, class SafeMap extends Map {}); exports.SafeSet = makeSafe(Set, class SafeSet extends Set {}); exports.SafePromise = makeSafe(Promise, class SafePromise extends Promise {}); diff --git a/lib/internal/url.js b/lib/internal/url.js index cff94e6b7d2b5b..3b24748a4b6573 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -6,6 +6,8 @@ const { isHexTable } = require('internal/querystring'); +const { makeSafe } = require('internal/safe_globals'); + const { getConstructorOf, removeColors } = require('internal/util'); const { ERR_ARG_NOT_ITERABLE, @@ -107,6 +109,20 @@ class URLContext { this.query = null; this.fragment = null; } + + static clone(old) { + const ctx = new URLContext(); + ctx.flags = old.flags; + ctx.scheme = old.scheme; + ctx.username = old.username; + ctx.password = old.password; + ctx.host = old.host; + ctx.port = old.port; + ctx.path = [...old.path]; + ctx.query = old.query; + ctx.fragment = old.fragment; + return ctx; + } } class URLSearchParams { @@ -1308,11 +1324,26 @@ function domainToUnicode(domain) { return _domainToUnicode(`${domain}`); } +const SafeURL = makeSafe(URL, class SafeURL extends URL { + // Wraps an existing URL in SafeURL's prototype object so we can be sure that + // the getters/setters have not been tampered with. No such guarantees for + // the url.searchParams object however. All modifications reflect on the + // original URL object. + static fromURL(url) { + return { + __proto__: SafeURL.prototype, + [context]: url[context], + [searchParams]: url[searchParams] + }; + } +}); + // Utility function that converts a URL object into an ordinary // options object as expected by the http.request and https.request // APIs. function urlToOptions(url) { - var options = { + url = SafeURL.fromURL(url); + const options = { protocol: url.protocol, hostname: url.hostname.startsWith('[') ? url.hostname.slice(1, -1) : diff --git a/test/parallel/test-whatwg-url-properties.js b/test/parallel/test-whatwg-url-properties.js index 230315a70efdfc..214a92c568c859 100644 --- a/test/parallel/test-whatwg-url-properties.js +++ b/test/parallel/test-whatwg-url-properties.js @@ -148,6 +148,25 @@ assert.strictEqual(url.searchParams, oldParams); assert.strictEqual(hostname, '::1'); } +// Ensure urlToOptions works on URL with prototype that has been tampered with. +{ + const url = new URL('http://user:pass@foo.bar.com:21/aaa/zzz?l=24#test'); + Object.setPrototypeOf(url, Object.create(null)); + const opts = urlToOptions(url); + assert.strictEqual(opts instanceof URL, false); + assert.strictEqual(opts.protocol, 'http:'); + assert.strictEqual(opts.auth, 'user:pass'); + assert.strictEqual(opts.hostname, 'foo.bar.com'); + assert.strictEqual(opts.port, 21); + assert.strictEqual(opts.path, '/aaa/zzz?l=24'); + assert.strictEqual(opts.pathname, '/aaa/zzz'); + assert.strictEqual(opts.search, '?l=24'); + assert.strictEqual(opts.hash, '#test'); + + const { hostname } = urlToOptions(new URL('http://[::1]:21')); + assert.strictEqual(hostname, '::1'); +} + // Test special origins [ { expected: 'https://whatwg.org', From 5f111354db0b504153b33d2472dbb8e7941d798b Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Fri, 20 Apr 2018 22:32:04 -0700 Subject: [PATCH 2/3] http: use WHATWG URL parser for http.request/get --- doc/api/deprecations.md | 12 ++++++++++++ lib/_http_client.js | 26 ++++++++++++++++++++++---- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 6689f03a25edef..1b11d574f42ebe 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -994,6 +994,16 @@ because it also made sense to interpret the value as the number of bytes read by the engine, but is inconsistent with other streams in Node.js that expose values under these names. + +### DEP00XX: http.request() and http.get() URL parsing + +Type: Runtime + +Some previously supported (but strictly invalid) URLs were accepted through the +[`http.request()`][] and related APIs. This behavior is now deprecated. It is +recommended that one passes a [`URL`][] object to the API as the first argument +which ensures URL validity. + [`--pending-deprecation`]: cli.html#cli_pending_deprecation [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array @@ -1009,6 +1019,7 @@ expose values under these names. [`Server.getConnections()`]: net.html#net_server_getconnections_callback [`Server.listen({fd: })`]: net.html#net_server_listen_handle_backlog_callback [`SlowBuffer`]: buffer.html#buffer_class_slowbuffer +[`URL`]: url.html#url_class_url [`asyncResource.runInAsyncScope()`]: async_hooks.html#async_hooks_asyncresource_runinasyncscope_fn_thisarg_args [`child_process`]: child_process.html [`console.error()`]: console.html#console_console_error_data_args @@ -1035,6 +1046,7 @@ expose values under these names. [`fs.read()`]: fs.html#fs_fs_read_fd_buffer_offset_length_position_callback [`fs.readSync()`]: fs.html#fs_fs_readsync_fd_buffer_offset_length_position [`fs.stat()`]: fs.html#fs_fs_stat_path_callback +[`http.request()`]: http.html#http_http_request_options_callback [`os.networkInterfaces`]: os.html#os_os_networkinterfaces [`os.tmpdir()`]: os.html#os_os_tmpdir [`process.env`]: process.html#process_process_env diff --git a/lib/_http_client.js b/lib/_http_client.js index c3ef9de20498df..38c7ab4fe62225 100644 --- a/lib/_http_client.js +++ b/lib/_http_client.js @@ -37,7 +37,7 @@ const { OutgoingMessage } = require('_http_outgoing'); const Agent = require('_http_agent'); const { Buffer } = require('buffer'); const { defaultTriggerAsyncIdScope } = require('internal/async_hooks'); -const { urlToOptions, searchParamsSymbol } = require('internal/url'); +const { urlToOptions, searchParamsSymbol, URL } = require('internal/url'); const { outHeadersKey, ondrain } = require('internal/http'); const { ERR_HTTP_HEADERS_SENT, @@ -51,6 +51,8 @@ const { validateTimerDuration } = require('internal/timers'); const INVALID_PATH_REGEX = /[^\u0021-\u00ff]/; +let urlWarningEmitted = false; + function validateHost(host, name) { if (host !== null && host !== undefined && typeof host !== 'string') { throw new ERR_INVALID_ARG_TYPE(`options.${name}`, @@ -64,9 +66,25 @@ function ClientRequest(options, cb) { OutgoingMessage.call(this); if (typeof options === 'string') { - options = url.parse(options); - if (!options.hostname) { - throw new ERR_INVALID_DOMAIN_NAME(); + const urlStr = options; + try { + options = urlToOptions(new URL(urlStr)); + } catch (err) { + try { + options = url.parse(urlStr); + if (!options.hostname) { + throw new ERR_INVALID_DOMAIN_NAME(); + } + if (!urlWarningEmitted) { + urlWarningEmitted = true; + process.emitWarning( + `The provided URL ${urlStr} is not a valid URL, and is supported ` + + 'in the http module solely for compatibility.', + 'DeprecationWarning', 'DEP00XX'); + } + } catch (urlParseErr) { + throw err; + } } } else if (options && options[searchParamsSymbol] && options[searchParamsSymbol][searchParamsSymbol]) { From c2c172dc570a87807604253e86e68ea47845ba8e Mon Sep 17 00:00:00 2001 From: Timothy Gu Date: Wed, 25 Apr 2018 07:26:31 -0700 Subject: [PATCH 3/3] fixup! url: make urlToOptions tamper-safe --- lib/internal/url.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/lib/internal/url.js b/lib/internal/url.js index 3b24748a4b6573..7e9f390092fe10 100644 --- a/lib/internal/url.js +++ b/lib/internal/url.js @@ -109,20 +109,6 @@ class URLContext { this.query = null; this.fragment = null; } - - static clone(old) { - const ctx = new URLContext(); - ctx.flags = old.flags; - ctx.scheme = old.scheme; - ctx.username = old.username; - ctx.password = old.password; - ctx.host = old.host; - ctx.port = old.port; - ctx.path = [...old.path]; - ctx.query = old.query; - ctx.fragment = old.fragment; - return ctx; - } } class URLSearchParams {