Merge pull request #4122 from alphagov/no-text-in-svg

Don’t allow <text> elements in letter logos

app/main/forms.py

@@ -53,6 +53,7 @@ from app.main.validators import (
     NoCommasInPlaceHolders,
     NoEmbeddedImagesInSVG,
     NoPlaceholders,
+    NoTextInSVG,
     OnlySMSCharacters,
     ValidEmail,
     ValidGovEmail,
@@ -1884,7 +1885,8 @@ class SVGFileUpload(StripWhitespaceForm):
         validators=[
             FileAllowed(['svg'], 'SVG Images only!'),
             DataRequired(message="You need to upload a file to submit"),
-            NoEmbeddedImagesInSVG()
+            NoEmbeddedImagesInSVG(),
+            NoTextInSVG(),
         ]
     )
 

app/main/validators.py

@@ -1,4 +1,5 @@
 import re
+from abc import ABC, abstractmethod
 
 from notifications_utils.field import Field
 from notifications_utils.formatters import formatted_list
@@ -77,18 +78,35 @@ class NoCommasInPlaceHolders:
             raise ValidationError(self.message)
 
 
-class NoEmbeddedImagesInSVG:
+class NoElementInSVG(ABC):
 
-    def __init__(self, message='This SVG has an embedded raster image in it and will not render well'):
+    @property
-        self.message = message
+    @abstractmethod
+    def element(self):
+        pass
+
+    @property
+    @abstractmethod
+    def message(self):
+        pass
 
     def __call__(self, form, field):
-        is_image_embedded = '<image' in field.data.stream.read().decode("utf-8")
+        svg_contents = field.data.stream.read().decode("utf-8")
         field.data.stream.seek(0)
-        if is_image_embedded:
+        if f'<{self.element}' in svg_contents.lower():
             raise ValidationError(self.message)
 
 
+class NoEmbeddedImagesInSVG(NoElementInSVG):
+    element = 'image'
+    message = 'This SVG has an embedded raster image in it and will not render well'
+
+
+class NoTextInSVG(NoElementInSVG):
+    element = 'text'
+    message = 'This SVG has text which has not been converted to paths and may not render well'
+
+
 class OnlySMSCharacters:
 
     def __init__(self, *args, template_type, **kwargs):

tests/app/main/views/test_letter_branding.py

@@ -2,6 +2,7 @@ from io import BytesIO
 from unittest.mock import Mock, call
 from uuid import UUID
 
+import pytest
 from botocore.exceptions import ClientError as BotoClientError
 from bs4 import BeautifulSoup
 from flask import url_for
@@ -348,29 +349,44 @@ def test_create_letter_branding_when_uploading_valid_file(
     assert mock_delete_temp_files.called is False
 
 
-def test_create_letter_branding_fails_validation_when_uploading_SVG_with_embedded_image(
+@pytest.mark.parametrize('svg_contents, expected_error', (
+    (
+        '''
+            <svg height="100" width="100">
+            <image href="someurlgoeshere" x="0" y="0" height="100" width="100"></image></svg>
+        ''',
+        'This SVG has an embedded raster image in it and will not render well',
+    ),
+    (
+        '''
+            <svg height="100" width="100">
+                <text>Will render differently depending on fonts installed</text>
+            </svg>
+        ''',
+        'This SVG has text which has not been converted to paths and may not render well',
+    ),
+))
+def test_create_letter_branding_fails_validation_when_uploading_SVG_with_bad_element(
     mocker,
-    platform_admin_client,
+    client_request,
-    fake_uuid
+    platform_admin_user,
+    fake_uuid,
+    svg_contents,
+    expected_error,
 ):
     filename = 'test.svg'
 
     mock_s3_upload = mocker.patch('app.s3_client.s3_logo_client.utils_s3upload')
 
-    response = platform_admin_client.post(
+    client_request.login(platform_admin_user)
-        url_for('.create_letter_branding'),
+    page = client_request.post(
-        data={'file': (BytesIO("""
+        '.create_letter_branding',
-            <svg height="100" width="100">
+        _data={'file': (BytesIO(svg_contents.encode('utf-8')), filename)},
-            <image href="someurlgoeshere" x="0" y="0" height="100" width="100"></image></svg>
+        _follow_redirects=True,
-        """.encode('utf-8')), filename)},
-        follow_redirects=True,
     )
 
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
-
     assert normalize_spaces(page.find('h1').text) == "Add letter branding"
-    message = 'This SVG has an embedded raster image in it and will not render well'
+    assert normalize_spaces(page.select_one(".error-message").text) == expected_error
-    assert normalize_spaces(page.find("span", {"class": "error-message"}).text) == message
 
     assert page.findAll('div', {'id': 'logo-img'}) == []