From dbe35bc12696de3be4bda9cec4358f15cc371b90 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 27 Sep 2026 11:25:33 +0200 Subject: [PATCH] fix(github): close image and link bypasses in the issue Markdown sanitiser Checked against markdown-rs (the parser TextView uses), several inputs got past the line-based rewrite: - is_github_hosted cut the host only at `/`, so `https://evil.io?.githubusercontent.com/x.png` (and `#`, `\`, `/`) counted as GitHub-hosted and was fetched from evil.io. The host now ends at the first of `/?#\` and may hold only DNS characters. - `` values were written into `![..](..)` unescaped, so a `)` in the value closed the image and opened a second one from any host. Written destinations are now percent-encoded. - `` (which the HTML parser reads as ``) passed as an ordinary tag and loaded its src. - A kept link target was copied without scanning; when the parser ended the link elsewhere (open title, unbalanced paren) a `![..](..)` inside it came alive. Markup characters in it are now encoded. - `file:///...` and similar character references passed is_safe_target and decoded to a `file:` link. References are decoded before judging. The rewrite still cannot see every construct the way the parser does (code spans inside tag attributes, fences the parser rejects, multi-line link definitions), so the detail view now also checks the parsed tree: a block containing a non-GitHub image, an unsafe link or definition, or raw `` is drawn as its plain source instead. --- crates/tty7-core/src/core/github/markdown.rs | 282 +++++++++++++++++-- src/ui/github/detail.rs | 44 ++- 2 files changed, 309 insertions(+), 17 deletions(-) diff --git a/crates/tty7-core/src/core/github/markdown.rs b/crates/tty7-core/src/core/github/markdown.rs index 64fd363f..9801044b 100644 --- a/crates/tty7-core/src/core/github/markdown.rs +++ b/crates/tty7-core/src/core/github/markdown.rs @@ -148,7 +148,7 @@ fn sanitize_text(text: &str, image_label: &str, own_block: bool) -> String { && let Some((alt, url, len)) = inline_image(rest) && is_github_hosted(url) { - out.push_str(&image(alt, url)); + out.push_str(&image(alt, &escape_destination(url))); i += len; continue; } @@ -179,7 +179,18 @@ fn sanitize_text(text: &str, image_label: &str, own_block: bool) -> String { }; out.push_str(&target[..lead]); if is_safe_target(url) { - out.push_str(&body[..len.min(body.len())]); + // Copied, not scanned, so nothing in it may read as markup + // of its own: were the parser to end the link elsewhere + // (an unbalanced `(`, a title left open), a `![…](…)` or a + // tag inside the URL would come alive unsanitized. + let pointy = body.starts_with('<'); + if pointy { + out.push('<'); + } + out.push_str(&neutralize_markup(url)); + if pointy && len <= body.len() && body[..len].ends_with('>') { + out.push('>'); + } } else { out.push('#'); } @@ -188,7 +199,7 @@ fn sanitize_text(text: &str, image_label: &str, own_block: bool) -> String { } if rest.starts_with('<') { // `` → a link to what it would have loaded. - if starts_with_tag(rest, "img") { + if starts_with_tag(rest, "img") || starts_with_tag(rest, "image") { let end = rest.find('>').map_or(rest.len(), |e| e + 1); let tag = &rest[..end]; let src = attr(tag, "src").unwrap_or_default(); @@ -196,9 +207,9 @@ fn sanitize_text(text: &str, image_label: &str, own_block: bool) -> String { let label = alt.as_deref().unwrap_or(image_label); let label = label.replace(['[', ']'], ""); if is_github_hosted(&src) { - out.push_str(&image(&label, &src.replace(' ', "%20"))); + out.push_str(&image(&label, &escape_destination(&src))); } else if is_safe_target(&src) && !src.is_empty() { - out.push_str(&format!("[{label}]({})", src.replace(' ', "%20"))); + out.push_str(&format!("[{label}]({})", escape_destination(&src))); } else { out.push_str(&format!("[{label}](#)")); } @@ -287,7 +298,7 @@ fn attr(tag: &str, name: &str) -> Option { let before_ok = lower[..at] .chars() .last() - .is_some_and(|c| c.is_whitespace()); + .is_some_and(|c| c.is_whitespace() || c == '/'); let after = lower[from..].trim_start(); if !before_ok || !after.starts_with('=') { continue; @@ -395,9 +406,17 @@ pub fn is_github_hosted(url: &str) -> bool { let Some(rest) = url.strip_prefix("https://") else { return false; }; - let (host, path) = rest.split_at(rest.find('/').unwrap_or(rest.len())); - // Userinfo or a port would make the "host" above something else. - if host.contains(['@', ':']) { + // The authority ends at the first of these for a URL parser (`\\` counts + // as `/` in an `https` URL), so `https://evil.io?.githubusercontent.com` + // is evil.io. + let (host, path) = rest.split_at(rest.find(['/', '?', '#', '\\']).unwrap_or(rest.len())); + // Only plain DNS characters: userinfo, a port, an entity or a percent + // escape would each make the "host" above something else. + if host.is_empty() + || !host + .bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'.' || b == b'-') + { return false; } let host = host.to_ascii_lowercase(); @@ -417,15 +436,169 @@ pub fn is_github_hosted(url: &str) -> bool { /// Relative targets and fragments carry no scheme and so cannot name a program; /// they are kept. Whitespace and control characters are stripped first, the /// way a browser does, so ` jav\tascript:` is judged as `javascript:`. +/// +/// Character references are decoded before judging too — both the Markdown +/// and the HTML parser decode them in a URL, so `file:///x` is `file:///x` +/// by the time it is opened. An `&` still standing before the path, after +/// that, is a reference this decoder does not know, and is not guessed at. pub fn is_safe_target(url: &str) -> bool { - let cleaned: String = url - .chars() - .filter(|c| !c.is_whitespace() && !c.is_control()) - .collect(); - match scheme(&cleaned) { - None => !cleaned.contains(':') || cleaned.starts_with(['/', '#', '?', '.']), - Some(s) => matches!(s.to_ascii_lowercase().as_str(), "http" | "https" | "mailto"), + let judge = |url: &str| { + let cleaned: String = url + .chars() + .filter(|c| !c.is_whitespace() && !c.is_control()) + .collect(); + let head = &cleaned[..cleaned.find(['/', '?', '#']).unwrap_or(cleaned.len())]; + if head.contains('&') { + return false; + } + match scheme(&cleaned) { + None => !cleaned.contains(':') || cleaned.starts_with(['/', '#', '?', '.']), + Some(s) => matches!(s.to_ascii_lowercase().as_str(), "http" | "https" | "mailto"), + } + }; + judge(&decode_char_refs(url)) +} + +/// Decode the character references a parser would decode in a URL: numeric +/// ones (with or without the `;`, as HTML allows) and the few named ones that +/// spell URL syntax or whitespace. Anything else is left as written. +fn decode_char_refs(s: &str) -> String { + const NAMED: [(&str, char); 10] = [ + ("colon", ':'), + ("Tab", '\t'), + ("NewLine", '\n'), + ("sol", '/'), + ("quest", '?'), + ("num", '#'), + ("period", '.'), + ("amp", '&'), + ("lpar", '('), + ("rpar", ')'), + ]; + let mut out = String::with_capacity(s.len()); + let mut rest = s; + while let Some(at) = rest.find('&') { + out.push_str(&rest[..at]); + let tail = &rest[at + 1..]; + if let Some(num) = tail.strip_prefix('#') { + let (hex, digits) = match num.strip_prefix(['x', 'X']) { + Some(h) => (true, h), + None => (false, num), + }; + let n = digits + .find(|c: char| { + !(if hex { + c.is_ascii_hexdigit() + } else { + c.is_ascii_digit() + }) + }) + .unwrap_or(digits.len()); + if n > 0 { + let ch = u32::from_str_radix(&digits[..n], if hex { 16 } else { 10 }) + .ok() + .and_then(char::from_u32) + .unwrap_or('\u{FFFD}'); + out.push(ch); + let mut used = 1 + usize::from(hex) + n; + if tail[used..].starts_with(';') { + used += 1; + } + rest = &tail[used..]; + continue; + } + } else { + let n = tail + .find(|c: char| !c.is_ascii_alphanumeric()) + .unwrap_or(tail.len()); + if tail[n..].starts_with(';') + && let Some((_, ch)) = NAMED.iter().find(|(name, _)| *name == &tail[..n]) + { + out.push(*ch); + rest = &tail[n + 1..]; + continue; + } + } + out.push('&'); + rest = tail; } + out.push_str(rest); + out +} + +/// `url` percent-encoded where Markdown would read it as syntax — the link's +/// own end, a nested image or link, a tag, an escape, a code span — for +/// writing as an inline link's destination. +fn escape_destination(url: &str) -> String { + let mut out = String::with_capacity(url.len()); + for c in url.chars() { + match c { + ' ' | '(' | ')' | '<' | '>' | '[' | ']' | '\\' | '`' | '"' | '\'' => { + out.push_str(&format!("%{:02X}", c as u32)); + } + c if c.is_whitespace() || c.is_control() => { + let mut buf = [0u8; 4]; + for b in c.encode_utf8(&mut buf).bytes() { + out.push_str(&format!("%{b:02X}")); + } + } + c => out.push(c), + } + } + out +} + +/// [`escape_destination`]'s lighter cousin for a destination copied from the +/// source as written: only what could open markup of its own is encoded, so +/// a URL with balanced parentheses (`…/Foo_(bar)`) still reads the same. +fn neutralize_markup(url: &str) -> String { + let mut out = String::with_capacity(url.len()); + for c in url.chars() { + match c { + '<' | '>' | '[' | ']' => out.push_str(&format!("%{:02X}", c as u32)), + c => out.push(c), + } + } + out +} + +/// Whether a fragment of raw HTML, as the Markdown parser hands it over, is +/// one [`sanitize`] could have produced: no `` (every one is rewritten +/// into Markdown, and the HTML parser reads `` as ``), and every +/// `href`/`src` a target [`is_safe_target`] allows. +/// +/// For checking what the parser actually saw, after the fact — the rewrite is +/// line-based, and a construct it reads differently from the parser (a code +/// span that is really inside a tag, a fence the parser does not accept) +/// would otherwise slip through. +pub fn html_is_safe(html: &str) -> bool { + let lower = html.to_ascii_lowercase(); + if lower.contains(" value[1..].split(q).next().unwrap_or(""), + _ => value + .split(|c: char| c.is_whitespace() || c == '>') + .next() + .unwrap_or(""), + }; + if !is_safe_target(value) { + return false; + } + } + } + true } #[cfg(test)] @@ -569,6 +742,83 @@ mod tests { assert!(is_safe_target("/abs/path")); } + #[test] + fn a_host_is_what_a_url_parser_would_call_the_host() { + for no in [ + "https://evil.io?.githubusercontent.com/a.png", + "https://evil.io#.githubusercontent.com/a.png", + "https://evil.io\\.githubusercontent.com/a.png", + "https://evil.io/.githubusercontent.com/a.png", + "https://evil.io%2F.githubusercontent.com/a.png", + "https://x.githubusercontent.com:8443/a.png", + ] { + assert!(!is_github_hosted(no), "{no}"); + } + assert!(is_github_hosted( + "https://camo.githubusercontent.com/abc?x=1#y" + )); + } + + #[test] + fn an_image_url_cannot_smuggle_a_second_image() { + // `)` in an attribute value would close the Markdown image early and + // open a new one from the rest of the value. + let out = s(r#""#); + assert!(!out.contains("![](https://evil"), "{out}"); + assert!(out.contains("/a%29!%5B%5D%28https://evil.io"), "{out}"); + // A kept link target cannot hide an image in itself either. + let out = s("[x](https://ok.io/![b](https://evil.io/p.png) \"open title"); + assert!(!out.contains("![b]"), "{out}"); + } + + #[test] + fn the_html_parsers_image_alias_is_an_image_too() { + assert_eq!( + s(r#""#), + "[image](https://x.io/a.png)" + ); + assert_eq!( + s(""), + "[image](https://x.io/a.png)" + ); + } + + #[test] + fn character_references_do_not_disguise_a_scheme() { + for no in [ + "file:///System/Applications/Calculator.app", + "file:///x", + "file:///x", + "file:///x", + "jav ascript:alert(1)", + "file:///x", + "&unknown;:x", + ] { + assert!(!is_safe_target(no), "{no}"); + } + assert!(is_safe_target("https://x.io/?a=1&b=2")); + assert!(is_safe_target("https://x.io/a&b")); + assert_eq!( + s("[x](file:///System/Applications/Calculator.app)"), + "[x](#)" + ); + assert_eq!( + s(r#"x"#), + r##"x"## + ); + } + + #[test] + fn html_left_standing_is_checked_as_the_parser_sees_it() { + assert!(html_is_safe(r#""#)); + assert!(html_is_safe("
x")); + assert!(!html_is_safe(r#""#)); + assert!(!html_is_safe(r#""#)); + assert!(!html_is_safe(r#""#)); + assert!(!html_is_safe(r#""#)); + assert!(!html_is_safe(r#""#)); + } + #[test] fn attachments_are_swapped_for_their_signed_twins() { let id = "352d14e0-d11c-42db-b8b3-042b31e34ce7"; diff --git a/src/ui/github/detail.rs b/src/ui/github/detail.rs index 7038df8e..fa63daed 100644 --- a/src/ui/github/detail.rs +++ b/src/ui/github/detail.rs @@ -507,11 +507,53 @@ fn markdown_block(id: String, source: &str, empty: &str, cx: &gpui::App) -> AnyE .child( gpui_component::text::TextView::markdown(SharedString::from(id), safe) .style(panel_markdown_style(cx)) - .selectable(true), + .selectable(true) + // The rewrite above works on lines of source; this checks what + // the parser actually made of them. A block holding anything + // the rewrite should have removed is drawn as its plain source + // instead, so a construct the two read differently can never + // load a third-party image or hand the OS a `file:` link. + .markdown_block_parser(|node, parse| { + (!renders_safely(node)).then(|| { + let text = parse.node_source(node).unwrap_or_default().to_string(); + gpui_component::text::MarkdownNode::new(UNSAFE_BLOCK, text.clone()) + .text(text) + }) + }) + .markdown_block_renderer(UNSAFE_BLOCK, |node, _window, cx| { + div() + .text_color(cx.theme().muted_foreground) + .whitespace_normal() + .child(node.data::().cloned().unwrap_or_default()) + }), ) .into_any_element() } +/// The custom block a Markdown block that failed [`renders_safely`] becomes. +const UNSAFE_BLOCK: &str = "tty7-github-unsafe-block"; + +/// Whether `node` and everything under it is what +/// `core::github::markdown::sanitize` promises: images only from GitHub's own +/// hosts, links (and link definitions) only to web and mail targets, and raw +/// HTML with no `` or unsafe `href`/`src`. +fn renders_safely(node: &gpui_component::text::markdown_ast::Node) -> bool { + use gpui_component::text::markdown_ast::Node; + use tty7_core::core::github::markdown::{html_is_safe, is_github_hosted, is_safe_target}; + let here = match node { + Node::Image(image) => is_github_hosted(&image.url), + // Never written by the rewrite (it makes every `![…][…]` a link). + Node::ImageReference(_) => false, + Node::Link(link) => is_safe_target(&link.url), + Node::Definition(def) => is_safe_target(&def.url), + Node::Html(html) => html_is_safe(&html.value), + _ => true, + }; + here && node + .children() + .is_none_or(|children| children.iter().all(renders_safely)) +} + #[allow(clippy::too_many_arguments)] fn comment_block( slug: &RepoSlug,