search

validatorjs / validator.js

String validation
MIT License
23.17k stars 2.32k forks source link

isURL false positive. #1691

Open charlesomer opened 3 years ago

charlesomer commented 3 years ago

Describe the bug console.log(validator.isURL('FAQ.md', {require_valid_protocol: true})) Returns true when I believe it should be false?

Additional context Validator.js version: 13.6.0 Node.js version: v16.4.1 OS platform: macOS

profnandaa commented 3 years ago

PR welcome.

anirudh-modi commented 3 years ago

@profnandaa I am picking this issue up.. but before that I need some clarification...

  1. Why is this a bug?

    • Is it because the url is having FAQ.md where .md is perhaps invalid
    • or is it because require_valid_protocol:true was passed and the protocol is missing?
      • If this is a bug because the protocol is missing, then I believe the user must be sending require_protocol: true param.

  2. By looking at the doc, it looks like the purpose of require_valid_protocol is not to validate whether a protocol is present or not, but to validate that if a protocol is present it must be in list of valid protocols.

    • And if we are intending to change that behaviour, it might introduce a breaking a change, as our default config for isUrl is with require_valid_protocol: true which will break the existing behaviour
    • and sending require_valid_protocol will imply that the protocol must always be present.
Prajwalrajbasnet commented 3 years ago

As, described by @anirudh-modi above require_valid_protocol is not to validate to verify the presence of a protocol. Which however can be achieved instead by using require_protocol option. By using both the options at once the expected behavior can be achieved:

console.log(
    validator.isURL('FAQ.md', {
        require_protocol: true,
        require_valid_protocol: true,
    })
);
// returns false

If the bug is because of some different reason, please mention more in here. I would like to work on this if that's the case. Else I think we can close this issue.

charrismatic commented 3 years ago

I'm seeing the behavior for isURL changed recently. I have a unit test that expects the following to return false,

const urlOptions = {
  protocols: [ 'http', 'https' ],
  require_tld: false,
  require_protocol: false,
  require_host: true,
  require_valid_protocol: true,
  allow_underscores: true,
  host_whitelist: false,
  host_blacklist: false,
  allow_trailing_dot: false,
  allow_protocol_relative_urls: false,
  disallow_auth: false
}
> validator.isURL('example@domain.com', urlOptions)
true

This was previously returning false

charrismatic commented 3 years ago

I downgraded the package from 13.6.0 to 13.5.2 and it works as expected