From: Antoine Musso Date: Mon, 28 Oct 2013 16:56:37 +0000 (+0100) Subject: redact exception traces and abstract getTrace X-Git-Tag: 1.31.0-rc.0~18319^2 X-Git-Url: https://git.cyclocoop.org/%7B%24www_url%7Dadmin/compta/exercices/bilan.php?a=commitdiff_plain;h=0ee7ea85d0a4569e2129cc565e6be82255c7aeb5;p=lhc%2Fweb%2Fwiklou.git redact exception traces and abstract getTrace * Partially reverts I0a9e92448 (rationale: http://www.gossamer-threads.com/lists/wiki/wikitech/401558) - wfDebugLog()'d exceptions are always unredacted - Other backtraces are redacted by replacing all argument values with class / type names. * Adds a pair of static methods to MWExceptionHandler: - MWExceptionHandler::getRedactedTrace equivalent to Exception::getTrace, but replaces each argument value in the trace with its class or type name. - MWExceptionHandler::getRedactedTraceAsString equivalent to Exception::getTraceAsString, but with argument values likewise redacted. * The rename of 'formatRedactedTrace' to 'getRedactedTraceAsString' is justified on two grounds: - 'formatRedactedTrace' didn't actually take a trace object (it took an exception). - 'getRedactedTraceAsString' maintains the symmetry with Exception::getTraceAsString. Change-Id: I3d570a6385f96a606e1af53c50faa03b9ebacd38 --- diff --git a/RELEASE-NOTES-1.22 b/RELEASE-NOTES-1.22 index 769f661833..827e003cd8 100644 --- a/RELEASE-NOTES-1.22 +++ b/RELEASE-NOTES-1.22 @@ -227,8 +227,6 @@ production. * Add deferrable update support for callback/closure. * Add TitleMove hook before page renames. * Revision deletion backend code is moved out of SpecialRevisiondelete -* Add a variable (wgRedactedFunctionArguments) to redact the values sent as certain function - parameters from exception stack traces. * Added {{REVISIONSIZE}} variable to get the current size of a revision. * Add support for the LESS stylesheet language to ResourceLoader. LESS is a stylesheet language that compiles into CSS. ResourceLoader file modules may diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php index d3c7b5f7b4..ebae1101d5 100644 --- a/includes/DefaultSettings.php +++ b/includes/DefaultSettings.php @@ -4940,37 +4940,6 @@ $wgShowSQLErrors = false; */ $wgShowExceptionDetails = false; -/** - * Array of functions which need parameters redacted from stack traces shown to - * clients and logged. Keys are in the format '[class::]function', and the - * values should be either an integer or an array of integers. These are the - * indexes of the parameters which need to be kept secret. - * @since 1.22 - */ -$wgRedactedFunctionArguments = array( - 'AuthPlugin::setPassword' => 1, - 'AuthPlugin::authenticate' => 1, - 'AuthPlugin::addUser' => 1, - - 'DatabaseBase::__construct' => 2, - 'DatabaseBase::open' => 2, - - 'SpecialChangeEmail::attemptChange' => 1, - 'SpecialChangePassword::attemptReset' => 0, - - 'User::setPassword' => 0, - 'User::setInternalPassword' => 0, - 'User::checkPassword' => 0, - 'User::setNewpassword' => 0, - 'User::comparePasswords' => array( 0, 1 ), - 'User::checkTemporaryPassword' => 0, - 'User::setToken' => 0, - 'User::crypt' => 0, - 'User::oldCrypt' => 0, - 'User::getPasswordValidity' => 0, - 'User::isValidPassword' => 0, -); - /** * If true, show a backtrace for database errors */ diff --git a/includes/Exception.php b/includes/Exception.php index aeafda8805..9623a8444a 100644 --- a/includes/Exception.php +++ b/includes/Exception.php @@ -141,8 +141,7 @@ class MWException extends Exception { if ( $wgShowExceptionDetails ) { return '

' . nl2br( htmlspecialchars( $this->getMessage() ) ) . - '

Backtrace:

' . - nl2br( htmlspecialchars( MWExceptionHandler::formatRedactedTrace( $this ) ) ) . + '

Backtrace:

' . nl2br( htmlspecialchars( MWExceptionHandler::getRedactedTraceAsString( $this ) ) ) . "

\n"; } else { return "
" . @@ -167,7 +166,7 @@ class MWException extends Exception { if ( $wgShowExceptionDetails ) { return $this->getMessage() . - "\nBacktrace:\n" . MWExceptionHandler::formatRedactedTrace( $this ) . "\n"; + "\nBacktrace:\n" . MWExceptionHandler::getRedactedTraceAsString( $this ) . "\n"; } else { return "Set \$wgShowExceptionDetails = true; " . "in LocalSettings.php to show detailed debugging information.\n"; @@ -619,8 +618,10 @@ class MWExceptionHandler { $message = "MediaWiki internal error.\n\n"; if ( $wgShowExceptionDetails ) { - $message .= 'Original exception: ' . self::formatRedactedTrace( $e ) . "\n\n" . - 'Exception caught inside exception handler: ' . $e2->__toString(); + $message .= 'Original exception: ' . self::getLogMessage( $e ) . + "\nBacktrace:\n" . self::getRedactedTraceAsString( $e ) . + "\n\nException caught inside exception handler: " . self::getLogMessage( $e2 ) . + "\nBacktrace:\n" . self::getRedactedTraceAsString( $e2 ); } else { $message .= "Exception caught inside exception handler.\n\n" . "Set \$wgShowExceptionDetails = true; at the bottom of LocalSettings.php " . @@ -642,7 +643,7 @@ class MWExceptionHandler { if ( $wgShowExceptionDetails ) { $message .= "\nexception '" . get_class( $e ) . "' in " . $e->getFile() . ":" . $e->getLine() . "\nStack trace:\n" . - self::formatRedactedTrace( $e ) . "\n"; + self::getRedactedTraceAsString( $e ) . "\n"; } if ( $cmdLine ) { @@ -700,68 +701,66 @@ class MWExceptionHandler { } /** - * Get the stack trace from the exception as a string, redacting certain - * function arguments in the process. - * @param Exception $e The exception - * @return string The stack trace as a string + * Generate a string representation of an exception's stack trace + * + * Like Exception::getTraceAsString, but replaces argument values with + * argument type or class name. + * + * @param Exception $e + * @return string */ - public static function formatRedactedTrace( Exception $e ) { - global $wgRedactedFunctionArguments; - $finalExceptionText = ''; + public static function getRedactedTraceAsString( Exception $e ) { + $text = ''; - // Unique value to indicate redacted parameters - $redacted = new stdClass(); - - foreach ( $e->getTrace() as $i => $call ) { - $checkFor = array(); - if ( isset( $call['class'] ) ) { - $checkFor[] = $call['class'] . '::' . $call['function']; - foreach ( class_parents( $call['class'] ) as $parent ) { - $checkFor[] = $parent . '::' . $call['function']; - } - } else { - $checkFor[] = $call['function']; - } - - foreach ( $checkFor as $check ) { - if ( isset( $wgRedactedFunctionArguments[$check] ) ) { - foreach ( (array)$wgRedactedFunctionArguments[$check] as $argNo ) { - $call['args'][$argNo] = $redacted; - } - } - } - - if ( isset( $call['file'] ) && isset( $call['line'] ) ) { - $finalExceptionText .= "#{$i} {$call['file']}({$call['line']}): "; + foreach ( self::getRedactedTrace( $e ) as $level => $frame ) { + if ( isset( $frame['file'] ) && isset( $frame['line'] ) ) { + $text .= "#{$level} {$frame['file']}({$frame['line']}): "; } else { // 'file' and 'line' are unset for calls via call_user_func (bug 55634) // This matches behaviour of Exception::getTraceAsString to instead // display "[internal function]". - $finalExceptionText .= "#{$i} [internal function]: "; + $text .= "#{$level} [internal function]: "; } - if ( isset( $call['class'] ) ) { - $finalExceptionText .= $call['class'] . $call['type'] . $call['function']; + if ( isset( $frame['class'] ) ) { + $text .= $frame['class'] . $frame['type'] . $frame['function']; } else { - $finalExceptionText .= $call['function']; + $text .= $frame['function']; } - $args = array(); - if ( isset( $call['args'] ) ) { - foreach ( $call['args'] as $arg ) { - if ( $arg === $redacted ) { - $args[] = 'REDACTED'; - } elseif ( is_object( $arg ) ) { - $args[] = 'Object(' . get_class( $arg ) . ')'; - } elseif( is_array( $arg ) ) { - $args[] = 'Array'; - } else { - $args[] = var_export( $arg, true ); - } - } + + if ( isset( $frame['args'] ) ) { + $text .= '(' . implode( ', ', $frame['args'] ) . ")\n"; + } else { + $text .= "()\n"; } - $finalExceptionText .= '(' . implode( ', ', $args ) . ")\n"; } - return $finalExceptionText . '#' . ( $i + 1 ) . ' {main}'; + + $level = $level + 1; + $text .= "#{$level} {main}"; + + return $text; + } + + /** + * Return a copy of an exception's backtrace as an array. + * + * Like Exception::getTrace, but replaces each element in each frame's + * argument array with the name of its class (if the element is an object) + * or its type (if the element is a PHP primitive). + * + * @since 1.22 + * @param Exception $e + * @return array + */ + public static function getRedactedTrace( Exception $e ) { + return array_map( function ( $frame ) { + if ( isset( $frame['args'] ) ) { + $frame['args'] = array_map( function ( $arg ) { + return is_object( $arg ) ? get_class( $arg ) : gettype( $arg ); + }, $frame['args'] ); + } + return $frame; + }, $e->getTrace() ); } /** @@ -824,7 +823,7 @@ class MWExceptionHandler { if ( !( $e instanceof MWException ) || $e->isLoggable() ) { $log = self::getLogMessage( $e ); if ( $wgLogExceptionBacktrace ) { - wfDebugLog( 'exception', $log . "\n" . self::formatRedactedTrace( $e ) . "\n" ); + wfDebugLog( 'exception', $log . "\n" . $e->getTraceAsString() . "\n" ); } else { wfDebugLog( 'exception', $log ); } diff --git a/tests/phpunit/includes/MWExceptionHandlerTest.php b/tests/phpunit/includes/MWExceptionHandlerTest.php new file mode 100644 index 0000000000..987dfa8347 --- /dev/null +++ b/tests/phpunit/includes/MWExceptionHandlerTest.php @@ -0,0 +1,73 @@ +getTrace(); + $hasObject = false; + $hasArray = false; + foreach ( $trace as $frame ) { + if ( ! isset( $frame['args'] ) ) { + continue; + } + foreach ( $frame['args'] as $arg ) { + $hasObject = $hasObject || is_object( $arg ); + $hasArray = $hasArray || is_array( $arg ); + } + + if( $hasObject && $hasArray ) { + break; + } + } + $this->assertTrue( $hasObject, + "The stacktrace must have a function having an object has parameter" ); + $this->assertTrue( $hasArray, + "The stacktrace must have a function having an array has parameter" ); + + # Now we redact the trace.. and make sure no function arguments are + # arrays or objects. + $redacted = MWExceptionHandler::getRedactedTrace( $e ); + + foreach ( $redacted as $frame ) { + if ( ! isset( $frame['args'] ) ) { + continue; + } + foreach ( $frame['args'] as $arg ) { + $this->assertNotInternalType( 'array', $arg); + $this->assertNotInternalType( 'object', $arg); + } + } + } + + /** + * Helper function for testExpandArgumentsInCall + * + * Pass it an object and an array :-) + * + * @throws Exception + */ + protected static function helperThrowAnException( $a, $b ) { + throw new Exception(); + } +}