From 3c81165c1bad4402cceb0a3b3fd57cd19ecf1339 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 30 Sep 2026 20:06:18 +0000 Subject: [PATCH 1/3] fix: parse QTI items to rewrite them at publish instead of matching regexes - set_qti_item_language finds the real root past prolog comments (#6235) and on a namespace-prefixed root (#6236). - rewrite_qti_media_paths rewrites a src written as an entity or character reference (#6240). - Items are parsed with the validator's parser, so a validated item publishes. - Published items are lxml's serialization, not the authored bytes; canonical XML is unchanged for valid items. - A converted item that is not well-formed XML is logged and left out. Co-Authored-By: Claude Opus 5.5 --- .../tests/utils/qti/test_media.py | 186 ++++++++++++------ .../tests/utils/test_exercise_creation.py | 102 ++++++++-- .../utils/assessment/qti/archive.py | 10 +- .../utils/assessment/qti/media.py | 131 ++++-------- 4 files changed, 261 insertions(+), 168 deletions(-) diff --git a/contentcuration/contentcuration/tests/utils/qti/test_media.py b/contentcuration/contentcuration/tests/utils/qti/test_media.py index 48b8320e1d..9043b263cb 100644 --- a/contentcuration/contentcuration/tests/utils/qti/test_media.py +++ b/contentcuration/contentcuration/tests/utils/qti/test_media.py @@ -4,6 +4,7 @@ from contentcuration.utils.assessment.qti.media import rewrite_qti_sized_image_paths from contentcuration.utils.assessment.qti.media import set_qti_item_language from contentcuration.utils.assessment.qti.media import strip_studio_attributes +from contentcuration.utils.assessment.qti.media import XML_LANG_ATTRIBUTE from contentcuration.utils.assessment.qti.validation import parse_qti_xml from contentcuration.utils.assessment.qti.validation import validate_qti_item @@ -39,29 +40,43 @@ def test_accepts_bytes(): assert get_qti_media_references(xml) == {f"{CHECKSUM_A}.png"} +def _root(xml): + return parse_qti_xml(xml.encode("utf-8")).getroot() + + +def _attribute_values(xml, attribute): + return [ + element.get(attribute) + for element in _root(xml).iter() + if attribute in element.attrib + ] + + def test_rewrite_leaves_input_untouched_with_no_mapping(): xml = f'' assert rewrite_qti_media_paths(xml, {}) == xml -def test_rewrite_remaps_src_href_data_and_preserves_formatting(): +def test_rewrite_remaps_src_href_data(): xml = ( f'diagram' f'x' f'' ) - result = rewrite_qti_media_paths( - xml, - { - f"{CHECKSUM_A}.png": f"images/{CHECKSUM_A}.png", - f"{CHECKSUM_B}.pdf": f"images/{CHECKSUM_B}.pdf", - }, - ) - assert result == ( - f'diagram' - f'x' - f'' + result = _root( + rewrite_qti_media_paths( + xml, + { + f"{CHECKSUM_A}.png": f"images/{CHECKSUM_A}.png", + f"{CHECKSUM_B}.pdf": f"images/{CHECKSUM_B}.pdf", + }, + ) ) + img, a, obj = result + assert img.attrib == {"src": f"images/{CHECKSUM_A}.png", "alt": "diagram"} + assert a.get("href") == f"images/{CHECKSUM_B}.pdf" + assert a.text == "x" + assert obj.get("data") == f"images/{CHECKSUM_A}.png" def test_rewrite_remaps_srcset_entries_preserving_descriptors(): @@ -73,9 +88,9 @@ def test_rewrite_remaps_srcset_entries_preserving_descriptors(): f"{CHECKSUM_B}.png": f"images/{CHECKSUM_B}.png", }, ) - assert result == ( - f'' - ) + assert _attribute_values(result, "srcset") == [ + f"images/{CHECKSUM_A}.png 1x, images/{CHECKSUM_B}.png 2x" + ] def test_rewrite_ignores_values_not_in_mapping(): @@ -83,9 +98,29 @@ def test_rewrite_ignores_values_not_in_mapping(): result = rewrite_qti_media_paths( xml, {f"{CHECKSUM_A}.png": f"images/{CHECKSUM_A}.png"} ) - assert result == ( - f'x' + assert _attribute_values(result, "src") == [f"images/{CHECKSUM_A}.png"] + assert _attribute_values(result, "href") == [f"{CHECKSUM_B}.pdf"] + + +def test_rewrite_remaps_a_src_written_with_a_character_reference(): + xml = f'' + result = rewrite_qti_media_paths( + xml, {f"{CHECKSUM_A}.png": f"images/{CHECKSUM_A}.png"} + ) + assert _attribute_values(result, "src") == [f"images/{CHECKSUM_A}.png"] + + +def test_rewrite_remaps_a_src_written_as_an_entity(): + declaration, _, item = VALID_CHOICE_ITEM.partition("?>") + xml = ( + f'{declaration}?>]>' + + item.replace("", 'x') + ) + result = rewrite_qti_media_paths( + xml, {f"{CHECKSUM_A}.png": f"images/{CHECKSUM_A}.png"} ) + assert _attribute_values(result, "src") == [f"images/{CHECKSUM_A}.png"] + assert "&e;" not in result def _path_for_size(filename, width, height): @@ -98,11 +133,24 @@ def test_sized_rewrite_points_each_sized_img_at_its_size(): f"" f'' ) - assert rewrite_qti_sized_image_paths(xml, _path_for_size) == ( - f'\n a > b' - f"" - f'' + assert _attribute_values( + rewrite_qti_sized_image_paths(xml, _path_for_size), "src" + ) == [ + f"images/200x150-{CHECKSUM_A}.png", + f"images/100x75-{CHECKSUM_A}.png", + f"images/200x150-{CHECKSUM_B}.png", + ] + + +def test_sized_rewrite_points_a_namespaced_img_at_its_size(): + result = rewrite_qti_sized_image_paths( + VALID_CHOICE_ITEM.replace( + "", + f'

', + ), + _path_for_size, ) + assert _attribute_values(result, "src") == [f"images/200x150-{CHECKSUM_A}.png"] def test_sized_rewrite_leaves_imgs_without_a_pixel_size_or_checksum_src(): @@ -117,7 +165,10 @@ def test_sized_rewrite_leaves_imgs_without_a_pixel_size_or_checksum_src(): '' f'' ) - assert rewrite_qti_sized_image_paths(xml, _path_for_size) == xml + result = _root(rewrite_qti_sized_image_paths(xml, _path_for_size)) + assert [element.attrib for element in result] == [ + element.attrib for element in _root(xml) + ] def test_sized_rewrite_leaves_imgs_in_comments_and_cdata(): @@ -127,10 +178,9 @@ def test_sized_rewrite_leaves_imgs_in_comments_and_cdata(): f"{hidden}" f'' ) - assert rewrite_qti_sized_image_paths(xml, _path_for_size) == ( - f"{hidden}" - f'' - ) + result = rewrite_qti_sized_image_paths(xml, _path_for_size) + assert sized_img in result.replace("<", "<").replace(">", ">") + assert _attribute_values(result, "src") == [f"images/100x75-{CHECKSUM_A}.png"] ITEM_WITHOUT_LANGUAGE = ( @@ -141,58 +191,35 @@ def test_sized_rewrite_leaves_imgs_in_comments_and_cdata(): ) +def _language(xml): + return _root(xml).get(XML_LANG_ATTRIBUTE) + + def test_set_language_adds_it_when_the_item_has_none(): result = set_qti_item_language(ITEM_WITHOUT_LANGUAGE, "es") - assert 'xml:lang="es"' in result + assert _language(result) == "es" assert "

Body

" in result def test_set_language_replaces_a_language_the_item_already_had(): already = ITEM_WITHOUT_LANGUAGE.replace('title="t"', 'title="t" xml:lang="en"') - result = set_qti_item_language(already, "sw") - # Rewritten where it stands, so the value is the only thing that differs. - assert result == already.replace('xml:lang="en"', 'xml:lang="sw"') - - -def test_set_language_replaces_a_single_quoted_language(): - already = ITEM_WITHOUT_LANGUAGE.replace('title="t"', "title=\"t\" xml:lang='en'") - result = set_qti_item_language(already, "sw") - assert result == already.replace("xml:lang='en'", 'xml:lang="sw"') - parse_qti_xml(result.encode("utf-8")) - - -def test_set_language_replaces_a_language_containing_the_other_quote(): - already = ITEM_WITHOUT_LANGUAGE.replace('title="t"', 'title="t" xml:lang="it\'s"') - result = set_qti_item_language(already, "sw") - assert result == already.replace('xml:lang="it\'s"', 'xml:lang="sw"') - parse_qti_xml(result.encode("utf-8")) - - -def test_set_language_replaces_a_language_with_spaces_around_the_equals_sign(): - already = ITEM_WITHOUT_LANGUAGE.replace('title="t"', "title=\"t\" xml:lang = 'en'") - result = set_qti_item_language(already, "sw") - assert result == already.replace("xml:lang = 'en'", 'xml:lang="sw"') + assert _language(set_qti_item_language(already, "sw")) == "sw" def test_set_language_finds_the_language_after_a_greater_than_in_an_attribute(): already = ITEM_WITHOUT_LANGUAGE.replace( 'title="t"', "title=\"a > b\" xml:lang='en'" ) - result = set_qti_item_language(already, "sw") - assert result == already.replace("xml:lang='en'", 'xml:lang="sw"') + root = _root(set_qti_item_language(already, "sw")) + assert root.get(XML_LANG_ATTRIBUTE) == "sw" + assert root.get("title") == "a > b" def test_set_language_ignores_language_text_inside_another_attribute_value(): tricky = ITEM_WITHOUT_LANGUAGE.replace('title="t"', "title=\"see xml:lang='x'\"") - result = set_qti_item_language(tricky, "sw") - assert result == tricky.replace( - 'time-dependent="false">', 'time-dependent="false" xml:lang="sw">' - ) - - -def test_set_language_leaves_an_item_already_declaring_it_byte_for_byte(): - already = ITEM_WITHOUT_LANGUAGE.replace('title="t"', 'title="t" xml:lang="sw"') - assert set_qti_item_language(already, "sw") == already + root = _root(set_qti_item_language(tricky, "sw")) + assert root.get(XML_LANG_ATTRIBUTE) == "sw" + assert root.get("title") == "see xml:lang='x'" def test_set_language_leaves_the_item_alone_without_a_language_to_set(): @@ -213,6 +240,25 @@ def test_set_language_keeps_the_item_schema_valid(): assert validation.is_valid, validation.errors +def test_set_language_sets_it_on_a_namespace_prefixed_root(): + prefixed = ( + ITEM_WITHOUT_LANGUAGE.replace("") + comment = '' + result = set_qti_item_language(f"{declaration}?>{comment}{root}", "fr") + assert comment in result + assert _language(result) == "fr" + validation = validate_qti_item(result) + assert validation.is_valid, validation.errors + + def test_strip_studio_attributes_removes_every_one_and_nothing_else(): marked = ITEM_WITHOUT_LANGUAGE.replace( "

Body

", @@ -221,10 +267,15 @@ def test_strip_studio_attributes_removes_every_one_and_nothing_else(): "data-studio-sentinel='' shuffle=\"false\">" '

', ) - result = strip_studio_attributes(marked) - assert result == marked.replace(' data-studio-prompt=""', "").replace( - " data-studio-sentinel=''", "" + result = _root(strip_studio_attributes(marked)) + expected = _root( + marked.replace(' data-studio-prompt=""', "").replace( + " data-studio-sentinel=''", "" + ) ) + assert [(e.tag, e.attrib, e.text) for e in result.iter()] == [ + (e.tag, e.attrib, e.text) for e in expected.iter() + ] def test_strip_studio_attributes_ignores_lookalikes_in_values_and_text(): @@ -232,4 +283,9 @@ def test_strip_studio_attributes_ignores_lookalikes_in_values_and_text(): "

Body

", '

data-studio-prompt=""

', ) - assert strip_studio_attributes(tricky) == tricky + paragraph = _root(strip_studio_attributes(tricky)).find(".//{*}p") + assert paragraph.attrib == { + "title": "a > b data-studio-prompt=''", + "data-other": "1", + } + assert paragraph.text == 'data-studio-prompt=""' diff --git a/contentcuration/contentcuration/tests/utils/test_exercise_creation.py b/contentcuration/contentcuration/tests/utils/test_exercise_creation.py index 5ffd864422..701f39b4fc 100644 --- a/contentcuration/contentcuration/tests/utils/test_exercise_creation.py +++ b/contentcuration/contentcuration/tests/utils/test_exercise_creation.py @@ -43,6 +43,12 @@ UNRESIZABLE_IMAGE_BYTES = b"not an image" +def _canonical_xml(xml_string): + return etree.tostring( + etree.fromstring(_normalize_xml(xml_string).encode("utf-8")), method="c14n" + ) + + def _create_unresizable_image(): return create_studio_file( UNRESIZABLE_IMAGE_BYTES, preset=format_presets.EXERCISE_IMAGE, ext="jpg" @@ -2201,10 +2207,9 @@ def test_image_resizing(self): """ - # Compare normalized XML self.assertEqual( - _normalize_xml(expected_item_xml), - _normalize_xml(actual_item_xml), + _canonical_xml(expected_item_xml), + _canonical_xml(actual_item_xml), ) def test_image_resize_failure_uses_original(self): @@ -2244,6 +2249,32 @@ def test_image_resize_failure_uses_original(self): ).decode("utf-8") self.assertIn(f'src="images/{bad_image.filename()}"', item_xml) + def test_legacy_item_that_is_not_well_formed_xml_is_skipped(self): + """A converted item with a sized image whose text isn't legal XML is logged and + excluded, not fatal to publish.""" + image_file = create_studio_file( + _png_bytes((400, 300)), preset=format_presets.EXERCISE_IMAGE, ext="png" + )["db_file"] + image_url = exercises.CONTENT_STORAGE_FORMAT.format(image_file.filename()) + item = self._create_assessment_item( + exercises.INPUT_QUESTION, + f"![x]({image_url} =60x45)", + [{"answer": "a\x0bb", "correct": True, "order": 1}], + ) + image_file.assessment_item = item + image_file.save() + + with self.assertLogs(level="ERROR") as logs: + self._create_qti_zip(self._exercise_data([item])) + self.assertTrue( + any("is not well-formed XML" in message for message in logs.output) + ) + exercise_file = self.exercise_node.files.get(preset_id=format_presets.QTI_ZIP) + zip_file = self._validate_qti_zip_structure(exercise_file) + self.assertNotIn( + f"items/{hex_to_qti_id(item.assessment_id)}.xml", zip_file.namelist() + ) + def _publish_legacy_sized_image(self, content, ext, width, height, ref_ext=None): image_file = create_studio_file( content, preset=format_presets.EXERCISE_IMAGE, ext=ext @@ -2509,12 +2540,10 @@ def test_manifest_structure_single_item(self): _normalize_xml(actual_manifest_xml), ) - def test_native_qti_item_written_verbatim(self): - """The item XML in the zip is the authored raw_data, not a rebuild of it. - - Publishing rewrites the root's xml:lang to the node's language, which here is the - language the item already declares, so every byte of it — formatting included — has - to survive. Stamping an item that declares none is covered in test_exportchannel. + def test_native_qti_item_written_as_authored(self): + """Publishing rewrites the root's xml:lang to the node's language, which here is the + language the item already declares, so the item is otherwise unchanged. Stamping an + item that declares none is covered in test_exportchannel. """ raw_data = _item_xml( "native_item_1", @@ -2542,7 +2571,7 @@ def test_native_qti_item_written_verbatim(self): zip_file = self._validate_qti_zip_structure(exercise_file) self.assertIn("items/native_item_1.xml", zip_file.namelist()) item_xml = zip_file.read("items/native_item_1.xml").decode("utf-8") - self.assertEqual(item_xml, raw_data) + self.assertEqual(_canonical_xml(item_xml), _canonical_xml(raw_data)) def test_native_qti_item_published_without_studio_attributes(self): """The QTI editor's data-studio-* markers are for reopening an item, not for Kolibri.""" @@ -2570,7 +2599,10 @@ def test_native_qti_item_published_without_studio_attributes(self): exercise_file = self.exercise_node.files.get(preset_id=format_presets.QTI_ZIP) zip_file = self._validate_qti_zip_structure(exercise_file) item_xml = zip_file.read("items/native_item_1.xml").decode("utf-8") - self.assertEqual(item_xml, raw_data.replace(' data-studio-prompt=""', "")) + self.assertEqual( + _canonical_xml(item_xml), + _canonical_xml(raw_data.replace(' data-studio-prompt=""', "")), + ) def test_native_qti_item_media_included_and_addressed(self): # fileobj_exercise_image() writes real bytes to storage keyed by their @@ -2606,10 +2638,42 @@ def test_native_qti_item_media_included_and_addressed(self): item_xml = zip_file.read("items/native_item_1.xml").decode("utf-8") # The media path is remapped; nothing else about the item changes. self.assertEqual( - item_xml, - raw_data.replace(media_filename, f"images/{media_filename}"), + _canonical_xml(item_xml), + _canonical_xml( + raw_data.replace(media_filename, f"images/{media_filename}") + ), ) + def test_native_qti_item_media_written_as_an_entity_is_addressed(self): + image_file = fileobj_exercise_image() + media_filename = f"{image_file.checksum}.{image_file.file_format_id}" + declaration, _, item_body = self.NATIVE_ITEM_XML.format( + checksum=image_file.checksum, ext=image_file.file_format_id + ).partition("?>") + raw_data = ( + f'{declaration}?>]>' + + item_body.replace(f'src="{media_filename}"', 'src="&e;"') + ) + item = self._create_native_qti_item(raw_data) + image_file.assessment_item = item + image_file.save() + exercise_data = { + "mastery_model": exercises.M_OF_N, + "randomize": True, + "n": 5, + "m": 3, + "all_assessment_items": [item.assessment_id], + "assessment_mapping": {item.assessment_id: exercises.QTI}, + } + self._create_qti_zip(exercise_data) + exercise_file = self.exercise_node.files.get(preset_id=format_presets.QTI_ZIP) + zip_file = self._validate_qti_zip_structure(exercise_file) + self.assertIn(f"items/images/{media_filename}", zip_file.namelist()) + item_xml = zip_file.read("items/native_item_1.xml").decode("utf-8") + self.assertNotIn("&e;", item_xml) + img = parse_qti_xml(item_xml.encode("utf-8")).find(".//{*}img") + self.assertEqual(img.get("src"), f"images/{media_filename}") + def test_native_qti_item_invalid_raw_data_is_skipped(self): """An item that fails schema validation is logged and excluded, not fatal to publish.""" invalid_raw_data = VALID_CHOICE_ITEM.replace( @@ -2635,6 +2699,18 @@ def test_native_qti_item_invalid_raw_data_is_skipped(self): [name for name in zip_file.namelist() if name.startswith("items/")], [] ) + def test_native_qti_item_valid_only_in_its_declared_encoding_is_packaged(self): + """U+FFFF's UTF-8 bytes are legal XML text read as Latin-1: validation passes the + item, so publishing must read it the same way.""" + raw_data = VALID_CHOICE_ITEM.replace( + 'encoding="UTF-8"', 'encoding="ISO-8859-1"' + ).replace("", "￿") + item = self._create_native_qti_item(raw_data) + self._create_qti_zip(self._exercise_data([item])) + exercise_file = self.exercise_node.files.get(preset_id=format_presets.QTI_ZIP) + zip_file = self._validate_qti_zip_structure(exercise_file) + self.assertIn("items/item_1.xml", zip_file.namelist()) + def _exercise_data(self, items): return { "mastery_model": exercises.M_OF_N, diff --git a/contentcuration/contentcuration/utils/assessment/qti/archive.py b/contentcuration/contentcuration/utils/assessment/qti/archive.py index af77f65d21..1225fb7219 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/archive.py +++ b/contentcuration/contentcuration/utils/assessment/qti/archive.py @@ -10,6 +10,7 @@ from le_utils.constants import exercises from le_utils.constants import format_presets +from lxml import etree from contentcuration.utils.assessment.base import ExerciseArchiveGenerator from contentcuration.utils.assessment.qti.constants import ResourceType @@ -299,7 +300,14 @@ def create_assessment_item( path_by_filename = { os.path.basename(image["name"]): image["name"] for image in image_entries } - item_xml = rewrite_qti_media_paths(result.xml, path_by_filename) + try: + item_xml = rewrite_qti_media_paths(result.xml, path_by_filename) + except etree.XMLSyntaxError as e: + logging.error( + f"QTI item {assessment_item.assessment_id} on node {self.ccnode.pk} " + f"is not well-formed XML and will be excluded from the package: {e}" + ) + return None file_dependencies = list( dict.fromkeys(path_by_filename.get(d, d) for d in result.file_dependencies) ) diff --git a/contentcuration/contentcuration/utils/assessment/qti/media.py b/contentcuration/contentcuration/utils/assessment/qti/media.py index a2c2a1d607..9c38c0d318 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/media.py +++ b/contentcuration/contentcuration/utils/assessment/qti/media.py @@ -16,32 +16,8 @@ + " or ".join(f"@{attribute}" for attribute in QTI_REFERENCE_ATTRIBUTES) + " or @srcset]" ) -ITEM_ROOT_START_TAG_REGEX = re.compile( - r""""'])*>""" -) -# Quoted values are matched first so a lookalike inside another attribute is skipped. -XML_LANG_OR_QUOTED_VALUE_REGEX = re.compile( - r"""(?P\s+xml:lang\s*=\s*(?:"[^"]*"|'[^']*'))|"[^"]*"|'[^']*'""" -) -START_TAG_REGEX = re.compile(r"""<[A-Za-z_](?:"[^"]*"|'[^']*'|[^>"'])*>""") -STUDIO_ATTRIBUTE_OR_QUOTED_VALUE_REGEX = re.compile( - r"""(?P\s+data-studio-[\w.-]+\s*=\s*(?:"[^"]*"|'[^']*'))|"[^"]*"|'[^']*'""" -) PIXEL_LENGTH_REGEX = re.compile(r"[1-9][0-9]*") -# Comments and CDATA match whole, so an inside one is never read as a start tag -COMMENT_CDATA_OR_IMG_START_TAG_REGEX = re.compile( - r"""||])(?:[^>"']|"[^"]*"|'[^']*')*>""", - re.DOTALL, -) -XML_ATTRIBUTE_REGEX = re.compile( - r"""\s(?P[^\s=/>]+)\s*=\s*(?P["'])(?P.*?)(?P=quote)""", - re.DOTALL, -) - -QTI_MEDIA_ATTRIBUTE_VALUE_REGEX = re.compile( - r"(?P" + "|".join(QTI_REFERENCE_ATTRIBUTES + ("srcset",)) + r")" - r'(?P\s*=\s*)(?P["\'])(?P[^"\']*)(?P=quote)' -) +XML_LANG_ATTRIBUTE = "{http://www.w3.org/XML/1998/namespace}lang" def img_pixel_size(width, height) -> Optional[Tuple[int, int]]: @@ -54,6 +30,14 @@ def img_pixel_size(width, height) -> Optional[Tuple[int, int]]: return int(width), int(height) +def _parse(raw_data): + return parse_qti_xml(raw_data.encode("utf-8")) + + +def _serialize(doc): + return etree.tostring(doc, encoding="UTF-8", xml_declaration=True).decode("utf-8") + + def get_qti_media_references(raw_data): """ Scan QTI item XML for . media references in src/href/data/srcset @@ -86,10 +70,7 @@ def get_qti_media_references(raw_data): def rewrite_qti_media_paths(raw_data, path_by_filename): """ Rewrite src/href/data/srcset attribute values referencing keys of - `path_by_filename` to the corresponding new path. Operates as a targeted - text substitution rather than a parse/serialize round-trip, so every other - byte of `raw_data` (formatting, attribute order, self-closing tag style, - etc.) is left untouched. + `path_by_filename` to the corresponding new path. """ if not path_by_filename: return raw_data @@ -101,44 +82,36 @@ def _replace_srcset_entry(match): return match.group(0) return match.group(0).replace(filename, new_path, 1) - def _replace_attribute(match): - attribute, eq, quote, value = match.group("attr", "eq", "quote", "value") - if attribute == "srcset": - value = re.sub(srcset_entry_pattern, _replace_srcset_entry, value) - elif value in path_by_filename: - value = path_by_filename[value] - return f"{attribute}{eq}{quote}{value}{quote}" - - return QTI_MEDIA_ATTRIBUTE_VALUE_REGEX.sub(_replace_attribute, raw_data) + doc = _parse(raw_data) + for element in QTI_MEDIA_REFERENCE_XPATH(doc): + for attribute in QTI_REFERENCE_ATTRIBUTES: + value = element.get(attribute) + if value in path_by_filename: + element.set(attribute, path_by_filename[value]) + srcset = element.get("srcset") + if srcset: + element.set( + "srcset", re.sub(srcset_entry_pattern, _replace_srcset_entry, srcset) + ) + return _serialize(doc) def rewrite_qti_sized_image_paths(raw_data, path_for_size): """ Point the ``src`` of each ```` with a pixel size at ``path_for_size(filename, width, height)``, per element so one image at two - sizes gets two paths. A ``None`` path leaves the element alone. Every other - byte is kept, as in ``rewrite_qti_media_paths``. + sizes gets two paths. A ``None`` path leaves the element alone. """ - - def _replace_img(match): - tag = match.group(0) - if tag.startswith("" - - return ITEM_ROOT_START_TAG_REGEX.sub(_replace_root, raw_data, count=1) + doc = _parse(raw_data) + doc.getroot().set(XML_LANG_ATTRIBUTE, language) + return _serialize(doc) def strip_studio_attributes(raw_data): """ Remove every ``data-studio-*`` attribute, such as the ``data-studio-prompt`` marker the QTI editor writes for its own use when it reopens an item, so none reaches Kolibri. - - Operates as a targeted text substitution on start tags, for the same reason - ``rewrite_qti_media_paths`` does. """ if "data-studio-" not in raw_data: return raw_data - - def _strip_attribute(match): - return "" if match.group("studio") else match.group(0) - - def _strip_tag(match): - return STUDIO_ATTRIBUTE_OR_QUOTED_VALUE_REGEX.sub( - _strip_attribute, match.group(0) - ) - - return START_TAG_REGEX.sub(_strip_tag, raw_data) + doc = _parse(raw_data) + for element in doc.iter(etree.Element): + for name in list(element.attrib): + if name.startswith("data-studio-"): + del element.attrib[name] + return _serialize(doc) From 65c30fdf1f577593dbf59b91d231aeab8281e4dd Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 7 Oct 2026 05:33:42 +0000 Subject: [PATCH 2/3] fix: parse converted legacy markup with the validator's secure parser Co-Authored-By: Claude Opus 5.5 --- .../contentcuration/utils/assessment/qti/convert.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contentcuration/contentcuration/utils/assessment/qti/convert.py b/contentcuration/contentcuration/utils/assessment/qti/convert.py index 311334b02f..743c7a3f41 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/convert.py +++ b/contentcuration/contentcuration/utils/assessment/qti/convert.py @@ -57,6 +57,7 @@ TextEntryInteraction, ) from contentcuration.utils.assessment.qti.prompt import Prompt +from contentcuration.utils.assessment.qti.validation import secure_parser from contentcuration.utils.parser import extract_value @@ -165,7 +166,7 @@ def _adapt_unsupported_markup(markup: str) -> str: Runs on the rendered markup, so tags typed as raw HTML are adapted too — an author's own style attribute included, which is filtered rather than refused. """ - root = etree.fromstring(f"{markup}") + root = etree.fromstring(f"{markup}", secure_parser()) for element in list(root.iter("*")): if element.get("style") is not None: _filter_style(element) From 6a9d16acc86f143662a637cc1e35f3a5a45bca96 Mon Sep 17 00:00:00 2001 From: rtibblesbot Date: Wed, 7 Oct 2026 05:34:27 +0000 Subject: [PATCH 3/3] refactor: let parse_qti_xml take text and encode it itself Co-Authored-By: Claude Opus 5.5 --- .../utils/assessment/qti/archive.py | 6 ++---- .../contentcuration/utils/assessment/qti/ingest.py | 2 -- .../contentcuration/utils/assessment/qti/media.py | 14 ++++---------- .../utils/assessment/qti/perseus_derive.py | 2 +- .../utils/assessment/qti/validation.py | 8 ++++---- 5 files changed, 11 insertions(+), 21 deletions(-) diff --git a/contentcuration/contentcuration/utils/assessment/qti/archive.py b/contentcuration/contentcuration/utils/assessment/qti/archive.py index 1225fb7219..a979125658 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/archive.py +++ b/contentcuration/contentcuration/utils/assessment/qti/archive.py @@ -111,9 +111,7 @@ def _skip_answerless_input(self, assessment_item, answers) -> bool: return is_answerless def _create_native_qti_item(self, assessment_item) -> Optional[Tuple[str, bytes]]: - raw_bytes = assessment_item.raw_data.encode("utf-8") - - result = validate_qti_item(raw_bytes) + result = validate_qti_item(assessment_item.raw_data) if not result.is_valid: error_messages = "; ".join( f"line {e.line}, column {e.column}: {e.message}" for e in result.errors @@ -129,7 +127,7 @@ def _create_native_qti_item(self, assessment_item) -> Optional[Tuple[str, bytes] self._warn_answerless(assessment_item) return None - identifier = parse_qti_xml(raw_bytes).getroot().get("identifier") + identifier = parse_qti_xml(assessment_item.raw_data).getroot().get("identifier") if not identifier: raise ValueError( f"QTI item {assessment_item.assessment_id} is missing a root identifier attribute" diff --git a/contentcuration/contentcuration/utils/assessment/qti/ingest.py b/contentcuration/contentcuration/utils/assessment/qti/ingest.py index 4b89c5e020..119999425e 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/ingest.py +++ b/contentcuration/contentcuration/utils/assessment/qti/ingest.py @@ -48,8 +48,6 @@ def convert_legacy_question_to_qti(question_data: dict) -> QTIConversionResult: def find_perseus_custom_interaction_path(raw_data) -> Optional[str]: - if isinstance(raw_data, str): - raw_data = raw_data.encode("utf-8") try: doc = parse_qti_xml(raw_data) except etree.XMLSyntaxError: diff --git a/contentcuration/contentcuration/utils/assessment/qti/media.py b/contentcuration/contentcuration/utils/assessment/qti/media.py index 9c38c0d318..9222eb0e2e 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/media.py +++ b/contentcuration/contentcuration/utils/assessment/qti/media.py @@ -30,10 +30,6 @@ def img_pixel_size(width, height) -> Optional[Tuple[int, int]]: return int(width), int(height) -def _parse(raw_data): - return parse_qti_xml(raw_data.encode("utf-8")) - - def _serialize(doc): return etree.tostring(doc, encoding="UTF-8", xml_declaration=True).decode("utf-8") @@ -43,8 +39,6 @@ def get_qti_media_references(raw_data): Scan QTI item XML for . media references in src/href/data/srcset attributes, matching the TipTap editor's permanentSrc="." convention. """ - if isinstance(raw_data, str): - raw_data = raw_data.encode("utf-8") checksums = set() try: doc = parse_qti_xml(raw_data) @@ -82,7 +76,7 @@ def _replace_srcset_entry(match): return match.group(0) return match.group(0).replace(filename, new_path, 1) - doc = _parse(raw_data) + doc = parse_qti_xml(raw_data) for element in QTI_MEDIA_REFERENCE_XPATH(doc): for attribute in QTI_REFERENCE_ATTRIBUTES: value = element.get(attribute) @@ -102,7 +96,7 @@ def rewrite_qti_sized_image_paths(raw_data, path_for_size): ``path_for_size(filename, width, height)``, per element so one image at two sizes gets two paths. A ``None`` path leaves the element alone. """ - doc = _parse(raw_data) + doc = parse_qti_xml(raw_data) for img in doc.iter("{*}img"): src = img.get("src") size = img_pixel_size(img.get("width"), img.get("height")) @@ -124,7 +118,7 @@ def set_qti_item_language(raw_data, language): """ if not language: return raw_data - doc = _parse(raw_data) + doc = parse_qti_xml(raw_data) doc.getroot().set(XML_LANG_ATTRIBUTE, language) return _serialize(doc) @@ -136,7 +130,7 @@ def strip_studio_attributes(raw_data): """ if "data-studio-" not in raw_data: return raw_data - doc = _parse(raw_data) + doc = parse_qti_xml(raw_data) for element in doc.iter(etree.Element): for name in list(element.attrib): if name.startswith("data-studio-"): diff --git a/contentcuration/contentcuration/utils/assessment/qti/perseus_derive.py b/contentcuration/contentcuration/utils/assessment/qti/perseus_derive.py index e94c8c6a11..85cb556ea6 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/perseus_derive.py +++ b/contentcuration/contentcuration/utils/assessment/qti/perseus_derive.py @@ -214,7 +214,7 @@ def _derive_text(interaction, item_body, declaration): def _parse(raw_data): """Parse untrusted XML; return the item-body element or ``None``.""" try: - root = parse_qti_xml(raw_data.encode("utf-8")).getroot() + root = parse_qti_xml(raw_data).getroot() except etree.XMLSyntaxError: logger.warning("Unable to parse QTI item XML during Perseus derivation") return None, None diff --git a/contentcuration/contentcuration/utils/assessment/qti/validation.py b/contentcuration/contentcuration/utils/assessment/qti/validation.py index 8579320f7a..0d378c3363 100644 --- a/contentcuration/contentcuration/utils/assessment/qti/validation.py +++ b/contentcuration/contentcuration/utils/assessment/qti/validation.py @@ -54,7 +54,10 @@ def secure_parser() -> etree.XMLParser: ) -def parse_qti_xml(xml: bytes) -> etree._Element: +def parse_qti_xml(xml: Union[str, bytes]) -> etree._Element: + if isinstance(xml, str): + # lxml refuses text that carries an encoding declaration. + xml = xml.encode("utf-8") doc = etree.parse(BytesIO(xml), parser=secure_parser()) # Unresolved entity nodes crash schema validation and derivation. entity = next(doc.iter(etree.Entity), None) @@ -66,9 +69,6 @@ def parse_qti_xml(xml: bytes) -> etree._Element: def validate_qti_item(xml: Union[str, bytes]) -> QTIValidationResult: - if isinstance(xml, str): - xml = xml.encode("utf-8") - try: doc = parse_qti_xml(xml) except etree.XMLSyntaxError as exc: