Closed Bug 1016300 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 34

People

(Reporter: freddy, Assigned: aryx, Mentored)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [lang=html][good first bug][lang=js][r+ in comment 23, Try run in comment 22])

Attachments

(1 file, 5 obsolete files)

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
Blocks: 718011
Summary: Move inline scripts and styles into separate file for Move inline scripts and styles into separate file for browser/content/browser/preferences/in-content/preferences.xul (URL=about:preferences) → Move inline scripts and styles into separate file for browser/content/browser/preferences/in-content/preferences.xul (URL=about:preferences)
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.
Build instructions are in the wiki <https://developer.mozilla.org/en-US/docs/Introduction> the git repo is <https://github.com/mozilla/gecko-dev>.
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.
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.
Attached patch pref.patch (obsolete) — Splinter Review
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
Flags: needinfo?(fbraun)
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.
Attachment #8431664 - Flags: feedback+
Flags: needinfo?(fbraun)
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.
Attachment #8431664 - Flags: review?(nfitzgerald)
Comment on attachment 8431664 [details] [diff] [review]
pref.patch

Wrong bug.
Attachment #8431664 - Flags: review?(nfitzgerald)
Mentor: fbraun
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js] → [lang=html][good first bug][lang=js]
Assignee: nobody → lviknesh
vikneshwar: Are you still working on this?
aryx , you can start with it if you want . Thank you :)
Attached patch patch, v2 (obsolete) — Splinter Review
Assignee: lviknesh → archaeopteryx
Attachment #8431664 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8472505 - Flags: review?(fbraun)
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?
Attachment #8472505 - Flags: review?(fbraun) → feedback+
(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
Flags: needinfo?(fbraun)
Yes, please keep the synthesized events in place, aryx.
Flags: needinfo?(fbraun)
Attached patch patch, v3 (obsolete) — Splinter Review
Successful Try run: https://tbpl.mozilla.org/?tree=Try&showall=1&rev=9b9c5276fc74
Attachment #8472505 - Attachment is obsolete: true
Attachment #8476577 - Flags: review?(fbraun)
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?
Attachment #8476577 - Flags: review?(gavin.sharp)
Attachment #8476577 - Flags: review?(fbraun)
Attachment #8476577 - Flags: feedback+
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?
Attachment #8476577 - Flags: review?(gavin.sharp) → feedback+
(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...
Flags: needinfo?(fbraun)
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.
Flags: needinfo?(fbraun)
Attached patch patch, v4 (obsolete) — Splinter Review
Attachment #8476577 - Attachment is obsolete: true
Attachment #8477903 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8477903 - Flags: review?(gijskruitbosch+bugs)
Attached patch patch, v6 (obsolete) — Splinter Review
(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
Attachment #8477903 - Attachment is obsolete: true
Attachment #8479223 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8479223 - Flags: review?(gijskruitbosch+bugs) → review+
(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. :-)
Keywords: checkin-needed
Whiteboard: [lang=html][good first bug][lang=js] → [lang=html][good first bug][lang=js][r+ in comment 23, Try run in comment 22]
Version: 32 Branch → Trunk
https://hg.mozilla.org/integration/fx-team/rev/076b9e5d8208
Keywords: checkin-needed
Whiteboard: [lang=html][good first bug][lang=js][r+ in comment 23, Try run in comment 22] → [lang=html][good first bug][lang=js][r+ in comment 23, Try run in comment 22][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/076b9e5d8208
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [lang=html][good first bug][lang=js][r+ in comment 23, Try run in comment 22][fixed-in-fx-team] → [lang=html][good first bug][lang=js][r+ in comment 23, Try run in comment 22]
Target Milestone: --- → Firefox 34
Depends on: 1119229
Depends on: 1119233
Blocks: 1192606
You need to log in before you can comment on or make changes to this bug.