From 746b14618dc1df922224f8cb3f258bc3f788e0e1 Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Thu, 23 Nov 2023 12:35:32 -0300 Subject: [PATCH] [fix] Make sure to HTML escape all dashboard labels --- openwisp_utils/admin_theme/dashboard.py | 12 ++++++++++- tests/test_project/apps.py | 4 +++- tests/test_project/tests/test_dashboard.py | 25 +++++++++++++++++++++- 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/openwisp_utils/admin_theme/dashboard.py b/openwisp_utils/admin_theme/dashboard.py index 106fe1d1..1f3ccf6f 100644 --- a/openwisp_utils/admin_theme/dashboard.py +++ b/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 @@ -145,7 +146,12 @@ def get_dashboard_context(request): aggregate = query_params.get('aggregate') org_field = query_params.get('organization_field') default_org_field = 'organization_id' + 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) @@ -216,9 +222,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 diff --git a/tests/test_project/apps.py b/tests/test_project/apps.py index d4049920..7e7a4395 100644 --- a/tests/test_project/apps.py +++ b/tests/test_project/apps.py @@ -89,7 +89,9 @@ def register_dashboard_charts(self): 'without_operator__sum': '#353c44', }, 'labels': { - 'with_operator__sum': _('Projects with operators'), + # the is for testing purposes to + # verify it's being HTML escaped correctly + 'with_operator__sum': _('Projects with operators'), 'without_operator__sum': _('Projects without operators'), }, 'filters': { diff --git a/tests/test_project/tests/test_dashboard.py b/tests/test_project/tests/test_dashboard.py index 3d9f1c70..97af6f9a 100644 --- a/tests/test_project/tests/test_dashboard.py +++ b/tests/test_project/tests/test_dashboard.py @@ -15,7 +15,7 @@ ) from openwisp_utils.admin_theme.dashboard import get_dashboard_context -from ..models import Project, RadiusAccounting +from ..models import Operator, Project, RadiusAccounting from . import AdminTestMixin, CreateMixin from .utils import MockRequest, MockUser @@ -271,3 +271,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='') + 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], + '<script>alert(1)</script>', + ) + # ensure configured labels are escaped + self.assertEqual( + context['dashboard_charts'][1]['labels']['with_operator__sum'], + '<strong>Projects with operators</strong>', + ) + self.assertEqual( + context['dashboard_charts'][1]['query_params']['labels'][0], + '<strong>Projects with operators</strong>', + )