Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable25] fix(dav): Always respond custom error page on exceptions #48304

Merged
merged 5 commits into from
Oct 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 @@ -234,7 +234,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 @@ -249,7 +249,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
118 changes: 0 additions & 118 deletions apps/dav/lib/Files/BrowserErrorPagePlugin.php

This file was deleted.

127 changes: 127 additions & 0 deletions apps/dav/lib/Files/ErrorPagePlugin.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
<?php
/**
* @copyright Copyright (c) 2016, ownCloud, Inc.
*
* @author Arthur Schiwon <blizzz@arthur-schiwon.de>
* @author Christoph Wurst <christoph@winzerhof-wurst.at>
* @author Lukas Reschke <lukas@statuscode.ch>
* @author Thomas Müller <thomas.mueller@tmit.eu>
*
* @license AGPL-3.0
*
* This code is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License, version 3,
* as published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License, version 3,
* along with this program. If not, see <http://www.gnu.org/licenses/>
*
*/
namespace OCA\DAV\Files;

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 ErrorPagePlugin extends ServerPlugin {
private ?Server $server = null;
private IRequest $request;
private IConfig $config;

public function __construct(
IRequest $request,
IConfig $config
) {
$this->request = $request;
$this->config = $config;
}

/**
* This initializes the plugin.
*
* This function is called by Sabre\DAV\Server, after
* addPlugin is called.
*
* This method should set up the required event subscriptions.
*/
public function initialize(Server $server): void {
$this->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);

Check notice

Code scanning / Psalm

PossiblyNullArgument Note

Argument 1 of Sabre\DAV\Exception::getHTTPHeaders cannot be null, possibly null value provided
} else {
$httpCode = 500;
$headers = [];
}
$this->server->httpResponse->addHeaders($headers);

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

Cannot get property on possibly null variable $this->server of type Sabre\DAV\Server|null

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method addHeaders on possibly null value
$this->server->httpResponse->setStatus($httpCode);
$body = $this->generateBody($ex, $httpCode);
$this->server->httpResponse->setBody($body);
Fixed Show fixed Hide fixed
$csp = new ContentSecurityPolicy();
$this->server->httpResponse->addHeader('Content-Security-Policy', $csp->buildPolicy());
$this->sendResponse();
}

/**
* @codeCoverageIgnore
* @return string
*/
public function generateBody(\Throwable $ex, int $httpCode) {
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');

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

Cannot get property on possibly null variable $this->server of type Sabre\DAV\Server|null

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method setHeader on possibly null value
}

$debug = $this->config->getSystemValueBool('debug', false);

$content = new OC_Template('core', $templateName, $renderAs);

Check notice

Code scanning / Psalm

PossiblyNullArgument Note

Argument 3 of OC_Template::__construct cannot be null, possibly null value provided
$content->assign('title', $this->server->httpResponse->getStatusText());

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

Cannot get property on possibly null variable $this->server of type Sabre\DAV\Server|null

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method getStatusText on possibly null value
$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() {

Check notice

Code scanning / Psalm

MissingReturnType Note

Method OCA\DAV\Files\ErrorPagePlugin::sendResponse does not have a return type, expecting void
$this->server->sapi->sendResponse($this->server->httpResponse);

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

Cannot get property on possibly null variable $this->server of type Sabre\DAV\Server|null

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method sendResponse on possibly null value

Check notice

Code scanning / Psalm

PossiblyNullPropertyFetch Note

Cannot get property on possibly null variable $this->server of type Sabre\DAV\Server|null

Check notice

Code scanning / Psalm

PossiblyNullArgument Note

Argument 1 of Sabre\HTTP\Sapi::sendResponse cannot be null, possibly null value provided
}

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 @@ -66,7 +66,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 @@ -236,9 +236,7 @@
$this->server->addPlugin(new FakeLockerPlugin());
}

if (BrowserErrorPagePlugin::isBrowserRequest($request)) {
$this->server->addPlugin(new BrowserErrorPagePlugin());
}
$this->server->addPlugin(new ErrorPagePlugin($this->request, \OC::$server->getConfig()));

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OC\Server::getConfig has been marked as deprecated

$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) {
/** @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
Loading
Loading