Moodle
  1. Moodle
  2. MDL-22654

HTMLPurify is replacing line breaks with \n causing comparisons after cleaning to always fail

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 1.9.9
    • Component/s: Other
    • Labels:
      None
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      26759

      Description

      Hi Petr,

      We've managed to find a bug ~ set back with the HTML purify library. It would appear currently there are alot of checks that are broken after cleaning has occurred (PARAM_TEXT or PARAM_CLEAN).
      The problem appears to be that HTMLPurify is normalising line breaks in text to \n causing the comparisons to break as many clients send \r\n.

      I've been talking to Martin and Jerome about it and the general feeling seems to be that it would wonderful to normalise all text in Moodle but the task would be too huge to be worth tackling.
      We've had a quick look at the HTMLPurify library and come up with a small hack that resolves the problem by sorting it so that HTMLPurify doesn't normalise the text or replace line breaks with the system preferred after cleaning.
      I've attached a patch to illustrate what we found to work.

      Keen to see what you think on this one!

      Cheers
      Sam

        Activity

        Hide
        Martin Dougiamas added a comment -

        Yep. I can see the theoretical "niceness" value of normalising line endings for all cleaned text, and I don't like patching libraries, but:

        1) Such a big policy change might break lots of assumptions being made all over Moodle from the KSES days, and we don't need those kind of regressions in Moodle 2 right now.
        2) I don't think there are really any security issues with different texts having different line endings, and anyway, HTML Purifier is converting everything to the system default so different Moodle sites will have different results anyway.
        3) Even if HTML purifier does clean line endings, on upgraded sites there will always be a mix of old and new texts.

        So for all these reasons, I think we can justify a very minimal patch to HTML Purifier to disable replacement of line feeds totally (since it's not possible to disable the function via config variables). My +1 for that.

        Show
        Martin Dougiamas added a comment - Yep. I can see the theoretical "niceness" value of normalising line endings for all cleaned text, and I don't like patching libraries, but: 1) Such a big policy change might break lots of assumptions being made all over Moodle from the KSES days, and we don't need those kind of regressions in Moodle 2 right now. 2) I don't think there are really any security issues with different texts having different line endings, and anyway, HTML Purifier is converting everything to the system default so different Moodle sites will have different results anyway. 3) Even if HTML purifier does clean line endings, on upgraded sites there will always be a mix of old and new texts. So for all these reasons, I think we can justify a very minimal patch to HTML Purifier to disable replacement of line feeds totally (since it's not possible to disable the function via config variables). My +1 for that.
        Hide
        Petr Škoda added a comment -

        +1 to patch it too, we already do a few tweaks there...

        Show
        Petr Škoda added a comment - +1 to patch it too, we already do a few tweaks there...
        Hide
        Sam Hemelryk added a comment -

        Has been commit. Thanks all

        Show
        Sam Hemelryk added a comment - Has been commit. Thanks all
        Hide
        Petr Škoda added a comment -

        we have the same code in 1.9, please apply the patch there too

        Show
        Petr Škoda added a comment - we have the same code in 1.9, please apply the patch there too
        Hide
        Sam Hemelryk added a comment -

        Oops thanks for pointing that out Petr, I've not commit the fix on 1.9 as well

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Oops thanks for pointing that out Petr, I've not commit the fix on 1.9 as well Cheers Sam
        Hide
        Petr Škoda added a comment -

        should be finally solved in latest HTMLPurifier 4.2.0

        Show
        Petr Škoda added a comment - should be finally solved in latest HTMLPurifier 4.2.0

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: