Fix username enumeration vulnerability

[jayjay-w]

Description

When creating a user account any error messages should be generic and should not indicate that the email already exists, or that the email does not exist.

References: CV2-5909

How has this been tested?

Confirmed that the error messages when registering with an existing email or when registering in with a non-existent email are generic and do not leak details about the email addresses stored in the database.

Checklist

• I have performed a self-review of my own code
• I have added unit and feature tests, if the PR implements a new feature or otherwise would benefit from additional testing
• I have added regression tests, if the PR fixes a bug
• I have added logging, exception reporting, and custom tracing with any additional information required for debugging
• I considered secure coding practices when writing this code. Any security concerns are noted above.
• I have commented my code in hard-to-understand areas, if any
• I have made needed changes to the README
• My changes generate no new warnings
• If I added a third party module, I included a rationale for doing so and followed our current guidelines

[codeclimate]

Code Climate has analyzed commit 7a9de42 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (100% is the threshold).

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

[caiosba]

@jayjay-w , code looks good, and I also executed the two tests you implemented and they look good as well. Just one thing I noticed, this is why I'm leaving this review as "comment"... for both cases (email exists and email doesn't exist), we return the "Please check your email" message, but with different design/behavior from Check Web:

User doesn't exist:

User exists:

Can you please:

1. Check if there is a way to unify that from the backend side?
2. If not, file a ticket for the frontend work that is needed?

[jayjay-w]

Thank you for your review, @caiosba. I checked the implementation and this will need more work on the front end side to change the behavior.

This is because:

1. When the user account creation is successful, we navigate to the 'Please check your email' page in the first screenshot above.
2. When the email is already taken, we stay in the sign up form and display the error message.

In the current set up, an attacker would know whether an email exists or not, just by the behavior of the pages, even if we change the messages to be uniform.

I will open a ticket so that we can discuss on the best way to update the workflow.

[caiosba]

@jayjay-w , what's the difference in the API response between these cases?

[jayjay-w]

In the API layer, the responses are the same (401 error with a message for the user to check their email)

The difference is in the payload. When a registration is successful, we return the new User object in the response. So the question is whether we should change the successful response to not return the user object. If we do this, then the two scenarios will behave the same, however we will have lost the current UX for successful sign ups.

[caiosba]

The difference is in the payload. When a registration is successful, we return the new User object in the response. So the question is whether we should change the successful response to not return the user object. If we do this, then the two scenarios will behave the same, however we will have lost the current UX for successful sign ups.

@jayjay-w I think we should make the response be exactly the same for both cases... do you see any issues with that?

[jayjay-w]

The difference is in the payload. When a registration is successful, we return the new User object in the response. So the question is whether we should change the successful response to not return the user object. If we do this, then the two scenarios will behave the same, however we will have lost the current UX for successful sign ups.

@jayjay-w I think we should make the response be exactly the same for both cases... do you see any issues with that?

No that should be OK, other than the fact that a user who has successfully signed up will still see the form with an error message.

As discussed, we will change the approach to return a 200 response in the case where the user signs up successfully and where the only error is an email that's already in use.

[caiosba]

The difference is in the payload. When a registration is successful, we return the new User object in the response. So the question is whether we should change the successful response to not return the user object. If we do this, then the two scenarios will behave the same, however we will have lost the current UX for successful sign ups.

@jayjay-w I think we should make the response be exactly the same for both cases... do you see any issues with that?

No that should be OK, other than the fact that a user who has successfully signed up will still see the form with an error message.

As discussed, we will change the approach to return a 200 response in the case where the user signs up successfully and where the only error is an email that's already in use.

Thanks, I'm going to add a comment to the ticket with the other things we discussed.

[caiosba]

Great work, @jayjay-w ! I tested locally that for both cases (email exists and email doesn't exist), the error message is the same and the request response is the same too. I also confirmed that the password error is displayed and that users are still able to register and login. Before merging, please fix the Code Climate issue and make sure that all tests are passing and that the code is 100% covered.