From 7fbed172e7a0ef9b79b51401be18ced3b1c0d72a Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Thu, 3 Sep 2026 21:17:11 +0200 Subject: [PATCH] fix(document): refuse to save a package we only hold decrypted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `is_savable(false)` was true for a document decrypted from a password-protected package, and odf's `save` stripped every `manifest:encryption-data` on the way out. Opening a protected file with its password and saving it therefore produced a plaintext package, silently: the content left the protection its author asked for, and nothing said so. `.docx` had the same shape, rebuilding the zip from the decrypted filesystem. A document now carries the `EncryptionState` its file was decoded at, and the two savable engines refuse one that was decrypted. Every public `save` overload consults `is_savable` before it writes — the stream and memory ones did not — so the refusal reaches all of them. The manifest rewrite goes with it: nothing else ever had encryption data to strip. Re-encrypting on save is #64 and unaffected; this only stops the hole in the meantime. Towards #64. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UDi21mwuKRGvGiFcGwETtS --- CHANGELOG.md | 4 ++ src/odr/document.cpp | 17 +++++-- src/odr/document.hpp | 3 ++ src/odr/internal/abstract/document.hpp | 3 +- src/odr/internal/common/document.cpp | 9 +++- src/odr/internal/common/document.hpp | 8 +++- src/odr/internal/odf/AGENTS.md | 9 ++-- src/odr/internal/odf/odf_document.cpp | 28 ++++------- src/odr/internal/odf/odf_document.hpp | 3 +- src/odr/internal/odf/odf_file.cpp | 10 ++-- src/odr/internal/ooxml/ooxml_file.cpp | 2 +- .../ooxml/text/ooxml_text_document.cpp | 11 +++-- .../ooxml/text/ooxml_text_document.hpp | 3 +- test/src/document_test.cpp | 48 +++++++++++++++---- 14 files changed, 107 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f48187aa3..e2def7c47 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 +- **Breaking**: a document decrypted from a password-protected package is no + longer savable — every `save` overload throws `UnsupportedOperation`, where it + used to write the content out unprotected. Towards #64. + - A `.docx` `wp:anchor` drawing floats at its offset with its wrap and side, instead of sitting in the line. Closes #803. diff --git a/src/odr/document.cpp b/src/odr/document.cpp index 6deff47bb..3eca9763c 100644 --- a/src/odr/document.cpp +++ b/src/odr/document.cpp @@ -29,7 +29,8 @@ bool Document::is_savable(const bool encrypted) const noexcept { return m_impl->is_savable(encrypted); } -// Checked here so an unsavable format leaves no empty file behind. +// Every overload checks before it writes, so an unsavable format leaves no +// empty file behind and no half-written stream. void Document::save(const std::string &path) const { if (!m_impl->is_savable(false)) { throw UnsupportedOperation(); @@ -47,21 +48,29 @@ void Document::save(const std::string &path, m_impl->save(out, password.c_str()); } -void Document::save(std::ostream &out) const { m_impl->save(out); } +void Document::save(std::ostream &out) const { + if (!m_impl->is_savable(false)) { + throw UnsupportedOperation(); + } + m_impl->save(out); +} void Document::save(std::ostream &out, const std::string &password) const { + if (!m_impl->is_savable(true)) { + throw UnsupportedOperation(); + } m_impl->save(out, password.c_str()); } File Document::save_to_memory() const { std::ostringstream out; - m_impl->save(out); + save(out); return File::from_memory(std::move(out).str()); } File Document::save_to_memory(const std::string &password) const { std::ostringstream out; - m_impl->save(out, password.c_str()); + save(out, password); return File::from_memory(std::move(out).str()); } diff --git a/src/odr/document.hpp b/src/odr/document.hpp index 8c9895d31..d8a5fa2b6 100644 --- a/src/odr/document.hpp +++ b/src/odr/document.hpp @@ -22,6 +22,9 @@ class Document final { explicit Document(std::shared_ptr); [[nodiscard]] bool is_editable() const noexcept; + /// Savable, @p encrypted to ask for an encrypted save. False for a document + /// decrypted from a password-protected package: saving one can only write it + /// out in the clear. [[nodiscard]] bool is_savable(bool encrypted = false) const noexcept; void save(const std::string &path) const; diff --git a/src/odr/internal/abstract/document.hpp b/src/odr/internal/abstract/document.hpp index 4d406676c..07241beca 100644 --- a/src/odr/internal/abstract/document.hpp +++ b/src/odr/internal/abstract/document.hpp @@ -67,7 +67,8 @@ class Document { /// Editable in any way. [[nodiscard]] virtual bool is_editable() const noexcept = 0; - /// Savable, @p encrypted to ask for an encrypted save. + /// Savable, @p encrypted to ask for an encrypted save. A decrypted package + /// is not: `save` would write it out in the clear. [[nodiscard]] virtual bool is_savable(bool encrypted) const noexcept = 0; virtual void save(std::ostream &out) const = 0; diff --git a/src/odr/internal/common/document.cpp b/src/odr/internal/common/document.cpp index 3ee647c15..b7ee6c672 100644 --- a/src/odr/internal/common/document.cpp +++ b/src/odr/internal/common/document.cpp @@ -7,9 +7,10 @@ namespace odr::internal { Document::Document(const FileType file_type, const DocumentType document_type, - std::shared_ptr files) + std::shared_ptr files, + const EncryptionState encryption_state) : m_file_type{file_type}, m_document_type{document_type}, - m_files{std::move(files)} {} + m_encryption_state{encryption_state}, m_files{std::move(files)} {} Document::~Document() = default; @@ -44,4 +45,8 @@ const abstract::ElementAdapter *Document::element_adapter() const { return m_element_adapter.get(); } +bool Document::is_decrypted() const noexcept { + return m_encryption_state == EncryptionState::decrypted; +} + } // namespace odr::internal diff --git a/src/odr/internal/common/document.hpp b/src/odr/internal/common/document.hpp index 6af8d3ca0..94300d35d 100644 --- a/src/odr/internal/common/document.hpp +++ b/src/odr/internal/common/document.hpp @@ -16,7 +16,8 @@ namespace odr::internal { class Document : public abstract::Document { public: Document(FileType file_type, DocumentType document_type, - std::shared_ptr files); + std::shared_ptr files, + EncryptionState encryption_state = EncryptionState::not_encrypted); ~Document() override; /// Read-only, which every engine but odf and ooxml text is. @@ -36,9 +37,14 @@ class Document : public abstract::Document { [[nodiscard]] const abstract::ElementAdapter * element_adapter() const override; + /// Decoded from a package that was password-encrypted. `save` has no + /// encryption to put back, so a savable engine refuses one. + [[nodiscard]] bool is_decrypted() const noexcept; + protected: FileType m_file_type{FileType::unknown}; DocumentType m_document_type{DocumentType::unknown}; + EncryptionState m_encryption_state{EncryptionState::not_encrypted}; std::shared_ptr m_files; diff --git a/src/odr/internal/odf/AGENTS.md b/src/odr/internal/odf/AGENTS.md index 59371d94b..2078b8e6f 100644 --- a/src/odr/internal/odf/AGENTS.md +++ b/src/odr/internal/odf/AGENTS.md @@ -160,10 +160,11 @@ The structural/foundational gaps, roughly by value: for one text run; that's the whole editor. 2. **Spreadsheet editing is force-disabled** (`is_editable` hardcodes `false` for spreadsheets — `odf_document.cpp`, `// TODO fix spreadsheet editability`). -3. **Save never re-encrypts.** Plain `save` strips all `manifest:encryption-data` - and emits an unencrypted package; `save(path, password)` throws - `UnsupportedOperation`. `save` also doesn't yet guard `is_savable`. Only - `content.xml` is re-serialised, so hypothetical style edits wouldn't persist. +3. **Save never re-encrypts**, and refuses rather than dropping the encryption: + a document decrypted from a password-protected package reports + `is_savable(false) == false` and every `save` overload throws + `UnsupportedOperation`. Only `content.xml` is re-serialised, so hypothetical + style edits wouldn't persist. 4. **No streaming.** Crypto and save read whole files into memory and round-trip the entire filesystem through a rebuilt ZIP (`// TODO stream` throughout). 5. **Repeated / covered spreadsheet cells are heuristic.** Several diff --git a/src/odr/internal/odf/odf_document.cpp b/src/odr/internal/odf/odf_document.cpp index 0c157dd8b..b2a1b0ebb 100644 --- a/src/odr/internal/odf/odf_document.cpp +++ b/src/odr/internal/odf/odf_document.cpp @@ -33,8 +33,10 @@ create_element_adapter(const Document &document, ElementRegistry ®istry); } Document::Document(const FileType file_type, const DocumentType document_type, - std::shared_ptr files) - : internal::Document(file_type, document_type, std::move(files)) { + std::shared_ptr files, + const EncryptionState encryption_state) + : internal::Document(file_type, document_type, std::move(files), + encryption_state) { m_content_xml = xml::parse(*m_files, AbsPath("/content.xml")); if (m_files->exists(AbsPath("/styles.xml"))) { @@ -84,10 +86,14 @@ bool Document::is_editable() const noexcept { } bool Document::is_savable(const bool encrypted) const noexcept { - return !encrypted; + return !encrypted && !is_decrypted(); } void Document::save(std::ostream &out) const { + if (!is_savable(false)) { + throw UnsupportedOperation(); + } + // no package to rebuild: a flat document is the one tree, and `save` puts // back the declaration the parse dropped if (m_files == nullptr) { @@ -123,21 +129,6 @@ void Document::save(std::ostream &out) const { archive.insert_file(std::end(archive), rel_path, tmp); continue; } - if (abs_path == Path("/META-INF/manifest.xml")) { - // TODO - auto manifest = xml::parse(*m_files, AbsPath("/META-INF/manifest.xml")); - - for (auto &&node : manifest.select_nodes("//manifest:encryption-data")) { - node.node().parent().remove_child(node.node()); - } - - std::stringstream content; - manifest.print(content, "", pugi::format_raw); - auto tmp = std::make_shared(content.str()); - archive.insert_file(std::end(archive), rel_path, tmp); - - continue; - } archive.insert_file(std::end(archive), rel_path, m_files->open(abs_path)); } @@ -145,7 +136,6 @@ void Document::save(std::ostream &out) const { } void Document::save(std::ostream & /*out*/, const char * /*password*/) const { - // TODO throw if not savable throw UnsupportedOperation(); } diff --git a/src/odr/internal/odf/odf_document.hpp b/src/odr/internal/odf/odf_document.hpp index cc6dd43f2..e2d5ef2ac 100644 --- a/src/odr/internal/odf/odf_document.hpp +++ b/src/odr/internal/odf/odf_document.hpp @@ -13,7 +13,8 @@ namespace odr::internal::odf { class Document final : public internal::Document { public: Document(FileType file_type, DocumentType document_type, - std::shared_ptr files); + std::shared_ptr files, + EncryptionState encryption_state); /// A flat document: one tree holding both content and styles, no filesystem. Document(FileType file_type, DocumentType document_type, pugi::xml_document flat_xml); diff --git a/src/odr/internal/odf/odf_file.cpp b/src/odr/internal/odf/odf_file.cpp index bc8685fc5..80ce79afc 100644 --- a/src/odr/internal/odf/odf_file.cpp +++ b/src/odr/internal/odf/odf_file.cpp @@ -95,16 +95,18 @@ std::shared_ptr OpenDocumentFile::document() const { switch (file_type()) { case FileType::opendocument_text: return std::make_shared(m_file_meta.type, DocumentType::text, - m_filesystem); + m_filesystem, m_encryption_state); case FileType::opendocument_presentation: return std::make_shared(m_file_meta.type, - DocumentType::presentation, m_filesystem); + DocumentType::presentation, m_filesystem, + m_encryption_state); case FileType::opendocument_spreadsheet: return std::make_shared(m_file_meta.type, - DocumentType::spreadsheet, m_filesystem); + DocumentType::spreadsheet, m_filesystem, + m_encryption_state); case FileType::opendocument_graphics: return std::make_shared(m_file_meta.type, DocumentType::drawing, - m_filesystem); + m_filesystem, m_encryption_state); default: throw UnsupportedFileType(file_type()); } diff --git a/src/odr/internal/ooxml/ooxml_file.cpp b/src/odr/internal/ooxml/ooxml_file.cpp index f7cce5713..6f816ba4b 100644 --- a/src/odr/internal/ooxml/ooxml_file.cpp +++ b/src/odr/internal/ooxml/ooxml_file.cpp @@ -105,7 +105,7 @@ std::shared_ptr OfficeOpenXmlFile::document() const { switch (file_type()) { case FileType::office_open_xml_document: - return std::make_shared(m_files); + return std::make_shared(m_files, m_encryption_state); case FileType::office_open_xml_presentation: return std::make_shared(m_files); case FileType::office_open_xml_workbook: diff --git a/src/odr/internal/ooxml/text/ooxml_text_document.cpp b/src/odr/internal/ooxml/text/ooxml_text_document.cpp index 7e04345ea..4e7b450d8 100644 --- a/src/odr/internal/ooxml/text/ooxml_text_document.cpp +++ b/src/odr/internal/ooxml/text/ooxml_text_document.cpp @@ -61,9 +61,10 @@ PageLayout read_page_layout(const pugi::xml_node body) { } } // namespace -Document::Document(std::shared_ptr files) +Document::Document(std::shared_ptr files, + const EncryptionState encryption_state) : internal::Document(FileType::office_open_xml_document, DocumentType::text, - std::move(files)) { + std::move(files), encryption_state) { m_document_xml = xml::parse(*m_files, AbsPath("/word/document.xml")); m_styles_xml = xml::parse(*m_files, AbsPath("/word/styles.xml")); @@ -112,10 +113,14 @@ const PageLayout &Document::page_layout() const { return m_page_layout; } bool Document::is_editable() const noexcept { return true; } bool Document::is_savable(const bool encrypted) const noexcept { - return !encrypted; + return !encrypted && !is_decrypted(); } void Document::save(std::ostream &out) const { + if (!is_savable(false)) { + throw UnsupportedOperation(); + } + // TODO this would decrypt/inflate and encrypt/deflate again zip::ZipArchive archive; diff --git a/src/odr/internal/ooxml/text/ooxml_text_document.hpp b/src/odr/internal/ooxml/text/ooxml_text_document.hpp index 2435dde6a..2e002d0cb 100644 --- a/src/odr/internal/ooxml/text/ooxml_text_document.hpp +++ b/src/odr/internal/ooxml/text/ooxml_text_document.hpp @@ -17,7 +17,8 @@ namespace odr::internal::ooxml::text { class Document final : public internal::Document { public: - explicit Document(std::shared_ptr files); + Document(std::shared_ptr files, + EncryptionState encryption_state); ElementRegistry &element_registry(); StyleRegistry &style_registry(); diff --git a/test/src/document_test.cpp b/test/src/document_test.cpp index 5b3df990e..e37c46e16 100644 --- a/test/src/document_test.cpp +++ b/test/src/document_test.cpp @@ -68,14 +68,10 @@ void expect_text_at(const Document &document, const std::string &path, /// Applies `diff` to `path`'s document, saves to `output_name` in the working /// directory and reopens it, so the assertions see what was written. Document edit_and_reload(const std::string &path, const char *diff, - const std::string &output_name, - const std::optional &password = {}) { + const std::string &output_name) { const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); - DocumentFile document_file(TestData::test_file_path(path), logger); - if (password.has_value()) { - document_file = document_file.decrypt(*password); - } + const DocumentFile document_file(TestData::test_file_path(path), logger); const Document document = document_file.document(); html::edit(document, diff); @@ -87,6 +83,15 @@ Document edit_and_reload(const std::string &path, const char *diff, return DocumentFile(output_path).document(); } +/// `pages.ods` is password-protected; every test that wants its content opens +/// it this way. +Document decrypted_pages_ods() { + const std::string path = "odr-public/ods/pages.ods"; + return DocumentFile(TestData::test_file_path(path)) + .decrypt(TestData::test_file(path).password.value()) + .document(); +} + } // namespace TEST(Document, odt) { @@ -301,13 +306,14 @@ TEST(Document, edit_odt_diff) { expect_text_at(document, "/child:6/child:0", "Text hello world!"); } +// Asserted in memory: `pages.ods` is password-protected, and a decrypted +// package is not savable — see `a_decrypted_package_is_not_savable`. TEST(Document, edit_ods_diff) { const char *diff = R"({"modifiedText":{"/child:0/cell:A1/child:0/child:0":"Page 1 hi","/child:1/cell:A1/child:0/child:0":"Page 2 hihi","/child:2/cell:A1/child:0/child:0":"Page 3 hihihi","/child:3/cell:A1/child:0/child:0":"Page 4 hihihihi","/child:4/cell:A1/child:0/child:0":"Page 5 hihihihihi"}})"; - const std::string path = "odr-public/ods/pages.ods"; - const Document document = - edit_and_reload(path, diff, "pages_edit_diff.ods", - TestData::test_file(path).password.value()); + const Document document = decrypted_pages_ods(); + + html::edit(document, diff); expect_text_at(document, "/child:0/cell:A1/child:0/child:0", "Page 1 hi"); expect_text_at(document, "/child:1/cell:A1/child:0/child:0", "Page 2 hihi"); @@ -318,6 +324,24 @@ TEST(Document, edit_ods_diff) { "Page 5 hihihihihi"); } +// Saving would rebuild the package from the decrypted filesystem, handing out +// what the password protects. +TEST(Document, a_decrypted_package_is_not_savable) { + const Document document = decrypted_pages_ods(); + + EXPECT_FALSE(document.is_savable()); + EXPECT_FALSE(document.is_savable(true)); + + const std::string path = + (std::filesystem::current_path() / "decrypted_save.ods").string(); + EXPECT_THROW(document.save(path), UnsupportedOperation); + EXPECT_FALSE(std::filesystem::exists(path)); + + std::ostringstream out; + EXPECT_THROW(document.save(out), UnsupportedOperation); + EXPECT_THROW((void)document.save_to_memory(), UnsupportedOperation); +} + TEST(Document, edit_docx_diff) { const char *diff = R"({"modifiedText":{"/child:16/child:0/child:0":"Outasdfsdafdline","/child:24/child:0/child:0":"Colorasdfasdfasdfed Line","/child:6/child:0/child:0":"Text hello world!"}})"; @@ -386,4 +410,8 @@ TEST(Document, saving_an_unsavable_format_leaves_no_file) { (std::filesystem::current_path() / "unsavable_save.pptx").string(); EXPECT_THROW(document.save(path), UnsupportedOperation); EXPECT_FALSE(std::filesystem::exists(path)); + + std::ostringstream out; + EXPECT_THROW(document.save(out), UnsupportedOperation); + EXPECT_THROW((void)document.save_to_memory(), UnsupportedOperation); }