Closed Bug 1444441 Opened 6 years ago Closed 6 years ago

Remove unsafeSetInnerHTML in browser_reservedkey.js

Categories

(Core :: DOM: UI Events & Focus Handling, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: johannh, Assigned: trisha, Mentored)

References

Details

Attachments

(1 file, 6 obsolete files)

We're using unsafeSetInnerHTML to inject HTML in the browser_reservedkey.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/browser/base/content/test/permissions/browser_reservedkey.js#13
Assignee: nobody → guptatrisha97
Mentor: jhofmann
Priority: -- → P3
(In reply to Johann Hofmann [:johannh] from comment #0)
> We're using unsafeSetInnerHTML to inject HTML in the browser_reservedkey.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/browser/base/content/test/
> permissions/browser_reservedkey.js#13

Does this mean I will have to use the SetAttribute method to solve the issue? Can you give me a bit more background information on this, please? Thanks in advance :)
(In reply to Trisha from comment #1)
> (In reply to Johann Hofmann [:johannh] from comment #0)
> > We're using unsafeSetInnerHTML to inject HTML in the browser_reservedkey.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/browser/base/content/test/
> > permissions/browser_reservedkey.js#13
> 
> Does this mean I will have to use the SetAttribute method to solve the
> issue? Can you give me a bit more background information on this, please?
> Thanks in advance :)

You need to construct the elements that are currently in the HTML string using document.createElement (https://developer.mozilla.org/en-US/docs/Web/API/Document/createElement), so <keyset> would become document.createElement("keyset") and so on. You probably need to use other DOM APIs such as appendChild https://developer.mozilla.org/en-US/docs/Web/API/Node/appendChild and setAttribute (https://developer.mozilla.org/en-US/docs/Web/API/Element/setAttribute), too.

The point is that we can remove the unsafeSetInnerHTML function there, so we need to get rid of that HTML string :)
Attached patch bug1444441.patch (obsolete) — Splinter Review
I have created a work-in-progress patch to receive some feedback from you. Can you please look at it and tell me if it looks okay?
Attachment #8962986 - Flags: feedback?(jhofmann)
Comment on attachment 8962986 [details] [diff] [review]
bug1444441.patch

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

This looks great, just a few things. I assume the test is still passing? :)

Thanks!

::: browser/base/content/test/permissions/browser_reservedkey.js
@@ +1,2 @@
>  add_task(async function test_reserved_shortcuts() {
>    /* eslint-disable no-unsanitized/property */

You should be able to remove this now.

@@ +6,5 @@
> +  key1.setAttribute("modifiers", "shift");
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.setAttribute("oncommand", "this.setAttribute("count", Number(this.getAttribute("count")) + 1));

Can you convert this to an addEventListener call?

key1.addEventListener("command", () => ... )

Please do the same for the other "oncommand" attributes below.

@@ +8,5 @@
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.setAttribute("oncommand", "this.setAttribute("count", Number(this.getAttribute("count")) + 1));
> +  keyset.appendChild(key1);
> +  key1.append("\n                  ");

I don't think it's important to add whitespace here. Can you remove this line and the other ones that add whitespace?

@@ +34,2 @@
>    document.documentElement.appendChild(container);
>    /* eslint-enable no-unsanitized/property */

Please remove this, too. I don't think these linter comments were actually needed...
Attachment #8962986 - Flags: feedback?(jhofmann) → feedback+
Attached patch bug1444441.patch (obsolete) — Splinter Review
Attachment #8962986 - Attachment is obsolete: true
Attachment #8963508 - Flags: review?(jhofmann)
Comment on attachment 8963508 [details] [diff] [review]
bug1444441.patch

I'm a bit overloaded on reviews right now, Prathiksha, can you take a look at this?

Thanks!
Attachment #8963508 - Flags: review?(jhofmann) → review?(prathikshaprasadsuman)
Attached patch bug1444441.patch (obsolete) — Splinter Review
Attachment #8963508 - Attachment is obsolete: true
Attachment #8963508 - Flags: review?(prathikshaprasadsuman)
Attachment #8964372 - Flags: review?(prathikshaprasadsuman)
Please let me know if this looks okay, thanks!
Status: NEW → ASSIGNED
Comment on attachment 8964372 [details] [diff] [review]
bug1444441.patch

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

Hi Trisha,

Weren't you trying to submit this patch using mozReview? I know that you've already configured mercurial to use mozReview. You can submit your patch on mozReview by first selecting your changeset(hg up <changeset_ID>) and pushing it for review(hg push review). If you're still stuck, feel free to ping me or Johann on IRC.
Attachment #8964372 - Flags: review?(prathikshaprasadsuman)
(In reply to :prathiksha from comment #9)
> Comment on attachment 8964372 [details] [diff] [review]
> bug1444441.patch
> 
> Review of attachment 8964372 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Trisha,
> 
> Weren't you trying to submit this patch using mozReview? I know that you've
> already configured mercurial to use mozReview. You can submit your patch on
> mozReview by first selecting your changeset(hg up <changeset_ID>) and
> pushing it for review(hg push review). If you're still stuck, feel free to
> ping me or Johann on IRC.

I had few doubts which I think are best to be solved on IRC. I've left you a message! Thanks
Comment on attachment 8965008 [details]
Bug 1444441 - Remove unsafeSetInnerHTML in browser_reservedkey.js

https://reviewboard.mozilla.org/r/233746/#review239388


Code analysis found 8 defects in this patch:
 - 8 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/test/permissions/browser_reservedkey.js:10
(Diff revision 1)
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                  <key id='kt_notreserved' modifiers='shift' key='P' reserved='false' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                  <key id='kt_reserveddefault' modifiers='shift' key='Q' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                </keyset>`;
> +  key1.setAttribute("modifiers", "shift");
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1)

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/test/permissions/browser_reservedkey.js:21
(Diff revision 1)
> +  key2.setAttribute("modifiers", "shift");
> +  key2.setAttribute("key", "P");
> +  key2.setAttribute("reserved", "false");
> +  key2.setAttribute("count", "0");
> +  key2.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1)

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/test/permissions/browser_reservedkey.js:31
(Diff revision 1)
> +  key3.setAttribute("id", "kt_reserveddefault");
> +  key3.setAttribute("modifiers", "shift");
> +  key3.setAttribute("key", "Q");
> +  key3.setAttribute("count", "0");
> +  key3.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1)

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/test/permissions/browser_reservedkey.js:39
(Diff revision 1)
> +  key3.append("\n                ");
>    let container = document.createElement("box");
> -  container.unsafeSetInnerHTML(keyset);
> +  container.appendChild(keyset);
> +  keyset.append("\n                  ");
>    document.documentElement.appendChild(container);
> -  /* eslint-enable no-unsanitized/property */
> +         

Error: Trailing spaces not allowed. [eslint: no-trailing-spaces]

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:21
(Diff revision 1)
>    let str3 = TOOLBOX_L10N.getStr("toolbox.defaultTitle");
>    ok(str1 && str2 && str3, "If this failed, strings should be updated in the test");
>  
>    info("Create the test markup");
>    let div = document.createElement("div");
> -  div.unsafeSetInnerHTML(
> +  div.setAttribute("data-localization-bundle", "devtools/client/locales/startup.properties");

Error: Line 21 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:33
(Diff revision 1)
> -     <div id="d3" data-localization="content=inspector.label;
> -                                     title=inspector.accesskey"></div>
> -     <div id="d4" data-localization="aria-label=inspector.label">Some content</div>
> -     <div data-localization-bundle="devtools/client/locales/toolbox.properties">
> -       <div id="d5" data-localization="content=toolbox.defaultTitle"></div>
> -     </div>
> +  d1.setAttribute("data-localization", "content=inspector.label");
> +  div.appendChild(d1);
> +  d1.append("Text will disappear");
> +  let d2 = document.createElement("div");
> +  d2.setAttribute("id", "d2");
> +  d2.setAttribute("data-localization", "content=inspector.label;title=inspector.accesskey");

Error: Line 33 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:37
(Diff revision 1)
> -       <div id="d5" data-localization="content=toolbox.defaultTitle"></div>
> -     </div>
> -   </div>
> -  `);
> -
> +  d2.setAttribute("id", "d2");
> +  d2.setAttribute("data-localization", "content=inspector.label;title=inspector.accesskey");
> +  div.appendChild(d2);
> +  let d3 = document.createElement("div");
> +  d3.setAttribute("id", "d3");
> +  d3.setAttribute("data-localization", "content=inspector.label;title=inspector.accesskey");

Error: Line 37 exceeds the maximum line length of 90. [eslint: max-len]

::: devtools/shared/tests/browser/browser_l10n_localizeMarkup.js:45
(Diff revision 1)
> +  d4.setAttribute("id", "d4");
> +  d4.setAttribute("data-localization", "aria-label=inspector.label");
> +  div.appendChild(d4);
> +  d4.append("Some content");
> +  let div0 = document.createElement("div");
> +  div0.setAttribute("data-localization-bundle", "devtools/client/locales/toolbox.properties");

Error: Line 45 exceeds the maximum line length of 90. [eslint: max-len]
Attachment #8964372 - Attachment is obsolete: true
Comment on attachment 8965008 [details]
Bug 1444441 - Remove unsafeSetInnerHTML in browser_reservedkey.js

https://reviewboard.mozilla.org/r/233746/#review239622

Thank you for working on this!

I'm holding off on fully reviewing this patch for now. Please upload an updated patch with fixes after you run this mochitest locally. It's a good idea to always check that your changes to test file(s) are not breaking the tests. Here's how you can run a mochitest:

./mach mochitest <path_to_file/path_to_folder>

Feel free to ping me or Johann on IRC if you have any questions or if you get stuck. :)

::: browser/base/content/test/permissions/browser_reservedkey.js:11
(Diff revision 2)
> -                  <key id='kt_notreserved' modifiers='shift' key='P' reserved='false' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                  <key id='kt_reserveddefault' modifiers='shift' key='Q' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                </keyset>`;
> -
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });

Please add a new line here

::: browser/base/content/test/permissions/browser_reservedkey.js:21
(Diff revision 2)
> +  key2.setAttribute("key", "P");
> +  key2.setAttribute("reserved", "false");
> +  key2.setAttribute("count", "0");
> +  key2.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });

Add a new line here also

::: browser/base/content/test/permissions/browser_reservedkey.js:31
(Diff revision 2)
> +  key3.setAttribute("key", "Q");
> +  key3.setAttribute("count", "0");
> +  key3.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });
> +  key2.appendChild(key3);

Please append key1, key2 and key3 to keyset here
Attachment #8965008 - Flags: review?(prathikshaprasadsuman) → review-
Attached patch bug1444441.patch (obsolete) — Splinter Review
I have added a work-in-progress patch for feedback because I made the changes you asked me to in the previous review, but couldn't understand some stuff.

Firstly I was appending key1 to keyset as soon as I set its attributes, and same with key2, key3. But since your review said I should append all keys towards the end, I want to know which one is the preferred method? 

Also, I ran mochitest using both the above options yet got 3 failed tests. Can you give me a hint where I'm going wrong?

Thanks for your feedback on my work-in-progress patch in advance! :)
Attachment #8965008 - Attachment is obsolete: true
Attachment #8965008 - Flags: review?(prathikshaprasadsuman)
Attachment #8966831 - Flags: feedback?(prathikshaprasadsuman)
Comment on attachment 8965008 [details]
Bug 1444441 - Remove unsafeSetInnerHTML in browser_reservedkey.js

https://reviewboard.mozilla.org/r/233746/#review241488

I assume the test is still failing. I recommend getting help on IRC to resolve it. Feel free to ping me or Johann for any questions you might have. Thanks for working on this!

::: browser/base/content/test/permissions/browser_reservedkey.js:13
(Diff revision 3)
> -                  <key id='kt_reserveddefault' modifiers='shift' key='Q' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                </keyset>`;
> +  key1.setAttribute("count", "0");
> +  key1.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });
> +
> +  keyset.appendChild(key1);

You're appending key1 to keyset twice in this patch (here and in line 35)

::: browser/base/content/test/permissions/browser_reservedkey.js:20
(Diff revision 3)
> +  key2.setAttribute("id", "kt_notreserved");
> +  key2.setAttribute("modifiers", "shift");
> +  key2.setAttribute("key", "P");
> +  key2.setAttribute("reserved", "false");
> +  key2.setAttribute("count", "0");
> +  key2.addEventListener("oncommand", () => {

This should be "command", not "oncommand"

::: browser/base/content/test/permissions/browser_reservedkey.js:24
(Diff revision 3)
> +  key2.setAttribute("count", "0");
> +  key2.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });
>  
> +  key1.appendChild(key2);

Please remove this line

::: browser/base/content/test/permissions/browser_reservedkey.js:34
(Diff revision 3)
> +  key3.setAttribute("count", "0");
> +  key3.addEventListener("oncommand", () => {
> +    this.setAttribute("count", Number(this.getAttribute("count")) + 1);
> +  });
> +
> +  key2.appendChild(key3);

Please remove this line also
Attachment #8965008 - Flags: review-
(In reply to Trisha from comment #16)
> Created attachment 8966831 [details] [diff] [review]
> bug1444441.patch
> 
> I have added a work-in-progress patch for feedback because I made the
> changes you asked me to in the previous review, but couldn't understand some
> stuff.
> 
> Firstly I was appending key1 to keyset as soon as I set its attributes, and
> same with key2, key3. But since your review said I should append all keys
> towards the end, I want to know which one is the preferred method? 
> 
> Also, I ran mochitest using both the above options yet got 3 failed tests.
> Can you give me a hint where I'm going wrong?
> 
> Thanks for your feedback on my work-in-progress patch in advance! :)

I guess the point I was trying to make is that you have to append all keys to the parent keyset element directly.
Attachment #8966831 - Flags: feedback?(prathikshaprasadsuman) → feedback+
Attached patch bug1444441.patch (obsolete) — Splinter Review
I debugged my code and came down to one failed test now. I will be happy if you can help me get feedback on why this one case's failing. This is what it says: "FAIL reserved='true' with preference on - Got 1, expected 2" Thanks in advance!
Attachment #8966831 - Attachment is obsolete: true
Attachment #8967983 - Flags: feedback?(prathikshaprasadsuman)
Comment on attachment 8967983 [details] [diff] [review]
bug1444441.patch

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

::: browser/base/content/test/permissions/browser_reservedkey.js
@@ +6,5 @@
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.setAttribute("oncommand", "//");
> +  let attri1 = key1.getAttribute("count");

Try getting this attribute (and the ones below) inside the event handler callback :)
(In reply to Johann Hofmann [:johannh] from comment #20)
> Comment on attachment 8967983 [details] [diff] [review]
> bug1444441.patch
> 
> Review of attachment 8967983 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/permissions/browser_reservedkey.js
> @@ +6,5 @@
> > +  key1.setAttribute("key", "O");
> > +  key1.setAttribute("reserved", "true");
> > +  key1.setAttribute("count", "0");
> > +  key1.setAttribute("oncommand", "//");
> > +  let attri1 = key1.getAttribute("count");
> 
> Try getting this attribute (and the ones below) inside the event handler
> callback :)

Thanks! That worked. Submitted on MozReview for one final review.
Attachment #8967983 - Attachment is obsolete: true
Attachment #8967983 - Flags: feedback?(prathikshaprasadsuman)
Comment on attachment 8965008 [details]
Bug 1444441 - Remove unsafeSetInnerHTML in browser_reservedkey.js

https://reviewboard.mozilla.org/r/233746/#review242670

Looks good to me. Thank you for working on this. :)

::: browser/base/content/test/permissions/browser_reservedkey.js:11
(Diff revision 4)
> -                  <key id='kt_notreserved' modifiers='shift' key='P' reserved='false' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                  <key id='kt_reserveddefault' modifiers='shift' key='Q' count='0'
> -                       oncommand='this.setAttribute("count", Number(this.getAttribute("count")) + 1)'/>
> -                </keyset>`;
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.setAttribute("oncommand", "//");
> +  key1.addEventListener("command", () => {
> +    let attri1 = key1.getAttribute("count");

I would just call this variable "attribute" :)

::: browser/base/content/test/permissions/browser_reservedkey.js:23
(Diff revision 4)
> +  key2.setAttribute("key", "P");
> +  key2.setAttribute("reserved", "false");
> +  key2.setAttribute("count", "0");
> +  key2.setAttribute("oncommand", "//");
> +  key2.addEventListener("command", () => {
> +    let attri2 = key2.getAttribute("count");

this one too

::: browser/base/content/test/permissions/browser_reservedkey.js:34
(Diff revision 4)
> +  key3.setAttribute("modifiers", "shift");
> +  key3.setAttribute("key", "Q");
> +  key3.setAttribute("count", "0");
> +  key3.setAttribute("oncommand", "//");
> +  key3.addEventListener("command", () => {
> +    let attri3 = key3.getAttribute("count");

and this one!
Attachment #8965008 - Flags: review?(prathikshaprasadsuman) → review+
Attachment #8965008 - Attachment is obsolete: true
Attached patch bug1444441.patch (obsolete) — Splinter Review
Changed the variable names. Hope this looks fine now! Thanks
Attachment #8968271 - Flags: review?(prathikshaprasadsuman)
Comment on attachment 8968271 [details] [diff] [review]
bug1444441.patch

Tagging Johann for a final review so we can be sure here.
Attachment #8968271 - Flags: review?(prathikshaprasadsuman) → review?(jhofmann)
Comment on attachment 8968271 [details] [diff] [review]
bug1444441.patch

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

Looks good, thank you for all the work!

::: browser/base/content/test/permissions/browser_reservedkey.js
@@ +5,5 @@
> +  key1.setAttribute("modifiers", "shift");
> +  key1.setAttribute("key", "O");
> +  key1.setAttribute("reserved", "true");
> +  key1.setAttribute("count", "0");
> +  key1.setAttribute("oncommand", "//");

nit: can you add a comment that we need to do this, otherwise the "command" event won't fire?

also in the other 2 occurrences, please.
Attachment #8968271 - Flags: review?(jhofmann) → review+
Attachment #8968271 - Attachment is obsolete: true
(In reply to Johann Hofmann [:johannh] from comment #27)
> Comment on attachment 8968271 [details] [diff] [review]
> bug1444441.patch
> 
> Review of attachment 8968271 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, thank you for all the work!
> 
> ::: browser/base/content/test/permissions/browser_reservedkey.js
> @@ +5,5 @@
> > +  key1.setAttribute("modifiers", "shift");
> > +  key1.setAttribute("key", "O");
> > +  key1.setAttribute("reserved", "true");
> > +  key1.setAttribute("count", "0");
> > +  key1.setAttribute("oncommand", "//");
> 
> nit: can you add a comment that we need to do this, otherwise the "command"
> event won't fire?
> 
> also in the other 2 occurrences, please.

Is this attachment okay?
Comment on attachment 8965008 [details]
Bug 1444441 - Remove unsafeSetInnerHTML in browser_reservedkey.js

https://reviewboard.mozilla.org/r/233746/#review243270

That looks good, thank you!

Before you set checkin-needed, you will need to resolve all outstanding issues in mozreview (click on the yellow label with the number and set each individual item to fixed or dropped).
Attachment #8965008 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/081b471aa819
Remove unsafeSetInnerHTML in browser_reservedkey.js r=johannh,prathiksha
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/081b471aa819
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.