dburkart / check-sieve

Syntax checker for mail sieves.
MIT License
35 stars 7 forks source link

Add support for Proton Mail sieve extensions #54

Closed johnlettman closed 1 year ago

johnlettman commented 1 year ago

This change implements support for:

Accompanying tests are included in this commit.

johnlettman commented 1 year ago

Ah, whoops. I am missing the hasexpiration and expiration test implementations. I will take care of those tomorrow.

johnlettman commented 1 year ago

I'm having trouble with the parsing of the IF block.

I suspect the code is looking for the conditional + 1, for example:

if conditional :arg { }

Where I have a unique case require no arguments to the conditional:

if conditional { }

I suppose this is a great opportunity for me to learn Bison. :sweat_smile:

dburkart commented 1 year ago

What's the example sieve you're trying to get working?

dburkart commented 1 year ago

Oh I see, you're talking about the failing test:

            require "vnd.proton.expire";

            if hasexpiration {
                stop;
            }
johnlettman commented 1 year ago

Oh I see, you're talking about the failing test:

Yep! That's correct. On my branch, it hits the unexpected token error for {.

dburkart commented 1 year ago

Looking at the grammar in RFC 5228, it looks like we should allow a test with no arguments!:

https://www.rfc-editor.org/rfc/rfc5228#section-8.2

I can push something up to your branch, it should be an easy fix.

johnlettman commented 1 year ago

It seems to fit the use case Proton is using. I would greatly appreciate that, thank you kindly! :)

dburkart commented 1 year ago

Should be fixed now. LMK when you're ready for a review :^)

johnlettman commented 1 year ago

Fab! I am ready for review @dburkart.

I did need to modify a bit of core code here:

_required_capabilities->find(ASTString("variables")) != _required_capabilities->children().end()

The condition checking for the presence of "variables" appears to do so only in a single require call. As an example:

# This would work
require ["variables", "vnd.proton.eval"];
set :eval "a" "asdf";

# This would fail
require "variables";
require "vnd.proton.eval";
set :eval "a" "asdf";

From the available literature, both approaches in sieve code should work. To fix this, I kept the original check but included a call to requires_capability(), since this accesses a map of previously required extensions. The resulting function is has_required(): https://github.com/dburkart/check-sieve/pull/54/commits/3c8cb5d8f00588e2daac89718d8437668946ec35#diff-0f0ac58248317c33b2377f658ec096a2f4630709b267ad6e45a02c42093fbbd9

Otherwise, all tests pass, and the new extensions work.

Thank you so much for the Bison help!