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

sast-snyk-check: Added functionatlity to ignore directories and files #1803

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jperezdealgaba
Copy link
Contributor

Resolves: https://issues.redhat.com/browse/OSH-795

Making use of the snyk ignore functionality, users are now able to specify the IGNORE parameter with a list of comma-separated directories and folders and they will be ignored from the scans.

@jperezdealgaba jperezdealgaba requested review from kdudka and a team as code owners January 9, 2025 13:05
@jperezdealgaba jperezdealgaba force-pushed the remove-dir-snyk branch 6 times, most recently from 5f7fbad to b4f75de Compare January 9, 2025 14:08
@kdudka kdudka changed the title sast-snyk-check: Added functionatlity to ignore directories and folders sast-snyk-check: Added functionatlity to ignore directories and files Jan 9, 2025
@@ -57,6 +57,10 @@ spec:
type: string
description: Write excluded records in file. Useful for auditing (defaults to false).
default: "false"
- name: IGNORE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would name the parameter IGNORE_FILE_PATHS to make it more specific.

paths="(${IGNORE//,/ })" # Split by comma into an array
command=""
for path in "${paths[@]}"; do
command+="snyk ignore --file-path=source/$path && "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage of chaining the snyk ignore commands with && in a string, which is expanded by eval later on? Cannot we just run the snyk ignore commands directly? Note that the script is executed with set -e, so the chaining with && should not be needed.

Copy link
Contributor Author

@jperezdealgaba jperezdealgaba Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no special advantage of doing that. I was just following Snyk docs: https://docs.snyk.io/snyk-cli/scan-and-maintain-projects-using-the-cli/snyk-cli-for-snyk-code/exclude-directories-and-files-from-snyk-code-cli-tests but we can execute the commands one by one.

@@ -195,8 +213,8 @@ spec:
# Generation of scan stats
total_files=$(jq '[.runs[0].properties.coverage[].files] | add' "${SOURCE_CODE_DIR}"/sast_snyk_check_out.json)
supported_files=$(jq '[.runs[0].properties.coverage[] | select(.type == "SUPPORTED") | .files] | add' "${SOURCE_CODE_DIR}"/sast_snyk_check_out.json)
total_files=$(jq '[.runs[0].properties.coverage[].files // 0] | add' "${SOURCE_CODE_DIR}"/sast_snyk_check_out.json)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is unclear to me how this case is specific to the ignored paths feature. Cannot the same happen also when the files that we ignore simply do not exist (without specifying the parameter)? How do these cases differ from each other?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While testing this, if the scan had results and they were ignored with the .snyk file policy, the resulted null value was never converted into 0 in the following line:
https://github.com/konflux-ci/build-definitions/blob/f1fa9480c40f63aae17974aa11d1b9d23c20f582/task/sast-snyk-check-oci-ta/0.3/sast-snyk-check-oci-ta.yaml#L243C10-L243C40

With this change, the null value was converted into 0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the ignored files to be excluded from scanning rather than being excluded from scan results.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And they are, the files are ignored from the scan by looking at the .snyk file and ignoring them.
Will find the root cause of this

@@ -135,13 +141,25 @@ spec:
echo "${TEST_OUTPUT}" | tee "$(results.TEST_OUTPUT.path)"
exit 0
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems to be removed by mistake.

@jperezdealgaba jperezdealgaba marked this pull request as draft January 10, 2025 14:32
@jperezdealgaba
Copy link
Contributor Author

Moving to draft while testing new changes...

Resolves: https://issues.redhat.com/browse/OSH-795

The parameter IGNORE has been added where users can input a list of files and directories (comma-separated) and they will be ignored using the snyk ignore functionality
Resolves: https://issues.redhat.com/browse/OSH-795

The jq command returned null when the scan ignored files and there were findings. That null value is now converted into 0
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.

2 participants