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

Switch sanitization to a Set and add cleanup #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

developit
Copy link
Owner

This fixes #20.

@odinhb
Copy link

odinhb commented Oct 20, 2022

@developit Hey I'm using vhtml and quite liking it but I happened to look at some of these open pull requests and issues, and it looks like development has stalled a bit.

There are outstanding problems I'd deem quite important to fix (#20, #34), and this pull request seems good to merge far as I can tell.

There is also the fragment support which supposedly warrants some code changes, and a waiting MR (which also looks good to me) for that as well. (#31 (comment), #33)

Is there something I could do to help?

@johannesodland
Copy link

Wouldn't this be prone to the same issue you mentioned in #23 (comment)?

The string might be evicted from the sanitized set before we use it, changing the output of vhtml(), returning escaped text instead of HTML.

Example from the comment:

const link = vhtml('a', { href: '/' }, 'hello');
console.log(link);
// `<a href="/">hello</a>`  <-- now mapped as an allowed string

const div = vhtml('div', {}, link);
console.log(div);
// `<div><a href="/">hello</a></div>`  <-- `link` is in the map, doesn't get escaped

// Imagine enough time passes that `link` is evicted from the string mappings.
await sleep(60);

// we pass `link` expecting it to be HTML, but it's no longer in the mapping.
const newHtml = vhtml('p', {}, link);

console.log(newHtml);
// `<div>&lt;a href=&quot;/&quot;&gt;hello&lt;/a&gt;</div>`  <-- the value of `link` gets escaped

@odinhb
Copy link

odinhb commented Oct 25, 2023

@johannesodland You are correct that this PR introduces more problems than it fixes.

I forked this project here in order to fix the outstanding issues with this project. It might suit your use case. My impression is that vhtml is abandoned by @developit.

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

Successfully merging this pull request may close these issues.

SSR Memory leak
4 participants