Skip to content

Commit

Permalink
Merge pull request #48297 from nextcloud/artonge/backport/stable27/47770
Browse files Browse the repository at this point in the history
[stable27] fix(dav): Always respond custom error page on exceptions
  • Loading branch information
sorbaugh authored Oct 1, 2024
2 parents 041c8e2 + 6b026d8 commit f77dd11
Show file tree
Hide file tree
Showing 14 changed files with 167 additions and 91 deletions.
2 changes: 1 addition & 1 deletion apps/dav/composer/composer/autoload_classmap.php
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@
'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',
Expand Down
2 changes: 1 addition & 1 deletion apps/dav/composer/composer/autoload_static.php
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
23 changes: 23 additions & 0 deletions apps/dav/lib/Connector/Sabre/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,27 @@ public function __construct($treeOrNode = null) {
self::$exposeVersion = false;
$this->enablePropfindDepthInfinity = true;
}

// Copied from 3rdparty/sabre/dav/lib/DAV/Server.php
// Should be them exact same without the exception output.
public function start(): void {
try {
// If nginx (pre-1.2) is used as a proxy server, and SabreDAV as an
// origin, we must make sure we send back HTTP/1.0 if this was
// requested.
// This is mainly because nginx doesn't support Chunked Transfer
// Encoding, and this forces the webserver SabreDAV is running on,
// to buffer entire responses to calculate Content-Length.
$this->httpResponse->setHTTPVersion($this->httpRequest->getHTTPVersion());

// Setting the base url
$this->httpRequest->setBaseUrl($this->getBaseUri());
$this->invokeMethod($this->httpRequest, $this->httpResponse);
} catch (\Throwable $e) {
try {
$this->emit('exception', [$e]);
} catch (\Exception $ignore) {
}
}
}
}
6 changes: 2 additions & 4 deletions apps/dav/lib/Connector/Sabre/ServerFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
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;
Expand Down Expand Up @@ -120,9 +120,7 @@ public function createServer(string $baseUri,
$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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
Expand All @@ -82,7 +64,7 @@ public function logException(\Exception $ex) {
}
$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());
Expand All @@ -93,18 +75,32 @@ public function logException(\Exception $ex) {
* @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();
}

Expand All @@ -113,6 +109,15 @@ public function generateBody(int $httpCode) {
*/
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;
}
}
6 changes: 2 additions & 4 deletions apps/dav/lib/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
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;
Expand Down Expand Up @@ -244,9 +244,7 @@ public function __construct(IRequest $request, string $baseUri) {
$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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,20 @@
*/
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
* @param $expectedCode
* @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 */
Expand Down
18 changes: 6 additions & 12 deletions build/integration/features/caldav.feature
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,15 @@ 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
Given "admin" creates a calendar named "MyCalendar"
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
Expand All @@ -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
Expand All @@ -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"
Expand All @@ -64,14 +60,12 @@ 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
When "admin" creates a calendar named "MyCalendar2"
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"
15 changes: 5 additions & 10 deletions build/integration/features/carddav.feature
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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"
Expand Down Expand Up @@ -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"
15 changes: 1 addition & 14 deletions core/templates/exception.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,7 @@

style('core', ['styles', 'header']);

function print_exception(Throwable $e, \OCP\IL10N $l): void {
print_unescaped('<pre>');
p($e->getTraceAsString());
print_unescaped('</pre>');

if ($e->getPrevious() !== null) {
print_unescaped('<br />');
print_unescaped('<h4>');
p($l->t('Previous'));
print_unescaped('</h4>');

print_exception($e->getPrevious(), $l);
}
}
require_once __DIR__ . '/print_exception.php';

?>
<div class="guest-box wide">
Expand Down
21 changes: 21 additions & 0 deletions core/templates/print_exception.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?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 {
print_unescaped('<pre>');
p($e->getTraceAsString());
print_unescaped('</pre>');

if ($e->getPrevious() !== null) {
print_unescaped('<br />');
print_unescaped('<h4>');
p($l->t('Previous'));
print_unescaped('</h4>');

print_exception($e->getPrevious(), $l);
}
}
Loading

0 comments on commit f77dd11

Please sign in to comment.