Moodle
  1. Moodle
  2. MDL-40633

Regression: auto-linking filter behaving badly

    Details

    • Rank:
      51473

      Description

      Where previously images were being displayed, moodle now shows this in content:

      www.ahsgerman.com/vle/pluginfile.php?file=/110/mod_book/chapter/27/sophias%20pictures%201.jpg" alt="sophia 1" height="320" width="240" />
      
      www.ahsgerman.com/vle/pluginfile.php?file=/110/mod_book/chapter/27/sophias%20pictures%202.jpg" alt="sophia 2" style="border: 2px solid black;" height="320" width="240" />
      
      www.ahsgerman.com/vle/pluginfile.php?file=/110/mod_book/chapter/27/sophias%20pictures%203.jpg" alt="sophia 3" style="border: 2px solid black;" height="320" width="240" />
      
      www.ahsgerman.com/vle/pluginfile.php?file=/110/mod_book/chapter/27/sophias%20pictures%204.jpg" alt="sophia 4" style="border: 2px solid black;" height="240" width="320" />
      
      www.ahsgerman.com/vle/pluginfile.php?file=/110/mod_book/chapter/27/sophias%20pictures%205.jpg" alt="sophia 5" style="border: 2px solid black;" height="240" width="320" />
      

      reported in forums:
      https://moodle.org/mod/forum/discuss.php?d=231543
      https://moodle.org/mod/forum/discuss.php?d=232357

      seems to be due to the patch in MDL-22390

        Issue Links

          Activity

          Hide
          Dan Marsden added a comment -

          confirmed this is an issue in 2.3.8 as well.

          Show
          Dan Marsden added a comment - confirmed this is an issue in 2.3.8 as well.
          Hide
          Michael de Raadt added a comment -

          Thanks for reporting that, Dan.

          Tim: I've added you as you were involved in the original issue.

          It would be good if we had Behat tests to cover this (if we don't already).

          Show
          Michael de Raadt added a comment - Thanks for reporting that, Dan. Tim: I've added you as you were involved in the original issue. It would be good if we had Behat tests to cover this (if we don't already).
          Hide
          Tim Hunt added a comment -

          Michael, we already have extensive unit tests for the URL auto-linking filter, which is the right sort of test for this.

          Dan, you need to add some more unit tests to illustrate the problem, which really means working with the people affected to discover the HTML that is being filtered wrongly.

          If someone else can write failing unit tests, I am happy to help fix the filter code. Or if you want to fix this yourself, then I will peer review. I don't have time to investigate and create the necessary unit tests myself.

          Show
          Tim Hunt added a comment - Michael, we already have extensive unit tests for the URL auto-linking filter, which is the right sort of test for this. Dan, you need to add some more unit tests to illustrate the problem, which really means working with the people affected to discover the HTML that is being filtered wrongly. If someone else can write failing unit tests, I am happy to help fix the filter code. Or if you want to fix this yourself, then I will peer review. I don't have time to investigate and create the necessary unit tests myself.
          Hide
          Tim Hunt added a comment -

          Actually, I think I see the problem. The problem case is

          <img src="http://www.example.com/image.png" />

          We should be leaving that alone. The filter does that by using a regex:

          (?<!=[\"'])(?:http(s)?://|(www\.))...

          That is

          • Verify the preceeding characters are not =" (which is what should prevent this from being made into a link).
          • Then we should either have one of http://, https:// or www.
          • Then we must have at least 2 more bits of domain name.

          What is going wrong is that the regex can start matching at the www., so the not preceded by =" test does not work as intended.

          I think my proposed fix is to replace the

          (?:http(s)?://|(www\.))

          bit by

          (?:http(s)?://|(?<!://)www\.)

          So that www is not considered a possible start for a URL if it is preceded by ://.

          OK, I will find time to code this this morning, since I caused the regression.

          Show
          Tim Hunt added a comment - Actually, I think I see the problem. The problem case is <img src="http://www.example.com/image.png" /> We should be leaving that alone. The filter does that by using a regex: (?<!=[\"'])(?:http(s)?://|(www\.))... That is Verify the preceeding characters are not =" (which is what should prevent this from being made into a link). Then we should either have one of http:// , https:// or www. Then we must have at least 2 more bits of domain name. What is going wrong is that the regex can start matching at the www. , so the not preceded by =" test does not work as intended. I think my proposed fix is to replace the (?:http(s)?://|(www\.)) bit by (?:http(s)?://|(?<!://)www\.) So that www is not considered a possible start for a URL if it is preceded by :// . OK, I will find time to code this this morning, since I caused the regression.
          Hide
          Tim Hunt added a comment -

          Here is a fix, with new unit tests.

          If anyone who is seeing this problem on their servers can verify the fix before it is integrated, that would be great.

          TO INTEGRATORS: I know that 2.3 branch is in 'security fixes only' mode, but since this is a recent regression on that branch, I think we should back-port the fix there too.

          Show
          Tim Hunt added a comment - Here is a fix, with new unit tests. If anyone who is seeing this problem on their servers can verify the fix before it is integrated, that would be great. TO INTEGRATORS: I know that 2.3 branch is in 'security fixes only' mode, but since this is a recent regression on that branch, I think we should back-port the fix there too.
          Hide
          Dan Marsden added a comment -

          thanks for the quick fix Tim!

          Show
          Dan Marsden added a comment - thanks for the quick fix Tim!
          Hide
          Marina Glancy added a comment -

          Thanks Tim, this has been integrated in 2.3, 2.4, 2.5 and master even though 2.3 is no longer supported but this is a recent regression.

          BTW thanks to your issue, I've learned today that regex are even more powerful than I thought

          Show
          Marina Glancy added a comment - Thanks Tim, this has been integrated in 2.3, 2.4, 2.5 and master even though 2.3 is no longer supported but this is a recent regression. BTW thanks to your issue, I've learned today that regex are even more powerful than I thought
          Hide
          Tim Hunt added a comment -

          Thanks Marina. Yes, regex have an awesome power for producing incomprehensible code.

          Show
          Tim Hunt added a comment - Thanks Marina. Yes, regex have an awesome power for producing incomprehensible code.
          Hide
          Tim Hunt added a comment -

          Art Lader reports that the fix works for him: https://moodle.org/mod/forum/discuss.php?d=232357#p1010116

          Show
          Tim Hunt added a comment - Art Lader reports that the fix works for him: https://moodle.org/mod/forum/discuss.php?d=232357#p1010116
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected.

          Tested for 2.3, 2.4, 2.5 and master.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Tested for 2.3, 2.4, 2.5 and master. Test passed.
          Hide
          Damyon Wiese added a comment -

          Moodle has many old functions,
          And although they cause no malfunction,
          There comes a day,
          When they get deprecated away,
          And get and put on the list for expulsion.

          Thanks for all the reports/testing/fixes this week. This issue has been sent upstream.

          Show
          Damyon Wiese added a comment - Moodle has many old functions, And although they cause no malfunction, There comes a day, When they get deprecated away, And get and put on the list for expulsion. Thanks for all the reports/testing/fixes this week. This issue has been sent upstream.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: