You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
crypto: variable-length hash algorithms do not throw errors for non-integer/non-safe-integer lengths, tests cases assume they do, but test logic is broken and suppresses the assertion failures
#3798
Closed
jeremyBanks opened this issue
Nov 13, 2023
· 0 comments
· Fixed by #3799
@quentinadam and @iuioiua noticed in #3793 (comment) that the crypto_test.ts logic was failing to check that errors were being thrown when expected.
I've made a commit that fixes the test logic: jeremyBanks@e571f76. Upon fixing it, three tests start to fail for two cases each: the three variable-length hash algorithms, BLAKE3, SHAKE128, and SHAKE256 do not throw errors for the cases Non-integer length with options {"length":1.5}) (which they treat as length: 1) and Too-large length with options {"length":8.112963841460666e+31}) (which they treat as length: 0), but our current test cases expect them to throw errrors.
The test logic definitely needs to be fixed, so that commit should be applied, but obviously that can't be merged until we also fix the crypto.ts logic to perform the expected checks (which might be considered BREAKING if we're being super-pedantic, but is probably safe in practice), or to change the expected result.
Some might argue that that truncating 1.5 to 1 is okay (I would prefer it to be an error), but I think all would agree that any length that's too large to fit into 32 bits (usize for WASM targets) should throw an error instead of being truncated to 0.
The text was updated successfully, but these errors were encountered:
jeremyBanks
changed the title
crypto: variable-length hash algorithms do not throw errors for non-integer/non-safe-integer lengths, but tests assume they do
crypto: variable-length hash algorithms do not throw errors for non-integer/non-safe-integer lengths, tests cases assume they do, but test logic is broken and suppresses the assertion failures
Nov 13, 2023
@quentinadam and @iuioiua noticed in #3793 (comment) that the
crypto_test.ts
logic was failing to check that errors were being thrown when expected.I've made a commit that fixes the test logic: jeremyBanks@e571f76. Upon fixing it, three tests start to fail for two cases each: the three variable-length hash algorithms,
BLAKE3
,SHAKE128
, andSHAKE256
do not throw errors for the casesNon-integer length with options {"length":1.5})
(which they treat aslength: 1
) andToo-large length with options {"length":8.112963841460666e+31})
(which they treat aslength: 0
), but our current test cases expect them to throw errrors.The test logic definitely needs to be fixed, so that commit should be applied, but obviously that can't be merged until we also fix the
crypto.ts
logic to perform the expected checks (which might be consideredBREAKING
if we're being super-pedantic, but is probably safe in practice), or to change the expected result.Some might argue that that truncating
1.5
to1
is okay (I would prefer it to be an error), but I think all would agree that any length that's too large to fit into 32 bits (usize
for WASM targets) should throw an error instead of being truncated to0
.The text was updated successfully, but these errors were encountered: