[fix] Make sure to HTML escape all dashboard labels

openwisp_utils/admin_theme/dashboard.py

@@ -1,4 +1,5 @@
 import copy
+import html
 
 from django.core.exceptions import ImproperlyConfigured
 from django.db.models import Count
@@ -126,7 +127,12 @@ def get_dashboard_context(request):
         group_by = query_params.get('group_by')
         annotate = query_params.get('annotate')
         aggregate = query_params.get('aggregate')
+
         labels_i18n = value.get('labels')
+        # HTML escape labels defined in configuration to prevent breaking the JS
+        if labels_i18n:
+            for label_key, label_value in labels_i18n.items():
+                labels_i18n[label_key] = html.escape(label_value)
 
         try:
             model = load_model(app_label, model_name)
@@ -175,9 +181,13 @@ def get_dashboard_context(request):
                 if labels_i18n and qs_key in labels_i18n:
                     # store original label as filter, but only
                     # if we have more than the empty default label defined
-                    # if len(labels_i18n.keys()) > 1
                     filters.append(label)
                     label = labels_i18n[qs_key]
+                else:
+                    # HTML escape labels coming from values in the DB
+                    # to avoid possible XSS attacks caused by
+                    # malicious DB values set by users
+                    label = html.escape(label)
                 labels.append(label)
                 # use predefined colors if available,
                 # otherwise the JS lib will choose automatically

requirements-test.txt

@@ -1,3 +1,3 @@
 # For testing Dependency loaders
-openwisp_controller @ https://github.com/openwisp/openwisp-controller/tarball/master
-selenium~=3.141.0
+openwisp_controller @ https://github.com/openwisp/openwisp-controller/tarball/1.0
+selenium~=4.9.1

tests/test_project/apps.py

@@ -81,7 +81,9 @@ def register_dashboard_charts(self):
                     'without_operator__sum': '#353c44',
                 },
                 'labels': {
-                    'with_operator__sum': _('Projects with operators'),
+                    # the <strong> is for testing purposes to
+                    # verify it's being HTML escaped correctly
+                    'with_operator__sum': _('<strong>Projects with operators</strong>'),
                     'without_operator__sum': _('Projects without operators'),
                 },
                 'filters': {

tests/test_project/tests/test_dashboard.py

@@ -11,9 +11,11 @@
     unregister_dashboard_chart,
     unregister_dashboard_template,
 )
+from openwisp_utils.admin_theme.dashboard import get_dashboard_context
 
-from ..models import Project
+from ..models import Operator, Project
 from . import AdminTestMixin
+from .utils import MockRequest, MockUser
 
 
 class TestDashboardSchema(UnitTestCase):
@@ -178,3 +180,26 @@ def test_dashboard_disabled(self):
         with self.subTest('Test "Dashboard" is absent from menu items'):
             response = self.client.get(reverse('admin:index'))
             self.assertNotContains(response, 'Dashboard')
+
+    def test_get_dashboard_context_html_escape(self):
+        # craft malicious DB value which will be shown in labels
+        project = Project.objects.create(name='<script>alert(1)</script>')
+        Operator.objects.create(project=project, first_name='xss', last_name='xss')
+        # prepare mock request and get context
+        mocked_user = MockUser(is_superuser=True)
+        mocked_request = MockRequest(user=mocked_user)
+        context = get_dashboard_context(mocked_request)
+        # ensure DB value is escaped
+        self.assertEqual(
+            context['dashboard_charts'][0]['query_params']['labels'][0],
+            '&lt;script&gt;alert(1)&lt;/script&gt;',
+        )
+        # ensure configured labels are escaped
+        self.assertEqual(
+            context['dashboard_charts'][1]['labels']['with_operator__sum'],
+            '&lt;strong&gt;Projects with operators&lt;/strong&gt;',
+        )
+        self.assertEqual(
+            context['dashboard_charts'][1]['query_params']['labels'][0],
+            '&lt;strong&gt;Projects with operators&lt;/strong&gt;',
+        )

tests/test_project/tests/test_selenium.py

@@ -222,7 +222,7 @@ def test_addition_of_transition_effect(self):
         transition = 'none 0s ease 0s'
         # none because transition has been set to none during tests
         self.login()
-        menu = self.web_driver.find_element_by_id('menu')
+        menu = self.web_driver.find_element(By.ID, 'menu')
         main_content = self._get_main_content()
         menu_toggle = self._get_menu_toggle()
         self.assertEqual(menu.value_of_css_property('transition'), transition)
@@ -485,7 +485,7 @@ def test_shelf_filter(self):
                 EC.visibility_of_element_located((By.CSS_SELECTOR, '#site-name'))
             )
             self.assertEqual(self.check_exists_by_id('changelist-filter-clear'), True)
-            paginator = self.web_driver.find_element_by_css_selector('.paginator')
+            paginator = self.web_driver.find_element(By.CSS_SELECTOR, '.paginator')
             self.assertEqual(paginator.get_attribute('innerText'), '2 shelfs')
 
         with self.subTest('Test clear filter button'):
@@ -494,7 +494,7 @@ def test_shelf_filter(self):
             WebDriverWait(self.web_driver, 2).until(
                 EC.visibility_of_element_located((By.CSS_SELECTOR, '#site-name'))
             )
-            paginator = self.web_driver.find_element_by_css_selector('.paginator')
+            paginator = self.web_driver.find_element(By.CSS_SELECTOR, '.paginator')
             self.assertEqual(paginator.get_attribute('innerText'), '4 shelfs')
 
         with self.subTest('Test multiple filters'):
@@ -512,7 +512,7 @@ def test_shelf_filter(self):
             WebDriverWait(self.web_driver, 2).until(
                 EC.visibility_of_element_located((By.CSS_SELECTOR, '#site-name'))
             )
-            paginator = self.web_driver.find_element_by_css_selector('.paginator')
+            paginator = self.web_driver.find_element(By.CSS_SELECTOR, '.paginator')
             self.assertEqual(paginator.get_attribute('innerText'), '0 shelfs')
 
     def test_book_filter(self):
@@ -548,7 +548,7 @@ def test_book_filter(self):
             selected_option = self._get_filter_selected_option('name')
             self.assertNotEqual(old_value, selected_option.get_attribute('innerText'))
             self.assertEqual(selected_option.get_attribute('innerText'), 'horror book')
-            paginator = self.web_driver.find_element_by_css_selector('.paginator')
+            paginator = self.web_driver.find_element(By.CSS_SELECTOR, '.paginator')
             self.assertEqual(paginator.get_attribute('innerText'), '1 book')
 
 
@@ -584,65 +584,66 @@ def test_input_filters(self):
             input_field.send_keys('Horror')
             self._get_filter_button().click()
             # Horror shelf is present
-            self.web_driver.find_element_by_xpath(horror_result_xpath)
+            self.web_driver.find_element(By.XPATH, horror_result_xpath)
             with self.assertRaises(NoSuchElementException):
                 # Factual shelf is absent
-                self.web_driver.find_element_by_xpath(factual_result_xpath)
+                self.web_driver.find_element(By.XPATH, factual_result_xpath)
             # Both shelves should be present after clearing filter
-            self.web_driver.find_element_by_css_selector('.field-clear').click()
-            self.web_driver.find_element_by_xpath(horror_result_xpath)
-            self.web_driver.find_element_by_xpath(factual_result_xpath)
+            self.web_driver.find_element(By.CSS_SELECTOR, '.field-clear').click()
+            self.web_driver.find_element(By.XPATH, horror_result_xpath)
+            self.web_driver.find_element(By.XPATH, factual_result_xpath)
 
         with self.subTest('Test InputFilter'):
             self.open(url)
             input_field = self._get_input_filter()
             input_field.send_keys('HORROR')
             self._get_filter_button().click()
             # Horror shelf is present
-            self.web_driver.find_element_by_xpath(horror_result_xpath)
+            self.web_driver.find_element(By.XPATH, horror_result_xpath)
             with self.assertRaises(NoSuchElementException):
                 # Factual shelf is absent
-                self.web_driver.find_element_by_xpath(factual_result_xpath)
+                self.web_driver.find_element(By.XPATH, factual_result_xpath)
             # Both shelves should be present after clearing filter
-            self.web_driver.find_element_by_css_selector('.field-clear').click()
-            self.web_driver.find_element_by_xpath(horror_result_xpath)
-            self.web_driver.find_element_by_xpath(factual_result_xpath)
+            self.web_driver.find_element(By.CSS_SELECTOR, '.field-clear').click()
+            self.web_driver.find_element(By.XPATH, horror_result_xpath)
+            self.web_driver.find_element(By.XPATH, factual_result_xpath)
 
         with self.subTest('Test InputFilter: UUID'):
             self.open(url)
-            input_field = self.web_driver.find_element_by_css_selector(
-                'input[name=id__exact]'
+            input_field = self.web_driver.find_element(
+                By.CSS_SELECTOR, 'input[name=id__exact]'
             )
             input_field.send_keys(str(horror_shelf.id))
             self._get_filter_button().click()
             # Horror shelf is present
-            self.web_driver.find_element_by_xpath(horror_result_xpath)
+            self.web_driver.find_element(By.XPATH, horror_result_xpath)
             with self.assertRaises(NoSuchElementException):
                 # Factual shelf is absent
-                self.web_driver.find_element_by_xpath(factual_result_xpath)
+                self.web_driver.find_element(By.XPATH, factual_result_xpath)
             # Both shelves should be present after clearing filter
-            self.web_driver.find_element_by_css_selector('.field-clear').click()
-            self.web_driver.find_element_by_xpath(horror_result_xpath)
-            self.web_driver.find_element_by_xpath(factual_result_xpath)
+            self.web_driver.find_element(By.CSS_SELECTOR, '.field-clear').click()
+            self.web_driver.find_element(By.XPATH, horror_result_xpath)
+            self.web_driver.find_element(By.XPATH, factual_result_xpath)
 
         with self.subTest('Test InputFilter: Related field'):
             admin_xpath = f'//*[@id="result_list"]/tbody/tr/th/a[contains(text(), "{self.admin.username}")]'
             user_xpath = f'//*[@id="result_list"]/tbody/tr/th/a[contains(text(), "{user.username}")]'
             self.open(reverse('admin:auth_user_changelist'))
-            input_field = self.web_driver.find_element_by_xpath(
-                '//*[@id="ow-changelist-filter"]/div[1]/div/div/div[2]/div[1]/form/input'
+            input_field = self.web_driver.find_element(
+                By.XPATH,
+                '//*[@id="ow-changelist-filter"]/div[1]/div/div/div[2]/div[1]/form/input',
             )
             input_field.send_keys(str(horror_shelf.id))
             self._get_filter_button().click()
             # Admin user is present
-            self.web_driver.find_element_by_xpath(admin_xpath)
+            self.web_driver.find_element(By.XPATH, admin_xpath)
             with self.assertRaises(NoSuchElementException):
                 # User is absent
-                self.web_driver.find_element_by_xpath(user_xpath)
+                self.web_driver.find_element(By.XPATH, user_xpath)
             # Both users should be present after clearing filter
-            self.web_driver.find_element_by_css_selector('.field-clear').click()
-            self.web_driver.find_element_by_xpath(admin_xpath)
-            self.web_driver.find_element_by_xpath(user_xpath)
+            self.web_driver.find_element(By.CSS_SELECTOR, '.field-clear').click()
+            self.web_driver.find_element(By.XPATH, admin_xpath)
+            self.web_driver.find_element(By.XPATH, user_xpath)
 
 
 class TestDashboardCharts(SeleniumTestMixin, CreateMixin, StaticLiveServerTestCase):
@@ -671,7 +672,7 @@ def test_pie_chart_zero_annotation(self):
         except TimeoutException:
             self.fail('Failed to find annotation text element in the chart')
         else:
-            annotation_text = self.web_driver.find_element_by_css_selector(
-                '.operator-project-distribution .annotation-text tspan'
+            annotation_text = self.web_driver.find_element(
+                By.CSS_SELECTOR, '.operator-project-distribution .annotation-text tspan'
             )
             self.assertEqual(annotation_text.text, '0')