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

feat(graphql): more security : with max query depth and max query complexity #6859

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mauriau
Copy link

@mauriau mauriau commented Dec 10, 2024

Q A
Branch? main
Tickets -
License MIT
Doc PR api-platform/docs#2107

On our GraphQL APi, we run GraphQL COP and it's detected some security leak.

So I tried to fix some of this like "Alias overloading" and "Field Duplication".

Webonyx has this following rules : QueryComplexity and QueryDepth. So I implemented this on Api Platform to be configurable in api_platform.yml. And setted by default to 100

api_platform:
  graphql:
     max_query_depth: 20
     max_query_complexity: 500
`

@masseuro
Copy link

Really important for avoid dos attack

@mauriau
Copy link
Author

mauriau commented Dec 11, 2024

🤔 Hmm, the scenario "Introspect the GraphQL schema" from features/graphql/introspection.feature need a max query depth configured to 200. But I dont want to put this value as default value. How can I change this value only for the introspection test ?

@mauriau
Copy link
Author

mauriau commented Dec 11, 2024

🤔 Hmm, the scenario "Introspect the GraphQL schema" from features/graphql/introspection.feature need a max query depth configured to 200. But I dont want to put this value as default value. How can I change this value only for the introspection test ?

I changed the value in tests/Fixtures/app/AppKernel.php to 200 to pass the introspection tests

@mauriau mauriau force-pushed the feat/graphql-security branch from 258f72b to 940b169 Compare December 11, 2024 13:26
@mauriau mauriau changed the title feat(graphql): allow to configure max query depth and max query compl… feat(graphql): more security : with max query depth and max query complexity Dec 13, 2024
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

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

Nice one, just a few comments, thanks for this addition!

src/Laravel/config/api-platform.php Outdated Show resolved Hide resolved
],

'url_generation_strategy' => UrlGeneratorInterface::ABS_PATH,

'serializer' => [
'hydra_prefix' => false,
// 'datetime_format' => \DateTimeInterface::RFC3339
]
],
];
Copy link
Member

Choose a reason for hiding this comment

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

I see this file modified but it looks like cs fixes that I should probably do in another PR. Would you be able to add the configuration at https://github.com/api-platform/core/blob/main/src/Laravel/ApiPlatformProvider.php#L1295 as well?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, cs fixer did it :)

src/Symfony/Bundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
src/Symfony/Bundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved
@mauriau mauriau force-pushed the feat/graphql-security branch from d7eb6cd to 8862596 Compare December 17, 2024 16:14
@mauriau mauriau force-pushed the feat/graphql-security branch from 1ab86e4 to ee4c125 Compare December 17, 2024 16:17
@mauriau
Copy link
Author

mauriau commented Dec 17, 2024

@soyuka I have some failed on behat's tests (features/hydra/docs.feature:10), but I don't know why

@mauriau mauriau requested a review from soyuka December 19, 2024 15:04
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.

3 participants