Skip to content

Commit

Permalink
MySqlDatabase: clean up string and array methods
Browse files Browse the repository at this point in the history 
* Remove quoting options from `escapeString` and `escapeArray`.
  These complicated the interface without adding any useful logic.
  We now always quote the result.

* Bool and array types can no longer be passed to `escapeString`
  (as enforced by the argument type-hint). Use `escapeBoolean` or
  `escapeArray` instead (or just `escape` to let the function be
  chosen automatically). It's possible there are some places where
  we are still passing these types to `escapeString`, and if so we
  will fix them as they are identified.

* Add unit tests for `escapeString` and `escapeArray`.
  • Loading branch information
@hemberger
hemberger committed Dec 20, 2020
1 parent a7e18b1 commit 3b996d9
Show file tree
Hide file tree
Showing 11 changed files with 79 additions and 67 deletions.
2 changes: 1 addition & 1 deletion admin/Default/game_status_processing.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
$action = Request::get('action');
if ($action == 'Close') {
$reason = Request::get('close_reason');
$db->query('REPLACE INTO game_disable (reason) VALUES (' . $db->escapeString($reason, true) . ');');
$db->query('REPLACE INTO game_disable (reason) VALUES (' . $db->escapeString($reason) . ');');
$db->query('DELETE FROM active_session;');
$container['msg'] = '<span class="green">SUCCESS: </span>You have closed the server. You will now be logged out!';
} elseif ($action == 'Open') {
Expand Down
4 changes: 2 additions & 2 deletions engine/Default/hall_of_fame_new.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,11 @@
$query = 'SELECT account_id, ' . $statements['CASE'] . ' amount FROM (SELECT account_id, type, SUM(amount) amount FROM player_hof WHERE type IN (' . $statements['IN'] . ')' . $gameIDSql . ' GROUP BY account_id,type) x GROUP BY account_id ORDER BY amount DESC, account_id ASC LIMIT 25';
$db->query($query);
} else {
$db->query('SELECT visibility FROM hof_visibility WHERE type = ' . $db->escapeArray($viewType, false, true, ':', false) . ' LIMIT 1');
$db->query('SELECT visibility FROM hof_visibility WHERE type = ' . $db->escapeArray($viewType, ':', false) . ' LIMIT 1');
if ($db->nextRecord()) {
$vis = $db->getField('visibility');
}
$db->query('SELECT account_id,SUM(amount) amount FROM player_hof WHERE type=' . $db->escapeArray($viewType, false, true, ':', false) . $gameIDSql . ' GROUP BY account_id ORDER BY amount DESC, account_id ASC LIMIT 25');
$db->query('SELECT account_id,SUM(amount) amount FROM player_hof WHERE type=' . $db->escapeArray($viewType, ':', false) . $gameIDSql . ' GROUP BY account_id ORDER BY amount DESC, account_id ASC LIMIT 25');
}
$rows = [];
while ($db->nextRecord()) {
Expand Down
2 changes: 1 addition & 1 deletion engine/Default/rankings_alliance_profit.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
$db->requireRecord();
$numAlliances = $db->getInt('count(*)');
$profitType = array('Trade', 'Money', 'Profit');
$profitTypeEscaped = $db->escapeArray($profitType, false, true, ':', false);
$profitTypeEscaped = $db->escapeArray($profitType, ':', false);

$ourRank = 0;
if ($player->hasAlliance()) {
Expand Down
2 changes: 1 addition & 1 deletion engine/Default/rankings_player_profit.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
Menu::rankings(0, 1);

$profitType = array('Trade', 'Money', 'Profit');
$profitTypeEscaped = $db->escapeArray($profitType, false, true, ':', false);
$profitTypeEscaped = $db->escapeArray($profitType, ':', false);

// what rank are we?
$db->query('SELECT count(*)
Expand Down
18 changes: 9 additions & 9 deletions lib/Default/AbstractSmrAccount.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,12 @@ public static function getUserScoreCaseStatement($db) {
$userRankingTypes = array();
$case = 'FLOOR(SUM(CASE type ';
foreach (self::USER_RANKINGS_SCORE as $userRankingScore) {
$userRankingType = $db->escapeArray($userRankingScore[0], false, false, ':', false);
$userRankingType = $db->escapeArray($userRankingScore[0], ':', false);
$userRankingTypes[] = $userRankingType;
$case .= ' WHEN ' . $db->escapeString($userRankingType) . ' THEN POW(amount*' . $userRankingScore[1] . ',' . SmrAccount::USER_RANKINGS_EACH_STAT_POW . ')*' . $userRankingScore[2];
$case .= ' WHEN ' . $userRankingType . ' THEN POW(amount*' . $userRankingScore[1] . ',' . SmrAccount::USER_RANKINGS_EACH_STAT_POW . ')*' . $userRankingScore[2];
}
$case .= ' END))';
return array('CASE'=>$case, 'IN'=>$db->escapeArray($userRankingTypes));
return array('CASE' => $case, 'IN' => join(',', $userRankingTypes));
}

protected function __construct($accountID) {
Expand Down Expand Up @@ -284,16 +284,16 @@ public function update() {
', logging = ' . $this->db->escapeBoolean($this->logging) .
', time_short = ' . $this->db->escapeString($this->timeShort) .
', date_short = ' . $this->db->escapeString($this->dateShort) .
', discord_id = ' . $this->db->escapeString($this->discordId, true, true) .
', irc_nick = ' . $this->db->escapeString($this->ircNick, true, true) .
', discord_id = ' . $this->db->escapeString($this->discordId, true) .
', irc_nick = ' . $this->db->escapeString($this->ircNick, true) .
', hof_name = ' . $this->db->escapeString($this->hofName) .
', template = ' . $this->db->escapeString($this->template) .
', colour_scheme = ' . $this->db->escapeString($this->colourScheme) .
', fontsize = ' . $this->db->escapeNumber($this->fontSize) .
', css_link = ' . $this->db->escapeString($this->cssLink, true, true) .
', friendly_colour = ' . $this->db->escapeString($this->friendlyColour, true, true) .
', neutral_colour = ' . $this->db->escapeString($this->neutralColour, true, true) .
', enemy_colour = ' . $this->db->escapeString($this->enemyColour, true, true) .
', css_link = ' . $this->db->escapeString($this->cssLink, true) .
', friendly_colour = ' . $this->db->escapeString($this->friendlyColour, true) .
', neutral_colour = ' . $this->db->escapeString($this->neutralColour, true) .
', enemy_colour = ' . $this->db->escapeString($this->enemyColour, true) .
' WHERE ' . $this->SQL . ' LIMIT 1');
$this->hasChanged = false;
}
Expand Down
8 changes: 4 additions & 4 deletions lib/Default/AbstractSmrPlayer.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1742,7 +1742,7 @@ public function addDestinationButton($sectorID, $label) {

$this->db->query('
INSERT INTO player_stored_sector (account_id, game_id, sector_id, label, offset_top, offset_left)
VALUES (' . $this->db->escapeNumber($this->getAccountID()) . ', ' . $this->db->escapeNumber($this->getGameID()) . ', ' . $this->db->escapeNumber($sectorID) . ',' . $this->db->escapeString($label, true) . ',1,1)'
VALUES (' . $this->db->escapeNumber($this->getAccountID()) . ', ' . $this->db->escapeNumber($this->getGameID()) . ', ' . $this->db->escapeNumber($sectorID) . ',' . $this->db->escapeString($label) . ',1,1)'
);
}

Expand Down Expand Up @@ -2221,7 +2221,7 @@ public function &killPlayerByPlayer(AbstractSmrPlayer $killer) {
}
$msg .= ' in Sector&nbsp;' . Globals::getSectorBBLink($this->getSectorID());
$this->getSector()->increaseBattles(1);
$this->db->query('INSERT INTO news (game_id,time,news_message,type,killer_id,killer_alliance,dead_id,dead_alliance) VALUES (' . $this->db->escapeNumber($this->getGameID()) . ',' . $this->db->escapeNumber(SmrSession::getTime()) . ',' . $this->db->escapeString($msg, true) . ',\'regular\',' . $this->db->escapeNumber($killer->getAccountID()) . ',' . $this->db->escapeNumber($killer->getAllianceID()) . ',' . $this->db->escapeNumber($this->getAccountID()) . ',' . $this->db->escapeNumber($this->getAllianceID()) . ')');
$this->db->query('INSERT INTO news (game_id,time,news_message,type,killer_id,killer_alliance,dead_id,dead_alliance) VALUES (' . $this->db->escapeNumber($this->getGameID()) . ',' . $this->db->escapeNumber(SmrSession::getTime()) . ',' . $this->db->escapeString($msg) . ',\'regular\',' . $this->db->escapeNumber($killer->getAccountID()) . ',' . $this->db->escapeNumber($killer->getAllianceID()) . ',' . $this->db->escapeNumber($this->getAccountID()) . ',' . $this->db->escapeNumber($this->getAllianceID()) . ')');

self::sendMessageFromFedClerk($this->getGameID(), $this->getAccountID(), 'You were <span class="red">DESTROYED</span> by ' . $killer->getBBLink() . ' in sector ' . Globals::getSectorBBLink($this->getSectorID()));
self::sendMessageFromFedClerk($this->getGameID(), $killer->getAccountID(), 'You <span class="red">DESTROYED</span>&nbsp;' . $this->getBBLink() . ' in sector ' . Globals::getSectorBBLink($this->getSectorID()));
Expand Down Expand Up @@ -3185,12 +3185,12 @@ protected function doHOFSave(array $hasChangedList, array $typeList = array()) {
$amount = $this->getHOF($tempTypeList);
if ($hofChanged == self::HOF_NEW) {
if ($amount > 0) {
$this->db->query('INSERT INTO player_hof (account_id,game_id,type,amount) VALUES (' . $this->db->escapeNumber($this->getAccountID()) . ',' . $this->db->escapeNumber($this->getGameID()) . ',' . $this->db->escapeArray($tempTypeList, false, true, ':', false) . ',' . $this->db->escapeNumber($amount) . ')');
$this->db->query('INSERT INTO player_hof (account_id,game_id,type,amount) VALUES (' . $this->db->escapeNumber($this->getAccountID()) . ',' . $this->db->escapeNumber($this->getGameID()) . ',' . $this->db->escapeArray($tempTypeList, ':', false) . ',' . $this->db->escapeNumber($amount) . ')');
}
} elseif ($hofChanged == self::HOF_CHANGED) {
$this->db->query('UPDATE player_hof
SET amount=' . $this->db->escapeNumber($amount) . '
WHERE ' . $this->SQL . ' AND type = ' . $this->db->escapeArray($tempTypeList, false, true, ':', false) . ' LIMIT 1');
WHERE ' . $this->SQL . ' AND type = ' . $this->db->escapeArray($tempTypeList, ':', false) . ' LIMIT 1');
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/Default/ChessGame.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -680,7 +680,7 @@ public function tryMove($x, $y, $toX, $toY, $forAccountID, $pawnPromotionPiece)
$this->db->query('INSERT INTO chess_game_moves
(chess_game_id,piece_id,start_x,start_y,end_x,end_y,checked,piece_taken,castling,en_passant,promote_piece_id)
VALUES
(' . $this->db->escapeNumber($p->chessGameID) . ',' . $this->db->escapeNumber($pieceID) . ',' . $this->db->escapeNumber($x) . ',' . $this->db->escapeNumber($y) . ',' . $this->db->escapeNumber($toX) . ',' . $this->db->escapeNumber($toY) . ',' . $this->db->escapeString($checking, true, true) . ',' . ($moveInfo['PieceTaken'] == null ? 'NULL' : $this->db->escapeNumber($moveInfo['PieceTaken']->pieceID)) . ',' . $this->db->escapeString($castlingType, true, true) . ',' . $this->db->escapeBoolean($moveInfo['EnPassant']) . ',' . ($moveInfo['PawnPromotion'] == false ? 'NULL' : $this->db->escapeNumber($moveInfo['PawnPromotion']['PieceID'])) . ');');
(' . $this->db->escapeNumber($p->chessGameID) . ',' . $this->db->escapeNumber($pieceID) . ',' . $this->db->escapeNumber($x) . ',' . $this->db->escapeNumber($y) . ',' . $this->db->escapeNumber($toX) . ',' . $this->db->escapeNumber($toY) . ',' . $this->db->escapeString($checking, true) . ',' . ($moveInfo['PieceTaken'] == null ? 'NULL' : $this->db->escapeNumber($moveInfo['PieceTaken']->pieceID)) . ',' . $this->db->escapeString($castlingType, true) . ',' . $this->db->escapeBoolean($moveInfo['EnPassant']) . ',' . ($moveInfo['PawnPromotion'] == false ? 'NULL' : $this->db->escapeNumber($moveInfo['PawnPromotion']['PieceID'])) . ');');


$currentPlayer->increaseHOF(1, array($chessType, 'Moves', 'Total Taken'), HOF_PUBLIC);
Expand Down
57 changes: 15 additions & 42 deletions lib/Default/MySqlDatabase.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,71 +203,44 @@ protected function error($err) {
throw new RuntimeException($err);
}

public function escape($escape, $autoQuotes = true, $quotes = true) {
public function escape($escape) {
if (is_bool($escape)) {
return $this->escapeBoolean($escape);
}
if (is_numeric($escape)) {
return $this->escapeNumber($escape);
}
if (is_string($escape)) {
if ($autoQuotes) {
return $this->escapeString($escape);
} else {
return $this->escapeString($escape, $quotes);
}
return $this->escapeString($escape);
}
if (is_array($escape)) {
return $this->escapeArray($escape, $autoQuotes, $quotes);
return $this->escapeArray($escape);
}
if (is_object($escape)) {
if ($autoQuotes) {
return $this->escapeObject($escape);
} else {
return $this->escapeObject($escape, $quotes);
}
return $this->escapeObject($escape);
}
}

public function escapeString($string, $quotes = true, $nullable = false) {
public function escapeString(?string $string, bool $nullable = false) : string {
if ($nullable === true && ($string === null || $string === '')) {
return 'NULL';
}
if ($string === true) {
$string = 'TRUE';
} elseif ($string === false) {
$string = 'FALSE';
}
if (is_array($string)) {
$escapedString = '';
foreach ($string as $value) {
$escapedString .= $this->escapeString($value, $quotes) . ',';
}
return substr($escapedString, 0, -1);
}
if ($quotes) {
return '\'' . $this->dbConn->real_escape_string($string) . '\'';
}
return $this->dbConn->real_escape_string($string);
return '\'' . $this->dbConn->real_escape_string($string) . '\'';
}

public function escapeBinary($binary) {
return '0x' . bin2hex($binary);
}

public function escapeArray(array $array, $autoQuotes = true, $quotes = true, $implodeString = ',', $escapeIndividually = true) {
$string = '';
/**
* Warning: If escaping a nested array, use escapeIndividually=true,
* but beware that the escaped array is flattened!
*/
public function escapeArray(array $array, string $delimiter = ',', bool $escapeIndividually = true) : string {
if ($escapeIndividually) {
foreach ($array as $value) {
if (is_array($value)) {
$string .= $this->escapeArray($value, $autoQuotes, $quotes, $implodeString, $escapeIndividually) . $implodeString;
} else {
$string .= $this->escape($value, $autoQuotes, $quotes) . $implodeString;
}
}
$string = substr($string, 0, -1);
$string = join($delimiter, array_map(function($item) { return $this->escape($item); }, $array));
} else {
$string = $this->escape(implode($implodeString, $array), $autoQuotes, $quotes);
$string = $this->escape(join($delimiter, $array));
}
return $string;
}
Expand Down Expand Up @@ -296,10 +269,10 @@ public function escapeBoolean(bool $bool) : string {
}
}

public function escapeObject($object, $compress = false, $quotes = true, $nullable = false) {
public function escapeObject($object, $compress = false, $nullable = false) {
if ($compress === true) {
return $this->escapeBinary(gzcompress(serialize($object)));
}
return $this->escapeString(serialize($object), $quotes, $nullable);
return $this->escapeString(serialize($object), $nullable);
}
}
6 changes: 3 additions & 3 deletions lib/Default/SmrAlliance.class.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,13 +463,13 @@ public function update() : void {
alliance_password = ' . $this->db->escapeString($this->password) . ',
recruiting = ' . $this->db->escapeBoolean($this->recruiting) . ',
alliance_account = ' . $this->db->escapeNumber($this->bank) . ',
alliance_description = ' . $this->db->escapeString($this->description, true, true) . ',
alliance_description = ' . $this->db->escapeString($this->description, true) . ',
`mod` = ' . $this->db->escapeString($this->motd) . ',
img_src = ' . $this->db->escapeString($this->imgSrc) . ',
alliance_kills = ' . $this->db->escapeNumber($this->kills) . ',
alliance_deaths = ' . $this->db->escapeNumber($this->deaths) . ',
discord_server = ' . $this->db->escapeString($this->discordServer, true, true) . ',
discord_channel = ' . $this->db->escapeString($this->discordChannel, true, true) . ',
discord_server = ' . $this->db->escapeString($this->discordServer, true) . ',
discord_channel = ' . $this->db->escapeString($this->discordChannel, true) . ',
flagship_id = ' . $this->db->escapeNumber($this->flagshipID) . ',
leader_id = ' . $this->db->escapeNumber($this->leaderID) . '
WHERE ' . $this->SQL);
Expand Down
6 changes: 3 additions & 3 deletions lib/Default/hof.functions.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,12 @@ function getHofRank($view, $viewType, $accountID, $gameID) {
$query = 'SELECT ' . $statements['CASE'] . ' amount FROM (SELECT type, SUM(amount) amount FROM player_hof WHERE type IN (' . $statements['IN'] . ') AND account_id=' . $db->escapeNumber($accountID) . $gameIDSql . ' GROUP BY account_id,type) x ORDER BY amount DESC';
$db->query($query);
} else {
$db->query('SELECT visibility FROM hof_visibility WHERE type=' . $db->escapeArray($viewType, false, true, ':', false) . ' LIMIT 1');
$db->query('SELECT visibility FROM hof_visibility WHERE type=' . $db->escapeArray($viewType, ':', false) . ' LIMIT 1');
if (!$db->nextRecord()) {
return $rank;
}
$vis = $db->getField('visibility');
$db->query('SELECT SUM(amount) amount FROM player_hof WHERE type=' . $db->escapeArray($viewType, false, true, ':', false) . ' AND account_id=' . $db->escapeNumber($accountID) . $gameIDSql . ' GROUP BY account_id LIMIT 1');
$db->query('SELECT SUM(amount) amount FROM player_hof WHERE type=' . $db->escapeArray($viewType, ':', false) . ' AND account_id=' . $db->escapeNumber($accountID) . $gameIDSql . ' GROUP BY account_id LIMIT 1');
}

$realAmount = 0;
Expand All @@ -102,7 +102,7 @@ function getHofRank($view, $viewType, $accountID, $gameID) {
$query = 'SELECT COUNT(account_id) `rank` FROM (SELECT account_id FROM player_hof WHERE type IN (' . $statements['IN'] . ')' . $gameIDSql . ' GROUP BY account_id HAVING ' . $statements['CASE'] . '>' . $db->escapeNumber($rank['Amount']) . ') x';
$db->query($query);
} else {
$db->query('SELECT COUNT(account_id) `rank` FROM (SELECT account_id FROM player_hof WHERE type=' . $db->escapeArray($viewType, false, true, ':', false) . $gameIDSql . ' GROUP BY account_id HAVING SUM(amount)>' . $db->escapeNumber($realAmount) . ') x');
$db->query('SELECT COUNT(account_id) `rank` FROM (SELECT account_id FROM player_hof WHERE type=' . $db->escapeArray($viewType, ':', false) . $gameIDSql . ' GROUP BY account_id HAVING SUM(amount)>' . $db->escapeNumber($realAmount) . ') x');
}
if ($db->nextRecord()) {
$rank['Rank'] = $db->getInt('rank') + 1;
Expand Down
39 changes: 39 additions & 0 deletions test/SmrTest/lib/DefaultGame/MySqlDatabaseIntegrationTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,4 +76,43 @@ public function test_escapeBoolean() {
self::assertSame("'TRUE'", $db->escapeBoolean(true));
self::assertSame("'FALSE'", $db->escapeBoolean(false));
}

public function test_escapeString() {
$db = MySqlDatabase::getInstance();
// Test the empty string
self::assertSame("''", $db->escapeString(''));
self::assertSame('NULL', $db->escapeString('', true)); // nullable
// Test null
self::assertSame('NULL', $db->escapeString(null, true)); // nullable
// Test a normal string
self::assertSame("'bla'", $db->escapeString('bla'));
self::assertSame("'bla'", $db->escapeString('bla', true)); // nullable
}

public function test_escapeString_null_throws() {
$db = MySqlDatabase::getInstance();
$this->expectException(\TypeError::class);
$db->escapeString(null);
}

public function test_escapeArray() {
$db = MySqlDatabase::getInstance();
// Test a mixed array
self::assertSame("'a',2,'c'", $db->escapeArray(['a', 2, 'c']));
// Test a different implodeString
self::assertSame("'a':2:'c'", $db->escapeArray(['a', 2, 'c'], ':'));
// Test escapeIndividually=false
self::assertSame("'a,2,c'", $db->escapeArray(['a', 2, 'c'], ',', false));
// Test nested arrays
// Warning: The array is flattened, which may be unexpected!
self::assertSame("'a','x',9,2", $db->escapeArray(['a', ['x', 9], 2], ',', true));
}

public function test_escapeArray_nested_array_throws() {
// Warning: It is dangerous to use nested arrays with escapeIndividually=false
$db = MySqlDatabase::getInstance();
$this->expectNotice();
$this->expectNoticeMessage('Array to string conversion');
$db->escapeArray(['a', ['x', 9, 'y'], 2, 'c'], ':', false);
}
}

0 comments on commit 3b996d9

Please sign in to comment.