Uploaded image for project: '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

          Attachments

            Issue Links

              Activity

              Hide
              danmarsden Dan Marsden added a comment -

              confirmed this is an issue in 2.3.8 as well.

              Show
              danmarsden Dan Marsden added a comment - confirmed this is an issue in 2.3.8 as well.
              Hide
              salvetore 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
              salvetore 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              timhunt 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
              danmarsden Dan Marsden added a comment -

              thanks for the quick fix Tim!

              Show
              danmarsden Dan Marsden added a comment - thanks for the quick fix Tim!
              Hide
              marina 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 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
              timhunt Tim Hunt added a comment -

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

              Show
              timhunt Tim Hunt added a comment - Thanks Marina. Yes, regex have an awesome power for producing incomprehensible code.
              Hide
              timhunt 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
              timhunt 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
              rwijaya Rossiani Wijaya added a comment -

              This is working as expected.

              Tested for 2.3, 2.4, 2.5 and master.

              Test passed.

              Show
              rwijaya Rossiani Wijaya added a comment - This is working as expected. Tested for 2.3, 2.4, 2.5 and master. Test passed.
              Hide
              damyon 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 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:
                    Fix Release Date:
                    9/Sep/13