From ba964907b92666d9e8250a4475a930e964ac6857 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Tue, 21 Feb 2023 15:13:46 +1300 Subject: [PATCH] Add new \Civi\Exception\DBQueryException & throw that rather than a PEAR_ERROR Fix api v3 handling of exception to not include the exception (fails on print_r()) m --- CRM/Core/DAO.php | 2 + CRM/Core/Error.php | 7 +- CRM/Core/Exception.php | 18 ++- Civi/API/Kernel.php | 9 +- Civi/Core/Exception/DBQueryException.php | 143 +++++++++++++++++++++++ tests/phpunit/CRM/Core/ErrorTest.php | 80 +++++++++++-- 6 files changed, 247 insertions(+), 12 deletions(-) create mode 100644 Civi/Core/Exception/DBQueryException.php diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php index b8c672a596..a4b27adfb8 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -1644,6 +1644,8 @@ LIKE %1 * object that holds the results of the query * NB - if this is defined as just returning a DAO phpstorm keeps pointing * out all the properties that are not part of the DAO + * + * @throws \Civi\Core\Exception\DBQueryException */ public static function &executeQuery( $query, diff --git a/CRM/Core/Error.php b/CRM/Core/Error.php index 1e0a718401..2b3ad63eaf 100644 --- a/CRM/Core/Error.php +++ b/CRM/Core/Error.php @@ -17,6 +17,8 @@ * @copyright CiviCRM LLC https://civicrm.org/licensing */ +use Civi\Core\Exception\DBQueryException; + require_once 'PEAR/ErrorStack.php'; require_once 'PEAR/Exception.php'; require_once 'CRM/Core/Exception.php'; @@ -946,7 +948,10 @@ class CRM_Core_Error extends PEAR_ErrorStack { public static function exceptionHandler($pearError) { CRM_Core_Error::debug_var('Fatal Error Details', self::getErrorDetails($pearError), TRUE, TRUE, '', PEAR_LOG_ERR); CRM_Core_Error::backtrace('backTrace', TRUE); - throw new PEAR_Exception($pearError->getMessage(), $pearError); + if ($pearError instanceof DB_Error) { + throw new DBQueryException($pearError->getMessage(), $pearError->getCode(), ['exception' => $pearError]); + } + throw new CRM_Core_Exception($pearError->getMessage(), $pearError->getCode(), ['exception' => $pearError]); } /** diff --git a/CRM/Core/Exception.php b/CRM/Core/Exception.php index 5591885acc..aa9faa26e9 100644 --- a/CRM/Core/Exception.php +++ b/CRM/Core/Exception.php @@ -39,8 +39,13 @@ class CRM_Core_Exception extends PEAR_Exception { * A previous exception which caused this new exception. */ public function __construct($message, $error_code = 0, $errorData = [], $previous = NULL) { - // Using int for error code "old way") ? - if (is_numeric($error_code)) { + + if (($errorData['exception'] ?? NULL) instanceof DB_Error) { + // Pass the exception to the PEAR_Exception parent as the code for it to handle. + $code = $errorData['exception']; + } + elseif (is_numeric($error_code)) { + // Using int for error code "old way") ? $code = $error_code; } else { @@ -99,6 +104,15 @@ class CRM_Core_Exception extends PEAR_Exception { return $this->errorData; } + /** + * Get a message suitable to be presented to the user. + * + * @return string + */ + public function getUserMessage(): string { + return $this->getMessage(); + } + /** * Get error codes. * diff --git a/Civi/API/Kernel.php b/Civi/API/Kernel.php index ed09dce093..8e67c3142c 100644 --- a/Civi/API/Kernel.php +++ b/Civi/API/Kernel.php @@ -336,6 +336,13 @@ class Kernel { */ public function formatApiException($e, $apiRequest) { $data = $e->getExtraParams(); + $errorCode = $e->getCode(); + if (($data['exception'] ?? NULL) instanceof \DB_Error) { + $errorCode = $e->getDBErrorMessage(); + $data['sql'] = $e->getSQL(); + $data['debug_info'] = $e->getUserInfo(); + } + unset($data['exception']); $data['entity'] = $apiRequest['entity'] ?? NULL; $data['action'] = $apiRequest['action'] ?? NULL; @@ -346,7 +353,7 @@ class Kernel { $data['trace'] = $e->getTraceAsString(); } - return $this->createError($e->getMessage(), $data, $apiRequest, $e->getCode()); + return $this->createError($e->getMessage(), $data, $apiRequest, $errorCode); } /** diff --git a/Civi/Core/Exception/DBQueryException.php b/Civi/Core/Exception/DBQueryException.php new file mode 100644 index 0000000000..d06a10760e --- /dev/null +++ b/Civi/Core/Exception/DBQueryException.php @@ -0,0 +1,143 @@ +getDBErrorMessage() . $this->getErrorCodeSpecificMessage(); + } + + /** + * Is the error message safe to show to users. + * + * Probably most of them are but error 1146 leaks the database name - eg. + * 'table dmaster.civicrm_contact does not exist'. + * + * However, for missing fields and syntax errors the error message gives + * useful clues we should pass on. We can add to this / tweak over time - if + * we care to. + * + * @return bool + */ + private function isErrorMessageUserSafe(): bool { + $errors = [ + // No such field, does not leak any table information. + 1054 => TRUE, + // Invalid schema - helpful hint as to where the error is, no leakage. + 1064 => TRUE, + // No such table - leaks db name. + 1146 => FALSE, + ]; + return $errors[$this->getSQLErrorCode()] ?? FALSE; + } + + /** + * @return int + */ + protected function getPEARErrorCode(): int { + return $this->getDBError()->getCode(); + } + + /** + * @return \DB_Error + */ + protected function getDBError(): \DB_Error { + return $this->getErrorData()['exception']; + } + + /** + * Get the mysql error code. + * + * @see https://mariadb.com/kb/en/mariadb-error-codes/ + * + * @return int + */ + public function getSQLErrorCode(): int { + $dbErrorMessage = $this->getUserInfo(); + $matches = []; + preg_match('/\[nativecode=(\d*) /', $dbErrorMessage, $matches); + return (int) $matches[1]; + } + + /** + * Get the PEAR data intended to be use useful to the user. + * + * @return string + */ + public function getUserInfo(): string { + return $this->getCause()->getUserInfo(); + } + + /** + * Get the attempted sql. + * + * @return string + */ + public function getSQL(): string { + $dbErrorMessage = $this->getUserInfo(); + $matches = []; + preg_match('/(.*) \[nativecode=/', $dbErrorMessage, $matches); + return $matches[1]; + } + + /** + * Get the attempted sql. + * + * @return string + */ + public function getDebugInfo(): string { + return $this->getDBError()->getUserInfo(); + } + + /** + * Get additional user-safe error message information. + * + * @return string + */ + private function getErrorCodeSpecificMessage(): string { + $matches = []; + preg_match('/\[nativecode=\d* \*\* (.*)]/', $this->getUserInfo(), $matches); + if ($this->isErrorMessageUserSafe()) { + return ' ' . $matches[1]; + } + // We could return additional info e.g when we log deadlocks we log + // 'Database deadlock encountered' (1213) or 'Database lock encountered' (1205). + return ''; + } + + /** + * Get the DB error code converted to a test code. + * + * @return string + */ + public function getDBErrorMessage(): string { + return \DB::errorMessage($this->getPEARErrorCode()); + } + +} diff --git a/tests/phpunit/CRM/Core/ErrorTest.php b/tests/phpunit/CRM/Core/ErrorTest.php index 9daaa2fd35..9991300e81 100644 --- a/tests/phpunit/CRM/Core/ErrorTest.php +++ b/tests/phpunit/CRM/Core/ErrorTest.php @@ -9,6 +9,8 @@ +--------------------------------------------------------------------+ */ +use Civi\Core\Exception\DBQueryException; + /** * Tests for linking to resource files * @group headless @@ -63,24 +65,24 @@ class CRM_Core_ErrorTest extends CiviUnitTestCase { * * This tests a theory about what caused CRM-10766. */ - public function testMixLog() { - CRM_Core_Error::debug_log_message("static-1"); + public function testMixLog(): void { + CRM_Core_Error::debug_log_message('static-1'); $logger = CRM_Core_Error::createDebugLogger(); - CRM_Core_Error::debug_log_message("static-2"); + CRM_Core_Error::debug_log_message('static-2'); $logger->info('obj-1'); - CRM_Core_Error::debug_log_message("static-3"); + CRM_Core_Error::debug_log_message('static-3'); $logger->info('obj-2'); - CRM_Core_Error::debug_log_message("static-4"); + CRM_Core_Error::debug_log_message('static-4'); $logger2 = CRM_Core_Error::createDebugLogger(); $logger2->info('obj-3'); - CRM_Core_Error::debug_log_message("static-5"); + CRM_Core_Error::debug_log_message('static-5'); $this->assertLogRegexp('/static-1.*static-2.*obj-1.*static-3.*obj-2.*static-4.*obj-3.*static-5/s'); } /** * @param $pattern */ - public function assertLogRegexp($pattern) { + public function assertLogRegexp($pattern): void { $config = CRM_Core_Config::singleton(); $logFiles = glob($config->configAndLogDir . '/CiviCRM*.log'); $this->assertEquals(1, count($logFiles), 'Expect to find 1 file matching: ' . $config->configAndLogDir . '/CiviCRM*log*/'); @@ -94,7 +96,7 @@ class CRM_Core_ErrorTest extends CiviUnitTestCase { * * Do some basic content checks. */ - public function testDebugLoggerFormat() { + public function testDebugLoggerFormat(): void { $log = CRM_Core_Error::createDebugLogger('my-test'); $log->log('Mary had a little lamb'); $log->log('Little lamb'); @@ -107,4 +109,66 @@ class CRM_Core_ErrorTest extends CiviUnitTestCase { $this->assertStringContainsString('[info] Little lamb', $fileContents); } + /** + * Test the contents of the exception thrown for invalid sql. + * + * @dataProvider getErrorSQL + * + * @param array $testData + */ + public function testDBError(array $testData): void { + try { + CRM_Core_DAO::executeQuery($testData['sql']); + } + catch (DBQueryException $e) { + $this->assertEquals(0, $e->getCode()); + $this->assertInstanceOf('DB_Error', $e->getCause()); + $this->assertEquals($testData['message'], $e->getMessage()); + $this->assertEquals($testData['error_code'], $e->getErrorCode()); + $this->assertEquals($testData['sql_error_code'], $e->getSQLErrorCode()); + $this->assertStringStartsWith($testData['sql'] . ' [nativecode=' . $testData['sql_error_code'], $e->getDebugInfo()); + $this->assertEquals($testData['sql'], $e->getSQL()); + $this->assertStringStartsWith($testData['user_message'], $e->getUserMessage()); + return; + } + $this->fail(); + } + + /** + * Data provider for sql error testing. + * + * @return array[] + */ + public function getErrorSQL(): array { + return [ + 'invalid_table' => [ + [ + 'sql' => 'SELECT a FROM b', + 'message' => 'DB Error: no such table', + 'error_code' => -18, + 'user_message' => 'Invalid Query no such table', + 'sql_error_code' => 1146, + ], + ], + 'invalid_field' => [ + [ + 'sql' => 'SELECT a FROM civicrm_contact', + 'message' => 'DB Error: no such field', + 'error_code' => -19, + 'user_message' => "Invalid Query no such field Unknown column 'a' in 'field list'", + 'sql_error_code' => 1054, + ], + ], + 'invalid_syntax' => [ + [ + 'sql' => 'FROM civicrm_contact', + 'message' => 'DB Error: syntax error', + 'error_code' => -2, + 'user_message' => 'Invalid Query syntax error You have an error in your SQL syntax; check the manual that corresponds to your', + 'sql_error_code' => 1064, + ], + ], + ]; + } + } -- 2.25.1