948900 - Move inline scripts and styles into separate file for browser/base/content/downloadManagerOverlay.xul and downloads.xul (URL=about:downloads)

Status: RESOLVED INVALID

(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

[Afshin Mehrabani]

I'm ready to work on this part. Apologize if I make it noisy but I don't see any take button or something to assign the bug to myself (maybe I don't have enough permission?). So, someone should do it

Comment 2

[Afshin Mehrabani]

Can I know where is the `downloads.xul` located?

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Afshin:

toolkit/mozapps/downloads/content/downloads.xul.

Also, this tool is great for finding files in the tree: http://dxr.mozilla.org/

-Mike

Comment 4

[Frederik Braun [:freddy]]

It's in toolkit/mozapps/downloads/content/downloads.xul, as Mike already said.

Comment 5

[Afshin Mehrabani]

Thanks, I'm working on that.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Afshin, are you still working on this? Or can we re-open this for other contributors?

Comment 7

[Shubham Somani]

Attachment: Proposing a patch for Bug 948900

Seemed like no one was working on this bug, so I gave it a try myself. Attached a patch to the bug. Was in doubt where to move script present in downloadManagerOverlay.xul so created a new downloadManagerOverlay.js file.Please review and tell me any changes. Cheers!

Comment 8

[Frederik Braun [:freddy]]

Comment on attachment 8371660 [details] [diff] [review]
Proposing a patch for Bug 948900

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

This looks all pretty good, but there are some minor changes needed:

Now that you introduced a new file, you have to make sure it is included in the browser. There's the file browser/base/jar.mn, that contains references to the existing files (which you have patched). Please add a line that includes downloadManagerOverlay.js.

::: browser/base/content/downloadManagerOverlay.xul
@@ -25,5 @@
> -    selectAllMenuItem.setAttribute("command", "cmd_selectAllDownloads");
> -    selectAllMenuItem.setAttribute("key", "key_selectAllDownloads");
> -  }, false);
> -]]></script>
> -

Having removed this script tag, you now need one that references the newly created .js-File.

::: toolkit/mozapps/downloads/content/downloads.js
@@ +123,5 @@
> +  downloadView.addEventListener("drop", function(event) {gDownloadDNDObserver.onDrop(event)});
> +  
> +  let search = document.getElementById("search");
> +  search.addEventListener("command", function(event) {buildDownloadList()});
> +  

Please remove all trailing whitespaces (there are some in other files too).

Comment 9

[Shubham Somani]

Attachment: 948900.patch

Thanks for the helpful review! :)
I have tried to abide by your pointers. 
Please see the updated patch.

Comment 10

[Frederik Braun [:freddy]]

Comment on attachment 8375386 [details] [diff] [review]
948900.patch

Please don't touch the review flag. Use "feedback?" with my id to gather feedback. We will go through a full review from someone else once this is done.

Comment 11

[Frederik Braun [:freddy]]

Comment on attachment 8375386 [details] [diff] [review]
948900.patch

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

Looks good. Please address these items and test if the download things still work!
Then ask for a "review?" from a Firefox Module peer (see https://wiki.mozilla.org/Modules/Firefox).

::: browser/base/jar.mn
@@ +73,5 @@
>  *       content/browser/browser.xul                   (content/browser.xul)
>  *       content/browser/browser-tabPreviews.xml       (content/browser-tabPreviews.xml)
>  *       content/browser/chatWindow.xul                (content/chatWindow.xul)
>          content/browser/content.js                    (content/content.js)
> +        content/browser/downloadManagerOverlay.js     (content/downloadManagerOverlay.js)

I'm not sure, but considering the #ifdef/#endif instructions in the code, I think this line might have to belong to the other download manager entries. Can anyone else confirm?

::: toolkit/mozapps/downloads/content/downloads.js
@@ +81,5 @@
>  let gStmt = null;
> +document.addEventListener("DOMContentLoaded", function() {
> +
> +  let generalCommands = document.getElementById("generalCommands");
> +  generalCommands.addEventListener("command", function(event) {setSearchboxFocus()});

Please add some space in the  { } block. You could also have tried something like addEventListener("command", funcName), but that heavily depends on the function itself.

::: toolkit/mozapps/downloads/content/downloads.xul
@@ +133,5 @@
>              tooltiptext="&cmd.clearList.tooltip;"/>
>      <spacer flex="1"/>
>      <textbox type="search" id="searchbox" class="compact"
>               aria-controls="downloadView"
> +             " placeholder="&searchBox.label;"/>

Looks like you misplaced a quote here.

Comment 12

[Frederik Braun [:freddy]]

Comment on attachment 8375386 [details] [diff] [review]
948900.patch

Restoring the feedback approval from comment 11.
Removing the timed out needinfo to Afshin.

Comment 13

[Shubham Somani]

Attachment: updated patch

Added changes according to comment 11 and everything seems to work.
Please review. Thanks!

Comment 14

[Shubham Somani]

Attachment: updated_948900.patch

Had attached diff file by mistake last time.

Comment 15

[Shubham Somani]

Attachment: updated_948900.patch

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8379156 [details] [diff] [review]
updated_948900.patch

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

Sorry for the delay - some questions / concerns, see below.

::: browser/base/content/downloadManagerOverlay.js
@@ +5,5 @@
> +
> +////////////////////////////////////////////////////////////////////////////////
> +
> +document.addEventListener("DOMContentLoaded", function() {
> +

Nit - I think we can go ahead and drop this newline - I don't think it's adding to readability.

::: toolkit/mozapps/downloads/content/downloads.js
@@ +81,5 @@
>  let gStmt = null;
> +document.addEventListener("DOMContentLoaded", function() {
> +
> +  let generalCommands = document.getElementById("generalCommands");
> +  generalCommands.addEventListener("command", function(event) { setSearchboxFocus() });

Hold on - this doesn't make sense here... this looks like if we fire "command" from generalCommands, all three of these functions (setSearchboxFocus, gDownloadsView.selectAll, clearDownloadList) are going to be fired... am I wrong on that?

::: toolkit/mozapps/downloads/content/downloads.xul
@@ -26,5 @@
>          id="downloadManager" windowtype="Download:Manager"
>          orient="vertical" title="&downloads.title;"
>          width="&window.width2;" height="&window.height;" screenX="10" screenY="10"
> -        persist="width height screenX screenY sizemode"
> -        onload="Startup();" onunload="Shutdown();"

I'm confused - you've removed the calls to Startup, Shutdown and closeWindow, but I don't see any calls to Startup, Shutdown or closeWindow being added in the DOMContentLoaded event listener...

Comment 17

[Frederik Braun [:freddy]]

Hey, Shubham. It looks like you got some positive feedback on your patch here and there. Do you want to continue working on this?

Comment 18

[Frederik Braun [:freddy]]

It seems this is open for a volunteer again. 
If you want to take this, please make sure to read the previous comments. A basic understanding of event listeners is desired.

Comment 19

[Paul Adenot (:padenot)]

Nicolas is going to take this to the finish line !

Comment 20

[nicoo]

Finished rebasing Shubham's patch.
Fixed a few things which the patch broke; found a few more that the testsuite doesn't catch (I'll either write a test or file a bug).
There are still lots of oncommand and similar things that need fixing.

Now diving in `nsDocument.cpp` to figure out how to run the testsuite with CSP enabled.
Happy happy fun times :)

Comment 21

[nicoo]

It seems that, in `nsPrincipal`, the `mCSP` object is never set (for a chrome page).
(See http://dxr.mozilla.org/mozilla-central/source/caps/src/nsPrincipal.cpp#90)

Comment 22

[nicoo]

Oops. I meant https://bugzilla.mozilla.org/show_bug.cgi?id=1028721

Comment 23

[nicoo]

Long-overdue status update:
- due to the amount of code that must be lugged around, I've begun writing a XUL source transformation tool;
- no progress these last months due to extended illness.

Comment 24

[Hemanth Kumar Veeranki [:harry7]]

Hi is This bug unassigned .. I would like to work on it . But i am new to all this stuff. Can someone help me with starting this ??

Comment 25

[u586192]

I would like to work on this bug. I'm a beginner. Please help me getting started with this bug.

Comment 26

[Frederik Braun [:freddy]]

Thank you for reaching out.
The file in question does not exist anymore and does not need rewriting.
Also, the project to move/rewrite inline styles and scripts is on halt.
If you want to help with Firefox take a look at http://www.joshmatthews.net/bugsahoy/ and https://whatcanidoformozilla.org/.