Skip to content
This repository has been archived by the owner on Sep 9, 2022. It is now read-only.

Replace instances of innerHTML with Node.cloneNode() #954

Closed
gorhill opened this issue Mar 6, 2015 · 4 comments
Closed

Replace instances of innerHTML with Node.cloneNode() #954

gorhill opened this issue Mar 6, 2015 · 4 comments

Comments

@gorhill
Copy link
Contributor

gorhill commented Mar 6, 2015

As per Mozilla's preliminary review.

@Deathamns
Copy link
Contributor

They're referring to this, but that goes through this sanitizer.
The first version I've sent contained an innerHTML assignment, and since it was plain text, I asked if that could be accepted, and probably this response for that request.
I guess the reviewer thought that I was talking about the code in 3p-filters.js, because the real innerHTML assignment was removed in the meantime (element-picker.js), so he couldn't see it.
So, there's nothing to do here, since we only read innerHTML (which is safe).

@gorhill
Copy link
Contributor Author

gorhill commented Mar 6, 2015

I think I should change it anyway, if only for easier maintenance. That code dates from the early days of HTTPSB. Cloning from a hidden template element in the HTML seems to be a better way to do this.

@Deathamns
Copy link
Contributor

Sure.

@gorhill
Copy link
Contributor Author

gorhill commented Mar 8, 2015

Actually, there is another non-trivial use (and thus "inefficient" as per AMO reviewer) of innerHTML in dyna-rules.html.

For trivial uses, I will leave them unchanged, since they are sanitized through vAPI.insertHTML.

@gorhill gorhill reopened this Mar 8, 2015
gorhill added a commit that referenced this issue Mar 9, 2015
@gorhill gorhill closed this as completed Mar 9, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants