From aecfee0f02893ad0d1c212fc6f0b91b89237dff6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Barrag=C3=A1n=20Merino?= Date: Fri, 14 Aug 2026 20:14:29 +0200 Subject: [PATCH 01/14] :wrench: Align MCP workflow name with the rest of CI workflows The MCP workflow was named "MCP CI" while every other tests-*.yml workflow uses the "CI: " pattern. Rename it to "CI: MCP" for consistency in the GitHub Actions listing. --- .github/workflows/tests-mcp.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/tests-mcp.yml b/.github/workflows/tests-mcp.yml index 6f8eadcadd..317d82dca2 100644 --- a/.github/workflows/tests-mcp.yml +++ b/.github/workflows/tests-mcp.yml @@ -1,4 +1,4 @@ -name: "MCP CI" +name: "CI: MCP" on: pull_request: From 3033da4409e32e906fc8130ef27064ed6ea84fa7 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 11:08:07 +0200 Subject: [PATCH 02/14] :bug: Add concurrency limit to import-binfile RPC handler (#11024) Apply climit with 4 global permits and 1 per-profile permit (queue 2) to prevent connection pool exhaustion from concurrent imports. Each import holds a DB connection for its entire duration with idle transaction timeout disabled, so unbounded concurrency could exhaust the pool (default 60 connections). AI-assisted-by: mimo-v2.5-pro --- backend/resources/climit.edn | 8 +++++++- backend/src/app/rpc/commands/binfile.clj | 5 ++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/backend/resources/climit.edn b/backend/resources/climit.edn index 66ac82b174..ded9b5c9b8 100644 --- a/backend/resources/climit.edn +++ b/backend/resources/climit.edn @@ -45,4 +45,10 @@ {:permits 4} :send-user-feedback/by-profile - {:permits 1 :queue 3}} + {:permits 1 :queue 3} + + :import-binfile/global + {:permits 4} + + :import-binfile/by-profile + {:permits 1 :queue 2}} diff --git a/backend/src/app/rpc/commands/binfile.clj b/backend/src/app/rpc/commands/binfile.clj index e68210aae3..685b450ecb 100644 --- a/backend/src/app/rpc/commands/binfile.clj +++ b/backend/src/app/rpc/commands/binfile.clj @@ -21,6 +21,7 @@ [app.loggers.webhooks :as-alias webhooks] [app.media.validation :as media.v] [app.rpc :as-alias rpc] + [app.rpc.climit :as-alias climit] [app.rpc.commands.files :as files] [app.rpc.commands.media :as media-cmd] [app.rpc.commands.projects :as projects] @@ -142,7 +143,9 @@ ::webhooks/event? true ::sse/stream? true - ::sm/params schema:import-binfile} + ::sm/params schema:import-binfile + ::climit/id [[:import-binfile/by-profile ::rpc/profile-id] + [:import-binfile/global]]} [{:keys [::db/pool] :as cfg} {:keys [::rpc/profile-id project-id version upload-id] :as params}] (projects/check-edition-permissions! pool profile-id project-id) (let [version (or version 3) From 68e1db984d71aa4fbddb8e497561fa7f4d22ebdf Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 11:20:04 +0200 Subject: [PATCH 03/14] :sparkles: Add resolve-git-conflicts opencode command --- .opencode/commands/resolve-git-conflicts.md | 40 +++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 .opencode/commands/resolve-git-conflicts.md diff --git a/.opencode/commands/resolve-git-conflicts.md b/.opencode/commands/resolve-git-conflicts.md new file mode 100644 index 0000000000..1b17ca0001 --- /dev/null +++ b/.opencode/commands/resolve-git-conflicts.md @@ -0,0 +1,40 @@ +--- +description: Resolve local git conflicts and stage the resolved files with git add — never continues the rebase +agent: build +--- + +# Fix Git Conflicts + +Resolve conflicts in the local repository. The user handles finishing the +rebase themselves — you must **never** run `git rebase --continue`, +`git rebase --skip`, `git merge --continue`, or anything similar. + +## Phase 1 — Understand the problem (read-only) + +1. Run `git status` to detect the conflict state (rebase, merge, cherry-pick, etc.) and list conflicted files. +2. For each conflicted (unmerged) file, understand the situation **without modifying anything**: + - Read the file and identify the conflict markers (`<<<<<<<`, `=======`, `>>>>>>>`). + - Inspect both sides — `git show :` and `git show :` — plus `git log`/`git show` on the commits involved to understand intent. + - Identify what each side changed and why, and how they should be combined. + +## Phase 2 — Present the resolution plan + +3. **Present a clear plan to the user before touching any file.** For each conflicted file, state: + - What each side changed and why. + - Your proposed resolution and the reasoning behind it. + - How the two sides are combined (both additive → merge; both modify the same code → keep the semantically correct version, merging intent from both sides when clear from code and context). +4. **Ask the user only when genuinely unclear.** Do not ask about anything you can determine yourself from the code, commit messages, or context. Only decisions that are not determinable and change the outcome (e.g. conflicting product decisions, which side to discard) warrant a question. **Collect all such questions together in an "Open Questions" section at the end of the plan**, so the user has full context to answer them properly. +5. **Wait for the user to accept the plan** (and answer any open questions) before editing, staging, or otherwise modifying anything. + +## Phase 3 — Execute + +6. Resolve each conflicted file by editing the file to the agreed merged content and removing all conflict markers. + +## Phase 4 — Stage and verify + +7. **Stage every resolved file** with `git add `. Do not stage unrelated untracked files unless clearly part of the resolution. +8. Verify no conflict markers remain (search for `<<<<<<<` / `>>>>>>>` in resolved files) and that `git status` shows no unmerged paths. + +## Phase 5 — Report + +9. Briefly report the conflict state, how each conflicted file was resolved (and any answers received to open questions), and stop — do **not** run `git rebase --continue` or any other continuation command. From c688cba8d8eb2bbd2eb81cd2e220caccc03994f2 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 11:21:25 +0200 Subject: [PATCH 04/14] :bug: Mock DNS resolution in SSRF tests for environments without public DNS (#11040) The validate-url-allows-public-{https,http} tests relied on real DNS resolution of example.com, which fails in containers without public DNS access. Mock resolve-host to return a known public IP, consistent with the pattern used by other tests in the same file. AI-assisted-by: mimo-v2.5-pro --- backend/test/backend_tests/util_ssrf_test.clj | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/backend/test/backend_tests/util_ssrf_test.clj b/backend/test/backend_tests/util_ssrf_test.clj index 2dadb8282a..c3b5b435bb 100644 --- a/backend/test/backend_tests/util_ssrf_test.clj +++ b/backend/test/backend_tests/util_ssrf_test.clj @@ -13,11 +13,25 @@ [clojure.test :as t])) (t/deftest validate-url-allows-public-https - (t/is (true? (ssrf/safe-url? "https://example.com/foo"))) - (t/is (true? (ssrf/safe-url? "https://example.com:8080/path?q=1")))) + (let [original ssrf/resolve-host] + (with-redefs [ssrf/resolve-host + (fn [hostname] + (if (= hostname "example.com") + (into-array java.net.InetAddress + [(java.net.InetAddress/getByName "93.184.216.34")]) + (original hostname)))] + (t/is (true? (ssrf/safe-url? "https://example.com/foo"))) + (t/is (true? (ssrf/safe-url? "https://example.com:8080/path?q=1")))))) (t/deftest validate-url-allows-public-http - (t/is (true? (ssrf/safe-url? "http://example.com/foo")))) + (let [original ssrf/resolve-host] + (with-redefs [ssrf/resolve-host + (fn [hostname] + (if (= hostname "example.com") + (into-array java.net.InetAddress + [(java.net.InetAddress/getByName "93.184.216.34")]) + (original hostname)))] + (t/is (true? (ssrf/safe-url? "http://example.com/foo")))))) (t/deftest validate-url-blocks-disallowed-schemes (t/is (false? (ssrf/safe-url? "file:///etc/passwd"))) From 9e97477a984a9e550efe126b110b7ceb2bf949ed Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 11:05:27 +0200 Subject: [PATCH 05/14] :arrow_up: Update to latest nodejs lts --- .nvmrc | 2 +- docker/devenv/Dockerfile | 2 +- docker/images/Dockerfile.exporter | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.nvmrc b/.nvmrc index 87d8620cc6..3648bfc346 100644 --- a/.nvmrc +++ b/.nvmrc @@ -1 +1 @@ -v24.18.1 +v24.19.0 diff --git a/docker/devenv/Dockerfile b/docker/devenv/Dockerfile index 4aff930e9e..989fb39f63 100644 --- a/docker/devenv/Dockerfile +++ b/docker/devenv/Dockerfile @@ -66,7 +66,7 @@ RUN set -ex; \ FROM base AS setup-node -ENV NODE_VERSION=v24.18.1 \ +ENV NODE_VERSION=v24.19.0 \ PATH=/opt/node/bin:$PATH RUN set -eux; \ diff --git a/docker/images/Dockerfile.exporter b/docker/images/Dockerfile.exporter index 7c0b1a14ff..97189bc5be 100644 --- a/docker/images/Dockerfile.exporter +++ b/docker/images/Dockerfile.exporter @@ -1,4 +1,4 @@ -FROM dhi.io/node:24.18.1-debian13-dev +FROM dhi.io/node:24.19.0-debian13-dev LABEL maintainer="Penpot " ENV LANG=en_US.UTF-8 \ From e219ce20eba7597a8c4924f2d7c49405ab142d3d Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 11:05:42 +0200 Subject: [PATCH 06/14] :arrow_up: Update opencode version on devenv --- docker/devenv/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/devenv/Dockerfile b/docker/devenv/Dockerfile index 989fb39f63..8a3d99216d 100644 --- a/docker/devenv/Dockerfile +++ b/docker/devenv/Dockerfile @@ -100,7 +100,7 @@ RUN set -eux; \ FROM base AS setup-opencode -ENV OPENCODE_VERSION=1.18.11 +ENV OPENCODE_VERSION=1.18.18 RUN set -ex; \ ARCH="$(dpkg --print-architecture)"; \ From 5efd9cc3c5689485322f57b644b83a3bd2e33cee Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 11:37:51 +0200 Subject: [PATCH 07/14] :bug: Prevent admins from granting owner role in team invitations (#11099) Add role-ceiling check to create-team-invitations and update-team-invitation-role methods. These RPC methods allowed team admins to grant or elevate invitations to :owner role, bypassing the protection that exists in update-team-member-role. The fix replicates the existing check from update-team-member-role: reject promotion to :owner when the caller is not an owner. Closes #11098 AI-assisted-by: qwen3.7-plus --- .../app/rpc/commands/teams_invitations.clj | 12 +++ backend/test/backend_tests/rpc_team_test.clj | 99 +++++++++++++++++++ 2 files changed, 111 insertions(+) diff --git a/backend/src/app/rpc/commands/teams_invitations.clj b/backend/src/app/rpc/commands/teams_invitations.clj index 93b15df5c2..b96cb0a8ce 100644 --- a/backend/src/app/rpc/commands/teams_invitations.clj +++ b/backend/src/app/rpc/commands/teams_invitations.clj @@ -490,6 +490,13 @@ :code :insufficient-permissions :hint "Organization policy does not allow you to send invitations")) + ;; Don't allow promote to owner to admin users. + (when (and (not (:is-owner perms)) + (or (= role :owner) + (some #(= :owner (:role %)) (:invitations params)))) + (ex/raise :type :validation + :code :cant-promote-to-owner)) + (when (> invitation-count max-invitations-by-request-threshold) (ex/raise :type :validation :code :max-invitations-by-request @@ -633,6 +640,11 @@ (ex/raise :type :validation :code :insufficient-permissions)) + ;; Don't allow promote to owner to admin users. + (when (and (not (:is-owner perms)) (= role :owner)) + (ex/raise :type :validation + :code :cant-promote-to-owner)) + (db/update! conn :team-invitation {:role (name role) :updated-at (ct/now)} {:team-id team-id :email-to (profile/clean-email email)}) diff --git a/backend/test/backend_tests/rpc_team_test.clj b/backend/test/backend_tests/rpc_team_test.clj index 7f8fb136e1..4bca0fbaa2 100644 --- a/backend/test/backend_tests/rpc_team_test.clj +++ b/backend/test/backend_tests/rpc_team_test.clj @@ -1207,3 +1207,102 @@ (let [team (:result out)] (t/is (uuid? (:id team))) (t/is (= "Test Team" (:name team))))))))) + +;; --- T7-F-01: Role ceiling in team invitations --- + +(t/deftest admin-cannot-create-invitation-with-owner-role + (with-mocks [mock {:target 'app.email/send! :return nil}] + (let [owner (th/create-profile* 1 {:is-active true}) + admin (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)})] + + ;; Add admin as team member with :admin role + (th/create-team-role* {:team-id (:id team) + :profile-id (:id admin) + :role :admin}) + + ;; Admin tries to create invitation with :owner role (emails+role format) + ;; This should FAIL with :cant-promote-to-owner + (let [data {::th/type :create-team-invitations + ::rpc/profile-id (:id admin) + :team-id (:id team) + :role :owner + :emails ["invitee@example.com"]} + out (th/command! data)] + (t/is (not (th/success? out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :cant-promote-to-owner)) + (t/is (= 0 (:call-count @mock))))))) + +(t/deftest admin-cannot-create-invitation-with-owner-role-invitations-format + (with-mocks [mock {:target 'app.email/send! :return nil}] + (let [owner (th/create-profile* 1 {:is-active true}) + admin (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)})] + + ;; Add admin as team member with :admin role + (th/create-team-role* {:team-id (:id team) + :profile-id (:id admin) + :role :admin}) + + ;; Admin tries to create invitation with :owner role (invitations format) + ;; This should FAIL with :cant-promote-to-owner + (let [data {::th/type :create-team-invitations + ::rpc/profile-id (:id admin) + :team-id (:id team) + :invitations [{:email "invitee@example.com" :role :owner}]} + out (th/command! data)] + (t/is (not (th/success? out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :cant-promote-to-owner)) + (t/is (= 0 (:call-count @mock))))))) + +(t/deftest admin-cannot-update-invitation-role-to-owner + (with-mocks [mock {:target 'app.email/send! :return nil}] + (let [owner (th/create-profile* 1 {:is-active true}) + admin (th/create-profile* 2 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)})] + + ;; Add admin as team member with :admin role + (th/create-team-role* {:team-id (:id team) + :profile-id (:id admin) + :role :admin}) + + ;; Owner creates an invitation with :editor role + (let [data {::th/type :create-team-invitations + ::rpc/profile-id (:id owner) + :team-id (:id team) + :role :editor + :emails ["invitee@example.com"]} + out (th/command! data)] + (t/is (th/success? out))) + + (th/reset-mock! mock) + + ;; Admin tries to update invitation role to :owner + ;; This should FAIL with :cant-promote-to-owner + (let [data {::th/type :update-team-invitation-role + ::rpc/profile-id (:id admin) + :team-id (:id team) + :email "invitee@example.com" + :role :owner} + out (th/command! data)] + (t/is (not (th/success? out))) + (t/is (th/ex-of-type? (:error out) :validation)) + (t/is (th/ex-of-code? (:error out) :cant-promote-to-owner)))))) + +(t/deftest owner-can-create-invitation-with-owner-role + (with-mocks [mock {:target 'app.email/send! :return nil}] + (let [owner (th/create-profile* 1 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id owner)})] + + ;; Owner creates invitation with :owner role + ;; This should SUCCEED (owner has full privileges) + (let [data {::th/type :create-team-invitations + ::rpc/profile-id (:id owner) + :team-id (:id team) + :role :owner + :emails ["invitee@example.com"]} + out (th/command! data)] + (t/is (th/success? out)) + (t/is (= 1 (:call-count @mock))))))) From 0797d7235a608a105af2ac8317e6678bba802915 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 11:51:24 +0200 Subject: [PATCH 08/14] :bug: Fix workspace crash on rapid sidebar measures input changes (#10793) (#10794) The sidebar measures panel numeric inputs (X, Y, width, height, rotation) emitted one full apply-modifiers commit per DOM event with no throttle: every arrow key-repeat, wheel tick and scrub pointermove became update-positions / update-dimensions / increase-rotation. A sustained gesture starved the React renderer and crashed the workspace with error #185 (Maximum update depth exceeded). Coalesce those bursts at the data layer (potok), following the update-position-data debounce pattern in texts.cljs: - update-positions is now burst-coalesced in place (its only caller is the measures panel); new update-dimensions-coalesced and increase-rotation-coalesced variants are used by the measures panel, while the immediate events keep serving plugins, variants and token application (including the delta? rotation path). - The first event of a burst commits immediately (leading edge, so single edits stay synchronous); further ticks commit at most once per 50 ms (throttle); a trailing debounced flush guarantees the exact final value lands. All payloads are absolute values, so keeping the latest queued value per shape/attribute is lossless. - Pending payloads are drained atomically and stale shape ids (deleted mid-burst) are skipped. The drain stream lives until the workspace is finalized, so bursts reuse a single subscription. - Fewer commits per burst also means fewer undo entries; scrub drags still produce a single entry via the input's outer transaction. Tests: new frontend-tests.logic.sidebar-transform-coalescing-test (8 tests, legacy SVG and WASM renderer branches) guards the invariant that a 20-event burst commits the exact final value in a handful of commits. The previously unregistered update-position-test is wired into the runner with WASM mock fixtures (it fails in full-suite context without them due to a pre-existing global mock-state issue). AI-assisted-by: kimi-k3 --- frontend/src/app/main/constants.cljs | 6 + frontend/src/app/main/data/workspace.cljs | 2 + .../app/main/data/workspace/transforms.cljs | 164 ++++++++++++- .../sidebar/options/menus/measures.cljs | 4 +- .../sidebar_transform_coalescing_test.cljs | 226 ++++++++++++++++++ .../logic/update_position_test.cljs | 7 +- frontend/test/frontend_tests/runner.cljs | 4 + 7 files changed, 403 insertions(+), 10 deletions(-) create mode 100644 frontend/test/frontend_tests/logic/sidebar_transform_coalescing_test.cljs diff --git a/frontend/src/app/main/constants.cljs b/frontend/src/app/main/constants.cljs index d473e621c6..40390e2ee4 100644 --- a/frontend/src/app/main/constants.cljs +++ b/frontend/src/app/main/constants.cljs @@ -341,3 +341,9 @@ (def ^:const resize-sample-time default-sample-time) (def ^:const rotation-sample-time default-sample-time) (def ^:const move-sample-time default-sample-time) + +(def ^:const sidebar-transform-sample-time + "Time in ms for coalescing sidebar measures-panel transform commits: at + most one full commit per window during a burst, plus a trailing flush + with the exact final value." + 50) diff --git a/frontend/src/app/main/data/workspace.cljs b/frontend/src/app/main/data/workspace.cljs index 9bf11fb701..72e5f93f3e 100644 --- a/frontend/src/app/main/data/workspace.cljs +++ b/frontend/src/app/main/data/workspace.cljs @@ -1552,9 +1552,11 @@ (dm/export dwt/trigger-bounding-box-cloaking) (dm/export dwt/start-resize) (dm/export dwt/update-dimensions) +(dm/export dwt/update-dimensions-coalesced) (dm/export dwt/change-orientation) (dm/export dwt/start-rotate) (dm/export dwt/increase-rotation) +(dm/export dwt/increase-rotation-coalesced) (dm/export dwt/start-move-selected) (dm/export dwt/move-selected) (dm/export dwt/update-position) diff --git a/frontend/src/app/main/data/workspace/transforms.cljs b/frontend/src/app/main/data/workspace/transforms.cljs index d0e612b493..f53cf8b2ee 100644 --- a/frontend/src/app/main/data/workspace/transforms.cljs +++ b/frontend/src/app/main/data/workspace/transforms.cljs @@ -1125,19 +1125,169 @@ :ignore-touched (:ignore-touched options) :ignore-snap-pixel true})))))))) +;; -- Sidebar measures transform coalescing ---------------------------- + +;; The sidebar measures panel numeric inputs emit one event per DOM +;; gesture tick (held arrow keys, mouse wheel, scrub drags). Committing +;; each tick would run a full `apply-modifiers` per DOM event and starve +;; the renderer (React error #185). The events in this section coalesce +;; those bursts at the data layer: the first event of a burst commits +;; immediately (leading edge, so single edits stay synchronous), further +;; ticks commit at most once per `mconst/sidebar-transform-sample-time` +;; (throttle), and a trailing debounced flush guarantees the exact final +;; value lands. All payloads are absolute values, so keeping only the +;; latest queued value per shape/attribute is lossless. + +(defn- sidebar-commit-events + "Build the real commit events for a drained pending entry of `kind`, + skipping shapes that no longer exist on the queued page." + [state kind entry] + (let [options (:options entry) + page-id (or (:page-id options) (:current-page-id state)) + objects (dsh/lookup-page-objects state page-id) + options (assoc options :page-id page-id) + live-ids (fn [ids] (into [] (filter #(contains? objects %)) ids))] + (case kind + ::positions + (keep (fn [[id position]] + (when (contains? objects id) + (update-position id position options))) + (:positions entry)) + + ::dimensions + (let [ids (live-ids (:ids entry))] + (when (seq ids) + (map (fn [[attr value]] + (update-dimensions ids attr value options)) + (:values entry)))) + + ::rotation + (let [ids (live-ids (:ids entry))] + (when (seq ids) + [(increase-rotation ids (:value entry) nil :page-id page-id)]))))) + +(defn- flush-sidebar-transforms + "Internal: atomically drain the pending sidebar transform payloads and + emit their commit events. No-op when nothing is pending." + [] + (ptk/reify ::flush-sidebar-transforms + ptk/UpdateEvent + (update [_ state] + (let [pending (::pending-sidebar-transforms state)] + (-> state + (dissoc ::pending-sidebar-transforms) + (assoc ::flushing-sidebar-transforms pending)))) + + ptk/WatchEvent + (watch [_ state _] + (let [pending (::flushing-sidebar-transforms state)] + (rx/concat + (if (empty? pending) + (rx/empty) + (->> pending + (mapcat (fn [[kind entry]] (sidebar-commit-events state kind entry))) + (rx/from))) + (rx/of (fn [state] (dissoc state ::flushing-sidebar-transforms)))))))) + +(defn- queue-sidebar-transform + "Internal: accumulate the latest payload of `kind` with `update-entry` + (a fn from the previous pending entry to the new one). + + The very first queued event of the workspace session also installs the + drain stream that commits pending payloads: a leading flush for the + first event, at most one flush per + `mconst/sidebar-transform-sample-time` while a burst is ongoing + (throttle), and a trailing flush (debounce) that guarantees the exact + final value lands. The drain stream lives until the workspace is + finalized, so subsequent bursts reuse it." + [kind update-entry] + (let [cur-event (js/Symbol)] + (ptk/reify ::queue-sidebar-transform + ptk/UpdateEvent + (update [_ state] + (let [state (update-in state [::pending-sidebar-transforms kind] + (fn [entry] (update-entry (or entry {}))))] + (if (nil? (::sidebar-transform-drain state)) + (assoc state ::sidebar-transform-drain cur-event) + state))) + + ptk/WatchEvent + (watch [_ state stream] + (if (= cur-event (::sidebar-transform-drain state)) + (let [stopper (->> stream (rx/filter (ptk/type? :app.main.data.workspace/finalize)))] + (rx/merge + ;; Leading edge: commit the payload this first event queued. + (rx/of (flush-sidebar-transforms)) + ;; At most one commit per window while a burst is ongoing. + (->> stream + (rx/filter (ptk/type? ::queue-sidebar-transform)) + (rx/throttle mconst/sidebar-transform-sample-time) + (rx/map (fn [_] (flush-sidebar-transforms))) + (rx/take-until stopper)) + ;; Trailing edge: guarantee the exact final value lands. + (->> stream + (rx/filter (ptk/type? ::queue-sidebar-transform)) + (rx/debounce mconst/sidebar-transform-sample-time) + (rx/map (fn [_] (flush-sidebar-transforms))) + (rx/take-until stopper)))) + (rx/empty)))))) + (defn update-positions - "Move multiple shapes to a new position." + "Move multiple shapes to a new position, from the sidebar options form. + + Burst-coalesced (see `queue-sidebar-transform`): rapid successive calls + from the sidebar numeric inputs commit at most once per + `mconst/sidebar-transform-sample-time`, and the trailing flush commits + the exact final position. A single call still commits synchronously." ([ids position] (update-positions ids position nil)) ([ids position options] (assert (every? uuid? ids) "expected valid coll of uuids") (assert (map? position) "expected a valid map for `position`") - (ptk/reify ::update-positions - ptk/WatchEvent - (watch [_ _ _] - (->> ids - (map (fn [id] (update-position id position options))) - (rx/from)))))) + (queue-sidebar-transform + ::positions + (fn [entry] + (-> entry + (update :positions + (fn [positions] + (reduce (fn [positions id] + (update positions id merge position)) + (or positions {}) + ids))) + (assoc :options options)))))) + +(defn update-dimensions-coalesced + "Like `update-dimensions`, but burst-coalesced (see + `queue-sidebar-transform`); used by the sidebar measures panel numeric + inputs. The latest queued value per attribute wins." + ([ids attr value] (update-dimensions-coalesced ids attr value nil)) + ([ids attr value options] + (assert (number? value)) + (assert (every? uuid? ids) + "expected valid coll of uuids") + (assert (contains? #{:width :height} attr) + "expected valid attr") + (queue-sidebar-transform + ::dimensions + (fn [entry] + (-> entry + (assoc-in [:values attr] value) + (assoc :ids ids :options options)))))) + +(defn increase-rotation-coalesced + "Like `increase-rotation` with an absolute rotation value, but + burst-coalesced (see `queue-sidebar-transform`); used by the sidebar + measures panel rotation input. The latest queued absolute value wins; + the delta is recomputed from the current rotation when the burst + commits." + [ids rotation] + (assert (every? uuid? ids) + "expected valid coll of uuids") + (assert (number? rotation)) + (queue-sidebar-transform + ::rotation + (fn [entry] + (assoc entry :value rotation :ids ids :options nil)))) (defn position-shapes [shapes] diff --git a/frontend/src/app/main/ui/workspace/sidebar/options/menus/measures.cljs b/frontend/src/app/main/ui/workspace/sidebar/options/menus/measures.cljs index f4a454af54..c9b3190574 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/options/menus/measures.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/options/menus/measures.cljs @@ -374,7 +374,7 @@ (fn [value attr] (if (or (string? value) (number? value)) (st/emit! (udw/trigger-bounding-box-cloaking ids) - (udw/update-dimensions ids attr value)) + (udw/update-dimensions-coalesced ids attr value)) (st/emit! (udw/trigger-bounding-box-cloaking ids) (dwta/apply-token-from-input {:token (first value) :attrs #{attr} @@ -408,7 +408,7 @@ (if (or (string? value) (number? value)) (let [value (fixed-decimal-value value)] (st/emit! (udw/trigger-bounding-box-cloaking ids)) - (st/emit! (udw/increase-rotation ids value))) + (st/emit! (udw/increase-rotation-coalesced ids value))) (st/emit! (udw/trigger-bounding-box-cloaking ids) (dwta/apply-token-from-input {:token (first value) :attrs #{:rotation} diff --git a/frontend/test/frontend_tests/logic/sidebar_transform_coalescing_test.cljs b/frontend/test/frontend_tests/logic/sidebar_transform_coalescing_test.cljs new file mode 100644 index 0000000000..40c174d9cf --- /dev/null +++ b/frontend/test/frontend_tests/logic/sidebar_transform_coalescing_test.cljs @@ -0,0 +1,226 @@ +;; 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.logic.sidebar-transform-coalescing-test + "Regression tests for the sidebar measures panel transform coalescing + (React error #185): a burst of numeric-input gestures (held arrow key, + wheel, scrub) must collapse to a handful of commits, and the trailing + flush must land the exact final value." + (:require + [app.common.geom.rect :as grc] + [app.common.test-helpers.compositions :as ctho] + [app.common.test-helpers.files :as cthf] + [app.common.test-helpers.shapes :as cths] + [app.main.data.workspace :as dw] + [app.main.data.workspace.transforms :as-alias dwt] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.pages :as thp] + [frontend-tests.helpers.state :as ths] + [frontend-tests.helpers.wasm :as thw] + [potok.v2.core :as ptk])) + +(t/use-fixtures :each + {:before (fn [] (thp/reset-idmap!) (thw/setup-wasm-mocks!)) + :after (fn [] (thw/teardown-wasm-mocks!))}) + +(def ^:private flush-wait-ms + "How long to keep the store running after a burst so the 50 ms + trailing flush fires before checking the final state." + 150) + +(defn- count-events + "Return an atom counting how many events of `type` get emitted on the + store input stream (i.e. the real commits triggered by the coalescer)." + [store type] + (let [counter (atom 0)] + (->> (ptk/input-stream store) + (rx/filter (ptk/type? type)) + (rx/tap (fn [_] (swap! counter inc))) + (rx/subs! (fn [_] nil))) + counter)) + +(defn- run-store-timed + "Like `ths/run-store`, but emits `:the/end` `wait-ms` after `events` + so the timer-based coalescing (throttle/debounce) gets to fire." + [store done events wait-ms completed-cb] + (->> (ptk/input-stream store) + (rx/filter #(= :the/end %)) + (rx/take 1) + (rx/tap (fn [_] (completed-cb @store))) + (rx/subs! (fn [_] nil) + (fn [cause] + (done) + (t/do-report {:type :error :message "Stream error" :actual cause})) + (fn [_] (done)))) + (doseq [event events] + (ptk/emit! store event)) + (js/setTimeout (fn [] (ptk/emit! store :the/end)) wait-ms)) + +(defn- burst + "A burst of `n` events built with `make-event`, like the stream of + calls a held arrow key or a scrub gesture produces." + [n make-event] + (mapv make-event (range 1 (inc n)))) + +;; --- Positions (update-positions, coalesced in place) ----------------- + +(t/deftest update-positions-burst-commits-exact-final-value-wasm + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (ctho/add-frame :frame1 :x 0 :y 0 :width 100 :height 100)) + store (ths/setup-store file) + frame1 (cths/get-shape file :frame1) + commits (count-events store ::dwt/update-position) + events (burst 20 (fn [i] (dw/update-positions [(:id frame1)] {:x (+ 100 i)})))] + (run-store-timed + store done events flush-wait-ms + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + frame1' (cths/get-shape file' :frame1) + x (-> frame1' :points grc/points->rect :x)] + ;; The trailing flush lands the exact final value... + (t/is (= 120 x)) + ;; ...and the 20-event burst collapsed to a handful of commits. + (t/is (<= @commits 3)))))))) + +(t/deftest update-positions-burst-commits-exact-final-value-svg + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (ctho/add-frame :frame1 :x 0 :y 0 :width 100 :height 100)) + store (ths/setup-store file {:renderer :svg}) + frame1 (cths/get-shape file :frame1) + commits (count-events store ::dwt/update-position) + events (burst 20 (fn [i] (dw/update-positions [(:id frame1)] {:x (+ 100 i)})))] + (run-store-timed + store done events flush-wait-ms + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + frame1' (cths/get-shape file' :frame1) + x (-> frame1' :points grc/points->rect :x)] + (t/is (= 120 x)) + (t/is (<= @commits 3)))))))) + +(t/deftest update-positions-burst-merges-x-and-y + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (ctho/add-frame :frame1 :x 0 :y 0 :width 100 :height 100)) + store (ths/setup-store file) + frame1 (cths/get-shape file :frame1) + commits (count-events store ::dwt/update-position) + events (into (burst 10 (fn [i] (dw/update-positions [(:id frame1)] {:x (+ 100 i)}))) + (burst 10 (fn [i] (dw/update-positions [(:id frame1)] {:y (+ 200 i)}))))] + (run-store-timed + store done events flush-wait-ms + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + frame1' (cths/get-shape file' :frame1) + rect (-> frame1' :points grc/points->rect)] + ;; Partial position maps of the same shape merge, so the last + ;; value of each attribute lands. + (t/is (= 110 (:x rect))) + (t/is (= 210 (:y rect))) + (t/is (<= @commits 3)))))))) + +;; --- Dimensions (update-dimensions-coalesced) -------------------------- + +(t/deftest update-dimensions-burst-commits-exact-final-value-wasm + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (ctho/add-rect :rect1 :x 0 :y 0 :width 100 :height 100)) + store (ths/setup-store file) + rect1 (cths/get-shape file :rect1) + commits (count-events store ::dwt/update-dimensions) + events (burst 20 (fn [i] (dw/update-dimensions-coalesced [(:id rect1)] :width (+ 100 i))))] + (run-store-timed + store done events flush-wait-ms + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + rect1' (cths/get-shape file' :rect1) + width (-> rect1' :points grc/points->rect :width)] + (t/is (= 120 width)) + (t/is (<= @commits 3)))))))) + +(t/deftest update-dimensions-burst-commits-exact-final-value-svg + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (ctho/add-rect :rect1 :x 0 :y 0 :width 100 :height 100)) + store (ths/setup-store file {:renderer :svg}) + rect1 (cths/get-shape file :rect1) + commits (count-events store ::dwt/update-dimensions) + events (burst 20 (fn [i] (dw/update-dimensions-coalesced [(:id rect1)] :width (+ 100 i))))] + (run-store-timed + store done events flush-wait-ms + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + rect1' (cths/get-shape file' :rect1) + width (-> rect1' :points grc/points->rect :width)] + (t/is (= 120 width)) + (t/is (<= @commits 3)))))))) + +(t/deftest update-dimensions-burst-merges-width-and-height + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (ctho/add-rect :rect1 :x 0 :y 0 :width 100 :height 100)) + store (ths/setup-store file) + rect1 (cths/get-shape file :rect1) + commits (count-events store ::dwt/update-dimensions) + events (into (burst 10 (fn [i] (dw/update-dimensions-coalesced [(:id rect1)] :width (+ 100 i)))) + (burst 10 (fn [i] (dw/update-dimensions-coalesced [(:id rect1)] :height (+ 200 i)))))] + (run-store-timed + store done events flush-wait-ms + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + rect1' (cths/get-shape file' :rect1) + rect (-> rect1' :points grc/points->rect)] + ;; Each attribute keeps its own latest queued value. + (t/is (= 110 (:width rect))) + (t/is (= 210 (:height rect))) + ;; At most 3 flushes; the trailing one commits both pending + ;; attributes, hence 4 commit events. + (t/is (<= @commits 4)))))))) + +;; --- Rotation (increase-rotation-coalesced) ---------------------------- + +(t/deftest increase-rotation-burst-commits-exact-final-value-wasm + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (ctho/add-rect :rect1 :x 0 :y 0 :width 100 :height 100)) + store (ths/setup-store file) + rect1 (cths/get-shape file :rect1) + commits (count-events store ::dwt/increase-rotation) + events (burst 20 (fn [i] (dw/increase-rotation-coalesced [(:id rect1)] (* i 3))))] + (run-store-timed + store done events flush-wait-ms + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + rect1' (cths/get-shape file' :rect1)] + (t/is (= 60 (:rotation rect1'))) + (t/is (<= @commits 3)))))))) + +(t/deftest increase-rotation-burst-commits-exact-final-value-svg + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (ctho/add-rect :rect1 :x 0 :y 0 :width 100 :height 100)) + store (ths/setup-store file {:renderer :svg}) + rect1 (cths/get-shape file :rect1) + commits (count-events store ::dwt/increase-rotation) + events (burst 20 (fn [i] (dw/increase-rotation-coalesced [(:id rect1)] (* i 3))))] + (run-store-timed + store done events flush-wait-ms + (fn [new-state] + (let [file' (ths/get-file-from-state new-state) + rect1' (cths/get-shape file' :rect1)] + (t/is (= 60 (:rotation rect1'))) + (t/is (<= @commits 3)))))))) diff --git a/frontend/test/frontend_tests/logic/update_position_test.cljs b/frontend/test/frontend_tests/logic/update_position_test.cljs index 8c55dacd43..93b51f95d6 100644 --- a/frontend/test/frontend_tests/logic/update_position_test.cljs +++ b/frontend/test/frontend_tests/logic/update_position_test.cljs @@ -12,7 +12,12 @@ [app.common.test-helpers.shapes :as cths] [app.main.data.workspace :as dw] [cljs.test :as t :include-macros true] - [frontend-tests.helpers.state :as ths])) + [frontend-tests.helpers.state :as ths] + [frontend-tests.helpers.wasm :as thw])) + +(t/use-fixtures :each + {:before (fn [] (thw/setup-wasm-mocks!)) + :after (fn [] (thw/teardown-wasm-mocks!))}) (t/deftest test-update-positions-multiple-ids (t/async diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index a29132a22e..31896b987e 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -35,6 +35,8 @@ [frontend-tests.logic.groups-test] [frontend-tests.logic.nudge-selected-shapes-test] [frontend-tests.logic.pasting-in-containers-test] + [frontend-tests.logic.sidebar-transform-coalescing-test] + [frontend-tests.logic.update-position-test] [frontend-tests.main-errors-test] [frontend-tests.plugins.comments-test] [frontend-tests.plugins.context-shapes-test] @@ -130,6 +132,8 @@ 'frontend-tests.logic.nudge-selected-shapes-test 'frontend-tests.logic.pasting-in-containers-test 'frontend-tests.main-errors-test + 'frontend-tests.logic.sidebar-transform-coalescing-test + 'frontend-tests.logic.update-position-test 'frontend-tests.plugins.comments-test 'frontend-tests.plugins.context-shapes-test 'frontend-tests.plugins.file-test From 4ecd8ffb89da83e959e7cf69d391358556bdc522 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 11:53:48 +0200 Subject: [PATCH 09/14] :bug: Fix crash when editing tokens with group nodes (#11144) Make sd-token-uuid nil-safe when accessing .original.id to prevent crashes when StyleDictionary emits group nodes alongside real tokens. Group nodes have an original object but no id property, causing undefined is not an object errors during interactive token resolution in the edit modal. Closes #11143 AI-assisted-by: qwen3.7-plus --- frontend/src/app/main/data/style_dictionary.cljs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frontend/src/app/main/data/style_dictionary.cljs b/frontend/src/app/main/data/style_dictionary.cljs index 446e130bdb..ba1f2dd67f 100644 --- a/frontend/src/app/main/data/style_dictionary.cljs +++ b/frontend/src/app/main/data/style_dictionary.cljs @@ -557,7 +557,8 @@ (.. sd-token -original -name)) (defn sd-token-uuid [^js sd-token] - (uuid (.-uuid (.. sd-token -original -id)))) + (when-let [id (.. sd-token -original -id)] + (uuid (.-uuid id)))) (defn- merge-name-collisions "Re-attach tokens that `ctob/tokens-tree` / `backtrace-tokens-tree` From 29dbf9ab12c789b4b85e50d5c5b41c949553a0fa Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 12:13:56 +0200 Subject: [PATCH 10/14] :bug: Validate content-type on management upload endpoints (#11026) Add media type validation to upload-tempfile and upload-org-logo management endpoints. Both stored user-supplied mtype without checking against an allowlist. Only image types and PDF are permitted. Non-public bucket assets now also carry Content-Disposition: attachment to prevent inline rendering. AI-assisted-by: mimo-v2.5-pro --- backend/src/app/http/assets.clj | 23 ++++++++++++------- backend/src/app/rpc/management/exporter.clj | 6 +++-- backend/src/app/rpc/management/nitrate.clj | 2 ++ .../backend_tests/rpc_management_test.clj | 16 +++++++++++++ common/src/app/common/media.cljc | 3 +++ 5 files changed, 40 insertions(+), 10 deletions(-) diff --git a/backend/src/app/http/assets.clj b/backend/src/app/http/assets.clj index 04dd7842ca..6258760548 100644 --- a/backend/src/app/http/assets.clj +++ b/backend/src/app/http/assets.clj @@ -49,12 +49,16 @@ [{:keys [::sto/storage ::signature-max-age ::cache-max-age] :as cfg} obj] (let [sig-max-age (or signature-max-age default-signature-max-age) cch-max-age (or cache-max-age default-cache-max-age) - {:keys [host port] :as url} (sto/get-object-url storage obj {:max-age sig-max-age})] + {:keys [host port] :as url} (sto/get-object-url storage obj {:max-age sig-max-age}) + bucket (-> obj meta :bucket) + headers (cond-> {"location" (str url) + "x-host" (cond-> host port (str ":" port)) + "x-mtype" (-> obj meta :content-type) + "cache-control" (str "max-age=" (inst-ms cch-max-age))} + (not (contains? public-buckets bucket)) + (assoc "content-disposition" "attachment"))] {::yres/status 307 - ::yres/headers {"location" (str url) - "x-host" (cond-> host port (str ":" port)) - "x-mtype" (-> obj meta :content-type) - "cache-control" (str "max-age=" (inst-ms cch-max-age))}})) + ::yres/headers headers})) (defn- serve-object-from-fs [{:keys [::path ::cache-max-age]} obj] @@ -62,9 +66,12 @@ purl (u/join (u/uri path) (sto/object->relative-path obj)) mdata (meta obj) - headers {"x-accel-redirect" (:path purl) - "content-type" (:content-type mdata) - "cache-control" (str "max-age=" (inst-ms cch-max-age))}] + bucket (:bucket mdata) + 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)) + (assoc "content-disposition" "attachment"))] {::yres/status 204 ::yres/headers headers})) diff --git a/backend/src/app/rpc/management/exporter.clj b/backend/src/app/rpc/management/exporter.clj index aac508669d..f4b7d9547f 100644 --- a/backend/src/app/rpc/management/exporter.clj +++ b/backend/src/app/rpc/management/exporter.clj @@ -6,11 +6,12 @@ (ns app.rpc.management.exporter (:require + [app.common.media :as cm] [app.common.schema :as sm] [app.common.time :as ct] [app.common.uri :as u] [app.config :as cf] - [app.media.validation :refer [schema:upload]] + [app.media.validation :as media.v] [app.rpc :as-alias rpc] [app.rpc.doc :as doc] [app.storage :as sto] @@ -21,7 +22,7 @@ (def ^:private schema:upload-tempfile-params [:map {:title "upload-templfile-params"} - [:content schema:upload]]) + [:content media.v/schema:upload]]) (def ^:private schema:upload-tempfile-result @@ -32,6 +33,7 @@ ::sm/params schema:upload-tempfile-params ::sm/result schema:upload-tempfile-result} [cfg {:keys [::rpc/profile-id content]}] + (media.v/validate-media-type! content cm/tempfile-types) (let [storage (sto/resolve cfg) hash (sto/calculate-hash (:path content)) data (-> (sto/content (:path content)) diff --git a/backend/src/app/rpc/management/nitrate.clj b/backend/src/app/rpc/management/nitrate.clj index 8f07c90612..5df54b7e7f 100644 --- a/backend/src/app/rpc/management/nitrate.clj +++ b/backend/src/app/rpc/management/nitrate.clj @@ -12,6 +12,7 @@ [app.auth.oidc :as oidc] [app.common.data :as d] [app.common.exceptions :as ex] + [app.common.media :as cm] [app.common.schema :as sm] [app.common.time :as ct] [app.common.types.organization :as cto] @@ -136,6 +137,7 @@ ::sm/result schema:upload-organization-logo-result ::nitrate/sso false} [{:keys [::sto/storage]} {:keys [content organization-id previous-id]}] + (media.v/validate-media-type! content cm/image-types) (when previous-id (sto/touch-object! storage previous-id)) (let [hash (sto/calculate-hash (:path content)) diff --git a/backend/test/backend_tests/rpc_management_test.clj b/backend/test/backend_tests/rpc_management_test.clj index 601e8b3d35..2dd25694a1 100644 --- a/backend/test/backend_tests/rpc_management_test.clj +++ b/backend/test/backend_tests/rpc_management_test.clj @@ -57,6 +57,22 @@ (t/is (not= (get-in out1 [:result :id]) (get-in out2 [:result :id]))))) +(t/deftest upload-tempfile-rejects-html-content-type + ;; N2-13: upload-tempfile must reject non-allowed content types + (let [profile (th/create-profile* 1 {:is-active true}) + path (fs/create-tempfile :dir "/tmp/penpot" :prefix "test-upload-tempfile-") + _ (io/write* path "") + params {::th/type :upload-tempfile + ::rpc/profile-id (:id profile) + :content {:filename "evil.html" + :path path + :mtype "text/html" + :size 27}} + out (th/management-command! params)] + (t/is (some? (:error out))) + (t/is (= :validation (th/ex-type (:error out)))) + (t/is (= :media-type-not-allowed (th/ex-code (:error out)))))) + (t/deftest duplicate-file (let [storage (-> (:app.storage/storage th/*system*) (configure-storage-backend)) diff --git a/common/src/app/common/media.cljc b/common/src/app/common/media.cljc index 3507ba5f59..3d67bc75b6 100644 --- a/common/src/app/common/media.cljc +++ b/common/src/app/common/media.cljc @@ -22,6 +22,9 @@ "image/gif" "image/svg+xml"}) +(def tempfile-types + (conj image-types "application/pdf")) + (defn format->extension [format] (case format From 57c9c3f6a493f7fc87853c6d47e10c2cf10ef269 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Valderrama?= Date: Mon, 17 Aug 2026 12:26:49 +0200 Subject: [PATCH 11/14] :bug: Fix sso error message (#11252) --- backend/src/app/auth/oidc.clj | 13 +++++++++---- frontend/src/app/main/ui/routes.cljs | 20 +++++++++++--------- frontend/src/app/main/ui/static.cljs | 8 ++++---- frontend/translations/en.po | 4 ++-- frontend/translations/es.po | 4 ++-- 5 files changed, 28 insertions(+), 21 deletions(-) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 715661b234..c636aeba24 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -648,10 +648,12 @@ (redirect-response uri)))) (defn- redirect-with-organization-sso-error - [{:keys [dest-url organization-id]}] + [{:keys [dest-url organization-id organization-name]}] (-> (str (or dest-url (cf/get :public-uri))) (u/append-query-param :sso-error true) (u/append-query-param :organization-id organization-id) + (cond-> organization-name + (u/append-query-param :organization-name organization-name)) (redirect-response))) (defn- redirect-to-register @@ -920,9 +922,12 @@ (l/err :hint "unexpected error on organization sso callback" :organization-id (:organization-id state) :cause cause)))) - (redirect-with-organization-sso-error - {:dest-url dest-url - :organization-id (:organization-id state)}))))) + (let [organization-id (:organization-id state) + organization-name (:name (nitrate/call cfg :get-organization-summary {:organization-id organization-id}))] + (redirect-with-organization-sso-error + {:dest-url dest-url + :organization-id organization-id + :organization-name organization-name})))))) (defn- callback-handler [cfg {:keys [params] :as request}] diff --git a/frontend/src/app/main/ui/routes.cljs b/frontend/src/app/main/ui/routes.cljs index cc0d07537d..0a898d7992 100644 --- a/frontend/src/app/main/ui/routes.cljs +++ b/frontend/src/app/main/ui/routes.cljs @@ -149,22 +149,24 @@ (defn- handle-sso-error-and-navigate "Check if the current route has an SSO error marker. If so, assign an - exception with type :sso-error and organization-id from query params, + exception with type :sso-error and organization-id/name from query params, and deliberately do NOT proceed with normal navigation: emitting `rt/navigated` would clear the exception that was just assigned. Otherwise, delegate to `check-sso-and-navigate`." [match send-event-info? url] - (let [route-name (name (get-in match [:data :name])) - sso-error? (some? (get-in match [:query-params :sso-error])) - organization-id (some-> (get-in match [:query-params :organization-id]) uuid/parse*) - team-id-str (or (get-in match [:query-params :team-id]) - (get-in match [:params :path :team-id])) ;; Fallback: team-id may be in path params for workspace routes - team-id (some-> team-id-str uuid/parse*) - is-workspace? (str/starts-with? route-name "workspace") - is-dashboard? (str/starts-with? route-name "dashboard")] + (let [route-name (name (get-in match [:data :name])) + sso-error? (some? (get-in match [:query-params :sso-error])) + organization-id (some-> (get-in match [:query-params :organization-id]) uuid/parse*) + organization-name (some-> (get-in match [:query-params :organization-name]) str/trim) + team-id-str (or (get-in match [:query-params :team-id]) + (get-in match [:params :path :team-id])) ;; Fallback: team-id may be in path params for workspace routes + team-id (some-> team-id-str uuid/parse*) + is-workspace? (str/starts-with? route-name "workspace") + is-dashboard? (str/starts-with? route-name "dashboard")] (if sso-error? (st/emit! (rt/assign-exception {:type :sso-error :organization-id organization-id + :organization-name organization-name :team-id team-id :is-workspace is-workspace? :is-dashboard is-dashboard?})) diff --git a/frontend/src/app/main/ui/static.cljs b/frontend/src/app/main/ui/static.cljs index 9232c6598c..95b73d0191 100644 --- a/frontend/src/app/main/ui/static.cljs +++ b/frontend/src/app/main/ui/static.cljs @@ -484,7 +484,7 @@ `request-dialog*` used by the no-permission dialogs) when the organization SSO exchange with the identity provider fails." {::mf/private true} - [{:keys [organization-id team-id profile is-workspace is-dashboard]}] + [{:keys [organization-id team-id profile is-workspace is-dashboard organization-name]}] (let [clean-url (mf/with-memo [] (-> (rt/get-current-href) @@ -520,7 +520,7 @@ [:> context-wrapper* {:is-dashboard (or is-dashboard (not is-workspace)) :is-workspace is-workspace :profile profile} - [:> request-dialog* {:title (tr "labels.sso-error.title") + [:> request-dialog* {:title (tr "labels.sso-error.title", organization-name) :content [(tr "labels.sso-error.desc-message")] :button-text (tr "labels.sso-error.retry") :on-button-click on-retry @@ -532,7 +532,6 @@ [{:keys [data] :as props}] (let [type (get data :type) cause (get data ::errors/instance) - organization-id (get data :organization-id) report (mf/with-memo [cause] (when (ex/exception? cause) @@ -564,7 +563,8 @@ [:> nitrate-unavailable*] :sso-error - [:> sso-error-section* {:organization-id organization-id + [:> sso-error-section* {:organization-id (get data :organization-id) + :organization-name (get data :organization-name) :team-id (get data :team-id) :profile (mf/deref refs/profile) :is-workspace (get data :is-workspace false) diff --git a/frontend/translations/en.po b/frontend/translations/en.po index ff36a11deb..014b63b258 100644 --- a/frontend/translations/en.po +++ b/frontend/translations/en.po @@ -10338,10 +10338,10 @@ msgid "notifications.invitation-canceled" msgstr "This invitation is no longer available." msgid "labels.sso-error.title" -msgstr "We couldn't sign you in to your organization" +msgstr "We couldn't sign you in to %s" msgid "labels.sso-error.desc-message" -msgstr "Sign-in with your organization's identity provider didn't complete. The provider may be unavailable, or your account may not be in its directory yet. Your Penpot account isn't affected." +msgstr "Sign-in with your organization's identity provider didn't complete. The provider may be unavailable, or your account may not be in its directory yet." msgid "labels.sso-error.retry" msgstr "Try again" diff --git a/frontend/translations/es.po b/frontend/translations/es.po index 28c0689ba4..eacd139708 100644 --- a/frontend/translations/es.po +++ b/frontend/translations/es.po @@ -9987,10 +9987,10 @@ msgid "notifications.invitation-canceled" msgstr "Esta invitación ya no está disponible." msgid "labels.sso-error.title" -msgstr "No pudimos iniciar sesión en tu organización" +msgstr "No pudimos iniciar sesión en %s" msgid "labels.sso-error.desc-message" -msgstr "El inicio de sesión con el proveedor de identidad de tu organización no se completó. Es posible que el proveedor no esté disponible o que tu cuenta aún no esté en su directorio. Tu cuenta de Penpot no se ha visto afectada." +msgstr "El inicio de sesión con el proveedor de identidad de tu organización no se completó. Es posible que el proveedor no esté disponible o que tu cuenta aún no esté en su directorio." msgid "labels.sso-error.retry" msgstr "Intentar de nuevo" From ed04d509ed4bb99f41c9cc4ccf8f7e3d7ba3f0b0 Mon Sep 17 00:00:00 2001 From: Pablo Alba Date: Mon, 17 Aug 2026 12:40:18 +0200 Subject: [PATCH 12/14] :bug: Fix bad managed error on backend sso failure (#11247) --- backend/src/app/rpc.clj | 40 +-- backend/src/app/rpc/commands/nitrate.clj | 15 +- .../test/backend_tests/rpc_nitrate_test.clj | 120 ++++++++- frontend/src/app/main/data/nitrate.cljs | 19 +- frontend/src/app/main/errors.cljs | 86 ++++++- .../test/frontend_tests/main_errors_test.cljs | 237 +++++++++++++++++- 6 files changed, 487 insertions(+), 30 deletions(-) diff --git a/backend/src/app/rpc.clj b/backend/src/app/rpc.clj index c7e33312ca..bdc912ef2d 100644 --- a/backend/src/app/rpc.clj +++ b/backend/src/app/rpc.clj @@ -261,23 +261,28 @@ (defn- wrap-nitrate-sso "Enforce Nitrate organization SSO authentication for RPC handlers. - Resolves the organization/team context from request params using priority order: - 1. Explicit :organization-id param - 2. Explicit :team-id param - 3. Explicit :project-id param -> lookup project.team_id - 4. Explicit :file-id param -> lookup file's team via join - 5. :id param dispatched by ::rpc/id-type metadata (:team, :project, or :file) + Resolves the organization/team context from request params: + 1. Explicit :organization-id param identifies the organization directly + 2. The team comes from the first available of: explicit :team-id, explicit + :project-id -> lookup project.team_id, explicit :file-id -> lookup file's + team via join, or the :id param dispatched by ::rpc/id-type metadata + (:team, :project, or :file) Once the context is resolved, checks if the user is authorized within that organization's - SSO session using nitrate/sso-session-authorized?. Authorized results are cached - by [profile-id cache-ref] for 15 minutes to avoid repeated lookups. + SSO session using nitrate/sso-session-authorized?, against the organization when it is + known and against the team otherwise. The team is resolved either way, so the raised + error can carry it. Authorized results are cached by [profile-id cache-ref] for 15 + minutes to avoid repeated lookups. Only activates when: - Nitrate flag is enabled - Endpoint requires authentication (::auth true by default) - Endpoint is not marked with ::nitrate/organization-sso false - Raises :nitrate-sso-required error if user is not authorized in the organization." + Raises :nitrate-sso-required error if user is not authorized in the organization. + The error carries the resolved :organization-id and :team-id so the client can + restart the SSO flow (via :check-nitrate-sso) instead of reporting a plain + permission failure." [_ f mdata] (if (and (contains? cf/flags :admin-console) (::auth mdata true) ;; only for endpoints that needs auth @@ -302,17 +307,22 @@ cached (cache/get organization-sso-auth-cache cache-key) result (if (some? cached) cached - (let [team-id (when-not organization-id - (or team-id - (when project-id - (:team-id (db/get-by-id cfg :project project-id {:columns [:id :team-id]}))) + ;; The team is resolved even when the organization is + ;; already known: the client needs it to restart the + ;; SSO flow without sending non-members through the + ;; organization's identity provider. + (let [team-id (or team-id + (when project-id + (:team-id (db/get-by-id cfg :project project-id {:columns [:id :team-id]}))) + (when file-id (:id (teams/get-team-for-file cfg file-id)))) request (-> (meta params) (get ::http/request)) {:keys [authorized sso]} (if organization-id (nitrate/sso-session-authorized? cfg organization-id nil request) (nitrate/sso-session-authorized? cfg nil team-id request)) entry {:authorized authorized - :organization-id (:organization-id sso)}] + :organization-id (or (:organization-id sso) organization-id) + :team-id team-id}] (when authorized (cache/get organization-sso-auth-cache cache-key (constantly entry))) entry))] @@ -320,6 +330,8 @@ (f cfg params) (ex/raise :type :authentication :code :nitrate-sso-required + :organization-id (:organization-id result) + :team-id (:team-id result) :hint "organization SSO authentication required"))) (f cfg params)))) f)) diff --git a/backend/src/app/rpc/commands/nitrate.clj b/backend/src/app/rpc/commands/nitrate.clj index 76b34e8be8..c48834662e 100644 --- a/backend/src/app/rpc/commands/nitrate.clj +++ b/backend/src/app/rpc/commands/nitrate.clj @@ -673,10 +673,15 @@ (sv/defmethod ::check-nitrate-sso "Check if a user needs to login into the organization SSO. Accepts either team-id (to look up the organization via the team) or organization-id directly. - Returns {:authorized true} when SSO is not active or the user cannot access the team. + Returns {:authorized true :reason :sso-satisfied} when SSO is not active or the + session already holds a valid entry for the organization, and + {:authorized true :reason :no-team-access} when the gate was skipped because the + user cannot access the team; the reason lets the client tell a usable session + apart from a plain permission failure. Returns {:authorized false :redirect-uri } when SSO is active; the client must redirect there. The OIDC provider itself handles - re-authentication transparently if the user already has an active SSO session." + re-authentication transparently if the user already has an active SSO session. + A nil :redirect-uri means SSO is required but the provider is not usable." {::rpc/auth true ::doc/added "2.18" ::sm/params schema:check-nitrate-sso @@ -687,11 +692,11 @@ (not (teams/has-read-permissions? cfg profile-id team-id))) ;; Let the destination RPC enforce its own permissions. Starting SSO before ;; access is established sends unrelated users through the organization's IdP. - {:authorized true} + {:authorized true :reason :no-team-access} (let [request (rph/get-request params) {:keys [authorized sso]} (nitrate/sso-session-authorized? cfg organization-id team-id request)] (if authorized - {:authorized true} + {:authorized true :reason :sso-satisfied} (if (oidc/organization-sso-discovery-uri sso) {:authorized false :redirect-uri (oidc/build-organization-sso-auth-redirect-uri cfg sso @@ -699,4 +704,4 @@ :organization-id organization-id)} {:authorized false :redirect-uri nil})))) - {:authorized true})) + {:authorized true :reason :sso-satisfied})) diff --git a/backend/test/backend_tests/rpc_nitrate_test.clj b/backend/test/backend_tests/rpc_nitrate_test.clj index 640e68f2fc..d2ce0043bf 100644 --- a/backend/test/backend_tests/rpc_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_nitrate_test.clj @@ -15,15 +15,17 @@ [app.db :as-alias db] [app.email :as eml] [app.http :as-alias http] + [app.http.errors :as http-errors] [app.nitrate :as nitrate] - [app.rpc :as-alias rpc] + [app.rpc :as rpc] [app.rpc.commands.nitrate] [app.rpc.commands.teams :as teams] [app.rpc.helpers :as rph] [backend-tests.helpers :as th] [buddy.core.codecs :as bc] [clojure.test :as t] - [cuerdas.core :as str])) + [cuerdas.core :as str] + [yetti.response :as-alias yres])) (t/use-fixtures :once th/state-init) (t/use-fixtures :each th/database-reset) @@ -87,6 +89,31 @@ nil))) +(defn- unauthorized-sso-mock + "Creates a mock for nitrate/sso-session-authorized? that reports an active + SSO the session does not satisfy. Pass nil to leave the organization out of + the nitrate payload." + [organization-id] + (fn [_cfg _organization-id _team-id _request] + {:authorized false + :sso (cond-> {:active true + :issuer "https://idp.example.com"} + (some? organization-id) + (assoc :organization-id organization-id))})) + +(defn- sso-gate-error + "Builds the SSO gate around a handler that must never be reached, and + returns the exception it raises for `params`." + [mdata params cfg] + (let [handler (fn [_cfg _params] ::handler-called) + wrapped (binding [cf/flags (conj cf/flags :admin-console)] + (#'rpc/wrap-nitrate-sso nil handler mdata))] + (try + (wrapped cfg (with-meta params {::http/request {}})) + nil + (catch Throwable cause + cause)))) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; Tests ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; @@ -112,7 +139,9 @@ (constantly "https://idp.example.com/authorize")] (let [out (th/command! params)] (t/is (th/success? out)) - (t/is (= {:authorized true} (:result out)))))))) + ;; The reason tells the client this is a permission problem, not a + ;; usable SSO session. + (t/is (= {:authorized true :reason :no-team-access} (:result out)))))))) (t/deftest check-nitrate-sso-keeps-gate-for-team-member (let [team-owner (th/create-profile* 1 {:is-active true}) @@ -169,6 +198,91 @@ :redirect-uri redirect-uri} (:result out)))))))) +(t/deftest check-nitrate-sso-reports-a-satisfied-gate-for-a-valid-session + (let [team-owner (th/create-profile* 1 {:is-active true}) + team (th/create-team* 1 {:profile-id (:id team-owner)}) + organization-id (uuid/random) + params (with-meta + {::th/type :check-nitrate-sso + ::rpc/profile-id (:id team-owner) + :team-id (:id team) + :url "https://penpot.example.com/#/workspace"} + {::http/request {}})] + (binding [cf/flags (conj cf/flags :admin-console)] + (with-redefs [nitrate/sso-session-authorized? + (fn [_cfg _organization-id _team-id _request] + {:authorized true + :sso {:active true + :issuer "https://idp.example.com" + :organization-id organization-id}})] + (let [out (th/command! params)] + (t/is (th/success? out)) + (t/is (= {:authorized true :reason :sso-satisfied} (:result out)))))))) + +(t/deftest nitrate-sso-required-error-resolves-the-team-from-the-file + (t/testing "the workspace path, where the file id arrives as :id, still reports the team" + (let [profile (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + organization-id (uuid/random)] + (with-redefs [nitrate/sso-session-authorized? (unauthorized-sso-mock organization-id)] + (let [data (ex-data (sso-gate-error {::rpc/id-type :file} + {::rpc/profile-id (:id profile) + :id (:id file)} + th/*system*))] + (t/is (= :authentication (:type data))) + (t/is (= :nitrate-sso-required (:code data))) + (t/is (= organization-id (:organization-id data))) + (t/is (= (:default-team-id profile) (:team-id data)))))))) + +(t/deftest nitrate-sso-required-error-keeps-the-team-known-by-the-request + (t/testing "an explicit team-id is not dropped by an explicit organization-id" + (let [profile-id (uuid/random) + team-id (uuid/random) + organization-id (uuid/random)] + ;; The nitrate payload carries no organization-id here, so the one from + ;; the request params is the only one left to report. + (with-redefs [nitrate/sso-session-authorized? (unauthorized-sso-mock nil)] + (let [data (ex-data (sso-gate-error {} + {::rpc/profile-id profile-id + :team-id team-id + :organization-id organization-id} + {}))] + (t/is (= organization-id (:organization-id data))) + (t/is (= team-id (:team-id data)))))))) + +(t/deftest nitrate-sso-required-error-resolves-the-team-with-a-known-organization + (t/testing "knowing the organization does not stop the team lookup" + (let [profile (th/create-profile* 1 {:is-active true}) + file (th/create-file* 1 {:profile-id (:id profile) + :project-id (:default-project-id profile)}) + organization-id (uuid/random)] + (with-redefs [nitrate/sso-session-authorized? (unauthorized-sso-mock nil)] + (let [data (ex-data (sso-gate-error {} + {::rpc/profile-id (:id profile) + :organization-id organization-id + :file-id (:id file)} + th/*system*))] + (t/is (= organization-id (:organization-id data))) + (t/is (= (:default-team-id profile) (:team-id data)))))))) + +(t/deftest nitrate-sso-required-error-reaches-the-client-in-the-401-body + (t/testing "the ids survive the http error response, not only the exception" + (let [profile-id (uuid/random) + team-id (uuid/random) + organization-id (uuid/random)] + (with-redefs [nitrate/sso-session-authorized? (unauthorized-sso-mock organization-id)] + (let [cause (sso-gate-error {} + {::rpc/profile-id profile-id + :team-id team-id} + {}) + response (http-errors/handle cause {}) + body (::yres/body response)] + (t/is (= 401 (::yres/status response))) + (t/is (= :nitrate-sso-required (:code body))) + (t/is (= organization-id (:organization-id body))) + (t/is (= team-id (:team-id body)))))))) + (t/deftest leave-organization-happy-path-no-extra-teams (let [profile-owner (th/create-profile* 1 {:is-active true}) profile-user (th/create-profile* 2 {:is-active true}) diff --git a/frontend/src/app/main/data/nitrate.cljs b/frontend/src/app/main/data/nitrate.cljs index 3470dcc81d..81008c3fff 100644 --- a/frontend/src/app/main/data/nitrate.cljs +++ b/frontend/src/app/main/data/nitrate.cljs @@ -352,19 +352,30 @@ (rx/empty))))))))))) +(defn check-organization-sso + "Asks the backend whether the organization SSO gate can be satisfied for + `dest-url`, returning an observable of the raw `:check-nitrate-sso` + result: `:authorized` with a `:reason` of `:sso-satisfied` or + `:no-team-access`, or `:authorized false` with a `:redirect-uri` (nil + when SSO is required but the provider is unusable). Failures are not + caught, so a network blip stays a network error for the caller to + handle instead of masquerading as an answer." + [{:keys [team-id organization-id dest-url]}] + (rp/cmd! :check-nitrate-sso (d/without-nils {:team-id team-id + :organization-id organization-id + :url dest-url}))) + (defn retry-organization-sso "Retries the organization SSO login flow after a failed attempt, reusing the same check-nitrate-sso RPC used elsewhere to move the user through the organization's identity provider. Passing `team-id` enables the backend's non-member short-circuit. Falls back to navigating straight to `dest-url` when no fresh SSO redirect is needed or available." - [{:keys [team-id organization-id dest-url]}] + [{:keys [dest-url] :as params}] (ptk/reify ::retry-organization-sso ptk/WatchEvent (watch [_ _ _] - (->> (rp/cmd! :check-nitrate-sso (d/without-nils {:team-id team-id - :organization-id organization-id - :url dest-url})) + (->> (check-organization-sso params) (rx/map (fn [{:keys [redirect-uri]}] (rt/nav-raw :uri (or redirect-uri dest-url)))) (rx/catch (fn [_] diff --git a/frontend/src/app/main/errors.cljs b/frontend/src/app/main/errors.cljs index de4ca4f5bd..f8b5ad9dc4 100644 --- a/frontend/src/app/main/errors.cljs +++ b/frontend/src/app/main/errors.cljs @@ -13,6 +13,7 @@ [app.main.data.auth :as da] [app.main.data.event :as ev] [app.main.data.modal :as modal] + [app.main.data.nitrate :as dnt] [app.main.data.notifications :as ntf] [app.main.data.workspace :as-alias dw] [app.main.router :as rt] @@ -21,6 +22,7 @@ [app.util.globals :as g] [app.util.i18n :refer [tr]] [app.util.timers :as ts] + [beicon.v2.core :as rx] [cuerdas.core :as str] [potok.v2.core :as ptk])) @@ -234,9 +236,8 @@ ;; We receive a explicit authentication error; If the uri is for ;; workspace, dashboard, viewer or settings, then assign the exception ;; for show the error page. Otherwise this explicitly clears all -;; profile data and redirect the user to the login page. This is here -;; and not in app.main.errors because of circular dependency. -(defmethod ptk/handle-error :authentication +;; profile data and redirect the user to the login page. +(defn- show-authentication-error [error] (let [message (tr "errors.auth.unable-to-login") uri (rt/get-current-href) @@ -253,6 +254,85 @@ (st/emit! (da/logout)) (ts/schedule 500 #(st/emit! (ntf/warn message))))))) +;; The user does belong to an organization with SSO active, but there is +;; no provider to send them to (unusable or incomplete SSO config). Show +;; the SSO error dialog, which offers an explicit retry, rather than +;; claiming they have no access. +(defn- show-sso-error + [{:keys [organization-id team-id]}] + (let [uri (rt/get-current-href)] + (st/async-emit! + (rt/assign-exception {:type :sso-error + :organization-id organization-id + :team-id team-id + :is-workspace (str/includes? uri "workspace") + :is-dashboard (str/includes? uri "dashboard")})))) + +;; A page issues many SSO-guarded requests at once, and all of them fail +;; together the moment the organization SSO session lapses; without this +;; only-one-in-flight guard each of them would start its own identity +;; provider round-trip. +(def ^:private sso-renewal-pending? (volatile! false)) + +(defn- renew-organization-sso + "Recover from a request rejected by the organization SSO gate. + + Asks the backend what can be done for the current location and acts on + the answer: go through the identity provider when there is one (it + re-authenticates transparently while the user still has a live session + with it), retry the location when the gate turns out to be satisfied + already (another tab renewed the session, or SSO was turned off), show + the SSO error dialog when SSO is required but unusable, and report a + permission failure only when the user really has no access to the team. + A failing check is left to the generic error handling, so a network + blip is not turned into a permission error." + [{:keys [organization-id team-id] :as error}] + (when-not @sso-renewal-pending? + (vreset! sso-renewal-pending? true) + (let [dest-url (rt/get-current-href)] + (->> (dnt/check-organization-sso + {:organization-id organization-id + :team-id team-id + :dest-url dest-url}) + ;; Release the guard however the check ends, including an + ;; unsubscription or a completion without a result: a stuck guard + ;; would silently drop every later rejection. + (rx/finalize (fn [] (vreset! sso-renewal-pending? false))) + (rx/subs! (fn [{:keys [authorized reason redirect-uri]}] + (cond + ;; SSO must be renewed and we know where to send them + (some? redirect-uri) + (st/emit! (rt/nav-raw :uri (str redirect-uri))) + + ;; The gate is satisfied after all, so the request + ;; that failed can be retried. Only an affirmative + ;; reason is accepted here: reloading on any + ;; unrecognized "authorized" answer would spin + ;; whenever the reload hits the same rejection. + (= :sso-satisfied reason) + (st/emit! (rt/reload false)) + + ;; SSO is required but the provider is unusable + (not authorized) + (show-sso-error error) + + ;; No access to the team, so the gate was never + ;; evaluated: this really is a permission failure + :else + (show-authentication-error error))) + on-error))))) + +(defmethod ptk/handle-error :authentication + [error] + ;; Without an organization or a team there is nothing to check, and asking + ;; anyway would fail schema validation and report that instead of the + ;; authentication problem the user actually hit. + (if (and (= :nitrate-sso-required (get error :code)) + (or (some? (get error :organization-id)) + (some? (get error :team-id)))) + (renew-organization-sso error) + (show-authentication-error error))) + ;; Error that happens on an active business model validation does not ;; passes an validation (example: profile can't leave a team). From ;; the user perspective a error flash message should be visualized but diff --git a/frontend/test/frontend_tests/main_errors_test.cljs b/frontend/test/frontend_tests/main_errors_test.cljs index d09024ac5c..fbbf852c82 100644 --- a/frontend/test/frontend_tests/main_errors_test.cljs +++ b/frontend/test/frontend_tests/main_errors_test.cljs @@ -11,10 +11,17 @@ - stale-asset-error? – pure predicate - exception->error-data – pure transformer - on-error re-entrancy guard – prevents recursive invocations - - flash schedules async emit – ntf/show is not emitted synchronously" + - flash schedules async emit – ntf/show is not emitted synchronously + - organization SSO recovery – expired SSO sessions go back to the provider" (:require [app.main.errors :as errors] + [app.main.repo :as rp] + [app.main.router :as rt] + [app.main.store :as st] + [app.util.timers :as tm] + [beicon.v2.core :as rx] [cljs.test :as t :include-macros true] + [frontend-tests.helpers.mock :as mock] [potok.v2.core :as ptk])) ;; --------------------------------------------------------------------------- @@ -134,3 +141,231 @@ (errors/on-error (ex-info "test" {:type ::test-reentrant :hint "first"})) ;; The guard must have allowed only the first invocation through. (t/is (= 1 @reentrant-call-count)))) + +;; --------------------------------------------------------------------------- +;; Expired organization SSO session +;; +;; The backend rejects SSO-guarded requests with an :authentication error +;; coded :nitrate-sso-required once the organization SSO session lapses. +;; The user must be sent back through the identity provider instead of +;; being told they have no access to the file. +;; --------------------------------------------------------------------------- + +(def ^:private workspace-href + "https://penpot.example.com/#/workspace?team-id=b8f8bb52-8b70-8144-8004-4a5085f0bdc9") + +(def ^:private organization-id "d1a4c0f2-2f36-8114-8006-1b0e6d9d0c11") + +(defn- sso-required-error + [] + {:type :authentication + :code :nitrate-sso-required + :organization-id organization-id + :team-id "b8f8bb52-8b70-8144-8004-4a5085f0bdc9"}) + +(t/deftest expired-organization-sso-navigates-to-identity-provider + (t/testing "the browser is sent to the identity provider instead of an error page" + (let [events (atom [])] + (with-redefs [rp/cmd! + (mock/stub + (fn [_command _params] + (rx/of {:authorized false + :redirect-uri "https://idp.example.com/authorize"}))) + + rt/get-current-href + (constantly workspace-href) + + st/emit! + (mock/stub (fn [& emitted] (swap! events into emitted)))] + + (errors/on-error (sso-required-error)) + + (t/is (= [::rt/nav-raw] (mapv ptk/type @events))))))) + +(t/deftest expired-organization-sso-comes-back-to-the-current-location + (t/testing "the SSO check asks the provider to return the user where they were" + (let [rpc-calls (atom [])] + (with-redefs [rp/cmd! + (mock/stub + (fn [command params] + (swap! rpc-calls conj {:command command :params params}) + (rx/of {:authorized false + :redirect-uri "https://idp.example.com/authorize"}))) + + rt/get-current-href + (constantly workspace-href) + + st/emit! mock/noop] + + (errors/on-error (sso-required-error)) + + (t/is (= [{:command :check-nitrate-sso + :params {:team-id "b8f8bb52-8b70-8144-8004-4a5085f0bdc9" + :organization-id organization-id + :url workspace-href}}] + @rpc-calls)))))) + +(t/deftest already-satisfied-organization-sso-retries-the-location + (t/testing "a session renewed meanwhile (e.g. in another tab) reloads instead of erroring" + (let [events (atom [])] + (with-redefs [rp/cmd! + (mock/stub + (fn [_command _params] + (rx/of {:authorized true :reason :sso-satisfied}))) + + rt/get-current-href + (constantly workspace-href) + + st/emit! + (mock/stub (fn [& emitted] (swap! events into emitted)))] + + (errors/on-error (sso-required-error)) + + (t/is (= [::rt/reload] (mapv ptk/type @events))))))) + +(t/deftest organization-sso-without-usable-provider-shows-the-sso-error-dialog + (t/testing "SSO is required but there is nowhere to go: offer a retry, not a permission error" + (let [assigned* (atom nil)] + (with-redefs [rp/cmd! + (mock/stub + (fn [_command _params] + (rx/of {:authorized false :redirect-uri nil}))) + + rt/get-current-href + (constantly workspace-href) + + rt/assign-exception + (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))] + + (errors/on-error (sso-required-error)) + + (t/is (= :sso-error (:type @assigned*))) + (t/is (= organization-id (:organization-id @assigned*))) + (t/is (true? (:is-workspace @assigned*))))))) + +(t/deftest organization-sso-without-team-access-reports-a-permission-failure + (t/testing "a user who cannot reach the team keeps getting the authentication error" + (let [assigned* (atom nil)] + (with-redefs [rp/cmd! + (mock/stub + (fn [_command _params] + (rx/of {:authorized true :reason :no-team-access}))) + + rt/get-current-href + (constantly workspace-href) + + rt/assign-exception + (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))] + + (errors/on-error (sso-required-error)) + + (t/is (= :authentication (:type @assigned*))) + (t/is (= :nitrate-sso-required (:code @assigned*))))))) + +(t/deftest organization-sso-does-not-retry-on-an-unexplained-authorization + (t/testing "reloading on an answer we don't understand would spin on the same rejection" + (let [events (atom [])] + (with-redefs [rp/cmd! + (mock/stub (fn [_command _params] (rx/of {:authorized true}))) + + rt/get-current-href + (constantly workspace-href) + + rt/assign-exception + (fn [error] (ptk/data-event ::assigned error)) + + ;; async-emit! is variadic-only, so the replacement must be + ;; variadic too for the compiled static dispatch to find it + st/async-emit! + (fn [& emitted] (swap! events into emitted))] + + (errors/on-error (sso-required-error)) + + (t/is (= [::assigned] (mapv ptk/type @events))))))) + +(t/deftest organization-sso-error-without-context-is-reported-as-it-arrives + (t/testing "with no organization and no team there is nothing to check" + (let [rpc-calls (atom 0) + assigned* (atom nil)] + (with-redefs [rp/cmd! + (mock/stub (fn [_command _params] + (swap! rpc-calls inc) + (rx/empty))) + + rt/get-current-href + (constantly workspace-href) + + rt/assign-exception + (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))] + + (errors/on-error {:type :authentication + :code :nitrate-sso-required}) + + (t/is (zero? @rpc-calls)) + (t/is (= :nitrate-sso-required (:code @assigned*))))))) + +(t/deftest a-resultless-organization-sso-check-does-not-wedge-later-rejections + (t/testing "the one-in-flight guard is released even when no answer arrives" + (let [rpc-calls (atom 0)] + (with-redefs [rp/cmd! + (mock/stub (fn [_command _params] + (swap! rpc-calls inc) + (rx/empty))) + + rt/get-current-href + (constantly workspace-href) + + st/emit! mock/noop] + + (errors/on-error (sso-required-error)) + (errors/on-error (sso-required-error)) + + (t/is (= 2 @rpc-calls)))))) + +;; A failing check must stay a failing check: the generic handling turns it +;; into a toast, whereas swallowing it would show a permission error for +;; what may be a momentary network blip. The mocked RPC fails on a later +;; tick, like a real request, so the handler is not inside on-error's +;; re-entrancy guard when the failure arrives. + +(def ^:private check-failures (atom [])) + +(defmethod ptk/handle-error ::test-check-failure + [error] + (swap! check-failures conj error)) + +(t/deftest failing-organization-sso-check-is-not-reported-as-missing-access + (t/async done + (reset! check-failures []) + (let [assigned* (atom nil)] + (mock/with-mocks + {rp/cmd! + (mock/stub + (fn [_command _params] + (->> (rx/timer 0) + (rx/mapcat (fn [_] + (rx/throw (ex-info "boom" {:type ::test-check-failure}))))))) + + rt/get-current-href + (constantly workspace-href) + + rt/assign-exception + (fn [error] + (reset! assigned* error) + (ptk/data-event ::assigned error))} + + (fn [done'] + (errors/on-error (sso-required-error)) + (tm/schedule + 50 + (fn [] + (t/is (= [::test-check-failure] (mapv :type @check-failures))) + (t/is (nil? @assigned*)) + (done')))) + done)))) From f96d850049e07709480f867a15fa0ace1156e47c Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 13:31:24 +0200 Subject: [PATCH 13/14] :paperclip: Add ste skill to opencode --- .opencode/skills/ste/SKILL.md | 78 +++++++++++++++++++ .opencode/skills/ste/references/examples.md | 67 ++++++++++++++++ .../ste/references/word-substitutions.md | 68 ++++++++++++++++ AGENTS.md | 6 ++ 4 files changed, 219 insertions(+) create mode 100644 .opencode/skills/ste/SKILL.md create mode 100644 .opencode/skills/ste/references/examples.md create mode 100644 .opencode/skills/ste/references/word-substitutions.md diff --git a/.opencode/skills/ste/SKILL.md b/.opencode/skills/ste/SKILL.md new file mode 100644 index 0000000000..a53456ccf0 --- /dev/null +++ b/.opencode/skills/ste/SKILL.md @@ -0,0 +1,78 @@ +--- +name: ste +description: Write or rewrite text in ASD-STE100 Simplified Technical English. ONLY use this skill when the user explicitly invokes it by name — i.e. they type "/ste" or literally write "use the ste skill" / "apply ASD-STE100". Do NOT trigger it on paraphrased intent such as "simplify this", "make it clearer", "write technical documentation", or "shorter sentences please" — the user has deliberately scoped this skill to explicit invocation only. For those requests, respond normally without loading this skill unless they name it. +--- + +# ASD-STE100 Simplified Technical English + +Apply the ASD-STE100 standard to all prose you produce in this task. Do not announce that you use STE, do not name the standard, and do not explain the style unless the user asks. If the user later asks you to "write more naturally," ask one short question to confirm they want to leave STE before you drop it. + +Compliance note (for you, not for output): the official specification and its dictionary are copyright ASD. This skill encodes paraphrased rules and a publicly sourced word list. For certified aerospace/defense deliverables, tell the user that full compliance requires the free official specification (asd-ste100.org) and a human sign-off. Never claim certified compliance. + +## Step 0 — Classify the text + +Before writing a single sentence, decide: is this **procedural** text (instructions someone follows) or **descriptive** text (explanation, background, description)? Every limit below depends on this. Mixed documents get classified section by section. + +## Core rules + +### Sentences +- Procedural: maximum **20 words** per sentence. +- Descriptive: maximum **25 words** per sentence. +- Maximum **6 sentences** per paragraph. One topic per paragraph. +- One instruction per sentence. Two actions in one sentence only if they occur at the same time. +- Put a condition BEFORE its command: "If the pressure decreases, close the valve." +- Do not omit articles, subjects, or verbs to save words. "Ensure file exists" is wrong; "Make sure that the file exists" is correct. Keep the word "that" after verbs like "make sure." +- Numbers, units with numbers, abbreviations, quoted strings, code identifiers, and proper nouns each count as one word. + +### Verbs +- Allowed forms only: infinitive, imperative, simple present, simple past, simple future, and past participle used as an adjective. +- Never use present perfect or continuous forms. "We have received" → "We received." "is being tested" → a simple form. +- Never use an -ing form as a verb. An -ing word is allowed only inside a technical name ("the mounting bracket," "logging"). +- Active voice. Passive is allowed only in descriptive text when the agent is unknown or unimportant. +- Instructions use the imperative: "Open the panel," not "You must open the panel" or "The panel should be opened." +- Express actions as verbs, not nouns: "compress the file," not "perform compression of the file." +- Modals: use **can** (possibility), **will** (future), **must** (requirement). Do not use should, would, could, may, might. A hedge becomes a fact or a "can": "an explosion can occur." +- No phrasal verbs: "go down" → "decrease," "set up" → "install," "carry out" → "do." + +### Words +- One word, one meaning, one part of speech, used consistently. Never rotate synonyms: pick one name for a thing and repeat it. +- Before drafting, replace unapproved vocabulary. Read `references/word-substitutions.md` and apply it; it is the working dictionary for this skill. +- Domain-specific nouns (part names, tool names, product names, UI labels) and domain verbs (drill, ream, boot, compile) are your **technical nouns/verbs** — keep them as-is, use each consistently, and do not verb a noun or noun a verb. +- Noun clusters: maximum **3 words** ("overhead panel light" is the limit). Longer clusters get decomposed with prepositions or hyphenated on first use: "main-gear-door retraction-winch handle." +- American English spelling. +- No Latin abbreviations: "e.g." → "for example," "i.e." → "that is," delete "etc." + +### Punctuation +- No semicolons — write two sentences. +- Parentheses only for references, abbreviations, and item numbers. +- Hyphenate words that act as one unit; a hyphenated word counts as one word. +- No contractions. + +### Warnings, cautions, notes +- **WARNING** = risk of injury or death. **CAUTION** = risk of damage. **NOTE** = information only, never an instruction. +- Start a warning or caution with the command or condition, then give the risk: + "WARNING: Do not touch the terminal. The terminal has a dangerous voltage." +- Notes obey the 25-word descriptive limit. + +## Step 2 — Self-check pass + +After drafting, scan your text once for each of these and fix every hit before you respond: + +1. Any sentence over the 20/25-word limit for its type +2. Contractions, semicolons +3. "should," "would," "could," "may," "might" +4. "has been," "have been," "had been," "is being," "was being" +5. -ing words used as verbs +6. Missing articles (a/an/the/this) before nouns +7. Synonym rotation (the same object under two names) +8. Any word in the unapproved column of `references/word-substitutions.md` +9. Warnings that state the risk before the command + +## Reference files + +- `references/word-substitutions.md` — unapproved → approved word mappings and one-meaning rulings. Read it before drafting; it is short. +- `references/examples.md` — worked before/after rewrites (procedural, descriptive, warnings, common mistakes). Read it when rewriting existing text or when unsure how a rule applies. + +## What NOT to touch + +Code blocks, command strings, file paths, error messages, quoted UI text, and proper nouns stay exactly as written. STE applies to the prose around them. diff --git a/.opencode/skills/ste/references/examples.md b/.opencode/skills/ste/references/examples.md new file mode 100644 index 0000000000..b111db6229 --- /dev/null +++ b/.opencode/skills/ste/references/examples.md @@ -0,0 +1,67 @@ +# Worked before/after examples + +## Verb forms + +| Before | After | +|---|---| +| We have received the technical reports from HQ. | We received the technical reports from HQ. | +| This device has been being used at Boeing since 2005. | Boeing started to use this device in 2005. | +| The test is continued by the operator. | Continue the test. | +| The screws should be replaced. | Replace the screws. | +| The system is currently running diagnostics. | The system does diagnostic tests now. | + +## Vocabulary and phrasing + +| Before | After | +|---|---| +| Ensure file exists before running. | Make sure that the file exists before you run the command. | +| Rotate the cover until the jacks are accessible. | Turn the cover until you can get access to the jacks. | +| Extend the jack until the wheels are clear of the ground. | Extend the jack until the wheels do not touch the ground. | +| Make sure that these steps are followed. | Obey these steps. | +| Utilize approximately 3 liters of water. | Use about 3 liters of water. | +| Perform a compression of the log files prior to upload. | Compress the log files before you upload them. | + +## Noun clusters + +| Before | After | +|---|---| +| Main gear door retraction winch handle | Main-gear-door retraction-winch handle | +| Runway light connection resistance calibration | Calibration of the resistance of the runway light connection | +| Remove the engine transmission housing attachment bolts. | Remove the bolts that attach the transmission housing to the engine. | + +## Procedural rewrite (condition first, one instruction per sentence) + +Before: +> You should check that the reservoir has been filled with the correct fluid and, after attaching a clear tube to the bleed screw while ensuring its free end is submerged, the pedal can be pumped and held so that opening the screw releases trapped air. + +After: +> 1. Fill the reservoir with the correct fluid. +> 2. Attach a clear tube to the bleed screw. +> 3. Put the free end of the tube in a container of fluid. +> 4. Push the pedal three times. Hold the pedal down. +> 5. Open the bleed screw one half turn. Air and fluid flow into the tube. +> 6. Close the bleed screw. Release the pedal. +> 7. If air continues to come out, do steps 4 thru 6 again. + +## Warnings and cautions (command first, then risk) + +Before: +> Note that serious data loss may potentially occur if the --force flag is used against production. + +After: +> CAUTION: Do not use the --force flag on the production database. The flag deletes the rows that do not match the source. + +Before: +> Touching the terminal could result in electrocution. + +After: +> WARNING: Do not touch the terminal. The terminal has a dangerous voltage. + +## Common mistakes checklist + +- Dropped articles: "Insert pin in bracket" → "Insert the pin in the bracket." +- Synonym rotation: check/verify/confirm for the same action → one term, everywhere. +- Hedges: "you may want to," "it is recommended that" → an imperative or "must." +- Instruction buried in a NOTE: notes never instruct. Move the instruction to a numbered step. +- Semicolon joining two clauses → two sentences. +- "There are three bolts on the panel" → "The panel has three bolts." diff --git a/.opencode/skills/ste/references/word-substitutions.md b/.opencode/skills/ste/references/word-substitutions.md new file mode 100644 index 0000000000..8cda2511c1 --- /dev/null +++ b/.opencode/skills/ste/references/word-substitutions.md @@ -0,0 +1,68 @@ +# Word substitutions and one-meaning rulings + +Compiled from public secondary sources (STEMG/ASD public pages, TechScribe, Acrolinx, training materials). This is a working approximation, not the official ASD dictionary. When a word is not listed here and feels formal or Latin-derived, prefer the shortest common alternative. + +## Unapproved → approved + +| Do not use | Use instead | +|---|---| +| utilize, leverage, employ | use | +| commence, initiate, begin, originate | start | +| terminate, cease, conclude | stop, end | +| ensure, verify, confirm, validate, check | make sure (that), examine | +| perform, conduct, execute, carry out | do | +| facilitate, assist | help | +| obtain, acquire, procure | get | +| sufficient, adequate | enough | +| approximately | about | +| prior to | before | +| subsequent to, following (prep.) | after | +| adjacent to | near | +| accomplish | do | +| additional, supplementary | more | +| attempt | try | +| require, necessitate | need, must | +| mandatory | necessary | +| indicate, signify | show | +| observe (=watch) | look at, examine | +| rotate | turn | +| deactivate | turn off, set to off | +| activate, energize (unless technical verb) | turn on, start | +| toxic | poisonous | +| in order to | to | +| via, by means of | through, with | +| due to, owing to | because of | +| in the event of/that | if | +| accessible | (rewrite: "you can get access to") | +| remainder | rest | +| demonstrate | show | +| modify, alter | change | +| construct, fabricate, build | assemble, make | +| retain | keep | +| locate (=find) | find | +| depress (a button) | push, press | +| proceed | continue, go | + +## One meaning, one part of speech (canonical rulings) + +- **close** — verb only: to move to a position that stops flow, or to operate a circuit breaker. The adjective is unapproved → use **near** ("do not go near the propeller"). +- **test** — noun only: "do a test," never "test the system." +- **check** — do not use as a verb for verification → "make sure that" or "examine." +- **follow** — means only "come after." For rules and steps use **obey**: "Obey the safety instructions." +- **fall** — means only "move down by gravity." For quantities use **decrease**. Never the season. +- **oil** — noun only. "Oil the bearing" → "Put oil on the bearing" / "Lubricate the bearing." +- **right** — direction only, never "correct." +- **clear** — "without blockage." "Wheels are clear of the ground" → "wheels do not touch the ground." +- **help** — verb only; the noun is **aid** ("with the aid of a mirror"). +- **above / below** — physical position only. For quantities: **more than / less than**. +- **about** — two approved senses: "approximately" and "on the subject of." Use carefully. +- **turn** — the general verb for rotation; "turn on / turn off" for power state is standard. +- **level** — approved as noun and adjective (documented exception to the one-POS rule). + +## Frequent-offender function words + +- **should / would / could / may / might** — never. Requirement → **must**. Possibility → **can**. Future → **will**. +- **etc.** — delete, or write the full list. +- **e.g. / i.e.** — "for example" / "that is." +- **any / appropriate / applicable / relevant** as hedges — replace with the specific thing meant. +- **there is / there are** openers — rewrite with a real subject: "There are three bolts on the panel" → "The panel has three bolts." diff --git a/AGENTS.md b/AGENTS.md index ac4da5c663..e542f93eb2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -34,6 +34,12 @@ Skipping this step is the #1 cause of incorrect or incomplete work. --- +## Writing Rules + +Use the `ste` skill when the user explicitly requests STE, `/ste`, or ASD-STE100. + +--- + # Memory system Memories are the **primary project guidance** — not docs or readme files. From c797656d17da345fcfa4ca75ebd689809f69ed9d Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Mon, 17 Aug 2026 13:32:12 +0200 Subject: [PATCH 14/14] :bug: Fix crash when pasting into an empty or element-focused caret (#11150) Pasting text could throw "Unknown node type" and lose the paste. The insertion paths assume the caret sits on a text node or a
, but the browser can report it on a container element (the offset being a child index, common in Firefox) or, for an empty text shape that was just focused, on nothing at all: selectAll() returned early without ever setting a selection. Add resolveTextNodePosition(), which walks a (node, offset) pair down to the addressed text node or line break and returns null instead of throwing when it cannot. The selection controller normalizes the caret with it before inserting text or a pasted fragment, and selectAll() now collapses on the line break of an empty editor so the caret is always usable. Closes #11149 AI-assisted-by: longcat-2.0-free --- .../src/editor/clipboard/paste.test.js | 67 +++++++++++ .../src/editor/content/dom/TextNode.js | 53 +++++++++ .../src/editor/content/dom/TextNode.test.js | 109 +++++++++++++++++- .../editor/controllers/SelectionController.js | 44 +++++++ .../controllers/SelectionController.test.js | 90 +++++++++++++++ .../text-editor/src/test/TextEditorMock.js | 14 ++- 6 files changed, 375 insertions(+), 2 deletions(-) create mode 100644 frontend/text-editor/src/editor/clipboard/paste.test.js diff --git a/frontend/text-editor/src/editor/clipboard/paste.test.js b/frontend/text-editor/src/editor/clipboard/paste.test.js new file mode 100644 index 0000000000..14dd7a9ac0 --- /dev/null +++ b/frontend/text-editor/src/editor/clipboard/paste.test.js @@ -0,0 +1,67 @@ +import { describe, test, expect } from "vitest"; +import { TextEditorMock } from "../../test/TextEditorMock.js"; +import { SelectionController } from "../controllers/SelectionController.js"; +import { paste } from "./paste.js"; + +/* @vitest-environment jsdom */ + +/** + * Creates a minimal `ClipboardEvent`-like object carrying plain text. + * + * @param {string} text + * @returns {object} + */ +function createPlainTextClipboardEvent(text) { + return { + preventDefault() {}, + clipboardData: { + types: ["text/plain"], + getData(type) { + return type === "text/plain" ? text : ""; + }, + }, + }; +} + +describe("paste", () => { + test("should insert plain text into an empty editor that was just focused", () => { + const textEditorMock = TextEditorMock.createTextEditorMockWithText(""); + const selection = document.getSelection(); + const selectionController = new SelectionController( + textEditorMock, + selection, + ); + textEditorMock.element.focus(); + + paste( + createPlainTextClipboardEvent("Hello, World!"), + textEditorMock, + selectionController, + ); + + expect(textEditorMock.root.textContent).toBe("Hello, World!"); + }); + + test("should insert plain text when the caret is on a paragraph element", () => { + const textEditorMock = + TextEditorMock.createTextEditorMockWithText("Hello, "); + const root = textEditorMock.root; + const paragraph = root.firstChild; + const selection = document.getSelection(); + const selectionController = new SelectionController( + textEditorMock, + selection, + ); + textEditorMock.element.focus(); + selection.setBaseAndExtent(paragraph, 1, paragraph, 1); + document.dispatchEvent(new Event("selectionchange")); + + paste( + createPlainTextClipboardEvent("World!"), + textEditorMock, + selectionController, + ); + + expect(root.textContent).toBe("Hello, World!"); + }); +}); diff --git a/frontend/text-editor/src/editor/content/dom/TextNode.js b/frontend/text-editor/src/editor/content/dom/TextNode.js index 25d484d6f4..24b2fe93c3 100644 --- a/frontend/text-editor/src/editor/content/dom/TextNode.js +++ b/frontend/text-editor/src/editor/content/dom/TextNode.js @@ -62,3 +62,56 @@ export function getClosestTextNode(node) { if (isEditor(node)) return node.firstChild.firstChild.firstChild.firstChild; throw new Error("Cannot find a text node"); } + +/** + * @typedef {Object} TextNodePosition + * @property {Text|HTMLBRElement} node + * @property {number} offset + */ + +/** + * Resolves a (node, offset) pair to an equivalent position on a text node + * or a line break. + * + * Browsers are free to report a caret on a container element, in which case + * the offset is a child index instead of a character index (Firefox does this + * routinely, e.g. on empty paragraphs). This function walks down the content + * tree to the addressed descendant so callers can always work with text + * nodes. + * + * Unlike `getClosestTextNode`, this never throws: it returns `null` when the + * position cannot be resolved, letting the caller decide the fallback. + * + * @param {Node} node + * @param {number} [offset=0] + * @returns {TextNodePosition|null} + */ +export function resolveTextNodePosition(node, offset = 0) { + if (!node) return null; + if (node.nodeType === Node.TEXT_NODE || isLineBreak(node)) { + return { node, offset }; + } + + if (isTextSpan(node)) { + // Within a text span the children are text nodes or a line break, so an + // index past the last child means "at the end of the last child". + const child = node.childNodes[offset]; + if (child) return resolveTextNodePosition(child, 0); + const lastChild = node.lastChild; + if (!lastChild) return null; + if (lastChild.nodeType !== Node.TEXT_NODE && !isLineBreak(lastChild)) { + return null; + } + return resolveTextNodePosition(lastChild, getTextNodeLength(lastChild)); + } + + if (isParagraph(node) || isRoot(node) || isEditor(node)) { + const child = node.children[offset]; + if (child) return resolveTextNodePosition(child, 0); + const lastChild = node.lastElementChild; + if (!lastChild) return null; + return resolveTextNodePosition(lastChild, lastChild.childNodes.length); + } + + return null; +} diff --git a/frontend/text-editor/src/editor/content/dom/TextNode.test.js b/frontend/text-editor/src/editor/content/dom/TextNode.test.js index fc44374b85..166da09d07 100644 --- a/frontend/text-editor/src/editor/content/dom/TextNode.test.js +++ b/frontend/text-editor/src/editor/content/dom/TextNode.test.js @@ -1,6 +1,13 @@ import { describe, test, expect } from "vitest"; -import { isTextNode, getTextNodeLength } from "./TextNode.js"; +import { + isTextNode, + getTextNodeLength, + resolveTextNodePosition, +} from "./TextNode.js"; import { createLineBreak } from "./LineBreak.js"; +import { createTextSpan, createEmptyTextSpan } from "./TextSpan.js"; +import { createParagraph } from "./Paragraph.js"; +import { createRoot } from "./Root.js"; /* @vitest-environment jsdom */ describe("TextNode", () => { @@ -25,4 +32,104 @@ describe("TextNode", () => { expect(() => getTextNodeLength(null)).toThrowError("Invalid text node"); expect(() => getTextNodeLength(0)).toThrowError("Invalid text node"); }); + + describe("resolveTextNodePosition", () => { + test("should return the same position when the node is already a text node", () => { + const textNode = new Text("Hello, World!"); + expect(resolveTextNodePosition(textNode, 5)).toStrictEqual({ + node: textNode, + offset: 5, + }); + }); + + test("should return the same position when the node is a line break", () => { + const lineBreak = createLineBreak(); + expect(resolveTextNodePosition(lineBreak, 0)).toStrictEqual({ + node: lineBreak, + offset: 0, + }); + }); + + test("should resolve a text span to its child at the given index", () => { + const textNode = new Text("Hello"); + const textSpan = createTextSpan(textNode); + expect(resolveTextNodePosition(textSpan, 0)).toStrictEqual({ + node: textNode, + offset: 0, + }); + }); + + test("should resolve a text span index past the last child to the end of its text", () => { + const textNode = new Text("Hello"); + const textSpan = createTextSpan(textNode); + expect(resolveTextNodePosition(textSpan, 1)).toStrictEqual({ + node: textNode, + offset: 5, + }); + }); + + test("should resolve a paragraph to the text node of the indexed text span", () => { + const first = new Text("Hello, "); + const second = new Text("World!"); + const paragraph = createParagraph([ + createTextSpan(first), + createTextSpan(second), + ]); + expect(resolveTextNodePosition(paragraph, 0)).toStrictEqual({ + node: first, + offset: 0, + }); + expect(resolveTextNodePosition(paragraph, 1)).toStrictEqual({ + node: second, + offset: 0, + }); + expect(resolveTextNodePosition(paragraph, 2)).toStrictEqual({ + node: second, + offset: 6, + }); + }); + + test("should resolve an empty paragraph to its line break", () => { + const textSpan = createEmptyTextSpan(); + const paragraph = createParagraph([textSpan]); + expect(resolveTextNodePosition(paragraph, 0)).toStrictEqual({ + node: textSpan.firstChild, + offset: 0, + }); + }); + + test("should resolve a root to the text node of the indexed paragraph", () => { + const first = new Text("Hello, "); + const second = new Text("World!"); + const root = createRoot([ + createParagraph([createTextSpan(first)]), + createParagraph([createTextSpan(second)]), + ]); + expect(resolveTextNodePosition(root, 1)).toStrictEqual({ + node: second, + offset: 0, + }); + }); + + test("should resolve an editor element to the first text node of its root", () => { + const textNode = new Text("Hello"); + const root = createRoot([createParagraph([createTextSpan(textNode)])]); + const editor = document.createElement("div"); + editor.dataset.itype = "editor"; + editor.appendChild(root); + expect(resolveTextNodePosition(editor, 0)).toStrictEqual({ + node: textNode, + offset: 0, + }); + }); + + test("should return null instead of throwing when the position cannot be resolved", () => { + expect(resolveTextNodePosition(null, 0)).toBe(null); + expect(resolveTextNodePosition(undefined, 0)).toBe(null); + expect(resolveTextNodePosition(document.createElement("div"), 0)).toBe( + null, + ); + expect(resolveTextNodePosition(createParagraph([]), 0)).toBe(null); + }); + }); }); diff --git a/frontend/text-editor/src/editor/controllers/SelectionController.js b/frontend/text-editor/src/editor/controllers/SelectionController.js index 371d94e99f..4ba0b4655a 100644 --- a/frontend/text-editor/src/editor/controllers/SelectionController.js +++ b/frontend/text-editor/src/editor/controllers/SelectionController.js @@ -46,6 +46,7 @@ import { getTextNodeLength, getClosestTextNode, isTextNode, + resolveTextNodePosition, } from "../content/dom/TextNode.js"; import TextNodeIterator from "../content/dom/TextNodeIterator.js"; import TextEditor from "../TextEditor.js"; @@ -537,6 +538,14 @@ export class SelectionController extends EventTarget { */ selectAll() { if (this.#textEditor.isEmpty) { + // There is nothing to select, but we still need a valid caret: leaving + // the selection untouched keeps `focusNode` null and makes any later + // insertion (typing, pasting) fail. + const lineBreak = + this.#textEditor.root?.firstElementChild?.firstElementChild?.firstChild; + if (lineBreak) { + this.collapse(lineBreak, 0); + } return this; } @@ -1132,6 +1141,10 @@ export class SelectionController extends EventTarget { * @param {DocumentFragment} fragment */ insertPaste(fragment) { + if (this.isCollapsed && !this.#normalizeFocus()) { + return; + } + const hasOnlyOneParagraph = fragment.children.length === 1; const forceTextSpan = fragment.firstElementChild?.dataset?.textSpan === "force"; @@ -1395,6 +1408,33 @@ export class SelectionController extends EventTarget { return this.collapse(this.focusNode, this.focusOffset + newText.length); } + /** + * Moves the caret to an equivalent position on a text node or a line break. + * + * The browser can report the caret on a container element (with the offset + * being a child index) or, when the editor was focused without any content, + * on nothing at all. Both states break every insertion path, which expects + * the focus node to be a text node or a
. + * + * @returns {boolean} true when the focus is usable. + */ + #normalizeFocus() { + if (this.isTextFocus || this.isLineBreakFocus) { + return true; + } + + const position = + resolveTextNodePosition(this.focusNode, this.focusOffset) ?? + resolveTextNodePosition(this.#textEditor.root, 0); + + if (!position?.node?.isConnected) { + return false; + } + + this.collapse(position.node, position.offset); + return true; + } + /** * Replaces the currently focus element * with some text. @@ -1402,6 +1442,10 @@ export class SelectionController extends EventTarget { * @param {string} newText */ insertIntoFocus(newText) { + if (!this.#normalizeFocus()) { + return; + } + if (this.isTextFocus) { this.focusNode.nodeValue = insertInto( this.focusNode.nodeValue, diff --git a/frontend/text-editor/src/editor/controllers/SelectionController.test.js b/frontend/text-editor/src/editor/controllers/SelectionController.test.js index 533e4c751c..662119ed29 100644 --- a/frontend/text-editor/src/editor/controllers/SelectionController.test.js +++ b/frontend/text-editor/src/editor/controllers/SelectionController.test.js @@ -1706,6 +1706,96 @@ describe("SelectionController", () => { ); }); + test("`selectAll` should collapse the caret on the line break when the editor is empty", () => { + const textEditorMock = TextEditorMock.createTextEditorMockWithText(""); + const root = textEditorMock.root; + const selection = document.getSelection(); + const selectionController = new SelectionController( + textEditorMock, + selection, + ); + textEditorMock.element.focus(); + selectionController.selectAll(); + expect(selectionController.focusNode).toBe( + root.firstChild.firstChild.firstChild, + ); + expect(selectionController.isCollapsed).toBe(true); + }); + + test("`insertIntoFocus` should insert text when the focus node is a paragraph", () => { + const textEditorMock = + TextEditorMock.createTextEditorMockWithText("Hello, "); + const root = textEditorMock.root; + const paragraph = root.firstChild; + const selection = document.getSelection(); + const selectionController = new SelectionController( + textEditorMock, + selection, + ); + focus(selection, textEditorMock, paragraph, 1); + selectionController.insertIntoFocus("World!"); + expect(root.textContent).toBe("Hello, World!"); + }); + + test("`insertIntoFocus` should insert text when the focus node is the root", () => { + const textEditorMock = + TextEditorMock.createTextEditorMockWithText("Hello, "); + const root = textEditorMock.root; + const selection = document.getSelection(); + const selectionController = new SelectionController( + textEditorMock, + selection, + ); + focus(selection, textEditorMock, root, 1); + selectionController.insertIntoFocus("World!"); + expect(root.textContent).toBe("Hello, World!"); + }); + + test("`insertIntoFocus` should insert text when the focus node is the editor element", () => { + const textEditorMock = + TextEditorMock.createTextEditorMockWithText("Hello, "); + const root = textEditorMock.root; + const selection = document.getSelection(); + const selectionController = new SelectionController( + textEditorMock, + selection, + ); + focus(selection, textEditorMock, textEditorMock.element, 0); + selectionController.insertIntoFocus("World!"); + expect(root.textContent).toBe("World!Hello, "); + }); + + test("`insertIntoFocus` should insert text when there is no known focus node", () => { + const textEditorMock = TextEditorMock.createTextEditorMockWithText(""); + const root = textEditorMock.root; + const selection = document.getSelection(); + const selectionController = new SelectionController( + textEditorMock, + selection, + ); + textEditorMock.element.focus(); + expect(selectionController.focusNode).toBe(null); + selectionController.insertIntoFocus("Hello, World!"); + expect(root.textContent).toBe("Hello, World!"); + }); + + test("`insertPaste` should insert a fragment when the focus node is a paragraph", () => { + const textEditorMock = + TextEditorMock.createTextEditorMockWithText(", World!"); + const root = textEditorMock.root; + const paragraph = root.firstChild; + const selection = document.getSelection(); + const selectionController = new SelectionController( + textEditorMock, + selection, + ); + focus(selection, textEditorMock, paragraph, 0); + const fragment = document.createDocumentFragment(); + fragment.append(createParagraphWith(["Hello"])); + selectionController.insertPaste(fragment); + expect(root.textContent).toBe("Hello, World!"); + }); + test("`cursorToEnd` should move cursor to the end", () => { const textEditorMock = TextEditorMock.createTextEditorMockWithParagraphs([ createParagraphWith(["Hello, "], { diff --git a/frontend/text-editor/src/test/TextEditorMock.js b/frontend/text-editor/src/test/TextEditorMock.js index 0e20d209e7..457d184eb1 100644 --- a/frontend/text-editor/src/test/TextEditorMock.js +++ b/frontend/text-editor/src/test/TextEditorMock.js @@ -4,7 +4,10 @@ import { createEmptyTextSpan, createTextSpan, } from "../editor/content/dom/TextSpan.js"; -import { createLineBreak } from "../editor/content/dom/LineBreak.js"; +import { + createLineBreak, + isLineBreak, +} from "../editor/content/dom/LineBreak.js"; export class TextEditorMock extends EventTarget { /** @@ -135,6 +138,7 @@ export class TextEditorMock extends EventTarget { this.#element = element; this.#root = options?.root; this.#selectionImposterElement = options?.selectionImposterElement; + this.#element.dataset.itype = "editor"; this.#element.appendChild(options?.root); } @@ -145,6 +149,14 @@ export class TextEditorMock extends EventTarget { get root() { return this.#root; } + + get isEmpty() { + return ( + this.#root.children.length === 1 && + this.#root.firstElementChild.children.length === 1 && + isLineBreak(this.#root.firstElementChild.firstElementChild.firstChild) + ); + } } export default TextEditorMock;