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

Revert: Restore dav error response #49960

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
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 @@ -277,7 +277,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 @@ -292,7 +292,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
75 changes: 3 additions & 72 deletions apps/dav/lib/Connector/Sabre/Server.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@
*/
namespace OCA\DAV\Connector\Sabre;

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

/**
* Class \OCA\DAV\Connector\Sabre\Server
*
Expand All @@ -29,11 +26,9 @@ public function __construct($treeOrNode = null) {
$this->enablePropfindDepthInfinity = true;
}

/**
*
* @return void
*/
public function start() {
// 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
Expand All @@ -47,74 +42,10 @@ public function start() {
$this->httpRequest->setBaseUrl($this->getBaseUri());
$this->invokeMethod($this->httpRequest, $this->httpResponse);
} catch (\Throwable $e) {
if ($e instanceof \TypeError) {
/*
* The TypeError includes the file path where the error occurred,
* potentially revealing the installation directory.
*
* By re-throwing the exception, we ensure that the
* default exception handler processes it.
*/
throw $e;
}

try {
$this->emit('exception', [$e]);
} catch (\Exception $ignore) {
}

$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) {
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);
}
}
}
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 @@ -12,7 +12,7 @@
use OCA\DAV\CalDAV\DefaultCalendarValidator;
use OCA\DAV\DAV\CustomPropertiesBackend;
use OCA\DAV\DAV\ViewOnlyPlugin;
use OCA\DAV\Files\BrowserErrorPagePlugin;
use OCA\DAV\Files\ErrorPagePlugin;
use OCA\Theming\ThemingDefaults;
use OCP\EventDispatcher\IEventDispatcher;
use OCP\Files\Folder;
Expand Down Expand Up @@ -90,9 +90,7 @@ public function createServer(string $baseUri,
$server->addPlugin(new 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): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,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 @@ -26,35 +31,12 @@ 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 \Throwable $ex
*/
public function logException(\Throwable $ex): void {
if ($ex instanceof Exception) {
$httpCode = $ex->getHTTPCode();
Expand All @@ -65,7 +47,7 @@ public function logException(\Throwable $ex): void {
}
$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 @@ -76,18 +58,32 @@ public function logException(\Throwable $ex): void {
* @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 @@ -96,6 +92,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 @@ -54,7 +54,7 @@
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\FileSearchBackend;
use OCA\DAV\Files\LazySearchBackend;
use OCA\DAV\Paginate\PaginatePlugin;
Expand Down Expand Up @@ -246,9 +246,7 @@ public function __construct(
$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 @@ -2701,7 +2701,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 @@ -7,20 +7,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/dav_features/caldav.feature
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,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 @@ -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
Expand All @@ -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"
Expand All @@ -66,17 +62,15 @@ 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"

Scenario: Update a principal's schedule-default-calendar-URL
Given user "user0" exists
Expand Down
Loading
Loading