Reports Field Duplication (DoS) as an issue, which is now developer best practice

[strongpauly]

I've raised this issue apollographql/apollo-client#12997 with @apollo/client after seeing that your tool has issues with multiple __typename's in queries, however it seems like the community is pushing for field duplication to become the norm.

If @phryneas is correct and field duplication is not an issue, would you consider removing it as a test in your tool, or at least allow flags to be passed to exclude it?

[dolevf]

Developer best practice is not necessarily a security best practice. As long as field duplication continues to create DoS scenarios for servers i can't see how this would make sense

[strongpauly]

Perhaps you could your two cents to the discussion on the apollo-client issue? As it stands, if a server passes all checks with your tool it rejects most, if not all, queries created by one of the most popular graphql client libraries. That doesn't seem like security to me, and I feel underqualified to argue the point further.

[phryneas]

Probably better to evaluate this here. Tbh., the more I look into this it seems like someone found a bug in some server implementation and made a general rule out of it.

I can't speak for every possible GraphQL server implementation obviously, but I can speak for the reference implementation, graphql-js, and for the underlying spec.

The reference implementation

Let's look at what happens with a query like this in graphql-js:

query {
  __typename
  __typename
  field
  field
  obj {
    a
    b
  }
  obj {
    a
    b
  }
}

Inside of the graphql execute function, something along these lines happens:

const context = buildExecutionContext({ schema, document: parse(query) });
const { operation, fragments, variableValues } = context;
const collected = collectFields(
  schema,
  fragments,
  variableValues,
  queryType,
  operation,
);
executeRootGroupedFieldSet(
  exeContext,
  operation.operation,
  rootType,
  rootValue,
  groupedFieldSet,
  undefined,
);

The call to collectFields returns a "groupedFieldSet", essentially something like this:

{
  __typename: [ FieldNode, FieldNode ],
  field: [ FieldNode, FieldNode ],
  obj: [ FieldNode, FieldNode ]
}

Following the execute algorithm down from there, these functions are called:

• executeRootGroupedFieldSet
• executeFields
• executeField
• completeValue

Inside of completeValue, the first FieldNode in the field set is looked at to determine the type, and based on that type, a distincition is made:

• As __typename and field entries are "Leaf Nodes", only the first FieldNode in each array would be executed, and the duplicates ignored.
  (See https://github.com/graphql/graphql-js/blob/bd2fe71d2a7ca8619ec71bae919692ce61e10958/src/execution/execute.ts#L955-L956)

• for obj, as it is describing an object type, completeObjectValue will be entered.
  The collectAndExecuteSubfields function will be called.
  (See https://github.com/graphql/graphql-js/blob/bd2fe71d2a7ca8619ec71bae919692ce61e10958/src/execution/execute.ts#L1666)
  collectAndExecuteSubfields calls two things: collectFields and then with the result of that, executeFields, which we already analyzed above.

At no point are duplicated fields executed more than once.
Yes, it is mildly more work to parse and collect them, but that's a lot less work than a DOS attacker maybe just making two requests instead of one.

The spec

• collectFields is specified here: https://spec.graphql.org/October2021/#sec-Field-Collection
• executeField is specified here: https://spec.graphql.org/October2021/#sec-Executing-Fields
• completeValue is specified here: https://spec.graphql.org/October2021/#sec-Value-Completion

---

So, looking at that, I can't find a way in which duplicated fields would be executed more than once, either in the reference implementation or in the spec itself.
If this is a DOS risk, it would only be so for a server not following the spec and not deduplicating fields in the manner laid out in the spec.
I don't think there should be a general recommended security role for what seems to me like a server implementation bug that probably needs to be fixed at the server's implementation level.

Generally, servers can easily do any kind of optimization pass on incoming queries before executing them - which is not possible for all clients, as they might not be schema aware (e.g. Apollo Client and the urql default configuration), which means they don't know what types they actually query for, and which interfaces might match and which won't.
Field duplication in queries through fragments will always happen, and even moreso as Fragment Colocation is a recommended pattern by pretty much all frontend GraphQL clients.

[dolevf]

Thanks @phryneas for the analysis!

At the time of writing the tool, the majority of the servers I've personally looked at suffered from this. To gauge the number of servers I've looked at personally, you can look at https://github.com/dolevf/graphw00f?tab=readme-ov-file#detections which is another research project I've worked on. Most of them suffered from this one way or another.

However! if this causes problems, I'm more than happy to give the flexibility to the end user to decide what matters to them more. Version 1.15 has a way to exclude rules now with -e field_duplication :-)

[martinbonnin]

suffered from this

@dolevf can you ellaborate on what the suffering is? Looking at the test, it looks like it tests whether server support a query like { __typename __typename ... }.

I don't think that's a security problem? The parsing/validation time grows linearly with the size of the request and the resolver should be called only once. Where is the security issue?

[phryneas]

Another thing worth mentioning is that yesterday the GraphQL WG agreed on merging this spec PR (with a little bit additional bikeshedding) that redefines how fragments should be used:
graphql/graphql-spec#1193

This clearly lays out how fragments should be used - and that pattern will result in fragments with overlapping fields, triggering this rule.

Is there any way to make this rule opt-in for people that run a server that is vulnerable to DOS (note: accepting a query like { __typename __typename __typename*512 } does not mean a vulnerability if the server can deal with it correctly!) and actually need it?
Otherwise this rule might be at odds with how GraphQL is supposed to be used and deter people from following best practices.

[dolevf]

What's your recommendation for someone who's standing up a GraphQL server and wants to avoid getting DoSed using a vulnerability such as https://nvd.nist.gov/vuln/detail/CVE-2024-39895, specifically this way: GHSA-7hmh-pfrp-vcx4? How can they test their server? If you have a suggestion, I can look into changing how this logic looks like in and presented by the tool.

[phryneas]

The general advice would be to keep dependencies up to date and ensure they are well-maintained?

If there were a single vulnerability in a specific Apache version related to POST requests, the solution wouldn't be to block all POST requests to every type of HTTP server on the market, but to upgrade Apache to a non-vulnerable version.
Why would that be different here?

If you want to go a step further, you can start testing known server implementations against suspected vulnerabilies, and if you find them file a CVE and see that they get their implementation fixed.

But in the end, allowing for a field to be specified more than once through fragments is part of the spec - it's not a DOS. Just one of many many possible attack vectors that also have a genuine use case, like "POST requests in general" or "HTTP compression".

[jerelmiller]

I'd like to add, it seems the fix in that particular CVE was related strictly to the total number of tokens parsed in the query itself, not the execution of those fields. While the CVE states it related to duplicated fields, I don't think that's the underlying issue, otherwise I would have expected to see something related to resolvers and the actual execution in that fix.

What might be more helpful here is to have the tool check if it can handle a large number of tokens since that seems more like the crux of the issue. While you could use duplicate fields as a means to trigger this behavior, you should theoretically be able to trigger this with something like:

query {
  a
  b
  c
  # ...
  a1
  b1
  c1
  # ...etc
}

where you give it something like 10000 tokens. Eventually you'd want the server to fail as a result of either the query complexity being too high or the query size too big.

[dolevf]

I understand the concern - I've excluded the test from the suite.

[phryneas]

Thank you!

[strongpauly]

Is there a release planned with this change, or should I filter out the test via the command line?

[dolevf]

Yes, 1.16