1733060 - browser_action icon is not shown (unless it is listed in web_accessible_resources)

Status: RESOLVED FIXED

(Thunderbird :: Add-Ons: Extensions API, defect)

Description

[John Bieling (:TbSync)]

It appears that browser_action buttons no longer show their icons, if the icon itself is not listed in the web_accessible_resources entry of the add-on's manifest (which should not be necessary).

Comment 1

[John Bieling (:TbSync)]

compose_action and message_display_action buttons are not affected.

Comment 2

[John Bieling (:TbSync)]

Introduced by commit :

changeset: 33796:1392ebed9caf
user: Henry Wilkes <[e-mail]>
date: Wed Sep 15 13:27:16 2021 +0300
summary: Bug 1707211 - Merge messenger.css content into shared/skin. r=mkmelin
(https://phabricator.services.mozilla.com/D125420)

Last working commit :

changeset: 33795:9ef7f431cd0b
user: Kai Engert <[e-mail]>
date: Wed Sep 15 13:26:28 2021 +0300
summary: Bug 1704820 - Fix buffers to allow decrypting binary attachments with external GnuPG. r=mkmelin

Used m-c revision for test builds:

changeset: 591942:eb333d4ecb10
user: R. Martinho Fernandes <[e-mail]>
date: Tue Sep 14 18:11:05 2021 +0000
summary: Bug 1713605 - Avoid NSS usage in CertVerifier::VerifySSLServerCert r=keeler

Edit: Added Phabricator link to offending patch.

Comment 3

[John Bieling (:TbSync)]

Attachment: missing-icon.png

Install a simple add-on which uses a browser action button, for example:
https://addons.thunderbird.net/addon/addon-compatibility-check/

Browser action button does not show the icon.

Comment 4

[Henry Wilkes (they/them) [:henry-x]]

The content security is blocking the image source. Using the extension you suggested I get the error message

Security Error: Content at chrome://messenger/skin/shared/messenger.css may not load or link to moz-extension://02ebfc23-3b43-46c5-aec2-3ffc281b82a9/skin/puzzle-piece32.png.

Note this is referring to the list-style-image rule in the css https://searchfox.org/comm-central/rev/f96a8d19993aa49a1e482430c634255377b788a2/mail/themes/shared/mail/messenger.css#443-501

I'm not sure what web_accessible_resources is myself, but maybe you know if this would somehow approves the resource for the CSS security policy, or leads to the source being set directly in javascript outside of the CSS file.

Also, it is strange that this was triggered by revision 1392ebed9caf: it just moved the rules that were in content/messenger.css into skin/shared/messenger.css. I guess somehow the content/messenger.css file was able to slip through the same security policy :/

Note that a similar thing happened in Bug 1726341, but that was because mozilla-central changed the resource path. There it was fixed without changing the security policy by using a html attribute (an <img> src in that case, I think this would be image for the <xul:toolbarbutton>) instead of the css list-style-image rule.

Comment 5

[John Bieling (:TbSync)]

I'm not sure what web_accessible_resources is myself, but maybe you know if this would somehow approves the resource for the CSS security policy, or leads to the source being set directly in javascript outside of the CSS file.

That was just a side node. I saw that adding the img to web_accessible_resources fixed it and maybe it would give a hint on what happened, but that is not something we can use as real fix. We need to revert to the situation that the image is shown automatically.

Also, it is strange that this was triggered by revision 1392ebed9caf: it just moved the rules that were in content/messenger.css into skin/shared/messenger.css. I guess somehow the content/messenger.css file was able to slip through the same security policy :/

So that is something we need to understand, why is skin/shared/messenger.css handled differently? Maybe Magnus and/or Geoff knows?

Note that a similar thing happened in Bug 1726341, but that was because mozilla-central changed the resource path. There it was fixed without changing the security policy by using a html attribute (an <img> src in that case, I think this would be image for the <xul:toolbarbutton>) instead of the css list-style-image rule.

That will break all our action buttons being generated with the same template. If this is inevitable, we might have to do that.

Comment 6

[John Bieling (:TbSync)]

Oh, it changed from a chrome/content to a chrome/skin URL and if I remember correct chrome/skin urls have the same limitations as resource urls..

So is it possible to allow chrome/skin to access moz-extensions://?

Comment 7

[Magnus Melin [:mkmelin]]

Maybe something with "contentaccessible=yes" can achieve it?
I don't think chrome/content and chrome/skin should need to have different policies now. In the XBL days there might have been a need for it.

Comment 8

[John Bieling (:TbSync)]

Maybe something with "contentaccessible=yes" can achieve it?
I was not able to get it working.

Can someone tell me why exactly we moved from content to skin, with skin being on death-row?
https://bugzilla.mozilla.org/show_bug.cgi?id=1385444

I was not yet able to fix this regression introduced by D125420, so it will be merged into Beta.

Could Henry move all skin usages to chrome?

Comment 9

[Magnus Melin [:mkmelin]]

I didn't know about that 4 year old bug, which has had no traction since. Doesn't seem like it's happening, and the majority of it is now in skin.
I wouldn't move the styles to chrome unless firefox also wanted to do so.

Comment 10

[John Bieling (:TbSync)]

Ok, but then we need a way forward here. Firefox has the style definitions in question still in content. Can we revert just those?

Comment 11

[Magnus Melin [:mkmelin]]

Would be good to get to the bottom of this problem.
If we don't figure it out, maybe just an override in the jar.mn file to map the content css url to the skin one?

Comment 12

[Geoff Lankow (:darktrojan)]

Yes, the rules in question will have to go back to content. I suggest creating a separate sheet just for these rules. While we're at it I notice there's a second set of rules in messengercompose.css which use the wrong class name and are never applied. They should be thrown out.

Comment 14

[Henry Wilkes (they/them) [:henry-x]]

(In reply to Geoff Lankow (:darktrojan) from comment #12)

Yes, the rules in question will have to go back to content. I suggest creating a separate sheet just for these rules. While we're at it I notice there's a second set of rules in messengercompose.css which use the wrong class name and are never applied. They should be thrown out.

Note that there would have been similar security problems for themes from other non-messenger skin resources before the patches in bug 1707211 (see bug 1735529 comment 2). So we need a way to solve them as well, and I'm not sure creating a separate content resource is the best solution for each of these.

If we can get the skin security policy to accept moz-extension sources this should fix the issue. Is the problem that we're relying on mozilla central for this?

Comment 16

[Wayne Mery (:wsmwk)]

(In reply to Henry Wilkes [:henry] from comment #14)

...
If we can get the skin security policy to accept moz-extension sources this should fix the issue. Is the problem that we're relying on mozilla central for this?

Comment 17

[Henry Wilkes (they/them) [:henry-x]]

Note that this the part where the moz-extension source is rejected https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/caps/nsScriptSecurityManager.cpp#911-912

And this is the part that sets the URI_DANGEROUS_TO_LOAD flag https://searchfox.org/mozilla-central/rev/2e3b0483e31abffe0b4374480a34c6d23f5186ea/netwerk/protocol/res/ExtensionProtocolHandler.cpp#425-430 Note the exception for IsWebAccessiblePath, which would explain why it is allowed if listed in web_accessible_resources.

I'm not sure how the content resources get around this.

Comment 18

[John Bieling (:TbSync)]

Looks like you are getting closer!

I spoke with Magnus and I would like to propose the following:

• I work on a patch, that returns to content URLs for the important parts, so we have a working Thunderbird again.
• Henry continues to investigate if we can get skin URLs to work as desired. As soon as that works, my patch can be backed out again.

How does that sound?

Comment 19

[Richard Marti (:Paenglab)]

(In reply to John Bieling (:TbSync) from comment #18)

How does that sound?

But then please also fix bug 1735529 comment 2.

Comment 20

[John Bieling (:TbSync)]

CheckLoadURIFlags() is never called for resources loaded from chrome/content, so the check for URI_DANGEROUS_TO_LOAD is never reached. I think the flag is still set though.

Comment 21

[John Bieling (:TbSync)]

Attachment: Bug 1733060 - Temporary fix for moz-extension:// urls denied in skin resources. r=mkmelin

Comment 22

[John Bieling (:TbSync)]

The proposed patch is using two different strategies. For the action buttons, I created a new css file in content which includes the problematic image definitions.

To fix the theme issue reported in bug 1735529, this did not seem feasible, as it requires to move a ton of code snippets. Instead, I forced the theme icons to use a file:// url instead of a moz-extension:// url, which seems to work even if used from skin resources. Not pretty.

Comment 23

[Henry Wilkes (they/them) [:henry-x]]

(In reply to John Bieling (:TbSync) from comment #18)

• Henry continues to investigate if we can get skin URLs to work as desired. As soon as that works, my patch can be backed out again.

I'm not sure what should be done. All the relevant code seems to be in mozilla-central, so it might be better to pass this on to there. I'm not sure who though.

Comment 24

[John Bieling (:TbSync)]

I'm not sure what should be done. All the relevant code seems to be in mozilla-central, so it might be better to pass this on to there. I'm not sure who though.

Not really. They did not move their styles using moz-extension:// urls into skin. Only we did that.

Comment 25

[John Bieling (:TbSync)]

What I wanted to say is that they will probably see no need to fix this, as nothing is broken for them. You are of course correct, that this has to be fixed in m-c, but probably by us, not by them. You could create a bug and see what they say.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/caaedc96fda6
Temporary fix for moz-extension:// urls denied in skin resources. r=mkmelin