diff --git a/php/class-media.php b/php/class-media.php index 43f95e68..8a98dc47 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|false + */ + 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 dd2d9100..aa8b7f9d 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,82 @@ 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 + * 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. + * + * @return bool + */ + 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; + } + // Byte-identical content between two unrelated attachments isn't proof of ownership: the + // second overwrite would still clobber the first's context and advance its version. Only + // proceed if no other attachment already claims this public ID. + if ( ! $this->is_solely_linked_to( $attachment_id, empty( $result['public_id'] ) ? null : $result['public_id'] ) ) { + return false; + } + $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; + } + // Hashing a vip:// stream wrapper path pulls the whole object over the network; a failed + // read returns false rather than throwing, which would wrongly read as a mismatch. Bytes + // alone is the safer signal to rely on there. + if ( false !== strpos( $file, 'vip://' ) ) { + return true; + } + + // Bytes alone can coincide between unrelated files; confirm with the content hash when available. + return empty( $result['etag'] ) || md5_file( $file ) === $result['etag']; + } + + /** + * Check that no other attachment is already tracked as linked to a public ID. + * + * Mirrors the ownership guard Delete_Sync::delete_asset() uses before destroying an asset. + * + * @param int $attachment_id The attachment ID. + * @param string|null $public_id The public ID to check. + * + * @return bool + */ + protected function is_solely_linked_to( $attachment_id, $public_id ) { + if ( empty( $public_id ) ) { + return false; + } + $linked = $this->media->get_linked_attachments( $public_id ); + if ( count( $linked ) > 1 ) { + // More than one attachment already shares this public ID. + return false; + } + if ( 1 === count( $linked ) && (int) $attachment_id !== (int) $linked[0] ) { + // Exactly one other attachment is already linked to it. + return false; + } + + return true; + } + /** * 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..2b81fcd2 --- /dev/null +++ b/tests/phpunit/tests/test-upload-sync.php @@ -0,0 +1,358 @@ +attachment->create_upload_object( $file ); + self::$attachment_bytes = filesize( get_attached_file( self::$attachment_id ) ); + } + + /** + * Build a fully wired Upload_Sync instance. + * + * 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() { + $upload_sync = new Upload_Sync( \Cloudinary\get_plugin_instance() ); + $upload_sync->setup(); + + return $upload_sync; + } + + /** + * Mark an attachment as linked to a public ID, the way a completed upload_asset() call + * would via its trackable postmeta key -- what get_linked_attachments() looks up. + * + * @param int $attachment_id The attachment ID. + * @param string $public_id The public ID. + * + * @return void + */ + protected function link_attachment_to_public_id( $attachment_id, $public_id ) { + update_post_meta( $attachment_id, '_' . md5( $public_id ), true ); + } + + /** + * 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. No etag in the result falls back to the + * byte comparison alone. + * + * @return void + */ + public function test_matches_when_existing_asset_bytes_equal_the_local_file() { + $result = array( + 'bytes' => self::$attachment_bytes, + 'public_id' => 'canola', + ); + + $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, + 'public_id' => 'canola', + ); + + $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 `public_id` field, there's no way to check who else might already be linked to + * it, so it must not be overwritten either. + * + * @return void + */ + public function test_does_not_match_when_result_has_no_public_id_field() { + $result = array( 'bytes' => self::$attachment_bytes ); + + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + + /** + * 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, + 'public_id' => 'canola', + ); + + $this->assertFalse( + $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 ) ), + 'public_id' => 'canola', + ); + + $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', + 'public_id' => 'canola', + ); + + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + + /** + * Byte-identical content is not proof of ownership: if another attachment is already + * tracked as linked to this public ID, overwriting it would clobber that attachment's + * context and advance its version out from under it, even though the bytes line up. + * + * @return void + */ + public function test_does_not_match_when_another_attachment_already_owns_the_public_id() { + $other_id = self::factory()->attachment->create_upload_object( DIR_TESTDATA . '/images/canola.jpg' ); + $this->link_attachment_to_public_id( $other_id, 'shared-id' ); + + $result = array( + 'bytes' => self::$attachment_bytes, + 'etag' => md5_file( get_attached_file( self::$attachment_id ) ), + 'public_id' => 'shared-id', + ); + + $this->assertFalse( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } + + /** + * This attachment being the one already tracked as linked to the public ID is the PR #1182 + * scenario itself (a prior successful upload whose local public_id record was then lost) -- + * still safe to overwrite. + * + * @return void + */ + public function test_matches_when_this_attachment_is_the_only_one_linked_to_the_public_id() { + $this->link_attachment_to_public_id( self::$attachment_id, 'canola' ); + + $result = array( + 'bytes' => self::$attachment_bytes, + 'public_id' => 'canola', + ); + + $this->assertTrue( + $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, 'public_id' => 'canola-original' ) ) + ); + // 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, 'public_id' => 'canola-original' ) ) + ); + } + + /** + * A vip:// stream wrapper path is resolved without hashing it: doing so would pull the + * whole object over the network, and a failed read (false from md5_file()) would wrongly + * read as a content mismatch. Byte size alone is what's checked there. + * + * @return void + */ + public function test_matches_on_a_vip_path_by_bytes_alone_even_with_a_wrong_etag() { + add_filter( 'cloudinary_use_original_image', '__return_false' ); + add_filter( 'get_attached_file', array( $this, 'filter_attached_file_to_vip_path' ), 10, 2 ); + + try { + $result = array( + 'bytes' => self::$attachment_bytes, + 'etag' => 'not-the-real-hash', + 'public_id' => 'canola', + ); + + $this->assertTrue( + $this->get_upload_sync()->is_matching_existing_asset( self::$attachment_id, $result ) + ); + } finally { + remove_filter( 'get_attached_file', array( $this, 'filter_attached_file_to_vip_path' ), 10 ); + remove_filter( 'cloudinary_use_original_image', '__return_false' ); + } + } + + /** + * Rewrites an attached file path onto a fake vip:// stream wrapper, keeping filesize() + * resolvable (a plain file underneath) while making the path itself look VIP-hosted. + * + * @param string $file The attached file path. + * @param int $attachment_id The attachment ID. + * + * @return string + */ + public function filter_attached_file_to_vip_path( $file, $attachment_id ) { + if ( (int) $attachment_id !== (int) self::$attachment_id ) { + return $file; + } + if ( ! in_array( 'vip', stream_get_wrappers(), true ) ) { + stream_wrapper_register( 'vip', 'Test_Upload_Sync_Vip_Stream_Wrapper' ); + } + Test_Upload_Sync_Vip_Stream_Wrapper::$real_path = $file; + + return 'vip://canola.jpg'; + } +} + +/** + * A minimal stream wrapper standing in for VIP's, backed by a real local file. + * + * Only url_stat() is implemented: it's all is_matching_existing_asset() needs for + * file_exists()/filesize() to resolve. md5_file() is deliberately never exercised through this + * path in the test -- that's the whole point of the vip:// short-circuit being tested. + */ +class Test_Upload_Sync_Vip_Stream_Wrapper { + + /** + * The stream context resource, set automatically by PHP; must be declared or its creation is + * a deprecated dynamic property under PHPUnit's convertDeprecationsToExceptions. + * + * @var resource|null + */ + public $context; + + /** + * The real, local file path this wrapper reads from. + * + * @var string + */ + public static $real_path; + + /** + * Stat the underlying real file, so file_exists()/filesize() resolve. + * + * @return array|false + */ + public function url_stat() { + return @stat( self::$real_path ); // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged + } +}