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

Misconfigured sendVariableValues results in similar behavior as {all: true} #7812

Open
bryancuster opened this issue Jan 3, 2024 · 3 comments

Comments

@bryancuster
Copy link

bryancuster commented Jan 3, 2024

Issue Description

When configuring ApolloServerPluginUsageReporting, if you misconfigure sendVariableValues it seems to result in the same behavior as {all: true}.

Intended outcome

I think I would expect either an error due to the misconfiguration or a better logical fallback inline with {none: true}.

Actual outcome

We mistakenly set sendVariableValues to a function instead of transform and then saw all variables sent to apollo studio.

      sendVariableValues: ({ operationString, variables }) => {
        if (includes(operationString, "search")) {
          return { searchTerm: variables?.searchTerm };
        }

        // Return an empty object or null for other queries to avoid logging any data
        return {};
      }

I can fork and add this to git if you'd like

  it('Random config does not filter variables', () => {
    // @ts-ignore
    expect(makeTraceDetails(variables, { literally: 'anything but a good value' })).toEqual(
      nonFilteredOutput,
    );
  });

Link to Reproduction

https://codesandbox.io/p/devbox/stupefied-faraday-4vfmt4

Reproduction Steps

No response

@glasser
Copy link
Member

glasser commented Jan 3, 2024

Just to check — this is prevented by using TypeScript, right?

I'm not sure that "every single API needs to have runtime checks on every single argument to prevent misuse which could already be caught at compile time by the use of TypeScript (even just for typechecking on JS files, which it supports)" is the best use of resources.

@bryancuster
Copy link
Author

I believe this is the line of code in question, where if no valid config is given to exclude and it is also not undefined then it just sends the raw value.

@bryancuster
Copy link
Author

Just to check — this is prevented by using TypeScript, right?

That seems correct given the CodeSandbox. Our team uses just JS so didn't notice any issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants