From 9775f0d1551917526ab9765965c7354f62d25c58 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sat, 29 Aug 2026 10:26:47 +0200 Subject: [PATCH] fix(html): drop the blanket link target, and mark only external links Every view was written with ``, so every link was a new tab. A browser has one; an embedded web view does not, and unless the host implements window opening the tap does nothing at all. It showed worst on the archive listing, where the links are the whole point: a zip rendered as a table of its entries and none of them could be opened. The base threw away a distinction the renderer already has. `pdf_file.cpp` was paying to undo it, writing `target="_self"` on every `#pN` anchor so an internal link would scroll instead of opening a copy of the page. `uri_kind` - the scan `is_safe_uri` already did - now says relative, external or refused, and the target follows: a link that leaves the page carries `target="_blank" rel="noopener noreferrer"`, which is what a browser makes a tab of and what an embedded web view raises its window delegate for, and a link back into what serves the page carries nothing and navigates in place. `` goes, and with it the pdf's `_self` workaround and `HtmlWriter::write_header_target`. Reference output moves in 1525 files, in the base tag and link targets alone - nothing else differs. Fixes #730 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Re7MMYiM7fL58uELzKGy77 --- AGENTS.md | 9 ++-- CHANGELOG.md | 5 +++ src/odr/internal/html/common.cpp | 22 +++++++--- src/odr/internal/html/common.hpp | 22 ++++++++-- src/odr/internal/html/document.cpp | 1 - src/odr/internal/html/document_element.cpp | 16 +++++-- src/odr/internal/html/filesystem.cpp | 1 - src/odr/internal/html/font_file.cpp | 1 - src/odr/internal/html/html_writer.cpp | 8 ---- src/odr/internal/html/html_writer.hpp | 1 - src/odr/internal/html/image_file.cpp | 1 - src/odr/internal/html/media_file.cpp | 1 - src/odr/internal/html/pdf_file.cpp | 12 +++--- src/odr/internal/html/text_file.cpp | 1 - src/odr/internal/html/xml_file.cpp | 1 - test/data.cmake | 4 +- test/src/html_test.cpp | 50 ++++++++++++++++++---- test/src/internal/pdf/pdf_file.cpp | 27 ++++++------ 18 files changed, 119 insertions(+), 64 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3b17c9608..5e20c133f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -192,10 +192,11 @@ Dispatch `release.yml` against main, publish the draft that appears — 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. 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. + classifier, `html::uri_kind` 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, an external one gets + `target="_blank"`, a relative one no target. No view declares a document-wide + ``. 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 968937c03..278d194a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,11 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- No view declares `` any more. A link back into what + serves the page — an archive entry, a PDF `#pN` anchor, a relative hyperlink — + navigates in place; only a link that leaves the page carries + `target="_blank" rel="noopener noreferrer"`. + - 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 diff --git a/src/odr/internal/html/common.cpp b/src/odr/internal/html/common.cpp index 87a25f14d..a6dd17119 100644 --- a/src/odr/internal/html/common.cpp +++ b/src/odr/internal/html/common.cpp @@ -282,19 +282,21 @@ std::string html::escape_attribute(std::string value) { return value; } -bool html::is_safe_uri(const std::string_view uri) { +html::UriKind html::uri_kind(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); - }); + const bool navigable = + std::ranges::any_of(allowed, [&scheme](const std::string_view s) { + return util::string::equals_ignore_case(scheme, s); + }); + return navigable ? UriKind::external : UriKind::refused; } if (ch == '/' || ch == '?' || ch == '#') { - return true; // path/query/fragment reached first -> relative reference + return UriKind::relative; // a path/query/fragment came first } if (c <= 0x20) { continue; // browsers strip embedded whitespace/control bytes @@ -303,9 +305,15 @@ bool html::is_safe_uri(const std::string_view uri) { scheme.push_back(ch); continue; } - return true; // not a valid scheme character -> relative reference + return UriKind::relative; // not a scheme character } - return true; // no ':' -> relative reference + return UriKind::relative; // no ':' +} + +std::string_view html::link_target_attributes(const UriKind kind) { + return kind == UriKind::external + ? R"(target="_blank" rel="noopener noreferrer")" + : std::string_view(); } std::string diff --git a/src/odr/internal/html/common.hpp b/src/odr/internal/html/common.hpp index 9045dc827..8b72be1a6 100644 --- a/src/odr/internal/html/common.hpp +++ b/src/odr/internal/html/common.hpp @@ -84,10 +84,24 @@ 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); +/// What a target is, as an `href` would be dispatched. Whitespace and control +/// bytes are skipped while reading the scheme, as browsers strip them first. +enum class UriKind { + relative, ///< no scheme + external, ///< a navigable scheme + refused, ///< `javascript:` and kin +}; + +[[nodiscard]] UriKind uri_kind(std::string_view uri); + +/// Safe to emit as an `href`. +[[nodiscard]] inline bool is_safe_uri(const std::string_view uri) { + return uri_kind(uri) != UriKind::refused; +} + +/// The `` attributes for @p kind, unprefixed; empty but for +/// @ref UriKind::external. +[[nodiscard]] std::string_view link_target_attributes(UriKind kind); std::string color(const Color &color); diff --git a/src/odr/internal/html/document.cpp b/src/odr/internal/html/document.cpp index 2aab9b041..88a34ec66 100644 --- a/src/odr/internal/html/document.cpp +++ b/src/odr/internal/html/document.cpp @@ -131,7 +131,6 @@ void front(const Document &document, const WritingState &state, out.write_begin(); out.write_header_begin(); out.write_header_charset("UTF-8"); - out.write_header_target("_blank"); out.write_header_title( document.document_type() == DocumentType::spreadsheet && !name.empty() ? escape_text(name) diff --git a/src/odr/internal/html/document_element.cpp b/src/odr/internal/html/document_element.cpp index 8908a8df2..a7614980a 100644 --- a/src/odr/internal/html/document_element.cpp +++ b/src/odr/internal/html/document_element.cpp @@ -408,15 +408,23 @@ 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(); + const UriKind kind = uri_kind(href); + // A refused target loses the attribute, not the element. HtmlAttributesVector attributes; - if (is_safe_uri(href)) { + if (kind != UriKind::refused) { attributes.emplace_back("href", escape_attribute(href)); } - state.out().write_element_begin( - "a", HtmlElementOptions().set_inline(true).set_attributes( - std::move(attributes))); + HtmlElementOptions options = + HtmlElementOptions().set_inline(true).set_attributes( + std::move(attributes)); + if (const std::string_view target = link_target_attributes(kind); + !target.empty()) { + options.set_extra(std::string(target)); + } + + state.out().write_element_begin("a", options); translate_children(link.children(), state); state.out().write_element_end("a"); } diff --git a/src/odr/internal/html/filesystem.cpp b/src/odr/internal/html/filesystem.cpp index 913bef0fd..bc0bb172f 100644 --- a/src/odr/internal/html/filesystem.cpp +++ b/src/odr/internal/html/filesystem.cpp @@ -185,7 +185,6 @@ class HtmlServiceImpl final : public HtmlService { out.write_header_begin(); out.write_header_charset("UTF-8"); - out.write_header_target("_blank"); out.write_header_title("odr"); write_viewport_meta(out, config(), false); write_zoom_style(out, config(), WidthFit::none, {}); diff --git a/src/odr/internal/html/font_file.cpp b/src/odr/internal/html/font_file.cpp index c2f92c68b..6a488351f 100644 --- a/src/odr/internal/html/font_file.cpp +++ b/src/odr/internal/html/font_file.cpp @@ -74,7 +74,6 @@ class HtmlServiceImpl final : public HtmlService { out.write_begin(); out.write_header_begin(); out.write_header_charset("UTF-8"); - out.write_header_target("_blank"); out.write_header_title("odr"); write_viewport_meta(out, config(), false); write_content_margin_style(out, config()); diff --git a/src/odr/internal/html/html_writer.cpp b/src/odr/internal/html/html_writer.cpp index 0ef852a0b..4fe2f0549 100644 --- a/src/odr/internal/html/html_writer.cpp +++ b/src/odr/internal/html/html_writer.cpp @@ -176,14 +176,6 @@ void HtmlWriter::write_header_viewport(const std::string &viewport) { write_header_meta("viewport", viewport); } -void HtmlWriter::write_header_target(const std::string &target) { - write_new_line(); - - out() << ""; -} - void HtmlWriter::write_header_charset(const std::string &charset) { write_new_line(); diff --git a/src/odr/internal/html/html_writer.hpp b/src/odr/internal/html/html_writer.hpp index 88068c0fc..08fc275e5 100644 --- a/src/odr/internal/html/html_writer.hpp +++ b/src/odr/internal/html/html_writer.hpp @@ -60,7 +60,6 @@ class HtmlWriter { void write_header_title(const std::string &title); void write_header_meta(const std::string &name, const std::string &content); void write_header_viewport(const std::string &viewport); - void write_header_target(const std::string &target); void write_header_charset(const std::string &charset); /// @p media, when given, gates the stylesheet on that media query. void write_header_style(const std::string &href, std::string_view media = {}); diff --git a/src/odr/internal/html/image_file.cpp b/src/odr/internal/html/image_file.cpp index 1d558364a..94df1dab6 100644 --- a/src/odr/internal/html/image_file.cpp +++ b/src/odr/internal/html/image_file.cpp @@ -115,7 +115,6 @@ class HtmlServiceImpl final : public HtmlService { out.write_begin(); out.write_header_begin(); out.write_header_charset("UTF-8"); - out.write_header_target("_blank"); out.write_header_title("odr"); write_viewport_meta(out, config(), true); // An image has no layout width to preserve, so css alone fits it, framed diff --git a/src/odr/internal/html/media_file.cpp b/src/odr/internal/html/media_file.cpp index f83dab673..08a56b943 100644 --- a/src/odr/internal/html/media_file.cpp +++ b/src/odr/internal/html/media_file.cpp @@ -166,7 +166,6 @@ class HtmlServiceImpl final : public HtmlService { out.write_begin(); out.write_header_begin(); out.write_header_charset("UTF-8"); - out.write_header_target("_blank"); out.write_header_title("odr"); write_viewport_meta(out, config(), false); write_media_style(state); diff --git a/src/odr/internal/html/pdf_file.cpp b/src/odr/internal/html/pdf_file.cpp index 45aaca1b5..1c275fb0d 100644 --- a/src/odr/internal/html/pdf_file.cpp +++ b/src/odr/internal/html/pdf_file.cpp @@ -257,11 +257,12 @@ std::vector collect_page_links(const pdf::Page &page, void write_page_links(HtmlWriter &out, const std::vector &links) { for (const LinkOut &link : links) { std::ostringstream a; - // Internal `#pN` links must override the document's `` or they open a new copy instead of scrolling. - a << ""; out.write_raw(std::move(a).str()); @@ -2672,7 +2673,6 @@ class HtmlServiceImpl final : public HtmlService { out.write_begin(); out.write_header_begin(); out.write_header_charset("UTF-8"); - out.write_header_target("_blank"); out.write_header_title("odr"); write_viewport_meta(out, config(), true); write_zoom_style(out, config(), width_fit(config(), true), content); diff --git a/src/odr/internal/html/text_file.cpp b/src/odr/internal/html/text_file.cpp index 38df832c8..4b82390d9 100644 --- a/src/odr/internal/html/text_file.cpp +++ b/src/odr/internal/html/text_file.cpp @@ -96,7 +96,6 @@ class HtmlServiceImpl final : public HtmlService { out.write_header_begin(); out.write_header_charset(charset); - out.write_header_target("_blank"); out.write_header_title("odr"); write_viewport_meta(out, config(), false); write_zoom_style(out, config(), WidthFit::none, {}); diff --git a/src/odr/internal/html/xml_file.cpp b/src/odr/internal/html/xml_file.cpp index bcd52cb1d..bb89ed8e0 100644 --- a/src/odr/internal/html/xml_file.cpp +++ b/src/odr/internal/html/xml_file.cpp @@ -256,7 +256,6 @@ class HtmlServiceImpl final : public HtmlService { out.write_header_begin(); out.write_header_charset("UTF-8"); - out.write_header_target("_blank"); out.write_header_title("odr"); write_viewport_meta(out, config(), false); write_zoom_style(out, config(), WidthFit::none, {}); diff --git a/test/data.cmake b/test/data.cmake index 88aa6af8c..1905450a1 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 "55f56ffe133c9140fe88f6352eb634ccd32e4173") + REVISION "54fe51a0e28d95287fa3ca5d1112825706511067") odr_test_data( PATH "reference-output/odr-private" URL "https://github.com/opendocument-app/OpenDocument.test-private.output.git" - REVISION "d7274fe8e03b43f3a2857883478a7a01e2d6b7c5") + REVISION "420e0669f520aba75be4eb966f306ef53829dccd") diff --git a/test/src/html_test.cpp b/test/src/html_test.cpp index 121b128f1..7e63a3fd5 100644 --- a/test/src/html_test.cpp +++ b/test/src/html_test.cpp @@ -628,17 +628,51 @@ TEST(html, a_link_the_page_must_not_navigate_to_loses_its_href) { } } +// #730 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)"), + const std::string away = R"( target="_blank" rel="noopener noreferrer")"; + + EXPECT_NE(render_markdown("[a](https://x.example/?u=1&v=2)\n") + .find(R"(a"), std::string::npos); - EXPECT_NE(render_markdown("[a](#bookmark)\n") - .find(R"(a)"), + EXPECT_NE(render_markdown("[a](mailto:someone@x.example)\n") + .find(R"(a"), std::string::npos); + + for (const std::string_view target : + {"#bookmark", "other.html", "a/b.html"}) { + const std::string page = + render_markdown("[a](" + std::string(target) + ")\n"); + EXPECT_NE(page.find(R"(a)"), + std::string::npos) + << target; + EXPECT_EQ(page.find("_blank"), std::string::npos) << target; + } +} + +// #730 +TEST(html, no_view_declares_a_document_wide_link_target) { + const auto render = [](const DecodedFile &file) { + std::ostringstream out; + html::translate(file, HtmlConfig()).list_views().at(0).write_html(out); + return std::move(out).str(); + }; + + const std::array views{ + render(DecodedFile(File::from_memory("a,b\n1,2\n"), + FileType::comma_separated_values)), + render(DecodedFile(File::from_memory("c"), FileType::xml)), + render(DecodedFile(File::from_memory("plain text"), FileType::text_file)), + render(DecodedFile(File::from_memory("[a](https://x.example)\n"), + FileType::markdown)), + }; + + for (const std::string &view : views) { + EXPECT_EQ(view.find("` overlays: a `/URI` action → external href -// (with `&` attr-escaped), a direct `/Dest` and a named `/GoTo` → internal -// `#pN` anchors that carry `target="_self"` (overriding ``); each page div carries a matching `id`. An active-scheme -// `/URI` is dropped. +// `/Link` annotations render as `` overlays: a `/URI` action → an external +// href (`&` attr-escaped, opening away from the page), a direct `/Dest` and a +// named `/GoTo` → internal `#pN` anchors with a matching page div `id`. An +// active-scheme `/URI` is dropped. TEST(PdfFile, link_annotations_render_as_anchors) { const std::string pdf = link_annotations_mini_pdf(); for (const PdfTextMode mode : @@ -209,11 +208,13 @@ TEST(PdfFile, link_annotations_render_as_anchors) { << "mode " << static_cast(mode); EXPECT_TRUE(contains(html, R"(id="p3")")) << "mode " << static_cast(mode); - EXPECT_TRUE(contains(html, R"(href="http://example.com/?a=1&b=2")")) + EXPECT_TRUE(contains( + html, + R"(href="http://example.com/?a=1&b=2" target="_blank" rel="noopener noreferrer")")) << "mode " << static_cast(mode); - EXPECT_TRUE(contains(html, R"(href="#p2" target="_self")")) + EXPECT_TRUE(contains(html, R"(href="#p2" style=)")) << "mode " << static_cast(mode); - EXPECT_TRUE(contains(html, R"(href="#p3" target="_self")")) + EXPECT_TRUE(contains(html, R"(href="#p3" style=)")) << "mode " << static_cast(mode); // The `javascript:` action is not emitted as a link. EXPECT_FALSE(contains(html, "javascript:alert")) @@ -267,8 +268,8 @@ TEST(PdfFile, page_views_link_between_page_files) { /*path=*/"page0.html"); EXPECT_TRUE(contains(html, R"(id="p1")")); EXPECT_FALSE(contains(html, R"(id="p2")")); - EXPECT_TRUE(contains(html, R"(href="page1.html" target="_self")")); - EXPECT_TRUE(contains(html, R"(href="page2.html" target="_self")")); + EXPECT_TRUE(contains(html, R"(href="page1.html" style=)")); + EXPECT_TRUE(contains(html, R"(href="page2.html" style=)")); const std::string page3 = render_html(pdf, PdfTextMode::dual_layer, /*path=*/"page2.html"); @@ -303,7 +304,7 @@ TEST(PdfFile, page_views_nested_output_pattern_links_relatively) { const HtmlService service = make_service(pdf, config); EXPECT_EQ(service.list_views().at(1).path(), "pages/page0.html"); const std::string html = render_path(service, "pages/page0.html"); - EXPECT_TRUE(contains(html, R"(href="page1.html" target="_self")")); + EXPECT_TRUE(contains(html, R"(href="page1.html" style=)")); EXPECT_FALSE(contains(html, R"(href="pages/page1.html")")); } @@ -313,8 +314,8 @@ TEST(PdfFile, page_views_nested_output_pattern_links_relatively) { config.page_output_file_name = "p{index}/index.html"; const HtmlService service = make_service(pdf, config); const std::string html = render_path(service, "p0/index.html"); - EXPECT_TRUE(contains(html, R"(href="../p1/index.html" target="_self")")); - EXPECT_TRUE(contains(html, R"(href="../p2/index.html" target="_self")")); + EXPECT_TRUE(contains(html, R"(href="../p1/index.html" style=)")); + EXPECT_TRUE(contains(html, R"(href="../p2/index.html" style=)")); } }