From 23894fc9a3f17784a4ef2bf2bc3e5d0590e5d45d Mon Sep 17 00:00:00 2001
From: Alex Vandiver <alexmv@zulip.com>
Date: Wed, 11 Jan 2023 16:43:14 +0000
Subject: [PATCH] uploads: Set Content-Type and -Disposition from Django for
 local files.

Similar to the previous commit, Django was responsible for setting the
Content-Disposition based on the filename, whereas the Content-Type
was set by nginx based on the filename.  This difference is not
exploitable, as even if they somehow disagreed with Django's expected
Content-Type, nginx will only ever respond with Content-Types found in
`uploads.types` -- none of which are unsafe for user-supplied content.

However, for consistency, have Django provide both Content-Type and
Content-Disposition headers.
---
 .../uploads-internal.conf                     |  4 ++-
 zerver/views/upload.py                        | 31 +++++++++++++------
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/puppet/zulip/files/nginx/zulip-include-frontend/uploads-internal.conf b/puppet/zulip/files/nginx/zulip-include-frontend/uploads-internal.conf
index 426f5d6757..9012561d01 100644
--- a/puppet/zulip/files/nginx/zulip-include-frontend/uploads-internal.conf
+++ b/puppet/zulip/files/nginx/zulip-include-frontend/uploads-internal.conf
@@ -50,7 +50,9 @@ location /internal/local/uploads {
     internal;
     include /etc/nginx/zulip-include/headers;
     add_header Content-Security-Policy "default-src 'none'; style-src 'self' 'unsafe-inline'; img-src 'self'; object-src 'self'; plugin-types application/pdf;";
-    include /etc/nginx/zulip-include/uploads.types;
+
+    # Django handles setting Content-Type, Content-Disposition, and Cache-Control.
+
     alias /home/zulip/uploads/files;
 }
 
diff --git a/zerver/views/upload.py b/zerver/views/upload.py
index 25856ee97b..967567a7e9 100644
--- a/zerver/views/upload.py
+++ b/zerver/views/upload.py
@@ -68,7 +68,7 @@ def patch_disposition_header(response: HttpResponse, url: str, is_attachment: bo
     response.headers["Content-Disposition"] = f"{disposition}; {file_expr}"
 
 
-def internal_nginx_redirect(internal_path: str) -> HttpResponse:
+def internal_nginx_redirect(internal_path: str, content_type: Optional[str] = None) -> HttpResponse:
     # The following headers from this initial response are
     # _preserved_, if present, and sent unmodified to the client;
     # all other headers are overridden by the redirected URL:
@@ -78,13 +78,16 @@ def internal_nginx_redirect(internal_path: str) -> HttpResponse:
     #  - Set-Cookie
     #  - Cache-Control
     #  - Expires
-    # As such, we unset the Content-type header to allow nginx to set
-    # it from the static file; the caller can set Content-Disposition
-    # and Cache-Control on this response as they desire, and the
-    # client will see those values.
-    response = HttpResponse()
+    # As such, we default to unsetting the Content-type header to
+    # allow nginx to set it from the static file; the caller can set
+    # Content-Disposition and Cache-Control on this response as they
+    # desire, and the client will see those values.  In some cases
+    # (local files) we do wish to control the Content-Type, so also
+    # support setting it explicitly.
+    response = HttpResponse(content_type=content_type)
     response["X-Accel-Redirect"] = internal_path
-    del response["Content-Type"]
+    if content_type is None:
+        del response["Content-Type"]
     return response
 
 
@@ -133,15 +136,23 @@ def serve_local(
 
     if settings.DEVELOPMENT:
         # In development, we do not have the nginx server to offload
-        # the response to; serve it directly ourselves.
-        # FileResponse handles setting Content-Disposition, etc.
+        # the response to; serve it directly ourselves.  FileResponse
+        # handles setting Content-Type, Content-Disposition, etc.
         response: HttpResponseBase = FileResponse(
             open(local_path, "rb"), as_attachment=download  # noqa: SIM115
         )
         patch_cache_control(response, private=True, immutable=True)
         return response
 
-    response = internal_nginx_redirect(quote(f"/internal/local/uploads/{path_id}"))
+    # For local responses, we are in charge of generating both
+    # Content-Type and Content-Disposition headers; unlike with S3
+    # storage, the Content-Type is not stored with the file in any
+    # way, so Django makes the determination of it, and thus as well
+    # if that type is safe to have a Content-Disposition of "inline".
+    # nginx respects the values we send.
+    response = internal_nginx_redirect(
+        quote(f"/internal/local/uploads/{path_id}"), content_type=mimetype
+    )
     patch_disposition_header(response, local_path, download)
     patch_cache_control(response, private=True, immutable=True)
     return response