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

FullUrl regex needs improvement #138

Open
junaid-ali opened this issue Jun 6, 2022 · 5 comments
Open

FullUrl regex needs improvement #138

junaid-ali opened this issue Jun 6, 2022 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@junaid-ali
Copy link
Contributor

junaid-ali commented Jun 6, 2022

Describe the bug

Currently, fullUrl returns invalid URLs as valid ones. Same is the case with Url. We should improve the regex like here:
https://github.com/asaskevich/govalidator/blob/master/patterns.go#L41

Examples:

https://www.googl_?e.com/testme
https://www
https://not%23

To Reproduce

Add the above invalid URLs to TestIsFullURL function, the tests will pass.

@junaid-ali
Copy link
Contributor Author

You can assign the issue to me, I can create a PR.

@junaid-ali
Copy link
Contributor Author

There's an open PR in the linked repo to further improve that regex: asaskevich/govalidator#451

@inhere inhere assigned junaid-ali and unassigned inhere Jun 7, 2022
@inhere inhere added the enhancement New feature or request label Jun 7, 2022
@inhere
Copy link
Member

inhere commented Jun 20, 2022

hi @junaid-ali 😄 Can you create a PR?

@junaid-ali
Copy link
Contributor Author

@inhere I'm still working on it. The regex in the other repo has some issues as well. What I'm trying to do is: instead of using a complex regex, I'm thinking of updating the method like this

// IsFullURL string.
func IsFullURL(s string) bool {
	if s == "" {
		return false
	}

	u, err := url.Parse(s)
	if err != nil {
		return false
	}

	if u.Scheme != "" && !strings.Contains(URLSchema, u.Scheme) {
		return false
	}

	if ip := net.ParseIP(u.Host); ip == nil {
		return rxDNSName.MatchString(u.Host)
	}

	return false
}

This also has an issue if the URL has a raw percentage sign instead of an HTML encode sign but that should be easy to fix.
I will put up a PR today.

@junaid-ali
Copy link
Contributor Author

sorry still working on it.

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

No branches or pull requests

2 participants