1016300 - Move inline scripts and styles into separate file for browser/content/browser/preferences/in-content/preferences.xul (URL=about:preferences)

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Frederik Braun [:freddy]]

With the current plan to harden the security of Firefox, we want to disallow internal privileged pages to use inline JavaScript. Since their amount is fairly limited, we start this by rewriting them bit by bit.

Taking this bug requires some knowledge in event handlers (e.g. addEventListener) and their scope. This wiki page should be a good reference guide: https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles

Comment 1

[Sunil Kumar]

Hi. I would like to work on this bug. Since I'm new to mozilla community, i need some help in getting started.

I use 64 bit ArchLinux. I'm more comfortable with using git than Mercurial. So I wanted to know which repository I have to fork and clone to work on this bug.

Comment 2

[Frederik Braun [:freddy]]

Build instructions are in the wiki <https://developer.mozilla.org/en-US/docs/Introduction> the git repo is <https://github.com/mozilla/gecko-dev>.

Comment 3

[Sunil Kumar]

Hi. I followed the instructions given in that link for building the product. But I'm getting the below error

 0:00.26 /usr/bin/make -f client.mk -s
 0:00.54 Traceback (most recent call last):
 0:00.54   File "/home/sunil/repos/git/gecko-dev/config/pythonpath.py", line 56, in <module>
 0:00.54     main(sys.argv[1:])
 0:00.54   File "/home/sunil/repos/git/gecko-dev/config/pythonpath.py", line 48, in main
 0:00.54     execfile(script, frozenglobals)
 0:00.54 NameError: name 'execfile' is not defined
 0:00.55 client.mk:345: recipe for target 'obj-x86_64-unknown-linux-gnu/CLOBBER' failed
 0:00.55 make: *** [obj-x86_64-unknown-linux-gnu/CLOBBER] Error 1
 0:00.58 0 compiler warnings present.

Please help.

Comment 4

[Frederik Braun [:freddy]]

A lot of people get automated emails when this bug changes. Let us move troubleshooting to another channel and keep this bug focused on the actual topic (rewriting the XUL pages).
Sunil, please make sure you have read through all necessary steps for setting up your build environment on MDN. Once you have confirmed that the error persists, try in #developers on irc.mozilla.org.

Comment 5

[vikneshwar]

Attachment: pref.patch

The only inline script i found is [1] . Moved that to [2] 

[1]mozilla-central/source/browser/components/preferences/in-content/preferences.xul#172
[2]http://dxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/preferences.js

Comment 6

[Frederik Braun [:freddy]]

Comment on attachment 8431664 [details] [diff] [review]
pref.patch

Review of attachment 8431664 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! But there's a tricky thing you missed: preferences.xul includes other XUL documents (through #include preprocessor statements).
Can you try to address those too? Otherwise, we can file individual bugs for each included XUL file.

::: browser/components/preferences/in-content/preferences.js
@@ +18,1 @@
>  

Please put this piece of code in the DOMConentLoaded event listener below.

Comment 7

[Frederik Braun [:freddy]]

Comment on attachment 8431664 [details] [diff] [review]
pref.patch

Review of attachment 8431664 [details] [diff] [review]:
-----------------------------------------------------------------

I was trying to cite the piece of code in line 14-17: Don't add a new event listener, but use the DOMContentLoaded listener that already exists in preferences.js.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8431664 [details] [diff] [review]
pref.patch

Wrong bug.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

vikneshwar: Are you still working on this?

Comment 10

[vikneshwar]

aryx , you can start with it if you want . Thank you :)

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v2

Comment 12

[Frederik Braun [:freddy]]

Comment on attachment 8472505 [details] [diff] [review]
patch, v2

Review of attachment 8472505 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! Patch still mentions you as the contributor though.

Did you notice how preferences.xul includes additional files that may need fixing?
Are you willing to work on them as well, aryx?

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Frederik Braun [:freddyb] from comment #12)
> Did you notice how preferences.xul includes additional files that may need
> fixing?
> Are you willing to work on them as well, aryx?
The onsyncfrompreference/onsynctopreference attributes should remain in place because the events are only synthesized?
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/preferences.xml#429

Example: http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/main.xul#130

Comment 14

[Frederik Braun [:freddy]]

Yes, please keep the synthesized events in place, aryx.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v3

Successful Try run: https://tbpl.mozilla.org/?tree=Try&showall=1&rev=9b9c5276fc74

Comment 16

[Frederik Braun [:freddy]]

Comment on attachment 8476577 [details] [diff] [review]
patch, v3

Review of attachment 8476577 [details] [diff] [review]:
-----------------------------------------------------------------

This looks neat and very well executed. Thank you for this, aryx!
I'm not a module peer so all I can give is feedback+.
Let's ask Gavin, can you review this?

Comment 17

[:Gijs (out for now; he/him)]

Comment on attachment 8476577 [details] [diff] [review]
patch, v3

Review of attachment 8476577 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing.

This is almost there, but please see my remarks about the sync stuff, and the inline style change. :-)

::: browser/components/preferences/in-content/advanced.js
@@ +65,5 @@
> +    setEventListener("offlineNotifyExceptions", "command", function (event) {
> +      gAdvancedPane.offlineAppSelected(event); })
> +    let bundlePrefs = document.getElementById("bundlePreferences");
> +    document.getElementById("offlineAppsList")
> +            .style.height = bundlePrefs.getString("offlineAppsList.height");

Freddy, is this OK per the CSP? This will dynamically add a style attribute... in particular, https://developer.mozilla.org/en-US/docs/Web/Security/CSP/Using_Content_Security_Policy#Writing_a_policy is a little awkwardly worded but says:

" A policy needs to include a default-src or style-src directive in order for CSP to restrict inline styles from being applied from a <style> element or a .style attribute"

Reading the spec, the 1.0 version seems to imply CSSOM is OK, and the 1.1 version here: https://w3c.github.io/webappsec/specs/content-security-policy/#directive-style-src seems to indicate that if you specify unsafe-eval, the above will fail.

Please advise? :-)

(note that this sets styles dynamically based on l10n properties; afaik there's basically no sane way to workaround this to include the l10n info 'statically' to avoid these issues)

::: browser/components/preferences/in-content/advanced.xul
@@ -313,5 @@
>          <hbox>
>            <vbox flex="1">
>              <label id="offlineAppsListLabel">&offlineAppsList2.label;</label>
>              <listbox id="offlineAppsList"
> -                    style="height: &offlineAppsList.height;;"

While I realize we don't need to update the non-incontent preferences.xul, could you port this particular bit to the windowed preferences so we don't duplicate the string as much? You can test by toggling the inContent and instantApply about:config flags.

::: browser/components/preferences/in-content/applications.js
@@ +942,5 @@
> +    setEventListener("filter", "command", gApplicationsPane.filter);
> +    setEventListener("handlersView", "select",
> +      gApplicationsPane.onSelectionChanged);
> +    setEventListener("typeColumn", "click", function (event) {
> +      gApplicationsPane.sort(event); });

Nit: this can be gApplicationsPane.sort, right? Ditto for the one below.

::: browser/components/preferences/in-content/sync.js
@@ +113,5 @@
> +    setEventListener("unlinkDevice", "click", function () {
> +      gSyncPane.startOver(true);
> +      return false;
> +    });
> +    setEventListener("tosPP-normal-ToS", "click", function () {

Nit: name this function (e.g. openToS) and the next one (e.g. openPrivacyPolicy) and pass them by-reference here and for tosPP-small-*.

::: browser/components/preferences/in-content/sync.xul
@@ +146,5 @@
>            &unlinkDevice.label;
>          </label>
>        </hbox>
>      </groupbox>
> +    <hbox id="tosPP-normal" pack="center">

Hey look, duplicate IDs. The things you find in code cleanup...

Could I ask you to:
1) update the IDs in the non-in-content one as well.
2) update the ID ref in sync.js (both in-content and not), which is missing from this patch? (line 174 in both, at time of writing:

document.getElementById("tosPP").hidden = this._usingCustomServer;

)

I'm guessing we need to hide both ToS's, or figure out the right one somehow?

::: browser/themes/shared/incontentprefs/preferences.css
@@ +249,5 @@
>  #weavePrefsDeck > vbox > groupbox,
>  #weavePrefsDeck > vbox > description,
>  #weavePrefsDeck > vbox > #pairDevice > label,
>  #weavePrefsDeck > #needsUpdate > hbox > #loginError,
> +#weavePrefsDeck > #hasFxaAccount > hbox:not(#tosPP-small) > label {

why only the -small version, and not the -normal one?

Comment 18

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #17)
> ::: browser/components/preferences/in-content/advanced.js
> @@ +65,5 @@
> > +    setEventListener("offlineNotifyExceptions", "command", function (event) {
> > +      gAdvancedPane.offlineAppSelected(event); })
> > +    let bundlePrefs = document.getElementById("bundlePreferences");
> > +    document.getElementById("offlineAppsList")
> > +            .style.height = bundlePrefs.getString("offlineAppsList.height");
> 
> Freddy, is this OK per the CSP? This will dynamically add a style
> attribute... in particular,
> https://developer.mozilla.org/en-US/docs/Web/Security/CSP/
> Using_Content_Security_Policy#Writing_a_policy is a little awkwardly worded
> but says:
> 
> " A policy needs to include a default-src or style-src directive in order
> for CSP to restrict inline styles from being applied from a <style> element
> or a .style attribute"
> 
> Reading the spec, the 1.0 version seems to imply CSSOM is OK, and the 1.1
> version here:
> https://w3c.github.io/webappsec/specs/content-security-policy/#directive-
> style-src seems to indicate that if you specify unsafe-eval, the above will
> fail.
> 
> Please advise? :-)
> 
> (note that this sets styles dynamically based on l10n properties; afaik
> there's basically no sane way to workaround this to include the l10n info
> 'statically' to avoid these issues)

Now with added needinfo goodness that's not available from the splinter page...

Comment 19

[Frederik Braun [:freddy]]

Thanks for raising this Gijs!

As per our implementation CSSOM is *currently* fine.
I'd lean towards enabling it in a future proof way by adding unsafe-eval if we go with 1.1.
It is *potentially* bad given that CSS is a bit worse in XUL/Chrome land than in content. In this case, fortunately, the CSS properties don't come from something potentially malicious.

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v4

Comment 21

[:Gijs (out for now; he/him)]

Comment on attachment 8477903 [details] [diff] [review]
patch, v4

Review of attachment 8477903 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/advanced.xul
@@ +227,5 @@
>          <caption><label>&crashReporterSection.label;</label></caption>
>          <description>&crashReporterDesc.label;</description>
>          <hbox>
>            <checkbox id="submitCrashesBox"
> +                    oncommand=";"

Why leave the ";" everywhere?

::: browser/components/preferences/sync.js
@@ +171,5 @@
>        this.page = PAGE_HAS_ACCOUNT;
>        document.getElementById("accountName").value = Weave.Service.identity.account;
>        document.getElementById("syncComputerName").value = Weave.Service.clientsEngine.localName;
> +      document.getElementById("tosPP-normal").hidden = this._usingCustomServer;
> +      document.getElementById("tosPP-small").hidden = this._usingCustomServer;

This will hide and show both... is that really correct?

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v6

(In reply to :Gijs Kruitbosch from comment #21)
> Comment on attachment 8477903 [details] [diff] [review]
> patch, v4
> >            <checkbox id="submitCrashesBox"
> > +                    oncommand=";"
> 
> Why leave the ";" everywhere?
As requested by freddyb over IRC due to bug 547189 and because I experienced an issue without it. I later discovered that the issue had a different cause, so they are gone in this version.

> ::: browser/components/preferences/sync.js
> @@ +171,5 @@
> >        this.page = PAGE_HAS_ACCOUNT;
> >        document.getElementById("accountName").value = Weave.Service.identity.account;
> >        document.getElementById("syncComputerName").value = Weave.Service.clientsEngine.localName;
> > +      document.getElementById("tosPP-normal").hidden = this._usingCustomServer;
> > +      document.getElementById("tosPP-small").hidden = this._usingCustomServer;
> 
> This will hide and show both... is that really correct?
I changed it to only manage the old Sync part ("-normal"), because the code block is for this condition and there isn't UI for custom Firefox Accounts servers.

Successful Try run: https://tbpl.mozilla.org/?tree=Try&rev=c4819c73b858

Comment 23

[:Gijs (out for now; he/him)]

Comment on attachment 8479223 [details] [diff] [review]
patch, v6

Review of attachment 8479223 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/sync.js
@@ +78,5 @@
>        this._init();
>      }.bind(this);
>  
>      Services.obs.addObserver(onReady, "weave:service:ready", false);
> +

Nit: no need for an empty line here.

@@ +198,5 @@
>    updateWeavePrefs: function () {
>      let service = Components.classes["@mozilla.org/weave/service;1"]
>                    .getService(Components.interfaces.nsISupports)
>                    .wrappedJSObject;
> +    // service.fxAccountsEnabled is false if sync is already configured for

Nit: "iff" is intentional here - it means "if and only if". Please leave it intact.

Comment 24

[:Gijs (out for now; he/him)]

(In reply to Archaeopteryx [:aryx] from comment #22)
> > ::: browser/components/preferences/sync.js
> > @@ +171,5 @@
> > >        this.page = PAGE_HAS_ACCOUNT;
> > >        document.getElementById("accountName").value = Weave.Service.identity.account;
> > >        document.getElementById("syncComputerName").value = Weave.Service.clientsEngine.localName;
> > > +      document.getElementById("tosPP-normal").hidden = this._usingCustomServer;
> > > +      document.getElementById("tosPP-small").hidden = this._usingCustomServer;
> > 
> > This will hide and show both... is that really correct?
> I changed it to only manage the old Sync part ("-normal"), because the code
> block is for this condition and there isn't UI for custom Firefox Accounts
> servers.

Makes perfect sense. Thanks a lot for all the hard work here, this kind of stuff always looks easy on the surface and then ends up being a boatload of annoying work. :-)

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: patch, v7, r=Gijs

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/076b9e5d8208

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/076b9e5d8208