Skip to content

[proxy] Remove ineffectual cors on /api/* and /headless-log-download* #13740

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

Closed
wants to merge 1 commit into from

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Oct 11, 2022

Description

See context

Because the configuration results in AllowedOrigins = ["*.<domain>"], it doesn't actually match anything because the AllowedOrigins must contain a URL (ie. https://*.gitpod.io), rather than just the host.

To prove the existing configuration does not work as intended, do the following requests:

curl https://gitpod.io/api/foo-bar \
   -H "Origin: https://api.gitpod.io/" \
   -H "Access-Control-Request-Method: POST" \
   -H "Access-Control-Request-Headers: X-Requested-With" \
   -X OPTIONS \
   -v

 HTTP/2 204
< access-control-allow-credentials: true
< access-control-allow-headers: X-Requested-With
< access-control-allow-methods: POST
< access-control-allow-origin: https://api.gitpod.io/
< access-control-max-age: 60
< content-security-policy: frame-ancestors 'self' https://*.gitpod.io https://gitpod.io/
< referrer-policy: no-referrer-when-downgrade
< strict-transport-security: max-age=31536000
< vary: Origin
< vary: Access-Control-Request-Method
< vary: Access-Control-Request-Headers
< x-content-type-options: nosniff
< x-xss-protection: 1; mode=block
< date: Tue, 11 Oct 2022 06:53:14 GMT
< via: 1.1 google
< alt-svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000


curl https://gitpod.io/api/foo-bar \
   -H "Origin: https://gitpod.io/" \
   -H "Access-Control-Request-Method: POST" \
   -H "Access-Control-Request-Headers: X-Requested-With" \
   -X OPTIONS -v

 HTTP/2 204
< content-security-policy: frame-ancestors 'self' https://*.gitpod.io https://gitpod.io/
< referrer-policy: no-referrer-when-downgrade
< strict-transport-security: max-age=31536000
< vary: Origin
< vary: Access-Control-Request-Method
< vary: Access-Control-Request-Headers
< x-content-type-options: nosniff
< x-xss-protection: 1; mode=block
< date: Tue, 11 Oct 2022 06:54:42 GMT
< via: 1.1 google
< alt-svc: h3=":443"; ma=2592000,h3-29=":443"; ma=2592000

Neither of these contain the required Access-Control-Allow-* headers on the responses.

Related Issue(s)

Fixes #

How to test

Release Notes

NONE

Documentation

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

@easyCZ easyCZ requested a review from a team October 11, 2022 07:12
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Oct 11, 2022
@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-mp-proxy-api-cors.1 because the annotations in the pull request description changed
(with .werft/ from main)

@geropl
Copy link
Member

geropl commented Oct 11, 2022

/hold

@easyCZ Can we wait with this PR until I had a chance to review, please?

The segment of the API is a "cargo cult" artifact IIRC. But still I'd like to understand better why it's there before we remove it - even it turns out to be superfluous as you said. 👍

@easyCZ
Copy link
Member Author

easyCZ commented Oct 11, 2022

/hold

@easyCZ Can we wait with this PR until I had a chance to review, please?

The segment of the API is a "cargo cult" artifact IIRC. But still I'd like to understand better why it's there before we remove it - even it turns out to be superfluous as you said. 👍

Sure, no problem. Check out the linked context for demonstration that the definition actually does not take any effect.

@easyCZ
Copy link
Member Author

easyCZ commented Oct 12, 2022

@geropl PTAL when you get a chance.

@easyCZ easyCZ marked this pull request as draft October 12, 2022 21:14
@geropl
Copy link
Member

geropl commented Oct 13, 2022

@easyCZ to me it looks like as we're still using and relying on it. This example is for the workspace auth cookie:

image

Also, the fact that we don't see it on every request does not prove it's not effective (compare the allowedHeaders = []string{"Accept", "Authorization", "Cache-Control", "Content-Type", "DNT", "Keep-Alive", "Origin", "User-Agent",
"If-Match", "If-Modified-Since", "If-None-Match",
"X-Requested-With", "X-Account-Type", "X-Client-Commit", "X-Client-Name", "X-Client-Version", "X-Execution-Id", "X-Machine-Id", "X-Machine-Session-Id", "X-User-Session-Id",
}).

When looking at your example in the description:

Neither of these contain the required Access-Control-Allow-* headers on the responses.

The first response contains them, no?

< access-control-allow-credentials: true
< access-control-allow-headers: X-Requested-With
< access-control-allow-methods: POST
< access-control-allow-origin: https://api.gitpod.io/
< access-control-max-age: 60

Still could be missing sth though. 🤔

@easyCZ
Copy link
Member Author

easyCZ commented Oct 13, 2022

The first response contains them, no?

It does. Not sure how I missed this. Will run through this again to validate. Thanks for investigating.

@geropl
Copy link
Member

geropl commented Oct 13, 2022

I'm starting a workspace on this preview env now to test if it just works 😬

@geropl
Copy link
Member

geropl commented Oct 13, 2022

Ok, at least Firefox does not want to:
image
Note that this behavior might very well be different between clients! E.g. Chrome behaved differently when I was last testing CORS behavior Jan. 2020 😬

@easyCZ easyCZ closed this Oct 16, 2022
@easyCZ easyCZ deleted the mp/proxy-api-cors branch December 8, 2022 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants