Return status 400 on invalid file names

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))))))))

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)))))))