Closed Bug 948879 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: freddy, Assigned: afshin.meh)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][qa-])

Attachments

(1 file, 2 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.
I'm ready to work on this item.
Assignee: nobody → afshin.meh
Thanks, I will ask questions here.
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?
Flags: needinfo?(fbraun)
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.
Flags: needinfo?(fbraun)
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.
Attached patch move_inline_js_about.patch (obsolete) — Splinter Review
Attachment #8347108 - Flags: review?(fbraun)
Status: NEW → ASSIGNED
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).
Attachment #8347108 - Flags: review?(fbraun)
Excuse my ignorance, but wouldn't it be slightly faster if we leave the script at the end of the body?
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.):-)
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.
Attached patch move_inline_js_about.patch (obsolete) — Splinter Review
Attachment #8348543 - Flags: review?(fbraun)
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.
Attachment #8348543 - Flags: review?(fbraun) → review+
Attachment #8347108 - Attachment is obsolete: true
Comment on attachment 8348543 [details] [diff] [review]
move_inline_js_about.patch

Tim, can you take another look?
Attachment #8348543 - Flags: review?(ttaubert)
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.
Attachment #8348543 - Flags: review?(ttaubert) → feedback+
Attachment #8348543 - Attachment is obsolete: true
Attachment #8349774 - Flags: review?(ttaubert)
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 :)
Attachment #8349774 - Flags: review?(ttaubert)
Attachment #8349774 - Flags: review?(gavin.sharp)
Attachment #8349774 - Flags: review+
Attachment #8349774 - Flags: review?(gavin.sharp) → review+
Gave it a spin locally, looks sweet.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b463b606cf9d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js] → [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][qa-]
You need to log in before you can comment on or make changes to this bug.