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

Failed to validate a valid url #1681

Closed
getlarge opened this issue May 31, 2021 · 9 comments
Closed

Failed to validate a valid url #1681

getlarge opened this issue May 31, 2021 · 9 comments

Comments

@getlarge
Copy link

getlarge commented May 31, 2021

Describe the bug
isUrl is failing to validate urls using that format ${protocol}://:${password}@${hostname}:${port}

Surprisingly with version 13.5.2 i don't encounter this issue, but it seems to happen with version 13.6.0, i noticed this after digging into class-validator dependencies in package-lock.json

The failure is triggered by the second condition here. Any idea what is the purpose of that condition, what is it suppose to avoid ?

Examples
this type of url shape redis://:p4ssw0rd@server.com:6379 will always return false even though it is a valid format.

@fasenderos
Copy link
Contributor

Any idea what is the purpose of that condition, what is it suppose to avoid

The colon in the username isn't RFC 1738 compliant
05ceb18#diff-1ced9bbbf98fe2779af1d618687cec44dd27ac19c72c7cc6d85da8b67978ce90

@getlarge
Copy link
Author

getlarge commented Jun 1, 2021

This assumption based on the RFC is not really obvious to me, like said in this section, :
"...URL:ftp://foo:@host.com/ has a user name of "foo" and an empty password."
And i did not see that the case where we have a password and an empty username like in the example of this issue is excluded.

Moreover, in Node URL API, this kind of URL (${protocol}://:${password}@${hostname}:${port}) can be parsed and allows to extract the password.

@Jassi10000-zz
Copy link

@getlarge Can you help me on this one
I would like to work on this issue

@getlarge
Copy link
Author

@Jassi10000 Sure, how could i help you ?

@Jassi10000-zz
Copy link

@getlarge Thanks
I wanted to know more about the issue , what it actually demands to be corrected , being a first - timer founding it difficult to understand

@getlarge
Copy link
Author

@Jassi10000 In that block, the authentication part of the URL is checked.
In line 114 there is (i guess) the condition (split[0].substr(0, 1) === ':') that detects the URL pattern ${protocol}://:${password}@${hostname}:${port} as invalid but that is also useful to avoid URLs such as http://:@foobar.com to be considered valid.
The fix would imply to make a more accurate check of split[0].

To check if the fix is working, you can update the list of valid URLs here and try to run the tests.

I hope this will help you.

@MiguelSavignano
Copy link
Contributor

MiguelSavignano commented Oct 27, 2021

Hello @getlarge @Jassi10000, sorry I was working on this issue last week but I didn't have time to make the PR.

I tried adding a new condition and avoid the split[0].substr(0, 1) === ':' condition, all test are passed in this the #1833

With this only URLs without user and password are invalid.

  const user_password = split[0].split(':');
  if (user_password[0] === '' && user_password[1] === '') {
    return false;
  }

@Jassi10000-zz
Copy link

No worries @MiguelSavignano
You can carry on with PR
All the best

@tux-tn
Copy link
Member

tux-tn commented Oct 30, 2021

Thank you @MiguelSavignano, @Jassi10000 and @getlarge for your efforts 🙏
Closing the issue since the PR has been merged!

@tux-tn tux-tn closed this as completed Oct 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants