Support for CORS on OCS and Webdav APIs + domain whitelisting by noveens · Pull Request #28457 · owncloud/core

noveens · 2017-07-21T10:10:22Z

Description

Manually added the * header

Motivation and Context

Trying to enable CORS.

Testing instructions:

Download this file: https://github.com/noveens/js-owncloud-client/blob/master/browser/bundle.js
Create a html file in the same directory with the following code.

<!DOCTYPE html>
<html>
	<head>
		<title>owncloud Lib. test</title>
		<script type="text/javascript" src="./bundle.js"></script>

		<script type="text/javascript">
			// var oc is global
			oc.setInstance('PUT OC INSTANCE HERE');
			
			oc.login('USERNAME', 'PASSWORD').then(status => {
				alert(status);
			}).catch(error => {
				alert(error);
			});

		</script>
	</head>

	<body>
		Check the console.
	</body>
</html>

Try sniffing the request to see it's response headers (Can be done using the Network tab on the right to Console in browser)

@PVince81

noveens · 2017-07-21T11:27:53Z

Screenshot of the request and response headers :

Do I need to remove any more headers or add some more, maybe?

peterprochaska · 2017-07-26T07:15:13Z

 	 * @return array
 	 */
 	public function getCapabilities() {
+		header("Access-Control-Allow-Origin: *");


Don't use * as a wildcard domain list. We should use the value of the origin header in the initial request. A much better solution ist to have a whitelist with domains that are allowed to request content from the other domain.

@Peter-Prochaska ,

In this Pull Request, I was first trying to manually enable CORS for all (The * header) and I later attempt to make an OC app, which would maintain a table of the authenticated users authenticated via OAuth2 and then enable CORS for them.

We need to discuss about a concept for this.

I'm not sure if CORS can be used per authenticated user/domain. If it can, then the OPTIONS call might need to be pre-authenticated to be able to go through and receive the headers.
You'd then need a list of whitelisted domains per users. Maybe this can work, I suggest you experiment with this.

peterprochaska · 2017-07-26T07:15:31Z

+	 */
+	public function options() {
+		// for cross-domain request checks
+		header("Access-Control-Allow-Origin: *");


peterprochaska · 2017-07-26T07:16:40Z

 				break;
 		}
 		header($protocol.' '.$status);
+		header("Access-Control-Allow-Origin: *");


peterprochaska · 2017-08-02T13:26:08Z

+		header("Access-Control-Allow-Origin: *");
+		header("Access-Control-Allow-Headers: authorization, OCS-APIREQUEST, Origin, X-Requested-With, Content-Type, Access-Control-Allow-Origin");
+		header("Access-Control-Allow-Methods: GET, OPTIONS, POST, PUT, DELETE, MKCOL, PROPFIND");
+		header("Access-Control-Allow-Credentials: true");


Please don't use Access-Control-Allow-Origin: * together with Access-Control-Allow-Credentials: true. With this you can send cookies, headers and certificates from every other domain to the server. Chrome and firefox will block this with an error. You should use one or more domain names, coming from a whitelist, in Access-Control-Allow-Origin.

"Cannot use wildcard in Access-Control-Allow-Origin when credentials flag is true". This is the error

noveens · 2017-08-03T15:58:59Z

@PVince81 @Peter-Prochaska

Please review.
I have stored the white-listed domains using my app: https://github.com/noveens/cors
After storing the white-listed domains, I check if the requester domain is white-listed and if is, I add the CORS Headers.

Steps For Testing:

Install the CORS App ( https://github.com/noveens/cors )
Enable the App.
Add a domain from which you will request under Settings->Personal->security (I used a virtual host) (Don't forget to add "http://www.")
Use the js-owncloud-client library to request Provisioning API methods.
For example use this html:

<!DOCTYPE html>
<html>
	<head>
		<title>owncloud Lib. test</title>
		<script type="text/javascript" src="./bundle.js"></script>
		<script type="text/javascript">
				// var oc is global
				oc.setInstance('INSTANCE');

				oc.login('USERNAME', 'PASSWORD').then(status => {
					// alert(status);
					return oc.users.getUsers();
				}).then(users => {
					console.log(users);
					return oc.groups.groupExists('admin');
				}).then(groups => {
					console.log(groups);
					return oc.apps.getApps();
				}).then(apps => {
					console.log(apps);
				}).catch(error => {
					alert(error);
				});
		</script>
	</head>

	<body>
		Check the console.
	</body>
</html>

Don't forget to use this bundle: https://github.com/noveens/js-owncloud-client/blob/master/browser/bundle.js

PVince81 · 2017-08-04T08:52:57Z

 	\OC::$server->getLogger(),
 	\OC::$server->getTwoFactorAuthManager()
 );
+API::register('options', '/cloud/users', [$users, 'options'], 'provisioning_api', API::GUEST_AUTH);


Maybe better implement this directly in the OCS router ?

One idea would be to have a @CORS annotation in every method that allow it which would tell the router to also automatically accept the OPTIONS method.

ah right... OCS might not be as flexible...

But the app framework supports annotations so it would work there.

I thought we already rewired OCS to App framework somewhere... needs some research

I had a quick look at API::register(), it seems we might not be able to use annotations easily as OCS is not the AppFramework unfortunately. One alternative would be to add an extra argument $options to register() and then register the routes this way:

API::register('post', '/cloud/users', [$users, 'options'], 'provisioning_api', API::GUEST_AUTH, ['cors' => true]);

but then multiple methods might be registered for the same routes and CORS doesn't restrict by verb.

So yet another approach, add a new method:

API::enableCors('/cloud/users');

This would enable CORS for the given route.

PVince81 · 2017-08-04T08:55:25Z

+	 * @return array
+	 */
+	public function options() {
+		// for cross-domain request checks


this duplicate, again... yeah I think the annotation / route approach is way better so all this header logic would be in the router code instead

PVince81 · 2017-08-04T09:01:37Z

 	}

+	/**
+	 * @PublicPage


oh, looks like annotations do work, hmm...

PVince81 · 2017-08-04T09:03:54Z

+			if (in_array($domain, $allowedDomains)) {
+				header("Access-Control-Allow-Origin: " . $domain);
+				header("Access-Control-Allow-Headers: Origin, X-Requested-With, Content-Type, Access-Control-Allow-Origin");
+				header("Access-Control-Allow-Methods: GET, OPTIONS, POST, PUT, DELETE, MKCOL, PROPFIND");


this list could be (optionally) retrieved from all known registered routes.

So if only "GET" and "POST" were registered for that route, only allow these then.

Nice to have, not a requirement I think.

PVince81 · 2017-08-04T09:11:53Z

@noveens can you explain the CORS approach that you wanted to use ?

I remember you said that you had to pick "*" for the first request / preflight and then restrict the domain for the second one ? Can you summarize the flow step by step ?

noveens · 2017-08-04T10:02:57Z

My Approach:

Q1. First, store domains for which the user wants to allow CORS for. (User savable, not admin)
A1. I made an OC App for this: https://github.com/noveens/cors

Q2. Pre-flight request(OPTIONS)?
A2. Give the " * " header as the response since these requests are not authenticated.

Q3. For the main request?
A3. - Retrieve the user white-listed domains
- Check the requester domain
- Check if requester domain is white-listed
- If is, add "Access-Control-Allow-Origin: White-listed Domain" and other headers.

@PVince81 @Peter-Prochaska Please Check the approach.

PVince81 · 2017-08-07T07:03:42Z

 );
 $requestUri = \OC::$server->getRequest()->getRequestUri();

+if (\OC::$server->getRequest()->getMethod() === "OPTIONS") {


This should be written as a Sabre plugin instead, if possible

PVince81 · 2017-08-07T07:06:41Z

 	 * @param array $parameters
 	 * @return OC_OCS_Result
 	 */
 	public function getApps($parameters) {


use @CORS annotation instead ? or is it having trouble with OCS API ?

The @CORS notation doesn't work with OCS (Basically all those APIs whose routes are registered using the API::register method).

PVince81 · 2017-08-07T07:07:19Z

 		OC::handleLogin(\OC::$server->getRequest());
 	}

+	if (\OC::$server->getRequest()->getMethod() === "OPTIONS") {


any chance to put this inside the OCS router instead ?

noveens · 2017-08-07T18:47:07Z

@PVince81 @Peter-Prochaska ,

Please check this static web-app I built:
https://github.com/noveens/staticOwncloudWebapp

PVince81 · 2017-08-09T12:00:41Z

 		if(isset($this->request->server['HTTP_ORIGIN']) &&
 			$this->reflector->hasAnnotation('CORS')) {

+			if (\OC::$server->getAppManager()->isEnabledForUser('cors')) {


We should probably put the whitelisting thing directly into the core. I don't see any benefit of having this in a separate app.

Also having core depend on apps and hard-coded app names is usually not a good idea.

Can you bring the whitelist logic into this PR ? The likely right location is under settings/ somewhere

the other alternative is bundling the "cors" app into core's "apps/cors" but I don't see any benefit of having this code into a separate app.

@DeepDiver1975 thoughts ? https://github.com/noveens/cors

I agree with @PVince81. Why should we have a separate app?

peterprochaska

The code looks good. But no need to have a separate app.

peterprochaska · 2017-08-10T10:22:07Z

 		if(isset($this->request->server['HTTP_ORIGIN']) &&
 			$this->reflector->hasAnnotation('CORS')) {

+			if (\OC::$server->getAppManager()->isEnabledForUser('cors')) {


I agree with @PVince81. Why should we have a separate app?

noveens · 2017-08-10T11:30:46Z

@PVince81 @Peter-Prochaska,

Thanks for your approvals, I just have one doubt now:
For now, I am returning the response from here: https://github.com/owncloud/core/pull/28457/files#diff-c738eef186bd6ff91da09dff428779d1L120

So I am worried that whether the previous implementation of @CORS might break some functionality since it used to give cross origin access to all requester domains.

PVince81 · 2017-08-11T12:20:14Z

Not sure but since @Peter-Prochaska said that the wildcard domain is not acceptable, it is likely that the way how the old @CORS worked is also insecure so having the whitelist approach is a security improvement.

@Peter-Prochaska can you confirm ?

PVince81 · 2017-08-11T12:29:57Z

+	 * This function adds the CORS headers if the requester domain is white-listed
+	 */
+	public static function setCorsHeaders($userId, $domain) {
+		$allowedDomains = explode(",", \OC::$server->getConfig()->getUserValue($userId, 'cors', 'domains'));


I suggest renaming "cors" to "core" and the key to "cors_domains" as there is no more "cors" app.
Please check if other locations in code require a similar change.

peterprochaska · 2017-08-14T06:39:43Z

Yes, i can confirm

PVince81 · 2017-08-14T07:34:26Z

@noveens please fix//enhance the unit tests

PVince81 · 2017-08-16T08:06:55Z

Looks like the HTTP_ORIGIN header is not guaranteed to be always there ?

6. mkcol_over_plain...... [Mon Aug 14 16:32:59 2017] Undefined index: HTTP_ORIGIN at /home/travis/build/owncloud/core/apps/dav/lib/Connector/Sabre/CorsPlugin.php#62

from the Travis failures.

@noveens Maybe add a isset and proper logic to fallback to if the header is no set.

PVince81 · 2017-08-28T15:51:30Z

backport for 10.0.4, I'd like @Peter-Prochaska to have a second look

PVince81 · 2017-08-29T07:28:40Z

@noveens please backport to stable10 (PR for stable10 with the same commit)

phil-davis · 2017-08-29T15:31:00Z

Note: needs #28833 backported on top of the backport of this.

SamuAlfageme · 2017-08-30T14:03:13Z

@jesmrec I think this one might be the one behind the behavior we saw today with OAuth app; i.e. getting no-content on the /ocs endpoints' replies

EDIT: Error seems to be located in (the lack of)

core/lib/private/AppFramework/Middleware/Security/CORSMiddleware.php

Line 99 in e3564e6

$user = $this->request->server['PHP_AUTH_USER'];

(see #28860 (comment)) & likely to break on other PHP_AUTH_USER-less authentication flows (SAML?)

jesmrec · 2017-08-30T14:13:21Z

thanks @SamuAlfageme. I will check. The point is mobile apps that are using OAuth2 as auth method are broken with the owncloud daily master from today (30th Aug). It is caused at least for those request with "format=json" as parameter, that are being returned an empty body from the server side. I will send more info when i confirm.

CC @PVince81

PVince81 · 2017-08-30T15:18:04Z

@SamuAlfageme @jesmrec any detailed steps to reproduce ? Does the oauth app use the OPTIONS method ?

PVince81 · 2017-08-30T15:18:35Z

@SamuAlfageme @jesmrec once the problem identified, if any fixing needed, please raise a new ticket and ping @noveens

jesmrec · 2017-08-30T15:21:17Z

#28860

PVince81 · 2017-08-31T18:24:10Z

Problem was fixed in #28864, good job!

[stable10] Backport of #28457 (Enabled CORS on ownCloud)

Emil-G · 2018-02-28T10:15:38Z

Why is this option shown to end users if an Admin only has permissions to edit?

PVince81 · 2018-02-28T10:18:43Z

No, this option is editable by users only, not the admin. At some point there should be a new page where the admin can see the CORS domains set up by all users.

Emil-G · 2018-02-28T10:27:23Z

@PVince81 : Well in 10.0.7 if a user attempts, they are shown this error:
Access forbidden
Logged in user must be an admin

PVince81 · 2018-02-28T11:12:16Z

Ouch, then it's a bug...

lock · 2019-07-31T22:02:47Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

noveens requested review from LukasReschke, PVince81 and peterprochaska and removed request for LukasReschke July 22, 2017 07:24

peterprochaska suggested changes Jul 26, 2017

View reviewed changes

PVince81 assigned noveens Jul 27, 2017

PVince81 added this to the development milestone Jul 27, 2017

peterprochaska suggested changes Aug 2, 2017

View reviewed changes

noveens requested a review from DeepDiver1975 August 3, 2017 16:12

PVince81 reviewed Aug 4, 2017

View reviewed changes

PVince81 reviewed Aug 7, 2017

View reviewed changes

PVince81 reviewed Aug 9, 2017

View reviewed changes

peterprochaska suggested changes Aug 10, 2017

View reviewed changes

owncloud deleted a comment from PVince81 Aug 10, 2017

PVince81 changed the title ~~[WIP] Trying enabling CORS~~ Support for CORS on OCS and Webdav APIs + domain whitelisting Aug 11, 2017

PVince81 added the 3 - To Review label Aug 11, 2017

PVince81 reviewed Aug 11, 2017

View reviewed changes

PVince81 added backport-request 4 - To release and removed 3 - To Review labels Aug 28, 2017

PVince81 merged commit 1af53bc into master Aug 29, 2017

PVince81 deleted the enablingCORS branch August 29, 2017 07:28

phil-davis mentioned this pull request Aug 29, 2017

PHP array warning in response.php #28832

Closed

noveens mentioned this pull request Aug 30, 2017

[stable10] Backport of #28457 (Enabled CORS on ownCloud) #28852

Merged

jesmrec mentioned this pull request Aug 30, 2017

Requests against some /ocs endpoints return empty body and invalidate the session if OAuth2 is enabled. #28860

Closed

noveens mentioned this pull request Aug 30, 2017

OAuth2 App enabled with CORS fix #28864

Merged

9 tasks

PVince81 mentioned this pull request Sep 7, 2017

Enable CORS on useful endpoints #10415

Closed

4 tasks

tcitworld mentioned this pull request Sep 13, 2017

Support for cross-domain WebDAV access (CORS) nextcloud/server#3131

Open

PVince81 pushed a commit that referenced this pull request Sep 18, 2017

Merge pull request #28852 from owncloud/stable10-backport-CORS

ab10b76

[stable10] Backport of #28457 (Enabled CORS on ownCloud)

DeepDiver1975 removed the backport-request label Nov 3, 2017

ownclouders added the status/STALE label Dec 20, 2017

ownclouders removed the status/STALE label Feb 28, 2018

lock Bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Uh oh!

Conversation

noveens commented Jul 21, 2017

Description

Motivation and Context

Testing instructions:

Uh oh!

noveens commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noveens Jul 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterprochaska Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noveens commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Aug 4, 2017

Uh oh!

noveens commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noveens commented Aug 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterprochaska left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noveens commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Aug 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterprochaska commented Aug 14, 2017

Uh oh!

PVince81 commented Aug 14, 2017

Uh oh!

PVince81 commented Aug 16, 2017

noveens commented Jul 21, 2017 •

edited

Loading

noveens Jul 28, 2017 •

edited

Loading

peterprochaska Aug 2, 2017 •

edited

Loading

noveens commented Aug 3, 2017 •

edited

Loading

noveens commented Aug 10, 2017 •

edited

Loading

SamuAlfageme commented Aug 30, 2017 •

edited

Loading

jesmrec commented Aug 30, 2017 •

edited

Loading