Closed Bug 146401 Opened 19 years ago Closed 19 years ago
need XSLT enable/disable pref
I think we're going to want to take this for 1.0.1, but it's a useful backstop in case of late-breaking issues with XSL-T.
I'll attach a UI-less patch shortly, once I get the darned thing working.
Status: NEW → ASSIGNED
Summary: need XSL-T enable/disable pref → need XSLT enable/disable pref
Target Milestone: --- → mozilla1.0.1
Why exactly do we need this? If you don't want XSLT in the build, just remove the transformiix library.
as the 1st comment says, it's probably about breaking features. If we want this, it should be like the js prefs, that is, the switch should be made available for specific sites/domains.
Drivers and others want this patch, to protect against future bugs in XSLT. Not everyone can remove the DLL in their installation, and it's a lot harder to write instructions for all platforms on how to find and remove it than to tell them how to flip the pref. Also, turning XSLT back on after removing the DLL requires a lot of foresight and care, else a reinstall. This patch doesn't use an entity for the pref label, because the UI freeze date has come and gone, and I'm willing to bet that nobody would actually make "XSLT" into a different string. In a better world, we've have better text for it, but that's a post-1.0 thing.
What sort of future bugs are we talking about and how is this different for the Transformiix module than for other code? Why no pref for turning off CSS for example? I could understand that you want to block stylesheet loads for a document in the same way that you block image loads etc., but you would implement that in the content module and make it work with all kinds of stylesheet loads and it would allow host specific setting, etc. Why did no one (not drivers nor anyone of the "others" shaver mentions) discuss this with anyone working on the XSLT module? I think we're reasonable people, we try to respond to requests for help and comments and we do fix bugs. Railroading in a fix like this does certainly demoralize me.
Comment on attachment 84761 [details] [diff] [review] Pref with (non-localizable) UI for XSLT switching sr=jst
Attachment #84761 - Flags: superreview+
1. shaver says drivers spoke with heikki 2. there's at least one bug about css, bug 32372 '. i'm going to try to make a very redimentry patch to disable css
Comment on attachment 84761 [details] [diff] [review] Pref with (non-localizable) UI for XSLT switching Duh, this is no good, we need some refcounting fixes here... Shaver's on it...
Attachment #84761 - Flags: needs-work+
I'd like to blame lack of sleep, but the fact is that I'm just an idiot.
Attachment #84761 - Attachment is obsolete: true
Comment on attachment 84788 [details] [diff] [review] Now with the all.js part and real refcounting. (Shame on me.) sr=jst
Attachment #84788 - Flags: superreview+
Peter, this was discussed at the drivers/staff meeting on Wednesday. After the recent security problem with xmlhttprequest and the fact that xsl contains similar functionality we thought it might be worth it to make sure that it was possible for a user to disable the feature if there was another problem found in the XSLT module. You have to admit that XSLT is a large chunk of code, that allows you to do a lot of things with data from a lot of different sources. CSS allows you to do similar things but doesn't have the breadth that XSLT does so XSLT seems like the first natural target. We thought that it was worth the time and effort to make it possible to turn it off. We knew that you were busy with other things so shaver was volunteered to help you out. Sorry about the last minute nature of this, but we're running up against the wall on a release and we're trying to make it easier on you, if possible.
I guess it was easiest to do it this way, but it would be kind of nice to have a complete pref panel for XML features. I tried to do that once but the prefs never saved. Anyone have cycles to provide an XML panel that saves its prefs, I could then take over to add the stuff that I think would benefit from prefs?
So, I might see an argument for rushing something in for 1.0. Not very good ones, but wth. I don't want this patch on the trunk though as it's plain wallpaper. E.g. the rationales given here may work for XSLT, but they don't work for the XPath DOM functionality. And the pref says nothing about the side effects that this patch induces, as switching off XPath, and not only XSLT. Plus, this patch probably conflicts with work in progress, and so harms the little work we can do on the trunk at this stage. PS: I assume that drivers are aware that they pretty much screwed any procedure one could come up with in such a situation.
BTW, "wallpaper" connotes adding a null-check to avoid a crash, when invariants that should preclude a null value somehow varied, and we don't have time to figure out why. Worse, "spackle" is when you add a null-check, but the pointer is probably corrupt and may be non-zero but invalid. We don't like either, but we'll take wallpaper at the last minute if the crash is frequent (topcrash). Adding a pref (for JS, CSS, XSLT, or other "executable content"), OTOH, is neither wallpaper nor spackle. It's "belt and braces", and it is good for users who may need it if there is a firedrill we can't foresee. Such users cannot easily remove a .dll or .so file, and probably won't reinstall the library later, when the coast is clear. /be
re be, I was not talking against the pref itself, but about the nature of the current patch. Heikki et al, see comment #3 for example. Note that this comment has been made before the patch came, I don't see this addressed yet. The pref only talks about XSLT. And yes, XSLT is a complex engine with parts in the spec that allow evil things to be done. Not just potential bugs in our impl, but more or less intentional bad things in standards-conforming content. Note the recursive template problem for example. So a pref is useful, but as I suggested, it makes more sense if it's customizable, like the js dom prefs are. As to side effects,  XSLT does not imply XPath, nor does it mention DOM level 3. XSLT bases on XPath, but the evil it can do has nothing to do with the nature of XPath. Oh, is netscape aware that switching off XSLT like this will disable the P3P policy viewer, too?
Axel: no one is switching XSLT off by default. BTW, mozilla.org doesn't care about p3p for 1.0. /be
I don't think disabling any feature per site is sufficient if there is a security hole. If you go to a site for the first time it will not be on your list and by the time the page starts loading it is too late (this could work better if you explicitly deny all but few trusted sites). Or if you REALLY wanted to make it fully customizable you'd have a dialog box alerting you to every feature on the page and asking you if you want to block it for that page or allow it etc. That is obviously too much for anyone to do in practice. Disabling popup adds per site, or window resize per size etc. is ok though, because it is not about hiding a security hole but denying sites to annoying things you don't want to experience. Disabling XSLT of course means that any page using it is totally unusable so it is not about annoyance factor. Or maybe you meant disabling certain features of XSLT, possibly based on site. That would be slightly better, but my understanding of XSLT is that if you disabled document function (for example) for all sites expect the ones on your explicit allow list, you'd get some very unpredictable results on sites that used it legitimately and were blocked from doing this. In any case, this is obviously too much work to have any change of getting into 1.0: the sledgehammer approach is the only one we can safely take. I am worried about your comment that the XSLT spec itself allows bad things to happen. Please file a security sensitive bug where we can discuss this more. The XSLT pref is a perception thing I guess. I assumed automatically that it would also disable XPath, but I see some people wouldn't make that connection. So after 1.0 we need more explanation/configurability. Please file a new bug for more advanced pref settings and we can work things out there. The summary view in policy viewer would break if XSLT was disabled. We can live with it.
Comment on attachment 84788 [details] [diff] [review] Now with the all.js part and real refcounting. (Shame on me.) a=tor,scc,(chofmann|brendan) assuming this gets an r=
Attachment #84788 - Flags: approval+
Comment on attachment 84788 [details] [diff] [review] Now with the all.js part and real refcounting. (Shame on me.) r=peterv. I'll file follow-up bugs to implement this in a more configurable way.
Attachment #84788 - Flags: review+
I'm in on the branch. I think we're going to take the more flexible version peterv's filing about for the trunk, so this baby's closed.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
we didn't verify for a long time. I really checked, so VERIFIED.
Status: RESOLVED → VERIFIED
No further news for the trunk? I am trying to isolate some bugs (e.g., bug 184159, bug 189338, bug 189725) and such a pref to bypass XSLT for a moment would be useful, even if it just for debugging/perception purposes.
You need to log in before you can comment on or make changes to this bug.