(Minor) Additional unit tests and fixes for injection (or previously …

…untested) issues. (#464)

* Tiff remove c2pa was removing all the rest of the asset

* PNG - add size of header when overwriting existing XMP

* Fix logic when adding provenance to existing XMP
handled self closing tags better

* (MINOR) Add String parameter to XMP Errors

* Consistently inject XMP header and use same segment location for existing XMP

* Refactor to add more external manifest unit tests

* Refactor into individual  streams tests and add remote_reference tests where possible.
Note: on some formats c2pa manifests fail to be removed.
On others XMP write is supposed to be implemented but read does not work.
These errors are bypassed for now but should be followed up with issues

* Add a bunch of allow dead_code to get around new WASM clippy  warnings.
Deferring a better for this until a later PR

* A bunch of new clippy warnings fixed.

* clippy unrwap_used inner/outer changes

* more extreme clippy checks for dead code in certain builds

sdk/src/assertion.rs

@@ -236,7 +236,7 @@ impl Assertion {
     }
 
     pub(crate) fn set_content_type(mut self, content_type: &str) -> Self {
-        self.content_type = content_type.to_owned();
+        content_type.clone_into(&mut self.content_type);
         self
     }
 

sdk/src/asset_handlers/jpeg_io.rs

@@ -56,7 +56,7 @@ fn vec_compare(va: &[u8], vb: &[u8]) -> bool {
        .all(|(a,b)| a == b)
 }
 
-// todo decide if want to keep this just for in-memory use cases
+// Return contents of APP1 segment if it is an XMP segment.
 fn extract_xmp(seg: &JpegSegment) -> Option<String> {
     let contents = seg.contents();
     if contents.starts_with(XMP_SIGNATURE) {
@@ -67,6 +67,7 @@ fn extract_xmp(seg: &JpegSegment) -> Option<String> {
     }
 }
 
+// Extract XMP from bytes.
 fn xmp_from_bytes(asset_bytes: &[u8]) -> Option<String> {
     if let Ok(jpeg) = Jpeg::from_bytes(Bytes::copy_from_slice(asset_bytes)) {
         let segs = jpeg.segments_by_marker(markers::APP1);
@@ -145,7 +146,7 @@ fn get_cai_segments(jpeg: &img_parts::jpeg::Jpeg) -> Result<Vec<usize>> {
                 if is_cai {
                     cai_segs.push(i);
                     cai_seg_cnt = 1;
-                    cai_en = en.clone(); // store the identifier
+                    cai_en.clone_from(&en); // store the identifier
                 }
             }
         }
@@ -226,7 +227,7 @@ impl CAIReader for JpegIO {
 
                                     buffer.append(&mut raw_vec.as_mut_slice()[8..].to_vec());
                                     cai_seg_cnt = 1;
-                                    cai_en = en.clone(); // store the identifier
+                                    cai_en.clone_from(&en); // store the identifier
 
                                     manifest_store_cnt += 1;
                                 }
@@ -380,7 +381,7 @@ impl CAIWriter for JpegIO {
                                     let is_cai = vec_compare(&C2PA_MARKER, &jumb_type);
                                     if is_cai {
                                         cai_seg_cnt = 1;
-                                        cai_en = en.clone(); // store the identifier
+                                        cai_en.clone_from(&en); // store the identifier
 
                                         let v = HashObjectPositions {
                                             offset: curr_offset,
@@ -593,28 +594,29 @@ impl RemoteRefEmbed for JpegIO {
                 let mut jpeg =
                     Jpeg::from_bytes(buf.into()).map_err(|_err| Error::EmbeddingError)?;
 
-                // first extract the xmp from APP1 markers
-                let app1_segs = jpeg.segments_by_marker(markers::APP1);
-                let mut xmp: String = app1_segs.filter_map(extract_xmp).collect();
-
-                // remove existing XMP segments
+                // find any existing XMP segment and remember where it was
+                let mut xmp = MIN_XMP.to_string(); // default minimal XMP
+                let mut xmp_index = None;
                 let segments = jpeg.segments_mut();
-                segments.retain(|seg| {
-                    !(seg.marker() == markers::APP1 && seg.contents().starts_with(XMP_SIGNATURE))
-                });
-
-                if xmp.is_empty() {
-                    // Init with minimal xmp segment
-                    // JPEG APP1 segment with XMP are defined with this null terminated string
-                    // todo: add format and other minimal metadata to xmp ?
-                    xmp = format!("http://ns.adobe.com/xap/1.0/\0 {}", MIN_XMP);
-                };
-                let xmp = add_provenance(&xmp, &manifest_uri)?;
-                let xmp_bytes = Bytes::from(xmp);
-                let segment = JpegSegment::new_with_contents(markers::APP1, xmp_bytes);
-                segments.insert(1, segment);
+                for (i, seg) in segments.iter().enumerate() {
+                    if seg.marker() == markers::APP1 && seg.contents().starts_with(XMP_SIGNATURE) {
+                        xmp = extract_xmp(seg).unwrap_or_else(|| xmp.clone());
+                        xmp_index = Some(i);
+                        break;
+                    }
+                }
+                // add provenance and JPEG XMP prefix
+                let xmp = format!(
+                    "http://ns.adobe.com/xap/1.0/\0 {}",
+                    add_provenance(&xmp, &manifest_uri)?
+                );
+                let segment = JpegSegment::new_with_contents(markers::APP1, Bytes::from(xmp));
+                // insert or add the segment
+                match xmp_index {
+                    Some(i) => segments[i] = segment,
+                    None => segments.insert(1, segment),
+                }
 
-                output_stream.rewind()?;
                 jpeg.encoder()
                     .write_to(output_stream)
                     .map_err(|_err| Error::InvalidAsset("JPEG write error".to_owned()))?;
@@ -800,7 +802,7 @@ fn make_box_maps(input_stream: &mut dyn CAIRead) -> Result<Vec<BoxMap>> {
                         let is_cai = vec_compare(&C2PA_MARKER, &jumb_type);
                         if is_cai {
                             cai_seg_cnt = 1;
-                            cai_en = en.clone(); // store the identifier
+                            cai_en.clone_from(&en); // store the identifier
 
                             let c2pa_bm = BoxMap {
                                 names: vec![C2PA_BOXHASH.to_string()],

sdk/src/asset_handlers/png_io.rs

@@ -607,7 +607,7 @@ fn get_xmp_insertion_point(asset_reader: &mut dyn CAIRead) -> Option<(u64, u32)>
 
     if let Some(xmp) = xmp_box {
         // overwrite existing box
-        Some((xmp.start, xmp.length))
+        Some((xmp.start, xmp.length + PNG_HDR_LEN as u32))
     } else {
         // insert after IHDR
         ps.iter()

sdk/src/asset_handlers/tiff_io.rs

@@ -161,9 +161,9 @@ pub(crate) struct TiffStructure {
 
 impl TiffStructure {
     #[allow(dead_code)]
-    pub fn load<R: ?Sized>(reader: &mut R) -> Result<Self>
+    pub fn load<R>(reader: &mut R) -> Result<Self>
     where
-        R: Read + Seek,
+        R: Read + Seek + ?Sized,
     {
         let mut endianness = [0u8, 2];
         reader.read_exact(&mut endianness)?;
@@ -231,14 +231,14 @@ impl TiffStructure {
     }
 
     // read IFD entries, all value_offset are in source endianness
-    pub fn read_ifd_entries<R: ?Sized>(
+    pub fn read_ifd_entries<R>(
         byte_reader: &mut ByteOrdered<&mut R, Endianness>,
         big_tiff: bool,
         entry_cnt: u64,
         entries: &mut HashMap<u16, IfdEntry>,
     ) -> Result<()>
     where
-        R: Read + Seek,
+        R: Read + Seek + ?Sized,
     {
         for _ in 0..entry_cnt {
             let tag = byte_reader.read_u16()?;
@@ -290,14 +290,14 @@ impl TiffStructure {
     }
 
     // read IFD from reader
-    pub fn read_ifd<R: ?Sized>(
+    pub fn read_ifd<R>(
         reader: &mut R,
         byte_order: Endianness,
         big_tiff: bool,
         ifd_type: IfdType,
     ) -> Result<ImageFileDirectory>
     where
-        R: Read + Seek + ReadBytesExt,
+        R: Read + Seek + ReadBytesExt + ?Sized,
     {
         let mut byte_reader = ByteOrdered::runtime(reader, byte_order);
 
@@ -365,11 +365,9 @@ fn stream_len(reader: &mut dyn CAIRead) -> crate::Result<u64> {
     Ok(len)
 }
 // create tree of TIFF structure IFDs and IFD entries.
-fn map_tiff<R: ?Sized>(
-    input: &mut R,
-) -> Result<(Arena<ImageFileDirectory>, Token, Endianness, bool)>
+fn map_tiff<R>(input: &mut R) -> Result<(Arena<ImageFileDirectory>, Token, Endianness, bool)>
 where
-    R: Read + Seek,
+    R: Read + Seek + ?Sized,
 {
     let _size = input.seek(SeekFrom::End(0))?;
     input.rewind()?;
@@ -1329,9 +1327,9 @@ fn add_required_tags_to_stream(
     }
 }
 
-fn get_cai_data<R: ?Sized>(asset_reader: &mut R) -> Result<Vec<u8>>
+fn get_cai_data<R>(asset_reader: &mut R) -> Result<Vec<u8>>
 where
-    R: Read + Seek,
+    R: Read + Seek + ?Sized,
 {
     let (tiff_tree, page_0, e, big_tiff) = map_tiff(asset_reader)?;
 
@@ -1362,9 +1360,9 @@ where
     Ok(data)
 }
 
-fn get_xmp_data<R: ?Sized>(asset_reader: &mut R) -> Option<Vec<u8>>
+fn get_xmp_data<R>(asset_reader: &mut R) -> Option<Vec<u8>>
 where
-    R: Read + Seek,
+    R: Read + Seek + ?Sized,
 {
     let (tiff_tree, page_0, e, big_tiff) = map_tiff(asset_reader).ok()?;
     let first_ifd = &tiff_tree[page_0].data;
@@ -1560,13 +1558,9 @@ impl CAIWriter for TiffIO {
         let mut bo = ByteOrdered::new(output_stream, e);
         let mut tc = TiffCloner::new(e, big_tiff, &mut bo)?;
 
-        match idfs[page_0].data.entries.remove(&C2PA_TAG) {
-            Some(_ifd) => {
-                tc.clone_tiff(&mut idfs, page_0, input_stream)?;
-                Ok(())
-            }
-            None => Ok(()),
-        }
+        idfs[page_0].data.entries.remove(&C2PA_TAG);
+        tc.clone_tiff(&mut idfs, page_0, input_stream)?;
+        Ok(())
     }
 }
 
@@ -1632,6 +1626,7 @@ impl RemoteRefEmbed for TiffIO {
                 }
 
                 // write will replace exisiting contents
+                output_stream.rewind()?;
                 std::fs::write(asset_path, output_stream.into_inner())?;
                 Ok(())
             }
@@ -1755,6 +1750,9 @@ pub mod tests {
 
         let tiff_io = TiffIO {};
 
+        // first make sure that calling this without a manifest does not error
+        tiff_io.remove_cai_store(&output).unwrap();
+
         // save data to tiff
         tiff_io.save_cai_store(&output, data.as_bytes()).unwrap();
 

sdk/src/asset_io.rs

@@ -169,9 +169,11 @@ pub trait AssetIO: Sync + Send {
     /// If the offsets exist return the start of those locations other it should
     /// return the calculated location of when it should start.  There may still be a
     /// length if the format contains extra header information for example.
+    #[allow(dead_code)] // this here for wasm builds to pass clippy  (todo: remove)
     fn get_object_locations(&self, asset_path: &Path) -> Result<Vec<HashObjectPositions>>;
 
     // Remove entire C2PA manifest store from asset
+    #[allow(dead_code)] // this here for wasm builds to pass clippy  (todo: remove)
     fn remove_cai_store(&self, asset_path: &Path) -> Result<()>;
 
     // List of supported extensions and mime types
@@ -180,6 +182,7 @@ pub trait AssetIO: Sync + Send {
     /// OPTIONAL INTERFACES
 
     // Returns [`AssetPatch`] trait if this I/O handler supports patching.
+    #[allow(dead_code)] // this here for wasm builds to pass clippy  (todo: remove)
     fn asset_patch_ref(&self) -> Option<&dyn AssetPatch> {
         None
     }
@@ -208,6 +211,7 @@ pub trait AssetPatch {
     // Patches an existing manifest store with new manifest store.
     // Only existing manifest stores of the same size may be patched
     // since any other changes will invalidate asset hashes.
+    #[allow(dead_code)] // this here for wasm builds to pass clippy  (todo: remove)
     fn patch_cai_store(&self, asset_path: &Path, store_bytes: &[u8]) -> Result<()>;
 }
 
@@ -236,6 +240,7 @@ pub enum RemoteRefEmbedType {
 // all embedding choices need be supported.
 pub trait RemoteRefEmbed {
     // Embed RemoteRefEmbedType into the asset
+    #[allow(dead_code)] // this here for wasm builds to pass clippy  (todo: remove)
     fn embed_reference(&self, asset_path: &Path, embed_ref: RemoteRefEmbedType) -> Result<()>;
     // Embed RemoteRefEmbedType into the asset stream
     fn embed_reference_to_stream(

sdk/src/builder.rs

@@ -500,8 +500,8 @@ impl Builder {
         if let Some(title) = definition.title.as_ref() {
             claim.set_title(Some(title.to_owned()));
         }
-        claim.format = definition.format.to_owned();
-        claim.instance_id = definition.instance_id.to_owned();
+        definition.format.clone_into(&mut claim.format);
+        definition.instance_id.clone_into(&mut claim.instance_id);
 
         if let Some(thumb_ref) = definition.thumbnail.as_ref() {
             // Setting the format to "none" will ensure that no claim thumbnail is added
@@ -712,7 +712,7 @@ impl Builder {
         W: Write + Read + Seek + Send,
     {
         let format = format_to_mime(format);
-        self.definition.format = format.clone();
+        self.definition.format.clone_from(&format);
         // todo:: read instance_id from xmp from stream ?
         self.definition.instance_id = format!("xmp:iid:{}", Uuid::new_v4());
 

sdk/src/cose_validator.rs

@@ -1293,10 +1293,10 @@ async fn validate_with_cert_async(
     }
 }
 #[allow(unused_imports)]
+#[allow(clippy::unwrap_used)]
 #[cfg(feature = "openssl_sign")]
 #[cfg(test)]
 pub mod tests {
-    #![allow(clippy::unwrap_used)]
 
     use sha2::digest::generic_array::sequence::Shorten;
 

sdk/src/error.rs

@@ -216,10 +216,10 @@ pub enum Error {
     ResourceNotFound(String),
 
     #[error("XMP read error")]
-    XmpReadError,
+    XmpReadError(String),
 
     #[error("XMP write error")]
-    XmpWriteError,
+    XmpWriteError(String),
 
     #[error("XMP is not supported")]
     XmpNotSupported,

sdk/src/ingredient.rs

@@ -1049,7 +1049,7 @@ impl Ingredient {
                 &data_box.format,
                 data_box.data.clone(),
             )?;
-            data_ref.data_types = data_box.data_types.clone();
+            data_ref.data_types.clone_from(&data_box.data_types);
             ingredient.set_data_ref(data_ref)?;
         }
 
@@ -1228,15 +1228,22 @@ impl Ingredient {
 
         let mut ingredient_assertion = assertions::Ingredient::new_v2(&self.title, &self.format);
         ingredient_assertion.instance_id = instance_id;
-        ingredient_assertion.document_id = self.document_id.to_owned();
+        self.document_id
+            .clone_into(&mut ingredient_assertion.document_id);
         ingredient_assertion.c2pa_manifest = c2pa_manifest;
         ingredient_assertion.relationship = self.relationship.clone();
         ingredient_assertion.thumbnail = thumbnail;
-        ingredient_assertion.metadata = self.metadata.clone();
-        ingredient_assertion.validation_status = self.validation_status.clone();
+        ingredient_assertion.metadata.clone_from(&self.metadata);
+        ingredient_assertion
+            .validation_status
+            .clone_from(&self.validation_status);
         ingredient_assertion.data = data;
-        ingredient_assertion.description = self.description.clone();
-        ingredient_assertion.informational_uri = self.informational_uri.clone();
+        ingredient_assertion
+            .description
+            .clone_from(&self.description);
+        ingredient_assertion
+            .informational_uri
+            .clone_from(&self.informational_uri);
         claim.add_assertion(&ingredient_assertion)
     }