Skip to content

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!

// '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 :)

@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

@soyuka
Copy link
Member

soyuka commented Dec 20, 2024

Thanks!

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