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

fix: filter out file content from iac test --experimental analytics #1715

Closed
wants to merge 1 commit into from

Conversation

aron
Copy link
Contributor

@aron aron commented Mar 12, 2021

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

We have a minor bug in our Snyk CLI that when the snyk iac test --experimental command is run with a directory instead of a single file we include the iacDirFiles on the options object. This options object is then logged as part of the analytics flow and sent to Big Query. We do not want to be storing user file content in any part of our system so this needs to be filtered out here.

This PR filters the File content and JSON output from the iacDirFiles property added to the options object in the test command and adds a smoke test and unit test to assert that the content is not present.

This is not a long term strategy for solving this issue but fixes the immediate problem that prevents users from adopting the beta. In future we'll want to decouple the file + parsed content from any metadata and ensure that the file content is only passed where needed and discarded when used. We should also look into using a whitelist for the logger to only allow specific arguments to be logged.

Where should the reviewer start?

Start at the smoke tests then the test() function and finish up with the unit tests. The unit tests are really a temporary measure I think to ensure that the iacDirFiles object is clean, we can remove the tests when the flow has been refactored.

How should this be manually tested?

Run the following and verify that the "args" property under analytics is clean of file content.

node ./dist/cli iac test -d --experimental ./test/fixtures/iac/file-logging

@github-actions
Copy link
Contributor

github-actions bot commented Mar 12, 2021

Warnings
⚠️

Looks like you added a new Tap test. Consider making it a Jest test instead. See files like test/*.spec.ts for examples. Files found:

  • test/fixtures/iac/file-logging/file_content_logging.yaml
Messages
📖 You are modifying something in test/smoke directory, yet you are not on the branch starting with smoke/. You can prefix your branch with smoke/ and Smoke tests will trigger for this PR.

Generated by 🚫 dangerJS against 0a760fe

@aron aron force-pushed the fix/iac-analytics-logging branch 2 times, most recently from 7c45f51 to c6be3af Compare March 12, 2021 15:29
@aron aron changed the title chore: add regression tests for iac logging fix: filter out file content from iac test --experimental analytics Mar 12, 2021
@aron aron force-pushed the fix/iac-analytics-logging branch from 40dd18b to 67e296d Compare March 12, 2021 16:44
@aron aron requested a review from rontalx March 12, 2021 16:52
@aron aron force-pushed the fix/iac-analytics-logging branch from 67e296d to f42f87e Compare March 12, 2021 20:29
@rontalx rontalx marked this pull request as ready for review March 14, 2021 08:06
@rontalx rontalx requested a review from a team as a code owner March 14, 2021 08:06
@rontalx rontalx requested a review from a team March 14, 2021 08:06
@rontalx rontalx requested a review from a team as a code owner March 14, 2021 08:06
@aron aron requested a review from a team as a code owner March 14, 2021 08:12
@rontalx rontalx force-pushed the fix/iac-analytics-logging branch 2 times, most recently from 756bd6c to cdf1822 Compare March 14, 2021 08:14
@github-actions
Copy link
Contributor

Expected release notes (by @rontalx)

fixes:
file content in iac analytics (0a760fe)

  • I hereby acknowledge these release notes are 🥙 AWESOME 🥙

@rontalx
Copy link
Contributor

rontalx commented Mar 14, 2021

I mistakenly messed up the commits, re-opening PR here:
#1719

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

Successfully merging this pull request may close these issues.

3 participants