From d23b52e6cdabbea3980bf738d8ae7d2d16f6abe9 Mon Sep 17 00:00:00 2001 From: Gabriel de Tassigny Date: Wed, 26 Aug 2026 12:16:59 +0200 Subject: [PATCH 1/2] fix: only overwrite an existing Cloudinary asset when it's this attachment's own orphan PR #1182 treated any attachment with no locally-saved public_id as proof that a colliding Cloudinary asset was an orphan from that same attachment's own crashed upload, and overwrote it. That's wrong for a brand new attachment too, since it also has no public_id yet -- so a fresh upload that happens to derive the same public_id as an unrelated, older asset (e.g. WordPress reusing a filename across months) silently clobbers that older asset. Only take the overwrite path when the existing asset's byte size also matches the local file, which is true for a genuine orphan of this attachment's own upload but not for an unrelated collision. Fixes #1241 --- php/sync/class-upload-sync.php | 35 ++++++- tests/phpunit/tests/test-upload-sync.php | 118 +++++++++++++++++++++++ 2 files changed, 150 insertions(+), 3 deletions(-) create mode 100644 tests/phpunit/tests/test-upload-sync.php diff --git a/php/sync/class-upload-sync.php b/php/sync/class-upload-sync.php index dd2d9100..aabd6ffa 100644 --- a/php/sync/class-upload-sync.php +++ b/php/sync/class-upload-sync.php @@ -338,9 +338,14 @@ function ( $is_synced, $post_id ) use ( $attachment_id ) { // Check that this wasn't an existing. if ( ! empty( $result['existing'] ) ) { - // If no public_id is recorded in WordPress, this asset in Cloudinary is from a - // failed previous upload. Overwrite it instead of creating a suffixed duplicate. - if ( empty( $suffix ) && ! $this->media->get_post_meta( $attachment_id, Sync::META_KEYS['public_id'], true ) ) { + // A missing public_id in WordPress isn't enough on its own to prove the conflicting + // Cloudinary asset is an orphan of this attachment's own failed upload -- any never + // synced attachment also has no public_id. Only treat it as our own orphan, safe to + // overwrite, when the existing asset's file size also matches the local file. + if ( empty( $suffix ) + && ! $this->media->get_post_meta( $attachment_id, Sync::META_KEYS['public_id'], true ) + && $this->is_matching_existing_asset( $attachment_id, $result ) + ) { return $this->upload_asset( $attachment_id, $type, null, true ); } // Add a suffix and try again. @@ -382,6 +387,30 @@ function ( $is_synced, $post_id ) use ( $attachment_id ) { return $result; } + /** + * 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). + * + * @param int $attachment_id The attachment ID. + * @param array $result The Cloudinary upload result. + * + * @return bool + */ + public function is_matching_existing_asset( $attachment_id, $result ) { + if ( empty( $result['bytes'] ) ) { + return false; + } + $file = get_attached_file( $attachment_id ); + if ( empty( $file ) || ! file_exists( $file ) ) { + return false; + } + + return (int) filesize( $file ) === (int) $result['bytes']; + } + /** * Update an assets context.. * diff --git a/tests/phpunit/tests/test-upload-sync.php b/tests/phpunit/tests/test-upload-sync.php new file mode 100644 index 00000000..40bd98a7 --- /dev/null +++ b/tests/phpunit/tests/test-upload-sync.php @@ -0,0 +1,118 @@ +attachment->create_upload_object( $file ); + self::$attachment_bytes = filesize( get_attached_file( self::$attachment_id ) ); + } + + /** + * Build an 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. + * + * @return Upload_Sync + */ + protected function get_upload_sync() { + return new Upload_Sync( \Cloudinary\get_plugin_instance() ); + } + + /** + * 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. + * + * @return void + */ + public function test_matches_when_existing_asset_bytes_equal_the_local_file() { + $result = array( 'bytes' => self::$attachment_bytes ); + + $this->assertTrue( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + + /** + * An existing asset with a different byte size is a different, unrelated asset -- the + * collision this attachment must not overwrite. + * + * @return void + */ + public function test_does_not_match_when_existing_asset_bytes_differ() { + $result = array( 'bytes' => self::$attachment_bytes + 1 ); + + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + + /** + * Without a `bytes` field to compare against, there's no basis to treat the collision as + * this attachment's own asset, so it must not be overwritten. + * + * @return void + */ + public function test_does_not_match_when_result_has_no_bytes_field() { + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, array() ) + ); + } + + /** + * Without a local file to compare against, there's no basis for a match either. + * + * @return void + */ + public function test_does_not_match_when_the_attachment_has_no_local_file() { + $post_id = self::factory()->post->create( array( 'post_type' => 'attachment' ) ); + + $result = array( 'bytes' => self::$attachment_bytes ); + + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( $post_id, $result ) + ); + } +} From 9187312e044dcd5137f2fafbfc2fa2ec20916474 Mon Sep 17 00:00:00 2001 From: Gabriel de Tassigny Date: Thu, 27 Aug 2026 11:03:04 +0200 Subject: [PATCH 2/2] 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. --- php/class-media.php | 21 ++++++ php/connect/class-api.php | 7 +- php/sync/class-upload-sync.php | 21 ++++-- tests/phpunit/tests/test-upload-sync.php | 87 ++++++++++++++++++++++-- 4 files changed, 121 insertions(+), 15 deletions(-) diff --git a/php/class-media.php b/php/class-media.php index 43f95e68..3f4700c1 100644 --- a/php/class-media.php +++ b/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. * diff --git a/php/connect/class-api.php b/php/connect/class-api.php index d3687603..606ccda8 100644 --- a/php/connect/class-api.php +++ b/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'] ) ) { diff --git a/php/sync/class-upload-sync.php b/php/sync/class-upload-sync.php index aabd6ffa..dfe5d6b3 100644 --- a/php/sync/class-upload-sync.php +++ b/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']; } /** diff --git a/tests/phpunit/tests/test-upload-sync.php b/tests/phpunit/tests/test-upload-sync.php index 40bd98a7..8d43bf8d 100644 --- a/tests/phpunit/tests/test-upload-sync.php +++ b/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 ) ) + ); + } }