From 70030fa9e365dfe7f5c18ee7cd34889c8e26052a Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Tue, 10 Mar 2026 10:04:07 +0100 Subject: [PATCH] :bug: Fix download-image to properly handle network errors and non-2xx responses (#8554) The download-image function in app.media silently succeeded when the remote image URL was unreachable or returned an error status code, causing create-file-media-object-from-url to report success with no actual image stored. Add exception handling for connection refused, timeouts, and I/O errors around the HTTP request, and validate the HTTP status code in parse-and-validate before processing the response body. Fixes #8499 Signed-off-by: Andrey Antukh --- backend/src/app/media.clj | 36 ++++++- backend/test/backend_tests/rpc_media_test.clj | 102 +++++++++++++++++- 2 files changed, 133 insertions(+), 5 deletions(-) diff --git a/backend/src/app/media.clj b/backend/src/app/media.clj index a1d22c0832..00b6db36d4 100644 --- a/backend/src/app/media.clj +++ b/backend/src/app/media.clj @@ -293,12 +293,17 @@ (defn download-image "Download an image from the provided URI and return the media input object" [{:keys [::http/client]} uri] - (letfn [(parse-and-validate [{:keys [headers] :as response}] + (letfn [(parse-and-validate [{:keys [status headers] :as response}] (let [size (some-> (get headers "content-length") d/parse-integer) mtype (get headers "content-type") format (cm/mtype->format mtype) max-size (cf/get :media-max-file-size default-max-file-size)] + (when-not (<= 200 status 299) + (ex/raise :type :validation + :code :unable-to-download-image + :hint (str/ffmt "unable to download image from '%': unexpected status code %" uri status))) + (when-not size (ex/raise :type :validation :code :unknown-size @@ -318,9 +323,32 @@ {:size size :mtype mtype :format format}))] - (let [{:keys [body] :as response} (http/req! client - {:method :get :uri uri} - {:response-type :input-stream}) + (let [{:keys [body] :as response} + (try + (http/req! client + {:method :get :uri uri} + {:response-type :input-stream}) + (catch java.net.ConnectException cause + (ex/raise :type :validation + :code :unable-to-download-image + :hint (str/ffmt "unable to download image from '%': connection refused or host unreachable" uri) + :cause cause)) + (catch java.net.http.HttpConnectTimeoutException cause + (ex/raise :type :validation + :code :unable-to-download-image + :hint (str/ffmt "unable to download image from '%': connection timeout" uri) + :cause cause)) + (catch java.net.http.HttpTimeoutException cause + (ex/raise :type :validation + :code :unable-to-download-image + :hint (str/ffmt "unable to download image from '%': request timeout" uri) + :cause cause)) + (catch java.io.IOException cause + (ex/raise :type :validation + :code :unable-to-download-image + :hint (str/ffmt "unable to download image from '%': I/O error" uri) + :cause cause))) + {:keys [size mtype]} (parse-and-validate response) path (tmp/tempfile :prefix "penpot.media.download.") written (io/write* path body :size size)] diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index d583565f39..79df6d38b4 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -9,11 +9,14 @@ [app.common.time :as ct] [app.common.uuid :as uuid] [app.db :as db] + [app.http.client :as http] + [app.media :as media] [app.rpc :as-alias rpc] [app.storage :as sto] [backend-tests.helpers :as th] [clojure.test :as t] - [datoteka.fs :as fs])) + [datoteka.fs :as fs] + [mockery.core :refer [with-mocks]])) (t/use-fixtures :once th/state-init) (t/use-fixtures :each th/database-reset) @@ -278,3 +281,100 @@ error-data (ex-data error)] (t/is (th/ex-info? error)) (t/is (= (:type error-data) :not-found))))) + + +(t/deftest download-image-connection-error + (t/testing "connection refused raises validation error" + (with-mocks [http-mock {:target 'app.http.client/req! + :throw (java.net.ConnectException. "Connection refused")}] + (let [cfg {::http/client :mock-client} + err (try + (media/download-image cfg "http://unreachable.invalid/image.png") + nil + (catch clojure.lang.ExceptionInfo e e))] + (t/is (some? err)) + (t/is (= :validation (:type (ex-data err)))) + (t/is (= :unable-to-download-image (:code (ex-data err))))))) + + (t/testing "connection timeout raises validation error" + (with-mocks [http-mock {:target 'app.http.client/req! + :throw (java.net.http.HttpConnectTimeoutException. "Connect timed out")}] + (let [cfg {::http/client :mock-client} + err (try + (media/download-image cfg "http://unreachable.invalid/image.png") + nil + (catch clojure.lang.ExceptionInfo e e))] + (t/is (some? err)) + (t/is (= :validation (:type (ex-data err)))) + (t/is (= :unable-to-download-image (:code (ex-data err))))))) + + (t/testing "request timeout raises validation error" + (with-mocks [http-mock {:target 'app.http.client/req! + :throw (java.net.http.HttpTimeoutException. "Request timed out")}] + (let [cfg {::http/client :mock-client} + err (try + (media/download-image cfg "http://unreachable.invalid/image.png") + nil + (catch clojure.lang.ExceptionInfo e e))] + (t/is (some? err)) + (t/is (= :validation (:type (ex-data err)))) + (t/is (= :unable-to-download-image (:code (ex-data err))))))) + + (t/testing "I/O error raises validation error" + (with-mocks [http-mock {:target 'app.http.client/req! + :throw (java.io.IOException. "Stream closed")}] + (let [cfg {::http/client :mock-client} + err (try + (media/download-image cfg "http://unreachable.invalid/image.png") + nil + (catch clojure.lang.ExceptionInfo e e))] + (t/is (some? err)) + (t/is (= :validation (:type (ex-data err)))) + (t/is (= :unable-to-download-image (:code (ex-data err)))))))) + + +(t/deftest download-image-status-code-error + (t/testing "404 status raises validation error" + (with-mocks [http-mock {:target 'app.http.client/req! + :return {:status 404 + :headers {"content-type" "text/html" + "content-length" "0"} + :body nil}}] + (let [cfg {::http/client :mock-client} + err (try + (media/download-image cfg "http://example.com/not-found.png") + nil + (catch clojure.lang.ExceptionInfo e e))] + (t/is (some? err)) + (t/is (= :validation (:type (ex-data err)))) + (t/is (= :unable-to-download-image (:code (ex-data err))))))) + + (t/testing "500 status raises validation error" + (with-mocks [http-mock {:target 'app.http.client/req! + :return {:status 500 + :headers {"content-type" "text/html" + "content-length" "0"} + :body nil}}] + (let [cfg {::http/client :mock-client} + err (try + (media/download-image cfg "http://example.com/server-error.png") + nil + (catch clojure.lang.ExceptionInfo e e))] + (t/is (some? err)) + (t/is (= :validation (:type (ex-data err)))) + (t/is (= :unable-to-download-image (:code (ex-data err))))))) + + (t/testing "302 status raises validation error" + (with-mocks [http-mock {:target 'app.http.client/req! + :return {:status 302 + :headers {"content-type" "text/html" + "content-length" "0"} + :body nil}}] + (let [cfg {::http/client :mock-client} + err (try + (media/download-image cfg "http://example.com/redirect.png") + nil + (catch clojure.lang.ExceptionInfo e e))] + (t/is (some? err)) + (t/is (= :validation (:type (ex-data err)))) + (t/is (= :unable-to-download-image (:code (ex-data err))))))))