__typename added multiple times when fragments used

[strongpauly]

Issue Description

One of the suggested mitigations for field duplication DDOS attacks (https://docs.ostorlab.co/kb/FIELD_DUPLICATION/index.html) is to reject any queries that contain duplicate fields. However, @apollo/client adds __typename multiple times when fragments are used meaning that if the suggested mitigation is implemented server side it would reject all queries using fragments.

Link to Reproduction

https://github.com/strongpauly/react-apollo-error-template

Reproduction Steps

See console.log in example repo.

@apollo/client version

4.0.3

[phryneas]

I'm sorry, but that recommendation goes against modern GraphQL client development best practices.

We strongly recommend to use fragment colocation, which means that each of your component declares their data needs as a fragment, and parent components just compose those fragments together until finally a query is created.
As a result, it will always be very likely that a field is repeated in multiple fragments - this is very normal, not only for __typename, but for all fields.

This is not something only we recommend - here is a video from last GraphQLConf where this same pattern is introduced as a best practice with Relay. Likewise, any GraphQL client author will recommend you variations of this pattern.

This will even go further - Relay already adds aliases to __typename to fragments to identify individual fragment matches and we are exploring that same route in #12916. These are guaranteed duplications and they serve a purpose.

It is the role of your server to normalize incoming queries and not try to call the id resolver in that example multiple times, but only once.

One addition: I am very confused by that page that you linked - the mitigations that they recommend don't seem to exist. I can't find a maxFieldDuplicates option to graphql-armor in their source code - probably for good reasons.

[strongpauly]

The lack of maxFieldDuplicates option in graphql-armor also confused me to be honest. However, it did make sense to me to reject queries with field duplicates as they serve no purpose from the server's view. Are you saying that DOS attacks of these kinds are not an issue, or that its ok for the client to push the reponsibility to the server to ensure that duplicate fields aren't a performance problem? Are you sure that all server implementations do this out of the box?

If __typename's were added with aliases, I would say this would fix the issue as they are therefore not actually duplicated and would fall under https://docs.ostorlab.co/kb/GRAPHQL_ALIAS_OVERLOADING/index.html, and graphql-armor's maxAliases option.

[phryneas]

Are you saying that DOS attacks of these kinds are not an issue, or that its ok for the client to push the reponsibility to the server to ensure that duplicate fields aren't a performance problem?

I've been maintaining this library for almost three years, and you're the first person I see ask about this, so I don't believe it's a serious problem.

Are you sure that all server implementations do this out of the box?

I believe that's the purpose of the collect fields algorithm in the spec. I can't guarantee that every server implements it, but the JavaScript graphql reference implementation does.

If __typename's were added with aliases, I would say this would fix the issue as they are therefore not actually duplicated and would fall under docs.ostorlab.co/kb/GRAPHQL_ALIAS_OVERLOADING/index.html, and graphql-armor's maxAliases option.

My point is that they will be added aliased on top. You will probably also reach that maxAliases limit sooner or later.
I would probably look more into general query complexity and if you are not providing a public API use trusted/whitelisted persisted queries.

[strongpauly]

So graphql-armor and analysis tools like https://github.com/dolevf/graphql-cop, which reports this as an issue, are wrong to suggest restricting queries in this way?

[phryneas]

They suggest a few dozen tools/rules, and just like eslint rules, turning on every single rule is not the best thing to do in every situation and might not always work.

[phryneas]

We discussed this again in our team meeting, and I also asked some maintainers of other clients.
It doesn't seem like anyone has brought up similar concerns to us in the last few years, so it doesn't seem to be a general concern.

That in combination with fragment colocation really encouraging patterns that lead to multiple mentions of a field means we don't really see anything we can or should do here on our side.

I'm going ahead and close this issue. But if anything else comes up, please feel free to revive it :)

[github-actions]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

[github-actions]

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.