apollographql / apollo-rs

Spec compliant GraphQL Tools in Rust.
Apache License 2.0
566 stars 45 forks source link

stack overflow on fragment cycle #716

Closed qwerdenkerXD closed 10 months ago

qwerdenkerXD commented 10 months ago

Description

During validation aren't all fragment cycles detected. This causes a stack overflow in latest beta apollo-compiler@1.0.0-beta.5

Steps to reproduce

Validate the following query for your schema:

query Introspection{
  __schema {
    types {
      ...cycle
    }
  }
}

fragment cycle on __Type {
  kind
  ...frag
}

fragment frag on __Type {
  kind
  ofType {
    ...cycle
  }
}

Here a code sample.

Expected result

The validation should pass and in the diagnostics should be one like:

{
  "errors": [
    {
      "message": "compiler error: `cycle` fragment cannot reference itself",
      "locations": [
        {
          "line": 9,
          "column": 1
        }
      ]
    }
  ]
}

Actual result

In apollo-compiler@1.0.0-beta.5:

thread 'main' has overflowed its stack
fatal runtime error: stack overflow
[1]    2496 abort      ./target/debug/gus start -m testing/server

So the program crashes.

In apollo-compiler@0.11.3: An internal error occurs because salsa detected this cycle, so still an error but no crashing stack overflow. So in terminal it is:

panicked at .../salsa-0.16.1/src/lib.rs:490:48:
Internal error, cycle detected:

DatabaseKeyIndex { group_index: 3, query_index: 43, key_index: 3 }
DatabaseKeyIndex { group_index: 3, query_index: 44, key_index: 3 }
DatabaseKeyIndex { group_index: 3, query_index: 38, key_index: 1 }
DatabaseKeyIndex { group_index: 3, query_index: 43, key_index: 5 }
DatabaseKeyIndex { group_index: 3, query_index: 44, key_index: 5 }
DatabaseKeyIndex { group_index: 3, query_index: 31, key_index: 4 }
DatabaseKeyIndex { group_index: 3, query_index: 43, key_index: 6 }
DatabaseKeyIndex { group_index: 3, query_index: 44, key_index: 6 }
DatabaseKeyIndex { group_index: 3, query_index: 38, key_index: 2 }

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Here the code sample for this version.

Environment

goto-bus-stop commented 10 months ago

Thanks for the report, should definitely fix this as it's a potential DOS vector. It's also explicitly outlawed in https://spec.graphql.org/draft/#example-cd11c.