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

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

    Details

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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            jerome 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
            jerome Jérôme Mouneyrac added a comment -

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

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

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

            Show
            jerome Jérôme Mouneyrac added a comment - Added Apu as peer review as we came up to this solution yesterday.
            Hide
            nebgor 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
            nebgor 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
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            jerome 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
            jerome 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda 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: