Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: graceful recursive fragments #361

Merged
merged 7 commits into from
Mar 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/tough-weeks-yawn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@escape.tech/graphql-armor-max-aliases': minor
---

add fragmentRecursionCost
7 changes: 7 additions & 0 deletions .changeset/wicked-shrimps-arrive.md
Original file line number Diff line number Diff line change
@@ -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
4 changes: 3 additions & 1 deletion examples/apollo/test/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
2 changes: 1 addition & 1 deletion examples/yoga/test/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
7 changes: 7 additions & 0 deletions packages/plugins/cost-limit/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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: [],
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
32 changes: 32 additions & 0 deletions packages/plugins/cost-limit/test/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
);
});
});
10 changes: 8 additions & 2 deletions packages/plugins/max-aliases/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
FragmentSpreadNode,
GraphQLError,
InlineFragmentNode,
Kind,
OperationDefinitionNode,
ValidationContext,
} from 'graphql';
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
19 changes: 19 additions & 0 deletions packages/plugins/max-aliases/test/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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".');
});
});
9 changes: 7 additions & 2 deletions packages/plugins/max-depth/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand Down
21 changes: 21 additions & 0 deletions packages/plugins/max-depth/test/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
);
});
});