From 55f84dd38ade98b864f0d99371826be0a00a0712 Mon Sep 17 00:00:00 2001 From: Dessalines Date: Wed, 22 May 2024 04:28:47 -0400 Subject: [PATCH] Fixing proxy images (#4722) * Adding an image_details table to store image dimensions. - Adds an image_details table, which stores the height, width, and content_type for local and remote images. - For LocalImages, this information already comes back with the upload. - For RemoteImages, it calls the pictrs details endpoint. - Fixed some issues with proxying non-image urls. - Fixes #3328 - Also fixes #4703 * Running sql format. * Running fmt. * Don't fetch metadata in background for local API requests. * Dont export remote_image table to typescript. * Cleaning up validate. * Dont proxy url. * Fixing tests, fixing issue with federated thumbnails. * Fix tests. * Updating corepack, fixing issue. * Refactoring image inserts to use transactions. * Use select exists again. * Fixing imports. * Fix test. * Removing pointless backgrounded metadata generation version. * Removing public pictrs details route. * Fixing clippy. * Fixing proxy image fetching. Fixes #4703 - This extracts only the proxy image fixes from #4704, leaving off thumbnails. * Fix test. * Addressing PR comments. * Address PR comments 2. --------- Co-authored-by: SleeplessOne1917 <28871516+SleeplessOne1917@users.noreply.github.com> --- api_tests/package.json | 4 +- api_tests/prepare-drone-federation-test.sh | 2 +- api_tests/src/comment.spec.ts | 1 - api_tests/src/community.spec.ts | 14 +- api_tests/src/image.spec.ts | 30 ++-- api_tests/src/post.spec.ts | 8 +- api_tests/src/shared.ts | 48 +++-- crates/api_common/src/request.rs | 195 ++++++++++----------- crates/api_common/src/utils.rs | 20 ++- crates/api_crud/src/post/create.rs | 8 +- crates/api_crud/src/post/update.rs | 12 +- crates/apub/src/objects/post.rs | 15 +- crates/db_schema/src/source/images.rs | 8 +- crates/db_views/src/structs.rs | 1 + crates/routes/src/images.rs | 62 +++---- docker/docker-compose.yml | 2 +- docker/federation/docker-compose.yml | 2 +- 17 files changed, 220 insertions(+), 212 deletions(-) diff --git a/api_tests/package.json b/api_tests/package.json index 1b0c91d80..b194dae30 100644 --- a/api_tests/package.json +++ b/api_tests/package.json @@ -6,11 +6,11 @@ "repository": "https://github.com/LemmyNet/lemmy", "author": "Dessalines", "license": "AGPL-3.0", - "packageManager": "pnpm@9.0.6", + "packageManager": "pnpm@9.1.1+sha256.9551e803dcb7a1839fdf5416153a844060c7bce013218ce823410532504ac10b", "scripts": { "lint": "tsc --noEmit && eslint --report-unused-disable-directives --ext .js,.ts,.tsx src && prettier --check 'src/**/*.ts'", "fix": "prettier --write src && eslint --fix src", - "api-test": "jest -i follow.spec.ts && jest -i post.spec.ts && jest -i comment.spec.ts && jest -i private_message.spec.ts && jest -i user.spec.ts && jest -i community.spec.ts && jest -i image.spec.ts", + "api-test": "jest -i follow.spec.ts && jest -i image.spec.ts && jest -i user.spec.ts && jest -i private_message.spec.ts && jest -i community.spec.ts && jest -i post.spec.ts && jest -i comment.spec.ts ", "api-test-follow": "jest -i follow.spec.ts", "api-test-comment": "jest -i comment.spec.ts", "api-test-post": "jest -i post.spec.ts", diff --git a/api_tests/prepare-drone-federation-test.sh b/api_tests/prepare-drone-federation-test.sh index 136f8ddfc..31eb111c2 100755 --- a/api_tests/prepare-drone-federation-test.sh +++ b/api_tests/prepare-drone-federation-test.sh @@ -15,7 +15,7 @@ export LEMMY_TEST_FAST_FEDERATION=1 # by default, the persistent federation queu # pictrs setup if [ ! -f "api_tests/pict-rs" ]; then - curl "https://git.asonix.dog/asonix/pict-rs/releases/download/v0.5.0-beta.2/pict-rs-linux-amd64" -o api_tests/pict-rs + curl "https://git.asonix.dog/asonix/pict-rs/releases/download/v0.5.13/pict-rs-linux-amd64" -o api_tests/pict-rs chmod +x api_tests/pict-rs fi ./api_tests/pict-rs \ diff --git a/api_tests/src/comment.spec.ts b/api_tests/src/comment.spec.ts index fc547481c..dfab4109c 100644 --- a/api_tests/src/comment.spec.ts +++ b/api_tests/src/comment.spec.ts @@ -45,7 +45,6 @@ let postOnAlphaRes: PostResponse; beforeAll(async () => { await setupLogins(); - await unfollows(); await Promise.all([followBeta(alpha), followBeta(gamma)]); betaCommunity = (await resolveBetaCommunity(alpha)).community; if (betaCommunity) { diff --git a/api_tests/src/community.spec.ts b/api_tests/src/community.spec.ts index 5aa9fdfc8..d172f7045 100644 --- a/api_tests/src/community.spec.ts +++ b/api_tests/src/community.spec.ts @@ -380,8 +380,8 @@ test("User blocks instance, communities are hidden", async () => { test("Community follower count is federated", async () => { // Follow the beta community from alpha let community = await createCommunity(beta); - let community_id = community.community_view.community.actor_id; - let resolved = await resolveCommunity(alpha, community_id); + let communityActorId = community.community_view.community.actor_id; + let resolved = await resolveCommunity(alpha, communityActorId); if (!resolved.community) { throw "Missing beta community"; } @@ -389,7 +389,7 @@ test("Community follower count is federated", async () => { await followCommunity(alpha, true, resolved.community.community.id); let followed = ( await waitUntil( - () => resolveCommunity(alpha, community_id), + () => resolveCommunity(alpha, communityActorId), c => c.community?.subscribed === "Subscribed", ) ).community; @@ -398,7 +398,7 @@ test("Community follower count is federated", async () => { expect(followed?.counts.subscribers).toBe(1); // Follow the community from gamma - resolved = await resolveCommunity(gamma, community_id); + resolved = await resolveCommunity(gamma, communityActorId); if (!resolved.community) { throw "Missing beta community"; } @@ -406,7 +406,7 @@ test("Community follower count is federated", async () => { await followCommunity(gamma, true, resolved.community.community.id); followed = ( await waitUntil( - () => resolveCommunity(gamma, community_id), + () => resolveCommunity(gamma, communityActorId), c => c.community?.subscribed === "Subscribed", ) ).community; @@ -415,7 +415,7 @@ test("Community follower count is federated", async () => { expect(followed?.counts?.subscribers).toBe(2); // Follow the community from delta - resolved = await resolveCommunity(delta, community_id); + resolved = await resolveCommunity(delta, communityActorId); if (!resolved.community) { throw "Missing beta community"; } @@ -423,7 +423,7 @@ test("Community follower count is federated", async () => { await followCommunity(delta, true, resolved.community.community.id); followed = ( await waitUntil( - () => resolveCommunity(delta, community_id), + () => resolveCommunity(delta, communityActorId), c => c.community?.subscribed === "Subscribed", ) ).community; diff --git a/api_tests/src/image.spec.ts b/api_tests/src/image.spec.ts index a1b3c3f3e..806c5b313 100644 --- a/api_tests/src/image.spec.ts +++ b/api_tests/src/image.spec.ts @@ -29,14 +29,16 @@ import { unfollows, getPost, waitUntil, - randomString, createPostWithThumbnail, + sampleImage, } from "./shared"; const downloadFileSync = require("download-file-sync"); beforeAll(setupLogins); -afterAll(unfollows); +afterAll(async () => { + await Promise.all([unfollows(), deleteAllImages(alpha)]); +}); test("Upload image and delete it", async () => { // Before running this test, you need to delete all previous images in the DB @@ -159,7 +161,6 @@ test("Purge post, linked image removed", async () => { expect(post.post_view.post.url).toBe(upload.url); // purge post - const purgeForm: PurgePost = { post_id: post.post_view.post.id, }; @@ -183,13 +184,13 @@ test("Images in remote post are proxied if setting enabled", async () => { gamma, community.community_view.community.id, upload.url, - "![](http://example.com/image2.png)", + `![](${sampleImage})`, ); expect(post.post_view.post).toBeDefined(); // remote image gets proxied after upload expect( - post.post_view.post.url?.startsWith( + post.post_view.post.thumbnail_url?.startsWith( "http://lemmy-gamma:8561/api/v3/image_proxy?url", ), ).toBeTruthy(); @@ -202,14 +203,20 @@ test("Images in remote post are proxied if setting enabled", async () => { let epsilonPost = await resolvePost(epsilon, post.post_view.post); expect(epsilonPost.post).toBeDefined(); - // remote image gets proxied after federation + // Fetch the post again, the metadata should be backgrounded now + // Wait for the metadata to get fetched, since this is backgrounded now + let epsilonPost2 = await waitUntil( + () => getPost(epsilon, epsilonPost.post!.post.id), + p => p.post_view.post.thumbnail_url != undefined, + ); + expect( - epsilonPost.post!.post.url?.startsWith( + epsilonPost2.post_view.post.thumbnail_url?.startsWith( "http://lemmy-epsilon:8581/api/v3/image_proxy?url", ), ).toBeTruthy(); expect( - epsilonPost.post!.post.body?.startsWith( + epsilonPost2.post_view.post.body?.startsWith( "![](http://lemmy-epsilon:8581/api/v3/image_proxy?url", ), ).toBeTruthy(); @@ -232,7 +239,7 @@ test("No image proxying if setting is disabled", async () => { alpha, community.community_view.community.id, upload.url, - "![](http://example.com/image2.png)", + `![](${sampleImage})`, ); expect(post.post_view.post).toBeDefined(); @@ -240,7 +247,7 @@ test("No image proxying if setting is disabled", async () => { 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)"); + expect(post.post_view.post.body).toBe(`![](${sampleImage})`); let betaPost = await waitForPost( beta, @@ -253,8 +260,7 @@ test("No image proxying if setting is disabled", async () => { expect( betaPost.post.url?.startsWith("http://127.0.0.1:8551/pictrs/image/"), ).toBeTruthy(); - expect(betaPost.post.body).toBe("![](http://example.com/image2.png)"); - + expect(betaPost.post.body).toBe(`![](${sampleImage})`); // Make sure the alt text got federated expect(post.post_view.post.alt_text).toBe(betaPost.post.alt_text); }); diff --git a/api_tests/src/post.spec.ts b/api_tests/src/post.spec.ts index e2c198bf6..cb45274c6 100644 --- a/api_tests/src/post.spec.ts +++ b/api_tests/src/post.spec.ts @@ -48,7 +48,6 @@ beforeAll(async () => { await setupLogins(); betaCommunity = (await resolveBetaCommunity(alpha)).community; expect(betaCommunity).toBeDefined(); - await unfollows(); }); afterAll(unfollows); @@ -83,10 +82,7 @@ async 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); + const editSiteForm: EditSite = {}; editSiteForm.allowed_instances = []; editSiteForm.blocked_instances = ["lemmy-alpha"]; @@ -749,7 +745,7 @@ test("Block post that contains banned URL", async () => { await epsilon.editSite(editSiteForm); - await delay(500); + await delay(); if (!betaCommunity) { throw "Missing beta community"; diff --git a/api_tests/src/shared.ts b/api_tests/src/shared.ts index 1a8a9afaf..6cc8d5aca 100644 --- a/api_tests/src/shared.ts +++ b/api_tests/src/shared.ts @@ -81,6 +81,8 @@ import { ListingType } from "lemmy-js-client/dist/types/ListingType"; export const fetchFunction = fetch; export const imageFetchLimit = 50; +export const sampleImage = + "https://i.pinimg.com/originals/df/5f/5b/df5f5b1b174a2b4b6026cc6c8f9395c1.jpg"; export let alphaUrl = "http://127.0.0.1:8541"; export let betaUrl = "http://127.0.0.1:8551"; @@ -180,6 +182,10 @@ export async function setupLogins() { ]; await gamma.editSite(editSiteForm); + // Setup delta allowed instance + editSiteForm.allowed_instances = ["lemmy-beta"]; + await delta.editSite(editSiteForm); + // Create the main alpha/beta communities // Ignore thrown errors of duplicates try { @@ -693,8 +699,8 @@ export async function saveUserSettingsBio( export async function saveUserSettingsFederated( api: LemmyHttp, ): Promise { - let avatar = "https://image.flaticon.com/icons/png/512/35/35896.png"; - let banner = "https://image.flaticon.com/icons/png/512/36/35896.png"; + let avatar = sampleImage; + let banner = sampleImage; let bio = "a changed bio"; let form: SaveUserSettings = { show_nsfw: false, @@ -760,6 +766,7 @@ export async function unfollowRemotes( await Promise.all( remoteFollowed.map(cu => followCommunity(api, false, cu.community.id)), ); + let siteRes = await getSite(api); return siteRes; } @@ -889,14 +896,17 @@ export async function deleteAllImages(api: LemmyHttp) { limit: imageFetchLimit, }); imagesRes.images; - - for (const image of imagesRes.images) { - const form: DeleteImage = { - token: image.local_image.pictrs_delete_token, - filename: image.local_image.pictrs_alias, - }; - await api.deleteImage(form); - } + Promise.all( + imagesRes.images + .map(image => { + const form: DeleteImage = { + token: image.local_image.pictrs_delete_token, + filename: image.local_image.pictrs_alias, + }; + return form; + }) + .map(form => api.deleteImage(form)), + ); } export async function unfollows() { @@ -907,6 +917,24 @@ export async function unfollows() { unfollowRemotes(delta), unfollowRemotes(epsilon), ]); + await Promise.all([ + purgeAllPosts(alpha), + purgeAllPosts(beta), + purgeAllPosts(gamma), + purgeAllPosts(delta), + purgeAllPosts(epsilon), + ]); +} + +export async function purgeAllPosts(api: LemmyHttp) { + // The best way to get all federated items, is to find the posts + let res = await api.getPosts({ type_: "All", limit: 50 }); + await Promise.all( + Array.from(new Set(res.posts.map(p => p.post.id))) + .map(post_id => api.purgePost({ post_id })) + // Ignore errors + .map(p => p.catch(e => e)), + ); } export function getCommentParentId(comment: Comment): number | undefined { diff --git a/crates/api_common/src/request.rs b/crates/api_common/src/request.rs index a2720998b..c304bcba7 100644 --- a/crates/api_common/src/request.rs +++ b/crates/api_common/src/request.rs @@ -3,9 +3,10 @@ use crate::{ lemmy_db_schema::traits::Crud, post::{LinkMetadata, OpenGraphData}, send_activity::{ActivityChannel, SendActivityData}, - utils::{local_site_opt_to_sensitive, proxy_image_link, proxy_image_link_opt_apub}, + utils::{local_site_opt_to_sensitive, proxy_image_link}, }; use activitypub_federation::config::Data; +use chrono::{DateTime, Utc}; use encoding_rs::{Encoding, UTF_8}; use lemmy_db_schema::{ newtypes::DbUrl, @@ -18,14 +19,13 @@ use lemmy_db_schema::{ use lemmy_utils::{ error::{LemmyError, LemmyErrorType, LemmyResult}, settings::structs::{PictrsImageMode, Settings}, - spawn_try_task, REQWEST_TIMEOUT, VERSION, }; use mime::Mime; use reqwest::{header::CONTENT_TYPE, Client, ClientBuilder}; use reqwest_middleware::ClientWithMiddleware; -use serde::Deserialize; +use serde::{Deserialize, Serialize}; use tracing::info; use url::Url; use urlencoding::encode; @@ -65,95 +65,70 @@ pub async fn fetch_link_metadata(url: &Url, context: &LemmyContext) -> LemmyResu }) } -/// Generate post thumbnail in background task, because some sites can be very slow to respond. +/// Generates and saves a post thumbnail and metadata. /// /// Takes a callback to generate a send activity task, so that post can be federated with metadata. /// /// TODO: `federated_thumbnail` param can be removed once we federate full metadata and can /// write it to db directly, without calling this function. /// https://github.com/LemmyNet/lemmy/issues/4598 -pub fn generate_post_link_metadata( +pub async fn generate_post_link_metadata( post: Post, custom_thumbnail: Option, - federated_thumbnail: Option, send_activity: impl FnOnce(Post) -> Option + Send + 'static, local_site: Option, context: Data, -) { - spawn_try_task(async move { - let metadata = match &post.url { - Some(url) => fetch_link_metadata(url, &context).await.unwrap_or_default(), - _ => Default::default(), - }; +) -> LemmyResult<()> { + let metadata = match &post.url { + Some(url) => fetch_link_metadata(url, &context).await.unwrap_or_default(), + _ => Default::default(), + }; - let is_image_post = metadata - .content_type - .as_ref() - .is_some_and(|content_type| content_type.starts_with("image")); + let is_image_post = metadata + .content_type + .as_ref() + .is_some_and(|content_type| content_type.starts_with("image")); - // Decide if we are allowed to generate local thumbnail - let allow_sensitive = local_site_opt_to_sensitive(&local_site); - let allow_generate_thumbnail = allow_sensitive || !post.nsfw; + // Decide if we are allowed to generate local thumbnail + let allow_sensitive = local_site_opt_to_sensitive(&local_site); + let allow_generate_thumbnail = allow_sensitive || !post.nsfw; - // Use custom thumbnail if available and its not an image post - let thumbnail_url = if !is_image_post && custom_thumbnail.is_some() { - custom_thumbnail - } - // Use federated thumbnail if available - else if federated_thumbnail.is_some() { - federated_thumbnail - } - // Generate local thumbnail if allowed - else if allow_generate_thumbnail { - match post - .url - .filter(|_| is_image_post) - .or(metadata.opengraph_data.image) - { - Some(url) => generate_pictrs_thumbnail(&url, &context).await.ok(), - None => None, - } - } - // Otherwise use opengraph preview image directly - else { - metadata.opengraph_data.image.map(Into::into) - }; + let image_url = if is_image_post { + post.url + } else { + metadata.opengraph_data.image.clone() + }; - // Proxy the image fetch if necessary - let proxied_thumbnail_url = proxy_image_link_opt_apub(thumbnail_url, &context).await?; + let thumbnail_url = if let (false, Some(url)) = (is_image_post, custom_thumbnail) { + proxy_image_link(url, &context).await.ok() + } else if let (true, Some(url)) = (allow_generate_thumbnail, image_url) { + generate_pictrs_thumbnail(&url, &context) + .await + .ok() + .map(Into::into) + } else { + metadata.opengraph_data.image.clone() + }; - let form = PostUpdateForm { - embed_title: Some(metadata.opengraph_data.title), - embed_description: Some(metadata.opengraph_data.description), - embed_video_url: Some(metadata.opengraph_data.embed_video_url), - thumbnail_url: Some(proxied_thumbnail_url), - url_content_type: Some(metadata.content_type), - ..Default::default() - }; - let updated_post = Post::update(&mut context.pool(), post.id, &form).await?; - if let Some(send_activity) = send_activity(updated_post) { - ActivityChannel::submit_activity(send_activity, &context).await?; - } - Ok(()) - }); + let form = PostUpdateForm { + embed_title: Some(metadata.opengraph_data.title), + embed_description: Some(metadata.opengraph_data.description), + embed_video_url: Some(metadata.opengraph_data.embed_video_url), + thumbnail_url: Some(thumbnail_url), + url_content_type: Some(metadata.content_type), + ..Default::default() + }; + let updated_post = Post::update(&mut context.pool(), post.id, &form).await?; + if let Some(send_activity) = send_activity(updated_post) { + ActivityChannel::submit_activity(send_activity, &context).await?; + } + Ok(()) } /// Extract site metadata from HTML Opengraph attributes. fn extract_opengraph_data(html_bytes: &[u8], url: &Url) -> LemmyResult { let html = String::from_utf8_lossy(html_bytes); - // Make sure the first line is doctype html - let first_line = html - .trim_start() - .lines() - .next() - .ok_or(LemmyErrorType::NoLinesInHtml)? - .to_lowercase(); - - if !first_line.starts_with(" LemmyResult, - msg: String, +#[derive(Deserialize, Serialize, Debug)] +pub struct PictrsResponse { + pub files: Option>, + pub msg: String, } -#[derive(Deserialize, Debug)] -struct PictrsFile { - file: String, - delete_token: String, +#[derive(Deserialize, Serialize, Debug)] +pub struct PictrsFile { + pub file: String, + pub delete_token: String, + pub details: PictrsFileDetails, } -#[derive(Deserialize, Debug)] +impl PictrsFile { + pub fn thumbnail_url(&self, protocol_and_hostname: &str) -> Result { + Url::parse(&format!( + "{protocol_and_hostname}/pictrs/image/{}", + self.file + )) + } +} + +/// Stores extra details about a Pictrs image. +#[derive(Deserialize, Serialize, Debug)] +pub struct PictrsFileDetails { + /// In pixels + pub width: u16, + /// In pixels + pub height: u16, + pub content_type: String, + pub created_at: DateTime, +} + +#[derive(Deserialize, Serialize, Debug)] struct PictrsPurgeResponse { msg: String, } @@ -295,33 +291,34 @@ async fn generate_pictrs_thumbnail(image_url: &Url, context: &LemmyContext) -> L encode(image_url.as_str()) ); - let response = context + let res = context .client() .get(&fetch_url) .timeout(REQWEST_TIMEOUT) .send() + .await? + .json::() .await?; - let response: PictrsResponse = response.json().await?; + let files = res.files.unwrap_or_default(); - if response.msg == "ok" { - let thumbnail_url = Url::parse(&format!( - "{}/pictrs/image/{}", - context.settings().get_protocol_and_hostname(), - response.files.first().expect("missing pictrs file").file - ))?; - for uploaded_image in response.files { - let form = LocalImageForm { - local_user_id: None, - pictrs_alias: uploaded_image.file.to_string(), - pictrs_delete_token: uploaded_image.delete_token.to_string(), - }; - LocalImage::create(&mut context.pool(), &form).await?; - } - Ok(thumbnail_url) - } else { - Err(LemmyErrorType::PictrsResponseError(response.msg))? - } + let image = files + .first() + .ok_or(LemmyErrorType::PictrsResponseError(res.msg))?; + + let form = LocalImageForm { + // This is none because its an internal request. + // IE, a local user shouldn't get to delete the thumbnails for their link posts + local_user_id: None, + pictrs_alias: image.file.clone(), + pictrs_delete_token: image.delete_token.clone(), + }; + let protocol_and_hostname = context.settings().get_protocol_and_hostname(); + let thumbnail_url = image.thumbnail_url(&protocol_and_hostname)?; + + LocalImage::create(&mut context.pool(), &form).await?; + + Ok(thumbnail_url) } // TODO: get rid of this by reading content type from db diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 7db4c56ea..f1e9c4c04 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -965,13 +965,10 @@ async fn proxy_image_link_internal( if link.domain() == Some(&context.settings().hostname) { Ok(link.into()) } else if image_mode == PictrsImageMode::ProxyAllImages { - let proxied = format!( - "{}/api/v3/image_proxy?url={}", - context.settings().get_protocol_and_hostname(), - encode(link.as_str()) - ); + let proxied = build_proxied_image_url(&link, &context.settings().get_protocol_and_hostname())?; + RemoteImage::create(&mut context.pool(), vec![link]).await?; - Ok(Url::parse(&proxied)?.into()) + Ok(proxied.into()) } else { Ok(link.into()) } @@ -1025,6 +1022,17 @@ pub async fn proxy_image_link_opt_apub( } } +fn build_proxied_image_url( + link: &Url, + protocol_and_hostname: &str, +) -> Result { + Url::parse(&format!( + "{}/api/v3/image_proxy?url={}", + protocol_and_hostname, + encode(link.as_str()) + )) +} + #[cfg(test)] #[allow(clippy::unwrap_used)] #[allow(clippy::indexing_slicing)] diff --git a/crates/api_crud/src/post/create.rs b/crates/api_crud/src/post/create.rs index fcd274c03..82a208a9c 100644 --- a/crates/api_crud/src/post/create.rs +++ b/crates/api_crud/src/post/create.rs @@ -14,7 +14,6 @@ use lemmy_api_common::{ local_site_to_slur_regex, mark_post_as_read, process_markdown_opt, - proxy_image_link_opt_apub, EndpointType, }, }; @@ -75,7 +74,6 @@ pub async fn create_post( is_url_blocked(&url, &url_blocklist)?; check_url_scheme(&url)?; check_url_scheme(&custom_thumbnail)?; - let url = proxy_image_link_opt_apub(url, &context).await?; check_community_user_action( &local_user_view.person, @@ -125,7 +123,7 @@ pub async fn create_post( let post_form = PostInsertForm::builder() .name(data.name.trim().to_string()) - .url(url) + .url(url.map(Into::into)) .body(body) .alt_text(data.alt_text.clone()) .community_id(data.community_id) @@ -159,11 +157,11 @@ pub async fn create_post( generate_post_link_metadata( updated_post.clone(), custom_thumbnail, - None, |post| Some(SendActivityData::CreatePost(post)), Some(local_site), context.reset_request_count(), - ); + ) + .await?; // They like their own post by default let person_id = local_user_view.person.id; diff --git a/crates/api_crud/src/post/update.rs b/crates/api_crud/src/post/update.rs index 74e0c0d47..4b4bd9845 100644 --- a/crates/api_crud/src/post/update.rs +++ b/crates/api_crud/src/post/update.rs @@ -11,7 +11,6 @@ use lemmy_api_common::{ get_url_blocklist, local_site_to_slur_regex, process_markdown_opt, - proxy_image_link_opt_apub, }, }; use lemmy_db_schema::{ @@ -86,11 +85,6 @@ pub async fn update_post( Err(LemmyErrorType::NoPostEditAllowed)? } - let url = match url { - Some(url) => Some(proxy_image_link_opt_apub(Some(url), &context).await?), - _ => Default::default(), - }; - let language_id = data.language_id; CommunityLanguage::is_allowed_community_language( &mut context.pool(), @@ -101,7 +95,7 @@ pub async fn update_post( let post_form = PostUpdateForm { name: data.name.clone(), - url, + url: Some(url.map(Into::into)), body: diesel_option_overwrite(body), alt_text: diesel_option_overwrite(data.alt_text.clone()), nsfw: data.nsfw, @@ -118,11 +112,11 @@ pub async fn update_post( generate_post_link_metadata( updated_post.clone(), custom_thumbnail, - None, |post| Some(SendActivityData::UpdatePost(post)), Some(local_site), context.reset_request_count(), - ); + ) + .await?; build_post_response( context.deref(), diff --git a/crates/apub/src/objects/post.rs b/crates/apub/src/objects/post.rs index 929d598cd..0931349b6 100644 --- a/crates/apub/src/objects/post.rs +++ b/crates/apub/src/objects/post.rs @@ -45,6 +45,7 @@ use lemmy_db_schema::{ use lemmy_db_views_actor::structs::CommunityModeratorView; use lemmy_utils::{ error::{LemmyError, LemmyErrorType, LemmyResult}, + spawn_try_task, utils::{markdown::markdown_to_html, slurs::check_slurs_opt, validation::check_url_scheme}, }; use std::ops::Deref; @@ -255,15 +256,13 @@ impl Object for ApubPost { let timestamp = page.updated.or(page.published).unwrap_or_else(naive_now); let post = Post::insert_apub(&mut context.pool(), timestamp, &form).await?; + let post_ = post.clone(); + let context_ = context.reset_request_count(); - generate_post_link_metadata( - post.clone(), - None, - page.image.map(|i| i.url), - |_| None, - local_site, - context.reset_request_count(), - ); + // Generates a post thumbnail in background task, because some sites can be very slow to respond. + spawn_try_task(async move { + generate_post_link_metadata(post_, None, |_| None, local_site, context_).await + }); Ok(post.into()) } diff --git a/crates/db_schema/src/source/images.rs b/crates/db_schema/src/source/images.rs index da6be2a14..9d48e011b 100644 --- a/crates/db_schema/src/source/images.rs +++ b/crates/db_schema/src/source/images.rs @@ -22,7 +22,7 @@ use typed_builder::TypedBuilder; diesel(belongs_to(crate::source::local_user::LocalUser)) )] #[cfg_attr(feature = "full", diesel(check_for_backend(diesel::pg::Pg)))] -#[cfg_attr(feature = "full", diesel(primary_key(local_user_id)))] +#[cfg_attr(feature = "full", diesel(primary_key(pictrs_alias)))] pub struct LocalImage { pub local_user_id: Option, pub pictrs_alias: String, @@ -41,9 +41,11 @@ pub struct LocalImageForm { /// 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. -#[derive(PartialEq, Eq, Debug, Clone)] -#[cfg_attr(feature = "full", derive(Queryable, Identifiable))] +#[skip_serializing_none] +#[derive(Clone, PartialEq, Eq, Debug, Serialize, Deserialize)] +#[cfg_attr(feature = "full", derive(Queryable, Selectable, Identifiable))] #[cfg_attr(feature = "full", diesel(table_name = remote_image))] +#[cfg_attr(feature = "full", diesel(check_for_backend(diesel::pg::Pg)))] pub struct RemoteImage { pub id: i32, pub link: DbUrl, diff --git a/crates/db_views/src/structs.rs b/crates/db_views/src/structs.rs index 4311710ab..c1facfcee 100644 --- a/crates/db_views/src/structs.rs +++ b/crates/db_views/src/structs.rs @@ -216,6 +216,7 @@ pub struct VoteView { pub score: i16, } +#[skip_serializing_none] #[derive(Debug, Serialize, Deserialize, Clone)] #[cfg_attr(feature = "full", derive(TS, Queryable))] #[cfg_attr(feature = "full", diesel(check_for_backend(diesel::pg::Pg)))] diff --git a/crates/routes/src/images.rs b/crates/routes/src/images.rs index 671aa223e..96d7d317c 100644 --- a/crates/routes/src/images.rs +++ b/crates/routes/src/images.rs @@ -1,18 +1,16 @@ use actix_web::{ body::BodyStream, - error, http::{ header::{HeaderName, ACCEPT_ENCODING, HOST}, StatusCode, }, web, web::Query, - Error, HttpRequest, HttpResponse, }; use futures::stream::{Stream, StreamExt}; -use lemmy_api_common::context::LemmyContext; +use lemmy_api_common::{context::LemmyContext, request::PictrsResponse}; use lemmy_db_schema::source::{ images::{LocalImage, LocalImageForm, RemoteImage}, local_site::LocalSite, @@ -21,7 +19,7 @@ use lemmy_db_views::structs::LocalUserView; use lemmy_utils::{error::LemmyResult, rate_limit::RateLimitCell, REQWEST_TIMEOUT}; use reqwest::Body; use reqwest_middleware::{ClientWithMiddleware, RequestBuilder}; -use serde::{Deserialize, Serialize}; +use serde::Deserialize; use std::time::Duration; use url::Url; use urlencoding::decode; @@ -43,20 +41,8 @@ pub fn config( .service(web::resource("/pictrs/image/delete/{token}/{filename}").route(web::get().to(delete))); } -#[derive(Debug, Serialize, Deserialize)] -struct Image { - file: String, - delete_token: String, -} - -#[derive(Debug, Serialize, Deserialize)] -struct Images { - msg: String, - files: Option>, -} - #[derive(Deserialize)] -struct PictrsParams { +struct PictrsGetParams { format: Option, thumbnail: Option, } @@ -92,7 +78,7 @@ async fn upload( local_user_view: LocalUserView, client: web::Data, context: web::Data, -) -> Result { +) -> LemmyResult { // TODO: check rate limit here let pictrs_config = context.settings().pictrs_config()?; let image_url = format!("{}image", pictrs_config.url); @@ -106,21 +92,18 @@ async fn upload( .timeout(Duration::from_secs(pictrs_config.upload_timeout)) .body(Body::wrap_stream(make_send(body))) .send() - .await - .map_err(error::ErrorBadRequest)?; + .await?; let status = res.status(); - let images = res.json::().await.map_err(error::ErrorBadRequest)?; + let images = res.json::().await?; if let Some(images) = &images.files { - for uploaded_image in images { + for image in images { let form = LocalImageForm { local_user_id: Some(local_user_view.local_user.id), - pictrs_alias: uploaded_image.file.to_string(), - pictrs_delete_token: uploaded_image.delete_token.to_string(), + pictrs_alias: image.file.to_string(), + pictrs_delete_token: image.delete_token.to_string(), }; - LocalImage::create(&mut context.pool(), &form) - .await - .map_err(error::ErrorBadRequest)?; + LocalImage::create(&mut context.pool(), &form).await?; } } @@ -129,16 +112,14 @@ async fn upload( async fn full_res( filename: web::Path, - web::Query(params): web::Query, + web::Query(params): web::Query, req: HttpRequest, client: web::Data, context: web::Data, local_user_view: Option, -) -> Result { +) -> LemmyResult { // block access to images if instance is private and unauthorized, public - let local_site = LocalSite::read(&mut context.pool()) - .await - .map_err(error::ErrorBadRequest)?; + let local_site = LocalSite::read(&mut context.pool()).await?; if local_site.private_instance && local_user_view.is_none() { return Ok(HttpResponse::Unauthorized().finish()); } @@ -169,7 +150,7 @@ async fn image( url: String, req: HttpRequest, client: &ClientWithMiddleware, -) -> Result { +) -> LemmyResult { let mut client_req = adapt_request(&req, client, url); if let Some(addr) = req.head().peer_addr { @@ -180,7 +161,7 @@ async fn image( client_req = client_req.header("X-Forwarded-For", addr.to_string()); } - let res = client_req.send().await.map_err(error::ErrorBadRequest)?; + let res = client_req.send().await?; if res.status() == StatusCode::NOT_FOUND { return Ok(HttpResponse::NotFound().finish()); @@ -202,7 +183,7 @@ async fn delete( context: web::Data, // require login _local_user_view: LocalUserView, -) -> Result { +) -> LemmyResult { let (token, file) = components.into_inner(); let pictrs_config = context.settings().pictrs_config()?; @@ -214,11 +195,9 @@ async fn delete( client_req = client_req.header("X-Forwarded-For", addr.to_string()); } - let res = client_req.send().await.map_err(error::ErrorBadRequest)?; + let res = client_req.send().await?; - LocalImage::delete_by_alias(&mut context.pool(), &file) - .await - .map_err(error::ErrorBadRequest)?; + LocalImage::delete_by_alias(&mut context.pool(), &file).await?; Ok(HttpResponse::build(res.status()).body(BodyStream::new(res.bytes_stream()))) } @@ -230,6 +209,8 @@ pub struct ImageProxyParams { pub async fn image_proxy( Query(params): Query, + req: HttpRequest, + client: web::Data, context: web::Data, ) -> LemmyResult { let url = Url::parse(&decode(¶ms.url)?)?; @@ -240,9 +221,8 @@ pub async fn image_proxy( let pictrs_config = context.settings().pictrs_config()?; let url = format!("{}image/original?proxy={}", pictrs_config.url, ¶ms.url); - let image_response = context.client().get(url).send().await?; - Ok(HttpResponse::Ok().streaming(image_response.bytes_stream())) + image(url, req, &client).await } fn make_send(mut stream: S) -> impl Stream + Send + Unpin + 'static diff --git a/docker/docker-compose.yml b/docker/docker-compose.yml index 141368936..c690a5f48 100644 --- a/docker/docker-compose.yml +++ b/docker/docker-compose.yml @@ -75,7 +75,7 @@ services: init: true pictrs: - image: asonix/pictrs:0.5.0-rc.2 + image: asonix/pictrs:0.5.13 # this needs to match the pictrs url in lemmy.hjson hostname: pictrs # we can set options to pictrs like this, here we set max. image size and forced format for conversion diff --git a/docker/federation/docker-compose.yml b/docker/federation/docker-compose.yml index 8dd70f3bc..ac4ef4649 100644 --- a/docker/federation/docker-compose.yml +++ b/docker/federation/docker-compose.yml @@ -49,7 +49,7 @@ services: pictrs: restart: always - image: asonix/pictrs:0.5.0-rc.2 + image: asonix/pictrs:0.5.13 user: 991:991 volumes: - ./volumes/pictrs_alpha:/mnt:Z