Skip to content

Loading…

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

Closed
gorhill opened this Issue · 4 comments

2 participants

@gorhill

As per Mozilla's preliminary review.

@Deathamns

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

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

Sure.

@gorhill gorhill added a commit that closed this issue
@gorhill gorhill this fixes #954 3544989
@gorhill gorhill closed this in 3544989
@gorhill

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
@gorhill gorhill added a commit that referenced this issue
@gorhill gorhill this completes fix of #954 5a2f6e0
@gorhill gorhill closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.