From 27fda337f099e582c835b8e7229419b6e87d7ff2 Mon Sep 17 00:00:00 2001 From: rgraber Date: Wed, 8 Jul 2026 11:39:39 -0400 Subject: [PATCH 1/4] fix(import): forbid bad names in import --- kpi/models/import_export_task.py | 12 ++++++++++++ kpi/tests/api/v2/test_api_imports.py | 27 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/kpi/models/import_export_task.py b/kpi/models/import_export_task.py index 8b021b895f..9dc040e569 100644 --- a/kpi/models/import_export_task.py +++ b/kpi/models/import_export_task.py @@ -58,6 +58,7 @@ from kpi.exceptions import ConcurrentExportException, XlsFormatException from kpi.fields import KpiUidField from kpi.models import Asset +from kpi.utils.autoname import _is_group_end from kpi.utils.data_exports import ( ACCESS_LOGS_EXPORT_FIELDS, ASSET_FIELDS, @@ -76,6 +77,7 @@ rename_xls_sheet, rename_xlsx_sheet, ) +from kpi.utils.sluggify import is_valid_node_name from kpi.utils.storage import is_filesystem_storage from kpi.utils.strings import to_str from kpi.zip_importer import HttpContentParse @@ -362,6 +364,11 @@ def _load_assets_from_url(self, url, messages, **kwargs): kontent = xlsx_to_dict(item.readable) except InvalidFileException: kontent = xls_to_dict(item.readable) + survey_list = kontent.get('survey') + for node in survey_list: + name = node.get('name') + if not _is_group_end(node) and not is_valid_node_name(name): + raise ValueError(f'Invalid node name: {name}') if not destination: extra_args['content'] = _strip_header_keys(kontent) @@ -405,6 +412,11 @@ def _parse_b64_upload(self, base64_encoded_upload, messages, **kwargs): library = kwargs.get('library') survey_dict = _b64_xls_to_dict(base64_encoded_upload) survey_dict_keys = survey_dict.keys() + survey_list = survey_dict.get('survey', []) + for node in survey_list: + name = node.get('name') + if not _is_group_end(node) and not is_valid_node_name(name): + raise ValueError(f'Invalid node name: {name}') destination = kwargs.get('destination', False) has_necessary_perm = kwargs.get('has_necessary_perm', False) diff --git a/kpi/tests/api/v2/test_api_imports.py b/kpi/tests/api/v2/test_api_imports.py index f82d0c69d7..6478c85d37 100644 --- a/kpi/tests/api/v2/test_api_imports.py +++ b/kpi/tests/api/v2/test_api_imports.py @@ -13,6 +13,7 @@ from kobo.apps.kobo_auth.shortcuts import User from kpi.constants import ASSET_TYPE_BLOCK, ASSET_TYPE_QUESTION from kpi.models import Asset, ImportTask +from kpi.models.import_export_task import ImportExportStatusChoices from kpi.tests.base_test_case import BaseTestCase from kpi.urls.router_api_v2 import URL_NAMESPACE as ROUTER_URL_NAMESPACE from kpi.utils.strings import to_str @@ -1087,3 +1088,29 @@ def test_import_to_existing_asset_base64_xls_records_audit_log_info(self): 'project_owner': self.asset.owner.username, }, ) + + def test_import_raises_error_with_invalid_names(self): + survey_sheet_content = [ + ['type', 'name', 'label::English (en)'], + ['today', 'today', ''], + ['begin group', 'this is a bad group name', 'Bad bad bad, no donut'], + ['note', 'note_1', 'Hi there 👋'], + ['end group', '', ''], + ] + choices_sheet_content = [ + ['list_name', 'name', 'label::English (en)'], + ] + content = ( + ('survey', survey_sheet_content), + ('choices', choices_sheet_content), + ) + + task_data = self._construct_xls_for_import(content, name='Bad file') + encoded_str = task_data['base64Encoded'] + encoded_substr = encoded_str[encoded_str.index('base64') + 7 :] + task_data['base64Encoded'] = encoded_substr + task = ImportTask.objects.create(user=self.asset.owner, data=task_data) + result = task.run() + assert result.status == ImportExportStatusChoices.ERROR + error_message = result.messages['error'] + assert 'this is a bad group name' in error_message From 4e2f741ef9ce83a306ef7dbb455944492f13dca1 Mon Sep 17 00:00:00 2001 From: rgraber Date: Wed, 8 Jul 2026 11:50:43 -0400 Subject: [PATCH 2/4] fixup!: darker is confused --- kpi/tests/api/v2/test_api_imports.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kpi/tests/api/v2/test_api_imports.py b/kpi/tests/api/v2/test_api_imports.py index 6478c85d37..007f1c060a 100644 --- a/kpi/tests/api/v2/test_api_imports.py +++ b/kpi/tests/api/v2/test_api_imports.py @@ -1107,7 +1107,7 @@ def test_import_raises_error_with_invalid_names(self): task_data = self._construct_xls_for_import(content, name='Bad file') encoded_str = task_data['base64Encoded'] - encoded_substr = encoded_str[encoded_str.index('base64') + 7 :] + encoded_substr = encoded_str[encoded_str.index('base64') + 7:] task_data['base64Encoded'] = encoded_substr task = ImportTask.objects.create(user=self.asset.owner, data=task_data) result = task.run() From 63e88c689b2304a6c6a5ba2b7e86aa1cadab79b0 Mon Sep 17 00:00:00 2001 From: rgraber Date: Wed, 8 Jul 2026 12:18:36 -0400 Subject: [PATCH 3/4] fixup!: touche, reptile --- kpi/models/import_export_task.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/kpi/models/import_export_task.py b/kpi/models/import_export_task.py index 9dc040e569..9189f2217b 100644 --- a/kpi/models/import_export_task.py +++ b/kpi/models/import_export_task.py @@ -364,10 +364,14 @@ def _load_assets_from_url(self, url, messages, **kwargs): kontent = xlsx_to_dict(item.readable) except InvalidFileException: kontent = xls_to_dict(item.readable) - survey_list = kontent.get('survey') + survey_list = kontent.get('survey', []) for node in survey_list: name = node.get('name') - if not _is_group_end(node) and not is_valid_node_name(name): + if ( + bool(name) + and not _is_group_end(node) + and not is_valid_node_name(name) + ): raise ValueError(f'Invalid node name: {name}') if not destination: @@ -415,7 +419,7 @@ def _parse_b64_upload(self, base64_encoded_upload, messages, **kwargs): survey_list = survey_dict.get('survey', []) for node in survey_list: name = node.get('name') - if not _is_group_end(node) and not is_valid_node_name(name): + if bool(name) and not _is_group_end(node) and not is_valid_node_name(name): raise ValueError(f'Invalid node name: {name}') destination = kwargs.get('destination', False) From cc6635dd0600a6d39be6122b17979d7585f0aa8a Mon Sep 17 00:00:00 2001 From: rgraber Date: Fri, 10 Jul 2026 11:49:47 -0400 Subject: [PATCH 4/4] fixup!: changes from review --- kpi/models/import_export_task.py | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/kpi/models/import_export_task.py b/kpi/models/import_export_task.py index 9189f2217b..44039ff608 100644 --- a/kpi/models/import_export_task.py +++ b/kpi/models/import_export_task.py @@ -364,15 +364,7 @@ def _load_assets_from_url(self, url, messages, **kwargs): kontent = xlsx_to_dict(item.readable) except InvalidFileException: kontent = xls_to_dict(item.readable) - survey_list = kontent.get('survey', []) - for node in survey_list: - name = node.get('name') - if ( - bool(name) - and not _is_group_end(node) - and not is_valid_node_name(name) - ): - raise ValueError(f'Invalid node name: {name}') + self._ensure_valid_node_names(kontent) if not destination: extra_args['content'] = _strip_header_keys(kontent) @@ -405,6 +397,14 @@ def _load_assets_from_url(self, url, messages, **kwargs): orm_obj.parent = parent_item orm_obj.save() + @staticmethod + def _ensure_valid_node_names(survey_dict): + survey_list = survey_dict.get('survey', []) + for node in survey_list: + name = node.get('name') + if bool(name) and not _is_group_end(node) and not is_valid_node_name(name): + raise ValueError(f'Invalid node name: {name}') + def _parse_b64_upload(self, base64_encoded_upload, messages, **kwargs): filename = kwargs.get('filename', False) desired_type = kwargs.get('desired_type') @@ -415,12 +415,8 @@ def _parse_b64_upload(self, base64_encoded_upload, messages, **kwargs): filename = '' library = kwargs.get('library') survey_dict = _b64_xls_to_dict(base64_encoded_upload) + self._ensure_valid_node_names(survey_dict) survey_dict_keys = survey_dict.keys() - survey_list = survey_dict.get('survey', []) - for node in survey_list: - name = node.get('name') - if bool(name) and not _is_group_end(node) and not is_valid_node_name(name): - raise ValueError(f'Invalid node name: {name}') destination = kwargs.get('destination', False) has_necessary_perm = kwargs.get('has_necessary_perm', False)