From 7bf0e7abeca0515a7fb4b5d220b5bfc788577f93 Mon Sep 17 00:00:00 2001 From: Lars Andersen Date: Thu, 23 Aug 2018 10:58:13 +0200 Subject: [PATCH] Return status 400 on invalid file names The multipart middleware would throw an exception when it encountered an invalid filename. For most apps that exception would be caught by the default exception handler and result in a status code of 500 instead of 400. This commit catches this specific exception and returns a 400 response from the server instead. The error response is made customizable through the optional :invalid-filename-handler. --- .../src/ring/middleware/multipart_params.clj | 94 +++++++++++++------ .../ring/middleware/test/multipart_params.clj | 36 ++++++- 2 files changed, 98 insertions(+), 32 deletions(-) diff --git a/ring-core/src/ring/middleware/multipart_params.clj b/ring-core/src/ring/middleware/multipart_params.clj index e2831f59..b65772ad 100644 --- a/ring-core/src/ring/middleware/multipart_params.clj +++ b/ring-core/src/ring/middleware/multipart_params.clj @@ -9,11 +9,13 @@ ring.middleware.multipart-params.temp-file/temp-file-store" (:require [ring.util.codec :refer [assoc-conj]] [ring.util.request :as req] + [ring.util.response :as response] [ring.util.parsing :refer [re-charset]]) (:import [org.apache.commons.fileupload UploadContext FileItemIterator FileItemStream FileUpload + InvalidFileNameException ProgressListener] [org.apache.commons.io IOUtils])) (defn- progress-listener @@ -135,41 +137,73 @@ {:multipart-params params} {:params params})))) +(defn default-invalid-filename-handler + ([request] + (response/bad-request + (str "Invalid filename in multipart upload: " + (::invalid-filename request)))) + ([request respond raise] + (respond (default-invalid-filename-handler request)))) + +(defn- add-invalid-filename-to-req + [request ^InvalidFileNameException invalid-filename-exception] + (assoc request ::invalid-filename (.getName invalid-filename-exception))) + (defn wrap-multipart-params - "Middleware to parse multipart parameters from a request. Adds the - following keys to the request map: + "Middleware to parse multipart parameters from a request. Adds the following + keys to the request map: :multipart-params - a map of multipart parameters :params - a merged map of all types of parameter - The following options are accepted - - :encoding - character encoding to use for multipart parsing. - Overrides the encoding specified in the request. If not - specified, uses the encoding specified in a part named - \"_charset_\", or the content type for each part, or - request character encoding if the part has no encoding, - or \"UTF-8\" if no request character encoding is set. - - :fallback-encoding - specifies the character encoding used in parsing if a - part of the request does not specify encoding in its - content type or no part named \"_charset_\" is present. - Has no effect if :encoding is also set. - - :store - a function that stores a file upload. The function - should expect a map with :filename, :content-type and - :stream keys, and its return value will be used as the - value for the parameter in the multipart parameter map. - The default storage function is the temp-file-store. - - :progress-fn - a function that gets called during uploads. The - function should expect four parameters: request, - bytes-read, content-length, and item-count." + The following options are accepted: + + :encoding + - Character encoding to use for multipart parsing. Overrides the encoding + specified in the request. If not specified,uses the encoding specified in a + part named \"_charset_\", or the content type for each part, or request + character encoding if the part has no encoding, or \"UTF-8\" if no request + character encoding is set. + + :fallback-encoding + - Specifies the character encoding used in parsing if a part of the request + does not specify encoding in its content type or no part named \"_charset_\" + is present. Has no effect if :encoding is also set. + + :store + - A function that stores a file upload. The function should expect a map with + :filename, :content-type and :stream keys, and its return value will be used + as the value for the parameter in the multipart parameter map. The default + storage function is the temp-file-store. + + :progress-fn + - A function that gets called during uploads. The function should expect four + parameters: request, bytes-read, content-length, and item-count. + + :invalid-filename-handler + - A handler that gets called when the file being uploaded has an invalid name. + When this handler receives the request it can expect one additional key, + ::invalid-filename, which contains the name of the invalid file." ([handler] (wrap-multipart-params handler {})) ([handler options] - (fn - ([request] - (handler (multipart-params-request request options))) - ([request respond raise] - (handler (multipart-params-request request options) respond raise))))) + (let [invalid-filename-handler + (:invalid-filename-handler options default-invalid-filename-handler)] + (fn ([request] + (let [req-or-ex (try + (multipart-params-request request options) + (catch InvalidFileNameException ex ex))] + (if (instance? InvalidFileNameException req-or-ex) + (-> request + (add-invalid-filename-to-req req-or-ex) + invalid-filename-handler) + (handler req-or-ex)))) + ([request respond raise] + (let [req-or-ex (try + (multipart-params-request request options) + (catch InvalidFileNameException ex ex))] + (if (instance? InvalidFileNameException req-or-ex) + (-> request + (add-invalid-filename-to-req req-or-ex) + (invalid-filename-handler respond raise)) + (handler req-or-ex respond raise)))))))) diff --git a/ring-core/test/ring/middleware/test/multipart_params.clj b/ring-core/test/ring/middleware/test/multipart_params.clj index 88b3aac1..a194d5c8 100644 --- a/ring-core/test/ring/middleware/test/multipart_params.clj +++ b/ring-core/test/ring/middleware/test/multipart_params.clj @@ -1,8 +1,9 @@ (ns ring.middleware.test.multipart-params (:require [clojure.test :refer :all] - [ring.middleware.multipart-params :refer :all] + [ring.middleware.multipart-params :refer :all :as multipart-params] [ring.middleware.multipart-params.byte-array :refer :all] - [ring.util.io :refer [string-input-stream]])) + [ring.util.io :refer [string-input-stream]]) + (:import org.apache.commons.fileupload.InvalidFileNameException)) (defn string-store [item] (-> (select-keys item [:filename :content-type]) @@ -175,3 +176,34 @@ :body (string-input-stream form-body "ISO-8859-15")} request* (multipart-params-request request {:fallback-encoding "ISO-8859-15"})] (is (= (get-in request* [:multipart-params "foo"]) "äÄÖöÅå€")))) + +(deftest wrap-multipart-params-error-test + (let [invalid-filename (str "foo" \u0000 ".bar") + form-body (str "--XXXX\r\n" + "Content-Disposition: form-data;" + "name=foo; filename=" invalid-filename "\r\n" + "Content-Type: text/plain\r\n\r\n" + "foo\r\n" + "--XXXX--") + request {:headers {"content-type" + (str "multipart/form-data; boundary=XXXX; charset=US-ASCII")} + :body (string-input-stream form-body "ISO-8859-1")} + invalid-filename-exception + (try + (org.apache.commons.fileupload.util.Streams/checkFileName invalid-filename) + (catch Exception e (println (.getName e)) e)) + err-response (default-invalid-filename-handler + {::multipart-params/invalid-filename + (.getName invalid-filename-exception)})] + (testing "Synchronous error response" + (let [handler (wrap-multipart-params identity)] + (is (= err-response (handler request))))) + (testing "Asynchronous error response" + (let [handler (wrap-multipart-params (fn [req respond _] + (respond req))) + response (promise) + exception (promise) + request (assoc request :body (string-input-stream form-body "ISO-8859-1"))] + (handler request response exception) + (is (= err-response (deref response 2000 :timed-out))) + (is (not (realized? exception)))))))