Number.isNaN is not supported in MSIE

[tulach]

Newly introduced nesting depth checks utilizes function Number.isNaN which is not supported in MSIE.

Background & Context

Branch 2.x is supposed to work in MSIE.

Nesting depth checks uses numberIsNaN() which is result of call unapply(Number.isNaN). In this case it uses apply helper which is defined as

    apply = function apply(fun, thisValue, args) {
      return fun.apply(thisValue, args);
    };

Parameter fun is undefined because Number.isNaN doesn't exists and exception is thrown.

Proposed solution:

Replace

  var numberIsNaN = unapply(Number.isNaN);

with

  var numberIsNaN = unapply(isNaN);

Bug

Input

<table cellspacing="0" cellpadding="0"><tr><td></td><td></td></tr><tr><td></td><td></td></tr></table>

Given output

Exception is thrown Cannot read property apply of undefined or null reference

Expected output

<table cellspacing="0" cellpadding="0"><tr><td></td><td></td></tr><tr><td></td><td></td></tr></table>

[cure53]

Oh, that's a good find, thanks! Wanna spin up a PR real quick against the 2.x branch?

[tulach]

Unfortunately I found that proposed solution is not working. It doesn't throw exception but return empty string. Honestly I don't know exact reason why you need numberIsNaN. It seems it doesn't have any reason even in 3.x branch because Number.isNaN is called with empty arguments array. But maybe I'm wrong and don't see the whole picture.

[tulach]

Ok. I think I got it.

unapply() helper is for prototypes but this is not the case of Number.isNaN.

The difference between Number.isNaN() and isNaN() is return value when input parameter is undefined. isNaN(undefined) returns true but Number.isNaN(undefined) returns false.

shadowNode.__depth can be undefined.

[tulach]

I suppose the unapply() with prototypes is all about to prevent function spoofing.

My proposal is to avoid buildin isNaN functions and use following code:

var numberIsNaN = function(n) { return n !== n };

It is equivalent to Number.isNaN because NaN is the only value in JavaScript which is not equal to itself.

There are also other options how to deal with it. I will not create PR. I'll let you decide which is the right solution.

[cure53]

I think using the custom implementation of isNaN for 2.x makes most sense to keep compatibility with MSIE.

I am not 100% if what you mention above also applied for 3.x and main, the tests seemed happy (specifically the ones we created to cover prototype pollution), do you still see any concerns with the approach we follow in 3.x?

[cure53]

I checked on MSIE via VM and it seems that your proposed path is the way to go, that being the custom implementation, are you planning to PR this or should we push some code for 2.x? :slightly_smiling_face:

[cure53]

I think this might already do the trick, no?

https://github.com/cure53/DOMPurify/commit/f3a97107b29e6af78884e05a63638622eddb6df6

[tulach]

I don't use 3.x branch but I think unapply() to non-prototype results of calling function without proper parameters (unapply removes first argument).

It seems that Number.isNaN unfortunately has no prototype.

This means that

const numberIsNaN = unapply(Number.isNaN);

is the same as

const numberIsNaN = function(v) { return false; }

which is totaly useless :(.

Therefor I think the fix is also for branch 3.x.

[tulach]

To validate my statement

numberIsNaN(NaN)

should return true but returns false.

[cure53]

It looks like you are correct, the return value is always "false". However, that is the same for you function, correct? I am not quite certain how the given function body does an actual isNaN check. I get false every time as well.

Imho, we should use this instead:

export function numberIsNaN(x) {
  // eslint-disable-next-line unicorn/prefer-number-properties
  return typeof x === 'number' && isNaN(x);
}

[tulach]

This is weird. As I said before "NaN is the only value in JavaScript which is not equal to itself.". This means NaN!==NaN is the only case when comparing two same values is not equal and return true. That is why function(x) { return x!==x; } is polyfill to Number.isNaN.

In other words I don't believe you that this polyfill returns also false everytime as well.

Check this https://github.com/es-shims/Number.isNaN

Also I managed to polute Number.isNaN which is also not good.

[cure53]

Ah, I understand, so you refer to actually comparing NaN with NaN but we are checking to see if we have an actual number here :slightly_smiling_face:

So yeah, bottom line, we were both right, we will go with the extended version of the function body.

[cure53]

That should do the trick on 2.x then and we will add the same for main.

https://github.com/cure53/DOMPurify/commit/707b3d682451b69c003c4ecb363550b93ef595a9

[tulach]

Great! Thank you very much. Looking forward for new release :)

[cure53]

Thanks again :smile: