diff --git a/AGENTS.md b/AGENTS.md
index 9d5fd89bd..3b17c9608 100644
--- a/AGENTS.md
+++ b/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 `
` 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
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c116171b0..2a2ef1b6e 100644
--- a/CHANGELOG.md
+++ b/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`.
diff --git a/src/odr/internal/html/common.cpp b/src/odr/internal/html/common.cpp
index 56f5503e4..87a25f14d 100644
--- a/src/odr/internal/html/common.cpp
+++ b/src/odr/internal/html/common.cpp
@@ -12,6 +12,7 @@
#include
#include
+#include
#include
#include
#include
@@ -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(ch);
+ if (ch == ':') {
+ static constexpr std::array 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 index) {
diff --git a/src/odr/internal/html/common.hpp b/src/odr/internal/html/common.hpp
index 6eab71fe5..9045dc827 100644
--- a/src/odr/internal/html/common.hpp
+++ b/src/odr/internal/html/common.hpp
@@ -4,6 +4,7 @@
#include
#include
#include
+#include
#include
#include
@@ -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.
diff --git a/src/odr/internal/html/document_element.cpp b/src/odr/internal/html/document_element.cpp
index 510ed5dfd..fc6603424 100644
--- a/src/odr/internal/html/document_element.cpp
+++ b/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");
}
diff --git a/src/odr/internal/html/pdf_file.cpp b/src/odr/internal/html/pdf_file.cpp
index 0ce4af955..45aaca1b5 100644
--- a/src/odr/internal/html/pdf_file.cpp
+++ b/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(ch);
- if (ch == ':') {
- static constexpr std::array 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 collect_page_links(const pdf::Page &page,
diff --git a/test/data.cmake b/test/data.cmake
index 3a8a84bc8..88aa6af8c 100644
--- a/test/data.cmake
+++ b/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")
diff --git a/test/src/html_test.cpp b/test/src/html_test.cpp
index 2f936b372..5cfba7852 100644
--- a/test/src/html_test.cpp
+++ b/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)", "javascript:alert(1)",
+ "javascript:alert(1)", "JAVASCRIPT:alert(1)",
+ "data:text/html,", "vbscript:msgbox(1)",
+ "file:///etc/passwd"}) {
+ const std::string page =
+ render_markdown("[click me](" + std::string(target) + ")\n");
+
+ EXPECT_NE(page.find("click me"), 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&v=2)\n")
+ .find(R"(a)"),
+ std::string::npos);
+ EXPECT_NE(render_markdown("[a](mailto:someone@x.example)\n")
+ .find(R"(a)"),
+ std::string::npos);
+ EXPECT_NE(render_markdown("[a](#bookmark)\n")
+ .find(R"(a)"),
+ std::string::npos);
+}
diff --git a/test/src/internal/html/common_test.cpp b/test/src/internal/html/common_test.cpp
index ca128728a..dd267fcda 100644
--- a/test/src/internal/html/common_test.cpp
+++ b/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,"));
+ 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)"));
+}