901470 - Rewrite instance of setting innerHTML to uses of textContent or value

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Frederik Braun [:freddy]]

The b2g email app makes use of innerHTML in:
 js/value_selector.js:72
 js/tmpl.js:7
 js/ext/mailapi/worker-bootstrap.js:9523
 js/html_cache_restore.js:87

Some of them look avoidable and might cause content injection / XSS if not maintained properly in the future.


(This is a best practice recommendation. While no XSS has been found it is still desirable to change the respective code to reduce the likelihood of regressions)

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Frederik Braun [:freddyb] from comment #0)
> The b2g email app makes use of innerHTML in:
>  js/value_selector.js:72

Yes, we should fix this.  The dynamic HTML build there doesn't need to happen, although it's safe since the cancel string is statically provided from code.  Ideally we might just clean up our use of value_selector.js entirely

>  js/tmpl.js:7

That needs to be there; it's part of our dynamic HTML to improve start-up time.  The strings are statically sourced from .html files on disk at build-time.  We should add comments to the loader that justify the use of innerHTML and why we think it's safe and what the risk patterns are.  Speaking of which...

From a paranoia perspective, since the template loader is a generic path, we might want to see what we can do about providing invariants about our use of dynamic require().  If we can ensure that all dynamic requires are using string literals instead of computed strings, that would be a good thing.

>  js/ext/mailapi/worker-bootstrap.js:9523

This is searchfilter.js, and this code is actually broken right now because the code runs in the worker and it can't actually use the DOM there.  This is bug 882917.  The logic will want to be updated to use the sanitizer and its HTML parser to implement search anyways, obviating the need to use innerHTML.  (It was a trick to get us a DOM; note that it was using a data document and the rootNode never got appened into the document so most things would remain inert and unparsed, like the CSS.  Of course those important security details should have been comments in there...)

>  js/html_cache_restore.js:87

This is required for our fast startup.  This wants comments too.  It's inductively safe as long as we ensure that only we control our cookie-writing code and the HTML we cache stays safe.  Since that HTML is always pulled out of our DOM tree via innerHTML, our HTML is always just as safe after re-introducing it.

Comment 2

[Frederik Braun [:freddy]]

Right. Security reviews don't scale that well if we have to many inappropriate uses of innerHTML. After all, it is our job to question *every* innerHTML assignment, no matter what the comments say.. ;)

Comment 3

[Frederik Braun [:freddy]]

too many*

Comment 4

[Frederik Braun [:freddy]]

(In reply to Andrew Sutherland (:asuth) from comment #1)
> Yes, we should fix this.  The dynamic HTML build there doesn't need to
> happen, although it's safe since the cancel string is statically provided
> from code.  Ideally we might just clean up our use of value_selector.js
> entirely
> 
> >  js/tmpl.js:7
> 

Regarding this instance: It can easily be converted to DOM calls like createElement and so on using html2dom: http://freddyb.github.io/html2dom/#


(By the way thanks for addressing all the affected code pieces ;))

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Frederik Braun [:freddyb] from comment #4)
> Regarding this instance: It can easily be converted to DOM calls like
> createElement and so on using html2dom: http://freddyb.github.io/html2dom/#

That's a good point.  Since all of the template stuff is done as part of our build step, we could indeed generate JS code in lieu of using .innerHTML.  We could also just generate static .innerHTML assignments.  (That would increase the grep count, but make it impossible to finagle the no-longer-present dynamic innerHTML code path to load nefarious HTML.)

I think the driving concern for us would be the performance.  We are somewhat latency-sensitive, but we're also not talking about a lot of HTML.  The larger concern might end up being the code size, JS parse and re-compression time has been troublesome for us.  (Although the JS code could be minified one way or another...)

Comment 6

[Frederik Braun [:freddy]]

Oh, please don't generate JS code :) I'd prefer you just replace the iinnerHTML with a document.createElement/appendChild code.
But to save you typing out these multiple extra lines you could use my tool offline and then copypasta the output of it into the source code of the email app. Done. You never need to touch html2dom agian, if you don't want to :))

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

It would be part of the build step if we went that way.

Specifically, if you check out the contents of apps/email/js/cards, you'll see a bunch of .html files like https://github.com/mozilla-b2g/gaia/blob/master/apps/email/js/cards/message_reader.html

They include just the HTML for the card we want to display.  The build step converts these into strings which get integrated into our concatenated/minified JS.

These end up looking like so in the concatenated file:
define('tmpl!cards/message_reader.html',['tmpl'], function (tmpl) { return tmpl.toDom('...'); });


Note: not everything gets optimized like that.  See https://github.com/mozilla-b2g/gaia/blob/master/apps/email/build/email.build.js#L12 for the specific things we roll-up right now.

Comment 8

[Frederik Braun [:freddy]]

Ah, that's enlightening. If you do decide using html2dom in there, I'll be happy to make my tiny side-project more useful when it comes to integration :)

Comment 9

[BMO Automation]

Firefox OS is not being worked on