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

Validate hostname in uri #2846

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

Conversation

trizotti
Copy link
Contributor

#2690
Adds validation for given hostname.

lib/types/string.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@Marsup Marsup left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!
I think you're on the right path, documentation might need a bit of work here since it differentiate from string.domain now.

I feel like this API would benefit greatly from supporting an array of hostnames as well, or even regexps, or a mix of both.

@@ -662,6 +662,13 @@ module.exports = Any.extend({
return helpers.error('string.domain', { value: matched });
}

if(options?.domain?.hostname){
Copy link
Collaborator

Choose a reason for hiding this comment

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

domain is already checked a few lines above, you can probably refactor that if to handle your case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you talking about the domain variable few lines above (line 659)?
I think it's not the same as options.domain...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically, it kind of is. Look at the $_addRule just above.

options (the 3rd argument of validate) is basically the raw argument you provided.

domain (the 4th argument) is the post-processed version of that raw argument through the call to addressOptions, which I think should be augmented to validate that hostname satisfies expectations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added info to the docs and also support for regex and arrays.

@@ -639,7 +639,7 @@ module.exports = Any.extend({
Common.assertOptions(options, ['allowRelative', 'allowQuerySquareBrackets', 'domain', 'relativeOnly', 'scheme']);

if (options.domain) {
Common.assertOptions(options.domain, ['allowFullyQualified', 'allowUnicode', 'maxDomainSegments', 'minDomainSegments', 'tlds']);
Common.assertOptions(options.domain, ['allowFullyQualified', 'allowUnicode', 'maxDomainSegments', 'minDomainSegments', 'tlds', 'hostname']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is going to be part of domain, you should probably exclude it from the call to isValid to avoid any possible side-effect from an evolution of that module.

@@ -721,6 +728,7 @@ module.exports = Any.extend({
'string.pattern.invert.name': '{{#label}} with value {:[.]} matches the inverted {{#name}} pattern',
'string.trim': '{{#label}} must not have leading or trailing whitespace',
'string.uri': '{{#label}} must be a valid uri',
'string.uri.hostname': '{{#label}} is not from given hostname',
Copy link
Collaborator

Choose a reason for hiding this comment

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

The error might be more useful if it detailed what it expected, like so:

Suggested change
'string.uri.hostname': '{{#label}} is not from given hostname',
'string.uri.hostname': '{{#label}} does not match {{#expected}} hostname'

Copy link
Contributor Author

@trizotti trizotti Oct 13, 2022

Choose a reason for hiding this comment

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

I made some changes to make the error more useful, following the example.

@Marsup Marsup self-assigned this Sep 29, 2022
@Marsup Marsup added the feature New functionality or improvement label Oct 11, 2022
@trizotti trizotti marked this pull request as draft October 11, 2022 13:23
if (domain) {

if ((!options.allowRelative || matched) &&
!Domain.isValid(matched, domain)) {
Copy link
Contributor Author

@trizotti trizotti Oct 24, 2022

Choose a reason for hiding this comment

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

This call to isValid should be avoided? @Marsup

@trizotti trizotti marked this pull request as ready for review October 24, 2022 13:59
@trizotti trizotti requested a review from Marsup October 24, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants