browser_action icon is not shown (unless it is listed in web_accessible_resources)
Categories
(Thunderbird :: Add-Ons: Extensions API, defect)
Tracking
(thunderbird_esr91 unaffected, thunderbird93 unaffected, thunderbird94+ wontfix)
Tracking | Status | |
---|---|---|
thunderbird_esr91 | --- | unaffected |
thunderbird93 | --- | unaffected |
thunderbird94 | + | wontfix |
People
(Reporter: TbSync, Assigned: TbSync)
References
(Regression)
Details
(Keywords: regression)
Attachments
(2 files)
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).
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
compose_action and message_display_action buttons are not affected.
Assignee | ||
Comment 2•3 years ago
|
||
Introduced by commit :
changeset: 33796:1392ebed9caf
user: Henry Wilkes <henry@thunderbird.net>
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 <kaie@kuix.de>
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 <bugs@rmf.io>
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.
Assignee | ||
Comment 3•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Comment 4•3 years ago
|
||
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.
Assignee | ||
Comment 5•3 years ago
|
||
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.
Assignee | ||
Comment 6•3 years ago
|
||
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://?
Updated•3 years ago
|
Comment 7•3 years ago
|
||
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.
Assignee | ||
Comment 8•3 years ago
|
||
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•3 years ago
|
||
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.
Assignee | ||
Comment 10•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
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•3 years ago
|
||
(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•3 years ago
|
||
(In reply to Henry Wilkes [:henry] from comment #14)
...
If we can get theskin
security policy to acceptmoz-extension
sources this should fix the issue. Is the problem that we're relying on mozilla central for this?
Comment 17•3 years ago
|
||
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.
Assignee | ||
Comment 18•3 years ago
|
||
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•3 years ago
|
||
(In reply to John Bieling (:TbSync) from comment #18)
How does that sound?
But then please also fix bug 1735529 comment 2.
Assignee | ||
Comment 20•3 years ago
|
||
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.
Assignee | ||
Comment 21•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 22•3 years ago
|
||
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•3 years ago
|
||
(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.
Assignee | ||
Comment 24•3 years ago
|
||
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.
Assignee | ||
Comment 25•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 26•3 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/caaedc96fda6
Temporary fix for moz-extension:// urls denied in skin resources. r=mkmelin
Updated•3 years ago
|
Description
•