From c726d2ec014543d8e7fd2b675d95b5bf50d84605 Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Wed, 24 Jul 2024 20:35:18 +0000 Subject: [PATCH] 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 | 11 ------- zerver/tests/test_thumbnail.py | 26 +++++++-------- zerver/views/development/camo.py | 9 ++++-- zerver/views/thumbnail.py | 54 +++++++++++++------------------- 4 files changed, 40 insertions(+), 60 deletions(-) diff --git a/zerver/lib/thumbnail.py b/zerver/lib/thumbnail.py index e6d3475899..5980f90d26 100644 --- a/zerver/lib/thumbnail.py +++ b/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, diff --git a/zerver/tests/test_thumbnail.py b/zerver/tests/test_thumbnail.py index 16d50e8b2b..8543035cb6 100644 --- a/zerver/tests/test_thumbnail.py +++ b/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(url, result["Location"]) + self.assertEqual(result.status_code, 200) + 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) - base = "https://external-content.zulipcdn.net/external_content/56c362a24201593891955ff526b3b412c0f9fcd2/68747470733a2f2f7777772e676f6f676c652e636f6d2f696d616765732f737270722f6c6f676f34772e706e67" - self.assertEqual(base, result["Location"]) + self.assertEqual(result.status_code, 403) 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) - base = "https://external-content.zulipcdn.net/external_content/7b6552b60c635e41e8f6daeb36d88afc4eabde79/687474703a2f2f7777772e676f6f676c652e636f6d2f696d616765732f737270722f6c6f676f34772e706e67" - self.assertEqual(base, result["Location"]) + self.assertEqual(result.status_code, 403) url = "//www.google.com/images/srpr/logo4w.png" result = self.client_get("/thumbnail", {"url": url, "size": "full"}) - self.assertEqual(result.status_code, 302, result) - base = "https://external-content.zulipcdn.net/external_content/676530cf4b101d56f56cc4a37c6ef4d4fd9b0c03/2f2f7777772e676f6f676c652e636f6d2f696d616765732f737270722f6c6f676f34772e706e67" - self.assertEqual(base, result["Location"]) + self.assertEqual(result.status_code, 403) @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): diff --git a/zerver/views/development/camo.py b/zerver/views/development/camo.py index 07f86f8f18..3da40b2323 100644 --- a/zerver/views/development/camo.py +++ b/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("

Not a valid URL.

") else: return HttpResponseForbidden("

Not a valid URL.

") diff --git a/zerver/views/thumbnail.py b/zerver/views/thumbnail.py index b9938c6c7e..2d1a1ade16 100644 --- a/zerver/views/thumbnail.py +++ b/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.shortcuts import redirect -from django.utils.translation import gettext as _ +from django.http import HttpRequest, HttpResponseBase, HttpResponseForbidden -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 - - -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 +from zerver.models import UserProfile +from zerver.views.upload import serve_file @typed_endpoint @@ -32,15 +15,22 @@ def backend_serve_thumbnail( *, url: str, size: str, -) -> HttpResponse: - if not maybe_user_profile.is_authenticated: - realm = get_valid_realm_from_request(request) - else: - assert isinstance(maybe_user_profile, UserProfile) - realm = maybe_user_profile.realm +) -> HttpResponseBase: + # This URL used to be passed arbitrary URLs, and pass them through + # Camo; we no longer support doing so, and instead return a 403. + # + # Modern thumbnailing uses URLs of the style + # `/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): - return HttpResponseForbidden(_("

You are not authorized to view this file.

")) + realm_id_str = upload_path_parts[1] + path_id = upload_path_parts[2] - thumbnail_url = generate_thumbnail_url(url) - return redirect(thumbnail_url) + # We do not have ImageAttachment rows for historical uploads, so + # 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)