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.

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?

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.

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 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).

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.

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 on attachment 8348543 [details] [diff] [review] move_inline_js_about.patch Tim, can you take another look?

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 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 :)

Gave it a spin locally, looks sweet.