Joi: Painful defaults

[Marsup]

Hi people!

I'm trying to create a list of the painful defaults that you would like to see disappear in the next breaking release, so please help me if you see something I don't.

So far I have:

• [ ] empty strings not being valid
• [x] ~stripUnknown stripping arrays (does anyone even use that ?)~ Done in v14.
• [x] most people expect minDomainAtoms: 2 in string().email() apparently, should it change?

Anything else ?

[sholladay]

empty strings not being valid

IMO this is good default behavior, but it's made confusing by the fact that .required() is _not_ the default. As a result, the current behavior feels inconsistent...

joi.attempt(undefined, joi.string());  // no error
joi.attempt('', joi.string());         // ValidationError: "value" is not allowed to be empty

My preference would be this:

joi.attempt(undefined, joi.string());  ValidationError: "value" is required
joi.attempt('', joi.string());         // ValidationError: "value" is not allowed to be empty
joi.attempt('', joi.string().min(0));      // no error
joi.attempt(undefined, joi.string().min(0));  ValidationError: "value" is required

[devinivy]

Hmmm, not sure how I feel about required() being the default—would you want it that way _only_ for the string type? If we go that route, I would at least request that it be consistent across all types.

The way I often handle this behavior (naturally, depends on the situation!) is with empty('').

[Marsup]

When is it a good default apart from query strings ? (which can easily be solved by Joi.string().min(1))

[sholladay]

To be clear, I would want required on all types. That would be very strange if it were only for strings.

As for .empty(''), I find it almost laughable that I have to tell joi that '' is empty. IMO, it should know that already and on my side, I should specify if I want to allow empty things, or allow specific empty things. But I shouldn't have to specify _what_ is empty, at least for such a general case as this.

[WesTyler]

To be fair, .empty() also allows you to tell Joi to stamp in defaults if
they are provided. It's a "don't worry, this value isn't invalid, go ahead
and add my default" flag.

On Fri, Aug 3, 2018 at 2:58 PM Seth Holladay [e-mail]
wrote:

To be clear, I would want required on all types. That would be very
strange if it were only for strings.

As for .empty(''), I find it almost laughable that I have to tell joi
that '' is empty. IMO, it should know that already and on my side, I
should specify if I want to allow empty things, or allow specific empty
things. But I shouldn't have to specify what is empty, at least for
such a general case as this.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/hapijs/joi/issues/1559#issuecomment-410360548, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ALl4O01zZVbS6fbXTRau4xsznL5dNpifks5uNKt5gaJpZM4VuW1a
.

[WesTyler]

I'm also not a big fan of .required() as the default. I like it being explicit.

[sholladay]

When is it a good default apart from query strings ?

The vast majority of forms that I've wrriten contain more required fields, such as username, than optional fields. For them, an empty string and undefined are approximately the same thing. The user didn't fill it out.

I prefer for strictness to be the default and force people to be explicit about optionality. It's important for me to know when something is optional because that implies extra complexity - there usually needs to be double the code paths to handle whether the field was given or not.

[Marsup]

To be clear I was asking about empty strings being denied, not required becoming default, for which I haven't seen evidence in the last few years that people were confused about it.

The default presence seems very subjective to me, personally I prefer seeing mandatory properties being mentioned, pretty much following the standard of putting red stars on form fields for the same purpose. Considering the migration hell it would create, I'd rather have really compelling advantages before changing it.

[DavidTPate]

• string.uri([options]) the option allowRelative being false by default has been a little weird as it is a very common thing to have relative URLs. I haven't really seen much complaints about it though so it might just be a non-issue.

[rokoroku]

:+1: for disable stripUnknown stripping arrays.
its behavior (stripping failed value rather than returning a validation error) is mostly not an expected result, and also a thing that somewhat hard to inspect

[skeggse]

I'm just gonna toss in my vote that we leave the current email behavior until there are no more gTLDs with MX records. I do appreciate the desire to reject common false-positives, but IMO we use isemail to be super accurate and using minDomainAtoms begins to reject "valid" (though discouraged) email addresses. As of today, there are 23 gTLDs with MX records

[Marsup]

@skeggse How often does it really happen though? I'm trying to go for the most common case here, but of course if that doesn't make sense people are free to override.

[skeggse]

To be sure, that happens super rarely. My inclination is to steer towards the more conservative approach, but you're more familiar with joi's users and use-cases, so I'm deferring to your judgement.

[Marsup]

In my experience, people expect email validation to be [email protected] at least, they don't want to validate user@localhost, that's not something you see very often in a web context, which represents most of the joi usage. I just have to understand the little issue I posted on isemail repo ;p

[skeggse]

Fwiw I would usually add localhost to the blacklist. Alternatively it would also fail the DNS resolution test (provided you make the call to a public DNS server like 1.1.1.1 or 8.8.8.8). I think minDomainAtoms=2 is reasonable but I'd still never leave it as default :p

[adamreisnz]

I ran into what I consider a painful default when trying out Joi and doing validation on the following query:

?fromDate=2019-04-21T12:00:00.000Z&limit=25&offset=0&search=&toDate=2019-04-29T11:59:59.999Z

The documentation states:

When validating a schema:
Values (or keys in case of objects) are optional by default.
Joi.validate(undefined, Joi.string()); // validates fine

Great I thought, so I don't need to specify all the optional fields, so I made the following schema as only fromDate and toDate are required:

{
  query: {
    fromDate: joi.date().iso().required(),
    toDate: joi.date().iso().required(),
    limit: joi.number().min(0).integer(),
    offset: joi.number().min(0).integer(),
  }
}

However, this yielded an error: "search" is not allowed
Why is it not allowed? I didn't even define it in the schema.

So then, I added the following:

{
  query: {
    fromDate: joi.date().iso().required(),
    toDate: joi.date().iso().required(),
    limit: joi.number().min(0).integer(),
    offset: joi.number().min(0).integer(),
    search: joi.string(),
  }
}

This resulted in another error: '"search" is not allowed to be empty

As a last resort I tried to use optional(), which according to the docs should only be

Used to annotate the schema for readability as all keys are optional by default.

{
  query: {
    fromDate: joi.date().iso().required(),
    toDate: joi.date().iso().required(),
    limit: joi.number().min(0).integer(),
    offset: joi.number().min(0).integer(),
    search: joi.string().optional(),
  }
}

Still the same error though: "search" is not allowed to be empty

So I'm at a loss, the defaults which are documented to be optional, seem to break as soon as an empty string is passed. In my opinion validation of '' should be treated the same way as undefined: if it's optional (even explicitly stated as optional), it shouldn't invalidate it and say it's not allowed to be empty.

And also, if a key is not specified at all, but an empty string is provided, I don't think it should complain that the field is not allowed.

In the end I had to resort to this overly verbose declaration, just to allow an empty string for an optional parameter:

search: joi.string().optional().allow('')

So I guess I'm in favour of empty strings being allowed by default 👍

[sholladay]

@adamreisnz, in your case, you want to use .unknown():

{
  query: joi.object().unknown().keys({
    fromDate: joi.date().iso().required(),
    toDate: joi.date().iso().required(),
    limit: joi.number().min(0).integer(),
    offset: joi.number().min(0).integer(),
  })
}

That will allow search as well as any other keys to exist on the object without validating them. Note that, by default, the unknown keys are not filtered out of the validated object, but you can use the stripUnknown option if you want them to be.

[adamreisnz]

I tried that, it still complained about the empty string unfortunately. I think this is the same problem, with undefined values vs empty string.

I also find that a painful default to be honest – if empty values are not required by default, why are unknown values forbidden by default? They should also be allowed by default, to make the validation more relaxed, and let us explicitly specify that unknown values are not okay.

[sholladay]

it still complained about the empty string

Indeed. Unknown keys and what is considered "empty" are two separate issues.

I guess they probably chose to make unknown keys forbidden by default because a major use case for joi is to validate input on hapi routes. It would be dangerous for a client to be able to add arbitrary keys to an object stored in a database, for example. That would be a relatively easy path to NoSQL injection among other problems.

That said, I could see an argument for _stripping_ but not _forbidding_ unknown keys by default.

[Marsup]

I'd disagree there. Joi is meant to validate, what good is a validator if its default is to strip whatever it can't validate ?

[Marsup]

Also, if someone calls your API and sends searc instead of search, that's imho a major issue waiting to happen.

[adamreisnz]

I agree that stripping by default is not the right approach.

Also, if someone calls your API and sends searc instead of search, that's imho a major issue waiting to happen.

By major issue, do you mean that it won't work because they made a typo? Is that the use case for warning for unknown keys by default?

[Marsup]

If it looks like it works but in fact it doesn't, it can only result in a lot of wasted time and vicious bugs. Silent failures are the worst, you cannot possibly expect that to be the default behavior of a validator, even as an option it looks odd to me.

[adamreisnz]

I guess my perspective is that I would want the option to have a validator which validates only the fields that I specify, and ignores all the rest. If users send unknown parameters to our API, then the problem lies with them, not the API. But I appreciate your point of view in that the API can report these errors to make debugging such issues easier.

[Marsup]

If you want a "safe" workaround for that you can use pattern with a strip like so: https://runkit.com/embed/989c0zylrd8o. That doesn't remove the keys but at least you drop the values.

[hueniverse]

I am against changing the default to required or allowing empty string by default - this would be a huge breaking change that would require rewriting every single schema.

[hueniverse]

The rest has already been applied.