nuwave / lighthouse

A framework for serving GraphQL from Laravel
https://lighthouse-php.com
MIT License
3.37k stars 438 forks source link

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

Open renepardon opened 1 year ago

renepardon commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.