From 132be7de2ee27b31bb58e6abcb19791c502aa461 Mon Sep 17 00:00:00 2001 From: Mathieu Pillard Date: Wed, 28 Aug 2024 18:08:03 +0200 Subject: [PATCH] Prevent URLs from being added to ratings text (#22604) --- src/olympia/addons/serializers.py | 2 +- src/olympia/addons/validators.py | 10 +------ src/olympia/api/validators.py | 13 +++++++-- src/olympia/ratings/serializers.py | 37 ++++++++++++------------- src/olympia/ratings/tests/test_views.py | 18 ++---------- 5 files changed, 32 insertions(+), 48 deletions(-) diff --git a/src/olympia/addons/serializers.py b/src/olympia/addons/serializers.py index a891f8fbe0..42fcea5bff 100644 --- a/src/olympia/addons/serializers.py +++ b/src/olympia/addons/serializers.py @@ -32,6 +32,7 @@ from olympia.api.fields import ( ) from olympia.api.serializers import AMOModelSerializer, BaseESSerializer from olympia.api.utils import is_gate_active +from olympia.api.validators import NoURLsValidator from olympia.applications.models import AppVersion from olympia.bandwagon.models import Collection from olympia.constants.applications import APP_IDS, APPS_ALL @@ -85,7 +86,6 @@ from .validators import ( MatchingGuidValidator, NoFallbackDefaultLocaleValidator, NoThemesValidator, - NoURLsValidator, ReviewedSourceFileValidator, VerifyMozillaTrademark, VersionAddonMetadataValidator, diff --git a/src/olympia/addons/validators.py b/src/olympia/addons/validators.py index 47bbb64386..fb6722f159 100644 --- a/src/olympia/addons/validators.py +++ b/src/olympia/addons/validators.py @@ -5,7 +5,7 @@ from django.utils.translation import gettext from rest_framework import exceptions, fields from olympia import amo -from olympia.amo.utils import find_language, verify_no_urls +from olympia.amo.utils import find_language from olympia.versions.models import License from .models import Addon @@ -318,14 +318,6 @@ class CanSetCompatibilityValidator: ) -class NoURLsValidator: - def __call__(self, value): - try: - verify_no_urls(value) - except forms.ValidationError as exc: - raise exceptions.ValidationError(exc.message) - - class NoThemesValidator: requires_context = True diff --git a/src/olympia/api/validators.py b/src/olympia/api/validators.py index fc67b9d8b9..a5695b9d46 100644 --- a/src/olympia/api/validators.py +++ b/src/olympia/api/validators.py @@ -1,7 +1,8 @@ from django.core.exceptions import ValidationError as DjangoValidationError -from rest_framework import serializers +from rest_framework import exceptions +from olympia.amo.utils import verify_no_urls from olympia.amo.validators import OneOrMorePrintableCharacterValidator @@ -13,4 +14,12 @@ class OneOrMorePrintableCharacterAPIValidator(OneOrMorePrintableCharacterValidat try: return super().__call__(value) except DjangoValidationError: - raise serializers.ValidationError(self.message) + raise exceptions.ValidationError(self.message) + + +class NoURLsValidator: + def __call__(self, value): + try: + verify_no_urls(value) + except DjangoValidationError as exc: + raise exceptions.ValidationError(exc.message) diff --git a/src/olympia/ratings/serializers.py b/src/olympia/ratings/serializers.py index b73868fb05..29a9841c58 100644 --- a/src/olympia/ratings/serializers.py +++ b/src/olympia/ratings/serializers.py @@ -1,11 +1,9 @@ import re from collections import OrderedDict -from urllib.parse import unquote from django.conf import settings from django.utils.translation import gettext, ngettext -from bleach.linkifier import TLDS from rest_framework import serializers from rest_framework.relations import PrimaryKeyRelatedField @@ -13,23 +11,13 @@ from olympia.accounts.serializers import BaseUserSerializer from olympia.addons.serializers import SimpleAddonSerializer, SimpleVersionSerializer from olympia.api.serializers import AMOModelSerializer from olympia.api.utils import is_gate_active +from olympia.api.validators import NoURLsValidator from olympia.users.utils import RestrictionChecker from olympia.versions.models import Version from .models import DeniedRatingWord, Rating, RatingFlag -# This matches the following three types of patterns: -# http://... or https://..., generic domain names, and IPv4 octets. It does not -# match IPv6 addresses or long strings such as "example dot com". -link_pattern = re.compile( - r'((://)|' # Protocols (e.g.: http://) - r'((\d{1,3}\.){3}(\d{1,3}))|' - r'([0-9a-z\-%%]+\.(%s)))' % '|'.join(TLDS), - (re.I | re.U | re.M), -) - - class RatingAddonSerializer(SimpleAddonSerializer): def get_attribute(self, obj): # Avoid database queries if possible by re-using the addon object from @@ -39,6 +27,12 @@ class RatingAddonSerializer(SimpleAddonSerializer): class BaseRatingSerializer(AMOModelSerializer): addon = RatingAddonSerializer(read_only=True) + body = serializers.CharField( + allow_blank=True, + allow_null=True, + required=False, + validators=[NoURLsValidator()], + ) is_deleted = serializers.BooleanField(read_only=True, source='deleted') is_developer_reply = serializers.SerializerMethodField() is_latest = serializers.BooleanField(read_only=True) @@ -149,12 +143,10 @@ class BaseRatingSerializer(AMOModelSerializer): f'{user.email} or {{{ip_address}}} matched a UserRestriction', ) - # Flag the review if there was a word match or a URL was in it. - # Unquote when searching for links, in case someone tries 'example%2ecom'. - if ( - hasattr(self, '_rating_flag_to_save') - or link_pattern.search(unquote(data.get('body') or '')) is not None - ): + if hasattr(self, '_rating_flag_to_save'): + # If we have the _rating_flag_to_save set, we should be set the + # flag and editorreview attributes on the instance we're about to + # save. data['flag'] = True data['editorreview'] = True @@ -188,7 +180,12 @@ class BaseRatingSerializer(AMOModelSerializer): class RatingSerializerReply(BaseRatingSerializer): """Serializer used for replies only.""" - body = serializers.CharField(allow_null=False, required=True, allow_blank=False) + body = serializers.CharField( + allow_null=False, + required=True, + allow_blank=False, + validators=[NoURLsValidator()], + ) def to_representation(self, obj): should_access_deleted = getattr( diff --git a/src/olympia/ratings/tests/test_views.py b/src/olympia/ratings/tests/test_views.py index fd88ab08e2..5529a401f5 100644 --- a/src/olympia/ratings/tests/test_views.py +++ b/src/olympia/ratings/tests/test_views.py @@ -1824,7 +1824,6 @@ class TestRatingViewSetPost(TestCase): self.client.login_api(self.user) assert not Rating.objects.exists() body = 'Trying to spam
http://éxample.com' - cleaned_body = 'Trying to spam \n http://éxample.com' response = self.client.post( self.url, { @@ -1834,21 +1833,8 @@ class TestRatingViewSetPost(TestCase): 'version': self.addon.current_version.pk, }, ) - assert response.status_code == 201 - review = Rating.objects.latest('pk') - assert review.pk == response.data['id'] - assert str(review.body) == response.data['body'] == cleaned_body - assert review.rating == response.data['score'] == 5 - assert review.user == self.user - assert review.reply_to is None - assert review.addon == self.addon - assert review.version == self.addon.current_version - assert response.data['version'] == { - 'id': review.version.id, - 'version': review.version.version, - } - assert review.flag - assert review.editorreview + assert response.status_code == 400 + assert response.data['body'] == ['URLs are not allowed.'] def test_post_rating_float(self): self.user = user_factory()