Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recent change breaks transformers that modify DOM #177

Closed
zetter opened this issue Mar 20, 2018 · 2 comments
Closed

Recent change breaks transformers that modify DOM #177

zetter opened this issue Mar 20, 2018 · 2 comments
Labels

Comments

@zetter
Copy link
Contributor

zetter commented Mar 20, 2018

Thanks for the quick release of 4.6.3 to fix the vulnerability.

I upgraded the sanitize gem on a project I'm working on and found that an integration test was failing. I tracked down the cause of the break to a 4.6.2 change to optimize the memory usage of sanitize.

Previously transformers could make changes to the DOM and other transformers would work on the updated DOM. One of the changes to optimize memory usage caches the name of the DOM node between transformers so this is no longer the case.

I've provided a test case and possible fix by removing this particular memory optimization.

My project's use case is similar to the test- using transformers to change the name of a HTML tag. I was worried that we might be using sanitize in an unusual and unsupported way, but looking at the readme it sounds like this is an expected use of transformers:

Any changes made to the current node or to the document will be reflected instantly in the document and passed on to subsequently called transformers and to Sanitize itself.

If you think the way the transformer is written is surprising and there is a better way, please let me know.

Thanks for your time

@janklimo
Copy link
Contributor

Thank you for catching that @zetter. I wasn't aware people use transformers to change node names,
but it looks like a perfectly valid use case, so the change I introduced in 4.6.2 in transform_node! might have to be reverted.

@rgrove
Copy link
Owner

rgrove commented Mar 20, 2018

@zetter Good catch! I don't think that's a common use case, but it's definitely one Sanitize is meant to support. I've merged your fix and test case and released 4.6.4. Thanks!

Fixed in 71d84a8

@rgrove rgrove closed this as completed Mar 20, 2018
@rgrove rgrove added the bug label Mar 20, 2018
stanhu added a commit to stanhu/sanitize that referenced this issue Jul 24, 2018
In 4.6.2, rgrove#175 significantly cut down on memory usage by caching the lowercase
version of the node name. However, as reported in rgrove#177, this broke certain
transformers that modified the node name since the name was cached. We can
bring back this optimization by updating the node name only if it has been
changed.

Closes rgrove#184
stanhu added a commit to stanhu/sanitize that referenced this issue Jul 24, 2018
In 4.6.2, rgrove#175 significantly cut down on memory usage by caching the lowercase
version of the node name. However, as reported in rgrove#177, this broke certain
transformers that modified the node name since the name was cached. We can
bring back this optimization by updating the node name only if it has been
changed.

Closes rgrove#184
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants