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);


aleixq · 2024-08-14T11:17:56Z

I am coming from #37716 for which I was proposing #37896 .
If the reason to allow dav cors is to make it easier to offer a filepicker, I think that filesharing api and filepreview api may be covered to be allowed in a similar way too. If not it will be a raw basic one IMHO.
By now the way to offer a functional filepicker is to use the webapppassword app as it also includes the filesharing api extended and preview api extended, for which I made the accepted PR some time ago when I was trying to offer Nextcloud as a DAM for a CMS (drupal).

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

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

skjnldsv added 2. developing Work in progress stale Ticket or PR with no recent activity and removed 3. to review Waiting for reviews labels Jul 27, 2024

This was referenced Jul 30, 2024

30.0.0 beta 2 #46870

Merged

30.0.0 beta 3 #46946

Merged

This was referenced Aug 5, 2024

30.0.0 beta 4 #47028

Merged

30.0.0 beta 5 #47100

Merged

skjnldsv mentioned this pull request Aug 13, 2024

30.0.0 RC1 #47190

Merged

skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024

aleixq mentioned this pull request Aug 14, 2024

Draft proposal of allowed origins cors list #37896

Closed

3 tasks

aleixq mentioned this pull request Aug 14, 2024

Files Sharing API breaks in Nc 28+ digital-blueprint/webapppassword#197

Closed

AndyScherzinger added this to the Nextcloud 31 milestone Aug 14, 2024

sunjam mentioned this pull request Aug 19, 2024

Loginflow SSO for Nextcloud johannesjo/super-productivity#914

Open

skjnldsv marked this pull request as draft November 15, 2024 13:11

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

Loading

This comment was marked as outdated.

Altahrim Sep 25, 2023

Altahrim Sep 25, 2023

This comment was marked as resolved.

aleixq commented Aug 14, 2024

	\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)));

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 Loading

Summary

Why do we need CORS on DAV?

Screenshot

Todo

Checklist

This comment was marked as outdated.

Altahrim Sep 25, 2023

Choose a reason for hiding this comment

Altahrim Sep 25, 2023

Choose a reason for hiding this comment

This comment was marked as resolved.

aleixq commented Aug 14, 2024

susnux commented Sep 20, 2023 •

edited

Loading