-
Notifications
You must be signed in to change notification settings - Fork 646
feat: add --exclude parameter support for IaC scanning [IDE-1462] #6172
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
base: main
Are you sure you want to change the base?
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
|
d7fa6c9
to
2be5d61
Compare
Please note that the package/dependency bumps also fixes |
ca2bb60
to
11a0fa5
Compare
Thank you @bastiandoetsch for the contribution. Is there any reason why the test description on how to manually test this change is not automated as an acceptance test? This looks pretty straight forward and will help detect regression in the future. |
@@ -1,67 +1,67 @@ | |||
{ | |||
"_meta": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is this change related to the feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know where it comes from.
@@ -1,120 +1,120 @@ | |||
{ | |||
"_meta": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is this change related to the feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know where it comes from.
@@ -1,120 +1,120 @@ | |||
{ | |||
"_meta": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is this change related to the feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know where it comes from.
const normalizedFilePath = path.normalize(relativePath); | ||
|
||
// Check if the file path is exactly the exclude pattern | ||
if (normalizedFilePath === normalizedExcludePattern) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: This seems to implement --exclude
different to the existing implementation in test. Is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The corresponding implementation lives here.
IMHO we need to ensure consistency between our implementations when we have the same arguments being used. Otherwise it can easily get confusing and decrease the UX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with the test implementation, but it is the minimally invasive approach with regards to IaC. I'm pretty sure, the test implementation does filtering based on the .snyk
file. Where do you see issues with the current logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description of --exclude
for test states the following:
Can be used with --all-projects and --yarn-workspaces to indicate directory names and file names to exclude. Must be comma-separated, and cannot include a path.
Example: $ snyk test --all-projects --exclude=dir1,file2
This will exclude any directories and files named dir1 and file2 when scanning for project manifest files such as: ./dir1, ./src/dir1, ./file2, ./src/file2 and so on.
Note: --exclude=dir1 will find both ./dir1, and ./src/dir1.
However, --exclude=./src/dir1 will result in an error because it includes a path.
From the implementation here, it explicitly requires a path while the other doesn't allow it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at the linked code, the comparison is an includes
rather than an equals.
added an acceptance test. will not update this pr more this week, though. |
Pull Request Submission Checklist
What does this PR do?
This PR adds support for the
--exclude
parameter in IaC scanning commands, allowing users to exclude specific files and directories from IaC scans. The implementation supports both the legacy IaC flow (v1) and the new IaC engine (v2).Key Features:
--exclude=dir1,dir2,file.tf
)path.normalize()
andpath.relative()
Where should the reviewer start?
src/cli/commands/test/iac/local-execution/types.ts
andsrc/lib/iac/test/v2/types.ts
to understand the new exclude parametersrc/cli/commands/test/iac/local-execution/directory-loader.ts
for the main exclude logicsrc/cli/main.ts
for CLI validation updatestest/jest/unit/iac/directory-loader.spec.ts
for comprehensive test coverageHow should this be manually tested?
What's the product update that needs to be communicated to CLI users?
New Feature: Exclude files and directories from IaC scans
Users can now exclude specific files and directories from IaC scans using the
--exclude
parameter:This is particularly useful for excluding test files, temporary files, or other non-production IaC files from security scans.
Risk assessment: Low
Any background context you want to provide?
This feature was requested to allow users to exclude test files, temporary files, and other non-production IaC files from security scans. The implementation uses proper filesystem semantics to ensure reliable path matching across different operating systems.
What are the relevant tickets?
Files Changed
src/cli/commands/test/iac/local-execution/
- Legacy IaC flow implementationsrc/cli/commands/test/iac/v2/
- New IaC flow implementationsrc/lib/iac/test/v2/
- IaC v2 types and scanning logicsrc/cli/main.ts
- CLI validation updatestest/jest/
- Comprehensive test coverage (316 new lines of tests)