uploads: Set X-Accel-Redirect manually, without using django-sendfile2.

The `django-sendfile2` module unfortunately only supports a single
`SENDFILE` root path -- an invariant which subsequent commits need to
break.  Especially as Zulip only runs with a single webserver, and
thus sendfile backend, the functionality is simple to inline.

It is worth noting that the following headers from the initial Django
response are _preserved_, if present, and sent unmodified to the
client; all other headers are overridden by those supplied by the
internal redirect[^1]:
 - Content-Type
 - Content-Disposition
 - Accept-Ranges
 - Set-Cookie
 - Cache-Control
 - Expires

As such, we explicitly unset the Content-type header to allow nginx to
set it from the static file, but set Content-Disposition and
Cache-Control as we want them to be.

[^1]: https://www.nginx.com/resources/wiki/start/topics/examples/xsendfile/

puppet/zulip/manifests/app_frontend_base.pp

@@ -71,7 +71,7 @@ class zulip::app_frontend_base {
 
   # Configuration for how uploaded files and profile pictures are
   # served.  The default is to serve uploads using using the `nginx`
-  # `internal` feature via django-sendfile2, which basically does an
+  # `internal` feature via X-Accel-Redirect, which basically does an
   # internal redirect and returns the file content from nginx in an
   # HttpResponse that would otherwise have been a redirect.  Profile
   # pictures are served directly off disk.

pyproject.toml

@@ -66,7 +66,6 @@ module = [
     "django_cte.*",
     "django_otp.*",
     "django_scim.*",
-    "django_sendfile.*",
     "django_statsd.*",
     "DNS.*",
     "fakeldap.*",

requirements/common.in

@@ -137,9 +137,6 @@ django-two-factor-auth[call,phonenumberslite,sms]
 # Needed for processing payments (in corporate)
 stripe
 
-# Needed for serving uploaded files from nginx but perform auth checks in django.
-django-sendfile2
-
 # For checking whether email of the user is from a disposable email provider.
 disposable-email-domains
 

requirements/dev.txt

@@ -447,7 +447,6 @@ django[argon2]==4.1.5 \
     #   django-otp
     #   django-phonenumber-field
     #   django-scim2
-    #   django-sendfile2
     #   django-stubs
     #   django-stubs-ext
     #   django-two-factor-auth
@@ -481,10 +480,6 @@ django-scim2==0.18.0 \
     --hash=sha256:5055099fdbfa55b46488cece7b378225263265ae4acd1669c47b2c286d2cfbb2 \
     --hash=sha256:f3353df68b469f494a5c7f53bb487411125a08be77d2ccc2d4c048138895614c
     # via -r requirements/common.in
-django-sendfile2==0.7.0 \
-    --hash=sha256:0ee17b4f7ce8cc7159f75fa4e5d62e7795c1217de8f1e52ee6265d4aa46dce03 \
-    --hash=sha256:d900b1557cb1ba881798728de7ed7c82bff808868f331136b867a106e73bcd1f
-    # via -r requirements/common.in
 django-statsd-mozilla==0.4.0 \
     --hash=sha256:0d87cb63de8107279cbb748caad9aa74c6a44e7e96ccc5dbf07b89f77285a4b8 \
     --hash=sha256:81084f3d426f5184f0a0f1dbfe035cc26b66f041d2184559d916a228d856f0d3

requirements/prod.txt

@@ -299,7 +299,6 @@ django[argon2]==4.1.5 \
     #   django-otp
     #   django-phonenumber-field
     #   django-scim2
-    #   django-sendfile2
     #   django-stubs-ext
     #   django-two-factor-auth
 django-auth-ldap==4.1.0 \
@@ -332,10 +331,6 @@ django-scim2==0.18.0 \
     --hash=sha256:5055099fdbfa55b46488cece7b378225263265ae4acd1669c47b2c286d2cfbb2 \
     --hash=sha256:f3353df68b469f494a5c7f53bb487411125a08be77d2ccc2d4c048138895614c
     # via -r requirements/common.in
-django-sendfile2==0.7.0 \
-    --hash=sha256:0ee17b4f7ce8cc7159f75fa4e5d62e7795c1217de8f1e52ee6265d4aa46dce03 \
-    --hash=sha256:d900b1557cb1ba881798728de7ed7c82bff808868f331136b867a106e73bcd1f
-    # via -r requirements/common.in
 django-statsd-mozilla==0.4.0 \
     --hash=sha256:0d87cb63de8107279cbb748caad9aa74c6a44e7e96ccc5dbf07b89f77285a4b8 \
     --hash=sha256:81084f3d426f5184f0a0f1dbfe035cc26b66f041d2184559d916a228d856f0d3

version.py

@@ -48,4 +48,4 @@ API_FEATURE_LEVEL = 159
 #   historical commits sharing the same major version, in which case a
 #   minor version bump suffices.
 
-PROVISION_VERSION = (218, 1)
+PROVISION_VERSION = (219, 0)

zerver/lib/rest.py

@@ -1,7 +1,7 @@
 from functools import wraps
 from typing import Callable, Dict, Set, Tuple, Union
 
-from django.http import HttpRequest, HttpResponse
+from django.http import HttpRequest, HttpResponse, HttpResponseBase
 from django.urls import path
 from django.urls.resolvers import URLPattern
 from django.utils.cache import add_never_cache_headers
@@ -205,6 +205,9 @@ def rest_dispatch(request: HttpRequest, /, **kwargs: object) -> HttpResponse:
 
 def rest_path(
     route: str,
-    **handlers: Union[Callable[..., HttpResponse], Tuple[Callable[..., HttpResponse], Set[str]]],
+    **handlers: Union[
+        Callable[..., HttpResponseBase],
+        Tuple[Callable[..., HttpResponseBase], Set[str]],
+    ],
 ) -> URLPattern:
     return path(route, rest_dispatch, handlers)

zerver/lib/test_runner.py

@@ -248,7 +248,6 @@ def initialize_worker_path(worker_id: int) -> None:
             "test_uploads",
         )
     )
-    settings.SENDFILE_ROOT = os.path.join(settings.LOCAL_UPLOADS_DIR, "files")
 
 
 class Runner(DiscoverRunner):

zerver/tests/test_upload.py

@@ -15,7 +15,6 @@ from django.conf import settings
 from django.http.response import StreamingHttpResponse
 from django.test import override_settings
 from django.utils.timezone import now as timezone_now
-from django_sendfile.utils import _get_sendfile
 from PIL import Image
 from urllib3 import encode_multipart_formdata
 
@@ -934,33 +933,29 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
             content_disposition: str = "",
             download: bool = False,
         ) -> None:
-            with self.settings(SENDFILE_BACKEND="django_sendfile.backends.nginx"):
+            self.login("hamlet")
-                _get_sendfile.cache_clear()  # To clearout cached version of backend from djangosendfile
+            fp = StringIO("zulip!")
-                self.login("hamlet")
+            fp.name = name
-                fp = StringIO("zulip!")
+            result = self.client_post("/json/user_uploads", {"file": fp})
-                fp.name = name
+            uri = self.assert_json_success(result)["uri"]
-                result = self.client_post("/json/user_uploads", {"file": fp})
+            fp_path_id = re.sub("/user_uploads/", "", uri)
-                uri = self.assert_json_success(result)["uri"]
+            fp_path = os.path.split(fp_path_id)[0]
-                fp_path_id = re.sub("/user_uploads/", "", uri)
+            if download:
-                fp_path = os.path.split(fp_path_id)[0]
+                uri = uri.replace("/user_uploads/", "/user_uploads/download/")
-                if download:
+            with self.settings(DEVELOPMENT=False):
-                    uri = uri.replace("/user_uploads/", "/user_uploads/download/")
                 response = self.client_get(uri)
-                _get_sendfile.cache_clear()
+            assert settings.LOCAL_UPLOADS_DIR is not None
-                assert settings.LOCAL_UPLOADS_DIR is not None
+            test_run, worker = os.path.split(os.path.dirname(settings.LOCAL_UPLOADS_DIR))
-                test_run, worker = os.path.split(os.path.dirname(settings.LOCAL_UPLOADS_DIR))
+            self.assertEqual(
-                self.assertEqual(
+                response["X-Accel-Redirect"],
-                    response["X-Accel-Redirect"],
+                "/serve_uploads/" + fp_path + "/" + name_str_for_test,
-                    "/serve_uploads/" + fp_path + "/" + name_str_for_test,
+            )
-                )
+            if content_disposition != "":
-                if content_disposition != "":
+                self.assertIn("attachment;", response["Content-disposition"])
-                    self.assertIn("attachment;", response["Content-disposition"])
+                self.assertIn(content_disposition, response["Content-disposition"])
-                    self.assertIn(content_disposition, response["Content-disposition"])
+            else:
-                else:
+                self.assertIn("inline;", response["Content-disposition"])
-                    self.assertIn("inline;", response["Content-disposition"])
+            self.assertEqual(set(response["Cache-Control"].split(", ")), {"private", "immutable"})
-                self.assertEqual(
-                    set(response["Cache-Control"].split(", ")), {"private", "immutable"}
-                )
 
         check_xsend_links("zulip.txt", "zulip.txt", 'filename="zulip.txt"')
         check_xsend_links(

zerver/views/upload.py

@@ -6,11 +6,17 @@ from urllib.parse import quote, urlparse
 from django.conf import settings
 from django.contrib.auth.models import AnonymousUser
 from django.core.files.uploadedfile import UploadedFile
-from django.http import HttpRequest, HttpResponse, HttpResponseForbidden, HttpResponseNotFound
+from django.http import (
+    FileResponse,
+    HttpRequest,
+    HttpResponse,
+    HttpResponseBase,
+    HttpResponseForbidden,
+    HttpResponseNotFound,
+)
 from django.shortcuts import redirect
 from django.utils.cache import patch_cache_control
 from django.utils.translation import gettext as _
-from django_sendfile import sendfile
 
 from zerver.context_processors import get_valid_realm_from_request
 from zerver.lib.exceptions import JsonableError
@@ -57,6 +63,26 @@ 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:
+    # 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:
+    #  - Content-Type
+    #  - Content-Disposition
+    #  - Accept-Ranges
+    #  - 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()
+    response["X-Accel-Redirect"] = internal_path
+    del response["Content-Type"]
+    return response
+
+
 def serve_s3(
     request: HttpRequest, url_path: str, url_only: bool, download: bool = False
 ) -> HttpResponse:
@@ -69,7 +95,7 @@ def serve_s3(
 
 def serve_local(
     request: HttpRequest, path_id: str, url_only: bool, download: bool = False
-) -> HttpResponse:
+) -> HttpResponseBase:
     local_path = get_local_file_path(path_id)
     if local_path is None:
         return HttpResponseNotFound("<p>File not found</p>")
@@ -81,20 +107,23 @@ def serve_local(
     mimetype, encoding = guess_type(local_path)
     attachment = download or mimetype not in INLINE_MIME_TYPES
 
-    response = sendfile(
+    if settings.DEVELOPMENT:
-        request, local_path, attachment=attachment, mimetype=mimetype, encoding=encoding
+        # In development, we do not have the nginx server to offload
-    )
+        # the response to; serve it directly ourselves.
-    patch_cache_control(response, private=True, immutable=True)
+        # FileResponse handles setting Content-Disposition, etc.
-    # sendfile adds a content-disposition header, but it incorrectly
+        response: HttpResponseBase = FileResponse(open(local_path, "rb"), as_attachment=attachment)
-    # slash-escapes Unicode filenames; Django has a correct
+        patch_cache_control(response, private=True, immutable=True)
-    # implementation, but it is not easily callable until Django 4.2.
+        return response
+
+    response = internal_nginx_redirect(quote(f"/serve_uploads/{path_id}"))
     patch_disposition_header(response, local_path, attachment)
+    patch_cache_control(response, private=True, immutable=True)
     return response
 
 
 def serve_file_download_backend(
     request: HttpRequest, user_profile: UserProfile, realm_id_str: str, filename: str
-) -> HttpResponse:
+) -> HttpResponseBase:
     return serve_file(request, user_profile, realm_id_str, filename, url_only=False, download=True)
 
 
@@ -103,13 +132,13 @@ def serve_file_backend(
     maybe_user_profile: Union[UserProfile, AnonymousUser],
     realm_id_str: str,
     filename: str,
-) -> HttpResponse:
+) -> HttpResponseBase:
     return serve_file(request, maybe_user_profile, realm_id_str, filename, url_only=False)
 
 
 def serve_file_url_backend(
     request: HttpRequest, user_profile: UserProfile, realm_id_str: str, filename: str
-) -> HttpResponse:
+) -> HttpResponseBase:
     """
     We should return a signed, short-lived URL
     that the client can use for native mobile download, rather than serving a redirect.
@@ -125,7 +154,7 @@ def serve_file(
     filename: str,
     url_only: bool = False,
     download: bool = False,
-) -> HttpResponse:
+) -> HttpResponseBase:
     path_id = f"{realm_id_str}/{filename}"
     realm = get_valid_realm_from_request(request)
     is_authorized = validate_attachment_request(maybe_user_profile, path_id, realm)
@@ -140,7 +169,7 @@ def serve_file(
     return serve_s3(request, path_id, url_only, download=download)
 
 
-def serve_local_file_unauthed(request: HttpRequest, token: str, filename: str) -> HttpResponse:
+def serve_local_file_unauthed(request: HttpRequest, token: str, filename: str) -> HttpResponseBase:
     path_id = get_local_file_path_id_from_token(token)
     if path_id is None:
         raise JsonableError(_("Invalid token"))

zproject/computed_settings.py

@@ -40,7 +40,6 @@ from .configured_settings import (
     EXTRA_INSTALLED_APPS,
     GOOGLE_OAUTH2_CLIENT_ID,
     IS_DEV_DROPLET,
-    LOCAL_UPLOADS_DIR,
     MEMCACHED_LOCATION,
     MEMCACHED_USERNAME,
     RATE_LIMITING_RULES,
@@ -50,7 +49,6 @@ from .configured_settings import (
     REMOTE_POSTGRES_PORT,
     REMOTE_POSTGRES_SSLMODE,
     ROOT_SUBDOMAIN_ALIASES,
-    SENDFILE_BACKEND,
     SENTRY_DSN,
     SOCIAL_AUTH_APPLE_APP_ID,
     SOCIAL_AUTH_APPLE_SERVICES_ID,
@@ -440,12 +438,6 @@ ROOT_DOMAIN_URI = EXTERNAL_URI_SCHEME + EXTERNAL_HOST
 S3_KEY = get_secret("s3_key")
 S3_SECRET_KEY = get_secret("s3_secret_key")
 
-if LOCAL_UPLOADS_DIR is not None:
-    if SENDFILE_BACKEND is None:
-        SENDFILE_BACKEND = "django_sendfile.backends.nginx"
-    SENDFILE_ROOT = os.path.join(LOCAL_UPLOADS_DIR, "files")
-    SENDFILE_URL = "/serve_uploads"
-
 # GCM tokens are IP-whitelisted; if we deploy to additional
 # servers you will need to explicitly add their IPs here:
 # https://cloud.google.com/console/project/apps~zulip-android/apiui/credential

zproject/default_settings.py

@@ -171,7 +171,6 @@ REMOTE_POSTGRES_HOST = ""
 REMOTE_POSTGRES_PORT = ""
 REMOTE_POSTGRES_SSLMODE = ""
 THUMBNAIL_IMAGES = False
-SENDFILE_BACKEND: Optional[str] = None
 
 TORNADO_PORTS: List[int] = []
 USING_TORNADO = True

zproject/dev_settings.py

@@ -102,9 +102,6 @@ PASSWORD_MIN_GUESSES = 0
 TWO_FACTOR_CALL_GATEWAY = "two_factor.gateways.fake.Fake"
 TWO_FACTOR_SMS_GATEWAY = "two_factor.gateways.fake.Fake"
 
-# Make sendfile use django to serve files in development
-SENDFILE_BACKEND = "django_sendfile.backends.development"
-
 # Set this True to send all hotspots in development
 ALWAYS_SEND_ALL_HOTSPOTS = False