Page MenuHomePhabricator

$wgShowSQLErrors doesn't seem to be working anymore
Closed, ResolvedPublic

Description

I have

$wgShowExceptionDetails = true;
$wgShowSQLErrors = true;

But yet all I'm getting is:

[25bc6d1aaf86cfe5163266da] 2017-05-19 14:26:14: Fatal exception of type "Wikimedia\Rdbms\DBQueryError"

(I'm using sqlite, but i doubt that matters.)

Event Timeline

IIRC, you need to set $wgShowDBErrorBacktrace = true;

Yay it works, but it seems silly that we now have 2 separate globals for showing sql errors (The $wgShowSQLErrors still seems to be needed for the api)

tstarling added subscribers: BPirkle, tstarling.

Prior to 00bee029718f3215396e984d04b9450bc3872503 , the exception classes did their own formatting, and DBQueryError used this to provide a lot of information when $wgShowSQLErrors was true. As part of removing "lots of pointless prettification" (as the commit message puts it), query errors are now shown like ordinary exceptions, and $wgShowDBErrorBacktrace is interpreted to mean the same thing for DBError instances as $wgShowExceptionDetails means for other exceptions. $wgShowExceptionDetails is more paranoid, in particular it suppresses display of Exception::getMessage() which contains the error text from MySQL. So interpreting $wgShowDBErrorBacktrace like $wgShowExceptionDetails means that the MySQL error text is no longer displayed unless $wgShowDBErrorBacktrace is true.

I think the best option here is to deprecate $wgShowDBErrorBacktrace and $wgShowSQLErrors and to always show exception details if $wgShowExceptionDetails is true. So until removal, the condition for showing details of DBError instances would be if ( $wgShowExceptionDetails || $wgShowDBErrorBacktrace )

(The $wgShowSQLErrors still seems to be needed for the api)

ApiMain::errorMessagesFromException() is currently showing exception messages regardless of $wgShowExceptionDetails, except for DB errors which hide the message if $wgShowSQLErrors is false. Showing messages when $wgShowExceptionDetails is false seems to undermine the security objective. It should probably be brought into line with MWExceptionRenderer.

@BPirkle, could you give this a go?

One subtlety is that ApiMain::errorMessagesFromException() is currently sending a key of 'apierror-databaseerror' only for DBError exceptions when $wgShowSQLErrors is false. For DBError exceptions when $wgShowSQLErrors is true, it is sending a key of 'apierror-exceptioncaught'. In other words, it either sends a key indicating the exception was database-related OR it sends the details, but never both.

Is that intentional? For consistency, I'm tempted to send 'apierror-databaseerror' for all DBError exceptions, whether we are including details or not.

One subtlety is that ApiMain::errorMessagesFromException() is currently sending a key of 'apierror-databaseerror' only for DBError exceptions when $wgShowSQLErrors is false. For DBError exceptions when $wgShowSQLErrors is true, it is sending a key of 'apierror-exceptioncaught'. In other words, it either sends a key indicating the exception was database-related OR it sends the details, but never both.

Is that intentional? For consistency, I'm tempted to send 'apierror-databaseerror' for all DBError exceptions, whether we are including details or not.

The key you're referring to is the i18n message, apierror-databaseerror versus apierror-exceptioncaught. The actual error code is set separately there, unless something is broken.

Confirmed that the actual error code is set separately. The two paths currently yield either:

$params = [ 'apierror-exceptioncaught', WebRequest::getRequestId(), $msg ]
(DBError exceptions with $wgShowSQLErrors == true. Also used for or a variety of other exception types. Includes a message string based on the exception)

or

$params = [ 'apierror-databaseerror', WebRequest::getRequestId() ];
(DBError exceptions with $wgShowSQLErrors == false, does not include a message string)

Either way, $params is then used in ApiMessage::create (along with the actual error code), as follows:

$messages[] = ApiMessage::create( $params, $code );

Maybe the value I'm calling "key" (could be wrong nomenclature) is not all that relevant? It seemed the existing behavior might have been an unintentional. So I'm just looking for a quick cleanup, if appropriate, while I'm touching this anyway.

Key is correct nomenclature.

Most of the time API error codes actually are derived from the i18n message key, because most of the time there's a 1:1 mapping. Just in this case they aren't.

ApiMain::errorMessagesFromException() is currently showing exception messages regardless of $wgShowExceptionDetails, except for DB errors which hide the message if $wgShowSQLErrors is false. Showing messages when $wgShowExceptionDetails is false seems to undermine the security objective. It should probably be brought into line with MWExceptionRenderer.

Hmm. The inline documentation for $wgShowExceptionDetails only mentions the stack trace, not the message. It had more or less the same message back in 2006 when it was added (6c86df8f2), even though the behavior then was to hide absolutely everything, and the release note and commit summary match.

I guess when support for $wgShowExceptionDetails was added to the API in 2008 (e539d061) people went by the documentation rather than the actual behavior.

Change 446370 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Deprecate $wgShowSQLErrors and $wgShowDBErrorBacktrace in favor of $wgShowExceptionDetails

https://gerrit.wikimedia.org/r/446370

Change 446370 merged by jenkins-bot:
[mediawiki/core@master] Deprecate $wgShowSQLErrors and $wgShowDBErrorBacktrace and make nonfunctional

https://gerrit.wikimedia.org/r/446370

Change 449732 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Remove deprecated variable usage from Maintenance.php

https://gerrit.wikimedia.org/r/449732

Change 449732 merged by jenkins-bot:
[mediawiki/core@master] Remove deprecated variable usage from Maintenance.php

https://gerrit.wikimedia.org/r/449732