Moodle
  1. Moodle
  2. MDL-30297

Redirect cleaning transforms "language" parameter into "Xlanguage" parameter

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Duplicate
    • Affects Version/s: 2.0, 2.1, 2.2
    • Fix Version/s: None
    • Component/s: Hub, Libraries
    • Labels:
    • Rank:
      32643

      Description

      In admin/register.php:

      redirect(new moodle_url($huburl . '/local/hub/siteregistration.php', $params));
      

      the $params contains a parameter called 'language'. Once redirected, the param become $Xlanguage into the $_REQUEST variable. It is not caused by new moodle_url() but by the redirect() call.

      I checked a bit more and in weblib.php, the following line seems to add a X in front the language parameter:

      $text = preg_replace("~([^a-z])language([[:space:]]*)=~i", "$1Xlanguage=", $text);
      

      If I removed the last commit: https://github.com/moodle/moodle/commit/581e8dba387f090d89382115fd850d8b44351526#lib/weblib.php , then it works.

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment - - edited

          Actually it's not really this line but somewhere in the last change (https://github.com/moodle/moodle/commit/581e8dba387f090d89382115fd850d8b44351526#lib/weblib.php). Regular expression are not my strenght, Petr can you have a look? Thanks.

          Show
          Jérôme Mouneyrac added a comment - - edited Actually it's not really this line but somewhere in the last change ( https://github.com/moodle/moodle/commit/581e8dba387f090d89382115fd850d8b44351526#lib/weblib.php ). Regular expression are not my strenght, Petr can you have a look? Thanks.
          Hide
          Jérôme Mouneyrac added a comment - - edited

          After a bit more looking the:

          $url = str_replace('&', '&', $encodedurl);
          

          is what cause the wrong url. However I suppose it's not the real problem. I detected in the previous version that the $encodedurl had "Xlanguage" when $url had "language".

          At least in the last commit, it's consistent ($encodedurl and $url have "Xlanguage"). It seems the issue is anterior and was located in $encodedurl... so the issue is now also in $url. I'm still looking to this, I don't know what this X protection is for so maybe I'm totaly not looking to the right thing...

          Show
          Jérôme Mouneyrac added a comment - - edited After a bit more looking the: $url = str_replace('&', '&', $encodedurl); is what cause the wrong url. However I suppose it's not the real problem. I detected in the previous version that the $encodedurl had "Xlanguage" when $url had "language". At least in the last commit, it's consistent ($encodedurl and $url have "Xlanguage"). It seems the issue is anterior and was located in $encodedurl... so the issue is now also in $url. I'm still looking to this, I don't know what this X protection is for so maybe I'm totaly not looking to the right thing...
          Hide
          Jérôme Mouneyrac added a comment -

          This is a fix that "revert back" the $url without adding X to language parameter.

          So with this fix it behaves like before the last commit:
          a) when redirect is straight forward, then nothing is added to parameter named 'language' or 'on'. (maybe this is a security issue?)
          b) when redirect is on a page with displayed content then a continue link is displayed. This continue link add X to 'language'/'on' (so this break the registration process but luckily it should never happen).

          In conclusion this is a temporary fix that bring back registration process to live. A new issue need to be filled about the X (it seems to be a fix for a security issue, but it breaks all 'language'/'on' redirect parameter.)

          Show
          Jérôme Mouneyrac added a comment - This is a fix that "revert back" the $url without adding X to language parameter. So with this fix it behaves like before the last commit: a) when redirect is straight forward, then nothing is added to parameter named 'language' or 'on'. (maybe this is a security issue?) b) when redirect is on a page with displayed content then a continue link is displayed. This continue link add X to 'language'/'on' (so this break the registration process but luckily it should never happen). In conclusion this is a temporary fix that bring back registration process to live. A new issue need to be filled about the X (it seems to be a fix for a security issue, but it breaks all 'language'/'on' redirect parameter.)
          Hide
          Jérôme Mouneyrac added a comment -

          The commit needs to be merged to 2.0 and 2.1 if accepted.

          Show
          Jérôme Mouneyrac added a comment - The commit needs to be merged to 2.0 and 2.1 if accepted.
          Hide
          Jérôme Mouneyrac added a comment -

          Added Apu as peer review as we came up to this solution yesterday.

          Show
          Jérôme Mouneyrac added a comment - Added Apu as peer review as we came up to this solution yesterday.
          Hide
          Aparup Banerjee added a comment -

          Jerome, it looks good to me. I've also commented in MDL-22925 for Petr just so he's aware.

          Show
          Aparup Banerjee added a comment - Jerome, it looks good to me. I've also commented in MDL-22925 for Petr just so he's aware.
          Hide
          Petr Škoda added a comment -

          The proposed str_replace('&', '&', $encodedurl); reordering creates a regression, do not integrate. I do not know how to solve this, the easiest workaround is to not use language= in URLs.

          Show
          Petr Škoda added a comment - The proposed str_replace('&', '&', $encodedurl); reordering creates a regression, do not integrate. I do not know how to solve this, the easiest workaround is to not use language= in URLs.
          Hide
          Jérôme Mouneyrac added a comment -

          OK no worries, in the meanwhile we find a fix, I'll add support for Xlanguage in the hub. I'll update Mooch. Cheers

          Show
          Jérôme Mouneyrac added a comment - OK no worries, in the meanwhile we find a fix, I'll add support for Xlanguage in the hub. I'll update Mooch. Cheers
          Hide
          Petr Škoda added a comment -

          The MDL-21617 should resolve this too, closing, thanks.

          Show
          Petr Škoda added a comment - The MDL-21617 should resolve this too, closing, thanks.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: