From df01f7605629a3176755faf28428627ef186e0dc Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Wed, 6 May 2026 14:20:55 +0200 Subject: [PATCH] :bug: Fix incorrect invitation token handling on register process (#9380) * :bug: Fix incorrect invitation token handling on register process - Reject prepare-register-profile when an active profile already exists for the requested email. - Stop embedding an existing profile's :profile-id into the prepared-register JWE. Profile resolution in register-profile is now done exclusively by email lookup, never by a JWE claim. - Add created? guard to the invitation-success branch in register-profile, so existing profiles (active or not) cannot reach session creation via anonymous registration. Signed-off-by: Andrey Antukh * :recycle: Restructure invitation handling inside register-profile Move the invitation-success branch into the created? sub-cond so it sits alongside the other post-creation branches, making the control flow consistent. - Active new profile + matching invitation: mint session and return :invitation-token (frontend redirects to :auth-verify-token). - Not-yet-active new profile + matching invitation: embed the invitation token inside the verify-email JWE and send the verification email. When the user clicks the link, they get logged in and the frontend completes the team-invitation flow. - Extend send-email-verification! with an optional invitation-token parameter propagated into the verify-email JWE claims. - Update the frontend verify-email handler to navigate to :auth-verify-token when the response carries :invitation-token. Signed-off-by: Andrey Antukh * :bug: Handle email-already-exists error on registration form Add a specific handler for the [:validation :email-already-exists] error code in the registration form's on-error callback. The backend raises this error when an active profile already exists for the requested email, but the frontend was falling through to the generic error message. Now it shows the existing "Email already used" i18n message instead of the generic "Something wrong has happened" toast. * :bug: Reset submitted state on registration form error The on-error handler in the registration form was not resetting the submitted? state, causing the submit button to remain disabled after any error. The completion callback in rx/subs! only fires on success, not on error. Add (reset! submitted? false) at the beginning of the on-error handler so the form becomes submittable again after any error, allowing the user to fix their input and retry. --------- Signed-off-by: Andrey Antukh --- CHANGES.md | 7 + backend/src/app/binfile/v1.clj | 2 +- backend/src/app/db.clj | 2 +- backend/src/app/email.clj | 6 +- backend/src/app/media.clj | 2 +- backend/src/app/metrics.clj | 4 +- backend/src/app/redis.clj | 20 +- backend/src/app/rpc/commands/auth.clj | 219 ++++++++------ backend/src/app/rpc/commands/verify_token.clj | 5 + backend/src/app/storage/s3.clj | 14 +- .../test/backend_tests/rpc_profile_test.clj | 285 ++++++++++++++++-- frontend/src/app/main/ui/auth/register.cljs | 4 + .../src/app/main/ui/auth/verify_token.cljs | 12 +- 13 files changed, 442 insertions(+), 140 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index f9fb9fe1a7..6a1a0f8da5 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,12 @@ # CHANGELOG +## 2.14.5 + +### :bug: Bugs fixed + +- Fix incorrect invitation token handling on register process [Github #9380](https://github.com/penpot/penpot/pull/9380) + + ## 2.14.4 ### :bug: Bugs fixed diff --git a/backend/src/app/binfile/v1.clj b/backend/src/app/binfile/v1.clj index 04b390bb99..75f6f36994 100644 --- a/backend/src/app/binfile/v1.clj +++ b/backend/src/app/binfile/v1.clj @@ -40,8 +40,8 @@ [promesa.util :as pu] [yetti.adapter :as yt]) (:import - com.github.luben.zstd.ZstdIOException com.github.luben.zstd.ZstdInputStream + com.github.luben.zstd.ZstdIOException com.github.luben.zstd.ZstdOutputStream java.io.DataInputStream java.io.DataOutputStream diff --git a/backend/src/app/db.clj b/backend/src/app/db.clj index b00f84e3e2..c23ea07524 100644 --- a/backend/src/app/db.clj +++ b/backend/src/app/db.clj @@ -36,11 +36,11 @@ java.sql.Connection java.sql.PreparedStatement java.sql.Savepoint - org.postgresql.PGConnection org.postgresql.geometric.PGpoint org.postgresql.jdbc.PgArray org.postgresql.largeobject.LargeObject org.postgresql.largeobject.LargeObjectManager + org.postgresql.PGConnection org.postgresql.util.PGInterval org.postgresql.util.PGobject)) diff --git a/backend/src/app/email.clj b/backend/src/app/email.clj index 44d5cd7e67..b42206dc93 100644 --- a/backend/src/app/email.clj +++ b/backend/src/app/email.clj @@ -22,13 +22,13 @@ [cuerdas.core :as str] [integrant.core :as ig]) (:import - jakarta.mail.Message$RecipientType - jakarta.mail.Session - jakarta.mail.Transport jakarta.mail.internet.InternetAddress jakarta.mail.internet.MimeBodyPart jakarta.mail.internet.MimeMessage jakarta.mail.internet.MimeMultipart + jakarta.mail.Message$RecipientType + jakarta.mail.Session + jakarta.mail.Transport java.util.Properties)) ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/backend/src/app/media.clj b/backend/src/app/media.clj index d54f19ab10..863d2e9df2 100644 --- a/backend/src/app/media.clj +++ b/backend/src/app/media.clj @@ -31,8 +31,8 @@ (:import clojure.lang.XMLHandler java.io.InputStream - javax.xml.XMLConstants javax.xml.parsers.SAXParserFactory + javax.xml.XMLConstants org.apache.commons.io.IOUtils org.im4java.core.ConvertCmd org.im4java.core.IMOperation)) diff --git a/backend/src/app/metrics.clj b/backend/src/app/metrics.clj index 1c7456b7ab..a1f816a304 100644 --- a/backend/src/app/metrics.clj +++ b/backend/src/app/metrics.clj @@ -15,16 +15,16 @@ io.prometheus.client.CollectorRegistry io.prometheus.client.Counter io.prometheus.client.Counter$Child + io.prometheus.client.exporter.common.TextFormat io.prometheus.client.Gauge io.prometheus.client.Gauge$Child io.prometheus.client.Histogram io.prometheus.client.Histogram$Child + io.prometheus.client.hotspot.DefaultExports io.prometheus.client.SimpleCollector io.prometheus.client.Summary io.prometheus.client.Summary$Builder io.prometheus.client.Summary$Child - io.prometheus.client.exporter.common.TextFormat - io.prometheus.client.hotspot.DefaultExports java.io.StringWriter)) (set! *warn-on-reflection* true) diff --git a/backend/src/app/redis.clj b/backend/src/app/redis.clj index 96e6b07be5..dc1bff9669 100644 --- a/backend/src/app/redis.clj +++ b/backend/src/app/redis.clj @@ -24,28 +24,28 @@ [integrant.core :as ig]) (:import clojure.lang.MapEntry - io.lettuce.core.KeyValue - io.lettuce.core.RedisClient - io.lettuce.core.RedisCommandInterruptedException - io.lettuce.core.RedisCommandTimeoutException - io.lettuce.core.RedisException - io.lettuce.core.RedisURI - io.lettuce.core.ScriptOutputType - io.lettuce.core.SetArgs io.lettuce.core.api.StatefulRedisConnection io.lettuce.core.api.sync.RedisCommands io.lettuce.core.api.sync.RedisScriptingCommands io.lettuce.core.codec.RedisCodec io.lettuce.core.codec.StringCodec + io.lettuce.core.KeyValue + io.lettuce.core.pubsub.api.sync.RedisPubSubCommands io.lettuce.core.pubsub.RedisPubSubListener io.lettuce.core.pubsub.StatefulRedisPubSubConnection - io.lettuce.core.pubsub.api.sync.RedisPubSubCommands + io.lettuce.core.RedisClient + io.lettuce.core.RedisCommandInterruptedException + io.lettuce.core.RedisCommandTimeoutException + io.lettuce.core.RedisException + io.lettuce.core.RedisURI io.lettuce.core.resource.ClientResources io.lettuce.core.resource.DefaultClientResources + io.lettuce.core.ScriptOutputType + io.lettuce.core.SetArgs io.netty.channel.nio.NioEventLoopGroup + io.netty.util.concurrent.EventExecutorGroup io.netty.util.HashedWheelTimer io.netty.util.Timer - io.netty.util.concurrent.EventExecutorGroup java.lang.AutoCloseable java.time.Duration)) diff --git a/backend/src/app/rpc/commands/auth.clj b/backend/src/app/rpc/commands/auth.clj index c3592d790c..26082e0488 100644 --- a/backend/src/app/rpc/commands/auth.clj +++ b/backend/src/app/rpc/commands/auth.clj @@ -258,24 +258,44 @@ (validate-register-attempt! cfg params) (let [email (profile/clean-email email) - profile (profile/get-profile-by-email pool email) - props (-> (audit/extract-utm-params params) - (cond-> (:accept-newsletter-updates params) - (assoc :newsletter-updates true))) - params {:email email - :fullname fullname - :password (:password params) - :invitation-token (:invitation-token params) - :backend "penpot" - :iss :prepared-register - :profile-id (:id profile) - :exp (ct/in-future {:days 7}) - :props props} - params (d/without-nils params) - token (tokens/generate cfg params)] + profile (profile/get-profile-by-email pool email)] - (-> {:token token} - (with-meta {::audit/profile-id uuid/zero})))) + ;; SECURITY: refuse to issue a prepared-register token when an active + ;; profile already exists for this email. + ;; + ;; Active accounts must use the standard login flow; existing-but- + ;; not-yet-active profiles fall through to the duplicate-detection branch in + ;; `register-profile`, which never creates a session. + (when (and (some? profile) + (true? (:is-active profile))) + (ex/raise :type :validation + :code :email-already-exists + :hint "email already exists")) + + (let [props (-> (audit/extract-utm-params params) + (cond-> (:accept-newsletter-updates params) + (assoc :newsletter-updates true))) + ;; SECURITY: do NOT embed `:profile-id` of an existing + ;; profile into the prepared-register JWE. Doing so would + ;; let an anonymous caller, in possession of a valid + ;; team-invitation JWE, ask `register-profile` to load that + ;; profile by id and mint a session for it without password + ;; verification. `register-profile` independently re-detects + ;; duplicates by email and handles them in the + ;; "repeated-registry" branch. + params {:email email + :fullname fullname + :password (:password params) + :invitation-token (:invitation-token params) + :backend "penpot" + :iss :prepared-register + :exp (ct/in-future {:days 7}) + :props props} + params (d/without-nils params) + token (tokens/generate cfg params)] + + (-> {:token token} + (with-meta {::audit/profile-id uuid/zero}))))) (def schema:prepare-register-profile [:map {:title "prepare-register-profile"} @@ -387,25 +407,32 @@ (profile/decode-row)))) (defn send-email-verification! - [{:keys [::db/conn] :as cfg} profile] - (let [vtoken (tokens/generate cfg - {:iss :verify-email - :exp (ct/in-future "72h") - :profile-id (:id profile) - :email (:email profile)}) - ;; NOTE: this token is mainly used for possible complains - ;; identification on the sns webhook - ptoken (tokens/generate cfg - {:iss :profile-identity - :profile-id (:id profile) - :exp (ct/in-future {:days 30})})] - (eml/send! {::eml/conn conn - ::eml/factory eml/register - :public-uri (cf/get :public-uri) - :to (:email profile) - :name (:fullname profile) - :token vtoken - :extra-data ptoken}))) + ([cfg profile] (send-email-verification! cfg profile nil)) + ([{:keys [::db/conn] :as cfg} profile invitation-token] + (let [vclaims (cond-> {:iss :verify-email + :exp (ct/in-future "72h") + :profile-id (:id profile) + :email (:email profile)} + ;; If the user registered through a team-invitation flow but + ;; their profile is not yet active, we carry the invitation + ;; token inside the verify-email JWE so the team-invitation + ;; flow can resume after the user clicks the email link. + (some? invitation-token) + (assoc :invitation-token invitation-token)) + vtoken (tokens/generate cfg vclaims) + ;; NOTE: this token is mainly used for possible complains + ;; identification on the sns webhook + ptoken (tokens/generate cfg + {:iss :profile-identity + :profile-id (:id profile) + :exp (ct/in-future {:days 30})})] + (eml/send! {::eml/conn conn + ::eml/factory eml/register + :public-uri (cf/get :public-uri) + :to (:email profile) + :name (:fullname profile) + :token vtoken + :extra-data ptoken})))) (defn register-profile [{:keys [::db/conn ::wrk/executor] :as cfg} {:keys [token] :as params}] @@ -414,23 +441,16 @@ (:accept-newsletter-updates params) (update :props assoc :newsletter-updates true)) - profile (if-let [profile-id (:profile-id claims)] - (profile/get-profile conn profile-id) - ;; NOTE: we first try to match existing profile - ;; by email, that in normal circumstances will - ;; not return anything, but when a user tries to - ;; reuse the same token multiple times, we need - ;; to detect if the profile is already registered - (or (profile/get-profile-by-email conn (:email claims)) - (let [is-active (or (boolean (:is-active claims)) - (boolean (:email-verified claims)) - (not (contains? cf/flags :email-verification))) - params (-> params - (assoc :is-active is-active) - (update :password auth/derive-password)) - profile (->> (create-profile cfg params) - (create-profile-rels conn))] - (vary-meta profile assoc :created true)))) + profile (or (profile/get-profile-by-email conn (:email claims)) + (let [is-active (or (boolean (:is-active claims)) + (boolean (:email-verified claims)) + (not (contains? cf/flags :email-verification))) + params (-> params + (assoc :is-active is-active) + (update :password auth/derive-password)) + profile (->> (create-profile cfg params) + (create-profile-rels cfg))] + (vary-meta profile assoc :created true))) created? (-> profile meta :created true?) @@ -461,48 +481,67 @@ ::audit/profile-id (:id profile) ::audit/name "register-profile-retry"})) - ;; If invitation token comes in params, this is because the user - ;; comes from team-invitation process; in this case, regenerate - ;; token and send back to the user a new invitation token (and - ;; mark current session as logged). This happens only if the - ;; invitation email matches with the register email. - (and (some? invitation) - (= (:email profile) - (:member-email invitation))) - (let [invitation (assoc invitation :member-id (:id profile)) - token (tokens/generate cfg invitation)] - (-> {:id (:id profile) - :email (:email profile) - :invitation-token token} - (rph/with-transform (session/create-fn cfg profile claims)) - (rph/with-meta {::audit/replace-props props - ::audit/context {:action "accept-invitation"} - ::audit/profile-id (:id profile)}))) - - ;; When a new user is created and it is already activated by - ;; configuration or specified by OIDC, we just mark the profile - ;; as logged-in + ;; A profile was just created in this call. Invitation handling is a + ;; sub-case of "newly created profile": we never honor invitations for + ;; pre-existing profiles via this anonymous RPC. The split below mirrors + ;; the non-invitation branches but threads the invitation through the + ;; appropriate path: + ;; + ;; - active + matching invitation → mint session and + ;; return :invitation-token. The frontend redirects to + ;; :auth-verify-token, which immediately accepts the + ;; invitation. + ;; - active + no/mismatched invitation → mint session + ;; ("login" action). New profile, no further action. + ;; - not-active + matching invitation → send the + ;; verify-email mail with the invitation token EMBEDDED + ;; into the verify-email JWE. No session yet. When the + ;; user clicks the link, verify-token activates the + ;; profile, mints a session, and propagates the + ;; invitation token to the frontend so it can complete + ;; the team-invitation flow. + ;; - not-active + no/mismatched invitation → standard + ;; "check your email" verification flow. created? - (if (:is-active profile) - (-> (profile/strip-private-attrs profile) - (rph/with-transform (session/create-fn cfg profile claims)) - (rph/with-defer create-welcome-file-when-needed) - (rph/with-meta - {::audit/replace-props props - ::audit/context {:action "login"} - ::audit/profile-id (:id profile)})) + (let [accept-invitation? (and (some? invitation) + (= (:email profile) + (:member-email invitation)))] + (cond + (and (:is-active profile) accept-invitation?) + (let [invitation (assoc invitation :member-id (:id profile)) + token (tokens/generate cfg invitation)] + (-> {:id (:id profile) + :email (:email profile) + :invitation-token token} + (rph/with-transform (session/create-fn cfg profile claims)) + (rph/with-defer create-welcome-file-when-needed) + (rph/with-meta {::audit/replace-props props + ::audit/context {:action "accept-invitation"} + ::audit/profile-id (:id profile)}))) - (do - (when-not (eml/has-reports? conn (:email profile)) - (send-email-verification! cfg profile)) - - (-> {:id (:id profile) - :email (:email profile)} + (:is-active profile) + (-> (profile/strip-private-attrs profile) + (rph/with-transform (session/create-fn cfg profile claims)) (rph/with-defer create-welcome-file-when-needed) (rph/with-meta {::audit/replace-props props - ::audit/context {:action "email-verification"} - ::audit/profile-id (:id profile)})))) + ::audit/context {:action "login"} + ::audit/profile-id (:id profile)})) + + :else + (do + (when-not (eml/has-reports? conn (:email profile)) + (send-email-verification! cfg profile + (when accept-invitation? + (:invitation-token params)))) + + (-> {:id (:id profile) + :email (:email profile)} + (rph/with-defer create-welcome-file-when-needed) + (rph/with-meta + {::audit/replace-props props + ::audit/context {:action "email-verification"} + ::audit/profile-id (:id profile)}))))) :else (let [elapsed? (elapsed-verify-threshold? profile) diff --git a/backend/src/app/rpc/commands/verify_token.clj b/backend/src/app/rpc/commands/verify_token.clj index a3454f7135..38a3e2b42f 100644 --- a/backend/src/app/rpc/commands/verify_token.clj +++ b/backend/src/app/rpc/commands/verify_token.clj @@ -72,6 +72,11 @@ {:is-active true} {:id (:id profile)})) + ;; NOTE: `claims` is returned verbatim (besides :profile). When the + ;; verify-email JWE was minted by `register-profile` for a not-yet- + ;; active profile that came from an invitation flow, `:invitation- + ;; token` will be present here and the frontend will use it to + ;; complete the team-invitation flow after login. (-> claims (rph/with-transform (session/create-fn cfg profile)) (rph/with-meta {::audit/name "verify-profile-email" diff --git a/backend/src/app/storage/s3.clj b/backend/src/app/storage/s3.clj index ef56e8a9b4..9322de70e6 100644 --- a/backend/src/app/storage/s3.clj +++ b/backend/src/app/storage/s3.clj @@ -30,21 +30,18 @@ java.nio.file.Path java.time.Duration java.util.Collection - java.util.Optional java.util.concurrent.atomic.AtomicLong + java.util.Optional org.reactivestreams.Subscriber software.amazon.awssdk.auth.credentials.DefaultCredentialsProvider - software.amazon.awssdk.core.ResponseBytes software.amazon.awssdk.core.async.AsyncRequestBody software.amazon.awssdk.core.async.AsyncResponseTransformer software.amazon.awssdk.core.async.BlockingInputStreamAsyncRequestBody software.amazon.awssdk.core.client.config.ClientAsyncConfiguration + software.amazon.awssdk.core.ResponseBytes software.amazon.awssdk.http.nio.netty.NettyNioAsyncHttpClient software.amazon.awssdk.http.nio.netty.SdkEventLoopGroup software.amazon.awssdk.regions.Region - software.amazon.awssdk.services.s3.S3AsyncClient - software.amazon.awssdk.services.s3.S3AsyncClientBuilder - software.amazon.awssdk.services.s3.S3Configuration software.amazon.awssdk.services.s3.model.Delete software.amazon.awssdk.services.s3.model.DeleteObjectRequest software.amazon.awssdk.services.s3.model.DeleteObjectsRequest @@ -54,9 +51,12 @@ software.amazon.awssdk.services.s3.model.ObjectIdentifier software.amazon.awssdk.services.s3.model.PutObjectRequest software.amazon.awssdk.services.s3.model.S3Error - software.amazon.awssdk.services.s3.presigner.S3Presigner software.amazon.awssdk.services.s3.presigner.model.GetObjectPresignRequest - software.amazon.awssdk.services.s3.presigner.model.PresignedGetObjectRequest)) + software.amazon.awssdk.services.s3.presigner.model.PresignedGetObjectRequest + software.amazon.awssdk.services.s3.presigner.S3Presigner + software.amazon.awssdk.services.s3.S3AsyncClient + software.amazon.awssdk.services.s3.S3AsyncClientBuilder + software.amazon.awssdk.services.s3.S3Configuration)) (def ^:private max-retries "A maximum number of retries on internal operations" diff --git a/backend/test/backend_tests/rpc_profile_test.clj b/backend/test/backend_tests/rpc_profile_test.clj index 1cdf16a99f..22f0e966f2 100644 --- a/backend/test/backend_tests/rpc_profile_test.clj +++ b/backend/test/backend_tests/rpc_profile_test.clj @@ -514,32 +514,89 @@ (t/is (= 0 (:call-count @mock)))))))) (t/deftest prepare-and-register-with-invitation-and-enabled-registration-1 - (let [itoken (tokens/generate th/*system* - {:iss :team-invitation - :exp (ct/in-future "48h") - :role :editor - :team-id uuid/zero - :member-email "user@example.com"}) - data {::th/type :prepare-register-profile - :invitation-token itoken - :fullname "foobar" - :email "user@example.com" - :password "foobar"} + ;; With email-verification ENABLED (the default), a brand-new + ;; profile created via the invitation flow is NOT active yet, so + ;; `register-profile` must NOT mint a session and must NOT echo + ;; back the invitation token. Instead it must dispatch the + ;; verify-email mail with the invitation token EMBEDDED into the + ;; verify-email JWE (so the team-invitation flow can resume after + ;; the user clicks the email link). + (with-mocks [mock {:target 'app.email/send! :return nil}] + (let [itoken (tokens/generate th/*system* + {:iss :team-invitation + :exp (ct/in-future "48h") + :role :editor + :team-id uuid/zero + :member-email "user@example.com"}) + prep-data {::th/type :prepare-register-profile + :invitation-token itoken + :fullname "foobar" + :email "user@example.com" + :password "foobar"} - {:keys [result error] :as out} (th/command! data)] - (t/is (nil? error)) - (t/is (map? result)) - (t/is (string? (:token result))) + {prep-result :result prep-error :error} (th/command! prep-data)] + (t/is (nil? prep-error)) + (t/is (map? prep-result)) + (t/is (string? (:token prep-result))) - (let [rtoken (:token result) - data {::th/type :register-profile - :token rtoken} + (let [reg-data {::th/type :register-profile + :token (:token prep-result)} - {:keys [result error] :as out} (th/command! data)] - ;; (th/print-result! out) - (t/is (nil? error)) - (t/is (map? result)) - (t/is (string? (:invitation-token result)))))) + {reg-result :result reg-error :error} (th/command! reg-data) + mdata (meta reg-result)] + (t/is (nil? reg-error)) + (t/is (map? reg-result)) + + ;; No invitation token echoed back, no session minted. + (t/is (nil? (:invitation-token reg-result))) + (t/is (empty? (:app.rpc/response-transform-fns mdata))) + + ;; The verify-email mail was dispatched, and its token claims + ;; carry the invitation-token through to the verification step. + (t/is (= 1 (:call-count @mock))) + (let [send-args (-> @mock :call-args) + email-token (->> send-args (some (fn [m] (when (map? m) (:token m))))) + vclaims (tokens/decode th/*system* email-token)] + (t/is (= :verify-email (:iss vclaims))) + (t/is (= itoken (:invitation-token vclaims)))))))) + +(t/deftest prepare-and-register-with-invitation-and-enabled-registration-1b + ;; With email-verification DISABLED, the brand-new profile is + ;; immediately active, so `register-profile` mints a session and + ;; returns the regenerated invitation token in the body — the + ;; frontend then redirects to :auth-verify-token to complete the + ;; team-invitation flow. + (with-redefs [app.config/flags #{:registration :login-with-password}] + (let [itoken (tokens/generate th/*system* + {:iss :team-invitation + :exp (ct/in-future "48h") + :role :editor + :team-id uuid/zero + :member-email "user@example.com"}) + prep-data {::th/type :prepare-register-profile + :invitation-token itoken + :fullname "foobar" + :email "user@example.com" + :password "foobar"} + + {prep-result :result prep-error :error} (th/command! prep-data)] + (t/is (nil? prep-error)) + (t/is (string? (:token prep-result))) + + (let [reg-data {::th/type :register-profile + :token (:token prep-result)} + + {reg-result :result reg-error :error} (th/command! reg-data) + mdata (meta reg-result)] + (t/is (nil? reg-error)) + (t/is (map? reg-result)) + + ;; Active branch: invitation-token is echoed back and a session + ;; is minted via `session/create-fn`. + (t/is (string? (:invitation-token reg-result))) + (t/is (seq (:app.rpc/response-transform-fns mdata))) + (t/is (= "accept-invitation" + (get-in mdata [:app.loggers.audit/context :action]))))))) (t/deftest prepare-and-register-with-invitation-and-enabled-registration-2 (let [itoken (tokens/generate th/*system* @@ -692,6 +749,188 @@ (t/is (= :validation (:type edata))) (t/is (= :email-as-password (:code edata)))))) +(t/deftest prepare-register-rejects-active-profile-email + ;; SECURITY: `prepare-register` must reject any attempt to prepare a + ;; registration for an email that already belongs to an *active* + ;; profile, regardless of whether an invitation token is supplied. + ;; Active profiles must use the standard login flow. + (let [_victim (th/create-profile* 1 {:is-active true + :email "victim@corp.tld"})] + + ;; Without invitation token. + (let [out (th/command! {::th/type :prepare-register-profile + :fullname "Mallory" + :email "victim@corp.tld" + :password "Whatever1!"})] + (t/is (not (th/success? out))) + (let [edata (-> out :error ex-data)] + (t/is (= :validation (:type edata))) + (t/is (= :email-already-exists (:code edata))))) + + ;; With invitation token (the GHSA-4937-35vc-hqjj exploit shape). + (let [itoken (tokens/generate th/*system* + {:iss :team-invitation + :exp (ct/in-future "48h") + :role :editor + :team-id uuid/zero + :member-email "victim@corp.tld"}) + out (th/command! {::th/type :prepare-register-profile + :invitation-token itoken + :fullname "Mallory" + :email "victim@corp.tld" + :password "Whatever1!"})] + (t/is (not (th/success? out))) + (let [edata (-> out :error ex-data)] + (t/is (= :validation (:type edata))) + (t/is (= :email-already-exists (:code edata))))))) + +(t/deftest prepare-register-must-not-leak-existing-profile-id + ;; Victim is a pre-existing profile that has not yet activated (e.g. + ;; freshly registered, has not clicked the email verification link). + ;; `prepare-register` allows the call (no active profile exists), but + ;; the issued JWE must NOT carry the existing profile's id. + (let [_victim (th/create-profile* 1 {:is-active false + :email "victim@corp.tld"}) + + ;; Attacker holds a cryptographically valid `:team-invitation` JWE + ;; for the victim's email. (In a real exploit this is obtained + ;; from `create-team-invitations` or `get-team-invitation-token` + ;; on a team the attacker owns.) + itoken (tokens/generate th/*system* + {:iss :team-invitation + :exp (ct/in-future "48h") + :role :editor + :team-id uuid/zero + :member-email "victim@corp.tld"}) + + ;; Anonymous request — no ::rpc/profile-id. + data {::th/type :prepare-register-profile + :invitation-token itoken + :fullname "Mallory" + :email "victim@corp.tld" + :password "Whatever1!"} + + out (th/command! data)] + + ;; The current behaviour either returns a token or rejects the request; + ;; what MUST hold is that the issued prepared-register JWE does not + ;; carry the victim's profile id. + (t/is (th/success? out)) + + (let [token (-> out :result :token) + claims (tokens/decode th/*system* token)] + (t/is (= :prepared-register (:iss claims))) + ;; This is the root-cause assertion: an anonymous prepare-register + ;; call must NEVER embed an existing profile's id. + (t/is (nil? (:profile-id claims)) + "prepare-register must not embed existing profile id of an anonymous caller")))) + +(t/deftest register-profile-with-invitation-must-not-take-over-existing-account + (with-mocks [_mock {:target 'app.email/send! :return nil}] + (let [;; Victim profile exists but is not yet active (e.g. registered + ;; but has not clicked the verification link). This is the + ;; remaining attack surface after fix 1b: `prepare-register` + ;; will not reject this case, so the `register-profile` path + ;; must enforce the security invariants on its own. + victim (th/create-profile* 1 {:is-active false + :email "victim@corp.tld"}) + + ;; Attacker mints a valid `:team-invitation` JWE for the victim's + ;; email. No member-id is included (matches what an attacker + ;; obtains via `create-team-invitations` against their own team + ;; before the victim has joined). + itoken (tokens/generate th/*system* + {:iss :team-invitation + :exp (ct/in-future "48h") + :role :editor + :team-id uuid/zero + :member-email "victim@corp.tld"}) + + ;; Step 1 (anonymous): prepare-register-profile with the victim's + ;; email + the invitation token. + prep-out (th/command! {::th/type :prepare-register-profile + :invitation-token itoken + :fullname "Mallory" + :email "victim@corp.tld" + :password "Whatever1!"}) + + rtoken (-> prep-out :result :token) + + ;; Step 2 (anonymous): register-profile with the prepared token. + reg-out (th/command! {::th/type :register-profile + :token rtoken}) + + result (:result reg-out) + mdata (meta result)] + + ;; The first call may succeed; the issue is what the second call + ;; produces. We assert the security invariants on its result. + (t/is (th/success? prep-out)) + + ;; INVARIANT 1: register-profile must NOT install a session for the + ;; victim. `session/create-fn` is wired via + ;; `rph/with-transform`, which appends to + ;; `:app.rpc/response-transform-fns`. If that vector is non-empty + ;; for an anonymous register that targets an EXISTING profile, the + ;; server is about to mint an `auth-token` cookie bound to the + ;; victim — i.e. account takeover. + (t/is (empty? (:app.rpc/response-transform-fns mdata)) + "register-profile must not create a session for an existing victim profile") + + ;; INVARIANT 2: register-profile must NOT echo back an invitation + ;; token that authenticates as the victim. When the response + ;; contains both `:id` matching the victim and `:invitation-token`, + ;; the frontend treats the user as logged-in for that profile. + (when (and (map? result) + (= (:id victim) (:id result))) + (t/is (not (contains? result :invitation-token)) + "register-profile must not return an invitation-token bound to an existing victim profile")) + + ;; INVARIANT 3: the server must NOT have taken the + ;; "accept-invitation" branch (which is the one that mints a + ;; session). For an existing victim profile, the operation + ;; should fall through to the harmless "repeated registry" path. + (t/is (not= "accept-invitation" + (get-in mdata [:app.loggers.audit/context :action])) + "register-profile must not run the accept-invitation branch for an existing victim profile") + ;; The victim must remain inactive: nothing in this anonymous + ;; flow should have flipped `is-active` to true. + (let [reloaded (th/db-get :profile {:id (:id victim)})] + (t/is (false? (:is-active reloaded)) + "register-profile must not activate the victim profile"))))) + +(t/deftest verify-email-with-invitation-token-propagates-it + ;; A `:verify-email` JWE that carries `:invitation-token` (as + ;; produced by `register-profile` for the not-active+invitation + ;; case) must propagate that token through the verify-token RPC + ;; result so the frontend can resume the team-invitation flow. + (let [profile (th/create-profile* 1 {:is-active false}) + itoken (tokens/generate th/*system* + {:iss :team-invitation + :exp (ct/in-future "48h") + :role :editor + :team-id uuid/zero + :member-email (:email profile)}) + vtoken (tokens/generate th/*system* + {:iss :verify-email + :exp (ct/in-future "72h") + :profile-id (:id profile) + :email (:email profile) + :invitation-token itoken}) + + out (th/command! {::th/type :verify-token + :token vtoken}) + result (:result out)] + + (t/is (th/success? out)) + (t/is (= :verify-email (:iss result))) + (t/is (= itoken (:invitation-token result)) + "verify-token must echo back the invitation-token from the verify-email JWE") + + ;; And the profile must now be active. + (let [reloaded (th/db-get :profile {:id (:id profile)})] + (t/is (true? (:is-active reloaded)))))) + (t/deftest email-change-request (with-mocks [mock {:target 'app.email/send! :return nil}] (let [profile (th/create-profile* 1) diff --git a/frontend/src/app/main/ui/auth/register.cljs b/frontend/src/app/main/ui/auth/register.cljs index 917b272dd9..623b25d642 100644 --- a/frontend/src/app/main/ui/auth/register.cljs +++ b/frontend/src/app/main/ui/auth/register.cljs @@ -81,6 +81,7 @@ on-error (mf/use-fn (fn [cause] + (reset! submitted? false) (let [{:keys [type code] :as edata} (ex-data cause)] (condp = [type code] [:restriction :email-does-not-match-invitation] @@ -98,6 +99,9 @@ [:restriction :email-has-complaints] (st/emit! (ntf/error (tr "errors.email-has-permanent-bounces" (:email edata)))) + [:validation :email-already-exists] + (st/emit! (ntf/error (tr "errors.email-already-exists"))) + [:validation :email-as-password] (swap! form assoc-in [:errors :password] {:message (tr "errors.email-as-password")}) diff --git a/frontend/src/app/main/ui/auth/verify_token.cljs b/frontend/src/app/main/ui/auth/verify_token.cljs index 16e818e4b2..c401ac8f67 100644 --- a/frontend/src/app/main/ui/auth/verify_token.cljs +++ b/frontend/src/app/main/ui/auth/verify_token.cljs @@ -25,11 +25,19 @@ (defmulti handle-token (fn [token] (:iss token))) (defmethod handle-token :verify-email - [data] + [{:keys [invitation-token] :as data}] (cf/external-notify-register-success (:profile-id data)) (let [msg (tr "dashboard.notifications.email-verified-successfully")] (ts/schedule 1000 #(st/emit! (ntf/success msg))) - (st/emit! (da/login-from-token data)))) + ;; If the verify-email JWE carries an :invitation-token, it means + ;; the user registered via a team-invitation flow but had to verify + ;; their email first. Log them in and then redirect to + ;; :auth-verify-token with the invitation token, which will accept + ;; the invitation as a logged-in user. + (if invitation-token + (st/emit! (da/login-from-token data) + (rt/nav :auth-verify-token {:token invitation-token})) + (st/emit! (da/login-from-token data))))) (defmethod handle-token :change-email [_data]