diff --git a/zerver/tests/test_upload_s3.py b/zerver/tests/test_upload_s3.py index 1e2424c67c..73204cbc75 100644 --- a/zerver/tests/test_upload_s3.py +++ b/zerver/tests/test_upload_s3.py @@ -308,6 +308,31 @@ class S3Test(ZulipTestCase): body = f"First message ...[zulip.txt](http://{hamlet.realm.host}" + url + ")" self.send_stream_message(hamlet, "Denmark", body, "test") + @use_s3_backend + def test_user_uploads_empty_file(self) -> None: + bucket = create_s3_buckets(settings.S3_AUTH_UPLOADS_BUCKET)[0] + + self.login("hamlet") + fp = StringIO("") + fp.name = "empty-file.txt" + + result = self.client_post("/json/user_uploads", {"file": fp}) + response_dict = self.assert_json_success(result) + self.assertIn("url", response_dict) + url = response_dict["url"] + + # Despite S3 being configured, we serve the 0-byte file directly + response = self.client_get(url) + self.assertEqual(response.status_code, 200) + self.assertEqual(response.headers["Content-Type"], "text/plain") + self.assertEqual(response.headers["Content-Length"], "0") + self.assertEqual( + response.headers["Content-Disposition"], 'inline; filename="empty-file.txt"' + ) + self.assertEqual(response.headers["Cache-Control"], "private, immutable") + key = url.removeprefix("/user_uploads/") + self.assertEqual(b"", bucket.Object(key).get()["Body"].read()) + @use_s3_backend def test_user_avatars_base(self) -> None: backend = zerver.lib.upload.upload_backend diff --git a/zerver/views/upload.py b/zerver/views/upload.py index 321b653752..dc7e0df536 100644 --- a/zerver/views/upload.py +++ b/zerver/views/upload.py @@ -81,8 +81,34 @@ def internal_nginx_redirect(internal_path: str, content_type: str | None = None) def serve_s3( - request: HttpRequest, path_id: str, filename: str, force_download: bool = False + request: HttpRequest, + path_id: str, + attachment: Attachment, + *, + filename: str | None = None, + force_download: bool = False, ) -> HttpResponse: + if filename is None: + filename = attachment.file_name + + # Save going to S3 for 0-byte files -- in part for performance, to + # save a round-trip to S3, but also because S3 can erroneously + # return HTTP 416 errors when a `Range: 0-5242879` header is sent + # (as nginx does by default with the `slice` directive) for a + # 0-byte file. + if attachment.size == 0: + content_type = maybe_add_charset( + attachment.content_type or guess_type(filename)[0] or "application/octet-stream", + attachment_source(path_id), + ) + download = force_download or bare_content_type(content_type) not in INLINE_MIME_TYPES + + response = HttpResponse("", content_type=content_type) + response.headers["Content-Length"] = "0" + patch_cache_control(response, private=True, immutable=True) + patch_disposition_header(response, filename, download) + return response + url = get_signed_upload_url(path_id, filename, force_download=force_download) assert url.startswith("https://") @@ -344,7 +370,13 @@ def serve_file( content_type=content_type, ) else: - return serve_s3(request, path_id, served_filename, force_download=force_download) + return serve_s3( + request, + path_id, + attachment, + filename=served_filename, + force_download=force_download, + ) USER_UPLOADS_ACCESS_TOKEN_SALT = "user_uploads_" @@ -392,7 +424,7 @@ def serve_file_unauthed_from_token( content_type=attachment.content_type, ) else: - return serve_s3(request, path_id, attachment.file_name) + return serve_s3(request, path_id, attachment) def serve_local_avatar_unauthed(request: HttpRequest, path: str) -> HttpResponseBase: