Bug 7656

Lots of talk here too that I haven't digested yet..
https://bz.apache.org/SpamAssassin/show_bug.cgi?id=7022

(In reply to Henrik Krohns from comment #0)
> Unless people want to use multiple rules to match non-utf8 and utf8
> messages, perhaps the only sane solution would be to "upgrade" all non-utf8
> rules to utf8 internally and do the matching to utf8 upgraded body. In such
> case the two rules above would actually be duplicates and work on any
> message.

Basically with this I mean that normalize_charset should affect rule parsing too and encode the rules (and resulting regexes) to UTF8? I don't think we can simply tell users to "convert all your rules/files to UTF8, if you want them to work". I don't use UTF8 in my editors or Linuxes anywhere. :-)

(In reply to Henrik Krohns from comment #0)
> latin1 message, no ct     RULE_LATIN1 / <no hits>
> latin1 message, utf8 ct   RULE_LATIN1 / <no hits>
> latin1 message, no ct     RULE_UTF8 / <no hits>
> latin1 message, utf8 ct   RULE_UTF8 / <no hits>

Ok these should be now fixed..

Basically Encode::Detect::Detector thinks body "päivää" is Windows-1255 (Hebrew!!). 

dbg: message: failed decoding as declared charset UTF-8
dbg: message: decoded as detected charset windows-1255, declared UTF-8

Why are we using a module that hasn't been updated in 10 years anyway? Maybe look at Encode::Guess which has been in core atleast from 5.8.8?

I simply added latin diacretic letters to SA's own basic Win-1252 detection. I borrowed the \xc0-\xd6\xd8-\xde\xe0-\xf6\xf8-\xfe bit from textcat, also looking at https://en.wikipedia.org/wiki/Windows-1252 it seems correct. Not sure if the missing ÿ (\xff) should be added to here and textcat..

Sending        spamassassin-3.4/lib/Mail/SpamAssassin/Message/Node.pm
Sending        trunk/lib/Mail/SpamAssassin/Message/Node.pm
Transmitting file data ..done
Committing transaction...
Committed revision 1846805.

So getting back to this.

I've been running my SA with normalize_charset 1 without any ill-effects so far. Should we head towards activating it by default in 4.0.0?

Only thing left after that would be documenting what format .cf files are expected to be in. Probably just "bytes" without any special encoding? For anything else than personal use, pure ascii should be used for portability (non-ascii characters should be in \xff format).

To be compatible for both normalize_charset 0/1, it should be clearly documented that any rules expected to hit latin1 extended characters would need to be written to include both latin1/utf8 - "ä" -> (?:\xe4|\xc3\xa4). We could also detect this automatically from rules and output warning that it should be fixed.

One thing to consider would be removing the whole normalize_charset option, and just force everything normalized, plain and simple.

I tried performance tests with mass-check, there's absolutely no difference here for normalize_charset, total duration was always within normal +-2% variance.

Rule differences between these were mainly:

__HIGHBITS
MPART_ALT_DIFF_COUNT
TVD_SPACE_RATIO
__freemail_safe_fwd

As we can see from __freemail_safe_fwd, if normalize is on, we can't assume that a single dot will match a character like "ä".. committed (?:\xe4|\xc3\xa4) fix for it. Question arises whether regexes should be run with unicode semantics (. = single character) instead of matching raw bytes.

Have to investigate if the others need fixing.

I'm a 0 on this.  I haven't see this proposed for default on dev@ or users@ and would like to see that done.  I know I have some rules that fire differently with normalize_charset.

(In reply to Kevin A. McGrail from comment #6)
> I know I have some rules that fire differently with normalize_charset.

Could you show some examples?

Sure, here's one example:

#ZWNJ
#ZWNJ 200C 157 https://en.wikipedia.org/wiki/Windows-1256
# Also want to look at Unicode U+200C. 
# Also 'zero-width joiner' which is Windows-1256 0x9E and Unicode U+200D. $a

# Per RW, switching for this to work with 'normalize_charset 1', \x9d needs to be replaced with (?:\x9d|\xe2\x80\x8c)
mimeheader      __KAM_ZWNJ1     Content-Type =~ /charset.+windows-1256/i
body            __KAM_ZWNJ2     /(?:\x9D|\xe2\x80\x8c)/
tflags          __KAM_ZWNJ2     multiple maxhits=16
body            __KAM_ZWNJ3     /\&\#x200B;/i

Well yes that pretty much sums up what was already said in this bug. You can't expect to match extended ascii characters like before. It's nothing but a documentation issue.

Well for it to be the default in 4.0.0, I'd like it to be discussed on list, please.

This bug already floods dev@ list, if someone wants to chime in, feel free.

I have no intention of spending time posting on users@ at this stage, when it's still only on idea and much left to do. Developers are the ones who need to steer this ship.

Bumping this bug. Comments? Monologs are getting a bit tiresome.. :-)

I am +1 to enable "normalize_charset 1" on 4.0.0 by default,
I have it enabled for a long time in production without any issues.

Good to hear, I cast my official +1 for normalize_charset 1 too.

There doesn't seen to be any dependencies, Encode::Detect can still remain optional and required HTML::Parser 3.46 is from 2005..

Will check if there's anything in tests that should be changed.

(In reply to Henrik Krohns from comment #12)
> Bumping this bug. Comments? Monologs are getting a bit tiresome.. :-)

+1

The minor pain of revamping rules that match non-ASCII characters is compensated by the fact that this is a *normalization* and so reduces the frequency of edge cases that escape rules written (perhaps inadvertently) to depend on a particular subset of possible encodings. My personal experience running SA instances that see a lot of non-ASCII messages is that enabling normalize_charset is a best practice, and the default is basically tech debt. 

As for requiring discussion on-list, these comments are sent to the dev list. I'm going to bump it there to get the attention of anyone filtering out Bugzilla mail (!? if that's a thing...) and will also post on the Users list to get a broader audience.

Changed normalized_charset 1 as default and added some docs.

Sending        trunk/UPGRADE
Sending        trunk/lib/Mail/SpamAssassin/Conf.pm
Sending        trunk/lib/Mail/SpamAssassin/Util/DependencyInfo.pm
Transmitting file data ...done
Committing transaction...
Committed revision 1890317.

This was sufficiently resolved with the previous commit. No changes into how rules/cf-files and body are processed, it would be too complicated for now and potentially backwards breaking.