expand homoglyph protection to UserProfile.display_name and Collection.name

Author: eviljeff

src/olympia/accounts/serializers.py

@@ -1,3 +1,4 @@
+from django import forms
 from django.conf import settings
 from django.utils.translation import gettext
 
@@ -14,6 +15,7 @@
     subscribe_newsletter,
     unsubscribe_newsletter,
     urlparams,
+    validate_name,
 )
 from olympia.api.fields import HttpHttpsOnlyURLField
 from olympia.api.serializers import AMOModelSerializer, SiteStatusSerializer
@@ -153,11 +155,16 @@ def validate_biography(self, value):
         return value
 
     def validate_display_name(self, value):
-        if DeniedName.blocked(value):
-            raise serializers.ValidationError(
-                gettext('This display name cannot be used.')
-            )
-        return value
+        error_msg = gettext('This display name cannot be used.')
+
+        def check_function(normalized_name, variant):
+            if DeniedName.blocked(variant):
+                raise serializers.ValidationError(error_msg)
+
+        try:
+            return validate_name(value, check_function, error_msg)
+        except forms.ValidationError as exc:
+            raise serializers.ValidationError(exc.messages)
 
     def validate_picture_upload(self, value):
         image_check = ImageCheck(value)

src/olympia/accounts/tests/test_views.py

@@ -45,7 +45,7 @@
 )
 from olympia.amo.tests.test_helpers import get_uploaded_file
 from olympia.api.tests.utils import APIKeyAuthTestMixin
-from olympia.users.models import UserNotification, UserProfile
+from olympia.users.models import DeniedName, UserNotification, UserProfile
 from olympia.users.notifications import (
     NOTIFICATIONS_BY_ID,
     NOTIFICATIONS_COMBINED,
@@ -1507,6 +1507,30 @@ def test_display_name_validation(self):
         response = self.patch(data={'display_name': 'a' * 50})
         assert response.status_code == 200
 
+    def test_display_name_denied_name(self):
+        DeniedName.objects.create(name='foo')
+        self.client.login_api(self.user)
+        response = self.patch(data={'display_name': 'foo_thing'})
+        assert response.status_code == 400
+        assert json.loads(response.content) == {
+            'display_name': ['This display name cannot be used.']
+        }
+
+        # homoglyphs don't bypass the validation
+        response = self.patch(data={'display_name': 'fѻѺ_thing'})
+        assert response.status_code == 400
+        assert json.loads(response.content) == {
+            'display_name': ['This display name cannot be used.']
+        }
+
+    def test_display_name_too_many_homoglyphs(self):
+        self.client.login_api(self.user)
+        response = self.patch(data={'display_name': 'l' * 17})
+        assert response.status_code == 400
+        assert json.loads(response.content) == {
+            'display_name': ['This display name cannot be used.']
+        }
+
     @override_settings(EXTERNAL_SITE_URL='https://example.org')
     def test_validate_homepage(self):
         self.client.login_api(self.user)

src/olympia/addons/utils.py

@@ -8,11 +8,7 @@
 
 from olympia import amo, core
 from olympia.access.acl import action_allowed_for
-from olympia.amo.utils import (
-    generate_lowercase_homoglyphs_variants_for_string,
-    normalize_string_for_name_checks,
-    verify_condition_with_locales,
-)
+from olympia.amo.utils import validate_name
 from olympia.translations.models import Translation
 
 
@@ -39,42 +35,25 @@ def validate_addon_name(name, user, *, form=None):
         and action_allowed_for(user, amo.permissions.TRADEMARK_BYPASS)
     )
 
-    def _check(name):
-        name_without_punctuation = normalize_string_for_name_checks(
-            name, categories_to_strip=('P')
-        ).lower()
-
-        variants = generate_lowercase_homoglyphs_variants_for_string(
-            normalize_string_for_name_checks(name)
-        )
-
-        for index, variant in enumerate(variants):
-            if index > 65535:
-                # index > 65535 means over 16 confusable characters with 2
-                # variants each. That name is likely suspicious, and it's too
-                # expensive to continue anyway. Reject it immediately.
-                raise forms.ValidationError(gettext('This name cannot be used.'))
-
-            if skip_trademark_check:
-                continue
-
-            for symbol in amo.MOZILLA_TRADEMARK_SYMBOLS:
-                symbol_count = variant.count(symbol)
-                violates_trademark = symbol_count > 1 or (
-                    symbol_count >= 1
-                    # 'XXX for Mozilla' or 'XXX for Firefox' is allowed.
-                    and not name_without_punctuation.endswith(f' for {symbol}')
+    def check_function(normalized_name, variant):
+        if skip_trademark_check:
+            return
+
+        for symbol in amo.MOZILLA_TRADEMARK_SYMBOLS:
+            symbol_count = variant.count(symbol)
+            violates_trademark = symbol_count > 1 or (
+                symbol_count >= 1
+                # 'XXX for Mozilla' or 'XXX for Firefox' is allowed.
+                and not normalized_name.endswith(f' for {symbol}')
+            )
+
+            if violates_trademark:
+                msg = gettext(
+                    'Add-on names cannot contain the Mozilla or Firefox trademarks.'
                 )
+                raise forms.ValidationError(msg)
 
-                if violates_trademark:
-                    msg = gettext(
-                        'Add-on names cannot contain the Mozilla or Firefox trademarks.'
-                    )
-                    raise forms.ValidationError(msg)
-
-    verify_condition_with_locales(
-        value=name, check_func=_check, form=form, field_name='name'
-    )
+    validate_name(name, check_function, gettext('This name cannot be used.'), form=form)
 
     return name
 

src/olympia/amo/utils.py

@@ -1173,6 +1173,31 @@ def verify_condition_with_locales(*, value, check_func, form=None, field_name=No
                     raise
 
 
+def validate_name(name, check_function, error_message, *, form=None):
+    def variant_checker(name):
+        normalized_name = normalize_string_for_name_checks(
+            name, categories_to_strip=('P')
+        ).lower()
+
+        variants = generate_lowercase_homoglyphs_variants_for_string(
+            normalize_string_for_name_checks(name)
+        )
+
+        for index, variant in enumerate(variants):
+            if index > 65535:
+                # index > 65535 means over 16 confusable characters with 2
+                # variants each. That name is likely suspicious, and it's too
+                # expensive to continue anyway. Reject it immediately.
+                raise forms.ValidationError(error_message)
+
+            check_function(normalized_name, variant)
+
+    verify_condition_with_locales(
+        value=name, check_func=variant_checker, form=form, field_name='name'
+    )
+    return name
+
+
 def verify_no_urls(value, *, form=None, field_name=None):
     def _check(value):
         if has_urls(value):

src/olympia/bandwagon/serializers.py

@@ -1,3 +1,4 @@
+from django import forms
 from django.utils.translation import gettext, gettext_lazy as _
 
 from rest_framework import serializers
@@ -9,7 +10,7 @@
 from olympia.addons.models import Addon
 from olympia.addons.serializers import AddonSerializer
 from olympia.amo.templatetags.jinja_helpers import absolutify
-from olympia.amo.utils import clean_nl, has_urls, slug_validator
+from olympia.amo.utils import clean_nl, has_urls, slug_validator, validate_name
 from olympia.api.fields import (
     SlugOrPrimaryKeyRelatedField,
     SplitField,
@@ -80,17 +81,18 @@ def get_url(self, obj):
         return absolutify(obj.get_url_path())
 
     def validate_name(self, value):
-        # if we have a localised dict of values validate them all.
-        if isinstance(value, dict):
-            return {
-                locale: self.validate_name(sub_value)
-                for locale, sub_value in value.items()
-            }
-        if DeniedName.blocked(value) and not can_use_denied_names(
-            self.context.get('request')
-        ):
-            raise serializers.ValidationError(gettext('This name cannot be used.'))
-        return value
+        error_msg = gettext('This name cannot be used.')
+
+        def check_function(normalized_name, variant):
+            if not can_use_denied_names(
+                self.context.get('request')
+            ) and DeniedName.blocked(variant):
+                raise serializers.ValidationError(error_msg)
+
+        try:
+            return validate_name(value, check_function, error_msg)
+        except forms.ValidationError as exc:
+            raise serializers.ValidationError(exc.messages)
 
     def validate_description(self, value):
         if has_urls(clean_nl(str(value))):
@@ -106,14 +108,18 @@ def validate_slug(self, value):
                 'numbers, underscores or hyphens.'
             ),
         )
-        if DeniedName.blocked(value) and not can_use_denied_names(
-            self.context.get('request')
-        ):
-            raise serializers.ValidationError(
-                gettext('This custom URL cannot be used.')
-            )
-
-        return value
+        error_msg = gettext('This custom URL cannot be used.')
+
+        def check_function(normalized_name, variant):
+            if not can_use_denied_names(
+                self.context.get('request')
+            ) and DeniedName.blocked(variant):
+                raise serializers.ValidationError(error_msg)
+
+        try:
+            return validate_name(value, check_function, error_msg)
+        except forms.ValidationError as exc:
+            raise serializers.ValidationError(exc.messages)
 
     def create(self, validated_data):
         validated_data['author'] = self.context['request'].user

src/olympia/bandwagon/tests/test_views.py

@@ -465,12 +465,32 @@ def test_name_denied_name(self):
         response = self.send(data=data)
         assert response.status_code == 400
         assert json.loads(response.content) == {'name': ['This name cannot be used.']}
+
+        # homoglyphs don't bypass the validation
+        data.update(name={'en-US': 'fѻѺ thing'})
+        response = self.send(data=data)
+        assert response.status_code == 400
+        assert json.loads(response.content) == {'name': ['This name cannot be used.']}
+
         # But you can if you have the correct permission
         self.grant_permission(self.user, 'Collections:Edit')
         self.client.login_api(self.user)
         response = self.send(data=data)
         assert response.status_code in (200, 201)
 
+    def test_name_too_many_homoglyphs(self):
+        self.client.login_api(self.user)
+        data = dict(self.data)
+        data.update(name={'en-US': 'l' * 17})
+        response = self.send(data=data)
+        assert response.status_code == 400
+        assert json.loads(response.content) == {'name': ['This name cannot be used.']}
+        # even with permission
+        self.grant_permission(self.user, 'Collections:Edit')
+        self.client.login_api(self.user)
+        response = self.send(data=data)
+        assert response.status_code == 400
+
     def test_slug_denied_name(self):
         DeniedName.objects.create(name='foo')
         self.client.login_api(self.user)
@@ -481,12 +501,36 @@ def test_slug_denied_name(self):
         assert json.loads(response.content) == {
             'slug': ['This custom URL cannot be used.']
         }
+
+        # homoglyphs don't bypass the validation
+        data.update(slug='fѻѺ_thing')
+        response = self.send(data=data)
+        assert response.status_code == 400
+        assert json.loads(response.content) == {
+            'slug': ['This custom URL cannot be used.']
+        }
+
         # But you can if you have the correct permission
         self.grant_permission(self.user, 'Collections:Edit')
         self.client.login_api(self.user)
         response = self.send(data=data)
         assert response.status_code in (200, 201)
 
+    def test_slug_too_many_homoglyphs(self):
+        self.client.login_api(self.user)
+        data = dict(self.data)
+        data.update(slug='l' * 17)
+        response = self.send(data=data)
+        assert response.status_code == 400
+        assert json.loads(response.content) == {
+            'slug': ['This custom URL cannot be used.']
+        }
+        # even with permission
+        self.grant_permission(self.user, 'Collections:Edit')
+        self.client.login_api(self.user)
+        response = self.send(data=data)
+        assert response.status_code == 400
+
 
 class TestCollectionViewSetCreate(CollectionViewSetDataMixin, TestCase):
     def send(self, url=None, data=None):