Address PR feedback for avatar caching

- Swap save_actor_avatar parameter order (actor_id first)
- Increase MAX_AVATAR_DIMENSION to 512px for better quality
- Add actor_id validation to prevent path traversal
- Add is_array check to cache_avatar for flexibility
- Return avatar_url from cache_avatar
- Use short ternary for avatar_url assignment
- Update tests to match new parameter order

Author: pfefferle

includes/class-attachments.php

@@ -47,7 +47,7 @@ class Attachments {
 	 *
 	 * @var int
 	 */
-	const MAX_AVATAR_DIMENSION = 100;
+	const MAX_AVATAR_DIMENSION = 512;
 
 	/**
 	 * Initialize the class and set up filters.
@@ -992,17 +992,24 @@ private static function get_files_gallery_block( $files ) {
 	 * Downloads the avatar image, optimizes it, and stores it in the actors directory.
 	 * Returns the local URL for the saved avatar.
 	 *
-	 * @param string $avatar_url The remote avatar URL.
 	 * @param int    $actor_id   The local actor post ID.
+	 * @param string $avatar_url The remote avatar URL.
 	 *
 	 * @return string|false The local avatar URL on success, false on failure.
 	 */
-	public static function save_actor_avatar( $avatar_url, $actor_id ) {
+	public static function save_actor_avatar( $actor_id, $avatar_url ) {
+		// Validate actor_id is a positive integer to prevent path traversal.
+		$actor_id = (int) $actor_id;
+		if ( $actor_id <= 0 ) {
+			return false;
+		}
+
 		if ( empty( $avatar_url ) || ! \filter_var( $avatar_url, FILTER_VALIDATE_URL ) ) {
 			return false;
 		}
 
 		// Delete existing avatar files before saving new one.
+		// This prevents accumulating old avatar files since save_file creates unique filenames.
 		self::delete_actors_directory( $actor_id );
 
 		$attachment_data = array( 'url' => $avatar_url );
@@ -1021,6 +1028,12 @@ public static function save_actor_avatar( $avatar_url, $actor_id ) {
 	 * @param int $actor_id The actor post ID.
 	 */
 	public static function delete_actors_directory( $actor_id ) {
+		// Validate actor_id is a positive integer to prevent path traversal.
+		$actor_id = (int) $actor_id;
+		if ( $actor_id <= 0 ) {
+			return;
+		}
+
 		// When used as hook callback, only process actor post types.
 		$post_type = \get_post_type( $actor_id );
 		if ( $post_type && Remote_Actors::POST_TYPE !== $post_type ) {

includes/collection/class-remote-actors.php

@@ -685,39 +685,40 @@ public static function get_avatar_url( $id ) {
 			return $default_avatar_url;
 		}
 
-		// Try to cache the avatar locally.
-		$remote_avatar_url = object_to_uri( $actor_data['icon'] );
-		$local_url         = Attachments::save_actor_avatar( $remote_avatar_url, $id );
-
-		$avatar_url = $local_url ? $local_url : $remote_avatar_url;
-		\update_post_meta( $id, '_activitypub_avatar_url', \esc_url_raw( $avatar_url ) );
-
-		return $avatar_url;
+		return self::cache_avatar( $id, $actor_data );
 	}
 
 	/**
 	 * Cache a remote actor's avatar locally.
 	 *
 	 * Downloads the avatar image, optimizes it (resize/WebP), and stores it locally.
 	 *
-	 * @param int   $post_id The actor post ID.
-	 * @param Actor $actor   The actor object.
+	 * @param int                $post_id The actor post ID.
+	 * @param Actor|array|object $actor   The actor object or data array.
+	 *
+	 * @return string|null The cached avatar URL, or null if no avatar.
 	 */
 	private static function cache_avatar( $post_id, $actor ) {
-		$remote_avatar_url = object_to_uri( $actor->get_icon() );
+		if ( \is_array( $actor ) || ( \is_object( $actor ) && ! $actor instanceof Actor ) ) {
+			$remote_avatar_url = object_to_uri( $actor['icon'] ?? ( $actor->icon ?? null ) );
+		} else {
+			$remote_avatar_url = object_to_uri( $actor->get_icon() );
+		}
 
 		if ( empty( $remote_avatar_url ) ) {
 			// No avatar to save, clean up any existing avatar.
 			Attachments::delete_actors_directory( $post_id );
 			\delete_post_meta( $post_id, '_activitypub_avatar_url' );
-			return;
+			return null;
 		}
 
 		// Download and save the avatar locally.
-		$local_url = Attachments::save_actor_avatar( $remote_avatar_url, $post_id );
+		$local_url = Attachments::save_actor_avatar( $post_id, $remote_avatar_url );
 
 		// Store the local URL if caching succeeded, otherwise store the remote URL.
 		$avatar_url = $local_url ?: $remote_avatar_url;
 		\update_post_meta( $post_id, '_activitypub_avatar_url', \esc_url_raw( $avatar_url ) );
+
+		return $avatar_url;
 	}
 }

tests/phpunit/tests/includes/class-test-attachments.php

@@ -1057,7 +1057,7 @@ public function test_save_actor_avatar_success() {
 		);
 
 		$avatar_url = 'https://example.com/avatar.jpg';
-		$result     = Attachments::save_actor_avatar( $avatar_url, $actor_id );
+		$result     = Attachments::save_actor_avatar( $actor_id, $avatar_url );
 
 		$this->assertIsString( $result );
 		$this->assertStringContainsString( 'wp-content/uploads/activitypub/actors/' . $actor_id, $result );
@@ -1077,7 +1077,7 @@ public function test_save_actor_avatar_empty_url() {
 			)
 		);
 
-		$result = Attachments::save_actor_avatar( '', $actor_id );
+		$result = Attachments::save_actor_avatar( $actor_id, '' );
 
 		$this->assertFalse( $result );
 	}
@@ -1094,7 +1094,7 @@ public function test_save_actor_avatar_invalid_url() {
 			)
 		);
 
-		$result = Attachments::save_actor_avatar( 'not-a-valid-url', $actor_id );
+		$result = Attachments::save_actor_avatar( $actor_id, 'not-a-valid-url' );
 
 		$this->assertFalse( $result );
 	}
@@ -1112,7 +1112,7 @@ public function test_save_actor_avatar_download_error() {
 		);
 
 		// Use the missing.jpg URL that our mock returns as an error.
-		$result = Attachments::save_actor_avatar( 'https://example.com/missing.jpg', $actor_id );
+		$result = Attachments::save_actor_avatar( $actor_id, 'https://example.com/missing.jpg' );
 
 		$this->assertFalse( $result );
 	}
@@ -1131,11 +1131,11 @@ public function test_save_actor_avatar_replaces_existing() {
 		);
 
 		// Save first avatar.
-		$first_result = Attachments::save_actor_avatar( 'https://example.com/avatar1.jpg', $actor_id );
+		$first_result = Attachments::save_actor_avatar( $actor_id, 'https://example.com/avatar1.jpg' );
 		$this->assertIsString( $first_result );
 
 		// Save second avatar (should replace the first).
-		$second_result = Attachments::save_actor_avatar( 'https://example.com/avatar2.jpg', $actor_id );
+		$second_result = Attachments::save_actor_avatar( $actor_id, 'https://example.com/avatar2.jpg' );
 		$this->assertIsString( $second_result );
 
 		// URLs should be different (different source filenames).
@@ -1155,7 +1155,7 @@ public function test_delete_actors_directory() {
 		);
 
 		// Save an avatar first.
-		$avatar_url = Attachments::save_actor_avatar( 'https://example.com/avatar.jpg', $actor_id );
+		$avatar_url = Attachments::save_actor_avatar( $actor_id, 'https://example.com/avatar.jpg' );
 		$this->assertIsString( $avatar_url );
 
 		// Get the directory path.