[#5816] Added more tests to maildrop Script and fixed REJECT action
| Summary | Added more tests to maildrop Script and fixed REJECT action |
| Queue | Ingo |
| Queue Version | HEAD |
| Type | Enhancement |
| State | Resolved |
| Priority | 2. Medium |
| Owners | Chuck Hagenbuch <chuck (at) horde (dot) org> |
| Requester | horde (at) nospam (dot) obeliks (dot) de |
| Created | 10/18/2007 (206 days ago) |
| Due | |
| Updated | 12/27/2007 (136 days ago) |
| Assigned | |
| Resolved | 12/27/2007 (136 days ago) |
| Attachments | horde-ingo_maildrop-support.diff  |
| Milestone | |
| Patch |
State ⇒ Resolved
Okay, I've removed the blacklist/whitelist changes for now, which makes the main bit of this ticket resolved. If you have a chance later to submit the black/whitelist changes in the future, that'd be great!> We're getting much closer to release. Any updates on a patch?
I'm sorry, my time is quite limited right now, since I'm very near to the deadline of a project myself. I certainly can't provide a solution for a more sophisticated expression model, like for example in Sieve support.
Improving support for black/whitelist isn't that hard in concept though. It would be best not to match against a header value, but against the predefined $FROM-Variable (cp. http://www.courier-mta.org/maildrop/maildropfilter.html) that contains the sender _address_ only.
This would however require a condition that isn't based on regular expressions being matched against the header lines, but a condition like ($FROM eq "address1"), which is why I though creating a more flexible recipe/condition model would be the right way to go.
To work around that, the easiest and most elegant way would probably be to create a new Maildrop_ListRecipe class, that inherits from Maildrop_Recipe and overrides addCondition() as well as generate() (the latter can be copied nearly 100%, just the condition->text transformation has to be altered -- maybe it would be better to change it in the original Maildrop_Recipe and only have to change both addCondition() implementations).
I'm sorry I can't provide a patch right now, maybe I can help out for the next release. If nobody can be found doing this work until the next release, it would probably still be best to revert to the old "contains"-Condition for While/Blacklists.
We're getting much closer to release. Any updates on a patch?Priority ⇒ 2. Medium
State ⇒ Feedback
> I recently noticed why 'contains' was used so far... because the
> From: header mostly consists of more than just the sender address.
> Using 'is' surely isn't the right solution, the proper way to do this
> is using maildrops getaddr() on the From header and checking each
> listed email address...
> I'll take a shot at this when dealing with the changes I mentioned in
>
bug 5843.>
> If a release is due, maybe you should switch back to 'contains' for
> time being. Otherwise just wait for my patch ;)
We're near an RC, but not a final release. There should really not be multiple addresses in From:, but I see the point about getaddr. I'll wait for (or look into myself, though that might be wishful) a patch for now.
Thanks for all the help on maildrop support!
> I missed one thing: imho, white- and blacklist should use 'is' as
> match type, not 'contains'... if I blacklist "one@domain.com", I
> don't want to blacklist "someone@domain.com" in one go...
I recently noticed why 'contains' was used so far... because the From: header mostly consists of more than just the sender address. Using 'is' surely isn't the right solution, the proper way to do this is using maildrops getaddr() on the From header and checking each listed email address...
I'll take a shot at this when dealing with the changes I mentioned in
bug 5843.If a release is due, maybe you should switch back to 'contains' for time being. Otherwise just wait for my patch ;)
Example added to backends.php.dist.>> In a standard courier-mta setup with maildrop filtering, the
>> resulting filter script becomes invalid due to the leading "INBOX."
>> within the path. strip_inbox is actually needed in this case.
>
> And so strip_inbox is configurable... or are you suggesting a change
> in one of the .dist files? I'm confused, sorry.
It would be nice to have this option shown at least as an example within one of the dist files . Otherwise I would suggest as setting it to true by default, to not break existing installations. Great work, btw :)
> In a standard courier-mta setup with maildrop filtering, the
> resulting filter script becomes invalid due to the leading "INBOX."
> within the path. strip_inbox is actually needed in this case.
And so strip_inbox is configurable... or are you suggesting a change in one of the .dist files? I'm confused, sorry.
In a standard courier-mta setup with maildrop filtering, the resulting filter script becomes invalid due to the leading "INBOX." within the path. strip_inbox is actually needed in this case.
These changes will be in Ingo 1.2.State ⇒ Resolved
Assigned to Chuck Hagenbuch
Committed to HEAD (only very minor rejects, easily fixed), thanks!> Fixes should always be for HEAD and then backported, unless the
> problem doesn't exist in HEAD, but I'll take a look. Thanks for the
> updated patch.
I just applied my changes to the file I had on my system, but I'll diff against HEAD next time... but since not much changed in the maildrop.php except for vacation support, the patch might even succeed with some fuzziness ;)
Fixes should always be for HEAD and then backported, unless the problem doesn't exist in HEAD, but I'll take a look. Thanks for the updated patch.New Attachment: horde-ingo_maildrop-support.diff
> Hmm - I get this trying to apply the patch:
>
> Maya:/var/www/horde/ingo/lib/Script chuck$ patch <
> /Users/chuck/Desktop/horde-ingo_maildrop-support\[2\].diff
> patching file maildrop.php
> patch: **** malformed patch at line 21: @@ -181,7 +190,7 @@
I'm sorry, I shouldn't have tried to manually edit the diff file ;) Here comes a new version, straight from `diff`. Fixes one typo too (missing comma).
Keep in mind that it's patched against 1.1.2 though
State ⇒ Feedback
Hmm - I get this trying to apply the patch:
Maya:/var/www/horde/ingo/lib/Script chuck$ patch < /Users/chuck/Desktop/horde-ingo_maildrop-support\[2\].diff
patching file maildrop.php
patch: **** malformed patch at line 21: @@ -181,7 +190,7 @@
New Attachment: horde-ingo_maildrop-support[2].diff
My apologies, I did it again ;)
Here comes the second update to the original patch, I've added "matches", "not matches", "exists" and "not exist" tests.
I also have a nice idea on how to implement relational tests, but that would require more restructuring, which is why I'll probably wait until I have a more recent version of the original file on my system.
Body and Size tests should be easy too.
Updated patch attached.
New Attachment: horde-ingo_maildrop-support[1].diff
I missed one thing: imho, white- and blacklist should use 'is' as match type, not 'contains'... if I blacklist "one@domain.com", I don't want to blacklist "someone@domain.com" in one go...
Another thing I didn't mention before: I changed the DISCARD action from "to /dev/null" to "exit", which looks cleaner to me.
Updated patch attached.
Summary ⇒ Added more tests to maildrop Script and fixed REJECT action
State ⇒ New
Priority ⇒ 1. Low
Type ⇒ Enhancement
New Attachment: horde-ingo_maildrop-support.diff
Queue ⇒ Ingo
This patch addresses three issues (all are still relevant in cvs, but the patch was generated against 1.1.2):
1. It adds the "is" test, as well as all negated versions of the currently available tests (i.e. 'not contain', 'is', 'not is', 'not begins with', 'not ends with') by prefixing the patterns with "! ".
2. It fixes the REJECT action for maildrop scripts. The current version uses "EXITCODE=reject reason", which is bogus, because maildrop needs a numerical exit code value. In order to pass a reject reason to the MTA, the latter must support reading it from the program output (e.g. postfix >= 2.3).
My code sets EXITCODE=77 (EX_NOPERM) and echoes "5.7.1 Reject Reason" to stdout before exiting. This way, if the MTA supports reading RFC3463 Status Codes from program output, it will provide the intended error message, otherwise it should still reject the message permanently.
fyi: 5.7.1 is the status code Postfix uses to reject messages (if no other code is given)
3. The current Maildrop script strips a leading "INBOX." from path names, which might be correct if INBOX is used as the root of the Maildir. However, IMAP servers like Dovecot support having root folders next to the INBOX. In such setups, the current code fails completely.
Therefore, I introduced another backend-parameter, strip_inbox, which let's the admin configure this behaviour. Maybe a more elegant approach would be to introduce a third mailbox format ("mbox", "maildir", "maildir-inbox" or similar).
The code has been tested briefly, and has so far created no problems on my system (postfix+dovecot+custom maildrop script).
hth, Bernhard Frauendienst