Misc fixes after review

- Make CPS variant mirror the sync error handling.
- Move tests to existing test file.
- Rewrite tests to avoid `with-redefs`.
- Make error handling customizable.customizable.

Author: expez

ring-core/src/ring/middleware/multipart_params.clj

@@ -137,6 +137,9 @@
                  {:multipart-params params}
                  {:params params}))))
 
+(defn default-invalid-filename-handler [request e]
+  (response/bad-request (.getMessage e)))
+
 (defn wrap-multipart-params
   "Middleware to parse multipart parameters from a request. Adds the
   following keys to the request map:
@@ -146,40 +149,47 @@
 
   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."
+  :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 function that gets called when the file being uploaded
+                              has an invalid name. The function should expect two
+                              parameters: request and an exception of type
+                              InvalidFileNameException."
   ([handler]
    (wrap-multipart-params handler {}))
   ([handler options]
    (fn
      ([request]
-      (try
-        (handler (multipart-params-request request options))
-        (catch InvalidFileNameException e
-          (response/bad-request (.getMessage e)))))
+      (let [invalid-file-name-handler
+            (:invalid-filename-handler options default-invalid-filename-handler)]
+        (try
+          (handler (multipart-params-request request options))
+          (catch InvalidFileNameException e
+            (invalid-file-name-handler request e)))))
      ([request respond raise]
-      (try
-        (handler (multipart-params-request request options) respond raise)
-        (catch InvalidFileNameException e
-          (handler request
-                   (fn [_] (respond (response/bad-request (.getMessage e))))
-                   raise)))))))
+      (let [invalid-file-name-handler
+            (:invalid-filename-handler options default-invalid-filename-handler)]
+        (try
+          (handler (multipart-params-request request options) respond raise)
+          (catch InvalidFileNameException e
+            (respond (invalid-file-name-handler request e)))))))))

ring-core/test/ring/middleware/multipart_params/test/multipart_params.clj

@@ -2,7 +2,8 @@
   (:require [clojure.test :refer :all]
             [ring.middleware.multipart-params :refer :all]
             [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
+            e))
+        err-response (default-invalid-filename-handler
+                       :unused-arg 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)))))))