1214634 - Unsafe innerHTML/outerHTML/insertAdjacentHTML usage in gaia::video

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect)

Description

[Frederik Braun [:freddy]]

Please see the hints in bug 1211384 about fixing these kinds of problems.
The Firefox OS Security team is there to help you with any kind of question that you may have. You can reach out by setting the needinfo or sec-review flag to [e-mail]

Unsafe assignment to innerHTML:
In apps/video/js/thumbnail_date_group.js, line 57, column 3:
> dummyDiv.innerHTML = htmlText;
In apps/video/js/thumbnail_item.js, line 67, column 5:
> dummyDiv.innerHTML = htmlText;
In apps/video/test/unit/mock_thumbnail_group.js, line 7, column 3:
> dummyNode.innerHTML = '<div>' + MockThumbnailGroup._GroupID + '</div>';

Comment 1

[Julian Hector [:tedd] [:jhector]]

I would like to work on this bug.

Comment 2

[Julian Hector [:tedd] [:jhector]]

Accidentally pressed the wrong 'take'

Comment 3

[Julian Hector [:tedd] [:jhector]]

Ok, so after I looked into it a little more, I saw that there is nothing unsafe, in fact, Sanitizer.escapeHTML is properly used (except for the test), but I believe those lines got flagged because Sanitizer is used inside a function whose return value is assigned to .innerHTML and therefore eslint doesn't know if the value is escaped or not.

So, I created two patches, the first one uses createSafeHTML and unwrapSafeHTML instead of escapeHTML from, which should in theory avoid the warnings.

And the second patch removes the usage of Sanitizer completely, and I have rewritten some of the template code to avoid the usage of innerHTML. Since I think the big goal is to reduce the usage of innerHTML to keep it to a manageable level since innerHTML can potentially lead to an injection.

On a different node, the test code seems to do some unnecessary work and I don't quite understand why it is done this way:

> var dummyNode = document.createElement('div');
> dummyNode.textContent = MockThumbnailGroup._GroupID;
> dummyNode.innerHTML = '<div>' + MockThumbnailGroup._GroupID + '</div>';
> dummyNode.htmlNode = dummyNode.firstElementChild;

first a 'div' element is created and the text is set properly, next the innerHTML is overwritten.
Both pull requests remove the innerHTML usage and just assign the dummyNode to htmlNode like so:

> dummyNode.htmlNode = dummyNode;

If that is not correct, please let me know.
What do you think :gandalf?

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] jhector:bug-1214634-no-innerHTML > mozilla-b2g:master

Comment 5

[GitHub Autolander Bot]

Attachment: [gaia] jhector:bug-1214634-warning-fix > mozilla-b2g:master

Comment 6

[Julian Hector [:tedd] [:jhector]]

I compiled both patches and flashed them on my Aries device, and checked for any failures.
But I would prefer if someone else would also check that no UI is broken or anything.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

:tedd, why did you ask me in comment 3 instead of :freddyb?

Comment 8

[Julian Hector [:tedd] [:jhector]]

Oh sorry, that was a confusion on my side, sorry about that.

:freddyb what do you think about Comment 3

Comment 9

[Frederik Braun [:freddy]]

I'm fine with either patch. As long as we stop hitting the eslint rule.

For the test question in comment 3, I'm not sure I understand either. Maybe _that_ was directed at gandalf? ;)

Comment 10

[Julian Hector [:tedd] [:jhector]]

Thanks freddy, I looked at git blame, and the code in question was introduced by :russn.

:russn, can you please look at Comment 3, I was wondering about the test code :)

I hope you are also the correct person to ask a review for the PRs. Although only one of the 2 PRs should get merged, I don't know which way you would prefer.

Comment 11

[Russ Nicoletti [:russn]]

Comment on attachment 8676581 [details] [review]
[gaia] jhector:bug-1214634-no-innerHTML > mozilla-b2g:master

r- to indicate I prefer the patch that retains the html as html as opposed to building it programmatically.

Comment 12

[Russ Nicoletti [:russn]]

Comment on attachment 8676582 [details] [review]
[gaia] jhector:bug-1214634-warning-fix > mozilla-b2g:master

Thanks for patch. r+ to indicate I prefer this patch (that retains the literal html text as opposed to building it programmatically). 

However, the patch appears to be missing the change to address the eslint issue in thumbnail_date_group.js (https://github.com/mozilla-b2g/gaia/blob/master/apps/video/js/thumbnail_date_group.js#L57). Adding NI for confirmation.

I left one nit in the PR.

Comment 13

[Russ Nicoletti [:russn]]

(In reply to Julian Hector [:tedd] [:jhector] from comment #10)
> Thanks freddy, I looked at git blame, and the code in question was
> introduced by :russn.
> 
> :russn, can you please look at Comment 3, I was wondering about the test
> code :)
> 

Your change looks good, a definite improvement.

> I hope you are also the correct person to ask a review for the PRs. Although
> only one of the 2 PRs should get merged, I don't know which way you would
> prefer.

Comment 14

[Julian Hector [:tedd] [:jhector]]

Thanks, you are definitely right, I don't know how that slipped away, but thanks. I will update the PR, can you please give it another look, I addressed the nit, but I don't want to carry over the r+ since it didn't contain thumbnail_data_group.js

Comment 15

[Russ Nicoletti [:russn]]

Comment on attachment 8676582 [details] [review]
[gaia] jhector:bug-1214634-warning-fix > mozilla-b2g:master

The changes look good. I left one nit regarding a comment that I only just noticed while reviewing your changes. Thanks for helping to clean up the code.

Comment 16

[Julian Hector [:tedd] [:jhector]]

Thanks again, I also changed the comment now, but somehow one test is failing, I will see what the problem is and try to fix it before requesting check-in.

Comment 17

[Julian Hector [:tedd] [:jhector]]

I have been a little side tracked with other projects, but I took a look at it again, after reading the code for a while I couldn't figure out what the problem was, so I re-based the changes and forced another test run:

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=46bec91348839f81d18c854c3911520a64aebd21

Everything is good, so the previous test failures couldn't have been caused by my patch.

All test passed, let's get it landed.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://github.com/mozilla-b2g/gaia/commit/05b6e39a5ac409aa1b4ba3c6ddd51b18c0d32ace