Closed Bug 1544917 Opened 6 years ago Closed 5 years ago

[tc-web-server] /graphql and /subscription endpoints should limit to rootUrl using CORS

Categories

(Taskcluster :: Services, task)

Product:
Taskcluster
Component:
Services
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: dustin, Assigned: dustin)

Details

These are currently serving graphql and subscriptions with

access-control-allow-origin: *

which is not great. Happily, we're not using cookies, so it doesn't allow hijacking credentials (I think), but it does meant that anyone could potentially access the graphql endpoint as an API, and we want to avoid that.

The tricky bit here is, this will break the nifty netlify preview deployments like
https://deploy-preview-583--taskcluster-web.netlify.com

Thoughts on

  • whether this is a security risk right now?
  • how to work around the trickiness?
Flags: needinfo?(helfi92)

Anyone could potentially access the graphql endpoint as an API, and we want to avoid that

Could you elaborate why we want to avoid that, please? Searching for Access-Control-Allow-Origin shows that we have always set it to "*" previously[1].

[1] https://github.com/search?q=org%3Ataskcluster+Access-Control-Allow-Origin&type=Code

Flags: needinfo?(helfi92) → needinfo?(dustin)

For the other APIs, we allow calls from any origin, since they are intended for use from non-browsers.

Part of the design of the graphql endpoint was that it only be available to the web UI -- we want to avoid graphql becoming a general API that other applications can use to communicate with Taskcluster.

https://stackoverflow.com/questions/9713644/when-is-it-safe-to-enable-cors/9725695#9725695 summarizes the situation nicely -- basically tc-web-server is only intended to be "accessed via XHR" (though of course not via XMLHTTPRequest!).

I think the sec issues here are minimal so long as we are not using cookies, which we aren't at the moment. So I've unmarked this as a sec bug.

But, I think we want to use cookies someday, which would require fixing this. And I'd like to fix it anyway, for consistency :)

Group: taskcluster-security
Flags: needinfo?(dustin)
Assignee: nobody → dustin

OK, so the CORS bit isn't too hard here since we're already using the cors package.

But configuration. I'm thinking of adding an allowedCorsOriginPatterns config, which for the staging deployment could be set to include the netlify preview URLs. But currently, we don't have any service-specific config exposed on a per-deployment basis. Brian, do you think it's worth threading this parameter all the way back to taskcluster-mozilla-terraform?

Flags: needinfo?(bstack)

Probably worth waiting on the helm stuff before we do anything. Does that make sense?

Flags: needinfo?(bstack)

Sure!

Depends on: 1545174
No longer depends on: 1545174
Type: defect → task
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

Hm, I'd like to verify this in a deployment first..

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Looks good on tasckluster-ui.

OPTIONS /graphql
Host: taskcluster-web-server.herokuapp.com
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Firefox/68.0
Accept: */*
Accept-Language: en-US,en;q=0.7,nl;q=0.3
Accept-Encoding: gzip, deflate, br
Access-Control-Request-Method: POST
Access-Control-Request-Headers: authorization,content-type
Referer: https://taskcluster-ui.herokuapp.com/
Origin: https://taskcluster-ui.herokuapp.com
DNT: 1
Connection: keep-alive

HTTP/1.1 204 No Content
Server: Cowboy
Connection: keep-alive
X-Powered-By: Express
Access-Control-Allow-Origin: https://taskcluster-ui.herokuapp.com
Vary: Origin, Access-Control-Request-Headers
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Access-Control-Allow-Headers: authorization,content-type
Content-Length: 0
Date: Tue, 02 Jul 2019 19:14:24 GMT
Via: 1.1 vegur

AJ, who could double-check that with this change it's safe to use cookies for auth0 and github logins (to carry the CSRF token)?

Flags: needinfo?(abahnken)

I seem to get CORS issues when viewing the deploy previews. For example, in https://deploy-preview-952--taskcluster-web.netlify.com/auth/clients the console shows:

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://taskcluster-web-server.herokuapp.com/graphql. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing).

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://taskcluster-web-server.herokuapp.com/graphql. (Reason: CORS request did not succeed).

Can you paste the request/response headers? It "should" work :)

Request headers:

Host: taskcluster-web-server.herokuapp.com
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:67.0) Gecko/20100101 Firefox/67.0
Accept: */*
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
Access-Control-Request-Method: POST
Access-Control-Request-Headers: content-type
Referer: https://deploy-preview-961--taskcluster-web.netlify.com/auth/clients
Origin: https://deploy-preview-961--taskcluster-web.netlify.com
DNT: 1
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache

Response headers:

HTTP/1.1 204 No Content
Server: Cowboy
Connection: keep-alive
X-Powered-By: Express
Vary: Origin, Access-Control-Request-Headers
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Access-Control-Allow-Headers: content-type
Content-Length: 0
Date: Wed, 03 Jul 2019 15:23:30 GMT
Via: 1.1 vegur

Should be OK now. I had over-escaped the \ in the env var. It's now /https://deploy-preview-\d+--taskcluster-web\.netlify\.com/ which seems to work.

I can confirm that it works now. Thanks!

dustin: lgtm!

Flags: needinfo?(abahnken)

OK! I think we can enable use of the login-strategy state parameters in a cookie, then (bug 1564112).

Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.