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

ApolloServerPlugin should be get info about the actual Query/Mutation, not just the Operation #4910

Closed
maclockard opened this issue Feb 9, 2021 · 5 comments

Comments

@maclockard
Copy link

I'm trying to implement a directive + plugin combo for safe logging. Roughly the idea being that you can add a @logSafe directive to a query/mutation definition that would allow you to declare what arguments are safe to log. For example:

type Query {
    getAFoo(fooId: String!, superSecretKey: String!): Foo! @logSafe(safeArgs: "[fooId]")
}

Which would allow one to implement a ApolloServerPlugin that could log information about a request in the executionDidStart lifecycle hook checking the @logSafe directive for whether or not a given argument is safe to log.

The issue with this approach is that ApolloServerPlugin lifecycle hooks only get the operation definition not the query definition, which looks like the following:

query MySpecificFooOperation($fooId: String!, $superSecretKey: String!) {
  getAFoo(fooId: $fooId, superSecretKey: $superSecretKey) {
    id
    someOtherField
    etc
  }
}

The plugin receives the following information on operations.

export interface OperationDefinitionNode {
  readonly kind: 'OperationDefinition';
  readonly loc?: Location;
  readonly operation: OperationTypeNode;
  readonly name?: NameNode;
  readonly variableDefinitions?: ReadonlyArray<VariableDefinitionNode>;
  readonly directives?: ReadonlyArray<DirectiveNode>;
  readonly selectionSet: SelectionSetNode;
}
export type OperationTypeNode = 'query' | 'mutation' | 'subscription';

The above directives array only contains directives declared on the operation which is usually declared client side, so not suitable for the safe logging use case.

Additionally, there does not seem to be a good way to get from operation definition to query field definition, which would contain the correct directives. The only way I could see to do it right now is to somehow manually parse the source of the operation for the relevant queries, then check against the schema. This feels pretty error prone, additionally would have a perf cost since we would be parsing the operation twice, once by apollo and a second time by this plugin.

Ideally the executionDidStart lifecycle hook could receive a list of parsed queries and their relevant metadata. Even getting access to the relevant GraphQLField for the queries would be good enough, since then I could add something to the extension field via an apollo schema visitor.

@glasser
Copy link
Member

glasser commented Feb 9, 2021

I'm a bit confused about your terminology here. I think when you say by "query field definition" you mean what we usually call the "schema"?

The GraphQLRequestContext passed to all the handlers does contain schema: GraphQLSchema, which is graphql-js's form of your schema, which is I think what you're talking about. Assuming your schema was created from parsing a schema language document, you can look at stuff like requestContext.schema.getType("Query")?.getFields()["getAFoo"]?.astNode?.directives. Is that what you're looking for? If so I think the current API supports this, so I'm going to close this issue for now. Let me know if I'm misinterpreting!

A different approach to your problem is that you could have a special scalar type scalar SensitiveString and have your sensitive arguments be of that type. Then you just need to look at the declared type name of the variable. That does mean that whether you consider an argument to be sensitive is visible to folks writing queries, which may be a positive or negative change.

@glasser glasser closed this as completed Feb 9, 2021
@maclockard
Copy link
Author

@glasser The schema is available in the request context, however, the code snippet you provided would not work without re-parsing the operation passed on the context.

requestContext.schema.getType("Query")?.getFields()["getAFoo"]?.astNode?.directives

The missing piece of information preventing me from using this snippet is the string "getAFoo". The GraphQLRequestContext only contains the operation MySpecificFooOperation. There are not any fields included on the context or the OperationDefinitionNode that I can use to link this operation to the query getAFoo in the schema. Effectively, all I'm asking for is the string "getAFoo" to appear somewhere on the context so I can use your code snippet.

The only way I can see getting this working right now would be to somehow parse requestContext.operation.loc.source.body and extract the query name getAFoo from the operation that way. However, this approach is pretty heavyweight and has perf tradeoffs.

The scalar type SensitiveString is an interesting approach, but only really works in cases that you can make the assumption that arguments are safe to log by default. For my use case, the assumption needs to be that arguments are unsafe by default. If I went that way, I would end up needing safe scalar variants of every possible type.

I apologize for using any confusing terminology, when dealing with graphQL it can be struggle to just not use the work 'query' for everything haha.

@maclockard
Copy link
Author

oh, additionally, the context currently only has info on the variables associated with the operation, not the query arguments. Using a modified version of the operation I included above:

query MySpecificFooOperation($myFooId: String!) {
  getAFoo(fooId: $myFooId, fooFamily: "bar") {
    id
    someOtherField
    etc
  }
}

The context currently has info on the variable myFooId and the passed values, but not on the arguments fooId and fooFamily and their respective passed values. I'm a bit shaky on the terms variable vs argument, but these seem to be the terms used in the code base to refer to these separate but related concepts.

@glasser
Copy link
Member

glasser commented Feb 9, 2021

You can find the rest of the parsed query from the OperationDefinitionNode by looking underselectionSet. In GraphQL, the "section set" is "the thing in curly braces that's a list of fields, etc".

So what you want to do is walk the operation's tree while using the GraphQLSchema to understand the types that you're looking at. graphql-js, the core GraphQL execution library that Apollo Server is built on, provides a tool for doing this called visitWithTypeInfo. It's not super well documented but it should do the trick: you write a visitor that triggers on Fields in your operation, and uses the TypeInfo object to be told the corresponding GraphQLField. (BTW you can also put directives on argument definitions if you'd rather do that than need to repeat the argument name in the value.)

@maclockard
Copy link
Author

@glasser looks like selectionSet was exactly what I was looking for! thank you for bearing with my incorrect vocab. Will check out visitWithTypeInfo.

Also, I had originally wanted to put the directive on the argument definition but unfortunately we are using type-graphql and they don't yet support directives on arguments: MichalLytek/type-graphql#77 (comment). I think there is some work on the spec going on rn to make it so that certain directive types are available outside of the SDL.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants