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

coerceVariableValues might log sensitive data in case of error #2629

Open
riedeljan opened this issue Jun 9, 2020 · 3 comments
Open

coerceVariableValues might log sensitive data in case of error #2629

riedeljan opened this issue Jun 9, 2020 · 3 comments

Comments

@riedeljan
Copy link

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
Copy link
Member

@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
Copy link
Author

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
Copy link

sgohlke commented Jun 10, 2020

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).

@IvanGoncharov IvanGoncharov added this to the post-16.0.0 milestone Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants