fix: reserve archive viewer resources before entries

CHANGELOG.md

@@ -16,6 +16,9 @@ The release run heads these entries with the version and opens a fresh
 
 ## Unreleased
 
+- Archive listings reserve shipped script and stylesheet paths before exposing
+  entries, and reject unsafe entry links. Archives cannot replace viewer scripts.
+
 - HTML and SVG rendering validates document length units before writing styles
   or coordinates, preventing malformed units from injecting output markup.
 

src/odr/internal/html/filesystem.cpp

@@ -12,8 +12,10 @@
 #include <odr/internal/html/frontend.hpp>
 #include <odr/internal/html/html_service.hpp>
 #include <odr/internal/html/html_writer.hpp>
+#include <odr/internal/util/string_util.hpp>
 #include <odr/internal/xml/xml_util.hpp>
 
+#include <algorithm>
 #include <array>
 
 #include <fmt/format.h>
@@ -162,10 +164,14 @@ class HtmlServiceImpl final : public HtmlService {
         entry_path->string(), file, false, false, true);
     HtmlResourceLocation location =
         config().resource_locator(resource, config());
-    // Two resources at one location write over each other; the stylesheet is
-    // registered first, so an entry landing on it yields.
+    // Shipped resources reserve their locations before archive entries.
     if (location.has_value() &&
-        resource_at(state.resources(), *location) != nullptr) {
+        (uri_kind(*location) == UriKind::refused ||
+         util::string::equals_ignore_case(*location, listing_path) ||
+         std::ranges::any_of(state.resources(), [&](const auto &entry) {
+           return entry.second &&
+                  util::string::equals_ignore_case(*entry.second, *location);
+         }))) {
       return std::nullopt;
     }
 
@@ -174,7 +180,7 @@ class HtmlServiceImpl final : public HtmlService {
   }
 
   HtmlResources write_filesystem(HtmlWriter &out) const {
-    HtmlResources resources;
+    HtmlResources resources = locate_filesystem_resources(config());
     const WritingState state(out, config(), resources, logger());
 
     const FileWalker file_walker = m_filesystem.file_walker("/");
@@ -223,10 +229,13 @@ class HtmlServiceImpl final : public HtmlService {
           HtmlElementOptions().set_inline(true).set_class("odr-files-name"));
       if (location.has_value()) {
         out.write_element_begin(
-            "a",
-            HtmlElementOptions().set_inline(true).set_attributes(
-                HtmlAttributesVector{{"href", xml::escape_attribute(*location)},
-                                     {"title", xml::escape_attribute(name)}}));
+            "a", HtmlElementOptions()
+                     .set_inline(true)
+                     .set_extra(std::string(
+                         link_target_attributes(uri_kind(*location))))
+                     .set_attributes(HtmlAttributesVector{
+                         {"href", xml::escape_attribute(*location)},
+                         {"title", xml::escape_attribute(name)}}));
         out.write_raw(escape_text(file_path.string()));
         out.write_element_end("a");
       } else {
@@ -251,11 +260,15 @@ class HtmlServiceImpl final : public HtmlService {
                   : entry_data_url(file, mime_type_of(file_path));
           href.has_value()) {
         out.write_element_begin(
-            "a",
-            HtmlElementOptions().set_inline(true).set_attributes(
-                HtmlAttributesVector{{"href", *href},
-                                     {"download", xml::escape_attribute(name)},
-                                     {"title", xml::escape_attribute(name)}}));
+            "a", HtmlElementOptions()
+                     .set_inline(true)
+                     .set_extra(std::string(
+                         location ? link_target_attributes(uri_kind(*location))
+                                  : std::string_view()))
+                     .set_attributes(HtmlAttributesVector{
+                         {"href", *href},
+                         {"download", xml::escape_attribute(name)},
+                         {"title", xml::escape_attribute(name)}}));
         out.write_raw("\u2193");
         out.write_element_end("a");
       }

src/odr/internal/html/frontend.cpp

@@ -135,6 +135,11 @@ constexpr Asset host_bridge_js_asset{HtmlResourceType::js, "text/javascript",
 /// Appends @p asset to @p resources; `nullopt` to embed it.
 HtmlResourceLocation locate(const Asset &asset, const HtmlConfig &config,
                             HtmlResources &resources) {
+  for (const auto &[resource, location] : resources) {
+    if (resource.is_shipped() && resource.name() == asset.name) {
+      return location;
+    }
+  }
   const odr::HtmlResource resource = HtmlResource::create(
       asset.type, std::string(asset.mime_type), std::string(asset.name),
       std::string(asset.name), odr::File::from_memory(written(asset)), true,
@@ -362,6 +367,16 @@ void html::write_host_bridge_script(const WritingState &state) {
   write_script(host_bridge_js_asset, state);
 }
 
+HtmlResources html::locate_filesystem_resources(const HtmlConfig &config) {
+  static constexpr std::array assets{filesystem_css_asset, search_css_asset,
+                                     search_js_asset, viewport_js_asset};
+  static constexpr std::array dark{filesystem_dark_css_asset,
+                                   search_dark_css_asset};
+  HtmlResources resources = locate_all(assets, dark, config);
+  locate_host_bridge(config, resources);
+  return resources;
+}
+
 HtmlResources html::locate_text_resources(const HtmlConfig &config) {
   static constexpr std::array assets{text_css_asset,  search_css_asset,
                                      search_js_asset, editing_js_asset,

src/odr/internal/html/frontend.hpp

@@ -68,6 +68,7 @@ void write_host_bridge_script(const WritingState &state);
 /// What the corresponding `write_*` calls would link, without writing anything:
 /// a service has to answer for these paths as well as for its views. Every
 /// entry is located `nullopt` when the config embeds them.
+HtmlResources locate_filesystem_resources(const HtmlConfig &config);
 HtmlResources locate_text_resources(const HtmlConfig &config);
 HtmlResources locate_xml_resources(const HtmlConfig &config);
 HtmlResources locate_media_resources(const HtmlConfig &config);

src/odr/internal/html/html_writer.cpp

@@ -87,7 +87,7 @@ void write_element_options(std::ostream &out,
   if (options.attributes) {
     write_attributes(out, *options.attributes);
   }
-  if (options.extra) {
+  if (options.extra && !is_empty(*options.extra)) {
     out << " ";
     write_writable(out, *options.extra);
   }

test/src/html_test.cpp

@@ -121,42 +121,70 @@ TEST(html, linked_images_are_served) {
   check("odr-public/xlsx/sample.xlsx");
 }
 
-// The one archive the reference-output suite renders has no directory in it.
-// An archive may hold a file named like the stylesheet. Forcing the collision
-// through the locator saves needing such an archive in the test data.
-TEST(html, archive_entry_yields_to_a_shipped_resource) {
-  const auto logger = Logger::create_stdio("odr-test", LogLevel::verbose);
-
+TEST(html, archive_entry_yields_to_each_shipped_resource) {
   const DecodedFile file =
       open(TestData::test_file_path("odr-public/odt/about.odt"),
-           DecodeOptions::as(FileType::zip), logger);
-
-  HtmlConfig config((std::filesystem::current_path() / "collision").string());
-  config.embed_shipped_resources = false;
-  config.resource_locator = [](const HtmlResource &resource,
-                               const HtmlConfig &) -> HtmlResourceLocation {
-    return resource.is_shipped() ? "content.xml" : resource.path();
-  };
-
-  std::ostringstream out;
-  const HtmlResources resources =
-      html::translate(file, config, logger).list_views().at(0).write_html(out);
-
-  // The locator puts every shipped resource on that one location, so what the
-  // count says is that the entry of that name is not among them.
-  std::size_t shipped = 0;
-  std::size_t claimants = 0;
-  for (const auto &[resource, location] : resources) {
-    if (resource.is_shipped()) {
-      ++shipped;
+           DecodeOptions::as(FileType::zip));
+  for (const std::string asset : {"filesystem.css", "search.js", "viewport.js",
+                                  "host-bridge.js", "SEARCH.JS"}) {
+    HtmlConfig config;
+    config.embed_shipped_resources = false;
+    config.host_message_handler = "host";
+    config.resource_locator =
+        [asset](const HtmlResource &resource,
+                const HtmlConfig &) -> HtmlResourceLocation {
+      return resource.is_shipped() &&
+                     resource.name() ==
+                         (asset == "SEARCH.JS" ? "search.js" : asset)
+                 ? (asset == "SEARCH.JS" ? "CONTENT.XML" : "content.xml")
+                 : resource.path();
+    };
+    const HtmlService service = html::translate(file, config);
+    std::ostringstream out;
+    const HtmlResources resources = service.list_views().at(0).write_html(out);
+    std::size_t claimants = 0;
+    for (const auto &[resource, location] : resources) {
+      if (location == "content.xml" || location == "CONTENT.XML") {
+        ++claimants;
+        EXPECT_TRUE(resource.is_shipped());
+        EXPECT_EQ(resource.name(), asset == "SEARCH.JS" ? "search.js" : asset);
+      }
     }
-    if (location.has_value() && *location == "content.xml") {
-      ++claimants;
-      EXPECT_TRUE(resource.is_shipped());
+    EXPECT_EQ(claimants, 1u);
+    std::ostringstream served;
+    const std::string path =
+        asset == "SEARCH.JS" ? "CONTENT.XML" : "content.xml";
+    service.write(path, served);
+    EXPECT_FALSE(served.str().starts_with("<?xml"));
+    EXPECT_EQ(service.mimetype(path),
+              asset.ends_with(".css") ? "text/css" : "text/javascript");
+  }
+}
+
+TEST(html, archive_entry_links_use_the_uri_policy) {
+  const DecodedFile file =
+      open(TestData::test_file_path("odr-public/odt/about.odt"),
+           DecodeOptions::as(FileType::zip));
+  for (const std::string location :
+       {"javascript:alert(1)", "files.html", "https://example.com/file"}) {
+    HtmlConfig config;
+    config.resource_locator =
+        [location](const HtmlResource &resource,
+                   const HtmlConfig &) -> HtmlResourceLocation {
+      return resource.is_shipped() ? std::nullopt : std::optional(location);
+    };
+    std::ostringstream out;
+    html::translate(file, config).list_views().at(0).write_html(out);
+    const std::string page = out.str();
+    if (location != "https://example.com/file") {
+      EXPECT_EQ(page.find(location), std::string::npos);
+    } else {
+      EXPECT_NE(page.find(R"(href="https://example.com/file")"),
+                std::string::npos);
+      EXPECT_NE(page.find(R"(target="_blank" rel="noopener noreferrer")"),
+                std::string::npos);
     }
   }
-  EXPECT_GT(shipped, 0);
-  EXPECT_EQ(claimants, shipped);
 }
 
 TEST(html, archive_listing) {