From 861e2692f62592cb7e71386ed3717573541756dc Mon Sep 17 00:00:00 2001 From: Mark Ekisa Date: Tue, 12 Jan 2021 17:25:57 +0300 Subject: [PATCH 1/3] Trigger error on version invalidity and sanitize description Signed-off-by: Mark Ekisa --- onadata/apps/logger/models/xform.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/onadata/apps/logger/models/xform.py b/onadata/apps/logger/models/xform.py index f061e6b395..3ee55c5c59 100644 --- a/onadata/apps/logger/models/xform.py +++ b/onadata/apps/logger/models/xform.py @@ -33,6 +33,7 @@ from onadata.apps.logger.xform_instance_parser import (XLSFormError, clean_and_parse_xml) +from django.utils.html import conditional_escape from onadata.libs.models.base_model import BaseModel from onadata.libs.utils.cache_tools import ( IS_ORG, PROJ_BASE_FORMS_CACHE, PROJ_FORMS_CACHE, @@ -901,6 +902,13 @@ def save(self, *args, **kwargs): 'in settings sheet or reduce the file name if you do' ' not have a settings sheets.' % self.MAX_ID_LENGTH)) + if contains_xml_invalid_char(self.version): + raise XLSFormError( + _("Version shouldn't have any invalid " + "characters ('>' '&' '<')")) + + self.description = conditional_escape(self.description) + super(XForm, self).save(*args, **kwargs) def __str__(self): From 66333302661601c3e2ae85dcbce97bd6478c0c04 Mon Sep 17 00:00:00 2001 From: Mark Ekisa Date: Tue, 12 Jan 2021 17:27:21 +0300 Subject: [PATCH 2/3] Test suggested enhancement Check that error is triggered when version is invalid and description is sanitize if it has html tag characters Signed-off-by: Mark Ekisa --- .../api/tests/viewsets/test_xform_viewset.py | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/onadata/apps/api/tests/viewsets/test_xform_viewset.py b/onadata/apps/api/tests/viewsets/test_xform_viewset.py index e4ea2f536d..45b6fa3cba 100644 --- a/onadata/apps/api/tests/viewsets/test_xform_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_xform_viewset.py @@ -27,6 +27,7 @@ from django.http import HttpResponseRedirect from django.test.utils import override_settings from django.utils.dateparse import parse_datetime +from django.utils.html import conditional_escape from django.utils.timezone import utc from django_digest.test import DigestAuth from httmock import HTTMock @@ -2687,6 +2688,52 @@ def test_update_xform_dropbox_url(self, mock_urlopen): self.assertEquals(self.xform.version, "212121211") self.assertEquals(form_id, self.xform.pk) + def test_update_xform_using_put_with_invalid_input(self): + with HTTMock(enketo_mock): + self._publish_xls_form_to_project() + form_id = self.xform.pk + + unsanitized_html_str = "

HTML Injection testing

" + version = unsanitized_html_str + view = XFormViewSet.as_view({ + 'put': 'update', + }) + + put_data = { + 'uuid': 'ae631e898bd34ced91d2a309d8b72das', + 'description': unsanitized_html_str, + 'downloadable': False, + 'owner': 'http://testserver/api/v1/users/{0}'. + format(self.user), + 'created_by': + 'http://testserver/api/v1/users/{0}'.format(self.user), + 'public': False, + 'public_data': False, + 'project': 'http://testserver/api/v1/projects/{0}'.format( + self.xform.project.pk), + 'title': 'Transport Form', + 'version': unsanitized_html_str + } + + # trigger error is form version is invalid + with self.assertRaises(XLSFormError): + request = self.factory.put('/', data=put_data, **self.extra) + response = view(request, pk=form_id) + + put_data['version'] = self.xform.version + + request = self.factory.put('/', data=put_data, **self.extra) + response = view(request, pk=form_id) + self.assertEqual(response.status_code, 200, response.data) + + self.xform.refresh_from_db() + + # check that description has been sanitized + self.assertEquals( + conditional_escape(unsanitized_html_str), + self.xform.description + ) + def test_update_xform_using_put(self): with HTTMock(enketo_mock): self._publish_xls_form_to_project() From f4fc1807004c00fba9b30ab5f32b1cb8568dd557 Mon Sep 17 00:00:00 2001 From: Mark Ekisa Date: Wed, 13 Jan 2021 08:32:28 +0300 Subject: [PATCH 3/3] Code cleaning Signed-off-by: Mark Ekisa --- onadata/apps/api/tests/viewsets/test_xform_viewset.py | 1 - onadata/apps/logger/models/xform.py | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/onadata/apps/api/tests/viewsets/test_xform_viewset.py b/onadata/apps/api/tests/viewsets/test_xform_viewset.py index 45b6fa3cba..c3293ae748 100644 --- a/onadata/apps/api/tests/viewsets/test_xform_viewset.py +++ b/onadata/apps/api/tests/viewsets/test_xform_viewset.py @@ -2694,7 +2694,6 @@ def test_update_xform_using_put_with_invalid_input(self): form_id = self.xform.pk unsanitized_html_str = "

HTML Injection testing

" - version = unsanitized_html_str view = XFormViewSet.as_view({ 'put': 'update', }) diff --git a/onadata/apps/logger/models/xform.py b/onadata/apps/logger/models/xform.py index 3ee55c5c59..8ab0888c66 100644 --- a/onadata/apps/logger/models/xform.py +++ b/onadata/apps/logger/models/xform.py @@ -902,7 +902,8 @@ def save(self, *args, **kwargs): 'in settings sheet or reduce the file name if you do' ' not have a settings sheets.' % self.MAX_ID_LENGTH)) - if contains_xml_invalid_char(self.version): + is_version_available = self.version is not None + if is_version_available and contains_xml_invalid_char(self.version): raise XLSFormError( _("Version shouldn't have any invalid " "characters ('>' '&' '<')"))