Skip to content

Commit

Permalink
Improve URL check for logme redirect (#22402)
Browse files Browse the repository at this point in the history
* Improve URL check for redirect

* adds regression test
  • Loading branch information
sgiehl committed Aug 5, 2024
1 parent 03895a3 commit 4045c95
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 1 deletion.
8 changes: 7 additions & 1 deletion core/UrlHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -179,8 +179,14 @@ public static function getParseUrlReverse($parsed)
return false;
}

// According to RFC 1738, the chars ':', '@' and '/' need to be encoded in username or password part of an url
// We also encode '\' as a username or password containing that char, might be handled incorrectly by browsers
$escapeSpecialChars = function ($value) {
return str_replace([':', '@', '/', '\\'], [urlencode(':'), urlencode('@'), urlencode('/'), urlencode('\\')], $value);
};

$uri = !empty($parsed['scheme']) ? $parsed['scheme'] . ':' . (!strcasecmp($parsed['scheme'], 'mailto') ? '' : '//') : '';
$uri .= !empty($parsed['user']) ? $parsed['user'] . (!empty($parsed['pass']) ? ':' . $parsed['pass'] : '') . '@' : '';
$uri .= !empty($parsed['user']) ? $escapeSpecialChars($parsed['user']) . (!empty($parsed['pass']) ? ':' . $escapeSpecialChars($parsed['pass']) : '') . '@' : '';
$uri .= !empty($parsed['host']) ? $parsed['host'] : '';
$uri .= !empty($parsed['port']) ? ':' . $parsed['port'] : '';

Expand Down
5 changes: 5 additions & 0 deletions plugins/Login/Controller.php
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,11 @@ protected function authenticateAndRedirect($login, $password, $urlToRedirect = f
throw $e;
}

// We put together the url based on the parsed parameters manually to ensure it might not redirect to unexpected locations
// unescaped slashes in username or password part for example have unexpected results in browsers
// for protocol less urls starting with //, we need to prepend the double slash to have a url that passes the valid url check in redirect logic
$urlToRedirect = (strpos($urlToRedirect, '//') === 0 ? '//' : '') . UrlHelper::getParseUrlReverse($parsedUrl);

if (empty($urlToRedirect)) {
$redirect = Request::fromRequest()->getStringParameter('form_redirect', '');
$module = Request::fromQueryString(UrlHelper::getQueryFromUrl($redirect))->getStringParameter('module', '');
Expand Down
10 changes: 10 additions & 0 deletions plugins/Login/tests/UI/Login_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -294,4 +294,14 @@ describe("Login", function () {

expect(await page.getWholeCurrentUrl()).to.equal("https://matomo.org/security/");
});

it("should correctly redirect for unencoded url", async function () {
testEnvironment.overrideConfig('General', 'login_allow_logme', '1');
testEnvironment.testUseMockAuth = 0;
testEnvironment.save();

await page.goto(formlessLoginUrl + "&url=//google.com\\@localhost/path");

expect(await page.getWholeCurrentUrl()).to.equal("http://localhost/path"); // username part is hidden
});
});

0 comments on commit 4045c95

Please sign in to comment.