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

Prevent information leakage on registration when using PowEmailConfirmation #350

Merged
merged 1 commit into from
Jan 5, 2020

Conversation

danschultzer
Copy link
Collaborator

@danschultzer danschultzer commented Dec 7, 2019

This adheres to OWASP recommendation to show the default confirmation required message when a user attempts to register with an already taken e-mail, but otherwise has valid params.

This to some degree prevents information leakage about an accounts existence as discussed in #238 (comment). I will also implement the same logic for authentication with invalid credentials for existing user to fully prevent fully any information leakage.

I'm not completely convinced that the controller callbacks should have this expectation of the changeset map since I try to make any Phoenix modules just pass along whatever is sent to them without caring what it is to have clear separation of responsibility. I kind of wish I could handle it at the Ecto level, but it really only makes sense to have this logic in the controller callbacks to figure out the correct response based on error type. In PowAssent I've let the context method return an error that could be acted upon. It might be difficult to implement this with how the Pow context works though, since there is only extension callback in the changeset, not for context methods.

One doubt I have about the logic is that this only checks the email taken. What if you have both email and username? In that case it should probably check both the user id and email. But what if there is an additional unique field defined by the user? I'll think about this and figure out what is appropriate.

Edit: I've changed the logic so the success message is always shown when email has been taken disregarding any other errors there may be.

@danschultzer danschultzer merged commit 6fe2d19 into master Jan 5, 2020
@danschultzer danschultzer deleted the generic-account-registration-response branch January 5, 2020 01:14
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

Successfully merging this pull request may close these issues.

1 participant