Concrete CMS 9 through 9.4.2 is vulnerable to Stored XSS from Home Folder on Members Dashboard page

@@ -678,7 +678,7 @@ private function getFolderList()
         $rows = $db->fetchAll("SELECT tn.treeNodeId, tn.treeNodeName FROM TreeNodes AS tn LEFT JOIN TreeNodeTypes AS tnt ON (tn.treeNodeTypeID = tnt.treeNodeTypeID) WHERE tnt.treeNodeTypeHandle = 'file_folder' AND tn.treeNodeName != ''");
 
         foreach ($rows as $row) {
-            $folderList[$row["treeNodeId"]] = $row["treeNodeName"];
+            $folderList[$row["treeNodeId"]] = h($row["treeNodeName"]);
         }
 
         return $folderList;

@@ -48,7 +48,8 @@ public static function getFolderName($ui)
         $app = Application::getFacadeApplication();
         /** @var Connection $db */
         $db = $app->make(Connection::class);
-        return (string)$db->fetchColumn("SELECT treeNodeName FROM TreeNodes WHERE treeNodeId = ? LIMIT 1", [$ui->getUserHomeFolderId()]);
+        $folderName = (string)$db->fetchColumn("SELECT treeNodeName FROM TreeNodes WHERE treeNodeId = ? LIMIT 1", [$ui->getUserHomeFolderId()]);
+        return h($folderName);
     }
 
     public function __construct()

@@ -5,16 +5,53 @@
 
 class Url
 {
+    /**
+     * @param string|string[] $variable
+     * @param $value
+     * @param string|bool $url
+     * @return string
+     */
     public function setVariable($variable, $value = false, $url = false)
     {
-        // either it's key/value as variables, or it's an associative array of key/values
+        // Minimal normalization for URLs that may be injected into HTML attributes.
+        // We ONLY percent-encode quotes and strip CR/LF to close an XSS vector where
+        // some call sites forget to escape with htmlspecialchars(). We do NOT HTML-escape
+        // here to avoid double-encoding at render time (callers often use specialchars(..., false)).
+        $encodeQuotesAndStripCRLF = static function ($s) {
+            // Encode " and ' so they can't break out of href="...".
+            // Remove \r and \n to prevent attribute splitting / header-style injection.
+            return str_replace(['"', "'", "\r", "\n"], ['%22', '%27', '', ''], (string) $s);
+        };
 
         if ($url == false) {
+            // Use the current request as the base, but first strip any HTML-ish content.
+            // sanitizeString() removes tags like <script>… and similar markup.
             $url = Loader::helper('security')->sanitizeString($_SERVER['REQUEST_URI']);
-        } elseif (!strstr($url, '?')) {
-            $url = $url . '?' . Loader::helper('security')->sanitizeString(isset($_SERVER['QUERY_STRING']) ? $_SERVER['QUERY_STRING'] : '');
+            $url = $encodeQuotesAndStripCRLF($url);
+        } elseif (strpos($url, '?') === false) {
+            // Base URL provided without a query: protect it too (in case it contains quotes).
+            $url = $encodeQuotesAndStripCRLF($url);
+
+            // Append the current query string, after sanitizing and applying the same light encoding.
+            $qs = isset($_SERVER['QUERY_STRING']) ? $_SERVER['QUERY_STRING'] : '';
+            $qs = Loader::helper('security')->sanitizeString($qs);
+            if ($qs !== false && $qs !== '') {
+                $url .= '?' . $encodeQuotesAndStripCRLF($qs);
+            }
         }
 
+        /*
+        Why this change?
+        - Problem: In some places the resulting URL goes directly into HTML attributes (e.g., href="…")
+          without being escaped, so a literal " or ' in the URL can break the attribute and enable XSS.
+        - Fix (light touch for backward compatibility): Percent-encode only the characters that break out
+          of attributes ( " → %22, ' → %27 ) and strip CR/LF. We do NOT HTML-escape here to avoid
+          double-encoding at call sites that *do* properly escape with specialchars(..., false).
+        - Explicit replacements: We target just quotes and CR/LF to minimize side effects on existing URLs.
+          Broader normalization/validation (e.g., rebuilding the URL, enforcing schemes, or encoding more
+          characters) could change behavior that callers rely on, so we intentionally keep it narrow.
+        */
+
         $vars = array();
         if (!is_array($variable)) {
             $vars[$variable] = $value;

@@ -0,0 +1,164 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Concrete\Tests\Utility\Service;
+
+use Concrete\Tests\TestCase;
+
+class UrlTest extends TestCase
+{
+
+    /** @var array */
+    private $serverBackup;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        // Backup and seed $_SERVER
+        $this->serverBackup = $_SERVER;
+        $_SERVER['REQUEST_URI'] = '/list"items\'here?existing=1';
+        $_SERVER['QUERY_STRING'] = 'foo="bar"&baz=\'qux\'';
+    }
+
+    protected function tearDown(): void
+    {
+        $_SERVER = $this->serverBackup;
+        parent::tearDown();
+    }
+
+    /**
+     * Helper: get the URL helper under test.
+     */
+    private function urlHelper()
+    {
+        // If your test bootstrap wires \Core::make, use that. Otherwise, new up your helper class directly.
+        return \Core::make('helper/url');
+    }
+
+    /**
+     * It should use REQUEST_URI when $url == false and encode quotes / strip CRLF.
+     */
+    public function testUsesRequestUriWhenUrlIsFalseAndEncodesQuotes(): void
+    {
+        $uh = $this->urlHelper();
+
+        $out = $uh->setVariable([], false, false);
+
+        // REQUEST_URI contained both " and ' → must be percent-encoded
+        $this->assertStringContainsString('/list%22items%27here', $out, 'Base path quotes must be encoded.');
+        // Ensure no CR/LF made it through
+        $this->assertStringNotContainsString("\r", $out);
+        $this->assertStringNotContainsString("\n", $out);
+    }
+
+    /**
+     * When base URL has no '?', it must:
+     *  - encode quotes in the base URL
+     *  - sanitize and encode quotes in the query string before appending
+     */
+    public function testBaseUrlWithoutQueryGetsSanitizedQueryAppendedWithEncodedQuotes(): void
+    {
+        $uh = $this->urlHelper();
+
+        $_SERVER['QUERY_STRING'] = 'a="1"&b=\'2\''; // quotes to be encoded
+
+        $base = "/products/rock'n\"roll"; // both quotes in base URL
+        $out  = $uh->setVariable([], false, $base);
+
+        // Base URL quotes encoded
+        $this->assertStringContainsString('/products/rock%27n%22roll', $out);
+
+        // Query string appended and quotes encoded
+        $this->assertStringContainsString('?a=%221%22&b=%272%27', $out);
+
+        // No CR/LF
+        $this->assertStringNotContainsString("\r", $out);
+        $this->assertStringNotContainsString("\n", $out);
+    }
+
+    /**
+     * It should not double-encode when the output is fed back into setVariable() again.
+     */
+    public function testReentryDoesNotDoubleEncode(): void
+    {
+        $uh = $this->urlHelper();
+
+        $_SERVER['QUERY_STRING'] = 'x="y"';
+
+        $base = '/path"quote\'apostrophe';
+        $first = $uh->setVariable(['p' => 'v'], false, $base);
+        // Feed the result back in (simulating re-entry)
+        $second = $uh->setVariable(['p2' => 'v2'], false, $first);
+
+        // Quotes should appear encoded once, not as %2522 / %2527
+        $this->assertStringContainsString('/path%22quote%27apostrophe', $second);
+        $this->assertStringNotContainsString('%2522', $second, 'No double-encoding of %22.');
+        $this->assertStringNotContainsString('%2527', $second, 'No double-encoding of %27.');
+
+        // Both parameter sets present
+        $this->assertStringContainsString('p=v', $second);
+        $this->assertStringContainsString('p2=v2', $second);
+        $this->assertStringContainsString('x=%22y%22', $second);
+    }
+
+    /**
+     * If there is already a '?', the elseif branch does not run; verify we still
+     * retain existing query and that added variables merge correctly (no duplicate encoding).
+     */
+    public function testExistingQueryIsPreservedAndMergedWithoutReencoding(): void
+    {
+        $uh = $this->urlHelper();
+
+        $in = '/search?q=rock%27n%22roll'; // already percent-encoded quotes
+        $out = $uh->setVariable(['page' => '1'], false, $in);
+
+        // Existing encoding should not change (no %2527 / %2522)
+        $this->assertStringContainsString('q=rock%27n%22roll', $out);
+        $this->assertStringNotContainsString('%2527', $out);
+        $this->assertStringNotContainsString('%2522', $out);
+
+        // New param merged
+        $this->assertStringContainsString('page=1', $out);
+    }
+
+    /**
+     * Control characters in QUERY_STRING should be removed.
+     */
+    public function testControlCharsAreStripped(): void
+    {
+        $uh = $this->urlHelper();
+
+        $_SERVER['QUERY_STRING'] = "a=1\r\nb=2\"c'3";
+        $out = $uh->setVariable([], false, '/x');
+
+        $this->assertStringNotContainsString("\r", $out);
+        $this->assertStringNotContainsString("\n", $out);
+        // Quotes encoded from the query string portion
+        $this->assertStringContainsString('b=2%22c%273', $out);
+    }
+
+    public function testSimpleUrlsBehaveNormally(): void
+    {
+        $uh = $this->urlHelper();
+
+        // 1) When $url == false, we use REQUEST_URI as-is (no quotes to encode)
+        $_SERVER['REQUEST_URI'] = '/about';
+        $_SERVER['QUERY_STRING'] = '';
+        $out1 = $uh->setVariable([], false, false);
+        $this->assertSame('/about', $out1, 'Plain REQUEST_URI should pass through unchanged.');
+
+        // 2) Base URL without "?" picks up QUERY_STRING as-is (ampersands retained; no HTML escaping here)
+        $_SERVER['QUERY_STRING'] = 'page=2&sort=name';
+        $out2 = $uh->setVariable([], false, '/shop');
+        $this->assertSame('/shop?page=2&sort=name', $out2, 'Query string should be appended normally.');
+
+        // 3) Base URL with existing query keeps it; added vars merge in without re-encoding
+        $in   = '/search?q=test';
+        $out3 = $uh->setVariable(['page' => '1'], false, $in);
+        $this->assertStringContainsString('/search?q=test', $out3, 'Existing query must be preserved.');
+        $this->assertStringContainsString('page=1', $out3, 'New parameter should be merged in.');
+        $this->assertStringNotContainsString('%2520', $out3, 'No double-encoding of percent sequences.');
+    }
+
+}