From 5e1ced03eaad9193fce4f0ef46162ea316bd836c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Valderrama?= Date: Wed, 26 Aug 2026 08:08:40 +0200 Subject: [PATCH 1/9] :bug: Fix missing warning when moving a team (#11357) --- backend/src/app/rpc/commands/nitrate.clj | 13 ++-- .../test/backend_tests/rpc_nitrate_test.clj | 60 ++++++++++++++++++- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/backend/src/app/rpc/commands/nitrate.clj b/backend/src/app/rpc/commands/nitrate.clj index a476ce0dbf..8714c5c841 100644 --- a/backend/src/app/rpc/commands/nitrate.clj +++ b/backend/src/app/rpc/commands/nitrate.clj @@ -458,13 +458,14 @@ (let [emails (map :email (noh/get-team-invitation-emails conn team-id))] (if (empty? emails) {:allows-anybody false :external-emails []} - (let [emails-array (db/create-array conn "text" (vec emails)) - profiles (db/exec! conn [sql:get-profiles-by-emails emails-array]) + (let [emails-array (db/create-array conn "text" (vec emails)) + profiles (db/exec! conn [sql:get-profiles-by-emails emails-array]) organization-member-ids (into #{} (nitrate/call cfg :get-organization-members {:organization-id organization-id})) - external-emails (->> profiles - (remove #(contains? organization-member-ids (:id %))) - (map :email) - (vec))] + member-emails (->> profiles + (filter #(contains? organization-member-ids (:id %))) + (map :email) + (into #{})) + external-emails (into [] (remove member-emails emails))] {:allows-anybody false :external-emails external-emails})))))) (def ^:private schema:add-team-to-organization diff --git a/backend/test/backend_tests/rpc_nitrate_test.clj b/backend/test/backend_tests/rpc_nitrate_test.clj index 90b746e2a1..8519021194 100644 --- a/backend/test/backend_tests/rpc_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_nitrate_test.clj @@ -1168,12 +1168,68 @@ @set-team-params)) (let [emails (->> @sent (map :to) set)] - (t/is (= 2 (count @sent))) - (t/is (= #{"member302@example.com" "external301@example.com"} emails)) + (t/is (= 1 (count @sent))) + (t/is (= #{"member302@example.com"} emails)) (doseq [email-params @sent] (t/is (= organization-name (:organization-name email-params))) (t/is (= eml/organization-setup-sso (::eml/factory email-params))))))) +(t/deftest add-team-to-organization-deletes-external-invitations-for-unregistered-users + (let [owner (th/create-profile* 305 {:is-active true + :fullname "Owner" + :email "owner305@example.com"}) + member (th/create-profile* 306 {:is-active true + :fullname "Member" + :email "member306@example.com"}) + team (th/create-team* 305 {:profile-id (:id owner)}) + _ (th/create-team-role* {:team-id (:id team) + :profile-id (:id member) + :role :editor}) + organization-id (uuid/random) + organization-summary {:id organization-id + :name "Test Org" + :owner-id (:id owner) + :teams []} + organization-perms {:owner-id (:id owner) + :permissions {:create-teams "any" + :move-teams "always" + :new-team-members "members"}}] + + (th/db-insert! :team-invitation + {:id (uuid/random) + :team-id (:id team) + :org-id nil + :email-to "unregistered@example.com" + :created-by (:id owner) + :role "editor" + :valid-until (ct/in-future "48h")}) + (th/db-insert! :team-invitation + {:id (uuid/random) + :team-id (:id team) + :org-id nil + :email-to "unregistered2@example.com" + :created-by (:id owner) + :role "editor" + :valid-until (ct/in-future "48h")}) + + (with-redefs [cf/flags (conj cf/flags :admin-console) + nitrate/call (add-team-to-organization-nitrate-mock + {:organization-id organization-id + :organization-summary organization-summary + :organization-perms organization-perms + :owner-id (:id owner) + :team-id (:id team) + :sso-active? false}) + teams/initialize-user-in-organization (fn [& _] nil)] + (let [out (th/command! {::th/type :add-team-to-organization + ::rpc/profile-id (:id owner) + :team-id (:id team) + :organization-id organization-id})] + (t/is (th/success? out)))) + + (let [remaining (th/db-query :team-invitation {:team-id (:id team)})] + (t/is (empty? remaining) "Both external invitations should be deleted")))) + (t/deftest create-team-in-organization-passes-association-to-nitrate (let [organization-id (uuid/random) team {:id (uuid/random) From b33213787e2eb4f52fade3a257ce82f76b3b3221 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 26 Aug 2026 08:33:20 +0200 Subject: [PATCH 2/9] :bug: Add accumulated storage byte quota for media uploads (#11038) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :bug: Add accumulated storage byte quota for media uploads Add media-storage-bytes-per-team quote to prevent persistent DoS via repeated uploads. The quota sums storage_object sizes from both file_media_object (media + thumbnails) and team_font_variant (otf/ttf/woff1/woff2). Default limit is 20 GiB per team, configurable via PENPOT_QUOTES_MEDIA_STORAGE_BYTES_PER_TEAM. The check is invoked in upload-file-media-object before processing, looking up the team-id via file -> project -> team_id join. AI-assisted-by: mimo-v2.5-pro * :bug: Fix deduplicated storage overcounting in media-storage-bytes-per-team quote The SQL query sql:get-media-storage-bytes-per-team used UNION ALL across six SELECT branches that each produce a so_id reference. When deduplication causes multiple file_media_object or team_font_variant rows to point at the same storage_object, UNION ALL counts that objects size once per reference — inflating "used bytes" and causing false :max-quote-reached rejections. Change all five UNION ALL to UNION so that duplicate so_id values are collapsed before the JOIN storage_object / SUM(so.size). Add a test (media-storage-bytes-quote-deduped) that creates one storage_object referenced by two file_media_object rows and asserts the computed usage reflects the deduplicated physical size, not 2x. AI-assisted-by: mimo-v2.5-pro --- backend/src/app/config.clj | 1 + backend/src/app/rpc/commands/media.clj | 12 ++ backend/src/app/rpc/quotes.clj | 70 +++++++++ .../test/backend_tests/rpc_quotes_test.clj | 134 ++++++++++++++++++ 4 files changed, 217 insertions(+) diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index f02136b1ca..3979f399d7 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -194,6 +194,7 @@ [:quotes-team-access-requests-per-requester {:optional true} ::sm/int] [:quotes-upload-sessions-per-profile {:optional true} ::sm/int] [:quotes-upload-chunks-per-session {:optional true} ::sm/int] + [:quotes-media-storage-bytes-per-team {:optional true} ::sm/int] [:auth-token-cookie-name {:optional true} :string] [:auth-token-cookie-max-age {:optional true} ::ct/duration] diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index 3dff04fa10..a5e1b9672a 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -40,6 +40,12 @@ (declare create-file-media-object) +(def ^:private sql:get-team-id-for-file + "SELECT p.team_id + FROM file AS f + JOIN project AS p ON (p.id = f.project_id) + WHERE f.id = ?") + (def ^:private schema:upload-file-media-object [:map {:title "upload-file-media-object"} [:id {:optional true} ::sm/uuid] @@ -58,6 +64,12 @@ (media.v/validate-media-type! content) (media.v/validate-media-size! content) + (let [team-id (:team-id (db/exec-one! pool [sql:get-team-id-for-file file-id]))] + (quotes/check! cfg {::quotes/id ::quotes/media-storage-bytes-per-team + ::quotes/profile-id profile-id + ::quotes/team-id team-id + ::quotes/incr (:size content)})) + (db/run! cfg (fn [{:keys [::db/conn] :as cfg}] ;; We get the minimal file for proper checking if ;; file is not already deleted diff --git a/backend/src/app/rpc/quotes.clj b/backend/src/app/rpc/quotes.clj index 0a7004cc54..e391f0b0ac 100644 --- a/backend/src/app/rpc/quotes.clj +++ b/backend/src/app/rpc/quotes.clj @@ -546,6 +546,76 @@ (assoc ::count-sql [sql:get-upload-sessions-per-profile profile-id]) (generic-check!))) +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; QUOTE: MEDIA-STORAGE-BYTES-PER-TEAM +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; + +(def ^:private schema:media-storage-bytes-per-team + [:map + [::profile-id ::sm/uuid] + [::team-id ::sm/uuid]]) + +(def ^:private valid-media-storage-bytes-per-team-quote? + (sm/lazy-validator schema:media-storage-bytes-per-team)) + +(def ^:private sql:get-media-storage-bytes-per-team + "SELECT COALESCE(SUM(so.size), 0) AS total + FROM ( + SELECT fmo.media_id AS so_id + FROM file_media_object AS fmo + JOIN file AS f ON (f.id = fmo.file_id) + JOIN project AS p ON (p.id = f.project_id) + WHERE p.team_id = ? + AND fmo.deleted_at IS NULL + AND f.deleted_at IS NULL + UNION + SELECT fmo.thumbnail_id AS so_id + FROM file_media_object AS fmo + JOIN file AS f ON (f.id = fmo.file_id) + JOIN project AS p ON (p.id = f.project_id) + WHERE p.team_id = ? + AND fmo.thumbnail_id IS NOT NULL + AND fmo.deleted_at IS NULL + AND f.deleted_at IS NULL + UNION + SELECT v.otf_file_id AS so_id + FROM team_font_variant AS v + WHERE v.team_id = ? + AND v.otf_file_id IS NOT NULL + AND v.deleted_at IS NULL + UNION + SELECT v.ttf_file_id AS so_id + FROM team_font_variant AS v + WHERE v.team_id = ? + AND v.ttf_file_id IS NOT NULL + AND v.deleted_at IS NULL + UNION + SELECT v.woff1_file_id AS so_id + FROM team_font_variant AS v + WHERE v.team_id = ? + AND v.woff1_file_id IS NOT NULL + AND v.deleted_at IS NULL + UNION + SELECT v.woff2_file_id AS so_id + FROM team_font_variant AS v + WHERE v.team_id = ? + AND v.woff2_file_id IS NOT NULL + AND v.deleted_at IS NULL + ) AS refs + JOIN storage_object AS so ON (so.id = refs.so_id) + WHERE so.deleted_at IS NULL") + +(defmethod check-quote ::media-storage-bytes-per-team + [{:keys [::profile-id ::team-id ::target] :as quote}] + (assert (valid-media-storage-bytes-per-team-quote? quote) "invalid quote parameters") + (-> quote + (assoc ::default (cf/get :quotes-media-storage-bytes-per-team + (* 20 1024 1024 1024))) + (assoc ::quote-sql [sql:get-quotes-2 target team-id profile-id profile-id]) + (assoc ::count-sql [sql:get-media-storage-bytes-per-team + team-id team-id team-id team-id team-id team-id]) + (generic-check!))) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; QUOTE: DEFAULT ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/backend/test/backend_tests/rpc_quotes_test.clj b/backend/test/backend_tests/rpc_quotes_test.clj index 94db804e17..b279ff96d3 100644 --- a/backend/test/backend_tests/rpc_quotes_test.clj +++ b/backend/test/backend_tests/rpc_quotes_test.clj @@ -338,3 +338,137 @@ (check-ok! 4) (check-ko! 5)))) + +(t/deftest media-storage-bytes-per-team-quote + (with-mocks [mock {:target 'app.config/get + :return (th/config-get-mock + {:quotes-media-storage-bytes-per-team 1000})}] + + (let [profile-1 (th/create-profile* 1) + profile-2 (th/create-profile* 2) + team-id (:default-team-id profile-1) + data {::quotes/id ::quotes/media-storage-bytes-per-team + ::quotes/profile-id (:id profile-1) + ::quotes/team-id team-id + ::quotes/incr 500} + + check-ok! (fn [msg] + (quotes/check! th/*system* data) + (t/is (true? true) msg)) + check-ko! (fn [msg] + (try + (quotes/check! th/*system* data) + (t/is false (str msg " — expected exception but none thrown")) + (catch Exception e + (let [ed (ex-data e)] + (t/is (= :restriction (:type ed))) + (t/is (= :max-quote-reached (:code ed))) + (t/is (= "media-storage-bytes-per-team" (:target ed)))))))] + + ;; Under default limit (1000) with incr=500 and no existing storage — ok + (check-ok! "first check under limit") + + ;; Insert a quote row for another profile on the same team — does not help + (th/db-insert! :usage-quote + {:profile-id (:id profile-2) + :target "media-storage-bytes-per-team" + :quote 100}) + + ;; Insert a team+profile quote that is still too low + (th/db-insert! :usage-quote + {:team-id team-id + :profile-id (:id profile-2) + :target "media-storage-bytes-per-team" + :quote 200}) + + ;; Insert a team-level quote (no profile) that is still too low + (th/db-insert! :usage-quote + {:team-id team-id + :target "media-storage-bytes-per-team" + :quote 400}) + + ;; total=0, incr=500, best quote=400 → 0+500 > 400 → blocked + (check-ko! "blocked by team-level quote") + + ;; Insert a team+profile quote that allows it + (th/db-insert! :usage-quote + {:team-id team-id + :profile-id (:id profile-1) + :target "media-storage-bytes-per-team" + :quote 1000}) + + ;; total=0, incr=500, best quote=1000 → 0+500 <= 1000 → ok + (check-ok! "allowed by team+profile quote")))) + +(t/deftest media-storage-bytes-quote-deduped + (with-mocks [mock {:target 'app.config/get + :return (th/config-get-mock + {:quotes-media-storage-bytes-per-team 1100})}] + + (let [prof (th/create-profile* 1) + team-id (:default-team-id prof) + proj (th/create-project* 1 {:profile-id (:id prof) + :team-id team-id}) + file1 (th/create-file* 1 {:profile-id (:id prof) + :project-id (:id proj) + :is-shared false}) + file2 (th/create-file* 2 {:profile-id (:id prof) + :project-id (:id proj) + :is-shared false}) + + ;; One physical storage object of 500 bytes + so-id (uuid/random) + _ (th/db-insert! :storage-object {:id so-id + :size 500 + :backend "test"}) + + ;; Two file_media_object rows pointing at the SAME storage object + ;; (simulates the deduplication path: same content uploaded twice) + _ (th/create-file-media-object* + {:file-id (:id file1) :media-id so-id + :name "icon" :mtype "image/svg+xml"}) + _ (th/create-file-media-object* + {:file-id (:id file2) :media-id so-id + :name "icon" :mtype "image/svg+xml"}) + + data {::quotes/id ::quotes/media-storage-bytes-per-team + ::quotes/profile-id (:id prof) + ::quotes/team-id team-id + ::quotes/incr 200}] + + ;; Physical size is 500. With UNION (correct), total=500, 500+200=700 ≤ 1100 → ok. + ;; With UNION ALL (buggy), total=1000, 1000+200=1200 > 1100 → rejected. + (quotes/check! th/*system* data) + (t/is (true? true) "deduped storage counted once, under quota")))) + +(t/deftest media-upload-enforces-storage-quote + (with-mocks [mock {:target 'app.config/get + :return (th/config-get-mock + {:quotes-media-storage-bytes-per-team 100})}] + + (let [prof (th/create-profile* 1) + proj (th/create-project* 1 {:profile-id (:id prof) + :team-id (:default-team-id prof)}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:id proj) + :is-shared false}) + mfile {:filename "sample.jpg" + :path (th/tempfile "backend_tests/test_files/sample.jpg") + :mtype "image/jpeg" + :size 312043} + + params {::th/type :upload-file-media-object + ::rpc/profile-id (:id prof) + :file-id (:id file) + :is-local true + :name "testfile" + :content mfile} + + out (th/command! params)] + + ;; 312043 bytes > 100 byte limit → should be rejected + (t/is (not (th/success? out))) + (let [error (:error out)] + (t/is (= :restriction (th/ex-type error))) + (t/is (= :max-quote-reached (th/ex-code error))) + (t/is (= "media-storage-bytes-per-team" (:target (ex-data error)))))))) From 7079d33ae197e4d9a7f62dca0ec6fee9365217a8 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 26 Aug 2026 13:05:36 +0200 Subject: [PATCH 3/9] :bug: Enforce ownership check on tempfile bucket access (#11270) * :bug: Enforce ownership check on tempfile bucket access The upload-tempfile RPC stores profile-id with tempfile objects, but objects-handler never verified the requester was the owner. Any authenticated user who knew the UUID could access the tempfile. Add ownership check: tempfile bucket now requires the request's profile-id to match the stored profile-id. Returns 404 on mismatch (not 403) to avoid leaking object existence. Legacy tempfiles without stored profile-id remain accessible to any authenticated user for backward compatibility. Closes #11269 AI-assisted-by: qwen3.7-plus * :recycle: Extract tempfile-bucket constant and fix docstring indentation Extract the 'tempfile' bucket string literal into a named constant (sto/tempfile-bucket) to prevent typos and make future bucket renames trivial. Updated 9 occurrences across 7 files. Also fixed minor docstring indentation inconsistency in authenticated? function. AI-assisted-by: qwen3.7-plus * :recycle: Refactor process-bucket! and authenticated? helpers Replace case with cond in process-bucket! to properly resolve sto/tempfile-bucket var from another namespace (case does not evaluate qualified vars at compile time). Redefine authenticated? in terms of request-profile-id to remove duplicated lookup logic. Closes #11269 AI-assisted-by: mimo-v2.5-pro --- backend/src/app/http/assets.clj | 25 +++++- backend/src/app/rpc/commands/binfile.clj | 2 +- backend/src/app/rpc/commands/fonts.clj | 2 +- backend/src/app/rpc/commands/media.clj | 2 +- backend/src/app/rpc/management/exporter.clj | 2 +- backend/src/app/storage.clj | 8 +- backend/src/app/storage/gc_touched.clj | 21 ++--- .../test/backend_tests/http_assets_test.clj | 82 +++++++++++++++++-- 8 files changed, 120 insertions(+), 24 deletions(-) diff --git a/backend/src/app/http/assets.clj b/backend/src/app/http/assets.clj index 22783be1e2..b0adb45b13 100644 --- a/backend/src/app/http/assets.clj +++ b/backend/src/app/http/assets.clj @@ -95,17 +95,32 @@ (let [bucket (-> obj meta :bucket)] (not (contains? public-buckets bucket)))) +(defn- request-profile-id + "Extract the authenticated profile-id from the request." + [request] + (or (::session/profile-id request) + (::actoken/profile-id request))) + (defn- authenticated? "Check if the request has an authenticated profile, either via session or access token." [request] - (or (some? (::session/profile-id request)) - (some? (::actoken/profile-id request)))) + (some? (request-profile-id request))) + +(defn- tempfile-owner-match? + "Check if the request's profile-id matches the tempfile's stored owner. + Returns true if no profile-id was stored (legacy objects)." + [obj request] + (let [stored-profile-id (:profile-id (meta obj)) + request-profile-id (request-profile-id request)] + (or (nil? stored-profile-id) + (= stored-profile-id request-profile-id)))) (defn objects-handler "Handler that serves storage objects by id. For non-public buckets (e.g. profile), requires authentication - via session cookie or access token." + via session cookie or access token. + For tempfile bucket, also requires ownership (profile-id match)." [{:keys [::sto/storage] :as cfg} request] (let [id (get-id request) obj (sto/get-object storage id)] @@ -117,6 +132,10 @@ (not (authenticated? request))) {::yres/status 401} + (and (= (-> obj meta :bucket) sto/tempfile-bucket) + (not (tempfile-owner-match? obj request))) + {::yres/status 404} + :else (serve-object cfg obj)))) diff --git a/backend/src/app/rpc/commands/binfile.clj b/backend/src/app/rpc/commands/binfile.clj index ec4510200d..44b7014968 100644 --- a/backend/src/app/rpc/commands/binfile.clj +++ b/backend/src/app/rpc/commands/binfile.clj @@ -60,7 +60,7 @@ {::sto/content data ::sto/touched-at (ct/in-future {:minutes 60}) :content-type "application/zip" - :bucket "tempfile"})] + :bucket sto/tempfile-bucket})] (-> (cf/get :public-uri) (u/join "/assets/by-id/") diff --git a/backend/src/app/rpc/commands/fonts.clj b/backend/src/app/rpc/commands/fonts.clj index 7b5ac6ac4e..4ab02627bc 100644 --- a/backend/src/app/rpc/commands/fonts.clj +++ b/backend/src/app/rpc/commands/fonts.clj @@ -353,7 +353,7 @@ ::sto/touched-at (ct/in-future {:minutes 30}) :profile-id profile-id :content-type mtype - :bucket "tempfile"}] + :bucket sto/tempfile-bucket}] (sto/put-object! storage content))) diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index a5e1b9672a..ffa94d5a6b 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -379,7 +379,7 @@ ::sto/deduplicate? false ::sto/touch true :content-type (:mtype content) - :bucket "tempfile" + :bucket sto/tempfile-bucket :upload-id (str session-id) :chunk-index index})) diff --git a/backend/src/app/rpc/management/exporter.clj b/backend/src/app/rpc/management/exporter.clj index f4b7d9547f..f6cdbdd820 100644 --- a/backend/src/app/rpc/management/exporter.clj +++ b/backend/src/app/rpc/management/exporter.clj @@ -43,7 +43,7 @@ ::sto/touched-at (ct/in-future {:minutes 10}) :profile-id profile-id :content-type (:mtype content) - :bucket "tempfile"} + :bucket sto/tempfile-bucket} object (sto/put-object! storage content)] {:id (:id object) :uri (-> (cf/get :public-uri) diff --git a/backend/src/app/storage.clj b/backend/src/app/storage.clj index f30d8762ec..019536e233 100644 --- a/backend/src/app/storage.clj +++ b/backend/src/app/storage.clj @@ -38,6 +38,10 @@ (def default-bucket "file-media-object") +(def tempfile-bucket + "Bucket name for temporary file uploads (10-minute expiry)." + "tempfile") + (def valid-buckets #{"file-media-object" "team-font-variant" @@ -45,7 +49,7 @@ "file-thumbnail" "profile" "organization" - "tempfile" + tempfile-bucket "file-data" "file-data-fragment" "file-change"}) @@ -136,7 +140,7 @@ result (when (and (::deduplicate? params) (:hash mdata) (:bucket mdata) - (not= "tempfile" (:bucket mdata))) + (not= tempfile-bucket (:bucket mdata))) (let [result (get-database-object-by-hash connectable backend (:bucket mdata) (:hash mdata))] diff --git a/backend/src/app/storage/gc_touched.clj b/backend/src/app/storage/gc_touched.clj index b7ace59ef3..49bce333ed 100644 --- a/backend/src/app/storage/gc_touched.clj +++ b/backend/src/app/storage/gc_touched.clj @@ -149,7 +149,7 @@ :status "delete" :bucket bucket) (recur to-freeze (conj to-delete id) (rest objects)))) - (let [deletion-delay (if (= "tempfile" bucket) + (let [deletion-delay (if (= sto/tempfile-bucket bucket) (ct/duration {:hours 2}) (cf/get-deletion-delay))] (some->> (seq to-freeze) (mark-freeze-in-bulk! conn)) @@ -158,15 +158,16 @@ (defn- process-bucket! [conn bucket objects] - (case bucket - "file-media-object" (process-objects! conn has-file-media-object-refs? bucket objects) - "team-font-variant" (process-objects! conn has-team-font-variant-refs? bucket objects) - "file-object-thumbnail" (process-objects! conn has-file-object-thumbnails-refs? bucket objects) - "file-thumbnail" (process-objects! conn has-file-thumbnails-refs? bucket objects) - "profile" (process-objects! conn has-profile-refs? bucket objects) - "file-data" (process-objects! conn has-file-data-refs? bucket objects) - "tempfile" (process-objects! conn (constantly false) bucket objects) - "organization" (process-objects! conn (constantly false) bucket objects) + (cond + (= bucket "file-media-object") (process-objects! conn has-file-media-object-refs? bucket objects) + (= bucket "team-font-variant") (process-objects! conn has-team-font-variant-refs? bucket objects) + (= bucket "file-object-thumbnail") (process-objects! conn has-file-object-thumbnails-refs? bucket objects) + (= bucket "file-thumbnail") (process-objects! conn has-file-thumbnails-refs? bucket objects) + (= bucket "profile") (process-objects! conn has-profile-refs? bucket objects) + (= bucket "file-data") (process-objects! conn has-file-data-refs? bucket objects) + (= bucket sto/tempfile-bucket) (process-objects! conn (constantly false) sto/tempfile-bucket objects) + (= bucket "organization") (process-objects! conn (constantly false) bucket objects) + :else (ex/raise :type :internal :code :unexpected-unknown-reference :hint (dm/fmt "unknown reference '%'" bucket)))) diff --git a/backend/test/backend_tests/http_assets_test.clj b/backend/test/backend_tests/http_assets_test.clj index e4f5ebff43..d689e9d2a6 100644 --- a/backend/test/backend_tests/http_assets_test.clj +++ b/backend/test/backend_tests/http_assets_test.clj @@ -37,11 +37,16 @@ (assoc storage ::sto/backend :fs)) (defn- create-storage-object! - "Create a storage object with the given bucket and content." - [storage bucket content] - (sto/put-object! storage {::sto/content (sto/content content) - :bucket bucket - :content-type "text/plain"})) + "Create a storage object with the given bucket and content. + Optional opts map can include :profile-id to set the owner." + ([storage bucket content] + (create-storage-object! storage bucket content {})) + ([storage bucket content {:keys [profile-id]}] + (sto/put-object! storage (cond-> {::sto/content (sto/content content) + :bucket bucket + :content-type "text/plain"} + (some? profile-id) + (assoc :profile-id profile-id))))) (defn- make-handler-cfg "Build a minimal cfg map for the assets handlers." @@ -708,3 +713,70 @@ ::session/profile-id (:id profile)} response (assets/objects-handler cfg request)] (t/is (= 404 (::yres/status response))))) + +;; ---------------------------------------------------------------- +;; Tests: objects-handler — tempfile bucket ownership (T9-F-10) +;; ---------------------------------------------------------------- + +(t/deftest objects-handler-tempfile-owner-can-access + ;; Owner of a tempfile should be able to access it via session auth. + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + cfg (make-handler-cfg storage) + owner (th/create-profile* 1) + object (create-storage-object! storage "tempfile" "temp data" {:profile-id (:id owner)}) + request {:path-params {:id (str (:id object))} + ::session/profile-id (:id owner)} + response (assets/objects-handler cfg request)] + (t/is (= 204 (::yres/status response))))) + +(t/deftest objects-handler-tempfile-non-owner-gets-404 + ;; Non-owner accessing a tempfile should get 404 (not 403, to avoid leaking existence). + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + cfg (make-handler-cfg storage) + owner (th/create-profile* 1) + stranger (th/create-profile* 2) + object (create-storage-object! storage "tempfile" "temp data" {:profile-id (:id owner)}) + request {:path-params {:id (str (:id object))} + ::session/profile-id (:id stranger)} + response (assets/objects-handler cfg request)] + (t/is (= 404 (::yres/status response))))) + +(t/deftest objects-handler-tempfile-access-token-owner-can-access + ;; Owner of a tempfile should be able to access it via access token auth. + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + cfg (make-handler-cfg storage) + owner (th/create-profile* 1) + object (create-storage-object! storage "tempfile" "temp data" {:profile-id (:id owner)}) + request {:path-params {:id (str (:id object))} + ::actoken/profile-id (:id owner)} + response (assets/objects-handler cfg request)] + (t/is (= 204 (::yres/status response))))) + +(t/deftest objects-handler-tempfile-access-token-non-owner-gets-404 + ;; Non-owner accessing a tempfile via access token should get 404. + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + cfg (make-handler-cfg storage) + owner (th/create-profile* 1) + stranger (th/create-profile* 2) + object (create-storage-object! storage "tempfile" "temp data" {:profile-id (:id owner)}) + request {:path-params {:id (str (:id object))} + ::actoken/profile-id (:id stranger)} + response (assets/objects-handler cfg request)] + (t/is (= 404 (::yres/status response))))) + +(t/deftest objects-handler-tempfile-no-stored-profile-id-serves + ;; Legacy tempfile objects without stored profile-id should be accessible + ;; to any authenticated user (backward compatibility). + (let [storage (-> (:app.storage/storage th/*system*) + (configure-storage-backend)) + cfg (make-handler-cfg storage) + stranger (th/create-profile* 1) + object (create-storage-object! storage "tempfile" "legacy temp data") + request {:path-params {:id (str (:id object))} + ::session/profile-id (:id stranger)} + response (assets/objects-handler cfg request)] + (t/is (= 204 (::yres/status response))))) From 4adfa5d2f2f6e15df603fc00add6e7fab3f99c47 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 25 Aug 2026 13:35:21 +0000 Subject: [PATCH 4/9] :bug: Require edition permissions for get-team-invitation-token The handler previously allowed any team member (including viewers) to generate invitation tokens. Now requires at least edition-level permissions (can-edit, admin, or owner). Closes #11358 AI-assisted-by: longcat-2.0 --- .../app/rpc/commands/teams_invitations.clj | 2 +- backend/test/backend_tests/rpc_team_test.clj | 31 ++++++++++++++----- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/backend/src/app/rpc/commands/teams_invitations.clj b/backend/src/app/rpc/commands/teams_invitations.clj index b96cb0a8ce..729205c4c9 100644 --- a/backend/src/app/rpc/commands/teams_invitations.clj +++ b/backend/src/app/rpc/commands/teams_invitations.clj @@ -603,7 +603,7 @@ ::doc/module :teams ::sm/params schema:get-team-invitation-token} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id team-id email] :as params}] - (teams/check-read-permissions! cfg profile-id team-id) + (teams/check-edition-permissions! cfg profile-id team-id) (let [email (profile/clean-email email) invit (-> (db/get pool :team-invitation {:team-id team-id diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index 4bca0fbaa2..22455ce66b 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -357,6 +357,28 @@ (t/is (= (:id profile2) (:member-id claims)))))))) +(t/deftest get-team-invitation-token-requires-edition-permissions + (let [profile1 (th/create-profile* 1 {:is-active true}) + profile2 (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id profile1)}) + pool (:app.db/pool th/*system*)] + (th/create-team-role* {:team-id (:id team) + :profile-id (:id profile2) + :role :viewer}) + (db/insert! pool :team-invitation + {:team-id (:id team) + :email-to "victim@example.com" + :role "editor" + :valid-until (ct/in-future "48h")}) + (let [data {::th/type :get-team-invitation-token + ::rpc/profile-id (:id profile2) + :team-id (:id team) + :email "victim@example.com"} + out (th/command! data)] + (t/is (not (th/success? out))) + (t/is (= :not-found (-> out :error ex-data :type)))))) + + (t/deftest accept-invitation-tokens (let [profile1 (th/create-profile* 1 {:is-active true}) profile2 (th/create-profile* 2 {:is-active true}) @@ -366,14 +388,7 @@ pool (:app.db/pool th/*system*)] - (let [token (tokens/generate th/*system* - {:iss :team-invitation - :exp (ct/in-future "1h") - :profile-id (:id profile1) - :role :editor - :team-id (:id team) - :member-email (:email profile2) - :member-id (:id profile2)})] + (let [token (tokens/generate th/*system*)] (t/testing "Verify token as anonymous user" (db/insert! pool :team-invitation From 33f13f9bfd41142c32a045edcba3be0df65d24d0 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 25 Aug 2026 14:14:25 +0000 Subject: [PATCH 5/9] :bug: Fix test formatting for get-team-invitation-token AI-assisted-by: longcat-2.0 --- backend/test/backend_tests/rpc_team_test.clj | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index 22455ce66b..bf803e57c7 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -388,7 +388,14 @@ pool (:app.db/pool th/*system*)] - (let [token (tokens/generate th/*system*)] + (let [token (tokens/generate th/*system* + {:iss :team-invitation + :exp (ct/in-future "1h") + :profile-id (:id profile1) + :role :editor + :team-id (:id team) + :member-email (:email profile2) + :member-id (:id profile2)})] (t/testing "Verify token as anonymous user" (db/insert! pool :team-invitation From 33e39bc7ed789e504edd9e1fed3acec1b2bdeb50 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 25 Aug 2026 14:59:52 +0000 Subject: [PATCH 6/9] :wrench: Fix backend format check script in CI The backend format check was using 'check-fmt' instead of 'check-fmt:clj', causing CI to always fail on the fmt step. Closes #11358 AI-assisted-by: longcat-2.0 --- scripts/ci | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/ci b/scripts/ci index f78bbd0795..cff93eca3a 100755 --- a/scripts/ci +++ b/scripts/ci @@ -45,24 +45,24 @@ declare -A TEST_CMD=( declare -A FMT_CHECK_CMD=( [frontend]="pnpm run check-fmt:clj && pnpm run check-fmt:js && pnpm run check-fmt:scss" - [backend]="pnpm run check-fmt" + [backend]="pnpm run check-fmt:clj" [common]="pnpm run check-fmt:clj && pnpm run check-fmt:js" [render-wasm]="cargo fmt --check" [exporter]="pnpm run check-fmt:clj" [mcp]="pnpm run fmt:check" [plugins]="pnpm run format:check" - [library]="pnpm run check-fmt" + [library]="pnpm run check-fmt:clj" ) declare -A FMT_FIX_CMD=( [frontend]="pnpm run fmt:clj && pnpm run fmt:js && pnpm run fmt:scss" - [backend]="pnpm run fmt" + [backend]="pnpm run fmt:clj" [common]="pnpm run fmt:clj && pnpm run fmt:js" [render-wasm]="cargo fmt" [exporter]="pnpm run fmt:clj" [mcp]="pnpm run fmt" [plugins]="pnpm run format" - [library]="pnpm run fmt" + [library]="pnpm run fmt:clj" ) declare -A PAREN_REPAIR_CMD=( From e1a2d0b932ebca87efbf4cf3dad3b68197336870 Mon Sep 17 00:00:00 2001 From: Pablo Alba Date: Wed, 26 Aug 2026 17:24:59 +0200 Subject: [PATCH 7/9] :bug: Fix nitrate sso failure message (#11214) --- backend/src/app/auth/oidc.clj | 180 ++++++++++++------ backend/test/backend_tests/auth_oidc_test.clj | 136 +++++++++++++ frontend/src/app/main/errors.cljs | 6 + .../test/frontend_tests/main_errors_test.cljs | 47 ++++- 4 files changed, 306 insertions(+), 63 deletions(-) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 164edafd76..9fb3e75bd1 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -42,31 +42,52 @@ ;; OIDC PROVIDER (GENERIC) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +(defn- raise-invalid-sso-config + "Raise a controlled validation error for OIDC provider configuration failures." + [& {:keys [hint cause] :as params}] + (throw (ex-info (or hint "invalid-sso-config") + (-> params + (dissoc :cause) + (assoc :type :validation + :code :invalid-sso-config)) + cause))) + (defn- discover-oidc-config [cfg {:keys [base-uri skip-ssrf-check?] :as provider}] - (let [uri (u/join base-uri ".well-known/openid-configuration") - rsp (http/req cfg {:method :get :uri (dm/str uri)} {:skip-ssrf-check? skip-ssrf-check?})] + (let [uri (u/join base-uri ".well-known/openid-configuration")] + (try + (let [rsp (http/req cfg {:method :get :uri (dm/str uri)} {:skip-ssrf-check? skip-ssrf-check?})] + (if (= 200 (:status rsp)) + (let [data (-> rsp :body json/decode) + token-uri (get data :token_endpoint) + auth-uri (get data :authorization_endpoint) + user-uri (get data :userinfo_endpoint) + jwks-uri (get data :jwks_uri) + logout-uri (get data :end_session_endpoint)] - (if (= 200 (:status rsp)) - (let [data (-> rsp :body json/decode) - token-uri (get data :token_endpoint) - auth-uri (get data :authorization_endpoint) - user-uri (get data :userinfo_endpoint) - jwks-uri (get data :jwks_uri) - logout-uri (get data :end_session_endpoint)] + (-> provider + (assoc :token-uri token-uri) + (assoc :auth-uri auth-uri) + (assoc :user-uri user-uri) + (assoc :jwks-uri jwks-uri) + (assoc :logout-uri logout-uri))) - (-> provider - (assoc :token-uri token-uri) - (assoc :auth-uri auth-uri) - (assoc :user-uri user-uri) - (assoc :jwks-uri jwks-uri) - (assoc :logout-uri logout-uri))) - - (ex/raise :type ::internal - :code :invalid-sso-config - :hint "unable to discover OIDC configuration" - :discover-uri uri - :response-status-code (:status rsp))))) + (raise-invalid-sso-config + :hint "unable to discover OIDC configuration" + :discover-uri uri + :response-status-code (:status rsp)))) + (catch Throwable cause + ;; Controlled raises above are ExceptionInfo and would otherwise be + ;; re-wrapped by this catch, dropping fields like :response-status-code. + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw cause) + ;; Wrap SSRF blocks, DNS failures, TLS errors, etc. — from the caller's + ;; perspective these are all "bad/unreachable issuer URL". + (raise-invalid-sso-config + :hint "unable to discover OIDC configuration" + :discover-uri uri + :cause cause)))))) (def ^:private default-oidc-scopes #{"openid" "profile" "email"}) @@ -107,16 +128,29 @@ (defn- fetch-oidc-jwks [cfg jwks-uri {:keys [skip-ssrf-check?]}] - (let [{:keys [status body]} (http/req cfg {:method :get :uri jwks-uri} {:skip-ssrf-check? skip-ssrf-check?})] - (if (= 200 status) - (-> body json/decode :keys process-oidc-jwks) - (ex/raise :type ::internal - :code :unable-to-fetch-sso-jwks - :hint "unable to retrieve JWKs (unexpected response status code)" - :response-status-code status)))) + (try + (let [{:keys [status body]} (http/req cfg {:method :get :uri jwks-uri} {:skip-ssrf-check? skip-ssrf-check?})] + (if (= 200 status) + (-> body json/decode :keys process-oidc-jwks) + (raise-invalid-sso-config + :hint "unable to retrieve JWKs (unexpected response status code)" + :jwks-uri jwks-uri + :response-status-code status))) + (catch Throwable cause + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw cause) + (raise-invalid-sso-config + :hint "unable to retrieve JWKs" + :jwks-uri jwks-uri + :cause cause))))) (defn- populate-jwks - "Fetch and Add (if possible) JWK's to the OIDC provider" + "Fetch and add JWKs to the OIDC provider. + + When `:strict-jwks?` is set (organization SSO), failures raise a controlled + validation error. Otherwise JWKS is best-effort: log and continue without keys + so global OIDC/GitLab providers can still initialize if JWKS is temporarily down." [cfg provider] (try (if-let [jwks (when-let [jwks-uri (:jwks-uri provider)] @@ -124,20 +158,28 @@ (assoc provider :jwks jwks) provider) (catch Throwable cause - (l/warn :hint "unable to fetch JWKs for the OIDC provider" - :provider (str (:id provider)) - :cause cause) - provider))) + (if (:strict-jwks? provider) + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw cause) + (raise-invalid-sso-config + :hint "unable to retrieve JWKs" + :provider (:id provider) + :cause cause)) + (do + (l/warn :hint "unable to fetch JWKs for the OIDC provider" + :provider (str (:id provider)) + :cause cause) + provider))))) (defn- prepare-oidc-provider [cfg params] (when-not (and (string? (:base-uri params)) (string? (:client-id params)) (string? (:client-secret params))) - (ex/raise :type ::internal - :code :invalid-sso-config - :hint "missing params for provider initialization" - :provider (:id params))) + (raise-invalid-sso-config + :hint "missing params for provider initialization" + :provider (:id params))) (try (if (and (string? (:token-uri params)) @@ -150,11 +192,13 @@ (with-meta provider {::discovered true}))) (catch Throwable cause - (ex/raise :type ::internal - :type :invalid-sso-config - :hint "unexpected exception on configuring provider" - :provider (:id params) - :cause cause)))) + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw cause) + (raise-invalid-sso-config + :hint "unexpected exception on configuring provider" + :provider (:id params) + :cause cause))))) (defmethod ig/assert-key ::providers/generic [_ params] @@ -322,10 +366,9 @@ [cfg params] (when-not (and (string? (:client-id params)) (string? (:client-secret params))) - (ex/raise :type ::internal - :code :invalid-sso-config - :hint "missing params for provider initialization" - :provider (:id params))) + (raise-invalid-sso-config + :hint "missing params for provider initialization" + :provider (:id params))) (try (let [provider (populate-jwks cfg params)] @@ -336,11 +379,13 @@ :client-secret (d/obfuscate-string (:client-secret provider))) provider) (catch Throwable cause - (ex/raise :type ::internal - :type :invalid-sso-config - :hint "unexpected exception on configuring provider" - :provider (:id params) - :cause cause)))) + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw cause) + (raise-invalid-sso-config + :hint "unexpected exception on configuring provider" + :provider (:id params) + :cause cause))))) (defmethod ig/init-key ::providers/gitlab [_ cfg] @@ -867,7 +912,10 @@ :base-uri (some-> (non-blank-uri issuer) (str/rtrim "/") (str "/")) - :scopes default-oidc-scopes})) + :scopes default-oidc-scopes + ;; Organization SSO is configured by customers; discovery + ;; and JWKS failures must surface as controlled errors. + :strict-jwks? true})) (defn build-organization-sso-auth-redirect-uri "Build the OIDC authorization redirect URI for an organization SSO config. @@ -877,16 +925,24 @@ issuer (organization-sso-discovery-uri sso) dest-url (or dest-url (str (cf/get :public-uri)))] (when-not issuer - (ex/raise :type :validation - :code :invalid-sso-config - :hint "missing issuer")) - (let [oidc-provider (or provider (prepare-organization-sso-provider cfg sso)) - state-token (tokens/generate cfg {:iss "oidc" - :dest-url dest-url - :organization-id organization-id - :issuer issuer - :exp (ct/in-future "4h")})] - (build-auth-redirect-uri oidc-provider state-token)))) + (raise-invalid-sso-config + :hint "missing issuer" + :organization-id organization-id)) + (try + (let [oidc-provider (or provider (prepare-organization-sso-provider cfg sso)) + state-token (tokens/generate cfg {:iss "oidc" + :dest-url dest-url + :organization-id organization-id + :issuer issuer + :exp (ct/in-future "4h")})] + (build-auth-redirect-uri oidc-provider state-token)) + (catch Throwable cause + (if (and (ex/error? cause) + (= :invalid-sso-config (:code (ex-data cause)))) + (throw (ex-info (ex-message cause) + (assoc (ex-data cause) :organization-id organization-id) + (ex-cause cause))) + (throw cause)))))) (def ^:private probe-auth-code "penpot-sso-config-probe") diff --git a/backend/test/backend_tests/auth_oidc_test.clj b/backend/test/backend_tests/auth_oidc_test.clj index b99de502c4..29469e36d7 100644 --- a/backend/test/backend_tests/auth_oidc_test.clj +++ b/backend/test/backend_tests/auth_oidc_test.clj @@ -15,6 +15,7 @@ [app.setup :as-alias setup] [app.tokens :as tokens] [clojure.test :as t] + [cuerdas.core :as str] [mockery.core :refer [with-mocks]] [yetti.response :as-alias yres])) @@ -587,3 +588,138 @@ :issuer "https://idp.example.com"}) (t/is (not (true? (:skip-ssrf-check? @captured-params))) "SSRF protection must be disabled for organization SSO"))))) + +(defn- ssl-handshake-failure + [] + (javax.net.ssl.SSLHandshakeException. "Remote host terminated the handshake")) + +(t/deftest prepare-organization-sso-provider-raises-on-discovery-network-failure + (t/testing "SSL/network failures during OIDC discovery become controlled validation errors" + (with-mocks [http-mock {:target 'app.http.client/req + :side-effect (fn [& _] (throw (ssl-handshake-failure)))}] + (let [e (try + (#'oidc/prepare-organization-sso-provider + {} + {:client-id "test-client" + :client-secret "test-secret" + :issuer "https://wrong-idp.example.com"}) + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :validation (:type (ex-data e)))) + (t/is (= :invalid-sso-config (:code (ex-data e)))))))) + +(t/deftest prepare-organization-sso-provider-raises-on-discovery-non-200 + (t/testing "non-200 OIDC discovery responses become controlled validation errors" + (with-mocks [http-mock {:target 'app.http.client/req + :return {:status 404 :body "not found"}}] + (let [e (try + (#'oidc/prepare-organization-sso-provider + {} + {:client-id "test-client" + :client-secret "test-secret" + :issuer "https://idp.example.com"}) + (catch Throwable t t)) + data (ex-data e)] + (t/is (ex/error? e)) + (t/is (= :validation (:type data))) + (t/is (= :invalid-sso-config (:code data))) + (t/is (= 404 (:response-status-code data))) + (t/is (= "unable to discover OIDC configuration" (ex-message e))) + (t/is (str/includes? (str (:discover-uri data)) "openid-configuration")))))) + +(t/deftest prepare-organization-sso-provider-raises-on-ssrf-blocked-issuer + (t/testing "SSRF/DNS failures for the issuer URL become invalid-sso-config, not ssrf-blocked-target" + (with-mocks [http-mock {:target 'app.http.client/req + :side-effect (fn [& _] + (ex/raise :type :validation + :code :ssrf-blocked-target + :hint "uri host could not be resolved"))}] + (let [e (try + (#'oidc/prepare-organization-sso-provider + {} + {:client-id "test-client" + :client-secret "test-secret" + :issuer "https://unresolvable.invalid"}) + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :validation (:type (ex-data e)))) + (t/is (= :invalid-sso-config (:code (ex-data e)))) + (t/is (= :ssrf-blocked-target (:code (ex-data (ex-cause e))))))))) + +(t/deftest prepare-organization-sso-provider-raises-on-jwks-network-failure + (t/testing "SSL/network failures while fetching JWKs become controlled validation errors" + (let [discovery-body (str "{\"authorization_endpoint\":\"https://idp.example.com/auth\"," + "\"token_endpoint\":\"https://idp.example.com/token\"," + "\"userinfo_endpoint\":\"https://idp.example.com/userinfo\"," + "\"jwks_uri\":\"https://idp.example.com/jwks\"}")] + (with-mocks [http-mock {:target 'app.http.client/req + :side-effect (fn [_cfg request & _] + (if (str/includes? (str (:uri request)) "openid-configuration") + {:status 200 :body discovery-body} + (throw (ssl-handshake-failure))))}] + (let [e (try + (#'oidc/prepare-organization-sso-provider + {} + {:client-id "test-client" + :client-secret "test-secret" + :issuer "https://idp.example.com"}) + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :validation (:type (ex-data e)))) + (t/is (= :invalid-sso-config (:code (ex-data e))))))))) + +(t/deftest populate-jwks-strict-wraps-non-invalid-sso-config-errors + (t/testing "strict JWKS path wraps unrelated structured errors instead of rethrowing them" + (with-mocks [fetch-mock {:target 'app.auth.oidc/fetch-oidc-jwks + :side-effect (fn [& _] + (ex/raise :type :validation + :code :ssrf-blocked-target + :hint "uri host could not be resolved"))}] + (let [e (try + (#'oidc/populate-jwks + {} + {:id "oidc" + :jwks-uri "https://idp.example.com/jwks" + :strict-jwks? true}) + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :validation (:type (ex-data e)))) + (t/is (= :invalid-sso-config (:code (ex-data e)))) + (t/is (= :ssrf-blocked-target (:code (ex-data (ex-cause e))))))))) + +(t/deftest populate-jwks-strict-rethrows-invalid-sso-config + (t/testing "strict JWKS path rethrows an already-controlled invalid-sso-config" + (with-mocks [fetch-mock {:target 'app.auth.oidc/fetch-oidc-jwks + :side-effect (fn [& _] + (ex/raise :type :validation + :code :invalid-sso-config + :hint "unable to retrieve JWKs" + :jwks-uri "https://idp.example.com/jwks"))}] + (let [e (try + (#'oidc/populate-jwks + {} + {:id "oidc" + :jwks-uri "https://idp.example.com/jwks" + :strict-jwks? true}) + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :invalid-sso-config (:code (ex-data e)))) + (t/is (= "unable to retrieve JWKs" (ex-message e))) + (t/is (= "https://idp.example.com/jwks" (:jwks-uri (ex-data e)))))))) + +(t/deftest build-organization-sso-auth-redirect-uri-raises-on-unreachable-provider + (t/testing "check-nitrate-sso path surfaces a controlled error when the issuer is unreachable" + (with-mocks [http-mock {:target 'app.http.client/req + :side-effect (fn [& _] (throw (ssl-handshake-failure)))}] + (let [e (try + (oidc/build-organization-sso-auth-redirect-uri + {} + {:client-id "test-client" + :client-secret "test-secret" + :issuer "https://wrong-idp.example.com"} + :dest-url "https://localhost:3449/#/dashboard" + :organization-id #uuid "00000000-0000-0000-0000-000000000001") + (catch Throwable t t))] + (t/is (ex/error? e)) + (t/is (= :validation (:type (ex-data e)))) + (t/is (= :invalid-sso-config (:code (ex-data e)))))))) diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index f8b5ad9dc4..48aba11881 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -389,6 +389,12 @@ :level :error :timeout 3000}))) + (= code :invalid-sso-config) + ;; SSO error page needs :organization-id to retry + (if (:organization-id error) + (st/async-emit! (rt/assign-exception (assoc error :type :sso-error))) + (st/async-emit! (rt/assign-exception error))) + :else (st/async-emit! (rt/assign-exception error)))) diff --git a/frontend/test/frontend_tests/main_errors_test.cljs b/frontend/test/frontend_tests/main_errors_test.cljs index 207b295a11..d35af2f2ca 100644 --- a/frontend/test/frontend_tests/main_errors_test.cljs +++ b/frontend/test/frontend_tests/main_errors_test.cljs @@ -12,7 +12,8 @@ - exception->error-data – pure transformer - on-error re-entrancy guard – prevents recursive invocations - flash schedules async emit – ntf/show is not emitted synchronously - - organization SSO recovery – expired SSO sessions go back to the provider" + - organization SSO recovery – expired SSO sessions go back to the provider + - invalid-sso-config handler – requires :organization-id to promote to :sso-error" (:require [app.main.errors :as errors] [app.main.repo :as rp] @@ -351,3 +352,47 @@ (t/is (nil? @assigned*)) (done')))) done)))) + +;; --------------------------------------------------------------------------- +;; :validation / :invalid-sso-config +;; +;; The SSO error page needs an organization-id to retry meaningfully. Promote +;; to :sso-error only when that id is present; otherwise keep :validation so +;; we do not surface a broken SSO dialog for a future code path that omits it. +;; --------------------------------------------------------------------------- + +(defn- capture-async-exception + "Invoke `ptk/handle-error` while capturing the error map passed to + `rt/assign-exception` via `st/async-emit!`. + + `st/async-emit!` is variadic (`[& params]`); the mock must be too, + otherwise CLJS looks up `IFn$_invoke$arity$variadic` and throws." + [error] + (let [captured (atom nil)] + (with-redefs [st/async-emit! (fn [& events] + (reset! captured (first events))) + rt/assign-exception (fn [err] err)] + (ptk/handle-error error) + @captured))) + +(t/deftest invalid-sso-config-with-organization-id-promotes-to-sso-error + (t/testing "invalid-sso-config with :organization-id is shown as :sso-error" + (let [org-id #uuid "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + assigned (capture-async-exception + {:type :validation + :code :invalid-sso-config + :organization-id org-id + :hint "missing issuer"})] + (t/is (= :sso-error (:type assigned))) + (t/is (= org-id (:organization-id assigned))) + (t/is (= :invalid-sso-config (:code assigned)))))) + +(t/deftest invalid-sso-config-without-organization-id-keeps-validation + (t/testing "invalid-sso-config without :organization-id must not become :sso-error" + (let [assigned (capture-async-exception + {:type :validation + :code :invalid-sso-config + :hint "missing issuer"})] + (t/is (= :validation (:type assigned))) + (t/is (nil? (:organization-id assigned))) + (t/is (= :invalid-sso-config (:code assigned)))))) From 4be749d45f1d34a900d912ddf24e47f7d8ec10f2 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 26 Aug 2026 19:18:59 +0200 Subject: [PATCH 8/9] :sparkles: Add minor improvements to scripts/gh.py --- scripts/gh.py | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/scripts/gh.py b/scripts/gh.py index c7e1f87aca..f8b4f74d45 100755 --- a/scripts/gh.py +++ b/scripts/gh.py @@ -473,8 +473,10 @@ query($owner: String!, $repo: String!, $milestone: Int!, $cursor: String) { state mergedAt createdAt + headRefName author { login } labels(first: 20) { nodes { name } } + files(first: 100) { nodes { path } } closingIssuesReferences(first: 5) { nodes { number } } } } @@ -494,8 +496,9 @@ def fetch_milestone_prs(milestone_num: int, states: str) -> list[dict]: states: GraphQL states enum array literal, e.g. ``"[MERGED]"`` or ``"[OPEN CLOSED MERGED]"`` Returns: - List of {number, title, body, state, merged_at, created_at, author, - labels: [str], closing_issues: [int]} + List of {number, title, body, state, merged_at, created_at, + head_ref_name, author, labels: [str], files: [str], + closing_issues: [int]} """ query = GQL_MILESTONE_PRS_QUERY.replace("__STATES__", states) all_nodes: list[dict] = [] @@ -522,8 +525,10 @@ def fetch_milestone_prs(milestone_num: int, states: str) -> list[dict]: "state": node["state"], "merged_at": node.get("mergedAt"), "created_at": node.get("createdAt"), + "head_ref_name": node.get("headRefName"), "author": node["author"]["login"] if node["author"] else None, "labels": [lbl["name"] for lbl in node["labels"]["nodes"]], + "files": [file["path"] for file in node["files"]["nodes"]], "closing_issues": [iss["number"] for iss in node["closingIssuesReferences"]["nodes"]], }) @@ -602,7 +607,20 @@ def cmd_prs(args: argparse.Namespace) -> None: def fetch_advisories() -> list[dict]: """Fetch all security advisories for the repository via REST API.""" - return run_gh_rest(f"repos/{REPO}/security-advisories") + all_advisories: list[dict] = [] + page = 1 + + while True: + advisories = run_gh_rest( + f"repos/{REPO}/security-advisories?per_page=100&page={page}" + ) + all_advisories.extend(advisories) + + if len(advisories) < 100: + break + page += 1 + + return all_advisories def fetch_advisory(ghsa_id: str) -> dict: From 6e173a02fb2f32dc734dd27398f8a4b11fa2e4a8 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 26 Aug 2026 18:11:40 +0000 Subject: [PATCH 9/9] :books: Split backend testing memory and link from testing skill Extract the backend Testing section from backend/core into a dedicated backend/testing memory, following the pattern of common, frontend, and exporter. Update the testing skill and root testing memory to point at the new location, and add exporter/testing to the skill's required reading list. AI-assisted-by: deepseek-v4-flash --- .opencode/skills/testing/SKILL.md | 3 ++- .serena/memories/backend/core.md | 8 ++------ .serena/memories/backend/testing.md | 11 +++++++++++ .serena/memories/testing.md | 2 +- 4 files changed, 16 insertions(+), 8 deletions(-) create mode 100644 .serena/memories/backend/testing.md diff --git a/.opencode/skills/testing/SKILL.md b/.opencode/skills/testing/SKILL.md index 5ad5d04e6e..29ab7112f8 100644 --- a/.opencode/skills/testing/SKILL.md +++ b/.opencode/skills/testing/SKILL.md @@ -34,7 +34,8 @@ Before writing any test, read: 2. Module-specific testing memory for the affected module: - `mem:common/testing` — CLJC unit tests - `mem:frontend/testing` — CLJS unit tests, Playwright E2E - - `mem:backend/core` — JVM clojure.test conventions + - `mem:backend/testing` — JVM clojure.test conventions + - `mem:exporter/testing` — exporter unit tests ## Key Rules diff --git a/.serena/memories/backend/core.md b/.serena/memories/backend/core.md index 7b085856d1..9a567932b7 100644 --- a/.serena/memories/backend/core.md +++ b/.serena/memories/backend/core.md @@ -102,9 +102,5 @@ misleading linter/compiler output. See `mem:scripts/paren-repair`. ## Testing -IMPORTANT: all CLI commands must be executed from the `backend/` subdirectory. JVM tests are invoked directly via `clojure -M:dev:test` — there is no pnpm wrapper. If you need to filter output, tee to a temp file first: `clojure -M:dev:test 2>&1 | tee /tmp/penpot-test-output.txt`. See `mem:testing` for execution discipline. - -* **Coverage:** If code is added or modified in `src/`, corresponding tests in `test/backend_tests/` must be added or updated. -* **Isolated run:** `clojure -M:dev:test --focus backend-tests.my-ns-test` for a specific test namespace. -* **Regression run:** `clojure -M:dev:test` to ensure no regressions in related functional areas. -* **Principles:** Cross-cutting testing principles, anti-patterns, and verification checklist: `mem:testing`. +Backend test commands, coverage rules, and conventions: `mem:backend/testing`. +Cross-cutting testing principles, anti-patterns, and verification checklist: `mem:testing`. diff --git a/.serena/memories/backend/testing.md b/.serena/memories/backend/testing.md new file mode 100644 index 0000000000..66d6f8a246 --- /dev/null +++ b/.serena/memories/backend/testing.md @@ -0,0 +1,11 @@ +# Backend Testing + +JVM `clojure.test` (kaocha runner) under `backend/test/backend_tests/`. + +- READ `mem:testing` FIRST — it defines the execution discipline (no piping, tee to file, preferred commands) that applies to all JVM test runs. +- All CLI commands must be executed from the `backend/` subdirectory. +- Tests are invoked directly via `clojure -M:dev:test` (kaocha) — there is no pnpm wrapper. Kaocha auto-discovers test namespaces, so no runner registration is needed. +- Coverage: if code is added or modified in `src/`, corresponding tests in `test/backend_tests/` must be added or updated. +- Isolated run: `clojure -M:dev:test --focus backend-tests.my-ns-test` for a specific test namespace, or `clojure -M:dev:test --focus backend-tests.my-ns-test/my-test-var` for a specific test var. +- Regression run: `clojure -M:dev:test` to ensure no regressions in related functional areas. +- If you need to filter output, tee to a temp file first: `clojure -M:dev:test 2>&1 | tee /tmp/penpot-test-output.txt`. \ No newline at end of file diff --git a/.serena/memories/testing.md b/.serena/memories/testing.md index 295da86212..ce301ff5bb 100644 --- a/.serena/memories/testing.md +++ b/.serena/memories/testing.md @@ -13,7 +13,7 @@ and helpers, consult: builders, production-path change helpers - `mem:frontend/testing` — CLJS unit tests, Playwright E2E integration tests, live browser verification via nREPL -- Backend — JVM `clojure.test` under `backend/test/`; see `mem:backend/core` +- `mem:backend/testing` — JVM `clojure.test` under `backend/test/` ## When to Use