Improvements around validity checks for redirect urls (#22474)

* Improve URL check for logme redirect (#22402) * Improve validity check for redirect urls (#22412)
matomo-org · Aug 5, 2024 · d8b4b3b · d8b4b3b
1 parent 03895a3
commit d8b4b3b
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 2 deletions.
diff --git a/core/UrlHelper.php b/core/UrlHelper.php
@@ -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'] : '';
 

diff --git a/plugins/Login/Controller.php b/plugins/Login/Controller.php
@@ -335,7 +335,10 @@ protected function authenticateAndRedirect($login, $password, $urlToRedirect = f
 
         $parsedUrl = parse_url($urlToRedirect);
 
-        if (!empty($urlToRedirect) && false === $parsedUrl) {
+        if (
+            (!empty($urlToRedirect) && false === $parsedUrl)
+            || (!empty($parsedUrl['scheme']) && empty($parsedUrl['host']))
+        ) {
             $e = new \Piwik\Exception\Exception('The redirect URL is not valid.');
             $e->setIsHtmlMessage();
             throw $e;
@@ -348,6 +351,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', '');

diff --git a/plugins/Login/tests/UI/Login_spec.js b/plugins/Login/tests/UI/Login_spec.js
@@ -294,4 +294,25 @@ 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
+    });
+
+    it("should not redirect to invalid url", async function () {
+        testEnvironment.overrideConfig('General', 'login_allow_logme', '1');
+        testEnvironment.testUseMockAuth = 0;
+        testEnvironment.save();
+
+        await page.goto(formlessLoginUrl + "&url=http:google.com");
+
+        expect(await page.getWholeCurrentUrl()).to.contain(formlessLoginUrl + "&url=http:google.com"); // no redirect
+        expect(await page.evaluate(() => document.getElementsByClassName('content')[0].innerText)).to.contain('The redirect URL is not valid.');
+    });
 });