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

Invalid non-nullable or required List Type variable should return UserInputError #6066

Merged
merged 5 commits into from
Feb 2, 2022

Conversation

zeroshine
Copy link
Contributor

@zeroshine zeroshine commented Jan 28, 2022

The original way handle required or non-nullable UserInputError is not considering List Type variable. It returns INTERNAL_SERVER_ERROR instead of BAD_USER_INPUT

The result of query

query ( x: [String]!) {
  hello(x:$x)
}

is

{
  errors: [
    {
      message: "Variable "$x" of required type "[String]!" was not provided.",
      extensions: {
        code: "INTERNAL_SERVER_ERROR"
      }
    }
  ]
}

should be

{
  errors: [
    {
      message: "Variable "$x" of required type "[String]!" was not provided.",
      extensions: {
        code: "BAD_USER_INPUT"
      }
    }
  ]
}

check node type and replace message.startWith with message.match to handle List type and single variable

@apollo-cla
Copy link

@zeroshine: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

e.nodes[0].variable.name.value +
'" of non-null type ".*!" must not be null.',
),
))
Copy link
Member

@IvanGoncharov IvanGoncharov Jan 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please use startsWith?
Passing client-controlled values into RegExp is bad practice(even if they are sanitized by GraphQL parser) because it can lead to ReDoS.

Also can you please extract this check in separate function (e.g. isBadUserInputGraphQLError) that checks if GraphQLError matches?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix in 863fe02

Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok as a temporary solution until AS4 and ready to merge.

cc @glasser

@glasser
Copy link
Member

glasser commented Feb 2, 2022

Looks good. I rebased, added a test of another case I was curious about, and added a CHANGELOG.

Hopefully we will eventually be able to rely on a fix to graphql/graphql-js#3169 instead of doing this heuristic.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 2, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5abf963:

Sandbox Source
Apollo Server Typescript Configuration
Apollo Server Configuration

@glasser glasser merged commit 444c403 into apollographql:main Feb 2, 2022
@zeroshine zeroshine deleted the ListTypeError branch February 3, 2022 10:44
glasser added a commit that referenced this pull request Feb 23, 2022
…rInputError (#6066)

Co-authored-by: gchen02 <[email protected]>
Co-authored-by: David Glasser <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants