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

compareSync function breaks with deno v1.23.0 #28

Closed
l3onh4rd opened this issue Jun 16, 2022 · 8 comments
Closed

compareSync function breaks with deno v1.23.0 #28

l3onh4rd opened this issue Jun 16, 2022 · 8 comments

Comments

@l3onh4rd
Copy link

Hi togehter,

I am using the compareSync functions to verify a password with a password hash and it is deployed to Deno Deploy. Since I would like to update to deno v1.23.0 the function just returns "false" for the same input which returns "true" on deno v1.22.3. Funnily the compare function (without "Sync") works on Deno v1.23.0 but this doesn't work on Deno Deploy (as described in another issue).

Has anybody a solution for that? Does deno v1.23.0 or std v0.144.0 breaks the library?

Looking forward to a solution.

Kind regards

Leo

@JamesBroadberry
Copy link
Owner

Could you provide some steps to replicate this? Someone else has reported a similar problem (#27) but hasn't provided any steps to replicate it. A hash and its plaintext would be useful. Thank you ☺️

@l3onh4rd
Copy link
Author

I can confirm that its working with a new generated password hash and the compareSync functions (Deno v1.23.0) But not with a password hash generated with Deno version < 1.23.0.

PasswordHash: $2a$10$ETViiyJVEWiPuItcwXHWaeR1HuYETfZiHjYE7qXETwS0LwcSPtbUG
Password: Secure1234

deno 1.23.0 (release, x86_64-pc-windows-msvc)
v8 10.4.132.5
typescript 4.7.2

I hope that will help you.

Kind regards
Leo

@Manioume
Copy link

Manioume commented Jun 16, 2022

I can also confirm that since Deno version 1.23.0 even async compare is broken:

Using

import * as bcrypt from "https://deno.land/x/[email protected]/mod.ts";

Here are the steps:

>deno --version
deno 1.22.3 (release, x86_64-pc-windows-msvc)
v8 10.3.174.6
typescript 4.6.2
const hash = await bcrypt.hash("replicate");
const result = await bcrypt.compare("replicate", hash);
console.log(`Should be true: ${result}`);
const resultKo = await bcrypt.compare("replicate2", hash);
console.log(`Should be false: ${resultKo}`);
Should be true: true
Should be false: false

Same code as 1.23.0

>deno --version       
deno 1.23.0 (release, x86_64-pc-windows-msvc)
v8 10.4.132.5
typescript 4.7.2
Should be true: true
Should be false: true

Hope it helps !

Best regards,

@JamesBroadberry
Copy link
Owner

I think I've found the root cause. I'll have a fix out soon. It seems that the whole cipher is ignored due to some changes in how the code is interpreted in Deno 1.23.0 and I've raised an issue on the Deno repository.

For some reason, when running the tests (even after adding more to account for these cases), this behavior doesn't seem to apply when testing.

For now, do NOT use bcrypt v0.3.0 (or lower) with Deno 1.23.0 since the cipher seems to be ignored entirely, meaning it's likely that ANY plaintext will verify any hash, and hashes generated won't be correct.

Thanks for all the steps to replicate and the hashes provided, they've been really useful. I'll update this issue when the fix is out.

@JamesBroadberry
Copy link
Owner

@Manioume @l3onh4rd
Could either of you use this import of bcrypt and verify that it's fixed?
import * as bcrypt from "https://raw.githubusercontent.com/JamesBroadberry/deno-bcrypt/issue/28/mod.ts";
Source code in branch issue/28 if you'd like to check the code changes

@Manioume
Copy link

Hello @JamesBroadberry,

imported import * as bcrypt from "https://raw.githubusercontent.com/JamesBroadberry/deno-bcrypt/issue/28/mod.ts"; and using Deno 1.23.0 with same block of code, can confirm it's fixed.

Love the fix btw :D i bet you sweated a bit to find this one out.

>deno --version
deno 1.23.0 (release, x86_64-pc-windows-msvc)
v8 10.4.132.5
typescript 4.7.2
Should be true: true
Should be false: false

Thanks for the fix and the fast replies.

Best regards,

@l3onh4rd
Copy link
Author

Hi together,

I can confirm as well that now everything is working again. Thank you so much for providing a fix!!! This is awesome!!!

What do you think, when you will release it?

Kind regards

@JamesBroadberry
Copy link
Owner

Thanks for verifying the fix worked. I've released it now as v0.4.0 and is now live.
Hopefully, the underlying bug will be fixed and later version of Deno won't have this problem.

@Manioume haha thanks, it really wasn't easy to find, especially as it was just a port and I'm no cryptography expert 😁
@l3onh4rd No problem 😄

I'll close this issue for now but if you find the fix didn't work for whatever reason, I'll re-open it.

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

No branches or pull requests

3 participants