Skip to content

Commit

Permalink
fix(stepfunctions): stack overflow when referenced json path finding …
Browse files Browse the repository at this point in the history
…encounters a circular object graph (#11225)

This change fixes #9319 where if an EcsRunTask step was used with the WAIT_FOR_TASK_TOKEN integration pattern, the logic for finding all referenced JSONPath elements in container overrides would encounter a circular reference and terminate with a stack overflow. 

With this change, `JsonPath.recurseObject` and `JsonPath.recurseArray` would maintain a list of already visited objects and use it to prune circular references back to an already visited object.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
t6nn authored Nov 3, 2020
1 parent 5e433b1 commit f14d823
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 5 deletions.
15 changes: 10 additions & 5 deletions packages/@aws-cdk/aws-stepfunctions/lib/json-path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,13 @@ interface FieldHandlers {
handleBoolean(key: string, x: boolean): {[key: string]: boolean};
}

export function recurseObject(obj: object | undefined, handlers: FieldHandlers): object | undefined {
export function recurseObject(obj: object | undefined, handlers: FieldHandlers, visited: object[] = []): object | undefined {
if (obj === undefined) { return undefined; }
if (visited.includes(obj)) {
return {};
} else {
visited.push(obj);
}

const ret: any = {};
for (const [key, value] of Object.entries(obj)) {
Expand All @@ -91,13 +96,13 @@ export function recurseObject(obj: object | undefined, handlers: FieldHandlers):
} else if (typeof value === 'number') {
Object.assign(ret, handlers.handleNumber(key, value));
} else if (Array.isArray(value)) {
Object.assign(ret, recurseArray(key, value, handlers));
Object.assign(ret, recurseArray(key, value, handlers, visited));
} else if (typeof value === 'boolean') {
Object.assign(ret, handlers.handleBoolean(key, value));
} else if (value === null || value === undefined) {
// Nothing
} else if (typeof value === 'object') {
ret[key] = recurseObject(value, handlers);
ret[key] = recurseObject(value, handlers, visited);
}
}

Expand All @@ -107,7 +112,7 @@ export function recurseObject(obj: object | undefined, handlers: FieldHandlers):
/**
* Render an array that may or may not contain a string list token
*/
function recurseArray(key: string, arr: any[], handlers: FieldHandlers): {[key: string]: any[] | string} {
function recurseArray(key: string, arr: any[], handlers: FieldHandlers, visited: object[] = []): {[key: string]: any[] | string} {
if (isStringArray(arr)) {
const path = jsonPathStringList(arr);
if (path !== undefined) {
Expand All @@ -126,7 +131,7 @@ function recurseArray(key: string, arr: any[], handlers: FieldHandlers): {[key:
throw new Error('Cannot use JsonPath fields in an array, they must be used in objects');
}
if (typeof value === 'object' && value !== null) {
return recurseObject(value, handlers);
return recurseObject(value, handlers, visited);
}
return value;
}),
Expand Down
22 changes: 22 additions & 0 deletions packages/@aws-cdk/aws-stepfunctions/test/fields.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -130,4 +130,26 @@ describe('Fields', () => {
field: `contains ${JsonPath.stringAt('$.hello')}`,
})).toThrowError(/Field references must be the entire string/);
});
test('infinitely recursive object graphs do not break referenced path finding', () => {
const deepObject = {
field: JsonPath.stringAt('$.stringField'),
deepField: JsonPath.numberAt('$.numField'),
recursiveField: undefined as any,
};
const paths = {
bool: false,
literal: 'literal',
field: JsonPath.stringAt('$.stringField'),
listField: JsonPath.listAt('$.listField'),
recursiveField: undefined as any,
deep: [
'literal',
deepObject,
],
};
paths.recursiveField = paths;
deepObject.recursiveField = paths;
expect(FieldUtils.findReferencedPaths(paths))
.toStrictEqual(['$.listField', '$.numField', '$.stringField']);
});
});

0 comments on commit f14d823

Please sign in to comment.