236839 - Disabling JavaScript should not prevent JavaScript contained in chrome XBL from running

Status: RESOLVED FIXED

(Core :: XBL, enhancement)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

The editor disables JavaScript because it makes no sense to edit a dynamic
document. That also disables the execution of all JavaScript code contained in
XBL bindings, wherever these bindings come from, chrome or not. In particular,
the constructors/destructors of the bindings are not executed...

This is a blocker for the implementation of templates in Composer.  I could use
workarounds for methods and handlers, but hardly for constructors and
destructors. And XBL remains the simplest solution to do that; the cost of a c++
implementation is prohibitive in comparison with the XBL solution.

It is then proposed to enable JavaScript code contained in chrome XBL bindings
attached through a chrome stylesheet. Given the existence of bug 208864, I think
this mechanism should not be limited to the editor, and the browser should keep
running XBLs when they come from the chrome even if JS is disabled.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So to summarize the discussion we've had up to now, as I understand it:

1)  Disabling JS for the document and disabling JS for the nsJSContext should
    not be the same thing.  Editor wants to do the former not the latter (it
    does the latter now).
2)  The nsJSContext api for compiling/evaluating/etc scripts should always take
    an nsIPrincipal for the caller and only use the ScriptGlobalObject principal
    if the caller doesn't pass one.
3)  The "JS disabled for the document" check should allow execution of JS with
    chrome principals.
4)  XBL should pass in principals as needed.

I'm happy to do steps 2 and 4, but I'm not quite sure what people would like to
do for step 1, and step 3 yet.....

Comment 2

[Johnny Stenback (:jst)]

I don't think that's quite what we agreed on (unless I missed some emails). I
don't think we should have more than one "JS is disabled" flag per
document/context, we need another flag that's separate from whether JS is
enabled or not that just tells if the document is "interactive" or not, and that
affects much more than just JS execution (link handling, form submission, etc).

I'd like to first settle on the two separate "states" we want. I proposed adding
an "interactive" property to nsIDocument (i.e. IsEnabled() and
SetIsEnabled(PRBool)), can we agree on this so far?

As for #3, I don't think we want that, if JS is disabled, JS is disabled, no
matter who calls it. IMO.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.. that is what we'd come to, yes.  My apologies for misremembering.

So with your proposal, the situation, as far as I can tell, is as follows:

PROPOSED ARCHITECTURE:
1)  JS can be disabled on nsJSContext
2)  JS can be disabled via prefs; security manager handles this
3)  Document can be non-interactive

With those three settings all sort of orthogonal to each other.  The behavior we
are proposing is then:

1)  If JS is disabled on nsJSContext, no JS executes, period.
2)  If JS is enabled on nsJSContext but the document is non-interactive, only
    execute JS that's coming from a chrome principal (fixes composer's issues).
3)  If JS is enabled on nsJSContext and the document is interactive, still allow
    chrome JS execution no matter what the user prefs are set to (fixes XBL form
    control issues and bug 208864).

The only issue with all that is that <marquee> would animate in composer with
that proposal.  This can probably be solved by just setting the -moz-binding on
it to some other value in the editor's stylesheets.

I'm in favor of SetInteractive() and Interactive() myself (rather than enabled).
 Seems to describe the situation more clearly....

Comment 4

[Peter Van der Beken [:peterv]]

So that wouldn't solve the issue for mail? I remember a mail bug (don't have the
number right now) where context menus on scrollbars weren't disabled on Mac
because the XBL JS for the scrollbars didn't run unless you enabled JS in mails.
Or would mail use the non-interactive setting? (I'm asking because from your
proposal it seems non-interactive disables some stuff that mail might want enabled)
The scrollbar bug got fixed in some other way btw, so it's probably a FIXED bug.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #3) 
> The only issue with all that is that <marquee> would animate in composer with
> that proposal.  

Does text-decoration:blink blink in composer? if it does, why should marquee not
animate?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Because it's possible to edit blinking text but not really possible to edit
moving text?  ;)

Comment 7

[Johnny Stenback (:jst)]

Um, I don't know where I pulled that IsEnabled/SetEnabled from, I meant to say
IsInteractive/SetInteractive. I think I'd prefer IsInteractive() over just
Interactive, but I think I can deal either way :-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

One issue here.... If the XBL JS is compiled with a chrome principal, does that
mean that a page can call a method on the binding and have it execute with
chrome priveleges?  If so, we need to carefully audit all methods that are
exported on chrome bindings that content can load.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note previous discussion in bug 59701 

Comment 10

[William Price]

Any chance of reviving work on this bug?  It's blocking XUL error pages in
addition to some other interesting bugs.

Comment 11

[Boris Zbarsky [:bzbarsky]]

This bug doesn't really affect error pages in any way.  Whoever added that
dependency is a little confused.

This bug _is_ important, and there _is_ interest in working on it.  It's not
clear what should happen here, however, and it's a hard problem to solve, in
general, without opening up security holes.  We're probably going to try to
solve it in the 1.9 cycle, though.

Comment 12

[Ben Basson]

Boris, I've just run into this problem (or something extremely similar) during
my rewrite of Image Toolbar for Firefox. Asqueella pointed out this bug and
something occurred to me that may be relevent. 

Currently, I'm using nsIStyleSheetService to overlay custom CSS (which includes
XBL bindings) onto content documents. This has the advantage of making my code
extremely efficient and work in 100% of the cases it should (AFAIK).

With JavaScript disabled, XBL event JavaScript fails to fire, which means that
essentially, my extension is rendered useless. If you're working on this in the
future then I can hackaround or put up with the emails until then.

Possible suggestion:

If CSS (w/XBL) is being inserted from the chrome, it would make sense to me if
at some point during that insertion it was placed inside a wrapper (if that even
makes sense in this context!) and executes in the chrome scope.

It seems that if code is being injected from chrome then it should retain that
scope (and security access) so long as it's unmodified. Making sure it's
inserted in this way would probably simplify this issue somewhat.

Disclaimer: I'm hardly an authority on Mozilla, C++ or security, but it would
make sense to me if this bug could be partially fixed that way.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> and executes in the chrome scope.

And this is the part that's got security hole potential like no tomorrow... 
Especially if the page can trick said XBL on operating on whatever content the
page passes it.

There's the separate issue that if the XBL is on the chrome wrapper (so has
chrome privileges), then the unprivileged page can't call those XBL methods. 
Which is very undesirable in a lot of cases.

Comment 14

[Doron Rosenberg (IBM)]

(In reply to comment #13)
> There's the separate issue that if the XBL is on the chrome wrapper (so has
> chrome privileges), then the unprivileged page can't call those XBL methods. 
> Which is very undesirable in a lot of cases.

If that is ever allowed, we need to have some security review of the XBL code so
that unpriv. code can't trick the chrome xbl into executing stuff with chrome
privs (like my recent PFS code injection bug).

Comment 15

[Giorgio Maone [:ma1]]

(In reply to comment #13)
> > and executes in the chrome scope.
> 
> And this is the part that's got security hole potential like no tomorrow... 
> Especially if the page can trick said XBL on operating on whatever content the
> page passes it.
> 

Notice that the very popular (and loved by Asa) GreaseMonkey extension (and its
children, like Patyplus) doesn't work when/where JS is disabled.
FlashBlock (a Macromedia Flash content blocker) doesn't work either, and this is
in my opinion less acceptable because someone who wants to block Flash
selectively on some pages may reasonably want to prevent JavaScritp execution as
well.
Both extensions use XBL to modify page content and inject scripts.
They need JS XBL-injected by the chrome to run even if JS is disabled.

Comment 16

[Giorgio Maone [:ma1]]

(In reply to comment #13)
> > and executes in the chrome scope.
> 
> And this is the part that's got security hole potential like no tomorrow... 
> Especially if the page can trick said XBL on operating on whatever content the
> page passes it.
> 

Notice that the very popular (and loved by Asa) GreaseMonkey extension (and its
children, like Patyplus) doesn't work when/where JS is disabled.
FlashBlock (a Macromedia Flash content blocker) doesn't work either, and this is
in my opinion less acceptable because someone who wants to block Flash
selectively on some pages may reasonably want to prevent JavaScritp execution as
well.
Both extensions use XBL to modify page content and inject scripts.
They need JS XBL-injected by the chrome to run even if JS is disabled.

Comment 17

[Philip Chee]

Bug 159932 either dependency or duplicate? "disabling javascript in browser
doesn't allow chrome:// to add eventListners to window"

Comment 18

[Boris Zbarsky [:bzbarsky]]

Neither dep nor dup.

Comment 19

[Brendan Eich [:brendan]]

We need to do a bunch of XBL work for 1.9, and this bug is just part of that
lot. cc'ing peterv and sicking.  This bug really needs a new assignee.

/be

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What I think we should do for now (until we dare go for bug 59701) is to let JS
from chrome XBL run even when JS is disabled, but with page privileges. This is
how XBL behaves when JS is disabled on docshell level.

Comment 21

[Johnny Stenback (:jst)]

Not a blocking issue.

Comment 22

[Jean-Philippe Fleury]

(In reply to comment #16)
> FlashBlock (a Macromedia Flash content blocker) doesn't work either, and this is
> in my opinion less acceptable because someone who wants to block Flash
> selectively on some pages may reasonably want to prevent JavaScritp execution as
> well.

+1. I'm on this situation. Is there a hope to see this old bug to be solved one day?

Comment 23

[Giorgio Maone [:ma1]]

(In reply to comment #22)
> (In reply to comment #16)
> > someone who wants to block Flash selectively on some pages
> >  may reasonably want to prevent JavaScript execution as well.
> 
> +1. I'm on this situation. Is there a hope to see this old bug to be solved one
> day?

Notice that my comment #16 is obsolete, because NoScript can replace FlashBlock: http://noscript.net/features#contentblocking

Comment 24

[Stephan Sokolow]

comment #16 may be obsolete, but this does still prevent me from writing a FlashBlock-style extension for controlling GIF/APNG animations and then using it alongside NoScript. (Not true blocking, of course. Just "Display only first frame unless an overlaid icon in the corner is clicked")

Comment 25

[Terrell Kelley]

Other than extensions that the user has to explicitly install, what else can load anything into chrome:// URLs? Heck, why can external pages even access chrome:// URLs?

And the fact that someone has written a workaround for a popular extension doesn't mean this isn't still a bug.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> what else can load anything into chrome:// URLs?

Nothing.

> Heck, why can external pages even access chrome:// URLs?

At this point, because the chrome package in question explicitly chose to allow that. Access is blocked by default.

Comment 27

[KevKeating]

(In reply to Giorgio Maone from comment #23)
> Notice that my comment #16 is obsolete, because NoScript can replace
> FlashBlock: http://noscript.net/features#contentblocking

This bug still causes problems for users who use FlashBlock with YesScript (https://addons.mozilla.org/en-US/firefox/addon/yesscript/), since YesScript doesn't include a Flash blocker.  For those who aren't familiar with it, YesScript provides a JavaScript blacklist rather than a whitelist.

Comment 28

[John Drinkwater (:beta)]

PDF.js, feed subscribe pages and HTML5 audio/video keep slapping me in the face with my NoScript use and I’d like to look at fixing it ‐ How hard would this be to tackle for someone of little Firefox hacking experience?

How bitrotted are comment 3 and comment 20 ?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment general arch proposal should still be ok.

Comment 20 should be ok as well, actually.

As far as how hard it is, that depends on which set of problems you're tackling.  The videocontrols can just be solved by comment 20, which should not be too bad to do.  Doing the entirety of comment 3 might take a bit more work.  In particular, I'm not sure how PDF.js hooks itself up.

The feed subscribe pages is just a recent regression, by the way.  See bug 786009.

Comment 30

[John Drinkwater (:beta)]

(In reply to Boris Zbarsky (:bz) from comment #29)
> The feed subscribe pages is just a recent regression, by the way.  See bug
> 786009.

Ooh, ta.

Comment 31

[Boris Zbarsky [:bzbarsky]]

This was fixed by bug 834697.