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

Failing "password" test #57

Merged
merged 2 commits into from Apr 26, 2018

Conversation

3 participants
@mcdappdev
Copy link
Member

mcdappdev commented Apr 26, 2018

Add's a failing test case because for some reason hashing "password" at a cost of 2 fails

@mcdappdev

This comment has been minimized.

Copy link
Member Author

mcdappdev commented Apr 26, 2018

Upon further investigation this seems to only happen with a cost of 1, 2, or 3

@tanner0101 tanner0101 added the bug label Apr 26, 2018

@tanner0101 tanner0101 self-assigned this Apr 26, 2018

@tanner0101 tanner0101 requested a review from bre7 Apr 26, 2018

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Apr 26, 2018

From what I can tell this is expected: https://github.com/libressl-portable/openbsd/blob/master/src/lib/libc/crypt/bcrypt.c#L143

We should probably just throw a nicer error here if the cost is below BCRYPT_MINLOGROUNDS

@mcdappdev

This comment has been minimized.

Copy link
Member Author

mcdappdev commented Apr 26, 2018

Oh interesting, that's good to know. Cool, I'm good with this being closed then

@bre7

This comment has been minimized.

Copy link
Contributor

bre7 commented Apr 26, 2018

I've seen this in some other wrappers:

if rounds < 4 or rounds > 31:
        raise ValueError("Invalid rounds")

So yeah, oversight on my part 😳 I'll add a guard asap

@bre7 bre7 referenced this pull request Apr 26, 2018

Merged

Added guard for invalid cost #58

@@ -20,6 +21,10 @@ class BCryptTests: XCTestCase {
let res = try BCrypt.verify("bar", created: digest)
XCTAssertEqual(res, false)
}

func testHashPassword() throws {
XCTAssertNoThrow(try BCrypt.hash("password", cost: 2))

This comment has been minimized.

@bre7

bre7 Apr 26, 2018

Contributor

Use XCTAssertThrowsError instead ;) Thanks for the catch btw

This comment has been minimized.

@bre7

bre7 Apr 26, 2018

Contributor

And rename the test case to testInvalidCost

@bre7

This comment has been minimized.

Copy link
Contributor

bre7 commented Apr 26, 2018

No need to close, in fact your test is ok. The only thing that needs to be changed is XCTAssertNoThrow for XCTAssertThrowsError 👍

@mcdappdev

This comment has been minimized.

Copy link
Member Author

mcdappdev commented Apr 26, 2018

Cool, since it's intended changing that makes sense. Will update now

@bre7

This comment has been minimized.

Copy link
Contributor

bre7 commented Apr 26, 2018

If you want to, add the guard also (paste it from #58) and I'll close my PR

@mcdappdev

This comment has been minimized.

Copy link
Member Author

mcdappdev commented Apr 26, 2018

Sure no problem! I just walked away from my computer but will be back in about an hour

@tanner0101 tanner0101 merged commit 98a85bf into vapor:master Apr 26, 2018

5 checks passed

ci/circleci: linux Your tests passed on CircleCI!
Details
ci/circleci: linux-jwt Your tests passed on CircleCI!
Details
ci/circleci: linux-mysql Your tests passed on CircleCI!
Details
ci/circleci: linux-postgresql Your tests passed on CircleCI!
Details
ci/circleci: linux-vapor Your tests passed on CircleCI!
Details

@tanner0101 tanner0101 added this to the 3.1.1 milestone Apr 26, 2018

@tanner0101

This comment has been minimized.

Copy link
Member

tanner0101 commented Apr 26, 2018

Thanks @mcdappdev!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment