Catch smtplib.SMTPRecipientsRefused w. form feedback #12

Open
dokterbob opened this Issue Sep 23, 2011 · 6 comments

Comments

Projects
None yet
2 participants
Owner

dokterbob commented Sep 23, 2011

See: #77 (comment)

Currently, all exceptions in transactional emails (activation, unsubscribe, update etc.) are caught and logged. Furthermore, an error sets the error context variable to true.

Ideally, we would simply catch SMTP-specific errors and make them result in the form not being valid (hence sending the email from the Form.save() functions). This makes it a lot harder to make mistakes in editing the templates, you simply display form errors like you would normally.

Furthermore, we should let all other errors to simply pass through and cause a 500, since this results in unpredictable behaviour.

Owner

dokterbob commented Nov 19, 2012

@adys: Had forgotten about this long-standing error report. Does the proposed solution here make sense to you?

Contributor

jleclanche commented Nov 19, 2012

In theory it sounds good - in practice, it might result in performance issues or hangs when submitting. Would have to test.

Owner

dokterbob commented Nov 19, 2012

@adys As the method I have in mind simply checks the existence of the domain, the check moves quite fast and (to me) doesn't seem too likely to cause a DDoS vector.

Example code: https://gist.github.com/876648 (requires dnspython)

Contributor

jleclanche commented Nov 19, 2012

It's definitely a DDoS vector, and it adds a dependency. Really sounds out of scope for the project, tbh. Is there a way to move this to a more controllable environment? eg. is it possible for the developer to replace the view that sends mail by one that does additional error checks?

Owner

dokterbob commented Nov 19, 2012

I agree with you on the dependency, it was mostly meant as an example. However, as this view is normally meant to send e-mail, doing an extra DNS-lookup seems to me harmless (in comparison).

A 'pluggable' view (or view-part) seems a bit complicated. However, something like a dynamic validators setting could solve this issue.

Something like a setting: NEWSLETTER_EMAIL_VALIDATORS

It would be lightweight, require no hacking of any kind and allows for a nice cleanup of code. Makes sense?

Owner

dokterbob commented Sep 10, 2013

It seems this has not been fixed in #77, considering this comment. SMTP refused is now caught causing error=True in the template rather than a ValidationError.

@dokterbob dokterbob added a commit that referenced this issue Oct 3, 2013

@dokterbob dokterbob Catch email related exceptions only.
Related to #12.
57620b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment