Skip to content

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

Open
ChALkeR wants to merge 1 commit into
ricmoo:masterfrom
ChALkeR:no-nan
Open

fix: don't allow NaN/Infinity params#31
ChALkeR wants to merge 1 commit into
ricmoo:masterfrom
ChALkeR:no-nan

Conversation

@ChALkeR

@ChALkeR ChALkeR commented Aug 6, 2021

Copy link
Copy Markdown

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

ricmoo commented Aug 6, 2021

Copy link
Copy Markdown
Owner

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

ChALkeR commented Aug 6, 2021

Copy link
Copy Markdown
Author

@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

ricmoo commented Aug 6, 2021

Copy link
Copy Markdown
Owner

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.

2 participants