diff --git a/.serena/memories/backend/auth-permissions-product-domains.md b/.serena/memories/backend/auth-permissions-product-domains.md index 2a58e49dfd..d9ee64c171 100644 --- a/.serena/memories/backend/auth-permissions-product-domains.md +++ b/.serena/memories/backend/auth-permissions-product-domains.md @@ -13,6 +13,9 @@ - Logout may return an OIDC provider redirect URI when the session claims include provider/session data and the provider has a logout URI. - Invitation tokens are verified through token issuers and only accepted when the token member id/email matches the authenticated profile; otherwise login proceeds without consuming the invitation. - When public registration is disabled, invitation-based password registration also requires a matching, non-expired `team_invitation` row; registration revalidates it under a row lock before creating the profile. +- Access tokens (`app.http.access-token`): personal tokens have JWT issuer `access-token` and DB type other than `mcp`; MCP tokens have issuer `urn:penpot:mcp-token` (`actoken/mcp-token-issuer`) and DB type `mcp`. Authentication requires issuer and type to agree, and the claim `uid` to match the row's profile; other issuers and undecryptable tokens are treated as anonymous. +- Personal tokens need flag `:access-tokens`. MCP tokens need `:mcp` or `:access-tokens`, and only on routes whose `actoken/authz` gets `::actoken/allow-mcp true` (the RPC routes); elsewhere (e.g. assets) they are anonymous. +- On RPC, an MCP token reaches only commands with `::rpc/token-type "mcp"` (`wrap-authentication` in `app.rpc`); every other command answers 403 `:token-auth-required`. A command with that metadata also rejects sessions and personal tokens. Currently only `send-mcp-feedback`. Blocking a profile invalidates sessions, not tokens, so such commands check `:is-blocked`/`:is-active` themselves. - HTTP/session parsing details such as cookie/header precedence, JWT session token versions, and SameSite behavior are in `mem:backend/subtleties`. ## Permission model diff --git a/.serena/memories/backend/core.md b/.serena/memories/backend/core.md index 2d44a88b76..7a70eb4669 100644 --- a/.serena/memories/backend/core.md +++ b/.serena/memories/backend/core.md @@ -32,7 +32,7 @@ Backend: JVM Clojure; Integrant; PostgreSQL; Redis/Valkey; RPC; HTTP; storage; m RPC commands are defined with `app.util.services/defmethod` and schemas. Use `get-` prefixes for read operations. Command metadata usually includes auth, docs version, params schema, and result schema. Return plain maps/vectors or raise structured exceptions from `app.common.exceptions`. -Backend RPC command areas without focused memories include access tokens, binfile, demo, feedback, file snapshots, fonts, management, Nitrate, and webhooks beyond the notes in `mem:backend/auth-permissions-product-domains`; inspect nearby command tests and command metadata before changing them. +Backend RPC command areas without focused memories include access tokens (auth rules, incl. MCP tokens, in `mem:backend/auth-permissions-product-domains`), binfile, demo, feedback, file snapshots, fonts, management, Nitrate, and webhooks beyond the notes in `mem:backend/auth-permissions-product-domains`; inspect nearby command tests and command metadata before changing them. ## DB conventions diff --git a/.serena/memories/mcp/core.md b/.serena/memories/mcp/core.md index 551ef961c6..dbc4b85b92 100644 --- a/.serena/memories/mcp/core.md +++ b/.serena/memories/mcp/core.md @@ -113,6 +113,15 @@ From the `mcp/` directory, run - Authorization/discovery URLs, self-contained client IDs, consent and PKCE/code binding, backend Redis state, deployment wiring, and compatibility limits: `mem:mcp/oauth`. +## Backend tools (`submit_feedback`) + +- Tools that call the Penpot backend directly (not through the plugin) register only when `PENPOT_MCP_BACKEND_URI` is nonblank (`PenpotMcpServer.initTools`). Currently only `SubmitFeedbackTool`. +- `PenpotBackendClient` owns the HTTP call and the status-to-message mapping; it posts to `api/main/methods/send-mcp-feedback` with `Authorization: Token `. The token comes from the session context (OAuth bearer or `?userToken=`); without one the tool fails before calling the backend. +- Tool zod schemas mirror the backend malli schema (lengths, blank, control characters); keep both in sync. The backend is the authority. +- Success means the mail job was queued, not delivered; the tool text says so. `formatArgs` hides the feedback from logs. +- Backend side: `send-mcp-feedback` in `app.rpc.commands.feedback`, configured by `PENPOT_MCP_FEEDBACK_EMAIL`; MCP-token auth rules in `mem:backend/auth-permissions-product-domains`. +- User docs: `mcp/README.md` (env tables), `docs/mcp/index.md` (tool list), `docs/technical-guide/configuration.md` (MCP feedback section). + ## Devenv plugin/server wiring In the normal Penpot devenv MCP path, the browser plugin does not discover or route through Postgres. The frontend provides the plugin extension API with `mcp.getServerUrl()`, currently derived from `frontend/src/app/config.cljs` as `penpotMcpServerURI` if set, otherwise `/mcp/ws`. The MCP plugin opens a direct WebSocket to that URL and appends the current MCP access token as a query parameter. diff --git a/.serena/memories/mcp/oauth.md b/.serena/memories/mcp/oauth.md index 097bdc1186..110372cb4c 100644 --- a/.serena/memories/mcp/oauth.md +++ b/.serena/memories/mcp/oauth.md @@ -3,7 +3,7 @@ ## Contract and scope - MCP-specific authorization; existing Penpot login identifies the user on the approval page. No application vetting or persistent consent policy. Access requires user approval; registration alone grants nothing. -- Return the existing fixed MCP user token. On approval, enable `:mcp-enabled` and create the token only if no unexpired one exists; preserve an existing unexpired token. Settings can give MCP tokens an expiry, and an expired one is replaced. No refresh token, `expires_in`, or new token lifetime. +- Return the existing fixed MCP user token. On approval, enable `:mcp-enabled` and create the token only if no usable one exists; preserve an existing usable token. A token is unusable, and replaced, when expired (settings can give MCP tokens an expiry), when its JWT issuer is not `urn:penpot:mcp-token` (tokens issued before MCP tokens had their own issuer), or when this instance cannot decrypt it (e.g. secret key changed); the backend rejects all three. No refresh token, `expires_in`, or new token lifetime. - No new client database table or schema migration. Client IDs must remain usable across separate authorization flows without server-side registration storage. Do not replace them with expiring Redis registrations. - MCP transport continues matching user tokens to connected Penpot plugins; no new per-request token introspection or independent semantic validation. The logged-in Penpot instance remains the content-access boundary. Do not silently add resource-server validation or change token semantics. - README scope: user/developer usage instructions; document `PENPOT_MCP_AUTH_URI` in its environment-variable table. Keep OAuth architecture and protocol details here and non-obvious invariants in source comments. @@ -38,7 +38,7 @@ - `/decision`: check exact Origin, session binding and CSRF, then atomically consume the request. Denial creates no token/enables nothing. Approval calls transactional `access-token/ensure-mcp-access!`, then stores code for 120 seconds. Code state holds token row ID, never the fixed token. Callback echoes OAuth `state` and RFC 9207 `iss`. - `/token`: validate grant, exact client ID/callback/resource and PKCE; resolve current client metadata/authentication method and verify any secret; atomically consume validated code; fetch the unexpired token by original row ID/profile/type. Regeneration deletes the old token row, so a pending code cannot silently yield the replacement token. Return fixed `access_token`, `token_type: Bearer`, scope `mcp`. - `backend/src/app/mcp/oauth_state.clj` and Lua: tenant/kind/hash-prefixed keys; random handles; SET NX EX must succeed before returning handle. Reads retain raw JSON for atomic comparison. Consume/delete and bind/replace compare the validated raw record in Lua; replacement preserves TTL. Redis failures fail closed. Backend Redis is required independently of Node's optional Redis plugin bridge. -- `backend/src/app/rpc/commands/access_token.clj`: lock profile row in both MCP generation and OAuth provisioning, including when no token exists. Serializes simultaneous approvals and settings regeneration. Provisioning must run within a transaction. +- `backend/src/app/rpc/commands/access_token.clj`: `ensure-mcp-access!` decodes the stored token with `ex/ignoring` to apply the usable-token rule above. Lock profile row in both MCP generation and OAuth provisioning, including when no token exists. Serializes simultaneous approvals and settings regeneration. Provisioning must run within a transaction. ## Verification and limits diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index 33fde1fd5f..515f3bec49 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -206,6 +206,7 @@ [:allow-demo-users {:optional true} ::sm/boolean] [:error-report-webhook {:optional true} :string] [:user-feedback-destination {:optional true} :string] + [:mcp-feedback-email {:optional true} ::sm/email] [:default-rpc-rlimit {:optional true} [::sm/vec :string]] [:rpc-rlimit-config {:optional true} ::fs/path] diff --git a/backend/src/app/http/access_token.clj b/backend/src/app/http/access_token.clj index 57f19fdda8..017b042bd4 100644 --- a/backend/src/app/http/access_token.clj +++ b/backend/src/app/http/access_token.clj @@ -14,14 +14,20 @@ [app.setup :as-alias setup] [app.tokens :as tokens])) +(def mcp-token-issuer "urn:penpot:mcp-token") + +(def ^:private valid-issuers + #{"access-token" mcp-token-issuer}) + (defn decode-token [cfg token] (try - (tokens/verify cfg {:token token :iss "access-token"}) - (catch Throwable cause - (l/trc :hint "exception on decoding token" - :token token - :cause cause)))) + (let [claims (tokens/verify cfg {:token token})] + (when (contains? valid-issuers (:iss claims)) + claims)) + ;; The cause is not logged because its data carries the token + (catch Throwable _ + (l/trc :hint "exception on decoding token")))) (def sql:get-token-data "SELECT perms, profile_id, expires_at, type @@ -34,32 +40,48 @@ [pool claims] (when-not (db/read-only? pool) (when-let [token-id (get claims :tid)] - (some-> (db/exec-one! pool [sql:get-token-data token-id]) - (update :perms db/decode-pgarray #{}))))) + (let [token (db/exec-one! pool [sql:get-token-data token-id]) + mcp-claims? (= mcp-token-issuer (:iss claims)) + mcp-token? (= "mcp" (:type token))] + (when (and (= (:uid claims) (:profile-id token)) + (= mcp-claims? mcp-token?)) + (update token :perms db/decode-pgarray #{})))))) (defn- wrap-authz - [handler {:keys [::db/pool]}] - (fn [request] - (let [{:keys [type claims]} (get request ::http/auth-data)] - (if (= :token type) - (let [{:keys [perms profile-id expires-at type]} (some->> claims (get-token-data pool)) - token-id (get claims :tid)] - (handler (cond-> request - (some? perms) - (assoc ::perms perms) - (some? profile-id) - (assoc ::profile-id profile-id) - (some? expires-at) - (assoc ::expires-at expires-at) - (some? token-id) - (assoc ::id token-id) - (some? type) - (assoc ::type type)))) + "Authenticate requests carrying an access token. Personal tokens need the + `access-tokens` flag. MCP tokens are restricted to specific commands, so + only RPC routes opt in (the RPC wrapper enforces those restrictions); they + need either the `mcp` or the `access-tokens` flag." + [handler {:keys [::db/pool ::allow-mcp]}] + (let [personal-enabled? (contains? cf/flags :access-tokens) + mcp-enabled? (and allow-mcp + (or personal-enabled? + (contains? cf/flags :mcp)))] + (fn [request] + (let [{:keys [type claims]} (get request ::http/auth-data)] + (if (and (= :token type) + (if (= mcp-token-issuer (:iss claims)) + mcp-enabled? + personal-enabled?)) + (let [{:keys [perms profile-id expires-at type]} (some->> claims (get-token-data pool)) + token-id (get claims :tid)] + (handler (cond-> request + (some? perms) + (assoc ::perms perms) + (some? profile-id) + (assoc ::profile-id profile-id) + (some? expires-at) + (assoc ::expires-at expires-at) + (some? token-id) + (assoc ::id token-id) + (some? type) + (assoc ::type type)))) - (handler request))))) + (handler request)))))) (def authz {:name ::authz :compile (fn [& _] - (when (contains? cf/flags :access-tokens) + (when (or (contains? cf/flags :access-tokens) + (contains? cf/flags :mcp)) wrap-authz))}) diff --git a/backend/src/app/rpc.clj b/backend/src/app/rpc.clj index 7ca7816cfc..8d922822c7 100644 --- a/backend/src/app/rpc.clj +++ b/backend/src/app/rpc.clj @@ -124,7 +124,8 @@ (cond-> (and (not (uuid? session-pid)) (uuid? token-pid)) (-> (assoc ::auth-type :token) - (assoc ::token-perms (set (::actoken/perms request #{}))))) + (assoc ::token-perms (set (::actoken/perms request #{}))) + (assoc ::token-type (::actoken/type request)))) (cond-> key-id (assoc ::auth-key-id key-id)) ;; Server-side only: request bodies cannot forge @@ -171,10 +172,12 @@ [_ f mdata] (let [required-auth? (::auth mdata true) required-auth-type (::auth-type mdata) + required-token-type (::token-type mdata) required-perms (into #{} (::perms mdata))] (fn [cfg params] (let [profile-id (::profile-id params) auth-type (::auth-type params) + token-type (::token-type params) token-perms (set (::token-perms params #{}))] (cond (and required-auth? (not (uuid? profile-id))) @@ -182,8 +185,13 @@ :code :authentication-required :hint "authentication required for this endpoint") - (and (= required-auth-type :token) - (not= auth-type :token)) + (or (and (= required-auth-type :token) + (not= auth-type :token)) + (and required-token-type + (or (not= auth-type :token) + (not= required-token-type token-type))) + (and (= token-type "mcp") + (not= required-token-type "mcp"))) (ex/raise :type :authorization :code :token-auth-required :hint "access token authentication required for this endpoint") @@ -522,7 +530,7 @@ {:middleware [[mw/cors] [sec/client-header-check] [session/authz cfg] - [actoken/authz cfg]] + [actoken/authz (assoc cfg ::actoken/allow-mcp true)]] :handler (make-rpc-handler methods)}] (doc/routes :methods methods @@ -540,5 +548,5 @@ {:middleware [[mw/cors] [sec/client-header-check] [session/authz cfg] - [actoken/authz cfg]] + [actoken/authz (assoc cfg ::actoken/allow-mcp true)]] :handler (make-rpc-handler methods)}]]) diff --git a/backend/src/app/rpc/commands/access_token.clj b/backend/src/app/rpc/commands/access_token.clj index 6681af3cd7..8474d2ba80 100644 --- a/backend/src/app/rpc/commands/access_token.clj +++ b/backend/src/app/rpc/commands/access_token.clj @@ -6,10 +6,12 @@ (ns app.rpc.commands.access-token (:require + [app.common.exceptions :as ex] [app.common.schema :as sm] [app.common.time :as ct] [app.common.uuid :as uuid] [app.db :as db] + [app.http.access-token :as actoken] [app.main :as-alias main] [app.rpc :as-alias rpc] [app.rpc.commands.profile :as profile] @@ -49,7 +51,7 @@ (let [token-id (uuid/next) expires-at (some-> expiration (ct/in-future)) created-at (ct/now) - token-iss (if (= type "mcp") "urn:penpot:mcp-token" "access-token") + token-iss (if (= type "mcp") actoken/mcp-token-issuer "access-token") token (tokens/generate cfg {:iss token-iss :uid profile-id :iat created-at @@ -79,10 +81,16 @@ Must run in a transaction. Lock the profile even when no token exists, so concurrent approvals and settings regeneration use the same lock. Enabling MCP must not regenerate a token already held by a connected client; an - expired token is replaced because no client can use it." + expired token is replaced because no client can use it, and so is a token + issued before MCP tokens had their own issuer, or one this instance cannot + decrypt, because the backend rejects both." [{:keys [::db/conn] :as cfg} profile-id] (let [profile (profile/get-profile conn profile-id ::db/for-update true) - token (db/exec-one! conn [sql:get-valid-mcp-token profile-id])] + token (db/exec-one! conn [sql:get-valid-mcp-token profile-id]) + ;; A token this instance cannot decrypt is as unusable as an expired one + token (when (= actoken/mcp-token-issuer + (ex/ignoring (:iss (tokens/decode cfg (:token token))))) + token)] (when-not (:mcp-enabled (:props profile)) (profile/update-profile-props cfg profile-id {:mcp-enabled true})) (if token diff --git a/backend/src/app/rpc/commands/feedback.clj b/backend/src/app/rpc/commands/feedback.clj index 7ff4076da7..69354345a1 100644 --- a/backend/src/app/rpc/commands/feedback.clj +++ b/backend/src/app/rpc/commands/feedback.clj @@ -13,11 +13,57 @@ [app.config :as cf] [app.db :as db] [app.email :as eml] + [app.loggers.audit :as-alias audit] [app.rpc :as-alias rpc] [app.rpc.climit :as-alias climit] [app.rpc.commands.profile :as profile] [app.rpc.doc :as-alias doc] - [app.util.services :as sv])) + [app.rpc.helpers :as rph] + [app.util.services :as sv] + [clojure.string :as str])) + +(def schema:send-mcp-feedback + [:map {:title "send-mcp-feedback" :closed true} + [:subject [:and [:string {:min 1 :max 120}] + [:fn #(and (not (str/blank? %)) + (not (re-find #"\p{Cc}" %)))]]] + ;; Line breaks and tabs are the only control characters allowed + [:feedback [:and [:string {:min 1 :max 5000}] + [:fn #(and (not (str/blank? %)) + (not (re-find #"[\p{Cc}&&[^\t\n\r]]" %)))]]]]) + +(sv/defmethod ::send-mcp-feedback + {::rpc/token-type "mcp" + ::db/transaction true + ::doc/added "2.20" + ::sm/params schema:send-mcp-feedback} + [cfg {:keys [::rpc/profile-id subject feedback]}] + (let [profile (profile/get-profile cfg profile-id) + destination (cf/get :mcp-feedback-email)] + ;; Blocking a profile invalidates its sessions but not its access tokens + (when (:is-blocked profile) + (ex/raise :type :authorization + :code :profile-blocked + :hint "profile is marked as blocked")) + (when-not (:is-active profile) + (ex/raise :type :authorization + :code :profile-not-active + :hint "profile is not active")) + (when (str/blank? destination) + (ex/raise :type :restriction + :code :feedback-disabled + :hint "MCP feedback is not configured")) + (eml/send cfg {::eml/reuse-conn true + :to destination + :reply-to (:email profile) + :subject (str "[MCP feedback] " subject) + :body {"text/plain" (str "Submitted by: " (:fullname profile) + " <" (:email profile) ">\n" + "Profile ID: " (:id profile) "\n\n" + "Feedback:\n\n" feedback)}}) + ;; The audit event keeps who sent feedback, not its text + (rph/with-meta {:status "queued"} + {::audit/replace-props {}}))) (declare ^:private send-user-feedback!) diff --git a/backend/test/backend_tests/http_assets_test.clj b/backend/test/backend_tests/http_assets_test.clj index 1f6c473a51..c9503f1cca 100644 --- a/backend/test/backend_tests/http_assets_test.clj +++ b/backend/test/backend_tests/http_assets_test.clj @@ -31,7 +31,9 @@ io.prometheus.client.Counter io.prometheus.client.Counter$Child)) -(t/use-fixtures :once th/state-init) +(t/use-fixtures :once (t/compose-fixtures + (partial th/init-config [:enable-access-tokens]) + th/init-system)) (t/use-fixtures :each (th/serial th/database-reset th/clean-storage)) @@ -464,7 +466,7 @@ ;; 2. actoken/authz reads ::http/auth-data and sets ::actoken/profile-id request {::http/auth-data {:type :token :token (:token atoken) - :claims {:tid (:id atoken)}} + :claims (actoken/decode-token th/*system* (:token atoken))} :path-params {:id (str (:id object))}} ;; Apply actoken/authz middleware @@ -517,7 +519,7 @@ (t/testing "access token auth sets ::actoken/profile-id" (let [request {::http/auth-data {:type :token :token (:token atoken) - :claims {:tid (:id atoken)}}} + :claims (actoken/decode-token th/*system* (:token atoken))}} result (with-session request)] (t/is (= (:id profile) (::actoken/profile-id result))))) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index d2c28857e9..57766ab3b7 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -34,7 +34,9 @@ io.prometheus.client.CollectorRegistry io.undertow.server.RequestTooBigException)) -(t/use-fixtures :once th/state-init) +(t/use-fixtures :once (t/compose-fixtures + (partial th/init-config [:enable-access-tokens]) + th/init-system)) (t/use-fixtures :each th/database-reset) (t/deftest auth-middleware-1 @@ -106,7 +108,8 @@ (let [response (handler nil)] (t/is (nil? response))) - (let [response (handler {::http/auth-data {:type :token :token "foobar" :claims {:tid (:id token)}}})] + (let [response (handler {::http/auth-data {:type :token :token (:token token) + :claims (tokens/verify th/*system* {:token (:token token)})}})] (t/is (= #{} (:app.http.access-token/perms response))) (t/is (= (:id profile) (:app.http.access-token/profile-id response)))))) @@ -114,9 +117,14 @@ (let [profile (th/create-profile* 1) token (db/tx-run! th/*system* app.rpc.commands.access-token/create-access-token (:id profile) "test" nil "mcp") - handler (#'app.http.access-token/wrap-authz identity th/*system*) - request {::http/auth-data {:type :token :token "foobar" :claims {:tid (:id token)}}} + handler (#'app.http.access-token/wrap-authz + identity (assoc th/*system* :app.http.access-token/allow-mcp true)) + request {::http/auth-data {:type :token :token (:token token) + :claims (tokens/verify th/*system* {:token (:token token)})}} response (handler request)] + (t/is (nil? (:app.http.access-token/profile-id + ((#'app.http.access-token/wrap-authz identity th/*system*) request))) + "MCP tokens do not authenticate routes that have not opted in") ;; Must set ::actoken/id from claims :tid (t/is (= (:id token) (:app.http.access-token/id response))) ;; Must set ::actoken/type from database @@ -125,6 +133,30 @@ (t/is (= #{} (:app.http.access-token/perms response))) (t/is (= (:id profile) (:app.http.access-token/profile-id response))))) +(t/deftest access-token-authz-follows-flags + (let [profile (th/create-profile* 1) + create #(db/tx-run! th/*system* app.rpc.commands.access-token/create-access-token + (:id profile) "test" nil %) + request (fn [{:keys [token]}] + {::http/auth-data {:type :token :token token + :claims (tokens/verify th/*system* {:token token})}}) + auth (fn [flags token] + (with-redefs [cf/flags flags] + (let [handler (#'app.http.access-token/wrap-authz + identity (assoc th/*system* :app.http.access-token/allow-mcp true))] + (:app.http.access-token/profile-id (handler (request token)))))) + personal (create nil) + mcp (create "mcp")] + (t/testing "only mcp: MCP tokens authenticate, personal tokens do not" + (t/is (= (:id profile) (auth #{:mcp} mcp))) + (t/is (nil? (auth #{:mcp} personal)))) + (t/testing "only access-tokens: both authenticate" + (t/is (= (:id profile) (auth #{:access-tokens} mcp))) + (t/is (= (:id profile) (auth #{:access-tokens} personal)))) + (t/testing "neither flag: the middleware is not installed" + (with-redefs [cf/flags #{}] + (t/is (nil? ((:compile app.http.access-token/authz)))))))) + (defrecord MethodAwareDummyRequest [req-method headers] yreq/IRequest (method [_] req-method) diff --git a/backend/test/backend_tests/rpc_access_tokens_test.clj b/backend/test/backend_tests/rpc_access_tokens_test.clj index e5fcaf5348..359bbf66fd 100644 --- a/backend/test/backend_tests/rpc_access_tokens_test.clj +++ b/backend/test/backend_tests/rpc_access_tokens_test.clj @@ -6,6 +6,7 @@ (ns backend-tests.rpc-access-tokens-test (:require + [app.common.time :as ct] [app.common.uuid :as uuid] [app.db :as db] [app.http :as http] @@ -13,6 +14,7 @@ [app.rpc :as-alias rpc] [app.rpc.commands.access-token :as access-token] [app.storage :as sto] + [app.tokens :as tokens] [backend-tests.helpers :as th] [clojure.test :as t] [mockery.core :refer [with-mocks]])) @@ -208,7 +210,7 @@ (t/is (not (contains? all-ids (:id second-mcp)))) (t/is (contains? all-ids (:id third-mcp)))))))) -(t/deftest mcp-tokens-cannot-be-used-as-access-tokens +(t/deftest access-token-decoder-accepts-mcp-and-personal-issuers (let [prof (th/create-profile* 1 {:is-active true}) cfg th/*system*] @@ -219,9 +221,9 @@ :name "mcp token"}) mcp-token (:token result) - ;; Try to decode as access token (should fail) decoded (actoken/decode-token cfg mcp-token)] - (t/is (nil? decoded)))) + (t/is (= "urn:penpot:mcp-token" (:iss decoded))) + (t/is (= (:id prof) (:uid decoded))))) (t/testing "Regular access tokens use access-token issuer claim" (let [{:keys [result]} (th/command! {::th/type :create-access-token @@ -264,6 +266,37 @@ (t/is (nil? (:expires-at token))) (t/is (= [(:id token)] (map :id (db/query th/*pool* :access-token {:profile-id (:id prof) :type "mcp"})))))) + (t/testing "approval replaces a token issued with the personal-token issuer" + (db/delete! th/*pool* :access-token {:profile-id (:id prof)}) + (let [token-id (uuid/next) + now (ct/now) + old (tokens/generate cfg {:iss "access-token" :uid (:id prof) + :iat now :tid token-id})] + (db/tx-run! cfg (fn [{:keys [::db/conn]}] + (db/insert! conn :access-token + {:id token-id :name "MCP" :token old :type "mcp" + :profile-id (:id prof) :created-at now :updated-at now + :perms (db/create-array conn "text" [])}))) + (let [token (provision)] + (t/is (not= old (:token token))) + (t/is (= actoken/mcp-token-issuer + (:iss (actoken/decode-token cfg (:token token))))) + (t/is (= [(:id token)] + (map :id (db/query th/*pool* :access-token + {:profile-id (:id prof) :type "mcp"}))))))) + (t/testing "approval replaces a token this instance cannot decrypt" + (db/delete! th/*pool* :access-token {:profile-id (:id prof)}) + (let [now (ct/now)] + (db/tx-run! cfg (fn [{:keys [::db/conn]}] + (db/insert! conn :access-token + {:id (uuid/next) :name "MCP" :token "not-a-jwe" :type "mcp" + :profile-id (:id prof) :created-at now :updated-at now + :perms (db/create-array conn "text" [])}))) + (let [token (provision)] + (t/is (not= "not-a-jwe" (:token token))) + (t/is (= [(:id token)] + (map :id (db/query th/*pool* :access-token + {:profile-id (:id prof) :type "mcp"}))))))) (t/testing "concurrent approvals share one token" (db/delete! th/*pool* :access-token {:profile-id (:id prof)}) (let [results (mapv deref (repeatedly 4 #(future (provision))))] diff --git a/backend/test/backend_tests/rpc_commands_mcp_feedback_test.clj b/backend/test/backend_tests/rpc_commands_mcp_feedback_test.clj new file mode 100644 index 0000000000..7987596481 --- /dev/null +++ b/backend/test/backend_tests/rpc_commands_mcp_feedback_test.clj @@ -0,0 +1,179 @@ +;; 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 SUBSIDIARY SL + +(ns backend-tests.rpc-commands-mcp-feedback-test + (:require + [app.common.json :as json] + [app.common.time :as ct] + [app.common.uuid :as uuid] + [app.config :as cf] + [app.db :as db] + [app.email :as email] + [app.http :as http] + [app.jobs :as jobs] + [app.rpc :as-alias rpc] + [app.rpc.commands.access-token :as access-token] + [app.tokens :as tokens] + [backend-tests.helpers :as th] + [clojure.test :as t] + [yetti.response :as-alias yres])) + +(t/use-fixtures :once (t/compose-fixtures + (partial th/init-config [:enable-access-tokens + :enable-client-header-check-middleware]) + th/init-system)) +(t/use-fixtures :each th/database-reset) + +(def ^:private feedback + {:subject "Export fails" + :feedback "Exporting a valid shape repeatedly fails. Expected a PNG."}) + +(defn- create-identity + [] + (let [profile (th/create-profile* (uuid/next) {:is-active true}) + token (db/tx-run! th/*system* access-token/create-access-token + (:id profile) "MCP" nil "mcp")] + {:profile profile :token token})) + +(defn- request + ([token] (request token feedback)) + ([token params] (request token params {})) + ([token params options] + (with-redefs [cf/config (assoc cf/config :mcp-feedback-email "mcp@example.com")] + ((#'http/router-handler (::http/router th/*system*)) + (th/make-dummy-request + (merge {:path "/api/main/methods/send-mcp-feedback" + :method :post + :headers (cond-> {"content-type" "application/json" + "accept" "application/json" + "x-client" "penpot-mcp"} + token (assoc "authorization" (str "Token " token))) + :body-bytes (json/encode params)} + options)))))) + +(defn- emails + [] + (map #(jobs/decode-params (jobs/get-job-def (::jobs/defs th/*system*) :sendmail) (:params %)) + (db/query th/*system* :job {:name "sendmail"}))) + +(t/deftest queues-plain-text-feedback + (let [{:keys [token profile]} (create-identity) + response (request (:token token)) + email (first (emails))] + (t/is (= 200 (::yres/status response))) + (t/is (= 1 (count (emails)))) + (t/is (= "mcp@example.com" (:to email))) + (t/is (= (:email profile) (:reply-to email))) + (t/is (= "[MCP feedback] Export fails" (:subject email))) + (t/is (= {"text/plain" (str "Submitted by: " (:fullname profile) + " <" (:email profile) ">\n" + "Profile ID: " (:id profile) "\n\n" + "Feedback:\n\n" (:feedback feedback))} + (:body email))) + (t/is (nil? (:attachments email))))) + +(t/deftest rejects-invalid-credentials + (let [{:keys [profile token]} (create-identity)] + (doseq [credential [nil "" "invalid" + (tokens/generate th/*system* {:iss "other" + :uid (:id profile) :tid (:id token)}) + (tokens/generate th/*system* {:iss "access-token" + :uid (:id profile) :tid (:id token)}) + (tokens/generate th/*system* {:iss "urn:penpot:mcp-token" + :uid (uuid/next) :tid (:id token)}) + (tokens/generate th/*system* {:iss "urn:penpot:mcp-token" + :uid (:id profile) :tid (uuid/next)}) + (tokens/generate th/*system* {:iss "urn:penpot:mcp-token" + :uid (:id profile) :tid (:id token) + :exp (ct/minus (ct/now) (ct/duration "1h"))})]] + (t/is (= 401 (::yres/status (request credential))))) + (t/is (empty? (emails))))) + +(t/deftest rejects-expired-revoked-and-wrong-type-tokens + (let [{:keys [token profile]} (create-identity)] + (db/update! th/*system* :access-token + {:expires-at (ct/minus (ct/now) (ct/duration "1h"))} {:id (:id token)}) + (t/is (= 401 (::yres/status (request (:token token))))) + (db/update! th/*system* :access-token {:expires-at nil :type "personal"} {:id (:id token)}) + (t/is (= 401 (::yres/status (request (:token token))))) + (db/update! th/*system* :access-token {:type "mcp"} {:id (:id token)}) + (db/tx-run! th/*system* access-token/create-access-token (:id profile) "Replacement" nil "mcp") + (t/is (= 401 (::yres/status (request (:token token))))) + (t/is (empty? (emails))))) + +(t/deftest rejects-unavailable-profiles + (doseq [changes [{:is-active false} + {:is-blocked true} + {:deleted-at (ct/minus (ct/now) (ct/duration "1h"))}]] + (let [{:keys [token profile]} (create-identity)] + (db/update! th/*system* :profile changes {:id (:id profile)}) + (t/is (= (if (:deleted-at changes) 404 403) + (::yres/status (request (:token token))))))) + (t/is (empty? (emails)))) + +(t/deftest validates-feedback-before-queueing + (let [{:keys [token]} (create-identity)] + (doseq [params [{} + (assoc feedback :subject "") + (assoc feedback :subject " ") + (assoc feedback :subject "a\r\nBcc: victim@example.com") + (assoc feedback :subject "a\tb") + (assoc feedback :subject "a\u0085b") + (assoc feedback :subject "a\u0000b") + (assoc feedback :feedback "a\u0000b") + (assoc feedback :feedback "a\u001bb") + (assoc feedback :subject (apply str (repeat 121 "x"))) + (assoc feedback :feedback " \n") + (assoc feedback :feedback (apply str (repeat 5001 "x"))) + (assoc feedback :to "victim@example.com") + (assoc feedback :attachments {}) + (assoc feedback :feedback 42)]] + (t/is (= 400 (::yres/status (request (:token token) params))))) + (t/is (empty? (emails))) + (t/is (= 200 (::yres/status + (request (:token token) {:subject (apply str (repeat 120 "x")) + :feedback (apply str (repeat 5000 "x"))})))) + (t/is (= 200 (::yres/status + (request (:token token) {:subject "Tabs and breaks" + :feedback "line one\r\n\tline two\n"})))))) + +(t/deftest audit-event-omits-feedback-text + (let [{:keys [profile]} (create-identity)] + (with-redefs [cf/config (assoc cf/config :mcp-feedback-email "mcp@example.com")] + (let [result (th/command-raw! (merge feedback {::th/type :send-mcp-feedback + ::rpc/profile-id (:id profile) + ::rpc/auth-type :token + ::rpc/token-type "mcp"}))] + (t/is (= {} (:app.loggers.audit/replace-props (meta result)))))))) + +(t/deftest disabled-without-feedback-email + (let [{:keys [token]} (create-identity)] + (with-redefs [cf/config (dissoc cf/config :mcp-feedback-email)] + (let [response ((#'http/router-handler (::http/router th/*system*)) + (th/make-dummy-request + {:path "/api/main/methods/send-mcp-feedback" :method :post + :headers {"authorization" (str "Token " (:token token)) + "content-type" "application/json" + "x-client" "penpot-mcp"} + :body-bytes (json/encode feedback)}))] + (t/is (= 400 (::yres/status response))))) + (t/is (empty? (emails))))) + +(t/deftest feedback-is-post-only + (let [{:keys [token]} (create-identity)] + (t/is (= 405 (::yres/status (request (:token token) feedback {:method :get}))))) + (t/is (empty? (emails)))) + +(t/deftest mcp-token-does-not-authorize-ordinary-rpc + (let [{:keys [token]} (create-identity)] + (t/is (= 403 (::yres/status + (request (:token token) {} {:path "/api/rpc/command/get-teams"})))))) + +(t/deftest does-not-acknowledge-a-failed-queue-submission + (let [{:keys [token]} (create-identity)] + (with-redefs [email/send (fn [& _] (throw (ex-info "Queue unavailable" {})))] + (t/is (= 500 (::yres/status (request (:token token))))))) + (t/is (empty? (emails)))) diff --git a/backend/test/backend_tests/rpc_test.clj b/backend/test/backend_tests/rpc_test.clj index 233ffdd8e2..a6e18d24f3 100644 --- a/backend/test/backend_tests/rpc_test.clj +++ b/backend/test/backend_tests/rpc_test.clj @@ -902,6 +902,27 @@ (t/is (some? ex)) (t/is (th/ex-of-type? ex :not-found)))) +;; --- TOKEN TYPE RESTRICTIONS + +(t/deftest mcp-token-authentication + (let [pid (uuid/custom 1 1) + method (fn [mdata] + (rpc/make-rpc-handler + {:test-method [mdata (partial (#'rpc/wrap-authentication + nil (fn [_ params] params) mdata) + nil)]})) + handler (method {::rpc/auth-type :token ::rpc/token-type "mcp"})] + (doseq [extras [{::session/profile-id pid} + {::actoken/profile-id pid} + {::actoken/profile-id pid ::actoken/type "personal"}]] + (t/is (th/ex-of-code? (call-ex handler {:extras extras}) :token-auth-required))) + (let [extras {::actoken/profile-id pid ::actoken/type "mcp"}] + (t/is (= "mcp" (::rpc/token-type (call-body handler {:extras extras})))) + (t/is (th/ex-of-code? (call-ex (method {}) {:extras extras}) :token-auth-required))) + (t/is (= pid (::rpc/profile-id + (call-body (method {}) {:extras {::actoken/profile-id pid + ::actoken/type "personal"}})))))) + ;; --- PARAMS VALIDATION: SERVER AUTH CONTEXT VS CLIENT PARAMS (t/deftest client-qualified-keys-do-not-override-server-auth-context @@ -915,7 +936,8 @@ request {:params {:name "valid" :app.rpc/profile-id attacker-pid :app.rpc/auth-type :token - :app.rpc/token-perms #{"admin"}}} + :app.rpc/token-perms #{"admin"} + :app.rpc/token-type "mcp"}} params (with-meta {::rpc/profile-id server-pid ::rpc/auth-type :session} {::http/request request})] @@ -923,6 +945,7 @@ (t/is (= server-pid (::rpc/profile-id @captured))) (t/is (= :session (::rpc/auth-type @captured))) (t/is (not (contains? @captured ::rpc/token-perms))) + (t/is (not (contains? @captured ::rpc/token-type))) (t/is (= "valid" (:name @captured))))) ;; --- RESPONSE: NIL BODY WITH CUSTOM HEADERS diff --git a/docker/images/docker-compose.yaml b/docker/images/docker-compose.yaml index 4c69760e24..5bca9bd6a0 100644 --- a/docker/images/docker-compose.yaml +++ b/docker/images/docker-compose.yaml @@ -191,6 +191,10 @@ services: PENPOT_SMTP_TLS: "false" PENPOT_SMTP_SSL: "false" + ## Inbox for the feedback that MCP agents send. Set it together with + ## PENPOT_MCP_BACKEND_URI on the penpot-mcp service. + # PENPOT_MCP_FEEDBACK_EMAIL: mcp-feedback@example.com + penpot-admin-console: image: "penpotapp/admin-console:${PENPOT_VERSION:-2.18}" restart: always @@ -215,6 +219,11 @@ services: networks: - penpot + # environment: + ## Offers the submit_feedback tool, which emails agent feedback through the + ## backend. Set it together with PENPOT_MCP_FEEDBACK_EMAIL on the backend. + # PENPOT_MCP_BACKEND_URI: http://penpot-backend:6060/ + penpot-exporter: image: "penpotapp/exporter:${PENPOT_VERSION:-2.18}" restart: always diff --git a/docs/mcp/index.md b/docs/mcp/index.md index fb9c927106..6b55a3f816 100644 --- a/docs/mcp/index.md +++ b/docs/mcp/index.md @@ -104,6 +104,9 @@ Current tools in **local MCP**: * `penpot_api_info` * `export_shape` * `import_image` +* `submit_feedback`, only when the server has `PENPOT_MCP_BACKEND_URI` set (see the [configuration guide](/technical-guide/configuration/#mcp)) + +`submit_feedback` emails plain-text feedback about Penpot MCP to the team that runs your Penpot instance. The agent sends it only when you ask or approve, and the email includes your name and email address. It needs your MCP key: the remote setup already sends it, and in local MCP you add it to the server URL as `userToken`. Because **remote MCP** does not expose local file-system access: diff --git a/docs/technical-guide/configuration.md b/docs/technical-guide/configuration.md index 91c6ab740d..4759855e14 100644 --- a/docs/technical-guide/configuration.md +++ b/docs/technical-guide/configuration.md @@ -959,6 +959,30 @@ The defaults match the service name used in the official `docker-compose.yaml`. them only if your MCP service has a different name or listens on other ports. Both variables are ignored when the `enable-mcp` flag is not set. +#### MCP feedback + +Agents can email feedback about Penpot MCP to an inbox you choose, through the +`submit_feedback` tool. Set the backend URI on the MCP container to offer the tool, +and the inbox on the backend to accept the emails: + +```bash +# MCP +PENPOT_MCP_BACKEND_URI: http://penpot-backend:6060/ + +# Backend +PENPOT_MCP_FEEDBACK_EMAIL: mcp-feedback@example.com +``` + +- `PENPOT_MCP_BACKEND_URI`: The URI the MCP server uses to reach the backend. When + unset, the MCP server does not offer the tool. +- `PENPOT_MCP_FEEDBACK_EMAIL`: The address that receives the feedback. When unset, + the backend rejects submissions. + +The backend accepts feedback when the `enable-mcp` or `enable-access-tokens` flag is +set, and sends it through the [email configuration](#email-configuration). Each +email carries the name and email address of the user who sent it, and uses that +address as reply-to. + ### Internal resolver The frontend container resolves the backend, exporter and MCP service names with the diff --git a/mcp/README.md b/mcp/README.md index b38b60d502..4c206d9e35 100644 --- a/mcp/README.md +++ b/mcp/README.md @@ -290,7 +290,7 @@ The Penpot MCP server can be configured using environment variables. | Environment Variable | Description | Default | |--------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------| | `PENPOT_MCP_SERVER_HOST` | Address on which the MCP server listens (binds to) | `localhost` | -| `PENPOT_MCP_SERVER_PORT` | Port for the MCP server | `4401` | +| `PENPOT_MCP_SERVER_PORT` | Port for the MCP server | `4401` | | `PENPOT_MCP_WEBSOCKET_PORT` | Port for the WebSocket server (plugin connection) | `4402` | | `PENPOT_MCP_REPL_PORT` | Port for the REPL server (development/debugging) | `4403` | | `PENPOT_MCP_REPL_HOST` | Address on which the REPL server listens (binds to) | `localhost` | @@ -301,6 +301,17 @@ The Penpot MCP server can be configured using environment variables. | `PENPOT_MCP_EXPORT_SHAPE_MAX_PARALLEL_REQUESTS` | Maximum number of parallel export shape requests (multi-user mode only). | `0` (no limit) | | `PENPOT_MCP_AUTH_URI` | Public base URL of the MCP authorization service, e.g. `https://penpot.example/api/mcp/oauth`. Set to enable OAuth; requires `--multi-user`. Use HTTPS (HTTP is allowed on loopback for development), without credentials, query parameters, or a fragment. | (unset; OAuth disabled) | | `PENPOT_MCP_REDIS_URI` | Redis connection URI (e.g. `redis://host:6379`) enabling multi-instance horizontal scaling via Redis pub/sub task routing (multi-user mode only). When unset, the server runs in single-instance mode, requiring the plugin and MCP client to connect to the same instance. | (unset) | +| `PENPOT_MCP_BACKEND_URI` | Penpot instance base URI. Setting a nonblank value enables tools that communicate with the Penpot backend (currently only the `submit_feedback` tool). The MCP server appends suffixes such as `api/main/methods/send-mcp-feedback`. Backend interactions require MCP user tokens to be provided. | (unset) | + +### Backend Configuration + +If `PENPOT_MCP_BACKEND_URI` is set, the backend must itself be configured correspondingly, i.e. set +the following variables for the backend process: + +| Environment Variable | Description | Default | +|----------------------|-------------|---------| +| `PENPOT_MCP_FEEDBACK_EMAIL` | Selects the inbox for MCP feedback. Unset means the command rejects submissions. Requires the `enable-mcp` or `enable-access-tokens` flag and uses the backend's existing SMTP configuration. | (unset) | + ### Logging Configuration diff --git a/mcp/packages/server/src/Feedback.test.ts b/mcp/packages/server/src/Feedback.test.ts new file mode 100644 index 0000000000..b5b6460250 --- /dev/null +++ b/mcp/packages/server/src/Feedback.test.ts @@ -0,0 +1,221 @@ +import assert from "node:assert/strict"; +import { createServer, type Server } from "node:http"; +import { once } from "node:events"; +import { afterEach, beforeEach, test } from "node:test"; +import { Client, StreamableHTTPClientTransport } from "@modelcontextprotocol/client"; +import { PenpotMcpServer } from "./PenpotMcpServer"; +import { PenpotBackendClient } from "./PenpotBackendClient"; + +let previousEnv: NodeJS.ProcessEnv; +let backend: Server; +let backendUri: string; +let server: PenpotMcpServer | undefined; +let client: Client | undefined; +let requests: { url: string; authorization?: string; client?: string; body: unknown }[]; +let responseStatus: number; +let nextPort = 17_700; + +beforeEach(async () => { + previousEnv = { ...process.env }; + process.env.PENPOT_MCP_SERVER_HOST = "127.0.0.1"; + process.env.PENPOT_MCP_SERVER_PORT = String(nextPort++); + process.env.PENPOT_MCP_WEBSOCKET_PORT = "0"; + process.env.PENPOT_MCP_DEVENV = "false"; + process.env.PENPOT_MCP_REPL_ENABLE = "false"; + delete process.env.PENPOT_MCP_REDIS_URI; + delete process.env.PENPOT_MCP_BACKEND_URI; + requests = []; + responseStatus = 200; + backend = createServer(async (req, res) => { + let body = ""; + for await (const chunk of req) body += chunk; + requests.push({ + url: req.url!, + authorization: req.headers.authorization, + client: req.headers["x-client"] as string, + body: JSON.parse(body), + }); + res.writeHead(responseStatus, { + "content-type": "application/json", + ...(responseStatus === 307 ? { location: `${backendUri}/redirected` } : {}), + }); + res.end(JSON.stringify({ status: "queued", detail: "private backend diagnostics" })); + }); + backend.listen(0, "127.0.0.1"); + await once(backend, "listening"); + const address = backend.address(); + assert.ok(address && typeof address === "object"); + backendUri = `http://127.0.0.1:${address.port}/penpot`; +}); + +afterEach(async () => { + await client?.close(); + await server?.stop(); + client = undefined; + server = undefined; + backend.closeAllConnections(); + await new Promise((resolve, reject) => backend.close((error) => (error ? reject(error) : resolve()))); + process.env = previousEnv; +}); + +async function connect(multiUser: boolean, token?: string) { + server = new PenpotMcpServer(multiUser); + await server.start(); + client = new Client({ name: "feedback-test", version: "1" }); + const url = new URL(`http://127.0.0.1:${server.port}/mcp`); + if (token) url.searchParams.set("userToken", token); + await client.connect(new StreamableHTTPClientTransport(url)); + return client; +} + +const feedback = { subject: "Export fails", feedback: "Expected a PNG, received an error." }; + +for (const multiUser of [false, true]) { + test(`feedback is absent without a backend URI (multi-user=${multiUser})`, async () => { + const connected = await connect(multiUser, "alice"); + assert.ok(!(await connected.listTools()).tools.some((tool) => tool.name === "submit_feedback")); + }); + + test(`feedback works without a plugin (multi-user=${multiUser})`, async () => { + process.env.PENPOT_MCP_BACKEND_URI = backendUri; + const connected = await connect(multiUser, "alice"); + const tool = (await connected.listTools()).tools.find((tool) => tool.name === "submit_feedback"); + assert.ok(tool); + assert.deepEqual(tool.inputSchema.required, ["subject", "feedback"]); + const result = await connected.callTool({ name: "submit_feedback", arguments: feedback }); + assert.deepEqual(result.content, [ + { type: "text", text: "Feedback queued. It is emailed in the background, so delivery is not confirmed." }, + ]); + assert.deepEqual(requests, [ + { + url: "/penpot/api/main/methods/send-mcp-feedback", + authorization: "Token alice", + client: "penpot-mcp", + body: feedback, + }, + ]); + }); + + test(`missing credentials leave the tool listed but prevent submission (multi-user=${multiUser})`, async () => { + process.env.PENPOT_MCP_BACKEND_URI = backendUri; + const connected = await connect(multiUser); + assert.ok((await connected.listTools()).tools.some((tool) => tool.name === "submit_feedback")); + const result = await connected.callTool({ name: "submit_feedback", arguments: feedback }); + assert.match(JSON.stringify(result.content), /userToken.*MCP.*URL/); + assert.equal(requests.length, 0); + }); +} + +test("blank backend URI does not enable feedback", async () => { + process.env.PENPOT_MCP_BACKEND_URI = " "; + const connected = await connect(false); + assert.ok(!(await connected.listTools()).tools.some((tool) => tool.name === "submit_feedback")); +}); + +test("rejects invalid input without contacting the backend", async () => { + process.env.PENPOT_MCP_BACKEND_URI = backendUri; + const connected = await connect(false, "alice"); + for (const args of [ + { ...feedback, subject: "" }, + { ...feedback, subject: " " }, + { ...feedback, subject: "a\r\nBcc: other@example.com" }, + { ...feedback, subject: "a\tb" }, + { ...feedback, subject: "a\u0085b" }, + { ...feedback, feedback: "a\u0000b" }, + { ...feedback, feedback: "a\u001bb" }, + { ...feedback, subject: "x".repeat(121) }, + { ...feedback, feedback: " \n" }, + { ...feedback, feedback: "x".repeat(5001) }, + ]) { + // SDK versions may return a tool error or reject with a protocol error. + try { + const result = await connected.callTool({ name: "submit_feedback", arguments: args }); + assert.equal(result.isError, true); + } catch (error) { + assert.match(String(error), /invalid|validation/i); + } + } + assert.equal(requests.length, 0); +}); + +test("preserves deployment prefixes with and without a trailing slash", async () => { + for (const uri of [backendUri, backendUri + "/"]) { + await new PenpotBackendClient(uri).submitFeedback("alice", feedback); + } + assert.deepEqual( + requests.map((request) => request.url), + ["/penpot/api/main/methods/send-mcp-feedback", "/penpot/api/main/methods/send-mcp-feedback"] + ); +}); + +test("keeps credentials isolated across concurrent submissions", async () => { + const backendClient = new PenpotBackendClient(backendUri); + await Promise.all( + ["alice", "bob"].map((token) => + backendClient.submitFeedback(token, { + subject: token, + feedback: "A concrete problem.", + }) + ) + ); + assert.deepEqual( + requests + .map((request) => ({ + authorization: request.authorization, + subject: (request.body as typeof feedback).subject, + })) + .sort((a, b) => a.subject.localeCompare(b.subject)), + [ + { authorization: "Token alice", subject: "alice" }, + { authorization: "Token bob", subject: "bob" }, + ] + ); +}); + +for (const status of [400, 401, 403, 404, 429, 503, 500, 307]) { + test(`does not retry or expose response bodies on status ${status}`, async () => { + responseStatus = status; + const backendClient = new PenpotBackendClient(backendUri); + await assert.rejects(backendClient.submitFeedback("secret-token", feedback), (error: Error) => { + assert.doesNotMatch(error.message, /private backend diagnostics|secret-token/); + return true; + }); + assert.equal(requests.length, 1); + }); +} + +test("bounds network waits and does not retry an uncertain submission", async (t) => { + backend.removeAllListeners("request"); + let count = 0; + backend.on("request", (_req, _res) => { + count++; + }); + const timeout = AbortSignal.timeout.bind(AbortSignal); + t.mock.method(AbortSignal, "timeout", (milliseconds: number) => { + assert.equal(milliseconds, 10_000); + return timeout(50); + }); + await assert.rejects( + new PenpotBackendClient(backendUri).submitFeedback("secret-token", feedback), + /could not be confirmed.*Do not retry automatically/ + ); + assert.ok(count <= 1); +}); + +test("rejects invalid backend configuration without echoing credentials", () => { + for (const uri of [ + "invalid", + "file:///tmp", + "https://user:secret@example.com", + "https://example.com/?secret=value", + "https://example.com/#fragment", + ]) { + assert.throws( + () => new PenpotBackendClient(uri), + (error: Error) => { + assert.doesNotMatch(error.message, /secret|value|user:/); + return true; + } + ); + } +}); diff --git a/mcp/packages/server/src/PenpotBackendClient.ts b/mcp/packages/server/src/PenpotBackendClient.ts new file mode 100644 index 0000000000..07bebddf97 --- /dev/null +++ b/mcp/packages/server/src/PenpotBackendClient.ts @@ -0,0 +1,86 @@ +import { createLogger } from "./logger"; + +/** A plain-text MCP feedback report. */ +export interface Feedback { + subject: string; + feedback: string; +} + +/** Backend operations available to MCP tools. */ +export interface PenpotBackend { + submitFeedback(userToken: string, feedback: Feedback): Promise; +} + +/** Client for the configured Penpot instance's backend services. */ +export class PenpotBackendClient implements PenpotBackend { + private readonly logger = createLogger("PenpotBackendClient"); + private readonly baseUri: URL; + + constructor(baseUri: string) { + try { + this.baseUri = new URL(baseUri); + } catch { + throw new Error("PENPOT_MCP_BACKEND_URI must be an absolute HTTP(S) instance URI."); + } + if ( + !["http:", "https:"].includes(this.baseUri.protocol) || + this.baseUri.username || + this.baseUri.password || + this.baseUri.search || + this.baseUri.hash + ) { + throw new Error( + "PENPOT_MCP_BACKEND_URI must be an HTTP(S) instance URI without credentials, query or fragment." + ); + } + if (!this.baseUri.pathname.endsWith("/")) this.baseUri.pathname += "/"; + } + + public async submitFeedback(userToken: string, feedback: Feedback): Promise { + await this.callRpc("send-mcp-feedback", userToken, feedback); + } + + /** Calls a backend RPC method whose result body is not needed. */ + private async callRpc(methodName: string, userToken: string, params: object): Promise { + let response: Response; + try { + response = await fetch(new URL(`api/main/methods/${encodeURIComponent(methodName)}`, this.baseUri), { + method: "POST", + headers: { + Authorization: `Token ${userToken}`, + "X-Client": "penpot-mcp", + "Content-Type": "application/json", + Accept: "application/json", + }, + body: JSON.stringify(params), + redirect: "manual", + signal: AbortSignal.timeout(10_000), + }); + // discard backend diagnostics, which may contain private data + await response.body?.cancel(); + } catch (error) { + this.logger.error(error, "Penpot RPC request failed"); + throw new Error("The backend operation could not be confirmed. Do not retry automatically."); + } + + if (response.ok) return; + + switch (response.status) { + case 401: + throw new Error( + "The backend did not accept the MCP user token. It may be outdated, " + + "or the Penpot instance may not have access tokens enabled." + ); + case 403: + throw new Error("The MCP user token is not authorized for this backend operation."); + case 400: + throw new Error("The backend rejected the request parameters or the operation is disabled."); + case 429: + throw new Error("The backend request limit has been reached. Do not retry automatically."); + default: + throw new Error( + `The backend operation could not be confirmed (HTTP ${response.status}). Do not retry automatically.` + ); + } + } +} diff --git a/mcp/packages/server/src/PenpotMcpServer.ts b/mcp/packages/server/src/PenpotMcpServer.ts index 2ec5cbe146..445fb4ee4e 100644 --- a/mcp/packages/server/src/PenpotMcpServer.ts +++ b/mcp/packages/server/src/PenpotMcpServer.ts @@ -8,17 +8,19 @@ import { ExecuteCodeTool } from "./tools/ExecuteCodeTool"; import { PluginBridge } from "./PluginBridge"; import { RedisBridge } from "./RedisBridge"; import { ConfigurationLoader } from "./ConfigurationLoader"; +import { PenpotBackendClient } from "./PenpotBackendClient"; +import { SubmitFeedbackTool } from "./tools/SubmitFeedbackTool"; import { createLogger } from "./logger"; import { Tool } from "./Tool"; import { HighLevelOverviewTool } from "./tools/HighLevelOverviewTool"; import { PenpotApiInfoTool } from "./tools/PenpotApiInfoTool"; import { ExportShapeTool } from "./tools/ExportShapeTool"; import { ImportImageTool } from "./tools/ImportImageTool"; -import { CljsReplTool } from "./tools/CljsReplTool"; -import { ImportPenpotFileTool } from "./tools/ImportPenpotFileTool"; -import { CljsCompilerOutputTool } from "./tools/CljsCompilerOutputTool"; -import { CljCheckParentheses } from "./tools/CljCheckParentheses"; -import { ReadTaigaIssueTool } from "./tools/ReadTaigaIssueTool"; +import { CljsReplTool } from "./tools/dev/CljsReplTool"; +import { ImportPenpotFileTool } from "./tools/dev/ImportPenpotFileTool"; +import { CljsCompilerOutputTool } from "./tools/dev/CljsCompilerOutputTool"; +import { CljCheckParentheses } from "./tools/dev/CljCheckParentheses"; +import { ReadTaigaIssueTool } from "./tools/dev/ReadTaigaIssueTool"; import { NreplClient } from "./NreplClient"; import { ReplServer } from "./ReplServer"; import { ApiDocs } from "./ApiDocs"; @@ -264,6 +266,10 @@ export class PenpotMcpServer { if (this.isFileSystemAccessEnabled()) { toolInstances.push(new ImportImageTool(this)); } + const backendUri = process.env.PENPOT_MCP_BACKEND_URI?.trim(); + if (backendUri) { + toolInstances.push(new SubmitFeedbackTool(this, new PenpotBackendClient(backendUri))); + } if (shouldRegisterDeveloperTools(this.isDevEnv(), this.isMultiUserMode())) { const nreplClient = new NreplClient(); toolInstances.push(new CljsReplTool(this, nreplClient)); diff --git a/mcp/packages/server/src/tools/SubmitFeedbackTool.ts b/mcp/packages/server/src/tools/SubmitFeedbackTool.ts new file mode 100644 index 0000000000..4c7b92f952 --- /dev/null +++ b/mcp/packages/server/src/tools/SubmitFeedbackTool.ts @@ -0,0 +1,67 @@ +import { z } from "zod"; +import type { Feedback, PenpotBackend } from "../PenpotBackendClient"; +import type { PenpotMcpServer } from "../PenpotMcpServer"; +import { Tool } from "../Tool"; +import { TextResponse, type ToolResponse } from "../ToolResponse"; + +/** Arguments for submitting a plain-text feedback report. */ +export class SubmitFeedbackArgs implements Feedback { + static schema = { + subject: z + .string() + .min(1) + .max(120) + .regex(/\S/) + .regex(/^\P{Cc}*$/u) + .describe("Short, specific summary without line breaks or other control characters (max 120 characters)."), + feedback: z + .string() + .min(1) + .max(5000) + .regex(/\S/) + // line breaks and tabs are the only control characters allowed + .regex(/^[^\x00-\x08\x0B\x0C\x0E-\x1F\x7F-\x9F]*$/) + .describe("Concise plain-text feedback (max 5,000 characters)."), + }; + + subject!: string; + feedback!: string; +} + +/** Tool for user-approved feedback about the Penpot MCP integration. */ +export class SubmitFeedbackTool extends Tool { + constructor( + mcpServer: PenpotMcpServer, + private readonly backend: PenpotBackend + ) { + super(mcpServer, SubmitFeedbackArgs.schema); + } + + public getToolName(): string { + return "submit_feedback"; + } + + public getToolDescription(): string { + return ( + "Submit feedback about Penpot MCP, provided that the user explicitly requests or approves submission " + + "and there is a concrete, actionable issue to report. " + + "Focus on bugs, limitations, or unclear documentation that blocked work or caused substantial rework. " + + "Briefly propose what to report and ask the user at a natural pause. " + + "Skip vague dissatisfaction, routine transient errors, and your own coding mistakes. " + + "Describe the operation, expected and actual behavior, and any workaround. Exclude sensitive content." + ); + } + + protected formatArgs(_args: SubmitFeedbackArgs): string { + return "[feedback omitted]"; + } + + protected async executeCore(args: SubmitFeedbackArgs): Promise { + const userToken = this.getSessionContext()?.userToken; + if (!userToken?.trim()) { + throw new Error("Include userToken in the MCP server URL to submit feedback."); + } + await this.backend.submitFeedback(userToken, { subject: args.subject, feedback: args.feedback }); + return new TextResponse("Feedback queued. It is emailed in the background, so delivery is not confirmed."); + } +} diff --git a/mcp/packages/server/src/tools/CljCheckParentheses.ts b/mcp/packages/server/src/tools/dev/CljCheckParentheses.ts similarity index 97% rename from mcp/packages/server/src/tools/CljCheckParentheses.ts rename to mcp/packages/server/src/tools/dev/CljCheckParentheses.ts index 81e2a9628c..d7a5af6fd2 100644 --- a/mcp/packages/server/src/tools/CljCheckParentheses.ts +++ b/mcp/packages/server/src/tools/dev/CljCheckParentheses.ts @@ -1,9 +1,9 @@ import { z } from "zod"; -import { Tool } from "../Tool"; +import { Tool } from "../../Tool"; import "reflect-metadata"; -import type { ToolResponse } from "../ToolResponse"; -import { TextResponse } from "../ToolResponse"; -import type { PenpotMcpServer } from "../PenpotMcpServer"; +import type { ToolResponse } from "../../ToolResponse"; +import { TextResponse } from "../../ToolResponse"; +import type { PenpotMcpServer } from "../../PenpotMcpServer"; import * as fs from "fs"; /** diff --git a/mcp/packages/server/src/tools/CljsCompilerOutputTool.ts b/mcp/packages/server/src/tools/dev/CljsCompilerOutputTool.ts similarity index 86% rename from mcp/packages/server/src/tools/CljsCompilerOutputTool.ts rename to mcp/packages/server/src/tools/dev/CljsCompilerOutputTool.ts index 11989c6303..772d9461df 100644 --- a/mcp/packages/server/src/tools/CljsCompilerOutputTool.ts +++ b/mcp/packages/server/src/tools/dev/CljsCompilerOutputTool.ts @@ -1,9 +1,9 @@ -import { Tool, EmptyToolArgs } from "../Tool"; +import { Tool, EmptyToolArgs } from "../../Tool"; import "reflect-metadata"; -import type { ToolResponse } from "../ToolResponse"; -import { TextResponse } from "../ToolResponse"; -import { PenpotMcpServer } from "../PenpotMcpServer"; -import { NreplClient } from "../NreplClient"; +import type { ToolResponse } from "../../ToolResponse"; +import { TextResponse } from "../../ToolResponse"; +import { PenpotMcpServer } from "../../PenpotMcpServer"; +import { NreplClient } from "../../NreplClient"; /** * Reports the compiler status of the shadow-cljs `:main` build. diff --git a/mcp/packages/server/src/tools/CljsReplTool.ts b/mcp/packages/server/src/tools/dev/CljsReplTool.ts similarity index 89% rename from mcp/packages/server/src/tools/CljsReplTool.ts rename to mcp/packages/server/src/tools/dev/CljsReplTool.ts index bd894caef3..ab2babf56d 100644 --- a/mcp/packages/server/src/tools/CljsReplTool.ts +++ b/mcp/packages/server/src/tools/dev/CljsReplTool.ts @@ -1,10 +1,10 @@ import { z } from "zod"; -import { Tool } from "../Tool"; +import { Tool } from "../../Tool"; import "reflect-metadata"; -import type { ToolResponse } from "../ToolResponse"; -import { TextResponse } from "../ToolResponse"; -import { PenpotMcpServer } from "../PenpotMcpServer"; -import { NreplClient } from "../NreplClient"; +import type { ToolResponse } from "../../ToolResponse"; +import { TextResponse } from "../../ToolResponse"; +import { PenpotMcpServer } from "../../PenpotMcpServer"; +import { NreplClient } from "../../NreplClient"; /** * Arguments for the CljsReplTool. diff --git a/mcp/packages/server/src/tools/ImportPenpotFileTool.ts b/mcp/packages/server/src/tools/dev/ImportPenpotFileTool.ts similarity index 98% rename from mcp/packages/server/src/tools/ImportPenpotFileTool.ts rename to mcp/packages/server/src/tools/dev/ImportPenpotFileTool.ts index f55ae194f2..50d18f5ae5 100644 --- a/mcp/packages/server/src/tools/ImportPenpotFileTool.ts +++ b/mcp/packages/server/src/tools/dev/ImportPenpotFileTool.ts @@ -1,10 +1,10 @@ import { z } from "zod"; -import { Tool } from "../Tool"; -import { TextResponse, ToolResponse } from "../ToolResponse"; +import { Tool } from "../../Tool"; +import { TextResponse, ToolResponse } from "../../ToolResponse"; import "reflect-metadata"; -import { PenpotMcpServer } from "../PenpotMcpServer"; -import { NreplClient } from "../NreplClient"; -import { createLogger } from "../logger"; +import { PenpotMcpServer } from "../../PenpotMcpServer"; +import { NreplClient } from "../../NreplClient"; +import { createLogger } from "../../logger"; import * as crypto from "crypto"; import * as fs from "fs"; import * as path from "path"; diff --git a/mcp/packages/server/src/tools/ReadTaigaIssueTool.ts b/mcp/packages/server/src/tools/dev/ReadTaigaIssueTool.ts similarity index 96% rename from mcp/packages/server/src/tools/ReadTaigaIssueTool.ts rename to mcp/packages/server/src/tools/dev/ReadTaigaIssueTool.ts index 29e8bea460..decd926182 100644 --- a/mcp/packages/server/src/tools/ReadTaigaIssueTool.ts +++ b/mcp/packages/server/src/tools/dev/ReadTaigaIssueTool.ts @@ -1,9 +1,9 @@ import { z } from "zod"; -import { Tool } from "../Tool"; +import { Tool } from "../../Tool"; import "reflect-metadata"; -import type { ToolResponse } from "../ToolResponse"; -import { TextResponse } from "../ToolResponse"; -import { PenpotMcpServer } from "../PenpotMcpServer"; +import type { ToolResponse } from "../../ToolResponse"; +import { TextResponse } from "../../ToolResponse"; +import { PenpotMcpServer } from "../../PenpotMcpServer"; /** * Arguments for the {@link ReadTaigaIssueTool}.