field access seemingly random behavior, data leaked to unauthorized users!

[akomm]

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	no
RFC?	no
Version/Branch	0.12-dev

Query:
  type: 'object'
  config:
    fieldsDefaultAccess: '@=isFullyAuthenticated()'

On Query fields, I have currently seemingly random behavior, when user is NOT fully authenticated:

Possible outcomes, when I query those fields:

1. error, can not return null on non nullable field
2. proper error graphql response, with "access denied"
3. data is returned as if the user is authenticated!

When I modify the config for a field with error (1.) by adding a const "access: false", the query correctly returns "access denied for this field". So it does not have to do with the fact, the field is non-nullable. Otherwise I would get the same error. I also do NOT have any logic inside the field's resolver, which returns null when user is not fully authenticated.

When I modify the config for the same field by replacing "false" with "@=isFullyAuthenticated()", it again result in error "can not return null on non nullable field.

It seems like it has something to do with expression evaluation?

A more critical issue is that certain fields return actual data to non-authenticated users!

I do not know what the reason is, but this one is special so I assume its the type of returned value from the expression, which matters. The field is only a "namespace" field:

Query:
  [...]
  fieldsDefaultAccess: '@=isFullyAuthenticated()'
  fields:
    thisFieldIsLeaked:
      type: 'FieldType'
      resolve: '@=[]'

While the FieldType is an actuall mapped type with resolvers. The data is returned to the client, even if the user is not fully authenticated.

When I add a const "access: false" to this field, I get a proper "access denied" response! So again, the expression seem to influence the behavior in a weird way.

I have tested that the user is actually really not fully authenticated, via debugging, symfony profiler, adding a field which is queries together with the problem-fields returning "resolve: @=isFullyAuthenticated()".

[akomm]

I think this is the reason for the data leakage:

GraphQLBundle/src/Resolver/AccessResolver.php

Lines 62 to 68 in c1c6341

	if (\is_array($result)) {
	$result = \array_map(
	function ($object) use ($accessChecker, $resolveArgs) {
	return $this->hasAccess($accessChecker, $object, $resolveArgs) ? $object : null;
	},
	$result
	);

It assumes all fields of the results are non-virtual properties and available ahead of time. But that is not how the resolve works.

Or it assumes it is an array of items and want to nullify them. But this is also wrong, because you are not only limited to return data as objects, but also plain arrays and resolve fields of such plain associative array via resolvers.

What is interessing though, that this path is not used, when I provide access: false instead of access: "@=isFullyAuthenticated()"

I see this array-nullify behavior can not be changed without BC. But I think the issue is at a different place. I think the issue is, that this logic is applied on a non-array type. The field, which I use as namespace returns an object type. It does not return [FieldType!]!, but FieldType. Therefore access check should behave like on a non-array field.

[mcg-web]

@akomm I don't really get the issue can you provide a minimal example. If this is a security issue we will BC if needed.

[mcg-web]

I think I get it:

It's normal behaviors that:

• field value is replaced by null if access: '@=isFullyAuthenticated()' result is false so your field should be null.
• If access: false with replace field resolve by an closure that throw an exception

[akomm]

@mcg-web I know it is replaced with null, but it should not trigger internal error.

Instead it should provide graphql error.

It does provide graphql error when I configure access: false, why should it not behave the same way with access: "@=isFullyAuthenticated()" on the same field?

Maybe you think the expression is still the fieldsDefaultAccess. I was in the narrative context, when I told that access: false on the field directly (not fieldsDefaultAccess) does behave different (graphql error), than @=isFullyAuthenticated() on the exact same field (internal error). If I was not clear, sorry.

I am currently looking through the tests and currently can not find one related to the AccessResolver. Otherwise I would add a test case for the failure. Currently looking into it.

[akomm]

Config:

Secure:
  type: 'object'
  config:
    fields:
      youShallNotSeeThis: {type: 'String!', resolve: 'top secret data'}
      youAreAuthenticated: {type: 'Boolean!', resolve: '@=isFullyAuthenticated()'}

Query:
  type: 'object'
  config:
    fieldsDefaultAccess: '@=isFullyAuthenticated()'
    fields:
      secureField:
        # access: '@=isFullyAuthenticated()' uncommenting this leads to the same result: leaked data
        type: 'Secure'
        resolve: '@=[]'

Query:

{
  secureField {
    youShallNotSeeThis
    youAreAuthenticated
  }
}

Result:

{
  "data": {
    "secureField": {
      "youShallNotSeeThis": "top secret data",
      "youAreAuthenticated": false
    }
  }
}

Change config (access: false):

Query:
  type: 'object'
  config:
    fieldsDefaultAccess: '@=isFullyAuthenticated()'
    fields:
      secureField:
        access: false
        type: 'Secure'
        resolve: '@=[]'

Result (correct, non 500 code http response with graphql error):

{
  "data": {
    "secureField": null
  },
  "extensions": {
    "warnings": [
      {
        "message": "Access denied to this field.",
        "category": "user",

[akomm]

So I have a fix, that does not break any other tests and seem to be BC. It should prevent access checks to be applied on an array resolve value, if the type is not actually a ListOfType . What do you think? It prevents leaking of data.

Let's first ignore the "null error" thing. If needed, I make a separate issue for this. When I posted this I did not look into it to deeply to see if it is the same issue or not. I do not want to make fixes and tests for different things in one shot. When this is fixed, I can see further.

[mcg-web]

Thank you @akomm for the minimal reproduction, I'm going to give it a try