feat(postgraphql): support graphql GET requests (#442)

[lanemt]

Resolves #442

Updates to PostGraphQL to support GraphQL GET requests:
http://graphql.org/learn/serving-over-http/#get-request

[benjie]

I'm concerned if we merge this as-is it may open a potentially huge XSS security flaw - it does not seem to prevent mutations? This would mean you could link someone like

<a
  href='http://blah/graphql?query=mutation{deleteMyAccount() { clientMutationId } }'>
    Check out my Twitter!
</a>

[lanemt]

Good point. Okay I will add checks to throw 4xx on GET mutations and add a unit test or two for that.

[benjie]

Awesome, thanks Matt. Are you aware of anyone detailing the security implications/best practices for GraphQL - it would be good to check we're compliant?

[lanemt]

Thanks Benjie --
Sure, I'll look into security best practices.

I Just looked around to see how other projects are handling mutation get requests.

The first project I looked into, graphql-go, allows mutations via GET.
Here's an issue where it is discussed: graphql-go/graphql#60 (comment)
And some examples: https://github.com/graphql-go/graphql/blob/master/examples/todo/main.go#L219

--

Thinking more about the XSS security -- and the link you gave as an example. If that mutation were secured the XSS attacker would need a token to authorize the request. It would be a best practice to have something like a deleteMyAccount mutation require an authenticated user.
An attacker would not know the token to add it to the query params or to the headers.

Is it correct that only default role mutations would be at risk?

Common public / anonymous mutations would be for signup / account creation and that likely would be secured with email verification or OAuth.

[benjie]

Yeah, I realised after sending that the GET isn't really much more vulnerable than POST in that regard since you can submit a form to a different domain too; however it's generally advised that you don't use GET for mutations anyway for many reasons including caching, leaking info via logs, tracking, etc. E.g. a createAccount mutation may specify an email or password which would then be in the query string which may well make it through to the server logs.

[lanemt]

Query params will be encrypted the same as a POST request body. HTTPS encrypts query parameters so we should be safe in that regard.

[lanemt]

Yes, the caching concerns are something we'll want to resolve. It's possible to run into caching issues if the cache is configured for all GET requests.

We could add Cache-Control headers to prevent mutation caching.

[benjie]

So express-graphql only allow mutations on POST; here's the code for that:

https://github.com/graphql/express-graphql/blob/f5f37b4f1fa0566c5e5e7eacbea076ed54951d72/src/index.js#L234-L253

Can we add something like that?

[lanemt]

Absolutely, Thanks. I will make updates to add that.

[chadfurman]

The XSS attacks are mitigated by HTTP-only cookies, and the CSRF attacks are mitigated by double-submit SameOrigin HttpOnly cookies. We would need to build these into postgraphile -- see #501

[benjie]

Could you point me to documentation on these "SameOrigin" cookies? My knowledge of cookies is a little dated, but AFAIK cookies are only ever sent back to the location that they originated (hence: same origin); however I'm not aware of a feature that prevents a submit from another website to our target origin from sending the cookie (other than features that the attacker can opt out of), which is what we need to prevent to protect against CSRF. A quick google does not reveal anything to the contrary, and I would expect an advancement like this to be big news.

Additional validations can take place (for example looking at the Referer and Origin headers of the incoming request) but these are not 100% reliable (though they're getting more reliable as time passes and more browsers enforce them).

CORS does not protect us because to perform an attack you don't need to receive the response, just send the request, so you don't need to opt into CORS.

The reason that the technique is called "Double Submit" is that you send it once through the HttpOnly cookie and once through the request itself; hence double submit. If you only send it through the cookie that's only a single submit and does not confer any protections.

Someone is going to have to prove to me beyond a shadow of a doubt that this is secure against CSRF before I allow it for mutations.

[benjie]

I wonder if you're referring to the SameSite proposal? AFAIK this has not been implemented yet (and even once it is implemented it won't be usable for a long period of time because older browsers will ignore the directive).

https://tools.ietf.org/html/draft-west-first-party-cookies-07

[chadfurman]

Yes, SameSite, sorry. And it is implemented in Chrome 51+ and Opera. That said, we don't need to rely on it.

Double-submit cookies are designed to protect against CSRF attacks.

From the OWASP page: https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)_Prevention_Cheat_Sheet#Double_Submit_Cookie

When a user authenticates to a site, the site should generate a (cryptographically strong) pseudorandom value and set it as a cookie on the user's machine separate from the session id. The site does not have to save this value in any way, thus avoiding server side state. The site then requires that every transaction request include this random value as a hidden form value (or other request parameter). A cross origin attacker cannot read any data sent from the server or modify cookie values, per the same-origin policy. This means that while an attacker can force a victim to send any value he wants with a malicious CSRF request, the attacker will be unable to modify or read the value stored in the cookie. Since the cookie value and the request parameter or form value must be the same, the attacker will be unable to successfully force the submission of a request with the random CSRF value.

[benjie]

The important part to note about that is it's not JUST a cookie, you also must submit the value through other means, just like the JWT. It cannot be passive or it will not work.

The site then requires that every transaction request include this random value as a hidden form value (or other request parameter).

[benjie]

(By the way, you can implement this by just using the csurf middleware before PostGraphQL and injecting the double submit cookie value into the page HTML, then sending it along with every XHR (which I do via a header to keep things tidy); I do this in a client project combined with pgSettings since we don't use JWT and I don't want it vulnerable to CSRF attacks.)

[chadfurman]

The important part to note about that is it's not JUST a cookie, you also must submit the value through other means, just like the JWT. It cannot be passive or it will not work.

Yeah, I get that. I don't get why you keep saying it. I'm not saying the current JWT method should be tossed (i.e. breaking backwards compatibility) -- just that, rather, we should have an option for cookie-based and double-submit protected JWTs.

By the way, you can implement this by just using the csurf middleware before PostGraphQL and injecting the double submit cookie value into the page HTML,

No, because my API and my SSR server are different. The API would have to write the token to the DB and the SSR server would also have to be directly connected to the DB (currently SSR server only talks to API). Because of this, the double-submit CSRF token is a good fit.

I will definitely be implementing double-submit tokens and HttpOnly JWT cookies because I do believe they are more secure. I've not yet seen evidence stating they are less secure. You've given me a great deal of guidance on the subject, and I hope to be able to contribute the effort back into the repo -- no love lost if you don't merge my PR at that point in time though.

[benjie]

I don't get why you keep saying it.

My apologies, I was under the impression you thought that sending the JWT through "properly configured" cookies only (and not also sending the DoubleSubmit value through another means) would be more secure - I see now I was misreading your messages and apologise for the miscommunication.

No, because my API and my SSR server are different. The API would have to write the token to the DB and the SSR server would also have to be directly connected to the DB (currently SSR server only talks to API). Because of this, the double-submit CSRF token is a good fit.

Ah, I get your intention now! By leveraging the fact that both servers can share the same domain regarding cookies they can share both the JWT cookie without having to communicate with each other (just using the user agent as the propagation mechanism). The SSR server can then just pass this cookie on verbatim to the GraphQL API for validation and authorization. To solve the CSRF issues this would introduce you introduce a DoubleSubmit cookie (with associated additional data transfer). Makes sense!

I think you can implement your CSRF token generation and cookie setting on your SSR server, and still validate it on your GraphQL API without needing to modify PostGraphQL in any way? Seems possible but I've not spent much time thinking about it. If you do achieve it that way it'd make a great documentation article!

---

I'm not saying the current JWT method should be tossed

I, however, am. I don't use the JWT features in my own apps and there are a number of security reasons I want to move away from JWT, but I've not yet formulated a full stateless solution that I am certain is secure and doesn't carry with it a large number of trade-offs. I really want to deprecate JWT with the release of v4 but cannot until I have a suitable alternative in place. More on this in a future issue though ;)

[chadfurman]

My apologies, I was under the impression you thought that sending the JWT through "properly configured" cookies only (and not also sending the DoubleSubmit value through another means) would be more secure - I see now I was misreading your messages and apologise for the miscommunication.

Double-submit tokens are in addition to the typical JWT. They don't have expiration times -- they last the duration of the session. They're just a (edit: cryptographically secure) pseudo-random CSRF token. The double-submit token is not the JWT. I do suggest sending the JWT only through an HTTP only cookie -- protected by the double-submit token alongside it.

Ah, I get your intention now! By leveraging the fact that both servers can share the same domain regarding cookies they can share both the JWT cookie without having to communicate with each other (just using the user agent as the propagation mechanism). The SSR server can then just pass this cookie on verbatim to the GraphQL API for validation and authorization. To solve the CSRF issues this would introduce you introduce a DoubleSubmit cookie (with associated additional data transfer). Makes sense!

Yes!!!!!

I think you can implement your CSRF token generation and cookie setting on your SSR server, and still validate it on your GraphQL API without needing to modify PostGraphQL in any way? Seems possible but I've not spent much time thinking about it. If you do achieve it that way it'd make a great documentation article!

My plan was to generate the JWT and the CSRF token alongside eachother when auth'd. Then, when the user requests a page from the SSR server, the SSR server would get the cookie too (same domain) and could pass it along to the API with the SSR server's own requests. Also, the SSR server would be able to see the additional double-submit token in the cookie and could simply replicate it from there.

I'm not saying the current JWT method should be tossed

I, however, am.

If we do get rid of it, we should make sure we replace it with something that pushes tokens into redis and has proper expiration times and refresh tokens.

[hmaurer]

Hi! What is the status on this? If this PR is abandoned, perhaps it should be archived @benjie .

[benjie]

This is effectively a TODO for me. I need to spend some time ensuring I understand the security implications/best practices; then I intend to add/merge this functionality. As always, lack of time is the issue here.

[benjie]

I'm going to archive this as @hmaurer suggests 👍