craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.28k stars 635 forks source link

GraphQL: CORS #4830

Closed edmotech closed 5 years ago

edmotech commented 5 years ago

Description

The documentation for the GraphQL api doesn't specify a way to seat CORS headers. How would you do that?

Steps to reproduce

  1. Update to the latest Craft and setup the GraphQL.
  2. Try to query the GraphQL endpoint for a localhost development environment.

Additional info

brandonkelly commented 5 years ago

Thanks for bringing that up. We just released Craft 3.3.0.1 which automatically sets the appropriate CORS headers to allow crossdomain API requests.

simeon-smith commented 5 years ago

This issue is still happening for me even after the update.

Screen Shot 2019-08-29 at 10 10 50 AM
kjbrum commented 5 years ago

I'm still having this issue on 3.3.0.1 as well.

brandonkelly commented 5 years ago

Hm strange, the Access-Control-Allow-Origin header referenced in your error message is clearly getting added:

https://github.com/craftcms/cms/blob/069f389944401cc8eb9fecddd69e408c5b03a912/src/controllers/GraphqlController.php#L71

simeon-smith commented 5 years ago

Could it not be using the port? CORS is port specific.

brandonkelly commented 5 years ago

Are you able to view the Origin header in the Request Headers section within the request details in your browser? Does it include the port?

kjbrum commented 5 years ago

I have Craft as a headless CMS with Next.js, using the Apollo client. Pages load fine on refresh, but when I click a link to navigate to different page, I'm getting the error. If I refresh the page, it works fine.

I'm using SSR, which seems to be the part that is working.

This is a screenshot of the headers for my request that is failing.

Screenshot 2019-08-30 10 02 10
simeon-smith commented 5 years ago

I'm in the same scenario as @kjbrum. I'll get it set back up and provide more details later today.

brandonkelly commented 5 years ago

If either of you know which headers we should be adding to actionApi(), let me know or submit a PR and we will get them in.

ghost commented 5 years ago

Getting the following error in 3.3.0.1

Access to XMLHttpRequest at 'http://template.local/de/graphql' from origin 'http://vue.template.local' has been blocked by CORS policy: Request header field content-type is not allowed by Access-Control-Allow-Headers in preflight response.

So adding ->add('Access-Control-Allow-Headers', 'content-type') makes it work in our setup (vue + axios post request)

/sh

simeon-smith commented 5 years ago

@wsydney76, removing content type from your request fixes this.

@brandonkelly I was able to get everything working fine this go around. Not sure what changed except maybe removing the content type from my request.

gregorterrill commented 5 years ago

In addition to ->add('Access-Control-Allow-Headers', 'content-type') I also had to add ->add('Access-Control-Allow-Headers', 'authorization') to get it working with the token I had set up in Craft.

brandonkelly commented 5 years ago

@wsydney76 Craft is allowing the Content-Type header as of ef002013b5507d204f07acea1c22ce79cb2f7334, but I had forgotten that commit hasn’t been released yet, sorry! So that’s fixed for the next release.

@gregorterrill The Authorization header should be covered by Access-Control-Allow-Credentials, per MDN:

Credentials are cookies, authorization headers or TLS client certificates.

https://github.com/craftcms/cms/blob/069f389944401cc8eb9fecddd69e408c5b03a912/src/controllers/GraphqlController.php#L72

rkingon commented 5 years ago

Brandon! Hope you've been well. I am actually encountering this as well on Craft Pro 3.3.4.1

I checked cms/src/controllers/GraphqlController.php and it indeed as the ->add('Access-Control-Allow-Origin', $request->getOrigin()) mentioned above -- but I think the issue is because of port?

Any reason to not just allow '*' as the origin?

I'm not sure it's any less secure than injecting the request origin -- but comes with the benefit of allowing any port (which in my local dev i'm on something random).

Thanks!

rkingon commented 5 years ago

Dang it! It was my fault... I totally forgot to add the URI that I exposed GraphQL under and was pointing to the root domain. Ignore me! 😜

brandonkelly commented 5 years ago

Good to hear from you @rkingon :) Glad you got this sorted.

Jones-S commented 4 years ago

I had similar issues when suddenly my local nuxt app was serving not under http://localhost but http://192.168.1.2. This caused CORS issues.

Now my question is: where in my craft installation is the crucial part, where I allow the graphql API to return values to localhost but not to http://192.168.1.2. I checked my .env and general.php but wherever I set the frontend Url I think I came up with those settings and they were not there initially. So I've lost track, where this setting actually is. Thanks for any hints on that.

Cheers

brandonkelly commented 4 years ago

@Jones-S That is a server configuration thing, not a Craft setting.

Jones-S commented 4 years ago

@brandonkelly Thank you for your answer. Do you have a suspicion where I would have to change a setting in my MAMP, to allow not only localhost to access the graphQL API but also an IP like 192.168.1.2. I just fear that somewhere in the future I have CORS issues again and I don't know how to tackle the problem... As far as I understand is the Origin (->add('Access-Control-Allow-Origin', $request->getOrigin())) allowed for any further request, but somehow it blocked my requests and I don't know why...

brandonkelly commented 4 years ago

@Jones-S Not sure how much control regular MAMP will give you over that; I’d suggest upgrading to MAMP Pro, which will let you set up multiple local hosts each with their own host names (e.g. site-a.test, site-b.test), or ditch MAMP altogether and go with something like Laravel Homestead, which is both free and much more reliable than MAMP.

Jones-S commented 4 years ago

I do have MAMP Pro, but still I would not know where to start looking for the right settings. I will think about Laravel thought 👍

kjbrum commented 4 years ago

@Jones-S Laravel Valet is an amazing tool for local development as well.

brandonkelly commented 4 years ago

@Jones-S to be clear I was referring to Laravel Homestead (or just “Homestead” for short), which is a local development environment powered by Vagrant. Laravel itself is an application framework, that is separate from Homestead. (Same goes for Laravel Valet as @kjbrum suggested – though I wouldn’t necessarily recommend that; a lot of people have struggled getting that working properly.)

rkingon commented 4 years ago

docker ftw! =D

rkingon commented 4 years ago

Hi Brandon, sorry to bring this one back to life -- I just started using GraphQL on an older project that is using a stock heroku php buildpack, and the way the server is setup, heroku seems to be passing two origins up via headers, which is causing the cors config in GraphqlController.php to bug out.

Right now, GraphqlController.php seems to be stuffing the value of $request->getOrigin() in, which results in the following error:

Access to fetch at '[URL]' from origin '[ORIGIN]' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: The 'Access-Control-Allow-Origin' header contains multiple values '*, [ORIGIN]', but only one is allowed. Have the server send the header with a valid value, or, if an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

A tiny bit of looking online says if you're going to stuff in the origin, try and detect if multiple origins (comma separated) and only use the first one.

However, I think a better solution would be to just sub out $request->getOrigin() for * -- thoughts?

brandonkelly commented 4 years ago

@rkingon Yeah I agree. Just changed it to * for the next release.

To get the fix early, change your craftcms/cms requirement in composer.json to:

"require": {
  "craftcms/cms": "dev-develop#a9e07bf9d1eb65c1fd9f6c1ed76ada349a705687 as 3.4.15",
  "...": "..."
}

Then run composer update.

rkingon commented 4 years ago

Awesome- thank you, Brandon! Super helpful.

(now I get to delete the reverse proxy I created this morning to get it onto the same domain =D)

brandonkelly commented 4 years ago

Craft 3.4.16 is out now with that change.