Rewrite images to use local proxy #4035

Nutomic · 2023-10-11T10:00:01Z

I decided to play around with markdown handling to see how we can rewrite image and link urls. Turns out its pretty simple. So far it does this:

Add rel=nofollow to all links to discourage spammers. Note that this wont affect lemmy-ui or other frontends as they do their own markdown rendering. It will only affect emails, RSS feeds and different federated platforms.
Rewrite all images to https://lemmy-alpha/api/v3/image_proxy?url={url} so that users dont connect directly to remote servers.

This PR doesnt have any breaking changes so it can be merged whenever.

Todo:

dessalines · 2023-10-23T16:29:37Z

crates/routes/src/image_proxy.rs

+  url: String,
+}
+
+async fn image_proxy(


@asonix would using the lemmy-backend to proxy picture requests be a better approach, or maybe waiting for pictrs v.50 which has picture proxying, and using those pictrs routes? Any pros/cons to either?

proxying images through lemmy is fine, but it means the original server must be online and capable of serving the image for each request. using pict-rs' proxy method in 0.5 will cache the proxied image and reduce load on the original server

I think doing this initial proxy work now probably makes sense, and using pict-rs' proxying in the future can be made a lower priority (and give more time to folks to upgrade to 0.5 when it releases before you start depending on 0.5 endpoints)

In that case @Nutomic just add a TODO somewhere that we can remove this once pictrs 0.5 is released.

Sounds good, adding comment.

Changed it to use pictrs for proxying. Also added a config option for image proxy which is disabled by default, with comment noting that pictrs 0.5 is required.

dessalines · 2023-10-23T16:56:13Z

One other concern I have that makes me think image proxying should probably be done in front ends: cross-posting.

If a link URL points to an image on instance1.tld/...pictrs/, then its currently possible to see all its cross-posts across the lemmyverse. That might not work correctly if we rewrite all image links.

IE so maybe instead of actually rewriting links, we just provide the image_proxy endpoint, and let front ends use it.

crates/utils/src/utils/markdown/mod.rs

Nutomic · 2023-10-24T08:41:55Z

@dessalines If multiple posts have the same url, those will all be rewritten to the identical image_proxy url so crossposts will still work. If this logic is handled in the frontend then each one will have to reimplement the same logic, doesnt make sense.

Nutomic · 2023-10-26T09:41:53Z

crates/utils/src/settings/structs.rs

@@ -65,7 +65,15 @@ pub struct PictrsConfig {

  /// Cache remote images
  #[default(true)]
-  pub cache_remote_images: bool,
+  pub cache_remote_thumbnails: bool,


This setting might be unnecessary now. However the file request.rs where its used is quite the mess, not sure when I can get around to cleaning it up (it needs almost a complete rewrite).

Nutomic · 2023-10-26T10:43:48Z

Ready to review now.

Edit: Did some testing using local docker setup and its working as expected

dessalines · 2024-01-05T17:26:24Z

crates/api_common/src/request.rs

+
+  if !pictrs_config.cache_external_link_previews {
+    return Ok(
+      proxy_image_link(


This block here is confusing now. Maybe we should remove cache_external_link_previews entirely now, and just leave image_proxy?

dessalines

The cache_external_link_previews and image_proxy seem like two bools that refer to the same thing now then.

I'm tracing down every usage of RemoteImage::create, and sometimes it uses the first setting, sometimes the 2nd. We should simplify and get rid of one of them.

kroese · 2024-01-05T19:34:10Z

@dessalines The cache_external_link_previews setting has a bit confusing name, I wish it was called store_thumbnails or something like that. Because that would make it much more clear what the setting does.

But it doesn't refer to the same thing, its purpose is to specify wether you want to store thumbnails locally (in pict-rs), or just hotlink to external images. And the image_proxy setting specifies if you want to use a proxy for external thumbnails.

In my case I dont want the images to be proxied and I also dont want to store them locally. So a single setting would not be enough.

dessalines · 2024-01-05T19:39:33Z

@kroese @Nutomic in that case they seem exclusive then, and we have 3 options, and should use an enum rather than 2 bools:

Use the pictrs proxy for all external images
Save all external images (cache is a bad term anyway)
Directly link to all external images

Some names could be {UsePictrsProxy, SaveInPictrs, UseDirectLink}

kroese · 2024-01-05T19:45:52Z

Yes they are exclusive, because the value of image_proxy has no meaning until you set cache_external_link_previews to false. So I agree an enum would make more sense.

Also in the current release there is a bug that cache_external_link_previews is only applied to thumbnails coming from federated posts. But it should also apply to thumbnails generated from the og:image metadata for local posts. I hope this pullrequests solves that issue.

* Extracting opengraph_data to its own type. * A few additions for markdown-link-rule. --------- Co-authored-by: Nutomic <me@nutomic.com>

Nutomic · 2024-01-08T11:39:58Z

Makes sense, Ive converted it to an enum now. You can see it in config/defaults.hjson. For now Im keeping the existing behaviour as default to avoid breaking changes in a minor version. But if the proxying works well might be able to remove StoreLinkPreviews in 0.20 and make ProxyAllImages the default.

kroese · 2024-01-08T11:54:05Z

I still think the naming is not as clear as it could be. The setting is called image_mode but there are other types of images (inside comments, avatars, banners) to which the setting doesn't apply. Also values like StoreLinkPreviews are way more verbose than needed.

If it was my decision I would just call the setting thumbnails and the values store, link and proxy for example.

Nutomic · 2024-01-08T14:01:49Z

The new image proxying from this PR applies to all images, including avatars, markdown embedded images etc. However the old storing of preview images only applies to post urls. So I think the naming makes sense.

dessalines

Nice! BTW pictrs v0.5.0 is released now.

Did you want this to go in the upcoming release, or leave it for later? My main concern is I want our release notes to give full instructions on how to upgrade pictrs.

Nutomic · 2024-01-10T10:15:44Z

Should be fine to include it as it doesnt change the main behaviour unless you add the config option. For pictrs we can simply link to the readme.

Nutomic · 2024-01-10T14:56:36Z

Added cache_external_link_previews config option back in to avoid breaking changes.

dessalines · 2024-01-23T23:44:56Z

LGTM, but lets have @phiresky take a look before merging.

And when you can, please add some detailed instructions for LemmyNet/lemmy-ansible#213 , because I'm sure this will cause some issues unless people know the proper way to upgrade pictrs seamlessly.

Nutomic added 3 commits October 11, 2023 11:49

Add markdown rule to add rel=nofollow for all links

c2584d3

Add markdown image rule to add local image proxy (fixes #1036)

3be2a55

comments

44d8168

Nutomic force-pushed the markdown-link-rule branch from def4f2d to 44d8168 Compare October 11, 2023 10:11

Nutomic changed the title ~~Markdown link rule~~ Rewrite images to use local proxy, add rel=nofollow to links Oct 13, 2023

Nutomic added 2 commits October 23, 2023 15:40

Merge branch 'main' into markdown-link-rule

650e3a7

rewrite markdown image links working

fc66bad

dessalines reviewed Oct 23, 2023

View reviewed changes

Nutomic commented Oct 24, 2023

View reviewed changes

crates/utils/src/utils/markdown/mod.rs Show resolved Hide resolved

add comment

540560b

Nutomic added 5 commits October 24, 2023 13:27

perform markdown image processing in api/apub receivers

aaf3833

clippy

89976b8

add db table to validate proxied links

ef79422

rewrite link fields for avatar, banner etc

ae96d86

sql fmt

7306153

Nutomic force-pushed the markdown-link-rule branch from 77d0af5 to 7306153 Compare October 24, 2023 15:10

proxy links received over federation

aa49a1b

Nutomic mentioned this pull request Oct 25, 2023

Create API tests for image endpoints #4105

Closed

4 tasks

Nutomic added 3 commits October 25, 2023 12:57

Merge branch 'main' into markdown-link-rule

f057abf

add config option

986913d

undo post.url rewriting, move http route definition

388eb42

Nutomic commented Oct 26, 2023

View reviewed changes

Nutomic changed the title ~~Rewrite images to use local proxy, add rel=nofollow to links~~ Rewrite images to use local proxy Oct 26, 2023

Nutomic added 2 commits October 26, 2023 12:32

add tests

98b5746

proxy images through pictrs

45f5448

Nutomic marked this pull request as ready for review October 26, 2023 10:39

Nutomic requested a review from phiresky as a code owner October 26, 2023 10:39

dessalines mentioned this pull request Jan 4, 2024

cache_external_link_previews cache grows indefinitely #4328

Closed

4 tasks

clippy

1f29e72

dessalines reviewed Jan 5, 2024

View reviewed changes

dessalines and others added 3 commits January 8, 2024 11:56

Markdown link rule-dess (#4356)

3399917

* Extracting opengraph_data to its own type. * A few additions for markdown-link-rule. --------- Co-authored-by: Nutomic <me@nutomic.com>

fix setting

a88f4d6

use enum for image proxy setting

518af87

fix test configs

4fc857a

dessalines approved these changes Jan 9, 2024

View reviewed changes

Nutomic added 2 commits January 10, 2024 11:49

Merge branch 'main' into markdown-link-rule

7e5e455

add config backwards compat

81359e2

clippy

7930b57

dessalines approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'main' into markdown-link-rule

2fd7edd

Nutomic force-pushed the markdown-link-rule branch from c1323e5 to 2fd7edd Compare January 22, 2024 10:35

Nutomic added 2 commits January 25, 2024 10:42

Merge branch 'main' into markdown-link-rule

b600bc5

machete

0a6a587

dessalines merged commit e8a52d3 into main Jan 25, 2024
2 checks passed

Nutomic mentioned this pull request Jan 29, 2024

Federating/Proxying links and media #2947

Closed

Nutomic mentioned this pull request Jun 20, 2024

Instance agnostic links #2987

Closed

phiresky mentioned this pull request Aug 3, 2024

[Bug]: extract_opengraph_data tries to parse HTML on large binary URLs (images/gifs/videos) and causes CPU spikes delaying all responses #4956

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite images to use local proxy #4035

Rewrite images to use local proxy #4035

Nutomic commented Oct 11, 2023 •

edited

Loading

dessalines Oct 23, 2023

asonix Oct 23, 2023

dessalines Oct 23, 2023

Nutomic Oct 24, 2023

Nutomic Oct 26, 2023 •

edited

Loading

dessalines commented Oct 23, 2023 •

edited

Loading

Nutomic commented Oct 24, 2023

Nutomic Oct 26, 2023

Nutomic commented Oct 26, 2023 •

edited

Loading

dessalines Jan 5, 2024

dessalines left a comment

kroese commented Jan 5, 2024

dessalines commented Jan 5, 2024 •

edited

Loading

kroese commented Jan 5, 2024

Nutomic commented Jan 8, 2024

kroese commented Jan 8, 2024

Nutomic commented Jan 8, 2024

dessalines left a comment

Nutomic commented Jan 10, 2024

Nutomic commented Jan 10, 2024

dessalines commented Jan 23, 2024 •

edited

Loading

Rewrite images to use local proxy #4035

Rewrite images to use local proxy #4035

Conversation

Nutomic commented Oct 11, 2023 • edited Loading

dessalines Oct 23, 2023

Choose a reason for hiding this comment

asonix Oct 23, 2023

Choose a reason for hiding this comment

dessalines Oct 23, 2023

Choose a reason for hiding this comment

Nutomic Oct 24, 2023

Choose a reason for hiding this comment

Nutomic Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

dessalines commented Oct 23, 2023 • edited Loading

Nutomic commented Oct 24, 2023

Nutomic Oct 26, 2023

Choose a reason for hiding this comment

Nutomic commented Oct 26, 2023 • edited Loading

dessalines Jan 5, 2024

Choose a reason for hiding this comment

dessalines left a comment

Choose a reason for hiding this comment

kroese commented Jan 5, 2024

dessalines commented Jan 5, 2024 • edited Loading

kroese commented Jan 5, 2024

Nutomic commented Jan 8, 2024

kroese commented Jan 8, 2024

Nutomic commented Jan 8, 2024

dessalines left a comment

Choose a reason for hiding this comment

Nutomic commented Jan 10, 2024

Nutomic commented Jan 10, 2024

dessalines commented Jan 23, 2024 • edited Loading

Nutomic commented Oct 11, 2023 •

edited

Loading

Nutomic Oct 26, 2023 •

edited

Loading

dessalines commented Oct 23, 2023 •

edited

Loading

Nutomic commented Oct 26, 2023 •

edited

Loading

dessalines commented Jan 5, 2024 •

edited

Loading

dessalines commented Jan 23, 2024 •

edited

Loading