Skip to content

Conversation

@jbuchmann-coosto
Copy link
Contributor

This PR addresses issue #1643

The isURL() function doesn't accept URLs that have a userinfo subcomponent without a colon. For example, all of these examples should be accpeted:

  1. http://user@example.com
  2. http://user:@example.com
  3. http://user:pass@example.com

However, currently, 1) is not accepted.

This PR fixes the isURL() function and adds several tests to define and validate the behavior.

In addition to the code and tests, I added some missing options to the isURL() section of the README (my editor also removed some trailing whitespace from a few other lines).

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

The 'userinfo' part of a URL may, according to RFC 1738, contain only
a username followed by an '@' sign. The previous behavior of the
isURL() function would return false if the userinfo section did not
have a colon.

In addition to the change in the function, tests have been added to
ensure the following exmaples are considered valid:

 - http://user@example.com
 - http://user:@example.com
 - http://user:pass@example.com

The following are considered not valid:

 - http://@example.com
 - http://:@example.com
 - http://:example.com

As a practical example, Sentry (https://github.com/getsentry/sentry)
uses a format like http://9b9cd2ef993c1fd9c14cbb88466@example.com/10
for it's DSNs (which are just URLs).
@codecov
Copy link

codecov bot commented Apr 12, 2021

Codecov Report

Merging #1644 (983cc43) into master (63b6162) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1644   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         1843      1845    +2     
=========================================
+ Hits          1843      1845    +2     
Impacted Files Coverage Δ
src/lib/isURL.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63b6162...983cc43. Read the comment docs.

Copy link
Member

@tux-tn tux-tn left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 !
Nice catch and thank you for adding the missing options!

@tux-tn tux-tn added ready-to-land For PRs that are reviewed and ready to be landed 🎉 first-pr labels Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🎉 first-pr ready-to-land For PRs that are reviewed and ready to be landed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants