You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
There are some issues with the way security works with GraphQL. This can make creating an API secure from traversal attacks difficult/impossible.
Here's a simple example, exposing User and Document resources with simple security - where only admins can query the full collection, and voters are used to check if the user is allowed query a single item.
When querying a User with documents (e.g. user(id) { documents: { totalCount } }), API Platform will attempt to run the item_querysecurity from Document, but with the paginator as the object - performing is_granted('VIEW', object) on the paginator. This results in access denied, as there's no voter that handles Paginator.
When querying a Document with a null createdBy field (e.g. documents(id) { createdBy: { name } }), API Platform will use the security from Useritem_query - performing is_granted('VIEW', null). This will always cause access denied, as there will be no voter set up to handle a null subject. The only voter that handles a null subject is RoleVoter/RoleHierarchyVoter but obviously these won't handle it, as it's not a role that's being checked here. This seems like a bug - security shouldn't be ran if the field is null. This has actually been indirectly resolved in 2.6.3 here [GraphQL] null iterable field should be resolved directly #4092 - as this causes security to be skipped for null fields.
I currently work around both of these issues with a NullVoter and PaginatorVoter that always grants access. Unless I'm missing something, I don't think that API Platform should be running security at all on associations, as it doesn't really make sense to - the security may depend on the context of where that property is. The item_query and collection_query security should only be used for the actual exposed GraphQL queries. Instead, I think it makes more sense to secure the individual fields with the @ApiPropertysecurity attribute - however this is currently not working with GraphQL (see #4143).
In the above example, I would want to secure User::$documents so that only certain users could access that collection (admins and the user themself). At the moment, it's not possible to do this. While the Document resource collection is secured from being queried directly (Query.document(id) or Query.documents()), if you are able to query the user through GraphQL, then you will be able to query their list of documents - potentially gaining access to documents that you shouldn't be able to view (traversal attack). There are ways to 'solve' this, such as making a custom normalizer to check view access on every object being serialized, however it's not possible to grant access based on the context of the query path.
TLDR - I think that security shouldn't be run on Doctrine associations, and that the @ApiResourcesecurity attribute should be fixed to work with GraphQL to be used for securing association fields. Thoughts?
There are some issues with the way security works with GraphQL. This can make creating an API secure from traversal attacks difficult/impossible.
Here's a simple example, exposing
UserandDocumentresources with simple security - where only admins can query the full collection, and voters are used to check if the user is allowed query a single item.There are two bugs(?) with the above example:
Userwith documents (e.g.user(id) { documents: { totalCount } }), API Platform will attempt to run theitem_querysecurityfromDocument, but with the paginator as theobject- performingis_granted('VIEW', object)on the paginator. This results in access denied, as there's no voter that handlesPaginator.When querying aThis has actually been indirectly resolved in 2.6.3 here [GraphQL] null iterable field should be resolved directly #4092 - as this causes security to be skipped for null fields.Documentwith a nullcreatedByfield (e.g.documents(id) { createdBy: { name } }), API Platform will use thesecurityfromUseritem_query- performingis_granted('VIEW', null). This will always cause access denied, as there will be no voter set up to handle a null subject. The only voter that handles a null subject isRoleVoter/RoleHierarchyVoterbut obviously these won't handle it, as it's not a role that's being checked here. This seems like a bug - security shouldn't be ran if the field is null.I currently work around both of these issues with a
NullVoterandPaginatorVoterthat always grants access. Unless I'm missing something, I don't think that API Platform should be running security at all on associations, as it doesn't really make sense to - the security may depend on the context of where that property is. Theitem_queryandcollection_querysecurity should only be used for the actual exposed GraphQL queries. Instead, I think it makes more sense to secure the individual fields with the@ApiPropertysecurityattribute - however this is currently not working with GraphQL (see #4143).In the above example, I would want to secure
User::$documentsso that only certain users could access that collection (admins and the user themself). At the moment, it's not possible to do this. While theDocumentresource collection is secured from being queried directly (Query.document(id)orQuery.documents()), if you are able to query the user through GraphQL, then you will be able to query their list of documents - potentially gaining access to documents that you shouldn't be able to view (traversal attack). There are ways to 'solve' this, such as making a custom normalizer to check view access on every object being serialized, however it's not possible to grant access based on the context of the query path.TLDR - I think that security shouldn't be run on Doctrine associations, and that the
@ApiResourcesecurityattribute should be fixed to work with GraphQL to be used for securing association fields. Thoughts?