Skip to content

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

Merged
merged 4 commits into from
Jul 2, 2025
Merged

Conversation

obenland
Copy link
Member

@obenland obenland commented Jun 30, 2025

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:

  • Sends a second knock on any 4xx response code.

@obenland obenland requested a review from pfefferle June 30, 2025 18:31
@obenland obenland self-assigned this Jun 30, 2025
@Copilot Copilot AI review requested due to automatic review settings June 30, 2025 18:31
Copy link

@Copilot Copilot AI left a 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 ) {

@obenland obenland added the Skip Changelog Disables the "Changelog Updated" action for PRs where changelog entries are not necessary. label Jun 30, 2025
@obenland obenland changed the title Signature: Double-know on any 400 response Signature: Double-knock on any 400 response Jun 30, 2025
@obenland obenland requested a review from pfefferle June 30, 2025 21:27
$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 ) {
Copy link
Member Author

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.

Copy link
Member

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!?

obenland and others added 2 commits July 2, 2025 07:58
Co-authored-by: Matthias Pfefferle <pfefferle@users.noreply.github.com>
@obenland obenland merged commit adeb65e into trunk Jul 2, 2025
11 checks passed
@obenland obenland deleted the add/lemmy-response branch July 2, 2025 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog Disables the "Changelog Updated" action for PRs where changelog entries are not necessary.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants