fix: address review feedback on the existing-asset overwrite check

- Compare against the same file Api::upload() actually sends, not always the attached
  file. For images over big_image_size_threshold, get_attached_file() returns the
  "-scaled" copy while the upload itself sends the unscaled original via
  wp_get_original_image_path(), so the two sizes never matched and the #1182
  crash-recovery path silently stopped working for large images. Extracted the shared
  resolution into Media::get_upload_file_path(), used by both Api::upload() and the new
  check, instead of a third inline copy.
- Confirm with the response's etag (MD5 of the stored asset) once byte sizes already
  match, closing the remaining false-positive where two unrelated files coincidentally
  share a byte count.
- Log via Utils::log() when the check bails out for lack of a `bytes` field, so a future
  API response change doesn't silently resurrect the #1182 duplicate-per-cycle bug.
- Mark is_matching_existing_asset() @internal and narrow its docblock to the sync type it
  actually runs for.

Extends the test suite with the scaled-image and etag scenarios.

Author: gabriel-detassigny

php/class-media.php

@@ -495,6 +495,27 @@ function_exists( 'wp_get_original_image_path' )
 		return $file_size;
 	}
 
+	/**
+	 * Get the local file path used to upload an attachment.
+	 *
+	 * Mirrors the file resolution in Connect\Api::upload(): the unscaled original when
+	 * `cloudinary_use_original_image` allows it, the attached file otherwise -- e.g. the
+	 * `-scaled` copy WordPress creates for images over `big_image_size_threshold`.
+	 *
+	 * @param int $attachment_id The attachment ID.
+	 *
+	 * @return string
+	 */
+	public function get_upload_file_path( $attachment_id ) {
+		/** This filter is documented in php/connect/class-api.php */
+		$use_original = apply_filters( 'cloudinary_use_original_image', true, $attachment_id );
+		if ( $use_original && function_exists( 'wp_get_original_image_path' ) && wp_attachment_is_image( $attachment_id ) ) {
+			return wp_get_original_image_path( $attachment_id );
+		}
+
+		return get_attached_file( $attachment_id );
+	}
+
 	/**
 	 * Get the Cloudinary delivery type.
 	 *

php/connect/class-api.php

@@ -560,12 +560,7 @@ public function upload( $attachment_id, $args, $headers = array(), $try_remote =
 		} else {
 			// We should have the file in args at this point, but if the transient was set, it will be defaulting here.
 			if ( empty( $args['file'] ) ) {
-				if ( wp_attachment_is_image( $attachment_id ) ) {
-					$get_path_func = $use_original && function_exists( 'wp_get_original_image_path' ) ? 'wp_get_original_image_path' : 'get_attached_file';
-					$args['file']  = call_user_func( $get_path_func, $attachment_id );
-				} else {
-					$args['file'] = get_attached_file( $attachment_id );
-				}
+				$args['file'] = $this->media->get_upload_file_path( $attachment_id );
 			}
 			// Headers indicate chunked upload.
 			if ( empty( $headers ) && file_exists( $args['file'] ) ) {

php/sync/class-upload-sync.php

@@ -391,8 +391,12 @@ function ( $is_synced, $post_id ) use ( $attachment_id ) {
 	 * Check whether a Cloudinary "existing" asset is likely this attachment's own local file.
 	 *
 	 * Used to tell apart an orphan left by this same attachment's previously interrupted upload
-	 * (safe to overwrite) from an unrelated asset that happens to share the same derived public
-	 * ID, e.g. WordPress reusing a filename across months (must not be overwritten).
+	 * of the default (non "folder"/"cloud_name") sync type (safe to overwrite) from an unrelated
+	 * asset that happens to share the same derived public ID, e.g. WordPress reusing a filename
+	 * across months (must not be overwritten). Only called once a public_id is unrecorded, so in
+	 * practice this only ever runs for that default sync type; the other types always have one.
+	 *
+	 * @internal Reachable for testing; not intended to be called from outside this class.
 	 *
 	 * @param int   $attachment_id The attachment ID.
 	 * @param array $result        The Cloudinary upload result.
@@ -401,14 +405,23 @@ function ( $is_synced, $post_id ) use ( $attachment_id ) {
 	 */
 	public function is_matching_existing_asset( $attachment_id, $result ) {
 		if ( empty( $result['bytes'] ) ) {
+			Utils::log(
+				sprintf( 'Cloudinary upload result for attachment %d has no "bytes" field; treating as a non-matching asset.', $attachment_id ),
+				'upload-sync-existing-asset-check'
+			);
+
 			return false;
 		}
-		$file = get_attached_file( $attachment_id );
+		$file = $this->media->get_upload_file_path( $attachment_id );
 		if ( empty( $file ) || ! file_exists( $file ) ) {
 			return false;
 		}
+		if ( (int) filesize( $file ) !== (int) $result['bytes'] ) {
+			return false;
+		}
 
-		return (int) filesize( $file ) === (int) $result['bytes'];
+		// Bytes alone can coincide between unrelated files; confirm with the content hash when available.
+		return empty( $result['etag'] ) || md5_file( $file ) === $result['etag'];
 	}
 
 	/**

tests/phpunit/tests/test-upload-sync.php

@@ -50,20 +50,25 @@ public static function wpSetUpBeforeClass( WP_UnitTest_Factory $factory ) {
 	}
 
 	/**
-	 * Build an Upload_Sync instance.
+	 * Build a fully wired Upload_Sync instance.
 	 *
-	 * is_matching_existing_asset() only calls core get_attached_file()/filesize(), never touches
-	 * $media/$sync/$connect, so the component doesn't need setup() to have wired those up.
+	 * is_matching_existing_asset() reads the upload file path through $media, so setup() needs
+	 * to have run to wire it -- the real Media component, already initialised by the plugin
+	 * bootstrap, is reused rather than stubbed.
 	 *
 	 * @return Upload_Sync
 	 */
 	protected function get_upload_sync() {
-		return new Upload_Sync( \Cloudinary\get_plugin_instance() );
+		$upload_sync = new Upload_Sync( \Cloudinary\get_plugin_instance() );
+		$upload_sync->setup();
+
+		return $upload_sync;
 	}
 
 	/**
 	 * An existing asset whose byte size matches the local file is treated as this attachment's
-	 * own orphaned upload, so it's safe to overwrite.
+	 * own orphaned upload, so it's safe to overwrite. No etag in the result falls back to the
+	 * byte comparison alone.
 	 *
 	 * @return void
 	 */
@@ -115,4 +120,76 @@ public function test_does_not_match_when_the_attachment_has_no_local_file() {
 			$this->get_upload_sync()->is_matching_existing_asset( $post_id, $result )
 		);
 	}
+
+	/**
+	 * Matching bytes plus a matching etag (the MD5 of the stored asset) confirms the content
+	 * itself, not just its size.
+	 *
+	 * @return void
+	 */
+	public function test_matches_when_bytes_and_etag_both_match() {
+		$result = array(
+			'bytes' => self::$attachment_bytes,
+			'etag'  => md5_file( get_attached_file( self::$attachment_id ) ),
+		);
+
+		$this->assertTrue(
+			$this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result )
+		);
+	}
+
+	/**
+	 * A byte size that coincidentally matches an unrelated file must not be enough on its own
+	 * once an etag is available to rule it out.
+	 *
+	 * @return void
+	 */
+	public function test_does_not_match_when_bytes_match_but_etag_differs() {
+		$result = array(
+			'bytes' => self::$attachment_bytes,
+			'etag'  => 'not-the-real-hash',
+		);
+
+		$this->assertFalse(
+			$this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result )
+		);
+	}
+
+	/**
+	 * Cloudinary uploads the unscaled original for a "-scaled" image (the file WordPress
+	 * attaches for images over big_image_size_threshold is a downsized copy, not what was
+	 * actually sent), so the check must compare against that original, not the attached file.
+	 *
+	 * @return void
+	 */
+	public function test_matches_using_the_unscaled_original_for_a_scaled_image() {
+		$id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' );
+
+		$original_file = get_attached_file( $id );
+		$scaled_file   = dirname( $original_file ) . '/canola-scaled.jpg';
+
+		// Stand in for the "-scaled" file WordPress would attach: same starting bytes, padded
+		// so its size provably differs from the original left alongside it.
+		copy( $original_file, $scaled_file );
+		file_put_contents( $scaled_file, file_get_contents( $scaled_file ) . 'padding' ); // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents, WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents
+		update_attached_file( $id, $scaled_file );
+
+		$metadata                   = wp_get_attachment_metadata( $id );
+		$metadata['original_image'] = wp_basename( $original_file );
+		wp_update_attachment_metadata( $id, $metadata );
+
+		$original_bytes = filesize( $original_file );
+		$scaled_bytes   = filesize( $scaled_file );
+
+		$this->assertNotSame( $original_bytes, $scaled_bytes, 'Fixture files must differ in size for this test to be meaningful.' );
+
+		// Cloudinary was sent the original -- its bytes must be what's compared against.
+		$this->assertTrue(
+			$this->get_upload_sync()->is_matching_existing_asset( $id, array( 'bytes' => $original_bytes ) )
+		);
+		// The attached (scaled) file's size is not what was actually uploaded.
+		$this->assertFalse(
+			$this->get_upload_sync()->is_matching_existing_asset( $id, array( 'bytes' => $scaled_bytes ) )
+		);
+	}
 }