[bug] Media Proxy attempts to proxy ActivityPub embedded images #859

Closed
opened 2024-12-14 16:53:39 +00:00 by nopjmp · 3 comments
Contributor

Your setup

From source

Extra details

Debian Linux

Version

develop 294de939

PostgreSQL version

15

What were you trying to do?

Look at a post that has an embedded base64 png.

What did you expect to happen?

The frontend loads the embedded image and does not proxy to the backend.

What actually happened?

The frontend is forced to proxy to the backend as the media is encoded into a media proxy url.

Logs

```
CAYAAAAfFcSJAAAAC0lEQVQIW2NgAAIAAAUAAR4f7BQAAAAASUVORK5CYII=" failed: :fetch_error
Dec 14 16:34:30 thewired.wtf mix[226431]: 16:34:30.685 request_id=GBEYUbLDUINy19wACDGC [error] Failed to fetch data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVQIW2NgAAIAAAUAAR4f7BQAAAAASUVORK5CYII=: %ArgumentError{message: "invalid scheme \"data\" for url: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVQIW2NgAAIAAAUAAR4f7BQAAAAASUVORK5CYII="}
```

Severity

I can manage

Have you searched for this issue?

  • I have double-checked and have not found this issue mentioned anywhere.
### Your setup From source ### Extra details Debian Linux ### Version develop 294de939 ### PostgreSQL version 15 ### What were you trying to do? Look at a post that has an embedded base64 png. ### What did you expect to happen? The frontend loads the embedded image and does not proxy to the backend. ### What actually happened? The frontend is forced to proxy to the backend as the media is encoded into a media proxy url. ### Logs ````shell ``` CAYAAAAfFcSJAAAAC0lEQVQIW2NgAAIAAAUAAR4f7BQAAAAASUVORK5CYII=" failed: :fetch_error Dec 14 16:34:30 thewired.wtf mix[226431]: 16:34:30.685 request_id=GBEYUbLDUINy19wACDGC [error] Failed to fetch data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVQIW2NgAAIAAAUAAR4f7BQAAAAASUVORK5CYII=: %ArgumentError{message: "invalid scheme \"data\" for url: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVQIW2NgAAIAAAUAAR4f7BQAAAAASUVORK5CYII="} ``` ```` ### Severity I can manage ### Have you searched for this issue? - [x] I have double-checked and have not found this issue mentioned anywhere.
nopjmp added the
bug
label 2024-12-14 16:53:39 +00:00
Author
Contributor

I've made the following modifications, but I'm not sure this will accurately cover all use cases as I'm still very new to the Akkoma / Pleroma code base. I just switched over to source (Elixir 0.14.0 and OTP 25) so I can start to fix issues I've encountered on my instance and attempt to contribute them back.

diff --git a/lib/pleroma/web/media_proxy.ex b/lib/pleroma/web/media_proxy.ex
index 61b6f2a62..afe8efe9b 100644
--- a/lib/pleroma/web/media_proxy.ex
+++ b/lib/pleroma/web/media_proxy.ex
@@ -52,11 +52,11 @@ def url(url) do

   @spec url_proxiable?(String.t()) :: boolean()
   def url_proxiable?(url) do
-    not local?(url) and not whitelisted?(url) and not blocked?(url)
+    not embedded?(url) and not local?(url) and not whitelisted?(url) and not blocked?(url)
   end

   def preview_url(url, preview_params \\ []) do
-    if preview_enabled?() do
+    if preview_enabled?() and not embedded?(url) do
       encode_preview_url(url, preview_params)
     else
       url(url)
@@ -71,6 +71,8 @@ def preview_enabled?, do: enabled?() and !!Config.get([:media_preview_proxy, :en

   def local?(url), do: String.starts_with?(url, Endpoint.url())

+  def embedded?(url), do: String.starts_with?(url, "data")
+
   def whitelisted?(url) do
     %{host: domain} = URI.parse(url)
I've made the following modifications, but I'm not sure this will accurately cover all use cases as I'm still very new to the Akkoma / Pleroma code base. I just switched over to source (Elixir 0.14.0 and OTP 25) so I can start to fix issues I've encountered on my instance and attempt to contribute them back. ```diff diff --git a/lib/pleroma/web/media_proxy.ex b/lib/pleroma/web/media_proxy.ex index 61b6f2a62..afe8efe9b 100644 --- a/lib/pleroma/web/media_proxy.ex +++ b/lib/pleroma/web/media_proxy.ex @@ -52,11 +52,11 @@ def url(url) do @spec url_proxiable?(String.t()) :: boolean() def url_proxiable?(url) do - not local?(url) and not whitelisted?(url) and not blocked?(url) + not embedded?(url) and not local?(url) and not whitelisted?(url) and not blocked?(url) end def preview_url(url, preview_params \\ []) do - if preview_enabled?() do + if preview_enabled?() and not embedded?(url) do encode_preview_url(url, preview_params) else url(url) @@ -71,6 +71,8 @@ def preview_enabled?, do: enabled?() and !!Config.get([:media_preview_proxy, :en def local?(url), do: String.starts_with?(url, Endpoint.url()) + def embedded?(url), do: String.starts_with?(url, "data") + def whitelisted?(url) do %{host: domain} = URI.parse(url) ```
Member

Thanks for your report and initial patch!

The diff looks generally good to me. I’m wondering why preview_url doesn't (already) use url_proxiable? but this is a pre-existing thing and I’m not too familiar with media proxy details.
To avoid mistakenly matching e.g. local, relative URLs, the embed? check should probably test for data: with colon at the end. Perhaps it might also make more sense to check against an allowed set of fetchable protocols instead (only http and https atm iinm).

You can run mix format to automatically adjust code style and run the testsuite with mix test || mix test --failed (the second rerun is to deal with a few flaky tests). If this doesn't reveal any problems either, it’s probably good and you can open a PR.

Thanks for your report and initial patch! The diff looks generally good to me. I’m wondering why `preview_url` doesn't (already) use `url_proxiable?` but this is a pre-existing thing and I’m not too familiar with media proxy details. To avoid mistakenly matching e.g. local, relative URLs, the `embed?` check should probably test for `data:` with colon at the end. Perhaps it might also make more sense to check against an allowed set of fetchable protocols instead *(only `http` and `https` atm iinm)*. You can run `mix format` to automatically adjust code style and run the testsuite with `mix test || mix test --failed` *(the second rerun is to deal with a few flaky tests)*. If this doesn't reveal any problems either, it’s probably good and you can open a PR.
Author
Contributor

I'll setup my local environment so I can avoid mixing the "production" setup with a few of your branches I've mixed in to avoid federation issues with IceShrimp.NET and then make a PR.

I'll setup my local environment so I can avoid mixing the "production" setup with a few of your branches I've mixed in to avoid federation issues with IceShrimp.NET and then make a PR.
Sign in to join this conversation.
No milestone
No project
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: AkkomaGang/akkoma#859
No description provided.