GeezerVison

Should we add the false positive and false negative as a test (maybe in the known_issues directory?) so that we know to update the docs if it ever gets changed/fixed?

I don't understand the problem here. timingSafeEqual() compares bytes, so the results seem expected to me.

The two examples given have to do with the larger "problem" of JavaScript equality operator behavior (NaN !== NaN and 0 === -0) rather than any gotcha with timingSafeEqual(). You can run into these same problems outside of this crypto context because they're language issues.

On a related note, you could use Object.is() instead, which should give you the equality checking you're looking for.

timingSafeEqual() compares bytes

@mscdex The current documentation does not explicitly say so, and that's exactly what this PR is trying to address.

because they're language issues.

This would apply to any language that implements a subset of IEE 754.

On a related note, you could use Object.is() instead, which should give you the equality checking you're looking for.

Sure, but this function is specifically for constant-time comparisons, which Object.is() does not implement.

The current documentation does not explicitly say so, and that's exactly what this PR is trying to address.

Perhaps we should say exactly that then, as the current language this PR adds doesn't make that obvious IMO. Something like "This method compares the underlying bytes that represent any typed array or Buffer."

Sure, but this function is specifically for constant-time comparisons, which Object.is() does not implement.

I was referring to the equality checks inside the .every() callbacks.

Added a test and reworded to closer match JS's understanding of equal versus same.

LGTM but pinging some other folks on these questions:

• Is the documentation as clear and concise on this as we can make it?
• Should the testing note that this is a surprising feature? Is it really a bug and should be in known_issues? Or is this expected behavior, should not change, and the test is in the right place?

@nodejs/documentation @nodejs/crypto @nodejs/testing

@mscdex Reworded again and removed most of the text as requested.

Edit: Reworded and extended again since this does not match Object.is() semantics.

@Trott I wouldn't consider it a bug, just a very unusual and permissive function signature... for example, comparing a Float64Array to a Uint32Array also seems unusual (and probably unintentional).

Edit: As @bnoordhuis pointed out, the semantics are neither equality nor same value. Accepting Float(32|64)Array seems questionable to me.

ping @mscdex @nodejs/documentation @nodejs/crypto

CI: https://ci.nodejs.org/job/node-test-pull-request/44774/

Landed in 800cff1