Don't apply the clobber prefix when it's already present about hast-util-sanitize HOT 9 CLOSED

Welcome @radarfox! 👋
I'm open to the idea, but am interested in further discussing alternatives first.
For example remark-rehype supports changing or not setting the clobberPrefix https://github.com/remarkjs/remark-rehype#options
Would it make more sense to ask it to not set the prefix there, rather than either:

• doing, undoing, then redoing the prefix
• adding conditionals everywhere to check if it has already been applied

Thoughts?

from hast-util-sanitize.

No, that wouldn't work, as I have mentioned in the issue description:

When the clobber prefix is NOT set in remark-rehype and the sanitize plugin is applied, then footnotes are broken, because IDs are correctly set to user-content-foo, but links are unprefixed e.g. foo.

from hast-util-sanitize.

adding conditionals everywhere to check if it has already been applied

This solution gives the most sense to me. It shouldn't be a performance problem. It will be checked as the last thing, so it will be invoked only for id and name attributes. Checking the constant length prefix shouldn't be a problem. For most values the first letter won't match and the startsWith will return false.

But I'm not insisting on this solution. If you manage to come up with some better one, that would work with enabled clobber prefix, I would be happy to use it.

from hast-util-sanitize.

I'm hotfixing this by adding another plugin

Unified does like small modular plugins 🙂

This solution gives the most sense to me. It shouldn't be a performance problem. It will be checked as the last thing, so it will be invoked only for id and name attributes. Checking the constant length prefix shouldn't be a problem.

I'm not opposed, it does feel a bit strange.
Generally plugins aim to be loosely coupled, this feels like tighter coupling.
Though interested what other @syntax-tree/maintainers think.

from hast-util-sanitize.

No, that wouldn't work, as I have mentioned in the issue description:

When the clobber prefix is NOT set in remark-rehype and the sanitize plugin is applied, then footnotes are broken, because IDs are correctly set to user-content-foo, but links are unprefixed e.g. foo.

That is intentional. The behavior there is like that to make sure things work safely by default.

This project here intentionally only rewrites id and name. Leaving href untouched.
Then with some browser code, you can make pretty URLs work with safe markup: https://github.com/rehypejs/rehype-sanitize#example-headings-dom-clobbering.

Perhaps this can be better documented, that if you use this, you can pass clobberPrefix: '' to remark-rehype?

Coming back into this, what if a user has a heading “User content 1”?
GitHub eventually ends up with

<h1 tabindex="-1" id="user-content-user-content-1" dir="auto"><a class="heading-link" href="#user-content-1">User content 1<svg …>…</svg></a></h1>

Adding this feature might help you here, but it would break this case.

from hast-util-sanitize.

Hit the same issue today.

@wooorm is the "pretty URLs" solution the only way to make this work? It feels like it adds a fair amount of complexity, particularly regarding where to inject the hash() function in my Next.js project. Also, the code uses setImmediate() which appears to be deprecated.

Note that someone else ran into this issue here.

Update

I was able to make the "pretty URLs" solution work with Next.js

from hast-util-sanitize.

That code shows what GitHub does. Instead of setImmediate, you can use requestIdleCallback, setTimeout, etc.

You can choose what to do. Whether you want pretty hashes or not. Whether you want safe hashes or not. It’s all possible.

from hast-util-sanitize.

Closing due to unresolved issues pointed out in #29 (comment). The issue is also sidestepped when doing what GH does: prefix the IDs but leave the hrefs untouched and use client-side code to “link” the two. I prefer that approach as it uses pretty URLs for users that share those URLs.

from hast-util-sanitize.

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

from hast-util-sanitize.