Closed Bug 522019 Opened 10 years ago Closed 10 years ago

thunderbird shows blocked content when composing reply

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0rc1

People

(Reporter: mads, Assigned: standard8)

References

Details

(Keywords: regression, Whiteboard: [no l10n impact])

Attachments

(2 files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.3) Gecko/20090909 Firefox/3.5.3-1.fc11 Firefox/3.5.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.4pre) Gecko/20090922 Fedora/3.0-2.7.b4.fc11 Thunderbird/3.0b4

It is nice that thunderbird blocks URL content by default when viewing mails.

But when a message with blocked content is replyed to or forwarded then the editor apparently loads the URLs by default. It shouldn't.

Isn't this a minor privacy issue?

Reproducible: Always
From the code and previous bugs, if this is verified then this is an issue we should fix for TB 3. I'll try and verify what is happening soon.
Assignee: nobody → bugzilla
Flags: blocking-thunderbird3?
Confirming, this is a regression from bug 424767 - I have a proposed fix, though would like to try and write a mozmill test for it first.
Blocks: 424767
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-thunderbird3? → blocking-thunderbird3+
Keywords: regression
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → Thunderbird 3.0rc1
Whiteboard: [no l10n impact]
Simplish fix, but I want more than one pair of eyes on it. This fixes the issue.

What is happening is that when we load the message into the compose window, the editor element (which acts like a browser element) actually has a requesting location (src) of about:blank.

This was fine until bug 424767 came along and blanket-allowed about: as the requesting location.

Therefore this patch checks to see if about:blank is the location and doesn't allow that, and therefore the content policy falls back on the existing code that does similar checks to message window - if remote content is allowed for the sender of the original email or if remote content was just allowed anyway.

I have a test case that is in progress for this but I wanted to get this moving anyway so that Dan has time to review before he goes travelling.
Attachment #406727 - Flags: superreview?(bienvenu)
Attachment #406727 - Flags: review?(dmose)
Comment on attachment 406727 [details] [diff] [review]
[checked in] Proposed fix

this looks reasonable to me - I'll try running with it to see if anything breaks.
Attachment #406727 - Flags: superreview?(bienvenu) → superreview+
Test case for this bug. This uses the video element as a test case for not allowing remote content in emails (video had the easiest way of detecting no-load allowed and hence was easier than images for this test, however, given we treat both the same it doesn't matter).

Don't be scared by the size of the patch, most of it is just a short video.

There's one element I had to add an id to so that mozmill could easily hook into the button as well.

Dan if you haven't got time for this please feel free to pass the review to David or someone else.
Attachment #407111 - Flags: review?(dmose)
Whiteboard: [no l10n impact] → [no l10n impact][needs review dmose]
Blocks: 491921
Whiteboard: [no l10n impact][needs review dmose] → [no l10n impact][has patch + unit test][needs review dmose]
Comment on attachment 406727 [details] [diff] [review]
[checked in] Proposed fix

r=dmose
Attachment #406727 - Flags: review?(dmose) → review+
Comment on attachment 407111 [details] [diff] [review]
[checked in] MozMill Test

Dan's now travelling, David could you take a look at this?
Attachment #407111 - Flags: review?(dmose) → review?(bienvenu)
Whiteboard: [no l10n impact][has patch + unit test][needs review dmose] → [no l10n impact][has patch + unit test][needs review bienvenu]
Comment on attachment 406727 [details] [diff] [review]
[checked in] Proposed fix

Checked in: http://hg.mozilla.org/comm-central/rev/520db1c594d9
Attachment #406727 - Attachment description: Proposed fix → [checked in] Proposed fix
This is now fixed, I'll check in the unit test once I get review for it.
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Comment on attachment 407111 [details] [diff] [review]
[checked in] MozMill Test

nit - license year is 2001 

+ * Portions created by the Initial Developer are Copyright (C) 2001
Attachment #407111 - Flags: review?(bienvenu) → review+
Comment on attachment 407111 [details] [diff] [review]
[checked in] MozMill Test

Checked in (first revisions were incorrect ones, backed out in the second):

http://hg.mozilla.org/comm-central/rev/ea7c702cf273
http://hg.mozilla.org/comm-central/rev/558474146a03
http://hg.mozilla.org/releases/comm-1.9.1/rev/7ea2d7831a65
http://hg.mozilla.org/releases/comm-1.9.1/rev/ff0c9980fffb
Attachment #407111 - Attachment description: MozMill Test → [checked in] MozMill Test
Flags: in-testsuite? → in-testsuite+
The windows mozmill comm 1.9.1 test box seems to reliably fail on this test.
(In reply to comment #12)
> The windows mozmill comm 1.9.1 test box seems to reliably fail on this test.

As the Mac box intermittently fails (i.e. it does pass sometimes), I'm assuming it is a timeout issue, therefore I've pushed a change to increase the timeout to 100 seconds on just the 1.9.1 branch. This should hopefully eliminate if it is due to the timeout being too short or if it is due to the notification not actually being (consistently?) received.

http://hg.mozilla.org/releases/comm-1.9.1/rev/e7df22acf0d1
Whiteboard: [no l10n impact][has patch + unit test][needs review bienvenu] → [no l10n impact]
Urgh, the other option is that by the time mozmill has decided the window is open, the notification has actually fired, and this is a bogus error. They haven't cycled yet, so I'll see what the current patch does first.
Duplicate of this bug: 452241
You need to log in before you can comment on or make changes to this bug.