graphql / graphql-js

A reference implementation of GraphQL for JavaScript
http://graphql.org/graphql-js/
MIT License
20.1k stars 2.03k forks source link

coerceVariableValues might log sensitive data in case of error #2629

Open riedeljan opened 4 years ago

riedeljan commented 4 years ago

The prefix in prefix + '; ' + error.message, (execution/values.js:141) logs the invalid input when a call to coerceVariableValues fails.

Since this might commonly fail, especially when invalid user input accidentialy reaches the API, according log messages can contain sensitive (user) data, if transmitted in the request:

While handling a GraphQL request the following error occurred: Variable "$myInput" got invalid value { abc: "xyz", address: { city: "SecretCity", country: "SecretCountry", state: "SecretState", streetName: "An Interesting Street", streetNumber: "42", zipCode: "12345" }, ... }; Field "abc" is not defined by type MyInput.

Please clarify how we could solve this problem (we can provide a PR too), or enlighten me if we happen to use the library in a wrong way.

IvanGoncharov commented 4 years ago

@riedeljan Thanks for reporting, it's a general issue that we need to look into. It's hard to balance good DX and not to leak sensitive data. What we can do in the future is to look into other API technologies/frameworks on how they handle such situations.

As a workaround, you can provide custom format function to your GraphQL server or logging framework and have some RegExp that sanitize this particular error message.

riedeljan commented 4 years ago

Thank you for the hint! I totally get that it's hard to balance from time to time.

Maybe this could be solved with some kind of implementation including different log-levels in the future. I'll keep my eyes open for upcoming features and watch this ticket! (:

sgohlke commented 4 years ago

Hello,

as this message is used to create a GraphQLError maybe it would be an idea to add the information about the variables to the error.extensions (i.e. error.extensions.variables). error.extensions is an optional parameter in the GraphQLError constructor. Implementations like apollo-server seem to handle it this way.

As suggested it is possible to sanitize the error message before log output is written but I'd prefer to have it fixed here (in values.flow.js).