griddler-mandrill
griddler-mandrill copied to clipboard
wingrunr21
SPF validation
I just wanted to ask why you added the SPF validation to the adapter? I think it is mandrill's responsiility to filter SPF. And if Mandrill sends the email, then griddle would not have to validate it.
Maybe Im wrong but at least it could have been optional or configurable, right? Can I do a MR with configurable this SPF validation?
@arunthampi needed SPF validation and Mandrill wasn't doing it. They submitted a PR (#22) and I merged it.
I'm not against a configuration option, but do want to ask what about SPF validation is causing you issues?
We have few domains that dont have SPF record but are valid email servers. And since version 1.1.3 we are not able to deliver emails to them. We had to downgrade to version 1.1.2. Or maybe there is missing a condition || event[:spf][:result] == 'none' ? since Mandrill sends within these emails:
"spf":{"result":"none","detail":""}
Thanks in advance!
hmm i'd rather have the option to check for spf than using event[:spf][:result] == 'none'. I confirmed that the webhook does get called for spoofed emails and that's pretty dangerous imo because almost all applications use email as a way of identifying users.
My preference would be to have strong security defaults and let users override it at their own risk.
In hindsight, I should have bumped this change to v1.2.x instead of leaving it on v1.1.x. My apologies for that.
I too feel that strong security defaults are preferential. However, I want to make sure the adapter is still usable in non-SPF environments. Will you get a 'none' result for a spoofed email? Maybe we should be looking for positive identification of spoofing (instead of negative)?
However, I want to make sure the adapter is still usable in non-SPF environments.
That's what @ciihla mean - now we are not able to receive emails from domains that doesn't have SPF record.
If we want to make this configurable, we could add something like adapter_config to Griddler::Configuration or just expose our own config object (seems a bit silly though).
From there, we could add something like spf_filter. The default could be [ 'pass', 'neutral'] so that we get our strong defaults, but people who need changes can do something like setting that to nil or changing the whitelisted result strings.
That would be ideal. Regarding the question whether to use Griddler::Configuration or own config:
I would go with own config. Because otherwise we would force other adapters to implement the exact SPF check method. What do you think?
Shall I prepare a PR? Or will you do it? Thanks
We wouldn't force anything. Adding the key to Griddler::Configuration would happen on the griddler side, but it would just be an arbitrary hash. The individual adapters would then just look for whatever they want within that key.
I can prepare the PR.
I just thought that functionality of one gem would depend on configuration of other gem, but maybe I understand it wrong and its okay.. Anyway thank you for your willingness!
This gem already depends on the functionality/configuration of griddler so I don't really see this option as being worse than what is already being done. To me it seems strange to have a config object for a dependency of the parent. IMO you should be doing all config through the parent.
I'm also considering that the other adapters may have a use case for this.
If you pointed that its a de-facto a parent, you are right. It makes sense to me too now. Although both options make sense to me :)
On the other hand: if griddle changes the DSL how it creates an adapter or passes options to them then it would be needed to change all adapters now, right?
Depends. You can check the arity of new and conditionally pass things to the method. Griddler can also bump their version to reflect the API change and the adapters can depend on the lower version until they make their changes.
:+1:
griddler is also in favor of a version bump. I'll put together the PR for them hopefully today.
If nothing else, it would be nice if the adapter handled SPF validation fails better. It's a completely silent error, responding 200 OK. I had to delve through the source to find out why some of my emails were being silently ignored.
Sorry, I haven't had time to dig into this yet. It is on my todo list but you know how it is...
@jalada You are welcome to submit a PR
For anyone else who had emails mysteriously disappear, there is a fork to have a more lax validation instead of #22: https://github.com/vijedi/griddler-mandrill
@tshakah that fork essentially disables SPF checks. Allowing all incoming outside of direct failures will allow several types of spam through.
To everyone watching this issue: does defaulting to pass, neutral, or none sound good? Would it be preferable to turn off SPF validation by default and have it be opt-in (I am less fond of this change since Mandrill calls the webhook for all kinds of SPF validation)?
Note that no matter what I do for this change, the SPF result whitelist will be configurable.
Well, it sets it to pass so long as the SPF isn't fail, which is what I need (some of the valid emails I get are softfail and I have no control over that). Apologies, I should have been more specific in my earlier comment.
I'm happy with a configurable whitelist, defaulting to one of the options you suggested.
Ok, that makes more sense.
I've got the configuration changes done locally. I'm checking backwards compatibility with griddler (so you won't have to run a git version) and also figuring out a workaround so that I can push this change independently of the griddler change for per-adapter configs.
Hi, is there any update on this issue ? Are you going to merge the mentioned branch into master ? Or is there something else you would like to do first, in which case I may be able to help if you provide directions.
It won't work without the griddler side of things done. I need to work on that and prep the adapter maintainers for the changes prior to this.
What about accepting none spf result for now ? Something like that ? https://github.com/wingrunr21/griddler-mandrill/compare/master...Uelb:spf_validation