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.

Sign up for GitHub

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

Jump to bottom

Update crypto hash to use sha256 #38

Closed
HappyZombies opened this issue Oct 13, 2021 · 6 comments · Fixed by #43
Closed

Update crypto hash to use sha256 #38

HappyZombies opened this issue Oct 13, 2021 · 6 comments · Fixed by #43
Labels
good first issue ✅ Good for newcomers security ❗ Address a security issue

Comments

@HappyZombies
Copy link
Member

HappyZombies commented Oct 13, 2021 •
edited

This line in token-utils.

Uses sha1 for the hash, we should update this to be sha256 and update our unit tests too.
@HappyZombies HappyZombies added good first issue ✅ Good for newcomers security ❗ Address a security issue labels Oct 13, 2021
@HappyZombies HappyZombies changed the title Updated crypto hash to use sha256 Update crypto hash to use sha256 Oct 13, 2021
@jwerre
Copy link
Contributor

jwerre commented Oct 13, 2021

Yes! nice find @HappyZombies

@jwerre
Copy link
Contributor

jwerre commented Oct 13, 2021

This line will need to change to: new chai.Assertion(obj).match(/^[a-f0-9]{64}$/i);

@jankapunkt
Copy link
Member

Guys I just realized the tests does not test anything, even If make the line new chai.Assertion(obj).match(/^foobar$/i); they all pass

That will get interesting now. I will dig a bit deeper to see where the issue comes from

@jankapunkt
Copy link
Member

Okay found the issue: data.should.be.a.sha256; needs to be changed to data.should.be.sha256(); then it will throw an error when passing an SHA1 value :-)

jankapunkt added a commit that referenced this issue Oct 14, 2021
@jankapunkt
fix(security): use sha256 for token generation #38
769878d
@jankapunkt jankapunkt mentioned this issue Oct 14, 2021
Merged
@jankapunkt jankapunkt linked a pull request Oct 14, 2021 that will close this issue
use sha256 for token generation #43
Merged
@jwerre
Copy link
Contributor

jwerre commented Oct 14, 2021

Unbelievable, I'm starting to think it was a really good idea we forked this repo.

@jankapunkt
Copy link
Member

implemented via #43

@Uzlopak Uzlopak mentioned this issue Nov 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue ✅ Good for newcomers security ❗ Address a security issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

use sha256 for token generation node-oauth/node-oauth2-server
3 participants
@jwerre @jankapunkt @HappyZombies