Respect force_signature in Delete handler's deferred verification#3223
Merged
Respect force_signature in Delete handler's deferred verification#3223
Conversation
The activitypub_defer_signature_verification filter was extended to three arguments in FEP-8fcf (#3202) so deferred-verification callbacks could opt out when the caller had already forced signing (e.g. the /followers/sync route). The Delete handler's registration was left at arity 2, silently dropping the new third argument. The carve-out would therefore still fire on any future endpoint that combines a POST body with force_signature=true — not exploitable today because the only force_signature endpoint is GET-only, but a footgun worth closing. Raise the filter arity to 3 and short-circuit with \$defer unchanged when force_signature is true. Non-Delete activities continue to pass through as before. Add regression tests for both branches.
There was a problem hiding this comment.
Pull request overview
This PR fixes a subtle security-footgun in the Delete handler by ensuring it receives (and respects) the force_signature flag that was added to deferred signature verification in PR #3202 (FEP-8fcf work).
Changes:
- Update the Delete handler’s
activitypub_defer_signature_verificationfilter registration to accept 3 arguments (so$force_signatureisn’t dropped). - Update
Delete::defer_signature_verification()to accept$force_signature(defaultfalse) and to short-circuit without overriding when forced verification is enabled. - Add PHPUnit coverage to confirm forced-signature requests are not overridden, and non-Delete requests remain pass-through.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| includes/handler/class-delete.php | Fixes filter arity and respects $force_signature to avoid bypassing mandatory signature verification. |
| tests/phpunit/tests/includes/handler/class-test-delete.php | Adds tests validating Delete deferral behavior with/without forced signature verification and for non-Delete types. |
| .github/changelog/fix-delete-handler-force-signature | Adds a patch-level changelog entry documenting the fix. |
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
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:
Addresses DELETE-1 from a recent security audit. The
activitypub_defer_signature_verificationfilter was extended in FEP-8fcf (PR #3202) to pass a third argument,\$force_signature, so deferred-verification callbacks could inspect it and opt out when mandatory signing was already in effect (e.g./followers/sync). The Delete handler's registration was left at arity 2, silently dropping that argument. No live exploit today because the onlyforce_signature=trueendpoint is GET-only and the Delete carve-out only fires on POST bodies, but it's a footgun that would bite the next time someone adds a POST endpoint with mandatory signing.includes/handler/class-delete.php:27— raise filter arity from 2 to 3.includes/handler/class-delete.php:306— add\$force_signatureparameter withfalsedefault; short-circuit returning\$deferunchanged when true. Non-Delete activities continue to pass through. Default preserves backwards compatibility for any third-party caller invoking the method directly.Other information:
Two new tests in
Test_Delete:test_defer_signature_verification_respects_force_signature— Delete body withforce_signature=falsedefers as before; withforce_signature=trueno longer overrides.test_defer_signature_verification_passthrough_for_non_delete— Create body preserves incoming\$deferregardless of force state.Test_Delete: 40/40 passing, lint clean.
Testing instructions:
Mostly a defensive change — no user-visible behavior difference today. To verify the fix:
npm run env-test -- --filter=Test_Delete. All 40 tests should pass./followers/sync(FEP-8fcf) still rejects unsigned requests — this is the endpoint that already forces signatures, though it's GET-only so wasn't affected anyway.Changelog entry
Included as
.github/changelog/fix-delete-handler-force-signature, significancepatch/ typefixed.