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

GraphQL deployed to production reveals hints to other types when one not found #4275

Closed
cannikin opened this issue Jan 26, 2022 · 10 comments · Fixed by #7291
Closed

GraphQL deployed to production reveals hints to other types when one not found #4275

cannikin opened this issue Jan 26, 2022 · 10 comments · Fixed by #7291
Assignees
Labels
bug/confirmed We have confirmed this is a bug topic/api topic/graphql

Comments

@cannikin
Copy link
Member

cannikin commented Jan 26, 2022

Given the following SDL:

export const schema = gql`
  type Account {
    id: String!
    address: String!
    createdAt: DateTime!
  }

  input CreateAccountInput {
    address: String!
  }

  type Mutation {
    account(input: CreateAccountInput!): Account @requireAuth
  }
`

If you make a mutation request for a type that does not exist (there is no createAccount type):

mutation CreateAccountMutation($input: String!) {
  createAccount(input: $input) {
    id
  }
}

The response contains suggestions for other endpoints which do exist:

{
	"errors": [
		{
			"message": "Cannot query field \"createAccount\" on type \"Mutation\". Did you mean \"account\"?",
			"locations": [
				{
					"line": 2,
					"column": 3
				}
			]
		}
	]
}

This response is great in development. But, since we disable introspection in production to be default-secure, this feels like a security risk as it gives away other endpoints to start probing.

@cannikin
Copy link
Member Author

/cc @dthyresson

@dthyresson
Copy link
Contributor

Looking at the useMaskedError envelop plugin, my hypothesis is that error masking only happens in the execution phase and these errors happen in either the parse or maybe validate phase and as such are not masked, but I will need to check with @dotansimha and @n1ru4l to confirm this.

@dthyresson
Copy link
Contributor

I should clarify:

https://github.com/dotansimha/envelop/blob/a685d98ea09e1870e980ce5ce2eea2b3bac7d98f/packages/core/src/plugins/use-masked-errors.ts#L73

Masking looks to be done at the execute and subscribe phases.

@n1ru4l
Copy link
Contributor

n1ru4l commented Jan 27, 2022

useMaskedError is not related to introspection at all. useMaskedErrors hides all outgoing errors occurring during the execution phase whose original error is not a GraphQLError instance.

This is an unsolved issue within graphql-js as this is raised by a validation rule during validate. See graphql/graphql-js#2247

In general, I think disabling introspection gives a false sense of security.

Theoretically, we could create an envelop plugin for masking the "did you mean x" suggestions by replacing the existing validation rules.

If you really want to achieve full protection of arbitrary GraphQL operations persisted queries/operations should be considered, as disabling introspection/"did you mean x" is just a light layer of security that is still pretty vulnerable to brute force.

@dthyresson
Copy link
Contributor

dthyresson commented Jan 27, 2022

Theoretically, we could create an envelop plugin for masking the "did you mean x" suggestions by replacing the existing validation rules.

Thanks @n1ru4l -- I think this is the concern @cannikin has -- which seems valid -- that some schema info is being leaked here with the "did you mean x" that ideally Redwood would mask.

I wonder if a "workaround" would be in the formatError.... oh I just realized this isn't caught.

But if useMaskedError was also used onValidate and the formatError

export const formatError: FormatErrorHandler = (err: any, message: string) => {
did some regex on the message to mask, might that work?

Edit: Now that I think on it, it might not be possible. But ..

Theoretically, we could create an envelop plugin for masking the "did you mean x" suggestions by replacing the existing validation rules

"by replacing the existing validation rules"

What might that look like, happy to take on a PR :)

@cannikin
Copy link
Member Author

Huh, just learning about persisted queries right now, that could be interesting. The Cell contains the full query, but at build time we could SHA hash the query and then replace the cell query with that hash string instead. Makes it much more difficult for someone to browse through the client code and find types/fields to mix and match and probe endpoints...

Having your API be effectively single, pre-defined endpoints that always return the same data sounds familiar...😉

@n1ru4l
Copy link
Contributor

n1ru4l commented Jan 27, 2022

@dthyresson You should be able to find all the affected validation rules by searching for didYouMean within the graphql-js repository: https://github.com/graphql/graphql-js/search?q=didYouMean

Seems like that are quite a few, so a lot of logic to copy/paste and maintain. A drawback would be high maintenance effort. 🤔

Also, the messages don't follow a specific pattern, so detecting the exact errors that leak schema information also requires maintaining a list of strings that needs to be double-checked and adjusted for each graphql-js release (in the worst case).

Unfortunately, there is no nice and straightforward solution for handling this.

Maybe adding a configuration option that is set on the ValidationContext and available in the validation rules might be a possible solution for disabling/enabling "did you mean x" within graphql-js: https://github.com/graphql/graphql-js/blob/47bd8c8897c72d3efc17ecb1599a95cee6bac5e8/src/validation/validate.ts#L42

Alternatively all errors that arise during validate in a production deployment could be interpreted as unexpected errors and masked by the server. This could, however, result in a frustrating experience when you want to run arbitrary operations in production, e.g. for debugging purposes.


@cannikin Usually you would only enable persisted operations for production environments 😉 . I agree with you that this also implies new problems that need to be solved. I actually never used them in production myself. At the same time I also never perceived exposing the introspection/"did you mean x" as a security issue in my projects.

@dthyresson
Copy link
Contributor

@n1ru4l has a PR graphql/graphql-js#3467 that adds an option to include the "didYouMean" info to the validation messages.

If added to graphql-js then we can set that as a sensible default.

Until then, there isn't a great way for v1 to address this issues.

@jtoar let's make this v2 and we'll watch that PR.

@jtoar jtoar added the next label Jan 31, 2022
@cannikin
Copy link
Member Author

In addition to this, would it be possible to exclude any helpful text from GraphQL about malformed queries? It's invaluable when in development mode, but in production, a malicious actor can use those hints to completely generate a valid request from scratch with zero other knowledge—GraphQL is constantly telling you what arguments are required, what their data type is, what field names are missing, etc. Here's a couple:

"Field \"users\" argument \"id\" of type \"String!\" is required, but it was not provided."
"Cannot query field \"types\" on type \"User\". Did you mean \"type\"?"
"Cannot query field \"id\" on type \"User\"."
"Variable \"$id\" is never used in operation \"FindUsers\"."

You almost don't need introspection when the error messages basically do it for you. :( In a RESTful land I'd return a 400 Bad Request for anything like this...

I can see enabling this if you have other clients than your own, and they need help consuming the API, but if it's just you using it then seems like you should expose as little information about how it works to prying eyes as possible. Or am I being paranoid?

@jtoar jtoar removed the next label May 5, 2022
@dthyresson
Copy link
Contributor

If RedwoodJS updates to use GraphQL Armor, there is a plugin that "blocks field suggestions":

https://github.com/Escape-Technologies/graphql-armor/tree/main/packages/plugins/block-field-suggestions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug topic/api topic/graphql
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants