fix(document): refuse to save a package we only hold decrypted

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.
 

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());
 }
 

src/odr/document.hpp

@@ -22,6 +22,9 @@ class Document final {
   explicit Document(std::shared_ptr<internal::abstract::Document>);
 
   [[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;

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;

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<abstract::ReadableFilesystem> files)
+                   std::shared_ptr<abstract::ReadableFilesystem> 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

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<abstract::ReadableFilesystem> files);
+           std::shared_ptr<abstract::ReadableFilesystem> 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<abstract::ReadableFilesystem> m_files;
 

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

src/odr/internal/odf/odf_document.cpp

@@ -33,8 +33,10 @@ create_element_adapter(const Document &document, ElementRegistry &registry);
 }
 
 Document::Document(const FileType file_type, const DocumentType document_type,
-                   std::shared_ptr<abstract::ReadableFilesystem> files)
-    : internal::Document(file_type, document_type, std::move(files)) {
+                   std::shared_ptr<abstract::ReadableFilesystem> 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,29 +129,13 @@ 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<MemoryFile>(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));
   }
 
   archive.save(out);
 }
 
 void Document::save(std::ostream & /*out*/, const char * /*password*/) const {
-  // TODO throw if not savable
   throw UnsupportedOperation();
 }
 

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<abstract::ReadableFilesystem> files);
+           std::shared_ptr<abstract::ReadableFilesystem> 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);

src/odr/internal/odf/odf_file.cpp

@@ -95,16 +95,18 @@ std::shared_ptr<abstract::Document> OpenDocumentFile::document() const {
   switch (file_type()) {
   case FileType::opendocument_text:
     return std::make_shared<Document>(m_file_meta.type, DocumentType::text,
-                                      m_filesystem);
+                                      m_filesystem, m_encryption_state);
   case FileType::opendocument_presentation:
     return std::make_shared<Document>(m_file_meta.type,
-                                      DocumentType::presentation, m_filesystem);
+                                      DocumentType::presentation, m_filesystem,
+                                      m_encryption_state);
   case FileType::opendocument_spreadsheet:
     return std::make_shared<Document>(m_file_meta.type,
-                                      DocumentType::spreadsheet, m_filesystem);
+                                      DocumentType::spreadsheet, m_filesystem,
+                                      m_encryption_state);
   case FileType::opendocument_graphics:
     return std::make_shared<Document>(m_file_meta.type, DocumentType::drawing,
-                                      m_filesystem);
+                                      m_filesystem, m_encryption_state);
   default:
     throw UnsupportedFileType(file_type());
   }

src/odr/internal/ooxml/ooxml_file.cpp

@@ -105,7 +105,7 @@ std::shared_ptr<abstract::Document> OfficeOpenXmlFile::document() const {
 
   switch (file_type()) {
   case FileType::office_open_xml_document:
-    return std::make_shared<text::Document>(m_files);
+    return std::make_shared<text::Document>(m_files, m_encryption_state);
   case FileType::office_open_xml_presentation:
     return std::make_shared<presentation::Document>(m_files);
   case FileType::office_open_xml_workbook:

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<abstract::ReadableFilesystem> files)
+Document::Document(std::shared_ptr<abstract::ReadableFilesystem> 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;
 

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<abstract::ReadableFilesystem> files);
+  Document(std::shared_ptr<abstract::ReadableFilesystem> files,
+           EncryptionState encryption_state);
 
   ElementRegistry &element_registry();
   StyleRegistry &style_registry();

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<std::string> &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);
 }