Default to use private variables and headers #2472
Conversation
To avoid accidentally sending PII to Apollo's cloud services, this changes the default settings for the apollo-engine-reporting package to not send variable or header values corresponding to trace information. Variable names will still be sent, as with the `privateVariables: true` option. A future change should extend the functionality of these options by allowing users to pass in a function to assess whether a variable or header value / name should be reported.
|
Related to #1498 |
|
This doesn't add significant new logic, and there is currently a very limited test infrastructure to add onto. |
zionts
force-pushed
the
adam/19/3/aer/default-private
branch
from
c606791
to
8366c0f
Compare
| if (this.options.privateVariables !== true && o.variables) { | ||
| // Default: all variables are private; == checks for both null and undefined | ||
| if ( | ||
| (!this.options.privateVariables == null || |
There was a problem hiding this comment.
I think this should be an && (it is noticeably different from the other one above).
I think they can both be simplified as if (![null, undefined, true].contains(this.options.privateWhatever)).
There was a problem hiding this comment.
Typing made that a real hassle, but I can make it work.
| @@ -3,6 +3,8 @@ | |||
| ### vNEXT | |||
|
|
|||
| - Allow `GraphQLRequestListener` callbacks in plugins to depend on `this`. [PR #2470](https://github.com/apollographql/apollo-server/pull/2470) | |||
| - Default `privateVariables` and `privateHeaders` to 'true' to prevent sending potential PII to Apollo's cloud service | |||
There was a problem hiding this comment.
FWIW I don't understand how versioning works between different packages in this repo. This maybe should be a major version bump if there wasn't other packages involved, but maybe it goes in lockstep with other packages? @abernix thoughts?
There was a problem hiding this comment.
Is changing default behaviour always considered a major version bump?
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems suitable to put it into AS3.0 @abernix , how do I do that?
There was a problem hiding this comment.
@zionts I've added it to the 3.x GitHub milestone so we keep tabs on it.
There was a problem hiding this comment.
While maybe this should be a major version bump for apollo-engine-reporting specifically, I don't think it needs to be tied up in waiting for the entire apollo-server project to get to a new major version. Yes, this is a backwards incompatible change, but I think there's something special about code that talks directly to our hosted services. We have an ongoing problem where people send us data by accident, and it's worth making the change faster rather than slower. If the protobufs were slightly different we could actually make this default change on our server side, and maybe we would have!
Note that my goal for this change is for the only place where it affects users (the trace inspector in Engine) to link to docs explaining how to enable sending trace values, and it's just a one line change (which can also be included in the CHANGELOG)... not something that should make it ultra hard to upgrade. I think we should do this sooner rather than later, as part of @helenwh 's change to generally make variable value filtering more powerful.
|
Defaults were changed in PR #2931 |
To avoid accidentally sending PII to Apollo's cloud services, this
changes the default settings for the apollo-engine-reporting package to
not send variable or header values corresponding to trace information.
Variable names will still be sent, as with the
privateVariables: trueoption. A future change should extend the functionality of these options
by allowing users to pass in a function to assess whether a variable or
header value / name should be reported.
TODO: