Closed g-k closed 6 years ago
@g-k @Rob--W I'll merge this.
@Rob--W do you mind filing issues for these comments you raised here, so we can tackle them separately, please?
The comments that I posted here were a code review. I expected the changes to be made before the PR was merged. The changes are really trivial: Remove line 34 (.replace(/\//g, '/');
) and remove escapeHTML(
and )
from line 905.
Hey Rob thanks for the reviews. I left the changes in to cover possible postxss / dangling tag type vectors for L35 and browser bugs in textContent for L905. Don't think it hurts to do extra escaping here, but would be interested in any cases where it reduces the security of the addon.
per discussion in https://github.com/mozilla/speaktome/pull/258
Assuming
item.confidence
anditem.text
are always text and don't need to insert elements into the DOM then we can html escape them instead.Also, as Andre pointed out errorMsg is always text (since we set it's value to one of a few options just above), but we'll escape it anyway in case the returned errors change or get passed through at some point.
This lets us remove DOMPurify and make the extensions slightly smaller.
r? @andrenatal