-
Notifications
You must be signed in to change notification settings - Fork 81
Signature: Double-knock on any 400 response #1875
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR broadens the "double knock" retry logic to apply on any 4xx HTTP response, not just 401 Unauthorized.
- Extracts the response code into a variable.
- Changes the condition from strictly 401 to any 400–499 status.
- Continues to remove signature headers and mark hosts unsupported on retry.
Comments suppressed due to low confidence (2)
includes/class-signature.php:206
- The comment above still references a 401 status; please update it to mention retrying on any 4xx response to match the new logic.
// Remove this filter to prevent infinite recursion.
includes/class-signature.php:210
- Consider adding or updating unit tests to cover the new retry behavior for various 4xx status codes (e.g., 404, 429) to ensure this path is exercised.
if ( 400 <= $response_code && 500 > $response_code ) {
includes/class-signature.php
Outdated
$response_code = \wp_remote_retrieve_response_code( $response ); | ||
|
||
// Fall back to Draft Cavage signature for any 4xx responses. | ||
if ( 400 <= $response_code && $response_code < 500 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pfefferle Are we being to broad with this? So far I've only observed 401s and 400s. And a 500 for Ghost, but I think that may be a one-off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am fine with checking all 4xx codes for now. Otherwise we could limit to 400 and 401 and start logging error codes!?
Co-authored-by: Matthias Pfefferle <pfefferle@users.noreply.github.com>
Lemmy processes content digest first and fails it with a
400
response. We'll need to expand the status code check to also send a second knock in that case.Follow-up to #1858.
Proposed changes: