From 17042f7bea19a564766e0ebd65cebf3a7c462d7f Mon Sep 17 00:00:00 2001 From: Gennadiy Ivashchenko Date: Fri, 21 Aug 2026 17:13:37 +0500 Subject: [PATCH] :recycle: Use uniform URI building pattern for OIDC redirects Replace the ad-hoc `public-uri-with-path` helper with `build-public-uri` that uses the standard `u/ensure-path-slash` + `u/join` pattern already used throughout the backend and frontend. This makes OIDC redirect URI construction consistent with the rest of the codebase while correctly preserving subpath configuration. For hash routes (frontend URLs), use `u/append-query-param` to place query params inside the fragment, matching the expected URL structure. Add 6 new tests covering subpath edge cases: - Trailing slash in public-uri - Query params inside hash fragments - No-subpath fallback - Multiple redirect functions with subpaths AI-assisted-by: qwen3.7-plus AI-assisted-by: gpt-5.6-luna --- backend/src/app/auth/oidc.clj | 16 ++++- backend/test/backend_tests/auth_oidc_test.clj | 72 ++++++++++++++++++- 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 1a309c1d5e..6de189c711 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -453,10 +453,22 @@ [fitem & items] (str/split path separator)] (into [(keyword (:type provider) (str/kebab fitem))] (map keyword) items))) +(defn- build-public-uri + "Join a relative path onto the configured public URI, preserving any + subpath. Uses `u/ensure-path-slash` + `u/join` to match the pattern + used throughout the backend and frontend. + `path` must not start with `/` — a leading slash would be treated as + absolute by `u/join`, silently dropping the configured subpath." + [path] + {:pre [(not (str/starts-with? path "/"))]} + (-> (cf/get :public-uri) + (u/uri) + (u/ensure-path-slash) + (u/join path))) + (defn- build-redirect-uri [] - (let [public (u/uri (cf/get :public-uri))] - (str (assoc public :path "/api/auth/oidc/callback")))) + (str (build-public-uri "api/auth/oidc/callback"))) (defn build-auth-redirect-uri [provider token] diff --git a/backend/test/backend_tests/auth_oidc_test.clj b/backend/test/backend_tests/auth_oidc_test.clj index 8fcbbe5c4f..3c18d2eb3a 100644 --- a/backend/test/backend_tests/auth_oidc_test.clj +++ b/backend/test/backend_tests/auth_oidc_test.clj @@ -157,6 +157,13 @@ (t/is (.contains loc "error=auth-error")) (t/is (not (.contains loc "hint="))))))) +(t/deftest redirect-with-error-preserves-public-uri-subpath + (binding [cf/config {:public-uri "http://localhost:3449/penpot"}] + (let [result (#'oidc/redirect-with-error "auth-error") + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449/penpot?screen=auth-login&")) + (t/is (.contains loc "error=auth-error"))))) + (t/deftest redirect-to-verify-token-builds-verify-url (binding [cf/config {:public-uri "http://localhost:3449"}] (let [result (#'oidc/redirect-to-verify-token "test-token-value") @@ -165,11 +172,72 @@ (t/is (.contains loc "http://localhost:3449?screen=auth-verify-token&")) (t/is (.contains loc "token=test-token-value"))))) +(t/deftest redirect-to-verify-token-preserves-public-uri-subpath + (binding [cf/config {:public-uri "http://localhost:3449/penpot"}] + (let [result (#'oidc/redirect-to-verify-token "test-token-value") + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449/penpot?screen=auth-verify-token&"))))) + (t/deftest build-redirect-uri-constructs-redirect (binding [cf/config {:public-uri "http://localhost:3449"}] (t/is (= "http://localhost:3449/api/auth/oidc/callback" (#'oidc/build-redirect-uri))))) +(t/deftest build-redirect-uri-preserves-public-uri-subpath + (binding [cf/config {:public-uri "http://localhost:3449/penpot"}] + (t/is (= "http://localhost:3449/penpot/api/auth/oidc/callback" + (#'oidc/build-redirect-uri))))) + +(t/deftest build-redirect-uri-with-subpath-and-trailing-slash + (binding [cf/config {:public-uri "http://localhost:3449/penpot/"}] + (t/is (= "http://localhost:3449/penpot/api/auth/oidc/callback" + (#'oidc/build-redirect-uri))))) + +(t/deftest redirect-with-error-subpath-screen-query + (binding [cf/config {:public-uri "http://localhost:3449/penpot"}] + (let [result (#'oidc/redirect-with-error "auth-error" "hint message") + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449/penpot?screen=auth-login&")) + (t/is (.contains loc "error=auth-error")) + (t/is (.contains loc "hint=hint")) + ;; Screen routing lives in the query string, not the fragment + (t/is (not (.contains loc "#")))))) + +(t/deftest redirect-to-register-subpath + (let [test-key (byte-array (map byte (range 32))) + cfg {::setup/props {:tokens-key test-key}}] + (binding [cf/config {:public-uri "http://localhost:3449/penpot"}] + (let [info {:email "u@e.com" :fullname "U" :backend "oidc" + :email-verified false :props {}} + provider {:type "oidc" :id "oidc"} + result (#'oidc/redirect-to-register cfg info provider) + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449/penpot?screen=auth-register-validate&")) + (t/is (.contains loc "token=")))))) + +(t/deftest redirect-to-register-no-subpath + (let [test-key (byte-array (map byte (range 32))) + cfg {::setup/props {:tokens-key test-key}}] + (binding [cf/config {:public-uri "http://localhost:3449"}] + (let [info {:email "u@e.com" :fullname "U" :backend "oidc" + :email-verified false :props {}} + provider {:type "oidc" :id "oidc"} + result (#'oidc/redirect-to-register cfg info provider) + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449?screen=auth-register-validate&")) + (t/is (.contains loc "token=")))))) + +(t/deftest redirect-to-verify-token-subpath-no-trailing-slash + (binding [cf/config {:public-uri "http://localhost:3449/my-app"}] + (let [result (#'oidc/redirect-to-verify-token "test-token-value") + loc (get-in result [::yres/headers "location"])] + (t/is (.contains loc "http://localhost:3449/my-app?screen=auth-verify-token&"))))) + +(t/deftest build-public-uri-no-subpath + (binding [cf/config {:public-uri "http://localhost:3449"}] + (t/is (= "http://localhost:3449/api/auth/oidc/callback" + (str (#'oidc/build-public-uri "api/auth/oidc/callback")))))) + (t/deftest fetch-user-info-returns-decoded-body-on-success (let [cfg {} provider {:user-uri "https://provider.example.com/userinfo"} @@ -477,7 +545,7 @@ (let [cfg (dissoc base-cfg :app.email/blacklist :app.email/whitelist) state (make-state-token cfg {}) request (default-request cfg :state state)] - (binding [cf/config {:public-uri "http://localhost:3449"} + (binding [cf/config {:public-uri "http://localhost:3449/penpot"} cf/flags #{:registration}] (with-redefs [app.auth.oidc/resolve-provider (constantly {:type "oidc" :id "oidc"}) app.auth.oidc/get-info (constantly {:email "u@e.com" :fullname "U" @@ -486,7 +554,7 @@ app.auth.oidc/get-profile (constantly (assoc test-profile :is-active false))] (let [result (#'oidc/callback-handler cfg request) loc (redirect-location result)] - (t/is (.contains loc "http://localhost:3449?screen=auth-register-validate&")) + (t/is (.contains loc "http://localhost:3449/penpot?screen=auth-register-validate&")) (t/is (.contains loc "token="))))))) (t/deftest callback-success-flow