-
-
Notifications
You must be signed in to change notification settings - Fork 383
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
header tags are html-rendered with name="" instead of id="" #135
Comments
This is a long standing issue in the GitHub markup pipeline, so much so that I think this should conclude with at least a statement in the README as to why GitHub does what they do. Intuition tells me that GitHub does it this way to avoid conflicts with CSS in user content. The challenge is that a markup file can have absolutely any heading, which means any anchor name. If assigned as an id attribute, there's a high likelyhood that one of the headings is going to match an id selector in the GitHub stylesheet and get styled in unexpected ways. Watching GitHub work over the last few years, I've seen several strategies applied here to avoid conflicts, from dropping anchors outright to prefixing them with something like "user-content-". Every change seems to break something different. From what I can see, the More transparency around decision making would be very welcomed here. I do understand the challenge that GitHub faces trying to avoid conflicts with open-ended content. Unless we all enjoy seeing the same issue get filed once a month, perhaps it's time to document the reason for why it is the way it is. In other words, answer the question, "Why do we define heading anchors using the |
To give some context and address the concerns that @mojavelinux raised… TL;DR: Ideally, browsers would implement a way to deep-link to elements that doesn't clobber the DOM and have a negative impact on CSS and JavaScript, but they don't, so we've had to come up with workarounds. We originally started sanitizing So a few months ago, we rolled out a change that prefixed all While this again is not a perfect solution, but it has worked really well. It allows users to deep link to content without DOM clobbering. Using a similar technique, I think we can go back to allow See #111 and github/markup#219 (comment) for related discussions. /cc @josh |
I think adding back id support with that prefix would be legit. Pretty much address any security concerns around breaking QSA and overriding application defined behaviors built on ids. |
As gjtorikian#135 points out, `name` is intended for form elements and is obsolete on anchors.
@bkeepers, what browsers does this issue come up in? I face the similar problem with anchors now, and |
@rlidwka I think I experienced it in both Firefox and Chrome a few months ago. Maybe that has since changed. Our security team was able to make a page completely unusable by adding content with |
This bug was previously reported in markup repo
A section header in markdown is rendered as
h3 > a[name]
To anchor an element in URL, the
id
attribute must be usedThe
name
attribute is reserved for usage in form elements. Its availability as aid
is a inheritance from Netscape days and should not have been used here.Alas, as I read in your README.md, « Note that the id attribute is not whitelisted. »
So how can I patch this ?
The text was updated successfully, but these errors were encountered: