From b799ec32b0eda8293d5db3c4b78e8a9d4d79dbd8 Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Tue, 2 Nov 2021 14:42:58 +0000 Subject: [PATCH] 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 | 18 +++++++++++ zerver/lib/rest.py | 2 +- zerver/models.py | 35 +++++++++++++++++--- zerver/tests/test_thumbnail.py | 52 ++++++++++++++++++++++++++++++ zerver/tests/test_upload.py | 59 +++++++++++++++++++++++++++++++--- zerver/views/thumbnail.py | 24 ++++++++++---- zerver/views/upload.py | 15 ++++++--- zproject/computed_settings.py | 3 ++ zproject/urls.py | 7 ++-- 9 files changed, 194 insertions(+), 21 deletions(-) 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//", - 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(