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(flag): early fail when the format is invalid #3370

Merged
merged 3 commits into from
Jan 4, 2023
Merged

feat(flag): early fail when the format is invalid #3370

merged 3 commits into from
Jan 4, 2023

Conversation

blueskyson
Copy link
Contributor

Description

This PR will make trivy fail earlier when the format is wrong.

Related issues

Before:

$ ./trivy image alpine:latest --format foo
2023-01-02T19:00:26.902+0800    INFO    Vulnerability scanning is enabled
2023-01-02T19:00:26.902+0800    INFO    Secret scanning is enabled
2023-01-02T19:00:26.902+0800    INFO    If your scanning is slow, please try '--security-checks vuln' to disable secret scanning
2023-01-02T19:00:26.902+0800    INFO    Please see also https://aquasecurity.github.io/trivy/v0.35/docs/secret/scanning/#recommendation for faster secret detection
2023-01-02T19:00:29.062+0800    INFO    Detected OS: alpine
2023-01-02T19:00:29.062+0800    INFO    This OS version is not on the EOL list: alpine 3.17
2023-01-02T19:00:29.062+0800    INFO    Detecting Alpine vulnerabilities...
2023-01-02T19:00:29.065+0800    INFO    Number of language-specific files: 0
2023-01-02T19:00:29.065+0800    FATAL   report error: unable to write results: unknown format: foo

After:

$ ./trivy image alpine:latest --format foo
2023-01-02T19:00:20.617+0800    FATAL   flag error: report flag error: unknown format: foo

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 in the description (if the PR is a user interface change).

@blueskyson blueskyson requested a review from knqyf263 as a code owner January 2, 2023 11:05
@CLAassistant
Copy link

CLAassistant commented Jan 2, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! I left some comments.

Comment on lines 107 to 112
switch format {
case FormatTable, FormatJSON, FormatGitHub, FormatCycloneDX, FormatSPDX, FormatSPDXJSON, FormatTemplate, FormatSarif, FormatCosignVuln:
return nil
default:
return xerrors.Errorf("unknown format: %v", format)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding SupportedFormats may be better.

var (
SupportedSBOMFormats = []string{FormatCycloneDX, FormatSPDX, FormatSPDXJSON, FormatGitHub}
)

Suggested change
switch format {
case FormatTable, FormatJSON, FormatGitHub, FormatCycloneDX, FormatSPDX, FormatSPDXJSON, FormatTemplate, FormatSarif, FormatCosignVuln:
return nil
default:
return xerrors.Errorf("unknown format: %v", format)
}
return slices.Contains(SupportedFormats, format)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, it would be great to replace these values with SupportedFormats.

Usage: "format (table, json, sarif, template, cyclonedx, spdx, spdx-json, github, cosign-vuln)",

@@ -155,6 +155,10 @@ func (f *ReportFlagGroup) ToOptions(out io.Writer) (ReportOptions, error) {
listAllPkgs := getBool(f.ListAllPkgs)
output := getString(f.Output)

if err := report.ValidateFormat(format); err != nil {
return ReportOptions{}, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return ReportOptions{}, err
return ReportOptions{}, xerrors.Errorf("unknown format: %v", format)

Copy link
Contributor Author

@blueskyson blueskyson Jan 2, 2023

Choose a reason for hiding this comment

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

@knqyf263 Thanks for your suggestions. I also thought replacing independent strings with something like SupportedSBOMFormats is a better solution. Shall I replace the original strings with the slice or I just add the slice?

Copy link
Collaborator

Choose a reason for hiding this comment

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

replace the original strings with the slice

What does it look like? I thought we would add the slice, but I would like to hear another approach.

Copy link
Contributor Author

@blueskyson blueskyson Jan 3, 2023

Choose a reason for hiding this comment

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

Still learning Golang, I wrongly thought that I can treat Golang's slice as Cshap's enum which can be used in switch-case or test if a value belongs to one of the enums. Anyway, I finally implemented your suggestion.

Comment on lines +158 to +160
if format != "" && !slices.Contains(report.SupportedFormats, format) {
return ReportOptions{}, xerrors.Errorf("unknown format: %v", format)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the unit tests in /pkg/flag/report_flags_test.go use "" as their format, so I make a wild card for "".

@knqyf263 knqyf263 merged commit 60cf4fe into aquasecurity:main Jan 4, 2023
@knqyf263
Copy link
Collaborator

knqyf263 commented Jan 4, 2023

Thanks!

@blueskyson blueskyson deleted the patch-1 branch January 4, 2023 11:49
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.

Validate output format types earlier (fail fast when invalid format supplied)
3 participants