Closed
Bug 1200298
Opened 9 years ago
Closed 9 years ago
Gaia's eslint marks safe .innerHTML use as unsafe
Categories
(Firefox OS Graveyard :: Gaia::Build, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: cr, Assigned: pauljt)
Details
Gaia's js linter is going over the top when it comes to .innerHTML. Here's an example from a test suite I wrote recently:
setup(function () {
MockHtml = document.createElement('html');
MockHtml.appendChild(document.createElement('div'));
MockHtml.firstChild.innerHTML = `<a role="key" href="#" data-key="2" ` +
`class="row0"><div>2<span>ABC</span></div></a>`;
});
It produces the error:
apps/system/test/unit/lockscreen/lockscreen_inputpad_test.js: line 69, col 6, Error - Unsafe assignment to innerHTML (no-unsafe-innerhtml/no-unsafe-innerhtml)
make: *** [eslint] Error 1
I'm perfectly aware of the perils of .innerHTML, but assigning from a constant is not unsafe.
I see three options to fix this:
1. Fix the occurrence, replace with 20 lines of step-by-step code that builds the required tree.
2. Disable innerHTML check for gaia tests.
3. Relax .innerHTML check, allow assignment with constant lvalue.
I doubt going for 1, effectively disallowing all .innerHTML is helping. I won't be the last who needs to write tests that check against complex HTML structures. 2 would be more of a workaround until 3 makes for proper linting.
Comment 1•9 years ago
|
||
The plugin already is pretty generous, allowing Literals and TemplateLiterals (like you're using), as long as there are no expressions inside them to be evaluated:
https://github.com/mozfreddyb/eslint-plugin-no-unsafe-innerhtml/blob/master/lib/rules/no-unsafe-innerhtml.js#L37
I suspect the problem here is that you are invoking string concatenation. It seems like eslint-plugin-no-unsafe-innerhtml could be enhanced to perform some type of constant-folding (or leveraging existing constant folding mechanisms in eslint or other plugins).
In the meantime, one workaround would be to leverage that backtick-quoted-strings can span newlines. The newline will become part of the string, but presumably that's not the end of the world?
Comment 2•9 years ago
|
||
Er, and although I think it's definitely worth considering the enhancement to the linter, I don't really see a need to require safe innerHTML in our test logic, so option 2 does seem fine to me.
Specifically, I think we would only want to lint innerHTML in tests if we felt there was a serious hygiene issue that would cause people to write bad/dangerous uses of innerHTML in our non-test code. While I do think newer developers could take more of a cavalier approach to innerHTML if they see it being used willy-nilly in tests, the linter would help providing a teaching opportunity when it fails their dangerous production code.
Reporter | ||
Comment 3•9 years ago
|
||
Thanks for the workaround. It works fine for me in this case, because it's just a test.
This strikes me as highly irregular code formatting one should really avoid, but the linter seems to approve.
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → ptheriault
Comment 4•9 years ago
|
||
Thanks for responding in my stead in Andrew!
The linter could indeed need a special case that allows concatenation (i.e., binary expressions with a '+' operator) of literals (Literal, TemplateLiteral). But the more special cases we include, the slower it gets :)
It shouldn't be too hard to add, but I don't consider this a high priority. I'm happy to tell you how this could be added, if you need this urgently ;)
Comment 5•9 years ago
|
||
(In reply to Christiane Ruetten [:cr] from comment #3)
> This strikes me as highly irregular code formatting one should really avoid,
> but the linter seems to approve.
The backtick quote is definitely intended to support multi-line strings. These are a new thing for JS, so indeed it may take some time for the sensation there's something wrong with code like that to go away. (I still find it disconcerting when I use it to span newlines :) Which is not to say that we shouldn't fix it...
(In reply to Frederik Braun [:freddyb] from comment #4)
> But the more special cases we include, the slower it gets :)
If the nested test is only run in what would otherwise be a failure case, any slowdown would still seem like a net productivity win for developers. Having said that, http://eslint.org/docs/developer-guide/working-with-rules#per-rule-performance documents how running eslint with TIMING set in the environment will provide info on the 10 slowest rules. If this rule is getting up there, it may be time to optimize the rule further and/or clean up constant-folding cases.
For what it's worth, I did look into constant folding in eslint, and it seemed like the eslint developers don't want any rules in eslint itself that would perform AST traversals other than the one used for rule triggering or that otherwise have a high time cost. Which means that there isn't really an infrastructure in place for deep magic static analysis. However, as you point out, this case doesn't/shouldn't require that much complexity since it's probably reasonable to constrain the concatenated literals to exist directly at the assignment site.
Comment 6•9 years ago
|
||
Yeah, I'd love to have proper constant folding… :)
For now I just allowed binary expressions, as long as the operators are what we already allow (literals, calls to safeHTML etc.)
See https://github.com/mozfreddyb/eslint-plugin-no-unsafe-innerhtml/commit/cef76e8227aceea946d30d0f9f349a8ad9318be0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•