Skip to content

More URL checks#2

Open
hemanth wants to merge 1 commit intotj:masterfrom
hemanth:master
Open

More URL checks#2
hemanth wants to merge 1 commit intotj:masterfrom
hemanth:master

Conversation

@hemanth
Copy link
Copy Markdown

@hemanth hemanth commented Apr 23, 2016

My editor has formatted the code, I hope you are ok with it.

The actual change is this.

//cc @tj

@radiovisual
Copy link
Copy Markdown
Contributor

radiovisual commented Jul 4, 2016

Unless TJ is wanting to keep this module dependency-free, may I suggest that you use the module url-regex for checking url patterns? The RegExp you supplied is pretty good, and catches all the most typical patterns, but it fails on protocol-less urls, such as 'www.google.com/foo':

/^(?:\w+:)?\/\/([^\s\.]+\.\S{2}|localhost[\:?\d]*)\S*$/.test('www.google.com/foo');
//=> false

But more importantly, your supplied RegExp lets some invalid url patterns through. Consider the following code:

var urlRegExp = /^(?:\w+:)?\/\/([^\s\.]+\.\S{2}|localhost[\:?\d]*)\S*$/;

var invalidUrls = [
     'http://-error-.invalid/',
     'http://-a.b.co',
     'http://a.b-.co',
     'http://123.123.123',
     'http://go/ogle.com',
     'http://google\.com',
     'http://www(google.com',
     'http://www.example.xn--overly-long-punycode-test-string-test-tests-123-test-test123/',
     'http://www=google.com',
     '///www.foo.bar./'
];

invalidUrls.forEach(function(url) {
    // these should all be `false`
    var isUrl = urlRegExp.test(url);
    console.log(isUrl);
});

Which results in the following output (these should all read false):

true
true
true
true
true
true
true
true
true
true

The url-regex module is pretty good at keeping up-to-date, and is pretty complete by most standards.

Full disclosure: To arrive at this conclusion, I ran your UrlRegExp against all the urls in the url-regex test file for comparison purposes, this is how I was able to catch some of the invalid patterns, and see that it was failing on protocol-less urls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants