MDL-70433 grades: prevent double escaping in titles

grade/edit/tree/lib.php

@@ -1021,7 +1021,7 @@ public function get_category_cell($category, $levelclass, $params) {
         $masterlabel = get_string('all');
         // Use category name if available.
         if ($category->fullname !== '?') {
-            $masterlabel = format_string($category->fullname);
+            $masterlabel = format_string($category->fullname, true, ['escape' => false]);
             // Limit the displayed category name to prevent the Select column from getting too wide.
             if (core_text::strlen($masterlabel) > 20) {
                 $masterlabel = get_string('textellipsis', 'core', core_text::substr($masterlabel, 0, 12));

grade/lib.php

@@ -1639,6 +1639,7 @@ public function get_element_header(&$element, $withlink = false, $icon = true, $
         }
 
         $title = $element['object']->get_name($fulltotal);
+        $titleunescaped = $element['object']->get_name($fulltotal, false);
         $header .= $title;
 
         if ($element['type'] != 'item' and $element['type'] != 'categoryitem' and
@@ -1649,12 +1650,12 @@ public function get_element_header(&$element, $withlink = false, $icon = true, $
         if ($withlink && $url = $this->get_activity_link($element)) {
             $a = new stdClass();
             $a->name = get_string('modulename', $element['object']->itemmodule);
-            $a->title = $title;
+            $a->title = $titleunescaped;
             $title = get_string('linktoactivity', 'grades', $a);
 
             $header = html_writer::link($url, $header, array('title' => $title, 'class' => 'gradeitemheader'));
         } else {
-            $header = html_writer::span($header, 'gradeitemheader', array('title' => $title, 'tabindex' => '0'));
+            $header = html_writer::span($header, 'gradeitemheader', array('title' => $titleunescaped, 'tabindex' => '0'));
         }
 
         if ($withdescription) {

grade/report/grader/lib.php

@@ -1666,9 +1666,10 @@ protected function get_course_header($element) {
         }
 
         $name = $element['object']->get_name();
+        $nameunescaped = $element['object']->get_name(false);
         $describedbyid = uniqid();
         $courseheader = html_writer::tag('span', $name, [
-            'title' => $name,
+            'title' => $nameunescaped,
             'class' => 'gradeitemheader',
             'aria-describedby' => $describedbyid
         ]);

grade/templates/edit_tree.mustache

@@ -60,7 +60,7 @@
                     <select name="moveafter" id="menumoveafter" class="ignoredirty singleselect custom-select form-control"
                             data-action="toggle" data-toggle="action" data-togglegroup="category" disabled>
                         {{#bulkmoveoptions}}
-                            <option value="{{value}}">{{name}}</option>
+                            <option value="{{value}}">{{{name}}}</option>
                         {{/bulkmoveoptions}}
                     </select>
                 </div>

grade/tests/behat/grade_aggregation_changes.feature

@@ -19,17 +19,17 @@ Feature: Changing the aggregation of an item affects its weight and extra credit
       | Cat weighted2 | C1     | 10          |
       | Cat simple    | C1     | 11          |
       | Cat ec        | C1     | 12          |
-      | Cat natural   | C1     | 13          |
+      | Cat natural & | C1     | 13          |
     And the following "grade items" exist:
-      | itemname  | course | category    | aggregationcoef | aggregationcoef2 | weightoverride |
-      | Item a1   | C1     | ?           | 0               | 0                | 0              |
-      | Item a2   | C1     | ?           | 0               | 0.40             | 1              |
-      | Item a3   | C1     | ?           | 1               | 0.10             | 1              |
-      | Item a4   | C1     | ?           | 1               | 0                | 0              |
-      | Item b1   | C1     | Cat natural | 0               | 0                | 0              |
-      | Item b2   | C1     | Cat natural | 0               | 0.40             | 1              |
-      | Item b3   | C1     | Cat natural | 1               | 0.10             | 1              |
-      | Item b4   | C1     | Cat natural | 1               | 0                | 0              |
+      | itemname  | course | category      | aggregationcoef | aggregationcoef2 | weightoverride |
+      | Item a1   | C1     | ?             | 0               | 0                | 0              |
+      | Item a2   | C1     | ?             | 0               | 0.40             | 1              |
+      | Item a3   | C1     | ?             | 1               | 0.10             | 1              |
+      | Item a4   | C1     | ?             | 1               | 0                | 0              |
+      | Item b1   | C1     | Cat natural & | 0               | 0                | 0              |
+      | Item b2   | C1     | Cat natural & | 0               | 0.40             | 1              |
+      | Item b3   | C1     | Cat natural & | 1               | 0.10             | 1              |
+      | Item b4   | C1     | Cat natural & | 1               | 0                | 0              |
     And I log in as "admin"
     And I set the following administration settings values:
       | grade_aggregations_visible | Mean of grades,Weighted mean of grades,Simple weighted mean of grades,Mean of grades (with extra credits),Median of grades,Lowest grade,Highest grade,Mode of grades,Natural |
@@ -133,7 +133,7 @@ Feature: Changing the aggregation of an item affects its weight and extra credit
     And I should not see "Weight" in the "#id_headerparent" "css_element"
     And I should not see "Extra credit"
     And I press "Cancel"
-    And I follow "Edit   Cat natural"
+    And I follow "Edit   Cat natural &"
     And I set the field "Aggregation" to "Mean of grades"
     And I press "Save changes"
     And I follow "Edit   Item b1"
@@ -191,7 +191,7 @@ Feature: Changing the aggregation of an item affects its weight and extra credit
     And the field "Weight adjusted" matches value "0"
     And the field "Extra credit" matches value "0"
     And I press "Cancel"
-    And I follow "Edit   Cat natural"
+    And I follow "Edit   Cat natural &"
     And I set the field "Aggregation" to "Natural"
     And I press "Save changes"
     And I follow "Edit   Item b1"
@@ -253,7 +253,7 @@ Feature: Changing the aggregation of an item affects its weight and extra credit
     And I should not see "Extra credit"
     And the field "Item weight" matches value "1"
     And I press "Cancel"
-    And I follow "Edit   Cat natural"
+    And I follow "Edit   Cat natural &"
     And I set the field "Aggregation" to "Weighted mean of grades"
     And I press "Save changes"
     And I follow "Edit   Item b1"
@@ -311,7 +311,7 @@ Feature: Changing the aggregation of an item affects its weight and extra credit
     And the field "Weight adjusted" matches value "0"
     And the field "Extra credit" matches value "0"
     And I press "Cancel"
-    And I follow "Edit   Cat natural"
+    And I follow "Edit   Cat natural &"
     And I set the field "Aggregation" to "Natural"
     And I press "Save changes"
     And I follow "Edit   Item b1"
@@ -339,7 +339,8 @@ Feature: Changing the aggregation of an item affects its weight and extra credit
     And I set the field "Select Item a2" to "1"
     And I set the field "Select Item a3" to "1"
     And I set the field "Select Item a4" to "1"
-    When I select "Cat natural" from the "Move selected items to" singleselect
+    And I should not see "Cat natural &" in the "select#menumoveafter" "css_element"
+    When I select "Cat natural &" from the "Move selected items to" singleselect
     And I navigate to "View > Grader report" in the course gradebook
     And I follow "Edit   Item a1"
     Then the field "Weight adjusted" matches value "0"

lib/grade/grade_category.php

@@ -2311,18 +2311,21 @@ public function get_parent_category() {
      * Returns the most descriptive field for this grade category
      *
      * @return string name
+     * @param bool $escape Whether the returned category name is to be HTML escaped or not.
      */
-    public function get_name() {
+    public function get_name($escape = true) {
         global $DB;
         // For a course category, we return the course name if the fullname is set to '?' in the DB (empty in the category edit form)
         if (empty($this->parent) && $this->fullname == '?') {
             $course = $DB->get_record('course', array('id'=> $this->courseid));
-            return format_string($course->fullname, false, array("context" => context_course::instance($this->courseid)));
+            return format_string($course->fullname, false, ['context' => context_course::instance($this->courseid),
+                'escape' => $escape]);
 
         } else {
             // Grade categories can't be set up at system context (unlike scales and outcomes)
             // We therefore must have a courseid, and don't need to handle system contexts when filtering.
-            return format_string($this->fullname, false, array("context" => context_course::instance($this->courseid)));
+            return format_string($this->fullname, false, ['context' => context_course::instance($this->courseid),
+                'escape' => $escape]);
         }
     }
 

lib/grade/grade_item.php

@@ -1471,9 +1471,10 @@ public static function fix_duplicate_sortorder($courseid) {
      * Determines what type of grade item it is then returns the appropriate string
      *
      * @param bool $fulltotal If the item is a category total, returns $categoryname."total" instead of "Category total" or "Course total"
+     * @param bool $escape Whether the returned category name is to be HTML escaped or not.
      * @return string name
      */
-    public function get_name($fulltotal=false) {
+    public function get_name($fulltotal=false, $escape = true) {
         global $CFG;
         require_once($CFG->dirroot . '/course/lib.php');
         if (strval($this->itemname) !== '') {
@@ -1483,7 +1484,7 @@ public function get_name($fulltotal=false) {
             $deletionpending = course_module_instance_pending_deletion($this->courseid, $this->itemmodule, $this->iteminstance);
             $deletionnotice = get_string('gradesmoduledeletionprefix', 'grades');
 
-            $options = ['context' => context_course::instance($this->courseid)];
+            $options = ['context' => context_course::instance($this->courseid), 'escape' => $escape];
             return $deletionpending ?
                 format_string($deletionnotice . ' ' . $this->itemname, true, $options) :
                 format_string($this->itemname, true, $options);
@@ -1495,7 +1496,7 @@ public function get_name($fulltotal=false) {
             if ($fulltotal) {
                 $category = $this->load_parent_category();
                 $a = new stdClass();
-                $a->category = $category->get_name();
+                $a->category = $category->get_name($escape);
                 return get_string('categorytotalfull', 'grades', $a);
             } else {
             return get_string('categorytotal', 'grades');

lib/grade/tests/fixtures/lib.php

@@ -181,7 +181,7 @@ private function load_grade_categories() {
 
         $grade_category = new stdClass();
 
-        $grade_category->fullname    = 'unittestcategory1';
+        $grade_category->fullname    = 'unittestcategory1 &';
         $grade_category->courseid    = $this->course->id;
         $grade_category->aggregation = GRADE_AGGREGATE_MEAN;
         $grade_category->aggregateonlygraded = 1;
@@ -324,7 +324,7 @@ protected function load_grade_items() {
 
         $grade_item->courseid = $this->course->id;
         $grade_item->categoryid = $this->grade_categories[1]->id;
-        $grade_item->itemname = 'unittestgradeitem1';
+        $grade_item->itemname = 'unittestgradeitem1 &';
         $grade_item->itemtype = 'mod';
         $grade_item->itemmodule = $this->course_module[0]->modname;
         $grade_item->iteminstance = $this->course_module[0]->instance;

lib/grade/tests/grade_category_test.php

@@ -48,7 +48,8 @@ public function test_grade_category() {
         $this->sub_test_grade_category_get_grade_item();
         $this->sub_test_grade_category_load_parent_category();
         $this->sub_test_grade_category_get_parent_category();
-        $this->sub_test_grade_category_get_name();
+        $this->sub_test_grade_category_get_name_escaped();
+        $this->sub_test_grade_category_get_name_unescaped();
         $this->sub_test_grade_category_generate_grades_aggregationweight();
         $this->sub_test_grade_category_set_parent();
         $this->sub_test_grade_category_get_final();
@@ -692,10 +693,24 @@ protected function sub_test_grade_category_get_parent_category() {
         $this->assertEquals($this->grade_categories[0]->id, $parent_category->id);
     }
 
-    protected function sub_test_grade_category_get_name() {
+    /**
+     * Tests the getter of the category fullname with escaped HTML.
+     */
+    protected function sub_test_grade_category_get_name_escaped() {
+        $category = new grade_category($this->grade_categories[0]);
+        $this->assertTrue(method_exists($category, 'get_name'));
+        $this->assertEquals(format_string($this->grade_categories[0]->fullname, true, ['escape' => true]),
+            $category->get_name(true));
+    }
+
+    /**
+     * Tests the getter of the category fullname with unescaped HTML.
+     */
+    protected function sub_test_grade_category_get_name_unescaped() {
         $category = new grade_category($this->grade_categories[0]);
         $this->assertTrue(method_exists($category, 'get_name'));
-        $this->assertEquals($this->grade_categories[0]->fullname, $category->get_name());
+        $this->assertEquals(format_string($this->grade_categories[0]->fullname, true, ['escape' => false]),
+            $category->get_name(false));
     }
 
     protected function sub_test_grade_category_set_parent() {

lib/grade/tests/grade_item_test.php

@@ -42,7 +42,8 @@ public function test_grade_item() {
         $this->sub_test_grade_item_get_sortorder();
         $this->sub_test_grade_item_set_sortorder();
         $this->sub_test_grade_item_move_after_sortorder();
-        $this->sub_test_grade_item_get_name();
+        $this->sub_test_grade_item_get_name_escaped();
+        $this->sub_test_grade_item_get_name_unescaped();
         $this->sub_test_grade_item_set_parent();
         $this->sub_test_grade_item_get_parent_category();
         $this->sub_test_grade_item_load_parent_category();
@@ -271,12 +272,24 @@ protected function sub_test_grade_item_move_after_sortorder() {
         $this->assertEquals($after->sortorder, 8);
     }
 
-    protected function sub_test_grade_item_get_name() {
-        $grade_item = new grade_item($this->grade_items[0], false);
-        $this->assertTrue(method_exists($grade_item, 'get_name'));
+    /**
+     * Tests the getter of the item name with escaped HTML.
+     */
+    protected function sub_test_grade_item_get_name_escaped() {
+        $gradeitem = new grade_item($this->grade_items[0], false);
+        $this->assertTrue(method_exists($gradeitem, 'get_name'));
+        $this->assertEquals(format_string($this->grade_items[0]->itemname, true, ['escape' => true]),
+            $gradeitem->get_name(false, true));
+    }
 
-        $name = $grade_item->get_name();
-        $this->assertEquals($this->grade_items[0]->itemname, $name);
+    /**
+     * Tests the getter of the item name with unescaped HTML.
+     */
+    protected function sub_test_grade_item_get_name_unescaped() {
+        $gradeitem = new grade_item($this->grade_items[0], false);
+        $this->assertTrue(method_exists($gradeitem, 'get_name'));
+        $this->assertEquals(format_string($this->grade_items[0]->itemname, true, ['escape' => false]),
+            $gradeitem->get_name(false, false));
     }
 
     protected function sub_test_grade_item_set_parent() {

lib/upgrade.txt

@@ -63,6 +63,8 @@ information provided here is intended especially for developers.
 * New method \core_user::awaiting_action() has been introduced to check if the user is fully ready to use the site or
   whether there is an action (such as filling the missing profile field, changing password or agreeing to the site
   policy) needed.
+* The signature of the get_name() function for grade_category and grade_item has been extended. The new parameter allows
+  callers to get the name without escaped characters.
 
 === 3.11.2 ===
 * For security reasons, filelib has been updated so all requests now use emulated redirects.