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
    • Rank:
      2096

      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.

      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 Škoda added a comment -

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

        Show
        Petr Škoda 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: