Moodle
  1. Moodle
  2. MDL-15377

Glossary Autolink replace javascript in resources

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.5, 1.9.1, 2.1, 2.2
    • Fix Version/s: 2.2
    • Component/s: Glossary
    • Labels:
      None
    • Testing Instructions:
      Hide

      Testing:
      1) Make sure you have turned on the 'Glossary auto linking' under Site administration -> Plugins -> Filters -> Manage filters
      2) Now go to any of your testing course to create a glossary activity. Make sure you have set the 'Automatically link glossary entries' to Yes.
      3) Create an entry called 'alert' in this glossary. Make sure you have ticked the box for 'This entry should be automatically linked'.
      4) Next we need to create a page activity which containing the glossary entry 'alert' in the javascript. To add javascript to the page content, we must first disabled the HTML editor as it automatically removes all the javascript tag when you save the page.
      5) Turn off the HTML editor by going to 'edit my profile'. Under 'When editing text', choose 'Use standard web forms' from the dropdown.
      6) Go to the same course which contains the glossary, create a 'Page activity'.
      7) Fill in the Name and Description fields with anything you like. Under the 'Page content box', copy the code below and click 'Save and display'
      <p>Glossary entry alert should not be autolinked in javascript.</p>
      <script language="JavaScript" type="text/javascript">
      alert ("hello world");
      </script>

      Expected result: When you save and view the page, the alert text ought to display.
      Actual result prior to this fix: The alert does not run. If you view the page source, you will find the alert has been turned into glossary link.

      Show
      Testing: 1) Make sure you have turned on the 'Glossary auto linking' under Site administration -> Plugins -> Filters -> Manage filters 2) Now go to any of your testing course to create a glossary activity. Make sure you have set the 'Automatically link glossary entries' to Yes. 3) Create an entry called 'alert' in this glossary. Make sure you have ticked the box for 'This entry should be automatically linked'. 4) Next we need to create a page activity which containing the glossary entry 'alert' in the javascript. To add javascript to the page content, we must first disabled the HTML editor as it automatically removes all the javascript tag when you save the page. 5) Turn off the HTML editor by going to 'edit my profile'. Under 'When editing text', choose 'Use standard web forms' from the dropdown. 6) Go to the same course which contains the glossary, create a 'Page activity'. 7) Fill in the Name and Description fields with anything you like. Under the 'Page content box', copy the code below and click 'Save and display' <p>Glossary entry alert should not be autolinked in javascript.</p> <script language="JavaScript" type="text/javascript"> alert ("hello world"); </script> Expected result: When you save and view the page, the alert text ought to display. Actual result prior to this fix: The alert does not run. If you view the page source, you will find the alert has been turned into glossary link.
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-15377-master

      Description

      Administration > Modules > Filters (Manage Filters for 1.9) > Enable glossary Auto-link (Filer uploaded files: all files for 1.9)
      In a course create a glossary with the entry name "else". Enter a description (but not "else"). Choose to automatically link this entry.
      In the same course create a file resource. Use the index.html file attached to this issue.
      Save.
      Open the resource, and view source. "else" keyword from javascript (line 167 in the index.html file) has been replaced by the glossary entry.

        Gliffy Diagrams

        1. index.html
          16 kB
          Jérôme Mouneyrac
        2. mdl-15377-bug-fix.patch.txt
          1 kB
          Simon Coggins
        3. mdl-15377-filter-phrases-fix.patch.txt
          4 kB
          Simon Coggins

          Activity

          Hide
          Petr Skoda added a comment -

          hmm, we should start using some javascript highlighting - it seems impossible to parse html properly in PHP only

          Show
          Petr Skoda added a comment - hmm, we should start using some javascript highlighting - it seems impossible to parse html properly in PHP only
          Hide
          Simon Coggins added a comment -

          This can be fixed by passing extra parameters to filter_phrases(). As well as excluding <script> tags I'd also suggest excluding <textarea>, and <select> tags as you don't really want auto-linking within them either. See patch mdl-15377-bug-fix.patch.txt.

          Filter_phrases() is also used by the other auto-linking filters (mod/data/, mod/resource/, mod/wiki, filter/activitynames) as well as by filter/censor. Since they could potentially lead to a similar bug I would suggest moving the fix to inside filter_phrases(). See patch mdl-15377-filter-phrases.patch.txt (to be applied instead of the first patch).

          At present filter_phrases() behaves slightly unintuitively when you pass extra tags to ignore (<a> tags are ignored by default but not if you pass extra tags). In the patch I've added a fifth parameter to default to automatic filtering but allow users to override the default filtered tags.

          I've also modified the regular expressions to allow for tags with or without attributes.

          These changes shouldn't impact on existing usage of filter_phrases() except to add filtering of script, textarea, and select tags by default.

          Show
          Simon Coggins added a comment - This can be fixed by passing extra parameters to filter_phrases(). As well as excluding <script> tags I'd also suggest excluding <textarea>, and <select> tags as you don't really want auto-linking within them either. See patch mdl-15377-bug-fix.patch.txt. Filter_phrases() is also used by the other auto-linking filters (mod/data/, mod/resource/, mod/wiki, filter/activitynames) as well as by filter/censor. Since they could potentially lead to a similar bug I would suggest moving the fix to inside filter_phrases(). See patch mdl-15377-filter-phrases.patch.txt (to be applied instead of the first patch). At present filter_phrases() behaves slightly unintuitively when you pass extra tags to ignore (<a> tags are ignored by default but not if you pass extra tags). In the patch I've added a fifth parameter to default to automatic filtering but allow users to override the default filtered tags. I've also modified the regular expressions to allow for tags with or without attributes. These changes shouldn't impact on existing usage of filter_phrases() except to add filtering of script, textarea, and select tags by default.
          Hide
          Sam Marshall added a comment -

          We came across this problem at the OU. Ray Guo (one of the developers on my team) has applied one of the patches above to current code, corrected whitespace etc. and put it on his github so that it can be submitted using standard procedure. He has also completed testing instructions which I have added to the bug.

          Hopefully with this assistance (on top of the person who actually wrote the code, well done!) we can get this into Moodle 2.2.

          Note: When reviewing the code I searched; in core, none of the calls to this function actually specify any arguments except the first two. That means that as best I can see, the behaviour change caused by this function is:

          • All places which use this function previously skipped text instead <head>, <nolink>, and <span class="nolink">.
          • They now additionally skip text inside <script>, <textarea>, <select>, and <a> tags (with any or no attributes).

          Second note: I am away the following 2 weeks, sorry if this causes delays.

          Show
          Sam Marshall added a comment - We came across this problem at the OU. Ray Guo (one of the developers on my team) has applied one of the patches above to current code, corrected whitespace etc. and put it on his github so that it can be submitted using standard procedure. He has also completed testing instructions which I have added to the bug. Hopefully with this assistance (on top of the person who actually wrote the code, well done!) we can get this into Moodle 2.2. Note: When reviewing the code I searched; in core, none of the calls to this function actually specify any arguments except the first two. That means that as best I can see, the behaviour change caused by this function is: All places which use this function previously skipped text instead <head>, <nolink>, and <span class="nolink">. They now additionally skip text inside <script>, <textarea>, <select>, and <a> tags (with any or no attributes). Second note: I am away the following 2 weeks, sorry if this causes delays.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks Sam, this has been integrated to master now.

          For those testing this has only been integrated to master as it is an improvement.

          Cheer
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Sam, this has been integrated to master now. For those testing this has only been integrated to master as it is an improvement. Cheer Sam
          Hide
          Rajesh Taneja added a comment -

          Works Great Sam.
          Thanks for fixing this issue

          Show
          Rajesh Taneja added a comment - Works Great Sam. Thanks for fixing this issue
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YTC !

          (aka, yay, thanks and ciao ) Closing.

          Show
          Eloy Lafuente (stronk7) added a comment - YTC ! (aka, yay, thanks and ciao ) Closing.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: