From a5ba39393e2ddeb95a0bad190cef3ad9bbf12c88 Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Thu, 3 Sep 2026 00:55:58 -0400 Subject: [PATCH] Escape the IMEX plate attributes written into the 3MF Closes review comment 1. imex_parallel_mode and imex_head_filament_map were streamed raw into XML attribute values, while every other free-text attribute in the same writer goes through xml_escape. Mode names are free text, so "PLA & ABS", a quote or a "<" made the document malformed. The failure is not a bad value on reload: both load paths for model_settings.config return false on an expat error, and m_is_bbl_3mf is set before the second entry loop runs, so the whole project fails to open with "Archive does not contain a valid model config". Both attributes now use xml_escape_double_quotes_attribute_value(), which also emits tab, CR and LF as numeric character references. That matters and plain xml_escape would not do: XML normalises literal whitespace in attribute values on read, so a tab in a mode name would come back as a space and silently rename the mode. The read side needs no change -- it takes expat's already-decoded value with no second unescape -- so this is a lossless round trip and a file written by the new code still loads in an older build. The round-trip test used "copy_mode", which exercised none of this; it now carries &, <, a quote and a tab, and also pins that ' and > come back unmodified, since both are legal raw inside a double-quoted value. Co-Authored-By: Claude Opus 5 (1M context) --- src/libslic3r/Format/bbs_3mf.cpp | 10 ++++++++-- tests/libslic3r/test_3mf.cpp | 29 +++++++++++++++++++++++------ 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/src/libslic3r/Format/bbs_3mf.cpp b/src/libslic3r/Format/bbs_3mf.cpp index a6467487c4..f3fe29e028 100644 --- a/src/libslic3r/Format/bbs_3mf.cpp +++ b/src/libslic3r/Format/bbs_3mf.cpp @@ -8105,14 +8105,20 @@ void PlateData::parse_filament_info(GCodeProcessorResult *result) stream << " <" << METADATA_TAG << " " << KEY_ATTR << "=\"" << SPIRAL_VASE_MODE << "\" " << VALUE_ATTR << "=\"" << spiral_mode_opt->getBool() << "\"/>\n"; { + // Mode names are user supplied free text, so the value has to be escaped for an + // attribute. xml_escape_double_quotes_attribute_value() is used rather than + // xml_escape() because it also emits tab/CR/LF as numeric character references: + // XML normalizes literal whitespace in attribute values on read, which would + // silently rename the mode. The reader takes the value straight from expat, + // which resolves both entities and character references, so this round-trips. auto* imex_mode_opt = plate_data->config.option("imex_parallel_mode"); if (imex_mode_opt && !imex_mode_opt->value.empty() && imex_mode_opt->value != kImexPrimaryMode) - stream << " <" << METADATA_TAG << " " << KEY_ATTR << "=\"" << IMEX_PARALLEL_MODE_ATTR << "\" " << VALUE_ATTR << "=\"" << imex_mode_opt->value << "\"/>\n"; + stream << " <" << METADATA_TAG << " " << KEY_ATTR << "=\"" << IMEX_PARALLEL_MODE_ATTR << "\" " << VALUE_ATTR << "=\"" << xml_escape_double_quotes_attribute_value(imex_mode_opt->value) << "\"/>\n"; } { auto* imex_hfm_opt = plate_data->config.option("imex_head_filament_map"); if (imex_hfm_opt && !imex_hfm_opt->value.empty()) - stream << " <" << METADATA_TAG << " " << KEY_ATTR << "=\"" << IMEX_HEAD_FILAMENT_MAP_ATTR << "\" " << VALUE_ATTR << "=\"" << imex_hfm_opt->value << "\"/>\n"; + stream << " <" << METADATA_TAG << " " << KEY_ATTR << "=\"" << IMEX_HEAD_FILAMENT_MAP_ATTR << "\" " << VALUE_ATTR << "=\"" << xml_escape_double_quotes_attribute_value(imex_hfm_opt->value) << "\"/>\n"; } //filament map related diff --git a/tests/libslic3r/test_3mf.cpp b/tests/libslic3r/test_3mf.cpp index ea448031f0..f90df77dd7 100644 --- a/tests/libslic3r/test_3mf.cpp +++ b/tests/libslic3r/test_3mf.cpp @@ -504,10 +504,27 @@ SCENARIO("BBS 3MF round-trips per-plate IMEX state (parallel mode + head filamen // Regression guard for the class of bug where per-plate state silently drops through // save/load (the variant-truncation bug was the precipitating example; IMEX plate state // rides the same XML metadata path and is equally vulnerable). + // + // The mode name is user-typed free text, so the values below deliberately carry every + // character class that is special inside an XML attribute value: + // & and < must be escaped or the document is not well formed and the project will + // not load at all, + // " must be escaped or it terminates the attribute early, + // tab must be written as a numeric character reference, because XML normalizes + // literal whitespace in attribute values on read and the mode would be + // silently renamed, + // ' and > are legal raw inside a double-quoted value and must survive untouched. + // The head filament map value is machine generated in practice; it is given hostile + // content here only to pin the escaping of the sibling attribute, so this asserts the + // XML transport, not the map grammar (the loader stores the string verbatim). + // // BBS exporter scaffolds a backup dir under temporary_dir() for the project config file; // point it at a writable location for the test process. set_temporary_dir(boost::filesystem::temp_directory_path().string()); + const std::string hostile_mode = "PLA & ABS \"2x\"\tcopy's mode"; + const std::string hostile_hfm = "1:2,2:3 & <\"\tx>"; + GIVEN("A Model with a single object on plate 0 and IMEX plate state set") { Model src_model; std::string src_file = std::string(TEST_DATA_DIR) + "/test_3mf/Prusa.stl"; @@ -519,8 +536,8 @@ SCENARIO("BBS 3MF round-trips per-plate IMEX state (parallel mode + head filamen PlateDataPtrs src_plates; auto *plate0 = new PlateData(); plate0->plate_index = 0; - plate0->config.set_key_value("imex_parallel_mode", new ConfigOptionString("copy_mode")); - plate0->config.set_key_value("imex_head_filament_map", new ConfigOptionString("1:2,2:3")); + plate0->config.set_key_value("imex_parallel_mode", new ConfigOptionString(hostile_mode)); + plate0->config.set_key_value("imex_head_filament_map", new ConfigOptionString(hostile_hfm)); src_plates.push_back(plate0); WHEN("the model is saved to BBS 3MF and loaded back") { @@ -551,17 +568,17 @@ SCENARIO("BBS 3MF round-trips per-plate IMEX state (parallel mode + head filamen THEN("the loaded plate list has the same number of plates") { REQUIRE(dst_plates.size() == src_plates.size()); } - THEN("imex_parallel_mode round-trips with its original value") { + THEN("imex_parallel_mode round-trips byte for byte, XML metacharacters included") { REQUIRE(dst_plates.size() >= 1); auto *mode_opt = dst_plates[0]->config.option("imex_parallel_mode"); REQUIRE(mode_opt != nullptr); - REQUIRE(mode_opt->value == "copy_mode"); + REQUIRE(mode_opt->value == hostile_mode); } - THEN("imex_head_filament_map round-trips with its original value") { + THEN("imex_head_filament_map round-trips byte for byte, XML metacharacters included") { REQUIRE(dst_plates.size() >= 1); auto *hfm_opt = dst_plates[0]->config.option("imex_head_filament_map"); REQUIRE(hfm_opt != nullptr); - REQUIRE(hfm_opt->value == "1:2,2:3"); + REQUIRE(hfm_opt->value == hostile_hfm); } release_PlateData_list(dst_plates);