Skip to content

Create snyk-security.yml#116

Closed
tomersein wants to merge 1 commit into
mainfrom
tomersein-patch-1
Closed

Create snyk-security.yml#116
tomersein wants to merge 1 commit into
mainfrom
tomersein-patch-1

Conversation

@tomersein

Copy link
Copy Markdown
Contributor

Pull Request

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@tomersein tomersein self-assigned this Jun 19, 2025
@github-actions

Copy link
Copy Markdown
  1. Review Summary: This PR adds a Snyk security workflow to the repository. It sets up Snyk CLI, runs security scans for code, open source dependencies, IaC, and containers, and uploads the Code scan results to GitHub Code Scanning.

  2. Critical Issues:

    • Exposed Credentials: While the SNYK_TOKEN is using a GitHub secret, the workflow comments directly mention that the token is the Snyk API token, which makes it a high value target. It should be mentioned that the token needs to be created, but not explicitly labeled for what service it is used.
    • Unnecessary || true: The use of || true after snyk code test and snyk iac test commands masks errors. This defeats the purpose of the security scan in a CI environment. A failed scan should halt the build.
  3. Suggestions:

    1. Remove || true: Remove || true from the snyk code test and snyk iac test commands to ensure the workflow fails when vulnerabilities are found. Security scans failing should be treated as a critical issue.
    2. Improve SNYK_TOKEN comment: Rephrase the comment about SNYK_TOKEN to avoid explicitly calling it the "Snyk API token". Focus on how to create a generic secret and refer to the Snyk documentation/actions repo for specifics.
    3. Conditional Container Build & Scan: The docker build and snyk container monitor steps should only run if the repository actually contains a Dockerfile. Add a check like if: ${{ files().any(contains(github.event.commits.*.message, 'Dockerfile')) }} or if: ${{ contains(github.event.commits.*.added, 'Dockerfile') }}. This avoids errors in projects without containers.
    4. Configure Snyk monitor: The snyk monitor command should specify the --org flag if possible to avoid ambiguous org selection issues, especially if the user belongs to multiple organizations.
    5. Consider Snyk Action Parameters: Instead of running the CLI directly, explore more granular configuration of the snyk/actions/setup action. It might provide better integration and error handling.
  4. Positive Aspects: The workflow attempts to cover various aspects of security (SAST, SCA, IaC, Container). The use of github/codeql-action/upload-sarif to integrate Snyk Code results with GitHub Code Scanning is a good practice.

@codecov

codecov Bot commented Jun 19, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

see 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tomersein tomersein closed this Jun 19, 2025
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.

1 participant