From 807125abdb99729cb6c694ab59aef17e334bc164 Mon Sep 17 00:00:00 2001 From: Bill Pirkle Date: Tue, 17 Jul 2018 11:51:36 -0500 Subject: [PATCH] Deprecate $wgShowSQLErrors and $wgShowDBErrorBacktrace and make nonfunctional Clarify and simplify exception output by deprecating $wgShowSQLErrors and wgShowDBErrorBacktrace. $wgShowExceptionDetails will now control most related output. $wgShowHostnames will now solely control output of MWExceptionRenderer::reportOutageHTML. Bug: T165768 Change-Id: Idead2c11c499463dfa6293c3d4b33be3bde92e1a --- RELEASE-NOTES-1.32 | 4 ++ includes/DefaultSettings.php | 13 ++++-- includes/DevelopmentSettings.php | 8 ++-- includes/api/ApiMain.php | 13 +++--- includes/api/i18n/en.json | 2 +- includes/api/i18n/qqq.json | 2 +- includes/exception/MWExceptionRenderer.php | 46 +++++++--------------- includes/installer/Installer.php | 5 +-- tests/phpunit/includes/ExtraParserTest.php | 2 +- tests/phpunit/includes/api/ApiMainTest.php | 6 +-- 10 files changed, 42 insertions(+), 59 deletions(-) diff --git a/RELEASE-NOTES-1.32 b/RELEASE-NOTES-1.32 index 7ac5479615..486645a867 100644 --- a/RELEASE-NOTES-1.32 +++ b/RELEASE-NOTES-1.32 @@ -279,6 +279,10 @@ because of Phabricator reports. 'log-show-hide-[type]' format. Instead use 'logeventslist-[type]-log'. * Global functions wfArrayFilter() and wfArrayFilterByKey() are deprecated. use array_filter() directly. +* The $wgShowSQLErrors global is deprecated and nonfunctional. + Set $wgShowExceptionDetails and/or $wgShowHostnames instead. +* The $wgShowDBErrorBacktrace global is deprecated and nonfunctional. + Set $wgShowExceptionDetails instead. === Other changes in 1.32 === * (T198811) The following tables have had their UNIQUE indexes turned into diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index 84131f0d53..13d5368fac 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -6296,14 +6296,17 @@ $wgSpecialVersionShowHooks = false; * Whether to show "we're sorry, but there has been a database error" pages. * Displaying errors aids in debugging, but may display information useful * to an attacker. + * + * @deprecated and nonfunctional since 1.32: set $wgShowExceptionDetails and/or + * $wgShowHostnames instead. */ $wgShowSQLErrors = false; /** - * If set to true, uncaught exceptions will print a complete stack trace - * to output. This should only be used for debugging, as it may reveal - * private information in function parameters due to PHP's backtrace - * formatting. + * If set to true, uncaught exceptions will print the exception message and a + * complete stack trace to output. This should only be used for debugging, as it + * may reveal private information in function parameters due to PHP's backtrace + * formatting. If set to false, only the exception's class will be shown. */ $wgShowExceptionDetails = false; @@ -6314,6 +6317,8 @@ $wgShowExceptionDetails = false; * reported in the normal manner. $wgShowExceptionDetails applies in other cases, * including those in which an uncaught exception is thrown from within the * exception handler. + * + * @deprecated and nonfunctional since 1.32: set $wgShowExceptionDetails instead. */ $wgShowDBErrorBacktrace = false; diff --git a/includes/DevelopmentSettings.php b/includes/DevelopmentSettings.php index 96ed56b94d..8b32de4f64 100644 --- a/includes/DevelopmentSettings.php +++ b/includes/DevelopmentSettings.php @@ -24,18 +24,16 @@ ini_set( 'display_errors', 1 ); /** * Debugging: MediaWiki */ -global $wgDevelopmentWarnings, $wgShowDBErrorBacktrace, $wgShowExceptionDetails, - $wgShowSQLErrors, $wgDebugRawPage, - $wgDebugComments, $wgDebugDumpSql, $wgDebugTimestamps, +global $wgDevelopmentWarnings, $wgShowExceptionDetails, $wgShowHostnames, + $wgDebugRawPage, $wgDebugComments, $wgDebugDumpSql, $wgDebugTimestamps, $wgCommandLineMode, $wgDebugLogFile, $wgDBerrorLog, $wgDebugLogGroups; // Use of wfWarn() should cause tests to fail $wgDevelopmentWarnings = true; // Enable showing of errors -$wgShowDBErrorBacktrace = true; $wgShowExceptionDetails = true; -$wgShowSQLErrors = true; +$wgShowHostnames = true; $wgDebugRawPage = true; // T49960 // Enable log files diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php index 84708e7147..e5e384c0f1 100644 --- a/includes/api/ApiMain.php +++ b/includes/api/ApiMain.php @@ -24,8 +24,6 @@ use MediaWiki\Logger\LoggerFactory; use MediaWiki\MediaWikiServices; use Wikimedia\Timestamp\TimestampException; -use Wikimedia\Rdbms\DBQueryError; -use Wikimedia\Rdbms\DBError; /** * This is the main API class, used for both external and internal processing. @@ -1038,9 +1036,7 @@ class ApiMain extends ApiBase { $config = $this->getConfig(); $class = preg_replace( '#^Wikimedia\\\Rdbms\\\#', '', get_class( $e ) ); $code = 'internal_api_error_' . $class; - if ( ( $e instanceof DBQueryError ) && !$config->get( 'ShowSQLErrors' ) ) { - $params = [ 'apierror-databaseerror', WebRequest::getRequestId() ]; - } else { + if ( $config->get( 'ShowExceptionDetails' ) ) { if ( $e instanceof ILocalizedException ) { $msg = $e->getMessageObject(); } elseif ( $e instanceof MessageSpecifier ) { @@ -1049,7 +1045,10 @@ class ApiMain extends ApiBase { $msg = wfEscapeWikiText( $e->getMessage() ); } $params = [ 'apierror-exceptioncaught', WebRequest::getRequestId(), $msg ]; + } else { + $params = [ 'apierror-exceptioncaughttype', WebRequest::getRequestId(), get_class( $e ) ]; } + $messages[] = ApiMessage::create( $params, $code ); } return $messages; @@ -1113,9 +1112,7 @@ class ApiMain extends ApiBase { ) ); } else { - if ( $config->get( 'ShowExceptionDetails' ) && - ( !$e instanceof DBError || $config->get( 'ShowDBErrorBacktrace' ) ) - ) { + if ( $config->get( 'ShowExceptionDetails' ) ) { $result->addContentValue( $path, 'trace', diff --git a/includes/api/i18n/en.json b/includes/api/i18n/en.json index 74efd820ca..d7cdc6cfa7 100644 --- a/includes/api/i18n/en.json +++ b/includes/api/i18n/en.json @@ -1712,12 +1712,12 @@ "apierror-copyuploadbadurl": "Upload not allowed from this URL.", "apierror-create-titleexists": "Existing titles can't be protected with create.", "apierror-csp-report": "Error processing CSP report: $1.", - "apierror-databaseerror": "[$1] Database query error.", "apierror-deletedrevs-param-not-1-2": "The $1 parameter cannot be used in modes 1 or 2.", "apierror-deletedrevs-param-not-3": "The $1 parameter cannot be used in mode 3.", "apierror-emptynewsection": "Creating empty new sections is not possible.", "apierror-emptypage": "Creating new, empty pages is not allowed.", "apierror-exceptioncaught": "[$1] Exception caught: $2", + "apierror-exceptioncaughttype": "[$1] Caught exception of type $2", "apierror-filedoesnotexist": "File does not exist.", "apierror-fileexists-sharedrepo-perm": "The target file exists on a shared repository. Use the ignorewarnings parameter to override it.", "apierror-filenopath": "Cannot get local file path.", diff --git a/includes/api/i18n/qqq.json b/includes/api/i18n/qqq.json index 2b4a587a0b..dd8e529db1 100644 --- a/includes/api/i18n/qqq.json +++ b/includes/api/i18n/qqq.json @@ -1600,12 +1600,12 @@ "apierror-copyuploadbadurl": "{{doc-apierror}}", "apierror-create-titleexists": "{{doc-apierror}}", "apierror-csp-report": "{{doc-apierror}}\n\nParameters:\n* $1 - Error code, e.g. \"toobig\".", - "apierror-databaseerror": "{{doc-apierror}}\n\nParameters:\n* $1 - Exception log ID code. This is meaningless to the end user, but can be used by people with access to the logs to easily find the logged error.", "apierror-deletedrevs-param-not-1-2": "{{doc-apierror}}\n\nParameters:\n* $1 - Parameter name.\n\nSee also:\n* {{msg-mw|apihelp-query+deletedrevs-extended-description}}", "apierror-deletedrevs-param-not-3": "{{doc-apierror}}\n\nParameters:\n* $1 - Parameter name.\n\nSee also:\n* {{msg-mw|apihelp-query+deletedrevs-extended-description}}", "apierror-emptynewsection": "{{doc-apierror}}", "apierror-emptypage": "{{doc-apierror}}", "apierror-exceptioncaught": "{{doc-apierror}}\n\nParameters:\n* $1 - Exception log ID code. This is meaningless to the end user, but can be used by people with access to the logs to easily find the logged error.\n* $2 - Exception message, which may end with punctuation. Probably in English.", + "apierror-exceptioncaughttype": "{{doc-apierror}}\n\nParameters:\n* $1 - Exception log ID code. This is meaningless to the end user, but can be used by people with access to the logs to easily find the logged error.\n* $2 - Exception type (for example, DBError).", "apierror-filedoesnotexist": "{{doc-apierror}}", "apierror-fileexists-sharedrepo-perm": "{{doc-apierror}}", "apierror-filenopath": "{{doc-apierror}}", diff --git a/includes/exception/MWExceptionRenderer.php b/includes/exception/MWExceptionRenderer.php index 88b28df385..49cf71e624 100644 --- a/includes/exception/MWExceptionRenderer.php +++ b/includes/exception/MWExceptionRenderer.php @@ -19,7 +19,6 @@ */ use Wikimedia\Rdbms\DBConnectionError; -use Wikimedia\Rdbms\DBError; use Wikimedia\Rdbms\DBReadOnlyError; use Wikimedia\Rdbms\DBExpectedError; @@ -37,7 +36,7 @@ class MWExceptionRenderer { * @param Exception|Throwable|null $eNew New exception from attempting to show the first */ public static function output( $e, $mode, $eNew = null ) { - global $wgMimeType; + global $wgMimeType, $wgShowExceptionDetails; if ( defined( 'MW_API' ) ) { // Unhandled API exception, we can't be sure that format printer is alive @@ -58,7 +57,7 @@ class MWExceptionRenderer { self::header( "Content-Type: $wgMimeType; charset=utf-8" ); if ( $eNew ) { $message = "MediaWiki internal error.\n\n"; - if ( self::showBackTrace( $e ) ) { + if ( $wgShowExceptionDetails ) { $message .= 'Original exception: ' . MWExceptionHandler::getLogMessage( $e ) . "\nBacktrace:\n" . MWExceptionHandler::getRedactedTraceAsString( $e ) . @@ -73,7 +72,7 @@ class MWExceptionRenderer { } $message .= "\n"; } else { - if ( self::showBackTrace( $e ) ) { + if ( $wgShowExceptionDetails ) { $message = MWExceptionHandler::getLogMessage( $e ) . "\nBacktrace:\n" . MWExceptionHandler::getRedactedTraceAsString( $e ) . "\n"; @@ -162,7 +161,9 @@ class MWExceptionRenderer { * @return string Html to output */ public static function getHTML( $e ) { - if ( self::showBackTrace( $e ) ) { + global $wgShowExceptionDetails; + + if ( $wgShowExceptionDetails ) { $html = "

" . nl2br( htmlspecialchars( MWExceptionHandler::getLogMessage( $e ) ) ) . '

Backtrace:

' . @@ -209,7 +210,9 @@ class MWExceptionRenderer { * @return string */ private static function getText( $e ) { - if ( self::showBackTrace( $e ) ) { + global $wgShowExceptionDetails; + + if ( $wgShowExceptionDetails ) { return MWExceptionHandler::getLogMessage( $e ) . "\nBacktrace:\n" . MWExceptionHandler::getRedactedTraceAsString( $e ) . "\n"; @@ -218,34 +221,13 @@ class MWExceptionRenderer { } } - /** - * @param Exception|Throwable $e - * @return bool - */ - private static function showBackTrace( $e ) { - global $wgShowExceptionDetails, $wgShowDBErrorBacktrace; - - return ( - $wgShowExceptionDetails && - ( !( $e instanceof DBError ) || $wgShowDBErrorBacktrace ) - ); - } - /** * @param Exception|Throwable $e * @return string */ private static function getShowBacktraceError( $e ) { - global $wgShowExceptionDetails, $wgShowDBErrorBacktrace; - $vars = []; - if ( !$wgShowExceptionDetails ) { - $vars[] = '$wgShowExceptionDetails = true;'; - } - if ( $e instanceof DBError && !$wgShowDBErrorBacktrace ) { - $vars[] = '$wgShowDBErrorBacktrace = true;'; - } - $vars = implode( ' and ', $vars ); - return "Set $vars at the bottom of LocalSettings.php to show detailed debugging information."; + $var = '$wgShowExceptionDetails = true;'; + return "Set $var at the bottom of LocalSettings.php to show detailed debugging information."; } /** @@ -294,7 +276,7 @@ class MWExceptionRenderer { * @param Exception|Throwable $e */ private static function reportOutageHTML( $e ) { - global $wgShowDBErrorBacktrace, $wgShowHostnames, $wgShowSQLErrors, $wgSitename; + global $wgShowExceptionDetails, $wgShowHostnames, $wgSitename; $sorry = htmlspecialchars( self::msg( 'dberr-problems', @@ -305,7 +287,7 @@ class MWExceptionRenderer { 'Try waiting a few minutes and reloading.' ) ); - if ( $wgShowHostnames || $wgShowSQLErrors ) { + if ( $wgShowHostnames ) { $info = str_replace( '$1', Html::element( 'span', [ 'dir' => 'ltr' ], $e->getMessage() ), @@ -327,7 +309,7 @@ class MWExceptionRenderer { '' . "

$sorry

$again

$info

"; - if ( $wgShowDBErrorBacktrace ) { + if ( $wgShowExceptionDetails ) { $html .= '

Backtrace:

' .
 				htmlspecialchars( $e->getTraceAsString() ) . '
'; } diff --git a/includes/installer/Installer.php b/includes/installer/Installer.php index 3905ba02ee..e7ee95c78a 100644 --- a/includes/installer/Installer.php +++ b/includes/installer/Installer.php @@ -1725,13 +1725,10 @@ abstract class Installer { $GLOBALS['wgLanguageConverterCacheType'] = CACHE_NONE; // Debug-friendly $GLOBALS['wgShowExceptionDetails'] = true; + $GLOBALS['wgShowHostnames'] = true; // Don't break forms $GLOBALS['wgExternalLinkTarget'] = '_blank'; - // Extended debugging - $GLOBALS['wgShowSQLErrors'] = true; - $GLOBALS['wgShowDBErrorBacktrace'] = true; - // Allow multiple ob_flush() calls $GLOBALS['wgDisableOutputCompression'] = true; diff --git a/tests/phpunit/includes/ExtraParserTest.php b/tests/phpunit/includes/ExtraParserTest.php index 75ebd31a21..164c83c7e4 100644 --- a/tests/phpunit/includes/ExtraParserTest.php +++ b/tests/phpunit/includes/ExtraParserTest.php @@ -17,7 +17,7 @@ class ExtraParserTest extends MediaWikiTestCase { $contLang = Language::factory( 'en' ); $this->setMwGlobals( [ - 'wgShowDBErrorBacktrace' => true, + 'wgShowExceptionDetails' => true, 'wgCleanSignatures' => true, ] ); $this->setUserLang( 'en' ); diff --git a/tests/phpunit/includes/api/ApiMainTest.php b/tests/phpunit/includes/api/ApiMainTest.php index b7869e689d..20d758e3aa 100644 --- a/tests/phpunit/includes/api/ApiMainTest.php +++ b/tests/phpunit/includes/api/ApiMainTest.php @@ -967,8 +967,7 @@ class ApiMainTest extends ApiTestCase { $context->setLanguage( 'en' ); $context->setConfig( new MultiConfig( [ new HashConfig( [ - 'ShowHostnames' => true, 'ShowSQLErrors' => false, - 'ShowExceptionDetails' => true, 'ShowDBErrorBacktrace' => true, + 'ShowHostnames' => true, 'ShowExceptionDetails' => true, ] ), $context->getConfig() ] ) ); @@ -1052,7 +1051,8 @@ class ApiMainTest extends ApiTestCase { [ 'code' => 'existing-error', 'text' => 'existing error', 'module' => 'main' ], [ 'code' => 'internal_api_error_DBQueryError', - 'text' => "[$reqId] Database query error.", + 'text' => "[$reqId] Exception caught: A database query error has occurred. " . + "This may indicate a bug in the software.", ] ], 'trace' => $dbtrace, -- 2.20.1