Moodle
  1. Moodle
  2. MDL-37020

Shibboleth processes wantsurl inconsistently

    Details

    • Testing Instructions:
      Hide
      1. Enable debugging
      2. Open a new browser window (no current Moodle session - including other tabs)
      3. Access /auth/shibboleth/index.php
      4. Verify you see an error about Shibboleth not being setup correctly
      5. Verify you do not see a warning like " Notice: Undefined property: stdClass::$wantsurl" in the output buffer on the page.
      Show
      Enable debugging Open a new browser window (no current Moodle session - including other tabs) Access /auth/shibboleth/index.php Verify you see an error about Shibboleth not being setup correctly Verify you do not see a warning like " Notice: Undefined property: stdClass::$wantsurl" in the output buffer on the page.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-37020-master

      Description

      MDL-35153 introduced support for WAYFless URLs in Shibboleth (which is working fine). It does this by checking for an optional "target" parameter passed from an external source. As written the patch doesn't anticipate use cases where both "target" and $SESSION->wantsurl are unset, and you can get a notice like this:

      Notice: Undefined property: stdClass::$wantsurl in .../auth/shibboleth/index.php on line 10

      More serious is that PARAM_LOCALURL behaves unexpectedly when the user isn't already authenticated. During an authentication scenario target can be set with the referring authentication URL. This is properly cleaned by optional_param, but it still results in $SESSION->wantsurl getting set, albeit empty. This means if you've got a bookmarked location on the Moodle instance in question, you're always getting dumped to the front page instead of the deep link unless you're already authenticated.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Damyon Wiese added a comment -

            Added test instructions.

            Show
            Damyon Wiese added a comment - Added test instructions.
            Hide
            Damyon Wiese added a comment -

            Peer review checklist:

            [Y] Syntax
            [-] Output
            [Y] Whitespace
            [-] Language
            [-] Databases
            [Y] Testing (I added the testing instructions for you - we really should get a shibboleth test vm setup)
            [Y] Security
            [-] Documentation
            [Y] Git
            [Y] Sanity check

            This patch looks OK to me - and these test instructions allow you to verify the bug without setting up shibboleth.

            Thanks Charles

            Show
            Damyon Wiese added a comment - Peer review checklist: [Y] Syntax [-] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing (I added the testing instructions for you - we really should get a shibboleth test vm setup) [Y] Security [-] Documentation [Y] Git [Y] Sanity check This patch looks OK to me - and these test instructions allow you to verify the bug without setting up shibboleth. Thanks Charles
            Hide
            Sam Hemelryk added a comment -

            Thanks Charles, your fix has been integrated now.

            Show
            Sam Hemelryk added a comment - Thanks Charles, your fix has been integrated now.
            Hide
            Michael de Raadt added a comment -

            Test result: Success.

            Tested in master only.

            Show
            Michael de Raadt added a comment - Test result: Success. Tested in master only.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

            Closing, ciao

            Show
            Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: