Merge pull request #471 from kachayev/fix-non-existing-file

Catch FileNotFoundException when sending file body, do not print exception stacktrace into HTTP response body

Author: kachayev

src/aleph/http/core.clj

@@ -65,6 +65,10 @@
      AtomicBoolean]
     [javax.net.ssl SSLHandshakeException]))
 
+(def ^CharSequence server-name (HttpHeaders/newEntity "Server"))
+(def ^CharSequence connection-name (HttpHeaders/newEntity "Connection"))
+(def ^CharSequence date-name (HttpHeaders/newEntity "Date"))
+
 (def non-standard-keys
   (let [ks ["Content-MD5"
             "ETag"
@@ -275,6 +279,17 @@
 (defn chunked-writer-enabled? [^Channel ch]
   (some? (-> ch netty/channel .pipeline (.get ChunkedWriteHandler))))
 
+(def default-error-response
+  {:status 500
+   :headers {"content-type" "text/plain"}
+   :body "Internal server error"})
+
+;; Logs exception and returns default 500 response
+;; not to expose internal logic to the client
+(defn error-response [^Throwable e]
+  (log/error e "error in HTTP handler")
+  default-error-response)
+
 (defn send-streaming-body [ch ^HttpMessage msg body]
 
   (HttpUtil/setTransferEncodingChunked msg (boolean (not (has-content-length? msg))))
@@ -441,6 +456,33 @@
         (.close raf)
         (.close fc)))))
 
+(defn send-contiguous-body [ch ^HttpMessage msg body]
+  (let [omitted? (identical? :aleph/omitted body)
+        body (if (or (nil? body) omitted?)
+               empty-last-content
+               (DefaultLastHttpContent. (netty/to-byte-buf ch body)))
+        length (-> ^HttpContent body .content .readableBytes)]
+
+    (when-not omitted?
+      (if (instance? HttpResponse msg)
+        (let [code (-> ^HttpResponse msg .status .code)]
+          (when-not (or (<= 100 code 199) (= 204 code))
+            (try-set-content-length! msg length)))
+        (try-set-content-length! msg length)))
+
+    (netty/write ch msg)
+    (netty/write-and-flush ch body)))
+
+(defn send-internal-error [ch ^HttpResponse msg]
+  (let [raw-headers (.headers msg)
+        headers {:server (.get raw-headers server-name)
+                 :connection (.get raw-headers connection-name)
+                 :date (.get raw-headers date-name)}
+        resp (-> default-error-response
+                 (update :headers merge headers))
+        msg' (ring-response->netty-response resp)]
+    (send-contiguous-body ch msg' (:body resp))))
+
 (defn send-chunked-file [ch ^HttpMessage msg ^HttpFile file]
   (let [raf (RandomAccessFile. ^File (.-fd file) "r")
         cf (ChunkedFile. raf
@@ -467,34 +509,17 @@
 (defn send-file-body [ch ssl? ^HttpMessage msg ^HttpFile file]
   (cond
     ssl?
-    (send-streaming-body ch msg
-      (-> file
-        (bs/to-byte-buffers {:chunk-size (.-chunk-size file)})
-        s/->source))
+    (let [source (-> file
+                     (bs/to-byte-buffers {:chunk-size 1e6})
+                     s/->source)]
+      (send-streaming-body ch msg source))
 
     (chunked-writer-enabled? ch)
     (send-chunked-file ch msg file)
 
     :else
     (send-file-region ch msg file)))
 
-(defn send-contiguous-body [ch ^HttpMessage msg body]
-  (let [omitted? (identical? :aleph/omitted body)
-        body (if (or (nil? body) omitted?)
-               empty-last-content
-               (DefaultLastHttpContent. (netty/to-byte-buf ch body)))
-        length (-> ^HttpContent body .content .readableBytes)]
-
-    (when-not omitted?
-      (if (instance? HttpResponse msg)
-        (let [code (-> ^HttpResponse msg .status .code)]
-          (when-not (or (<= 100 code 199) (= 204 code))
-            (try-set-content-length! msg length)))
-        (try-set-content-length! msg length)))
-
-    (netty/write ch msg)
-    (netty/write-and-flush ch body)))
-
 (let [ary-class (class (byte-array 0))
 
       ;; extracted to make `send-message` more inlineable
@@ -511,35 +536,37 @@
   (defn send-message
     [ch keep-alive? ssl? ^HttpMessage msg body]
 
-    (let [f (cond
-
-              (or
-                (nil? body)
-                (identical? :aleph/omitted body)
-                (instance? String body)
-                (instance? ary-class body)
-                (instance? ByteBuffer body)
-                (instance? ByteBuf body))
-              (send-contiguous-body ch msg body)
-
-              (instance? ChunkedInput body)
-              (send-chunked-body ch msg body)
-
-              (instance? File body)
-              (send-file-body ch ssl? msg (http-file body))
-
-              (instance? Path body)
-              (send-file-body ch ssl? msg (http-file body))
-
-              (instance? HttpFile body)
-              (send-file-body ch ssl? msg body)
-
-              :else
-              (let [class-name (.getName (class body))]
-                (try
-                  (send-streaming-body ch msg body)
-                  (catch Throwable e
-                    (log/error e "error sending body of type " class-name)))))]
+    (let [f (try
+              (cond
+                (or
+                 (nil? body)
+                 (identical? :aleph/omitted body)
+                 (instance? String body)
+                 (instance? ary-class body)
+                 (instance? ByteBuffer body)
+                 (instance? ByteBuf body))
+                (send-contiguous-body ch msg body)
+
+                (instance? ChunkedInput body)
+                (send-chunked-body ch msg body)
+
+                (instance? File body)
+                (send-file-body ch ssl? msg (http-file body))
+
+                (instance? Path body)
+                (send-file-body ch ssl? msg (http-file body))
+
+                (instance? HttpFile body)
+                (send-file-body ch ssl? msg body)
+
+                :else
+                (send-streaming-body ch msg body))
+              (catch Throwable e
+                (let [class-name (.getName (class body))]
+                  (log/errorf "error sending body of type %s: %s"
+                              class-name
+                              (.getMessage ^Throwable e)))
+                (send-internal-error ch msg)))]
 
       (when-not keep-alive?
         (handle-cleanup ch f))

src/aleph/http/server.clj

@@ -103,18 +103,7 @@
         TimeUnit/MILLISECONDS)
       (.get ref))))
 
-(defn error-response [^Throwable e]
-  (log/error e "error in HTTP handler")
-  {:status 500
-   :headers {"content-type" "text/plain"}
-   :body (let [w (java.io.StringWriter.)]
-           (.printStackTrace e (java.io.PrintWriter. w))
-           (str w))})
-
-(let [[server-name connection-name date-name]
-      (map #(HttpHeaders/newEntity %) ["Server" "Connection" "Date"])
-
-      [server-value keep-alive-value close-value]
+(let [[server-value keep-alive-value close-value]
       (map #(HttpHeaders/newEntity %) ["Aleph/0.4.6" "Keep-Alive" "Close"])]
   (defn send-response
     [^ChannelHandlerContext ctx keep-alive? ssl? rsp]
@@ -123,27 +112,27 @@
             [(http/ring-response->netty-response rsp)
              (get rsp :body)]
             (catch Throwable e
-              (let [rsp (error-response e)]
+              (let [rsp (http/error-response e)]
                 [(http/ring-response->netty-response rsp)
                  (get rsp :body)])))]
 
       (netty/safe-execute ctx
         (let [headers (.headers rsp)]
 
-          (when-not (.contains headers ^CharSequence server-name)
-            (.set headers ^CharSequence server-name server-value))
+          (when-not (.contains headers ^CharSequence http/server-name)
+            (.set headers ^CharSequence http/server-name server-value))
 
-          (when-not (.contains headers ^CharSequence date-name)
-            (.set headers ^CharSequence date-name (date-header-value ctx)))
+          (when-not (.contains headers ^CharSequence http/date-name)
+            (.set headers ^CharSequence http/date-name (date-header-value ctx)))
 
-          (.set headers ^CharSequence connection-name (if keep-alive? keep-alive-value close-value))
+          (.set headers ^CharSequence http/connection-name (if keep-alive? keep-alive-value close-value))
 
           (http/send-message ctx keep-alive? ssl? rsp body))))))
 
 ;;;
 
 (defn invalid-value-response [req x]
-  (error-response
+  (http/error-response
     (IllegalArgumentException.
       (str "cannot treat "
         (pr-str x)
@@ -174,7 +163,7 @@
                     (try
                       (rejected-handler req')
                       (catch Throwable e
-                        (error-response e)))
+                        (http/error-response e)))
                     {:status 503
                      :headers {"content-type" "text/plain"}
                      :body "503 Service Unavailable"})))
@@ -183,15 +172,15 @@
               (try
                 (handler req')
                 (catch Throwable e
-                  (error-response e))))]
+                  (http/error-response e))))]
 
     (-> previous-response
       (d/chain'
         netty/wrap-future
         (fn [_]
           (netty/release req)
           (-> rsp
-            (d/catch' error-response)
+            (d/catch' http/error-response)
             (d/chain'
               (fn [rsp]
                 (when (not (-> req' ^AtomicBoolean (.websocket?) .get))

test/aleph/http_test.clj

@@ -423,6 +423,26 @@
                   :body
                   bs/to-string)))))
 
+(defn non-existing-file-handler [req]
+  {:status 200
+   :body (File. "this-file-does-not-exist.sure")})
+
+(defn- assert-file-error []
+  (is (= 500 (-> (http/get (str "http://localhost:" port)
+                           {:throw-exceptions? false})
+                 (d/timeout! 1e3)
+                 deref
+                 :status))))
+
+(deftest test-sending-non-existing-file
+  (testing "sending FileRegion"
+    (with-handler non-existing-file-handler
+      (assert-file-error)))
+
+  (testing "sending chunked body"
+    (with-compressed-handler non-existing-file-handler
+      (assert-file-error))))
+
 ;;;
 
 (defn get-netty-client-event-threads []