From 43105021e59ef72c16de4a0cdd5403a2cf74128e Mon Sep 17 00:00:00 2001
From: Maximilian Haye <m.haye@tu-berlin.de>
Date: Wed, 20 Nov 2024 16:43:35 +0100
Subject: [PATCH] wip

---
 classes/question_ui_renderer.php    | 42 +++++++++++++++++++++++------
 tests/question_ui_renderer_test.php | 29 ++++++++++++++++++++
 2 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/classes/question_ui_renderer.php b/classes/question_ui_renderer.php
index 45feec1..0162de1 100644
--- a/classes/question_ui_renderer.php
+++ b/classes/question_ui_renderer.php
@@ -18,6 +18,7 @@
 
 use coding_exception;
 use DOMAttr;
+use DOMChildNode;
 use DOMDocument;
 use DOMElement;
 use DOMNameSpaceNode;
@@ -101,17 +102,29 @@ public function render(): string {
 
         mt_srand($id);
         try {
-            $this->resolve_placeholders();
+            // These handle our custom elements and attributes.
             $this->hide_unwanted_feedback();
             $this->hide_if_role();
+            $this->shuffle_contents();
+            $this->format_floats();
+
+            // Remove all unhandled custom elements, attributes, comments, and non-default xmlns declarations.
+            $this->clean_up();
+
+            // These modify standard HTML.
             $this->set_input_values_and_readonly();
             $this->soften_validation();
             $this->defuse_buttons();
-            $this->shuffle_contents();
+
+            // TODO: Do we want to style placeholder expansions? (Which may be student-generated)
             $this->add_styles();
-            $this->format_floats();
+
+            // We don't want to support QPy elements (and attributes, etc.) in placeholder expansions, so we resolve
+            // them after replacing QPy elements.
+            $this->resolve_placeholders();
+            // I'm not sure whether we should support names and IDs in placeholder expansion, but if we do, we should
+            // probably mangle them as well.
             $this->mangle_ids_and_names();
-            $this->clean_up();
         } finally {
             // I'm not sure whether it is strictly necessary to reset the PRNG seed here, but it feels safer.
             // Resetting it to its original state would be ideal, but that doesn't seem to be possible.
@@ -343,7 +356,7 @@ private function clean_up(): void {
      * Replace placeholder PIs such as `<?p my_key plain?>` with the appropriate value from `$this->placeholders`.
      *
      * Since QPy transformations should not be applied to the content of the placeholders, this method should be called
-     * last.
+     * near the end.
      *
      * @return void
      */
@@ -362,9 +375,12 @@ private function resolve_placeholders(): void {
                     // Allow (X)HTML, but clean using Moodle's clean_text to prevent XSS.
                     $element = $this->xpath->document->createDocumentFragment();
                     $element->appendXML(clean_text($rawvalue));
+                    $pi->parentNode->replaceChild($element, $pi);
                 } else if (strcasecmp($cleanoption, 'noclean') == 0) {
-                    $element = $this->xpath->document->createDocumentFragment();
-                    $element->appendXML($rawvalue);
+                    // TODO: DocumentFragment::appendXML breaks upon invalid XML, it'd be great if we could support
+                    // _some_ of it though. Testing DOMDocument::loadXML here.
+                    $this->append_xml_before($pi, $rawvalue);
+                    $pi->parentNode->removeChild($pi);
                 } else {
                     if (strcasecmp($cleanoption, 'plain') != 0) {
                         debugging("Unrecognized placeholder cleaning option: '$cleanoption', using 'plain'");
@@ -372,12 +388,22 @@ private function resolve_placeholders(): void {
                     // Treat the value as plain text and don't allow any kind of markup.
                     // Since we're adding a text node, the DOM handles escaping for us.
                     $element = new DOMText($rawvalue);
+                    $pi->parentNode->replaceChild($element, $pi);
                 }
-                $pi->parentNode->replaceChild($element, $pi);
             }
         }
     }
 
+    private function append_xml_before(DOMNode $before, string $rawxml): void {
+        $newdoc = new DOMDocument();
+        // TODO: Would interpreting placeholder values as HTML make more sense?
+        $newdoc->loadXML($rawxml);
+
+        while($newdoc->hasChildNodes()) {
+            $before->parentNode->insertBefore($before, $newdoc->firstChild);
+        }
+    }
+
     /**
      * Replaces the HTML attributes `pattern`, `required`, `minlength`, `maxlength`, `min, `max` so that submission is
      * not prevented.
diff --git a/tests/question_ui_renderer_test.php b/tests/question_ui_renderer_test.php
index 8318148..f0d422d 100644
--- a/tests/question_ui_renderer_test.php
+++ b/tests/question_ui_renderer_test.php
@@ -191,6 +191,35 @@ public function test_should_resolve_placeholders(): void {
         EXPECTED, $result);
     }
 
+    /**
+     * Tests that placeholders are replaced.
+     *
+     * @return void
+     * @throws coding_exception
+     * @covers \qtype_questionpy\question_ui_renderer
+     */
+    public function test_should_not_replace_qpy_elements_in_placeholder_expansion(): void {
+        $input = file_get_contents(__DIR__ . '/question_uis/placeholder.xhtml');
+        $qa = $this->create_question_attempt_stub();
+
+        $ui = new question_ui_renderer($input, [
+            'param' => "<qpy:format-float>123</qpy:format-float>",
+            'description' => 'My simple description.',
+        ], new \question_display_options(), $qa);
+        $result = $ui->render();
+
+        $this->assert_html_string_equals_html_string(<<<EXPECTED
+        <div xmlns="http://www.w3.org/1999/xhtml">
+            <div>My simple description.</div>
+            <span>By default cleaned parameter: Value of param <b>one</b>.</span>
+            <span>Explicitly cleaned parameter: Value of param <b>one</b>.</span>
+            <span>Noclean parameter: Value of param <b>one</b>.<script>'Oh no, danger!'</script></span>
+            <span>Plain parameter: Value of param &lt;b>one&lt;/b>.&lt;script>'Oh no, danger!'&lt;/script>
+            </span>
+        </div>
+        EXPECTED, $result);
+    }
+
     /**
      * Tests that placeholders are just removed when the corresponding value is missing.
      *