diff --git a/zerver/lib/rate_limiter.py b/zerver/lib/rate_limiter.py index 15e71ecdbb..6921f9279f 100644 --- a/zerver/lib/rate_limiter.py +++ b/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 diff --git a/zerver/lib/rest.py b/zerver/lib/rest.py index a279ad5128..3888bc7a6c 100644 --- a/zerver/lib/rest.py +++ b/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. diff --git a/zerver/models.py b/zerver/models.py index 525059f1b1..c8a64c2a98 100644 --- a/zerver/models.py +++ b/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 diff --git a/zerver/tests/test_thumbnail.py b/zerver/tests/test_thumbnail.py index 26bfab67dd..00312cf769 100644 --- a/zerver/tests/test_thumbnail.py +++ b/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) diff --git a/zerver/tests/test_upload.py b/zerver/tests/test_upload.py index 8db2138e7a..be274f76d3 100644 --- a/zerver/tests/test_upload.py +++ b/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("
You are not authorized to view this file.
", 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") diff --git a/zerver/views/thumbnail.py b/zerver/views/thumbnail.py index 8ad30bc845..a40aa22e4e 100644 --- a/zerver/views/thumbnail.py +++ b/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(_("You are not authorized to view this file.
")) thumbnail_url = generate_thumbnail_url(url) diff --git a/zerver/views/upload.py b/zerver/views/upload.py index d018002497..031e17f542 100644 --- a/zerver/views/upload.py +++ b/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(_("File not found.
")) diff --git a/zproject/computed_settings.py b/zproject/computed_settings.py index 36ba2935da..475b186824 100644 --- a/zproject/computed_settings.py +++ b/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, diff --git a/zproject/urls.py b/zproject/urls.py index 3f41ba2be1..617b3d8db7 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -676,11 +676,14 @@ urls += [ ), rest_path( "user_uploads/