POST request from mobile app never allowed

Martimiz commented 1 year ago

ISSUE: I'm building a mobile Flutter app that uses Silverstripe GraphQL to pull data via a POST request. Once I enable CORS, the request from the app will always fail because the app doesn't send an Origin header.

Please note that the app doesn't send a preflight check, as it isn't a browser, just a plain POST request. But the Silverstripe GraphQL Controller seems to handle a POST request in a similar manner to a preflight check, by calling the addCorsHeaders() function for it, requiring an Origin.

So the only way to to get through is manually adding an Origin Header to the POST, but afaik this is not according to the rules, and could even be considered a spoofing attempt, if a take the app to the app store...

https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_header_name

As far as I understand it by now, CORS is purely a browser thing, and handled in preflight. So I think restricting the actual POST request might maybe be handled separately, but at least the Origin thing can hopefully be tackled...

Thanks, Martine

Acceptance criteria

Can do GraphQL request without Origin header.
Security concerns are explicitly considered during peer review.

PRs

https://github.com/silverstripe/silverstripe-graphql/pull/527

blueo commented 1 year ago

Yeah I tend to agree here. The preflight request and validation is a browser concern so I suspect there should be some way to make a straight POST without a 'fake' origin.

Martimiz commented 1 year ago

Guys, maybe this just isn’t a priority? Maybe noone will ever build an app as a website companion using SilverStripe GraphQL, except for me?

I know I can just override the GraphQL controller, and I’ll probably have to, but I’m kind of curious as to the why 😉

maxime-rainville commented 1 year ago

Had a glance. I suspect we would just need to remove these lines Controller::validateOrigin.

https://github.com/silverstripe/silverstripe-graphql/blob/6b4c9a27d83c01a3e9a0422fcc1e1f3724c3d692/src/Controller.php#L234-L236

And maybe tweak this line in Controller::addCorsHeaders to handle null origin.

https://github.com/silverstripe/silverstripe-graphql/blob/6b4c9a27d83c01a3e9a0422fcc1e1f3724c3d692/src/Controller.php#L197

Given that I don't quite know what I'm doing and that this touches some key security feature, I'm not quite ready to just jump in there and make the change. But I would be keen to support this use case.

kinglozzer commented 1 year ago

I’m not sure why we check CORS server side at all, isn’t the point of CORS that it’s a set of headers for the browser to check? I.e. it protects the site making the requests, not the site receiving them.

The OPTIONS request outputs a list of valid hosts which an “attacker” (I’m using that term loosely here!) could use to set the Referer header manually, so I don’t see any security benefit in checking the origin headers server side.

michalkleiner commented 1 year ago

I would probably agree that the server doesn't need to check CORS headers. The server sends them, as instructed by the dev via config, to tell the browser what origins it can load content for your site/app. It'd be up to the webserver configuration to which requests it responds, what domains are the requestors trying to reach and whether it services them or not. The Origin header can be spoofed.

So I would say keep adding them as per the configuration (if not null), but we probably don't need to be checking for them back, or make that particular check configurable. If we receive a legitimate request that the server can handle based on its config and it goes to graphql, it should respond to it.

Martimiz commented 1 year ago

Can we assume that, if there is no Origin header (nor other cors headers) in the request, then it isn't a browser cors request? In which case there is no need anyway to add cors headers to the response?

michalkleiner commented 1 year ago

Can we assume that, if there is no Origin header (nor other cors headers) in the request, then it isn't a browser cors request? In which case there is no need anyway to add cors headers to the response?

Adding headers to the response is not an issue here, right?

Martimiz commented 1 year ago