Moodle
  1. Moodle
  2. MDL-34991

comments entered via comments API when sent to messaging as notification: viewing them in message/index.php?viewing=recentnotifications does not convert CRLF to html br tag.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.1, 2.4
    • Fix Version/s: 2.4
    • Component/s: Messages
    • Labels:
    • Testing Instructions:
      Hide

      1) this requires a moodle plugin that will send out notifications (with multiple lines, '\n'). In my case here i've here the plugins DB where i'm attempting to do this.

      2) but really all we need to do to test this is :
      a) test that messaging of notifications still work fine. One way is via mod forum post notifications (/mod/forum/lib.php:714: $eventdata->notification = 1; ). so you can play around with that, perhaps add multiple lines to that notification and see that they render fine in the messaging notifications page (message/index.php?viewing=recentnotifications).

      b) test that no malicious scripts can get through to any user via any of the messaging processors ( jabber, email, and we've tested the webui in (a) so we can trust core there )

      Show
      1) this requires a moodle plugin that will send out notifications (with multiple lines, '\n'). In my case here i've here the plugins DB where i'm attempting to do this. 2) but really all we need to do to test this is : a) test that messaging of notifications still work fine. One way is via mod forum post notifications (/mod/forum/lib.php:714: $eventdata->notification = 1; ). so you can play around with that, perhaps add multiple lines to that notification and see that they render fine in the messaging notifications page (message/index.php?viewing=recentnotifications). b) test that no malicious scripts can get through to any user via any of the messaging processors ( jabber, email, and we've tested the webui in (a) so we can trust core there )
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      43578

      Description

      While working on MDLSITE-1427 : notifications (part of which were the comment from plugin comments ) were being renderered before converting text to html.

      The patch here should now allow notifications to be formatted by text_to_html() which is called from within format_text().

        Activity

        Hide
        Aparup Banerjee added a comment - - edited

        putting this up for Andrew's peer-review.

        EDIT: added Petr for his insight into format_text(). no idea why FORMAT_HTML and FORMAT_MOODLE is so slightly different (call to text_to_html).

        Show
        Aparup Banerjee added a comment - - edited putting this up for Andrew's peer-review. EDIT: added Petr for his insight into format_text(). no idea why FORMAT_HTML and FORMAT_MOODLE is so slightly different (call to text_to_html).
        Hide
        Aparup Banerjee added a comment -

        please cherry-pick to stables as necessary.

        Show
        Aparup Banerjee added a comment - please cherry-pick to stables as necessary.
        Hide
        Andrew Davis added a comment -

        The only question is whether we are double extra sure that this doesn't introduce the ability to inject html. Is there any way I can now add html in a comment and get it converted to HTML in a bad way. For example a script tag containing malicious script.

        Show
        Andrew Davis added a comment - The only question is whether we are double extra sure that this doesn't introduce the ability to inject html. Is there any way I can now add html in a comment and get it converted to HTML in a bad way. For example a script tag containing malicious script.
        Hide
        Aparup Banerjee added a comment -

        Thanks for the pointer Andrew,

        i've tried with script tags and they aren't reflected after text_to_html() in the comments api (comment on a plugin here). They're not in the notification page either.

        They are however showing up (script tags) in my email as plain text which seems fine.

        So it seems i can trust our comments and core still! Sending up integration. (adding a test too)

        Show
        Aparup Banerjee added a comment - Thanks for the pointer Andrew, i've tried with script tags and they aren't reflected after text_to_html() in the comments api (comment on a plugin here). They're not in the notification page either. They are however showing up (script tags) in my email as plain text which seems fine. So it seems i can trust our comments and core still! Sending up integration. (adding a test too)
        Hide
        Aparup Banerjee added a comment -

        up for integration review.
        (this might be considered for picking to stable)

        Show
        Aparup Banerjee added a comment - up for integration review. (this might be considered for picking to stable)
        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
        Eloy Lafuente (stronk7) added a comment -

        The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

        This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.

        This is a bulk-automated message, so if you want to blame some-body/thing/where, don't do it here (use git instead) :-D :-P

        Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

        Show
        Eloy Lafuente (stronk7) added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame some-body/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
        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 Apu, this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Apu, this has been integrated now
        Hide
        Rajesh Taneja added a comment -

        Works Grt,

        Thanks for fixing this, Aparup.

        Show
        Rajesh Taneja added a comment - Works Grt, Thanks for fixing this, Aparup.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Many thanks for the hard work.

        These changes have been spread upstream and are already available in the git and cvs repositories.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Many thanks for the hard work. These changes have been spread upstream and are already available in the git and cvs repositories. Ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: