From d793d803b0a96fa03936a55ad8aa535eb4267ade Mon Sep 17 00:00:00 2001 From: Felix Ableitner Date: Thu, 4 Jan 2024 16:10:18 +0100 Subject: [PATCH] address review --- api_tests/src/image.spec.ts | 39 +++++++++++++++++++--- api_tests/src/post.spec.ts | 18 +++++++++- api_tests/src/shared.ts | 9 +---- crates/api/src/local_user/save_settings.rs | 3 +- crates/api_common/src/request.rs | 10 +++--- crates/api_common/src/utils.rs | 15 +++++++-- crates/api_crud/src/post/create.rs | 2 +- crates/apub/src/objects/post.rs | 2 +- crates/db_schema/src/source/images.rs | 2 ++ docker/lemmy.hjson | 4 --- 10 files changed, 75 insertions(+), 29 deletions(-) diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index 7587d1374..1926fee33 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -129,7 +129,7 @@ test("Purge post, linked image removed", async () => { expect(content2).toBe(""); }); -test("Images in remote post are proxied", async () => { +test("Images in remote post are proxied if setting enabled", async () => { let user = await registerUser(beta, betaUrl); let community = await createCommunity(gamma); @@ -137,7 +137,6 @@ test("Images in remote post are proxied", async () => { image: Buffer.from("test"), }; const upload = await user.uploadImage(upload_form); - console.log(upload); let post = await createPost( gamma, community.community_view.community.id, @@ -145,8 +144,8 @@ test("Images in remote post are proxied", async () => { "![](http://example.com/image2.png)", ); expect(post.post_view.post).toBeDefined(); + // remote image gets proxied after upload - console.log(post.post_view.post); expect( post.post_view.post.url?.startsWith( "http://lemmy-gamma:8561/api/v3/image_proxy?url", @@ -160,7 +159,7 @@ test("Images in remote post are proxied", async () => { let epsilonPost = await resolvePost(epsilon, post.post_view.post); expect(epsilonPost.post).toBeDefined(); - console.log(epsilonPost.post); + // remote image gets proxied after federation expect( epsilonPost.post!.post.url?.startsWith( @@ -173,3 +172,35 @@ test("Images in remote post are proxied", async () => { ), ).toBeTruthy(); }); + +test("No image proxying if setting is disabled", async () => { + let user = await registerUser(beta, betaUrl); + let community = await createCommunity(alpha); + + const upload_form: UploadImage = { + image: Buffer.from("test"), + }; + const upload = await user.uploadImage(upload_form); + let post = await createPost( + alpha, + community.community_view.community.id, + upload.url, + "![](http://example.com/image2.png)", + ); + expect(post.post_view.post).toBeDefined(); + + // remote image doesnt get proxied after upload + expect( + post.post_view.post.url?.startsWith("http://127.0.0.1:8551/pictrs/image/"), + ).toBeTruthy(); + expect(post.post_view.post.body).toBe("![](http://example.com/image2.png)"); + + let gammaPost = await resolvePost(delta, post.post_view.post); + expect(gammaPost.post).toBeDefined(); + + // remote image doesnt get proxied after federation + expect( + gammaPost.post!.post.url?.startsWith("http://127.0.0.1:8551/pictrs/image/"), + ).toBeTruthy(); + expect(gammaPost.post!.post.body).toBe("![](http://example.com/image2.png)"); +}); diff --git a/api_tests/src/post.spec.ts b/api_tests/src/post.spec.ts index 72029157d..20acaa28c 100644 --- a/api_tests/src/post.spec.ts +++ b/api_tests/src/post.spec.ts @@ -39,7 +39,7 @@ import { loginUser, } from "./shared"; import { PostView } from "lemmy-js-client/dist/types/PostView"; -import { ResolveObject } from "lemmy-js-client"; +import { EditSite, ResolveObject } from "lemmy-js-client"; let betaCommunity: CommunityView | undefined; @@ -72,6 +72,16 @@ function assertPostFederation(postOne?: PostView, postTwo?: PostView) { } test("Create a post", async () => { + // Setup some allowlists and blocklists + let editSiteForm: EditSite = { + allowed_instances: ["lemmy-beta"], + }; + await delta.editSite(editSiteForm); + + editSiteForm.allowed_instances = []; + editSiteForm.blocked_instances = ["lemmy-alpha"]; + await epsilon.editSite(editSiteForm); + if (!betaCommunity) { throw "Missing beta community"; } @@ -104,6 +114,12 @@ test("Create a post", async () => { await expect( resolvePost(epsilon, postRes.post_view.post), ).rejects.toStrictEqual(Error("couldnt_find_object")); + + // remove added allow/blocklists + editSiteForm.allowed_instances = []; + editSiteForm.blocked_instances = []; + await delta.editSite(editSiteForm); + await epsilon.editSite(editSiteForm); }); test("Create a post in a non-existent community", async () => { diff --git a/api_tests/src/shared.ts b/api_tests/src/shared.ts index f9580ce96..e8b6892ee 100644 --- a/api_tests/src/shared.ts +++ b/api_tests/src/shared.ts @@ -177,13 +177,6 @@ export async function setupLogins() { ]; await gamma.editSite(editSiteForm); - editSiteForm.allowed_instances = ["lemmy-beta"]; - await delta.editSite(editSiteForm); - - editSiteForm.allowed_instances = []; - editSiteForm.blocked_instances = ["lemmy-alpha"]; - await epsilon.editSite(editSiteForm); - // Create the main alpha/beta communities // Ignore thrown errors of duplicates try { @@ -528,7 +521,7 @@ export async function likeComment( export async function createCommunity( api: LemmyHttp, - name_: string = randomString(5), + name_: string = randomString(10), ): Promise { let description = "a sample description"; let form: CreateCommunity = { diff --git a/crates/api/src/local_user/save_settings.rs b/crates/api/src/local_user/save_settings.rs index 15858bcdd..79b95133e 100644 --- a/crates/api/src/local_user/save_settings.rs +++ b/crates/api/src/local_user/save_settings.rs @@ -34,11 +34,10 @@ pub async fn save_user_settings( let site_view = SiteView::read_local(&mut context.pool()).await?; let slur_regex = local_site_to_slur_regex(&site_view.local_site); - let bio = process_markdown_opt(&data.bio, &slur_regex, &context).await?; + let bio = diesel_option_overwrite(process_markdown_opt(&data.bio, &slur_regex, &context).await?); let avatar = proxy_image_link_opt_api(&data.avatar, &context).await?; let banner = proxy_image_link_opt_api(&data.banner, &context).await?; - let bio = diesel_option_overwrite(bio); let display_name = diesel_option_overwrite(data.display_name.clone()); let matrix_user_id = diesel_option_overwrite(data.matrix_user_id.clone()); let email_deref = data.email.as_deref().map(str::to_lowercase); diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index 435411f4a..fc739f480 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -49,7 +49,7 @@ pub async fn fetch_link_metadata( // https://github.com/LemmyNet/lemmy/issues/1964 let html_bytes = response.bytes().await.map_err(LemmyError::from)?.to_vec(); - let mut metadata = extract_opengraph_data(&html_bytes, url)?; + let mut metadata = extract_opengraph_data(&html_bytes, url).unwrap_or_default(); metadata.content_type = content_type.map(|c| c.to_string()); if generate_thumbnail && is_image { @@ -71,14 +71,13 @@ pub async fn fetch_link_metadata_opt( url: Option<&Url>, generate_thumbnail: bool, context: &LemmyContext, -) -> Result { - let metadata = match &url { +) -> LinkMetadata { + match &url { Some(url) => fetch_link_metadata(url, generate_thumbnail, context) .await .unwrap_or_default(), _ => Default::default(), - }; - Ok(metadata) + } } /// Extract site metadata from HTML Opengraph attributes. @@ -300,6 +299,7 @@ mod tests { context::LemmyContext, request::{extract_opengraph_data, fetch_link_metadata}, }; + use lemmy_utils::settings::SETTINGS; use pretty_assertions::assert_eq; use serial_test::serial; use url::Url; diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 5e37877d2..a6c0951c8 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -846,9 +846,13 @@ pub async fn process_markdown( context: &LemmyContext, ) -> LemmyResult { let text = remove_slurs(text, slur_regex); - let (text, links) = markdown_rewrite_image_links(text); - RemoteImage::create(&mut context.pool(), links).await?; - Ok(text) + if context.settings().pictrs_config()?.image_proxy { + let (text, links) = markdown_rewrite_image_links(text); + RemoteImage::create(&mut context.pool(), links).await?; + Ok(text) + } else { + Ok(text) + } } pub async fn process_markdown_opt( @@ -862,6 +866,11 @@ pub async fn process_markdown_opt( } } +/// Rewrite a link to go through `/api/v3/image_proxy` endpoint. This is only for remote urls and +/// if image_proxy setting is enabled. +/// +/// The parameter `image_proxy` is the config value of `pictrs.image_proxy`. Its necessary to pass +/// as separate parameter so it can be changed in tests. pub(crate) async fn proxy_image_link( link: Url, image_proxy: bool, diff --git a/crates/api_crud/src/post/create.rs b/crates/api_crud/src/post/create.rs index 775e5b7e9..3f42177c3 100644 --- a/crates/api_crud/src/post/create.rs +++ b/crates/api_crud/src/post/create.rs @@ -84,7 +84,7 @@ pub async fn create_post( } // Fetch post links and pictrs cached image - let metadata = fetch_link_metadata_opt(url.as_ref(), true, &context).await?; + let metadata = fetch_link_metadata_opt(url.as_ref(), true, &context).await; let url = proxy_image_link_opt_apub(url, &context).await?; // Only need to check if language is allowed in case user set it explicitly. When using default diff --git a/crates/apub/src/objects/post.rs b/crates/apub/src/objects/post.rs index fa65500cb..c05ad1214 100644 --- a/crates/apub/src/objects/post.rs +++ b/crates/apub/src/objects/post.rs @@ -224,7 +224,7 @@ impl Object for ApubPost { let do_generate_thumbnail = thumbnail_url.is_none() && allow_generate_thumbnail; // Generate local thumbnail only if no thumbnail was federated and 'sensitive' attributes allow it. - let metadata = fetch_link_metadata_opt(url.as_ref(), do_generate_thumbnail, context).await?; + let metadata = fetch_link_metadata_opt(url.as_ref(), do_generate_thumbnail, context).await; if let Some(thumbnail_url_) = metadata.thumbnail { thumbnail_url = Some(thumbnail_url_.into()); } diff --git a/crates/db_schema/src/source/images.rs b/crates/db_schema/src/source/images.rs index c95bd1bc1..f8befb856 100644 --- a/crates/db_schema/src/source/images.rs +++ b/crates/db_schema/src/source/images.rs @@ -30,6 +30,8 @@ pub struct LocalImageForm { pub pictrs_delete_token: String, } +/// Stores all images which are hosted on remote domains. When attempting to proxy an image, it +/// is checked against this table to avoid Lemmy being used as a general purpose proxy. #[skip_serializing_none] #[derive(PartialEq, Eq, Debug, Clone)] #[cfg_attr(feature = "full", derive(Queryable, Identifiable))] diff --git a/docker/lemmy.hjson b/docker/lemmy.hjson index 95f7e89b7..1446d3bb2 100644 --- a/docker/lemmy.hjson +++ b/docker/lemmy.hjson @@ -21,12 +21,8 @@ pictrs: { url: "http://pictrs:8080/" # api_key: "API_KEY" -<<<<<<< HEAD - cache_remote_images: true image_proxy: true -======= cache_external_link_previews: true ->>>>>>> main } #opentelemetry_url: "http://otel:4137"