Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CORS: always returning access-control-allow-origin: * #8231

Closed
wkloucek opened this issue Jan 17, 2024 · 11 comments
Closed

CORS: always returning access-control-allow-origin: * #8231

wkloucek opened this issue Jan 17, 2024 · 11 comments
Assignees
Labels
Priority:p1-urgent Consider a hotfix release with only that fix Type:Bug
Milestone

Comments

@wkloucek
Copy link
Contributor

Describe the bug

I'm configuring OCIS_CORS_ALLOW_ORIGINS to http://localhost:8080

Steps to reproduce

  1. Curl eg a ocdav service endpoint with the header origin: http://localhost:8080 and in verbose mode -vv
curl 'https://xxx/remote.php/dav/spaces/e9ca4e8f-d16b-4da0-9fa7-320798e3154b%2469d547aa-a6aa-467b-8981-e7be3d74d1a5' \
  -X 'PROPFIND' \
  -vv \
  -H 'authority: xxx' \
  -H 'accept: */*' \
  -H 'accept-language: de' \
  -H 'authorization: Bearer eyxxx' \
  -H 'cache-control: no-cache' \
  -H 'content-type: application/xml; charset=utf-8' \
  -H 'depth: 1' \
  -H 'origin: http://localhost:8080' \
  -H 'pragma: no-cache' \
  -H 'referer: https://xxx/files/spaces/personal/xxx?fileId=e9ca4e8f-d16b-4da0-9fa7-320798e3154b%2469d547aa-a6aa-467b-8981-e7be3d74d1a5%2169d547aa-a6aa-467b-8981-e7be3d74d1a5&tiles-size=1&files-spaces-generic-view-mode=resource-table&items-per-page=100' \
  -H 'sec-ch-ua: "Not_A Brand";v="8", "Chromium";v="120"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "Linux"' \
  -H 'sec-fetch-dest: empty' \
  -H 'sec-fetch-mode: cors' \
  -H 'sec-fetch-site: same-origin' \
  -H 'user-agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36' \
  -H 'x-request-id: c6fafded-a4f6-4d9d-a7fc-62faa8f5eebc' \
  -H 'x-requested-with: XMLHttpRequest' \
  --data-raw $'<d:propfind xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">\n  <d:prop>\n    <oc:permissions/>\n    <oc:favorite/>\n    <oc:fileid/>\n    <oc:file-parent/>\n    <oc:name/>\n    <d:lockdiscovery/>\n    <d:activelock/>\n    <oc:owner-id/>\n    <oc:owner-display-name/>\n    <oc:shareid/>\n    <oc:shareroot/>\n    <oc:share-types/>\n    <oc:privatelink/>\n    <d:getcontentlength/>\n    <oc:size/>\n    <d:getlastmodified/>\n    <d:getetag/>\n    <d:getcontenttype/>\n    <d:resourcetype/>\n    <oc:downloadURL/>\n    <oc:tags/>\n    <oc:audio/>\n    <oc:location/>\n  </d:prop>\n</d:propfind>\n' \
  --compressed

Expected behavior

We have following header in the response:

access-control-allow-origin: http://localhost:8080

Actual behavior

We have following header in the response:

access-control-allow-origin: *

Setup

oCIS 5.0.0-rc.2

Additional context

was supposed to fixed in #5108 ?

@wkloucek wkloucek changed the title CORS: always returing CORS: always returning access-control-allow-origin: * Jan 17, 2024
@micbar
Copy link
Contributor

micbar commented Jan 17, 2024

@wkloucek big 🤦 moment ... see cs3org/reva#4461

Somebody ™️ added a custom CORS handling for the DAV handler which overwrote the CORS middleware.

@micbar micbar moved this from Qualification to Prio 2 in Infinite Scale Team Board Jan 17, 2024
@micbar micbar added the Priority:p2-high Escalation, on top of current planning, release blocker label Jan 17, 2024
@micbar micbar self-assigned this Jan 17, 2024
@micbar micbar moved this from Prio 2 to In progress in Infinite Scale Team Board Jan 17, 2024
@micbar
Copy link
Contributor

micbar commented Jan 17, 2024

P2 because it blocks the Web Embed mode from another domain.

@JammingBen FYI

@wkloucek
Copy link
Contributor Author

Somebody ™️ added a custom CORS handling for the DAV handler which overwrote the CORS middleware.

Uff, seems like it could deserve some automated tests.

@micbar
Copy link
Contributor

micbar commented Jan 18, 2024

Yes. Preflight requests were working fine.

@ScharfViktor
Copy link
Contributor

ScharfViktor commented Jan 19, 2024

Uff, seems like it could deserve some automated tests.

I added some tests for that #8254

@dj4oC dj4oC added p1-urgent Priority:p1-urgent Consider a hotfix release with only that fix and removed Priority:p2-high Escalation, on top of current planning, release blocker p1-urgent labels Jan 22, 2024
@dj4oC
Copy link
Contributor

dj4oC commented Jan 22, 2024

I see this as a P1

@micbar micbar added this to the Release 5.0.0 milestone Jan 22, 2024
@micbar
Copy link
Contributor

micbar commented Jan 23, 2024

Fixed for WebDAV.

@wkloucek @dj4oC I just saw, that it still doesn't work for OCS (current sharing API)

Fixing also OCS needs more time.

@wkloucek
Copy link
Contributor Author

Fixed for WebDAV.

@wkloucek @dj4oC I just saw, that it still doesn't work for OCS (current sharing API)

Fixing also OCS needs more time.

Which means we should start using it in a later point in time and rely on the reverse proxy CORS settings because it would delay release 5.0.0 too much?

@micbar
Copy link
Contributor

micbar commented Jan 24, 2024

New finding: It works correctly with OCS, the test expectations are wrong.

The golang implementation is interpreting the CORS as in a way that only the needed values for Access-Control-Allow-Origin and for Access-Control-Allow-Headers are returned which are sent in the actual request.

@micbar micbar closed this as completed Jan 24, 2024
@github-project-automation github-project-automation bot moved this from In progress to Done in Infinite Scale Team Board Jan 24, 2024
@wkloucek
Copy link
Contributor Author

interpreting the CORS as in a way that only the needed values for Access-Control-Allow-Origin and for Access-Control-Allow-Headers are returned which are sent in the actual request.

Isn't that exactly how CORS work!?

From https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Origin:

<origin>
Specifies an origin. Only a single origin can be specified. If the server supports clients from multiple origins, it must return the origin for the specific client making the request.

@micbar
Copy link
Contributor

micbar commented Jan 24, 2024

The tests were expecting something different. Need to fix the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:p1-urgent Consider a hotfix release with only that fix Type:Bug
Projects
Archived in project
Development

No branches or pull requests

4 participants