522645 - Determining what is a feed message regression

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[alta88]

the patch in Bug 438429 specifically handled the following cases:
1)a feed message can be in any folder, not just News
2)a feed message in a standalone window is recognized as such

Bug 474701 removed the content-base check, which should be restored.  in addition, there won't be a gFolderDisplay object in a standalone message window, i believe, so the selectedMessageIsFeed there isn't going to work.

Comment 1

[Rain [:rain1] (inactive)]

> there won't be a gFolderDisplay object in a standalone message window

There should be.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Can you describe exactly what the user-visible effects of this bug are?

Comment 3

[Magnus Melin [:mkmelin]]

xref bug 493221 which likely WFM now (by the same cause)

Comment 4

[alta88]

Attachment: restore content-base feed check.

(In reply to comment #1)
> > there won't be a gFolderDisplay object in a standalone message window
> 
> There should be.

yep, it's there.  standalone message from News folder is recognized.

(In reply to comment #2)
> Can you describe exactly what the user-visible effects of this bug are?

any feed message not in the News folder will not have feed specific menuitems displayed.  this patch should revert to the spirit of the previous check.

(In reply to comment #3)
> xref bug 493221 which likely WFM now (by the same cause)

imo it is far more important to enable feed messages in different folder organizations, than remove the check to address that bug, which is a quite nonstandard mail header issued by the publisher, and in fact not formatted to spec.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 406713 [details] [diff] [review]
restore content-base feed check.

This code should probably be reviewed by myself, sid0, or bienvenu.

You shouldn't use currentHeaderData here.  Is there something on the nsIMsgDBHdr itself you can use?

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 406713 [details] [diff] [review]
restore content-base feed check.

No need for sr, since this is in mail/.  Myk has graciously agreed to review, which is good, because I'm not turning around reviews as quickly as I'd like to be at the moment.  Thanks, Myk!

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

Mmmm, bugzilla-collision-riffic.  Will aim review at bienvenu.

Comment 8

[David :Bienvenu]

Comment on attachment 406713 [details] [diff] [review]
restore content-base feed check.

We don't put the content base in the msg hdr - currentHeaderData could be empty if there's no message pane displayed.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

Right, this sounds familiar.  I think I tried to avoid removing the check in the code but there was no easy way to reliably have the data.

The best-case solution for this is to have the MessageDisplayWidget register for header notifications from the message reader.  If it sees that header is set, it can set an internal flag like "bodyContentIndicatesFeed" which the FolderDisplayWidget can check.  It could also trigger a toolbar update or whatever is required if it can't rely on one being produced elsewhere (although I suspect it can).

We definitely want to avoid having FolderDisplayWidget access a global provided by the message reader without active/inactive scoping and without some awareness of the availability of the data being event-driven.

Comment 10

[David :Bienvenu]

Comment on attachment 406713 [details] [diff] [review]
restore content-base feed check.

minusing based on asuth's comments.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Should we consider putting the Content-Base into the message header?

Comment 12

[David :Bienvenu]

We could, since only rss messages will have content-base. It wouldn't help with already downloaded rss messages, however, w/o a reparse.

Comment 13

[alta88]

hmm, the new internals look quite different from what i recall.  since the purpose here is to only do the check/show menuitems if 1 message is selected (not meaningful for possible mixed types in multiselect), it seems reasonable to require that message pane or message in standalone window is visible.  in which case header data should be very easily accessible and currentHeaderData should work.  otherwise the menuitems can be disabled.

any reason not to just go back to the prior method?

Comment 14

[David :Bienvenu]

See Andrew's comment #9 

>We definitely want to avoid having FolderDisplayWidget access a global provided
>by the message reader without active/inactive scoping and without some
>awareness of the availability of the data being event-driven.

Comment 15

[alta88]

right, i took that to mean using gFolderDisplay methods in folderDisplay.js, not the already available header data in mailWindowOverlay.js.  perhaps incorrectly..

Comment 16

[David :Bienvenu]

ah, by prior method, you mean going back to code in mailWindowOverlay.js...I don't know this code that well, asuth should probably chime in.

Comment 17

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #15)
> right, i took that to mean using gFolderDisplay methods in folderDisplay.js,
> not the already available header data in mailWindowOverlay.js.  perhaps
> incorrectly..

In an ideal world we could identify messages that are RSS messages without streaming them.  It would be most preferable to move towards that world, which would mean setting the message header and just using that.  (Given that RSS is a 'timely' medium, even if users do not reindex, most would probably not even have a problem since all the new messages would still get the header.)

Having said that, mailWindowOverlay.js is still an untamed wilderness from my perspective; I'm fine with doing the currentHeaderData check there.  The caveat is that when we actually improve the message reader situation, it may get broken again unless RSS becomes a priority and starts getting a lot of love and unit tests.  The 'more correct' solution is less likely to get steamrolled.

Comment 18

[David :Bienvenu]

yeah, I'm happy to make the local message parser set a content-base property and let alta88 query that property. I'll put up a patch in a minute for him to play with.

Comment 19

[alta88]

david, i don't know that you need to do anything, it would just go back to what worked before.  content-base was just available, filled in when a message was selected..

the thing that would be most correct to do, and take no time, would be to not depend on content-base, since per strict rfc it is freeform and an email client *could* publish it, though it's very much not customary.  it would be simple to add something like x-moz-feed and fill it with something useful like rss/atom version etc.

then we could check for x-moz-feed instead of content-base.

Comment 20

[David :Bienvenu]

Attachment: add contentBase as db hdr property if it exists in message

alta88, if you run with this patch, newly downloaded rss messages should have a string property set to the content-base header value, so nsIMsgDBHdr.getStringProperty("content-base") would tell if you if the message has a content base.

Comment 21

[David :Bienvenu]

ah, didn't see your comment til after. The same patch could be tweaked to add x-moz-feed instead...but you'd need to set that header in the feed code, as you know, I'm sure.

Comment 22

[David :Bienvenu]

What would be even nicer is to make nsILocalMailFolder::addMessage return the msg hdr of the header added, and you could set the property yourself. I think I'm going to do that. That would also make some of our test cases easier to write.

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Assuming we use Content-Base this way, what are the security implications (if any) of a black-hat/phisher sending an email with a Content-Base header pointing to site of their choosing.  Is there any chance we'd load that site rather than displaying the message itself?

Comment 24

[alta88]

(In reply to comment #23)
> Assuming we use Content-Base this way, what are the security implications (if
> any) of a black-hat/phisher sending an email with a Content-Base header
> pointing to site of their choosing.  Is there any chance we'd load that site
> rather than displaying the message itself?

yes, if the default were to show web page.  the issue exists in Tb2 as well.  and it would be true if this functionality depended on any header; x-moz-feed could be spoofed in an email too.

feedparser or whatever stores a msg to the db would have to set a feed flag, which could be trusted, or similar arch..

Comment 26

[alta88]

filed bug 647699, as per c#22 it's the best way to go.  we really need a trusted way of identifying feed messages (not based on folder).

Comment 27

[alta88]

now that 647699 is fixed thanks to magnus, i'd like to complete this, such that a message that is a feed is treated as such regardless of folder server (including re content policy).

it seems there is a free slot at x40 for an isFeed flag here:
http://mxr.mozilla.org/comm-central/source/mailnews/base/public/nsMsgMessageFlags.idl#43

david, is it ok to take this bit?  initially i thought there could be a separate feed uint property with other flags but couldn't think of any other flags.

Comment 28

[David :Bienvenu]

(In reply to alta88 from comment #27)

> david, is it ok to take this bit?  initially i thought there could be a
> separate feed uint property with other flags but couldn't think of any other
> flags.

yes, I'm OK with that; it seems like a relatively important attribute worthy of one of our precious remaining bits :-) Or you could also simply use a property on the nsIMsgDBHdr, that defaults to false and is only set for feeds. It's marginally slower, and adds a bit of bloat to the .msf files for feeds

Comment 29

[alta88]

Attachment: patch

yeah, if there were any reason to store anything more informative, a property would be the way to go, but there isn't.  so this is probably a 'bit' better.

part 1 to merely store the flag.

Comment 30

[David :Bienvenu]

Comment on attachment 628166 [details] [diff] [review]
patch

looks OK. I forget what 0x40 used to be used for. It looks like it's been unused for a very long time, but there's a small chance it might exist out there in some local mail folders in x-mozilla-status headers.

Comment 31

[alta88]

if that's indeed the case, subsequent code can also check to see if there's a content-base header.

making use of the new flag will be a separate bug.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Landed in comm-central as https://hg.mozilla.org/comm-central/rev/81cb0eec3150