From d263c23a58986e9d651f711f83419c9d3b17b0a8 Mon Sep 17 00:00:00 2001 From: Pablo Alba Date: Wed, 9 Sep 2026 11:23:53 +0200 Subject: [PATCH] :bug: Add ssrf check for nitrate sso and add timeouts to http client (#11576) --- backend/src/app/http/client.clj | 11 ++-- backend/src/app/main.clj | 2 +- backend/src/app/media/remote.clj | 6 +-- backend/src/app/rpc/management/nitrate.clj | 10 +++- .../test/backend_tests/http_client_test.clj | 30 +++++++++++ .../test/backend_tests/media_remote_test.clj | 17 ++++++ .../rpc_management_nitrate_test.clj | 53 +++++++++++++------ 7 files changed, 103 insertions(+), 26 deletions(-) create mode 100644 backend/test/backend_tests/http_client_test.clj diff --git a/backend/src/app/http/client.clj b/backend/src/app/http/client.clj index bba77f9aa0..891e581f70 100644 --- a/backend/src/app/http/client.clj +++ b/backend/src/app/http/client.clj @@ -15,6 +15,7 @@ (:require [app.common.schema :as sm] [app.util.ssrf :as ssrf] + [app.worker :as-alias wrk] [cuerdas.core :as str] [integrant.core :as ig] [java-http-clj.core :as http]) @@ -23,6 +24,8 @@ java.net.URI)) (def default-max-redirects 5) +(def default-connect-timeout 30000) +(def default-request-timeout 30000) (defn client? [o] @@ -33,15 +36,17 @@ :pred client?}) (defmethod ig/init-key ::client - [_ _] - (http/build-client {:connect-timeout 30000 + [_ {:keys [::wrk/executor]}] + (http/build-client {:connect-timeout default-connect-timeout + :executor executor :follow-redirects :never})) (defn send! ([client req] (send! client req {})) ([client req {:keys [response-type] :or {response-type :string}}] (assert (client? client) "expected valid http client") - (http/send req {:client client :as response-type}))) + (http/send (merge {:timeout default-request-timeout} req) + {:client client :as response-type}))) (defn- resolve-client [params] diff --git a/backend/src/app/main.clj b/backend/src/app/main.clj index d0ffd1cf58..cc627f3307 100644 --- a/backend/src/app/main.clj +++ b/backend/src/app/main.clj @@ -200,7 +200,7 @@ {::db/pool (ig/ref ::db/pool)} ::http.client/client - {} + {::wrk/executor (ig/ref ::wrk/executor)} ::session/manager {::db/pool (ig/ref ::db/pool)} diff --git a/backend/src/app/media/remote.clj b/backend/src/app/media/remote.clj index 0b5a0a4a42..9e717b2fd7 100644 --- a/backend/src/app/media/remote.clj +++ b/backend/src/app/media/remote.clj @@ -75,10 +75,10 @@ {:method method :uri uri :body body - :headers headers} + :headers headers + :timeout timeout} {:response-type :input-stream - :skip-ssrf-check? true - :timeout timeout}) + :skip-ssrf-check? true}) status (:status resp)] (when (not (<= 200 status 299)) (let [body (:body resp)] diff --git a/backend/src/app/rpc/management/nitrate.clj b/backend/src/app/rpc/management/nitrate.clj index a470f2451e..4224ca7264 100644 --- a/backend/src/app/rpc/management/nitrate.clj +++ b/backend/src/app/rpc/management/nitrate.clj @@ -41,6 +41,7 @@ [app.rpc.notifications :as notifications] [app.storage :as sto] [app.util.services :as sv] + [app.util.ssrf :as ssrf] [app.worker :as wrk] [cuerdas.core :as str])) @@ -960,13 +961,18 @@ RETURNING id, deleted_at;") (sv/defmethod ::check-organization-sso "Validate an organization SSO configuration by generating a login redirect URL. Nitrate calls this while configuring SSO to verify client credentials and OIDC - discovery before saving the settings." + discovery before saving the settings. The issuer URL is nitrate-supplied + (customer-configured), so it is checked against the SSRF blocklist before + any outbound request is attempted." {::doc/added "2.18" ::sm/params cto/schema:nitrate-sso ::sm/result schema:check-organization-sso-result ::rpc/auth false} [cfg params] - {:valid (oidc/is-organization-sso-config-valid? cfg params)}) + (let [issuer (oidc/organization-sso-discovery-uri params)] + {:valid (boolean (and issuer + (ssrf/safe-url? issuer) + (oidc/is-organization-sso-config-valid? cfg params)))})) ;; ---- API: notify-organization-sso-change (sv/defmethod ::notify-organization-sso-change diff --git a/backend/test/backend_tests/http_client_test.clj b/backend/test/backend_tests/http_client_test.clj new file mode 100644 index 0000000000..d67edcf182 --- /dev/null +++ b/backend/test/backend_tests/http_client_test.clj @@ -0,0 +1,30 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns backend-tests.http-client-test + (:require + [app.http.client :as http] + [clojure.test :as t] + [java-http-clj.core :as jhttp] + [mockery.core :refer [with-mocks]])) + +(t/deftest send-injects-default-timeout-when-absent + (with-mocks [mock {:target 'java-http-clj.core/send + :return {:status 200 :body ""}}] + (let [client (jhttp/build-client {})] + (http/send! client {:method :get :uri "https://example.com/"}) + (let [[req _opts] (:call-args @mock)] + (t/is (= http/default-request-timeout (:timeout req))))))) + +(t/deftest send-preserves-caller-supplied-timeout + (with-mocks [mock {:target 'java-http-clj.core/send + :return {:status 200 :body ""}}] + (let [client (jhttp/build-client {})] + (http/send! client {:method :get + :uri "https://example.com/" + :timeout 5000}) + (let [[req _opts] (:call-args @mock)] + (t/is (= 5000 (:timeout req))))))) \ No newline at end of file diff --git a/backend/test/backend_tests/media_remote_test.clj b/backend/test/backend_tests/media_remote_test.clj index dfa8b16d05..21eb46bec8 100644 --- a/backend/test/backend_tests/media_remote_test.clj +++ b/backend/test/backend_tests/media_remote_test.clj @@ -8,6 +8,7 @@ (:require [app.common.exceptions :as ex] [app.config :as cf] + [app.http.client :as http] [app.media.remote :as media.remote] [app.setup :as-alias setup] [app.util.json :as json] @@ -500,6 +501,22 @@ :headers {}})] (t/is (= 200 (:status resp)))))))) +(t/deftest service-request-puts-configured-timeout-in-request + (t/testing "service-request puts media-processing-service-timeout on the http request" + (let [captured (atom nil)] + (with-redefs [cf/get (th/config-get-mock config-mock) + http/req (fn [_client request _opts] + (reset! captured request) + {:status 200 + :body (json-stream {:width 100 :height 100})})] + (media.remote/service-request + (mk-system) + {:method :post + :uri "http://localhost:6065/api/image/info" + :body nil + :headers {}}) + (t/is (= 5000 (:timeout @captured))))))) + ;; --------------------------------------------------------------------------- ;; Shared key ;; --------------------------------------------------------------------------- diff --git a/backend/test/backend_tests/rpc_management_nitrate_test.clj b/backend/test/backend_tests/rpc_management_nitrate_test.clj index da78b2e733..1589026db1 100644 --- a/backend/test/backend_tests/rpc_management_nitrate_test.clj +++ b/backend/test/backend_tests/rpc_management_nitrate_test.clj @@ -17,6 +17,7 @@ [app.msgbus :as mbus] [app.nitrate :as nitrate] [app.rpc :as-alias rpc] + [app.util.ssrf :as ssrf] [app.worker :as wrk] [backend-tests.helpers :as th] [clojure.set :as set] @@ -1806,13 +1807,14 @@ (t/deftest check-organization-sso-returns-valid-true (let [organization-id (uuid/random) - out (with-redefs [oidc/is-organization-sso-config-valid? (constantly true)] - (th/management-command! - {::th/type :check-organization-sso - :organization-id organization-id - :client-id "test-client" - :client-secret "test-secret" - :issuer "https://idp.example.com"}))] + out (with-redefs [ssrf/safe-url? (constantly true) + oidc/is-organization-sso-config-valid? (constantly true)] + (th/management-command! + {::th/type :check-organization-sso + :organization-id organization-id + :client-id "test-client" + :client-secret "test-secret" + :issuer "https://idp.example.com"}))] (t/is (th/success? out)) (t/is (true? (-> out :result :valid))))) @@ -1827,19 +1829,36 @@ (t/deftest check-organization-sso-passes-issuer-to-validation (let [organization-id (uuid/random) - out (with-redefs [oidc/is-organization-sso-config-valid? - (fn [_cfg sso] - (and (= "test-client" (:client-id sso)) - (= "https://idp.example.com/" (:issuer sso))))] - (th/management-command! - {::th/type :check-organization-sso - :organization-id organization-id - :client-id "test-client" - :client-secret "test-secret" - :issuer "https://idp.example.com/"}))] + out (with-redefs [ssrf/safe-url? (constantly true) + oidc/is-organization-sso-config-valid? + (fn [_cfg sso] + (and (= "test-client" (:client-id sso)) + (= "https://idp.example.com/" (:issuer sso))))] + (th/management-command! + {::th/type :check-organization-sso + :organization-id organization-id + :client-id "test-client" + :client-secret "test-secret" + :issuer "https://idp.example.com/"}))] (t/is (th/success? out)) (t/is (true? (-> out :result :valid))))) +(t/deftest check-organization-sso-returns-valid-false-on-ssrf-blocked-issuer + (t/testing "an SSRF-blocked issuer must not reach the OIDC validation flow" + (let [called? (atom false) + out (with-redefs [oidc/is-organization-sso-config-valid? + (fn [_cfg _sso] (reset! called? true) true)] + (th/management-command! + {::th/type :check-organization-sso + :organization-id (uuid/random) + :client-id "test-client" + :client-secret "test-secret" + :issuer "http://127.0.0.1/idp"}))] + (t/is (th/success? out)) + (t/is (false? (-> out :result :valid))) + (t/is (false? @called?) + "OIDC validation should not run when the issuer is SSRF-blocked")))) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; PUSH AUDIT EVENTS ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;