Don't apply the clobber prefix when it's already present

[radarfox]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and couldn’t find anything (or linked relevant results below)

Problem

I would like to use the clobber prefix in both footnotes and sanitization. When the clobber prefix is set in remark-rehype, footnotes IDs and links to them are correctly rendered with the clobber prefix included. Then, when the sanitize plugin is applied (to prefix IDs in the text outside footnotes), the clobber prefix is added once again resulting in IDs like: user-content-user-content-foo. 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.

Solution

IMHO since the sanitize plugin doesn't know anything about footnotes structure, it should be the remark-rehype plugin that should apply the clobber prefix, where it's needed. The sanitize plugin should detect, that the clobber prefix is already applied and in that case it shouldn't do nothing for that element.

The implementation should be quite easy. Just add && !value.startsWith(state.schema.clobberPrefix) to the end of this line.

Alternatives

For now, I'm hotfixing this by adding another plugin between remark-rehype and rehype-sanitize, that is removing clobber prefix from all elements, so rehype-sanitize can add it again without the duplication.

[ChristianMurphy]

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?

[radarfox]

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.

[radarfox]

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.

[ChristianMurphy]

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.

[wooorm]

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.

[ben519]

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

[wooorm]

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.

[wooorm]

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.

[github-actions]

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