Signature: Prevent double knock if signing data is missing #1985
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds a check in maybe_double_knock to return early if key_id, private_key, or Date header are not set in the response, avoiding processing for unrelated requests.
There was a problem hiding this comment.
Pull Request Overview
This PR prevents potential errors when unrelated HTTP requests are processed by the double-knock signature fallback mechanism. The fix adds early return checks to ensure only relevant requests with proper signing data are processed.
- Adds validation in
maybe_double_knockto skip processing when signing data is missing - Includes unit test to verify the fix prevents errors with unrelated requests
- Refactors parameter name for consistency
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| includes/class-signature.php | Adds missing validation checks and renames parameter for clarity |
| tests/includes/class-test-signature.php | Adds unit test to verify unrelated requests don't cause errors |
Comments suppressed due to low confidence (1)
tests/includes/class-test-signature.php:459
- The nested HTTP request in the test filter doesn't clearly demonstrate the issue being tested. Consider simplifying the test to focus on the specific scenario where signing data is missing from the request arguments.
\wp_safe_remote_get( 'https://example.org/wp-json/activitypub/1.0/actors/0/inbox' );
| * | ||
| * @return array The HTTP response. | ||
| */ | ||
| public static function maybe_double_knock( $response, $parsed_args, $url ) { | ||
| public static function maybe_double_knock( $response, $args, $url ) { | ||
| // Remove this filter to prevent infinite recursion. | ||
| \remove_filter( 'http_response', array( self::class, 'maybe_double_knock' ) ); | ||
|
|
||
| $response_code = \wp_remote_retrieve_response_code( $response ); | ||
|
|
||
| // Fall back to Draft Cavage signature for any 4xx responses. | ||
| if ( $response_code >= 400 && $response_code < 500 ) { |
There was a problem hiding this comment.
The condition lacks validation to ensure the request contains signing data before processing. Add checks for required headers like key_id, private_key, or Date to prevent processing unrelated requests that happen to return 4xx status codes.
Copilot uses AI. Check for mistakes.
…/wordpress-activitypub into update/sig-keys-check
pfefferle
approved these changes
Jul 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes:
maybe_double_knockto return early if key_id, private_key, or Date header are not set in the response, avoiding processing for unrelated requests.Other information:
Testing instructions:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Fixed potential errors when unrelated requests get caught in double-knocking callback.