Increasing emphasis on clean_email() when adding custom account Adapter

[banagale]

I noticed an exception related to what appeared to be pentesting reconnaissance on a production sign up form.

This app has a custom user account adapter but had failed to include overriding the clean_email() method with basic checking of the validity of the email.

It seems like at a minimum the DefaultAccountAdapter should include django.core.validators.validate_email.

As is, this method acts largely as a pass with a comment

You can hook into this if you want to (dynamically) restrict what email addresses can be chosen.

This seems to imply its use is largely for say, handling known spammy domains.

Is it correct that even basic validation like using django's built in validate_email must be added? It appears without this, by default a setup of AllAuth that includes a child of DefaultAccountAdapter might be opening itself up to potentially dangerous user input.

If this is all correct, would it be reasonable to call django's packaged validate_email in that method by default? Or perhaps at least increase the emphasis on needing to do basic email validation in the docs and potentially the comment for that method?

[pennersr]

Making sure that an email is a valid email is essential and therefore not something that you should be able to influence with custom adapter logic. That's why the adapter allows for extra validation, but the core validation is in place by default (e.g. by making use of forms.EmailField and so on).

Having said that, I am aware of one place where an email address was being looked up without validation. As this is only a lookup (not a write) it does not pose a security risk, but, in case you put in email addresses with NUL (0x00) characters that does trigger a crash report with is definitely annoying. This is fixed here: 5fe150b0

Now, without having seen your exception I cannot tell if this also fixes your issue. Can you please show the exception?

[banagale]

Thank you for this feedback, Raymond. It didn't seem like this should be the only place email should be validated.

The error does go back to the app's signup form implementation which improperly used a CharField for the email.

This was allowing an email address, ${(843897959+840556688)?c} to get through the application all the way to an overriddensend_mail() from the adapter.

The form in question also does not inherit from AllAuth's signup form, but forms.Form. I'm not sure why at this moment.

I can provide more details, if still helpful, but otherwise I have enough to go on here to see I was barking up the wrong tree for this issue and it may be closed.

[pennersr]

Okay, then the issue you are experiencing is beyond scope of allauth, closing.

[banagale]

Yep. Thanks, again.