948880 - Move inline scripts and styles into separate file for toolkit/content/aboutAbout.xhtml (URL=about: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

[desiradaniel2007]

I am ready to take this bug. Another question? Should the JS be taken out in a single file, including that in inline XUL event listeners.

Comment 2

[Frederik Braun [:freddy]]

Yes. See bug 948879 and bug 948894 for examples.

Comment 3

[desiradaniel2007]

Attachment: Moving inline JS

Here is my manually created diff. Will check hg diff soon, looking at what MDN says.

Comment 4

[Frederik Braun [:freddy]]

Comment on attachment 8349988 [details] [diff] [review]
Moving inline JS

The patch in itself looks good. 
But it's not in the right format: HG omits the changes to the xhtml file somehow. Probably a syntax problem with the diff.

Comment 5

[desiradaniel2007]

Attachment: aboutAbout.patch

Here's the patch made by hg diff.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8351461 [details] [diff] [review]
aboutAbout.patch

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

This looks good but did you check that about:about in your custom build is still working? "aboutAbout.js" needs to be declared in toolkit/content/jar.mn otherwise it won't be included in the build.

Comment 7

[Frederik Braun [:freddy]]

Daniel, it seems your patch needs only a quick update. Let's get this ready to be checked in! :)

Comment 8

[desiradaniel2007]

Attachment: aboutAboutJs.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:

I haven't really looked into the build system yet, but given this is a simple bug I expect my latest patch to be OK. Would it be a problem if you try an incremental build on your machine?

I will delve deeper in Mozilla builds when I have some time. Sorry, for taking long with this too, but had some things to hurry up with :)

Comment 9

[desiradaniel2007]

Attachment: Patch ready for merging

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8361844 [details] [diff] [review]
aboutAboutJs.patch

This bug is not critical for aurora and should land first in central.

Besides that, your patch contains the hg help (and does not contain the information about the author, description of the bug, etc).
You should have a look to:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 11

[desiradaniel2007]

Exactly "hg qnew aboutAboutJs.patch" has messed my patch to that degree. I do not find the MDN docs to explain patches clearly. I am running Mercurial 2.2.2 and cannot upgrade since I am using Mint :( Switching to Ubuntu soon.

Going through this tutorial since I am completely new to mq http://mercurial.selenic.com/wiki/MqTutorial Perhaps it should get a reference on MDN.

I am sorry for these delays and would appreciate any further guidance on this.

Comment 12

[desiradaniel2007]

Attachment: a.patch

Comment 13

[desiradaniel2007]

Attachment: aboutAboutJs.patch

Comment 14

[desiradaniel2007]

Attachment: aboutAboutInlineJs.patch

Thanks for all who have helped me especially #introduction people on IRC.

I have everything in place for this patch! (At least, I think.)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8373332 [details] [diff] [review]
aboutAboutInlineJs.patch

This doesn't have review AFAICT. Also, please use the checkin-needed keyword when this meets the requirements for landing.

Comment 16

[Frederik Braun [:freddy]]

Comment on attachment 8373332 [details] [diff] [review]
aboutAboutInlineJs.patch

This looks good. Thank you Daniel.
Let's get a review from a module peer, before this is checked in.

Comment 17

[Brian R. Bondy [:bbondy]]

Comment on attachment 8373332 [details] [diff] [review]
aboutAboutInlineJs.patch

Gavin, or someone he will delegate to, is probably a better reviewer for this.  Forwarding to Gavin for now.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8373332 [details] [diff] [review]
aboutAboutInlineJs.patch

Looks like aboutAbout.js needs an MPL license header (from https://www.mozilla.org/MPL/headers/), and a newline at the end

r=me with those added.

Comment 19

[desiradaniel2007]

Attachment: aboutAboutInlineJs.patch

Thanks again!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8375661 [details] [diff] [review]
aboutAboutInlineJs.patch

Please just use checkin-needed in the future.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/3adf0dffdf19

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/3adf0dffdf19