-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[test] vitest can leak secrets in pipeline output #29630
Comments
/cc @jeremymeng |
I wonder whether we should remove the |
I feel like there are definitely production use cases where customers would want to see the content of the request in an error case. Sometimes there may be secrets in the response as well, which is an even more useful property. Maybe it would be possible to sub in a different mock implementation of |
I am a little concerned because error objects most likely get logged all the time. |
Yeah I agree that is a concern. I guess there are a few different ways people could log the error:
|
…erable (#30518) ### Packages impacted by this PR - `@azure/core-rest-pipeline` - `@typespec/ts-http-runtime` ### Issues associated with this PR - See e.g. #30247 - Also #29910 - And #29630 ### Describe the problem that is addressed by this PR Taken from my last PR #30483: > `PipelineRequest` and `PipelineResponse` objects may contain secrets in the request URL or headers. This is problematic since `RestError` objects are often logged. While we override `util.inspect.custom` to sanitize log output in Node, this does not work in all situations and environments. For example: > - `console.log` in browser does not respect `util.inspect.custom`, meaning secrets could be logged to the browser console. Other non-Node environments also may not respect this Node-specific functionality. > - JSON serialization of `RestError` objects. Calling `JSON.stringify` on the `RestError` currently results in an object that contains unsanitized secrets. We have encountered scenarios where the JSON serialization is logged, for example with vitest. This PR fixes this issue by making the `request` and `response` properties **non-enumerable**. This means they are ignored by JSON.stringify and `Object.entries`/`Object.keys`, but the properties are still directly accessible. This should prevent any potential sensitive information being accidentally logged in the majority of scenarios. I think this is a better balance of not breaking folks versus improving security than the approach in #30483.
When a test fails with an error,
vitest
outputs a JSON serialization of the error object to console. This is helpful when debugging, but can cause information that would have otherwise been sanitized to be output in pipeline runs, like in this example where an access token showed up (MS internal link): https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3768430&view=logs&j=8f098e13-557e-5a76-9331-06424800e0fa&t=5005ede9-2880-5bbf-ce42-ae979164a4d1&l=105. Is it possible to suppress this output or sanitize it somehow in the pipeline?Cc @mpodwysocki
The text was updated successfully, but these errors were encountered: