I might add unit tests but the Pr is up for review

This doesn't make sense to me. bcrypt.hashpw expects bytes. Why is to_unicode being called here?

The password should be decoded to unicode when the request is received and the encoding is known (from the request headers). It should then be encoded here using the same encoding used to check the password later.

I'm not sure the bcrypt library that you are pointing to is the one we use, we use the one that's packaged as py-bcrypt in Fedora

Oh my, that project seems very, very dead. Granted, I suppose it doesn't do much, but still.

I poked around and although I'm not very familiar with Python's C API it looks to me like it's expected a byte string (which makes sense as an API) .

I was very concerned that this was a CVE since using to_unicode with its default kwargs is not at all safe for passwords.

The problem is it

a) requires the bytes to be encoded with 'utf-8' which is not a safe assumption (and pointless in any case since you're immediately re-encoding it with utf-8)

b) ignores decoding errors by using 'replace' which replaces un-decodeable bytes with a special character, U+FFFD, that marks a character that was not valid UTF-8.

This means that if you submit a byte string to this function that is not UTF-8 and aren't valid UTF-8 bytes you will have a password made up of U+FFFD characters, which happy encodes to UTF-8 again. Instead of their original password, you've now made them a password of some number of U+FFFD characters.

Luckily, Flask and flask-wtf decode the password to unicode so the password is already decoded to unicode (properly, using the client-provided encoding header) when passed to this function. This function's API should be documented to require a unicode string, and then encode it to UTF-8.

@farhaan, you need to remove the call to to_unicode

bcrypt.hashpw(password.encode('utf-8'), ...)

It would also be good to add tests.

@jcline sorry for the delay was caught up with college :)

There is this line here also but it was being used for the previous version should I replace this also @jcline and @pingou ?

I think so, yes.

I have the utmost confidence on @jcline when it comes to unicode :)

Hey I have rebased and updated the pr @jcline and @pingou this is up for review :smile:

I really recommend breaking this out into its own test. Python's standard library has good documentation how how to organize tests.

This test will perform differently on Python 2 than it will on Python 3. In Python 2 this is going to be automatically encoded with UTF-8 to a byte string. In Python 3 this will remain a unicode string.

The password API should only accept unicode strings. You can add a u in front of the string to mark it as a unicode literal so it'll be the same in Python 2 and Python 3. I also think the function should raise a ValueError if the string provided isn't a unicode string. You can use six.text_types to get the correct class (str for Python 3, unicode for Python 2). It'd be good to have a test for that, as well.

@jcline thanks for the review I will look into organizing tests and put it in the right place :smile:

@jcline can you elaborate more on six.text_type do you want to check the password in lib/login and then raise ValueError ?

@jcline can you elaborate more on six.text_type do you want to check the password in lib/login and then raise ValueError ?

Yeah. I was referring to the six library. It provides a bunch of helpers for writing Python. In this case, we want to only accept unicode text. In Python 2, this is of type unicode, but in Python 3 this is just str. You can use six.text_type to check that the argument you got is unicode:

if not isinstance(password, six.text_type):
    raise ValueError('This API requires unicode text')

and your test case should assert that this is the case:

def test_unicode_required(self):
    self.assertRaises(ValueError, generate_hashed_value, u'hunter2'.encode('utf-8'))

If you're not already familiar with unicode and encoding in general, I recommend reading some of the excellent blog posts out there. joelonsoftware has a decent one.

This PyCon 2012 talk discusses it specifically in the context of Python.

Thanks a lot for the blogpost and the talk seems really interesting , I am going through it.

s/:return/:return:/

It'd also be good to note that the return type is an encoded string (bytes)

s/upoun/upon/. Might also be good to explicitly note that it returns True if the password matches.

If I recall correctly, the hashlib API requires bytestrings. This will sort of work in Python 2, but it won't in Python 3. You'll need to explicitly encode before you hash it:

password = u'%s%s' % (entered_password, seed)
password = hashlib.sha512(password.encode('utf-8').hexdigest()

@jcline do you want to have a final look at this one?

I'm not a fan of the length of that first test, but it's not a deal-breaker for me. Looks good to me.

I'm not a fan of the length of that first test, but it's not a deal-breaker for me. Looks good to me.

Can you help me reduce the length of the test case ?

Sure! I've left comments in-line.

This test isn't really related to the web form accepting a non-ASCII password. It'd be good to be its own test, perhaps, although I expect other tests are also running this.

This also isn't related to the test. It's best to narrow the scope of a unit test to the smallest possible piece of functionality - we want to know exactly what's not working when it fails, but adding all these pre-check assertions makes it more difficult to figure out what's going wrong. I recommend dropping this whole block, as well.

This isn't a test for CSRF token functionality, so you can drop this whole block

This isn't a test about the email needing to be unique (as an aside, HTTP 200 is almost certainly not the right error code) so this block can go, too.

This block can also be dropped, it's not a test about usernames being unique.

This block can be dropped.

This block can be dropped.

This block can be dropped.

This would probably be a good test all by itself to make sure, for example, it doesn't just accept any non-ascii password, but it's not something we should combine with a test about whether it's possible to sign up and authenticate with a non-ascii password.

This block can also be dropped.

This comment doesn't seem relevant? I'm not seeing an error being asserted below (it looks like the block below tests that it's possible to log in, which is what we want)

it should accept any non-ascii character as password right ?

Yes.

hey @jcline I read your comments and followed it as well didn't push the code because I have few clarification:

• How should we divide the tests here ?
• Which test should be placed where ?
• How should I think about generating test cases ?

P.S: Sorry for such late action on the PR was caught up with college

I added some docs about test organization to the infra docs. They also link to the Python documentation on the unittest package and I recommend reading them since they've got a section on how to write tests. I think it'll answer a lot of your questions. If it doesn't, we can update the documentation to be more helpful!

@jcline thanks for the link I will go through and improve the cases and get back to it :)

I've rebased this PR which I believe will also allow to drop pybcrypt in favor of python-bcrypt.

If so, I'll merge this PR and open one about the change in dependencies.

Commit 9d34ee48 fixes this pull-request