Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses the open redirect vulnerability in the auth_userkey login flow by introducing redirect hardening after key-based authentication, allowing external redirects only when explicitly configured.
Changes:
- Adds an
allowedredirecthostsadmin setting (semicolon-separated) to allowlist external redirect targets. - Routes post-authentication redirect selection through a new
get_redirect_url()helper that enforces host-based restrictions. - Adds new language strings describing the allowlist setting.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| settings.php | Adds the new auth_userkey/allowedredirecthosts configuration setting. |
| lang/en/auth_userkey.php | Adds UI strings for the new allowlist setting and its help text. |
| auth.php | Introduces get_redirect_url() and uses it to restrict redirects after userkey login. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| $redirecthost = parse_url($wantsurl, PHP_URL_HOST); | ||
| $localhost = parse_url($CFG->wwwroot, PHP_URL_HOST); | ||
|
|
||
| if (empty($redirecthost) || empty($localhost)) { | ||
| return $CFG->wwwroot; | ||
| } |
| if (empty($this->config->allowedredirecthosts)) { | ||
| return $CFG->wwwroot; | ||
| } | ||
|
|
||
| $allowedhosts = explode(';', $this->config->allowedredirecthosts); |
|
@Ewa55 thanks for submitting the patch. It mostly looks good. However, a couple of things still needs to be addressed before merging:
Hope that's not much. Thanks! Note: I haven't completed full review as the code may change after fixing all those items. |
|
Thanks @Ewa55 |
Fixes #124.
This change restricts external redirects after user key authentication.
External redirect URLs are now allowed only when their host is explicitly configured in the new 'allowedredirecthosts' setting. Local Moodle URLs continue to work without additional configuration.
The setting accepts a semicolon-separated list of allowed hosts, for example:
example.com;example.org
If an external host is not on the allowed list, the user is redirected to the Moodle site instead.
Manual testing instructions
wantsurl, for example/course/index.php, redirects successfully.wantsurlwhose host is not configured inallowedredirecthostsredirects to the Moodle site URL instead.example.com, to theAllowed redirect hostssetting.wantsurlusing the configured host redirects successfully to the requested external URL.Automated tests
Updated and extended the PHPUnit tests to cover:
wantsurlvalues;allowedredirecthosts;All
auth_userkeyPHPUnit tests pass successfully.