480674 - Formulate content policy for Audio/Video tags

Status: RESOLVED FIXED

(Thunderbird :: Security, defect, P1)

Description

[Joe Sabash [:JoeS1]]

This is spinoff of bug 463155

Since OGG Audio and Video are now supported in core, we should take a look at how we allow them in MailNews.

Right now it's all or nothing via the pref media.ogg.enabled

This bug is for how we should handle the remote content loaded if the tags are allowed.

Video/Audio support represent the same privacy concerns as remote images.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Handing to Standard8 to see if he's up for hacking on this.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Looks like I inadvertently didn't actually manage to pass the bug to Standard8; trying again.

Comment 3

[Mark Banner (:standard8)]

I've just done some testing on this, as I suspected from my recent work on content policy, the remote content for video and audio will be allowed to load (or not) depending on the existing remote images settings.

However, now I read bug 463155 in full:

(In reply to comment #9)
> The bad news is, that it does not block the media frame.
> So you get the "remote content frame" regardless of your setting of
> mailnews.message_display.disable_remote_image
> (that could be another bug, as I thought that pref should block all remote
> content, image and other content as well)

Joe, so are you seeing something different to me? I had disable_remote_image set to true (the default) and I made sure that any cards in my address books with my email address had "allow remote images" disabled as well.

When the video/audio email was displayed, I also got the remote images notification as well.



Assuming what I saw was what we expect and I didn't miss anything, my proposal to "fix" this would therefore be to:

- change the UI elements and replace "remote images" by "remote content"
- change the back-end preference "mailnews.message_display.disable_remote_image" to "mailnews.message_display.disable_remote_content".

I'd be tempted to maintain support for the old disable_remote_image preference, but if that is set (i.e. false) and the new one is true, what do we do? I think just a straight rename which may force some people to set it, but it is at least more explicit as to what they are allowing.

Dan, Bryan: thoughts?

Comment 4

[Joe Sabash [:JoeS1]]

Re-tested with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090423 Lightning/1.0pre Shredder/3.0b3pre ID:20090423033944

It seems the pref is being respected for Ogg "content" now.
At the time of my testing, it was not. Possibly, I had a duplicate entry in my AB that allowed images.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> It seems the pref is being respected for Ogg "content" now.
> At the time of my testing, it was not. Possibly, I had a duplicate entry in my
> AB that allowed images.

Thanks for confirming that, so the only question now is if we update the UI.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Should definitely change the prefs and UI, I think.  I wonder if there's less abstract wording for the UI to be found than "content" ("images, audio, and video" maybe).

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> Should definitely change the prefs and UI, I think.  I wonder if there's less
> abstract wording for the UI to be found than "content" ("images, audio, and
> video" maybe).

I think "images, audio and video" would be generally too long. Especially thinking of the current "Display images now" button.

Note that content here doesn't just include those, it also includes (for example) any http/https location included in an iframe.

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

It would be really nice to know the difference.  In a better world we could identify the types of things we blocked and then ask the person specifically about those.

I worry that expanding the text to "images, audio, and video" means people might think all those things are included in the message when that is not necessarily (or likely) true.  At the same time "content" is a word that completely lacks meaning, the text is content too.  So I'm not sure.

I think "content" is the safer choice that we should probably go with.

Comment 9

[Joe Sabash [:JoeS1]]

Maybe, "remote web content" but I think remote content is sufficient.
FWIW, why I thought the blocking pref was not working:
If you use disc caching large enough to hold the Ogg file, the file will play from cache, which is fine, since you had to have at one time allowed it to load.

Comment 10

[Mark Banner (:standard8)]

Attachment: Change UI strings, images -> content

Simple patch to change the strings from images -> content obeying l10n rules.

SeaMonkey's header view already uses the "Content" terminology - its just the address book that is inconsistent there.

I've decided not to rename the preference at this stage - firstly we need the strings in by Tuesday, secondly I'm not actually sure if it is worth it as it is a hidden preference in TB, and the UI in SM is correctly worded.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 374520 [details] [diff] [review]
Change UI strings, images -> content

thanks!

Comment 12

[Karsten Düsterloh]

Comment on attachment 374520 [details] [diff] [review]
Change UI strings, images -> content

(In reply to comment #10)
> I've decided not to rename the preference at this stage

Erm, but you did, and you didn't change its occurences in the code!

>diff --git a/mailnews/mailnews.js b/mailnews/mailnews.js
>-pref("mailnews.message_display.disable_remote_image", true);
>+pref("mailnews.message_display.disable_remote_content", true);

r=me *only* without this change to mailnews.js!

Comment 13

[Mark Banner (:standard8)]

Attachment: Change UI strings, images -> content v2

Dropped the mailnews.js changes that shouldn't have been in there.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 374752 [details] [diff] [review]
Change UI strings, images -> content v2

>diff --git a/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd b/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
>--- a/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
>+++ b/mail/locales/en-US/chrome/messenger/addressbook/abCardOverlay.dtd
>@@ -79,13 +79,13 @@
> <!ENTITY ScreenName.label               "Screen Name:">
> <!ENTITY ScreenName.accesskey           "S">
> 
>-<!ENTITY allowRemoteContent.label       "Allow remote images.">
>-<!ENTITY allowRemoteContent.accesskey   "r">
>-<!ENTITY allowRemoteContent.tooltip     "In HTML messages it is possible to
>-embed images from remote sources. Opening such a message will open a
>+<!ENTITY allowRemoteContent1.label      "Allow remote content.">

Stylistically, it strikes me as odd to have a period at the end of this, but I don't feel terribly strongly about it.

Thanks for the patch; sr=dmose.

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> Stylistically, it strikes me as odd to have a period at the end of this, but I
> don't feel terribly strongly about it.

I debated and decided it could be done either way. As no one has complained so far I decided to leave it in.

> Thanks for the patch; sr=dmose.

Checked in: http://hg.mozilla.org/comm-central/rev/36f643f97e8b

Setting in-testsuite? as we need to do tests for the various content policy types to check we do the right thing.

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> Setting in-testsuite? as we need to do tests for the various content policy
> types to check we do the right thing.

The tests for content policy were done in bug 491921. There's nothing really to test on the strings, so just setting in-testsuite+.