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=)")); } }