Do not print exception message

In case of an error the error message often contains sensitive data such as the full path which potentially leads to a full path disclosure.

Thus the error message should not directly get displayed to the user and instead be logged.

3 files changed, 64 insertions, 15 deletions

diff --git a/core/application.php b/core/application.php
index 12ec6b63fd4..eab2c686dc1 100644
--- a/core/application.php
+++ b/core/application.php
@@ -85,7 +85,8 @@ class Application extends App {
 				$c->query('L10N'),
 				$c->query('UserManager'),
 				$c->query('UserSession'),
-				$c->query('UserFolder')
+				$c->query('UserFolder'),
+				$c->query('Logger')
 			);
 		});
 
@@ -128,6 +129,9 @@ class Application extends App {
 		$container->registerService('Mailer', function(SimpleContainer $c) {
 			return $c->query('ServerContainer')->getMailer();
 		});
+		$container->registerService('Logger', function(SimpleContainer $c) {
+			return $c->query('ServerContainer')->getLogger();
+		});
 		$container->registerService('TimeFactory', function(SimpleContainer $c) {
 			return new TimeFactory();
 		});
diff --git a/core/avatar/avatarcontroller.php b/core/avatar/avatarcontroller.php
index 97b3615c032..e15b47e9a84 100644
--- a/core/avatar/avatarcontroller.php
+++ b/core/avatar/avatarcontroller.php
@@ -30,7 +30,7 @@ use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\DataDisplayResponse;
 use OCP\IAvatarManager;
-use OCP\ICache;
+use OCP\ILogger;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\IUserManager;
@@ -62,6 +62,9 @@ class AvatarController extends Controller {
 	/** @var Folder */
 	protected $userFolder;
 
+	/** @var ILogger */
+	protected $logger;
+
 	/**
 	 * @param string $appName
 	 * @param IRequest $request
@@ -71,6 +74,7 @@ class AvatarController extends Controller {
 	 * @param IUserManager $userManager
 	 * @param IUserSession $userSession
 	 * @param Folder $userFolder
+	 * @param ILogger $logger
 	 */
 	public function __construct($appName,
 								IRequest $request,
@@ -79,7 +83,8 @@ class AvatarController extends Controller {
 								IL10N $l10n,
 								IUserManager $userManager,
 								IUserSession $userSession,
-								Folder $userFolder) {
+								Folder $userFolder,
+								ILogger $logger) {
 		parent::__construct($appName, $request);
 
 		$this->avatarManager = $avatarManager;
@@ -88,6 +93,7 @@ class AvatarController extends Controller {
 		$this->userManager = $userManager;
 		$this->userSession = $userSession;
 		$this->userFolder = $userFolder;
+		$this->logger = $logger;
 	}
 
 	/**
@@ -218,11 +224,8 @@ class AvatarController extends Controller {
 				);
 			}
 		} catch (\Exception $e) {
-			return new DataResponse(
-				['data' => ['message' => $e->getMessage()]],
-				Http::STATUS_OK,
-				$headers
-			);
+			$this->logger->logException($e, ['app' => 'core']);
+			return new DataResponse(['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]], Http::STATUS_OK, $headers);
 		}
 	}
 
@@ -239,7 +242,8 @@ class AvatarController extends Controller {
 			$avatar->remove();
 			return new DataResponse();
 		} catch (\Exception $e) {
-			return new DataResponse(['data' => ['message' => $e->getMessage()]], Http::STATUS_BAD_REQUEST);
+			$this->logger->logException($e, ['app' => 'core']);
+			return new DataResponse(['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]], Http::STATUS_BAD_REQUEST);
 		}
 	}
 
@@ -307,10 +311,9 @@ class AvatarController extends Controller {
 		} catch (\OC\NotSquareException $e) {
 			return new DataResponse(['data' => ['message' => $this->l->t('Crop is not square')]],
 									Http::STATUS_BAD_REQUEST);
-
-		}catch (\Exception $e) {
-			return new DataResponse(['data' => ['message' => $e->getMessage()]],
-									Http::STATUS_BAD_REQUEST);
+		} catch (\Exception $e) {
+			$this->logger->logException($e, ['app' => 'core']);
+			return new DataResponse(['data' => ['message' => $this->l->t('An error occurred. Please contact your admin.')]], Http::STATUS_BAD_REQUEST);
 		}
 	}
 }
diff --git a/tests/core/avatar/avatarcontrollertest.php b/tests/core/avatar/avatarcontrollertest.php
index e8fb977fae1..929a4e5f91d 100644
--- a/tests/core/avatar/avatarcontrollertest.php
+++ b/tests/core/avatar/avatarcontrollertest.php
@@ -76,6 +76,8 @@ class AvatarControllerTest extends \Test\TestCase {
 			->disableOriginalConstructor()->getMock();
 		$this->container['UserFolder'] = $this->getMockBuilder('OCP\Files\Folder')
 			->disableOriginalConstructor()->getMock();
+		$this->container['Logger'] = $this->getMockBuilder('OCP\ILogger')
+			->disableOriginalConstructor()->getMock();
 
 		$this->avatarMock = $this->getMockBuilder('OCP\IAvatar')
 			->disableOriginalConstructor()->getMock();
@@ -217,8 +219,11 @@ class AvatarControllerTest extends \Test\TestCase {
 		$this->avatarMock->method('remove')->will($this->throwException(new \Exception("foo")));
 		$this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock);
 
-		$response = $this->avatarController->deleteAvatar();
-		$this->assertEquals(Http::STATUS_BAD_REQUEST, $response->getStatus());
+		$this->container['Logger']->expects($this->once())
+			->method('logException')
+			->with(new \Exception("foo"));
+		$expectedResponse = new Http\DataResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_BAD_REQUEST);
+		$this->assertEquals($expectedResponse, $this->avatarController->deleteAvatar());
 	}
 
 	/**
@@ -329,6 +334,26 @@ class AvatarControllerTest extends \Test\TestCase {
 	}
 
 	/**
+	 * Test what happens if the upload of the avatar fails
+	 */
+	public function testPostAvatarException() {
+		$this->container['Cache']->expects($this->once())
+			->method('set')
+			->will($this->throwException(new \Exception("foo")));
+		$file = $this->getMockBuilder('OCP\Files\File')
+			->disableOriginalConstructor()->getMock();
+		$file->method('getContent')->willReturn(file_get_contents(OC::$SERVERROOT.'/tests/data/testimage.jpg'));
+		$this->container['UserFolder']->method('get')->willReturn($file);
+
+		$this->container['Logger']->expects($this->once())
+			->method('logException')
+			->with(new \Exception("foo"));
+		$expectedResponse = new Http\DataResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_OK);
+		$this->assertEquals($expectedResponse, $this->avatarController->postAvatar('avatar.jpg'));
+	}
+
+
+	/**
 	 * Test invalid crop argument
 	 */
 	public function testPostCroppedAvatarInvalidCrop() {
@@ -372,6 +397,23 @@ class AvatarControllerTest extends \Test\TestCase {
 	}
 
 	/**
+	 * Test what happens if the cropping of the avatar fails
+	 */
+	public function testPostCroppedAvatarException() {
+		$this->container['Cache']->method('get')->willReturn(file_get_contents(OC::$SERVERROOT.'/tests/data/testimage.jpg'));
+
+		$this->avatarMock->method('set')->will($this->throwException(new \Exception('foo')));
+		$this->container['AvatarManager']->method('getAvatar')->willReturn($this->avatarMock);
+
+		$this->container['Logger']->expects($this->once())
+			->method('logException')
+			->with(new \Exception('foo'));
+		$expectedResponse = new Http\DataResponse(['data' => ['message' => 'An error occurred. Please contact your admin.']], Http::STATUS_BAD_REQUEST);
+		$this->assertEquals($expectedResponse, $this->avatarController->postCroppedAvatar(['x' => 0, 'y' => 0, 'w' => 10, 'h' => 11]));
+	}
+
+
+	/**
 	 * Check for proper reply on proper crop argument
 	 */
 	public function testFileTooBig() {