CORS on /data does not work

[wkloucek]

Describe the bug

Set OCIS_CORS_ALLOW_ORIGINS="https:/foobar.com" when OCIS_URL=https://something.else

Steps to reproduce

1. create an upload and use tus from https:/foobar.com (this is a cross origin scenario!)

Expected behavior

The upload works

Actual behavior

The upload fails because the OPTIONS request on /data doesn#t return thre right CORS header.

Responses on other routs contain this header, while /data doesn't:
Access-Control-Allow-Origin: https:/foobar.com

Setup

oCIS 5.0.0-rc.3 deployed with the oCIS Chart from next (owncloud/ocis-charts@ebbd8c0)

[DeepDiver1975]

Which version of tusd are you using? In late 2023 cors handling was added to tusd ...

see tus/tusd#987 & tus/tusd#997

Let me know if you need a helping hand ....

[wkloucek]

Which version of tusd are you using? In late 2023 cors handling was added to tusd ...

see tus/tusd#987 & tus/tusd#997

Let me know if you need a helping hand ....

As said, it's oCIS 5.0.0-rc.3 and I don't know if it has incorporated those changes.

[DeepDiver1975]

tusd is integrated via reva
in order to set the cors configuration in tusd reva needs to be patched here

ocis/vendor/github.com/cs3org/reva/v2/pkg/rhttp/datatx/manager/tus/tus.go

Line 94 in 5362f82

Logger: log.New(appctx.GetLogger(context.Background()), "", 0),

[micbar]

assigning @butonic
work in cs3org/reva#4507

[butonic]

reva part got merged. reva bump in ocis that also contains the necessary configuration is in #8412

[butonic]

#8412 took a while to turn green, but is now merged.

[wkloucek]

Not fixed, tested with 5.0.0-rc.5:

[micbar]

There is a passing test. See linked PR.

Are you sure that rc.5 should already work?

[micbar]

rc.5 should work.

Did you configure it?

The new defaults are not targeting ocis 5.0.0 #8514

[micbar]

@butonic Can you help? You did the reva fix. Was there something missing on the ocis side?

[wkloucek]

Did you configure it?

Sure, OCIS_CORS_ALLOW_ORIGINS is explicitly set, see description of this issue.

Actually the OPTIONS call responds correctly for Access-Control-Request-Method: HEAD but not for Access-Control-Request-Method: PATCH

There is a passing test. See linked PR.

I didn't see a obvious link / result fore the passing test. I just saw the whole REVA testsuite passing. Does the test suite cover OPTIONS calls for different request methods?

[saw-jan]

I see this test scenario with OPTIONS request:

ocis/tests/acceptance/features/apiCors/cors.feature

Lines 54 to 72 in b72e5ce

	Scenario Outline: CORS headers should be returned when an preflight request is sent
	Given using OCS API version "<ocs_api_version>"
	When user "Alice" sends HTTP method "OPTIONS" to OCS API endpoint "<endpoint>" with headers
	| header | value |
	| Origin | https://aphno.badal |
	| Access-Control-Request-Headers | Origin, Accept, Content-Type, Depth, Authorization, Ocs-Apirequest, If-None-Match, If-Match, Destination, Overwrite, X-Request-Id, X-Requested-With, Tus-Resumable, Tus-Checksum-Algorithm, Upload-Concat, Upload-Length, Upload-Metadata, Upload-Defer-Length, Upload-Expires, Upload-Checksum, Upload-Offset, X-Http-Method-Override, Cache-Control |
	| Access-Control-Request-Method | <request_method> |
	And the HTTP status code should be "204"
	And the following headers should be set
	| header | value |
	| Access-Control-Allow-Headers | Origin, Accept, Content-Type, Depth, Authorization, Ocs-Apirequest, If-None-Match, If-Match, Destination, Overwrite, X-Request-Id, X-Requested-With, Tus-Resumable, Tus-Checksum-Algorithm, Upload-Concat, Upload-Length, Upload-Metadata, Upload-Defer-Length, Upload-Expires, Upload-Checksum, Upload-Offset, X-Http-Method-Override, Cache-Control |
	| Access-Control-Allow-Origin | https://aphno.badal |
	| Access-Control-Allow-Methods | <request_method> |
	Examples:
	| ocs_api_version | | endpoint | request_method |
	| 1 | | /apps/files_sharing/api/v1/shares | GET |
	| 2 | | /apps/files_sharing/api/v1/shares | PUT |
	| 1 | | /apps/files_sharing/api/v1/shares | DELETE |
	| 2 | | /apps/files_sharing/api/v1/shares | POST |

[wkloucek]

Actually there was another change touching the same code path: cs3org/reva#4527

[wkloucek]

I see this test scenario with OPTIONS request:

ocis/tests/acceptance/features/apiCors/cors.feature

Lines 54 to 72 in b72e5ce

	Scenario Outline: CORS headers should be returned when an preflight request is sent
	Given using OCS API version "<ocs_api_version>"
	When user "Alice" sends HTTP method "OPTIONS" to OCS API endpoint "<endpoint>" with headers
	| header | value |
	| Origin | https://aphno.badal |
	| Access-Control-Request-Headers | Origin, Accept, Content-Type, Depth, Authorization, Ocs-Apirequest, If-None-Match, If-Match, Destination, Overwrite, X-Request-Id, X-Requested-With, Tus-Resumable, Tus-Checksum-Algorithm, Upload-Concat, Upload-Length, Upload-Metadata, Upload-Defer-Length, Upload-Expires, Upload-Checksum, Upload-Offset, X-Http-Method-Override, Cache-Control |
	| Access-Control-Request-Method | <request_method> |
	And the HTTP status code should be "204"
	And the following headers should be set
	| header | value |
	| Access-Control-Allow-Headers | Origin, Accept, Content-Type, Depth, Authorization, Ocs-Apirequest, If-None-Match, If-Match, Destination, Overwrite, X-Request-Id, X-Requested-With, Tus-Resumable, Tus-Checksum-Algorithm, Upload-Concat, Upload-Length, Upload-Metadata, Upload-Defer-Length, Upload-Expires, Upload-Checksum, Upload-Offset, X-Http-Method-Override, Cache-Control |
	| Access-Control-Allow-Origin | https://aphno.badal |
	| Access-Control-Allow-Methods | <request_method> |
	Examples:
	| ocs_api_version | | endpoint | request_method |
	| 1 | | /apps/files_sharing/api/v1/shares | GET |
	| 2 | | /apps/files_sharing/api/v1/shares | PUT |
	| 1 | | /apps/files_sharing/api/v1/shares | DELETE |
	| 2 | | /apps/files_sharing/api/v1/shares | POST |

But that one is about OCS API and not the reva data gateway 🤔

[saw-jan]

But that one is about OCS API and not the reva data gateway 🤔

I am only aware of the public API level. if the /data is an internal service level thing then I have no idea 😅

[wkloucek]

I am only aware of the public API level. if the /data is an internal service level thing then I have no idea 😅

That's a public API 😉 It's part of the TUS upload flow

[saw-jan]

That's a public API 😉 It's part of the TUS upload flow

Ahh, thanks. then I don't see the test coverage related to that (at least not for OPTIONS to /data with different methods)

[saw-jan]

do you think adding these to the above test scenario will cover your cases?

| 2 |  /data | HEAD           |
| 2 |  /data | PATCH          |

[wkloucek]

do you think adding these to the above test scenario will cover your cases?

| 2 |  /data | HEAD           |
| 2 |  /data | PATCH          |

From what I saw in the code of cs3org/reva#4507, the OPTIONS call checks authentication. That means the tests needs to initalize a upload first. I don't know how it's handled after the changes in cs3org/reva#4527.

But generally I'd really love to have tests for it (if there are none yet) 🤗

[saw-jan]

But generally I'd really love to have tests for it (if there are none yet) 🤗

Yeah, looks like there's no coverage for it. I will add to the suite 😃

Also, I confirmed these results

| 2 |  /data | HEAD  | ✔️
| 2 |  /data | PATCH | ❌
Missing expected header 'Access-Control-Allow-Headers'

[wkloucek]

Probably there are more methods allowed on the /data endpoint. At least POST comes to my mind. But that should be checked with somebody into TUS / the data gateway / provider

[saw-jan]

@wkloucek Could you check if this scenario helps?

ocis/tests/acceptance/features/apiCors/cors.feature

Lines 138 to 160 in b7b5d45

	Scenario Outline: CORS headers should be returned when an preflight request is sent with Tus upload
	Given user "Alice" has created a new TUS resource for the space "Personal" with content "" using the WebDAV API with these headers:
	| Upload-Length | 5 |
	# dGV4dEZpbGUudHh0 is the base64 encode of textFile.txt
	| Upload-Metadata | filename dGV4dEZpbGUudHh0 |
	| Tus-Resumable | 1.0.0 |
	When user "Alice" sends HTTP method "OPTIONS" to URL "<endpoint>" with headers
	| header | value |
	| Origin | https://aphno.badal |
	| Access-Control-Request-Headers | Origin, Accept, Content-Type, Depth, Authorization, Ocs-Apirequest, If-None-Match, If-Match, Destination, Overwrite, X-Request-Id, X-Requested-With, Tus-Resumable, Tus-Checksum-Algorithm, Upload-Concat, Upload-Length, Upload-Metadata, Upload-Defer-Length, Upload-Expires, Upload-Checksum, Upload-Offset, X-Http-Method-Override, Cache-Control |
	| Access-Control-Request-Method | <request_method> |
	And the HTTP status code should be "204"
	And the following headers should be set
	| header | value |
	| Access-Control-Allow-Headers | Origin, Accept, Content-Type, Depth, Authorization, Ocs-Apirequest, If-None-Match, If-Match, Destination, Overwrite, X-Request-Id, X-Requested-With, Tus-Resumable, Tus-Checksum-Algorithm, Upload-Concat, Upload-Length, Upload-Metadata, Upload-Defer-Length, Upload-Expires, Upload-Checksum, Upload-Offset, X-Http-Method-Override, Cache-Control |
	| Access-Control-Allow-Origin | https://aphno.badal |
	| Access-Control-Allow-Methods | <request_method> |
	Examples:
	| endpoint | request_method |
	| /%tus_upload_location% | PUT |
	| /%tus_upload_location% | POST |
	| /%tus_upload_location% | HEAD |
	| /%tus_upload_location% | PATCH |

[wkloucek]

@butonic will take over from here

[butonic]

@saw-jan Regarding the status code I found this comment in the tusd implementation when handling preflight requests:

			// Although the 204 No Content status code is a better fit in this case,
			// since we do not have a response body included, we cannot use it here
			// as some browsers only accept 200 OK as successful response to a
			// preflight request. If we send them the 204 No Content the response
			// will be ignored or interpreted as a rejection.
			// For example, the Presto engine, which is used in older versions of
			// Opera, Opera Mobile and Opera Mini, handles CORS this way.
			handler.sendResp(w, r, http.StatusOK)

Ok ... now why did we not run into that?

Turns out that we are also checking the CORS headers in the frontend. And OCIS_CORS_ALLOW_METHODS / FRONTEND_CORS_ALLOW_METHODS is missing the PATCH method.

fixed by #8651

can also be fixed by setting OCIS_CORS_ALLOW_METHODS="OPTIONS,HEAD,GET,PUT,POST,PATCH,DELETE,MKCOL,PROPFIND,PROPPATCH,MOVE,COPY,REPORT,SEARCH"

[butonic]

That being said, we should omit checking the CORS headers for the /data endpoint in the frontend service. The datagateway should just forward the preflight requests.

[micbar]

@wkloucek can we set the patch method for the frontend service as a workaround?

[wkloucek]

@micbar @butonic please give owncloud/ocis-charts#504 a review