From 9992e7d4395a773fec7148cf5b4111f894cb40b7 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Thu, 5 Sep 2024 16:17:32 +0200 Subject: fix(dav): Always respond custom error page on exceptions Signed-off-by: Louis Chemineau --- apps/dav/composer/composer/autoload_classmap.php | 2 +- apps/dav/composer/composer/autoload_static.php | 2 +- apps/dav/lib/Connector/Sabre/ServerFactory.php | 6 +- apps/dav/lib/Files/BrowserErrorPagePlugin.php | 101 ------------------- apps/dav/lib/Files/ErrorPagePlugin.php | 107 +++++++++++++++++++++ apps/dav/lib/Server.php | 6 +- .../travis/caldavtest/tests/CalDAV/sync-report.xml | 2 +- .../tests/unit/DAV/BrowserErrorPagePluginTest.php | 44 --------- apps/dav/tests/unit/DAV/ErrorPagePluginTest.php | 44 +++++++++ build/integration/dav_features/caldav.feature | 18 ++-- build/integration/dav_features/carddav.feature | 15 +-- core/templates/xml_exception.php | 47 +++++++++ 12 files changed, 216 insertions(+), 178 deletions(-) delete mode 100644 apps/dav/lib/Files/BrowserErrorPagePlugin.php create mode 100644 apps/dav/lib/Files/ErrorPagePlugin.php delete mode 100644 apps/dav/tests/unit/DAV/BrowserErrorPagePluginTest.php create mode 100644 apps/dav/tests/unit/DAV/ErrorPagePluginTest.php create mode 100644 core/templates/xml_exception.php diff --git a/apps/dav/composer/composer/autoload_classmap.php b/apps/dav/composer/composer/autoload_classmap.php index 802b2b50081..43c3cb12ddb 100644 --- a/apps/dav/composer/composer/autoload_classmap.php +++ b/apps/dav/composer/composer/autoload_classmap.php @@ -272,7 +272,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', diff --git a/apps/dav/composer/composer/autoload_static.php b/apps/dav/composer/composer/autoload_static.php index d9e607222d1..fd49b5fb45c 100644 --- a/apps/dav/composer/composer/autoload_static.php +++ b/apps/dav/composer/composer/autoload_static.php @@ -287,7 +287,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', diff --git a/apps/dav/lib/Connector/Sabre/ServerFactory.php b/apps/dav/lib/Connector/Sabre/ServerFactory.php index fc35d78a2f7..2d804aad842 100644 --- a/apps/dav/lib/Connector/Sabre/ServerFactory.php +++ b/apps/dav/lib/Connector/Sabre/ServerFactory.php @@ -10,7 +10,7 @@ namespace OCA\DAV\Connector\Sabre; use OCA\DAV\AppInfo\PluginManager; use OCA\DAV\CalDAV\DefaultCalendarValidator; use OCA\DAV\DAV\ViewOnlyPlugin; -use OCA\DAV\Files\BrowserErrorPagePlugin; +use OCA\DAV\Files\ErrorPagePlugin; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\Folder; use OCP\Files\IFilenameValidator; @@ -98,9 +98,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) { diff --git a/apps/dav/lib/Files/BrowserErrorPagePlugin.php b/apps/dav/lib/Files/BrowserErrorPagePlugin.php deleted file mode 100644 index 46598db2040..00000000000 --- a/apps/dav/lib/Files/BrowserErrorPagePlugin.php +++ /dev/null @@ -1,101 +0,0 @@ -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 \Throwable $ex - */ - public function logException(\Throwable $ex): void { - if ($ex instanceof Exception) { - $httpCode = $ex->getHTTPCode(); - $headers = $ex->getHTTPHeaders($this->server); - } else { - $httpCode = 500; - $headers = []; - } - $this->server->httpResponse->addHeaders($headers); - $this->server->httpResponse->setStatus($httpCode); - $body = $this->generateBody($httpCode); - $this->server->httpResponse->setBody($body); - $csp = new ContentSecurityPolicy(); - $this->server->httpResponse->addHeader('Content-Security-Policy', $csp->buildPolicy()); - $this->sendResponse(); - } - - /** - * @codeCoverageIgnore - * @return bool|string - */ - public function generateBody(int $httpCode) { - $request = \OC::$server->getRequest(); - - $templateName = 'exception'; - if ($httpCode === 403 || $httpCode === 404) { - $templateName = (string)$httpCode; - } - - $content = new OC_Template('core', $templateName, 'guest'); - $content->assign('title', $this->server->httpResponse->getStatusText()); - $content->assign('remoteAddr', $request->getRemoteAddress()); - $content->assign('requestID', $request->getId()); - return $content->fetchPage(); - } - - /** - * @codeCoverageIgnore - */ - public function sendResponse() { - $this->server->sapi->sendResponse($this->server->httpResponse); - exit(); - } -} diff --git a/apps/dav/lib/Files/ErrorPagePlugin.php b/apps/dav/lib/Files/ErrorPagePlugin.php new file mode 100644 index 00000000000..ddf04d0f763 --- /dev/null +++ b/apps/dav/lib/Files/ErrorPagePlugin.php @@ -0,0 +1,107 @@ +server = $server; + $server->on('exception', [$this, 'logException'], 1000); + } + + public function logException(\Throwable $ex): void { + if ($ex instanceof Exception) { + $httpCode = $ex->getHTTPCode(); + $headers = $ex->getHTTPHeaders($this->server); + } else { + $httpCode = 500; + $headers = []; + } + $this->server->httpResponse->addHeaders($headers); + $this->server->httpResponse->setStatus($httpCode); + $body = $this->generateBody($ex, $httpCode); + $this->server->httpResponse->setBody($body); + $csp = new ContentSecurityPolicy(); + $this->server->httpResponse->addHeader('Content-Security-Policy', $csp->buildPolicy()); + $this->sendResponse(); + } + + /** + * @codeCoverageIgnore + * @return bool|string + */ + 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'); + } + + $debug = $this->config->getSystemValueBool('debug', false); + + $content = new OC_Template('core', $templateName, $renderAs); + $content->assign('title', $this->server->httpResponse->getStatusText()); + $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(); + } + + /** + * @codeCoverageIgnore + */ + public function sendResponse() { + $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; + } +} diff --git a/apps/dav/lib/Server.php b/apps/dav/lib/Server.php index 449b8ef0ceb..55c8a5afec1 100644 --- a/apps/dav/lib/Server.php +++ b/apps/dav/lib/Server.php @@ -43,7 +43,7 @@ use OCA\DAV\DAV\PublicAuth; use OCA\DAV\DAV\ViewOnlyPlugin; use OCA\DAV\Events\SabrePluginAddEvent; 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; @@ -221,9 +221,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)); diff --git a/apps/dav/tests/travis/caldavtest/tests/CalDAV/sync-report.xml b/apps/dav/tests/travis/caldavtest/tests/CalDAV/sync-report.xml index fda5a5f3fe6..ff4c0c170cd 100644 --- a/apps/dav/tests/travis/caldavtest/tests/CalDAV/sync-report.xml +++ b/apps/dav/tests/travis/caldavtest/tests/CalDAV/sync-report.xml @@ -2701,7 +2701,7 @@ prepostcondition error - {DAV:}valid-sync-token + {http://sabredav.org/ns}exception ignoreextras diff --git a/apps/dav/tests/unit/DAV/BrowserErrorPagePluginTest.php b/apps/dav/tests/unit/DAV/BrowserErrorPagePluginTest.php deleted file mode 100644 index 088330cecff..00000000000 --- a/apps/dav/tests/unit/DAV/BrowserErrorPagePluginTest.php +++ /dev/null @@ -1,44 +0,0 @@ -getMockBuilder(BrowserErrorPagePlugin::class)->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 */ - $server = $this->getMockBuilder('Sabre\DAV\Server')->disableOriginalConstructor()->getMock(); - $server->expects($this->once())->method('on'); - $httpResponse = $this->getMockBuilder(Response::class)->disableOriginalConstructor()->getMock(); - $httpResponse->expects($this->once())->method('addHeaders'); - $httpResponse->expects($this->once())->method('setStatus')->with($expectedCode); - $httpResponse->expects($this->once())->method('setBody')->with(':boom:'); - $server->httpResponse = $httpResponse; - $plugin->initialize($server); - $plugin->logException($exception); - } - - public function providesExceptions() { - return [ - [ 404, new NotFound()], - [ 500, new \RuntimeException()], - ]; - } -} diff --git a/apps/dav/tests/unit/DAV/ErrorPagePluginTest.php b/apps/dav/tests/unit/DAV/ErrorPagePluginTest.php new file mode 100644 index 00000000000..a3569f34bd7 --- /dev/null +++ b/apps/dav/tests/unit/DAV/ErrorPagePluginTest.php @@ -0,0 +1,44 @@ +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 */ + $server = $this->getMockBuilder('Sabre\DAV\Server')->disableOriginalConstructor()->getMock(); + $server->expects($this->once())->method('on'); + $httpResponse = $this->getMockBuilder(Response::class)->disableOriginalConstructor()->getMock(); + $httpResponse->expects($this->once())->method('addHeaders'); + $httpResponse->expects($this->once())->method('setStatus')->with($expectedCode); + $httpResponse->expects($this->once())->method('setBody')->with(':boom:'); + $server->httpResponse = $httpResponse; + $plugin->initialize($server); + $plugin->logException($exception); + } + + public function providesExceptions() { + return [ + [ 404, new NotFound()], + [ 500, new \RuntimeException()], + ]; + } +} diff --git a/build/integration/dav_features/caldav.feature b/build/integration/dav_features/caldav.feature index 031685b580d..aa1e3f6bde8 100644 --- a/build/integration/dav_features/caldav.feature +++ b/build/integration/dav_features/caldav.feature @@ -5,8 +5,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 @@ -14,8 +13,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 @@ -30,8 +28,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 @@ -44,8 +41,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" @@ -66,8 +62,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 @@ -75,8 +70,7 @@ 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" Scenario: Update a principal's schedule-default-calendar-URL Given user "user0" exists diff --git a/build/integration/dav_features/carddav.feature b/build/integration/dav_features/carddav.feature index ffee11a284f..5b285dba2f8 100644 --- a/build/integration/dav_features/carddav.feature +++ b/build/integration/dav_features/carddav.feature @@ -4,15 +4,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 @@ -30,8 +28,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" @@ -69,13 +66,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" diff --git a/core/templates/xml_exception.php b/core/templates/xml_exception.php new file mode 100644 index 00000000000..d7fb276a730 --- /dev/null +++ b/core/templates/xml_exception.php @@ -0,0 +1,47 @@ +getTraceAsString()); + + if ($e->getPrevious() !== null) { + print_unescaped(''); + print_exception($e->getPrevious(), $l); + print_unescaped(''); + } +} + +?> + + + t('Internal Server Error')) ?> + + t('The server was unable to complete your request.')) ?> + t('If this happens again, please send the technical details below to the server administrator.')) ?> + t('More details can be found in the server log.')) ?> + + t('For more details see the documentation ↗.'))?>: + + + + + + + + + + + + + + + + + + + + -- cgit v1.2.3