Skip to main content

max / makenotwork

docengine: give headings ids so TOC anchors resolve render_toc_html emitted href="#anchor" and nothing ever put a matching id on a heading, so every TOC link went nowhere. Two things were needed, not one: injecting the id, and letting it survive ammonia, whose defaults strip id from everything. Renderer::with_heading_ids does both, off by default. It is the only setting that lets an id through sanitization, and an attacker-chosen id is a DOM-clobbering primitive, so it stays opt-in and widens the policy for id on h1-h6 alone. The TOC and the headings must agree exactly or the links are dead anyway, so both now drive one AnchorGen over the same heading sequence. That also lands the anchor-slugify item: repeats get GitHub's -1/-2 suffixes instead of colliding, and underscores survive (GitHub keeps them, so #render_raw addressed nothing before). Note the task's "Version 2.0 becomes version20" claim was wrong; the existing test asserts version-20 and passes. The real divergences were the missing dedup counter and the dropped underscores. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-20 18:43 UTC
Commit: a9d6d181ad3e455fa55d16a20bb5ca0c49b4fba1
Parent: da4de4c
3 files changed, +219 insertions, -18 deletions
M shared/docengine/src/render.rs +122 -2
@@ -1,6 +1,7 @@
1 1 use pulldown_cmark::{CowStr, Event, Options, Parser, Tag, TagEnd, html};
2 2
3 3 use crate::sanitize::SanitizePreset;
4 + use crate::toc::AnchorGen;
4 5
5 6 /// Returns true if the URL uses a scheme not in the safe allowlist.
6 7 ///
@@ -40,9 +41,47 @@ pub struct Renderer {
40 41 strip_images: bool,
41 42 strip_raw_html: bool,
42 43 dangerous_scheme_filter: bool,
44 + heading_ids: bool,
43 45 sanitize: SanitizePreset,
44 46 }
45 47
48 + /// Give every heading the anchor [`extract_toc`](crate::extract_toc) will point
49 + /// at, so a rendered TOC actually navigates.
50 + ///
51 + /// `id` is assigned in document order through the same [`AnchorGen`] the TOC
52 + /// uses, which is what keeps the two in agreement — including the `-1`, `-2`
53 + /// suffixes on repeated headings.
54 + fn inject_heading_ids(mut events: Vec<Event<'_>>) -> Vec<Event<'_>> {
55 + let mut anchors = AnchorGen::default();
56 + let mut i = 0;
57 + while i < events.len() {
58 + if !matches!(events[i], Event::Start(Tag::Heading { .. })) {
59 + i += 1;
60 + continue;
61 + }
62 + // Heading text is only known once its inner events have been seen.
63 + let mut text = String::new();
64 + let mut j = i + 1;
65 + while j < events.len() {
66 + match &events[j] {
67 + Event::End(TagEnd::Heading(_)) => break,
68 + Event::Text(t) => text.push_str(t),
69 + Event::Code(c) => text.push_str(c),
70 + _ => {}
71 + }
72 + j += 1;
73 + }
74 + let anchor = anchors.next(&text);
75 + if let Event::Start(Tag::Heading { id, .. }) = &mut events[i] {
76 + // Always None in practice: ENABLE_HEADING_ATTRIBUTES is never set,
77 + // so markdown cannot carry its own `{#id}`.
78 + *id = Some(CowStr::Boxed(anchor.into_boxed_str()));
79 + }
80 + i = j + 1;
81 + }
82 + events
83 + }
84 +
46 85 impl Renderer {
47 86 /// GFM features, default ammonia sanitization. Suitable for trusted content
48 87 /// like docs and blog posts.
@@ -56,6 +95,7 @@ impl Renderer {
56 95 strip_images: false,
57 96 strip_raw_html: false,
58 97 dangerous_scheme_filter: false,
98 + heading_ids: false,
59 99 sanitize: SanitizePreset::Permissive,
60 100 }
61 101 }
@@ -72,6 +112,7 @@ impl Renderer {
72 112 strip_images: true,
73 113 strip_raw_html: false,
74 114 dangerous_scheme_filter: false,
115 + heading_ids: false,
75 116 sanitize: SanitizePreset::Standard,
76 117 }
77 118 }
@@ -88,6 +129,7 @@ impl Renderer {
88 129 strip_images: true,
89 130 strip_raw_html: true,
90 131 dangerous_scheme_filter: true,
132 + heading_ids: false,
91 133 sanitize: SanitizePreset::Strict,
92 134 }
93 135 }
@@ -104,6 +146,7 @@ impl Renderer {
104 146 strip_images: false,
105 147 strip_raw_html: false,
106 148 dangerous_scheme_filter: false,
149 + heading_ids: false,
107 150 sanitize: SanitizePreset::Permissive,
108 151 }
109 152 }
@@ -153,6 +196,23 @@ impl Renderer {
153 196 self
154 197 }
155 198
199 + /// Emit `id` on every heading, matching the anchors [`extract_toc`] emits,
200 + /// so a rendered table of contents navigates instead of going nowhere.
201 + ///
202 + /// Off by default, and deliberately opt-in: it is the only setting that
203 + /// lets an `id` attribute survive sanitization, and an attacker-chosen `id`
204 + /// on a page is a DOM-clobbering primitive (an element whose `id` shadows a
205 + /// global the page's own script reads). Generated anchors are slugified to
206 + /// alphanumerics, hyphens, and underscores, but raw-HTML headings in the
207 + /// source can also carry an `id` once this is on. Enable it for trusted
208 + /// content — docs, your own long-form — not for arbitrary UGC.
209 + ///
210 + /// [`extract_toc`]: crate::extract_toc
211 + pub fn with_heading_ids(mut self, enabled: bool) -> Self {
212 + self.heading_ids = enabled;
213 + self
214 + }
215 +
156 216 fn build_options(&self) -> Options {
157 217 let mut opts = Options::empty();
158 218 if self.tables {
@@ -179,7 +239,7 @@ impl Renderer {
179 239 return String::new();
180 240 }
181 241 let html_output = self.render_raw(input);
182 - self.sanitize.clean(&html_output)
242 + self.sanitize.clean_with(&html_output, self.heading_ids)
183 243 }
184 244
185 245 /// Render markdown to sanitized HTML with metadata.
@@ -229,7 +289,13 @@ impl Renderer {
229 289 });
230 290
231 291 let mut output = String::new();
232 - html::push_html(&mut output, filtered);
292 + if self.heading_ids {
293 + // Buffering is required: a heading's anchor depends on text that
294 + // only arrives after its Start event.
295 + html::push_html(&mut output, inject_heading_ids(filtered.collect()).into_iter());
296 + } else {
297 + html::push_html(&mut output, filtered);
298 + }
233 299 output
234 300 }
235 301 }
@@ -535,4 +601,58 @@ mod tests {
535 601 fn result_has_rel(html: &str, rel_value: &str) -> bool {
536 602 html.contains(rel_value)
537 603 }
604 +
605 + // ===== heading ids =====
606 +
607 + #[test]
608 + fn heading_ids_are_off_by_default() {
609 + assert!(Renderer::permissive().render("## Section").contains("<h2>Section</h2>"));
610 + assert!(!Renderer::permissive().render("## Section").contains("id="));
611 + }
612 +
613 + #[test]
614 + fn heading_ids_survive_sanitization_when_enabled() {
615 + // Ammonia strips `id` by default, so this asserts the sanitizer opt-in
616 + // is wired, not just the event injection.
617 + let html = Renderer::permissive()
618 + .with_heading_ids(true)
619 + .render("## Section Title");
620 + assert!(html.contains(r#"<h2 id="section-title">"#), "got: {html}");
621 + }
622 +
623 + #[test]
624 + fn every_toc_anchor_has_a_matching_heading_id() {
625 + // The bug this fixes: TOC emitted href="#anchor" and nothing on the
626 + // page carried that id. Pin the two together, repeats included.
627 + let md = "# Guide\n\n## Setup\n\n### Notes\n\n## Usage\n\n### Notes\n\n## render_raw";
628 + let html = Renderer::permissive().with_heading_ids(true).render(md);
629 + let toc = crate::extract_toc(md);
630 + assert_eq!(toc.len(), 6);
631 + for entry in &toc {
632 + assert!(
633 + html.contains(&format!(r#"id="{}""#, entry.anchor)),
634 + "TOC points at #{} but no heading carries it: {html}",
635 + entry.anchor
636 + );
637 + }
638 + }
639 +
640 + #[test]
641 + fn heading_ids_enabled_on_other_presets_too() {
642 + let html = Renderer::strict().with_heading_ids(true).render("## Section");
643 + assert!(html.contains(r#"id="section""#), "got: {html}");
644 + }
645 +
646 + #[test]
647 + fn heading_ids_do_not_open_other_attributes() {
648 + // The opt-in must widen the sanitizer for `id` on headings only.
649 + let html = Renderer::permissive()
650 + .with_heading_ids(true)
651 + .render(r#"<h2 id="ok" onclick="evil()" class="x">T</h2>"#);
652 + assert!(!html.contains("onclick"), "event handler must stay stripped: {html}");
653 + let html = Renderer::permissive()
654 + .with_heading_ids(true)
655 + .render(r#"<p id="para">text</p>"#);
656 + assert!(!html.contains(r#"id="para""#), "id on non-headings stays stripped: {html}");
657 + }
538 658 }
M shared/docengine/src/sanitize.rs +29 -11
@@ -27,26 +27,44 @@ pub(crate) fn permissive_builder() -> ammonia::Builder<'static> {
27 27
28 28 impl SanitizePreset {
29 29 pub(crate) fn clean(&self, html: &str) -> String {
30 - match self {
31 - SanitizePreset::Permissive | SanitizePreset::Standard => {
32 - permissive_builder().clean(html).to_string()
30 + self.clean_with(html, false)
31 + }
32 +
33 + /// Clean `html`, optionally letting `id` survive on headings.
34 + ///
35 + /// Ammonia's defaults strip `id` from everything, which is why TOC anchors
36 + /// pointed at nothing: the renderer could emit ids all it liked and
37 + /// sanitization removed them. `allow_heading_ids` is scoped as tightly as
38 + /// the feature permits — `id` only, `h1`-`h6` only, and only when the
39 + /// caller set [`Renderer::with_heading_ids`], which carries the reasoning
40 + /// about when that is safe.
41 + ///
42 + /// [`Renderer::with_heading_ids`]: crate::Renderer::with_heading_ids
43 + pub(crate) fn clean_with(&self, html: &str, allow_heading_ids: bool) -> String {
44 + let mut builder = match self {
45 + SanitizePreset::Permissive | SanitizePreset::Standard => permissive_builder(),
46 + SanitizePreset::Strict => {
47 + let mut b = ammonia::Builder::default();
48 + b.link_rel(Some("noopener noreferrer nofollow"));
49 + b
33 50 }
34 - SanitizePreset::Strict => ammonia::Builder::default()
35 - .link_rel(Some("noopener noreferrer nofollow"))
36 - .clean(html)
37 - .to_string(),
38 51 SanitizePreset::Minimal => {
39 52 let tags: std::collections::HashSet<&str> =
40 53 ["p", "em", "strong", "code", "br", "pre"]
41 54 .iter()
42 55 .copied()
43 56 .collect();
44 - ammonia::Builder::default()
45 - .tags(tags)
46 - .clean(html)
47 - .to_string()
57 + let mut b = ammonia::Builder::default();
58 + b.tags(tags);
59 + b
60 + }
61 + };
62 + if allow_heading_ids {
63 + for tag in ["h1", "h2", "h3", "h4", "h5", "h6"] {
64 + builder.add_tag_attributes(tag, ["id"]);
48 65 }
49 66 }
67 + builder.clean(html).to_string()
50 68 }
51 69 }
52 70
M shared/docengine/src/toc.rs +68 -5
@@ -1,7 +1,39 @@
1 + use std::collections::HashMap;
2 + use std::collections::hash_map::Entry;
3 +
1 4 use pulldown_cmark::{Event, Options, Parser, Tag, TagEnd};
2 5
3 6 use crate::escape::html_escape;
4 7
8 + /// Assigns heading anchors over one document, disambiguating repeats the way
9 + /// GitHub does (`overview`, `overview-1`, `overview-2`).
10 + ///
11 + /// The table of contents and the rendered headings must agree character for
12 + /// character or every TOC link is dead, so both drive this same generator over
13 + /// the same heading sequence rather than each slugifying on its own.
14 + #[derive(Default)]
15 + pub(crate) struct AnchorGen {
16 + seen: HashMap<String, usize>,
17 + }
18 +
19 + impl AnchorGen {
20 + /// The anchor for the next heading in document order.
21 + pub(crate) fn next(&mut self, heading_text: &str) -> String {
22 + let base = make_anchor(heading_text);
23 + match self.seen.entry(base.clone()) {
24 + Entry::Vacant(slot) => {
25 + slot.insert(0);
26 + base
27 + }
28 + Entry::Occupied(mut slot) => {
29 + let n = slot.get() + 1;
30 + slot.insert(n);
31 + format!("{base}-{n}")
32 + }
33 + }
34 + }
35 + }
36 +
5 37 /// A single entry in a table of contents.
6 38 #[derive(Debug, Clone, PartialEq, Eq)]
7 39 pub struct TocEntry {
@@ -18,6 +50,7 @@ pub fn extract_toc(markdown: &str) -> Vec<TocEntry> {
18 50
19 51 let parser = Parser::new_ext(markdown, options);
20 52 let mut entries = Vec::new();
53 + let mut anchors = AnchorGen::default();
21 54 let mut in_heading: Option<u8> = None;
22 55 let mut heading_text = String::new();
23 56
@@ -35,7 +68,7 @@ pub fn extract_toc(markdown: &str) -> Vec<TocEntry> {
35 68 }
36 69 Event::End(TagEnd::Heading(_)) => {
37 70 if let Some(level) = in_heading.take() {
38 - let anchor = make_anchor(&heading_text);
71 + let anchor = anchors.next(&heading_text);
39 72 entries.push(TocEntry {
40 73 level,
41 74 text: heading_text.clone(),
@@ -67,13 +100,17 @@ pub fn render_toc_html(entries: &[TocEntry]) -> String {
67 100 html
68 101 }
69 102
70 - /// GitHub-style anchor generation: lowercase, spaces to hyphens, strip
71 - /// non-alphanumeric (except hyphens).
103 + /// GitHub-style anchor generation: lowercase, whitespace to hyphens, drop
104 + /// everything that is not alphanumeric, a hyphen, or an underscore.
105 + ///
106 + /// Underscores survive because GitHub keeps them, so `render_raw` and
107 + /// `#render_raw` address the same heading on both. Repeat headings are
108 + /// disambiguated by [`AnchorGen`], not here — this function is pure.
72 109 fn make_anchor(text: &str) -> String {
73 110 text.to_lowercase()
74 111 .chars()
75 - .map(|c| if c == ' ' { '-' } else { c })
76 - .filter(|c| c.is_alphanumeric() || *c == '-')
112 + .map(|c| if c.is_whitespace() { '-' } else { c })
113 + .filter(|c| c.is_alphanumeric() || *c == '-' || *c == '_')
77 114 .collect()
78 115 }
79 116
@@ -104,6 +141,32 @@ mod tests {
104 141 }
105 142
106 143 #[test]
144 + fn anchor_keeps_underscores() {
145 + // GitHub keeps `_`, so `render_raw` must not slug to `renderraw` or a
146 + // hand-written `#render_raw` link lands nowhere.
147 + assert_eq!(make_anchor("render_raw"), "render_raw");
148 + assert_eq!(make_anchor("Doc Loader_v2"), "doc-loader_v2");
149 + }
150 +
151 + #[test]
152 + fn repeat_headings_get_disambiguating_suffixes() {
153 + let mut anchors = AnchorGen::default();
154 + assert_eq!(anchors.next("Overview"), "overview");
155 + assert_eq!(anchors.next("Overview"), "overview-1");
156 + assert_eq!(anchors.next("Overview"), "overview-2");
157 + assert_eq!(anchors.next("Other"), "other");
158 + assert_eq!(anchors.next("Overview"), "overview-3");
159 + }
160 +
161 + #[test]
162 + fn toc_anchors_are_unique_across_repeat_headings() {
163 + let md = "## Setup\n\n### Notes\n\n## Usage\n\n### Notes";
164 + let toc = extract_toc(md);
165 + let anchors: Vec<&str> = toc.iter().map(|e| e.anchor.as_str()).collect();
166 + assert_eq!(anchors, ["setup", "notes", "usage", "notes-1"]);
167 + }
168 +
169 + #[test]
107 170 fn extract_empty() {
108 171 let toc = extract_toc("No headings here, just text.");
109 172 assert!(toc.is_empty());