The default bug view has changed. See this FAQ.

Remote content exceptions: Add option "Allow remote content in this message"

RESOLVED FIXED in Thunderbird 45.0

Status

Thunderbird
Message Reader UI
--
enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Jorg K (GMT+1), Assigned: aceman)

Tracking

Trunk
Thunderbird 45.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
This has been discussed widely in bug 1193200:

An option to "Allow remote content in this message" would be useful, so the users who don't want to use "Allow remote content for <e-mail>" don't have to click multiple times to allow all URLs offered in the popup.

Advantages of this option are:
- Protection against spoofed senders
- newsletter sender can change, but remote content is still retrieved.
- no obfuscated e-mail addresses as used by e.g. Amazon or eBay,
  like 57fzqhzthk4s020@marketplace.amazon.de, need to be added to
  the exceptions.

Of course this option would have to be used with caution, since once a tracker is allowed, it will be allowed in other messages as well.
(Reporter)

Updated

2 years ago
Severity: normal → enhancement
(Assignee)

Comment 1

2 years ago
Yes, I can look at this.
Assignee: nobody → acelists
(Assignee)

Comment 2

2 years ago
Created attachment 8669929 [details] [diff] [review]
WIP patch

This is a WIP patch that implements the functionality. We can now discuss where to put the item in the menu to not make it too easy for the user to click it accidentally (e.g. it could be on the bottom of the menu).

Notice, the action only allows all URLs, not the sender's email (but the info bar vanishes, which is what we want).

Also it fixes a bug that if there is no From address (may be some spam), we would display a half empty "Allow remote content from" menuitem for the email.
Attachment #8669929 - Flags: feedback?(richard.marti)
Attachment #8669929 - Flags: feedback?(mozilla)
(Reporter)

Comment 3

2 years ago
Created attachment 8670148 [details] [diff] [review]
Screenshot showing the WIP.

Nice work.

I don't like the order of the menu items.
I'd do:
Show remote content
---
Edit options
---  <-- maybe not necessary  
Allow for e-mail
Allow for all xx listed below
Allow content from site 1
...

Richard, what do you think?
(Reporter)

Updated

2 years ago
Attachment #8669929 - Flags: feedback?(mozilla)
(Reporter)

Comment 4

2 years ago
Created attachment 8670149 [details]
Screenshot showing the WIP.

(oops, wrong file attached, sorry)
Attachment #8670148 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
Now when I look at the screenshot I notice I used "content FROM all" but the original items use "content FOR". As I think we pull the content from the URL (or server), "from" seems more appropriate. I am not sure about the "from/for email" item.

Native speakers? :)
Status: NEW → ASSIGNED
Flags: needinfo?(neil)
(Reporter)

Comment 6

2 years ago
I'm only half native ;-) - but "for" would be more consistent, since we have
  Allow remote content for http://www.xx.com
which means
  Allow remote content for this URL http://www.xx.com

If you put
  Allow remote content from website addressed by http://www.xx.com
I'd agree with the "from".

Where it's an address, URL or e-mail, I'd stick with the "for".

How about changing the order?
(Assignee)

Comment 7

2 years ago
(In reply to Jorg K (GMT+2) from comment #3)
> Show remote content
> ---
> Edit options
> ---  <-- maybe not necessary  
> Allow for e-mail
> Allow for all xx listed below
> Allow content from site 1
> ...
> 
> Richard, what do you think?

I think more like:
> Show remote content
> Edit options
> ---
> Allow for e-mail
> Allow content from site 1
> ...
> ---
> Allow for all xx URLs listed above

So that if there are 50 URLs with trackers the user has to scroll over them to find the option. Maybe then he notices some junk URLs. We could also put the "allow from email" item as the last one :)
(Reporter)

Comment 8

2 years ago
Yes, that's cool, too. When can I have it ;-)
Note that currently we have a separator between "Show" and "Edit".

> We could also put the "allow from email" item as the last one :)
Yes!! So:

Show remote content
---
Edit options
--- 
Allow content from site 1
...
Allow for all xx listed above
Allow for e-mail
Yes, placing "Allow for all xx URLs listed above" below all the URLs looks good to let the user thinking a bit what he all wants to allow. Also placing the "Allow for e-mail" at the end is good because the email address can be easily spoofed and shouldn't be the first option in the list.
Comment on attachment 8669929 [details] [diff] [review]
WIP patch

Tried the patch and with the changes discussed f+
Attachment #8669929 - Flags: feedback?(richard.marti) → feedback+
(Assignee)

Comment 11

2 years ago
Created attachment 8670461 [details] [diff] [review]
WIP patch v2

OK, try this. It has some more fine-tuning, like if there is only one URL (excluding sender address), do not show the "allow all item". If there is no sender and only 1 URL, then the last separator can also be hidden.
Attachment #8669929 - Attachment is obsolete: true
Attachment #8670149 - Attachment is obsolete: true
Attachment #8670461 - Flags: feedback?(richard.marti)
Attachment #8670461 - Flags: feedback?(mozilla)
(Reporter)

Comment 12

2 years ago
Created attachment 8670469 [details]
Screenshot showing the WIP 2

Nice, but please, s/from/for/ !
(Reporter)

Comment 13

2 years ago
Comment on attachment 8670461 [details] [diff] [review]
WIP patch v2

Very good!
Attachment #8670461 - Flags: feedback?(mozilla) → feedback+
(Reporter)

Comment 14

2 years ago
I'm confused now. On IRC first Kent convinced us that "from" is correct in all cases:
Quote: ... from, these are all variants of the same idea.

But then he changed his tune:
OK here's my recommendation, reversing what I said before. When you put "FROM user@example.com" you have anchored the meaning of FROM to refer to an email sender. So I think you should not use the same word for the httls:// urls. Go ahead and use FOR [for the URLs].

Later he said:
I think use FOR for web urls, FROM for email addresses

and then:
the other way around works to

So my personal preference is:
Accept remote content from URL.
Accept remote content in messages from xx@xx.com.

"Accept remote content for URL" sounds like: "Accept remote content to be given to URL".
And an e-mail for Suzie is an e-mail sent to Suzie. So "for" expresses the wrong direction.
Comment on attachment 8670461 [details] [diff] [review]
WIP patch v2

Not tested yet but looking at the screenshot from Jörg it looks good. If for or from is used I let Magnus or Neil decide.
Attachment #8670461 - Flags: feedback?(richard.marti) → feedback+
(Assignee)

Comment 16

2 years ago
Created attachment 8670997 [details] [diff] [review]
patch v2.1

Cleaned up the "from" and fixed proper plural forms.
Attachment #8670461 - Attachment is obsolete: true
Attachment #8670997 - Flags: ui-review?(richard.marti)
Attachment #8670997 - Flags: review?(mkmelin+mozilla)
Attachment #8670997 - Flags: ui-review?(richard.marti) → ui-review+
"Allow remote content from all of the above domains?"
Flags: needinfo?(neil)
(Reporter)

Comment 18

2 years ago
(In reply to neil@parkwaycc.co.uk from comment #17)
> "Allow remote content from all of the above domains?"

That's not quite correct: The list will contain URLs, so protocol and domain.
(Assignee)

Comment 19

2 years ago
There is 'URL' used in the patch so I can keep that.

Comment 20

2 years ago
Comment on attachment 8670997 [details] [diff] [review]
patch v2.1

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

Thx, looks good! r=mkmelin with some minor changes.

::: mail/base/content/mailWindowOverlay.js
@@ +3163,5 @@
>  
>    let addresses = {};
>    MailServices.headerParser.parseHeadersWithArray(
>      gMessageDisplay.displayedMessage.author, addresses, {}, {});
> +  let adrCount = (addresses.value[0] != "") ? 1 : 0;

let adrCount = addresses.value[0] ? 1 : 0;

@@ +3184,5 @@
>        childNodes[i].remove();
>    }
>  
> +  let URLSepar = document.getElementById("remoteContentAllMenuSeparator");
> +

please lowercase url: urlSepar

@@ +3206,5 @@
> +  let allURLLabel = messengerBundle.getString("remoteAllowAll");
> +  allowAllItem.label = PluralForm.get(URLcount, allURLLabel).replace("#1", URLcount);
> +
> +  allowAllItem.collapsed = (URLcount < 2);
> +  URLSepar.collapsed = allowAllItem.collapsed && (adrCount == 0);

you should collaps the other one too.
now if there's only one host there's a separator between every menu item, which doesn't look so nice. In fact it would look better without the separator after "Edit remote.... " too for that case (only have them for the multi-uri case)

@@ +3228,5 @@
> + *
> + * @param aListNode  The menulist element containing the URIs to allow.
> + */
> +function allowRemoteContentForAll(aListNode) {
> +  let URInodes = aListNode.querySelectorAll(".allow-remote-uri");

lowercase uri: uriNodes

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +341,5 @@
>  remoteContentPrefAccesskey=O
>  remoteContentPrefLabelUnix=Preferences
>  remoteContentPrefAccesskeyUnix=P
>  
> +# LOCALIZATION NOTE(remoteAllowResource): %S is host name

origin

@@ +346,5 @@
> +remoteAllowResource=Allow remote content from %S
> +# LOCALIZATION NOTE(remoteAllowAll): Semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/Localization_and_Plurals
> +# #1 is the number of URLs
> +remoteAllowAll=Allow remote content from all #1 URL listed above;Allow remote content from all #1 URLs listed above

They aren't URLs, but origins, so let's use the correct term.
Attachment #8670997 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 21

2 years ago
Created attachment 8675446 [details] [diff] [review]
patch v2.2

Thanks, fixed.
Attachment #8670997 - Attachment is obsolete: true
Attachment #8675446 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 22

a year ago
https://hg.mozilla.org/comm-central/rev/b65f89541ec444346abd811fbb8f886c1d0275b4
Bug 1209910 - add "allow remote content from all URL menuitem" in the "allow remove content" menu. r=Paenglab, r=mkmelin

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
You need to log in before you can comment on or make changes to this bug.