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

[security] More backslash fixes #197

Merged
merged 1 commit into from
Feb 17, 2021
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
11 changes: 10 additions & 1 deletion SECURITY.md
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,22 @@ acknowledge your responsible disclosure, if you wish.

## History

> Using backslash in the protocol is valid in the browser, while url-parse
> thinks it’s a relative path. An application that validates a url using
> url-parse might pass a malicious link.
- **Reporter credits**
- CxSCA AppSec team at Checkmarx.
- Twitter: [Yaniv Nizry](https://twitter.com/ynizry)
- Fixed in: 1.5.0

> The `extractProtocol` method does not return the correct protocol when
> provided with unsanitized content which could lead to false positives.
- **Reporter credits**
- Reported through our security email & Twitter interaction.
- Twitter: [@ronperris](https://twitter.com/ronperris)
- Fixed in: 1.4.5
- Fixed in: 1.4.5

---

Expand Down
21 changes: 16 additions & 5 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@

var required = require('requires-port')
, qs = require('querystringify')
, slashes = /^[A-Za-z][A-Za-z0-9+-.]*:\/\//
, protocolre = /^([a-z][a-z0-9.+-]*:)?(\/\/)?([\S\s]*)/i
, slashes = /^[A-Za-z][A-Za-z0-9+-.]*:[\\/]+/
, protocolre = /^([a-z][a-z0-9.+-]*:)?([\\/]{1,})?([\S\s]*)/i
, whitespace = '[\\x09\\x0A\\x0B\\x0C\\x0D\\x20\\xA0\\u1680\\u180E\\u2000\\u2001\\u2002\\u2003\\u2004\\u2005\\u2006\\u2007\\u2008\\u2009\\u200A\\u202F\\u205F\\u3000\\u2028\\u2029\\uFEFF]'
, left = new RegExp('^'+ whitespace +'+');

Expand Down Expand Up @@ -115,11 +115,14 @@ function lolcation(loc) {
*/
function extractProtocol(address) {
address = trimLeft(address);
var match = protocolre.exec(address);

var match = protocolre.exec(address)
, protocol = match[1] ? match[1].toLowerCase() : ''
, slashes = !!(match[2] && match[2].length >= 2);

return {
protocol: match[1] ? match[1].toLowerCase() : '',
slashes: !!match[2],
protocol: protocol,
slashes: slashes,
rest: match[3]
};
}
Expand Down Expand Up @@ -280,6 +283,14 @@ function Url(address, location, parser) {
url.pathname = resolve(url.pathname, location.pathname);
}

//
// Default to a / for pathname if none exists. This normalizes the URL
// to always have a /
//
if (url.pathname.charAt(0) !== '/' && url.hostname) {
url.pathname = '/' + url.pathname;

Choose a reason for hiding this comment

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

This cause break change, see below demo code:

// assume this code exec in page http://cone-cf8b5c0e.app-dev.alipay.net/cone/strategy
const { pathname} = url('/cone/operate');

This PR cause pathname change from 'cone/operate' to '/cone/cone/operate'

debugger snapshot:
input:
image

output:
image

i don't know of pass '/cone/operate' is a valid argument, if no, i think throw error is a better way

Copy link
Member Author

@3rd-Eden 3rd-Eden Feb 18, 2021

Choose a reason for hiding this comment

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

Could you create an issue about this so we can track it? We do known issue with relative paths atm see #200 so it might be related to this bug.

}

//
// We should not add port numbers if they are already the default port number
// for a given protocol. As the host also contains the port number we're going
Expand Down
2 changes: 2 additions & 0 deletions test/fuzzy.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,8 @@ module.exports = function generate() {
, key;

spec.protocol = get('protocol');
spec.slashes = true;

spec.hostname = get('hostname');
spec.pathname = get('pathname');

Expand Down
77 changes: 57 additions & 20 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,10 @@ describe('url-parse', function () {
, parsed = parse(url);

assume(parsed.port).equals('');
assume(parsed.pathname).equals('/');
assume(parsed.host).equals('example.com');
assume(parsed.hostname).equals('example.com');
assume(parsed.href).equals('http://example.com');
assume(parsed.href).equals('http://example.com/');
});

it('understands an / as pathname', function () {
Expand Down Expand Up @@ -242,16 +243,30 @@ describe('url-parse', function () {
assume(parsed.hostname).equals('google.com');
assume(parsed.hash).equals('#what\\is going on');

parsed = parse('//\\what-is-up.com');
parsed = parse('http://yolo.com\\what-is-up.com');
assume(parsed.pathname).equals('/what-is-up.com');
});

it('correctly ignores multiple slashes //', function () {
var url = '////what-is-up.com'
, parsed = parse(url);

assume(parsed.host).equals('');
assume(parsed.hostname).equals('');
assume(parsed.host).equals('what-is-up.com');
assume(parsed.href).equals('//what-is-up.com/');
});

it('does not see a slash after the protocol as path', function () {
var url = 'https:\\/github.com/foo/bar'
, parsed = parse(url);

assume(parsed.host).equals('github.com');
assume(parsed.hostname).equals('github.com');
assume(parsed.pathname).equals('/foo/bar');

url = 'https:/\/\/\github.com/foo/bar';
Copy link
Member

Choose a reason for hiding this comment

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

@3rd-Eden did you mean /\\/\\/\\ here? 3 characters are currently unnecessarily escaped.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just testing that literally any slash (forward/backward) or combination of both is allowed.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then it should be url = 'https:/\\/\\/\\github.com/foo/bar'.

assume(parsed.host).equals('github.com');
assume(parsed.hostname).equals('github.com');
assume(parsed.pathname).equals('/foo/bar');
});

describe('origin', function () {
Expand Down Expand Up @@ -327,32 +342,52 @@ describe('url-parse', function () {
it('extracts the right protocol from a url', function () {
var testData = [
{
href: 'http://example.com',
href: 'http://example.com/',
protocol: 'http:',
pathname: ''
pathname: '/',
slashes: true
},
{
href: 'ws://example.com/',
protocol: 'ws:',
pathname: '/',
slashes: true
},
{
href: 'wss://example.com/',
protocol: 'wss:',
pathname: '/',
slashes: true
},
{
href: 'mailto:test@example.com',
pathname: 'test@example.com',
protocol: 'mailto:'
protocol: 'mailto:',
slashes: false
},
{
href: 'data:text/html,%3Ch1%3EHello%2C%20World!%3C%2Fh1%3E',
pathname: 'text/html,%3Ch1%3EHello%2C%20World!%3C%2Fh1%3E',
protocol: 'data:'
protocol: 'data:',
slashes: false,
},
{
href: 'sip:alice@atlanta.com',
pathname: 'alice@atlanta.com',
protocol: 'sip:'
protocol: 'sip:',
slashes: false,
}
];

var data;
var data, test;
for (var i = 0, len = testData.length; i < len; ++i) {
data = parse(testData[i].href);
assume(data.protocol).equals(testData[i].protocol);
assume(data.pathname).equals(testData[i].pathname);
test = testData[i];
data = parse(test.href);

assume(data.protocol).equals(test.protocol);
assume(data.pathname).equals(test.pathname);
assume(data.slashes).equals(test.slashes);
assume(data.href).equals(test.href);
}
});

Expand Down Expand Up @@ -391,13 +426,14 @@ describe('url-parse', function () {
});

it('parses ipv6 with auth', function () {
var url = 'http://user:password@[3ffe:2a00:100:7031::1]:8080'
var url = 'http://user:password@[3ffe:2a00:100:7031::1]:8080/'
, parsed = parse(url);

assume(parsed.username).equals('user');
assume(parsed.password).equals('password');
assume(parsed.host).equals('[3ffe:2a00:100:7031::1]:8080');
assume(parsed.hostname).equals('[3ffe:2a00:100:7031::1]');
assume(parsed.pathname).equals('/');
assume(parsed.href).equals(url);
});

Expand Down Expand Up @@ -467,7 +503,7 @@ describe('url-parse', function () {

assume(data.port).equals('');
assume(data.host).equals('localhost');
assume(data.href).equals('http://localhost');
assume(data.href).equals('http://localhost/');
});

it('inherits port numbers for relative urls', function () {
Expand Down Expand Up @@ -516,7 +552,8 @@ describe('url-parse', function () {
});

it('inherits protocol for relative protocols', function () {
var data = parse('//foo.com/foo', parse('http://sub.example.com:808/'));
var lolcation = parse('http://sub.example.com:808/')
, data = parse('//foo.com/foo', lolcation);

assume(data.port).equals('');
assume(data.host).equals('foo.com');
Expand All @@ -529,13 +566,13 @@ describe('url-parse', function () {

assume(data.port).equals('');
assume(data.host).equals('localhost');
assume(data.href).equals('http://localhost');
assume(data.href).equals('http://localhost/');
});

it('resolves pathname for relative urls', function () {
var data, i = 0;
var tests = [
['', 'http://foo.com', ''],
['', 'http://foo.com', '/'],
['', 'http://foo.com/', '/'],
['', 'http://foo.com/a', '/a'],
['a', 'http://foo.com', '/a'],
Expand Down Expand Up @@ -722,12 +759,12 @@ describe('url-parse', function () {
data.set('hash', 'usage');

assume(data.hash).equals('#usage');
assume(data.href).equals('http://example.com#usage');
assume(data.href).equals('http://example.com/#usage');

data.set('hash', '#license');

assume(data.hash).equals('#license');
assume(data.href).equals('http://example.com#license');
assume(data.href).equals('http://example.com/#license');
});

it('updates the port when updating host', function () {
Expand Down