Skip to content

Commit 6b383fa

Browse files
kesselbkesselb
committed
Revert "fix(dav): Always respond custom error page on exceptions"
This reverts commit 9992e7d. Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
1 parent 3c3b7f9 commit 6b383fa

File tree

9 files changed

+75
-66
lines changed

9 files changed

+75
-66
lines changed

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@
276276
'OCA\\DAV\\Events\\SubscriptionUpdatedEvent' => $baseDir . '/../lib/Events/SubscriptionUpdatedEvent.php',
277277
'OCA\\DAV\\Exception\\ServerMaintenanceMode' => $baseDir . '/../lib/Exception/ServerMaintenanceMode.php',
278278
'OCA\\DAV\\Exception\\UnsupportedLimitOnInitialSyncException' => $baseDir . '/../lib/Exception/UnsupportedLimitOnInitialSyncException.php',
279-
'OCA\\DAV\\Files\\ErrorPagePlugin' => $baseDir . '/../lib/Files/ErrorPagePlugin.php',
279+
'OCA\\DAV\\Files\\BrowserErrorPagePlugin' => $baseDir . '/../lib/Files/BrowserErrorPagePlugin.php',
280280
'OCA\\DAV\\Files\\FileSearchBackend' => $baseDir . '/../lib/Files/FileSearchBackend.php',
281281
'OCA\\DAV\\Files\\FilesHome' => $baseDir . '/../lib/Files/FilesHome.php',
282282
'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
@@ -291,7 +291,7 @@ class ComposerStaticInitDAV
291291
'OCA\\DAV\\Events\\SubscriptionUpdatedEvent' => __DIR__ . '/..' . '/../lib/Events/SubscriptionUpdatedEvent.php',
292292
'OCA\\DAV\\Exception\\ServerMaintenanceMode' => __DIR__ . '/..' . '/../lib/Exception/ServerMaintenanceMode.php',
293293
'OCA\\DAV\\Exception\\UnsupportedLimitOnInitialSyncException' => __DIR__ . '/..' . '/../lib/Exception/UnsupportedLimitOnInitialSyncException.php',
294-
'OCA\\DAV\\Files\\ErrorPagePlugin' => __DIR__ . '/..' . '/../lib/Files/ErrorPagePlugin.php',
294+
'OCA\\DAV\\Files\\BrowserErrorPagePlugin' => __DIR__ . '/..' . '/../lib/Files/BrowserErrorPagePlugin.php',
295295
'OCA\\DAV\\Files\\FileSearchBackend' => __DIR__ . '/..' . '/../lib/Files/FileSearchBackend.php',
296296
'OCA\\DAV\\Files\\FilesHome' => __DIR__ . '/..' . '/../lib/Files/FilesHome.php',
297297
'OCA\\DAV\\Files\\LazySearchBackend' => __DIR__ . '/..' . '/../lib/Files/LazySearchBackend.php',

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
use OCA\DAV\CalDAV\DefaultCalendarValidator;
1313
use OCA\DAV\DAV\CustomPropertiesBackend;
1414
use OCA\DAV\DAV\ViewOnlyPlugin;
15-
use OCA\DAV\Files\ErrorPagePlugin;
15+
use OCA\DAV\Files\BrowserErrorPagePlugin;
1616
use OCA\Theming\ThemingDefaults;
1717
use OCP\EventDispatcher\IEventDispatcher;
1818
use OCP\Files\Folder;
@@ -90,7 +90,9 @@ public function createServer(string $baseUri,
9090
$server->addPlugin(new FakeLockerPlugin());
9191
}
9292

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

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

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

Lines changed: 38 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -7,22 +7,17 @@
77
*/
88
namespace OCA\DAV\Files;
99

10+
use OC\AppFramework\Http\Request;
1011
use OC_Template;
1112
use OCP\AppFramework\Http\ContentSecurityPolicy;
12-
use OCP\IConfig;
1313
use OCP\IRequest;
1414
use Sabre\DAV\Exception;
1515
use Sabre\DAV\Server;
1616
use Sabre\DAV\ServerPlugin;
1717

18-
class ErrorPagePlugin extends ServerPlugin {
19-
private ?Server $server = null;
20-
21-
public function __construct(
22-
private IRequest $request,
23-
private IConfig $config,
24-
) {
25-
}
18+
class BrowserErrorPagePlugin extends ServerPlugin {
19+
/** @var Server */
20+
private $server;
2621

2722
/**
2823
* This initializes the plugin.
@@ -31,12 +26,35 @@ public function __construct(
3126
* addPlugin is called.
3227
*
3328
* This method should set up the required event subscriptions.
29+
*
30+
* @param Server $server
31+
* @return void
3432
*/
35-
public function initialize(Server $server): void {
33+
public function initialize(Server $server) {
3634
$this->server = $server;
3735
$server->on('exception', [$this, 'logException'], 1000);
3836
}
3937

38+
/**
39+
* @param IRequest $request
40+
* @return bool
41+
*/
42+
public static function isBrowserRequest(IRequest $request) {
43+
if ($request->getMethod() !== 'GET') {
44+
return false;
45+
}
46+
return $request->isUserAgent([
47+
Request::USER_AGENT_IE,
48+
Request::USER_AGENT_MS_EDGE,
49+
Request::USER_AGENT_CHROME,
50+
Request::USER_AGENT_FIREFOX,
51+
Request::USER_AGENT_SAFARI,
52+
]);
53+
}
54+
55+
/**
56+
* @param \Throwable $ex
57+
*/
4058
public function logException(\Throwable $ex): void {
4159
if ($ex instanceof Exception) {
4260
$httpCode = $ex->getHTTPCode();
@@ -47,7 +65,7 @@ public function logException(\Throwable $ex): void {
4765
}
4866
$this->server->httpResponse->addHeaders($headers);
4967
$this->server->httpResponse->setStatus($httpCode);
50-
$body = $this->generateBody($ex, $httpCode);
68+
$body = $this->generateBody($httpCode);
5169
$this->server->httpResponse->setBody($body);
5270
$csp = new ContentSecurityPolicy();
5371
$this->server->httpResponse->addHeader('Content-Security-Policy', $csp->buildPolicy());
@@ -58,32 +76,18 @@ public function logException(\Throwable $ex): void {
5876
* @codeCoverageIgnore
5977
* @return bool|string
6078
*/
61-
public function generateBody(\Throwable $ex, int $httpCode): mixed {
62-
if ($this->acceptHtml()) {
63-
$templateName = 'exception';
64-
$renderAs = 'guest';
65-
if ($httpCode === 403 || $httpCode === 404) {
66-
$templateName = (string)$httpCode;
67-
}
68-
} else {
69-
$templateName = 'xml_exception';
70-
$renderAs = null;
71-
$this->server->httpResponse->setHeader('Content-Type', 'application/xml; charset=utf-8');
72-
}
79+
public function generateBody(int $httpCode) {
80+
$request = \OC::$server->getRequest();
7381

74-
$debug = $this->config->getSystemValueBool('debug', false);
82+
$templateName = 'exception';
83+
if ($httpCode === 403 || $httpCode === 404) {
84+
$templateName = (string)$httpCode;
85+
}
7586

76-
$content = new OC_Template('core', $templateName, $renderAs);
87+
$content = new OC_Template('core', $templateName, 'guest');
7788
$content->assign('title', $this->server->httpResponse->getStatusText());
78-
$content->assign('remoteAddr', $this->request->getRemoteAddress());
79-
$content->assign('requestID', $this->request->getId());
80-
$content->assign('debugMode', $debug);
81-
$content->assign('errorClass', get_class($ex));
82-
$content->assign('errorMsg', $ex->getMessage());
83-
$content->assign('errorCode', $ex->getCode());
84-
$content->assign('file', $ex->getFile());
85-
$content->assign('line', $ex->getLine());
86-
$content->assign('exception', $ex);
89+
$content->assign('remoteAddr', $request->getRemoteAddress());
90+
$content->assign('requestID', $request->getId());
8791
return $content->fetchPage();
8892
}
8993

@@ -94,14 +98,4 @@ public function sendResponse() {
9498
$this->server->sapi->sendResponse($this->server->httpResponse);
9599
exit();
96100
}
97-
98-
private function acceptHtml(): bool {
99-
foreach (explode(',', $this->request->getHeader('Accept')) as $part) {
100-
$subparts = explode(';', $part);
101-
if (str_ends_with($subparts[0], '/html')) {
102-
return true;
103-
}
104-
}
105-
return false;
106-
}
107101
}

‎apps/dav/lib/Server.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@
5454
use OCA\DAV\DAV\ViewOnlyPlugin;
5555
use OCA\DAV\Events\SabrePluginAddEvent;
5656
use OCA\DAV\Events\SabrePluginAuthInitEvent;
57-
use OCA\DAV\Files\ErrorPagePlugin;
57+
use OCA\DAV\Files\BrowserErrorPagePlugin;
5858
use OCA\DAV\Files\FileSearchBackend;
5959
use OCA\DAV\Files\LazySearchBackend;
6060
use OCA\DAV\Profiler\ProfilerPlugin;
@@ -244,7 +244,9 @@ public function __construct(
244244
$this->server->addPlugin(new FakeLockerPlugin());
245245
}
246246

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

249251
$lazySearchBackend = new LazySearchBackend();
250252
$this->server->addPlugin(new SearchPlugin($lazySearchBackend));

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2701,7 +2701,7 @@
27012701
<callback>prepostcondition</callback>
27022702
<arg>
27032703
<name>error</name>
2704-
<value>{http://sabredav.org/ns}exception</value>
2704+
<value>{DAV:}valid-sync-token</value>
27052705
</arg>
27062706
<arg>
27072707
<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
@@ -7,20 +7,20 @@
77
*/
88
namespace OCA\DAV\Tests\unit\DAV;
99

10-
use OCA\DAV\Files\ErrorPagePlugin;
10+
use OCA\DAV\Files\BrowserErrorPagePlugin;
1111
use Sabre\DAV\Exception\NotFound;
1212
use Sabre\HTTP\Response;
1313

14-
class ErrorPagePluginTest extends \Test\TestCase {
14+
class BrowserErrorPagePluginTest extends \Test\TestCase {
1515

1616
/**
1717
* @dataProvider providesExceptions
1818
* @param $expectedCode
1919
* @param $exception
2020
*/
2121
public function test($expectedCode, $exception): void {
22-
/** @var ErrorPagePlugin | \PHPUnit\Framework\MockObject\MockObject $plugin */
23-
$plugin = $this->getMockBuilder(ErrorPagePlugin::class)->disableOriginalConstructor()->setMethods(['sendResponse', 'generateBody'])->getMock();
22+
/** @var BrowserErrorPagePlugin | \PHPUnit\Framework\MockObject\MockObject $plugin */
23+
$plugin = $this->getMockBuilder(BrowserErrorPagePlugin::class)->setMethods(['sendResponse', 'generateBody'])->getMock();
2424
$plugin->expects($this->once())->method('generateBody')->willReturn(':boom:');
2525
$plugin->expects($this->once())->method('sendResponse');
2626
/** @var \Sabre\DAV\Server | \PHPUnit\Framework\MockObject\MockObject $server */

‎build/integration/dav_features/caldav.feature‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,17 @@ Feature: caldav
55
Given user "user0" exists
66
When "admin" requests calendar "user0/MyCalendar" on the endpoint "/remote.php/dav/calendars/"
77
Then The CalDAV HTTP status code should be "404"
8-
And The exception is "Internal Server Error"
8+
And The exception is "Sabre\DAV\Exception\NotFound"
9+
And The error message is "Node with name 'MyCalendar' could not be found"
910

1011
Scenario: Accessing a not shared calendar of another user
1112
Given user "user0" exists
1213
Given "admin" creates a calendar named "MyCalendar"
1314
Given The CalDAV HTTP status code should be "201"
1415
When "user0" requests calendar "admin/MyCalendar" on the endpoint "/remote.php/dav/calendars/"
1516
Then The CalDAV HTTP status code should be "404"
16-
And The exception is "Internal Server Error"
17+
And The exception is "Sabre\DAV\Exception\NotFound"
18+
And The error message is "Calendar with name 'MyCalendar' could not be found"
1719

1820
Scenario: Accessing a not shared calendar of another user via the legacy endpoint
1921
Given user "user0" exists
@@ -28,7 +30,8 @@ Feature: caldav
2830
Given user "user0" exists
2931
When "user0" requests calendar "admin/MyCalendar" on the endpoint "/remote.php/dav/calendars/"
3032
Then The CalDAV HTTP status code should be "404"
31-
And The exception is "Internal Server Error"
33+
And The exception is "Sabre\DAV\Exception\NotFound"
34+
And The error message is "Node with name 'MyCalendar' could not be found"
3235

3336
Scenario: Accessing a not existing calendar of another user via the legacy endpoint
3437
Given user "user0" exists
@@ -41,7 +44,8 @@ Feature: caldav
4144
Given user "user0" exists
4245
When "user0" requests calendar "admin/MyCalendar" on the endpoint "/remote.php/dav/calendars/"
4346
Then The CalDAV HTTP status code should be "404"
44-
And The exception is "Internal Server Error"
47+
And The exception is "Sabre\DAV\Exception\NotFound"
48+
And The error message is "Node with name 'MyCalendar' could not be found"
4549

4650
Scenario: Creating a new calendar
4751
When "admin" creates a calendar named "MyCalendar"
@@ -62,15 +66,17 @@ Feature: caldav
6266
Given user "user0" exists
6367
When "user0" sends a create calendar request to "admin/MyCalendar2" on the endpoint "/remote.php/dav/calendars/"
6468
Then The CalDAV HTTP status code should be "404"
65-
And The exception is "Internal Server Error"
69+
And The exception is "Sabre\DAV\Exception\NotFound"
70+
And The error message is "Node with name 'admin' could not be found"
6671

6772
Scenario: Create calendar request for existing calendar of another user
6873
Given user "user0" exists
6974
When "admin" creates a calendar named "MyCalendar2"
7075
Then The CalDAV HTTP status code should be "201"
7176
When "user0" sends a create calendar request to "admin/MyCalendar2" on the endpoint "/remote.php/dav/calendars/"
7277
Then The CalDAV HTTP status code should be "404"
73-
And The exception is "Internal Server Error"
78+
And The exception is "Sabre\DAV\Exception\NotFound"
79+
And The error message is "Node with name 'admin' could not be found"
7480

7581
Scenario: Update a principal's schedule-default-calendar-URL
7682
Given user "user0" exists

‎build/integration/dav_features/carddav.feature‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,15 @@ Feature: carddav
44
Scenario: Accessing a not existing addressbook of another user
55
Given user "user0" exists
66
When "admin" requests addressbook "user0/MyAddressbook" with statuscode "404" on the endpoint "/remote.php/dav/addressbooks/users/"
7-
And The CardDAV exception is "Internal Server Error"
7+
And The CardDAV exception is "Sabre\DAV\Exception\NotFound"
8+
And The CardDAV error message is "Addressbook with name 'MyAddressbook' could not be found"
89

910
Scenario: Accessing a not shared addressbook of another user
1011
Given user "user0" exists
1112
Given "admin" creates an addressbook named "MyAddressbook" with statuscode "201"
1213
When "user0" requests addressbook "admin/MyAddressbook" with statuscode "404" on the endpoint "/remote.php/dav/addressbooks/users/"
13-
And The CardDAV exception is "Internal Server Error"
14+
And The CardDAV exception is "Sabre\DAV\Exception\NotFound"
15+
And The CardDAV error message is "Addressbook with name 'MyAddressbook' could not be found"
1416

1517
Scenario: Accessing a not existing addressbook of another user via legacy endpoint
1618
Given user "user0" exists
@@ -28,7 +30,8 @@ Feature: carddav
2830
Scenario: Accessing a not existing addressbook of myself
2931
Given user "user0" exists
3032
When "user0" requests addressbook "admin/MyAddressbook" with statuscode "404" on the endpoint "/remote.php/dav/addressbooks/users/"
31-
And The CardDAV exception is "Internal Server Error"
33+
And The CardDAV exception is "Sabre\DAV\Exception\NotFound"
34+
And The CardDAV error message is "Addressbook with name 'MyAddressbook' could not be found"
3235

3336
Scenario: Creating a new addressbook
3437
When "admin" creates an addressbook named "MyAddressbook" with statuscode "201"
@@ -66,11 +69,13 @@ Feature: carddav
6669
Given user "user0" exists
6770
When "user0" sends a create addressbook request to "admin/MyAddressbook2" on the endpoint "/remote.php/dav/addressbooks/"
6871
Then The CardDAV HTTP status code should be "404"
69-
And The CardDAV exception is "Internal Server Error"
72+
And The CardDAV exception is "Sabre\DAV\Exception\NotFound"
73+
And The CardDAV error message is "File not found: admin in 'addressbooks'"
7074

7175
Scenario: Create addressbook request for existing addressbook of another user
7276
Given user "user0" exists
7377
When "admin" creates an addressbook named "MyAddressbook2" with statuscode "201"
7478
When "user0" sends a create addressbook request to "admin/MyAddressbook2" on the endpoint "/remote.php/dav/addressbooks/"
7579
Then The CardDAV HTTP status code should be "404"
76-
And The CardDAV exception is "Internal Server Error"
80+
And The CardDAV exception is "Sabre\DAV\Exception\NotFound"
81+
And The CardDAV error message is "File not found: admin in 'addressbooks'"

0 commit comments

Comments
 (0)