Improve URL check for logme redirect (#22402)

* Improve URL check for redirect

* adds regression test

core/UrlHelper.php

@@ -177,8 +177,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'] : '';
 

plugins/Login/Controller.php

@@ -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 = Common::unsanitizeInputValue(Common::getRequestVar('form_redirect', false));
             $redirectParams = UrlHelper::getArrayFromQueryString(UrlHelper::getQueryFromUrl($redirect));

plugins/Login/tests/UI/Login_spec.js

@@ -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
+    });
 });