Closed Bug 948898 Opened 12 years ago Closed 11 years ago

Move inline scripts and styles into separate file for mobile/android/chrome/content/aboutHealthReport.xhtml (URL=about:healthreport)

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 31

People

(Reporter: freddy, Assigned: anton_11111)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 7 obsolete files)

added bind to the event handlers
1.65 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → mozbugs.retornam
I like to work on this bug, but I can't understand clearly what I have to do.
Flags: needinfo?
You would need some basic understanding of HTML and JavaScript. You will need to modify the HTML to remove event handler attributes (onclick=, onload=, on...) and replace them with JavaScript. We have a wiki page that explains this project: https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles
Assignee: mozbugs.retornam → sumit.engg.genious
Flags: needinfo?
Status: NEW → ASSIGNED
Hello, can I have this bug assigned to me please?
Go for it! I hope you'll know better how to handle this, than the previous assignees :)
Assignee: sumit.engg.genious → anton_11111
is it just the onload and onunload events in the body tags that need to be moved?
sorry I also meant to add this onto comment 5 does the script in the head also need changing?
It's just the onload and onunload.
Sorry about the delay was busy with other things, is this on the right track?
Attachment #8393376 - Flags: review?(fbraun)
Comment on attachment 8393376 [details] proposed changes to the abouthHealthReport.js Can you please provide this as a patch file instead? There's a good guide on how to contribute patches here on MDN: https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
Attachment #8393376 - Flags: review?(fbraun) → review-
added now as a patch.
Attachment #8393376 - Attachment is obsolete: true
Attachment #8394669 - Flags: review?(fbraun)
Sorry, but this is the patched file again, not a diff. Please read the instructions on the wiki. If you need additional help, I'm on IRC right now (Server irc.mozilla.org, channel #security)
Attachment #8394669 - Flags: review?(fbraun) → review-
Attached patch patch1forbug948898.patch (obsolete) — — Splinter Review
is this correct?, sorry about the other attempts as this is my first time.
Attachment #8394669 - Attachment is obsolete: true
Attachment #8394687 - Flags: review?(fbraun)
Comment on attachment 8394687 [details] [diff] [review] patch1forbug948898.patch Review of attachment 8394687 [details] [diff] [review]: ----------------------------------------------------------------- This patch has the correct format, but some bits are not functional. You need to add an event listener seperately from the definition of the functions. Please check our this wiki page that links to bugs similar to this. They should have attachments that show how to add event listeners. https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles#Finding_Inspiration If you need additional help on event listeners, take a look at https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener ::: mobile/android/chrome/content/aboutHealthReport.js @@ +32,5 @@ > // SharedPreferences. > let sharedPrefs = new SharedPreferences(); > > let healthReportWrapper = { > + window.onload = (init): function () { Both this and the "onunload" bit are not valid JavaScript :( ::: mobile/android/chrome/content/aboutHealthReport.xhtml @@ +25,5 @@ > type="text/css" /> > <script type="text/javascript;version=1.8" > src="chrome://browser/content/aboutHealthReport.js" /> > </head> > + <body> This bit is correct! :)
Attachment #8394687 - Flags: review?(fbraun) → review-
Attached patch patch version 2 for bug 948898 (obsolete) — — Splinter Review
I used the event handlers but wasn't too sure on location so I'm not fully sure if it is correct.
Attachment #8394687 - Attachment is obsolete: true
Attachment #8396367 - Flags: review?(fbraun)
Attachment #8396367 - Flags: review?(fbraun)
I am having issues creating a patch the lines I have changed aren't showing as modified in the patch file
Got my problem sorted so I hope this is closer to the goal :).
Attachment #8396663 - Flags: review?(fbraun)
Comment on attachment 8396663 [details] [diff] [review] attempt number two for bug 948898 now with event listeners Review of attachment 8396663 [details] [diff] [review]: ----------------------------------------------------------------- This looks good. Please address these comments and then you should be good to go. ::: mobile/android/chrome/content/aboutHealthReport.js @@ +59,5 @@ > Services.obs.removeObserver(this, EVENT_HEALTH_RESPONSE); > }, > > + window.addEventListener("unload", uninit, false); > + About your changes to aboutHealthReport.js: First, they don't belong in the definition of healthReportWrapper, they should go after it (basically, the end of the file). For the scope to still work you'll have to prefix the function names with "healthReportWrapper." again. Please also remove the extra blank lines after the window.addEventListener() calls.
Attachment #8396663 - Flags: review?(fbraun) → feedback+
Is it ok just updating the previous patch file with the new changes I made?
Attachment #8396367 - Attachment is obsolete: true
Attachment #8396663 - Attachment is obsolete: true
Attachment #8399413 - Flags: review?(fbraun)
Comment on attachment 8399413 [details] [diff] [review] An updated patch with the changes requested from the previous patch Review of attachment 8399413 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. Please address this minor comment and then flag someone else fore review, e.g. Margaret Leibovic. ::: mobile/android/chrome/content/aboutHealthReport.js @@ +192,5 @@ > }, > }; > + > + window.addEventListener("load", healthReportWrapper.init, false); > + window.addEventListener("unload", healthReportWrapper.uninit, false); Nit: Please remove the spaces at the start of the lines.
Attachment #8399413 - Flags: review?(fbraun) → feedback+
hopefully this is the final patch I removed the spaces on the lines as requested :)
Attachment #8399413 - Attachment is obsolete: true
Attachment #8399415 - Flags: review?(margaret.leibovic)
Comment on attachment 8399415 [details] [diff] [review] patch for bug 948898 with the removed spaces Review of attachment 8399415 [details] [diff] [review]: ----------------------------------------------------------------- Did you test this patch? If you did, you would see that about:healthreport doesn't load :) ::: mobile/android/chrome/content/aboutHealthReport.js @@ +192,5 @@ > }, > }; > + > +window.addEventListener("load", healthReportWrapper.init, false); > +window.addEventListener("unload", healthReportWrapper.uninit, false); These will not work because thet will call healthReportWrapper.init/uninit with the event context, not with the healthReportWrapper context (meaning the value of `this` will not be what we want inside the init/uninit functions). To fix this, add .bind(healthReportWrapper) to these functions, like this: window.addEventListener("unload", healthReportWrapper.init.bind(healthReportWrapper), false); For more info, see: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind
Attachment #8399415 - Flags: review?(margaret.leibovic) → review-
I'm not sure i can test the changes on windows if I'm looking at the wiki correctly. I have made the changes but I'm not entirely sure how to test it locally?
is it ok if i can get feedback on this patch suggestion while I'm setting up a way to be able to test changes? sorry for any inconvenience as this is my first time and I'm getting used to the different tools and techniques :)
Attachment #8399415 - Attachment is obsolete: true
Attachment #8400150 - Flags: review?(margaret.leibovic)
(In reply to anton_11111 from comment #22) > I'm not sure i can test the changes on windows if I'm looking at the wiki > correctly. I have made the changes but I'm not entirely sure how to test it > locally? Yes, unfortunately we don't support building Firefox for Android on Windows, so I'm guessing that means you didn't build these changes? If you're using Windows, you can use a Linux VM to build Firefox: https://wiki.mozilla.org/Mobile/Fennec/Android#Windows_.28using_Linux_VM.29 After you have a build, you can test it on most Android devices, or using the Android emulator. In general, we like to encourage contributors to get a local build/test environment set up before working on patches, but it's fine that you got a head start :)
Comment on attachment 8400150 [details] [diff] [review] added bind to the event handlers Review of attachment 8400150 [details] [diff] [review]: ----------------------------------------------------------------- This looks good, thanks for addressing my feedback.
Attachment #8400150 - Flags: review?(margaret.leibovic) → review+
I'm not sure if this is a bad question but what do I need to do next?
(In reply to anton_11111 from comment #26) > I'm not sure if this is a bad question but what do I need to do next? Have you been able to test this? This patch is good to land, so I'll mark this bug as checkin-needed so that someone will commit it for you, but you should make sure you can test local changes before taking on your next Firefox for Android bug :)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/6e1666ecd351
Keywords: checkin-needed
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js] → [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6e1666ecd351
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js][fixed-in-fx-team] → [mentor=fbraun@mozilla.com][lang=html][good first bug][lang=js]
Target Milestone: --- → Firefox 31
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: