fix(dav): Always respond custom error page on exceptions

Signed-off-by: Louis Chemineau <louis@chmn.me>

apps/dav/composer/composer/autoload_classmap.php

@@ -247,7 +247,7 @@ return array(
     'OCA\\DAV\\Events\\SubscriptionUpdatedEvent' => $baseDir . '/../lib/Events/SubscriptionUpdatedEvent.php',
     'OCA\\DAV\\Exception\\ServerMaintenanceMode' => $baseDir . '/../lib/Exception/ServerMaintenanceMode.php',
     'OCA\\DAV\\Exception\\UnsupportedLimitOnInitialSyncException' => $baseDir . '/../lib/Exception/UnsupportedLimitOnInitialSyncException.php',
-    'OCA\\DAV\\Files\\BrowserErrorPagePlugin' => $baseDir . '/../lib/Files/BrowserErrorPagePlugin.php',
+    'OCA\\DAV\\Files\\ErrorPagePlugin' => $baseDir . '/../lib/Files/ErrorPagePlugin.php',
     'OCA\\DAV\\Files\\FileSearchBackend' => $baseDir . '/../lib/Files/FileSearchBackend.php',
     'OCA\\DAV\\Files\\FilesHome' => $baseDir . '/../lib/Files/FilesHome.php',
     'OCA\\DAV\\Files\\LazySearchBackend' => $baseDir . '/../lib/Files/LazySearchBackend.php',

apps/dav/composer/composer/autoload_static.php

@@ -262,7 +262,7 @@ class ComposerStaticInitDAV
         'OCA\\DAV\\Events\\SubscriptionUpdatedEvent' => __DIR__ . '/..' . '/../lib/Events/SubscriptionUpdatedEvent.php',
         'OCA\\DAV\\Exception\\ServerMaintenanceMode' => __DIR__ . '/..' . '/../lib/Exception/ServerMaintenanceMode.php',
         'OCA\\DAV\\Exception\\UnsupportedLimitOnInitialSyncException' => __DIR__ . '/..' . '/../lib/Exception/UnsupportedLimitOnInitialSyncException.php',
-        'OCA\\DAV\\Files\\BrowserErrorPagePlugin' => __DIR__ . '/..' . '/../lib/Files/BrowserErrorPagePlugin.php',
+        'OCA\\DAV\\Files\\ErrorPagePlugin' => __DIR__ . '/..' . '/../lib/Files/ErrorPagePlugin.php',
         'OCA\\DAV\\Files\\FileSearchBackend' => __DIR__ . '/..' . '/../lib/Files/FileSearchBackend.php',
         'OCA\\DAV\\Files\\FilesHome' => __DIR__ . '/..' . '/../lib/Files/FilesHome.php',
         'OCA\\DAV\\Files\\LazySearchBackend' => __DIR__ . '/..' . '/../lib/Files/LazySearchBackend.php',

apps/dav/lib/Connector/Sabre/ServerFactory.php

@@ -34,7 +34,7 @@ namespace OCA\DAV\Connector\Sabre;
 use OCP\Files\Folder;
 use OCA\DAV\AppInfo\PluginManager;
 use OCA\DAV\DAV\ViewOnlyPlugin;
-use OCA\DAV\Files\BrowserErrorPagePlugin;
+use OCA\DAV\Files\ErrorPagePlugin;
 use OCP\Files\Mount\IMountManager;
 use OCP\IConfig;
 use OCP\IDBConnection;
@@ -120,9 +120,7 @@ class ServerFactory {
 			$server->addPlugin(new \OCA\DAV\Connector\Sabre\FakeLockerPlugin());
 		}
 
-		if (BrowserErrorPagePlugin::isBrowserRequest($this->request)) {
-			$server->addPlugin(new BrowserErrorPagePlugin());
-		}
+		$server->addPlugin(new ErrorPagePlugin($this->request, $this->config));
 
 		// wait with registering these until auth is handled and the filesystem is setup
 		$server->on('beforeMethod:*', function () use ($server, $objectTree, $viewCallBack) {

apps/dav/lib/Files/ErrorPagePlugin.php

@@ -24,17 +24,22 @@
  */
 namespace OCA\DAV\Files;
 
-use OC\AppFramework\Http\Request;
 use OC_Template;
 use OCP\AppFramework\Http\ContentSecurityPolicy;
+use OCP\IConfig;
 use OCP\IRequest;
 use Sabre\DAV\Exception;
 use Sabre\DAV\Server;
 use Sabre\DAV\ServerPlugin;
 
-class BrowserErrorPagePlugin extends ServerPlugin {
-	/** @var Server */
-	private $server;
+class ErrorPagePlugin extends ServerPlugin {
+	private ?Server $server = null;
+
+	public function __construct(
+		private IRequest $request,
+		private IConfig $config,
+	) {
+	}
 
 	/**
 	 * This initializes the plugin.
@@ -43,36 +48,13 @@ class BrowserErrorPagePlugin extends ServerPlugin {
 	 * addPlugin is called.
 	 *
 	 * This method should set up the required event subscriptions.
-	 *
-	 * @param Server $server
-	 * @return void
 	 */
-	public function initialize(Server $server) {
+	public function initialize(Server $server): void {
 		$this->server = $server;
 		$server->on('exception', [$this, 'logException'], 1000);
 	}
 
-	/**
-	 * @param IRequest $request
-	 * @return bool
-	 */
-	public static function isBrowserRequest(IRequest $request) {
-		if ($request->getMethod() !== 'GET') {
-			return false;
-		}
-		return $request->isUserAgent([
-			Request::USER_AGENT_IE,
-			Request::USER_AGENT_MS_EDGE,
-			Request::USER_AGENT_CHROME,
-			Request::USER_AGENT_FIREFOX,
-			Request::USER_AGENT_SAFARI,
-		]);
-	}
-
-	/**
-	 * @param \Exception $ex
-	 */
-	public function logException(\Exception $ex) {
+	public function logException(\Throwable $ex): void {
 		if ($ex instanceof Exception) {
 			$httpCode = $ex->getHTTPCode();
 			$headers = $ex->getHTTPHeaders($this->server);
@@ -82,7 +64,7 @@ class BrowserErrorPagePlugin extends ServerPlugin {
 		}
 		$this->server->httpResponse->addHeaders($headers);
 		$this->server->httpResponse->setStatus($httpCode);
-		$body = $this->generateBody($httpCode);
+		$body = $this->generateBody($ex, $httpCode);
 		$this->server->httpResponse->setBody($body);
 		$csp = new ContentSecurityPolicy();
 		$this->server->httpResponse->addHeader('Content-Security-Policy', $csp->buildPolicy());
@@ -93,18 +75,32 @@ class BrowserErrorPagePlugin extends ServerPlugin {
 	 * @codeCoverageIgnore
 	 * @return bool|string
 	 */
-	public function generateBody(int $httpCode) {
-		$request = \OC::$server->getRequest();
-
-		$templateName = 'exception';
-		if ($httpCode === 403 || $httpCode === 404) {
-			$templateName = (string)$httpCode;
+	public function generateBody(\Throwable $ex, int $httpCode): mixed {
+		if ($this->acceptHtml()) {
+			$templateName = 'exception';
+			$renderAs = 'guest';
+			if ($httpCode === 403 || $httpCode === 404) {
+				$templateName = (string)$httpCode;
+			}
+		} else {
+			$templateName = 'xml_exception';
+			$renderAs = null;
+			$this->server->httpResponse->setHeader('Content-Type', 'application/xml; charset=utf-8');
 		}
 
-		$content = new OC_Template('core', $templateName, 'guest');
+		$debug = $this->config->getSystemValueBool('debug', false);
+
+		$content = new OC_Template('core', $templateName, $renderAs);
 		$content->assign('title', $this->server->httpResponse->getStatusText());
-		$content->assign('remoteAddr', $request->getRemoteAddress());
-		$content->assign('requestID', $request->getId());
+		$content->assign('remoteAddr', $this->request->getRemoteAddress());
+		$content->assign('requestID', $this->request->getId());
+		$content->assign('debugMode', $debug);
+		$content->assign('errorClass', get_class($ex));
+		$content->assign('errorMsg', $ex->getMessage());
+		$content->assign('errorCode', $ex->getCode());
+		$content->assign('file', $ex->getFile());
+		$content->assign('line', $ex->getLine());
+		$content->assign('exception', $ex);
 		return $content->fetchPage();
 	}
 
@@ -115,4 +111,14 @@ class BrowserErrorPagePlugin extends ServerPlugin {
 		$this->server->sapi->sendResponse($this->server->httpResponse);
 		exit();
 	}
+
+	private function acceptHtml(): bool {
+		foreach (explode(',', $this->request->getHeader('Accept')) as $part) {
+			$subparts = explode(';', $part);
+			if (str_ends_with($subparts[0], '/html')) {
+				return true;
+			}
+		}
+		return false;
+	}
 }

apps/dav/lib/Server.php

@@ -69,7 +69,7 @@ use OCA\DAV\DAV\CustomPropertiesBackend;
 use OCA\DAV\DAV\PublicAuth;
 use OCA\DAV\DAV\ViewOnlyPlugin;
 use OCA\DAV\Events\SabrePluginAuthInitEvent;
-use OCA\DAV\Files\BrowserErrorPagePlugin;
+use OCA\DAV\Files\ErrorPagePlugin;
 use OCA\DAV\Files\LazySearchBackend;
 use OCA\DAV\Profiler\ProfilerPlugin;
 use OCA\DAV\Provisioning\Apple\AppleProvisioningPlugin;
@@ -244,9 +244,7 @@ class Server {
 			$this->server->addPlugin(new FakeLockerPlugin());
 		}
 
-		if (BrowserErrorPagePlugin::isBrowserRequest($request)) {
-			$this->server->addPlugin(new BrowserErrorPagePlugin());
-		}
+		$this->server->addPlugin(new ErrorPagePlugin($this->request, \OC::$server->getConfig()));
 
 		$lazySearchBackend = new LazySearchBackend();
 		$this->server->addPlugin(new SearchPlugin($lazySearchBackend));

apps/dav/tests/travis/caldavtest/tests/CalDAV/sync-report.xml

@@ -2712,7 +2712,7 @@
 					<callback>prepostcondition</callback>
 					<arg>
 						<name>error</name>
-						<value>{DAV:}valid-sync-token</value>
+						<value>{http://sabredav.org/ns}exception</value>
 					</arg>
 					<arg>
 						<name>ignoreextras</name>

apps/dav/tests/unit/DAV/ErrorPagePluginTest.php

@@ -23,11 +23,11 @@
  */
 namespace OCA\DAV\Tests\unit\DAV;
 
-use OCA\DAV\Files\BrowserErrorPagePlugin;
+use OCA\DAV\Files\ErrorPagePlugin;
 use Sabre\DAV\Exception\NotFound;
 use Sabre\HTTP\Response;
 
-class BrowserErrorPagePluginTest extends \Test\TestCase {
+class ErrorPagePluginTest extends \Test\TestCase {
 
 	/**
 	 * @dataProvider providesExceptions
@@ -35,8 +35,8 @@ class BrowserErrorPagePluginTest extends \Test\TestCase {
 	 * @param $exception
 	 */
 	public function test($expectedCode, $exception): void {
-		/** @var BrowserErrorPagePlugin | \PHPUnit\Framework\MockObject\MockObject $plugin */
-		$plugin = $this->getMockBuilder(BrowserErrorPagePlugin::class)->setMethods(['sendResponse', 'generateBody'])->getMock();
+		/** @var ErrorPagePlugin | \PHPUnit\Framework\MockObject\MockObject $plugin */
+		$plugin = $this->getMockBuilder(ErrorPagePlugin::class)->disableOriginalConstructor()->setMethods(['sendResponse', 'generateBody'])->getMock();
 		$plugin->expects($this->once())->method('generateBody')->willReturn(':boom:');
 		$plugin->expects($this->once())->method('sendResponse');
 		/** @var \Sabre\DAV\Server | \PHPUnit\Framework\MockObject\MockObject $server */

build/integration/features/caldav.feature

@@ -3,8 +3,7 @@ Feature: caldav
     Given user "user0" exists
     When "admin" requests calendar "user0/MyCalendar" on the endpoint "/remote.php/dav/calendars/"
     Then The CalDAV HTTP status code should be "404"
-    And The exception is "Sabre\DAV\Exception\NotFound"
-    And The error message is "Node with name 'MyCalendar' could not be found"
+    And The exception is "Internal Server Error"
 
   Scenario: Accessing a not shared calendar of another user
     Given user "user0" exists
@@ -12,8 +11,7 @@ Feature: caldav
     Given The CalDAV HTTP status code should be "201"
     When "user0" requests calendar "admin/MyCalendar" on the endpoint "/remote.php/dav/calendars/"
     Then The CalDAV HTTP status code should be "404"
-    And The exception is "Sabre\DAV\Exception\NotFound"
-    And The error message is "Calendar with name 'MyCalendar' could not be found"
+    And The exception is "Internal Server Error"
 
   Scenario: Accessing a not shared calendar of another user via the legacy endpoint
     Given user "user0" exists
@@ -28,8 +26,7 @@ Feature: caldav
     Given user "user0" exists
     When "user0" requests calendar "admin/MyCalendar" on the endpoint "/remote.php/dav/calendars/"
     Then The CalDAV HTTP status code should be "404"
-    And The exception is "Sabre\DAV\Exception\NotFound"
-    And The error message is "Node with name 'MyCalendar' could not be found"
+    And The exception is "Internal Server Error"
 
   Scenario: Accessing a not existing calendar of another user via the legacy endpoint
     Given user "user0" exists
@@ -42,8 +39,7 @@ Feature: caldav
     Given user "user0" exists
     When "user0" requests calendar "admin/MyCalendar" on the endpoint "/remote.php/dav/calendars/"
     Then The CalDAV HTTP status code should be "404"
-    And The exception is "Sabre\DAV\Exception\NotFound"
-    And The error message is "Node with name 'MyCalendar' could not be found"
+    And The exception is "Internal Server Error"
 
   Scenario: Creating a new calendar
     When "admin" creates a calendar named "MyCalendar"
@@ -64,8 +60,7 @@ Feature: caldav
     Given user "user0" exists
     When "user0" sends a create calendar request to "admin/MyCalendar2" on the endpoint "/remote.php/dav/calendars/"
     Then The CalDAV HTTP status code should be "404"
-    And The exception is "Sabre\DAV\Exception\NotFound"
-    And The error message is "Node with name 'admin' could not be found"
+    And The exception is "Internal Server Error"
 
   Scenario: Create calendar request for existing calendar of another user
     Given user "user0" exists
@@ -73,5 +68,4 @@ Feature: caldav
     Then The CalDAV HTTP status code should be "201"
     When "user0" sends a create calendar request to "admin/MyCalendar2" on the endpoint "/remote.php/dav/calendars/"
     Then The CalDAV HTTP status code should be "404"
-    And The exception is "Sabre\DAV\Exception\NotFound"
-    And The error message is "Node with name 'admin' could not be found"
+    And The exception is "Internal Server Error"

build/integration/features/carddav.feature

@@ -2,15 +2,13 @@ Feature: carddav
   Scenario: Accessing a not existing addressbook of another user
     Given user "user0" exists
     When "admin" requests addressbook "user0/MyAddressbook" with statuscode "404" on the endpoint "/remote.php/dav/addressbooks/users/"
-    And The CardDAV exception is "Sabre\DAV\Exception\NotFound"
-    And The CardDAV error message is "Addressbook with name 'MyAddressbook' could not be found"
+    And The CardDAV exception is "Internal Server Error"
 
   Scenario: Accessing a not shared addressbook of another user
     Given user "user0" exists
     Given "admin" creates an addressbook named "MyAddressbook" with statuscode "201"
     When "user0" requests addressbook "admin/MyAddressbook" with statuscode "404" on the endpoint "/remote.php/dav/addressbooks/users/"
-    And The CardDAV exception is "Sabre\DAV\Exception\NotFound"
-    And The CardDAV error message is "Addressbook with name 'MyAddressbook' could not be found"
+    And The CardDAV exception is "Internal Server Error"
 
   Scenario: Accessing a not existing addressbook of another user via legacy endpoint
     Given user "user0" exists
@@ -28,8 +26,7 @@ Feature: carddav
   Scenario: Accessing a not existing addressbook of myself
     Given user "user0" exists
     When "user0" requests addressbook "admin/MyAddressbook" with statuscode "404" on the endpoint "/remote.php/dav/addressbooks/users/"
-    And The CardDAV exception is "Sabre\DAV\Exception\NotFound"
-    And The CardDAV error message is "Addressbook with name 'MyAddressbook' could not be found"
+    And The CardDAV exception is "Internal Server Error"
 
   Scenario: Creating a new addressbook
     When "admin" creates an addressbook named "MyAddressbook" with statuscode "201"
@@ -67,13 +64,11 @@ Feature: carddav
     Given user "user0" exists
     When "user0" sends a create addressbook request to "admin/MyAddressbook2" on the endpoint "/remote.php/dav/addressbooks/"
     Then The CardDAV HTTP status code should be "404"
-    And The CardDAV exception is "Sabre\DAV\Exception\NotFound"
-    And The CardDAV error message is "File not found: admin in 'addressbooks'"
+    And The CardDAV exception is "Internal Server Error"
 
   Scenario: Create addressbook request for existing addressbook of another user
     Given user "user0" exists
     When "admin" creates an addressbook named "MyAddressbook2" with statuscode "201"
     When "user0" sends a create addressbook request to "admin/MyAddressbook2" on the endpoint "/remote.php/dav/addressbooks/"
     Then The CardDAV HTTP status code should be "404"
-    And The CardDAV exception is "Sabre\DAV\Exception\NotFound"
-    And The CardDAV error message is "File not found: admin in 'addressbooks'"
+    And The CardDAV exception is "Internal Server Error"

core/templates/xml_exception.php

@@ -0,0 +1,47 @@
+<?php
+/**
+ * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-FileCopyrightText: 2012-2015 ownCloud, Inc.
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+
+function print_exception(Throwable $e, \OCP\IL10N $l): void {
+	p($e->getTraceAsString());
+
+	if ($e->getPrevious() !== null) {
+		print_unescaped('<s:previous-exception>');
+		print_exception($e->getPrevious(), $l);
+		print_unescaped('</s:previous-exception>');
+	}
+}
+
+?>
+<?xml version="1.0" encoding="utf-8"?>
+<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
+	<s:exception><?php p($l->t('Internal Server Error')) ?></s:exception>
+	<s:message>
+		<?php p($l->t('The server was unable to complete your request.')) ?>
+		<?php p($l->t('If this happens again, please send the technical details below to the server administrator.')) ?>
+		<?php p($l->t('More details can be found in the server log.')) ?>
+		<?php if (isset($_['serverLogsDocumentation']) && $_['serverLogsDocumentation'] !== ''): ?>
+			<?php p($l->t('For more details see the documentation ↗.'))?>: <?php print_unescaped($_['serverLogsDocumentation']) ?>
+		<?php endif; ?>
+	</s:message>
+
+	<s:technical-details>
+		<s:remote-address><?php p($_['remoteAddr']) ?></s:remote-address>
+		<s:request-id><?php p($_['requestID']) ?></s:request-id>
+
+	<?php if (isset($_['debugMode']) && $_['debugMode'] === true): ?>
+		<s:type><?php p($_['errorClass']) ?></s:type>
+		<s:code><?php p($_['errorCode']) ?></s:code>
+		<s:message><?php p($_['errorMsg']) ?></s:message>
+		<s:file><?php p($_['file']) ?></s:file>
+		<s:line><?php p($_['line']) ?></s:line>
+
+		<s:stacktrace>
+			<?php print_exception($_['exception'], $l); ?>
+		</s:stacktrace>
+	<?php endif; ?>
+	</s:technical-details>
+</d:error>