Allow to configure "allowed domains" for CORS on DAV #40537

susnux · 2023-09-20T13:15:42Z

Resolves: Support for cross-domain WebDAV access (CORS) #3131
Resolves: CORS setting at user level #30964
~~check if CORS support in login v2 and OAuth2 flow #34898~~ (not yet)
Resolves: Cors origin allowed list check #37716 (by checking a list of allowed domains, so the sharing routes can make use of it)

Summary

This adds CORS support to the DAV routes, the default is non breaking -> so no other domain is allowed.
But the admin can configure a list of allowed domains which will be allowed to access the DAV routes using CORS.

Moreover the admin can enable user defined lists, meaning that users can add their own allowed domains for WebDAV related requests, see #30964.

Why do we need CORS on DAV?

See #3131

Basically to access it within the browser from a different page, like e.g. the Dropbox file picker with allows you to pick files from your cloud onto another page (e.g. upload something from cloud to form etc).

Screenshot

Todo

Follow-up: User config UI
Follow-up: allowed header UI
If this is decided: Write documentation

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

* Exclude DAV CORS handling when no Origin specified This will exclude non-browser clients from CORS handling. Fixes some clients like davfs which break when CORS is enabled. * fix: CORS on WebDAV is not working WebDAV is not working at all when used by on browser Javascript because the CORS headers are only present in the OPTION request, but not in the subsequent WebDAV methods. * This behavior is caused by a erroneous json_decode call while retriving the user's domains whitelist. It return an object, so the is_array always fails and no header are sent. * Add Access-Control-Expose-Headers - to allow clients to access certain headers * Adding many headers as allowed headers + add capability to read additional allowed headers from config.php

@publicpage

I removed the beforeController logic here due to the change of handling CORS since PR 28457[1] According to previous implementation, CORS was only allowed with methods that had @publicpage notation for preventing CSRF attacks. But in the latest PR by me, the current implementations is as follows: * maintain a white-list of domains for whom CORS is enabled * This list can be viewed and edited under settings -> personal -> security This implementation removes the need for `@PublicPage`[2]. [1] owncloud/core#28457 [2] owncloud/core#28864

Also make sure to only return allowed methods for DAV responses Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

apps/dav/lib/Connector/Sabre/CorsPlugin.php

Altahrim · 2023-09-25T07:55:23Z

apps/dav/lib/Connector/Sabre/CorsPlugin.php

+				return;
+			}
+		} catch (\InvalidArgumentException $e) {
+			\OC::$server->getLogger()->debug('Invalid origin header was passed', ['Origin' => $originHeader, 'exception' => $e]);


Suggested change

\OC::$server->getLogger()->debug('Invalid origin header was passed', ['Origin' => $originHeader, 'exception' => $e]);

\OC::$server->get(LoggerInterface::class)->debug('Invalid origin header was passed', ['origin' => $originHeader, 'exception' => $e]);

apps/dav/lib/Connector/Sabre/CorsPlugin.php

Altahrim · 2023-09-25T08:03:34Z

apps/dav/lib/Server.php

@@ -130,6 +131,8 @@
 		$this->server->addPlugin(new ProfilerPlugin($this->request));
 		$this->server->addPlugin(new BlockLegacyClientPlugin(\OC::$server->getConfig()));
 		$this->server->addPlugin(new AnonymousOptionsPlugin());
+		$this->server->addPlugin(new CorsPlugin(\OC::$server->getUserSession(), \OC::$server->getConfig()));


I think you can change to:

Suggested change

$this->server->addPlugin(new CorsPlugin(\OC::$server->getUserSession(), \OC::$server->getConfig()));

$this->server->addPlugin(new CorsPlugin(\OC::$server->get(IUserSession), \OC::$server->get(IConfig::class)));

With:

use \OCP\IConfig; use \OCP\IUserSession

apps/dav/lib/Server.php

apps/settings/lib/Controller/CORSSettingsController.php

+	 * @return DataResponse
+	 */
+	public function updateUserEnabled($value) {
+		if (!is_bool($value)) {


lib/private/Route/Router.php

+
+				// Reach here if it's valid
+				$response = new \OC\OCS\Result(null, 100, 'OPTIONS request successful');
+				\OC_Response::setOptionsRequestHeaders($response, $this->config);


Altahrim

First quick read ^^

apps/dav/lib/Connector/Sabre/CorsPlugin.php

Altahrim · 2023-09-25T07:51:13Z

apps/dav/lib/Connector/Sabre/CorsPlugin.php

+		if (!$request->hasHeader('Origin')) {
+			return;
+		}


I think you can skip this part. The test $originHeader === null on line 72 is equivalent

Altahrim · 2023-09-25T07:55:23Z

apps/dav/lib/Connector/Sabre/CorsPlugin.php

+				return;
+			}
+		} catch (\InvalidArgumentException $e) {
+			\OC::$server->getLogger()->debug('Invalid origin header was passed', ['Origin' => $originHeader, 'exception' => $e]);


Suggested change

\OC::$server->getLogger()->debug('Invalid origin header was passed', ['Origin' => $originHeader, 'exception' => $e]);

\OC::$server->get(LoggerInterface::class)->debug('Invalid origin header was passed', ['origin' => $originHeader, 'exception' => $e]);

Altahrim · 2023-09-25T07:57:30Z

apps/dav/lib/Connector/Sabre/CorsPlugin.php

+	 */
+	public function setOptionsRequestHeaders(RequestInterface $request, ResponseInterface $response) {
+		$authorization = $request->getHeader('Authorization');
+		if ($authorization === null || $authorization === '') {


Maybe:

Suggested change

if ($authorization === null || $authorization === '') {

if (empty($authorization)) {

Altahrim · 2023-09-25T08:03:34Z

apps/dav/lib/Server.php

@@ -130,6 +131,8 @@
 		$this->server->addPlugin(new ProfilerPlugin($this->request));
 		$this->server->addPlugin(new BlockLegacyClientPlugin(\OC::$server->getConfig()));
 		$this->server->addPlugin(new AnonymousOptionsPlugin());
+		$this->server->addPlugin(new CorsPlugin(\OC::$server->getUserSession(), \OC::$server->getConfig()));


I think you can change to:

Suggested change

$this->server->addPlugin(new CorsPlugin(\OC::$server->getUserSession(), \OC::$server->getConfig()));

$this->server->addPlugin(new CorsPlugin(\OC::$server->get(IUserSession), \OC::$server->get(IConfig::class)));

With:

use \OCP\IConfig; use \OCP\IUserSession

Altahrim · 2023-09-25T08:30:58Z

lib/public/Util.php

+			if ($protocol === 'http') {
+				$port = 80;
+			} elseif ($protocol === 'https') {
+				$port = 443;
+			} else {
+				throw new \InvalidArgumentException('Only http based URLs supported');
+			}


Shorter version:

Suggested change

if ($protocol === 'http') {

$port = 80;

} elseif ($protocol === 'https') {

$port = 443;

} else {

throw new \InvalidArgumentException('Only http based URLs supported');

}

$port = match($protocol) {

'http' => 80,

'https' => 443,

};

Altahrim · 2023-09-25T08:33:58Z

settings/Controller/CorsController.php

+	public function __construct($AppName, IRequest $request,
+								IUserSession $userSession,
+								ILogger $logger,
+								IURLGenerator $urlGenerator,
+								IConfig $config) {
+		parent::__construct($AppName, $request);
+
+		$this->AppName = $AppName;
+		$this->config = $config;
+		$this->userId = $userSession->getUser()->getUID();
+		$this->logger = $logger;
+		$this->urlGenerator = $urlGenerator;
+	}


Can use properties promotion:

Suggested change

public function __construct($AppName, IRequest $request,

IUserSession $userSession,

ILogger $logger,

IURLGenerator $urlGenerator,

IConfig $config) {

parent::__construct($AppName, $request);

$this->AppName = $AppName;

$this->config = $config;

$this->userId = $userSession->getUser()->getUID();

$this->logger = $logger;

$this->urlGenerator = $urlGenerator;

}

public function __construct(

$AppName, IRequest $request,

IUserSession $userSession,

private LoggerInterface $logger,

private IURLGenerator $urlGenerator,

private IConfig $config

) {

parent::__construct($AppName, $request);

$this->userId = $userSession->getUser()->getUID();

}

Also replace deprecated ILogger

Altahrim · 2023-09-25T08:36:25Z

settings/Controller/CorsController.php

+		if (empty($this->config->getUserValue($userId, 'core', 'domains'))) {
+			$domains = [];
+		} else {
+			$domains = json_decode($this->config->getUserValue($userId, 'core', 'domains'));
+		}


Suggested change

if (empty($this->config->getUserValue($userId, 'core', 'domains'))) {

$domains = [];

} else {

$domains = json_decode($this->config->getUserValue($userId, 'core', 'domains'));

}

$domains = empty($this->config->getUserValue($userId, 'core', 'domains')

? []

: json_decode($this->config->getUserValue($userId, 'core', 'domains'), flags: JSON_THROW_ON_ERROR);

Altahrim · 2023-09-25T08:37:15Z

settings/Controller/CorsController.php

+	 * @param string $domain The domain to whitelist
+	 * @return RedirectResponse Redirection to the settings page.
+	 */
+	public function addDomain($domain) {


Can we enforce it here?

Suggested change

public function addDomain($domain) {

public function addDomain(string $domain) {

Altahrim · 2023-09-25T08:37:49Z

settings/Controller/CorsController.php

+	 * @return RedirectResponse Redirection to the settings page.
+	 */
+	public function addDomain($domain) {
+		if (!isset($domain) || !self::isValidUrl($domain)) {


If so:

Suggested change

if (!isset($domain) || !self::isValidUrl($domain)) {

if ($domain === '' || !self::isValidUrl($domain)) {

susnux added enhancement 2. developing Work in progress feature: dav security labels Sep 20, 2023

susnux added this to the Nextcloud 28 milestone Sep 20, 2023

susnux requested review from nickvergessen, skjnldsv, a team, ArtificialOwl and icewind1991 and removed request for a team September 20, 2023 13:15

susnux force-pushed the feat/cors-on-webdav branch from b0a6b70 to 6955c85 Compare September 20, 2023 13:18

susnux added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 20, 2023

This comment was marked as outdated.

Sign in to view

susnux force-pushed the feat/cors-on-webdav branch from 6955c85 to 4c2f7b1 Compare September 20, 2023 13:53

susnux changed the title ~~Allow to configure trusted domains for CORS on DAV~~ Allow to configure "allowed domains" for CORS on DAV Sep 20, 2023

susnux added 3 commits September 20, 2023 16:06

fix: Make CORS handling admin configurable and fix tests

e608e5d

Also make sure to only return allowed methods for DAV responses Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

feat: Implement settings frontend for allowed CORS domains

33ae58e

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

chore: Compile assets

bcfaa85

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

susnux force-pushed the feat/cors-on-webdav branch from 4c2f7b1 to bcfaa85 Compare September 20, 2023 14:06

susnux requested a review from AndyScherzinger September 20, 2023 14:09

fix: Resolve some psalm issues

d00b9cd

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

github-advanced-security bot found potential problems Sep 20, 2023

View reviewed changes

nickvergessen removed their request for review September 21, 2023 08:37

skjnldsv removed their request for review September 22, 2023 09:16

This comment was marked as resolved.

Sign in to view

susnux requested a review from Altahrim September 22, 2023 09:57

Altahrim reviewed Sep 25, 2023

View reviewed changes

skjnldsv mentioned this pull request Nov 1, 2023

chore: 28.0.0 beta 1 #41228

Merged

This was referenced Nov 6, 2023

28.0.0 beta 2 #41314

Merged

28.0.0 beta 3 #41379

Merged

This was referenced Nov 14, 2023

28.0.0 beta 4 #41477

Merged

28.0.0 RC1 #41621

Merged

blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023

This was referenced Mar 12, 2024

29.0.0 beta 2 #44152

Merged

29.0.0 beta 3 #44186

Merged

29.0.0 beta 4 #44275

Merged

This was referenced Mar 20, 2024

29.0.0 beta 5 #44361

Merged

29.0.0 beta 6 #44449

Merged

skjnldsv mentioned this pull request Mar 28, 2024

29.0.0 RC1 #44552

Merged

81 tasks

skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to configure "allowed domains" for CORS on DAV #40537

Allow to configure "allowed domains" for CORS on DAV #40537

susnux commented Sep 20, 2023 •

edited

This comment was marked as outdated.

Altahrim Sep 25, 2023

Altahrim Sep 25, 2023

This comment was marked as resolved.

Altahrim left a comment

Altahrim Sep 25, 2023

Altahrim Sep 25, 2023

Altahrim Sep 25, 2023

Altahrim Sep 25, 2023

Altahrim Sep 25, 2023

Altahrim Sep 25, 2023

Altahrim Sep 25, 2023

Altahrim Sep 25, 2023

Altahrim Sep 25, 2023

	\OC::$server->getLogger()->debug('Invalid origin header was passed', ['Origin' => $originHeader, 'exception' => $e]);
	\OC::$server->get(LoggerInterface::class)->debug('Invalid origin header was passed', ['origin' => $originHeader, 'exception' => $e]);

	$this->server->addPlugin(new CorsPlugin(\OC::$server->getUserSession(), \OC::$server->getConfig()));
	$this->server->addPlugin(new CorsPlugin(\OC::$server->get(IUserSession), \OC::$server->get(IConfig::class)));

	if ($authorization === null \|\| $authorization === '') {
	if (empty($authorization)) {

	public function addDomain($domain) {
	public function addDomain(string $domain) {

	if (!isset($domain) \|\| !self::isValidUrl($domain)) {
	if ($domain === '' \|\| !self::isValidUrl($domain)) {

Allow to configure "allowed domains" for CORS on DAV #40537

Are you sure you want to change the base?

Allow to configure "allowed domains" for CORS on DAV #40537

Conversation

susnux commented Sep 20, 2023 • edited

Summary

Why do we need CORS on DAV?

Screenshot

Todo

Checklist

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Altahrim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

susnux commented Sep 20, 2023 •

edited