Skip to content

[stable29] Bug/48678/restore dav error response #49483

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.

Sign up for GitHub

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

Merged
kesselb merged 6 commits into from
Feb 15, 2025
Merged

[stable29] Bug/48678/restore dav error response #49483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aa30a1b
Revert "fix: Override start method of \Sabre\DAV\Server to remove exc…
kesselb Oct 30, 2024
13a3e5e
Revert "fix: Drop unnecessary exit"
kesselb Oct 30, 2024
1794829
Revert "fix(dav): Always respond custom error page on exceptions"
kesselb Oct 30, 2024
df590d3
fix: Replace the TypeError to prevent exposing the installation path
kesselb Nov 22, 2024
e020cbb
fix: make locked exception path relative to the view
icewind1991 Jan 28, 2025
f01f393
fix: use relative paths for upload locks
icewind1991 Feb 3, 2025
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,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\\ErrorPagePlugin' => $baseDir . '/../lib/Files/ErrorPagePlugin.php',
'OCA\\DAV\\Files\\BrowserErrorPagePlugin' => $baseDir . '/../lib/Files/BrowserErrorPagePlugin.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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,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\\ErrorPagePlugin' => __DIR__ . '/..' . '/../lib/Files/ErrorPagePlugin.php',
'OCA\\DAV\\Files\\BrowserErrorPagePlugin' => __DIR__ . '/..' . '/../lib/Files/BrowserErrorPagePlugin.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
4 changes: 2 additions & 2 deletions apps/dav/lib/Connector/Sabre/Directory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,11 @@ public function createFile($name, $data = null) {

// only allow 1 process to upload a file at once but still allow reading the file while writing the part file
$node->acquireLock(ILockingProvider::LOCK_SHARED);
$this->fileView->lockFile($path . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
$this->fileView->lockFile($this->path . '/' . $name . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);

$result = $node->put($data);

$this->fileView->unlockFile($path . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
$this->fileView->unlockFile($this->path . '/' . $name . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
$node->releaseLock(ILockingProvider::LOCK_SHARED);
return $result;
} catch (\OCP\Files\StorageNotAvailableException $e) {
Expand Down
75 changes: 71 additions & 4 deletions apps/dav/lib/Connector/Sabre/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@
*/
namespace OCA\DAV\Connector\Sabre;

use Sabre\DAV\Exception;
use Sabre\DAV\Version;
use TypeError;

/**
* Class \OCA\DAV\Connector\Sabre\Server
*
Expand All @@ -44,9 +48,11 @@
$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 {
/**
*
* @return void
*/
public function start() {
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
Expand All @@ -62,8 +68,69 @@
} catch (\Throwable $e) {
try {
$this->emit('exception', [$e]);
} catch (\Exception $ignore) {
} catch (\Exception) {
}

if ($e instanceof TypeError) {
/*
* The TypeError includes the file path where the error occurred,
* potentially revealing the installation directory.
*/
$e = new TypeError('A type error occurred. For more details, please refer to the logs, which provide additional context about the type error.');
}

$DOM = new \DOMDocument('1.0', 'utf-8');
$DOM->formatOutput = true;

$error = $DOM->createElementNS('DAV:', 'd:error');
$error->setAttribute('xmlns:s', self::NS_SABREDAV);
$DOM->appendChild($error);

$h = function ($v) {

Check notice

Code scanning / Psalm

MissingClosureReturnType Note

Closure does not have a return type, expecting string

Check notice

Code scanning / Psalm

MissingClosureParamType Note

Parameter $v has no provided type
return htmlspecialchars((string)$v, ENT_NOQUOTES, 'UTF-8');
};

if (self::$exposeVersion) {
$error->appendChild($DOM->createElement('s:sabredav-version', $h(Version::VERSION)));
}

$error->appendChild($DOM->createElement('s:exception', $h(get_class($e))));
$error->appendChild($DOM->createElement('s:message', $h($e->getMessage())));
if ($this->debugExceptions) {
$error->appendChild($DOM->createElement('s:file', $h($e->getFile())));
$error->appendChild($DOM->createElement('s:line', $h($e->getLine())));
$error->appendChild($DOM->createElement('s:code', $h($e->getCode())));
$error->appendChild($DOM->createElement('s:stacktrace', $h($e->getTraceAsString())));
}

if ($this->debugExceptions) {
$previous = $e;
while ($previous = $previous->getPrevious()) {
$xPrevious = $DOM->createElement('s:previous-exception');
$xPrevious->appendChild($DOM->createElement('s:exception', $h(get_class($previous))));
$xPrevious->appendChild($DOM->createElement('s:message', $h($previous->getMessage())));
$xPrevious->appendChild($DOM->createElement('s:file', $h($previous->getFile())));
$xPrevious->appendChild($DOM->createElement('s:line', $h($previous->getLine())));
$xPrevious->appendChild($DOM->createElement('s:code', $h($previous->getCode())));
$xPrevious->appendChild($DOM->createElement('s:stacktrace', $h($previous->getTraceAsString())));
$error->appendChild($xPrevious);
}
}

if ($e instanceof Exception) {
$httpCode = $e->getHTTPCode();
$e->serialize($this, $error);
$headers = $e->getHTTPHeaders($this);
} else {
$httpCode = 500;
$headers = [];
}
$headers['Content-Type'] = 'application/xml; charset=utf-8';

$this->httpResponse->setStatus($httpCode);
$this->httpResponse->setHeaders($headers);
$this->httpResponse->setBody($DOM->saveXML());
$this->sapi->sendResponse($this->httpResponse);
}
}
}
7 changes: 5 additions & 2 deletions apps/dav/lib/Connector/Sabre/ServerFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,11 @@
*/
namespace OCA\DAV\Connector\Sabre;

use OC\Files\View;
use OCA\DAV\AppInfo\PluginManager;
use OCA\DAV\CalDAV\DefaultCalendarValidator;
use OCA\DAV\DAV\ViewOnlyPlugin;
use OCA\DAV\Files\ErrorPagePlugin;
use OCA\DAV\Files\BrowserErrorPagePlugin;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\Folder;
use OCP\Files\Mount\IMountManager;
Expand Down Expand Up @@ -121,7 +122,9 @@ public function createServer(string $baseUri,
$server->addPlugin(new \OCA\DAV\Connector\Sabre\FakeLockerPlugin());
}

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

// wait with registering these until auth is handled and the filesystem is setup
$server->on('beforeMethod:*', function () use ($server, $objectTree, $viewCallBack) {
Expand Down
83 changes: 39 additions & 44 deletions apps/dav/lib/Files/ErrorPagePlugin.php → .../dav/lib/Files/BrowserErrorPagePlugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,17 @@
*/
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 ErrorPagePlugin extends ServerPlugin {
private ?Server $server = null;

public function __construct(
private IRequest $request,
private IConfig $config,
) {
}
class BrowserErrorPagePlugin extends ServerPlugin {
/** @var Server */
private $server;

Check notice

Code scanning / Psalm

MissingConstructor Note

OCA\DAV\Files\BrowserErrorPagePlugin has an uninitialized property OCA\DAV\Files\BrowserErrorPagePlugin::$server, but no constructor

/**
* This initializes the plugin.
Expand All @@ -48,12 +43,35 @@
* addPlugin is called.
*
* This method should set up the required event subscriptions.
*
* @param Server $server
* @return void
*/
public function initialize(Server $server): void {
public function initialize(Server $server) {
$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 \Throwable $ex
*/
public function logException(\Throwable $ex): void {
if ($ex instanceof Exception) {
$httpCode = $ex->getHTTPCode();
Expand All @@ -64,7 +82,7 @@
}
$this->server->httpResponse->addHeaders($headers);
$this->server->httpResponse->setStatus($httpCode);
$body = $this->generateBody($ex, $httpCode);
$body = $this->generateBody($httpCode);
$this->server->httpResponse->setBody($body);
$csp = new ContentSecurityPolicy();
$this->server->httpResponse->addHeader('Content-Security-Policy', $csp->buildPolicy());
Expand All @@ -75,32 +93,18 @@
* @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');
}
public function generateBody(int $httpCode) {
$request = \OC::$server->getRequest();

Check notice

Code scanning / Psalm

DeprecatedMethod Note

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

$debug = $this->config->getSystemValueBool('debug', false);
$templateName = 'exception';
if ($httpCode === 403 || $httpCode === 404) {
$templateName = (string)$httpCode;
}

$content = new OC_Template('core', $templateName, $renderAs);
$content = new OC_Template('core', $templateName, 'guest');
$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);
$content->assign('remoteAddr', $request->getRemoteAddress());
$content->assign('requestID', $request->getId());
return $content->fetchPage();
}

Expand All @@ -109,15 +113,6 @@
*/
public function sendResponse() {
$this->server->sapi->sendResponse($this->server->httpResponse);
}

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;
exit();
}
}
7 changes: 5 additions & 2 deletions apps/dav/lib/Server.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
*/
namespace OCA\DAV;

use OC\Files\Filesystem;
use OCA\DAV\AppInfo\PluginManager;
use OCA\DAV\BulkUpload\BulkUploadPlugin;
use OCA\DAV\CalDAV\BirthdayService;
Expand Down Expand Up @@ -73,7 +74,7 @@
use OCA\DAV\DAV\ViewOnlyPlugin;
use OCA\DAV\Events\SabrePluginAddEvent;
use OCA\DAV\Events\SabrePluginAuthInitEvent;
use OCA\DAV\Files\ErrorPagePlugin;
use OCA\DAV\Files\BrowserErrorPagePlugin;
use OCA\DAV\Files\LazySearchBackend;
use OCA\DAV\Profiler\ProfilerPlugin;
use OCA\DAV\Provisioning\Apple\AppleProvisioningPlugin;
Expand Down Expand Up @@ -247,7 +248,9 @@ public function __construct(IRequest $request, string $baseUri) {
$this->server->addPlugin(new FakeLockerPlugin());
}

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

$lazySearchBackend = new LazySearchBackend();
$this->server->addPlugin(new SearchPlugin($lazySearchBackend));
Expand Down
2 changes: 1 addition & 1 deletion apps/dav/tests/travis/caldavtest/tests/CalDAV/sync-report.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2712,7 +2712,7 @@
<callback>prepostcondition</callback>
<arg>
<name>error</name>
<value>{http://sabredav.org/ns}exception</value>
<value>{DAV:}valid-sync-token</value>
</arg>
<arg>
<name>ignoreextras</name>
Expand Down
8 changes: 4 additions & 4 deletions ...av/tests/unit/DAV/ErrorPagePluginTest.php → ...s/unit/DAV/BrowserErrorPagePluginTest.php
View file
Open in desktop
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\ErrorPagePlugin;
use OCA\DAV\Files\BrowserErrorPagePlugin;
use Sabre\DAV\Exception\NotFound;
use Sabre\HTTP\Response;

class ErrorPagePluginTest extends \Test\TestCase {
class BrowserErrorPagePluginTest extends \Test\TestCase {

/**
* @dataProvider providesExceptions
* @param $expectedCode
* @param $exception
*/
public function test($expectedCode, $exception): void {
/** @var ErrorPagePlugin | \PHPUnit\Framework\MockObject\MockObject $plugin */
$plugin = $this->getMockBuilder(ErrorPagePlugin::class)->disableOriginalConstructor()->setMethods(['sendResponse', 'generateBody'])->getMock();
/** @var BrowserErrorPagePlugin | \PHPUnit\Framework\MockObject\MockObject $plugin */
$plugin = $this->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 */
Expand Down
Loading
Loading