pleroma/pleroma
Watch
4
6
Fork
You've already forked pleroma
 13
Code Issues 358 Pull requests 45 Projects Releases 1 Packages 13 Wiki Activity

WIP: Detect extensionless Document images during ingestion #7952

Draft
lambadalambda wants to merge 2 commits from fix/issue-7951-extensionless-document-images into develop
pull from: fix/issue-7951-extensionless-document-images
merge into: pleroma:develop
Conversation 2 Commits 2 Files changed 16 +1005 -15
lambadalambda commented 2026-07-30 05:46:19 +00:00
Owner
Copy link

Summary

  • add opt-in ingestion-time MIME detection for generic ActivityStreams Document attachments
  • share bounded image classification with the reverse proxy
  • validate every destination and redirect against conservative IANA special-purpose ranges, enforce an end-to-end deadline, and preserve HTTP stream ownership
  • keep the feature disabled by default and document DNS rebinding/proxy limitations

Fixes #7951

Testing

  • mix format --check-formatted on all changed Elixir/config/test files
  • focused safety, reverse proxy, classifier, and ingestion suites: 76 tests, 0 failures
  • full suite as an unprivileged user with mix test --max-cases 4: 5155 tests, 0 failures, 3 excluded, 5 skipped
  • mix analyze (new files clean; existing repository findings remain)
## Summary - add opt-in ingestion-time MIME detection for generic ActivityStreams `Document` attachments - share bounded image classification with the reverse proxy - validate every destination and redirect against conservative IANA special-purpose ranges, enforce an end-to-end deadline, and preserve HTTP stream ownership - keep the feature disabled by default and document DNS rebinding/proxy limitations Fixes #7951 ## Testing - `mix format --check-formatted` on all changed Elixir/config/test files - focused safety, reverse proxy, classifier, and ingestion suites: 76 tests, 0 failures - full suite as an unprivileged user with `mix test --max-cases 4`: 5155 tests, 0 failures, 3 excluded, 5 skipped - `mix analyze` (new files clean; existing repository findings remain)
fix(activitypub): detect extensionless document images
All checks were successful
ci/woodpecker/pr/changelog Pipeline was successful
Details
ci/woodpecker/pr/lint Pipeline was successful
Details
ci/woodpecker/pr/unit-testing-elixir-1.15 Pipeline was successful
Details
ci/woodpecker/pr/unit-testing-elixir-1.19 Pipeline was successful
Details
0ae5714fcc
Merge branch 'develop' into fix/issue-7951-extensionless-document-images
All checks were successful
ci/woodpecker/pr/changelog Pipeline was successful
Details
ci/woodpecker/pr/lint Pipeline was successful
Details
ci/woodpecker/pr/unit-testing-elixir-1.15 Pipeline was successful
Details
ci/woodpecker/pr/unit-testing-elixir-1.19 Pipeline was successful
Details
opencode/review OpenCode execution completed
Details
8a8513dca8
lambadalambda commented 2026-08-04 09:34:37 +00:00
Author
Owner
Copy link

/oc review

/oc review
opencode-bot commented 2026-08-04 09:45:15 +00:00
Owner
Copy link

OpenCode Review

Execution completed for pleroma/pleroma at 8a8513dca851 (job #16).

Findings

SSRF: DNS validation is not pinned to the connection (TOCTOU) — Medium

lib/pleroma/http/safe_stream.ex:99-126, lib/pleroma/web/activity_pub/attachment_classifier.ex:65-82

SafeStream.validate_url/3 resolves the hostname and verifies that every returned address is outside IANA special-purpose ranges, but the resolved addresses are never handed to the HTTP client. Client.request/5 is invoked with the original URL, so Hackney/Gun performs a second, independent resolution. An attacker who controls DNS for an attachment hostname can return a public address during validation and a link-local/private address (e.g. 169.254.169.254) during the actual connect, bypassing the guard entirely. This is the classic DNS-rebinding SSRF and the feature expands the attack surface to anyone who can deliver a Create to the inbox.

The PR is honest about this in config/description.exs, docs/configuration/cheatsheet.md, and the changelog, and the feature is opt-in (ingestion_content_type_sniffing: false by default in config/config.exs). Still, operators enabling it are effectively getting “partial” SSRF protection: the check defeats direct literals and hostnames that resolve only to private ranges, but not rebinding. Consider pinning the connection to a validated address (e.g. Hackney’s :ip/Gun’s connection options with correct SNI) or, at minimum, surfacing this limitation more prominently than a config description string.

SSRF: configured HTTP proxies bypass validation entirely — Low

lib/pleroma/web/activity_pub/attachment_classifier.ex:66-74

classify/2 builds request opts from [:media_proxy, :proxy_opts][:http], which typically carries the deployment’s outbound proxy settings. When a proxy is configured, the proxy — not Pleroma — resolves the hostname, so SafeStream.validate_url/3’s address checks are dead weight for those deployments. This is acknowledged in the docs but worth a code comment at the call site, since an operator reading only the classifier would reasonably assume the SSRF guard always applies.

Synchronous, blocking fetches on the inbox path — Low

lib/pleroma/web/activity_pub/transmogrifier.ex:694, lib/pleroma/web/activity_pub/attachment_classifier.ex:20-34

classify_attachments/1 runs inline in handle_incoming_normalized/2 for Create activities and can issue up to @max_candidates (4) sequential outbound requests, all sharing a single 5 s monotonic deadline. On instances receiving a high volume of remote Creates whose attachments lack a media type, this directly stalls federator workers. The deadline and candidate cap bound the worst case, and the test in fedidev_fun_attachments_test.exs:111 usefully asserts refute Repo.in_transaction?() so the fetches cannot accidentally hold a DB transaction, but the synchronous coupling to the federator is still a throughput risk worth documenting for operators who enable the flag.

Tesla.close/1 with fin: true can double-release — Low

lib/pleroma/reverse_proxy/client/tesla.ex:56-59, 81-83

Pleroma.ReverseProxy.Client.Tesla.stream_body/1 already calls ConnectionPool.release_stream/2 and returns :done when invoked on a %{fin: true} client. The new close/1 clause for %{fin: true} calls release_stream again. Today no caller reaches that path after :done (SafeStream stops calling close once it sees :done, and ReverseProxy doesn’t close on success), but the contract is now fragile: any future caller that closes after a completed stream will release twice. release_stream → remove_client(true) decrements the client’s reference count, so a double release can prematurely drop the connection’s bookkeeping. Either drop the fin: true state after releasing in stream_body/1, or make close/1 a no-op once the stream has been released (e.g. via a released flag).

SafeStream requests carry no User-Agent — Low

lib/pleroma/http/safe_stream.ex:61-67, 118-126, compare lib/pleroma/reverse_proxy.ex:459-466

AttachmentClassifier only sends range and accept-encoding headers. The ReverseProxy codebase uniformly injects Pleroma.Application.user_agent/0; SafeStream does not, so the sniff request is anonymous and some origins (CDNs, WAFs) will 403/429 the fetch, silently degrading classification with no log.

Sniffing may classify image/svg+xml, later served inline — Low

lib/pleroma/media_type.ex:26-29, lib/pleroma/reverse_proxy.ex:87-102

MediaType.sniff_image/1 accepts any image/* MIME from Majic, and image/svg+xml is in @inline_content_types, so an extensionless Document that sniffs as SVG would be stored as an image and could be proxied inline. SVGs can carry scripts; embedding via <img> neutralizes script execution in modern browsers, but if the client ever renders the URL via <embed>/<object>/direct navigation, the sniffing path becomes an XSS amplifier. Consider restricting sniffing to a safe subset (e.g. jpeg/png/gif/webp/avif) the way @inline_content_types is already curated.

Update activities are not covered — Low

lib/pleroma/web/activity_pub/transmogrifier.ex:557-567, 675-701

classify_attachments/1 is wired only into the Create clause. Update activities for Note/Article objects (line 714) skip it, so an existing object whose attachment is later replaced with an extensionless Document image will never be reclassified even when the flag is on. If the intent is “detect extensionless images on ingest,” Updates are a real ingest path and should be handled (or the omission documented).

Minor / maintainability

  • @sniff_bytes 8 * 1024 is redefined in both lib/pleroma/media_type.ex:6 and lib/pleroma/web/activity_pub/attachment_classifier.ex:10; the classifier should reference MediaType’s constant so the byte budget stays in sync. (lib/pleroma/reverse_proxy.ex was correctly refactored to drop its own copy.)
  • SafeStream.validate_url/2 is @spec-public and @callback-absent purely to support safe_stream_test.exs. If it is meant to be part of the API, document it; otherwise prefer keeping it private and testing through fetch_prefix/3.
  • MediaType.sniff_image/1 always copies up to 8 KiB via :binary.copy/1; SafeStream already bounds the prefix to the same 8 KiB, so the copy is harmless but redundant in that path. Fine as defence-in-depth, just noting it.
  • The redirect-following header/2 helper downcases header names for comparison but the rest of the codebase tends to normalise headers at the boundary; make sure the underlying client returns lower-cased header names (ReverseProxy does this via downcase_headers/1, but SafeStream consumes the raw client response directly).

Positive aspects

  • Conservative, IANA-aligned blocklists for both IPv4 and IPv6, with mixed-result DNS rejection (safe_stream_test.exs:172-177), redirect re-validation (safe_stream_test.exs:52-64), credential stripping on cross-origin redirects (safe_stream_test.exs:89-115), and rejection of non-http(s) schemes/userinfo.
  • Shared monotonic deadline propagated through DNS, connect, body reads, and cleanup, with within_deadline/2 spawning and killing a worker so adapter-level hangs cannot exceed the budget.
  • Graceful degradation: every failure mode (:private_address, :timeout, :empty_body, :invalid_chunk, non-image sniff) leaves the attachment unchanged (attachment_classifier.ex:76-81).
  • Good unit and integration coverage, including the clever second-ingest short-circuit that verifies the mock is not re-invoked (fedidev_fun_attachments_test.exs:125), and a guard asserting the classifier never runs inside a transaction.
  • Refactor correctly extracts generic?/1 and sniff_image/1 into Pleroma.MediaType, and the existing ReverseProxy behavior is preserved (including its rescue/catch wrapper).

Overall

A well-structured, opt-in fix for issue #7951 with thoughtful timeout/deadline handling, conservative IP filtering, and solid tests. The headline concern is SSRF: the DNS validation is a TOCTOU check that does not pin the connection, so DNS-rebinding attackers and proxy-fronted deployments get little real protection — which the PR admits in docs but which operators may underweight. Secondary issues are a fragile close(fin: true) contract, blocking fetches on the federator path, missing User-Agent, and SVG-as-image classification. None block merge for an explicitly WIP, default-off feature, but the SSRF caveat and the close/1 double-release risk should be addressed (or explicitly deferred) before this leaves WIP.

<!-- opencode-review:v1:68c95eb30f9c3f919008e903585c9e9f --> <!-- opencode-reviewed-head:8a8513dca851ab779bb36a5c5c20022cb50f1dcc --> ## OpenCode Review _Execution completed for `pleroma/pleroma` at `8a8513dca851` (job #16)._ ## Findings ### SSRF: DNS validation is not pinned to the connection (TOCTOU) — Medium `lib/pleroma/http/safe_stream.ex:99-126`, `lib/pleroma/web/activity_pub/attachment_classifier.ex:65-82` `SafeStream.validate_url/3` resolves the hostname and verifies that every returned address is outside IANA special-purpose ranges, but the resolved addresses are never handed to the HTTP client. `Client.request/5` is invoked with the original URL, so Hackney/Gun performs a *second*, independent resolution. An attacker who controls DNS for an attachment hostname can return a public address during validation and a link-local/private address (e.g. `169.254.169.254`) during the actual connect, bypassing the guard entirely. This is the classic DNS-rebinding SSRF and the feature expands the attack surface to anyone who can deliver a Create to the inbox. The PR is honest about this in `config/description.exs`, `docs/configuration/cheatsheet.md`, and the changelog, and the feature is opt-in (`ingestion_content_type_sniffing: false` by default in `config/config.exs`). Still, operators enabling it are effectively getting “partial” SSRF protection: the check defeats direct literals and hostnames that resolve only to private ranges, but not rebinding. Consider pinning the connection to a validated address (e.g. Hackney’s `:ip`/Gun’s connection options with correct SNI) or, at minimum, surfacing this limitation more prominently than a config description string. ### SSRF: configured HTTP proxies bypass validation entirely — Low `lib/pleroma/web/activity_pub/attachment_classifier.ex:66-74` `classify/2` builds request opts from `[:media_proxy, :proxy_opts][:http]`, which typically carries the deployment’s outbound proxy settings. When a proxy is configured, the proxy — not Pleroma — resolves the hostname, so `SafeStream.validate_url/3`’s address checks are dead weight for those deployments. This is acknowledged in the docs but worth a code comment at the call site, since an operator reading only the classifier would reasonably assume the SSRF guard always applies. ### Synchronous, blocking fetches on the inbox path — Low `lib/pleroma/web/activity_pub/transmogrifier.ex:694`, `lib/pleroma/web/activity_pub/attachment_classifier.ex:20-34` `classify_attachments/1` runs inline in `handle_incoming_normalized/2` for Create activities and can issue up to `@max_candidates` (4) sequential outbound requests, all sharing a single 5 s monotonic deadline. On instances receiving a high volume of remote Creates whose attachments lack a media type, this directly stalls federator workers. The deadline and candidate cap bound the worst case, and the test in `fedidev_fun_attachments_test.exs:111` usefully asserts `refute Repo.in_transaction?()` so the fetches cannot accidentally hold a DB transaction, but the synchronous coupling to the federator is still a throughput risk worth documenting for operators who enable the flag. ### `Tesla.close/1` with `fin: true` can double-release — Low `lib/pleroma/reverse_proxy/client/tesla.ex:56-59, 81-83` `Pleroma.ReverseProxy.Client.Tesla.stream_body/1` already calls `ConnectionPool.release_stream/2` and returns `:done` when invoked on a `%{fin: true}` client. The new `close/1` clause for `%{fin: true}` calls `release_stream` again. Today no caller reaches that path after `:done` (SafeStream stops calling `close` once it sees `:done`, and ReverseProxy doesn’t close on success), but the contract is now fragile: any future caller that closes after a completed stream will release twice. `release_stream` → `remove_client(true)` decrements the client’s reference count, so a double release can prematurely drop the connection’s bookkeeping. Either drop the `fin: true` state after releasing in `stream_body/1`, or make `close/1` a no-op once the stream has been released (e.g. via a `released` flag). ### SafeStream requests carry no User-Agent — Low `lib/pleroma/http/safe_stream.ex:61-67, 118-126`, compare `lib/pleroma/reverse_proxy.ex:459-466` `AttachmentClassifier` only sends `range` and `accept-encoding` headers. The ReverseProxy codebase uniformly injects `Pleroma.Application.user_agent/0`; SafeStream does not, so the sniff request is anonymous and some origins (CDNs, WAFs) will 403/429 the fetch, silently degrading classification with no log. ### Sniffing may classify `image/svg+xml`, later served inline — Low `lib/pleroma/media_type.ex:26-29`, `lib/pleroma/reverse_proxy.ex:87-102` `MediaType.sniff_image/1` accepts any `image/*` MIME from Majic, and `image/svg+xml` is in `@inline_content_types`, so an extensionless Document that sniffs as SVG would be stored as an image and could be proxied inline. SVGs can carry scripts; embedding via `<img>` neutralizes script execution in modern browsers, but if the client ever renders the URL via `<embed>`/`<object>`/direct navigation, the sniffing path becomes an XSS amplifier. Consider restricting sniffing to a safe subset (e.g. jpeg/png/gif/webp/avif) the way `@inline_content_types` is already curated. ### Update activities are not covered — Low `lib/pleroma/web/activity_pub/transmogrifier.ex:557-567, 675-701` `classify_attachments/1` is wired only into the `Create` clause. `Update` activities for Note/Article objects (line 714) skip it, so an existing object whose attachment is later replaced with an extensionless Document image will never be reclassified even when the flag is on. If the intent is “detect extensionless images on ingest,” Updates are a real ingest path and should be handled (or the omission documented). ### Minor / maintainability - `@sniff_bytes 8 * 1024` is redefined in both `lib/pleroma/media_type.ex:6` and `lib/pleroma/web/activity_pub/attachment_classifier.ex:10`; the classifier should reference `MediaType`’s constant so the byte budget stays in sync. (`lib/pleroma/reverse_proxy.ex` was correctly refactored to drop its own copy.) - `SafeStream.validate_url/2` is `@spec`-public and `@callback`-absent purely to support `safe_stream_test.exs`. If it is meant to be part of the API, document it; otherwise prefer keeping it private and testing through `fetch_prefix/3`. - `MediaType.sniff_image/1` always copies up to 8 KiB via `:binary.copy/1`; SafeStream already bounds the prefix to the same 8 KiB, so the copy is harmless but redundant in that path. Fine as defence-in-depth, just noting it. - The redirect-following `header/2` helper downcases header names for comparison but the rest of the codebase tends to normalise headers at the boundary; make sure the underlying client returns lower-cased header names (ReverseProxy does this via `downcase_headers/1`, but SafeStream consumes the raw client response directly). ### Positive aspects - Conservative, IANA-aligned blocklists for both IPv4 and IPv6, with mixed-result DNS rejection (`safe_stream_test.exs:172-177`), redirect re-validation (`safe_stream_test.exs:52-64`), credential stripping on cross-origin redirects (`safe_stream_test.exs:89-115`), and rejection of non-http(s) schemes/userinfo. - Shared monotonic deadline propagated through DNS, connect, body reads, and cleanup, with `within_deadline/2` spawning and killing a worker so adapter-level hangs cannot exceed the budget. - Graceful degradation: every failure mode (`:private_address`, `:timeout`, `:empty_body`, `:invalid_chunk`, non-image sniff) leaves the attachment unchanged (`attachment_classifier.ex:76-81`). - Good unit and integration coverage, including the clever second-ingest short-circuit that verifies the mock is not re-invoked (`fedidev_fun_attachments_test.exs:125`), and a guard asserting the classifier never runs inside a transaction. - Refactor correctly extracts `generic?/1` and `sniff_image/1` into `Pleroma.MediaType`, and the existing ReverseProxy behavior is preserved (including its rescue/catch wrapper). ## Overall A well-structured, opt-in fix for issue #7951 with thoughtful timeout/deadline handling, conservative IP filtering, and solid tests. The headline concern is SSRF: the DNS validation is a TOCTOU check that does not pin the connection, so DNS-rebinding attackers and proxy-fronted deployments get little real protection — which the PR admits in docs but which operators may underweight. Secondary issues are a fragile `close(fin: true)` contract, blocking fetches on the federator path, missing User-Agent, and SVG-as-image classification. None block merge for an explicitly WIP, default-off feature, but the SSRF caveat and the `close/1` double-release risk should be addressed (or explicitly deferred) before this leaves WIP.
All checks were successful
ci/woodpecker/pr/changelog Pipeline was successful
Details
ci/woodpecker/pr/lint Pipeline was successful
Details
ci/woodpecker/pr/unit-testing-elixir-1.15 Pipeline was successful
Details
ci/woodpecker/pr/unit-testing-elixir-1.19 Pipeline was successful
Details
opencode/review OpenCode execution completed
Details
This pull request has changes conflicting with the target branch.
  • lib/pleroma/reverse_proxy.ex
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin fix/issue-7951-extensionless-document-images:fix/issue-7951-extensionless-document-images
git switch fix/issue-7951-extensionless-document-images

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 fix/issue-7951-extensionless-document-images
git switch fix/issue-7951-extensionless-document-images
git rebase develop
git switch develop
git merge --ff-only fix/issue-7951-extensionless-document-images
git switch fix/issue-7951-extensionless-document-images
git rebase develop
git switch develop
git merge --no-ff fix/issue-7951-extensionless-document-images
git switch develop
git merge --squash fix/issue-7951-extensionless-document-images
git switch develop
git merge --ff-only fix/issue-7951-extensionless-document-images
git switch develop
git merge fix/issue-7951-extensionless-document-images
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!7952
No description provided.