From d1b5cb6987d00beff348bd20b577dffd86e82f58 Mon Sep 17 00:00:00 2001 From: Luca Perna <12624410+MrMadClown@users.noreply.github.com> Date: Thu, 20 Oct 2022 06:52:38 +0200 Subject: [PATCH] 16276 - Reduce the usage of SuperGlobals (#17795) Signed-off-by: Luca Perna --- libraries/classes/Common.php | 2 +- .../Server/ReplicationController.php | 17 +++- .../Controllers/Table/TrackingController.php | 2 +- libraries/classes/ReplicationGui.php | 81 ++++++++----------- libraries/classes/Tracking.php | 4 +- libraries/classes/UrlRedirector.php | 12 ++- test/classes/ReplicationGuiTest.php | 5 +- 7 files changed, 61 insertions(+), 62 deletions(-) diff --git a/libraries/classes/Common.php b/libraries/classes/Common.php index 097ce789ad..198a05f48c 100644 --- a/libraries/classes/Common.php +++ b/libraries/classes/Common.php @@ -218,7 +218,7 @@ final class Common Tracker::enable(); if ($route === '/url') { - UrlRedirector::redirect(); + UrlRedirector::redirect($_GET['url'] ?? ''); } return; diff --git a/libraries/classes/Controllers/Server/ReplicationController.php b/libraries/classes/Controllers/Server/ReplicationController.php index b8b889164b..490df4d082 100644 --- a/libraries/classes/Controllers/Server/ReplicationController.php +++ b/libraries/classes/Controllers/Server/ReplicationController.php @@ -75,7 +75,15 @@ class ReplicationController extends AbstractController isset($_POST['replica_changeprimary']), isset($_POST['sr_replica_server_control']), $_POST['sr_replica_action'] ?? null, - isset($_POST['sr_replica_skip_error']) + isset($_POST['sr_replica_skip_error']), + isset($_POST['sr_skip_errors_count']) ? (int) $_POST['sr_skip_errors_count'] : 1, + $_POST['sr_replica_control_param'] ?? null, + [ + 'username' => $GLOBALS['dbi']->escapeString($_POST['username']), + 'pma_pw' => $GLOBALS['dbi']->escapeString($_POST['pma_pw']), + 'hostname' => $GLOBALS['dbi']->escapeString($_POST['hostname']), + 'port' => (int) $GLOBALS['dbi']->escapeString($_POST['text_port']), + ] ); } @@ -85,7 +93,9 @@ class ReplicationController extends AbstractController $primaryReplicationHtml = $this->replicationGui->getHtmlForPrimaryReplication( $_POST['primary_connection'] ?? null, $params['repl_clear_scr'], - $_POST['primary_add_user'] ?? null + $_POST['primary_add_user'] ?? null, + $_POST['username'] ?? null, + $_POST['hostname'] ?? null ); } @@ -96,7 +106,8 @@ class ReplicationController extends AbstractController $replicaConfigurationHtml = $this->replicationGui->getHtmlForReplicaConfiguration( $_POST['primary_connection'] ?? null, $replicaInfo['status'], - $replicationInfo->getReplicaStatus() + $replicationInfo->getReplicaStatus(), + isset($_POST['replica_configure']) ); } diff --git a/libraries/classes/Controllers/Table/TrackingController.php b/libraries/classes/Controllers/Table/TrackingController.php index 7fb9b15dee..567645083e 100644 --- a/libraries/classes/Controllers/Table/TrackingController.php +++ b/libraries/classes/Controllers/Table/TrackingController.php @@ -139,7 +139,7 @@ final class TrackingController extends AbstractController // Export as file download if (isset($_POST['report_export']) && $_POST['export_type'] === 'sqldumpfile') { - $this->tracking->exportAsFileDownload($GLOBALS['entries']); + $this->tracking->exportAsFileDownload($_POST['table'], $GLOBALS['entries']); } $actionMessage = ''; diff --git a/libraries/classes/ReplicationGui.php b/libraries/classes/ReplicationGui.php index d5383d27b1..0174c9da0a 100644 --- a/libraries/classes/ReplicationGui.php +++ b/libraries/classes/ReplicationGui.php @@ -17,7 +17,6 @@ use function mb_strtolower; use function mb_substr; use function sprintf; use function str_replace; -use function strlen; use function strtok; use function time; @@ -73,7 +72,9 @@ class ReplicationGui public function getHtmlForPrimaryReplication( ?string $connection, ?bool $replClearScr, - ?string $primaryAddUser + ?string $primaryAddUser, + ?string $username, + ?string $hostname ): string { if ($replClearScr === null) { $primaryStatusTable = $this->getHtmlForReplicationStatusTable($connection, 'primary', true, false); @@ -85,7 +86,7 @@ class ReplicationGui } if ($primaryAddUser !== null) { - $primaryAddReplicaUser = $this->getHtmlForReplicationPrimaryAddReplicaUser(); + $primaryAddReplicaUser = $this->getHtmlForReplicationPrimaryAddReplicaUser($username, $hostname); } return $this->template->render('server/replication/primary_replication', [ @@ -116,16 +117,18 @@ class ReplicationGui /** * returns HTML for replica replication configuration * - * @param ?string $connection Primary connection - * @param bool $serverReplicaStatus Whether it is Primary or Replica - * @param array $serverReplicaReplication Replica replication + * @param string|null $connection Primary connection + * @param bool $serverReplicaStatus Whether it is Primary or Replica + * @param array $serverReplicaReplication Replica replication + * @param bool $replicaConfigure Replica configure * * @return string HTML code */ public function getHtmlForReplicaConfiguration( ?string $connection, $serverReplicaStatus, - array $serverReplicaReplication + array $serverReplicaReplication, + bool $replicaConfigure ): string { $serverReplicaMultiReplication = $GLOBALS['dbi']->fetchResult('SHOW ALL SLAVES STATUS'); if ($serverReplicaStatus) { @@ -197,7 +200,7 @@ class ReplicationGui 'replica_control_io_link' => $replicaControlIoLink ?? '', 'replica_skip_error_link' => $replicaSkipErrorLink ?? '', 'reconfigure_primary_link' => $reconfigurePrimaryLink ?? '', - 'has_replica_configure' => isset($_POST['replica_configure']), + 'has_replica_configure' => $replicaConfigure, ]); } @@ -245,10 +248,10 @@ class ReplicationGui /** * This function returns html code for table with replication status. * - * @param ?string $connection primary connection - * @param string $type either primary or replica - * @param bool $isHidden if true, then default style is set to hidden, default value false - * @param bool $hasTitle if true, then title is displayed, default true + * @param string|null $connection primary connection + * @param string $type either primary or replica + * @param bool $isHidden if true, then default style is set to hidden, default value false + * @param bool $hasTitle if true, then title is displayed, default true * * @return string HTML code */ @@ -356,20 +359,18 @@ class ReplicationGui * * @return string HTML code */ - public function getHtmlForReplicationPrimaryAddReplicaUser(): string + public function getHtmlForReplicationPrimaryAddReplicaUser(?string $postUsername, ?string $hostname): string { [ $usernameLength, $hostnameLength, ] = $this->getUsernameHostnameLength(); - if (isset($_POST['username']) && strlen($_POST['username']) === 0) { - $GLOBALS['pred_username'] = 'any'; - } - $username = ''; - if (! empty($_POST['username'])) { - $username = $GLOBALS['new_username'] ?? $_POST['username']; + if ($postUsername === '') { + $GLOBALS['pred_username'] = 'any'; + } elseif ($postUsername !== null && $postUsername !== '0') { + $username = $GLOBALS['new_username'] ?? $postUsername; } $currentUser = $GLOBALS['dbi']->fetchValue('SELECT USER();'); @@ -388,8 +389,8 @@ class ReplicationGui } // when we start editing a user, $GLOBALS['pred_hostname'] is not defined - if (! isset($GLOBALS['pred_hostname']) && isset($_POST['hostname'])) { - switch (mb_strtolower($_POST['hostname'])) { + if (! isset($GLOBALS['pred_hostname']) && $hostname !== null) { + switch (mb_strtolower($hostname)) { case 'localhost': case '127.0.0.1': $GLOBALS['pred_hostname'] = 'localhost'; @@ -406,9 +407,9 @@ class ReplicationGui return $this->template->render('server/replication/primary_add_replica_user', [ 'username_length' => $usernameLength, 'hostname_length' => $hostnameLength, - 'has_username' => isset($_POST['username']), + 'has_username' => $postUsername !== null, 'username' => $username, - 'hostname' => $_POST['hostname'] ?? '', + 'hostname' => $hostname ?? '', 'predefined_username' => $GLOBALS['pred_username'] ?? '', 'predefined_hostname' => $GLOBALS['pred_hostname'] ?? '', 'this_host' => $thisHost ?? null, @@ -423,7 +424,10 @@ class ReplicationGui bool $replicaChangePrimary, bool $srReplicaServerControl, ?string $srReplicaAction, - bool $srReplicaSkipError + bool $srReplicaSkipError, + int $srSkipErrorsCount, + ?string $srReplicaControlParam, + array $sr ): void { if (! $srTakeAction) { return; @@ -441,9 +445,9 @@ class ReplicationGui . ' $cfg[\'AllowArbitraryServer\'] in phpMyAdmin configuration.' ); } elseif ($replicaChangePrimary) { - $result = $this->handleRequestForReplicaChangePrimary(); + $result = $this->handleRequestForReplicaChangePrimary($sr); } elseif ($srReplicaServerControl) { - $result = $this->handleRequestForReplicaServerControl($srReplicaAction); + $result = $this->handleRequestForReplicaServerControl($srReplicaAction, $srReplicaControlParam); $refresh = true; switch ($srReplicaAction) { @@ -465,7 +469,7 @@ class ReplicationGui break; } } elseif ($srReplicaSkipError) { - $result = $this->handleRequestForReplicaSkipError(); + $result = $this->handleRequestForReplicaSkipError($srSkipErrorsCount); } if ($refresh) { @@ -489,15 +493,8 @@ class ReplicationGui unset($refresh); } - public function handleRequestForReplicaChangePrimary(): bool + public function handleRequestForReplicaChangePrimary(array $sr): bool { - $sr = [ - 'username' => $GLOBALS['dbi']->escapeString($_POST['username']), - 'pma_pw' => $GLOBALS['dbi']->escapeString($_POST['pma_pw']), - 'hostname' => $GLOBALS['dbi']->escapeString($_POST['hostname']), - 'port' => (int) $GLOBALS['dbi']->escapeString($_POST['text_port']), - ]; - $_SESSION['replication']['m_username'] = $sr['username']; $_SESSION['replication']['m_password'] = $sr['pma_pw']; $_SESSION['replication']['m_hostname'] = $sr['hostname']; @@ -559,11 +556,8 @@ class ReplicationGui return $_SESSION['replication']['sr_action_status'] === 'success'; } - public function handleRequestForReplicaServerControl(?string $srReplicaAction): bool + public function handleRequestForReplicaServerControl(?string $srReplicaAction, ?string $control): bool { - /** @var string|null $control */ - $control = $_POST['sr_replica_control_param'] ?? null; - if ($srReplicaAction === 'reset') { $qStop = $this->replication->replicaControl('STOP', null, DatabaseInterface::CONNECT_USER); $qReset = $GLOBALS['dbi']->tryQuery('RESET SLAVE;'); @@ -583,15 +577,10 @@ class ReplicationGui return $result; } - public function handleRequestForReplicaSkipError(): bool + public function handleRequestForReplicaSkipError(int $srSkipErrorsCount): bool { - $count = 1; - if (isset($_POST['sr_skip_errors_count'])) { - $count = $_POST['sr_skip_errors_count'] * 1; - } - $qStop = $this->replication->replicaControl('STOP', null, DatabaseInterface::CONNECT_USER); - $qSkip = $GLOBALS['dbi']->tryQuery('SET GLOBAL SQL_SLAVE_SKIP_COUNTER = ' . $count . ';'); + $qSkip = $GLOBALS['dbi']->tryQuery('SET GLOBAL SQL_SLAVE_SKIP_COUNTER = ' . $srSkipErrorsCount . ';'); $qStart = $this->replication->replicaControl('START', null, DatabaseInterface::CONNECT_USER); return $qStop !== false && $qStop !== -1 && $qSkip !== false && $qStart !== false && $qStart !== -1; diff --git a/libraries/classes/Tracking.php b/libraries/classes/Tracking.php index 33cc713793..34ebf14355 100644 --- a/libraries/classes/Tracking.php +++ b/libraries/classes/Tracking.php @@ -837,12 +837,12 @@ class Tracking * * @param array $entries entries */ - public function exportAsFileDownload(array $entries): void + public function exportAsFileDownload(string $table, array $entries): void { ini_set('url_rewriter.tags', ''); // Replace all multiple whitespaces by a single space - $table = htmlspecialchars(preg_replace('/\s+/', ' ', $_POST['table'])); + $table = htmlspecialchars(preg_replace('/\s+/', ' ', $table)); $dump = '# ' . sprintf( __('Tracking report for table `%s`'), $table diff --git a/libraries/classes/UrlRedirector.php b/libraries/classes/UrlRedirector.php index ff9b729e42..559adb1294 100644 --- a/libraries/classes/UrlRedirector.php +++ b/libraries/classes/UrlRedirector.php @@ -5,9 +5,7 @@ declare(strict_types=1); namespace PhpMyAdmin; use function __; -use function is_scalar; use function preg_match; -use function strlen; /** * URL redirector to avoid leaking Referer with some sensitive information. @@ -17,7 +15,7 @@ final class UrlRedirector /** * @psalm-return never */ - public static function redirect(): void + public static function redirect(string $url): void { // Load database service because services.php is not available here $GLOBALS['dbi'] = DatabaseInterface::load(); @@ -30,9 +28,9 @@ final class UrlRedirector $response->disable(); if ( - ! isset($_GET['url']) || ! is_scalar($_GET['url']) || strlen((string) $_GET['url']) === 0 - || ! preg_match('/^https:\/\/[^\n\r]*$/', (string) $_GET['url']) - || ! Core::isAllowedDomain((string) $_GET['url']) + $url === '' + || ! preg_match('/^https:\/\/[^\n\r]*$/', $url) + || ! Core::isAllowedDomain($url) ) { Core::sendHeaderLocation('./'); @@ -47,7 +45,7 @@ final class UrlRedirector */ $template = $container->get('template'); echo $template->render('javascript/redirect', [ - 'url' => (string) $_GET['url'], + 'url' => Sanitize::escapeJsString($url), ]); // Display redirecting msg on screen. // Do not display the value of $_GET['url'] to avoid showing injected content diff --git a/test/classes/ReplicationGuiTest.php b/test/classes/ReplicationGuiTest.php index a5e4f37b7d..49c084e8cc 100644 --- a/test/classes/ReplicationGuiTest.php +++ b/test/classes/ReplicationGuiTest.php @@ -55,7 +55,7 @@ class ReplicationGuiTest extends AbstractTestCase */ public function testGetHtmlForPrimaryReplication(): void { - $html = $this->replicationGui->getHtmlForPrimaryReplication(null, null, 'primary_add_user'); + $html = $this->replicationGui->getHtmlForPrimaryReplication(null, null, 'primary_add_user', null, null); //validate 1: Primary replication $this->assertStringContainsString('
Primary replication
', $html); @@ -98,7 +98,8 @@ class ReplicationGuiTest extends AbstractTestCase $html = $this->replicationGui->getHtmlForReplicaConfiguration( null, true, - $replicationInfo->getReplicaStatus() + $replicationInfo->getReplicaStatus(), + isset($_POST['replica_configure']) ); //legend