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
Open
11 changes: 10 additions & 1 deletion lib/types/string.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const Uri = require('@sideway/address/lib/uri');

const Any = require('./any');
const Common = require('../common');
const { throws } = require('assert')
trizotti marked this conversation as resolved.
Show resolved Hide resolved


const internals = {
Expand Down Expand Up @@ -639,7 +640,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.

}

const { regex, scheme } = Uri.regex(options);
Expand All @@ -662,6 +663,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.

const { hostname } = options.domain;
if(hostname !== matched){
return helpers.error('string.uri.hostname', { value: matched });
}
}

return value;
}

Expand Down Expand Up @@ -721,6 +729,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.

'string.uriCustomScheme': '{{#label}} must be a valid uri with a scheme matching the {{#scheme}} pattern',
'string.uriRelativeOnly': '{{#label}} must be a valid relative uri',
'string.uppercase': '{{#label}} must only contain uppercase characters'
Expand Down
10 changes: 10 additions & 0 deletions test/types/string.js
Original file line number Diff line number Diff line change
Expand Up @@ -8582,6 +8582,16 @@ describe('string', () => {

expect(() => Joi.string().uri({ foo: 'bar', baz: 'qux' })).to.throw('Options contain unknown keys: foo,baz');
});

it('validates if uri is from a given hostname', () => {
const schema = Joi.string().uri({ scheme: 'https', domain: { hostname: 'example.com'}});
Helper.validate(schema, [
['https://example.com', true],
['https://example.com/test', true],
['https://test.com', false, '"value" is not from given hostname'],
['https://test.com/example', false, '"value" is not from given hostname']
]);
});
});

describe('valid()', () => {
Expand Down