upload: Allow rate limited access to spectators for uploaded files.

We allow spectators access to uploaded files in web public streams
but rate limit the daily requests to 1000 per file by default.

zerver/lib/rate_limiter.py

@@ -516,3 +516,21 @@ class RateLimitResult:
         self.secs_to_freedom = secs_to_freedom
         self.over_limit = over_limit
         self.remaining = remaining
+
+
+class RateLimitedSpectatorAttachmentAccessByFile(RateLimitedObject):
+    def __init__(self, path_id: str) -> None:
+        self.path_id = path_id
+        super().__init__()
+
+    def key(self) -> str:
+        return f"{type(self).__name__}:{self.path_id}"
+
+    def rules(self) -> List[Tuple[int, int]]:
+        return settings.RATE_LIMITING_RULES["spectator_attachment_access_by_file"]
+
+
+def rate_limit_spectator_attachment_access_by_file(path_id: str) -> None:
+    ratelimited, _ = RateLimitedSpectatorAttachmentAccessByFile(path_id).rate_limit()
+    if ratelimited:
+        raise RateLimited

zerver/lib/rest.py

@@ -148,7 +148,7 @@ def rest_dispatch(request: HttpRequest, **kwargs: Any) -> HttpResponse:
                 allow_webhook_access="allow_incoming_webhooks" in view_flags,
             )(target_function)
         elif (
-            request.path.startswith(("/json", "/avatar"))
+            request.path.startswith(("/json", "/avatar", "/user_uploads", "/thumbnail"))
             and "allow_anonymous_user_web" in view_flags
         ):
             # For endpoints that support anonymous web access, we do that.

zerver/models.py

@@ -25,7 +25,12 @@ import re2
 from bitfield import BitField
 from bitfield.types import BitHandler
 from django.conf import settings
-from django.contrib.auth.models import AbstractBaseUser, PermissionsMixin, UserManager
+from django.contrib.auth.models import (
+    AbstractBaseUser,
+    AnonymousUser,
+    PermissionsMixin,
+    UserManager,
+)
 from django.contrib.contenttypes.fields import GenericRelation
 from django.contrib.postgres.indexes import GinIndex
 from django.contrib.postgres.search import SearchVectorField
@@ -74,7 +79,7 @@ from zerver.lib.cache import (
     user_profile_by_id_cache_key,
     user_profile_cache_key,
 )
-from zerver.lib.exceptions import JsonableError
+from zerver.lib.exceptions import JsonableError, RateLimited
 from zerver.lib.pysa import mark_sanitized
 from zerver.lib.timestamp import datetime_to_timestamp
 from zerver.lib.types import (
@@ -3378,15 +3383,37 @@ def validate_attachment_request_for_spectator_access(
         )
         attachment.refresh_from_db()
 
-    return attachment.is_web_public
+    if not attachment.is_web_public:
+        return False
+
+    if settings.RATE_LIMITING:
+        try:
+            from zerver.lib.rate_limiter import rate_limit_spectator_attachment_access_by_file
+
+            rate_limit_spectator_attachment_access_by_file(attachment.path_id)
+        except RateLimited:
+            return False
+
+    return True
 
 
-def validate_attachment_request(user_profile: UserProfile, path_id: str) -> Optional[bool]:
+def validate_attachment_request(
+    maybe_user_profile: Union[UserProfile, AnonymousUser],
+    path_id: str,
+    realm: Optional[Realm] = None,
+) -> Optional[bool]:
     try:
         attachment = Attachment.objects.get(path_id=path_id)
     except Attachment.DoesNotExist:
         return None
 
+    if isinstance(maybe_user_profile, AnonymousUser):
+        assert realm is not None
+        return validate_attachment_request_for_spectator_access(realm, attachment)
+
+    user_profile = maybe_user_profile
+    assert isinstance(user_profile, UserProfile)
+
     # Update cached is_realm_public property, if necessary.
     if attachment.is_realm_public is None:
         # Fill the cache in a single query. This is important to avoid

zerver/tests/test_thumbnail.py

@@ -1,7 +1,9 @@
 from io import StringIO
 
 import orjson
+from django.test import override_settings
 
+from zerver.lib.rate_limiter import add_ratelimit_rule, remove_ratelimit_rule
 from zerver.lib.test_classes import ZulipTestCase
 
 
@@ -45,3 +47,53 @@ class ThumbnailTest(ZulipTestCase):
         self.assertEqual(result.status_code, 302, result)
         base = "https://external-content.zulipcdn.net/external_content/676530cf4b101d56f56cc4a37c6ef4d4fd9b0c03/2f2f7777772e676f6f676c652e636f6d2f696d616765732f737270722f6c6f676f34772e706e67"
         self.assertEqual(base, result.url)
+
+    @override_settings(RATE_LIMITING=True)
+    def test_thumbnail_redirect_for_spectator(self) -> None:
+        self.login("hamlet")
+        fp = StringIO("zulip!")
+        fp.name = "zulip.jpeg"
+
+        result = self.client_post("/json/user_uploads", {"file": fp})
+        self.assert_json_success(result)
+        json = orjson.loads(result.content)
+        uri = json["uri"]
+
+        add_ratelimit_rule(86400, 1000, domain="spectator_attachment_access_by_file")
+        # Deny file access for non web public stream
+        self.subscribe(self.example_user("hamlet"), "Denmark")
+        host = self.example_user("hamlet").realm.host
+        body = f"First message ...[zulip.txt](http://{host}" + uri + ")"
+        self.send_stream_message(self.example_user("hamlet"), "Denmark", body, "test")
+
+        self.logout()
+        response = self.client_get("/thumbnail", {"url": uri[1:], "size": "full"})
+        self.assertEqual(response.status_code, 403)
+
+        # Allow file access for web public stream
+        self.login("hamlet")
+        self.make_stream("web-public-stream", is_web_public=True)
+        self.subscribe(self.example_user("hamlet"), "web-public-stream")
+        body = f"First message ...[zulip.txt](http://{host}" + uri + ")"
+        self.send_stream_message(self.example_user("hamlet"), "web-public-stream", body, "test")
+
+        self.logout()
+        response = self.client_get("/thumbnail", {"url": uri[1:], "size": "full"})
+        self.assertEqual(response.status_code, 302)
+        remove_ratelimit_rule(86400, 1000, domain="spectator_attachment_access_by_file")
+
+        # Deny file access since rate limited
+        add_ratelimit_rule(86400, 0, domain="spectator_attachment_access_by_file")
+        response = self.client_get("/thumbnail", {"url": uri[1:], "size": "full"})
+        self.assertEqual(response.status_code, 403)
+        remove_ratelimit_rule(86400, 0, domain="spectator_attachment_access_by_file")
+
+        # Deny random file access
+        response = self.client_get(
+            "/thumbnail",
+            {
+                "url": "user_uploads/2/71/QYB7LA-ULMYEad-QfLMxmI2e/zulip-non-existent.txt",
+                "size": "full",
+            },
+        )
+        self.assertEqual(response.status_code, 403)

zerver/tests/test_upload.py

@@ -13,6 +13,7 @@ import botocore.exceptions
 import orjson
 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
@@ -32,6 +33,7 @@ from zerver.lib.avatar_hash import user_avatar_path
 from zerver.lib.cache import cache_get, get_realm_used_upload_space_cache_key
 from zerver.lib.create_user import copy_default_settings
 from zerver.lib.initial_password import initial_password
+from zerver.lib.rate_limiter import add_ratelimit_rule, remove_ratelimit_rule
 from zerver.lib.realm_icon import realm_icon_url
 from zerver.lib.realm_logo import get_realm_logo_url
 from zerver.lib.test_classes import UploadSerializeMixin, ZulipTestCase
@@ -240,7 +242,52 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
         self.assert_streaming_content(self.client_get(url_only_url), b"zulip!")
         # The original uri shouldn't work when logged out:
         result = self.client_get(uri)
-        self.assertEqual(result.status_code, 401)
+        self.assertEqual(result.status_code, 403)
+
+    @override_settings(RATE_LIMITING=True)
+    def test_serve_file_unauthed(self) -> None:
+        self.login("hamlet")
+        fp = StringIO("zulip!")
+        fp.name = "zulip_web_public.txt"
+
+        result = self.client_post("/json/user_uploads", {"file": fp})
+        uri = result.json()["uri"]
+        self.assert_json_success(result)
+
+        add_ratelimit_rule(86400, 1000, domain="spectator_attachment_access_by_file")
+        # Deny file access for non web public stream
+        self.subscribe(self.example_user("hamlet"), "Denmark")
+        host = self.example_user("hamlet").realm.host
+        body = f"First message ...[zulip.txt](http://{host}" + uri + ")"
+        self.send_stream_message(self.example_user("hamlet"), "Denmark", body, "test")
+
+        self.logout()
+        response = self.client_get(uri)
+        self.assertEqual(response.status_code, 403)
+
+        # Allow file access for web public stream
+        self.login("hamlet")
+        self.make_stream("web-public-stream", is_web_public=True)
+        self.subscribe(self.example_user("hamlet"), "web-public-stream")
+        body = f"First message ...[zulip.txt](http://{host}" + uri + ")"
+        self.send_stream_message(self.example_user("hamlet"), "web-public-stream", body, "test")
+
+        self.logout()
+        response = self.client_get(uri)
+        self.assertEqual(response.status_code, 200)
+        remove_ratelimit_rule(86400, 1000, domain="spectator_attachment_access_by_file")
+
+        # Deny file access since rate limited
+        add_ratelimit_rule(86400, 0, domain="spectator_attachment_access_by_file")
+        response = self.client_get(uri)
+        self.assertEqual(response.status_code, 403)
+        remove_ratelimit_rule(86400, 0, domain="spectator_attachment_access_by_file")
+
+        # Deny random file access
+        response = self.client_get(
+            "/user_uploads/2/71/QYB7LA-ULMYEad-QfLMxmI2e/zulip-non-existent.txt"
+        )
+        self.assertEqual(response.status_code, 404)
 
     def test_serve_local_file_unauthed_invalid_token(self) -> None:
         result = self.client_get("/user_uploads/temporary/badtoken/file.png")
@@ -294,9 +341,8 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
 
         self.logout()
         response = self.client_get(uri)
-        self.assert_json_error(
-            response, "Not logged in: API authentication or user session required", status_code=401
-        )
+        self.assertEqual(response.status_code, 403)
+        self.assert_in_response("<p>You are not authorized to view this file.</p>", response)
 
     def test_removed_file_download(self) -> None:
         """
@@ -613,6 +659,11 @@ class FileUploadTest(UploadSerializeMixin, ZulipTestCase):
         self.assertEqual(response.status_code, 403)
         self.assert_in_response("You are not authorized to view this file.", response)
 
+        # Verify that cross-realm access to files for spectators is denied.
+        self.logout()
+        response = self.client_get(uri, subdomain=test_subdomain)
+        self.assertEqual(response.status_code, 403)
+
     def test_file_download_authorization_invite_only(self) -> None:
         hamlet = self.example_user("hamlet")
         cordelia = self.example_user("cordelia")

zerver/views/thumbnail.py

@@ -1,21 +1,27 @@
 # See https://zulip.readthedocs.io/en/latest/subsystems/thumbnailing.html
-from typing import Optional
+from typing import Optional, Union
 
+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 zerver.context_processors import get_valid_realm_from_request
 from zerver.lib.request import REQ, has_request_variables
 from zerver.lib.thumbnail import generate_thumbnail_url
-from zerver.models import UserProfile, validate_attachment_request
+from zerver.models import Realm, UserProfile, validate_attachment_request
 
 
-def validate_thumbnail_request(user_profile: UserProfile, path: str) -> Optional[bool]:
+def validate_thumbnail_request(
+    realm: Realm,
+    maybe_user_profile: Union[UserProfile, AnonymousUser],
+    path: str,
+) -> Optional[bool]:
     # 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(user_profile, path_id)
+        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
@@ -24,11 +30,17 @@ def validate_thumbnail_request(user_profile: UserProfile, path: str) -> Optional
 @has_request_variables
 def backend_serve_thumbnail(
     request: HttpRequest,
-    user_profile: UserProfile,
+    maybe_user_profile: Union[UserProfile, AnonymousUser],
     url: str = REQ(),
     size_requested: str = REQ("size"),
 ) -> HttpResponse:
-    if not validate_thumbnail_request(user_profile, url):
+    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
+
+    if not validate_thumbnail_request(realm, maybe_user_profile, url):
         return HttpResponseForbidden(_("<p>You are not authorized to view this file.</p>"))
 
     thumbnail_url = generate_thumbnail_url(url)

zerver/views/upload.py

@@ -1,12 +1,15 @@
 from mimetypes import guess_type
+from typing import Union
 
 from django.conf import settings
+from django.contrib.auth.models import AnonymousUser
 from django.http import HttpRequest, HttpResponse, 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
 from zerver.lib.response import json_success
 from zerver.lib.upload import (
@@ -76,9 +79,12 @@ def serve_file_download_backend(
 
 
 def serve_file_backend(
-    request: HttpRequest, user_profile: UserProfile, realm_id_str: str, filename: str
+    request: HttpRequest,
+    maybe_user_profile: Union[UserProfile, AnonymousUser],
+    realm_id_str: str,
+    filename: str,
 ) -> HttpResponse:
-    return serve_file(request, user_profile, realm_id_str, filename, url_only=False)
+    return serve_file(request, maybe_user_profile, realm_id_str, filename, url_only=False)
 
 
 def serve_file_url_backend(
@@ -94,14 +100,15 @@ def serve_file_url_backend(
 
 def serve_file(
     request: HttpRequest,
-    user_profile: UserProfile,
+    maybe_user_profile: Union[UserProfile, AnonymousUser],
     realm_id_str: str,
     filename: str,
     url_only: bool = False,
     download: bool = False,
 ) -> HttpResponse:
     path_id = f"{realm_id_str}/{filename}"
-    is_authorized = validate_attachment_request(user_profile, path_id)
+    realm = get_valid_realm_from_request(request)
+    is_authorized = validate_attachment_request(maybe_user_profile, path_id, realm)
 
     if is_authorized is None:
         return HttpResponseNotFound(_("<p>File not found.</p>"))

zproject/computed_settings.py

@@ -394,6 +394,9 @@ RATE_LIMITING_RULES = {
     "sends_email_by_ip": [
         (86400, 5),
     ],
+    "spectator_attachment_access_by_file": [
+        (86400, 1000),  # 1000 per day per file
+    ],
 }
 
 # List of domains that, when applied to a request in a Tornado process,

zproject/urls.py

@@ -676,11 +676,14 @@ urls += [
     ),
     rest_path(
         "user_uploads/<realm_id_str>/<path:filename>",
-        GET=(serve_file_backend, {"override_api_url_scheme"}),
+        GET=(serve_file_backend, {"override_api_url_scheme", "allow_anonymous_user_web"}),
     ),
     # This endpoint redirects to camo; it requires an exception for the
     # same reason.
-    rest_path("thumbnail", GET=(backend_serve_thumbnail, {"override_api_url_scheme"})),
+    rest_path(
+        "thumbnail",
+        GET=(backend_serve_thumbnail, {"override_api_url_scheme", "allow_anonymous_user_web"}),
+    ),
     # Avatars have the same constraint because their URLs are included
     # in API data structures used by both the mobile and web clients.
     rest_path(