From 1bef70219f75d10d64c2e9d369dfbca74980628b Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 23 Aug 2026 20:25:39 +0200 Subject: [PATCH] fix(open): honour the requested type for every document container `open(file, as)` returned whatever an odf, ooxml or legacy MS container turned out to hold, whatever `as` said, while the flat-odf branch added alongside them checked. So `open(packaged_odt, opendocument_graphics)` answered with a text document and `open(flat_odt, opendocument_graphics)` threw - the public contract depended on which encoding of the same document the caller happened to hold. `open_file_as` documents itself as decoding "as exactly @p as", and the probing path already treats a throw as "not this type, try the next", so the check costs it nothing. An encrypted ooxml is the one reading that is not its own container's: it names no inner type until it is decrypted, so it still answers for the type that was asked for - which is what `html_output_test` expects of every encrypted docx and xlsx in the reference set. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_015E5WrxdYxfoBE7JUotBtqh --- CHANGELOG.md | 6 ++++ src/odr/internal/open_strategy.cpp | 33 +++++++++++++++--- test/src/file_test.cpp | 56 ++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43b05b5ff..ea3327cfb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,12 @@ The release run heads these entries with the version and opens a fresh ## Unreleased +- `open(file, as)` honours `as` for a packaged OpenDocument, an ooxml and a + legacy MS document the way it already did for a flat OpenDocument: a + container that turns out to hold another document type is no longer returned + as the type that was asked for. An encrypted ooxml is unchanged - it names no + inner type until it is decrypted, and still opens as whichever was asked for. + - Flat OpenDocument files decode as documents rather than as an xml source view: `.fodt`, `.fodp`, `.fods` and `.fodg` open, render, edit and save like their packaged counterparts. Their images ride in the markup, and an diff --git a/src/odr/internal/open_strategy.cpp b/src/odr/internal/open_strategy.cpp index 645312afb..2e995af1d 100644 --- a/src/odr/internal/open_strategy.cpp +++ b/src/odr/internal/open_strategy.cpp @@ -47,6 +47,14 @@ template auto priority_comparator(const std::vector &priority) { }; } +/// Whether @p file is the ooxml that was asked for. An encrypted one names no +/// inner type until it is decrypted, so it answers for whichever was asked. +bool is_the_requested_ooxml(const ooxml::OfficeOpenXmlFile &file, + const FileType as) { + return file.file_type() == as || + file.file_type() == FileType::office_open_xml_encrypted; +} + /// Decodes @p file as exactly @p as, or throws the format's "not a ..." /// exception (@ref UnsupportedFileType for a type we cannot decode at all). std::unique_ptr @@ -60,7 +68,11 @@ open_file_as(const std::shared_ptr &file, const FileType as, try { auto zip_file = std::make_unique(file); auto filesystem = zip_file->archive()->as_filesystem(); - return std::make_unique(filesystem); + auto odf_file = std::make_unique(filesystem); + if (odf_file->file_type() == as) { + return odf_file; + } + ODR_VERBOSE(logger, "odf is a different document type"); } catch (...) { ODR_VERBOSE(logger, "failed to open as odf"); } @@ -87,14 +99,22 @@ open_file_as(const std::shared_ptr &file, const FileType as, try { auto zip_file = std::make_unique(file); auto filesystem = zip_file->archive()->as_filesystem(); - return std::make_unique(filesystem); + auto ooxml_file = std::make_unique(filesystem); + if (is_the_requested_ooxml(*ooxml_file, as)) { + return ooxml_file; + } + ODR_VERBOSE(logger, "ooxml zip is a different document type"); } catch (...) { ODR_VERBOSE(logger, "failed to open as ooxml zip"); } try { auto cfb_file = std::make_unique(file); auto filesystem = cfb_file->archive()->as_filesystem(); - return std::make_unique(filesystem); + auto ooxml_file = std::make_unique(filesystem); + if (is_the_requested_ooxml(*ooxml_file, as)) { + return ooxml_file; + } + ODR_VERBOSE(logger, "ooxml cfb is a different document type"); } catch (...) { ODR_VERBOSE(logger, "failed to open as ooxml cfb"); } @@ -108,7 +128,12 @@ open_file_as(const std::shared_ptr &file, const FileType as, try { auto cfb_file = std::make_unique(file); auto filesystem = cfb_file->archive()->as_filesystem(); - return std::make_unique(filesystem); + auto oldms_file = + std::make_unique(filesystem); + if (oldms_file->file_type() == as) { + return oldms_file; + } + ODR_VERBOSE(logger, "legacy ms is a different document type"); } catch (...) { ODR_VERBOSE(logger, "failed to open as legacy ms"); } diff --git a/test/src/file_test.cpp b/test/src/file_test.cpp index e74db7b42..90858a601 100644 --- a/test/src/file_test.cpp +++ b/test/src/file_test.cpp @@ -28,6 +28,62 @@ TEST(File, from_disk_matches_the_path_constructor) { EXPECT_EQ(file.size(), File(path).size()); } +/// `open(file, as)` decodes as exactly what it is asked for. A container +/// names its own document type, and `as` is a claim about what is inside it - +/// so a claim the container contradicts is no reading of the file at all. +TEST(File, opening_as_the_wrong_document_type_throws) { + const struct { + const char *path; + FileType is; + FileType is_not; + } cases[]{ + {"odr-public/odt/about.odt", FileType::opendocument_text, + FileType::opendocument_graphics}, + {"odr-public/docx/file-sample_100kB.docx", + FileType::office_open_xml_document, + FileType::office_open_xml_presentation}, + {"odr-public/doc/file-sample_100kB.doc", FileType::legacy_word_document, + FileType::legacy_excel_worksheets}, + }; + + for (const auto &[path, is, is_not] : cases) { + const std::string file_path = TestData::test_file_path(path); + + EXPECT_EQ(DecodedFile(file_path, is).file_type(), is) << path; + EXPECT_THROW(std::ignore = DecodedFile(file_path, is_not), UnknownFileType) + << path; + } +} + +/// The one reading that is not its own container's: an encrypted ooxml names +/// no inner type until it is decrypted, so it stands in for the one asked for. +TEST(File, an_encrypted_ooxml_opens_as_the_type_asked_for) { + const DecodedFile file( + TestData::test_file_path("odr-public/docx/encrypted.docx"), + FileType::office_open_xml_document); + + EXPECT_EQ(file.file_type(), FileType::office_open_xml_encrypted); + EXPECT_TRUE(file.password_encrypted()); +} + +/// The same claim about the same document in its other encoding answers the +/// same way - which is what this fix is about. +TEST(File, a_flat_document_and_a_package_answer_a_wrong_type_alike) { + const std::string flat = + R"()" + R"()" + R"()"; + + EXPECT_THROW(std::ignore = DecodedFile(File::from_memory(flat), + FileType::opendocument_graphics), + UnknownFileType); + EXPECT_THROW(std::ignore = DecodedFile( + TestData::test_file_path("odr-public/odt/about.odt"), + FileType::opendocument_graphics), + UnknownFileType); +} + TEST(File, from_memory_holds_its_bytes) { const File file = File::from_memory("hello");