Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-75516

Mustache quote helper does not escape all special JSON characters

    XMLWordPrintable

Details

    • MOODLE_311_STABLE, MOODLE_39_STABLE, MOODLE_400_STABLE, MOODLE_401_STABLE
    • MOODLE_400_STABLE, MOODLE_401_STABLE
    • MDL-75516_400
    • MDL-75516_401
    • Hide

      Test Javascript implementation:

      1. Create or edit a course so it's full name is

        foo \ bar
        

      2. Make sure the course's start date is in the past and the end date is in the future
      3. Add an assignment with a deadline for submitting
      4. Navigate to the dashboard (make sure there is a timeline block)
      5. Verify there is no popup with an error message
      6. Verify the assignment shows up on the timeline with the correct course name ("foo \ bar")

      Test PHP implementation:

      1. Download test.mustache and move it to lib/templates/test.mustache
      2. Download test.php and move it to lib/test.php
      3. Navigate to /lib/test.php in your browser
      4. Verify that you see two identical lines:

        "foo \b\f\n\r\t\"\\ bar"
        "foo \b\f\n\r\t\"\\ bar"
        

      Show
      Test Javascript implementation: Create or edit a course so it's full name is foo \ bar Make sure the course's start date is in the past and the end date is in the future Add an assignment with a deadline for submitting Navigate to the dashboard (make sure there is a timeline block) Verify there is no popup with an error message Verify the assignment shows up on the timeline with the correct course name ("foo \ bar") Test PHP implementation: Download test.mustache and move it to lib/templates/test.mustache Download test.php and move it to lib/test.php Navigate to /lib/test.php in your browser Verify that you see two identical lines: "foo \b\f\n\r\t\"\\ bar" "foo \b\f\n\r\t\"\\ bar"

    Description

      The mustache quote helper is used to escape quotes when inserting variables into JSON notation. It was introduced in MDL-52136. Unfortunately, to escape a string for use in JSON, more characters than just the double quote have to be escaped. To be exact:

      • Backspace to be replaced with \b
      • Form feed to be replaced with \f
      • Newline to be replaced with \n
      • Carriage return to be replaced with \r
      • Tab to be replaced with \t
      • Double quote to be replaced with \"
      • Backslash to be replaced with \\

      Source: https://www.tutorialspoint.com/json_simple/json_simple_escape_characters.htm

      This leads to some bugs: MDL-67640, MDL-68865 and MDL-69398 to name a few.

      In MDL-65203 and MDL-65183, some of these special characters (\t, \n and \r) were added to the list of characters that the JS implementation of the quote helper replaces. It's still incomplete, though. And the PHP implementation hasn't been updated to match the Javascript one.

      Most importantly, the backslash character is not being escaped, yet. This means that the escaping of other characters (like double quotes) can be undone easily. For example, the string

      foo\"bar

      is "escaped" to

      foo\\"bar

      by the current implementation of the quote helper. As you can see, the "escaped" version contains an unescaped double quote because of the extra backslash.

      Suggested solution

      Fortunately, there is a function in both Javascript and PHP that takes care of escaping all these special characters correctly: JSON encoding!

      Just passing the input of the quote helper to json_encode in PHP and JSON.stringify in Javascript yields a string with all special JSON characters being escaped. The resulting string also starts and ends with an extra quote, but adding quotes is exactly what the quote helper is supposed to do anyway. So we don't even have to remove them.

      Attachments

        1. php_template_success.png
          php_template_success.png
          17 kB
        2. test.mustache
          0.9 kB
        3. test.php
          0.3 kB
        4. timeline_no_errors.png
          timeline_no_errors.png
          26 kB
        5. unit_test_passed.png
          unit_test_passed.png
          46 kB

        Issue Links

          Activity

            People

              Unassigned Unassigned
              bonczek Lars Bonczek
              David Woloszyn David Woloszyn
              Ilya Tregubov Ilya Tregubov
              Kim Jared Lucas Kim Jared Lucas
              Adrian Greeve, David Woloszyn, Huong Nguyen, Jake Dallimore, Meirza, Michael Hawkins, Raquel Ortega, Safat Shahin, Stevani Andolo, David Woloszyn, Huong Nguyen, Jake Dallimore, Meirza, Michael Hawkins, Raquel Ortega, Safat Shahin, Stevani Andolo
              Votes:
              4 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

                Created:
                Updated:
                16/Jan/23

                Time Tracking

                  Estimated:
                  Original Estimate - 0 minutes
                  0m
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 1 hour, 20 minutes
                  1h 20m