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

feat: Added better url validation that supports wider range of urls #1859

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vojtechportes
Copy link

@vojtechportes vojtechportes commented Dec 14, 2022

string().url() method is not validating urls correctly. Urls like http://localhost:3000 are not considered valid.

Changes in this branch are supporting wider range of urls as valid.

…s valid (for example localhost, ipv4, ipv6 based urls etc)
@vojtechportes vojtechportes changed the title feat: Added better url validation that supports wider range of urls a… feat: Added better url validation that supports wider range of urls Dec 14, 2022
@anli-xsigns
Copy link

👍

@pavlovalor
Copy link

Why is it still not merged? Good job.

src/util/getUrlRegex.ts Outdated Show resolved Hide resolved
@vojtechportes
Copy link
Author

vojtechportes commented Feb 12, 2023

@pavlovalor I guess the best way how to use this without merging this change, which might take a while, is to create custom validation method.

@@ -186,7 +183,7 @@ export default class StringSchema<
}

url(message = locale.url) {
return this.matches(rUrl, {
return this.matches(getUrlRegex(), {

Choose a reason for hiding this comment

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

Why would you create same RegExp every time? Isn't it better to have const rUrl = getUrlRegExp()?

expect(v.isValid('this is not a url')).resolves.toBe(false),
expect(v.isValid('128.0.0.1')).resolves.toBe(false),

Choose a reason for hiding this comment

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

Also, you've implemented support for IRI but there's no test case for that. I guess some tlds from this article might help or this one

'\\[(([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]).){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]).){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]))\\]';
const domain =
'(?:\\.(?:[a-z\\u00a1-\\uffff0-9]-*)*[a-z\\u00a1-\\uffff0-9]+)*';
const tld = '(?:\\.(?:[a-z\\u00a1-\\uffff]{2,}))\\.?';

Choose a reason for hiding this comment

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

this TLD regex does not allow becoded names such as .xn--90ae which stands for internationalized .бг for Bulgaria.

const port = '(?::\\d{2,5})?';
const path = '(?:[/?#][^\\s"]*)?';

const regex = `(?:${protocol}|www\\.)${auth}(?:localhost|${ipv4}|${ipv6}|${host}${domain}${tld})${port}${path}`;

Choose a reason for hiding this comment

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

this regex will pass with string hi, i'm obviously not a good string www.uwu.www and something other. Which was not the case for previous version.

Suggested change
const regex = `(?:${protocol}|www\\.)${auth}(?:localhost|${ipv4}|${ipv6}|${host}${domain}${tld})${port}${path}`;
const regex = `^(?:${protocol}|www\\.)${auth}(?:localhost|${ipv4}|${ipv6}|${host}${domain}${tld})${port}${path}$`;

@vojtechportes
Copy link
Author

@BANOnotIT I will go through your comments this weekend and will try to resolve them

@hymair
Copy link

hymair commented Jun 29, 2023

Any updates? I just stumbled upon this when I got a ValidationError for a valid localhost url.

@harveyconnor
Copy link

any updates on this?
LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants