Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(input[url]): Invalid URLS incorrectly valid #7816

Closed
wants to merge 4 commits into from

Conversation

btesser
Copy link
Contributor

@btesser btesser commented Jun 13, 2014

The current URL regex incorrectly matches invalid urls.
Examples:
http://www..
http:///
http://[[
The new regex should correctly identify urls

btesser added 4 commits June 12, 2014 22:30
The current URL regex incorrectly matches invalid urls.
Examples:
http://www..
http:///
http://[[
The new regex should correctly identify urls
Switched regex to new RegExp format so that I could pass the JShint test for lines that are too long
Fix spacing so quotes line up (with the exception of 1 line where \ needs to be escaped
Fix typo when changing spacing
@btesser btesser added cla: yes and removed cla: no labels Jun 13, 2014
@btesser
Copy link
Contributor Author

btesser commented Jun 13, 2014

my fix is currently failing the test for:
http://server:123/asdfkj

Pretty much it's handling all cases except for domains without an extension. Any regex experts want to help?

@Narretz Narretz added this to the Backlog milestone Jul 2, 2014
@Narretz
Copy link
Contributor

Narretz commented Jul 7, 2014

Url validation is currently following Chromium and FF, which is following rfc1035
In the latest snapshot, your examples are still valid. So I don't know if these changes will make it in or if they are even in rfc 1035. Maybe @caitp knows more.

@caitp
Copy link
Contributor

caitp commented Jul 7, 2014

I'm not sure we want a regexp like that in the tree, because that is kind of frightening. As I've said in another issue, URL validation in browsers is a bit more complicated and relies on quite a bit more than a simple regexp, so it's hard for us to be as robust as them.

I think there are some things we could add which would be worth doing, like supporting any scheme, or scheme-less urls (such as CDN urls), but there's a limit to how robust we can be when we're relying on a regular expression.

Even so, maybe someone else has a different opinion, @shahata WDYT about this?

@btesser
Copy link
Contributor Author

btesser commented Jul 8, 2014

at the very least I don't think the regex should allow things like http://www.. and http:///

The way I came across the bug was doing url validation for a form. Those are 2 probable mistakes that could be entered that I feel should be caught by the regex.

@Narretz
Copy link
Contributor

Narretz commented Apr 9, 2016

There's been a change last year that made the url regex much less strict: e3be5d6 I haven't tested your examples, but I assume they are covered by this.

@Narretz Narretz closed this Apr 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants