Issue #97: Fix for open redirect in logout function #99
Conversation
| public function user_logout_userkey() { | ||
| global $CFG, $USER; | ||
|
|
||
| $redirect = required_param('return', PARAM_URL); |
There was a problem hiding this comment.
Would it be easier to change this line to $redirect = required_param('return', PARAM_LOCALURL); ?
There was a problem hiding this comment.
Well observed @dmitriim! Because in this suggestion I thought about the option of preserving the redirection to an external address, according to the url registered in "Logout redirection URL". In this case, I have a question: if we define it as PARAM_LOCALURL the redirection will only be possible to addresses related to the Moodle installation, right?
There was a problem hiding this comment.
if we define it as PARAM_LOCALURL the redirection will only be possible to addresses related to the Moodle installation, right?
Yes, that's correct.
In your patch you're using $this->config->redirecturl which potentially confused you. This config is set in Moodle and used to always redirect a user after logging out from Moodle. E.g. redirecting to IDP logout page so a user is logged out from all apps at once.
However, in context of this issue $redirect is coning from outside of Moodle. E.g. when an external app requires users to logout from Moodle and then come back to the app for some other actions.
Hope that makes more sense now.
There was a problem hiding this comment.
Got it, @dmitriim! Thank you for the clarifications. I will make a new commit with your suggestion.
|
Hi @Cyber-Wo0dy! Thanks for proposed fix! Would be nice to support that fix with unit tests. |
#97 This is a suggested change to the logout function, limiting redirection to the url address selected in the configuration or, if there is no valid url in the configuration, redirecting to the Moodle home page. ;)