thumbnail: Do not Camo old thumbor URLs; serve images directly.

Providing a signed Camo URL for arbitrary URLs opened the server up to
being an open redirector.  Return 403 if the URL is not a user upload,
and the backend image if it is.  Since we do not have ImageAttachment
rows for uploads at a time we wrote `/thumbnail?` URLs, return the
full-size content.

zerver/lib/thumbnail.py

@@ -5,15 +5,12 @@ from collections.abc import Iterator
 from contextlib import contextmanager
 from dataclasses import dataclass
 from typing import TypeVar
-from urllib.parse import urljoin
 
 import pyvips
 from bs4 import BeautifulSoup
-from django.utils.http import url_has_allowed_host_and_scheme
 from django.utils.translation import gettext as _
 from typing_extensions import override
 
-from zerver.lib.camo import get_camo_url
 from zerver.lib.exceptions import ErrorCode, JsonableError
 from zerver.lib.queue import queue_event_on_commit
 from zerver.models import AbstractAttachment, ImageAttachment
@@ -139,14 +136,6 @@ class BadImageError(JsonableError):
     code = ErrorCode.BAD_IMAGE
 
 
-def generate_thumbnail_url(path: str, size: str = "0x0") -> str:
-    path = urljoin("/", path)
-
-    if url_has_allowed_host_and_scheme(path, allowed_hosts=None):
-        return path
-    return get_camo_url(path)
-
-
 @contextmanager
 def libvips_check_image(image_data: bytes) -> Iterator[pyvips.Image]:
     # The primary goal of this is to verify that the image is valid,

zerver/tests/test_thumbnail.py

@@ -51,8 +51,8 @@ class ThumbnailRedirectEndpointTest(ZulipTestCase):
         self.assertEqual(base, url[: len(base)])
 
         result = self.client_get("/thumbnail", {"url": url[1:], "size": "full"})
-        self.assertEqual(result.status_code, 302, result)
+        self.assertEqual(result.status_code, 200)
-        self.assertEqual(url, result["Location"])
+        self.assertEqual(result.getvalue(), b"zulip!")
 
         self.login("iago")
         result = self.client_get("/thumbnail", {"url": url[1:], "size": "full"})
@@ -62,21 +62,15 @@ class ThumbnailRedirectEndpointTest(ZulipTestCase):
     def test_thumbnail_external_redirect(self) -> None:
         url = "https://www.google.com/images/srpr/logo4w.png"
         result = self.client_get("/thumbnail", {"url": url, "size": "full"})
-        self.assertEqual(result.status_code, 302, result)
+        self.assertEqual(result.status_code, 403)
-        base = "https://external-content.zulipcdn.net/external_content/56c362a24201593891955ff526b3b412c0f9fcd2/68747470733a2f2f7777772e676f6f676c652e636f6d2f696d616765732f737270722f6c6f676f34772e706e67"
-        self.assertEqual(base, result["Location"])
 
         url = "http://www.google.com/images/srpr/logo4w.png"
         result = self.client_get("/thumbnail", {"url": url, "size": "full"})
-        self.assertEqual(result.status_code, 302, result)
+        self.assertEqual(result.status_code, 403)
-        base = "https://external-content.zulipcdn.net/external_content/7b6552b60c635e41e8f6daeb36d88afc4eabde79/687474703a2f2f7777772e676f6f676c652e636f6d2f696d616765732f737270722f6c6f676f34772e706e67"
-        self.assertEqual(base, result["Location"])
 
         url = "//www.google.com/images/srpr/logo4w.png"
         result = self.client_get("/thumbnail", {"url": url, "size": "full"})
-        self.assertEqual(result.status_code, 302, result)
+        self.assertEqual(result.status_code, 403)
-        base = "https://external-content.zulipcdn.net/external_content/676530cf4b101d56f56cc4a37c6ef4d4fd9b0c03/2f2f7777772e676f6f676c652e636f6d2f696d616765732f737270722f6c6f676f34772e706e67"
-        self.assertEqual(base, result["Location"])
 
     @override_settings(RATE_LIMITING=True)
     def test_thumbnail_redirect_for_spectator(self) -> None:
@@ -99,7 +93,8 @@ class ThumbnailRedirectEndpointTest(ZulipTestCase):
 
             self.logout()
             response = self.client_get("/thumbnail", {"url": url[1:], "size": "full"})
-            self.assertEqual(response.status_code, 403)
+            self.assertEqual(response.status_code, 302)
+            self.assertTrue(response["Location"].startswith("/accounts/login/?next="))
 
             # Allow file access for web-public stream
             self.login("hamlet")
@@ -110,12 +105,13 @@ class ThumbnailRedirectEndpointTest(ZulipTestCase):
 
             self.logout()
             response = self.client_get("/thumbnail", {"url": url[1:], "size": "full"})
-            self.assertEqual(response.status_code, 302)
+            self.assertEqual(response.status_code, 200)
 
         # Deny file access since rate limited
         with ratelimit_rule(86400, 0, domain="spectator_attachment_access_by_file"):
             response = self.client_get("/thumbnail", {"url": url[1:], "size": "full"})
-            self.assertEqual(response.status_code, 403)
+            self.assertEqual(response.status_code, 302)
+            self.assertTrue(response["Location"].startswith("/accounts/login/?next="))
 
         # Deny random file access
         response = self.client_get(
@@ -125,7 +121,7 @@ class ThumbnailRedirectEndpointTest(ZulipTestCase):
                 "size": "full",
             },
         )
-        self.assertEqual(response.status_code, 403)
+        self.assertEqual(response.status_code, 404)
 
 
 class ThumbnailEmojiTest(ZulipTestCase):

zerver/views/development/camo.py

@@ -1,8 +1,10 @@
+from urllib.parse import urljoin
+
 from django.http import HttpRequest, HttpResponse, HttpResponseForbidden
 from django.shortcuts import redirect
+from django.utils.http import url_has_allowed_host_and_scheme
 
 from zerver.lib.camo import is_camo_url_valid
-from zerver.lib.thumbnail import generate_thumbnail_url
 
 
 def handle_camo_url(
@@ -10,6 +12,9 @@ def handle_camo_url(
 ) -> HttpResponse:  # nocoverage
     original_url = bytes.fromhex(received_url).decode()
     if is_camo_url_valid(digest, original_url):
-        return redirect(generate_thumbnail_url(original_url))
+        original_url = urljoin("/", original_url)
+        if url_has_allowed_host_and_scheme(original_url, allowed_hosts=None):
+            return redirect(original_url)
+        return HttpResponseForbidden("<p>Not a valid URL.</p>")
     else:
         return HttpResponseForbidden("<p>Not a valid URL.</p>")

zerver/views/thumbnail.py

@@ -1,28 +1,11 @@
+import re
+
 from django.contrib.auth.models import AnonymousUser
-from django.http import HttpRequest, HttpResponse, HttpResponseForbidden
+from django.http import HttpRequest, HttpResponseBase, HttpResponseForbidden
-from django.shortcuts import redirect
-from django.utils.translation import gettext as _
 
-from zerver.context_processors import get_valid_realm_from_request
-from zerver.lib.attachments import validate_attachment_request
-from zerver.lib.thumbnail import generate_thumbnail_url
 from zerver.lib.typed_endpoint import typed_endpoint
-from zerver.models import Realm, UserProfile
+from zerver.models import UserProfile
-
+from zerver.views.upload import serve_file
-
-def validate_thumbnail_request(
-    realm: Realm,
-    maybe_user_profile: UserProfile | AnonymousUser,
-    path: str,
-) -> bool | None:
-    # path here does not have a leading / as it is parsed from request hitting the
-    # thumbnail endpoint (defined in urls.py) that way.
-    if path.startswith("user_uploads/"):
-        path_id = path[len("user_uploads/") :]
-        return validate_attachment_request(maybe_user_profile, path_id, realm)
-
-    # This is an external link and we don't enforce restricted view policy here.
-    return True
 
 
 @typed_endpoint
@@ -32,15 +15,22 @@ def backend_serve_thumbnail(
     *,
     url: str,
     size: str,
-) -> HttpResponse:
+) -> HttpResponseBase:
-    if not maybe_user_profile.is_authenticated:
+    # This URL used to be passed arbitrary URLs, and pass them through
-        realm = get_valid_realm_from_request(request)
+    # Camo; we no longer support doing so, and instead return a 403.
-    else:
+    #
-        assert isinstance(maybe_user_profile, UserProfile)
+    # Modern thumbnailing uses URLs of the style
-        realm = maybe_user_profile.realm
+    # `/user_uploads/thumbnail/.../300x200.webp`; this endpoint is
+    # kept for backward compatibility, and for future extension for
+    # thumbnailing external URLs.
+    upload_path_parts = re.match(r"user_uploads/(\d+)/(.*)", url)
+    if not upload_path_parts:
+        return HttpResponseForbidden()
 
-    if not validate_thumbnail_request(realm, maybe_user_profile, url):
+    realm_id_str = upload_path_parts[1]
-        return HttpResponseForbidden(_("<p>You are not authorized to view this file.</p>"))
+    path_id = upload_path_parts[2]
 
-    thumbnail_url = generate_thumbnail_url(url)
+    # We do not have ImageAttachment rows for historical uploads, so
-    return redirect(thumbnail_url)
+    # we cannot serve a "new" thumbnail for these requests; serve the
+    # full-size file.
+    return serve_file(request, maybe_user_profile, realm_id_str, path_id)