diff --git a/crates/cos/src/document.rs b/crates/cos/src/document.rs index 38979ee839..db8da24ed3 100644 --- a/crates/cos/src/document.rs +++ b/crates/cos/src/document.rs @@ -563,7 +563,7 @@ impl Document { /// encrypted differently fails with `NeedsPassword`. pub fn open_with_same_credentials(&self, data: Arc>) -> Result { let credentials = match &self.security { - Some(handler) => Credentials::Handler { handler, id0: self.first_id() }, + Some(handler) => Credentials::Handler { handler, id0: self.first_id().to_vec() }, None => Credentials::Password(None), }; Self::open_as(data, credentials, self.stream_limit) @@ -671,15 +671,16 @@ impl Document { return Ok(()); }; let params = crate::security::encrypt_dict(self, &dict); - let id0 = self.first_id(); + let id0 = self.first_id().to_vec(); let handler = match credentials { Credentials::Password(password) => Arc::new(pdfcraft_crypt::SecurityHandler::open(params, &id0, *password).map_err(|e| match e { pdfcraft_crypt::CryptError::WrongPassword if password.is_none() => CosError::NeedsPassword, pdfcraft_crypt::CryptError::WrongPassword => CosError::WrongPassword, other => CosError::Security(other.to_string()), })?), - // The same encryption as the document whose key this is: that key opens it. - Credentials::Handler { handler, id0: known } if *handler.dict() == params && *known == id0 => Arc::clone(handler), + // The same encryption as the document whose key this is: that key opens it. Below + // revision 5 the first /ID is part of the key; AES-256 keys don't depend on it. + Credentials::Handler { handler, id0: known } if *handler.dict() == params && (params.r >= 5 || *known == id0) => Arc::clone(handler), Credentials::Handler { .. } => return Err(CosError::NeedsPassword), }; self.security = Some(handler); @@ -690,11 +691,12 @@ impl Document { Ok(()) } - /// The first file identifier, part of the encryption key (empty when there is none). - fn first_id(&self) -> Vec { + /// The first file identifier, part of the encryption key below revision 5 (empty when there + /// is none, as key derivation reads a missing one). + fn first_id(&self) -> &[u8] { match self.trailer.get(b"ID") { - Some(Object::Array(a)) => a.first().and_then(|s| s.as_string()).map(|s| s.bytes.clone()).unwrap_or_default(), - _ => Vec::new(), + Some(Object::Array(a)) => a.first().and_then(|s| s.as_string()).map_or(&[], |s| s.bytes.as_slice()), + _ => &[], } } @@ -707,7 +709,7 @@ impl Document { if current.auth() == pdfcraft_crypt::Auth::Owner { return Ok(()); } - let owner = current.authenticate_owner(&self.first_id(), password).map_err(|e| match e { + let owner = current.authenticate_owner(self.first_id(), password).map_err(|e| match e { pdfcraft_crypt::CryptError::WrongPassword => CosError::WrongPassword, other => CosError::Security(other.to_string()), })?; @@ -753,18 +755,18 @@ impl Document { self.output_security().0 } - /// True when an incremental save of `self` can carry `other`'s objects under `self`'s - /// encryption (see [`Document::overlay_changes_from`]). Passwords and authentication level - /// are not an encryption identity: the same passwords can protect a different key, - /// algorithm or permissions. The dictionary's object number also matters, since the writer - /// must leave that object unencrypted. A different file key always changes the dictionary - /// too; comparing it as well costs nothing. + /// 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. pub fn has_same_output_security(&self, other: &Document) -> bool { - match (self.output_security(), other.output_security()) { - ((None, _), (None, _)) => true, - ((Some(a), a_num), (Some(b), b_num)) => { - a_num == b_num && a.dict() == b.dict() && a.file_key() == b.file_key() && self.first_id() == other.first_id() - } + 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()), _ => false, } } @@ -1289,15 +1291,19 @@ impl Document { /// Record `current`'s object graph as unsaved edits of this document. /// /// Used when the working snapshot was restored from an earlier revision and the next - /// incremental save must still append to this file (the last saved revision). Objects - /// that already match are left untouched. Reads go through private readers so neither + /// incremental save must still append to this file (the last saved revision), opened afresh: + /// which objects sit in object streams is read from the file. Objects that already match are + /// left untouched. Reads go through private readers so neither /// document's editing cache is filled. /// - /// Output security must match: the copied trailer must describe the handler and - /// encryption-dictionary exemption this writer uses. Otherwise write `current` in full. - pub fn overlay_changes_from(&mut self, current: &Document) -> Result<(), CosError> { + /// 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 + /// 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 { if !self.has_same_output_security(current) { - return Err(CosError::Security("cannot overlay different output encryption; write the current document in full".into())); + return Ok(false); } let saved_nums = self.object_numbers(); let current_nums = current.object_numbers(); @@ -1321,6 +1327,19 @@ impl Document { } } } + // An unchanged object packed in an object stream that this update replaces or frees would + // be lost: the new section no longer has that stream. Write it as an object of its own. + // (A stream container is never itself in a stream, so one pass is enough.) + let gone = |stream: u32| changed.binary_search(&stream).is_ok() || freed.binary_search(&stream).is_ok(); + let stranded: Vec = current_nums + .iter() + .copied() + .filter(|&num| { + changed.binary_search(&num).is_err() && matches!(self.xref_entry(num), Some(XrefEntry::InStream { stream, .. }) if gone(stream)) + }) + .collect(); + changed.extend(stranded); + changed.sort_unstable(); for num in freed { let generation = self.generation(num); self.free(ObjRef::new(num, generation)); @@ -1332,7 +1351,15 @@ impl Document { self.set(ObjRef::new(num, generation), (*object).clone()); } *self.trailer_mut() = current.trailer().clone(); - Ok(()) + // Both numbers normally changed above (or the dictionary is inline), so nothing is read + // from this file under the other exemption. + let encrypt_num = current.output_security().1; + if self.encryption_changed { + self.out_encrypt_num = encrypt_num; + } else { + self.encrypt_num = encrypt_num; + } + Ok(true) } /// Edit a dictionary (or stream dictionary) object in place. diff --git a/crates/cos/tests/encryption.rs b/crates/cos/tests/encryption.rs index 93409eb603..a3cf5b47b4 100644 --- a/crates/cos/tests/encryption.rs +++ b/crates/cos/tests/encryption.rs @@ -378,10 +378,19 @@ fn removing_an_inline_encrypt_dictionary_leaves_an_unencrypted_file() { // Undo can restore a snapshot whose output security differs from the last saved file (#3276). // Overlaying such a snapshot onto that file must be refused before anything changes. -/// Edit `/Title` in the document information dictionary: an unsaved change to compare. +/// Edit `/Title` in the document information dictionary, adding one if there is none: an unsaved +/// change to compare. fn set_title(doc: &mut Document, title: &str) { - let info = doc.trailer().reference(b"Info").unwrap(); - doc.update_dict(info, |d| d.set(b"Title".to_vec(), Object::String(PdfString { bytes: title.as_bytes().to_vec(), hex: false }))).unwrap(); + let title = Object::String(PdfString { bytes: title.as_bytes().to_vec(), hex: false }); + match doc.trailer().reference(b"Info") { + Some(info) => doc.update_dict(info, |d| d.set(b"Title".to_vec(), title)).unwrap(), + None => { + let mut info = Dict::new(); + info.set(b"Title".to_vec(), title); + let info = doc.add(Object::Dict(info)); + doc.trailer_mut().set(b"Info".to_vec(), Object::Ref(info)); + } + } } /// Protect again with the same passwords and no restrictions; `seed` picks the key and `/ID`. @@ -410,8 +419,18 @@ fn state(doc: &Document) -> (Dict, Vec, Vec, bool, Vec) { (doc.trailer().clone(), doc.object_numbers(), doc.modified_objects(), doc.is_modified(), save) } -/// Every incompatible pairing is refused with `CosError::Security` and leaves the target exactly -/// as it was; the same overlay with matching security does apply. +/// Protect `fixture()` as [`protect`] does, without its `/Info` dictionary. A full save numbers +/// the trailer's `/Root`, `/Info` and `/Encrypt` first, in that order, so the same seed keeps the +/// dictionary, key and `/ID` but `/Encrypt` is written under another number. +fn protect_moved(alg: Algorithm) -> Vec { + let mut doc = Document::open(Arc::new(fixture())).unwrap(); + doc.trailer_mut().remove(b"Info"); + rekey(&mut doc, alg, 9); + write_full(&doc, &SaveOptions::default()).unwrap() +} + +/// Every incompatible pairing returns `false` and leaves the target exactly as it was; the same +/// overlay with matching security does apply. #[test] fn an_overlay_with_different_output_security_is_refused_before_any_change() { let encrypted = Arc::new(protect(Algorithm::Aes256, "user", "owner", -1)); @@ -441,16 +460,20 @@ fn an_overlay_with_different_output_security_is_refused_before_any_change() { let mut other_alg = open_owner(); rekey(&mut other_alg, Algorithm::Rc4_128, 9); cases.push(("same passwords, other algorithm", encrypted_target(), other_alg)); - let mut other_id = open_owner(); + // Below revision 5 the first /ID is part of the key. + let aes128 = Arc::new(protect(Algorithm::Aes128, "user", "owner", -1)); + let mut aes128_target = Document::open_with_password(aes128.clone(), Some("owner")).unwrap(); + set_title(&mut aes128_target, "Saved title"); + let mut other_id = Document::open_with_password(aes128, Some("owner")).unwrap(); with_first_id(&mut other_id, b"another file id!"); set_title(&mut other_id, "Restored title"); - cases.push(("different first /ID", encrypted_target(), other_id)); + cases.push(("AES-128, different first /ID", aes128_target, other_id)); for (case, mut target, current) in cases { assert!(!target.has_same_output_security(¤t) && !current.has_same_output_security(&target), "{case}"); let before = state(&target); let result = target.overlay_changes_from(¤t); - assert!(matches!(result, Err(CosError::Security(_))), "{case}: {result:?}"); + assert!(matches!(result, Ok(false)), "{case}: {result:?}"); assert!(before == state(&target), "{case}: a refused overlay changed the target"); } @@ -460,11 +483,56 @@ fn an_overlay_with_different_output_security_is_refused_before_any_change() { set_title(&mut current, "Restored title"); assert!(target.has_same_output_security(¤t)); let before = state(&target); - target.overlay_changes_from(¤t).unwrap(); + assert!(target.overlay_changes_from(¤t).unwrap()); assert!(before != state(&target), "a compatible overlay applies the snapshot"); assert_eq!(title(&target).as_deref(), Some("Restored title")); } +/// A full save can move the `/Encrypt` dictionary (renumbered by compaction, or written inline by +/// another tool) without changing the encryption. Undo across it still appends to the saved +/// file, and the update leaves unencrypted the dictionary its trailer names. +#[test] +fn an_overlay_takes_the_encrypt_dictionary_from_the_snapshot() { + let place = |doc: &Document| match doc.trailer().get(b"Encrypt") { + Some(Object::Ref(r)) => Some(r.num), + _ => None, + }; + for alg in ALL { + let files = + [("indirect", protect(alg, "user", "owner", -1)), ("moved", protect_moved(alg)), ("inline", protect_inline(alg, "user", "owner"))]; + let open = |bytes: &Vec| Document::open_with_password(Arc::new(bytes.clone()), Some("owner")).unwrap(); + for (saved_name, saved) in &files { + for (snapshot_name, snapshot) in &files { + if saved_name == snapshot_name { + continue; + } + let case = format!("{alg:?}: {snapshot_name} snapshot over the {saved_name} save"); + let mut base = open(saved); + let mut current = open(snapshot); + assert_ne!(place(&base), place(¤t), "{case}"); + assert!(base.has_same_output_security(¤t), "{case}"); + set_title(&mut current, "Restored title"); + assert!(base.overlay_changes_from(¤t).unwrap(), "{case}"); + let bytes = write_incremental(&base, &SaveOptions::default()).unwrap(); + assert!(bytes.starts_with(saved), "{case}: appended to the saved file"); + assert!(!String::from_utf8_lossy(&bytes).contains("Secret page"), "{case}: plaintext leaked"); + let bytes = Arc::new(bytes); + assert!(matches!(Document::open(bytes.clone()), Err(CosError::NeedsPassword)), "{case}"); + for password in ["user", "owner"] { + let reopened = Document::open_with_password(bytes.clone(), Some(password)).unwrap_or_else(|e| panic!("{case}/{password}: {e:?}")); + assert_eq!(place(&reopened), place(¤t), "{case}"); + assert_eq!(content(&reopened), SECRET, "{case}/{password}"); + assert_eq!(title(&reopened).as_deref(), Some("Restored title"), "{case}/{password}"); + } + // Independent check, as in `every_algorithm_round_trips_and_hides_the_content`. + let hay = hayro_syntax::Pdf::new_with_password(bytes.as_ref().clone(), "user").unwrap_or_else(|e| panic!("{case}: {e:?}")); + let page = hay.pages().first().unwrap().page_stream().map(|s| String::from_utf8_lossy(s).trim().to_string()); + assert_eq!(page.as_deref(), Some(SECRET), "{case}"); + } + } + } +} + /// The user and owner passwords unlock the same file key: either opening may be overlaid on the /// other, and doing so neither elevates nor drops the user's restrictions in the saved file. #[test] @@ -485,7 +553,7 @@ fn output_security_does_not_depend_on_the_authentication_level() { let mut target = Document::open_with_password(bytes.clone(), Some(base)).unwrap(); let mut current = Document::open_with_password(bytes.clone(), Some(password)).unwrap(); set_title(&mut current, "Edited"); - target.overlay_changes_from(¤t).unwrap(); + assert!(target.overlay_changes_from(¤t).unwrap(), "{alg:?}"); let saved = Arc::new(write_incremental(&target, &SaveOptions::default()).unwrap()); assert!(!String::from_utf8_lossy(&saved).contains("Secret page"), "{alg:?}: plaintext leaked"); assert!(matches!(Document::open(saved.clone()), Err(CosError::NeedsPassword)), "{alg:?}"); @@ -500,37 +568,52 @@ fn output_security_does_not_depend_on_the_authentication_level() { } } -/// Compatible output security means the same `/Encrypt` object number, security dictionary, file -/// key and first file identifier. Each differs alone here; unencrypted documents always match. +/// Compatible output security means the same security dictionary and file key, and below +/// revision 5 the same first file identifier. Each differs alone here; where the dictionary is +/// stored is not compared, and unencrypted documents always match. #[test] -fn output_security_compares_number_dictionary_key_and_first_id() { +fn output_security_compares_dictionary_key_and_first_id() { let plain = || Document::open(Arc::new(fixture())).unwrap(); assert!(plain().has_same_output_security(&plain()), "two unencrypted documents"); - let encrypted = Arc::new(protect(Algorithm::Aes256, "user", "owner", -1)); - let open = || Document::open_with_password(encrypted.clone(), Some("owner")).unwrap(); - assert!(open().has_same_output_security(&open()), "the same file opened twice"); - assert!(!plain().has_same_output_security(&open()) && !open().has_same_output_security(&plain())); - - // Only the first /ID differs: same dictionary, key and object number. - let mut other_id = open(); - with_first_id(&mut other_id, b"another file id!"); - assert!(!open().has_same_output_security(&other_id), "first /ID"); - // The second /ID changes on every save and is not part of the key. - let Some(Object::Array(ids)) = open().trailer().get(b"ID").cloned() else { panic!("no /ID") }; - let mut second = open(); - second.trailer_mut().set(b"ID".to_vec(), Object::Array(vec![ids[0].clone(), Object::String(PdfString { bytes: b"x".to_vec(), hex: true })])); - assert!(open().has_same_output_security(&second), "second /ID"); - - // Same passwords, new key (and so a new dictionary). for alg in ALL { - let mut rekeyed = open(); - rekey(&mut rekeyed, alg, 3); - assert!(!open().has_same_output_security(&rekeyed), "{alg:?} rekey"); + let encrypted = Arc::new(protect(alg, "user", "owner", -1)); + let open = || Document::open_with_password(encrypted.clone(), Some("owner")).unwrap(); + assert!(open().has_same_output_security(&open()), "{alg:?}: the same file opened twice"); + assert!(!plain().has_same_output_security(&open()) && !open().has_same_output_security(&plain()), "{alg:?}"); + + // Only the first /ID differs. AES-256 (revision 6) draws its key at random instead. + let mut other_id = open(); + with_first_id(&mut other_id, b"another file id!"); + assert_eq!(open().has_same_output_security(&other_id), alg == Algorithm::Aes256, "{alg:?}: first /ID"); + // The second /ID changes on every save and is never part of the key. + let Some(Object::Array(ids)) = open().trailer().get(b"ID").cloned() else { panic!("no /ID") }; + let mut second = open(); + second.trailer_mut().set(b"ID".to_vec(), Object::Array(vec![ids[0].clone(), Object::String(PdfString { bytes: b"x".to_vec(), hex: true })])); + assert!(open().has_same_output_security(&second), "{alg:?}: second /ID"); + + // Protected again with the same passwords: AES-256 draws a new key, and another algorithm + // is another dictionary. (Below revision 5 the same algorithm derives the same key from + // the passwords and /ID, so whether it matches is not what this checks.) + for other in ALL.into_iter().filter(|&other| other != alg || alg == Algorithm::Aes256) { + let mut rekeyed = open(); + rekey(&mut rekeyed, other, 3); + assert!(!open().has_same_output_security(&rekeyed), "{alg:?} protected again as {other:?}"); + } + + // Only where the dictionary is stored differs: renumbered, or inline. + let moved = Document::open_with_password(Arc::new(protect_moved(alg)), Some("owner")).unwrap(); + let inline = Document::open_with_password(Arc::new(protect_inline(alg, "user", "owner")), Some("owner")).unwrap(); + assert_ne!(open().trailer().get(b"Encrypt"), moved.trailer().get(b"Encrypt"), "{alg:?}"); + assert!(matches!(inline.trailer().get(b"Encrypt"), Some(Object::Dict(_))), "{alg:?}"); + for other in [&moved, &inline] { + assert_eq!(open().security().unwrap().dict(), other.security().unwrap().dict(), "{alg:?}"); + assert!(open().has_same_output_security(other) && other.has_same_output_security(&open()), "{alg:?}: dictionary place"); + } } // Only the dictionary differs: AES-256 draws its file key from the seed, so new permissions - // or metadata encryption keep the key, /ID and object number. + // or metadata encryption keep the key and /ID. for (permissions, encrypt_metadata) in [(!(4 | 16), true), (-1, false)] { let mut a = plain(); rekey(&mut a, Algorithm::Aes256, 5); @@ -547,45 +630,6 @@ fn output_security_compares_number_dictionary_key_and_first_id() { let (sa, sb) = (a.output_handler().unwrap(), b.output_handler().unwrap()); assert_eq!(sa.file_key(), sb.file_key(), "{permissions}, {encrypt_metadata}"); assert_ne!(sa.dict(), sb.dict(), "{permissions}, {encrypt_metadata}"); - assert_eq!(a.trailer().reference(b"Encrypt"), b.trailer().reference(b"Encrypt")); assert!(!a.has_same_output_security(&b), "{permissions}, {encrypt_metadata}: security dictionary"); } - - // Only the /Encrypt object number differs: the same seed gives the same /ID, dictionary and - // key, but an extra object moves the new dictionary to the next number. - for alg in ALL { - let mut a = plain(); - rekey(&mut a, alg, 5); - let mut b = plain(); - b.add(Object::Int(0)); - rekey(&mut b, alg, 5); - let (sa, sb) = (a.output_handler().unwrap(), b.output_handler().unwrap()); - assert_eq!(sa.dict(), sb.dict(), "{alg:?}"); - assert_eq!(sa.file_key(), sb.file_key(), "{alg:?}"); - assert_eq!(a.trailer().get(b"ID"), b.trailer().get(b"ID"), "{alg:?}"); - assert_ne!(a.trailer().reference(b"Encrypt"), b.trailer().reference(b"Encrypt"), "{alg:?}"); - assert!(!a.has_same_output_security(&b), "{alg:?}: /Encrypt object number"); - let mut c = plain(); - rekey(&mut c, alg, 5); - assert!(a.has_same_output_security(&c), "{alg:?}: identical setup matches"); - } - - // An inline /Encrypt dictionary has no object number, an indirect one does: the same - // dictionary and key cannot share an overlay, since only one of them is exempt by number. - for alg in ALL { - let inline = || Document::open_with_password(Arc::new(protect_inline(alg, "user", "owner")), Some("owner")).unwrap(); - let indirect = Document::open_with_password(Arc::new(protect(alg, "user", "owner", -1)), Some("owner")).unwrap(); - assert!(matches!(inline().trailer().get(b"Encrypt"), Some(Object::Dict(_))), "{alg:?}"); - assert!(indirect.trailer().reference(b"Encrypt").is_some(), "{alg:?}"); - let (si, sd) = (inline(), &indirect); - assert_eq!(si.security().unwrap().dict(), sd.security().unwrap().dict(), "{alg:?}"); - assert_eq!(si.security().unwrap().file_key(), sd.security().unwrap().file_key(), "{alg:?}"); - assert!(!si.has_same_output_security(sd) && !sd.has_same_output_security(&si), "{alg:?}: inline vs indirect"); - assert!(inline().has_same_output_security(&inline()), "{alg:?}: inline opened twice"); - let mut target = inline(); - set_title(&mut target, "Saved title"); - let before = state(&target); - assert!(matches!(target.overlay_changes_from(&indirect), Err(CosError::Security(_))), "{alg:?}"); - assert!(before == state(&target), "{alg:?}: a refused overlay changed the target"); - } } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 591473dd49..720acd6a58 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -4096,9 +4096,10 @@ 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. That snapshot is written in full too: 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). + /// 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. 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()) }; @@ -4111,25 +4112,25 @@ impl Session { // 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. let rewrite = editor.cos.full_save_required() || editor.cos.encryption_changed() || editor.cos.revisions().is_empty(); - guard(|| { + let signed = doc.is_signed(); + let written = guard(|| { if on_saved || rewrite { - write_incremental(&editor.cos, &opts) + return write_incremental(&editor.cos, &opts).map(Some); + } + 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(&editor.cos)? { + write_incremental(&base, &opts).map(Some) + } else if signed { + Ok(None) } else { - 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. Never combine that - // trailer with the saved file's encryption, or retain its obsolete bytes. A full - // write renumbers objects, so older encrypted snapshots usually land here again. - if !base.has_same_output_security(&editor.cos) { - return write_full(&editor.cos, &opts); - } - base.overlay_changes_from(&editor.cos)?; - write_incremental(&base, &opts) + write_full(&editor.cos, &opts).map(Some) } }) .map_err(EditError::Write)? - .map(Arc::new) - .map_err(|e| EditError::Write(e.to_string())) + .map_err(|e| EditError::Write(e.to_string()))?; + written.map(Arc::new).ok_or_else(|| EditError::SignedRewrite("Saving after undoing a security change".into())) } /// 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 ca0c8c01cb..efee56c434 100644 --- a/crates/engine/src/tests/security_save.rs +++ b/crates/engine/src/tests/security_save.rs @@ -161,8 +161,8 @@ fn undo_saved_rekey_with_the_same_passwords_restores_original_security() { assert!(failures.is_empty(), "{}", failures.join("\n")); } -/// A full save renumbers `/Encrypt`; undoing past it must not leave the restored dictionary -/// encrypted as an ordinary object. +/// A full save renumbers `/Encrypt` but keeps the encryption. Undo past it appends to that save, +/// keeping its revision, and leaves the restored dictionary unencrypted. #[test] fn undo_encrypted_compaction_restores_the_encrypt_dictionary_exemption() { let mut cos = CosDocument::open(Arc::new(fixture(3))).unwrap(); @@ -184,10 +184,11 @@ fn undo_encrypted_compaction_restores_the_encrypt_dictionary_exemption() { let compact_doc = CosDocument::open_with_password(compact.clone(), Some("owner")).unwrap(); assert_eq!(original.security().unwrap().dict(), compact_doc.security().unwrap().dict()); assert_ne!(original.trailer().reference(b"Encrypt"), compact_doc.trailer().reference(b"Encrypt"), "compaction renumbered /Encrypt"); - s.mark_saved(id, compact, None).unwrap(); + s.mark_saved(id, compact.clone(), None).unwrap(); s.undo(id).unwrap(); let restored = s.save_bytes(id).unwrap(); s.check_save(id, &restored).unwrap(); + assert!(restored.starts_with(compact.as_slice()), "the same encryption still appends to the last save"); assert!(!restored.windows(b"(Page 1)".len()).any(|w| w == b"(Page 1)"), "page content stays encrypted"); assert!(CosDocument::open(restored.clone()).is_err() && CosDocument::open_with_password(restored.clone(), Some("wrong")).is_err()); let mut reopened = Session::new(); @@ -262,3 +263,86 @@ fn undo_saved_password_change_restores_original_passwords() { assert!(CosDocument::open_with_password(restored.clone(), Some("new-user")).is_err()); 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. +#[test] +fn undo_past_a_security_change_never_rewrites_a_signed_document() { + 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(); + 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(_)))); +} + +/// An optimized save packs objects into object streams under new numbers. Undo past it restores +/// a graph that puts other objects at those numbers: objects that look unchanged but sat in such +/// a stream must still be readable from the next save. +#[test] +fn undo_past_an_optimized_save_keeps_the_objects_it_packed() { + let mut encrypted = CosDocument::open(Arc::new(fixture(3))).unwrap(); + encrypted + .set_encryption(&NewEncryption { + algorithm: Algorithm::Aes256, + user_password: "user", + owner_password: "owner", + permissions: -1, + encrypt_metadata: true, + seed: [7; 32], + }) + .unwrap(); + let encrypted = Arc::new(write_full(&encrypted, &SaveOptions::default()).unwrap()); + for (bytes, password) in [(Arc::new(fixture(3)), None), (encrypted, Some("owner"))] { + let mut s = Session::new(); + let id = s.open("a.pdf", None, bytes, password).unwrap(); + s.apply(id, Edit::DeletePages { pages: vec![2] }).unwrap(); + let compact = s.save_full_bytes(id).unwrap(); + let packed = CosDocument::open_with_password(compact.clone(), password).unwrap(); + s.mark_saved(id, compact, None).unwrap(); + s.undo(id).unwrap(); + // The premise: an object the snapshot keeps unchanged sits in a stream whose number the + // snapshot reuses for something else. + let current = &s.get(id).unwrap().editor.as_ref().unwrap().cos; + let numbers = current.object_numbers(); + let at_risk: Vec = numbers + .iter() + .copied() + .filter(|&n| matches!(packed.xref_entry(n), Some(pdfcraft_cos::XrefEntry::InStream { stream, .. }) if numbers.contains(&stream))) + .filter(|&n| { + let r = pdfcraft_cos::ObjRef::new(n, current.generation(n)); + *packed.get(r) == *current.get(r) + }) + .collect(); + // Only the encrypted file sets this up; the plain one checks there is no regression. + if password.is_some() { + assert!(!at_risk.is_empty(), "no unchanged packed object in a reused stream"); + } + let restored = s.save_bytes(id).unwrap(); + s.check_save(id, &restored).unwrap(); + let saved = CosDocument::open_with_password(restored.clone(), password).unwrap(); + assert!(saved.repair_log().is_empty(), "{password:?}: {:?}", saved.repair_log()); + let current = &s.get(id).unwrap().editor.as_ref().unwrap().cos; + for num in current.object_numbers() { + let r = pdfcraft_cos::ObjRef::new(num, current.generation(num)); + assert_eq!(*saved.get(r), *current.get(r), "{password:?}: object {num}"); + } + let mut reopened = Session::new(); + let rid = reopened.open("restored.pdf", None, restored, password).unwrap(); + assert_eq!(page_texts(&reopened, rid), ["Page 1", "Page 2", "Page 3"], "{password:?}"); + } +}