Skip to content

detect K-9 signatures - #199

Open
sblondon wants to merge 1 commit into
mailgun:masterfrom
sblondon:detect-minus-minus-space-signature
Open

detect K-9 signatures#199
sblondon wants to merge 1 commit into
mailgun:masterfrom
sblondon:detect-minus-minus-space-signature

Conversation

@sblondon

@sblondon sblondon commented Oct 1, 2019

Copy link
Copy Markdown
Contributor

This PR detects K-9 signatures to separate them from the main message.

The regular expression is generic in order to fit several translations of the signature.

The original signature text is available at https://github.com/k9mail/k-9/blob/581937a7e33f8c1442d0a6225485fb28f8dab1db/app/ui/src/main/res/values/strings.xml#L64

@mailgun-ci

Copy link
Copy Markdown

Can one of the admins verify this patch?

@obukhov-sergey obukhov-sergey left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — default K-9 footers are indeed missed on current master. I ran extract_signature on the English fixture and got the full body back with signature=None.

The -- delimiter does not save us here: TOO_LONG_SIGNATURE_LINE is 60, and

Sent from my Android device with K-9 Mail. Please excuse my brevity.

is 68 characters, so the candidate is marked cl and dropped. The existing ^sent from my… phone pattern also misses it because K-9 has a hyphen and the line has periods, which are outside [\s,!\w]. A dedicated phone-signature rule is reasonable. The one in this PR is not safe to merge.

Blocker: the new pattern is too loose

^[\S ]*?Android[\S ]*?K-9[\S ]*?\.[\S ]*?\.

RE_PHONE_SIGNATURE is search() on the entire body, then .* with re.S consumes everything after the first hit. The other branches at least start with Sent from / Enviado desde. This one matches any line that mentions Android, then K-9, then two periods.

These all match as a “signature” (remainder of the message included):

  • I use Android and K-9 Mail. Please try it. followed by meeting notes
  • The Android version of K-9 is 5.603.
  • On Android, I prefer K-9. It is great.

That will strip real reply text in production. Please also add $ like the other alternatives in this group.

Same pattern is also too strict

It requires two periods. One-sentence footers such as Sent from my Android device with K-9 Mail. (and many translations) will miss. Builds that say “Thunderbird” instead of “K-9” will miss too.

Tests

EN + FR fixtures are a good start, but they only cover the happy path. Please add:

  1. A false-positive case: Android + K-9 in the body, then real content — body must be kept.
  2. A K-9 footer without -- . That is the case that actually needs this regex; with -- the current tests also rely on RE_SIGNATURE.
  3. Direct expected-signature strings (same style as test_blackberry_signature) instead of msg_body[len('Heeyyoooo.\n'):].

Suggested fix

Prefer a narrow pattern in the existing style, e.g.:

^sent[ ]from[ ]my[ ]Android[\S ]*K-9[\S ]*$
|
^Envoyé[ ]de[ ]mon[ ]appareil[ ]Android[\S ]*K-9[\S ]*$

Anchor with $. Add locales explicitly rather than “Android + K-9 + two dots”.

A more general fix, if you want to go that way: treat a line that is exactly -- / -- as an RFC 3676 delimiter and take everything after it even when the next line is longer than 60. That would catch default K-9 and any other client that uses a standard delimiter plus a long disclaimer.

Please tighten the regex and add the negative test before we merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants