mirror of
https://github.com/nodejs/node.git
synced 2026-11-09 15:11:11 -05:00
Timothy Gu
70157b9cb7
The legacy url.parse() function attempts to convert Unicode domains
(IDNs) into their ASCII/Punycode form through the use of the toASCII
function. However, toASCII can introduce or remove various characters
that at best invalidate the parsed URL, and at worst cause hostname
spoofing:
url.parse('http://bad.c℀.good.com/').href === 'http://bad.ca/c.good.com/'
(from [1])
url.parse('http://\u00AD/bad.com').href === 'http:///bad.com/'
While changes to the legacy URL parser are discouraged in general, the
security implications here outweigh the desire for strict compatibility.
This is since this commit only changes behavior when non-ASCII
characters appear in the hostname, an unusual situation for most use
cases. Additionally, despite the availability of the WHATWG URL API,
url.parse remain widely deployed in the Node.js ecosystem, as
exemplified by the recent un-deprecation of the legacy API.
This change is similar in spirit to CPython 3.8's change [2] fixing
bpo-36216 [3] aka CVE-2019-9636, which also occurred despite potential
compatibility concerns.
[1]: https://hackerone.com/reports/678487
[2]: https://github.com/python/cpython/commit/16e6f7dee7f02bb81aa6b385b982dcdda5b99286
[3]: https://bugs.python.org/issue36216
PR-URL: https://github.com/nodejs/node/pull/38631
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
73 lines
2.0 KiB
JavaScript
73 lines
2.0 KiB
JavaScript
'use strict';
|
||
const common = require('../common');
|
||
const assert = require('assert');
|
||
const url = require('url');
|
||
|
||
// https://github.com/joyent/node/issues/568
|
||
[
|
||
[undefined, 'undefined'],
|
||
[null, 'object'],
|
||
[true, 'boolean'],
|
||
[false, 'boolean'],
|
||
[0.0, 'number'],
|
||
[0, 'number'],
|
||
[[], 'object'],
|
||
[{}, 'object'],
|
||
[() => {}, 'function'],
|
||
[Symbol('foo'), 'symbol'],
|
||
].forEach(([val, type]) => {
|
||
assert.throws(() => {
|
||
url.parse(val);
|
||
}, {
|
||
code: 'ERR_INVALID_ARG_TYPE',
|
||
name: 'TypeError',
|
||
message: 'The "url" argument must be of type string.' +
|
||
common.invalidArgTypeHelper(val)
|
||
});
|
||
});
|
||
|
||
assert.throws(() => { url.parse('http://%E0%A4%A@fail'); },
|
||
(e) => {
|
||
// The error should be a URIError.
|
||
if (!(e instanceof URIError))
|
||
return false;
|
||
|
||
// The error should be from the JS engine and not from Node.js.
|
||
// JS engine errors do not have the `code` property.
|
||
return e.code === undefined;
|
||
});
|
||
|
||
if (common.hasIntl) {
|
||
// An array of Unicode code points whose Unicode NFKD contains a "bad
|
||
// character".
|
||
const badIDNA = (() => {
|
||
const BAD_CHARS = '#%/:?@[\\]^|';
|
||
const out = [];
|
||
for (let i = 0x80; i < 0x110000; i++) {
|
||
const cp = String.fromCodePoint(i);
|
||
for (const badChar of BAD_CHARS) {
|
||
if (cp.normalize('NFKD').includes(badChar)) {
|
||
out.push(cp);
|
||
}
|
||
}
|
||
}
|
||
return out;
|
||
})();
|
||
|
||
// The generation logic above should at a minimum produce these two
|
||
// characters.
|
||
assert(badIDNA.includes('℀'));
|
||
assert(badIDNA.includes('＠'));
|
||
|
||
for (const badCodePoint of badIDNA) {
|
||
const badURL = `http://fail${badCodePoint}fail.com/`;
|
||
assert.throws(() => { url.parse(badURL); },
|
||
(e) => e.code === 'ERR_INVALID_URL',
|
||
`parsing ${badURL}`);
|
||
}
|
||
|
||
assert.throws(() => { url.parse('http://\u00AD/bad.com/'); },
|
||
(e) => e.code === 'ERR_INVALID_URL',
|
||
'parsing http://\u00AD/bad.com/');
|
||
}
|