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

feat(cli): add include-dev-deps flag #4700

Merged
merged 21 commits into from
Jun 29, 2023

Conversation

DmitriyLewen
Copy link
Contributor

@DmitriyLewen DmitriyLewen commented Jun 23, 2023

Description

aquasecurity/go-dep-parser#237 adds Dev field for Package to mark dev dependencies.
Add include-dev-deps flag to work with these field.

These changes are currently only supported for npm.

Related issues

Related PRs

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@DmitriyLewen DmitriyLewen self-assigned this Jun 23, 2023
@DmitriyLewen DmitriyLewen changed the title feat: add Dev field for Package feat(nodejs): use Dev field for npm packages Jun 23, 2023
@DmitriyLewen DmitriyLewen changed the title feat(nodejs): use Dev field for npm packages feat(cli): add include-dev flag Jun 26, 2023
@DmitriyLewen DmitriyLewen marked this pull request as ready for review June 27, 2023 06:03
@knqyf263
Copy link
Collaborator

I asked someone to list candidates with the CLI flag name.

  • --scan-dev-deps: Indicates explicitly that you want to scan devDependencies. The "scan" part represents the main function of Trivy, and "-dev-deps" shows that this function is applied to devDependencies.
  • --include-dev-deps: Indicates that devDependencies are included in the scan when this flag is specified. The "include" part suggests the specification of additional targets.
  • --dev-deps-scan: This is another option with the same meaning as "--scan-dev-deps". However, with "dev-deps" at the beginning of the flag, users can quickly understand that this option is related to devDependencies.
  • --enable-dev-scan: This flag indicates that you are enabling the scanning of devDependencies. "Enable" suggests the activation of this particular function.
  • --dev-deps-detect: This flag indicates that you are detecting vulnerabilities in devDependencies. "Detect" represents the vulnerability detection feature of Trivy.
  • --check-dev-deps: "Check" suggests the action of inspecting or verifying, indicating that you are checking devDependencies.

@itaysk @DmitriyLewen Any preference? I'd vote on --include-dev-deps as it affects SBOM as well as vulnerability scanning, but it is not a strong opinion. I'm open to any options.

@DmitriyLewen
Copy link
Contributor Author

I think --include-dev-deps or --enable-dev-scan would be better. For other names users can this that we will only scan dev deps, but we will use prod + dev deps.

Thinking about SBOM, as you said - --include-dev-deps is better name.

@knqyf263
Copy link
Collaborator

@DmitriyLewen Let's go for --include-dev-deps, then.

@DmitriyLewen DmitriyLewen changed the title feat(cli): add include-dev flag feat(cli): add include-dev-deps flag Jun 29, 2023
@DmitriyLewen
Copy link
Contributor Author

@knqyf263 i updated flag name.

docs/docs/references/configuration/cli/trivy_filesystem.md Outdated Show resolved Hide resolved
docs/docs/scanner/vulnerability/language/nodejs.md Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't have to update this golden, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the same when i got error.
But we need to fix this golden file.

I added new Dev boolean field to Package - https://github.com/aquasecurity/trivy/pull/4700/files#diff-11d06805c304c7fe1d7be3057aede873f853bffbb46c6e997e92b1e7c60f80dfR72

To calculate ID for spdx package we use Package struct -

pkgID, err := calcPkgID(m.hasher, pkg)

That is why ID was changed.

About spdx tests - we overwrite hash fucntion. This fucntion only uses Name and FilePath -

str = v.(ftypes.Package).Name + v.(ftypes.Package).FilePath

Correctly me, if i missed something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, you're right. I forgot about how to calculate the SPDX package IDs. Thanks!

pkg/fanal/analyzer/language/analyze.go Outdated Show resolved Hide resolved
@knqyf263 knqyf263 added this pull request to the merge queue Jun 29, 2023
Merged via the queue into aquasecurity:main with commit 22463ab Jun 29, 2023
@DmitriyLewen DmitriyLewen deleted the feat/dev-field-for-npm branch June 30, 2023 03:24
AnaisUrlichs pushed a commit to AnaisUrlichs/trivy that referenced this pull request Aug 10, 2023
* add Dev field for Package

* fix integration test

* update docs

* feat(cli): add include-dev flag

* bump go-dep-parser

* update docs

* add integration test

* refactor

* refactor

* fix integration test

* refactor: rename flag to include-dev-deps

* update docs

* update docs

* filter dev deps when scanning packages

* add flag support for server mode

* refactor: remove comment that might confuse

* refactor: move --include-dev-deps to the scanner flag group

* refactor: not return apps

* docs: update

---------

Co-authored-by: knqyf263 <knqyf263@gmail.com>
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.

Include DevDependencies scans for npm packages
2 participants