From a91d81c69525a9d5d33eb158233524d45ac0a238 Mon Sep 17 00:00:00 2001 From: elhombretecla Date: Tue, 14 Jul 2026 14:47:54 +0200 Subject: [PATCH] :tada: Add link preview metadata for shared links --- .../resources/app/templates/link-preview.tmpl | 22 ++ backend/scripts/_env | 1 + backend/src/app/config.clj | 16 + backend/src/app/http.clj | 3 + backend/src/app/http/assets.clj | 15 +- backend/src/app/http/link_preview.clj | 82 +++++ backend/src/app/main.clj | 5 + backend/test/backend_tests/config_test.clj | 40 +++ .../test/backend_tests/http_assets_test.clj | 25 +- .../backend_tests/http_link_preview_test.clj | 211 ++++++++++++ common/src/app/common/flags.cljc | 7 +- docker/devenv/files/nginx.conf | 15 + docker/images/files/nginx.conf.template | 15 + docs/technical-guide/configuration.md | 4 + .../developer/subsystems/link-preview.md | 305 ++++++++++++++++++ .../register/get-profile-email-verified.json | 24 ++ frontend/playwright/ui/pages/RegisterPage.js | 8 + frontend/src/app/main/router.cljs | 45 ++- frontend/src/app/main/ui.cljs | 131 -------- frontend/src/app/main/ui/routes.cljs | 17 +- frontend/test/frontend_tests/router_test.cljs | 121 +++++++ frontend/test/frontend_tests/runner.cljs | 2 + 22 files changed, 951 insertions(+), 163 deletions(-) create mode 100644 backend/resources/app/templates/link-preview.tmpl create mode 100644 backend/src/app/http/link_preview.clj create mode 100644 backend/test/backend_tests/config_test.clj create mode 100644 backend/test/backend_tests/http_link_preview_test.clj create mode 100644 docs/technical-guide/developer/subsystems/link-preview.md create mode 100644 frontend/playwright/data/register/get-profile-email-verified.json create mode 100644 frontend/test/frontend_tests/router_test.cljs diff --git a/backend/resources/app/templates/link-preview.tmpl b/backend/resources/app/templates/link-preview.tmpl new file mode 100644 index 0000000000..997e92267d --- /dev/null +++ b/backend/resources/app/templates/link-preview.tmpl @@ -0,0 +1,22 @@ + + + + + {{title}} + + + + + + + + + + + + + + + + + diff --git a/backend/scripts/_env b/backend/scripts/_env index 0860314fea..05162024d5 100644 --- a/backend/scripts/_env +++ b/backend/scripts/_env @@ -62,6 +62,7 @@ export PENPOT_FLAGS="\ enable-file-validation \ enable-file-schema-validation \ enable-redis-cache \ + enable-link-preview \ enable-subscriptions"; # Uncomment for nexus integration testing diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index 879158119f..91155de5e9 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -393,6 +393,22 @@ (or (c/get config :file-clean-delay) (ct/duration {:days 2}))) +(defn join-uri + "Join path segments onto a base URI, preserving a potential subpath + (same semantics as the frontend config). The base is normalized with + a trailing slash; segments must not start with `/` (a leading slash + would resolve against the host root and drop the subpath)." + [base & segments] + (assert (not (some #(str/starts-with? % "/") segments)) + "URI segments must be relative (no leading slash)") + (str (apply u/join (u/ensure-path-slash base) segments))) + +(defn get-public-uri + "Canonical public URI builder: `join-uri` over the configured + :public-uri. With no segments, returns the normalized base." + [& segments] + (apply join-uri (c/get config :public-uri) segments)) + (defn get "A configuration getter. Helps code be more testable." ([key] diff --git a/backend/src/app/http.clj b/backend/src/app/http.clj index d3496a58e5..22e436227f 100644 --- a/backend/src/app/http.clj +++ b/backend/src/app/http.clj @@ -17,6 +17,7 @@ [app.http.awsns :as-alias awsns] [app.http.debug :as-alias debug] [app.http.errors :as errors] + [app.http.link-preview :as-alias link-preview] [app.http.management :as mgmt] [app.http.middleware :as mw] [app.http.security :as sec] @@ -149,6 +150,7 @@ [::rpc/routes schema:routes] [::oidc/routes schema:routes] [::assets/routes schema:routes] + [::link-preview/routes schema:routes] [::debug/routes schema:routes] [::mtx/routes schema:routes] [::awsns/routes schema:routes] @@ -177,6 +179,7 @@ (::mtx/routes cfg) (::assets/routes cfg) + (::link-preview/routes cfg) (::debug/routes cfg) ["/webhooks" diff --git a/backend/src/app/http/assets.clj b/backend/src/app/http/assets.clj index 5c35e9dbf4..9ae258d2a2 100644 --- a/backend/src/app/http/assets.clj +++ b/backend/src/app/http/assets.clj @@ -11,6 +11,7 @@ [app.common.exceptions :as ex] [app.common.time :as ct] [app.common.uri :as u] + [app.config :as cf] [app.db :as db] [app.http.access-token :as actoken] [app.http.session :as session] @@ -35,6 +36,14 @@ "file-data-fragment" "organization"}) +(defn- public-bucket? + [bucket] + (or (contains? public-buckets bucket) + ;; Dashboard file thumbnails become public when link previews + ;; are enabled, so link preview crawlers can fetch them. + (and (= "file-thumbnail" bucket) + (contains? cf/flags :link-preview)))) + (defn get-id [{:keys [path-params]}] (or (some-> path-params :id d/parse-uuid) @@ -56,7 +65,7 @@ (let [sig-max-age (or signature-max-age default-signature-max-age) cch-max-age (or cache-max-age default-cache-max-age) bucket (-> obj meta :bucket) - public? (contains? public-buckets bucket) + public? (public-bucket? bucket) ;; The disposition is also signed into the presigned url: this ;; response is a redirect, so the header below applies to the ;; redirect itself and not to the bytes the client then fetches @@ -84,7 +93,7 @@ headers (cond-> {"x-accel-redirect" (:path purl) "content-type" (:content-type mdata) "cache-control" (str "max-age=" (inst-ms cch-max-age))} - (not (contains? public-buckets bucket)) + (not (public-bucket? bucket)) (assoc "content-disposition" "attachment"))] {::yres/status 204 ::yres/headers headers})) @@ -101,7 +110,7 @@ "Check if the storage object requires authentication based on its bucket." [obj] (let [bucket (-> obj meta :bucket)] - (not (contains? public-buckets bucket)))) + (not (public-bucket? bucket)))) (defn- request-profile-id "Extract the authenticated profile-id from the request." diff --git a/backend/src/app/http/link_preview.clj b/backend/src/app/http/link_preview.clj new file mode 100644 index 0000000000..c024f6d651 --- /dev/null +++ b/backend/src/app/http/link_preview.clj @@ -0,0 +1,82 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns app.http.link-preview + "Link preview (Open Graph metadata) related handlers. + + Serves a minimal HTML page with Open Graph metadata used by link + preview crawlers (Slack, Discord, Twitter, ...). The reverse proxy + routes crawler requests for the application root to this endpoint, + preserving the query string params that the frontend mirrors on + navigation (`file-id`, `project-id` and `team-id`)." + (:require + [app.common.data :as d] + [app.config :as cf] + [app.db :as db] + [app.util.template :as tmpl] + [clojure.java.io :as io] + [integrant.core :as ig] + [yetti.response :as-alias yres])) + +(def ^:private default-context + {:title "Penpot | Full-stack design" + :description "Penpot is the open-source design platform for teams that build digital products at scale."}) + +(def ^:private sql:get-file + "SELECT f.name, ft.media_id + FROM file AS f + LEFT JOIN file_thumbnail AS ft + ON (ft.file_id = f.id AND ft.deleted_at IS NULL) + WHERE f.id = ? + AND f.deleted_at IS NULL + ORDER BY ft.revn DESC NULLS LAST + LIMIT 1") + +(defn- get-file-context + "Return the link preview context for a file link: the file name as title + and, when available, the last dashboard thumbnail as image." + [pool file-id] + (when-let [{:keys [name media-id]} (db/exec-one! pool [sql:get-file file-id])] + (cond-> (assoc default-context :title (str name " | Penpot")) + (some? media-id) + (assoc :image (cf/get-public-uri (str "assets/by-id/" media-id)))))) + +(defn- get-context + [pool params] + (let [project-id (some-> (:project-id params) d/parse-uuid) + team-id (some-> (:team-id params) d/parse-uuid)] + ;; A present file-id is decisive: file links never fall through to the + ;; project/team card, even when the value is malformed or unknown (both + ;; yield nil and the handler falls back to the default context). + (if (contains? params :file-id) + (when-some [file-id (d/parse-uuid (:file-id params))] + (get-file-context pool file-id)) + (cond + (some? project-id) (assoc default-context :title "Project | Penpot") + (some? team-id) (assoc default-context :title "Team dashboard | Penpot"))))) + +(defn- handler + [{:keys [::db/pool]} request] + (let [context (when (contains? cf/flags :link-preview) + (get-context pool (:query-params request))) + context (-> (or context default-context) + (update :image #(or % (cf/get-public-uri "images/penpot-link-preview.png"))))] + {::yres/status 200 + ::yres/headers {"content-type" "text/html; charset=utf-8" + "cache-control" "no-store, no-cache, max-age=0"} + ::yres/body (-> (io/resource "app/templates/link-preview.tmpl") + (tmpl/render context))})) + +;; --- Initialization + +(defmethod ig/assert-key ::routes + [_ params] + (assert (db/pool? (::db/pool params)) "expect valid database pool")) + +(defmethod ig/init-key ::routes + [_ cfg] + ["/link-preview" {:handler (partial handler cfg) + :allowed-methods #{:get :head}}]) diff --git a/backend/src/app/main.clj b/backend/src/app/main.clj index d1ca759df9..04e52592a7 100644 --- a/backend/src/app/main.clj +++ b/backend/src/app/main.clj @@ -20,6 +20,7 @@ [app.http.awsns :as http.awsns] [app.http.client :as-alias http.client] [app.http.debug :as-alias http.debug] + [app.http.link-preview :as-alias http.link-preview] [app.http.management :as mgmt] [app.http.session :as session] [app.http.session.tasks :as-alias session.tasks] @@ -283,9 +284,13 @@ ::mgmt/routes (ig/ref ::mgmt/routes) ::http.debug/routes (ig/ref ::http.debug/routes) ::http.assets/routes (ig/ref ::http.assets/routes) + ::http.link-preview/routes (ig/ref ::http.link-preview/routes) ::http.ws/routes (ig/ref ::http.ws/routes) ::http.awsns/routes (ig/ref ::http.awsns/routes)} + ::http.link-preview/routes + {::db/pool (ig/ref ::db/pool)} + ::http.debug/routes {::db/pool (ig/ref ::db/pool) ::rds/pool (ig/ref ::rds/pool) diff --git a/backend/test/backend_tests/config_test.clj b/backend/test/backend_tests/config_test.clj new file mode 100644 index 0000000000..bae0c3e6af --- /dev/null +++ b/backend/test/backend_tests/config_test.clj @@ -0,0 +1,40 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns backend-tests.config-test + (:require + [app.config :as cf] + [clojure.test :as t])) + +(t/deftest get-public-uri-normalizes-base + (t/testing "trailing slash is ensured with and without subpath" + (doseq [[base expected] [["http://localhost:3449" "http://localhost:3449/"] + ["http://localhost:3449/" "http://localhost:3449/"] + ["https://example.com/penpot" "https://example.com/penpot/"] + ["https://example.com/penpot/" "https://example.com/penpot/"]]] + (t/testing (str "base " base) + (with-redefs [cf/config (assoc cf/config :public-uri base)] + (t/is (= expected (cf/get-public-uri)))))))) + +(t/deftest get-public-uri-preserves-subpath + (t/testing "joined segments keep the subpath" + (with-redefs [cf/config (assoc cf/config :public-uri "https://example.com/penpot")] + (t/is (= "https://example.com/penpot/assets/by-id/123" + (cf/get-public-uri "assets/by-id/123"))) + (t/is (= "https://example.com/penpot/api/main/doc" + (cf/get-public-uri "api/main/doc")))))) + +(t/deftest join-uri-joins-arbitrary-base + (t/testing "segments join onto any base with trailing slash normalization" + (t/is (= "https://nitrate.example.com/api/teams/123" + (cf/join-uri "https://nitrate.example.com" "api/teams/123"))) + (t/is (= "https://nitrate.example.com/api/teams/123" + (cf/join-uri "https://nitrate.example.com/" "api/teams/123"))))) + +(t/deftest join-uri-rejects-leading-slash + (t/testing "a leading slash would drop the subpath, so it fails fast" + (t/is (thrown? AssertionError + (cf/join-uri "https://example.com/penpot" "/assets/by-id/123"))))) diff --git a/backend/test/backend_tests/http_assets_test.clj b/backend/test/backend_tests/http_assets_test.clj index 4c6198afd7..6ebddef0f3 100644 --- a/backend/test/backend_tests/http_assets_test.clj +++ b/backend/test/backend_tests/http_assets_test.clj @@ -8,6 +8,7 @@ (:require [app.common.time :as ct] [app.common.uuid :as uuid] + [app.config :as cf] [app.db :as db] [app.http :as-alias http] [app.http.access-token :as actoken] @@ -157,6 +158,26 @@ ;; Tests: objects-handler — non-public buckets (auth required) ;; ---------------------------------------------------------------- +(t/deftest objects-handler-file-thumbnail-bucket-link-preview-flag + ;; Objects in the file-thumbnail bucket are public only when the + ;; link-preview flag is enabled. + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + cfg (make-handler-cfg storage) + object (create-storage-object! storage "file-thumbnail" "thumbnail data") + request {:path-params {:id (str (:id object))}}] + + (t/testing "flag enabled" + (with-redefs [cf/flags (conj cf/flags :link-preview)] + (let [response (assets/objects-handler cfg request)] + (t/is (not= 401 (::yres/status response))) + (t/is (not= 404 (::yres/status response)))))) + + (t/testing "flag disabled" + (with-redefs [cf/flags (disj cf/flags :link-preview)] + (let [response (assets/objects-handler cfg request)] + (t/is (= 401 (::yres/status response)))))))) + (t/deftest objects-handler-non-public-bucket-no-auth ;; Objects in non-public buckets should return 401 without authentication. (let [storage (-> (:app.storage/storage th/*system*) @@ -218,10 +239,12 @@ cfg (make-handler-cfg storage) profile (th/create-profile* 1)] + ;; NOTE: file-thumbnail is not included here because it is public + ;; when the link-preview flag is enabled; see + ;; objects-handler-file-thumbnail-bucket-link-preview-flag. (doseq [bucket ["profile" "tempfile" "file-data" - "file-thumbnail" "file-change"]] (t/testing (str "bucket: " bucket) (let [object (create-storage-object! storage bucket "some data") diff --git a/backend/test/backend_tests/http_link_preview_test.clj b/backend/test/backend_tests/http_link_preview_test.clj new file mode 100644 index 0000000000..112af4f1a1 --- /dev/null +++ b/backend/test/backend_tests/http_link_preview_test.clj @@ -0,0 +1,211 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns backend-tests.http-link-preview-test + (:require + [app.common.time :as ct] + [app.common.uuid :as uuid] + [app.config :as cf] + [app.db :as db] + [app.http.link-preview :as link-preview] + [app.storage :as sto] + [backend-tests.helpers :as th] + [clojure.test :as t] + [cuerdas.core :as str] + [yetti.response :as-alias yres])) + +(t/use-fixtures :once th/state-init) +(t/use-fixtures :each (th/serial + th/database-reset + th/clean-storage)) + +(def ^:private default-title + "Penpot | Full-stack design") + +(defn- run-handler + [query-params] + (let [cfg {::db/pool (:app.db/pool th/*system*)}] + (#'link-preview/handler cfg {:query-params query-params}))) + +(defn- create-file-thumbnail! + [file-id] + (let [storage (::sto/storage th/*system*) + object (sto/put-object! storage {::sto/content (sto/content (th/tempfile "backend_tests/test_files/sample.png")) + :bucket "file-thumbnail" + :content-type "image/png"})] + (db/insert! (:app.db/pool th/*system*) :file-thumbnail + {:file-id file-id + :revn 1 + :media-id (:id object)}) + object)) + +(t/deftest link-preview-without-params + (let [response (run-handler {})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) default-title)) + (t/is (str/includes? (::yres/body response) "/images/penpot-link-preview.png")))) + +(t/deftest link-preview-file-without-thumbnail + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)})] + (with-redefs [cf/flags (conj cf/flags :link-preview)] + (let [response (run-handler {:file-id (str (:id file))})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) (str (:name file) " | Penpot"))) + (t/is (str/includes? (::yres/body response) "/images/penpot-link-preview.png")))))) + +(t/deftest link-preview-file-with-thumbnail + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + object (create-file-thumbnail! (:id file))] + (with-redefs [cf/flags (conj cf/flags :link-preview)] + (let [response (run-handler {:file-id (str (:id file))})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) (str (:name file) " | Penpot"))) + (t/is (str/includes? (::yres/body response) (str "/assets/by-id/" (:id object)))))))) + +(t/deftest link-preview-non-existent-file + (let [response (run-handler {:file-id (str (uuid/next))})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) default-title)))) + +(t/deftest link-preview-invalid-file-id + (let [response (run-handler {:file-id "not-a-uuid"})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) default-title)))) + +(t/deftest link-preview-team-link + (with-redefs [cf/flags (conj cf/flags :link-preview)] + (let [response (run-handler {:team-id (str (uuid/next))})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) "Team dashboard | Penpot"))))) + +(t/deftest link-preview-project-link + (with-redefs [cf/flags (conj cf/flags :link-preview)] + (let [response (run-handler {:team-id (str (uuid/next)) + :project-id (str (uuid/next))})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) "Project | Penpot"))))) + +(t/deftest link-preview-flag-disabled + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)})] + (with-redefs [cf/flags (disj cf/flags :link-preview)] + (let [response (run-handler {:file-id (str (:id file))})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) default-title)) + (t/is (not (str/includes? (::yres/body response) (:name file)))))))) + +(t/deftest link-preview-deleted-file + ;; A deleted file never leaks its name; crawlers get the generic card. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)})] + (th/mark-file-deleted* {:id (:id file)}) + (with-redefs [cf/flags (conj cf/flags :link-preview)] + (let [response (run-handler {:file-id (str (:id file))})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) default-title)) + (t/is (not (str/includes? (::yres/body response) (:name file)))))))) + +(t/deftest link-preview-file-with-only-deleted-thumbnail + ;; A file whose only thumbnail is deleted keeps its title but falls back + ;; to the default image. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + object (create-file-thumbnail! (:id file))] + (db/update! th/*system* :file-thumbnail + {:deleted-at (ct/now)} + {:file-id (:id file)}) + (with-redefs [cf/flags (conj cf/flags :link-preview)] + (let [response (run-handler {:file-id (str (:id file))})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) (str (:name file) " | Penpot"))) + (t/is (str/includes? (::yres/body response) "/images/penpot-link-preview.png")) + (t/is (not (str/includes? (::yres/body response) (str (:id object))))))))) + +(t/deftest link-preview-file-picks-latest-thumbnail + ;; With several thumbnail revisions, the latest non-deleted one wins. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + pool (:app.db/pool th/*system*) + storage (::sto/storage th/*system*) + old (sto/put-object! storage {::sto/content (sto/content (th/tempfile "backend_tests/test_files/sample.jpg")) + :bucket "file-thumbnail" + :content-type "image/jpeg"}) + latest (sto/put-object! storage {::sto/content (sto/content (th/tempfile "backend_tests/test_files/sample.png")) + :bucket "file-thumbnail" + :content-type "image/png"})] + (db/insert! pool :file-thumbnail + {:file-id (:id file) + :revn 1 + :media-id (:id old) + :deleted-at (ct/now)}) + (db/insert! pool :file-thumbnail + {:file-id (:id file) + :revn 2 + :media-id (:id latest)}) + (with-redefs [cf/flags (conj cf/flags :link-preview)] + (let [response (run-handler {:file-id (str (:id file))})] + (t/is (= 200 (::yres/status response))) + (t/is (str/includes? (::yres/body response) (str "/assets/by-id/" (:id latest)))) + (t/is (not (str/includes? (::yres/body response) (str (:id old))))))))) + +(t/deftest link-preview-escapes-file-name + ;; Hostile file names are HTML-escaped in the rendered meta tags. + (let [profile (th/create-profile* 1) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile) + :name " & co"})] + (with-redefs [cf/flags (conj cf/flags :link-preview)] + (let [response (run-handler {:file-id (str (:id file))})] + (t/is (= 200 (::yres/status response))) + (t/is (not (str/includes? (::yres/body response) " +``` + +so that if a *human* somehow lands on `/link-preview` (e.g. some clients let users +click through to the fetched URL), the browser bounces back to the SPA root +keeping the query string and the fragment, and the app loads normally. The +redirect strips only the trailing `link-preview` segment so subpath +deployments keep their prefix. Crawlers do not execute +JavaScript, so they just read the meta tags. + +### Making file thumbnails publicly accessible + +File: `backend/src/app/http/assets.clj`. + +Crawlers fetch `og:image` anonymously, so the thumbnail asset must be served +without authentication. The assets handler decides per storage bucket whether +auth is required; with this feature the `file-thumbnail` bucket is treated as +public **only while the `link-preview` flag is enabled**: + +```clojure +(defn- public-bucket? + [bucket] + (or (contains? public-buckets bucket) + (and (= "file-thumbnail" bucket) + (contains? cf/flags :link-preview)))) +``` + +With the flag disabled, `file-thumbnail` objects keep requiring an +authenticated profile with access to the file, as before. + +## The feature flag + +Defined in `common/src/app/common/flags.cljc` as `:link-preview`, listed in the +`varia` set and **not** included in the default flags. Enable it on the +backend with: + +```bash +export PENPOT_FLAGS="$PENPOT_FLAGS enable-link-preview" +``` + +It is a backend-only decision point; the frontend URL mirroring is always +active (it is harmless on its own), and the nginx crawler routing is also +unconditional — with the flag off the endpoint simply serves the generic +metadata. + +## Security considerations + +Enabling `link-preview` deliberately trades some privacy for shareability: + + * **File names become readable by anyone who knows the file id** (the + `/link-preview` endpoint does no permission check). + * **Dashboard thumbnails become downloadable by anyone who knows the media + id** (the `file-thumbnail` bucket becomes public). + +Both ids are random UUIDs, so they are not enumerable, but this is +knowledge-of-the-id access, not real authorization. This is the standard +trade-off that link preview features make; it is the reason the flag is off +by default and should be documented to self-hosters before they enable it. + +The preview page also sets `robots: noindex` to keep search engines from +indexing these preview pages, and responses are marked non-cacheable. + +## Testing it locally (devenv) + +1. Make sure the devenv nginx picked up the config (restart the devenv, or + `nginx -s reload` inside the container, if it predates these changes). + +2. The flag already ships enabled in devenv via `backend/scripts/_env`, so + no export is needed there; outside devenv, enable it before starting + the backend: + + ```bash + export PENPOT_FLAGS="$PENPOT_FLAGS enable-link-preview" + ``` + +3. In the browser (`http://localhost:3449`), open a file in the workspace and + go back to the dashboard — leaving the workspace is what generates the + dashboard thumbnail. Verify the address bar now shows `?file-id=...` + before the `#`. + +4. Hit the endpoint directly (bypasses the user-agent detection): + + ```bash + curl "http://localhost:3449/link-preview?file-id=" + ``` + + Expect HTML with `og:title` containing the file name and `og:image` + pointing to `/assets/by-id/` (or the default image if the file + has no thumbnail yet). + +5. Simulate a real crawler against the root, exercising the full + nginx → rewrite → backend path: + + ```bash + curl -A "Slackbot-LinkExpanding 1.0" "http://localhost:3449/?file-id=" + ``` + + The same URL with a normal user-agent must return the SPA `index.html`. + +6. Verify the thumbnail is public: + + ```bash + curl -I "http://localhost:3449/assets/by-id/" + ``` + + Expect `200` without any session cookie while the flag is on, and `401` + with the flag off (restart the backend after changing flags). + +7. To see the actual preview card rendered by Slack/Discord you need a + publicly reachable URL (`og:image` is built from `PENPOT_PUBLIC_URI`), so + use a tunnel such as ngrok; for local verification the `curl` checks above + are enough. + +## Automated tests + + * `backend/test/backend_tests/http_link_preview_test.clj` — endpoint behavior: + default context, file with/without thumbnail, non-existent and malformed + file ids, deleted file, only-deleted thumbnail, latest-thumbnail revision + ordering, file-name HTML escaping, response headers, file-beats-project + priority, decisive file-id (malformed vs absent with a project id), + project and team links, and flag disabled. + * `backend/test/backend_tests/http_assets_test.clj` + (`objects-handler-file-thumbnail-bucket-link-preview-flag`) — the + `file-thumbnail` bucket is public only while the flag is enabled. + * `frontend/test/frontend_tests/router_test.cljs` — the `navigated` URL + surgery (mirror, skip, stale-strip, clear, unrelated-param + preservation, every-present-id, repeated-key, subpath base). + +## Relevant files + +| File | Role | +|---|---| +| `backend/src/app/http/link_preview.clj` | `/link-preview` handler: flag check, DB lookup, template rendering | +| `backend/resources/app/templates/link-preview.tmpl` | Open Graph HTML template + human redirect script | +| `backend/src/app/http/assets.clj` | Makes `file-thumbnail` bucket public under the flag | +| `backend/src/app/http.clj`, `backend/src/app/main.clj` | Route registration and system wiring | +| `common/src/app/common/flags.cljc` | `:link-preview` flag definition | +| `frontend/src/app/main/router.cljs` | Mirrors context ids on the query string on navigation | +| `docker/devenv/files/nginx.conf` | Devenv crawler detection and `/link-preview` routing | +| `docker/images/files/nginx.conf.template` | Same routing for the production image | diff --git a/frontend/playwright/data/register/get-profile-email-verified.json b/frontend/playwright/data/register/get-profile-email-verified.json new file mode 100644 index 0000000000..8d3cfc9dae --- /dev/null +++ b/frontend/playwright/data/register/get-profile-email-verified.json @@ -0,0 +1,24 @@ +{ + "~:email": "foo@example.com", + "~:is-demo": false, + "~:auth-backend": "penpot", + "~:fullname": "Princesa Leia", + "~:modified-at": "~m1713533116365", + "~:is-active": true, + "~:default-project-id": "~uc7ce0794-0992-8105-8004-38e630f7920b", + "~:id": "~uc7ce0794-0992-8105-8004-38e630f29a9b", + "~:is-muted": false, + "~:default-team-id": "~uc7ce0794-0992-8105-8004-38e630f40f6d", + "~:created-at": "~m1713533116365", + "~:is-blocked": false, + "~:theme": "", + "~:props": { + "~:nudge": { + "~:big": 10, + "~:small": 1 + }, + "~:v2-info-shown": true, + "~:viewed-tutorial?": false, + "~:viewed-walkthrough?": false + } +} diff --git a/frontend/playwright/ui/pages/RegisterPage.js b/frontend/playwright/ui/pages/RegisterPage.js index 324a4fd1c2..5045d34b88 100644 --- a/frontend/playwright/ui/pages/RegisterPage.js +++ b/frontend/playwright/ui/pages/RegisterPage.js @@ -49,6 +49,14 @@ export class RegisterPage extends BasePage { "verify-token", "register/verify-token-email-verified.json", ); + // The settings step reloads the app via page.goto, which refetches + // get-profile on boot. Without this override the initial anonymous + // mock wins again and the app falls back to the login form instead + // of rendering settings. + await this.mockRPC( + "get-profile", + "register/get-profile-email-verified.json", + ); await this.mockRPCs({ "get-teams": "logged-in-user/get-teams-default.json", "get-font-variants?team-id=*": diff --git a/frontend/src/app/main/router.cljs b/frontend/src/app/main/router.cljs index f95f5f521e..cdaa6197cb 100644 --- a/frontend/src/app/main/router.cljs +++ b/frontend/src/app/main/router.cljs @@ -65,6 +65,16 @@ ;; --- Navigate (Event) +(defn get-query-param + "Safely extracts a scalar value for a query param key from a params + map. When the same key appears multiple times in a URL, + query-string->map returns a vector for that key; this function + always returns a single (last) element in that case, so downstream + consumers such as parse-long always receive a plain string or nil." + [params k] + (let [v (get params k)] + (if (sequential? v) (peek v) v))) + (defn navigated [match send-event-info?] (ptk/reify ::navigated @@ -85,7 +95,30 @@ (update [_ state] (-> state (assoc :route match) - (dissoc :exception))))) + (dissoc :exception))) + + ptk/EffectEvent + (effect [_ state _] + ;; The route is read from the state the `update` above just stored: + ;; the effect always runs after the update. The sharing-context ids + ;; are synced into the pre-fragment query (the fragment never reaches + ;; the server, so shared links need them there); every other param is + ;; left untouched, except valueless ones (`?flag`), which the query + ;; codec cannot round-trip and are dropped. The backend applies its + ;; own file > project > team priority, so no filtering is needed + ;; here. + (let [params (:query-params (:route state)) + uri (u/uri (.-href globals/location)) + search (reduce (fn [m k] + (let [v (get-query-param params k)] + (if (some? v) + (assoc m k v) + (dissoc m k)))) + (u/query-string->map (:query uri)) + [:file-id :team-id :project-id]) + href (str (assoc uri :query (u/map->query-string search)))] + (when (not= href (.-href globals/location)) + (.replaceState js/history nil "" href)))))) (defn navigate [id params & {:keys [::replace ::new-window] :as options}] @@ -135,16 +168,6 @@ [state] (dm/get-in state [:route :params :query])) -(defn get-query-param - "Safely extracts a scalar value for a query param key from a params - map. When the same key appears multiple times in a URL, - query-string->map returns a vector for that key; this function - always returns a single (last) element in that case, so downstream - consumers such as parse-long always receive a plain string or nil." - [params k] - (let [v (get params k)] - (if (sequential? v) (peek v) v))) - (defn nav-back [] (ptk/reify ::nav-back diff --git a/frontend/src/app/main/ui.cljs b/frontend/src/app/main/ui.cljs index cb7fe8c87a..2286652cff 100644 --- a/frontend/src/app/main/ui.cljs +++ b/frontend/src/app/main/ui.cljs @@ -6,21 +6,16 @@ (ns app.main.ui (:require - [app.common.data :as d] [app.common.uuid :as uuid] [app.config :as cf] - [app.main.data.common :as dcm] [app.main.data.nitrate :as dnt] [app.main.data.team :as dtm] - [app.main.errors :as errors] [app.main.refs :as refs] - [app.main.repo :as rp] [app.main.router :as rt] [app.main.store :as st] [app.main.ui.context :as ctx] [app.main.ui.debug.icons-preview :refer [icons-preview*]] [app.main.ui.debug.playground :refer [playground*]] - [app.main.ui.ds.product.loader :refer [loader*]] [app.main.ui.error-boundary :refer [error-boundary*]] [app.main.ui.exports.files] [app.main.ui.frame-preview :as frame-preview] @@ -31,10 +26,8 @@ [app.main.ui.releases :refer [release-notes-modal]] [app.main.ui.static :as static] [app.util.dom :as dom] - [app.util.i18n :refer [tr]] [app.util.modules :as mod] [app.util.theme :as theme] - [beicon.v2.core :as rx] [rumext.v2 :as mf])) (def auth-page @@ -55,79 +48,6 @@ (def workspace-page* (mf/lazy #(mod/load 'app.main.ui.workspace/workspace-page*))) -(mf/defc workspace-legacy-redirect* - {::mf/props :obj - ::mf/private true} - [{:keys [project-id file-id page-id layout]}] - (mf/with-effect [] - (->> (rp/cmd! :get-project {:id project-id}) - (rx/subs! (fn [{:keys [team-id]}] - (st/emit! (dcm/go-to-workspace :team-id team-id - :file-id file-id - :page-id page-id - :layout layout))) - errors/on-error))) - [:> loader* - {:title (tr "labels.loading") - :overlay true}]) - -(mf/defc dashboard-legacy-redirect* - {::mf/props :obj - ::mf/private true} - [{:keys [section team-id project-id search-term plugin-url template]}] - (let [section (case section - :dashboard-legacy-search - :dashboard-search - :dashboard-legacy-projects - :dashboard-recent - :dashboard-legacy-files - :dashboard-files - :dashboard-legacy-libraries - :dashboard-libraries - :dashboard-legacy-fonts - :dashboard-fonts - :dashboard-legacy-font-providers - :dashboard-font-providers - :dashboard-legacy-team-members - :dashboard-members - :dashboard-legacy-team-invitations - :dashboard-invitations - :dashboard-legacy-team-webhooks - :dashboard-webhooks - :dashboard-legacy-team-settings - :dashboard-settings)] - - (mf/with-effect [] - (let [params {:team-id team-id - :project-id project-id - :search-term search-term - :plugin plugin-url - :template template}] - (st/emit! (rt/nav section (d/without-nils params))))) - - [:> loader* - {:title (tr "labels.loading") - :overlay true}])) - -(mf/defc viewer-legacy-redirect* - {::mf/props :obj - ::mf/private true} - [{:keys [page-id file-id section index share-id interactions-mode frame-id share]}] - (mf/with-effect [] - (let [params {:page-id page-id - :file-id file-id - :section section - :index index - :share-id share-id - :interactions-mode interactions-mode - :frame-id frame-id - :share share}] - (st/emit! (rt/nav :viewer (d/without-nils params))))) - - [:> loader* - {:title (tr "labels.loading") - :overlay true}]) - (mf/defc team-container* {::mf/props :obj ::mf/private true} @@ -315,57 +235,6 @@ :share share}]]) - :workspace-legacy - (let [project-id (some-> params :path :project-id uuid/parse*) - file-id (some-> params :path :file-id uuid/parse*) - page-id (some-> params :query :page-id uuid/parse*) - layout (some-> params :query :layout keyword)] - - [:> workspace-legacy-redirect* - {:project-id project-id - :file-id file-id - :page-id page-id - :layout layout}]) - - (:dashboard-legacy-search - :dashboard-legacy-projects - :dashboard-legacy-files - :dashboard-legacy-libraries - :dashboard-legacy-fonts - :dashboard-legacy-font-providers - :dashboard-legacy-team-members - :dashboard-legacy-team-invitations - :dashboard-legacy-team-webhooks - :dashboard-legacy-team-settings) - (let [team-id (some-> params :path :team-id uuid/parse*) - project-id (some-> params :path :project-id uuid/parse*) - search-term (some-> params :query :search-term) - plugin-url (some-> params :query :plugin) - template (some-> params :template)] - [:> dashboard-legacy-redirect* - {:team-id team-id - :section section - :project-id project-id - :search-term search-term - :plugin-url plugin-url - :template template}]) - - :viewer-legacy - (let [{:keys [query-params path-params]} route - {:keys [index share-id section page-id interactions-mode frame-id share] - :or {section :interactions interactions-mode :show-on-click}} query-params - {:keys [file-id]} path-params] - - [:> viewer-legacy-redirect* - {:page-id page-id - :file-id file-id - :section section - :index index - :share-id share-id - :interactions-mode (keyword interactions-mode) - :frame-id frame-id - :share share}]) - :frame-preview [:> frame-preview/frame-preview*] diff --git a/frontend/src/app/main/ui/routes.cljs b/frontend/src/app/main/ui/routes.cljs index 6502958f1e..27f4ed8812 100644 --- a/frontend/src/app/main/ui/routes.cljs +++ b/frontend/src/app/main/ui/routes.cljs @@ -55,8 +55,6 @@ ["/view" :viewer] - ["/view/:file-id" :viewer-legacy] - (when *assert* ["/debug/icons-preview" :debug-icons-preview]) @@ -79,20 +77,7 @@ ["/files" :dashboard-files] ["/deleted" :dashboard-deleted]] - ["/dashboard/team/:team-id" - ["/members" :dashboard-legacy-team-members] - ["/invitations" :dashboard-legacy-team-invitations] - ["/webhooks" :dashboard-legacy-team-webhooks] - ["/settings" :dashboard-legacy-team-settings] - ["/projects" :dashboard-legacy-projects] - ["/search" :dashboard-legacy-search] - ["/fonts" :dashboard-legacy-fonts] - ["/fonts/providers" :dashboard-legacy-font-providers] - ["/libraries" :dashboard-legacy-libraries] - ["/projects/:project-id" :dashboard-legacy-files]] - - ["/workspace" :workspace] - ["/workspace/:project-id/:file-id" :workspace-legacy]]) + ["/workspace" :workspace]]) (defn- store-session-params diff --git a/frontend/test/frontend_tests/router_test.cljs b/frontend/test/frontend_tests/router_test.cljs new file mode 100644 index 0000000000..a3ba3763e7 --- /dev/null +++ b/frontend/test/frontend_tests/router_test.cljs @@ -0,0 +1,121 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns frontend-tests.router-test + (:require + [app.main.router :as rt] + [app.util.globals :as globals] + [cljs.test :as t :include-macros true] + [potok.v2.core :as ptk])) + +(defn- with-stubbed-href + "Run `thunk` with the `globals/location` href replaced and a recording + `js/history.replaceState`. Only the href is stubbed: the effect parses + everything it needs out of it." + [href replace-calls thunk] + (let [loc globals/location + old-href (.-href loc) + old-history (.-history js/globalThis)] + (set! (.-href loc) href) + (set! (.-history js/globalThis) + #js {:replaceState (fn [_ _ url] (swap! replace-calls conj url))}) + (try + (thunk) + (finally + (set! (.-href loc) old-href) + (set! (.-history js/globalThis) old-history))))) + +(defn- emit-navigated + "Run the `navigated` effect with `match` stored as the state route. + The closed-over match is deliberately empty to prove the effect reads + the route from the state, not from the closure." + [match] + (ptk/effect (rt/navigated {} false) {:route match} nil)) + +(t/deftest navigated-mirrors-context-on-change + ;; New context in the state route triggers exactly one mirrored write. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/#/workspace?file-id=file-1" + calls + (fn [] + (emit-navigated {:query-params {:file-id "file-1"}}) + (t/is (= ["http://localhost/?file-id=file-1#/workspace?file-id=file-1"] @calls)))))) + +(t/deftest navigated-skips-write-when-mirrored + ;; When the URL already carries the mirrored context, nothing is written. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/?file-id=file-1#/workspace?file-id=file-1" + calls + (fn [] + (emit-navigated {:query-params {:file-id "file-1"}}) + (t/is (= [] @calls)))))) + +(t/deftest navigated-strips-stale-context + ;; A stale pre-fragment query is replaced with the current context. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/?file-id=old#/dashboard/recent?team-id=team-1" + calls + (fn [] + (emit-navigated {:query-params {:team-id "team-1"}}) + (t/is (= ["http://localhost/?team-id=team-1#/dashboard/recent?team-id=team-1"] @calls)))))) + +(t/deftest navigated-clears-query-without-context + ;; Routes without context clear a stale pre-fragment query. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/?file-id=old#/auth/login" + calls + (fn [] + (emit-navigated {:query-params {:token "some-token"}}) + (t/is (= ["http://localhost/#/auth/login"] @calls)))))) + +(t/deftest navigated-preserves-unrelated-params + ;; Params owned by other code are kept as they are. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/?debug=1&file-id=old#/workspace?file-id=file-1" + calls + (fn [] + (emit-navigated {:query-params {:file-id "file-1"}}) + (t/is (= ["http://localhost/?debug=1&file-id=file-1#/workspace?file-id=file-1"] @calls)))))) + +(t/deftest navigated-mirrors-every-present-context-id + ;; Every present context id is mirrored; the backend applies its own + ;; file > project > team priority, so no filtering happens here. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/#/workspace?file-id=file-1&team-id=team-1&project-id=project-1&page-id=page-1" + calls + (fn [] + (emit-navigated {:query-params {:file-id "file-1" + :team-id "team-1" + :project-id "project-1" + :page-id "page-1"}}) + (t/is (= ["http://localhost/?file-id=file-1&team-id=team-1&project-id=project-1#/workspace?file-id=file-1&team-id=team-1&project-id=project-1&page-id=page-1"] + @calls)))))) + +(t/deftest navigated-repeated-key-last-wins + ;; A repeated query key arrives as a vector; the last value wins. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/#/workspace?file-id=file-1" + calls + (fn [] + (emit-navigated {:query-params {:file-id ["file-old" "file-1"]}}) + (t/is (= ["http://localhost/?file-id=file-1#/workspace?file-id=file-1"] @calls)))))) + +(t/deftest navigated-keeps-subpath-base + ;; Under a subpath deployment the prefix survives untouched. + (let [calls (atom [])] + (with-stubbed-href + "http://localhost/penpot/#/workspace?file-id=file-1" + calls + (fn [] + (emit-navigated {:query-params {:file-id "file-1"}}) + (t/is (= ["http://localhost/penpot/?file-id=file-1#/workspace?file-id=file-1"] @calls)))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index f54fe5cc67..93dbcdad06 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -73,6 +73,7 @@ [frontend-tests.render-wasm.process-objects-test] [frontend-tests.render-wasm.text-editor-apply-styles-test] [frontend-tests.render-wasm.text-editor-caret-color-test] + [frontend-tests.router-test] [frontend-tests.svg-fills-test] [frontend-tests.text-editor-paste-guard-test] [frontend-tests.tokens.copy-paste-props-test] @@ -185,6 +186,7 @@ 'frontend-tests.render-wasm.process-objects-test 'frontend-tests.render-wasm.text-editor-apply-styles-test 'frontend-tests.render-wasm.text-editor-caret-color-test + 'frontend-tests.router-test 'frontend-tests.svg-fills-test 'frontend-tests.tokens.copy-paste-props-test 'frontend-tests.tokens.import-export-test