Skip to content
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

Add security best practices page. #1820

Open
wants to merge 4 commits into
base: source
Choose a base branch
from

Conversation

mandiwise
Copy link
Contributor

Addresses a todo in #41

Description

This PR adds a new security best practices page with content on transport layer security, demand control, and schema considerations.

@benjie @jorydotcom

Copy link

vercel bot commented Nov 7, 2024

@mandiwise is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be a super valuable resource and one the community will refer to time and time again - thanks so much for your work on it! ✨ It reads really well and covers most of the important stuff without getting too bogged down in technical detail; I've noted a few minor issues with precision and have made a few suggestions for improvement below.

src/pages/learn/security.mdx Outdated Show resolved Hide resolved
src/pages/learn/security.mdx Outdated Show resolved Hide resolved
src/pages/learn/security.mdx Outdated Show resolved Hide resolved
src/pages/learn/security.mdx Outdated Show resolved Hide resolved
src/pages/learn/security.mdx Outdated Show resolved Hide resolved
src/pages/learn/security.mdx Outdated Show resolved Hide resolved
src/pages/learn/security.mdx Outdated Show resolved Hide resolved
src/pages/learn/security.mdx Show resolved Hide resolved
src/pages/learn/security.mdx Show resolved Hide resolved
src/pages/learn/security.mdx Outdated Show resolved Hide resolved
mandiwise and others added 2 commits November 10, 2024 14:17
Co-authored-by: Benjie <benjie@jemjie.com>
Co-authored-by: Benjie <benjie@jemjie.com>
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Excited to get this out there 🙌 🎉

Copy link

vercel bot commented Nov 11, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
graphql-github-io ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 11, 2024 5:34pm


As a build step during development clients may submit their GraphQL documents to the server's allowlist, where each document is stored with a unique ID—usually its hash. At runtime, the client can send a document ID instead of the full GraphQL document, and the server will only execute requests for known document IDs.

Trusted documents are simply _persisted documents_ that are deemed to not be malicious, usually because they were authored by your developers and approved through code review. Efforts are underway to [standardize _persisted documents_ as part of the GraphQL-over-HTTP specification](https://github.com/graphql/graphql-over-http/pull/264)—an increasing number of GraphQL clients and servers already support them (sometimes under their legacy misnomer: _persisted queries_).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

people also call these persisted operations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also "stored queries" and "stored operations" (and maybe "stored documents", but I don't recall that?) have been used. I think that's too much detail to include in these docs though; "persisted queries" is sufficiently pervasive to warrant mentioning but the others can be glossed over.

Copy link
Contributor

@Urigo Urigo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very exciting to have this content here!

I would also maybe take this opportunity to link from the article to sections of tools and libraries that are related to the concepts in the article?
We can add a section call "security" under https://graphql.org/community/tools-and-libraries/ and link it here


### Trusted documents

For GraphQL APIs that only serve first-party clients, using _trusted documents_ will allow you to create an allowlist of operations that can be executed against a schema.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe put Trusted Documents at the start?
Mostly because of how important they are and also because its one of the biggest misconceptions around GraphQL and security?

I would add maybe something along the lines of -
GraphQL is mostly use as a developer tool - meaning you can use queries as you develop your app and then lock your queries in production.
That means that you can lock in advance only the queries that your team needs.
Only in case you need public APIs, and can't use Trusted Documents, then you are leaving your API exposed to all these attacks and can use tools to defend against all the following attacks..

Copy link
Contributor

@iCarossio iCarossio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions :)

Depending on the client implementation, query batching can be a useful strategy for limiting the number of round trips to a server to fetch all of the required data to render a user interface. However, there should be an upper limit on the total number of queries allowed in a single batch.

As with depth limiting, a GraphQL implementation may have configuration options to restrict operation breadth, field alias usage, and batching.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add paragraph about recursive fragments :D

CleanShot 2024-11-18 at 17 34 09@2x

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A GraphQL API cannot have recursive fragments, as stated by the GraphQL specification:

https://spec.graphql.org/draft/#sec-Fragment-Spreads-Must-Not-Form-Cycles

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting thanks!

Note: seems like it’s not always implemented properly — a CVE we found with our team.

GHSA-4rx6-g5vg-5f3j

Depth, breadth, and batch limiting help prevent broad categories of malicious operations such as cyclic queries and batching attacks, but they don't provide a way to declare that a particular field is computationally expensive to resolve. So for more advanced demand control requirements, you may wish to implement rate limiting.

Rate limiting may take place in different layers of an application, for example, in the network layer or the business logic layer. Because GraphQL allows clients to specify exactly what data they need in their queries, a server may not be able to know in advance if a request includes fields that will place a higher load on its resources during execution. As a result, applying useful rate limits for a GraphQL API typically requires a different approach than simply keeping track of the total number of incoming requests over a time period in the network layer; therefore applying rate limits within the business logic layer is generally recommended.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The perfect common example here is trying to by pass authentication server rate limiting using aliasing or batching.

CleanShot 2024-11-18 at 17 40 07@2x


Even though the overall depth of this query is shallow, the underlying data source for the API will still have to handle a large number of requests to resolve data for the aliased `hero` field.

Similarly, a client may send a GraphQL document with many batched operations in a request:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is batching standard to GraphQL? I think it's not in the official specification: https://spec.graphql.org/October2021/

It's only on some GraphQL clients.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The official specification doesn't even cover HTTP. We're in the process of specifying batching as part of the GraphQL-over-HTTP working group:

[Introspection](/learn/introspection/) is a powerful feature of GraphQL that allows you to query a GraphQL API for information about its schema. However, GraphQL APIs that only serve first-party clients may forbid introspection queries in non-development environments.

Note that disabling introspection can be used as a form of "security through obscurity," but will not be sufficient to entirely obscure a GraphQL API by itself. It may be used as a part of a broader security strategy to limit the discoverability of API information from potential attackers. For more comprehensive protection of sensitive schema details and user data, trusted documents and authorization should be used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you already added considerations about batching which I think is not standard in GraphQL, you can also talk about Field Suggestion which exits in some GraphQL clients.

CleanShot 2024-11-18 at 17 42 31@2x

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of many many ways of extracting schema information despite introspection being disabled, I worry if we list this as an example then people will think they just need to solve that too and then they're good - they're not. Ultimately: GraphQL schemas should not be treated as secrets/if your schema is secret then it should not accept arbitrary operations (i.e. you should use trusted documents).

If you're interested, a little more information on this topic can be found in my GraphQLConf 2024 talk; here's a timestamped link to the relevant part: https://youtu.be/Ytt1_ZIlYdg?t=1267

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Looking over the shoulder" is a fun one ;)
But that's totally true!

And Nice talk and thanks for the various mentions 🙏

@benjie
Copy link
Member

benjie commented Nov 18, 2024

Thanks for taking the time to review @iCarossio 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants