Merge pull request #3348 from alphagov/no-embedded-images-in-SVG-logo-files

Do not allow to upload SVG logos with embedded raster images in them

app/main/forms.py

@@ -42,6 +42,7 @@ from app.main.validators import (
     LettersNumbersAndFullStopsOnly,
     MustContainAlphanumericCharacters,
     NoCommasInPlaceHolders,
+    NoEmbeddedImagesInSVG,
     OnlySMSCharacters,
     ValidEmail,
     ValidGovEmail,
@@ -1133,7 +1134,8 @@ class SVGFileUpload(StripWhitespaceForm):
         'Upload an SVG logo',
         validators=[
             FileAllowed(['svg'], 'SVG Images only!'),
-            DataRequired(message="You need to upload a file to submit")
+            DataRequired(message="You need to upload a file to submit"),
+            NoEmbeddedImagesInSVG()
         ]
     )
 

app/main/validators.py

@@ -80,6 +80,16 @@ class NoCommasInPlaceHolders:
             raise ValidationError(self.message)
 
 
+class NoEmbeddedImagesInSVG:
+
+    def __init__(self, message='This SVG has an embedded raster image in it and will not render well'):
+        self.message = message
+
+    def __call__(self, form, field):
+        if '<image' in field.data.stream.read().decode("utf-8"):
+            raise ValidationError(self.message)
+
+
 class OnlySMSCharacters:
     def __call__(self, form, field):
         non_sms_characters = sorted(list(SanitiseSMS.get_non_compatible_characters(field.data)))

tests/app/main/views/test_letter_branding.py

@@ -331,7 +331,10 @@ def test_create_letter_branding_when_uploading_valid_file(
 
     response = platform_admin_client.post(
         url_for('.create_letter_branding'),
-        data={'file': (BytesIO(''.encode('utf-8')), filename)},
+        data={'file': (BytesIO("""
+            <svg height="100" width="100">
+            <circle cx="50" cy="50" r="40" stroke="black" stroke-width="3" fill="red" /></svg>
+        """.encode('utf-8')), filename)},
         follow_redirects=True
     )
 
@@ -343,6 +346,35 @@ def test_create_letter_branding_when_uploading_valid_file(
     mock_delete_temp_files.assert_not_called()
 
 
+def test_create_letter_branding_fails_validation_when_uploading_SVG_with_embedded_image(
+    mocker,
+    platform_admin_client,
+    fake_uuid
+):
+    filename = 'test.svg'
+
+    mock_s3_upload = mocker.patch('app.s3_client.s3_logo_client.utils_s3upload')
+
+    response = platform_admin_client.post(
+        url_for('.create_letter_branding'),
+        data={'file': (BytesIO("""
+            <svg height="100" width="100">
+            <image href="someurlgoeshere" x="0" y="0" height="100" width="100"></image></svg>
+        """.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.find("span", {"class": "error-message"}).text) == message
+
+    assert page.findAll('div', {'id': 'logo-img'}) == []
+
+    mock_s3_upload.assert_not_called()
+
+
 def test_create_letter_branding_when_uploading_invalid_file(platform_admin_client):
     response = platform_admin_client.post(
         url_for('.create_letter_branding'),