Moodle
  1. Moodle
  2. MDL-40633

Regression: auto-linking filter behaving badly

    Details

    • Testing Instructions:
      Hide

      The unit tests are sufficient.

      You may wish to revert the change to filter/urltolink/filter.php, run the unit tests, and verify that you get two failures that look like the problem reported in the forum, then re-apply the filter.php changes and verify that they fix the problem.

      Show
      The unit tests are sufficient. You may wish to revert the change to filter/urltolink/filter.php, run the unit tests, and verify that you get two failures that look like the problem reported in the forum, then re-apply the filter.php changes and verify that they fix the problem.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      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

        Gliffy Diagrams

          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: