pleroma/pleroma
4
6
Fork 10
Code Issues 719 Pull requests 81 Projects Releases 1 Packages Wiki Activity

Reject unsupported HTTP Message Signatures #7918

Open
lambadalambda wants to merge 1 commit from activitypub-bot-federation into develop
pull from: activitypub-bot-federation
merge into: pleroma:develop
Conversation 1 Commits 1 Files changed 3 +103 -8
lambadalambda commented 2026-05-29 14:36:15 +00:00
Owner
Copy link

Summary

  • Return 401 for unsupported RFC 9421 HTTP Message Signatures on ActivityPub inbox delivery.
  • Preserve failed-signature retry behavior for legacy draft-cavage signatures with keyId.
  • Add regression tests and changelog entry.

Testing

  • mix format --check-formatted lib/pleroma/web/activity_pub/activity_pub_controller.ex test/pleroma/web/activity_pub/activity_pub_controller_test.exs
  • git diff --check
  • podman compose -f compose.yml up --build --abort-on-container-exit --exit-code-from activitypub-bot-test
  • Container focused tests: mix test test/pleroma/web/activity_pub/activity_pub_controller_test.exs:760 test/pleroma/web/activity_pub/activity_pub_controller_test.exs:797

Review

  • Independent code review completed.
  • Initial low-severity findings were addressed; re-review reported no findings.
## Summary - Return 401 for unsupported RFC 9421 HTTP Message Signatures on ActivityPub inbox delivery. - Preserve failed-signature retry behavior for legacy draft-cavage signatures with keyId. - Add regression tests and changelog entry. ## Testing - `mix format --check-formatted lib/pleroma/web/activity_pub/activity_pub_controller.ex test/pleroma/web/activity_pub/activity_pub_controller_test.exs` - `git diff --check` - `podman compose -f compose.yml up --build --abort-on-container-exit --exit-code-from activitypub-bot-test` - Container focused tests: `mix test test/pleroma/web/activity_pub/activity_pub_controller_test.exs:760 test/pleroma/web/activity_pub/activity_pub_controller_test.exs:797` ## Review - Independent code review completed. - Initial low-severity findings were addressed; re-review reported no findings.
Reject unsupported HTTP Message Signatures
All checks were successful
ci/woodpecker/pr/lint Pipeline was successful
Details
ci/woodpecker/pr/docker-armv7 Pipeline was successful
Details
ci/woodpecker/pr/unit-testing-elixir-1.15 Pipeline was successful
Details
ci/woodpecker/pr/unit-testing-elixir-1.18 Pipeline was successful
Details
ci/woodpecker/pr/changelog Pipeline was successful
Details
e676ec3507
lambadalambda changed title from WIP: Reject unsupported HTTP Message Signatures to Reject unsupported HTTP Message Signatures 2026-05-30 08:21:56 +00:00
phnt reviewed 2026-06-03 21:23:49 +00:00
lib/pleroma/web/activity_pub/activity_pub_controller.ex
@ -384,0 +395,4 @@
defp cavage_signature?(conn) do
conn
|> Plug.Conn.get_req_header("signature")
|> Enum.any?(&Regex.match?(~r/(^|,)\s*keyId\s*=/, &1))
phnt commented 2026-06-03 21:23:49 +00:00
Owner
Copy link

Probably should bail out on requests with multiple signature headers instead (more than one signature header, not signature and signature-input).

Probably should bail out on requests with multiple signature headers instead (more than one `signature` header, not `signature` and `signature-input`).
lambadalambda commented 2026-06-05 09:51:18 +00:00
Author
Owner
Copy link

can you elaborate on that?

can you elaborate on that?
phnt commented 2026-06-05 11:25:41 +00:00
Owner
Copy link

HTTP has no issue with sending/receiving a request that has multiple headers with the same name and different content. Plug.Conn.get_req_header will return you a list of all values for headers with that name. Similarly http_signatures uses Enum.into on the conn.req_headers which is a List of Tuples to get the headers into a Map. Enum.into has the side-effect where given input with the same key multiple times, only the last one occurrence is returned.

So above we are checking whether any signature header matches the regex, meanwhile http_signatures might process only the last one which might not have matched the regex and fail. This is not an issue now, but might become one in the future if more processing is added for some reason.

In general processing requests with the same header multiple times is discouraged. The ensure host plug already bails out on multiple Host headers, because you cannot be sure whether the request is intended for you at that point. Check first one and it is, check the second one and it might not. With signatures, the first one might be valid, the second one might not. And there's no reason why a request should have two signature headers besides something funky is going on.

HTTP has no issue with sending/receiving a request that has multiple headers with the same name and different content. `Plug.Conn.get_req_header` will return you a list of all values for headers with that name. Similarly `http_signatures` uses `Enum.into` on the `conn.req_headers` which is a List of Tuples to get the headers into a Map. `Enum.into` has the side-effect where given input with the same key multiple times, only the last one occurrence is returned. So above we are checking whether _any_ `signature` header matches the regex, meanwhile `http_signatures` might process only the last one which might not have matched the regex and fail. This is not an issue now, but might become one in the future if more processing is added for some reason. In general processing requests with the same header multiple times is discouraged. The ensure host plug already bails out on multiple `Host` headers, because you cannot be sure whether the request is intended for you at that point. Check first one and it is, check the second one and it might not. With signatures, the first one might be valid, the second one might not. And there's no reason why a request should have two `signature` headers besides something funky is going on.
All checks were successful
ci/woodpecker/pr/lint Pipeline was successful
Details
ci/woodpecker/pr/docker-armv7 Pipeline was successful
Details
ci/woodpecker/pr/unit-testing-elixir-1.15 Pipeline was successful
Details
ci/woodpecker/pr/unit-testing-elixir-1.18 Pipeline was successful
Details
ci/woodpecker/pr/changelog Pipeline was successful
Details
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin activitypub-bot-federation:activitypub-bot-federation
git switch activitypub-bot-federation

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch develop
git merge --no-ff activitypub-bot-federation
git switch activitypub-bot-federation
git rebase develop
git switch develop
git merge --ff-only activitypub-bot-federation
git switch activitypub-bot-federation
git rebase develop
git switch develop
git merge --no-ff activitypub-bot-federation
git switch develop
git merge --squash activitypub-bot-federation
git switch develop
git merge --ff-only activitypub-bot-federation
git switch develop
git merge activitypub-bot-federation
git push origin develop
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
AP C2S

   
AdminAPI

   
Breaking API
Affects clients

  
Breaks existing config

   
Check if still valid

   
Data Protection

   
Do not merge

   
Document in the changelog

   
Doing

   
Feature Request / Enhancement

   
MastoAPI

   
NL2
Improved Mobile Experience

  
NL4
Improved instance customization and QoL options

  
OStatus

   
OTP Releases

   
Refactor

   
Security

   
To Do

   
TwitterAPI

   
bug

   
complicated

   
confirmed

   
deps

   
discussion

   
documentation

   
easy

   
feature: to be merged in stable

   
fix: to be merged in stable

   
incident
Denotes a disruption to IT services and the associated issues require immediate attention

  
needs to be backported to maint

   
needs-changes

   
needs-info

   
needs-review

   
performance

   
regression

   
unclear
No labels
AP C2S
AdminAPI
Breaking API
Breaks existing config
Check if still valid
Data Protection
Do not merge
Document in the changelog
Doing
Feature Request / Enhancement
MastoAPI
NL2
NL4
OStatus
OTP Releases
Refactor
Security
To Do
TwitterAPI
bug
complicated
confirmed
deps
discussion
documentation
easy
feature: to be merged in stable
fix: to be merged in stable
incident
needs to be backported to maint
needs-changes
needs-info
needs-review
performance
regression
unclear
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
pleroma/pleroma!7918
No description provided.