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

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

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            dougiamas 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
            dougiamas 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
            skodak Petr Skoda added a comment -

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

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

            Has been commit. Thanks all

            Show
            samhemelryk Sam Hemelryk added a comment - Has been commit. Thanks all
            Hide
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - we have the same code in 1.9, please apply the patch there too
            Hide
            samhemelryk 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
            samhemelryk 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
            skodak Petr Skoda added a comment -

            should be finally solved in latest HTMLPurifier 4.2.0

            Show
            skodak Petr Skoda 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:
                  Fix Release Date:
                  8/Jun/10