Skip to content

Commit 564048d

Browse files
HackzzilaBridgeAR
Hackzzila
authored and
BridgeAR
committed
http,https,tls: switch to WHATWG URL parser
This switches the url parser from `url.parse()` to the WHATWG URL parser while keeping `url.parse()` as fallback. Also add tests for invalid url deprecations and correct hostname checks. PR-URL: #20270 Fixes: #19468 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
1 parent 511230f commit 564048d

File tree

13 files changed

+149
-27
lines changed

13 files changed

+149
-27
lines changed

‎doc/api/deprecations.md‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -994,6 +994,18 @@ because it also made sense to interpret the value as the number of bytes
994994
read by the engine, but is inconsistent with other streams in Node.js that
995995
expose values under these names.
996996
997+
<a id="DEP0109"></a>
998+
### DEP0109: http, https, and tls support for invalid URLs
999+
1000+
Type: Runtime
1001+
1002+
Some previously supported (but strictly invalid) URLs were accepted through the
1003+
[`http.request()`][], [`http.get()`][], [`https.request()`][],
1004+
[`https.get()`][], and [`tls.checkServerIdentity()`][] APIs because those were
1005+
accepted by the legacy `url.parse()` API. The mentioned APIs now use the WHATWG
1006+
URL parser that requires strictly valid URLs. Passing an invalid URL is
1007+
deprecated and support will be removed in the future.
1008+
9971009
[`--pending-deprecation`]: cli.html#cli_pending_deprecation
9981010
[`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
9991011
[`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array
@@ -1035,6 +1047,10 @@ expose values under these names.
10351047
[`fs.read()`]: fs.html#fs_fs_read_fd_buffer_offset_length_position_callback
10361048
[`fs.readSync()`]: fs.html#fs_fs_readsync_fd_buffer_offset_length_position
10371049
[`fs.stat()`]: fs.html#fs_fs_stat_path_callback
1050+
[`http.get()`]: http.html#http_http_get_options_callback
1051+
[`http.request()`]: http.html#http_http_request_options_callback
1052+
[`https.get()`]: https.html#https_https_get_options_callback
1053+
[`https.request()`]: https.html#https_https_request_options_callback
10381054
[`os.networkInterfaces`]: os.html#os_os_networkinterfaces
10391055
[`os.tmpdir()`]: os.html#os_os_tmpdir
10401056
[`process.env`]: process.html#process_process_env
@@ -1046,6 +1062,7 @@ expose values under these names.
10461062
[`tls.SecureContext`]: tls.html#tls_tls_createsecurecontext_options
10471063
[`tls.SecurePair`]: tls.html#tls_class_securepair
10481064
[`tls.TLSSocket`]: tls.html#tls_class_tls_tlssocket
1065+
[`tls.checkServerIdentity()`]: tls.html#tls_tls_checkserveridentity_host_cert
10491066
[`tls.createSecureContext()`]: tls.html#tls_tls_createsecurecontext_options
10501067
[`util._extend()`]: util.html#util_util_extend_target_source
10511068
[`util.debug()`]: util.html#util_util_debug_string

‎doc/api/errors.md‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1111,11 +1111,6 @@ Invalid characters were detected in headers.
11111111
A cursor on a given stream cannot be moved to a specified row without a
11121112
specified column.
11131113

1114-
<a id="ERR_INVALID_DOMAIN_NAME"></a>
1115-
### ERR_INVALID_DOMAIN_NAME
1116-
1117-
`hostname` can not be parsed from a provided URL.
1118-
11191114
<a id="ERR_INVALID_FD"></a>
11201115
### ERR_INVALID_FD
11211116

‎doc/api/http.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1900,7 +1900,7 @@ Node.js maintains several connections per server to make HTTP requests.
19001900
This function allows one to transparently issue requests.
19011901

19021902
`options` can be an object, a string, or a [`URL`][] object. If `options` is a
1903-
string, it is automatically parsed with [`url.parse()`][]. If it is a [`URL`][]
1903+
string, it is automatically parsed with [`new URL()`][]. If it is a [`URL`][]
19041904
object, it will be automatically converted to an ordinary `options` object.
19051905

19061906
The optional `callback` parameter will be added as a one-time listener for
@@ -2050,6 +2050,7 @@ not abort the request or do anything besides add a `'timeout'` event.
20502050
[`net.Server`]: net.html#net_class_net_server
20512051
[`net.Socket`]: net.html#net_class_net_socket
20522052
[`net.createConnection()`]: net.html#net_net_createconnection_options_connectlistener
2053+
[`new URL()`]: url.html#url_constructor_new_url_input_base
20532054
[`removeHeader(name)`]: #http_request_removeheader_name
20542055
[`request.end()`]: #http_request_end_data_encoding_callback
20552056
[`request.getHeader()`]: #http_request_getheader_name

‎doc/api/https.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ changes:
119119
Like [`http.get()`][] but for HTTPS.
120120

121121
`options` can be an object, a string, or a [`URL`][] object. If `options` is a
122-
string, it is automatically parsed with [`url.parse()`][]. If it is a [`URL`][]
122+
string, it is automatically parsed with [`new URL()`][]. If it is a [`URL`][]
123123
object, it will be automatically converted to an ordinary `options` object.
124124

125125
Example:
@@ -173,7 +173,7 @@ The following additional `options` from [`tls.connect()`][] are also accepted:
173173
`secureOptions`, `secureProtocol`, `servername`, `sessionIdContext`
174174

175175
`options` can be an object, a string, or a [`URL`][] object. If `options` is a
176-
string, it is automatically parsed with [`url.parse()`][]. If it is a [`URL`][]
176+
string, it is automatically parsed with [`new URL()`][]. If it is a [`URL`][]
177177
object, it will be automatically converted to an ordinary `options` object.
178178

179179
Example:
@@ -358,8 +358,8 @@ headers: max-age=0; pin-sha256="WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18="; p
358358
[`https.Agent`]: #https_class_https_agent
359359
[`https.request()`]: #https_https_request_options_callback
360360
[`net.Server`]: net.html#net_class_net_server
361+
[`new URL()`]: url.html#url_constructor_new_url_input_base
361362
[`server.listen()`]: net.html#net_server_listen
362363
[`tls.connect()`]: tls.html#tls_tls_connect_options_callback
363364
[`tls.createSecureContext()`]: tls.html#tls_tls_createsecurecontext_options
364365
[`tls.createServer()`]: tls.html#tls_tls_createserver_options_secureconnectionlistener
365-
[`url.parse()`]: url.html#url_url_parse_urlstring_parsequerystring_slashesdenotehost

‎lib/_http_client.js‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,11 @@ const { OutgoingMessage } = require('_http_outgoing');
3737
const Agent = require('_http_agent');
3838
const { Buffer } = require('buffer');
3939
const { defaultTriggerAsyncIdScope } = require('internal/async_hooks');
40-
const { urlToOptions, searchParamsSymbol } = require('internal/url');
40+
const { URL, urlToOptions, searchParamsSymbol } = require('internal/url');
4141
const { outHeadersKey, ondrain } = require('internal/http');
4242
const {
4343
ERR_HTTP_HEADERS_SENT,
4444
ERR_INVALID_ARG_TYPE,
45-
ERR_INVALID_DOMAIN_NAME,
4645
ERR_INVALID_HTTP_TOKEN,
4746
ERR_INVALID_PROTOCOL,
4847
ERR_UNESCAPED_CHARACTERS
@@ -60,13 +59,26 @@ function validateHost(host, name) {
6059
return host;
6160
}
6261

62+
let urlWarningEmitted = false;
6363
function ClientRequest(options, cb) {
6464
OutgoingMessage.call(this);
6565

6666
if (typeof options === 'string') {
67-
options = url.parse(options);
68-
if (!options.hostname) {
69-
throw new ERR_INVALID_DOMAIN_NAME();
67+
const urlStr = options;
68+
try {
69+
options = urlToOptions(new URL(urlStr));
70+
} catch (err) {
71+
options = url.parse(urlStr);
72+
if (!options.hostname) {
73+
throw err;
74+
}
75+
if (!urlWarningEmitted && !process.noDeprecation) {
76+
urlWarningEmitted = true;
77+
process.emitWarning(
78+
`The provided URL ${urlStr} is not a valid URL, and is supported ` +
79+
'in the http module solely for compatibility.',
80+
'DeprecationWarning', 'DEP0109');
81+
}
7082
}
7183
} else if (options && options[searchParamsSymbol] &&
7284
options[searchParamsSymbol][searchParamsSymbol]) {

‎lib/https.js‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,7 @@ const {
3434
const { ClientRequest } = require('_http_client');
3535
const { inherits } = util;
3636
const debug = util.debuglog('https');
37-
const { urlToOptions, searchParamsSymbol } = require('internal/url');
38-
const { ERR_INVALID_DOMAIN_NAME } = require('internal/errors').codes;
37+
const { URL, urlToOptions, searchParamsSymbol } = require('internal/url');
3938
const { IncomingMessage, ServerResponse } = require('http');
4039
const { kIncomingMessage } = require('_http_common');
4140
const { kServerResponse } = require('_http_server');
@@ -254,11 +253,24 @@ Agent.prototype._evictSession = function _evictSession(key) {
254253

255254
const globalAgent = new Agent();
256255

256+
let urlWarningEmitted = false;
257257
function request(options, cb) {
258258
if (typeof options === 'string') {
259-
options = url.parse(options);
260-
if (!options.hostname) {
261-
throw new ERR_INVALID_DOMAIN_NAME();
259+
const urlStr = options;
260+
try {
261+
options = urlToOptions(new URL(urlStr));
262+
} catch (err) {
263+
options = url.parse(urlStr);
264+
if (!options.hostname) {
265+
throw err;
266+
}
267+
if (!urlWarningEmitted && !process.noDeprecation) {
268+
urlWarningEmitted = true;
269+
process.emitWarning(
270+
`The provided URL ${urlStr} is not a valid URL, and is supported ` +
271+
'in the https module solely for compatibility.',
272+
'DeprecationWarning', 'DEP0109');
273+
}
262274
}
263275
} else if (options && options[searchParamsSymbol] &&
264276
options[searchParamsSymbol][searchParamsSymbol]) {

‎lib/internal/errors.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -881,7 +881,6 @@ E('ERR_INVALID_CALLBACK', 'Callback must be a function', TypeError);
881881
E('ERR_INVALID_CHAR', invalidChar, TypeError);
882882
E('ERR_INVALID_CURSOR_POS',
883883
'Cannot set cursor row without setting its column', TypeError);
884-
E('ERR_INVALID_DOMAIN_NAME', 'Unable to determine the domain name', TypeError);
885884
E('ERR_INVALID_FD',
886885
'"fd" must be a positive integer: %s', RangeError);
887886
E('ERR_INVALID_FD_TYPE', 'Unsupported fd type: %s', TypeError);

‎lib/tls.js‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ const url = require('url');
3232
const binding = process.binding('crypto');
3333
const { Buffer } = require('buffer');
3434
const EventEmitter = require('events');
35+
const { URL } = require('internal/url');
3536
const DuplexPair = require('internal/streams/duplexpair');
3637
const { canonicalizeIP } = process.binding('cares_wrap');
3738
const _tls_common = require('_tls_common');
@@ -169,6 +170,7 @@ function check(hostParts, pattern, wildcards) {
169170
return true;
170171
}
171172

173+
let urlWarningEmitted = false;
172174
exports.checkServerIdentity = function checkServerIdentity(host, cert) {
173175
const subject = cert.subject;
174176
const altNames = cert.subjectaltname;
@@ -183,7 +185,21 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {
183185
if (name.startsWith('DNS:')) {
184186
dnsNames.push(name.slice(4));
185187
} else if (name.startsWith('URI:')) {
186-
const uri = url.parse(name.slice(4));
188+
let uri;
189+
try {
190+
uri = new URL(name.slice(4));
191+
} catch (err) {
192+
uri = url.parse(name.slice(4));
193+
if (!urlWarningEmitted && !process.noDeprecation) {
194+
urlWarningEmitted = true;
195+
process.emitWarning(
196+
`The URI ${name.slice(4)} found in cert.subjectaltname ` +
197+
'is not a valid URI, and is supported in the tls module ' +
198+
'solely for compatibility.',
199+
'DeprecationWarning', 'DEP0109');
200+
}
201+
}
202+
187203
uriNames.push(uri.hostname); // TODO(bnoordhuis) Also use scheme.
188204
} else if (name.startsWith('IP Address:')) {
189205
ips.push(canonicalizeIP(name.slice(11)));

‎test/parallel/test-http-correct-hostname.js‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
/* eslint-disable node-core/crypto-check */
2+
// Flags: --expose-internals
3+
'use strict';
4+
5+
const common = require('../common');
6+
const assert = require('assert');
7+
8+
const { outHeadersKey } = require('internal/http');
9+
10+
const http = require('http');
11+
const modules = { http };
12+
13+
if (common.hasCrypto) {
14+
const https = require('https');
15+
modules.https = https;
16+
}
17+
18+
Object.keys(modules).forEach((module) => {
19+
const doNotCall = common.mustNotCall(
20+
`${module}.request should not connect to ${module}://example.com%60x.example.com`
21+
);
22+
const req = modules[module].request(`${module}://example.com%60x.example.com`, doNotCall);
23+
assert.deepStrictEqual(req[outHeadersKey].host, [
24+
'Host',
25+
'example.com`x.example.com',
26+
]);
27+
req.abort();
28+
});

‎test/parallel/test-http-deprecated-urls.js‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/* eslint-disable node-core/crypto-check */
2+
3+
'use strict';
4+
5+
const common = require('../common');
6+
7+
const http = require('http');
8+
const modules = { http };
9+
10+
const deprecations = [
11+
['The provided URL http://[www.nodejs.org] is not a valid URL, and is supported ' +
12+
'in the http module solely for compatibility.',
13+
'DEP0109'],
14+
];
15+
16+
if (common.hasCrypto) {
17+
const https = require('https');
18+
modules.https = https;
19+
deprecations.push(
20+
['The provided URL https://[www.nodejs.org] is not a valid URL, and is supported ' +
21+
'in the https module solely for compatibility.',
22+
'DEP0109'],
23+
);
24+
}
25+
26+
common.expectWarning('DeprecationWarning', deprecations);
27+
28+
Object.keys(modules).forEach((module) => {
29+
const doNotCall = common.mustNotCall(
30+
`${module}.request should not connect to ${module}://[www.nodejs.org]`
31+
);
32+
modules[module].request(`${module}://[www.nodejs.org]`, doNotCall).abort();
33+
});

0 commit comments

Comments
 (0)