redact exception traces and abstract getTrace
authorAntoine Musso <hashar@free.fr>
Mon, 28 Oct 2013 16:56:37 +0000 (17:56 +0100)
committerOri Livneh <ori@wikimedia.org>
Fri, 1 Nov 2013 18:07:18 +0000 (11:07 -0700)
* 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

RELEASE-NOTES-1.22
includes/DefaultSettings.php
includes/Exception.php
tests/phpunit/includes/MWExceptionHandlerTest.php [new file with mode: 0644]

index 769f661..827e003 100644 (file)
@@ -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
index d3c7b5f..ebae110 100644 (file)
@@ -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
  */
index aeafda8..9623a84 100644 (file)
@@ -141,8 +141,7 @@ class MWException extends Exception {
 
                if ( $wgShowExceptionDetails ) {
                        return '<p>' . nl2br( htmlspecialchars( $this->getMessage() ) ) .
-                               '</p><p>Backtrace:</p><p>' .
-                               nl2br( htmlspecialchars( MWExceptionHandler::formatRedactedTrace( $this ) ) ) .
+                               '</p><p>Backtrace:</p><p>' . nl2br( htmlspecialchars( MWExceptionHandler::getRedactedTraceAsString( $this ) ) ) .
                                "</p>\n";
                } else {
                        return "<div class=\"errorbox\">" .
@@ -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 (file)
index 0000000..987dfa8
--- /dev/null
@@ -0,0 +1,73 @@
+<?php
+/**
+ * Tests for includes/Exception.php.
+ *
+ * @author Antoine Musso
+ * @copyright Copyright © 2013, Antoine Musso
+ * @copyright Copyright © 2013, Wikimedia Foundation Inc.
+ * @file
+ */
+
+class MWExceptionHandlerTest extends MediaWikiTestCase {
+
+       /**
+        * @covers MWExceptionHandler::getRedactedTrace
+        */
+       function testGetRedactedTrace() {
+               try {
+                       $array = array( 'a', 'b' );
+                       $object = new StdClass();
+                       self::helperThrowAnException( $array, $object );
+               } catch (Exception $e) {
+               }
+
+               # Make sure our strack trace contains an array and an object passed to
+               # some function in the stacktrace. Else, we can not assert the trace
+               # redaction achieved its job.
+               $trace = $e->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();
+       }
+}