Use a more explicit signature verification (#1078)

* Use a more explicit signature verification This is a proposal to use the `permission_callback`, instead of a general hook, to verify signatures. The advantage is, that it is easier to enable/disable verification for specific endpoints this way. See #1077 * phpcs fix * fix test * ignore this for now * changelog * keep the old error and change the function name to be more desciptive props @jeherve * add some phpdoc props @jeherve * verify actor endpoints * no need to mention `activitypub` here * rename functions * Update includes/rest/class-server.php Co-authored-by: Konstantin Obenland <[email protected]> * Update includes/rest/class-server.php Co-authored-by: Konstantin Obenland <[email protected]> * messed up search/replace * one last change * add some checks to prevent PHP warnings * add integration test * fix phpcs --------- Co-authored-by: Konstantin Obenland <[email protected]>
Automattic · Dec 17, 2024 · f080cbd · f080cbd
1 parent cda1bf4
commit f080cbd
Show file tree

Hide file tree

Showing 10 changed files with 171 additions and 42 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * Improve Interactions moderation
 * Compatibility with Akismet
 * Comment type mapping for `Like` and `Announce`
+* Signature verification for API endpoints
 * Changed priority of Attachments, to favor `Image` over `Audio` and `Video`
 
 ### Fixed

diff --git a/includes/class-signature.php b/includes/class-signature.php
@@ -321,7 +321,6 @@ public static function verify_http_signature( $request ) {
 		}
 
 		$verified = \openssl_verify( $signed_data, $signature_block['signature'], $public_key, $algorithm ) > 0;
-
 		if ( ! $verified ) {
 			return new WP_Error( 'activitypub_signature', __( 'Invalid signature', 'activitypub' ), array( 'status' => 401 ) );
 		}
@@ -403,7 +402,11 @@ public static function parse_signature_header( $signature ) {
 			$parsed_header['signature'] = \base64_decode( preg_replace( '/\s+/', '', trim( $matches[1] ) ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.obfuscation_base64_decode
 		}
 
-		if ( ( $parsed_header['signature'] ) && ( $parsed_header['algorithm'] ) && ( ! $parsed_header['headers'] ) ) {
+		if (
+			! empty( $parsed_header['signature'] ) &&
+			! empty( $parsed_header['algorithm'] ) &&
+			empty( $parsed_header['headers'] )
+		) {
 			$parsed_header['headers'] = array( 'date' );
 		}
 
@@ -461,6 +464,10 @@ public static function get_signed_data( $signed_headers, $signature_block, $head
 				}
 			}
 			if ( 'date' === $header ) {
+				if ( empty( $headers[ $header ][0] ) ) {
+					continue;
+				}
+
 				// Allow a bit of leeway for misconfigured clocks.
 				$d = new DateTime( $headers[ $header ][0] );
 				$d->setTimeZone( new DateTimeZone( 'UTC' ) );
@@ -474,7 +481,10 @@ public static function get_signed_data( $signed_headers, $signature_block, $head
 					return false;
 				}
 			}
-			$signed_data .= $header . ': ' . $headers[ $header ][0] . "\n";
+
+			if ( ! empty( $headers[ $header ][0] ) ) {
+				$signed_data .= $header . ': ' . $headers[ $header ][0] . "\n";
+			}
 		}
 		return \rtrim( $signed_data, "\n" );
 	}

diff --git a/includes/rest/class-actors.php b/includes/rest/class-actors.php
@@ -41,7 +41,7 @@ public static function register_routes() {
 				array(
 					'methods'             => WP_REST_Server::READABLE,
 					'callback'            => array( self::class, 'get' ),
-					'permission_callback' => '__return_true',
+					'permission_callback' => array( 'Activitypub\Rest\Server', 'verify_signature' ),
 				),
 			)
 		);

diff --git a/includes/rest/class-followers.php b/includes/rest/class-followers.php
@@ -43,7 +43,7 @@ public static function register_routes() {
 					'methods'             => WP_REST_Server::READABLE,
 					'callback'            => array( self::class, 'get' ),
 					'args'                => self::request_parameters(),
-					'permission_callback' => '__return_true',
+					'permission_callback' => array( 'Activitypub\Rest\Server', 'verify_signature' ),
 				),
 			)
 		);

diff --git a/includes/rest/class-following.php b/includes/rest/class-following.php
@@ -43,7 +43,7 @@ public static function register_routes() {
 					'methods'             => \WP_REST_Server::READABLE,
 					'callback'            => array( self::class, 'get' ),
 					'args'                => self::request_parameters(),
-					'permission_callback' => '__return_true',
+					'permission_callback' => array( 'Activitypub\Rest\Server', 'verify_signature' ),
 				),
 			)
 		);

diff --git a/includes/rest/class-inbox.php b/includes/rest/class-inbox.php
@@ -45,7 +45,7 @@ public static function register_routes() {
 					'methods'             => WP_REST_Server::CREATABLE,
 					'callback'            => array( self::class, 'shared_inbox_post' ),
 					'args'                => self::shared_inbox_post_parameters(),
-					'permission_callback' => '__return_true',
+					'permission_callback' => array( 'Activitypub\Rest\Server', 'verify_signature' ),
 				),
 			)
 		);
@@ -58,13 +58,13 @@ public static function register_routes() {
 					'methods'             => WP_REST_Server::CREATABLE,
 					'callback'            => array( self::class, 'user_inbox_post' ),
 					'args'                => self::user_inbox_post_parameters(),
-					'permission_callback' => '__return_true',
+					'permission_callback' => array( 'Activitypub\Rest\Server', 'verify_signature' ),
 				),
 				array(
 					'methods'             => WP_REST_Server::READABLE,
 					'callback'            => array( self::class, 'user_inbox_get' ),
 					'args'                => self::user_inbox_get_parameters(),
-					'permission_callback' => '__return_true',
+					'permission_callback' => array( 'Activitypub\Rest\Server', 'verify_signature' ),
 				),
 			)
 		);

diff --git a/includes/rest/class-outbox.php b/includes/rest/class-outbox.php
@@ -45,7 +45,7 @@ public static function register_routes() {
 					'methods'             => WP_REST_Server::READABLE,
 					'callback'            => array( self::class, 'user_outbox_get' ),
 					'args'                => self::request_parameters(),
-					'permission_callback' => '__return_true',
+					'permission_callback' => array( 'Activitypub\Rest\Server', 'verify_signature' ),
 				),
 			)
 		);

diff --git a/includes/rest/class-server.php b/includes/rest/class-server.php
@@ -35,8 +35,7 @@ public static function init() {
 	 * Add sever hooks.
 	 */
 	public static function add_hooks() {
-		\add_filter( 'rest_request_before_callbacks', array( self::class, 'validate_activitypub_requests' ), 9, 3 );
-		\add_filter( 'rest_request_before_callbacks', array( self::class, 'authorize_activitypub_requests' ), 10, 3 );
+		\add_filter( 'rest_request_before_callbacks', array( self::class, 'validate_requests' ), 9, 3 );
 		\add_filter( 'rest_request_parameter_order', array( self::class, 'request_parameter_order' ), 10, 2 );
 	}
 
@@ -74,39 +73,24 @@ public static function application_actor() {
 	}
 
 	/**
-	 * Callback function to authorize each api requests
+	 * Callback function to authorize an api request.
 	 *
-	 * @see WP_REST_Request
+	 * The function is meant to be used as part of permission callbacks for rest api endpoints.
+	 *
+	 * It verifies the signature of POST, PUT, PATCH, and DELETE requests, as well as GET requests in secure mode.
+	 * You can use the filter 'activitypub_defer_signature_verification' to defer the signature verification.
+	 * HEAD requests are always bypassed.
 	 *
 	 * @see https://www.w3.org/wiki/SocialCG/ActivityPub/Primer/Authentication_Authorization#Authorized_fetch
 	 * @see https://swicg.github.io/activitypub-http-signature/#authorized-fetch
 	 *
-	 * @param WP_REST_Response|\WP_HTTP_Response|WP_Error|mixed $response Result to send to the client.
-	 *                                                                    Usually a WP_REST_Response or WP_Error.
-	 * @param array                                             $handler  Route handler used for the request.
-	 * @param \WP_REST_Request                                  $request  Request used to generate the response.
+	 * @param \WP_REST_Request $request The request object.
 	 *
-	 * @return mixed|WP_Error The response, error, or modified response.
+	 * @return bool|\WP_Error True if the request is authorized, WP_Error if not.
 	 */
-	public static function authorize_activitypub_requests( $response, $handler, $request ) {
+	public static function verify_signature( $request ) {
 		if ( 'HEAD' === $request->get_method() ) {
-			return $response;
-		}
-
-		if ( \is_wp_error( $response ) ) {
-			return $response;
-		}
-
-		$route = $request->get_route();
-
-		// Check if it is an activitypub request and exclude webfinger and nodeinfo endpoints.
-		if (
-			! \str_starts_with( $route, '/' . ACTIVITYPUB_REST_NAMESPACE ) ||
-			\str_starts_with( $route, '/' . \trailingslashit( ACTIVITYPUB_REST_NAMESPACE ) . 'webfinger' ) ||
-			\str_starts_with( $route, '/' . \trailingslashit( ACTIVITYPUB_REST_NAMESPACE ) . 'nodeinfo' ) ||
-			\str_starts_with( $route, '/' . \trailingslashit( ACTIVITYPUB_REST_NAMESPACE ) . 'application' )
-		) {
-			return $response;
+			return true;
 		}
 
 		/**
@@ -123,11 +107,11 @@ public static function authorize_activitypub_requests( $response, $handler, $req
 		$defer = \apply_filters( 'activitypub_defer_signature_verification', false, $request );
 
 		if ( $defer ) {
-			return $response;
+			return true;
 		}
 
 		if (
-			// POST-Requests are always signed.
+			// POST-Requests always have to be signed.
 			'GET' !== $request->get_method() ||
 			// GET-Requests only require a signature in secure mode.
 			( 'GET' === $request->get_method() && use_authorized_fetch() )
@@ -142,7 +126,7 @@ public static function authorize_activitypub_requests( $response, $handler, $req
 			}
 		}
 
-		return $response;
+		return true;
 	}
 
 	/**
@@ -155,7 +139,7 @@ public static function authorize_activitypub_requests( $response, $handler, $req
 	 *
 	 * @return mixed|WP_Error The response, error, or modified response.
 	 */
-	public static function validate_activitypub_requests( $response, $handler, $request ) {
+	public static function validate_requests( $response, $handler, $request ) {
 		if ( 'HEAD' === $request->get_method() ) {
 			return $response;
 		}

diff --git a/readme.txt b/readme.txt
@@ -142,6 +142,7 @@ For reasons of data protection, it is not possible to see the followers of other
 * Improved: Interactions moderation
 * Improved: Compatibility with Akismet
 * Improved: Comment type mapping for `Like` and `Announce`
+* Improved: Signature verification for API endpoints
 * Improved: Changed priority of Attachments, to favor `Image` over `Audio` and `Video`
 * Fixed: Empty `url` attributes in the Reply block no longer cause PHP warnings