🐛 Apply the asset attachment disposition on the s3 backend too (#10989)

29dbf9ab1 marks non public buckets as attachments, which works on the fs
backend because nginx applies those headers to the internally redirected
response. On the s3 backend the handler answers 307 and the client then
fetches the bytes from the object store, so the header set on the redirect
does not reach the response that carries the object.

Sign the disposition into the presigned url as well, so the object store
returns it. It is only signed when the bucket is not public, so urls for
inline served objects are unchanged.

Also cover the disposition in the handler tests, for the non public buckets
and for the public ones that stay inline.

Co-authored-by: Andrey Antukh <niwi@niwi.nz>

backend/src/app/http/assets.clj

@@ -49,13 +49,21 @@
   [{:keys [::sto/storage ::signature-max-age ::cache-max-age] :as cfg} obj]
   (let [sig-max-age (or signature-max-age default-signature-max-age)
         cch-max-age (or cache-max-age default-cache-max-age)
-        {:keys [host port] :as url} (sto/get-object-url storage obj {:max-age sig-max-age})
         bucket  (-> obj meta :bucket)
+        public? (contains? public-buckets bucket)
+        ;; The disposition is also signed into the presigned url: this
+        ;; response is a redirect, so the header below applies to the
+        ;; redirect itself and not to the bytes the client then fetches
+        ;; from the object store.
+        {:keys [host port] :as url} (sto/get-object-url storage obj
+                                                        (cond-> {:max-age sig-max-age}
+                                                          (not public?)
+                                                          (assoc :content-disposition "attachment")))
         headers (cond-> {"location" (str url)
                          "x-host"   (cond-> host port (str ":" port))
                          "x-mtype"  (-> obj meta :content-type)
                          "cache-control" (str "max-age=" (inst-ms cch-max-age))}
-                  (not (contains? public-buckets bucket))
+                  (not public?)
                   (assoc "content-disposition" "attachment"))]
     {::yres/status  307
      ::yres/headers headers}))

backend/src/app/storage/s3.clj

@@ -346,13 +346,21 @@
   (ct/duration {:minutes 10}))
 
 (defn- get-object-url
-  [{:keys [::presigner ::bucket ::prefix]} {:keys [id]} {:keys [max-age] :or {max-age default-max-age}}]
+  [{:keys [::presigner ::bucket ::prefix]} {:keys [id]}
+   {:keys [max-age content-disposition] :or {max-age default-max-age}}]
   (assert (ct/duration? max-age) "expected valid duration instance")
 
-  (let [gor  (.. (GetObjectRequest/builder)
+  ;; The content-disposition option is signed into the presigned url, so the
+  ;; object store sets that header on the response the client fetches after
+  ;; following the redirect. It is only set when asked for, so urls for
+  ;; objects served inline stay byte identical to before.
+  (let [gorb (.. (GetObjectRequest/builder)
                  (bucket bucket)
-                 (key (dm/str prefix (impl/id->path id)))
-                 (build))
+                 (key (dm/str prefix (impl/id->path id))))
+        gorb (cond-> gorb
+               (some? content-disposition)
+               (.responseContentDisposition ^String content-disposition))
+        gor  (.build gorb)
         gopr (.. (GetObjectPresignRequest/builder)
                  (signatureDuration ^Duration max-age)
                  (getObjectRequest ^GetObjectRequest gor)

backend/test/backend_tests/http_assets_test.clj

@@ -269,6 +269,50 @@
     (t/is (string? redirect))
     (t/is (clojure.string/includes? redirect (sto/object->relative-path object)))))
 
+;; ----------------------------------------------------------------
+;; Tests: objects-handler — content disposition
+;; ----------------------------------------------------------------
+
+(t/deftest objects-handler-non-public-bucket-served-as-attachment
+  ;; A non-public bucket holds bytes the user uploaded and is reachable by
+  ;; direct navigation, so the response marks it as an attachment.
+  (let [storage  (-> (:app.storage/storage th/*system*)
+                     (configure-storage-backend))
+        cfg      (make-handler-cfg storage)
+        profile  (th/create-profile* 1)]
+
+    (doseq [bucket ["profile"
+                    "tempfile"
+                    "file-data"
+                    "file-thumbnail"
+                    "file-change"]]
+      (t/testing (str "bucket: " bucket)
+        (let [object   (create-storage-object! storage bucket "some data")
+              request  {:path-params {:id (str (:id object))}
+                        ::session/profile-id (:id profile)}
+              response (assets/objects-handler cfg request)]
+          (t/is (= "attachment" (get (::yres/headers response) "content-disposition"))
+                (str "bucket " bucket " should be served as an attachment")))))))
+
+(t/deftest objects-handler-public-bucket-served-inline
+  ;; Public buckets are embedded by the viewer and by outgoing mail, so they
+  ;; keep being served without a disposition.
+  (let [storage  (-> (:app.storage/storage th/*system*)
+                     (configure-storage-backend))
+        cfg      (make-handler-cfg storage)]
+
+    (doseq [bucket ["file-media-object"
+                    "file-object-thumbnail"
+                    "team-font-variant"
+                    "file-data-fragment"
+                    "organization"]]
+      (t/testing (str "bucket: " bucket)
+        (let [object   (create-storage-object! storage bucket "some data")
+              request  {:path-params {:id (str (:id object))}}
+              response (assets/objects-handler cfg request)]
+          (t/is (nil? (get (::yres/headers response) "content-disposition"))
+                (str "bucket " bucket " should stay inline")))))))
+
 ;; ----------------------------------------------------------------
 ;; Tests: objects-handler — cache headers
 ;; ----------------------------------------------------------------