Rewrite inline script/style in browser/base/content/aboutaccounts/aboutaccounts.xhtml (about:accounts)

RESOLVED FIXED in Firefox 32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: freddyb, Assigned: bernd.loeber+mozbugzilla)

Tracking

(Blocks 1 bug)

unspecified
Firefox 32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
+++ This bug was initially created as a clone of Bug #956482 +++

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.

If you want to work on this, check our project page on the wiki, that will help resolving some initial questions: https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles

P.S.: I wonder if there's a way to convince Firefox and Toolkit Module peer that they will encourage upcoming internal pages not to contain things inline.

Comment 1

5 years ago
I would like to work on this one!

Comment 2

5 years ago
Posted patch Bug980911.patch (obsolete) — Splinter Review
Proposed solution for Bug 980911.  Please provide feedback.

Updated

5 years ago
Assignee: nobody → Iamthemovie3064+Bugzilla

Updated

5 years ago
Attachment #8388938 - Flags: feedback?(fbraun)
Reporter

Updated

5 years ago
Status: NEW → ASSIGNED
Reporter

Comment 3

5 years ago
Comment on attachment 8388938 [details] [diff] [review]
Bug980911.patch

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

Looking good!

Please address these comments in your next revision and flag me again for feedback.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +283,5 @@
> +var oldsync=document.getElementById("oldsync");
> +oldsync.onclick = function(){handleOldSync()};
> +
> +var openPrefs=document.getElementById("openPrefs");
> +openPrefs.onclick = function(){openPrefs()};

About this block: 
1) Please move it into the |init| function, at around line 251.
2) Please use addEventListener instead of onclick, see https://developer.mozilla.org/en-US/docs/Web/API/EventTarget.addEventListener
3) You don't need to wrap the function calls into another function. You can just reference the functions itself, like ...addEventListener("click"; handleOldSync);
4) Your variable "openPrefs" clashes with the existing function name. Choose a different name for the element, like "var openPrefsElement".
5) Use consistent naming for your variables (e.g. capitalization).

@@ +285,5 @@
> +
> +var openPrefs=document.getElementById("openPrefs");
> +openPrefs.onclick = function(){openPrefs()};
> +
> +document.getElementByID

This line appears to be a leftover. Please remove it.

::: browser/base/content/aboutaccounts/aboutaccounts.xhtml
@@ +46,5 @@
>          <section>
>              <div class="graphic graphic-sync-intro"> </div>
>  
>              <div class="button-row">
> +              <a class="button" id="openPrefs" href="#" >&aboutAccountsConfig.manage.label;</a>

There's a leftover whitespace before the closing '>'. It's in your other edits too.
Attachment #8388938 - Flags: feedback?(fbraun) → feedback+
Assignee

Comment 4

5 years ago
Posted patch bug980911.patch (obsolete) — Splinter Review
My try to fix the bug.
Attachment #8413193 - Flags: feedback?(fbraun)
Reporter

Comment 5

5 years ago
Comment on attachment 8413193 [details] [diff] [review]
bug980911.patch

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

Looks OK, please fix this minor thing:

::: browser/base/content/aboutaccounts/aboutaccounts.xhtml
@@ +46,4 @@
>              <div class="graphic graphic-sync-intro"> </div>
>  
>              <div class="button-row">
> +              <a id="buttonOpenPrefs"class="button" href="#">&aboutAccountsConfig.manage.label;</a>

Missing space here :)
Attachment #8413193 - Flags: feedback?(fbraun) → feedback+
Assignee

Comment 6

5 years ago
added missing space
Attachment #8413193 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Attachment #8413208 - Flags: feedback?(fbraun)
Reporter

Comment 7

5 years ago
Comment on attachment 8413208 [details] [diff] [review]
bug980911.patch

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

Works! Nice.
Tim, can you give this a review?
Attachment #8413208 - Flags: review?(ttaubert)
Attachment #8413208 - Flags: feedback?(fbraun)
Attachment #8413208 - Flags: feedback+
Reporter

Comment 8

5 years ago
I guess Zach is also interested...CCing him :)
Assignee: Iamthemovie3064+Bugzilla → bernd.loeber+mozbugzilla
Attachment #8388938 - Attachment is obsolete: true
Comment on attachment 8413208 [details] [diff] [review]
bug980911.patch

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

Thanks, great work! Looks like this patch is ready for checkin-needed [1].

[1] http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed
Attachment #8413208 - Flags: review?(ttaubert) → review+
Reporter

Comment 10

5 years ago
This seems to have skipped Bernd's attention. Proposing this for checkin.
Keywords: checkin-needed
Hint to the nice person that pushes this:

The commit message doesn't mention the reviewers currently. That's what I asked Bernd to do originally but we shouldn't nag him only about that :)
https://hg.mozilla.org/integration/fx-team/rev/13f0acc957ed
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/13f0acc957ed
Status: ASSIGNED → RESOLVED
Closed: 5 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 32
Reporter

Updated

5 years ago
Duplicate of this bug: 970359
You need to log in before you can comment on or make changes to this bug.