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.

Sign up for GitHub

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

Fixed isURL regex to take account file urls. #20435

Merged
merged 4 commits into from
Feb 28, 2020
Merged

Fixed isURL regex to take account file urls. #20435

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40f5560
Fixed isURL regex to take account file urls.
SergioEstevao Feb 25, 2020
024149c
Change regex to support url with the format file:/
SergioEstevao Feb 27, 2020
7ddfe49
Check for start and end of line.
SergioEstevao Feb 28, 2020
f9be985
Simplify test now that we test for start and end of line in regex.
SergioEstevao Feb 28, 2020
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
2 changes: 1 addition & 1 deletion packages/url/src/is-url.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const URL_REGEXP = /((([A-Za-z]{3,9}:(?:\/\/)?)(?:[\-;:&=\+\$,\w]+@)?[A-Za-z0-9\.\-]+|(?:www\.|[\-;:&=\+\$,\w]+@)[A-Za-z0-9\.\-]+)(?::\d{2,5})?((?:\/[\+~%\/\.\w\-_]*)?\??(?:[\-\+=&;%@\.\w_]*)#?(?:[\.\!\/\\\w]*))?)/i;
const URL_REGEXP = /((([A-Za-z]{3,9}:(?:\/\/?\/)?)(?:[\-;:&=\+\$,\w]+@)?[A-Za-z0-9\.\-]+|(?:www\.|[\-;:&=\+\$,\w]+@)[A-Za-z0-9\.\-]+)(?::\d{2,5})?((?:\/[\+~%\/\.\w\-_]*)?\??(?:[\-\+=&;%@\.\w_]*)#?(?:[\.\!\/\\\w]*))?)/i;
Copy link
Member

Choose a reason for hiding this comment

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

In wondering whether the proposed revisions are in-fact technically valid, it eventually clicked with me that the extra slash is meant to represent the path name (e.g. file: is the protocol, then //, then /foo constituting a root-relative path). So in looking at how this is implemented, I wonder if this is something where we should be accounting for this as part of the pattern matching for the path, and not strictly as an optional extra slash following the //. The pattern is a bit unwieldy to untangle, so it's not immediately clear to me where this change would need to occur.

It's also a bit awkward that it's the second of three slashes which is made to be optional. Though I expect it works just the same.

Suggested change
const URL_REGEXP = /((([A-Za-z]{3,9}:(?:\/\/?\/)?)(?:[\-;:&=\+\$,\w]+@)?[A-Za-z0-9\.\-]+|(?:www\.|[\-;:&=\+\$,\w]+@)[A-Za-z0-9\.\-]+)(?::\d{2,5})?((?:\/[\+~%\/\.\w\-_]*)?\??(?:[\-\+=&;%@\.\w_]*)#?(?:[\.\!\/\\\w]*))?)/i;
const URL_REGEXP = /((([A-Za-z]{3,9}:(?:\/\/\/?)?)(?:[\-;:&=\+\$,\w]+@)?[A-Za-z0-9\.\-]+|(?:www\.|[\-;:&=\+\$,\w]+@)[A-Za-z0-9\.\-]+)(?::\d{2,5})?((?:\/[\+~%\/\.\w\-_]*)?\??(?:[\-\+=&;%@\.\w_]*)#?(?:[\.\!\/\\\w]*))?)/i;

Noting that the // is technically considered optional based on this pattern, I wonder if the following should be considered valid, as omitting the optional //, but still root-relative:

file:/foo

In the browser, this is considered valid, and evaluates the same as file:///foo:

new URL( 'file:/ok' ).pathname
// "/ok"

Under this proposed implementation, I expect that would not be supported.

Copy link
Member

Choose a reason for hiding this comment

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

Possibly related contrary information for the above-given example:

  • If url’s scheme is "file", then:
    • If remaining does not start with "//", validation error.

https://url.spec.whatwg.org/#scheme-state

Copy link
Member

@aduth aduth Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Noting that the // is technically considered optional based on this pattern, I wonder if the following should be considered valid, as omitting the optional //, but still root-relative:

At least based on relevant specification, it is optional:

If c is U+002F (/), then set state to authority state.

Otherwise, set state to path state, and decrease pointer by one.

https://url.spec.whatwg.org/#path-or-authority-state

The diagram here is a little easier to interpret, though technically not the specification we adhere to:

https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Generic_syntax

If the extra slash we're accounting for is relevant for the path, then the pattern should match it separately from the optional authority //.

Edit: Oof, this gets really confusing, when you consider that http: is a "special scheme", and thus it would be subject to special authority slashes state, and omitting // would be deemed invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth I did some changes to support the case you mention, have a look to see if you agree.

Copy link
Member

Choose a reason for hiding this comment

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

@SergioEstevao I'm not seeing any changes. Did you push them? GitHub was having an outage incident this morning, so possible that it was missed?

Copy link
Member

Choose a reason for hiding this comment

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

My previous comments were admittedly a bit of a winding brain dump. In short, it's not clear to me if we should consider file:/foo as "valid". It's tolerated by the URL constructor and thus would return true in the browser, so there's an argument in favor of consistency. But as I note in #20435 (comment), the specification is fairly clear that a double-slash should be required to be considered valid.

Copy link
Member

Choose a reason for hiding this comment

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

Did you come up with this regular expression or find it somewhere? I was trying to decipher it yesterday, and it felt to me like there's some redundancies around www., and user/pass matching. It made it hard for me to figure out how best to change it, but ultimately it felt like the problem is in-part because there's an expectation that one or more characters exist before the pathname can start (which is what the changes here are proposing is an incorrect assumption).

Notably, the occurrences of: [A-Za-z0-9\.\-]+

Copy link
Contributor Author

@SergioEstevao SergioEstevao Feb 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I've updated the expression that @etoledom used. I think he used the one for Javascript defined here: https://urlregex.com/

I'm tempted to replace it by the one defined for HTML 5:
^(([^:/?#]+):)?(//([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm tempted to replace it by the one defined for HTML 5:
^(([^:/?#]+):)?(//([^/?#]))?([^?#])(?([^#]))?(#(.))?

I've tried this and it's no good, I think the current version of the commit is the best one that matches all the situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth ^


/**
* Determines whether the given string looks like a URL.
Expand Down
2 changes: 2 additions & 0 deletions packages/url/src/test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,10 @@ describe( 'isURL', () => {
[ 'https://wordpress.org/./foo' ],
[ 'https://wordpress.org/path?query#fragment' ],
[ 'https://localhost/foo#bar' ],
[ 'https:///localhost/foo#bar' ],
[ 'mailto:[email protected]' ],
[ 'ssh://user:[email protected]:8080' ],
[ 'file:///localfolder/file.mov' ],
] )( 'valid (true): %s', ( url ) => {
expect( isURL( url ) ).toBe( true );
} );
Expand Down