fix(html): filter a document hyperlink through the scheme allowlist

AGENTS.md

@@ -191,11 +191,11 @@ Dispatch `release.yml` against main, publish the draft that appears —
   emit our own html — text through `escape_text`, images as an `<img>` we
   construct. Nothing is passed through as live markup, which is why an svg goes
   out as a data url rather than inlined ([`svg/AGENTS.md`](src/odr/internal/svg/AGENTS.md))
-  and why the rendered page needs no sanitiser. What is *not* consistent today
-  is link targets: `html/pdf_file.cpp` filters a PDF `/URI` action down to an
-  allowlist of navigable schemes, while a document hyperlink
-  (`html/document_element.cpp`) is only `escape_attribute`d, so a `javascript:`
-  href in an odt reaches the page. Pick one policy before adding a third.
+  and why the rendered page needs no sanitiser. A link target goes through one
+  allowlist, `html::is_safe_uri` in `html/common.cpp`, called by both a PDF
+  `/URI` action and `html/document_element.cpp::translate_link`; a refused
+  target loses its `href` and keeps its text. A third writer of an `href` calls
+  it too.
 - **Public API**: value semantics; immutable handles; iterators only for immutable
   traversal (`docs/design/README.md`).
 - **Byte parsing**: read POD structs via `util::byte_stream::read`; assumes host

CHANGELOG.md

@@ -16,6 +16,10 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- A document hyperlink renders without an `href` unless its target is `http`,
+  `https`, `mailto`, `ftp`, `ftps`, `tel` or a relative reference — the
+  allowlist a PDF `/URI` action already went through. `Link::href()` is
+  unchanged.
 - A saved document opens in LibreOffice again: every zip entry's size goes into
   its local header instead of a trailing data descriptor, which LibreOffice
   rejects on a stored entry — an odf package always stores `mimetype`.

src/odr/internal/html/common.cpp

@@ -12,6 +12,7 @@
 
 #include <algorithm>
 #include <array>
+#include <cctype>
 #include <cmath>
 #include <cstdint>
 #include <fstream>
@@ -281,6 +282,32 @@ std::string html::escape_attribute(std::string value) {
   return value;
 }
 
+bool html::is_safe_uri(const std::string_view uri) {
+  std::string scheme;
+  for (const char ch : uri) {
+    const auto c = static_cast<unsigned char>(ch);
+    if (ch == ':') {
+      static constexpr std::array<std::string_view, 6> allowed = {
+          "http", "https", "mailto", "ftp", "ftps", "tel"};
+      return std::ranges::any_of(allowed, [&scheme](const std::string_view s) {
+        return util::string::equals_ignore_case(scheme, s);
+      });
+    }
+    if (ch == '/' || ch == '?' || ch == '#') {
+      return true; // path/query/fragment reached first -> relative reference
+    }
+    if (c <= 0x20) {
+      continue; // browsers strip embedded whitespace/control bytes
+    }
+    if (std::isalnum(c) != 0 || ch == '+' || ch == '-' || ch == '.') {
+      scheme.push_back(ch);
+      continue;
+    }
+    return true; // not a valid scheme character -> relative reference
+  }
+  return true; // no ':' -> relative reference
+}
+
 std::string
 html::fill_path_variables(const std::string &path,
                           const std::optional<std::uint32_t> index) {

src/odr/internal/html/common.hpp

@@ -4,6 +4,7 @@
 #include <iosfwd>
 #include <optional>
 #include <string>
+#include <string_view>
 
 #include <odr/html.hpp>
 #include <odr/internal/abstract/html_service.hpp>
@@ -83,6 +84,11 @@ std::string escape_text(std::string text);
 /// `<`, `>`). Unlike `escape_text`, it leaves leading/trailing spaces intact.
 std::string escape_attribute(std::string value);
 
+/// Whether a target is safe to emit as an `href`: the navigable schemes plus
+/// scheme-less (relative) references. Embedded whitespace and control bytes are
+/// skipped while reading the scheme, as browsers strip them before dispatch.
+[[nodiscard]] bool is_safe_uri(std::string_view uri);
+
 std::string color(const Color &color);
 
 /// Substitute `{index}` in an output file name pattern (e.g.

src/odr/internal/html/document_element.cpp

@@ -377,10 +377,16 @@ void html::translate_span(const Element &element, const WritingState &state) {
 
 void html::translate_link(const Element &element, const WritingState &state) {
   const Link link = element.as_link();
+  const std::string href = link.href();
+
+  HtmlAttributesVector attributes;
+  if (is_safe_uri(href)) {
+    attributes.emplace_back("href", escape_attribute(href));
+  }
 
   state.out().write_element_begin(
       "a", HtmlElementOptions().set_inline(true).set_attributes(
-               HtmlAttributesVector{{"href", escape_attribute(link.href())}}));
+               std::move(attributes)));
   translate_children(link.children(), state);
   state.out().write_element_end("a");
 }

src/odr/internal/html/pdf_file.cpp

@@ -181,37 +181,6 @@ LinkResolver build_link_resolver(pdf::DocumentParser &parser,
   return resolver;
 }
 
-/// Whether a `/URI` action target is safe to emit as an `href`: only the
-/// navigable schemes plus scheme-less (relative) references, so `javascript:`
-/// and friends cannot become a clickable link. Embedded whitespace/control
-/// bytes are ignored while reading the scheme, as browsers strip them before
-/// dispatch (`java\tscript:` must not slip through).
-bool is_safe_uri(std::string_view uri) {
-  std::string scheme;
-  for (const char ch : uri) {
-    const auto c = static_cast<unsigned char>(ch);
-    if (ch == ':') {
-      static constexpr std::array<std::string_view, 6> allowed = {
-          "http", "https", "mailto", "ftp", "ftps", "tel"};
-      return std::ranges::any_of(allowed, [&scheme](const std::string_view s) {
-        return util::string::equals_ignore_case(scheme, s);
-      });
-    }
-    if (ch == '/' || ch == '?' || ch == '#') {
-      return true; // path/query/fragment reached first -> relative reference
-    }
-    if (c <= 0x20) {
-      continue; // browsers strip embedded whitespace/control bytes
-    }
-    if (std::isalnum(c) != 0 || ch == '+' || ch == '-' || ch == '.') {
-      scheme.push_back(ch);
-      continue;
-    }
-    return true; // not a valid scheme character -> relative reference
-  }
-  return true; // no ':' -> relative reference
-}
-
 /// A page's `/Link` annotations (ISO 32000-1 12.5.6.5) as positioned overlays.
 /// `to_box` maps PDF user space to the page box (points, y-down).
 std::vector<LinkOut> collect_page_links(const pdf::Page &page,

test/data.cmake

@@ -17,9 +17,9 @@ odr_test_data(
 odr_test_data(
         PATH "reference-output/odr-public"
         URL "https://github.com/opendocument-app/OpenDocument.test.output.git"
-        REVISION "1a21f16c959293f1849430318806c09b84f17dcb")
+        REVISION "55f56ffe133c9140fe88f6352eb634ccd32e4173")
 
 odr_test_data(
         PATH "reference-output/odr-private"
         URL "https://github.com/opendocument-app/OpenDocument.test-private.output.git"
-        REVISION "9d2ebef410331fde4576faa3780fee360a26f604")
+        REVISION "d7274fe8e03b43f3a2857883478a7a01e2d6b7c5")

test/src/html_test.cpp

@@ -575,3 +575,46 @@ TEST(html, an_image_no_browser_decodes_is_not_translated) {
         << file_type_to_string(type);
   }
 }
+
+namespace {
+
+std::string render_markdown(const std::string &markdown) {
+  const DecodedFile file(File::from_memory(markdown), FileType::markdown);
+  std::ostringstream out;
+  html::translate(file, HtmlConfig()).list_views().at(0).write_html(out);
+  return std::move(out).str();
+}
+
+} // namespace
+
+// #737. The entity forms are resolved before the href is stored, so the filter
+// has to run after that resolution.
+TEST(html, a_link_the_page_must_not_navigate_to_loses_its_href) {
+  for (const std::string_view target :
+       {"javascript:alert(document.cookie)", "&#106;avascript:alert(1)",
+        "&#x6a;avascript:alert(1)", "JAVASCRIPT:alert(1)",
+        "data:text/html,<script>alert(1)</script>", "vbscript:msgbox(1)",
+        "file:///etc/passwd"}) {
+    const std::string page =
+        render_markdown("[click me](" + std::string(target) + ")\n");
+
+    EXPECT_NE(page.find("<a><x-s>click me</x-s></a>"), std::string::npos)
+        << target;
+    EXPECT_EQ(page.find("alert"), std::string::npos) << target;
+    EXPECT_EQ(page.find("msgbox"), std::string::npos) << target;
+    EXPECT_EQ(page.find("passwd"), std::string::npos) << target;
+  }
+}
+
+TEST(html, a_link_that_is_navigable_keeps_its_href) {
+  EXPECT_NE(
+      render_markdown("[a](https://x.example/?u=1&amp;v=2)\n")
+          .find(R"(<a href="https://x.example/?u=1&amp;v=2"><x-s>a</x-s></a>)"),
+      std::string::npos);
+  EXPECT_NE(render_markdown("[a](mailto:someone@x.example)\n")
+                .find(R"(<a href="mailto:someone@x.example"><x-s>a</x-s></a>)"),
+            std::string::npos);
+  EXPECT_NE(render_markdown("[a](#bookmark)\n")
+                .find(R"(<a href="#bookmark"><x-s>a</x-s></a>)"),
+            std::string::npos);
+}

test/src/internal/html/common_test.cpp

@@ -311,3 +311,45 @@ TEST(html_common, a_color_that_does_not_fully_cover_states_its_alpha) {
   EXPECT_EQ(ihtml::color(Color(1, 2, 3, 128)), "rgba(1,2,3,0.501961)");
   EXPECT_EQ(ihtml::color(Color(1, 2, 3, 1)), "rgba(1,2,3,0.00392157)");
 }
+
+TEST(html_common, the_navigable_schemes_are_safe) {
+  EXPECT_TRUE(ihtml::is_safe_uri("http://example.com/a?b#c"));
+  EXPECT_TRUE(ihtml::is_safe_uri("https://example.com"));
+  EXPECT_TRUE(ihtml::is_safe_uri("mailto:someone@example.com"));
+  EXPECT_TRUE(ihtml::is_safe_uri("ftp://example.com"));
+  EXPECT_TRUE(ihtml::is_safe_uri("ftps://example.com"));
+  EXPECT_TRUE(ihtml::is_safe_uri("tel:+123456789"));
+  EXPECT_TRUE(ihtml::is_safe_uri("HTTPS://example.com"));
+}
+
+TEST(html_common, a_reference_without_a_scheme_is_safe) {
+  EXPECT_TRUE(ihtml::is_safe_uri(""));
+  EXPECT_TRUE(ihtml::is_safe_uri("#bookmark"));
+  EXPECT_TRUE(ihtml::is_safe_uri("page1.html"));
+  EXPECT_TRUE(ihtml::is_safe_uri("../other/page1.html#top"));
+  EXPECT_TRUE(ihtml::is_safe_uri("/absolute/path"));
+  EXPECT_TRUE(ihtml::is_safe_uri("?query=1"));
+  // the colon is past a path separator, so it never named a scheme
+  EXPECT_TRUE(ihtml::is_safe_uri("path/to:file"));
+  // `_` is no scheme character, so what precedes the colon is no scheme
+  EXPECT_TRUE(ihtml::is_safe_uri("a_b:c"));
+}
+
+TEST(html_common, a_scheme_that_is_not_navigable_is_refused) {
+  EXPECT_FALSE(ihtml::is_safe_uri("javascript:alert(1)"));
+  EXPECT_FALSE(ihtml::is_safe_uri("JavaScript:alert(1)"));
+  EXPECT_FALSE(ihtml::is_safe_uri("data:text/html,<script>alert(1)</script>"));
+  EXPECT_FALSE(ihtml::is_safe_uri("vbscript:msgbox(1)"));
+  EXPECT_FALSE(ihtml::is_safe_uri("file:///etc/passwd"));
+  EXPECT_FALSE(ihtml::is_safe_uri("blob:https://example.com/uuid"));
+}
+
+TEST(html_common, whitespace_inside_a_scheme_does_not_hide_it) {
+  EXPECT_FALSE(ihtml::is_safe_uri("java\tscript:alert(1)"));
+  EXPECT_FALSE(ihtml::is_safe_uri("java\nscript:alert(1)"));
+  EXPECT_FALSE(ihtml::is_safe_uri("java\rscript:alert(1)"));
+  EXPECT_FALSE(ihtml::is_safe_uri(" javascript:alert(1)"));
+  EXPECT_FALSE(
+      ihtml::is_safe_uri(std::string("javascript") + '\0' + ":alert(1)"));
+  EXPECT_FALSE(ihtml::is_safe_uri("\x01javascript:alert(1)"));
+}