Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

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

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! :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants