Moodle
  1. Moodle
  2. MDL-21183

convert_urls_into_links is converting urls to links inside image tags if tags are escaped

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 1.9.7
    • Fix Version/s: 1.9.8
    • Component/s: General
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      31798

      Description

      text like <img src="blah.jpg"> is wrongly converted to something like <img src="<a href="blah.jpg">blah.jpg</a>">

      1. testweblib_noescapedtests.patch
        3 kB
        Andrew Davis
      2. testweblib_noJStests.patch
        2 kB
        Andrew Davis

        Issue Links

          Activity

          Hide
          Andrew Davis added a comment - - edited

          MDL-21168 related issue dealing with unescaped img tags.

          Show
          Andrew Davis added a comment - - edited MDL-21168 related issue dealing with unescaped img tags.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I really think that urls within entities and urls within javascript... aren't really problematic for the "normal" use of the convert_urls_into_links() function (to convert manually introduced urls to linked urls).

          Surely we could end supporting them... but also surely it would be at a higher processing time and also, surely it wouldn't able to process all URLs properly (imagine concatenated variables building one URL or so).

          So, my +1 is about to ignore these completely and delete the corresponding tests (4 if I'm not wrong).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I really think that urls within entities and urls within javascript... aren't really problematic for the "normal" use of the convert_urls_into_links() function (to convert manually introduced urls to linked urls). Surely we could end supporting them... but also surely it would be at a higher processing time and also, surely it wouldn't able to process all URLs properly (imagine concatenated variables building one URL or so). So, my +1 is about to ignore these completely and delete the corresponding tests (4 if I'm not wrong). Ciao
          Hide
          Andrew Davis added a comment -

          I've attached two patches. One comments out the Javascript tests. One comments out the escaped html tests.

          I just went back and had a look at the original version of this function before we started this refactoring. Maybe we're attempting to hold convert_urls_into_links to an unnecessarily high standard. The tests that are failing now are ones that have come from me and and not from any genuine problems from the real world.

          At a minimum we can comment out the Javascript tests. I'm now thinking we should also comment out the escaped html tests. Leave them there in case these become real problems in the future. Then we just leave the code as it is until any real problems are reported. I can come up with cases that break convert_urls_into_links until the end of time but that isn't really making Moodle better in any significant way.

          Show
          Andrew Davis added a comment - I've attached two patches. One comments out the Javascript tests. One comments out the escaped html tests. I just went back and had a look at the original version of this function before we started this refactoring. Maybe we're attempting to hold convert_urls_into_links to an unnecessarily high standard. The tests that are failing now are ones that have come from me and and not from any genuine problems from the real world. At a minimum we can comment out the Javascript tests. I'm now thinking we should also comment out the escaped html tests. Leave them there in case these become real problems in the future. Then we just leave the code as it is until any real problems are reported. I can come up with cases that break convert_urls_into_links until the end of time but that isn't really making Moodle better in any significant way.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          +1

          Show
          Eloy Lafuente (stronk7) added a comment - +1
          Hide
          Mauno Korpelainen added a comment -

          Yes, links look correct now

          It's actually interesting that Google Chart API is proposing people to copy their Chart Api URLs and they are using vertical bars "|" (pipe characters) to separate labels and data in "illegal URLs" ( for example http://code.google.com/apis/chart/labels.html ) so URLs like

          http://chart.apis.google.com/chart?cht=r&chs=200x200&chd=t:77,66,15,0,31,48,100,77|20,36,100,2,0,100&chco=FF0000,FF9900&chls=2.0,4.0,0.0|2.0,4.0,0.0&chxt=x&chxl=0:|0|45|90|135|180|225|270|315&chxr=0,0.0,360.0 should all have %7C instead of vertical bar "|"

          and image URLs like http://chart.apis.google.com/chart?cht=r&chs=200x200&chd=t:77,66,15,0,31,48,100,77%7C20,36,100,2,0,100&chco=FF0000,FF9900&chls=2.0,4.0,0.0%7C2.0,4.0,0.0&chxt=x&chxl=0:%7C0%7C45%7C90%7C135%7C180%7C225%7C270%7C315&chxr=0,0.0,360.0 are converted correctly to links in moodle auto-format.

          Still all modern browsers I have tested seem to be able to use and accept also "|" or backslash with cgi tex renderers...

          Show
          Mauno Korpelainen added a comment - Yes, links look correct now It's actually interesting that Google Chart API is proposing people to copy their Chart Api URLs and they are using vertical bars "|" (pipe characters) to separate labels and data in "illegal URLs" ( for example http://code.google.com/apis/chart/labels.html ) so URLs like http://chart.apis.google.com/chart?cht=r&chs=200x200&chd=t:77,66,15,0,31,48,100,77 |20,36,100,2,0,100&chco=FF0000,FF9900&chls=2.0,4.0,0.0|2.0,4.0,0.0&chxt=x&chxl=0:|0|45|90|135|180|225|270|315&chxr=0,0.0,360.0 should all have %7C instead of vertical bar "|" and image URLs like http://chart.apis.google.com/chart?cht=r&chs=200x200&chd=t:77,66,15,0,31,48,100,77%7C20,36,100,2,0,100&chco=FF0000,FF9900&chls=2.0,4.0,0.0%7C2.0,4.0,0.0&chxt=x&chxl=0:%7C0%7C45%7C90%7C135%7C180%7C225%7C270%7C315&chxr=0,0.0,360.0 are converted correctly to links in moodle auto-format. Still all modern browsers I have tested seem to be able to use and accept also "|" or backslash with cgi tex renderers...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, it's one of the "holes" or not really well defined things in the http/html/uri specs.

          From my experience... browsers use to encode them automatically if they detect urls are wrongly encoded. But not all them do the same. Some ones do it with links in HTML and with URLs pasted in the URL box, while others do it only with links in HTML. Worst yet, some of them show you the encoded URL in the box while others will show the un-encoded URL.

          Really crazy and prone to double encodes/decodes. So let's keep out converter out from that problem. Just accept encoded (correct) URLs.

          Thanks for feedback, Manuo. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, it's one of the "holes" or not really well defined things in the http/html/uri specs. From my experience... browsers use to encode them automatically if they detect urls are wrongly encoded. But not all them do the same. Some ones do it with links in HTML and with URLs pasted in the URL box, while others do it only with links in HTML. Worst yet, some of them show you the encoded URL in the box while others will show the un-encoded URL. Really crazy and prone to double encodes/decodes. So let's keep out converter out from that problem. Just accept encoded (correct) URLs. Thanks for feedback, Manuo. Ciao
          Hide
          Andrew Davis added a comment -

          Patches committed to 1.9 and trunk.

          Show
          Andrew Davis added a comment - Patches committed to 1.9 and trunk.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: