Support disabling literal stripping when extracting operations. by damondoucet · Pull Request #1703 · apollographql/apollo-tooling

damondoucet · 2019-12-06T03:23:24Z

Adds a flag --[no]-preserveStringAndNumericLiterals to client:extract
that controls whether literals are preserved in extracted queries. Prior to
this flag, object and list literals were already preserved; that's unaffected
by this change.

As noted in the linked issue, there are a few reasons to do this, but generally
they boil down to relying on the extracted queries for some functionality, and
literals in the queries controlling important behavior (e.g. generating mock data
for queries might rely on literal args).

A flag is preferred to changing the behavior outright since older clients likely
rely on the current behavior.

TODO:

Update CHANGELOG.md* with your change (include reference to issue & this PR)
Make sure all of the significant new logic is covered by tests
Rebase your changes on master so that they can be merged easily
Make sure all tests and linter rules pass

*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.

apollo-cla · 2019-12-06T03:23:26Z

@damondoucet: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

damondoucet · 2019-12-06T03:24:14Z

I left the push command alone, since the issue only references the extract
command, but I'd be happy to adjust that command as well if it's valuable!

damondoucet · 2019-12-06T03:30:49Z

I also wasn't sure about the casing of the no prefix - it looks a little funny
to me, but I figured I'd default to OCLIF's behavior unless encouraged otherwise. :)

jhampton · 2019-12-06T05:27:10Z

Thanks @damondoucet ! I'd like input from @jbaxleyiii and the team as to whether we should include the push command, but this looks clean to me.

Cheers!

jhampton · 2019-12-06T05:28:05Z

👍 LGTM

abernix

Thanks for opening this PR! I support this change and my suggestions below revolve purely around clarity of the flag name.

As some back-story, it's worth noting two things:

The literal-stripping was originally introduced to ensure that no personally identifiable information which might be encapsulated within such literals was inadvertently shipped to the Apollo Graph Manager operation registry (e.g. API keys or passwords in string literals).
The client:extract command was originally intended to be a more observable/debuggable version of the client:push command.

With that second bullet-point in mind, my initial reaction to this PR was to welcome it but suggest that we maintain parity between the client:push and client:extract commands (i.e. also extend this flag to client:push). Upon further consideration, I realized that would create confusion/problems since the operation registry safe-listing plugin itself reads from the manifest published with client:push. This means that the plugin would also need to have the same preserveLiterals option introduced to it and passed along to its own invocation of defaultOperationRegistrySignature on the receiving side (read: Apollo Server). Therefore, we should keep client:push as-is and not allow it to use this flag.

Tangentially (writing this here purely for posterity, not as something you as the PR author needs to act on), in a future version of this command (and the operation registry), I'm hopeful that we can make preserving all literals the default behavior. However, I think that would be best coupled with reminders to users to (perhaps a per-operation blessing of sorts? idk, a directive?) to not use string and numeric literals, but rather use variables whenever possible for their many benefits.

Anyhow, my suggestion is that we use a more clear flag name. But note that I didn't make all necessary suggestions for renaming it. Thoughts? (On any of this, but particularly the name. 😸 )

abernix · 2019-12-17T13:03:10Z

  static flags = {
-    ...ClientCommand.flags
+    ...ClientCommand.flags,
+    stripLiterals: flags.boolean({


Since "literals" also applies to object and list literals (which are not stripped), perhaps we should be a bit more clear with this option name. Even if it's a bit wordy, the intent is more clear, I think? (And its less necessary for someone to understand the notion of negated flags and perhaps more intrinsic understanding of the default behavior?). This change would require changing the default to false below and flipping other conditionals too, of course.)

Suggested change

stripLiterals: flags.boolean({

preserveStringAndNumericLiterals: flags.boolean({

abernix · 2019-12-17T13:06:33Z

+    stripLiterals: flags.boolean({
+      description:
+        "Whether to strip literals from extracted queries. DEFAULT: true",
+      default: true,


If heeding my suggestion above, this default would become false.

abernix · 2019-12-17T13:09:45Z

+    ...ClientCommand.flags,
+    stripLiterals: flags.boolean({
+      description:
+        "Whether to strip literals from extracted queries. DEFAULT: true",


Suggested change

"Whether to strip literals from extracted queries. DEFAULT: true",

"Disable redaction of string and numerical literals. Without this flag, these values will be replaced with empty strings (`''`) and zeroes (`0`) respectively. This redaction is intended to avoid inadvertently outputting potentially personally identifiable information (e.g. embedded passwords or API keys) into operation manifests.",

abernix · 2019-12-17T13:13:38Z

+      description:
+        "Whether to strip literals from extracted queries. DEFAULT: true",
+      default: true,
+      allowNo: true


I believe we could get rid of this if the default was false.

Suggested change

allowNo: true

abernix · 2019-12-17T13:14:19Z

        task: async ctx => {
-          ctx.operations = getOperationManifestFromProject(this.project);
+          ctx.operations = getOperationManifestFromProject(this.project, {
+            stripLiterals: flags.stripLiterals


I would also suggest renaming this to more accurately describe its scope:

Suggested change

stripLiterals: flags.stripLiterals

preserveStringAndNumericLiterals: flags.preserveStringAndNumericLiterals

abernix · 2019-12-17T13:14:54Z

 export function getOperationManifestFromProject(
-  project: GraphQLClientProject
+  project: GraphQLClientProject,
+  options: { stripLiterals: boolean } = { stripLiterals: true }


Suggested change

options: { stripLiterals: boolean } = { stripLiterals: true }

options: {

preserveStringAndNumericLiterals: boolean,

} = {

preserveStringAndNumericLiterals: true,

}

Presumably, the default should also flip to false? :)

abernix · 2019-12-17T13:15:39Z

      operationAST,
-      operationName
+      operationName,
+      { stripLiterals: options.stripLiterals }


Suggested change

{ stripLiterals: options.stripLiterals }

{

preserveStringAndNumericLiterals:

options.preserveStringAndNumericLiterals

}

damondoucet · 2019-12-18T02:31:57Z

Hey Jesse, thanks for getting back to me! So as not to leave this hanging - I had a chance to skim your feedback and it generally makes sense, but I won't be able to update this PR until the new year. It'll be a high priority for us then (1/7 or so). Happy holidays!

damondoucet · 2020-01-09T20:02:08Z

Thanks for the feedback @abernix. I read through and it all makes sense to me, so I've updated the PR with the corresponding changes. Let me know what you think!

damondoucet · 2020-01-15T23:14:25Z

bump @abernix - would be great to get this merged as soon as we can since it's blocking a high priority feature for us. thanks!

damondoucet · 2020-01-21T18:56:47Z

@abernix friendly bump - any updates on the timeline for getting this merged?

abernix

Thanks for the adjustments, and apologies for the delays in getting back to you.

I have one remaining thought and I think this is good to ship from my perspective.

(I sincerely apologize for not asking for this in my previous review. I think I got caught up with the flag name and also the holidays! If you're unable to make the change, I can make it this week and get it shipped.)

abernix

Yes, defaultOperationRegistrySignature is the public API and other packages are depending on it (internally and otherwise). We'll consider what changing those other packages to use the operationRegistrySignature method in the future looks like, but I think this is great for now.

We'll get this cut into a release as soon as possible. LGTM. Thanks so much! 😄

damondoucet · 2020-01-30T20:42:35Z

Woo, thanks!!

abernix · 2020-01-30T20:54:43Z

@damondoucet Can you try and see if this is working for you in the latest version of the just-published apollo (apollo@2.22.0)?

damondoucet · 2020-01-30T21:39:14Z

it works, thanks @abernix!

Damon Doucet added 3 commits December 5, 2019 18:54

Update operationId::defaultOperationRegistrySignature

1ae7f11

Add CLI flag, wire up

f9aa0a0

Update README

d78cce8

damondoucet changed the title ~~Fixes #1099: Support optionally stripping literals when extracting queries~~ Fixes #1099: Support disabling literal stripping when extracting queries Dec 6, 2019

Update CHANGELOG

fcc4724

jhampton requested a review from JakeDawkins December 6, 2019 05:24

jhampton added 🔨 cli related to the CLI itself 🎉 feature New addition or enhancement to existing solutions labels Dec 6, 2019

jhampton self-assigned this Dec 6, 2019

jhampton requested a review from glasser December 17, 2019 06:21

abernix reviewed Dec 17, 2019

View reviewed changes

Rename stripLiterals to preserveStringAndNumericLiterals; flip default

af125d5

damondoucet requested a review from abernix January 9, 2020 20:01

abernix reviewed Jan 28, 2020

View reviewed changes

Comment thread packages/apollo-graphql/src/operationId.ts

abernix assigned abernix and jhampton and unassigned jhampton Jan 28, 2020

Add operationRegistrySignature

9d0c5f0

damondoucet requested a review from abernix January 28, 2020 19:49

jhampton approved these changes Jan 30, 2020

View reviewed changes

Comment thread packages/apollo-graphql/src/operationId.ts

abernix approved these changes Jan 30, 2020

View reviewed changes

abernix changed the title ~~Fixes #1099: Support disabling literal stripping when extracting queries~~ Support disabling literal stripping when extracting operations. Jan 30, 2020

abernix merged commit bad312d into apollographql:master Jan 30, 2020

	stripLiterals: flags.boolean({
	preserveStringAndNumericLiterals: flags.boolean({

	"Whether to strip literals from extracted queries. DEFAULT: true",
	"Disable redaction of string and numerical literals. Without this flag, these values will be replaced with empty strings (`''`) and zeroes (`0`) respectively. This redaction is intended to avoid inadvertently outputting potentially personally identifiable information (e.g. embedded passwords or API keys) into operation manifests.",

	stripLiterals: flags.stripLiterals
	preserveStringAndNumericLiterals: flags.preserveStringAndNumericLiterals

-      { stripLiterals: options.stripLiterals }
+      {
+        preserveStringAndNumericLiterals:
+          options.preserveStringAndNumericLiterals
+      }

Conversation

damondoucet commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apollo-cla commented Dec 6, 2019

Uh oh!

damondoucet commented Dec 6, 2019

Uh oh!

damondoucet commented Dec 6, 2019

Uh oh!

jhampton commented Dec 6, 2019

Uh oh!

jhampton commented Dec 6, 2019

Uh oh!

abernix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damondoucet commented Dec 18, 2019

Uh oh!

damondoucet commented Jan 9, 2020

Uh oh!

damondoucet commented Jan 15, 2020

Uh oh!

damondoucet commented Jan 21, 2020

Uh oh!

abernix left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

abernix left a comment

Choose a reason for hiding this comment

Uh oh!

damondoucet commented Jan 30, 2020

Uh oh!

abernix commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damondoucet commented Jan 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

damondoucet commented Dec 6, 2019 •

edited

Loading

abernix left a comment •

edited

Loading

abernix commented Jan 30, 2020 •

edited

Loading