948879 - Move inline scripts and styles into separate file for toolkit/content/about.xhtml (URL=about:)

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.

Comment 1

[Afshin Mehrabani]

I'm ready to work on this item.

Comment 2

[Afshin Mehrabani]

Thanks, I will ask questions here.

Comment 3

[Afshin Mehrabani]

I just found this internal JS code in `about:`

<script type="application/javascript">
      // get release notes and vendor URL from prefs
      var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"]
                                .getService(Components.interfaces.nsIURLFormatter);
      var releaseNotesURL = formatter.formatURLPref("app.releaseNotesURL");
      if (releaseNotesURL != "about:blank") {
        var relnotes = document.getElementById("releaseNotesURL");
        relnotes.setAttribute("href", releaseNotesURL);
        relnotes.parentNode.removeAttribute("hidden");
      }

      var vendorURL = formatter.formatURLPref("app.vendorURL");
      if (vendorURL != "about:blank") {
        var vendor = document.getElementById("vendorURL");
        vendor.setAttribute("href", vendorURL);
      }

      // insert the version of the XUL application (!= XULRunner platform version)
      var versionNum = Components.classes["@mozilla.org/xre/app-info;1"]
                                 .getService(Components.interfaces.nsIXULAppInfo)
                                 .version;
      var version = document.getElementById("version");
      version.appendChild(document.createTextNode("&about.version; " + versionNum));

      // append user agent
      var ua = navigator.userAgent;
      if (ua) {
        var list = document.getElementById("aboutPageList");
        var listItem = list.appendChild(document.createElement("li"));
        listItem.appendChild(document.createTextNode("&about.buildIdentifier;"));
        listItem.appendChild(document.createTextNode(ua));
      }
    </script>

Just to make sure that I'm in a correct way, we should remove this code from the file and link the external js to the page, right?

Comment 4

[Frederik Braun [:freddy]]

Yes! :)

As indicated in the bug title, the xhtml file is located at |toolkit/content/about.xhtml|, so the JavaScript should go into |toolkit/content/| or |toolkit/content/js/| (new directory). The name |about.js| makes sense to me.

Comment 5

[Afshin Mehrabani]

Thanks for such a detailed information. I will submit a patch by tomorrow (because I'm a little bit busy today), I think I can help for other subsets also.

Comment 6

[Afshin Mehrabani]

Attachment: move_inline_js_about.patch

Comment 7

[Frederik Braun [:freddy]]

Comment on attachment 8347108 [details] [diff] [review]
move_inline_js_about.patch

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

Nice first try!

Since the script tries to access DOM nodes, your script tag should go into the bottom of the body element - right where you have removed the original element.
Also, the patch file does not contain the content of about.js ;)
Can you also make sure that the patch contains a good patch message (See https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F)

Once this is addressed, we can ask a Firefox module peer to conduct a review (See https://wiki.mozilla.org/Modules/Firefox).

Comment 8

[desiradaniel2007]

Excuse my ignorance, but wouldn't it be slightly faster if we leave the script at the end of the body?

Comment 9

[desiradaniel2007]

Excuse my ignorance, but wouldn't it be slightly faster if we leave the script at the end of the body? (Not sure if this applies since it is an internal page.):-)

Comment 10

[Afshin Mehrabani]

There is a problem with my patch and I will submit a new patch in next hours. I was really busy in last few days.

Comment 11

[Afshin Mehrabani]

Attachment: move_inline_js_about.patch

Comment 12

[Frederik Braun [:freddy]]

Comment on attachment 8348543 [details] [diff] [review]
move_inline_js_about.patch

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

Looks good to me, but I can only do a preliminary review. ;)
Let's find a module peer to do the review.

Comment 13

[Frederik Braun [:freddy]]

Comment on attachment 8348543 [details] [diff] [review]
move_inline_js_about.patch

Tim, can you take another look?

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8348543 [details] [diff] [review]
move_inline_js_about.patch

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

::: toolkit/content/about.js
@@ +22,5 @@
> +var versionNum = Components.classes["@mozilla.org/xre/app-info;1"]
> +                           .getService(Components.interfaces.nsIXULAppInfo)
> +                           .version;
> +var version = document.getElementById("version");
> +version.appendChild(document.createTextNode("&about.version; " + versionNum));

Previously, we automatically replaced &about.version; with the version number but that's not supported in JS files unfortunately.

We can fix this by moving |&about.version;| to the .xhtml file like |<p id="version">&about.version;</p>| and do |version.textContent += " " + versionNum;| here instead of appendChild().

@@ +29,5 @@
> +var ua = navigator.userAgent;
> +if (ua) {
> +  var list = document.getElementById("aboutPageList");
> +  var listItem = list.appendChild(document.createElement("li"));
> +  listItem.appendChild(document.createTextNode("&about.buildIdentifier;"));

Same thing here for &about.buildIdentifier;

We can put |<li id="buildID">&about.buildIdentifier;</li>| in the .xhtml file just before the <script> tag and do |document.getElementById("buildID").textContent += " " + ua;| here.

Comment 15

[Afshin Mehrabani]

Attachment: move_inline_js_about.patch

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8349774 [details] [diff] [review]
move_inline_js_about.patch

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

Thank you, this looks great. Unfortunately I just realized that this is code in toolkit/ which I'm not a peer of, sorry. We'll have to ask someone else to give the final go.

I hope Gavin has a spare minute for a simple patch :)

Comment 17

[Frederik Braun [:freddy]]

Gave it a spin locally, looks sweet.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b463b606cf9d

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b463b606cf9d