Closed Bug 480674 Opened 11 years ago Closed 10 years ago

Formulate content policy for Audio/Video tags

Categories

(Thunderbird :: Security, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b3

People

(Reporter: JoeS1, Assigned: standard8)

Details

(Whiteboard: [has simple ui patch][has l10n impact])

Attachments

(1 file, 1 obsolete file)

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.
OS: Windows XP → All
Hardware: x86 → All
Handing to Standard8 to see if he's up for hacking on this.
Component: Mail Window Front End → Security
Flags: blocking-thunderbird3+
QA Contact: front-end → thunderbird
Target Milestone: --- → Thunderbird 3.0b3
Looks like I inadvertently didn't actually manage to pass the bug to Standard8; trying again.
Assignee: nobody → bugzilla
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?
Whiteboard: [investigated][needs comment Joe,dmose,clarkbw]
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.
(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.
Whiteboard: [investigated][needs comment Joe,dmose,clarkbw] → [UI change only needed?][needs comment dmose,clarkbw]
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).
(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.
Whiteboard: [UI change only needed?][needs comment dmose,clarkbw] → [UI change only needed?][needs comment 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.
Whiteboard: [UI change only needed?][needs comment clarkbw] → [UI change only needed?]
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.
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.
Attachment #374520 - Flags: ui-review?(clarkbw)
Attachment #374520 - Flags: superreview?(dmose)
Attachment #374520 - Flags: review?(mnyromyr)
Whiteboard: [UI change only needed?] → [has simple ui patch][needs review clarkbw,Mnyromyr,dmose]
Comment on attachment 374520 [details] [diff] [review]
Change UI strings, images -> content

thanks!
Attachment #374520 - Flags: ui-review?(clarkbw) → ui-review+
Whiteboard: [has simple ui patch][needs review clarkbw,Mnyromyr,dmose] → [has simple ui patch][has l10n impact][needs review Mnyromyr,dmose]
Attachment #374520 - Flags: review?(mnyromyr) → review+
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!
Dropped the mailnews.js changes that shouldn't have been in there.
Attachment #374520 - Attachment is obsolete: true
Attachment #374752 - Flags: ui-review+
Attachment #374752 - Flags: superreview?(dmose)
Attachment #374752 - Flags: review+
Attachment #374520 - Flags: superreview?(dmose)
Whiteboard: [has simple ui patch][has l10n impact][needs review Mnyromyr,dmose] → [has simple ui patch][has l10n impact][needs review dmose]
Priority: -- → P1
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.
Attachment #374752 - Flags: superreview?(dmose) → superreview+
Whiteboard: [has simple ui patch][has l10n impact][needs review dmose] → [has simple ui patch][has l10n impact][needs landing]
(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.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [has simple ui patch][has l10n impact][needs landing] → [has simple ui patch][has l10n impact]
(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+.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.