Do not show exception to the end-user

Log the error instead of potentially leaking sensitive information

core/templates/error.php

@@ -2,7 +2,9 @@
 	<?php foreach($_["errors"] as $error):?>
 		<li class='error'>
 			<?php p($error['error']) ?><br/>
-			<p class='hint'><?php if(isset($error['hint']))print_unescaped($error['hint']) ?></p>
+			<?php if(isset($error['hint']) && $error['hint']): ?>
+				<p class='hint'><?php print_unescaped($error['hint']) ?></p>
+			<?php endif;?>
 		</li>
 	<?php endforeach ?>
 </ul>

core/templates/exception.php

@@ -0,0 +1,28 @@
+<?php
+	/** @var array $_ */
+	/** @var OC_L10N $l */
+?>
+<span class="error error-wide">
+	<h2><strong><?php p('Internal Server Error') ?></strong></h2><br/>
+		<p><?php p($l->t('The server encountered an internal error and was unable to complete your request.')) ?></p>
+		<p><?php p($l->t('Please contact the server administrator if this error reappears multiple times, please include the technical details below in your report.')) ?></p>
+		<p><?php p($l->t('More details can be found in the server log.')) ?></p>
+	<hr/>
+	<h2><strong><?php p($l->t('Technical details')) ?></strong></h2>
+	<ul>
+		<li><?php p($l->t('Remote Address: %s', $_['remoteAddr'])) ?></li>
+		<li><?php p($l->t('Request ID: %s', $_['requestID'])) ?></li>
+		<?php if($_['debugMode']): ?>
+			<li><?php p($l->t('Code: %s', $_['errorCode'])) ?></li>
+			<li><?php p($l->t('Message: %s', $_['errorMsg'])) ?></li>
+			<li><?php p($l->t('File: %s', $_['file'])) ?></li>
+			<li><?php p($l->t('Line: %s', $_['line'])) ?></li>
+		<?php endif; ?>
+	</ul>
+
+	<?php if($_['debugMode']): ?>
+		<hr/>
+		<h2><strong><?php p($l->t('Trace')) ?></strong></h2>
+		<pre><?php p($_['trace']) ?></pre>
+	<?php endif; ?>
+</span>

index.php

@@ -33,6 +33,9 @@ try {
 	//show the user a detailed error page
 	OC_Response::setStatus(OC_Response::STATUS_SERVICE_UNAVAILABLE);
 	OC_Template::printExceptionErrorPage($ex);
+} catch (\OC\HintException $ex) {
+	OC_Response::setStatus(OC_Response::STATUS_SERVICE_UNAVAILABLE);
+	OC_Template::printErrorPage($ex->getMessage(), $ex->getHint());
 } catch (Exception $ex) {
 	\OCP\Util::logException('index', $ex);
 

lib/private/hintexception.php

@@ -12,7 +12,7 @@ class HintException extends \Exception {
 
 	private $hint;
 
-	public function __construct($message, $hint = '', $code = 0, Exception $previous = null) {
+	public function __construct($message, $hint = '', $code = 0, \Exception $previous = null) {
 		$this->hint = $hint;
 		parent::__construct($message, $code, $previous);
 	}

lib/private/log/owncloud.php

@@ -28,7 +28,6 @@
 
 class OC_Log_Owncloud {
 	static protected $logFile;
-	static protected $reqId;
 
 	/**
 	 * Init class data
@@ -69,19 +68,17 @@ class OC_Log_Owncloud {
 				$timezone = new DateTimeZone('UTC');
 			}
 			$time = new DateTime(null, $timezone);
+			$reqId = \OC_Request::getRequestID();
+			$remoteAddr = \OC_Request::getRemoteAddress();
 			// remove username/passwords from URLs before writing the to the log file
 			$time = $time->format($format);
 			if($minLevel == OC_Log::DEBUG) {
-				if(empty(self::$reqId)) {
-					self::$reqId = uniqid();
-				}
-				$reqId = self::$reqId;
 				$url = isset($_SERVER['REQUEST_URI']) ? $_SERVER['REQUEST_URI'] : '--';
 				$method = isset($_SERVER['REQUEST_METHOD']) ? $_SERVER['REQUEST_METHOD'] : '--';
-				$entry = compact('reqId', 'app', 'message', 'level', 'time', 'method', 'url');
+				$entry = compact('reqId', 'remoteAddr', 'app', 'message', 'level', 'time', 'method', 'url');
 			}
 			else {
-				$entry = compact('app', 'message', 'level', 'time');
+				$entry = compact('reqId', 'remoteAddr', 'app', 'message', 'level', 'time');
 			}
 			$entry = json_encode($entry);
 			$handle = @fopen(self::$logFile, 'a');

lib/private/request.php

@@ -14,6 +14,7 @@ class OC_Request {
 	const USER_AGENT_FREEBOX = '#^Mozilla/5\.0$#';
 
 	const REGEX_LOCALHOST = '/^(127\.0\.0\.1|localhost)(:[0-9]+|)$/';
+	static protected $reqId;
 
 	/**
 	 * Returns the remote address, if the connection came from a trusted proxy and `forwarded_for_headers` has been configured
@@ -22,7 +23,7 @@ class OC_Request {
 	 * @return string IP address
 	 */
 	public static function getRemoteAddress() {
-		$remoteAddress = $_SERVER['REMOTE_ADDR'];
+		$remoteAddress = isset($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : '';
 		$trustedProxies = \OC::$server->getConfig()->getSystemValue('trusted_proxies', array());
 
 		if(is_array($trustedProxies) && in_array($remoteAddress, $trustedProxies)) {
@@ -43,6 +44,17 @@ class OC_Request {
 		return $remoteAddress;
 	}
 
+	/**
+	 * Returns an ID for the request, value is not guaranteed to be unique and is mostly meant for logging
+	 * @return string
+	 */
+	public static function getRequestID() {
+		if(self::$reqId === null) {
+			self::$reqId = hash('md5', microtime().\OC::$server->getSecureRandom()->getLowStrengthGenerator()->generate(20));
+		}
+		return self::$reqId;
+	}
+
 	/**
 	 * Check overwrite condition
 	 * @param string $type

lib/private/template.php

@@ -250,8 +250,7 @@ class OC_Template extends \OC\Template\Base {
 	/**
 		* Print a fatal error page and terminates the script
 		* @param string $error_msg The error message to show
-		* @param string $hint An optional hint message
+		* @param string $hint An optional hint message - needs to be properly escaped
-		* Warning: All data passed to $hint needs to get sanitized using OC_Util::sanitizeHTML
 		*/
 	public static function printErrorPage( $error_msg, $hint = '' ) {
 		$content = new \OC_Template( '', 'error', 'error', false );
@@ -266,28 +265,16 @@ class OC_Template extends \OC\Template\Base {
 	 * @param Exception $exception
 	 */
 	public static function printExceptionErrorPage(Exception $exception) {
-		$error_msg = $exception->getMessage();
+		$content = new \OC_Template('', 'exception', 'error', false);
-		if ($exception->getCode()) {
+		$content->assign('errorMsg', $exception->getMessage());
-			$error_msg = '['.$exception->getCode().'] '.$error_msg;
+		$content->assign('errorCode', $exception->getCode());
-		}
+		$content->assign('file', $exception->getFile());
-		if (defined('DEBUG') and DEBUG) {
+		$content->assign('line', $exception->getLine());
-			$hint = $exception->getTraceAsString();
+		$content->assign('trace', $exception->getTraceAsString());
-			if (!empty($hint)) {
+		$content->assign('debugMode', defined('DEBUG') && DEBUG === true);
-				$hint = '<pre>'.OC_Util::sanitizeHTML($hint).'</pre>';
+		$content->assign('remoteAddr', OC_Request::getRemoteAddress());
-			}
+		$content->assign('requestID', OC_Request::getRequestID());
-			while (method_exists($exception, 'previous') && $exception = $exception->previous()) {
+		$content->printPage();
-				$error_msg .= '<br/>Caused by:' . ' ';
+		die();
-				if ($exception->getCode()) {
-					$error_msg .= '['.OC_Util::sanitizeHTML($exception->getCode()).'] ';
-				}
-				$error_msg .= OC_Util::sanitizeHTML($exception->getMessage());
-			};
-		} else {
-			$hint = '';
-			if ($exception instanceof \OC\HintException) {
-				$hint = OC_Util::sanitizeHTML($exception->getHint());
-			}
-		}
-		self::printErrorPage($error_msg, $hint);
 	}
 }

lib/public/util.php

@@ -82,38 +82,20 @@ class Util {
 	}
 
 	/**
-	 * write exception into the log. Include the stack trace
+	 * write exception into the log
-	 * if DEBUG mode is enabled
 	 * @param string $app app name
 	 * @param \Exception $ex exception to log
-	 * @param string $level log level, defaults to \OCP\Util::FATAL
+	 * @param int $level log level, defaults to \OCP\Util::FATAL
 	 */
 	public static function logException( $app, \Exception $ex, $level = \OCP\Util::FATAL ) {
-		$class = get_class($ex);
+		$exception = array(
-		$message = $class . ': ' . $ex->getMessage();
+			'Message' => $ex->getMessage(),
-		if ($ex->getCode()) {
+			'Code' => $ex->getCode(),
-			$message .= ' [' . $ex->getCode() . ']';
+			'Trace' => $ex->getTraceAsString(),
-		}
+			'File' => $ex->getFile(),
-		\OCP\Util::writeLog($app, $message, $level);
+			'Line' => $ex->getLine(),
-		if (defined('DEBUG') and DEBUG) {
+		);
-			// also log stack trace
+		\OCP\Util::writeLog($app, 'Exception: ' . json_encode($exception), $level);
-			$stack = explode("\n", $ex->getTraceAsString());
-			// first element is empty
-			array_shift($stack);
-			foreach ($stack as $s) {
-				\OCP\Util::writeLog($app, 'Exception: ' . $s, $level);
-			}
-
-			// include cause
-			while (method_exists($ex, 'getPrevious') && $ex = $ex->getPrevious()) {
-				$message .= ' - Caused by:' . ' ';
-				$message .= $ex->getMessage();
-				if ($ex->getCode()) {
-					$message .= '[' . $ex->getCode() . '] ';
-				}
-				\OCP\Util::writeLog($app, 'Exception: ' . $message, $level);
-			}
-		}
 	}
 
 	/**