Bug 7848 - Rule parser doesn't support nested if/ifplugins.
Summary: Rule parser doesn't support nested if/ifplugins.
Status: RESOLVED FIXED
Alias: None
Product: Spamassassin
Classification: Unclassified
Component: spamassassin (show other bugs)
Version: 3.4 SVN branch
Hardware: PC Linux
: P2 normal
Target Milestone: 4.0.0
Assignee: SpamAssassin Developer Mailing List
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-07 14:43 UTC by matthias
Modified: 2021-04-15 17:16 UTC (History)
3 users (show)


Attachment Type Modified Status Actions Submitter/CLA Status
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description matthias 2020-08-07 14:43:49 UTC 
When then Mail::SpamAssassin::Plugin::WLBLEval module is not loaded the linter fails in 60_whitelist.cf with:

Aug  7 16:34:50.947 [22409] warn: rules: error: unknown eval 'check_from_in_whitelist' for USER_IN_WELCOMELIST
Aug  7 16:34:50.986 [22409] warn: rules: error: unknown eval 'check_to_in_whitelist' for USER_IN_WELCOMELIST_TO

I assume that's an issues in the parser, because it reverts $skip_parsing unconditionally when reaching the first "else" while it should be still disabled because of the "ifplugin".
Comment 1 Kevin A. McGrail 2020-08-07 16:45:59 UTC 
Thanks Matthias.  What version of SA are you using?

Does this warn cause sa-update not to install updates or a lint to fail?
Comment 2 matthias 2020-08-07 17:32:29 UTC 
As far as I can tell, just the lint fail.
Comment 3 Kevin A. McGrail 2020-08-07 17:42:18 UTC 
Can you confirm sa-update -D that you are have the latest rules and run spammassassin --lint && echo 'Worked' and tell me the output, please?

That will tell me if it's failing or just a warning.
Comment 4 matthias 2020-08-10 06:37:16 UTC 
Hi Kevin,

the exit code is 0, it's just warning.

I guess the error is here:

https://github.com/apache/spamassassin/blob/trunk/lib/Mail/SpamAssassin/Conf/Parser.pm#L360

the $skip_parsing is negated when the parser finds any 'else'.
The structure of 60_whitelist.cf is as follows:

ifplugin Mail::SpamAssassin::Plugin::WLBLEval //skip_parsing=true
   ...statements...
   if can(Mail::SpamAssassin::Conf::feature_blocklist_welcomelist) //skip_parsing=true
      ...statements...
   else //skip_parsing=false, reverted skip_parsing
     header USER_IN_WELCOMELIST  eval:check_from_in_whitelist() // (warning)
     ...statements...
   endif
....
Comment 5 Kevin A. McGrail 2020-08-11 02:11:54 UTC 
I can replicate this issue by commenting WLBLEval plugin in v320.pre

sa-update && echo 'worked'
rules: error: unknown eval 'check_from_in_whitelist' for USER_IN_WELCOMELIST
rules: error: unknown eval 'check_to_in_whitelist' for USER_IN_WELCOMELIST_TO
worked

spamassassin --lint && echo 'worked'
Aug 10 19:36:26.417 [21876] warn: rules: error: unknown eval 'check_from_in_whitelist' for USER_IN_WELCOMELIST
Aug 10 19:36:26.806 [21876] warn: rules: error: unknown eval 'check_from_in_list' for __FROM_INTERNALSUMMER
Aug 10 19:36:26.807 [21876] warn: rules: error: unknown eval 'check_to_in_whitelist' for USER_IN_WELCOMELIST_TO
Aug 10 19:36:26.808 [21876] warn: rules: error: unknown eval 'check_to_in_list' for __TO_INTERNALSUMMER
worked

I checked with 4.0.0 trunk and with 3.4.5 pre1.  

In both cases you get output but the sa-update works and the lint passes.

We need to fix this because it is noisy but it does not break anything.
Comment 6 Henrik Krohns 2021-04-15 17:16:30 UTC 
Sad case of parser. Just have to remember not to use nested if-if-else clauses anywhere for a while.

Committed fix to 3.4 too just in case we release something.

Sending        spamassassin-3.4/MANIFEST
Sending        spamassassin-3.4/lib/Mail/SpamAssassin/Conf/Parser.pm
Adding         spamassassin-3.4/t/if_else.t
Sending        trunk/MANIFEST
Sending        trunk/lib/Mail/SpamAssassin/Conf/Parser.pm
Adding         trunk/t/if_else.t
Transmitting file data ......done
Committing transaction...
Committed revision 1888798.