feat: optionally clean placeholder values

classes/question_ui_renderer.php

@@ -19,12 +19,12 @@
 use coding_exception;
 use DOMAttr;
 use DOMDocument;
-use DOMDocumentFragment;
 use DOMElement;
 use DOMException;
 use DOMNameSpaceNode;
 use DOMNode;
 use DOMProcessingInstruction;
+use DOMText;
 use DOMXPath;
 use question_attempt;
 use question_display_options;
@@ -328,7 +328,7 @@ private function replace_shuffled_indices(DOMXPath $xpath, DOMNode $element, int
                     break;
             }
 
-            $indexelement->parentNode->replaceChild(new \DOMText($indexstr), $indexelement);
+            $indexelement->parentNode->replaceChild(new DOMText($indexstr), $indexelement);
         }
     }
 
@@ -430,37 +430,50 @@ private function clean_up(DOMXPath $xpath): void {
     }
 
     /**
-     * Replace placeholder PIs such as `<?p my_key?>` with the appropriate value from `$this->placeholders`.
+     * 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.
+     * Since QPy transformations should not be applied to the content of the placeholders, this method should be called
+     * last.
      *
      * @param DOMXPath $xpath
      * @return void
      */
     private function resolve_placeholders(DOMXPath $xpath): void {
         /** @var DOMProcessingInstruction $pi */
         foreach ($xpath->query("//processing-instruction('p')") as $pi) {
-            $key = trim($pi->data);
+            $parts = preg_split("/\s+/", trim($pi->data));
+            $key = $parts[0];
+            $cleanoption = $parts[1] ?? "clean";
 
             if (!isset($this->placeholders[$key])) {
                 $pi->parentNode->removeChild($pi);
             } else {
-                /** @var DOMDocumentFragment $frag */
-                $frag = $xpath->document->createDocumentFragment();
-                /* TODO: This always interprets the parameter value as XHTML.
-                         While supporting markup here is important, perhaps we should allow placeholders to opt out? */
-                $frag->appendXML($this->placeholders[$key]);
-                $pi->parentNode->replaceChild($frag, $pi);
+                $rawvalue = $this->placeholders[$key];
+                if (strcasecmp($cleanoption, "clean") == 0) {
+                    // Allow (X)HTML, but clean using Moodle's clean_text to prevent XSS.
+                    $element = $xpath->document->createDocumentFragment();
+                    $element->appendXML(clean_text($rawvalue));
+                } else if (strcasecmp($cleanoption, "noclean") == 0) {
+                    $element = $xpath->document->createDocumentFragment();
+                    $element->appendXML($rawvalue);
+                } else {
+                    if (strcasecmp($cleanoption, "plain") != 0) {
+                        debugging("Unrecognized placeholder cleaning option: '$cleanoption', using 'plain'");
+                    }
+                    // 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);
             }
         }
     }
 
     /**
-     * Replaces the HTML attributes `pattern`, `required`, `minlength`, `maxlength` so that submission is not prevented.
+     * Replaces the HTML attributes `pattern`, `required`, `minlength`, `maxlength`, `min, `max` so that submission is
+     * not prevented.
      *
      * The standard attributes are replaced with `data-qpy_X`, which are then evaluated in JS.
-     * Ideally we'd also want to handle min and max here, but their evaluation in JS would be quite complicated.
      *
      * @param DOMXPath $xpath
      * @return void

tests/question_ui_renderer_test.php

@@ -285,14 +285,17 @@ public function test_should_resolve_placeholders() {
         $qa = $this->createStub(\question_attempt::class);
 
         $ui = new question_ui_renderer($input, [
-            "param" => "Value of param <b>one</b>",
+            "param" => "Value of param <b>one</b>.<script>'Oh no, danger!'</script>",
         ], mt_rand());
 
         $result = $ui->render_formulation($qa, new \question_display_options());
 
         $this->assertXmlStringEqualsXmlString(<<<EXPECTED
         <div xmlns="http://www.w3.org/1999/xhtml">
-            <span>Parameter: Value of param <b>one</b></span>
+            <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: <![CDATA[Value of param <b>one</b>.<script>'Oh no, danger!'</script>]]></span>
         </div>
         EXPECTED, $result);
     }
@@ -315,7 +318,10 @@ public function test_should_remove_placeholders_when_no_corresponding_value() {
 
         $this->assertXmlStringEqualsXmlString(<<<EXPECTED
         <div xmlns="http://www.w3.org/1999/xhtml">
-            <span>Parameter: </span>
+            <span>By default cleaned parameter: </span>
+            <span>Explicitly cleaned parameter: </span>
+            <span>Noclean parameter: </span>
+            <span>Plain parameter: </span>
         </div>
         EXPECTED, $result);
     }

tests/question_uis/placeholder.xhtml

@@ -1,5 +1,8 @@
 <qpy:question xmlns:qpy="http://questionpy.org/ns/question">
     <qpy:formulation>
-        <span>Parameter: <?p param?></span>
+        <span>By default cleaned parameter: <?p param?></span>
+        <span>Explicitly cleaned parameter: <?p param clean?></span>
+        <span>Noclean parameter: <?p param noclean?></span>
+        <span>Plain parameter: <?p param plain?></span>
     </qpy:formulation>
 </qpy:question>