diff --git a/crates/cos/src/document.rs b/crates/cos/src/document.rs index 1ad2ebe74..761845de5 100644 --- a/crates/cos/src/document.rs +++ b/crates/cos/src/document.rs @@ -802,17 +802,17 @@ impl Document { } /// True when `self` and `other` are written with the same encryption: the same security - /// dictionary and file key, and below revision 5, where the first file identifier is part of - /// the key, the same identifier. Passwords and authentication level are not an encryption - /// identity: the same passwords can protect a different key, algorithm or permissions, and - /// user and owner openings share one. Where the dictionary is stored (its object number, or - /// inline) is not compared: [`Document::overlay_changes_from`] takes that from the document - /// it copies. A different file key always changes the dictionary too; comparing it as well - /// costs nothing. + /// dictionary, and below revision 5, where the first file identifier is part of the key, the + /// same identifier. Together they determine the file key: revisions 2–4 derive it from the + /// password, `/O`, `/P` and `/ID`, and revisions 5–6 wrap it in `/UE` and `/OE`. Passwords + /// and authentication level are not an encryption identity: the same passwords can protect + /// a different key, algorithm or permissions, and user and owner openings share one. Where + /// the dictionary is stored (its object number, or inline) is not compared: + /// [`Document::overlay_changes_from`] takes that from the document it copies. pub fn has_same_output_security(&self, other: &Document) -> bool { match (self.output_handler(), other.output_handler()) { (None, None) => true, - (Some(a), Some(b)) => a.dict() == b.dict() && a.file_key() == b.file_key() && (a.dict().r >= 5 || self.first_id() == other.first_id()), + (Some(a), Some(b)) => a.dict() == b.dict() && (a.dict().r >= 5 || self.first_id() == other.first_id()), _ => false, } } @@ -1356,7 +1356,7 @@ impl Document { /// /// Returns `false`, changing nothing, when output security differs /// ([`Document::has_same_output_security`]): an update appended to this file would be read - /// under other security than the objects before it, so `current` must be written in full. Otherwise the `/Encrypt` dictionary + /// under other security than the objects before it, so append `current` to its own source. Otherwise the `/Encrypt` dictionary /// comes with `current`'s objects and trailer, and so does the number the writer leaves /// unencrypted, even where a full save had moved the dictionary. pub fn overlay_changes_from(&mut self, current: &Document) -> Result { diff --git a/crates/cos/tests/it/encryption.rs b/crates/cos/tests/it/encryption.rs index 9fde3ad82..d58e5cae4 100644 --- a/crates/cos/tests/it/encryption.rs +++ b/crates/cos/tests/it/encryption.rs @@ -513,7 +513,8 @@ fn set_title(doc: &mut Document, title: &str) { } } -/// Protect again with the same passwords and no restrictions; `seed` picks the key and `/ID`. +/// Protect again with the same passwords and no restrictions. `seed` picks the AES-256 key, and +/// the `/ID` when the document has none. fn rekey(doc: &mut Document, alg: Algorithm, seed: u8) { doc.set_encryption(&NewEncryption { algorithm: alg, diff --git a/crates/engine/src/session.rs b/crates/engine/src/session.rs index 0ab86612b..c130e47f6 100644 --- a/crates/engine/src/session.rs +++ b/crates/engine/src/session.rs @@ -665,10 +665,11 @@ impl Session { /// break its signatures, so with no edits it keeps its bytes, damage and all. /// /// Undo past a save that changed security restores a snapshot whose encryption differs from - /// the last saved file. Like any change of security, that snapshot is written in full: an - /// update appended to the saved file would be read with the wrong key, and would keep the - /// revision it was meant to replace (#3276). In a signed document that fails with - /// [`EditError::SignedRewrite`] rather than invalidate the signatures. + /// the last saved file. An update appended to that file would be read with the wrong key and + /// keep the revision it was meant to replace, so the snapshot is appended to its own source + /// instead, whose encryption it shares (#3276). That keeps the source's revisions, object + /// numbers and signatures. Revisions saved after that source are dropped: undo went back + /// past all of them. An unedited snapshot is its source's bytes as they are. pub fn save_bytes(&self, id: DocId) -> Result>, EditError> { let doc = self.get(id).ok_or(EditError::NoDocument)?; let Some(editor) = doc.editor.as_ref() else { return Ok(doc.bytes.clone()) }; @@ -679,29 +680,29 @@ impl Session { // What the user removed again before saving (a cleared redaction mark) stays out of the file. let opts = SaveOptions { mod_date: self.now().map(pdfcraft_cos::pdf_date), drop_unlinked: true, ..SaveOptions::default() }; // A restored snapshot can be based on an earlier revision. Append to the last saved - // file so that revision stays in the chain. A full rewrite (redaction, new encryption, - // a reconstructed file) still writes the current graph on its own. + // file when it has the snapshot's security, so that revision stays in the chain. A full + // rewrite (redaction, new encryption, a reconstructed file) still writes the current + // graph on its own. let rewrite = editor.cos.full_save_required() || editor.cos.encryption_changed() || editor.cos.revisions().is_empty(); let cos = writable(&editor.cos)?; - let signed = doc.is_signed(); - let written = guard(|| { + guard(|| { if on_saved || rewrite { - return write_incremental(&cos, &opts).map(Some); + return write_incremental(&cos, &opts); } let mut base = pdfcraft_cos::Document::open_with_password(editor.saved.clone(), editor.saved_keys.reopen.as_deref())?; - // Undo may restore security from before the last save even though it has not changed - // relative to the snapshot's own source. That can't be appended to the saved file. if base.overlay_changes_from(&cos)? { - write_incremental(&base, &opts).map(Some) - } else if signed { - Ok(None) + write_incremental(&base, &opts) + } else if !cos.is_modified() { + Ok(cos.bytes().to_vec()) } else { - write_full(&cos, &opts).map(Some) + // Undo restored security from before the last save: append the snapshot to its + // own source, which it hasn't changed security against. + write_incremental(&cos, &opts) } }) .map_err(EditError::Write)? - .map_err(|e| EditError::Write(e.to_string()))?; - written.map(Arc::new).ok_or_else(|| EditError::SignedRewrite("Saving after undoing a security change".into())) + .map(Arc::new) + .map_err(|e| EditError::Write(e.to_string())) } /// A compact, garbage-collected rewrite (Save As ▸ "Optimized" / Reduce File Size groundwork). diff --git a/crates/engine/src/tests/security_save.rs b/crates/engine/src/tests/security_save.rs index 41d2e71e6..6d217280e 100644 --- a/crates/engine/src/tests/security_save.rs +++ b/crates/engine/src/tests/security_save.rs @@ -69,7 +69,8 @@ fn undo_saved_security_removal_restores_encrypted_content() { for inline in [false, true] { for password in ["owner", "user"] { let mut s = Session::new(); - let id = s.open("encrypted.pdf", None, encrypted_fixture(algorithm, inline), Some(password)).unwrap(); + let original = encrypted_fixture(algorithm, inline); + let id = s.open("encrypted.pdf", None, original.clone(), Some(password)).unwrap(); s.apply(id, Edit::RemoveProtection).unwrap(); let plain = s.save_bytes(id).unwrap(); assert_saved_content(&s, id, plain.clone(), None, false).unwrap(); @@ -81,6 +82,7 @@ fn undo_saved_security_removal_restores_encrypted_content() { failures.push(format!("{algorithm:?}, inline={inline}, {password}: {error}")); continue; } + assert!(restored.starts_with(original.as_slice()), "appended to the encrypted original, not rewritten"); // Both original passwords still work; saving also retains this session's rights. assert_saved_content(&s, id, restored.clone(), Some("user"), true).unwrap(); assert_saved_content(&s, id, restored.clone(), Some("owner"), true).unwrap(); @@ -103,6 +105,7 @@ fn undo_saved_protection_restores_plain_content() { let mut failures = Vec::new(); for algorithm in [Algorithm::Rc4_128, Algorithm::Aes128, Algorithm::Aes256] { let (mut s, id) = session_with(2); + let original = s.get(id).unwrap().bytes.clone(); s.apply(id, Edit::Protect(Protection { algorithm, ..protection(Some("user"), Some("owner")) })).unwrap(); let protected = s.save_bytes(id).unwrap(); assert_saved_content(&s, id, protected.clone(), Some("owner"), true).unwrap(); @@ -114,6 +117,7 @@ fn undo_saved_protection_restores_plain_content() { failures.push(format!("{algorithm:?}: {error}")); continue; } + assert!(restored.starts_with(original.as_slice()), "{algorithm:?}: appended to the plain original"); s.mark_saved(id, restored.clone(), None).unwrap(); assert!(!s.get(id).unwrap().dirty); assert_eq!(*s.save_bytes(id).unwrap(), *restored); @@ -264,30 +268,59 @@ fn undo_saved_password_change_restores_original_passwords() { assert!(CosDocument::open_with_password(restored, Some("new-owner")).is_err()); } -/// A signed document is never rewritten in full, even by this fallback. Signing clears the undo -/// history and security can't be changed once signed, so no edit reaches this state; the test -/// marks the document signed directly. +/// Undo past a security save back to a signed state, then save: the signature stays valid. +/// Deleting a signed field unsigns the document, so security can change before the undo. #[test] -fn undo_past_a_security_change_never_rewrites_a_signed_document() { +fn undo_past_a_security_save_keeps_a_restored_signature_valid() { let p12 = std::fs::read(concat!(env!("CARGO_MANIFEST_DIR"), "/../sign/tests/data/ec-p256.p12")).unwrap(); let digital_id = sign::pkcs12::open(&p12, "test").unwrap(); // After the test certificate's start date (2026-10-02). - let mut signer = Session::new().with_clock(|| 1_800_000_000); - let signed_id = signer.open("signed.pdf", None, Arc::new(fixture(1)), None).unwrap(); - let signed = - signer.sign(signed_id, &digital_id, SignOptions { page: 0, rect: Some([20.0, 20.0, 180.0, 60.0]), ..SignOptions::default() }).unwrap(); - signer.mark_signed(signed_id, signed, None).unwrap(); - let signatures = signer.get(signed_id).unwrap().signatures.clone(); - - let mut s = Session::new(); - let id = s.open("encrypted.pdf", None, encrypted_fixture(Algorithm::Aes256, false), Some("owner")).unwrap(); - s.apply(id, Edit::RemoveProtection).unwrap(); - let plain = s.save_bytes(id).unwrap(); - s.mark_saved(id, plain, None).unwrap(); + let mut s = Session::new().with_clock(|| 1_800_000_000); + s.set_trusted_certificates(vec![digital_id.certificate.clone()]); + let id = s.open("fixture.pdf", None, Arc::new(fixture(1)), None).unwrap(); + let signed = s.sign(id, &digital_id, SignOptions { page: 0, rect: Some([20.0, 20.0, 180.0, 60.0]), ..SignOptions::default() }).unwrap(); + s.mark_signed(id, signed.clone(), None).unwrap(); + let field = s.get(id).unwrap().signatures[0].field.clone(); + s.apply(id, Edit::DeleteField { name: field }).unwrap(); + assert!(!s.get(id).unwrap().is_signed()); + s.apply(id, Edit::Protect(protection(Some("user"), Some("owner")))).unwrap(); + let protected = s.save_bytes(id).unwrap(); + s.mark_saved(id, protected, None).unwrap(); s.undo(id).unwrap(); - s.doc_mut(id).unwrap().signatures = signatures; - assert!(s.get(id).unwrap().is_signed()); - assert!(matches!(s.save_bytes(id), Err(EditError::SignedRewrite(_)))); + s.undo(id).unwrap(); + assert!(s.get(id).unwrap().is_signed(), "undo restores the signed snapshot"); + let restored = s.save_bytes(id).unwrap(); + assert_eq!(*restored, *signed, "the signed file as it was, with no revision after the signature"); + let mut u = Session::new().with_clock(|| 1_800_000_000); + u.set_trusted_certificates(vec![digital_id.certificate.clone()]); + let uid = u.open("restored.pdf", None, restored, None).unwrap(); + assert_eq!(u.get(uid).unwrap().signatures[0].status, SignatureStatus::Valid); +} + +/// The restored state can have unsaved edits of its own: they are appended to its source, in +/// that source's security, and the undone revision is still left out. +#[test] +fn undo_past_a_security_save_keeps_the_snapshot_edits() { + for encrypted in [false, true] { + let mut s = Session::new(); + let (original, password) = + if encrypted { (encrypted_fixture(Algorithm::Aes256, false), Some("owner")) } else { (Arc::new(fixture(2)), None) }; + let id = s.open("a.pdf", None, original.clone(), password).unwrap(); + s.apply(id, Edit::RotatePages { pages: vec![0], degrees: 90 }).unwrap(); + let change = if encrypted { Edit::RemoveProtection } else { Edit::Protect(protection(Some("user"), Some("owner"))) }; + s.apply(id, change).unwrap(); + let changed = s.save_bytes(id).unwrap(); + s.mark_saved(id, changed, None).unwrap(); + s.undo(id).unwrap(); + let restored = s.save_bytes(id).unwrap(); + assert!(restored.starts_with(original.as_slice()) && restored.len() > original.len(), "{encrypted}: appended to the source"); + let mut reopened = Session::new(); + let rid = reopened.open("restored.pdf", None, restored.clone(), password).unwrap(); + let doc = reopened.get(rid).unwrap(); + assert_eq!((doc.info.encrypted, doc.info.pages[0].rotation), (encrypted, 90), "{encrypted}"); + assert_eq!(page_texts(&reopened, rid), ["Page 1", "Page 2"], "{encrypted}"); + assert_eq!(restored.windows(b"(Page 1)".len()).any(|w| w == b"(Page 1)"), !encrypted, "{encrypted}: plaintext only in a plain file"); + } } /// An optimized save packs objects into object streams under new numbers. Undo past it restores