PDE-5641: feat(core) capture CLI arguments in analytics safely #944
Conversation
Natay
changed the title
Natay
changed the title
packages/cli/src/utils/analytics.js
Outdated
| const doNotRecord = [ | ||
| 'key-value pairs...', | ||
| 'keys...', | ||
| 'path', | ||
| 'email', | ||
| 'message', | ||
| 'user', | ||
| 'account', | ||
| 'redirect-uri', | ||
| 'inputData', | ||
| ]; |
There was a problem hiding this comment.
It'd be easy to forget to update this list when developing a new CLI command or adding a command argument.
Or, do we need to capture the values of arguments and flags? I think we can learn enough by capturing only the keys of arguments and flags.
There was a problem hiding this comment.
I was thinking the extra detail enables more precise segmentation of user behavior but we don't need to be that granular right now! I'll update this!
If we want this in the future, do you think having the doNotRecord list as an exported asset in ZapierBaseCommand.js close to where new CLI arguments/commands are added would be a sufficient safeguard?
There was a problem hiding this comment.
Thanks for updating!
Regarding:
do you think having the
doNotRecordlist as an exported asset inZapierBaseCommand.jsclose to where new CLI arguments/commands are added would be a sufficient safeguard?
I still don't think that would be sufficient. I think an allowlist (like doRecord) instead of a denylist (doNotRecord) would be a better safeguard. So, even if we forget to update the allowlist, we won't accidentally send unwanted data to the analytics endpoint.
Capture the argument + flags keys ( no values ) that are being used
Related MR: https://gitlab.com/zapier/zapier/-/merge_requests/60772