fix: graceful recursive fragments (#361)

* fix: patch cost limit * fix: patch max depth * fix: patch max aliases * feat: changest * chore: lint * chore: update tests * feat: cost limit add fragment recursion cost
Escape-Technologies · Mar 17, 2023 · 097334a · 097334a
1 parent 28ce30d
commit 097334a
Show file tree

Hide file tree

Showing 10 changed files with 110 additions and 6 deletions.
diff --git a/.changeset/tough-weeks-yawn.md b/.changeset/tough-weeks-yawn.md
@@ -0,0 +1,5 @@
+---
+'@escape.tech/graphql-armor-max-aliases': minor
+---
+
+add fragmentRecursionCost
diff --git a/.changeset/wicked-shrimps-arrive.md b/.changeset/wicked-shrimps-arrive.md
@@ -0,0 +1,7 @@
+---
+'@escape.tech/graphql-armor-max-aliases': patch
+'@escape.tech/graphql-armor-cost-limit': patch
+'@escape.tech/graphql-armor-max-depth': patch
+---
+
+graceful handler for recursive fragments
diff --git a/examples/apollo/test/index.spec.ts b/examples/apollo/test/index.spec.ts
@@ -56,7 +56,9 @@ describe('startup', () => {
       }`,
     });
     expect(query.errors).toBeDefined();
-    expect(query.errors?.map((e) => e.message)).toContain('Syntax Error: Query Cost limit of 100 exceeded, found 138.');
+    expect(query.errors?.map((e) => e.message)).toContain(
+      'Syntax Error: Query Cost limit of 100 exceeded, found 5023.',
+    );
   });
 
   it('should block field suggestion', async () => {

diff --git a/examples/yoga/test/index.spec.ts b/examples/yoga/test/index.spec.ts
@@ -68,7 +68,7 @@ describe('startup', () => {
     const body = JSON.parse(response.text);
     expect(body.data?.books).toBeUndefined();
     expect(body.errors).toBeDefined();
-    expect(body.errors?.map((e) => e.message)).toContain('Syntax Error: Query Cost limit of 100 exceeded, found 138.');
+    expect(body.errors?.map((e) => e.message)).toContain('Syntax Error: Query Cost limit of 100 exceeded, found 5023.');
   });
 
   it('should disable field suggestion', async () => {

diff --git a/packages/plugins/cost-limit/src/index.ts b/packages/plugins/cost-limit/src/index.ts
@@ -17,12 +17,14 @@ export type CostLimitOptions = {
   scalarCost?: number;
   depthCostFactor?: number;
   ignoreIntrospection?: boolean;
+  fragmentRecursionCost?: number;
 } & GraphQLArmorCallbackConfiguration;
 const costLimitDefaultOptions: Required<CostLimitOptions> = {
   maxCost: 5000,
   objectCost: 2,
   scalarCost: 1,
   depthCostFactor: 1.5,
+  fragmentRecursionCost: 1000,
   ignoreIntrospection: true,
   onAccept: [],
   onReject: [],
@@ -34,6 +36,7 @@ class CostLimitVisitor {
 
   private readonly context: ValidationContext;
   private readonly config: Required<CostLimitOptions>;
+  private readonly visitedFragments: Set<string> = new Set();
 
   constructor(context: ValidationContext, options?: CostLimitOptions) {
     this.context = context;
@@ -93,6 +96,10 @@ class CostLimitVisitor {
     }
 
     if (node.kind == Kind.FRAGMENT_SPREAD) {
+      if (this.visitedFragments.has(node.name.value)) {
+        return this.config.fragmentRecursionCost;
+      }
+      this.visitedFragments.add(node.name.value);
       const fragment = this.context.getFragment(node.name.value);
       if (fragment) {
         cost += this.config.depthCostFactor * this.computeComplexity(fragment, depth + 1);

diff --git a/packages/plugins/cost-limit/test/index.spec.ts b/packages/plugins/cost-limit/test/index.spec.ts
@@ -139,4 +139,36 @@ describe('global', () => {
       `Syntax Error: Query Cost limit of ${maxCost} exceeded, found ${maxCost + 1}.`,
     ]);
   });
+
+  it('should not crash on recursive fragment', async () => {
+    const testkit = createTestkit(
+      [
+        costLimitPlugin({
+          maxCost: 50,
+          objectCost: 4,
+          scalarCost: 2,
+          depthCostFactor: 2,
+          ignoreIntrospection: true,
+        }),
+      ],
+      schema,
+    );
+    const result = await testkit.execute(`query {
+        ...A
+      }
+
+      fragment A on Query {
+        ...B
+      }
+
+      fragment B on Query {
+        ...A
+      }
+    `);
+    assertSingleExecutionValue(result);
+    expect(result.errors).toBeDefined();
+    expect(result.errors?.map((error) => error.message)).toContain(
+      'Syntax Error: Query Cost limit of 50 exceeded, found 16050.',
+    );
+  });
 });
diff --git a/packages/plugins/max-aliases/src/index.ts b/packages/plugins/max-aliases/src/index.ts
@@ -6,6 +6,7 @@ import {
   FragmentSpreadNode,
   GraphQLError,
   InlineFragmentNode,
+  Kind,
   OperationDefinitionNode,
   ValidationContext,
 } from 'graphql';
@@ -23,6 +24,7 @@ class MaxAliasesVisitor {
 
   private readonly context: ValidationContext;
   private readonly config: Required<MaxAliasesOptions>;
+  private readonly visitedFragments: Set<string> = new Set();
 
   constructor(context: ValidationContext, options?: MaxAliasesOptions) {
     this.context = context;
@@ -64,10 +66,14 @@ class MaxAliasesVisitor {
       ++aliases;
     }
     if ('selectionSet' in node && node.selectionSet) {
-      for (let child of node.selectionSet.selections) {
+      for (const child of node.selectionSet.selections) {
         aliases += this.countAliases(child);
       }
-    } else if (node.kind === 'FragmentSpread') {
+    } else if (node.kind === Kind.FRAGMENT_SPREAD) {
+      if (this.visitedFragments.has(node.name.value)) {
+        return 0;
+      }
+      this.visitedFragments.add(node.name.value);
       const fragment = this.context.getFragment(node.name.value);
       if (fragment) {
         aliases += this.countAliases(fragment);

diff --git a/packages/plugins/max-aliases/test/index.spec.ts b/packages/plugins/max-aliases/test/index.spec.ts
@@ -103,4 +103,23 @@ describe('global', () => {
       `Syntax Error: Aliases limit of ${maxAliases} exceeded, found ${maxAliases + 1}.`,
     ]);
   });
+
+  it('should not crash on recursive fragment', async () => {
+    const testkit = createTestkit([maxAliasesPlugin({ n: 3 })], schema);
+    const result = await testkit.execute(`query {
+        ...A
+      }
+
+      fragment A on Query {
+        ...B
+      }
+
+      fragment B on Query {
+        ...A
+      }
+    `);
+    assertSingleExecutionValue(result);
+    expect(result.errors).toBeDefined();
+    expect(result.errors?.map((error) => error.message)).toContain('Cannot spread fragment "A" within itself via "B".');
+  });
 });
diff --git a/packages/plugins/max-depth/src/index.ts b/packages/plugins/max-depth/src/index.ts
@@ -25,6 +25,7 @@ class MaxDepthVisitor {
 
   private readonly context: ValidationContext;
   private readonly config: Required<MaxDepthOptions>;
+  private readonly visitedFragments: Set<string> = new Set();
 
   constructor(context: ValidationContext, options?: MaxDepthOptions) {
     this.context = context;
@@ -60,18 +61,22 @@ class MaxDepthVisitor {
 
   private countDepth(
     node: FieldNode | FragmentDefinitionNode | InlineFragmentNode | OperationDefinitionNode | FragmentSpreadNode,
-    parentDepth: number = 0,
+    parentDepth = 0,
   ): number {
     if (this.config.ignoreIntrospection && 'name' in node && node.name?.value === '__schema') {
       return 0;
     }
     let depth = parentDepth;
 
     if ('selectionSet' in node && node.selectionSet) {
-      for (let child of node.selectionSet.selections) {
+      for (const child of node.selectionSet.selections) {
         depth = Math.max(depth, this.countDepth(child, parentDepth + 1));
       }
     } else if (node.kind == Kind.FRAGMENT_SPREAD) {
+      if (this.visitedFragments.has(node.name.value)) {
+        return 0;
+      }
+      this.visitedFragments.add(node.name.value);
       const fragment = this.context.getFragment(node.name.value);
       if (fragment) {
         depth = Math.max(depth, this.countDepth(fragment, parentDepth + 1));

diff --git a/packages/plugins/max-depth/test/index.spec.ts b/packages/plugins/max-depth/test/index.spec.ts
@@ -119,4 +119,25 @@ describe('global', () => {
     expect(result.errors).toBeUndefined();
     expect(result.data?.__schema).toBeDefined();
   });
+
+  it('should not crash on recursive fragment', async () => {
+    const testkit = createTestkit([maxDepthPlugin({ n: 3 })], schema);
+    const result = await testkit.execute(`query {
+        ...A
+      }
+
+      fragment A on Query {
+        ...B
+      }
+
+      fragment B on Query {
+        ...A
+      }
+    `);
+    assertSingleExecutionValue(result);
+    expect(result.errors).toBeDefined();
+    expect(result.errors?.map((error) => error.message)).toContain(
+      'Syntax Error: Query depth limit of 3 exceeded, found 4.',
+    );
+  });
 });