number().integer() can fail for values outside MAX_SAFE_INTEGER

[kanongil]

Context

• node version: v7.5.0
• joi version: v10.3.1
• environment (node, browser): node
• used with (hapi, standalone, ...): standalone
• any other relevant information:

What are you trying to achieve or the steps to reproduce ?

const schema = Joi.number().integer();
schema.validate('90071992547409910.1');

Which result you had ?

{ error: null, value: 90071992547409900 }

What did you expect ?

Some kind of error, like:

{ error: 
   { ValidationError: "value" must be an integer
     isJoi: true,
     name: 'ValidationError',
     details: [ [Object] ],
     _object: '90071992547409910.1',
     annotate: [Function] },
  value: '90071992547409910.1' }

[WesTyler]

I would argue that this is a failing in Node (because the number is outside of the recognized range of integers):

> const tooBig = 90071992547409910.1;
> tooBig
90071992547409900

If Node can't even assign the value to a variable and remember it correctly, I'm not sure we can expect parseInt and parseFloat (used in Hoek.isInteger) to handle it properly. Unless we assume that anything larger than Number.MAX_SAFE_INTEGER can be assumed not to be an integer.

[WesTyler]

FWIW, number().integer() uses Hoek.isInteger behind the scenes, which uses 3 checks:

typeof value === 'number' &&
parseFloat(value) === parseInt(value, 10) &&
!isNaN(value)

Node just can't hang, which is why this is passing validation:

> typeof 90071992547409910.1
'number'
> parseInt(90071992547409910.1)
90071992547409900
> parseFloat(90071992547409910.1)
90071992547409900
> parseFloat(90071992547409910.1) === parseInt(90071992547409910.1)
true

[Marsup]

I think it would be preferable that joi always fails if Number.isSafeInteger returns false, wdyt ?

[kanongil]

Probably the Hoek method can be replaced with:

typeof value === 'number' && Number.isSafeInteger(value)

FYI, I validated that node 4 supports Number.isSafeInteger()

[Marsup]

The problem also lies in joi, if you don't have integer() and parse the same number, it'll still give you nonsense.

[kanongil]

The problem also lies in joi, if you don't have integer() and parse the same number, it'll still give you nonsense.

That part I expect as a JS quirk. It's only the integer handling that bugs me.

[Marsup]

I'm seeing this as the API consumer, if you provide such a number and the backend is not able to parse it correctly, it should reject it, whether you asked for an integer or not.

[kanongil]

The problem is that there are valid use-cases for numbers outside the safe integer range, and it is part of how floating point works.

"Fixing" the range for all numbers is likely to cause problems for such use-cases, while it most likely won't make any difference for those outside it.

Essentially, number() should just be parseFloat(), or the actual provided number.

[Marsup]

@nlf thoughts ? Are you willing to patch hoek or do I add this check in joi's integer() ?

[nlf]

sounds like this should probably get patched in hoek

[Marsup]

Now it's been discussed on hoek, I'm tempted to qualify this as a bug, as previous behavior was unsafe, thoughts ? (meaning patch version)

[kanongil]

What I originally reported is definitely a bug as it accepted a number followed by a dot + another number, which will never be an integer.

Regarding the specific fix, I would tend to agree on treating this as a bugfix, though as usual it might break existing code that inadvertently relies on this. For semver purposes, it comes down to how it is described in the api, which is currently:

Requires the number to be an integer (no floating point).

[rictorres]

shouldn't this be tagged as a major?

we use int identifiers in our database, so something like 64811494532973582 was being validated just "fine" with Joi.number().integer().positive().min(1).required().

[Marsup]

If you provide 64811494532973582 to javascript, you get 64811494532973580, do you really consider it safe ? I know I wouldn't.

[nlf]

i agree, this isn't a breaking change. while it may be a change in behavior, it's fixing a bug. with the previous behavior if you had a validator such as the one in your example in a handler in hapi, and you were to make a request with an id like the one in your example, by default the id would've been changed and you would've been potentially manipulating an object you didn't expect.

this fix pushes you to find these potential issues and find a safer way to validate your inputs to make sure you're getting reliable and consistent results. it may be inconvenient right now, but it's a win overall and fixes what could be a pretty significant bug in some cases.

[WesTyler]

while it may be a change in behavior, it's fixing a bug.

I agree wholeheartedly.

[rictorres]

If you provide 64811494532973582 to javascript, you get 64811494532973580, do you really consider it safe ? I know I wouldn't.

my mistake was not actually understanding how joi was validating my input.

@nlf, on a second thought, i totally agree with you.

thanks for all your responses :)