Remove unsafeSetInnerHTML in browser_l10n_localizeMarkup.js

RESOLVED FIXED in Firefox 61

Status

RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: johannh, Assigned: trisha, Mentored)

Tracking

60 Branch
Firefox 61

Firefox Tracking Flags

(firefox60 wontfix, firefox61 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

a year ago
We're using unsafeSetInnerHTML to inject HTML in the browser_l10n_localizeMarkup.js test file. While that in itself is not dangerous, we're aiming to remove unsafeSetInnerHTML as soon as possible, and so we'll need to rewrite the test.

https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/devtools/shared/tests/browser/browser_l10n_localizeMarkup.js#21
(Assignee)

Comment 1

11 months ago
Can I take this up since I'm already working on 1444441?
(Reporter)

Comment 2

11 months ago
Sure!
Mentor: jhofmann
(Assignee)

Updated

11 months ago
Assignee: nobody → guptatrisha97
(Assignee)

Comment 3

11 months ago
Created attachment 8963556 [details] [diff] [review]
bug1444445.patch
Attachment #8963556 - Flags: review?(jhofmann)
(Reporter)

Updated

11 months ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

11 months ago
Is there anything I need to change/add in this patch? Thanks.
(Reporter)

Comment 5

11 months ago
Comment on attachment 8963556 [details] [diff] [review]
bug1444445.patch

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

Thanks for working on this. A couple of comments below...

Did you run the test? Is it succeeding locally for you?

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js
@@ +22,5 @@
> +  let d0 = document.createElement("div");
> +  d0.setAttribute("id", "d0");
> +  d0.setAttribute("data-localization", "content=inspector.someInvalidKey");
> +  div.appendChild(d0);
> +  div.append("\n     ");

Please remove this and all other instances of adding whitespace.

@@ +35,5 @@
> +  d2.setAttribute("data-localization", "content=inspector.label;title=inspector.accesskey");
> +  div.appendChild(d2);
> +  d2.append("\n     ");
> +  div.append("\n     ");
> +  let comment = document.createComment("keep the following data-localization on two separate lines");

You don't need to create comments.

@@ +63,1 @@
>    

nit: you can remove the trailing whitespace while you're touching this file
Attachment #8963556 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 6

11 months ago
Created attachment 8965657 [details] [diff] [review]
bug1444445.patch

Thanks for your review. I ran the mochitest after the artifact build and it seemed to be successful. Does this patch look okay now?
Attachment #8963556 - Attachment is obsolete: true
Attachment #8965657 - Flags: review?(jhofmann)
(Reporter)

Comment 7

11 months ago
Comment on attachment 8965657 [details] [diff] [review]
bug1444445.patch

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

This looks good, but let's replace the variables we're getting here:

https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/devtools/shared/tests/browser/browser_l10n_localizeMarkup.js#40-44

in favor of d1, d2, etc. that are already defined?

Please also fix the two nits below :)

Thanks!

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js
@@ +43,5 @@
> +  d4.setAttribute("id", "d4");
> +  d4.setAttribute("data-localization", "aria-label=inspector.label");
> +  div.appendChild(d4);
> +  d4.append("Some content");
> +  let div0 = document.createElement("div");

nit: can we call this something a little less random than div0? Maybe toolboxDiv or something?

@@ +50,5 @@
> +  div.appendChild(div0);
> +  let d5 = document.createElement("div");
> +  d5.setAttribute("id", "d5");
> +  d5.setAttribute("data-localization", "content=toolbox.defaultTitle");
> +  div0.appendChild(d5);

nit: can you add a newline below this line please?
Attachment #8965657 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 8

11 months ago
Created attachment 8965775 [details] [diff] [review]
bug1444445.patch
Attachment #8965657 - Attachment is obsolete: true
Attachment #8965775 - Flags: review?(jhofmann)
(Assignee)

Comment 9

11 months ago
Can I get TryServer access so as to run the tests before checking this patch in (once it gets review+)? Thanks
(Reporter)

Comment 10

11 months ago
Comment on attachment 8965775 [details] [diff] [review]
bug1444445.patch

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

You still need to remove these variables: https://searchfox.org/mozilla-central/rev/d0413b711da4dac72b237d0895daba2537f1514c/devtools/shared/tests/browser/browser_l10n_localizeMarkup.js#40-44

The idea is that since we already have data1, data2, data3, etc. we don't need to get the same elements again as d1, d2, d3, etc. Just use the same variables everywhere instead.

Let me know if you need more help with that :)

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js
@@ +31,5 @@
> +  data1.append("Text will disappear");
> +  let data2 = document.createElement("div");
> +  data2.setAttribute("id", "d2");
> +  data2.setAttribute("data-localization",
> +                  "content=inspector.label;title=inspector.accesskey");

nit: please align this line with "data-...." from above (so that the two arguments are vertically aligned)

@@ +36,5 @@
> +  div.appendChild(data2);
> +  let data3 = document.createElement("div");
> +  data3.setAttribute("id", "d3");
> +  data3.setAttribute("data-localization",
> +                  "content=inspector.label;title=inspector.accesskey");

nit: please align the arguments

@@ +45,5 @@
> +  div.appendChild(data4);
> +  data4.append("Some content");
> +  let toolboxDiv = document.createElement("div");
> +  toolboxDiv.setAttribute("data-localization-bundle",
> +                    "devtools/client/locales/toolbox.properties");

nit: please align the arguments

@@ +51,5 @@
> +  let data5 = document.createElement("div");
> +  data5.setAttribute("id", "d5");
> +  data5.setAttribute("data-localization", "content=toolbox.defaultTitle");
> +  toolboxDiv.appendChild(data5);
> +         

nit: please remove the trailing whitespace here
Attachment #8965775 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 11

11 months ago
Created attachment 8966199 [details] [diff] [review]
bug1444445.patch
Attachment #8965775 - Attachment is obsolete: true
Attachment #8966199 - Flags: review?(jhofmann)
(Assignee)

Comment 12

11 months ago
Created attachment 8966214 [details] [diff] [review]
bug1444445.patch
Attachment #8966199 - Attachment is obsolete: true
Attachment #8966199 - Flags: review?(jhofmann)
Attachment #8966214 - Flags: review?(jhofmann)
(Reporter)

Comment 13

11 months ago
Comment on attachment 8966214 [details] [diff] [review]
bug1444445.patch

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

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js
@@ +59,5 @@
> +  div1 = div.querySelector("#d1");
> +  div2 = div.querySelector("#d2");
> +  div3 = div.querySelector("#d3");
> +  div4 = div.querySelector("#d4");
> +  div5 = div.querySelector("#d5");

You can remove these 5 lines :)

::: npm-shrinkwrap.json
@@ +1003,5 @@
> +      "integrity": "sha512-4AH6Z5fzNNBcH+6XDMfA/BTt87skxqJlO0lAh3Dker5zThcAxG6mKz+iGu308UKoPPQ8Dcqx/4JhujzltRa+hQ==",
> +      "requires": {
> +        "safe-buffer": "5.1.1"
> +      }
> +    },

Please revert the change to this file. It was done accidentally by a mach script.

hg revert -r central npm-shrinkwrap.json

should do the trick.

Thanks!
Attachment #8966214 - Flags: review?(jhofmann) → review-
(Assignee)

Comment 14

10 months ago
Created attachment 8966826 [details] [diff] [review]
bug1444445.patch
Attachment #8966214 - Attachment is obsolete: true
Attachment #8966826 - Flags: review?(jhofmann)
(Reporter)

Comment 15

10 months ago
Comment on attachment 8966826 [details] [diff] [review]
bug1444445.patch

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

That looks good, thank you! To get the patch checked in, please set the checkin-needed keyword.
Attachment #8966826 - Flags: review?(jhofmann) → review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 16

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dbca44c0855
Remove unsafeSetInnerHTML in browser_l10n_localizeMarkup.js. r=johannh
Keywords: checkin-needed

Comment 17

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8dbca44c0855
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
status-firefox60: affected → wontfix

Updated

8 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.