Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-44634

Regression: link to user profile in comments block fails due to double URI encoding

    Details

      Description

      There was a bug fix in MDL-28952 that was reintroduced by MDL-38431.

      The comments block was outputting a double encoded URI for user's profile, which meant that the query parameters where ignored by profile.php, denying access to the resource.

      The change in MDL-38431 seems as if it is intended to encode the URL safely by calling moodle_url->out(true). The problem occurs that later in the execution, html_writer::link() is called, which appears to encode the query parameters a second time.

      The method where the second encoding occurs seems to be in outputcomponenets.php:

          public static function attribute($name, $value) {
              if ($value instanceof moodle_url) {
                  return ' ' . $name . '="' . $value->out() . '"';
              }
       
              // special case, we do not want these in output
              if ($value === null) {
                  return '';
              }
       
              // no sloppy trimming here!
              return ' ' . $name . '="' . s($value) . '"';
          }
      

      If there is a string, the function s() is called, which calls htmlspecialchars(), which will encode the ampersand into &

      function s($var) {
       
          if ($var === false) {
              return '0';
          }
       
          // When we move to PHP 5.4 as a minimum version, change ENT_QUOTES on the
          // next line to ENT_QUOTES | ENT_HTML5 | ENT_SUBSTITUTE, and remove the
          // 'UTF-8' argument. Both bring a speed-increase.
          return preg_replace('/&#(\d+|x[0-9a-f]+);/i', '&#$1;', htmlspecialchars($var, ENT_QUOTES, 'UTF-8'));
      }
      

      Passing in an object will cause html_writer::attribute() to call moodle_url->out(), which is how I fix this issue in my patch.

        Gliffy Diagrams

          Issue Links

            Activity

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - Patch available at https://github.com/dbezborodovrp/moodle/commit/e6b79031314e6da13cc645fd5ac08bc64f7e184f
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            This might also affect 2.3.x. Currently testing... will advise.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - This might also affect 2.3.x. Currently testing... will advise.
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            Fixed 2.3.x and 2.4.x. Still need to test 2.6.x and master.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - Fixed 2.3.x and 2.4.x. Still need to test 2.6.x and master.
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            This issue is also on master, and I assume 2.6.x. Will patch them also.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - This issue is also on master, and I assume 2.6.x. Will patch them also.
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment - - edited

            Clicking on the link in the screenshot will result in the message:

            "The details of this user are not available to you"

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - - edited Clicking on the link in the screenshot will result in the message: "The details of this user are not available to you"
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            The mouse is not visible in the screenshot. It is hovering over the username of "Hello Gwellooo" in the Comments block.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - The mouse is not visible in the screenshot. It is hovering over the username of "Hello Gwellooo" in the Comments block.
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            Okay, should be ready for review and patching now.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - Okay, should be ready for review and patching now.
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            I represent a company Netspot Pty Ltd. As part of our internal review, I received the following feedback:

            "This changes the data type of what is returned. Rather than getting a string as a URI the caller receives a moodle_url object.

            "Therefore the fix should be $url->out(false) so that the caller still receives a string, just one that isn't encoded."

            I will update my patch accordingly.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - I represent a company Netspot Pty Ltd. As part of our internal review, I received the following feedback: "This changes the data type of what is returned. Rather than getting a string as a URI the caller receives a moodle_url object. "Therefore the fix should be $url->out(false) so that the caller still receives a string, just one that isn't encoded." I will update my patch accordingly.
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            Change on master as per review:

            https://github.com/dbezborodovrp/moodle/commit/0a849bc52a306e3c255d7b891da07aee2dca7e4e

            Patch is now ready for upstream integration with Moodle.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - Change on master as per review: https://github.com/dbezborodovrp/moodle/commit/0a849bc52a306e3c255d7b891da07aee2dca7e4e Patch is now ready for upstream integration with Moodle.
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            (Pending Moodle's own review process, off course)

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - (Pending Moodle's own review process, off course)
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            This ticket is ready for review and integration into Moodle.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - This ticket is ready for review and integration into Moodle.
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that and providing a fix.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that and providing a fix.
            Hide
            salvetore Michael de Raadt added a comment -

            Rajesh Taneja: I've added you as a watcher on this issue as you were involved in the related issue.

            Show
            salvetore Michael de Raadt added a comment - Rajesh Taneja : I've added you as a watcher on this issue as you were involved in the related issue.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for fixing this Damien,

            Patch (https://github.com/dbezborodovrp/moodle/commit/0a849bc52a306e3c255d7b891da07aee2dca7e4e) looks good.
            Not sure why I changed it in first place, but surely looking at history and related code it should not be escaped.

            Can you please update comment by removing MDL-44634. It would be nice to replace it with something like "// URL should not be escaped."
            Also, it would be nice if you can please update back-port branches.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Damien, Patch ( https://github.com/dbezborodovrp/moodle/commit/0a849bc52a306e3c255d7b891da07aee2dca7e4e ) looks good. Not sure why I changed it in first place, but surely looking at history and related code it should not be escaped. Can you please update comment by removing MDL-44634 . It would be nice to replace it with something like "// URL should not be escaped." Also, it would be nice if you can please update back-port branches.
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            Hi Rajesh,

            My changes in regards to your code review have been pushed to Github and I have updated the Diff URLs.

            I had a disagreement with Github being a twit, so if something is wrong, please let me know. Somehow the same Commit-ID for MDL-44634-27 was also pushed to MDL-44634-master. Anyway, hopefully it works!

            Cheers,

            Damien

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - Hi Rajesh, My changes in regards to your code review have been pushed to Github and I have updated the Diff URLs. I had a disagreement with Github being a twit, so if something is wrong, please let me know. Somehow the same Commit-ID for MDL-44634 -27 was also pushed to MDL-44634 -master. Anyway, hopefully it works! Cheers, Damien
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for fixing this Damien,

            27 master is on-sync and hence it should not be an issue.
            Although, can you please fix comment http://docs.moodle.org/dev/Coding_style#Inline_comments (Comment should end with a proper punctuation character)

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Damien, 27 master is on-sync and hence it should not be an issue. Although, can you please fix comment http://docs.moodle.org/dev/Coding_style#Inline_comments (Comment should end with a proper punctuation character)
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment -

            I have updated my change with another commit. I couldn't do `git commit --amend` since I had already pushed my changes to Github.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - I have updated my change with another commit. I couldn't do `git commit --amend` since I had already pushed my changes to Github.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Damien,

            Sorry, I don't see the changes.

            As per pushing updates on existing branch, you can use git push -f (force).

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Damien, Sorry, I don't see the changes. As per pushing updates on existing branch, you can use git push -f (force).
            Hide
            netspot_dbezborodov Damien Bezborodov added a comment - - edited

            I have updated the Diff URLs. There should be two commits on each branch with my changes.

            Show
            netspot_dbezborodov Damien Bezborodov added a comment - - edited I have updated the Diff URLs. There should be two commits on each branch with my changes.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Damien,

            Pushing it for integration.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Damien, Pushing it for integration.
            Hide
            damyon Damyon Wiese added a comment -

            Thanks Damien,

            Integrated to 25, 26, 27 and master.

            Strictly I shouldn't have done 25 as it's already security only - but we are still being lenient during the on-sync period (next 2 weeks).

            The 2 commits should have really been squashed into one and force-pushed (next time).

            Thanks again - off to testing.

            Show
            damyon Damyon Wiese added a comment - Thanks Damien, Integrated to 25, 26, 27 and master. Strictly I shouldn't have done 25 as it's already security only - but we are still being lenient during the on-sync period (next 2 weeks). The 2 commits should have really been squashed into one and force-pushed (next time). Thanks again - off to testing.
            Hide
            ankit_frenz Ankit Agarwal added a comment -

            Works as described.

            Show
            ankit_frenz Ankit Agarwal added a comment - Works as described.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            So, finally, this landed, making Moodle better, many thanks!

            Be patient and understanding.
            Life is too short to be
            vengeful or malicious.

            Phillips Brooks

            Closing as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - So, finally, this landed, making Moodle better, many thanks! Be patient and understanding. Life is too short to be vengeful or malicious. Phillips Brooks Closing as fixed, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jul/14