enqueue spam/dmarc failing emails instead of hiding

[LeoMcA]

This is how an email which fails DMARC, for instance, appears (mostly through previous work from @eviltrout):

At the moment this doesn't show the reason that this topic needs approval, and I think in the DMARC case at least, this is important (otherwise you'll be left wondering why a post from an admin, for instance, is in the reviewable queue). But this reason is already stored in the reviewable score, so @eviltrout I wondered if you'd already had thoughts about exposing this.

[discoursebot]

You've signed the CLA, LeoMcA. Thank you! This pull request is ready for review.

[eviltrout]

I think if you're adding a new reason for adding a post to the queue, you only need to add a translation for that reason to have it displayed in more detail:

https://github.com/discourse/discourse/blob/master/app/serializers/reviewable_score_serializer.rb#L22

[LeoMcA]

A-ha! That's very nice!

[davidtaylorhq]

@LeoMcA looks like the test failures are showing something legit here

Psych::SyntaxError: (/home/runner/work/discourse/discourse/config/locales/server.en.yml): found unknown escape character while parsing a quoted scalar at line 4659 column 31

I don't think you need to escape the single quote in the translation.

[eviltrout]

Could we default this to off for now? It seems a bit safer. Then we can give it a try and if it all works nicely we can default it on.

[LeoMcA]

I don't think you need to escape the single quote in the translation.

Oh yeah... for some reason Atom's syntax highlighting bugs out with an unescaped quote, so I assumed it needed to be escaped without checking. Odd that that exception isn't thrown locally for me, though.

Could we default this to off for now?

@eviltrout Which aspect of this you thinking? The spam checking element of this has existed as a feature for ages, so I wouldn't want to effectively turn that off on instances that have enabled it. The authentication results checking was added much more recently - but it's possible some instances are already relying on it.

[ghost]

DeepCode's analysis on #e42505 found:

• ❌ 0 critical issues. ⚠️ 0 warnings and 0 minor issues. ✔️ 0 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.

---

☺️ If you want to provide feedback on our bot, here is how to contact us.

[LeoMcA]

Any update here? We have near-daily phishing emails coming through which are marked as spam by SES or fail dmarc, but which still trigger email notifications to users because the current behaviour of posting + hiding doesn't prevent email notifications being sent.

[davidtaylorhq]

I think we would like to have the whole DMARC feature disabled by default for now. That matches the 'disabled by default' behaviour of the old spam detection site setting. The only DMARC failures we've had hitting our meta inbox are 'real' customers with misconfigured email servers, so this is just causing confusion on our end. Any actual spam emails are being filtered upstream by google.

What do you think about disabling the feature completely when email_in_authserv_id is blank?

Aside: how do the 'well known values' in the meta post get used? As far as I can tell, when email_in_authserv_id is blank, we read all Authentication-Results headers, and take the worst one. I can't see any specific checks for amazon/google.

[LeoMcA]

I think we would like to have the whole DMARC feature disabled by default for now. That matches the 'disabled by default' behaviour of the old spam detection site setting. The only DMARC failures we've had hitting our meta inbox are 'real' customers with misconfigured email servers, so this is just causing confusion on our end.

Fair enough, I'll change the behaviour when email_in_authserv_id is blank from "if there's a failing header, observe it" to "just do nothing". I made that decision from a naïve security perspective (where everyone has their email servers set up correctly!)

Aside: how do the 'well known values' in the meta post get used? As far as I can tell, when email_in_authserv_id is blank, we read all Authentication-Results headers, and take the worst one. I can't see any specific checks for amazon/google.

Those 'well known values' are for users to paste into the email_in_authserv_id setting if they know they're using amazon/google, so they don't have to grep through their received emails. We can't hard code them in, since, say a user's using amazon as their email receiver, an attacker could then include a Authentication-Results: mx.google.com; header in their email, and amazon wouldn't strip it out (because it's not mx.google.com).

[davidtaylorhq]

where everyone has their email servers set up correctly!

I wish that were the case! It would make this so much easier!

Those 'well known values' are for users to paste into the email_in_authserv_id

👍 makes sense, thanks for clarifying

[davidtaylorhq]

This looks good to me now. Will let @eviltrout give it a final check for review-queue things

[LeoMcA]

+1 makes sense, thanks for clarifying

The meta topic could probably do with a clarification (something like "Paste these values into your email_in_authserv_id setting:" under the "Well known values" header) but I don't have permission to edit the post.

[eviltrout]

Great, thanks for the legwork @LeoMcA - I've merged it now.