Closed Bug 1226869 Opened 10 years ago Closed 8 years ago

Bug 1182546 Breaks some add-ons

Categories

(Firefox :: Extension Compatibility, defect)

Product:
Firefox
Component:
Extension Compatibility
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID
Tracking Flags:
Tracking Status
firefox45 --- affected

People

(Reporter: streetwolf52, Unassigned)

References

Details

(Keywords: addon-compat)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:45.0) Gecko/20100101 Firefox/45.0 Build ID: 20151120103123 Steps to reproduce: Install either add-on 1. OmniSidebar or FindBar Tweak. These are by the same author. I suspect his other add-ons might also be broken. Actual results: Add-ons don't work. Expected results: Add-ons should work.
Regression range: Good: https://hg.mozilla.org/integration/mozilla-inbound/rev/e665231d446bbe3a6df82214548900b65e6c6435 Bad: https://hg.mozilla.org/integration/mozilla-inbound/rev/bd02f1cb9f4fe84ee6a42cafef7d2d5d9784b753 The probable patch causing the problem is 1182546. It first made it's way to inbound a few days ago. At that time the regression range pointed to 1182546. The same day it was backed off and the add-ons worked fine. It is now back on inbound in the bad cset. This is why I suspect it's this patch. I don't have the authority to look at it btw.
I added contentaccessible=yes to the add-ons in question and now they work. I am not the author of these add-ons. How is a developer to know that they might need to add this parameter to their chrome.manifest?
Quicksaver, the author of OmniSidebar and FindBar Tweak, is CCed here. For the others, they likely will be informed that theirs addons are incompatible with the upcoming Firefox version (if there can be test written which checks for the problematic code). The information will be also added to the release notes for developers, like https://developer.mozilla.org/en-US/Firefox/Releases/42 for Firefox 42.
Keywords: addon-compat
(In reply to Gary [:streetwolf] from comment #3) > I added contentaccessible=yes to the add-ons in question and now they work. > I am not the author of these add-ons. How is a developer to know that they > might need to add this parameter to their chrome.manifest? Adding contentaccessible=yes is the right fix, for more details see: > https://developer.mozilla.org/en/docs/Chrome_Registration#contentaccessible
Blocks: 1182546
But how I can fix some Custom Buttons code? For example https://github.com/Infocatcher/Custom_Buttons/tree/master/CB_Source_Editor I got ```Security Error: Content at moz-nullprincipal:{71b5947f-997b-4714-99f8-c24cfa8b85a0} may not load or link to chrome://devtools/locale/sourceeditor.dtd.``` because https://github.com/Infocatcher/Custom_Buttons/tree/master/CB_Source_Editor So, where should I add "contentaccessible=yes"?
Comment 6 is pointing to a situation in which untrusted code is trying to load a built-in Firefox DTD. This case is a bit weird because the untrusted code is only untrusted because DOMParser nerfs the principal on the document it's parsing when called from chrome, because it's supposed to be a "data" document that doesn't load any subresources. Of course this means that it probably shouldn't load DTDs... The fact that it ever did was arguably a bug. But since none of this is web-exposed, we may need to preserve bug compat here. So we should probably make the "DOMParser called from system code tries to load a DTD" case work again. I should note that in general adding "contentaccessible=yes" is NOT necessarily the right fix, since it probably exposes too much stuff unless the DTD is in its own chrome package. The right fix is to stop loading the DTD from untrusted content or move the DTD to its own chrome package and _then_ set "contentaccessible=yes" on that package.
Flags: needinfo?(mozilla)
(In reply to Boris Zbarsky [:bz] from comment #7) > Of course this means that it probably shouldn't load DTDs... The fact that > it ever did was arguably a bug. But since none of this is web-exposed, we > may need to preserve bug compat here. Mhm, should we try to weigh in how many addons are affected by that change before we are going to open up security checks for that case again? > So we should probably make the "DOMParser called from system code tries to > load a DTD" case work again. It's not clear to me what that Custom Button code is doing and why it need access to a Firefox build DTD that is not accessible from content. It seems it's already clear to you, but I would also like to understand what cases exactly should be allowed to load chrome:// and which cases we should continue to block. > The right fix is to stop loading the > DTD from untrusted content or move the DTD to its own chrome package and > _then_ set "contentaccessible=yes" on that package. But maybe that is the right fix then? Would it make sense to move the DTD the addon is trying to load into a separate chrome package and make that content accessible? But then again, we are back to my first question, how many addons are affected, and maybe for this addon it's that DTD and for another addon it's a different build in DTD.
Flags: needinfo?(mozilla)
> before we are going to open up security checks for that case again? To be clear, I'm suggesting that a DOMParser invocation in chrome code should be allowed to load DTDs from chrome://. I suppose that could cause a problem if the text being parsed is untrusted... So yes, it might be worth seeing how many addons are broken. But past that for the ones which are broken, we need to figure out how to fix them. > It's not clear to me what that Custom Button code is doing It looks to me like it modifies the devtools UI a bit and wants to use the same strings as the devtools UI is already using. > Would it make sense to move the DTD the addon is trying to load into a separate chrome > package and make that content accessible Possibly, yes. The question is whether this is an isolated case or whether there will be more problems like that.
I do not know whether it would be useful, but same errors with dtd in: https://github.com/diegocr/CleanLinks/issues/105 https://github.com/Infocatcher/Right_Links/issues/23 "Advanced Locationbar" and other, that opens their option page in detail-view-container.
(In reply to Boris Zbarsky [:bz] from comment #7) > I should note that in general adding "contentaccessible=yes" is NOT > necessarily the right fix, since it probably exposes too much stuff unless > the DTD is in its own chrome package. The right fix is to stop loading the > DTD from untrusted content or move the DTD to its own chrome package and > _then_ set "contentaccessible=yes" on that package. And will be the way to detect, that this extension is installed? Yet another fingerprinting to track users...
(In reply to Boris Zbarsky [:bz] from comment #9) > The question is whether this is an isolated case or whether > there will be more problems like that. In the case of my add-ons, they are simply loading their preferences in a tab and trying to access the add-on's own DTD files, and sometimes a few native DTDs (it's hard to troubleshoot which ones were causing the add-ons to stop working specifically, but I'm guessing it's the add-on ones). There are also other bits and pieces around that required entities in those DTDs (menu entries, misc labels, etc). Just judging from that, and I may be completely wrong here as I don't know every add-on out there and I don't have access to bug 1182546 to know what's happening exactly, it seems to me that every add-on that has more UI than a simple toolbar button created through the CustomizableUI module (or the equivalent SDK module) will be affected, because they're definitely going to get all those labels and tooltips and descriptions and stuff from the DTDs; that's why they're there anyway.
XMLHttpRequest does not work any more, too, which affects e.g. AdBlock Plus (bug 1226823 comment 13) and some of own add-ons. Trusted chrome: content is treated as untrusted there (because XHR is always untrusted). I found a work-around for the XHR case, tho (bug 1226823 comment 15). Many DOMParser code can be replaced with XHR (+ workaround) I'd guess.
> And will be the way to detect, that this extension is installed? Yes, that is the tradeoff with contentaccessible=yes... > In the case of my add-ons, they are simply loading their preferences in a tab Using what sort of URI?
(In reply to Boris Zbarsky [:bz] from comment #14) > > In the case of my add-ons, they are simply loading their preferences in a tab > > Using what sort of URI? I don't fully understand the question. Here's a quick example (amongst many) from one of my add-ons from which I removed the contentaccesssible=yes flag, leaving a chrome.manifest like this: > content findbartweak chrome/content/ > locale findbartweak en-US chrome/locale/en-US/ The add-on uses an XHR to load a XUL file from chrome://findbartweak/content/paneGeneral.xul. It fails with the following errors in the console: > Security Error: Content at moz-nullprincipal:{b0386d7e-bcc3-453e-85d3-f0c1711d3b4a} may not load or link to chrome://findbartweak/locale/options.dtd. > undefined entity paneGeneral.xul:7:3 in addition to the expected parsing error in that file: > XML Parsing Error: undefined entity I did notice that if I type that address directly in the location bar, it works (it doesn't fail to fetch the entities file). Do the new restrictions behave differently in this case? The XHR is sent from within the bootstrapped environment if that's relevant.
> The add-on uses an XHR to load a XUL file from chrome:// OK, then sure. That's covered by comment 13 (though you should NOT use the workaround suggested there, for the reasons discussed in bug 1226823). The bits of comment 7 about DOMParser also apply to XHR in terms of its security behavior. This setups is not "simply loading their preferences in a tab"... ;)
> This setups is not "simply loading their preferences in a tab"... ;) You are very right, I'm sorry about that. When I had first looked into it, I mistakenly thought the problem was in the page that was actually loaded in the tab, and not in the XHRs behind it. From what I'm reading in bug 1226823, I can more or less deduce that the contentaccessible=yes flag isn't the most accurate fix in my case either then. Can you tell me if that's correct?
Yeah. Like I said in that bug, I think we should adjust what we're doing on the Gecko end here...
I figured, thanks. In the meantime, what do you believe will happen, short-term? Will the patch be backed out until the security policies/restrictions are refined? I ask because it's not trivial to keep working versions of an add-on for Firefox release and Nightly in this case. Without contentaccessible=yes it won't work in Nightly, and with it may apparently open unforeseen security issues; I'm only extrapolating from what I've read so far. (I wonder if the following would actually work in keeping the current behavior for Release/Beta/DevEdition, while temporarily allowing the extension to work in Nightly) > content myaddon chrome/content > content myaddon chrome/content appversion>=45.0a1 contentaccessible=yes
> Will the patch be backed out until the security policies/restrictions are refined? That's the current plan, yes. Hopefully get that done today.
Blocks: 1226823
QA Whiteboard: [seamonkey-2.42-affected]
status-firefox45: --- → affected
OS: Unspecified → All
Hardware: Unspecified → All
Version: 45 Branch → Trunk
This issue is not resolved for me in the latest tree with the Firegestures addon, although #1228116 theoretically backs out these changes (and I've verified that my Mercurial tree contains the backout changesets and I've rebuilt Firefox from ground zero). It's possible that this is a separate bug that was being masked by this bug, but this issue is definitely what my bisection identified as the point where Firegestures broke. The browser console reports errors in Firegesture's code like: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "res is null" {file: "jar:file:///u/cks/tmp/cks-ffox-hg/.mozilla/firefox/0neujyc4.default/extensions/firegestures@xuldev.org.xpi!/components/xdGestureMapping.js" line: 122}]'[JavaScript Error: "res is null" {file: "jar:file:///u/cks/tmp/cks-ffox-hg/.mozilla/firefox/0neujyc4.default/extensions/firegestures@xuldev.org.xpi!/components/xdGestureMapping.js" line: 122}]' when calling method: [xdIGestureMapping::init] xdGestureService.js:69:0 NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this._gestureMapping is null" {file: "chrome://firegestures/content/browser.js" line: 101}]'[JavaScript Error: "this._gestureMapping is null" {file: "chrome://firegestures/content/browser.js" line: 101}]' when calling method: [xdIGestureObserver::onDirectionChanged] xdGestureHandler.js:413:0 TypeError: this._getLocaleString is not a function browser.js:121:6 NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "this._getLocaleString is not a function" {file: "chrome://firegestures/content/browser.js" line: 121}]'[JavaScript Error: "this._getLocaleString is not a function" {file: "chrome://firegestures/content/browser.js" line: 121}]' when calling method: [xdIGestureObserver::onMouseGesture] xdGestureHandler.js:447:0 Reproduction for me is simple with Firegestures, because no gestures work (things don't even report on what gesture you're making, although the mouse trails do appear).
Flags: needinfo?(mozilla)
Even with Bug 1228116, DOM Inspector still isn't working
Blocks: 1227554
Flags: needinfo?(philip.chee)
The DOM Inspector and Firegesture share the same error, see: https://bugzilla.mozilla.org/show_bug.cgi?id=1227554#c9
Flags: needinfo?(mozilla)
So, what is the fix here? Can the addon be manually fixed in the meantime somehow?
(In reply to Honza Bambas (:mayhemer) from comment #25) > So, what is the fix here? Can the addon be manually fixed in the meantime > somehow? Bug 1228116 and also Bug 1227554 should have fixed all of the problems described in this bug. If there are no further issues we can mark this bug as fixed.
Confirming fixed for FireGestures (1.10.3) on Nightly 45.0a1 (2015-12-04).
Flags: needinfo?(philip.chee)
With WebExtensions being the only valid way of doing extensions in Firefox 57, I don't think this bug is still relevant.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.