From ff63668c1ef61928878cded13bc1c2734c983aff Mon Sep 17 00:00:00 2001 From: Pablo Alba Date: Mon, 7 Sep 2026 15:59:35 +0200 Subject: [PATCH] :bug: Enforce SSRF checks and add timeouts to HTTP client (#11474) --- backend/src/app/http/client.clj | 11 ++- backend/src/app/main.clj | 2 +- backend/src/app/media/remote.clj | 6 +- backend/src/app/nitrate.clj | 3 +- .../test/backend_tests/http_client_test.clj | 30 ++++++++ .../test/backend_tests/media_remote_test.clj | 17 +++++ .../test/backend_tests/nitrate_ssrf_test.clj | 70 +++++++++++++++++++ 7 files changed, 130 insertions(+), 9 deletions(-) create mode 100644 backend/test/backend_tests/http_client_test.clj create mode 100644 backend/test/backend_tests/nitrate_ssrf_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/nitrate.clj b/backend/src/app/nitrate.clj index a189116458..5adafaeced 100644 --- a/backend/src/app/nitrate.clj +++ b/backend/src/app/nitrate.clj @@ -68,8 +68,7 @@ "x-profile-id" (str profile-id)} :uri uri :version :http1.1} - (= method :post) (assoc :body (json/encode request-params :key-fn json/write-camel-key))) - {:skip-ssrf-check? true}))) + (= method :post) (assoc :body (json/encode request-params :key-fn json/write-camel-key)))))) (defn- with-retries [handler max-retries] 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/nitrate_ssrf_test.clj b/backend/test/backend_tests/nitrate_ssrf_test.clj new file mode 100644 index 0000000000..e44f96356a --- /dev/null +++ b/backend/test/backend_tests/nitrate_ssrf_test.clj @@ -0,0 +1,70 @@ +;; 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.nitrate-ssrf-test + (:require + [app.config :as cf] + [app.http.client :as http] + [app.nitrate :as nitrate] + [app.setup :as-alias setup] + [clojure.string :as str] + [clojure.test :as t] + [integrant.core :as ig] + [java-http-clj.core :as jhttp])) + +(def ^:private private-admin-uri "http://127.0.0.1:9090") + +(defn- mk-cfg + "Minimal nitrate cfg with a real HttpClient and nitrate client methods." + [] + (let [http-client (jhttp/build-client {}) + base {::http/client http-client + ::setup/shared-keys {:admin-console "test-shared-key"}}] + (assoc base ::nitrate/client (ig/init-key ::nitrate/client base)))) + +(defn- with-admin-console-uri + "Run `f` with :admin-console enabled and the given admin-console URI / allowlist." + [admin-uri allowed-hosts f] + (let [original-get cf/get] + (with-redefs [cf/flags #{:admin-console} + cf/get (fn [key & args] + (case key + :admin-console-uri admin-uri + :ssrf-allowed-hosts allowed-hosts + (apply original-get key args)))] + (f)))) + +(t/deftest nitrate-blocks-private-admin-console-uri + (let [sent? (atom false)] + (with-admin-console-uri + private-admin-uri + #{} + (fn [] + (with-redefs [jhttp/send (fn [_req _opts] + (reset! sent? true) + {:status 200 :body "{\"licenses\":true}"})] + (try + (nitrate/call (mk-cfg) :connectivity {}) + (t/is false "should have raised :nitrate-unavailable") + (catch Exception e + (t/is (= :nitrate-unavailable (:type (ex-data e)))) + (t/is (false? @sent?) + "SSRF must stop the request before it reaches the network")))))))) + +(t/deftest nitrate-proceeds-when-admin-console-host-allowlisted + (let [captured (atom nil)] + (with-admin-console-uri + private-admin-uri + #{"127.0.0.1"} + (fn [] + (with-redefs [jhttp/send (fn [req _opts] + (reset! captured req) + {:status 200 + :body "{\"licenses\":true}"})] + (let [result (nitrate/call (mk-cfg) :connectivity {})] + (t/is (= {:licenses true} result)) + (t/is (some? @captured)) + (t/is (str/starts-with? (str (:uri @captured)) private-admin-uri))))))))