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.

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?

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.

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.

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 :-(

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.

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.

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.

(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.

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.

(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 .

OK, you convinced me. Let me see what I can do.

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.

Comment on attachment 8857165 [details] [diff] [review] Bug1355462.patch Sorry, Richard. My fault, my suggestion was a bad idea.

(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.

(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. ;-)

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).

(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

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.

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.

Comment on attachment 8857510 [details] [diff] [review] 1355462-filelink-icons.patch (v2). JS part copied from here: https://dxr.mozilla.org/comm-central/rev/b08dc613a3d275711f76b42cf0ceba1119d82f02/mail/components/compose/content/MsgComposeCommands.js#5681

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.

(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?

(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?

I wasn't so keen on fiddling with the original, hence a copy.

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?

(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.

(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.

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 ;-)

Thanks for the inspiration ;-)

Nice, I will test it soon :)

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):\/\// )?

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

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

Yes you do, in this same patch :)

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.

(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.

Beta (TB 53): https://hg.mozilla.org/releases/comm-beta/rev/0af09a53ee131f5862b45b5a6057204b09fe479b

TB 52 ESR: https://hg.mozilla.org/releases/comm-esr52/rev/2134f77a2cd9b562fab3ada5ccac48c946cc50a9