Composition: Filelink attachments embedded icons missing, caused by security errors (null principal may not load).

RESOLVED FIXED in Thunderbird 55.0

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: wavexx, Assigned: jorgk)

Tracking

({regression})

52 Branch
Thunderbird 55.0
regression

Thunderbird Tracking Flags

(thunderbird_esr5253+ fixed, thunderbird53 fixed, thunderbird54 fixed, thunderbird55 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20170401232357

Steps to reproduce:

Compose a new message. Attach a file through a filelink service.
The "I've linked 1 file to this email: ..." banner is inserted in the body.


Actual results:

The attachment icon and the filelink service's icon are missing in the banner due to a new security restriction. Open the console (Ctrl-Shift-J) and notice the following is written:

Security Error: Content at about:blank may not load or link to chrome://messenger/content/cloudfile/  
attachment-24.png.
Security Error: Content at about:blank may not load or link to chrome://thunderbird-filelink-dl/skin/ 
dl-icon.png.

The first is referenced by TB itself, the second is due to the "iconClass" filelink property pointing to a chrome resource, which used to work before.


Expected results:

If referencing chrome:// urls is no longer allowed, TB should recode them to data URIs on the fly.
(Assignee)

Comment 1

2 years ago
I see this:
In TB 52 the Box.com upload doesn't work. I don't see the errors mentioned in comment #0, but instead, a lot of others.

In TB 55, Daily, Box.com upload works, and I see the errors you mention in comment #0 but the upload works.

So what is the issue here? The error console is for developers and it's very talkative. We won't fix that.
Which version are you using? And does the upload work or not?

Magnus, I'm having trouble with Box.com uploads in TB 52, works OK in TB 55 Daily. The bug you fixed recently was uplifted to TB 52. Can you please give it a whirl. Richard, how is Box.com upload for you?
Flags: needinfo?(richard.marti)
Flags: needinfo?(mkmelin+mozilla)
(Reporter)

Comment 2

2 years ago
The upload itself works, but this report is not about this.

It's the attachment banner which is shown in the composer window which is generating the error: the images reference some resources in chrome://, which is no longer allowed. The "clip" image and the logo of the filelink service show up as blank because of this.
I never used box.com, I have dropbox and I get the same security errors. Only the icons aren't shown but the whole functionality is still here.

I remember we had a bug with not showing smilies etc. because they are blocked by security. So this is the same problem.
Flags: needinfo?(richard.marti)
(Assignee)

Comment 4

2 years ago
OK I started TB 45 to compare and I can see the paper clip and box icons in the message are missing in TB 55.

The full message reads:
Security Error: Content at moz-nullprincipal:{3856720e-d6bd-4a8e-bd81-b15f8a517f62} may not load or link to chrome://messenger/content/cloudfile/attachment-24.png.
Security Error: Content at moz-nullprincipal:{3856720e-d6bd-4a8e-bd81-b15f8a517f62} may not load or link to chrome://messenger/skin/icons/box-logo.png.

Of course a null principal can't load those icons. Of course I also remember the security bug.
Summary: Composition: Filelink attachments cause security errors on the console → Composition: Filelink attachments embedded icons missing, caused by security errors (null principal may not load).
(Assignee)

Comment 5

2 years ago
Thinking about this, Magnus, we broke this is our favourite bug.

The HTML of those blocks is:
<img ... src="chrome://messenger/content/cloudfile/attachment-24.png">
and of course the compose window that is no longer an editor can't load those.

Richard, can you fix this for us, please.

Just replace
paperclip.src = "chrome://messenger/content/cloudfile/attachment-24.png";
with
paperclip.src = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABgAAAAYCAYAAADgdz34AAAABHNCSVQICAgIfAhkiAAAAAlwSFlzAAAN1wAADdcBQiibeAAAABl0RVh0U29mdHdhcmUAd3d3Lmlua3NjYXBlLm9yZ5vuPBoAAAH+SURBVEiJ7ZVNaxNRFIaf09T6AXXpUgQpfmGnSmYSsCB0qe50pdJFuxEVKhUEQVBcCipIS+yiG21dxLoTEQQFV83MBJJpF0LxRyiK0EzmddEkGMlnxV0PDMzc97znuefeuTMmif8Zg+2EdDq9KyY1LZjFGGkSxYbBk0Gqi2EYVrYFiElNy8i1FI0RQS4mJWChE2CgnSCY3brRTBT6qXJQsHJQsCj0U5jdquXMdCreEVBflqgYzElKGmApOXLo4Fzt8RiA43orjpdZ7g/wR8FRN/PWcbNv6mP5fL7612wuIi638rfdgyY7nIftvW1dO/jX2AHsABohgKPj48MAjnPmQG08aevoE/AZYGgzvgTAUHwdAONdN2OPB81eCJ016bbjZr9T/05VedrN26mDbwCnPG8s/rU3j9gAToBWgGGMV+Vi4WN92foHyJYBEtmd9fVPPyzFBUEAIOlhFPhXAXZvVp7XDGutyli7P9rpbPZ4taoisMfgWikoLACcTKcPr4XhV4AxNzsltAj8lKrpKAy/9AwAGPW8KyZbAipmmir5/lJDc7PnDL0G9hk2WQpWX/bVQT0cN/MAuL+VbY+TJM4ZA5Nmdg8YMDRfCvyb7fxdAbXZ3jD0jOY9SwR3o6DwqJO3p3MQBavzMk0A74HE0AeZJroVB0BSX1cmk9nfT/5vekgJrtOb2V0AAAAASUVORK5CYII=

To get the data string, drop the icon onto a compose window, select the image, use |Insert > HTML| and then copy the src. Yes, it looks short, but it will be copied in full onto the clipboard.

Do the same for the other icon as well.

That's simpler than to drill open the code and replace the icons with data: URIs on the fly.

Loading icons from chrome into a message was of course a doomed idea to start with :-(
Flags: needinfo?(richard.marti)
Created attachment 8857165 [details] [diff] [review]
Bug1355462.patch

Yes, this works.

Jörg, please check also the test. I added there also the base64 image as I think it's used as a reference.
Assignee: nobody → richard.marti
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(richard.marti)
Attachment #8857165 - Flags: review?(jorgk)
(Reporter)

Comment 7

2 years ago
This work-around is quite annoying, as existing extensions have to be modified.
I would have favored doing the proper fix and recode the URI on the fly, which would avoid extensions to embed data: into the source itself.
(Assignee)

Comment 8

2 years ago
Yes, this works, nice, thank you.

I can't see where the test-cloudfile-helpers.js is used, it's not found at all in DXR. Aceman, can you enlighten us here?

I suggest to use a simplified image from content-utf8-rel-only.eml for the test.
iVBORw0KGgoAAAANSUhEUgAAAAYAAAALCAIAAADTMGvBAAAAEUlEQVQImWPgsi5DQwxDWggAlCEwN+YGfiYAAAAASUVORK5CYII=

Then push the thing to try on one platform that works, I suggest Mac or Linux opt:
hg qnew -m "try: -b o -p macosx64 -u Mozmill" try
hg qnew -m "try: -b o -p linux64 -u Mozmill" try

I would check the test of I knew which one it was, but especially those cloud file tests are a real pain to run locally.
(Assignee)

Comment 9

2 years ago
(In reply to wavexx from comment #7)
> This work-around is quite annoying, as existing extensions have to be
> modified.
Existing extensions doing what exactly? Referencing chrome images? Which extensions?
BTW, there is no automatic conversion from insecure stuff into images, that's the whole point. If extensions place that sort of stuff, they need it under their own program control. Anyway, I don't know which extensions you're talking about and what they're doing exactly.

===

NI for Aceman for comment #8 got lost by collision.
Flags: needinfo?(mkmelin+mozilla) → needinfo?(acelists)
(Reporter)

Comment 10

2 years ago
Maybe I should be more explicit here. I wrote a filelink provider:

https://addons.mozilla.org/en-US/thunderbird/addon/dl-for-thunderbird/

This is how I discovered the issue in the first place.
I also return a chrome:// URI in iconClass().
So do all the other filelink extensions currently available on AMO.

The cloudfile provider should take the URI and base64 encode the content *before* pasting it into the composer as a chrome:// resource so that there's no external reference.

Comment 11

2 years ago
(In reply to Jorg K (GMT+2) from comment #8)
> I can't see where the test-cloudfile-helpers.js is used, it's not found at
> all in DXR. Aceman, can you enlighten us here?

That's a catch:) The file is loaded in mozmill tests via MODULE_REQUIRES under the name of "cloudfile-helpers", see https://dxr.mozilla.org/comm-central/search?q=cloudfile-helpers&redirect=true .
Flags: needinfo?(acelists)
(Assignee)

Comment 12

2 years ago
OK, you convinced me. Let me see what I can do.
(Assignee)

Comment 13

2 years ago
Created attachment 8857215 [details] [diff] [review]
WIP: 1355462-filelink-icons.patch (v1).

This snipped is copied from
https://dxr.mozilla.org/comm-central/rev/b08dc613a3d275711f76b42cf0ceba1119d82f02/mail/components/compose/content/MsgComposeCommands.js#5681

This should do it, sadly Box.com right now doesn't want to accept my uploads although it worked before.

I'll have to test is later.

Sorry Richard, I gave wrong instructions before, my apologies.
Assignee: richard.marti → jorgk
(Assignee)

Comment 14

2 years ago
Comment on attachment 8857165 [details] [diff] [review]
Bug1355462.patch

Sorry, Richard. My fault, my suggestion was a bad idea.
Attachment #8857165 - Attachment is obsolete: true
Attachment #8857165 - Flags: review?(jorgk)
(Assignee)

Comment 15

2 years ago
(In reply to Jorg K (GMT+2) from comment #13)
> This should do it, sadly Box.com right now doesn't want to accept my uploads
> although it worked before.
This partly works, the Box icon to the right is successfully converted to a data: URL, but the icon to the left, the paperclip, isn't. I get an error on the console:
Chrome file doesn't exist: C:\mozilla-source\comm-central\obj-x86_64-pc-mingw32\
dist\bin\chrome\messenger\content\messenger\messengercompose\null
That's not the icon I asked for. On the error console I still see:
Security Error: Content at moz-nullprincipal:{6d6051bd-1b66-4ada-a069-959cfeb6d6d1} may not load or link to chrome://messenger/content/cloudfile/attachment-24.png.

Trying to repeat the problem, the upload fails altogether. Perhaps there is a restriction to a few uploads a day on the free account, hmm. This won't be easy to debug if I only have three tries a day :-(

Richard, maybe you can help me here. Take the patch and try with your Dropbox account. Perhaps we need to shift the icon to a different location, I don't see why chrome://messenger/content/cloudfile/attachment-24.png wouldn't be found and chrome://messenger/skin/icons/box-logo.png would. Or maybe the former location is blocked to content altogether and the latter one isn't?

Or how about a combined approach? Make the paper clip a fixed data: URI and the box icon, which comes from a different location a fixed string.

Reporter: Sorry, I'm not very familiar with this area. Which icon are you providing? Only the icon for the service? So the right-hand one?

Is so, I see two options here:
1) Move the paper clip icon to an accessible location.
2) Make the paper clip icon a fixed data: URI string like in the original patch.

I'm still hoping for Richards help to finish this off.
Flags: needinfo?(wavexx)
(In reply to Jorg K (GMT+2) from comment #15)
> 
> Richard, maybe you can help me here. Take the patch and try with your
> Dropbox account. Perhaps we need to shift the icon to a different location,
> I don't see why chrome://messenger/content/cloudfile/attachment-24.png
> wouldn't be found and chrome://messenger/skin/icons/box-logo.png would. Or
> maybe the former location is blocked to content altogether and the latter
> one isn't?

I have to work this evening, so I could do this on late evening.

> Is so, I see two options here:
> 1) Move the paper clip icon to an accessible location.
> 2) Make the paper clip icon a fixed data: URI string like in the original
> patch.

1) would need to move the icon to content/messagebody (see https://dxr.mozilla.org/comm-central/source/mail/base/jar.mn#2) or made through a override like https://dxr.mozilla.org/comm-central/source/mail/base/jar.mn#5
2) is a more simpler way and needs no special treating with overrides etc. When you choose this, then please use my base64 as it is a bit shorter because I optimised the png before conversion.

I'm for 2)

> I'm still hoping for Richards help to finish this off.

I doubt I will do after you have stolen the bug. ;-)
(Assignee)

Comment 17

2 years ago
I'm also for 2) when the reporter confirms that he only places the right icon.

(In reply to Richard Marti (:Paenglab) from comment #16)
> I doubt I will do after you have stolen the bug. ;-)
Borrowed ;-) I fact, I was going to tell you what to copy but then decided that it would be easier to do the snippet myself. Since that works but also doesn't work, I'd appreciate further help since I have other fires burning (see PM).
Assignee: jorgk → richard.marti
(Reporter)

Comment 18

2 years ago
(In reply to Jorg K (GMT+2) from comment #15)
> Reporter: Sorry, I'm not very familiar with this area. Which icon are you
> providing? Only the icon for the service? So the right-hand one?

The filelink extension only provides the right icon (from iconClass()), so that's the important one.

The paperclip is provided by TB itself, so it /could/ be hardcoded as a data: URI.

Since you seem to have problem testing, I've prepared you a test server.
This is the extension you need:

https://addons.mozilla.org/en-US/thunderbird/addon/dl-for-thunderbird/

0.17.2 already works on 52 (with the exception of the icon).
When adding the filelink storage, select "DL", and use:

  rest url: https://gemex.eurac.edu/dltest/rest.php
  user: test
  pass: test
Flags: needinfo?(wavexx)
(Assignee)

Comment 19

2 years ago
Created attachment 8857510 [details] [diff] [review]
1355462-filelink-icons.patch (v2).

OK, I'm stealing it again. I merged my patch and yours, you can review it, not hard, just check that I copied the JS from where I indicated in comment #13.
Assignee: richard.marti → jorgk
Attachment #8857215 - Attachment is obsolete: true
Attachment #8857510 - Flags: review?(richard.marti)
Comment on attachment 8857510 [details] [diff] [review]
1355462-filelink-icons.patch (v2).

You're in luck I had time to check. Now for several hours I have no time.

The patch works with Dropbox and Box.com.

For the JS part please ask aceman too to be sure all is okay.
Attachment #8857510 - Flags: review?(richard.marti) → review+

Comment 22

2 years ago
Comment on attachment 8857510 [details] [diff] [review]
1355462-filelink-icons.patch (v2).

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

::: mail/components/compose/content/cloudAttachmentLinkManager.js
@@ +403,4 @@
>          providerIcon.style.marginRight = "5px";
>          providerIdentity.appendChild(providerIcon);
> +
> +        if (!/^chrome:\/\//.test(aProvider.iconClass)) {

!aProvider.iconClass.startsWith("chrome://") ?

@@ +406,5 @@
> +        if (!/^chrome:\/\//.test(aProvider.iconClass)) {
> +          providerIcon.src = aProvider.iconClass;
> +        } else {
> +          try {
> +            let uri = Services.io.newURI(aProvider.iconClass);

Instead of duplicating all this code, can't you use whole or most of loadBlockedImage() in MsgComposeCommands ?

@@ +420,5 @@
> +            stream.setInputStream(inputStream);
> +            let encoded = btoa(stream.readBytes(stream.available()));
> +            stream.close();
> +            providerIcon.src = "data:image/" +
> +              aProvider.iconClass.substr(aProvider.iconClass.lastIndexOf(".") + 1) +

This seems hacky. The mime type may not exactly match the file extension. It seems loadBlockedImage() handles this nicely already.
(Assignee)

Comment 23

2 years ago
(In reply to :aceman from comment #22)
> > +        if (!/^chrome:\/\//.test(aProvider.iconClass)) {
> !aProvider.iconClass.startsWith("chrome://") ?
Oops, I forgot the "i":
  if (!/^chrome:\/\//i.test(aProvider.iconClass)) {
MsgComposeCommands.js has five of those written by Magnus, so it must be good ;-)

> Instead of duplicating all this code, can't you use whole or most of
> loadBlockedImage() in MsgComposeCommands ?
Use all of it: no, it does some editor stuff at the end.
Most of it: yes, I copied most of it here ;-)

> > +              aProvider.iconClass.substr(aProvider.iconClass.lastIndexOf(".") + 1) +
> This seems hacky. The mime type may not exactly match the file extension. It
> seems loadBlockedImage() handles this nicely already.
Right, I could copy some more of that here.

What do you suggest? Refactor loadBlockedImage() and place it where so I can call it here?

Comment 24

2 years ago
(In reply to Jorg K (GMT+2) from comment #23)
> What do you suggest? Refactor loadBlockedImage() and place it where so I can
> call it here?

I think it should stay where it is and you should be able to call it there. Both files should be loaded in the compose window (but I haven't tried).
Yes, maybe it could take an argument whether to do the editor stuff you mention. Also is it sure you do not need that editor stuff?
(Assignee)

Comment 25

2 years ago
Created attachment 8857990 [details] [diff] [review]
1355462-filelink-icons.patch (v3).

I wasn't so keen on fiddling with the original, hence a copy.
Attachment #8857510 - Attachment is obsolete: true
Attachment #8857510 - Flags: review?(acelists)
Attachment #8857990 - Flags: review?(acelists)
(Assignee)

Updated

2 years ago
tracking-thunderbird_esr52: --- → +
Keywords: regression

Comment 26

2 years ago
Comment on attachment 8857990 [details] [diff] [review]
1355462-filelink-icons.patch (v3).

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

What is the stuff below the commit message?

This is non-trivial block of code, why can't it be shared? How bad would the modified loadBlockedImage() look?
(Assignee)

Comment 27

2 years ago
(In reply to :aceman from comment #26)
> What is the stuff below the commit message?
Which stuff?
diff --git a/mail/components/cloudfile/content/attachment-24.png b/mail/components/cloudfile/content/attachment-24.png
deleted file mode 100644
index 1ea3ad4ad1ec95bda879317ac2c098c403aa8fa4..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
GIT binary patch
literal 0
Hc$@<O00001

That's deleting a file ;-)

> This is non-trivial block of code, why can't it be shared? How bad would the
> modified loadBlockedImage() look?
OK, then we depend on bug 1356303 attachment 8858153 [details] [diff] [review] where I'm introducing what we could use here. We'd have to get it to accept chrome images, though.

Comment 28

2 years ago
(In reply to Jorg K (GMT+2) from comment #27)
> 1ea3ad4ad1ec95bda879317ac2c098c403aa8fa4..
> e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
> GIT binary patch
> literal 0
> Hc$@<O00001
> 
> That's deleting a file ;-)

I have never seen that syntax. But maybe it is for binary files only.
(Assignee)

Updated

2 years ago
Depends on: 1322155
(Assignee)

Updated

2 years ago
Blocks: 1322155
No longer depends on: 1322155
(Assignee)

Comment 29

2 years ago
Hey, why don't you review bug 1356303 for me and then we can make head-way here? And while you're there, do bug 1356245 as well ;-)
(Assignee)

Comment 30

2 years ago
Created attachment 8858557 [details] [diff] [review]
1355462-filelink-icons.patch (v4).

Thanks for the inspiration ;-)
Attachment #8857990 - Attachment is obsolete: true
Attachment #8857990 - Flags: review?(acelists)
Attachment #8858557 - Flags: review?(acelists)

Comment 31

2 years ago
Nice, I will test it soon :)

Comment 32

2 years ago
Comment on attachment 8858557 [details] [diff] [review]
1355462-filelink-icons.patch (v4).

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

I observed the mozmill tests in cloudfile folder and after this patch there are now paperclip icons shown in the blocks informing of a remote attachment.

So it seems to work, thanks.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5687,5 @@
>   * @throw Error()   if reading the data failed
>   */
>  function loadBlockedImage(aURL, aReturnDataURL = false) {
>    let filename;
> +  if (/^(file|chrome):/i.test(aURL)) {

Can you also check the slashes as in other places (/^(file|chrome):\/\// )?
Attachment #8858557 - Flags: review?(acelists) → review+
(Assignee)

Comment 33

2 years ago
Thanks!

(In reply to :aceman from comment #32)
> Can you also check the slashes as in other places (/^(file|chrome):\/\// )?
I hate to disappoint, but we mostly don't check the slashes:
https://dxr.mozilla.org/comm-central/search?q=regexp%3Aif.*i%5C.test%5C(&redirect=false
(Assignee)

Comment 34

2 years ago
https://hg.mozilla.org/comm-central/rev/d4ab2edcaf02449a999eb7eeb3cda8307e720450

Sigh, I've just noticed that in the other spot I do test it:
https://hg.mozilla.org/comm-central/rev/d4ab2edcaf02449a999eb7eeb3cda8307e720450#l4.35
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-thunderbird53: --- → wontfix
status-thunderbird54: --- → affected
status-thunderbird55: --- → fixed
status-thunderbird_esr52: --- → affected
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
(Assignee)

Updated

2 years ago
Attachment #8858557 - Flags: approval-comm-esr52?
Attachment #8858557 - Flags: approval-comm-aurora+

Comment 35

2 years ago
Yes you do, in this same patch :)
(Assignee)

Comment 36

2 years ago
Aurora (TB 54):
https://hg.mozilla.org/releases/comm-aurora/rev/9d19a9b732a01570b748931f63bfec1dadf97fb3

(In reply to :aceman from comment #35)
> Yes you do, in this same patch :)
Yes, I noticed, see comment #34.
status-thunderbird54: affected → fixed
(In reply to :aceman from comment #32)
> > +  if (/^(file|chrome):/i.test(aURL)) {
> 
> Can you also check the slashes as in other places (/^(file|chrome):\/\// )?

FTR: I don't see a good reason to check the slashes. What you get by that is harder to read code. It's not like the url will resolve either way if it's invalid.
(Assignee)

Comment 38

2 years ago
Beta (TB 53):
https://hg.mozilla.org/releases/comm-beta/rev/0af09a53ee131f5862b45b5a6057204b09fe479b
status-thunderbird53: wontfix → fixed
(Assignee)

Comment 39

2 years ago
TB 52 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/2134f77a2cd9b562fab3ada5ccac48c946cc50a9
status-thunderbird_esr52: affected → fixed
(Assignee)

Updated

2 years ago
Attachment #8858557 - Flags: approval-comm-esr52? → approval-comm-esr52+
(Assignee)

Updated

2 years ago
tracking-thunderbird_esr52: + → 53+
You need to log in before you can comment on or make changes to this bug.