fix: add XSS checks to validation for abbr attributes

openedx · Sep 25, 2023 · 4eb5a22 · 4eb5a22
1 parent 851c02f
commit 4eb5a22
Show file tree

Hide file tree

Showing 6 changed files with 22 additions and 7 deletions.
diff --git a/i18n/validate.py b/i18n/validate.py
@@ -9,11 +9,12 @@
 import textwrap
 
 import polib
+from lxml.html import clean
 
+from i18n import Runner
+from i18n.converter import Converter
 from i18n.dummy import is_format_message
 from i18n.execute import call
-from i18n.converter import Converter
-from i18n import Runner
 
 log = logging.getLogger(__name__)
 
@@ -97,6 +98,11 @@ def is_linguistic_tag(tag):
         if tag.startswith("&"):
             return True
         if any(x in tag for x in ["<abbr>", "<abbr ", "</abbr>"]):
+            if "<abbr " in tag:
+                cleaned_tag = clean.clean_html(tag)
+                # clean_html will remove XSS from tag so check so don't skip abbr tag if cleaned_tag is different
+                if cleaned_tag != tag:
+                    return False
             return True
         return False
 

diff --git a/requirements/base.in b/requirements/base.in
@@ -5,3 +5,4 @@ Django
 polib
 path
 pyYaml
+lxml
diff --git a/requirements/base.txt b/requirements/base.txt
@@ -10,6 +10,9 @@ django==3.2.17
     # via
     #   -c requirements/common_constraints.txt
     #   -r requirements/base.in
+lxml==4.9.3
+    # via
+    #   -r requirements/base.in
 path==16.6.0
     # via -r requirements/base.in
 polib==1.1.1

diff --git a/requirements/common_constraints.txt b/requirements/common_constraints.txt
@@ -30,8 +30,3 @@ django-simple-history==3.0.0
 # tox>4.0.0 isn't yet compatible with many tox plugins, causing CI failures in almost all repos.
 # Details can be found in this discussion: https://github.com/tox-dev/tox/discussions/1810
 tox<4.0.0
-
-# edx-sphinx-theme is not compatible with latest Sphinx==6.0.0 version 
-# Pinning Sphinx version unless the compatibility issue gets resolved
-# For details, see issue https://github.com/openedx/edx-sphinx-theme/issues/197
-sphinx<6.0.0
diff --git a/tests/data/validation_problems.po b/tests/data/validation_problems.po
@@ -79,3 +79,7 @@ msgstr "Look -- a dog!"
 # <abbr> could come-and-go with translations
 msgid "The <abbr>CIA</abbr> said so"
 msgstr "The secret agency said so"
+
+# <abbr> may contain cross-site script attack which is usually skipped from validation
+msgid "No tags"
+msgstr "Added XSS tag <abbr title='Cascading Style Sheets' onmouseover='XSS ATTACK!!!'>CSS</abbr>"
diff --git a/tests/test_validate.py b/tests/test_validate.py
@@ -59,6 +59,12 @@
         '"{nomx}" added',
     ),
     ('Empty translation', 'This string should not be empty'),
+    (
+        'Different tags in source and translation',
+        'No tags',
+        "Added XSS tag <abbr title='Cascading Style Sheets' onmouseover='XSS ATTACK!!!'>CSS</abbr>",
+        '"<abbr title=\'Cascading Style Sheets\' onmouseover=\'XSS ATTACK!!!\'>" added'
+    ),
 ]