Remove aggressive sanitization from achievements; fix escaping for to…

…oltips (#874) * support unicode characters in achievement title/description * support unicode characters in leaderboard title/description * support unicode characters in game title
RetroAchievements · Dec 12, 2021 · 91845be · 91845be
1 parent 9ec1da8
commit 91845be
Show file tree

Hide file tree

Showing 11 changed files with 43 additions and 36 deletions.
diff --git a/lib/database/achievement.php b/lib/database/achievement.php
@@ -457,6 +457,10 @@ function UploadNewAchievement(
     $badge,
     &$errorOut
 ) {
+    settype($gameID, 'integer');
+    settype($type, 'integer');
+    settype($points, 'integer');
+
     // Prevent <= registered users from uploading or modifying achievements
     if (getUserPermissions($author) <= \RA\Permissions::Registered) {
         $errorOut = "You must be a developer to perform this action! Please drop a message in the forums to apply.";
@@ -474,14 +478,8 @@ function UploadNewAchievement(
         return false;
     }
 
-    $title = str_replace("'", "''", $title);
-    $desc = str_replace("'", "''", $desc);
-    $title = str_replace("/", "_", $title);
-    $desc = str_replace("/", "_", $desc);
-    $title = str_replace("\\", "_", $title);
-    $desc = str_replace("\\", "_", $desc);
-    $title = preg_replace('/[^\x20-\x7e]/', '_', $title);
-    $desc = preg_replace('/[^\x20-\x7e]/', '_', $desc);
+    $dbAuthor = $author;
+    sanitize_sql_inputs($title, $desc, $mem, $progress, $progressMax, $progressFmt, $dbAuthor);
 
     //    Assume authorised!
     if (!isset($idInOut) || $idInOut == 0) {
@@ -498,15 +496,15 @@ function UploadNewAchievement(
             VALUES (
                 NULL, '$gameID', '$title', '$desc',
                 '$mem', '$progress', '$progressMax',
-                '$progressFmt', '$points', '$type',
-                '$author', NOW(), NOW(),
+                '$progressFmt', $points, $type,
+                '$dbAuthor', NOW(), NOW(),
                 NOW(), 0, 0,
                 '$badge', 0, NULL,
                 0
             )";
         // log_sql($query);
-        if (s_mysql_query($query) !== false) {
-            global $db;
+        global $db;
+        if (mysqli_query($db, $query) !== false) {
             $idInOut = mysqli_insert_id($db);
             postActivity($author, ActivityType::UploadAchievement, $idInOut);
 
@@ -557,7 +555,8 @@ function UploadNewAchievement(
             $query = "UPDATE Achievements SET Title='$title', Description='$desc', Progress='$progress', ProgressMax='$progressMax', ProgressFormat='$progressFmt', MemAddr='$mem', Points=$points, Flags=$type, DateModified=NOW(), Updated=NOW(), BadgeName='$badge' WHERE ID=$idInOut";
             // log_sql($query);
 
-            if (s_mysql_query($query) !== false) {
+            global $db;
+            if (mysqli_query($db, $query) !== false) {
                 if ($changingAchSet || $changingPoints) {
                     // When changing achievement set, all existing achievements that rely on this should be purged.
                     //$query = "DELETE FROM Awarded WHERE ID='$idInOut'";

diff --git a/lib/database/leaderboard.php b/lib/database/leaderboard.php
@@ -791,8 +791,10 @@ function getLeaderboardsList($consoleIDInput, $gameID, $sortBy, $count, $offset,
 
 function submitLBData($user, $lbID, $lbMem, $lbTitle, $lbDescription, $lbFormat, $lbLowerIsBetter, $lbDisplayOrder)
 {
-    sanitize_sql_inputs($user, $lbID, $lbMem, $lbTitle, $lbDescription, $lbFormat, $lbLowerIsBetter, $lbDisplayOrder);
+    sanitize_sql_inputs($user, $lbMem, $lbTitle, $lbDescription, $lbFormat);
+    settype($lbID, 'integer');
     settype($lbDisplayOrder, 'integer');
+    settype($lbLowerIsBetter, 'integer');
 
     $query = "UPDATE LeaderboardDef AS ld SET
               ld.Mem = '$lbMem',
@@ -804,7 +806,8 @@ function submitLBData($user, $lbID, $lbMem, $lbTitle, $lbDescription, $lbFormat,
               ld.DisplayOrder = '$lbDisplayOrder'
               WHERE ld.ID = $lbID";
 
-    $dbResult = s_mysql_query($query);
+    global $db;
+    $dbResult = mysqli_query($db, $query);
     if ($dbResult !== false) {
         // error_log(__FILE__);
         // error_log("$user changed Leaderboard $lbID: $lbMem, $lbTitle, $lbDescription, $lbFormat, $lbLowerIsBetter, $lbDisplayOrder");

diff --git a/lib/render/achievement.php b/lib/render/achievement.php
@@ -49,7 +49,7 @@ function GetAchievementAndTooltipDiv(
     }
 
     if ($inclSmallBadge) {
-        $achNameAttr = htmlspecialchars($achName, ENT_QUOTES);
+        $achNameAttr = attributeEscape($achName);
         $smallBadgePath = "/Badge/$badgeName" . ".png";
         $smallBadge = "<img loading='lazy' width='$smallBadgeSize' height='$smallBadgeSize' src=\"" . getenv('ASSET_URL') . "$smallBadgePath\" alt='$achNameAttr' title='$achNameAttr' class='$imgclass' />";
 

diff --git a/lib/render/game.php b/lib/render/game.php
@@ -32,14 +32,9 @@ function GetGameAndTooltipDiv(
     $tooltip .= $consoleStr;
     $tooltip .= "</div>";
     $tooltip .= "</div>";
+    $tooltip = attributeEscape($tooltip);
 
-    // $tooltip = str_replace('<', '&lt;', $tooltip);
-    // $tooltip = str_replace('>', '&gt;', $tooltip);
-    //echo $tooltip;
-    //$tooltip = str_replace( "'", "\\'", $tooltip );
-    //echo $tooltip;
-
-    $tooltip = str_replace("'", "\'", $tooltip);
+    $gameNameEscaped = attributeEscape($gameName);
 
     $displayable = "";
 
@@ -48,7 +43,7 @@ function GetGameAndTooltipDiv(
     );
 
     if ($justText == false) {
-        $displayable = "<img loading='lazy' alt='' title=\"$gameName\" src='" . getenv('ASSET_URL') . "$gameIcon' width='$imgSizeOverride' height='$imgSizeOverride' class='badgeimg' />";
+        $displayable = "<img loading='lazy' alt='' title=\"$gameNameEscaped\" src='" . getenv('ASSET_URL') . "$gameIcon' width='$imgSizeOverride' height='$imgSizeOverride' class='badgeimg' />";
     }
 
     if ($justImage == false) {

diff --git a/lib/render/layout.php b/lib/render/layout.php
@@ -334,7 +334,7 @@ function RenderToolbar($user, $permissions = 0)
 
     $searchQuery = null;
     if ($_SERVER['SCRIPT_NAME'] === '/searchresults.php') {
-        $searchQuery = requestInputQuery('s', null);
+        $searchQuery = attributeEscape(requestInputQuery('s', null));
     }
     echo "<form action='/searchresults.php' method='get'>";
     echo "<div class='searchbox'>";

diff --git a/lib/render/user.php b/lib/render/user.php
@@ -235,6 +235,7 @@ function RenderSiteAwards($userAwards)
             }
 
             $tooltip .= "\r\nAwarded on $awardDate";
+            $tooltip = attributeEscape($tooltip);
             $displayable = "<a href=\"$linkdest\"><img class=\"$imgclass\" alt=\"$tooltip\" title=\"$tooltip\" src=\"$imagepath\" width=\"$imageSize\" height=\"$imageSize\" /></a>";
             $tooltipImagePath = "$imagepath";
             $tooltipImageSize = 96; //64;    //    screw that, lets make it big!
@@ -309,8 +310,7 @@ function RenderCompletedGamesList($user, $userCompletedGamesList)
 
         $tooltipImagePath = "$nextImageIcon";
         $tooltipImageSize = 96; //64;    //    screw that, lets make it big!
-        $tooltipTitle = "$nextTitle";
-        //$tooltipTitle = str_replace( "'", "\'", $tooltipTitle );
+        $tooltipTitle = attributeEscape($nextTitle);
         $tooltip = "Progress: $nextNumAwarded achievements won out of a possible $nextMaxPossible";
         $tooltip = sprintf("%s (%01.1f%%)", $tooltip, ($nextTotalAwarded / $nextMaxPossible) * 100);
 

diff --git a/lib/util/string.php b/lib/util/string.php
@@ -9,6 +9,16 @@ function sanitize_outputs(&...$outputs)
     }
 }
 
+function attributeEscape($input)
+{
+    // htmlspecialchars escapes a bunch of stuff that the tooltip can't handle
+    // (like &rsquo; $frac12; and &deg;). when placed in title or alt fields.
+    // just do the bare minimum.
+    $input = str_replace("'", "&#39;", $input);
+    $input = str_replace('"', "&quot;", $input);
+    return $input;
+}
+
 function isValidUsername($userTest)
 {
     if (

diff --git a/public/achievementInfo.php b/public/achievementInfo.php
@@ -122,11 +122,11 @@ function onUpdateEmbedComplete(data) {
 
         echo "<table class='nicebox'><tbody>";
 
-        $escapeForAttributes = "htmlentities"; // sanitize_outputs uses null for 2nd param, so it won't convert quotes to entities
+        $descAttr = attributeEscape($desc);
         echo "<tr>";
         echo "<td style='width:70px'>";
         echo "<div id='achievemententryicon'>";
-        echo "<a href=\"/achievement/$achievementID\"><img src=\"$badgeFullPath\" title=\"$gameTitle ($achPoints)\n{$escapeForAttributes($desc)}\" alt=\"{$escapeForAttributes($desc)}\" align=\"left\" width=\"64\" height=\"64\" /></a>";
+        echo "<a href=\"/achievement/$achievementID\"><img src=\"$badgeFullPath\" title=\"$gameTitle ($achPoints)\n$descAttr\" alt=\"$descAttr\" align=\"left\" width=\"64\" height=\"64\" /></a>";
         echo "</div>"; //achievemententryicon
         echo "</td>";
 

diff --git a/public/gameInfo.php b/public/gameInfo.php
@@ -515,11 +515,12 @@ function submitSetRequest(user, gameID) {
             $imageIcon = $gameData['ImageIcon'];
             $imageTitle = $gameData['ImageTitle'];
             $imageIngame = $gameData['ImageIngame'];
+            $pageTitleAttr = attributeEscape($pageTitle);
 
             echo "<h3 class='longheader'>$pageTitle</h3>";
             echo "<table><tbody>";
             echo "<tr>";
-            echo "<td style='width:110px; padding: 7px' ><img src='$imageIcon' title='$pageTitle' width='96' height='96'></td>";
+            echo "<td style='width:110px; padding: 7px' ><img src='$imageIcon' title='$pageTitleAttr' width='96' height='96'></td>";
             echo "<td>";
             echo "<table class='gameinfo'><tbody>";
             if ($developer) {
@@ -962,8 +963,6 @@ function submitSetRequest(user, gameID) {
 
                             $earnedOnHardcore = isset($nextAch['DateEarnedHardcore']);
 
-                            $achDesc = str_replace('"', '\'', $achDesc);
-
                             $imgClass = $earnedOnHardcore ? 'goldimagebig' : 'badgeimg';
                             $tooltipText = $earnedOnHardcore ? '<br clear=all>Unlocked: ' . getNiceDate(strtotime($nextAch['DateEarnedHardcore'])) . '<br>-=HARDCORE=-' : '';
 

diff --git a/public/leaderboardList.php b/public/leaderboardList.php
@@ -255,8 +255,8 @@ function onUpdateComplete(data) {
 
     foreach ($lbData as $nextLB) {
         $lbID = $nextLB['ID'];
-        $lbTitle = $nextLB['Title'];
-        $lbDesc = $nextLB['Description'];
+        $lbTitle = attributeEscape($nextLB['Title']);
+        $lbDesc = attributeEscape($nextLB['Description']);
         $lbMem = $nextLB['Mem'];
         $lbFormat = $nextLB['Format'];
         $lbLowerIsBetter = $nextLB['LowerIsBetter'];

diff --git a/public/searchresults.php b/public/searchresults.php
@@ -33,7 +33,8 @@
         echo "<div class='searchbox longer'>";
         echo "<form action='/searchresults.php' method='get'>";
         //echo "Search:&nbsp;";
-        echo "<input size='42' name='s' type='text' class='searchboxinput' value='$searchQuery' placeholder='Search the site...' />";
+        $searchQueryEscaped = attributeEscape($searchQuery);
+        echo "<input size='42' name='s' type='text' class='searchboxinput' value='$searchQueryEscaped' placeholder='Search the site...' />";
         echo "&nbsp;&nbsp;";
         echo "<input type='submit' value='Search' />";
         echo "</form>";
@@ -55,7 +56,7 @@
                     $nextType = $nextResult['Type'];
                     $nextID = $nextResult['ID'];
                     $nextTarget = $nextResult['Target'];
-                    $nextTitle = strip_tags($nextResult['Title']);
+                    $nextTitle = attributeEscape(strip_tags($nextResult['Title']));
 
                     if ($nextType !== $lastType) {
                         $lastType = $nextType;