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

url: disallow percent in hosts for url.parse() #45219

Closed
wants to merge 2 commits into from
Closed
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
11 changes: 1 addition & 10 deletions lib/url.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ const {
decodeURIComponent,
} = primordials;

const { toASCII } = require('internal/idna');
const { encodeStr, hexTable } = require('internal/querystring');
const querystring = require('querystring');

Expand Down Expand Up @@ -130,7 +129,6 @@ const {
CHAR_QUESTION_MARK,
CHAR_DOUBLE_QUOTE,
CHAR_SINGLE_QUOTE,
CHAR_PERCENT,
CHAR_SEMICOLON,
CHAR_BACKWARD_SLASH,
CHAR_CIRCUMFLEX_ACCENT,
Expand Down Expand Up @@ -325,7 +323,6 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
break;
case CHAR_SPACE:
case CHAR_DOUBLE_QUOTE:
case CHAR_PERCENT:
case CHAR_SINGLE_QUOTE:
case CHAR_SEMICOLON:
case CHAR_LEFT_ANGLE_BRACKET:
Expand Down Expand Up @@ -407,20 +404,14 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
// It only converts parts of the domain name that
// have non-ASCII characters, i.e. it doesn't matter if
// you call it with a domain that already is ASCII-only.

// Use lenient mode (`true`) to try to support even non-compliant
// URLs.
this.hostname = toASCII(this.hostname, true);
this.hostname = domainToASCII(this.hostname);

// Prevent two potential routes of hostname spoofing.
// 1. If this.hostname is empty, it must have become empty due to toASCII
// since we checked this.hostname above.
// 2. If any of forbiddenHostChars appears in this.hostname, it must have
// also gotten in due to toASCII. This is since getHostname would have
// filtered them out otherwise.
// Rather than trying to correct this by moving the non-host part into
// the pathname as we've done in getHostname, throw an exception to
// convey the severity of this issue.
if (this.hostname === '' || forbiddenHostChars.test(this.hostname)) {
throw new ERR_INVALID_URL(url);
}
Expand Down
1 change: 0 additions & 1 deletion test/parallel/test-bootstrap-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ const expectedModules = new Set([
'NativeModule internal/fs/watchers',
'NativeModule internal/heap_utils',
'NativeModule internal/histogram',
'NativeModule internal/idna',
Trott marked this conversation as resolved.
Show resolved Hide resolved
'NativeModule internal/linkedlist',
'NativeModule internal/mime',
'NativeModule internal/modules/cjs/helpers',
Expand Down
15 changes: 15 additions & 0 deletions test/parallel/test-url-parse-format.js
Original file line number Diff line number Diff line change
Expand Up @@ -991,6 +991,21 @@ const parseTests = {
path: '/',
href: 'https://evil.com$.example.com/'
},

'https://%E4%BD%A0/fhqwhgads': {
protocol: 'https:',
slashes: true,
auth: null,
host: 'xn--6qq',
port: null,
hostname: 'xn--6qq',
hash: null,
search: null,
query: null,
pathname: '/fhqwhgads',
path: '/fhqwhgads',
href: 'https://xn--6qq/fhqwhgads'
}
};

for (const u in parseTests) {
Expand Down
1 change: 1 addition & 0 deletions test/parallel/test-url-parse-invalid-input.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ if (common.hasIntl) {
const badURLs = [
'https://evil.com:.example.com',
'git+ssh://git@github.com:npm/npm',
'http://a%b.com/fhqwhgads',
];
badURLs.forEach((badURL) => {
assert.throws(() => { url.parse(badURL); },
Expand Down