1260059 - spam e-mail from yahoo imap server marked as junk doesn't keep junk flag

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160309193552

Steps to reproduce:

wait for spam to slip by Yahoo's spam filter, then try and mark it as spam


Actual results:

I click on the dot in the spam column, or right click and select mark -> as junk
The spam icon momentarily appears in the spam column, then immediately disappears.


Expected results:

The spam icon should appear and remain in the spam column.
I think this is related to imap since I don't have this problem with another e-mail account using a POP server.

Comment 1

[Reuben Peterkin]

I have found that the the Yahoo! junk flagging works on the $Junk $NotJunk IMAP flags. I am able to change the junk status report in Thunderbird by setting the flag $NotJunk directly using IMAP.
(a store # +flags $NotJunk)

Comment 2

[Reuben Peterkin]

(Sorry (In reply to Reuben Peterkin from comment #1)
> I have found that the the Yahoo! junk flagging works on the $Junk $NotJunk
> IMAP flags. I am able to change the junk status report in Thunderbird by
> setting the flag $NotJunk directly using IMAP.
> (a store # +flags $NotJunk)

Sorry should have clarified - that was for removing junk status, I assume to set it would be to set $Junk.

Comment 3

[[e-mail]]

I have never had a problem removing the junk flag, only setting it. It will set, then immediately, with no action from me, unset.

Comment 5

[Wayne Mery (:wsmwk)]

Another item about junk flags

xref 
Bug 1367443 - Email messages flagged as Junk/spam no longer being automatically sent to Junk folder
Bug 1237138 - Marking as not junk does not remove Junk Status

Comment 6

[gene smith]

It's something about yahoo causing it. When I click the junk icon on a message in my yahoo inbox, the message gets moved to "bulk mail" folder but the junk icon is gray in "bulk mail". I do see it in color for maybe 1/4 second after I click it before it moves. 
A couple other accounts I do the same and the junk icon remains colored after it is moved to "junk" folder.
I will need to look at what is happening in the imap log so I will leave my "needinfo" flag set.

Comment 7

[Wayne Mery (:wsmwk)]

(In reply to gene smith from comment #6)
> ...
> A couple other accounts I do the same and the junk icon remains colored
> after it is moved to "junk" folder.
> I will need to look at what is happening in the imap log so I will leave my
> "needinfo" flag set.

I had another look - didn't find any related bug reports

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to gene smith from comment #6)
> It's something about yahoo causing it. When I click the junk icon on a
> message in my yahoo inbox, the message gets moved to "bulk mail" folder but
> the junk icon is gray in "bulk mail". I do see it in color for maybe 1/4
> second after I click it before it moves. 
> A couple other accounts I do the same and the junk icon remains colored
> after it is moved to "junk" folder.
> I will need to look at what is happening in the imap log so I will leave my
> "needinfo" flag set.

hmm, bug 1489859?

Comment 10

[Wayne Mery (:wsmwk)]

So we have both http://forums.mozillazine.org/viewtopic.php?f=39&t=3041601 and http://forums.mozillazine.org/viewtopic.php?f=39&t=3038777

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to gene smith from comment #6)

It's something about yahoo causing it. When I click the junk icon on a
message in my yahoo inbox, the message gets moved to "bulk mail" folder but
the junk icon is gray in "bulk mail". I do see it in color for maybe 1/4
second after I click it before it moves.
A couple other accounts I do the same and the junk icon remains colored
after it is moved to "junk" folder.
I will need to look at what is happening in the imap log so I will leave my
"needinfo" flag set.

Have since found: bug 1489859, bug 1074055, bug 1237138

Comment 12

[gene smith]

The problem is that Yahoo defines PERMANENTFLAGS $Junk and $NotJunk. Tb assumes, mostly correctly, that the junk flags will be Junk and NonJunk like the others imap servers define (e.g., gmail and openwave that I verified).

On yahoo account, since tb doesn't see the expected flags Junk and NonJunk, it just sets the message \Seen when marking the message as spam. On response to the command, tb doesn't see the Junk flag in the response so it immediately clears the spam icon back to gray.

I'm not sure how hard this is to fix or if it should be. Also, not sure if Yahoo is violating a spec or RFC for the flag names it uses. TBD...

Comment 13

[gene smith]

My explanation in comment 12 is not quite right. The actual problem with yahoo (and aol, which uses the same server) is that the PERMANENTFLAGS don't allow for the creation of custom tags that tb is hardcoded to produce, "Junk" and "NonJunk". Yahoo's PERMANENTFLAGS and FLAGS (untagged responses to SELECT) contain the values "$Junk" and "$NotJunk", but PERMANENTFLAGS does not contain "\*" that indicates that any custom tag (a.k.a., keyword) can be accepted. So "Junk" or "NonJunk", as always produced by TB can't be accepted.

When the user marks a message as junk or not junk, tb produces a url containing the appropriate string "Junk" or "NonJunk". But right before the flag is about to be sent to the server via IMAP using the STORE command, tb determines that custom flags can't be set for the server and does not attempt to send the STORE command at all. It does go ahead and store the \Seen flag, however. When store of \Seen occurs, the yahoo server usually responds with a untagged FLAG response that contain the string "$NotJunk". This flag causes junk icon for the message just marked to quickly return to gray (not junk) state (assuming the action was to mark the message as junk).

At this time the RFCs don't specify exactly which keywords (tags) should be used to indicate junk and not junk for messages. However, this semi-official document recommends using $Junk and $NotJunk as Yahoo is assuming:
https://www.ietf.org/mail-archive/web/morg/current/msg00441.html (Thanks to Wayne for finding this link.)

Tb sending "Junk" or "NonJunk" are OK as long as the server allows for custom keywords ("\*" in the PERMANENTFLAGS). So instead of changing the several places where the strings "Junk" and "NonJunk" are hard-coded, I propose that at the point where the URL is decoded and tb determines that custom keywords are not allowed, instead of not sending the STORE command at all, instead change the flag value from "Junk" to "$Junk" or from "NonJunk" to "$NotJunk" and go ahead send the IMAP command. Yahoo/Aol accepts the flag when this is done. Other servers that also don't accept custom keywords just return a NO imap response and doesn't cause an error notification in tb.

The real problem with Yahoo/Aol is that the store of the \Seen flag causes the $NotJunk response to occur. Other servers I have observed don't return the $NotJunk flag unless it was explicitly set. Therefore, even if the server doesn't store custom flags, when marking a message tb stores in the .msf file (database) the proper junk status and it doesn't get reset. (Of course, other clients may not see the marked junk/not junk status.)

Comment 14

[gene smith]

Attachment: permanentflags.txt

This shows the FLAGS and PERMANENTFLAGS responses from servers that I have test accounts on. The general rule is that if the server supports custom keywords, tb will be able to store hard-coded string "Junk" or "NonJunk" and this bug should not occur.

Comment 15

[gene smith]

Attachment: yahoo.diff

This shows a 1st cut fix for the problem with yahoo and aol. This is the point where the hard-coded string "Junk" or "NonJunk" is sent via IMAP but the server doesn't accept custom keywords. All this does is change "Junk" to "$Junk" and "NonJunk" to "$NotJunk" which are the new "canonical" strings and they are accepted by Yahoo/Aol servers so the bug is fixed.

I have tested this some with other servers that don't accept custom keywords. Some return a NO response and others just accept the keywords but don't really store them. In both cases, the keywords are still stored locally and retained in the database .msf file.

If custom keywords are accepted, the normal hard-coded keywords are still stored, "Junk" and "NonJunk".

This is not an official patch but just for illustration. Also, just noticed some comment typo's in that at two places it says "Try $Junk" and it should say "Try $NotJunk".

Comment 16

[gene smith]

Attachment: 1260059-allow-yahoo-junk-marking-to-stick.patch(v0)

Here's a formal patch for making yahoo's (and AOL's) junk / not junk marking of the trash icon remain in place (stick) instead of possibly changing back to its previous state.

This differs from the preliminary diff (attachment 9040628 [details] [diff] [review]: yahoo.diff) in that the attempt to set the keywords only occurs if the keywords $Junk and $NotJunk are present in the FLAGS response. This will avoid attempting to set the keywords to servers that don't support them and thus avoid the "NO" response.

In addition a small change is made to the Bug 583677 fix since when function parse_folder_flags() is called for FLAGS (and not for PERMANENTFLAGS) the expression (fSupportsUserDefinedFlags & kImapMsgSupportUserFlag) is always false so no reason to check it. See https://searchfox.org/comm-central/rev/582bd5d57be824ff97c13c7b6e097b9cf666a953/mailnews/imap/src/nsImapServerResponseParser.cpp#1856

There is also a small issue that could occur if parse_folder_flags() is called first for PERMANENTFLAGS and then for FLAGS. In this case the user defined keywords (including $Junk and $NotJunk) won't be stored and this bug will not be fixed (nor will bug 583677). I don't know why the parsing of the FLAGS is skipped in this case. See
https://searchfox.org/comm-central/rev/582bd5d57be824ff97c13c7b6e097b9cf666a953/mailnews/imap/src/nsImapServerResponseParser.cpp#782 However, I have never seen this reverse order occur and have only seen FLAGS first followed by PERMANENTFLAGS. See attachment 9040627 [details]: permanentflags.txt

Comment 17

[Jorg K (CEST = GMT+2)]

Sorry for the delay. I've been on a conference and then side-tracked into other issues. And those IMAP changes aren't easy reviews.

Comment 18

[Jorg K (CEST = GMT+2)]

My sincere apologies for the delay here. We had a few weeks with terrible bustages (still not all fixed) and keeping Daily going and pushing releases out the door is my priority.

Let's start this with a try run to avoid surprises:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8f302c4cddce12e959b6674526fcd5a9a2db50db

Comment 19

[Jorg K (CEST = GMT+2)]

Comment on attachment 9041108 [details] [diff] [review]
1260059-allow-yahoo-junk-marking-to-stick.patch(v0)

Review of attachment 9041108 [details] [diff] [review]:
-----------------------------------------------------------------

Looks OK. Sorry once again about the delay. Please address the nits flagged below.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +2887,5 @@
>          break;
>        case nsIImapUrl::nsImapMsgStoreCustomKeywords:
>          {
> +          // If the server doesn't support user defined flags, don't usually try
> +          // to set or reset them. But if this an attempt to set/reset TB's user

Grammar. But if this *is* ...?

@@ +2889,5 @@
>          {
> +          // If the server doesn't support user defined flags, don't usually try
> +          // to set or reset them. But if this an attempt to set/reset TB's user
> +          // defined flags "Junk" or "NonJunk", even if the server doesn't
> +          // support  storing user defined flags, but it does allow storing

double space |support..storing|

@@ +2892,5 @@
> +          // defined flags "Junk" or "NonJunk", even if the server doesn't
> +          // support  storing user defined flags, but it does allow storing
> +          // standard flag names "$Junk" and "$NotJunk", change "Junk" and
> +          // "NonJunk" to these values respectively, and go ahead and store the
> +          // modified flags. (Flags are also referred to as keywords or tags.)

Maybe you could restructure this sentence, it's a little twisted.

@@ +2898,2 @@
>            GetSupportedUserFlags(&userFlags);
> +          bool userDefinedSettable = (userFlags & kImapMsgSupportUserFlag);

Lose parenthesis.

@@ +2911,5 @@
> +            if (!userDefinedSettable)
> +            {
> +              if (stdJunkOk)
> +              {
> +                if (!PL_strcasecmp(addFlags.get(), "junk"))

Please use
addFlag.EqualsIgnoreCase("junk") or .Equals(xxx, nsCaseInsensitiveCStringComparator()); Same here and below.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +1852,5 @@
>  }
>  
>  void nsImapServerResponseParser::parse_folder_flags(bool calledForFlags)
>  {
> +  uint16_t labelFlags = 0, junkNotJunkFlags = 0;

one variable per line, no commas.

@@ +1853,5 @@
>  
>  void nsImapServerResponseParser::parse_folder_flags(bool calledForFlags)
>  {
> +  uint16_t labelFlags = 0, junkNotJunkFlags = 0;
> +  bool storeUserFlags =  calledForFlags && fFlagState;

Double space after =

@@ +1906,4 @@
>  
> +      // Store user keywords defined for mailbox, usually by other clients.
> +      // But only do this for FLAGS response, not PERMANENTFLAGS response.
> +      // This is only needed if '\*' does not appeared in a PERMANENTFLAGS 

Trailing spaces.

@@ +1907,5 @@
> +      // Store user keywords defined for mailbox, usually by other clients.
> +      // But only do this for FLAGS response, not PERMANENTFLAGS response.
> +      // This is only needed if '\*' does not appeared in a PERMANENTFLAGS 
> +      // response indicating the user defined keywords not allowed; but this
> +      // is not known until this function called for PERMANENTFLAGS which 

And here.

Comment 20

[gene smith]

Attachment: 1260059-allow-yahoo-junk-marking-to-stick-v2.patch

Jorg, This addresses all your review items, built and retested.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 9047608 [details] [diff] [review]
1260059-allow-yahoo-junk-marking-to-stick-v2.patch

Review of attachment 9047608 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the update. Still some nits which I can fix myself.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +2932,5 @@
> +            if (!userDefinedSettable)
> +            {
> +              if (stdJunkOk)
> +              {
> +                if (!PL_strcasecmp(subtractFlags.get(), "junk"))

Still some undesirable string comparisons here.

::: mailnews/imap/src/nsImapServerResponseParser.cpp
@@ +1907,4 @@
>  
> +      // Store user keywords defined for mailbox, usually by other clients.
> +      // But only do this for FLAGS response, not PERMANENTFLAGS response.
> +      // This is only needed if '\*' does not appeared in a PERMANENTFLAGS

Nit: Grammar derailed. You changed "has not appeared" to "does not appeared".

Comment 22

[gene smith]

Attachment: 1260059-allow-yahoo-junk-marking-to-stick-v3.patch

Should be OK now. I wanted to test it again after changing the string compares I missed. Also, changed the wording some in the comment containing "...does not appeared in a PERMANENTFLAGS response..."

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 9047915 [details] [diff] [review]
1260059-allow-yahoo-junk-marking-to-stick-v3.patch

Thanks indeed, Gene!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/85404debffde
Allow setting/resetting junk marking by user for yahoo/aol to stick. r=jorgk DONTBUILD