From e9fe3d29b6056e53b15a92ef4040bb020c0395ab Mon Sep 17 00:00:00 2001
From: Martin Ball <martin.ball@ucscaregroup.co.uk>
Date: Thu, 22 Oct 2026 16:12:05 +0100
Subject: [PATCH] #3692: Markdown export escapes link destinations instead of
 deleting characters

Finding 53: link_target removed `<`, `>` and newlines from a destination after
is_safe_href had accepted it, so an accepted relative target such as
`java<script:alert(1)` was written as `<javascript:alert(1)>` (or `/</host` as
`//host`). The exporter now backslash-escapes `<`, `>`, a backslash that a reader would
treat as an escape, and `&` that starts an entity reference, so a CommonMark reader gets
back exactly the checked target; control characters, which the check ignores, are still
left out. The importer now ends a `<...>` destination at the first unescaped `>`, so these
round-trip. Ordinary destinations are written as before.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
---
 crates/formats/src/markdown.rs | 75 +++++++++++++++++++++++++++++++---
 1 file changed, 70 insertions(+), 5 deletions(-)

diff --git a/crates/formats/src/markdown.rs b/crates/formats/src/markdown.rs
index 8274effe0..3617af530 100644
--- a/crates/formats/src/markdown.rs
+++ b/crates/formats/src/markdown.rs
@@ -799,10 +799,18 @@ fn link_dest(s: &str, at: usize) -> Option<(String, usize)> {
     i += ws;
     let r = s.get(i..)?;
     let (dest, used) = if let Some(inner) = r.strip_prefix('<') {
-        let end = inner.find(['>', '\n'])?;
-        if inner.get(end..)?.starts_with('\n') {
-            return None;
-        }
+        // The first `>` not escaped by a backslash ends it; a line break first means no link.
+        let b = inner.as_bytes();
+        let mut k = 0;
+        let end = loop {
+            match b.get(k)? {
+                b'\\' => k += 1,
+                b'>' => break k,
+                b'\n' => return None,
+                _ => {}
+            }
+            k += 1;
+        };
         (inner.get(..end)?.to_string(), end + 2)
     } else {
         let mut depth = 0i32;
@@ -1436,8 +1444,35 @@ fn inlines_md_impl(inlines: &[Inline], in_table: bool) -> String {
     out
 }
 
+/// `l` as a Markdown link destination that reads back as exactly `l`, the target
+/// `is_safe_href` passed. What Markdown would decode (a backslash escape, an entity reference)
+/// or end the destination on (`<`, `>`) is backslash-escaped, never deleted: deleting `<` or `>`
+/// could turn an accepted relative target into `javascript:…` or `//host`. Control characters
+/// can't be written in a destination and are left out, as the safety check ignores them.
 fn link_target(l: &str) -> String {
-    if l.contains([' ', '(', ')', '<', '>']) { format!("<{}>", l.replace(['<', '>', '\n'], "")) } else { l.to_string() }
+    let l: String = l.chars().filter(|c| !c.is_ascii_control()).collect();
+    let mut out = String::with_capacity(l.len() + 2);
+    for (i, c) in l.char_indices() {
+        let rest = l.get(i + c.len_utf8()..).unwrap_or("");
+        match c {
+            '<' | '>' => out.push('\\'),
+            // A backslash is literal unless punctuation (or the closing `)`/`>`) follows it.
+            '\\' if rest.chars().next().is_none_or(|n| n.is_ascii_punctuation()) => out.push('\\'),
+            '&' if entity_like(rest) => out.push('\\'),
+            _ => {}
+        }
+        out.push(c);
+    }
+    if l.contains([' ', '(', ')', '<', '>']) { format!("<{out}>") } else { out }
+}
+
+/// Whether `rest` (the text after a `&`) starts like an entity reference (`amp;`, `#106;`,
+/// `#x6A;`), which a Markdown reader would decode.
+fn entity_like(rest: &str) -> bool {
+    let Some(end) = within(rest, 34).find(';') else { return false };
+    let body = rest.get(..end).unwrap_or("");
+    let body = body.strip_prefix('#').map_or(body, |n| n.strip_prefix(['x', 'X']).unwrap_or(n));
+    !body.is_empty() && body.chars().all(|c| c.is_ascii_alphanumeric())
 }
 
 fn marker(m: Mark) -> &'static str {
@@ -2167,4 +2202,34 @@ mod tests {
         let s = inl("[a](http://x.org) [b*](http://y.org)");
         assert!(s.iter().any(|i| matches!(i, Inline::Text(t, f) if t == "b*" && f.link.as_deref() == Some("http://y.org"))), "{s:?}");
     }
+
+    #[test]
+    fn exported_link_destinations_read_back_unchanged() {
+        // Removing `<` and `>` from a destination after the safety check could make a relative
+        // target a script or network-path one (`java<script:…` → `javascript:…`). They are
+        // escaped instead, as are backslash escapes and entity references a reader would decode.
+        let md = |l: &str| inlines_md(&[Inline::Text("x".into(), Fmt { link: Some(l.into()), ..Default::default() })]);
+        assert_eq!(md("java<script:alert(1)"), "[x](<java\\<script:alert(1)>)");
+        for l in [
+            "java<script:alert(1)",
+            "/</evil.example/(x)",
+            "a>b c",
+            "&#106;avascript:alert(1)",
+            "&#x6A;avascript:x",
+            "&amp;",
+            "javascript\\:x",
+            "dir\\",
+            "dir\\(1)\\",
+            "q?a=1&b=2",
+        ] {
+            let out = md(l);
+            let back = inl(&out);
+            let got: Vec<_> = back.iter().filter_map(|i| if let Inline::Text(_, f) = i { f.link.clone() } else { None }).collect();
+            assert_eq!(got, [l.to_string()], "{l:?} → {out:?}");
+        }
+        // Ordinary destinations are written as before.
+        assert_eq!(md("https://example.com/a?b=1&c=2"), "[x](https://example.com/a?b=1&c=2)");
+        assert_eq!(md("https://example.com/a b"), "[x](<https://example.com/a b>)");
+        assert_eq!(md("https://e.com/x\ny"), "[x](https://e.com/xy)");
+    }
 }