Prevent Infinite Loop in OverlappingFieldsCanBeMergedRule

[nicolaslt]

We have spotted some RangeError: Maximum call stack size exceeded exceptions while testing our own setup, and traced it to this validation rule.

Note: I don't feel 100% confident that I understand the logic flow and intent of this code in detail. Perhaps the bugfix I'm providing is isolated, or perhaps it's a whole class of use case that has been omitted in tests in which cases this might break some valid use cases but not be detected?

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Nicolas Lagoutte (f130906, 61352ec)

[Cito]

Great find - important to fix this! The proposed solution looks good to me.

[nicolaslt]

@Cito I've just realised the error still happens when the cyclical reference is transitive. I'm going to try to fix this as well within the next 3h, but I will have limited availability after that until the 4th of January.

[Cito]

@nicolaslt The algorithm is a bit complex. Will try to examine the issue more thoroughly and compare the validation algorithm with the spec where it looks simpler.

[nicolaslt]

I've worked out that we might be able to simple re-use comparedFragmentPairs, if we extend its scope to keep track of fields as well as fragment (in which case I'd rename it, eh).

The only reason I can think of not to do that is if it's valid to have a field named after a fragment. I'm on my way to test this.

[nicolaslt]

Yep, it's valid GraphQL it seems! (though tested with version 15.2.0, not with latest)

Schema:

type Query {                      
  getObject: Object               
}                                 

type Object {        
  field: Boolean     
  otherField: Boolean
}

Query:

query myQuery {
  getObject {
    ...field
    field
  }
}
                          
fragment field on Object {
   otherField             
}

Results (with a dummy resolver):

{
  "data": {
    "getObject": {
      "otherField": null,
      "field": null
    }
  }
}

So the proper fix I think would be to use comparedFragmentPairs and make getReferencedFieldsAndFragmentNames return type something that lets line 270 realise when the fieldOrFragmentName is actually a fragment or a field. If it's a fragment, read&write to comparedFragmentPairs, if it's a field carry on as usual.

[nicolaslt]

I'm starting to understand the codebase a bit better. I think just using comparedFragmentPairs is actually ok.

I'm about to push a new version with extra tests added for good measure.

[nicolaslt]

@Cito I think this is ready again

[Cito]

@nicolaslt Thanks - will look into this after the festivities.

[Cito]

@nicolaslt Had another look and I think breaking the recursion in collect_conflicts_between_fields_and_fragment like you do here is the proper solution. Nice!

Still, I wonder whether the validation algorithm could be simplified and made less complex, in terms of code and runtime complexity, which is quadratic in both number of fields in the selection sets, and in the levels of nested fragments. This might be exploited for denial of service attacks, by sending requests with many overlapping fields and/or deeply nested fragments - if you do both, you can construct queries that are not too large, but still would take an extremely long time to validate. However, this is essentially the algorithm from the spec, just a bit more complicated because it also provides a list of error messages for the individual conflicts.

Instead of iterating over all pairs of fields in a selection set, one could to the following: First, initialize an empty "MergedResponseShape". Then Iterate through all fields in the selection set, and create the ResponseShape for the field. If it is not compatible with the MergedResponseShape, report an error. Otherwise extend the MergedResponseShape and continue.

[Cito]

Oops - just noticed that regarding the runtime complexity of the algorithm, we already have #2185.

[nicolaslt]

Yeah, I didn't spot that other PR but it did feel like the implementation could be improved.

What's next for both PRs? Should we port all missing tests (if any) to the other PR, or merge mine in the interim?

[Cito]

I think we should merge this PR to fix the immediate problem and to have the additional tests until we change the implementation as suggested in #2185.

[nicolaslt]

That sounds good to me. I'm guessing there's isn't anything for me to do, but ping if needed!

…

On Sun, 26 Dec 2021, 10:15 Christoph Zwerschke, ***@***.***> wrote: I think we should merge this PR to fix the immediate problem and to have the additional tests until we change the implementation as suggested in #2185 <#2185>. — Reply to this email directly, view it on GitHub <#3442 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQP3DEUR4TM6X2WJLCAD7DUS3TKNANCNFSM5KWQT4RA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[nicolaslt]

Hi all, is there any ETA for when this could be merged/released?

I'm quite eager to patch my servers.

[dotansimha]

@IvanGoncharov any thoughts before merging this one?

[IvanGoncharov]

@nicolaslt Thanks for contributing this fix 👍
I did some small tweaks in the tests before merging it.

@Cito Big thanks for the excellent review!

[nicolaslt]

Nice, thanks @IvanGoncharov.

Would you have an estimate on when this would be released?