Skip to content

fix: don't allow NaN/Infinity params #31

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChALkeR
Copy link

@ChALkeR ChALkeR commented Aug 6, 2021

I believe this is a regression in e55eb39#diff-714a4bf03391301f9f8a3a4b026cd1a82fa422b664209252422963d9548341c6L255-R255, previous versions of scrypt-js correctly rejected NaN input but 3.x doesn't.

Also, perhaps more checks are needed there, e.g. for the number to be non-negative (>= 0) and <= Number.MAX_SAFE_INTEGER. Wdyt?

Tests included.

Without the patch, the lib misbehaves on NaN and Infinity input.

@ricmoo
Copy link
Owner

ricmoo commented Aug 6, 2021

Keep in mind that changing var to const/let will break this in older environments, so those can’t be made without a major version change.

I am planning to make a major version change in the near future though, which will allow modern syntax.

@ChALkeR
Copy link
Author

ChALkeR commented Aug 6, 2021

@ricmoo I don't understand, sorry. 3.x is already using const/let in both the package source and tests.

The only thing that was changed outside of tests in this PR is Number.isInteger usage.

@ricmoo
Copy link
Owner

ricmoo commented Aug 6, 2021

Oh!! Sorry, I just checked and you are correct. I think I confused this with my AES library?

I'm planning a major release soon, which will port the library to TypeScript. I'll look into this PR as soon as I can.

Thanks! :)

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants