Skip to content

Commit 3217953

Browse files
blizzzblizzz
authored
Merge pull request #49482 from nextcloud/backport/49004/stable28
[stable28] Bug/48678/restore dav error response
2 parents a89e6f1 + 67b013b commit 3217953

File tree

12 files changed

+159
-88
lines changed

12 files changed

+159
-88
lines changed

‎apps/dav/composer/composer/autoload_classmap.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@
258258
'OCA\\DAV\\Events\\SubscriptionUpdatedEvent' => $baseDir . '/../lib/Events/SubscriptionUpdatedEvent.php',
259259
'OCA\\DAV\\Exception\\ServerMaintenanceMode' => $baseDir . '/../lib/Exception/ServerMaintenanceMode.php',
260260
'OCA\\DAV\\Exception\\UnsupportedLimitOnInitialSyncException' => $baseDir . '/../lib/Exception/UnsupportedLimitOnInitialSyncException.php',
261-
'OCA\\DAV\\Files\\ErrorPagePlugin' => $baseDir . '/../lib/Files/ErrorPagePlugin.php',
261+
'OCA\\DAV\\Files\\BrowserErrorPagePlugin' => $baseDir . '/../lib/Files/BrowserErrorPagePlugin.php',
262262
'OCA\\DAV\\Files\\FileSearchBackend' => $baseDir . '/../lib/Files/FileSearchBackend.php',
263263
'OCA\\DAV\\Files\\FilesHome' => $baseDir . '/../lib/Files/FilesHome.php',
264264
'OCA\\DAV\\Files\\LazySearchBackend' => $baseDir . '/../lib/Files/LazySearchBackend.php',

‎apps/dav/composer/composer/autoload_static.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ class ComposerStaticInitDAV
273273
'OCA\\DAV\\Events\\SubscriptionUpdatedEvent' => __DIR__ . '/..' . '/../lib/Events/SubscriptionUpdatedEvent.php',
274274
'OCA\\DAV\\Exception\\ServerMaintenanceMode' => __DIR__ . '/..' . '/../lib/Exception/ServerMaintenanceMode.php',
275275
'OCA\\DAV\\Exception\\UnsupportedLimitOnInitialSyncException' => __DIR__ . '/..' . '/../lib/Exception/UnsupportedLimitOnInitialSyncException.php',
276-
'OCA\\DAV\\Files\\ErrorPagePlugin' => __DIR__ . '/..' . '/../lib/Files/ErrorPagePlugin.php',
276+
'OCA\\DAV\\Files\\BrowserErrorPagePlugin' => __DIR__ . '/..' . '/../lib/Files/BrowserErrorPagePlugin.php',
277277
'OCA\\DAV\\Files\\FileSearchBackend' => __DIR__ . '/..' . '/../lib/Files/FileSearchBackend.php',
278278
'OCA\\DAV\\Files\\FilesHome' => __DIR__ . '/..' . '/../lib/Files/FilesHome.php',
279279
'OCA\\DAV\\Files\\LazySearchBackend' => __DIR__ . '/..' . '/../lib/Files/LazySearchBackend.php',

‎apps/dav/lib/Connector/Sabre/Directory.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,11 +143,11 @@ public function createFile($name, $data = null) {
143143

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

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

150-
$this->fileView->unlockFile($path . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
150+
$this->fileView->unlockFile($this->path . '/' . $name . '.upload.part', ILockingProvider::LOCK_EXCLUSIVE);
151151
$node->releaseLock(ILockingProvider::LOCK_SHARED);
152152
return $result;
153153
} catch (\OCP\Files\StorageNotAvailableException $e) {

‎apps/dav/lib/Connector/Sabre/Server.php‎

Lines changed: 71 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,10 @@
2525
*/
2626
namespace OCA\DAV\Connector\Sabre;
2727

28+
use Sabre\DAV\Exception;
29+
use Sabre\DAV\Version;
30+
use TypeError;
31+
2832
/**
2933
* Class \OCA\DAV\Connector\Sabre\Server
3034
*
@@ -44,9 +48,11 @@ public function __construct($treeOrNode = null) {
4448
$this->enablePropfindDepthInfinity = true;
4549
}
4650

47-
// Copied from 3rdparty/sabre/dav/lib/DAV/Server.php
48-
// Should be them exact same without the exception output.
49-
public function start(): void {
51+
/**
52+
*
53+
* @return void
54+
*/
55+
public function start() {
5056
try {
5157
// If nginx (pre-1.2) is used as a proxy server, and SabreDAV as an
5258
// origin, we must make sure we send back HTTP/1.0 if this was
@@ -62,8 +68,69 @@ public function start(): void {
6268
} catch (\Throwable $e) {
6369
try {
6470
$this->emit('exception', [$e]);
65-
} catch (\Exception $ignore) {
71+
} catch (\Exception) {
72+
}
73+
74+
if ($e instanceof TypeError) {
75+
/*
76+
* The TypeError includes the file path where the error occurred,
77+
* potentially revealing the installation directory.
78+
*/
79+
$e = new TypeError('A type error occurred. For more details, please refer to the logs, which provide additional context about the type error.');
6680
}
81+
82+
$DOM = new \DOMDocument('1.0', 'utf-8');
83+
$DOM->formatOutput = true;
84+
85+
$error = $DOM->createElementNS('DAV:', 'd:error');
86+
$error->setAttribute('xmlns:s', self::NS_SABREDAV);
87+
$DOM->appendChild($error);
88+
89+
$h = function ($v) {
90+
return htmlspecialchars((string)$v, ENT_NOQUOTES, 'UTF-8');
91+
};
92+
93+
if (self::$exposeVersion) {
94+
$error->appendChild($DOM->createElement('s:sabredav-version', $h(Version::VERSION)));
95+
}
96+
97+
$error->appendChild($DOM->createElement('s:exception', $h(get_class($e))));
98+
$error->appendChild($DOM->createElement('s:message', $h($e->getMessage())));
99+
if ($this->debugExceptions) {
100+
$error->appendChild($DOM->createElement('s:file', $h($e->getFile())));
101+
$error->appendChild($DOM->createElement('s:line', $h($e->getLine())));
102+
$error->appendChild($DOM->createElement('s:code', $h($e->getCode())));
103+
$error->appendChild($DOM->createElement('s:stacktrace', $h($e->getTraceAsString())));
104+
}
105+
106+
if ($this->debugExceptions) {
107+
$previous = $e;
108+
while ($previous = $previous->getPrevious()) {
109+
$xPrevious = $DOM->createElement('s:previous-exception');
110+
$xPrevious->appendChild($DOM->createElement('s:exception', $h(get_class($previous))));
111+
$xPrevious->appendChild($DOM->createElement('s:message', $h($previous->getMessage())));
112+
$xPrevious->appendChild($DOM->createElement('s:file', $h($previous->getFile())));
113+
$xPrevious->appendChild($DOM->createElement('s:line', $h($previous->getLine())));
114+
$xPrevious->appendChild($DOM->createElement('s:code', $h($previous->getCode())));
115+
$xPrevious->appendChild($DOM->createElement('s:stacktrace', $h($previous->getTraceAsString())));
116+
$error->appendChild($xPrevious);
117+
}
118+
}
119+
120+
if ($e instanceof Exception) {
121+
$httpCode = $e->getHTTPCode();
122+
$e->serialize($this, $error);
123+
$headers = $e->getHTTPHeaders($this);
124+
} else {
125+
$httpCode = 500;
126+
$headers = [];
127+
}
128+
$headers['Content-Type'] = 'application/xml; charset=utf-8';
129+
130+
$this->httpResponse->setStatus($httpCode);
131+
$this->httpResponse->setHeaders($headers);
132+
$this->httpResponse->setBody($DOM->saveXML());
133+
$this->sapi->sendResponse($this->httpResponse);
67134
}
68135
}
69136
}

‎apps/dav/lib/Connector/Sabre/ServerFactory.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,10 @@
3131
*/
3232
namespace OCA\DAV\Connector\Sabre;
3333

34+
use OC\Files\View;
3435
use OCA\DAV\AppInfo\PluginManager;
3536
use OCA\DAV\DAV\ViewOnlyPlugin;
36-
use OCA\DAV\Files\ErrorPagePlugin;
37+
use OCA\DAV\Files\BrowserErrorPagePlugin;
3738
use OCP\EventDispatcher\IEventDispatcher;
3839
use OCP\Files\Folder;
3940
use OCP\Files\Mount\IMountManager;
@@ -120,7 +121,9 @@ public function createServer(string $baseUri,
120121
$server->addPlugin(new \OCA\DAV\Connector\Sabre\FakeLockerPlugin());
121122
}
122123

123-
$server->addPlugin(new ErrorPagePlugin($this->request, $this->config));
124+
if (BrowserErrorPagePlugin::isBrowserRequest($this->request)) {
125+
$server->addPlugin(new BrowserErrorPagePlugin());
126+
}
124127

125128
// wait with registering these until auth is handled and the filesystem is setup
126129
$server->on('beforeMethod:*', function () use ($server, $objectTree, $viewCallBack) {

apps/dav/lib/Files/ErrorPagePlugin.php renamed to apps/dav/lib/Files/BrowserErrorPagePlugin.php

Lines changed: 39 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -24,22 +24,17 @@
2424
*/
2525
namespace OCA\DAV\Files;
2626

27+
use OC\AppFramework\Http\Request;
2728
use OC_Template;
2829
use OCP\AppFramework\Http\ContentSecurityPolicy;
29-
use OCP\IConfig;
3030
use OCP\IRequest;
3131
use Sabre\DAV\Exception;
3232
use Sabre\DAV\Server;
3333
use Sabre\DAV\ServerPlugin;
3434

35-
class ErrorPagePlugin extends ServerPlugin {
36-
private ?Server $server = null;
37-
38-
public function __construct(
39-
private IRequest $request,
40-
private IConfig $config,
41-
) {
42-
}
35+
class BrowserErrorPagePlugin extends ServerPlugin {
36+
/** @var Server */
37+
private $server;
4338

4439
/**
4540
* This initializes the plugin.
@@ -48,12 +43,35 @@ public function __construct(
4843
* addPlugin is called.
4944
*
5045
* This method should set up the required event subscriptions.
46+
*
47+
* @param Server $server
48+
* @return void
5149
*/
52-
public function initialize(Server $server): void {
50+
public function initialize(Server $server) {
5351
$this->server = $server;
5452
$server->on('exception', [$this, 'logException'], 1000);
5553
}
5654

55+
/**
56+
* @param IRequest $request
57+
* @return bool
58+
*/
59+
public static function isBrowserRequest(IRequest $request) {
60+
if ($request->getMethod() !== 'GET') {
61+
return false;
62+
}
63+
return $request->isUserAgent([
64+
Request::USER_AGENT_IE,
65+
Request::USER_AGENT_MS_EDGE,
66+
Request::USER_AGENT_CHROME,
67+
Request::USER_AGENT_FIREFOX,
68+
Request::USER_AGENT_SAFARI,
69+
]);
70+
}
71+
72+
/**
73+
* @param \Throwable $ex
74+
*/
5775
public function logException(\Throwable $ex): void {
5876
if ($ex instanceof Exception) {
5977
$httpCode = $ex->getHTTPCode();
@@ -64,7 +82,7 @@ public function logException(\Throwable $ex): void {
6482
}
6583
$this->server->httpResponse->addHeaders($headers);
6684
$this->server->httpResponse->setStatus($httpCode);
67-
$body = $this->generateBody($ex, $httpCode);
85+
$body = $this->generateBody($httpCode);
6886
$this->server->httpResponse->setBody($body);
6987
$csp = new ContentSecurityPolicy();
7088
$this->server->httpResponse->addHeader('Content-Security-Policy', $csp->buildPolicy());
@@ -75,32 +93,18 @@ public function logException(\Throwable $ex): void {
7593
* @codeCoverageIgnore
7694
* @return bool|string
7795
*/
78-
public function generateBody(\Throwable $ex, int $httpCode): mixed {
79-
if ($this->acceptHtml()) {
80-
$templateName = 'exception';
81-
$renderAs = 'guest';
82-
if ($httpCode === 403 || $httpCode === 404) {
83-
$templateName = (string)$httpCode;
84-
}
85-
} else {
86-
$templateName = 'xml_exception';
87-
$renderAs = null;
88-
$this->server->httpResponse->setHeader('Content-Type', 'application/xml; charset=utf-8');
89-
}
96+
public function generateBody(int $httpCode) {
97+
$request = \OC::$server->getRequest();
9098

91-
$debug = $this->config->getSystemValueBool('debug', false);
99+
$templateName = 'exception';
100+
if ($httpCode === 403 || $httpCode === 404) {
101+
$templateName = (string)$httpCode;
102+
}
92103

93-
$content = new OC_Template('core', $templateName, $renderAs);
104+
$content = new OC_Template('core', $templateName, 'guest');
94105
$content->assign('title', $this->server->httpResponse->getStatusText());
95-
$content->assign('remoteAddr', $this->request->getRemoteAddress());
96-
$content->assign('requestID', $this->request->getId());
97-
$content->assign('debugMode', $debug);
98-
$content->assign('errorClass', get_class($ex));
99-
$content->assign('errorMsg', $ex->getMessage());
100-
$content->assign('errorCode', $ex->getCode());
101-
$content->assign('file', $ex->getFile());
102-
$content->assign('line', $ex->getLine());
103-
$content->assign('exception', $ex);
106+
$content->assign('remoteAddr', $request->getRemoteAddress());
107+
$content->assign('requestID', $request->getId());
104108
return $content->fetchPage();
105109
}
106110

@@ -109,15 +113,6 @@ public function generateBody(\Throwable $ex, int $httpCode): mixed {
109113
*/
110114
public function sendResponse() {
111115
$this->server->sapi->sendResponse($this->server->httpResponse);
112-
}
113-
114-
private function acceptHtml(): bool {
115-
foreach (explode(',', $this->request->getHeader('Accept')) as $part) {
116-
$subparts = explode(';', $part);
117-
if (str_ends_with($subparts[0], '/html')) {
118-
return true;
119-
}
120-
}
121-
return false;
116+
exit();
122117
}
123118
}

‎apps/dav/lib/Server.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
*/
3737
namespace OCA\DAV;
3838

39+
use OC\Files\Filesystem;
3940
use OCA\DAV\AppInfo\PluginManager;
4041
use OCA\DAV\BulkUpload\BulkUploadPlugin;
4142
use OCA\DAV\CalDAV\BirthdayService;
@@ -72,7 +73,7 @@
7273
use OCA\DAV\DAV\ViewOnlyPlugin;
7374
use OCA\DAV\Events\SabrePluginAddEvent;
7475
use OCA\DAV\Events\SabrePluginAuthInitEvent;
75-
use OCA\DAV\Files\ErrorPagePlugin;
76+
use OCA\DAV\Files\BrowserErrorPagePlugin;
7677
use OCA\DAV\Files\LazySearchBackend;
7778
use OCA\DAV\Profiler\ProfilerPlugin;
7879
use OCA\DAV\Provisioning\Apple\AppleProvisioningPlugin;
@@ -246,7 +247,9 @@ public function __construct(IRequest $request, string $baseUri) {
246247
$this->server->addPlugin(new FakeLockerPlugin());
247248
}
248249

249-
$this->server->addPlugin(new ErrorPagePlugin($this->request, \OC::$server->getConfig()));
250+
if (BrowserErrorPagePlugin::isBrowserRequest($request)) {
251+
$this->server->addPlugin(new BrowserErrorPagePlugin());
252+
}
250253

251254
$lazySearchBackend = new LazySearchBackend();
252255
$this->server->addPlugin(new SearchPlugin($lazySearchBackend));

‎apps/dav/tests/travis/caldavtest/tests/CalDAV/sync-report.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2712,7 +2712,7 @@
27122712
<callback>prepostcondition</callback>
27132713
<arg>
27142714
<name>error</name>
2715-
<value>{http://sabredav.org/ns}exception</value>
2715+
<value>{DAV:}valid-sync-token</value>
27162716
</arg>
27172717
<arg>
27182718
<name>ignoreextras</name>

apps/dav/tests/unit/DAV/ErrorPagePluginTest.php renamed to apps/dav/tests/unit/DAV/BrowserErrorPagePluginTest.php

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,20 +23,20 @@
2323
*/
2424
namespace OCA\DAV\Tests\unit\DAV;
2525

26-
use OCA\DAV\Files\ErrorPagePlugin;
26+
use OCA\DAV\Files\BrowserErrorPagePlugin;
2727
use Sabre\DAV\Exception\NotFound;
2828
use Sabre\HTTP\Response;
2929

30-
class ErrorPagePluginTest extends \Test\TestCase {
30+
class BrowserErrorPagePluginTest extends \Test\TestCase {
3131

3232
/**
3333
* @dataProvider providesExceptions
3434
* @param $expectedCode
3535
* @param $exception
3636
*/
3737
public function test($expectedCode, $exception): void {
38-
/** @var ErrorPagePlugin | \PHPUnit\Framework\MockObject\MockObject $plugin */
39-
$plugin = $this->getMockBuilder(ErrorPagePlugin::class)->disableOriginalConstructor()->setMethods(['sendResponse', 'generateBody'])->getMock();
38+
/** @var BrowserErrorPagePlugin | \PHPUnit\Framework\MockObject\MockObject $plugin */
39+
$plugin = $this->getMockBuilder(BrowserErrorPagePlugin::class)->setMethods(['sendResponse', 'generateBody'])->getMock();
4040
$plugin->expects($this->once())->method('generateBody')->willReturn(':boom:');
4141
$plugin->expects($this->once())->method('sendResponse');
4242
/** @var \Sabre\DAV\Server | \PHPUnit\Framework\MockObject\MockObject $server */

0 commit comments

Comments
 (0)