Merge pull request #154 from fogs/master

Bugfix #153: log messages must not contain clear text passwords
Maks3w · Feb 14, 2019 · 08b058b · 08b058b
2 parents 6bc696e + 7fa26a8
commit 08b058b
Show file tree

Hide file tree

Showing 3 changed files with 65 additions and 4 deletions.
diff --git a/Driver/ZendLdapDriver.php b/Driver/ZendLdapDriver.php
@@ -7,6 +7,7 @@
 use Symfony\Component\Security\Core\User\UserInterface;
 use Zend\Ldap\Exception\LdapException as ZendLdapException;
 use Zend\Ldap\Ldap;
+use FR3D\LdapBundle\Exception\SanitizingException;
 
 /**
  * This class adapt ldap calls to Zend Framework Ldap library functions.
@@ -83,7 +84,7 @@ public function bind(UserInterface $user, $password)
 
             return ($bind instanceof Ldap);
         } catch (ZendLdapException $exception) {
-            $this->zendExceptionHandler($exception);
+            $this->zendExceptionHandler($exception, $password);
         }
 
         return false;
@@ -94,19 +95,21 @@ public function bind(UserInterface $user, $password)
      *
      * @param ZendLdapException $exception
      */
-    protected function zendExceptionHandler(ZendLdapException $exception)
+    protected function zendExceptionHandler(ZendLdapException $exception, $password)
     {
+        $sanitizedException = new SanitizingException($exception, $password);
+
         switch ($exception->getCode()) {
             // Error level codes
             case ZendLdapException::LDAP_SERVER_DOWN:
                 if ($this->logger) {
-                    $this->logger->error('{exception}', ['exception' => $exception]);
+                    $this->logger->error('{exception}', ['exception' => $sanitizedException]);
                 }
                 break;
 
             // Other level codes
             default:
-                $this->logDebug('{exception}', ['exception' => $exception]);
+                $this->logDebug('{exception}', ['exception' => $sanitizedException]);
                 break;
         }
     }

diff --git a/Exception/SanitizingException.php b/Exception/SanitizingException.php
@@ -0,0 +1,20 @@
+<?php
+
+namespace FR3D\LdapBundle\Exception;
+
+class SanitizingException extends \Exception
+{
+    protected $actualException;
+    protected $secret;
+
+    public function __construct($actualException, $secret)
+    {
+        $this->actualException = $actualException;
+        $this->secret = $secret;
+    }
+
+    public function __toString()
+    {
+        return str_replace($this->secret, '****', $this->actualException->__toString());
+    }
+}
diff --git a/Tests/Driver/ZendLdapDriverTest.php b/Tests/Driver/ZendLdapDriverTest.php
@@ -7,6 +7,8 @@
 use Symfony\Component\Security\Core\User\UserInterface;
 use Zend\Ldap\Exception\LdapException as ZendLdapException;
 use Zend\Ldap\Ldap;
+use Psr\Log\LoggerInterface;
+use FR3D\LdapBundle\Exception\SanitizingException;
 
 /**
  * Test class for ZendLdapDriver.
@@ -88,4 +90,40 @@ public function testFailBindByDn(UserInterface $user, $password)
 
         self::assertFalse($this->driver->bind($user, $password));
     }
+
+    public function testZendExceptionHandler()
+    {
+        $password = 'veryverysecret';
+
+        $loggerMock = $this->getMockBuilder(LoggerInterface::class)
+        ->setMethods(['debug', 'error'])
+        ->getMockForAbstractClass();
+
+        $zendLdapExceptionMock = $this->getMockBuilder(ZendLdapException::class)->getMock();
+        $zendLdapExceptionMock
+        ->method('__toString')
+        ->willReturn("Zend\Ldap\Exception\LdapException: fr3d/ldap-bundle/Driver/ZendLdapDriver.php(82): Zend\Ldap\Ldap->bind('fogs', '$password')")
+        ;
+
+        $loggerMock->method('debug')->with('{exception}', $this->callback(function ($context) use ($password) {
+            if (!array_key_exists('exception', $context)) {
+                return $this->fail('Logger context must contain key "exception"');
+            }
+            if (!$context['exception'] instanceof SanitizingException) {
+                return $this->fail('Logger context "exception" must contain object of class SanitizingException');
+            }
+            if (strpos($context['exception']->__toString(), $password) !== false) {
+                return $this->fail('String representation of the SanitizingException must not contain the bind password');
+            }
+            return true;
+        }));
+
+        $reflectionClass = new \ReflectionClass($this->driver);
+        $reflectionProperty = $reflectionClass->getProperty('logger');
+        $reflectionProperty->setAccessible(true);
+        $reflectionProperty->setValue($this->driver, $loggerMock);
+        $reflectionMethod = $reflectionClass->getMethod('zendExceptionHandler');
+        $reflectionMethod->setAccessible(true);
+        $reflectionMethod->invoke($this->driver, $zendLdapExceptionMock, $password);
+    }
 }