$ctx.stash.defaultValues is inadvertently overwritten with user input

naedx commented 2 years ago

Before opening, please confirm:

[X] I have installed the latest version of the Amplify CLI (see above), and confirmed that the issue still persists.
[X] I have searched for duplicate or closed issues.
[X] I have read the guide for submitting bug reports.
[X] I have done my best to include a minimal, self-contained set of instructions for consistently reproducing the issue.
[X] I have removed any sensitive information from my code snippets and submission.

How did you install the Amplify CLI?

npm

If applicable, what version of Node.js are you using?

No response

Amplify CLI Version

8.5.1

What operating system are you using?

Windows

Did you make any manual changes to the cloud resources managed by Amplify? Please describe the changes made.

No manual changes made

Amplify Categories

api

Amplify Commands

push

Describe the bug

The $ctx.stash.defaultValues is updated during the execution of a pipeline with values from the input merged in. This occurs after the $mergedValues variable is created and set in the way described below (see Additional Details). This side-effect persists to downstream resolvers.

The problem is that any resolver that later expects a clean set of values in $ctx.stash.defaultValues will unknowingly be using user-generated data.

Expected behavior

ctx.stash.defaultValues should never be updated with user data.

Reproduction steps

Create project with api and auth using the schema below
Create a cognito user
Enable logs for the API
Execute createBlog

Observe that $ctx.stash.defaultValues is overwritten with merged data (input + defaults) after the RequestMapping template for "functionName": "MutationCreateBlogDataResolverFn" is executed (see Log output below).

GraphQL schema(s)

```graphql type Blog @model @auth(rules: [ { allow: owner, operations: [create, read, update, delete] } { allow: groups, groupsField: "groups", operations: [read, update, delete] } ]) { id: ID! name: String! description: String @default(value: "Powered by Foo Bar Blogs") posts: [Post] @hasMany(indexName: "byPost", fields: ["id"]) } type Post @model @auth(rules: [ { allow: owner, operations: [create, read, update, delete] } { allow: groups, groupsField: "groups", operations: [read, update, delete] } ]) { id: ID! postID: ID! @index(name: "byPost", sortKeyFields: ["title"]) title: String! rating: Int content: String } ```

Log output

```json { "logType": "RequestMapping", "fieldName": "createBlog", "resolverArn": "arn:aws:appsync:<>:apis/<>/types/Mutation/resolvers/createBlog", "functionName": "MutationCreateBlogDataResolverFn", "fieldInError": false, "parentType": "Mutation", "path": [ "createBlog" ], "requestId": "<>", "context": { "arguments": { "input": { "name": "Blog name 3", "description": "Powered by Foo Bar Blogs", "owner": "<>::<>" } }, "prev": { "result": {} }, "stash": { "conditions": [ { "id": { "attributeExists": false } } ], "defaultValues": { "id": "0cf6ce62-3453-44f8-a2d2-be97f95ba5e3", "createdAt": "2022-06-19T13:54:00.334Z", "updatedAt": "2022-06-19T13:54:00.334Z", "name": "Blog name 3", "description": "Powered by Foo Bar Blogs", "owner": "<>::<>", "__typename": "Blog" }, "fieldName": "createBlog", "hasAuth": true, "metadata": { "dataSourceType": "AMAZON_DYNAMODB", "apiId": "<>" }, "tableName": "Blog-<>-dev", "typeName": "Mutation" }, "outErrors": [] }, "errors": [], "graphQLAPIId": "<>", "functionArn": "arn:aws:appsync:<>:apis/<>/functions/<>", "transformedTemplate": "\n\n \n \n \n{\"version\":\"2018-05-29\",\"operation\":\"PutItem\",\"attributeValues\":{\"owner\":{\"S\":\"<>::<>\"},\"createdAt\":{\"S\":\"2022-06-19T13:54:00.334Z\"},\"__typename\":{\"S\":\"Blog\"},\"name\":{\"S\":\"Blog name 3\"},\"description\":{\"S\":\"Powered by Foo Bar Blogs\"},\"id\":{\"S\":\"0cf6ce62-3453-44f8-a2d2-be97f95ba5e3\"},\"updatedAt\":{\"S\":\"2022-06-19T13:54:00.334Z\"}},\"condition\":{\"expression\":\"(attribute_not_exists(#id))\",\"expressionNames\":{\"#id\":\"id\"}},\"key\":{\"id\":{\"S\":\"0cf6ce62-3453-44f8-a2d2-be97f95ba5e3\"}}}\n\n" } ```

Additional information

I believe this problem stems from:

https://github.com/aws-amplify/amplify-category-api/blob/2c2718284b557dcebb1c8de9f6021722be9ac60b/packages/amplify-graphql-model-transformer/src/resolvers/mutation.ts#L39-L42

which generates:


## Set the default values to put request **
#set( $mergedValues = $util.defaultIfNull($ctx.stash.defaultValues, {}) )
## copy the values from input **
$util.qr($mergedValues.putAll($util.defaultIfNull($args.input, {})))

The problem would be avoided if the following were to be generated instead:


## Set the default values to put request **
#set( $mergedValues = {} )
$util.qr($mergedValues.putAll($util.defaultIfNull($ctx.stash.defaultValues, {})))
## copy the values from input **
$util.qr($mergedValues.putAll($util.defaultIfNull($args.input, {})))
## set the typename **

ykethan commented 2 years ago

Hello @naedx, thank you for reaching out. Currently, AppSync resolvers are expected to merge the values. I believe this would make a great feature request. I would also encourage contributing this to the repository by creating a PR as the team would be able to review the request.

naedx commented 2 years ago

Hi @ykethan , thank you for looking into this! I understand that the resolvers should and do "merge the values" like you said but I still think this is a bug rather than a feature request.


#set( $mergedValues = $util.defaultIfNull($ctx.stash.defaultValues, {}) )

What I think is the problem is how the merging is done . In the line above $mergedValues is assigned a new, empty map if $ctx.stash.defaultValues is null. $mergedValues can then be manipulated (eg $mergedValues.put("key", "value") ) without downstream side-effects.

Otherwise, $mergedValues is assigned the address of $ctx.stash.defaultValues through pass-by-value when $ctx.stash.defaultValues is not null. The subsequent modifications on $mergedValues will therefore have the side-effect of changing the original object (for every subsequent resolver). I don't think this is intended because:

there would be no need to then repeat logic of merging the values if it was supposed to persist. And as it stands, several resolvers in the pipeline may be generated with this logic.
this changes the semantics of the variable name "defaultValues" since after these values are changed it may not contain defaults but instead the values that are specified by the user input. This becomes dangerous for the developer who is unaware of the possibility and may implement logic expecting that $ctx.stash.defaultValues contains default, clean values that are set by the developer when in fact it contains untrustworty user data.

The approach that I proposed in "Additional information" (above) would resolve the issue, merging the values into the $mergedValues variable - just without causing the side-effect on the stash variable.

Regarding contributing a PR for this scenario: I was in the process of creating PR some weeks ago that implements what I proposed but the change resulted in several tests failing in (the wider project) since they check against snapshots that contain the original code. I'll probably need some help with this.

aws-amplify / amplify-category-api