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

Add a new @sensitive directive to prevent marked fields from logging #2373

Open
renepardon opened this issue Mar 29, 2023 · 2 comments
Open
Labels
enhancement A feature or improvement

Comments

@renepardon
Copy link

What problem does this feature proposal attempt to solve?

Right now, sensitve information like tokens, password, bank account data etc. is logged with the incoming request when using LogGraphQLQueries class.

Which possible solutions should be considered?

A new directive like @sensitive on argument/field level should mark fields as such and then prevents them from being logged as plain text.

directive @sensitive(
  "An optional reason why the field is marked as sensitive"
  reason: String
) on ARGUMENT_DEFINITION
type Mutation {
  login(
    email:    String! @sensitive(reason: "should not be logged")
    password: String! @sensitive(reason: "it's a password")
  ): LoginResponse
}

The logging class should then check if the request variables contain @sensitive directive and if reason is good enough, they should not appear in log files ;)

Some default fields might be added anyway, just in case. E.g.: password password_confirmation So no matter if the directive is defined at any field, those passwords will and should never appear in log files.

@spawnia spawnia added the enhancement A feature or improvement label Mar 29, 2023
@spawnia
Copy link
Collaborator

spawnia commented Mar 29, 2023

Doing this right is a tricky, since LogGraphQLQueries just looks at the plain JSON parameters.

Because GraphQL values can be passed as literals inside the query string, we need access to the parsed query string. I think we would have to rebuild the logging functionality as a listener to the StartExecution event, which holds structured data. Logging would then not happen for queries that are entirely invalid, e.g. missing or misspellt parameters, or queries that do not parse, e.g. GraphQL syntax errors.

The next challenge is to correlate variables with argument/input field definitions, a process which is currently handled by webonyx/graphql-php. If we just look at variable names without considering where they land in the schema, we run into the issue that variable names are controlled by the client. For example, they could do a mutation like this: mutation ($innocuous: String!) { login(password: $innocuous) }. A suitable representation of args would be the ArgumentSet object, but that is only available inside of resolvers. We could of course do logging inside of a resolver, but that would give us access only to the variables of one field - not to all that were passed.

Due to the complexities involved, perhaps a simple list of variable names to exclude is actually the way to go - even though far from perfect, it might just be good enough/better than nothing.

@renepardon
Copy link
Author

So we would still end up in a "blacklist" of fields/arguments which could be configured inside of config/lighthouse.php to prevent them from being logged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement
Projects
None yet
Development

No branches or pull requests

2 participants