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

Variable validation errors leak entire variable in statusMessage telemetry. #1182

Open
Sam-tesouro opened this issue Sep 17, 2024 · 4 comments
Labels
enhancement New feature or request internally-reviewed The issue has been reviewed internally.

Comments

@Sam-tesouro
Copy link

Sam-tesouro commented Sep 17, 2024

Component(s)

router, studio, otelcollector

Is your feature request related to a problem? Please describe.

When a request to a federated Supergraph doesn't match spec and an $variable is used for input the entire $variable is leaked on the statusMessage via telemetry, potentially leaking sensitive information.
https://github.com/wundergraph/graphql-go-tools/blob/master/v2/pkg/variablesvalidation/variablesvalidation.go

The response will also contain sensitive information if the $variable has sensitive information included.

Describe the solution you'd like

It would be amazing to have a config item where I could globally enable/disable said $variable being included in statusMessage on telemetry and/or the Error message on the response.

Even better would be some way to define types containing PII that would then be automatically masked, either via router config or schema annotation, whilst still providing the additional benefit of returning the input $variable context.

Describe alternatives you've considered

It's possible to resolve the tracing leaks with an OTEL collector filter processor, but that unfortunately doesn't resolve the response also potentially including sensitive data.

Additional context

variableContent on the message leaks.

func (v *variablesVisitor) renderVariableInvalidObjectTypeError(typeName []byte, variablesNode astjson.Node) {
	out := &bytes.Buffer{}
	err := v.variables.PrintNode(variablesNode, out)
	if err != nil {
		v.err = err
		return
	}
	variableContent := out.String()
	v.err = &InvalidVariableError{
		Message: fmt.Sprintf(`Variable "$%s" got invalid value %s; Expected type "%s" to be an object.`, string(v.currentVariableName), variableContent, string(typeName)),
	}
}

func (v *variablesVisitor) renderVariableRequiredNotProvidedError(fieldName []byte, typeRef int) {
	out := &bytes.Buffer{}
	err := v.variables.PrintNode(v.variables.Nodes[v.currentVariableJsonNodeRef], out)
	if err != nil {
		v.err = err
		return
	}
	variableContent := out.String()
	out.Reset()
	err = v.definition.PrintType(typeRef, out)
	if err != nil {
		v.err = err
		return
	}
	v.err = &InvalidVariableError{
		Message: fmt.Sprintf(`Variable "$%s" got invalid value %s; Field "%s" of required type "%s" was not provided.`, string(v.currentVariableName), variableContent, string(fieldName), out.String()),
	}
}

Really appreciate the work y'all are doing!

@Sam-tesouro Sam-tesouro added the enhancement New feature or request label Sep 17, 2024
Copy link

WunderGraph commits fully to Open Source and we want to make sure that we can help you as fast as possible.
The roadmap is driven by our customers and we have to prioritize issues that are important to them.
You can influence the priority by becoming a customer. Please contact us here.

@jensneuse
Copy link
Member

Hey @Sam-tesouro we've discussed this internally.
What you could do is propose a PR on go tools that removes the verbose response, with a flag (debug) to re-enable it.
Once this is merged in, you can add it in a second PR to cosmo and we discuss how the debug flag can be passed on.

In general, our idea is that we can run the Router in "default" mode with a less verbose error message for compliance reasons, but still have an option to run the router in "dev mode" which is more verbose.

If you check the configuration (https://cosmo-docs.wundergraph.com/router/configuration), we've already got DEV_MODE, so we could apply it in this case.

Let me know what you think?

@Sam-tesouro
Copy link
Author

Good day @jensneuse, that sounds ideal to me. I will go ahead and work on an implementation for your review and touch base as necessary.

Thank you!

@jensneuse
Copy link
Member

Good day @jensneuse, that sounds ideal to me. I will go ahead and work on an implementation for your review and touch base as necessary.

Thank you!

sounds fantastic, looking forward to it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internally-reviewed The issue has been reviewed internally.
Projects
None yet
Development

No branches or pull requests

2 participants