diff --git a/cli/cmd/command_builder.go b/cli/cmd/command_builder.go index 64bfcf4f0..47c97cd22 100644 --- a/cli/cmd/command_builder.go +++ b/cli/cmd/command_builder.go @@ -58,6 +58,7 @@ type AnalyzerBuilder struct { jarPath string maxMemory string ruleIDs []string + ruleIDExcludes []string passthroughApproximations []string dataflowApproximations []string trackExternalMethods bool @@ -146,6 +147,11 @@ func (a *AnalyzerBuilder) AddRuleID(ruleID string) *AnalyzerBuilder { return a } +func (a *AnalyzerBuilder) AddRuleIDExclude(ruleID string) *AnalyzerBuilder { + a.ruleIDExcludes = append(a.ruleIDExcludes, ruleID) + return a +} + func (a *AnalyzerBuilder) AddPassthroughApproximations(path string) *AnalyzerBuilder { a.passthroughApproximations = append(a.passthroughApproximations, path) return a @@ -249,6 +255,10 @@ func (a *AnalyzerBuilder) BuildNativeCommand() []string { flags = append(flags, "--semgrep-rule-id", ruleID) } + for _, ruleID := range a.ruleIDExcludes { + flags = append(flags, "--semgrep-rule-id-exclude", ruleID) + } + for _, passthrough := range a.passthroughApproximations { flags = append(flags, "--passthrough-approximations", passthrough) } diff --git a/cli/cmd/command_builder_test.go b/cli/cmd/command_builder_test.go index 903585a70..a4f0e70a2 100644 --- a/cli/cmd/command_builder_test.go +++ b/cli/cmd/command_builder_test.go @@ -2,6 +2,7 @@ package cmd import ( "reflect" + "strings" "testing" ) @@ -99,3 +100,32 @@ func TestAutobuilderBuildNativeCommandRoutesDependenciesToDependencyFlag(t *test t.Fatalf("package com.example not passed as --pkg; command was %v", cmd) } } + +func TestAnalyzerBuilderEmitsRuleIDIncludeAndExclude(t *testing.T) { + cmd := NewAnalyzerBuilder(). + SetProject("p.yaml"). + AddRuleID("a.yaml:keep"). + AddRuleIDExclude("a.yaml:drop"). + BuildNativeCommand() + + joined := strings.Join(cmd, " ") + if !strings.Contains(joined, "--semgrep-rule-id a.yaml:keep") { + t.Errorf("missing inclusion flag: %s", joined) + } + if !strings.Contains(joined, "--semgrep-rule-id-exclude a.yaml:drop") { + t.Errorf("missing exclusion flag: %s", joined) + } +} + +func TestAnalyzerBuilderExclusionOnlyEmitsNoInclusionFlags(t *testing.T) { + cmd := NewAnalyzerBuilder(). + SetProject("p.yaml"). + AddRuleIDExclude("a.yaml:drop"). + BuildNativeCommand() + + for i, arg := range cmd { + if arg == "--semgrep-rule-id" { + t.Errorf("unexpected inclusion flag at %d: %v", i, cmd) + } + } +} diff --git a/cli/cmd/scan.go b/cli/cmd/scan.go index 327d0e215..19397b3b5 100644 --- a/cli/cmd/scan.go +++ b/cli/cmd/scan.go @@ -10,6 +10,7 @@ import ( "github.com/seqra/opentaint/internal/load_trace" "github.com/seqra/opentaint/internal/rules" "github.com/seqra/opentaint/internal/sarif" + "github.com/seqra/opentaint/internal/triage" "github.com/seqra/opentaint/internal/validation" "github.com/seqra/opentaint/internal/version" @@ -34,10 +35,17 @@ type ScanConfig struct { Recompile bool LogFile string RuleID []string + ExcludeRuleID []string PassthroughApproximations []string DataflowApproximations []string TrackExternalMethods bool + Baseline string + WriteBaselineState bool + FingerprintKey string + ErrorOnFindings bool + ErrorOnSeverity []string + DebugFactReachabilitySarif bool DebugRunAnalysisOnSelectedEntryPoints string ExpandRuleRefs bool @@ -113,6 +121,7 @@ func init() { func addRuleIDFlag(cmd *cobra.Command) { cmd.Flags().StringArrayVar(&scanFlags.RuleID, "rule-id", nil, "Filter active rules by ID (repeatable)") + cmd.Flags().StringArrayVar(&scanFlags.ExcludeRuleID, "exclude-rule-id", nil, "Never run rules matching this ID: full id, bare name, or glob over the full id (repeatable; overrides rules.exclude from the config)") } func addScanFlags(cmd *cobra.Command) { @@ -136,6 +145,10 @@ func addScanFlags(cmd *cobra.Command) { cmd.Flags().StringArrayVar(&scanFlags.DataflowApproximations, "dataflow-approximations", nil, "Dataflow approximation class directory or Java source directory (repeatable)") cmd.Flags().BoolVar(&scanFlags.TrackExternalMethods, "track-external-methods", false, "Write external-method coverage files next to the SARIF report") + + addBaselineFlags(cmd, &scanFlags.Baseline, &scanFlags.FingerprintKey) + cmd.Flags().BoolVar(&scanFlags.WriteBaselineState, "write-baseline-state", false, "Persist result.baselineState and run.baselineGuid into the output report (needs --baseline)") + addGateFlags(cmd, &scanFlags.ErrorOnFindings, &scanFlags.ErrorOnSeverity) } // currentScanBuilder returns a builder pre-populated with the user's current scan flags. @@ -146,15 +159,84 @@ func currentScanBuilder(cfg ScanConfig, sourcePath string) *utils.OpentaintComma WithRuleset(cfg.Ruleset). WithSemgrepCompatibility(cfg.SemgrepCompatibilitySarif). WithRuleID(cfg.RuleID). + WithExcludeRuleID(cfg.ExcludeRuleID). WithPassthroughApproximations(cfg.PassthroughApproximations). WithDataflowApproximations(cfg.DataflowApproximations). - WithTrackExternalMethods(cfg.TrackExternalMethods) + WithTrackExternalMethods(cfg.TrackExternalMethods). + WithBaseline(cfg.Baseline). + WithWriteBaselineState(cfg.WriteBaselineState). + WithFingerprintKey(cfg.FingerprintKey). + WithErrorOnFindings(cfg.ErrorOnFindings). + WithErrorOnSeverity(cfg.ErrorOnSeverity) if !isDefaultSeverity(cfg.Severity) { b.WithSeverity(cfg.Severity) } return b } +// resolveRuleIDs determines which rules the analyzer should run, as exact +// inclusion and exclusion ids (patterns never reach the analyzer). +// +// --rule-id wins over the config lists, as flags do everywhere else; honoring +// a flag and rules.only together would silently intersect two selections the +// user never asked to combine. --exclude-rule-id overrides rules.exclude the +// same way, and composes with --rule-id since both were asked for explicitly. +// Returns the zero value when nothing restricts the rules, which runs the +// whole ruleset. +func resolveRuleIDs(cfg ScanConfig, absRuleSetPaths []RulesetType) rules.Resolved { + var rulesetRoots []string + for _, r := range absRuleSetPaths { + rulesetRoots = append(rulesetRoots, r.Path) + } + + if len(cfg.RuleID) > 0 { + // The explicit list is small, so exclusions are subtracted right here + // and the analyzer sees only the survivors. + ids, err := rules.ApplyExclusions(cfg.RuleID, cfg.ExcludeRuleID) + if err != nil { + out.Fatalf("%s", err) + } + warnUnmatchedRulePatterns(rules.Selection{Exclude: cfg.ExcludeRuleID}, cfg.RuleID) + if cfg.ExpandRuleRefs { + ids = rules.ExpandRuleIDs(ids, rulesetRoots) + } + return rules.Resolved{Include: ids} + } + + selection := configuredRuleSelection(cfg) + selected, err := rules.Select(selection, rulesetRoots) + if err != nil { + out.Fatalf("%s", err) + } + if selection.Active() { + warnUnmatchedRulePatterns(selection, rules.ListRuleIDs(rulesetRoots)) + } + return selected +} + +// warnUnmatchedRulePatterns surfaces selection patterns that matched no rule. +// A pattern matching nothing is usually a typo, and staying silent would make +// an exclusion look effective when it never was. +func warnUnmatchedRulePatterns(selection rules.Selection, all []string) { + for _, pattern := range selection.Unmatched(all) { + out.Warnf("Rule pattern %q matches no rule in the active ruleset", pattern) + } +} + +// configuredRuleSelection merges the rules.only / rules.exclude lists from the +// configuration file with the --exclude-rule-id flag, which overrides the +// configured exclude list when set. +func configuredRuleSelection(cfg ScanConfig) rules.Selection { + selection := rules.Selection{ + Only: globals.Config.Rules.Only, + Exclude: globals.Config.Rules.Exclude, + } + if len(cfg.ExcludeRuleID) > 0 { + selection.Exclude = cfg.ExcludeRuleID + } + return selection +} + func isDefaultSeverity(sev []string) bool { return len(sev) == 2 && sev[0] == "warning" && sev[1] == "error" } @@ -227,6 +309,21 @@ func runScan(cmd *cobra.Command, cfg ScanConfig) { absSarifReportPath = utils.DefaultSarifReportPath(absProjectModelPath) } + // Validate the triage flags before compiling: a typo in --baseline should + // not surface only after a fifteen-minute analysis. + gateSeverities, err := triage.ParseGateSeverities(cfg.ErrorOnSeverity) + if err != nil { + out.Fatalf("%s", err) + } + if cfg.WriteBaselineState && cfg.Baseline == "" { + out.Fatalf("--write-baseline-state needs a --baseline to compare against") + } + var baseline *sarif.Report + var absBaselinePath string + if cfg.Baseline != "" { + baseline, absBaselinePath = loadBaselineOrExit(cfg.Baseline, absSarifReportPath) + } + sarifReportName := filepath.Base(absSarifReportPath) localVersion := utils.ArtifactDisplayVersion(globals.ArtifactByKind("analyzer")) @@ -272,6 +369,11 @@ func runScan(cmd *cobra.Command, cfg ScanConfig) { out.Fatalf("Input validation failed: %s", err) } + // Resolve the active rules before the dry-run bail-out, so that a bad + // rules.only/rules.exclude list is reported by --dry-run and never after a + // full compile. + resolvedRules := resolveRuleIDs(cfg, absRuleSetPaths) + if cfg.DryRun { runDryRun("Compilation and analysis") return @@ -363,17 +465,12 @@ func runScan(cmd *cobra.Command, cfg ScanConfig) { if maxMemory != "" { nativeBuilder.SetMaxMemory(maxMemory) } - ruleIDs := cfg.RuleID - if cfg.ExpandRuleRefs && len(ruleIDs) > 0 { - var roots []string - for _, r := range absRuleSetPaths { - roots = append(roots, r.Path) - } - ruleIDs = rules.ExpandRuleIDs(ruleIDs, roots) - } - for _, ruleID := range ruleIDs { + for _, ruleID := range resolvedRules.Include { nativeBuilder.AddRuleID(ruleID) } + for _, ruleID := range resolvedRules.Exclude { + nativeBuilder.AddRuleIDExclude(ruleID) + } addPassthroughApproximations(nativeBuilder, cfg.PassthroughApproximations) if cfg.TrackExternalMethods { nativeBuilder.SetTrackExternalMethods(true) @@ -452,10 +549,12 @@ func runScan(cmd *cobra.Command, cfg ScanConfig) { if analyzerFail != nil { suggestions = appendLogSuggestion(suggestions) } + var view *sarif.TriageView if report != nil { + view = triageScanReport(cfg, report, absSarifReportPath, baseline, absBaselinePath) // Scan does not expose summary's filter/group flags, so pass zero values: // no filtering, default group dimension, first-flow code-flow selection. - printSarifSummary(report, absSarifReportPath, sarif.Filters{}, sarif.ListingOptions{MaxNestingLevel: -1}) + printSarifSummary(report, absSarifReportPath, sarif.Filters{}, sarif.ListingOptions{MaxNestingLevel: -1}, view, false) suggestions = append(suggestions, output.Suggestion{ Description: "To view findings run", Command: utils.NewSummaryCommand(absSarifReportPath).WithShowFindings().Build(), @@ -466,6 +565,32 @@ func runScan(cmd *cobra.Command, cfg ScanConfig) { if analyzerFail != nil { os.Exit(analyzerFail.ExitCode) } + if report != nil { + exitOnGate(triage.Gate{Enabled: cfg.ErrorOnFindings, Severities: gateSeverities}, report, view) + } +} + +// triageScanReport applies the baseline and any inherited suppressions to the +// report the analyzer just wrote, rewriting the file when that changed it. With +// no baseline and no annotation requested, the report is left exactly as the +// analyzer produced it. The baseline was loaded (and validated) before the +// compile step, so a bad path fails fast and the file is read only once. +func triageScanReport(cfg ScanConfig, report *sarif.Report, absSarifReportPath string, baseline *sarif.Report, absBaselinePath string) *sarif.TriageView { + outcome, err := triage.Apply(report, triage.Options{ + WriteBaselineState: cfg.WriteBaselineState, + FingerprintKey: cfg.FingerprintKey, + Baseline: baseline, + BaselinePath: absBaselinePath, + }) + if err != nil { + out.Fatalf("%s", err) + } + if outcome.Changed { + if err := sarif.SaveReport(report, absSarifReportPath); err != nil { + out.Fatalf("Failed to write report: %s", err) + } + } + return outcome.View } func resolveScanPlan(cfg ScanConfig, absUserProjectRoot string) scanPlan { diff --git a/cli/cmd/summary.go b/cli/cmd/summary.go index 0959b5257..2da93323c 100644 --- a/cli/cmd/summary.go +++ b/cli/cmd/summary.go @@ -2,6 +2,7 @@ package cmd import ( "github.com/seqra/opentaint/internal/sarif" + "github.com/seqra/opentaint/internal/triage" "github.com/seqra/opentaint/internal/utils" "github.com/seqra/opentaint/internal/utils/log" "github.com/spf13/cobra" @@ -33,15 +34,47 @@ Arguments: out.Fatalf("%s", err) } + states, err := sarif.ParseBaselineStates(summaryBaselineStates) + if err != nil { + out.Fatalf("%s", err) + } + absSarifPath := log.AbsPathOrExit(args[0], "sarif path") report, err := sarif.LoadReport(absSarifPath) if err != nil { out.Fatalf("Failed to load SARIF report: %s", err) } - printSarifSummary(report, absSarifPath, summaryFilters(), summaryListingOptions(dim, codeFlowSel)) + + // summary never writes: the baseline comparison and any inherited + // suppressions are applied to the in-memory copy for display only. + view := applyTriageForDisplay(report, absSarifPath) + + filters := summaryFilters() + filters.BaselineStates = states + printSarifSummary(report, absSarifPath, filters, summaryListingOptions(dim, codeFlowSel), view, showFindings) }, } +// applyTriageForDisplay runs a read-only triage pass so that summary can show +// baseline states and inherited suppressions without touching the file. +func applyTriageForDisplay(report *sarif.Report, absSarifPath string) *sarif.TriageView { + if summaryBaseline == "" { + return &sarif.TriageView{Suppressions: sarif.CollectSuppressionStats(report)} + } + + baseline, absBaselinePath := loadBaselineOrExit(summaryBaseline, absSarifPath) + outcome, err := triage.Apply(report, triage.Options{ + Baseline: baseline, + BaselinePath: absBaselinePath, + FingerprintKey: summaryFingerprintKey, + ReadOnly: true, + }) + if err != nil { + out.Fatalf("%s", err) + } + return outcome.View +} + var showFindings bool var showCodeSnippets bool var verboseFlow bool @@ -50,10 +83,14 @@ var summaryPaths []string var summarySeverities []string var summaryRuleIDs []string var summaryFingerprints []string -var summaryFingerprintKey string +var summaryPartialFingerprintKey string var summaryGroupBy string var summaryMaxNestingLevel = -1 // -1 = no cap; >= 0 collapses deeper flow steps var summaryCodeFlow string +var summaryBaseline string +var summaryBaselineStates []string +var summaryFingerprintKey string +var summaryShowSuppressed bool func init() { rootCmd.AddCommand(summaryCmd) @@ -65,10 +102,34 @@ func init() { summaryCmd.Flags().StringArrayVar(&summarySeverities, "severity", nil, "Show only findings of this SARIF level: error, warning, note, none (repeatable)") summaryCmd.Flags().StringArrayVar(&summaryRuleIDs, "rule-id", nil, "Show only findings for this rule: full id, leaf name, or glob (repeatable)") summaryCmd.Flags().StringArrayVar(&summaryFingerprints, "partial-fingerprint", nil, "Show only findings whose partial fingerprint starts with this value (git-hash style, repeatable)") - summaryCmd.Flags().StringVar(&summaryFingerprintKey, "partial-fingerprint-key", "", "partialFingerprints key matched by --partial-fingerprint (default vulnerabilityWithTraceHash/v1)") + summaryCmd.Flags().StringVar(&summaryPartialFingerprintKey, "partial-fingerprint-key", "", "partialFingerprints key matched by --partial-fingerprint (default vulnerabilityWithTraceHash/v1)") summaryCmd.Flags().IntVar(&summaryMaxNestingLevel, "max-nesting-level", -1, "Collapse code-flow steps deeper than this call-nesting level (-1 = no cap)") summaryCmd.Flags().StringVar(&summaryGroupBy, "group-by", "", "Group the --show-findings listing by: severity, rule-id, file-path (default file-path)") summaryCmd.Flags().StringVar(&summaryCodeFlow, "code-flow", "", "Render code flows: \"all\", a 1-based index, or unset (first only)") + addBaselineFlags(summaryCmd, &summaryBaseline, &summaryFingerprintKey) + summaryCmd.Flags().StringArrayVar(&summaryBaselineStates, "baseline-state", nil, "Show only findings whose baseline state is one of: new | unchanged | updated | absent (repeatable, needs --baseline)") + summaryCmd.Flags().BoolVar(&summaryShowSuppressed, "suppressed", false, "Include suppressed findings in the listing") +} + +// addBaselineFlags registers the flags shared by every command that can compare +// a report against a baseline. +func addBaselineFlags(cmd *cobra.Command, baseline *string, fingerprintKey *string) { + cmd.Flags().StringVar(baseline, "baseline", "", "Previous SARIF report to compare against and inherit suppressions from") + cmd.Flags().StringVar(fingerprintKey, "fingerprint-key", "", "partialFingerprints key identifying a finding across reports (default "+sarif.DefaultIdentityKey+")") +} + +// loadBaselineOrExit resolves and loads a baseline report, refusing to use the +// report under inspection as its own baseline. +func loadBaselineOrExit(baselinePath, absReportPath string) (*sarif.Report, string) { + absBaselinePath := log.AbsPathOrExit(baselinePath, "baseline") + if absBaselinePath == absReportPath { + out.Fatalf("The baseline and the report are the same file: %s", absBaselinePath) + } + baseline, err := sarif.LoadReport(absBaselinePath) + if err != nil { + out.Fatalf("Failed to load baseline report: %s", err) + } + return baseline, absBaselinePath } // currentSummaryBuilder returns a builder pre-populated with the user's current summary flags. @@ -89,10 +150,14 @@ func currentSummaryBuilder(sarifPath string) *utils.OpentaintCommandBuilder { builder.WithSeverity(summarySeverities) builder.WithRuleID(summaryRuleIDs) builder.WithPartialFingerprint(summaryFingerprints) - builder.WithPartialFingerprintKey(summaryFingerprintKey) + builder.WithPartialFingerprintKey(summaryPartialFingerprintKey) builder.WithMaxNestingLevel(summaryMaxNestingLevel) builder.WithGroupBy(summaryGroupBy) builder.WithCodeFlow(summaryCodeFlow) + builder.WithBaseline(summaryBaseline) + builder.WithFingerprintKey(summaryFingerprintKey) + builder.WithBaselineStateFilter(summaryBaselineStates) + builder.WithSuppressed(summaryShowSuppressed) return builder } @@ -105,7 +170,7 @@ func summaryFilters() sarif.Filters { Severities: summarySeverities, RuleIDs: summaryRuleIDs, Fingerprints: summaryFingerprints, - FingerprintKey: summaryFingerprintKey, + FingerprintKey: summaryPartialFingerprintKey, } } @@ -119,23 +184,27 @@ func summaryListingOptions(dim sarif.GroupDimension, codeFlowSel sarif.CodeFlowS VerboseFlow: verboseFlow, MaxNestingLevel: summaryMaxNestingLevel, GroupBy: dim, - FingerprintKey: summaryFingerprintKey, + FingerprintKey: summaryPartialFingerprintKey, CodeFlows: codeFlowSel, + ShowSuppressed: summaryShowSuppressed, } } -func printSarifSummary(report *sarif.Report, absSarifPath string, filters sarif.Filters, opts sarif.ListingOptions) { +// printSarifSummary renders the optional finding listing followed by the scan +// summary. list controls whether the listing is printed; each command owns its +// own --show-findings flag. +func printSarifSummary(report *sarif.Report, absSarifPath string, filters sarif.Filters, opts sarif.ListingOptions, view *sarif.TriageView, list bool) { filtered := report.Filter(filters) hasOmittedFlow := false - if showFindings { + if list { hasOmittedFlow = filtered.PrintAll(out, opts) out.Blank() } - filtered.PrintSummary(out, absSarifPath) + filtered.PrintSummary(out, absSarifPath, view) - if showFindings && hasOmittedFlow && !verboseFlow { + if list && hasOmittedFlow && !verboseFlow { out.Suggest( "To see full code flow and code snippets, use:", currentSummaryBuilder(absSarifPath).WithVerboseFlow().WithShowCodeSnippets().Build(), diff --git a/cli/cmd/triage.go b/cli/cmd/triage.go new file mode 100644 index 000000000..5176ab2ab --- /dev/null +++ b/cli/cmd/triage.go @@ -0,0 +1,165 @@ +package cmd + +import ( + "fmt" + "os" + + "github.com/seqra/opentaint/internal/sarif" + "github.com/seqra/opentaint/internal/triage" + "github.com/seqra/opentaint/internal/utils/log" + "github.com/spf13/cobra" +) + +// ExitFindings is returned when --error-on-findings is set and findings remain. +// It matches the "results failed the check" code used by `opentaint test`, and +// stays clear of 1 (general failure) and 252-255 (analyzer failures). +const ExitFindings = 2 + +type TriageConfig struct { + Baseline string + WriteBaselineState bool + FingerprintKey string + Accept []string + Defer []string + Unsuppress []string + Justification string + Output string + ErrorOnFindings bool + ErrorOnSeverity []string + ShowSuppressed bool + ShowFindings bool +} + +var triageFlags TriageConfig + +var triageCmd = &cobra.Command{ + Use: "triage sarif", + Short: "Compare a SARIF report against a baseline and record suppressions", + Args: cobra.ExactArgs(1), + Long: `Compare a SARIF report against a baseline and record accept/defer decisions + +Findings are identified by fingerprint, so a decision survives edits elsewhere +in the code. Nothing is ever deleted from the report: an accepted or deferred +finding stays in the file, marked with a SARIF suppression that records who +decided what and why. + +Arguments: + sarif - Path to the SARIF report to triage + +A finding is named by a fingerprint prefix, git-style — the value shown as +"Fingerprint:" by 'opentaint summary --show-findings'. + +Examples: + # See what changed since the last release, without modifying anything + opentaint triage scan.sarif --baseline release.sarif + + # We will not fix this one + opentaint triage scan.sarif --accept q3Vf9k --justification "sink is a constant" + + # We are not fixing this one for now + opentaint triage scan.sarif --defer 8bc1d2 --justification "waiting on OT-412" + + # Carry earlier decisions forward and fail if anything new turned up + opentaint triage scan.sarif --baseline release.sarif -o triaged.sarif \ + --error-on-findings + +Exit codes: + 0 Triage completed + 1 General failure (bad input, unreadable report) + 2 Findings remain and --error-on-findings was set`, + + Run: func(cmd *cobra.Command, args []string) { + runTriage(triageFlags, args[0]) + }, +} + +func init() { + rootCmd.AddCommand(triageCmd) + + addBaselineFlags(triageCmd, &triageFlags.Baseline, &triageFlags.FingerprintKey) + triageCmd.Flags().BoolVar(&triageFlags.WriteBaselineState, "write-baseline-state", false, "Persist result.baselineState and run.baselineGuid into the output report (needs --baseline)") + triageCmd.Flags().StringArrayVar(&triageFlags.Accept, "accept", nil, "Accept the finding with this fingerprint prefix: won't fix (repeatable)") + triageCmd.Flags().StringArrayVar(&triageFlags.Defer, "defer", nil, "Defer the finding with this fingerprint prefix: not fixing for now (repeatable)") + triageCmd.Flags().StringArrayVar(&triageFlags.Unsuppress, "unsuppress", nil, "Remove the suppression from the finding with this fingerprint prefix (repeatable)") + triageCmd.Flags().StringVar(&triageFlags.Justification, "justification", "", "Why the finding is accepted or deferred (required with --accept/--defer)") + triageCmd.Flags().StringVarP(&triageFlags.Output, "output", "o", "", "Write the triaged report here (default: rewrite the input in place)") + addGateFlags(triageCmd, &triageFlags.ErrorOnFindings, &triageFlags.ErrorOnSeverity) + triageCmd.Flags().BoolVar(&triageFlags.ShowSuppressed, "suppressed", false, "Include suppressed findings in the listing") + triageCmd.Flags().BoolVar(&triageFlags.ShowFindings, "show-findings", false, "List the findings, not just the summary") +} + +// addGateFlags registers the failure-gate flags shared by scan and triage. +func addGateFlags(cmd *cobra.Command, errorOnFindings *bool, severities *[]string) { + cmd.Flags().BoolVar(errorOnFindings, "error-on-findings", false, "Exit with code 2 when findings remain (new ones only, with --baseline)") + cmd.Flags().StringArrayVar(severities, "error-on-severity", nil, "Restrict --error-on-findings to these levels: error, warning, note, none (comma-separated or repeated; default all)") +} + +func runTriage(cfg TriageConfig, reportPath string) { + gateSeverities, err := triage.ParseGateSeverities(cfg.ErrorOnSeverity) + if err != nil { + out.Fatalf("%s", err) + } + + absReportPath := log.AbsPathOrExit(reportPath, "sarif path") + report, err := sarif.LoadReport(absReportPath) + if err != nil { + out.Fatalf("Failed to load SARIF report: %s", err) + } + + opts := triage.Options{ + WriteBaselineState: cfg.WriteBaselineState, + FingerprintKey: cfg.FingerprintKey, + Accept: cfg.Accept, + Defer: cfg.Defer, + Unsuppress: cfg.Unsuppress, + Justification: cfg.Justification, + } + if cfg.Baseline != "" { + opts.Baseline, opts.BaselinePath = loadBaselineOrExit(cfg.Baseline, absReportPath) + } else if cfg.WriteBaselineState { + out.Fatalf("--write-baseline-state needs a --baseline to compare against") + } + + outcome, err := triage.Apply(report, opts) + if err != nil { + out.Fatalf("%s", err) + } + + outputPath := absReportPath + if cfg.Output != "" { + outputPath = log.AbsPathOrExit(cfg.Output, "output") + } + // Writing an unchanged report to its own path would be pure churn, but an + // explicit -o means "put a copy here" and is always honored. + if outcome.Changed || outputPath != absReportPath { + if err := sarif.SaveReport(report, outputPath); err != nil { + out.Fatalf("Failed to write report: %s", err) + } + } + + printSarifSummary(report, outputPath, sarif.Filters{}, sarif.ListingOptions{ + MaxNestingLevel: -1, + ShowSuppressed: cfg.ShowSuppressed, + }, outcome.View, cfg.ShowFindings) + + exitOnGate(triage.Gate{Enabled: cfg.ErrorOnFindings, Severities: gateSeverities}, report, outcome.View) +} + +// exitOnGate reports the gate verdict and exits with ExitFindings when it trips. +func exitOnGate(gate triage.Gate, report *sarif.Report, view *sarif.TriageView) { + count, tripped := gate.Evaluate(report, view) + if !tripped { + return + } + out.Blank() + scope := "finding" + if count != 1 { + scope = "findings" + } + qualifier := "" + if view != nil && view.Comparison != nil { + qualifier = "new " + } + out.Error(fmt.Sprintf("%d %s%s reported (--error-on-findings)", count, qualifier, scope)) + os.Exit(ExitFindings) +} diff --git a/cli/internal/globals/global.go b/cli/internal/globals/global.go index 8bcba3f39..ef07d3bd3 100644 --- a/cli/internal/globals/global.go +++ b/cli/internal/globals/global.go @@ -68,6 +68,12 @@ type Autobuilder struct { type Rules struct { Version string `mapstructure:"version"` + // Only and Exclude control which rules the analyzer runs. They are rule + // selection, not suppression: an excluded rule never loads, so it produces + // nothing in the report. Entries match a full "path.yaml:id", a bare rule + // name, or a glob over either. + Only []string `mapstructure:"only"` + Exclude []string `mapstructure:"exclude"` } type Java struct { diff --git a/cli/internal/rules/select.go b/cli/internal/rules/select.go new file mode 100644 index 000000000..fb361adcf --- /dev/null +++ b/cli/internal/rules/select.go @@ -0,0 +1,173 @@ +package rules + +import ( + "fmt" + "io/fs" + "os" + "path/filepath" + "sort" + "strings" + + "github.com/seqra/opentaint/internal/sarif" + "gopkg.in/yaml.v2" +) + +// Selection is the allow/deny list of rule ids from the configuration file. +// These control which rules the analyzer runs at all — they are not +// suppressions, and an excluded rule produces nothing to suppress. +type Selection struct { + Only []string // if non-empty, only rules matching these run + Exclude []string // rules matching these never run +} + +// Active reports whether the selection restricts anything. +func (s Selection) Active() bool { + return len(s.Only) > 0 || len(s.Exclude) > 0 +} + +// ListRuleIDs returns every rule id defined under the given ruleset roots, in +// the ".yaml:" form the analyzer matches on. +// Files that cannot be read or parsed are skipped: a malformed rule file is the +// rule loader's problem to report, not a reason to fail rule selection. +func ListRuleIDs(roots []string) []string { + var ids []string + for _, root := range roots { + _ = filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() || !isRuleFile(path) { + return nil + } + relPath, relErr := filepath.Rel(root, path) + if relErr != nil { + return nil + } + data, readErr := os.ReadFile(path) + if readErr != nil { + return nil + } + var rf ruleFile + if yaml.Unmarshal(data, &rf) != nil { + return nil + } + for _, r := range rf.Rules { + if r.ID == "" { + continue + } + ids = append(ids, filepath.ToSlash(relPath)+":"+r.ID) + } + return nil + }) + } + return ids +} + +func isRuleFile(path string) bool { + ext := strings.ToLower(filepath.Ext(path)) + return ext == ".yaml" || ext == ".yml" +} + +// Resolved is a rule selection lowered to the exact ids the analyzer accepts: +// Include feeds --semgrep-rule-id (empty = run everything), Exclude feeds +// --semgrep-rule-id-exclude. Patterns never reach the analyzer. +type Resolved struct { + Include []string + Exclude []string +} + +// Select resolves a Selection against the ruleset roots. +// +// An exclusion-only selection resolves to just the excluded ids — excluding +// one rule passes one exclusion arg, not the 150-rule complement. When an +// allow-list is present the inclusion list is unavoidable (the analyzer +// matches exact ids), so exclusions are subtracted from it CLI-side and rules +// referenced by the survivors are pulled back in: a rule whose joined library +// rule was excluded could never match anything, which is a silently broken +// scan rather than a narrower one. On the exclusion side the analyzer itself +// resolves join refs past exclusions, so no such repair is needed. +func Select(selection Selection, roots []string) (Resolved, error) { + if !selection.Active() { + return Resolved{}, nil + } + + all := ListRuleIDs(roots) + if len(all) == 0 { + return Resolved{}, fmt.Errorf("rules.only/rules.exclude are configured but no rules were found in the ruleset") + } + + var kept, excluded []string + for _, id := range all { + if len(selection.Only) > 0 && !matchesAny(id, selection.Only) { + continue + } + if matchesAny(id, selection.Exclude) { + excluded = append(excluded, id) + continue + } + kept = append(kept, id) + } + if len(kept) == 0 { + return Resolved{}, fmt.Errorf("rules.only/rules.exclude select no rules at all; nothing would be scanned") + } + + if len(selection.Only) == 0 { + sort.Strings(excluded) + return Resolved{Exclude: excluded}, nil + } + + expanded := ExpandRuleIDs(kept, roots) + sort.Strings(expanded) + return Resolved{Include: expanded}, nil +} + +// matchesAny delegates to the one rule-id grammar (sarif.MatchesRuleID), so a +// pattern behaves identically in rules.only/rules.exclude, --exclude-rule-id, +// and summary's --rule-id filter: exact full "path.yaml:id", exact bare name, +// or a doublestar glob over the full id. +func matchesAny(id string, patterns []string) bool { + return sarif.MatchesRuleID(id, patterns) +} + +// ApplyExclusions filters an explicit rule-id list (--rule-id) by exclusion +// patterns (--exclude-rule-id), so the two flags compose instead of one +// silently winning. Emptying the list is an error: every id in it was asked +// for by name, so excluding them all leaves a scan that checks nothing. +func ApplyExclusions(ids, patterns []string) ([]string, error) { + if len(patterns) == 0 { + return ids, nil + } + var kept []string + for _, id := range ids { + if !matchesAny(id, patterns) { + kept = append(kept, id) + } + } + if len(ids) > 0 && len(kept) == 0 { + return nil, fmt.Errorf("--exclude-rule-id excludes every rule selected by --rule-id; nothing would be scanned") + } + return kept, nil +} + +// Unmatched returns the selection patterns that match none of the given rule +// ids, in Only-then-Exclude order. A pattern matching nothing is usually a +// typo'd rule name, and silently ignoring it would make an exclusion look +// effective when it never was — the caller should surface these. +func (s Selection) Unmatched(all []string) []string { + var unmatched []string + for _, pattern := range append(append([]string{}, s.Only...), s.Exclude...) { + if pattern == "" { + continue + } + if !anyIDMatches(all, pattern) { + unmatched = append(unmatched, pattern) + } + } + return unmatched +} + +func anyIDMatches(all []string, pattern string) bool { + for _, id := range all { + if matchesAny(id, []string{pattern}) { + return true + } + } + return false +} diff --git a/cli/internal/rules/select_test.go b/cli/internal/rules/select_test.go new file mode 100644 index 000000000..9b1e123de --- /dev/null +++ b/cli/internal/rules/select_test.go @@ -0,0 +1,241 @@ +package rules + +import ( + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +// ruleset writes a ruleset tree and returns its root. +func ruleset(t *testing.T, files map[string]string) string { + t.Helper() + root := t.TempDir() + for name, content := range files { + path := filepath.Join(root, filepath.FromSlash(name)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + } + return root +} + +func TestListRuleIDs(t *testing.T) { + root := ruleset(t, map[string]string{ + "java/security/sqli.yaml": "rules:\n - id: sql-injection\n - id: sql-injection-jdbc\n", + "java/security/xss.yml": "rules:\n - id: reflected-xss\n", + "java/lib/sources.yaml": "rules:\n - id: servlet-source\n", + "README.md": "not a ruleset file", + }) + + got := ListRuleIDs([]string{root}) + sort.Strings(got) + want := []string{ + "java/lib/sources.yaml:servlet-source", + "java/security/sqli.yaml:sql-injection", + "java/security/sqli.yaml:sql-injection-jdbc", + "java/security/xss.yml:reflected-xss", + } + if strings.Join(got, "\n") != strings.Join(want, "\n") { + t.Errorf("got:\n%s\nwant:\n%s", strings.Join(got, "\n"), strings.Join(want, "\n")) + } +} + +func TestListRuleIDsSkipsUnparseableFiles(t *testing.T) { + root := ruleset(t, map[string]string{ + "good.yaml": "rules:\n - id: good-rule\n", + "bad.yaml": "this: [is: not: valid: yaml", + }) + got := ListRuleIDs([]string{root}) + if len(got) != 1 || got[0] != "good.yaml:good-rule" { + t.Errorf("got %v, want just the parseable rule", got) + } +} + +func TestListRuleIDsMergesRoots(t *testing.T) { + a := ruleset(t, map[string]string{"a.yaml": "rules:\n - id: rule-a\n"}) + b := ruleset(t, map[string]string{"b.yaml": "rules:\n - id: rule-b\n"}) + got := ListRuleIDs([]string{a, b}) + sort.Strings(got) + if len(got) != 2 || got[0] != "a.yaml:rule-a" || got[1] != "b.yaml:rule-b" { + t.Errorf("got %v", got) + } +} + +func TestMatchesAnyUsesTheSummaryRuleIDGrammar(t *testing.T) { + const id = "java/security/sqli.yaml:sql-injection" + cases := []struct { + pattern string + want bool + }{ + {"java/security/sqli.yaml:sql-injection", true}, // full id + {"sql-injection", true}, // exact leaf + {"java/security/**", true}, // glob over the full id + {"java/**/sqli.yaml:*", true}, + {"sql-*", false}, // globs match the FULL id only, same as summary --rule-id + {"sql-injection-jdbc", false}, + {"go/**", false}, + {"", false}, + } + for _, tc := range cases { + if got := matchesAny(id, []string{tc.pattern}); got != tc.want { + t.Errorf("matchesAny(%q, [%q]) = %v, want %v", id, tc.pattern, got, tc.want) + } + } +} + +func TestSelectWithNeitherListReturnsNothing(t *testing.T) { + root := ruleset(t, map[string]string{"a.yaml": "rules:\n - id: rule-a\n"}) + got, err := Select(Selection{}, []string{root}) + if err != nil { + t.Fatalf("select: %v", err) + } + if got.Include != nil || got.Exclude != nil { + t.Errorf("got %+v, want zero: with no lists the analyzer runs every rule", got) + } +} + +func TestSelectOnly(t *testing.T) { + root := ruleset(t, map[string]string{ + "a.yaml": "rules:\n - id: keep-me\n - id: drop-me\n", + }) + got, err := Select(Selection{Only: []string{"keep-me"}}, []string{root}) + if err != nil { + t.Fatalf("select: %v", err) + } + if len(got.Include) != 1 || got.Include[0] != "a.yaml:keep-me" || len(got.Exclude) != 0 { + t.Errorf("got %+v", got) + } +} + +func TestSelectExcludeResolvesToConcreteExcludedIDs(t *testing.T) { + // Exclusion alone must NOT expand into a giant inclusion list: the analyzer + // has --semgrep-rule-id-exclude, so only the excluded ids are passed. + root := ruleset(t, map[string]string{ + "a.yaml": "rules:\n - id: keep-me\n - id: drop-me\n", + }) + got, err := Select(Selection{Exclude: []string{"drop-me"}}, []string{root}) + if err != nil { + t.Fatalf("select: %v", err) + } + if len(got.Include) != 0 { + t.Errorf("no inclusion list expected, got %v", got.Include) + } + if len(got.Exclude) != 1 || got.Exclude[0] != "a.yaml:drop-me" { + t.Errorf("got %v, want the one excluded id", got.Exclude) + } +} + +func TestSelectExcludeAppliesAfterOnly(t *testing.T) { + root := ruleset(t, map[string]string{ + "a.yaml": "rules:\n - id: sqli-one\n - id: sqli-two\n - id: xss\n", + }) + got, err := Select(Selection{Only: []string{"a.yaml:sqli-*"}, Exclude: []string{"sqli-two"}}, []string{root}) + if err != nil { + t.Fatalf("select: %v", err) + } + if len(got.Include) != 1 || got.Include[0] != "a.yaml:sqli-one" { + t.Errorf("got %+v", got) + } +} + +func TestSelectPullsInReferencedRules(t *testing.T) { + root := ruleset(t, map[string]string{ + "security/sqli.yaml": "rules:\n - id: sql-injection\n join:\n refs:\n - rule: lib/sources.yaml#servlet-source\n", + "lib/sources.yaml": "rules:\n - id: servlet-source\n", + }) + got, err := Select(Selection{Only: []string{"sql-injection"}}, []string{root}) + if err != nil { + t.Fatalf("select: %v", err) + } + sort.Strings(got.Include) + if len(got.Include) != 2 || got.Include[1] != "security/sqli.yaml:sql-injection" || got.Include[0] != "lib/sources.yaml:servlet-source" { + t.Errorf("got %+v, want the rule plus the library rule it joins", got) + } +} + +func TestSelectOnlyReAddsAnExcludedRuleThatSurvivorsNeed(t *testing.T) { + // On the inclusion path, excluding a library rule that a kept rule joins + // against would produce a rule that cannot match anything. Reference + // expansion brings it back. (On the exclusion-only path the analyzer + // resolves join refs past the exclusion itself, covered by the jar-side + // RuleIdExcludeTest.) + root := ruleset(t, map[string]string{ + "security/sqli.yaml": "rules:\n - id: sql-injection\n join:\n refs:\n - rule: lib/sources.yaml#servlet-source\n", + "lib/sources.yaml": "rules:\n - id: servlet-source\n", + }) + got, err := Select(Selection{Only: []string{"**"}, Exclude: []string{"lib/**"}}, []string{root}) + if err != nil { + t.Fatalf("select: %v", err) + } + if len(got.Include) != 2 { + t.Errorf("got %+v, want the excluded library rule restored", got) + } +} + +func TestSelectEmptyResultIsAnError(t *testing.T) { + root := ruleset(t, map[string]string{"a.yaml": "rules:\n - id: rule-a\n"}) + if _, err := Select(Selection{Only: []string{"nothing-matches-this"}}, []string{root}); err == nil { + t.Error("expected an error rather than a scan with zero rules") + } + if _, err := Select(Selection{Exclude: []string{"**"}}, []string{root}); err == nil { + t.Error("excluding everything should error rather than scan with zero rules") + } +} + +func TestSelectWithNoRulesFoundIsAnError(t *testing.T) { + if _, err := Select(Selection{Only: []string{"x"}}, []string{t.TempDir()}); err == nil { + t.Error("expected an error when the ruleset holds no rules at all") + } +} + +func TestApplyExclusionsFiltersAnExplicitList(t *testing.T) { + ids := []string{"a.yaml:keep-me", "a.yaml:drop-me", "b.yaml:drop-me-too"} + got, err := ApplyExclusions(ids, []string{"*:drop-*"}) + if err != nil { + t.Fatalf("apply: %v", err) + } + if len(got) != 1 || got[0] != "a.yaml:keep-me" { + t.Errorf("got %v", got) + } +} + +func TestApplyExclusionsWithNoPatternsIsIdentity(t *testing.T) { + ids := []string{"a.yaml:x"} + got, err := ApplyExclusions(ids, nil) + if err != nil { + t.Fatalf("apply: %v", err) + } + if len(got) != 1 || got[0] != "a.yaml:x" { + t.Errorf("got %v", got) + } +} + +func TestApplyExclusionsEmptyingTheListIsAnError(t *testing.T) { + if _, err := ApplyExclusions([]string{"a.yaml:x"}, []string{"**"}); err == nil { + t.Error("excluding every explicitly requested rule should error, not scan nothing") + } +} + +func TestUnmatchedReportsPatternsThatSelectNothing(t *testing.T) { + all := []string{"a.yaml:keep-me", "java/security/sqli.yaml:sql-injection"} + sel := Selection{ + Only: []string{"keep-me", "no-such-rule"}, + Exclude: []string{"java/**", "typo-*"}, + } + got := sel.Unmatched(all) + if len(got) != 2 || got[0] != "no-such-rule" || got[1] != "typo-*" { + t.Errorf("got %v, want [no-such-rule typo-*]", got) + } +} + +func TestUnmatchedIsEmptyWhenEverythingMatches(t *testing.T) { + all := []string{"a.yaml:x"} + if got := (Selection{Exclude: []string{"x"}}).Unmatched(all); got != nil { + t.Errorf("got %v, want nil", got) + } +} diff --git a/cli/internal/sarif/baseline.go b/cli/internal/sarif/baseline.go new file mode 100644 index 000000000..efd32d6ec --- /dev/null +++ b/cli/internal/sarif/baseline.go @@ -0,0 +1,178 @@ +package sarif + +import ( + "crypto/rand" + "fmt" +) + +// Comparison is the classification of a report's results against a baseline +// report. States are keyed by result pointer, so a Comparison is only valid for +// the exact *Report it was computed from. +type Comparison struct { + states map[*Result]BaselineState + + // Counts holds the number of current results in each state, plus the number + // of baseline results with no match in the current report under Absent. + Counts map[BaselineState]int + // Absent lists the baseline results that no longer appear — the fixed + // findings. They are reported, never written back into the current report. + Absent []*Result + // Unmatchable counts current results carrying no identity fingerprint, which + // therefore cannot be compared at all. + Unmatchable int + // BaselineGUID is the baseline run's automation guid, or "" if it has none. + BaselineGUID string +} + +// StateOf returns the state computed for a result, or "" when the result could +// not be matched (no identity fingerprint). +func (c *Comparison) StateOf(r *Result) BaselineState { + if c == nil { + return "" + } + return c.states[r] +} + +// CompareToBaseline classifies every result in current against baseline, using +// key as the identity fingerprint. Results that match are additionally compared +// on the full-trace fingerprint to tell "unchanged" from "updated". +// +// A baseline that holds results but none carrying key is rejected: silently +// classifying everything as new would hide exactly the findings a baseline +// exists to remember. +func CompareToBaseline(current, baseline *Report, key string) (*Comparison, error) { + baselineResults := baseline.Results() + + byIdentity := make(map[string][]*Result, len(baselineResults)) + for _, r := range baselineResults { + id, ok := Identity(r, key) + if !ok { + continue + } + byIdentity[id] = append(byIdentity[id], r) + } + if len(baselineResults) > 0 && len(byIdentity) == 0 { + return nil, fmt.Errorf( + "no result in the baseline carries the %q fingerprint; "+ + "it was produced with a different fingerprint key or without fingerprints", key) + } + + cmp := &Comparison{ + states: make(map[*Result]BaselineState), + Counts: make(map[BaselineState]int), + BaselineGUID: baseline.RunGUID(), + } + + matched := make(map[string]bool, len(byIdentity)) + for _, r := range current.Results() { + id, ok := Identity(r, key) + if !ok { + cmp.Unmatchable++ + continue + } + + previous, found := byIdentity[id] + if !found { + cmp.states[r] = New + cmp.Counts[New]++ + continue + } + + matched[id] = true + state := Updated + if sameTrace(r, previous) { + state = Unchanged + } + cmp.states[r] = state + cmp.Counts[state]++ + } + + for id, results := range byIdentity { + if matched[id] { + continue + } + cmp.Absent = append(cmp.Absent, results...) + } + cmp.Counts[Absent] = len(cmp.Absent) + + return cmp, nil +} + +// sameTrace reports whether the current result's full-trace fingerprint equals +// that of any baseline result sharing its identity. A missing trace fingerprint +// on either side counts as unchanged: the finer comparison is unavailable, and +// claiming "updated" on missing data would be noise. +func sameTrace(current *Result, previous []*Result) bool { + currentTrace, ok := Identity(current, TraceFingerprintKey) + if !ok { + return true + } + for _, p := range previous { + previousTrace, ok := Identity(p, TraceFingerprintKey) + if !ok || previousTrace == currentTrace { + return true + } + } + return false +} + +// Apply writes the comparison into the report: result.baselineState on every +// matched result, and run.baselineGuid on every run when the baseline had a +// guid to cite. Unmatchable results are left untouched. +func (c *Comparison) Apply(report *Report) { + for _, r := range report.Results() { + state, ok := c.states[r] + if !ok { + continue + } + value := state + r.BaselineState = &value + } + if c.BaselineGUID == "" { + return + } + for i := range report.Runs { + guid := c.BaselineGUID + report.Runs[i].BaselineGUID = &guid + } +} + +// RunGUID returns the first run's automation guid, or "" when absent. This is +// what a later run cites as its baselineGuid. +func (report *Report) RunGUID() string { + for i := range report.Runs { + if details := report.Runs[i].AutomationDetails; details != nil && details.GUID != nil { + return *details.GUID + } + } + return "" +} + +// EnsureRunGUIDs stamps a v4 GUID into run.automationDetails.guid for every run +// that lacks one. The analyzer emits no automation details, so without this no +// report could ever be cited as a baseline by guid. Existing guids are kept. +func EnsureRunGUIDs(report *Report) { + for i := range report.Runs { + run := &report.Runs[i] + if run.AutomationDetails == nil { + run.AutomationDetails = &RunAutomationDetails{} + } + if run.AutomationDetails.GUID != nil && *run.AutomationDetails.GUID != "" { + continue + } + guid := newUUIDv4() + run.AutomationDetails.GUID = &guid + } +} + +// newUUIDv4 returns a random RFC 4122 version 4 UUID. Hand-rolled to avoid a +// dependency for sixteen bytes; rand.Read is documented never to fail. +func newUUIDv4() string { + var b [16]byte + if _, err := rand.Read(b[:]); err != nil { + panic(fmt.Sprintf("crypto/rand failed: %v", err)) + } + b[6] = (b[6] & 0x0f) | 0x40 // version 4 + b[8] = (b[8] & 0x3f) | 0x80 // variant 10 + return fmt.Sprintf("%x-%x-%x-%x-%x", b[0:4], b[4:6], b[6:8], b[8:10], b[10:16]) +} diff --git a/cli/internal/sarif/baseline_test.go b/cli/internal/sarif/baseline_test.go new file mode 100644 index 000000000..61fa0150d --- /dev/null +++ b/cli/internal/sarif/baseline_test.go @@ -0,0 +1,253 @@ +package sarif + +import ( + "regexp" + "testing" +) + +// fp builds a partialFingerprints map from a source/sink hash and a trace hash. +func fp(sourceSink, trace string) map[string]string { + m := map[string]string{} + if sourceSink != "" { + m[SourceSinkFingerprintKey] = sourceSink + } + if trace != "" { + m[TraceFingerprintKey] = trace + } + return m +} + +func TestCompareClassifiesNewUnchangedUpdatedAbsent(t *testing.T) { + baseline := makeReport( + makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a")), + makeResult("b", Error, "b.java", 2, fp("id-b", "trace-b")), + makeResult("gone", Error, "c.java", 3, fp("id-gone", "trace-gone")), + ) + current := makeReport( + makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a")), // unchanged + makeResult("b", Error, "b.java", 9, fp("id-b", "trace-b-moved")), // updated + makeResult("fresh", Error, "d.java", 4, fp("id-fresh", "trace-fresh")), + ) + + cmp, err := CompareToBaseline(current, baseline, SourceSinkFingerprintKey) + if err != nil { + t.Fatalf("compare: %v", err) + } + + results := current.Results() + if got := cmp.StateOf(results[0]); got != Unchanged { + t.Errorf("first result: got %q, want unchanged", got) + } + if got := cmp.StateOf(results[1]); got != Updated { + t.Errorf("second result: got %q, want updated", got) + } + if got := cmp.StateOf(results[2]); got != New { + t.Errorf("third result: got %q, want new", got) + } + if cmp.Counts[Absent] != 1 { + t.Errorf("absent count: got %d, want 1", cmp.Counts[Absent]) + } + if len(cmp.Absent) != 1 || *cmp.Absent[0].RuleID != "gone" { + t.Errorf("absent results: got %v", cmp.Absent) + } + for state, want := range map[BaselineState]int{New: 1, Unchanged: 1, Updated: 1} { + if cmp.Counts[state] != want { + t.Errorf("%s count: got %d, want %d", state, cmp.Counts[state], want) + } + } +} + +func TestCompareWithTraceKeyNeverReportsUpdated(t *testing.T) { + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + current := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + + cmp, err := CompareToBaseline(current, baseline, TraceFingerprintKey) + if err != nil { + t.Fatalf("compare: %v", err) + } + if got := cmp.StateOf(current.Results()[0]); got != Unchanged { + t.Errorf("got %q, want unchanged", got) + } +} + +func TestCompareTreatsMissingTraceHashAsUnchanged(t *testing.T) { + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", ""))) + current := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", ""))) + + cmp, err := CompareToBaseline(current, baseline, SourceSinkFingerprintKey) + if err != nil { + t.Fatalf("compare: %v", err) + } + if got := cmp.StateOf(current.Results()[0]); got != Unchanged { + t.Errorf("got %q, want unchanged", got) + } +} + +func TestCompareCountsUnmatchableResultsSeparately(t *testing.T) { + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + current := makeReport( + makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a")), + makeResult("nofp", Error, "b.java", 2, nil), + ) + + cmp, err := CompareToBaseline(current, baseline, SourceSinkFingerprintKey) + if err != nil { + t.Fatalf("compare: %v", err) + } + if cmp.Unmatchable != 1 { + t.Errorf("unmatchable: got %d, want 1", cmp.Unmatchable) + } + if got := cmp.StateOf(current.Results()[1]); got != "" { + t.Errorf("unmatchable result should have no state, got %q", got) + } + if cmp.Counts[New] != 0 { + t.Errorf("unmatchable must not be counted as new, got %d", cmp.Counts[New]) + } +} + +func TestCompareDuplicateIdentitiesBothMatch(t *testing.T) { + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + current := makeReport( + makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a")), + makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a")), + ) + + cmp, err := CompareToBaseline(current, baseline, SourceSinkFingerprintKey) + if err != nil { + t.Fatalf("compare: %v", err) + } + if cmp.Counts[Unchanged] != 2 { + t.Errorf("both duplicates should match: got %d unchanged", cmp.Counts[Unchanged]) + } + if cmp.Counts[Absent] != 0 { + t.Errorf("baseline entry was matched, want 0 absent, got %d", cmp.Counts[Absent]) + } +} + +func TestCompareEmptyBaselineMakesEverythingNew(t *testing.T) { + cmp, err := CompareToBaseline( + makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))), + &Report{}, + SourceSinkFingerprintKey, + ) + if err != nil { + t.Fatalf("compare: %v", err) + } + if cmp.Counts[New] != 1 { + t.Errorf("got %d new, want 1", cmp.Counts[New]) + } +} + +func TestCompareRejectsBaselineWithoutTheIdentityKey(t *testing.T) { + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("", "trace-a"))) + current := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + + _, err := CompareToBaseline(current, baseline, SourceSinkFingerprintKey) + if err == nil { + t.Fatal("expected an error when no baseline result carries the identity key") + } +} + +func TestCompareEmptyBaselineIsNotAKeyMismatch(t *testing.T) { + // A baseline with zero results has no fingerprints either, but that is a + // legitimate "nothing was known before", not a key mismatch. + if _, err := CompareToBaseline( + makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))), + &Report{Runs: []Run{{}}}, + SourceSinkFingerprintKey, + ); err != nil { + t.Errorf("unexpected error: %v", err) + } +} + +func TestApplyWritesBaselineStateAndGUID(t *testing.T) { + guid := "11111111-2222-3333-4444-555555555555" + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + baseline.Runs[0].AutomationDetails = &RunAutomationDetails{GUID: &guid} + current := makeReport( + makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a")), + makeResult("fresh", Error, "b.java", 2, fp("id-fresh", "trace-fresh")), + ) + + cmp, err := CompareToBaseline(current, baseline, SourceSinkFingerprintKey) + if err != nil { + t.Fatalf("compare: %v", err) + } + cmp.Apply(current) + + results := current.Results() + if results[0].BaselineState == nil || *results[0].BaselineState != Unchanged { + t.Errorf("first result state not written: %v", results[0].BaselineState) + } + if results[1].BaselineState == nil || *results[1].BaselineState != New { + t.Errorf("second result state not written: %v", results[1].BaselineState) + } + if current.Runs[0].BaselineGUID == nil || *current.Runs[0].BaselineGUID != guid { + t.Errorf("baselineGuid not written: %v", current.Runs[0].BaselineGUID) + } +} + +func TestApplyOmitsBaselineGUIDWhenBaselineHasNone(t *testing.T) { + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + current := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + + cmp, err := CompareToBaseline(current, baseline, SourceSinkFingerprintKey) + if err != nil { + t.Fatalf("compare: %v", err) + } + cmp.Apply(current) + + if current.Runs[0].BaselineGUID != nil { + t.Errorf("expected no baselineGuid, got %q", *current.Runs[0].BaselineGUID) + } + if current.Results()[0].BaselineState == nil { + t.Error("states should still be written without a baseline guid") + } +} + +func TestApplyLeavesUnmatchableResultsUnannotated(t *testing.T) { + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + current := makeReport(makeResult("nofp", Error, "b.java", 2, nil)) + + cmp, err := CompareToBaseline(current, baseline, SourceSinkFingerprintKey) + if err != nil { + t.Fatalf("compare: %v", err) + } + cmp.Apply(current) + + if current.Results()[0].BaselineState != nil { + t.Errorf("unmatchable result was annotated: %v", *current.Results()[0].BaselineState) + } +} + +func TestEnsureRunGUIDsStampsMissingOnesOnly(t *testing.T) { + existing := "11111111-2222-3333-4444-555555555555" + report := &Report{Runs: []Run{ + {AutomationDetails: &RunAutomationDetails{GUID: &existing}}, + {}, + }} + + EnsureRunGUIDs(report) + + if report.Runs[0].AutomationDetails.GUID == nil || *report.Runs[0].AutomationDetails.GUID != existing { + t.Error("existing guid was overwritten") + } + if report.Runs[1].AutomationDetails == nil || report.Runs[1].AutomationDetails.GUID == nil { + t.Fatal("missing guid was not stamped") + } + uuidV4 := regexp.MustCompile(`^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$`) + if got := *report.Runs[1].AutomationDetails.GUID; !uuidV4.MatchString(got) { + t.Errorf("stamped guid %q is not a v4 uuid", got) + } +} + +func TestReportBaselineGUIDReadsFirstRun(t *testing.T) { + guid := "11111111-2222-3333-4444-555555555555" + report := &Report{Runs: []Run{{AutomationDetails: &RunAutomationDetails{GUID: &guid}}}} + if got := report.RunGUID(); got != guid { + t.Errorf("got %q, want %q", got, guid) + } + if got := (&Report{Runs: []Run{{}}}).RunGUID(); got != "" { + t.Errorf("got %q, want empty", got) + } +} diff --git a/cli/internal/sarif/filter.go b/cli/internal/sarif/filter.go index 20c649200..83197f7e2 100644 --- a/cli/internal/sarif/filter.go +++ b/cli/internal/sarif/filter.go @@ -19,13 +19,15 @@ type Filters struct { RuleIDs []string // full id, leaf, or doublestar glob over the full id Fingerprints []string // git-style prefixes of the chosen fingerprint key's value FingerprintKey string // partialFingerprints key to match ("" = DefaultFingerprintKey) + BaselineStates []string // SARIF baselineState values: new/unchanged/updated/absent } // active reports whether any filter dimension is set. FingerprintKey is // intentionally excluded: it only selects which key Fingerprints matches // against, so it has no effect without Fingerprints set. func (f Filters) active() bool { - return len(f.Paths) > 0 || len(f.Severities) > 0 || len(f.RuleIDs) > 0 || len(f.Fingerprints) > 0 + return len(f.Paths) > 0 || len(f.Severities) > 0 || len(f.RuleIDs) > 0 || + len(f.Fingerprints) > 0 || len(f.BaselineStates) > 0 } // Filter returns a shallow copy of the report whose Runs[].Results contain only @@ -59,7 +61,7 @@ func (f Filters) matches(r *Result) bool { if len(f.Paths) > 0 && !matchPath(r, f.Paths) { return false } - if len(f.Severities) > 0 && !matchSeverity(r, f.Severities) { + if len(f.Severities) > 0 && !MatchesSeverity(r, f.Severities) { return false } if len(f.RuleIDs) > 0 && !matchRuleID(r, f.RuleIDs) { @@ -68,9 +70,53 @@ func (f Filters) matches(r *Result) bool { if len(f.Fingerprints) > 0 && !matchFingerprint(r, f.FingerprintKey, f.Fingerprints) { return false } + if len(f.BaselineStates) > 0 && !matchBaselineState(r, f.BaselineStates) { + return false + } return true } +// matchBaselineState reports whether the result's baselineState equals any +// supplied value (case-insensitive). A result with no baselineState never +// matches: it was not compared against a baseline, so no state claim holds. +func matchBaselineState(r *Result, states []string) bool { + if r.BaselineState == nil { + return false + } + actual := strings.ToLower(string(*r.BaselineState)) + for _, s := range states { + if strings.ToLower(strings.TrimSpace(s)) == actual { + return true + } + } + return false +} + +// ParseBaselineStates validates --baseline-state values against the SARIF +// enumeration, returning them normalized. +func ParseBaselineStates(values []string) ([]string, error) { + valid := map[string]BaselineState{ + "new": New, + "unchanged": Unchanged, + "updated": Updated, + "absent": Absent, + } + var out []string + for _, v := range values { + normalized := strings.ToLower(strings.TrimSpace(v)) + if normalized == "" { + continue + } + state, ok := valid[normalized] + if !ok { + return nil, fmt.Errorf( + "invalid baseline state %q: valid values are new, unchanged, updated, absent", v) + } + out = append(out, string(state)) + } + return out, nil +} + // matchPath reports whether the result's primary location's relative file path // matches any of the doublestar glob patterns. func matchPath(r *Result, patterns []string) bool { @@ -90,9 +136,9 @@ func matchPath(r *Result, patterns []string) bool { return false } -// matchSeverity reports whether the result's level equals any supplied level +// MatchesSeverity reports whether the result's level equals any supplied level // (case-insensitive). A nil/empty level is treated as "note". -func matchSeverity(r *Result, levels []string) bool { +func MatchesSeverity(r *Result, levels []string) bool { actual := strings.ToLower(string(findingLevel(r))) for _, l := range levels { if strings.ToLower(strings.TrimSpace(l)) == actual { @@ -115,13 +161,17 @@ func ruleLeaf(id string) string { return id } -// matchRuleID reports whether the result's rule-id matches any supplied value as -// a full-id exact match, a leaf exact match, or a doublestar glob over the full id. +// matchRuleID reports whether the result's rule-id matches any supplied value. func matchRuleID(r *Result, values []string) bool { - if r.RuleID == nil { - return false - } - full := *r.RuleID + return r.RuleID != nil && MatchesRuleID(*r.RuleID, values) +} + +// MatchesRuleID reports whether a rule id matches any supplied value as a +// full-id exact match, a leaf exact match, or a doublestar glob over the full +// id — globs deliberately never match the bare leaf. This is the one rule-id +// grammar: summary's --rule-id filter and scan's rules.only/rules.exclude and +// --exclude-rule-id selection all use it. +func MatchesRuleID(full string, values []string) bool { leaf := ruleLeaf(full) for _, v := range values { // skip blank values (cobra StringArrayVar can yield them) so an empty @@ -146,7 +196,8 @@ func fingerprintValue(r *Result, key string) string { if key == "" { key = DefaultFingerprintKey } - return r.PartialFingerprints[key] + v, _ := Identity(r, key) + return v } // matchFingerprint reports whether the result's partialFingerprints value under diff --git a/cli/internal/sarif/filter_test.go b/cli/internal/sarif/filter_test.go index 2175de600..e4c7d7eb9 100644 --- a/cli/internal/sarif/filter_test.go +++ b/cli/internal/sarif/filter_test.go @@ -22,14 +22,14 @@ func TestMatchPath(t *testing.T) { func TestMatchSeverity(t *testing.T) { r := makeResult("r", Error, "a.java", 1, nil) - if !matchSeverity(&r, []string{"ERROR"}) { + if !MatchesSeverity(&r, []string{"ERROR"}) { t.Error("expected case-insensitive error match") } - if matchSeverity(&r, []string{"warning"}) { + if MatchesSeverity(&r, []string{"warning"}) { t.Error("expected warning not to match an error") } nilLevel := Result{Locations: r.Locations} - if !matchSeverity(&nilLevel, []string{"note"}) { + if !MatchesSeverity(&nilLevel, []string{"note"}) { t.Error("expected nil level to be treated as note") } } diff --git a/cli/internal/sarif/group.go b/cli/internal/sarif/group.go index 2c444477a..1c010bbbe 100644 --- a/cli/internal/sarif/group.go +++ b/cli/internal/sarif/group.go @@ -23,6 +23,11 @@ type ListingOptions struct { GroupBy GroupDimension // default groupByFilePath FingerprintKey string // "" = DefaultFingerprintKey CodeFlows CodeFlowSelection // zero value = render first flow only + // ShowSuppressed lists findings that carry an honored suppression. They are + // hidden by default: a suppressed finding is one somebody already decided + // about. Hiding happens here rather than in Filters so that the summary + // counts still see every result and can report how many were suppressed. + ShowSuppressed bool } // ParseGroupDimension converts a --group-by flag value into a GroupDimension. diff --git a/cli/internal/sarif/identity.go b/cli/internal/sarif/identity.go new file mode 100644 index 000000000..a65f93e4d --- /dev/null +++ b/cli/internal/sarif/identity.go @@ -0,0 +1,98 @@ +package sarif + +import ( + "fmt" + "sort" + "strings" +) + +// Fingerprint keys emitted by the analyzer under result.partialFingerprints. +// +// TraceFingerprintKey hashes the rule id, the sink, and every location on every +// trace: an exact identity that changes whenever anything on the flow path +// moves. SourceSinkFingerprintKey hashes the rule id, the sink, and the source +// (first) location of each trace, so it survives refactoring of the +// intermediate call path. +const ( + TraceFingerprintKey = "vulnerabilityWithTraceHash/v1" + SourceSinkFingerprintKey = "vulnerabilitySourceSinkHash/v1" +) + +// DefaultIdentityKey is the fingerprint key used to decide whether a finding in +// one report is "the same finding" as one in another report. The source/sink +// hash is the default because a suppression or baseline entry should survive +// edits to helper methods the flow happens to pass through. +const DefaultIdentityKey = SourceSinkFingerprintKey + +// ResolveIdentityKey normalizes a user-supplied identity key, falling back to +// DefaultIdentityKey when unset. Any key is accepted — a report may carry +// fingerprints this build does not know about — but a blank one is rejected +// rather than silently matching nothing. +func ResolveIdentityKey(key string) (string, error) { + if key == "" { + return DefaultIdentityKey, nil + } + trimmed := strings.TrimSpace(key) + if trimmed == "" { + return "", fmt.Errorf("fingerprint key must not be blank") + } + return trimmed, nil +} + +// Identity returns the result's value for the given fingerprint key. The second +// return is false when the result carries no such fingerprint, which means it +// cannot be matched against a baseline or named in a suppression. +func Identity(r *Result, key string) (string, bool) { + if r == nil || r.PartialFingerprints == nil { + return "", false + } + v, ok := r.PartialFingerprints[key] + if !ok || v == "" { + return "", false + } + return v, true +} + +// Results returns pointers to every result across every run, so callers can +// annotate results in place. +func (report *Report) Results() []*Result { + var out []*Result + for runIdx := range report.Runs { + run := &report.Runs[runIdx] + for resultIdx := range run.Results { + out = append(out, &run.Results[resultIdx]) + } + } + return out +} + +// ResolvePrefix finds the single result whose identity fingerprint starts with +// prefix, git-style. An empty, unmatched, or ambiguous prefix is an error: a +// suppression must name exactly one finding, never "whichever matched first". +func ResolvePrefix(report *Report, key, prefix string) (*Result, error) { + if prefix == "" { + return nil, fmt.Errorf("fingerprint prefix must not be empty") + } + + var matches []*Result + var values []string + for _, r := range report.Results() { + fp, ok := Identity(r, key) + if !ok || !strings.HasPrefix(fp, prefix) { + continue + } + matches = append(matches, r) + values = append(values, fp) + } + + switch len(matches) { + case 0: + return nil, fmt.Errorf("no finding matches fingerprint %q (key %s)", prefix, key) + case 1: + return matches[0], nil + default: + sort.Strings(values) + return nil, fmt.Errorf("fingerprint %q is ambiguous, it matches %d findings: %s", + prefix, len(matches), strings.Join(values, ", ")) + } +} diff --git a/cli/internal/sarif/identity_test.go b/cli/internal/sarif/identity_test.go new file mode 100644 index 000000000..930ee07fb --- /dev/null +++ b/cli/internal/sarif/identity_test.go @@ -0,0 +1,130 @@ +package sarif + +import ( + "strings" + "testing" +) + +func TestResolveIdentityKeyDefaultsToSourceSink(t *testing.T) { + key, err := ResolveIdentityKey("") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if key != SourceSinkFingerprintKey { + t.Errorf("got %q, want %q", key, SourceSinkFingerprintKey) + } +} + +func TestResolveIdentityKeyAcceptsExplicitKey(t *testing.T) { + key, err := ResolveIdentityKey(TraceFingerprintKey) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if key != TraceFingerprintKey { + t.Errorf("got %q, want %q", key, TraceFingerprintKey) + } +} + +func TestResolveIdentityKeyRejectsBlank(t *testing.T) { + if _, err := ResolveIdentityKey(" "); err == nil { + t.Error("expected error for whitespace-only key") + } +} + +func TestIdentityReadsChosenKey(t *testing.T) { + r := makeResult("rule", Error, "a.java", 1, map[string]string{ + SourceSinkFingerprintKey: "src-sink-hash", + TraceFingerprintKey: "trace-hash", + }) + got, ok := Identity(&r, SourceSinkFingerprintKey) + if !ok || got != "src-sink-hash" { + t.Errorf("got (%q, %v), want (src-sink-hash, true)", got, ok) + } + got, ok = Identity(&r, TraceFingerprintKey) + if !ok || got != "trace-hash" { + t.Errorf("got (%q, %v), want (trace-hash, true)", got, ok) + } +} + +func TestIdentityMissingKeyIsNotIdentifiable(t *testing.T) { + r := makeResult("rule", Error, "a.java", 1, map[string]string{TraceFingerprintKey: "trace"}) + if _, ok := Identity(&r, SourceSinkFingerprintKey); ok { + t.Error("expected missing key to report not-identifiable") + } + + noPrints := makeResult("rule", Error, "a.java", 1, nil) + if _, ok := Identity(&noPrints, SourceSinkFingerprintKey); ok { + t.Error("expected nil partialFingerprints to report not-identifiable") + } +} + +func TestResultsIteratesEveryRun(t *testing.T) { + report := &Report{Runs: []Run{ + {Results: []Result{makeResult("a", Error, "a.java", 1, nil)}}, + {Results: []Result{makeResult("b", Error, "b.java", 2, nil), makeResult("c", Error, "c.java", 3, nil)}}, + }} + got := report.Results() + if len(got) != 3 { + t.Fatalf("got %d results, want 3", len(got)) + } + // Results must be pointers into the report so mutations stick. + got[0].Level = lvlptr(Note) + if *report.Runs[0].Results[0].Level != Note { + t.Error("Results() did not return pointers into the report") + } +} + +func TestResolvePrefixFindsUniqueMatch(t *testing.T) { + report := makeReport( + makeResult("a", Error, "a.java", 1, map[string]string{SourceSinkFingerprintKey: "q3Vf9k2nAAA"}), + makeResult("b", Error, "b.java", 2, map[string]string{SourceSinkFingerprintKey: "8bc1d2xxBBB"}), + ) + r, err := ResolvePrefix(report, SourceSinkFingerprintKey, "q3Vf9k") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if *r.RuleID != "a" { + t.Errorf("resolved to rule %q, want a", *r.RuleID) + } +} + +func TestResolvePrefixExactValueMatches(t *testing.T) { + report := makeReport( + makeResult("a", Error, "a.java", 1, map[string]string{SourceSinkFingerprintKey: "q3Vf9k2nAAA"}), + ) + if _, err := ResolvePrefix(report, SourceSinkFingerprintKey, "q3Vf9k2nAAA"); err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestResolvePrefixAmbiguousIsAnError(t *testing.T) { + report := makeReport( + makeResult("a", Error, "a.java", 1, map[string]string{SourceSinkFingerprintKey: "q3Vf9kAAA"}), + makeResult("b", Error, "b.java", 2, map[string]string{SourceSinkFingerprintKey: "q3Vf9kBBB"}), + ) + _, err := ResolvePrefix(report, SourceSinkFingerprintKey, "q3Vf9k") + if err == nil { + t.Fatal("expected ambiguous prefix to error") + } + if !strings.Contains(err.Error(), "q3Vf9kAAA") || !strings.Contains(err.Error(), "q3Vf9kBBB") { + t.Errorf("error should list the candidates, got: %v", err) + } +} + +func TestResolvePrefixNoMatchIsAnError(t *testing.T) { + report := makeReport( + makeResult("a", Error, "a.java", 1, map[string]string{SourceSinkFingerprintKey: "q3Vf9kAAA"}), + ) + if _, err := ResolvePrefix(report, SourceSinkFingerprintKey, "zzzz"); err == nil { + t.Error("expected unmatched prefix to error") + } +} + +func TestResolvePrefixEmptyIsAnError(t *testing.T) { + report := makeReport( + makeResult("a", Error, "a.java", 1, map[string]string{SourceSinkFingerprintKey: "q3Vf9kAAA"}), + ) + if _, err := ResolvePrefix(report, SourceSinkFingerprintKey, ""); err == nil { + t.Error("expected empty prefix to error rather than match everything") + } +} diff --git a/cli/internal/sarif/listing.go b/cli/internal/sarif/listing.go index 0d1b47b74..3f8a75258 100644 --- a/cli/internal/sarif/listing.go +++ b/cli/internal/sarif/listing.go @@ -7,6 +7,12 @@ import ( "github.com/seqra/opentaint/internal/output" ) +// listable reports whether a result belongs in the detailed listing. Suppressed +// findings are omitted unless ShowSuppressed is set. +func (opts ListingOptions) listable(r *Result) bool { + return opts.ShowSuppressed || !IsSuppressed(r) +} + // PrintAll renders every finding in report as a grouped, sorted listing. It // returns true when at least one finding had its code flow truncated (so the // caller can offer a "--verbose-flow" hint). Groups are determined by @@ -15,7 +21,11 @@ import ( func (report *Report) PrintAll(out *output.Printer, opts ListingOptions) bool { totalFindings := 0 for _, run := range report.Runs { - totalFindings += len(run.Results) + for i := range run.Results { + if opts.listable(&run.Results[i]) { + totalFindings++ + } + } } if totalFindings == 0 { return false @@ -36,8 +46,11 @@ func (report *Report) PrintAll(out *output.Printer, opts ListingOptions) bool { for runIdx := range report.Runs { run := &report.Runs[runIdx] for resultIdx := range run.Results { - order++ result := &run.Results[resultIdx] + if !opts.listable(result) { + continue + } + order++ file := "" line := int64(-1) diff --git a/cli/internal/sarif/print_findings.go b/cli/internal/sarif/print_findings.go index d9e90fe62..f718a15ed 100644 --- a/cli/internal/sarif/print_findings.go +++ b/cli/internal/sarif/print_findings.go @@ -96,6 +96,17 @@ func (report *Report) buildFindingTree(out *output.Printer, result *Result, runI findingNode.Child(out.FieldItem("Severity", coloredSeverity)) findingNode.Child(out.FieldItem("Location", locStr)) + if result.BaselineState != nil { + findingNode.Child(out.FieldItem("Baseline", string(*result.BaselineState))) + } + if IsSuppressed(result) { + suppressedLine := StatusOf(result) + if justification := JustificationOf(result); justification != "" { + suppressedLine += ": " + justification + } + findingNode.Child(out.FieldItem("Suppressed", suppressedLine)) + } + total := len(result.CodeFlows) if total > 1 { findingNode.Child(out.FieldItem("Code flows", total)) diff --git a/cli/internal/sarif/property_bag.go b/cli/internal/sarif/property_bag.go new file mode 100644 index 000000000..072af5654 --- /dev/null +++ b/cli/internal/sarif/property_bag.go @@ -0,0 +1,77 @@ +package sarif + +import ( + "bytes" + "encoding/json" + "sort" +) + +// UnmarshalJSON decodes a property bag, lifting "tags" into the typed field and +// keeping every other key as raw JSON in Extra. Raw JSON rather than any: +// re-encoding through map[string]any would reformat numbers and can lose +// precision on integers beyond float64's exact range. +func (p *PropertyBag) UnmarshalJSON(data []byte) error { + var raw map[string]json.RawMessage + if err := json.Unmarshal(data, &raw); err != nil { + return err + } + + p.Tags = nil + p.Extra = nil + + for key, value := range raw { + if key == "tags" { + var tags []string + if err := json.Unmarshal(value, &tags); err == nil { + p.Tags = tags + continue + } + // Not a string array: keep it verbatim rather than dropping it. + } + if p.Extra == nil { + p.Extra = make(map[string]json.RawMessage, len(raw)) + } + p.Extra[key] = value + } + return nil +} + +// MarshalJSON re-emits the bag with its preserved keys. Keys are sorted so that +// rewriting an unchanged report produces byte-identical output. +func (p PropertyBag) MarshalJSON() ([]byte, error) { + keys := make([]string, 0, len(p.Extra)+1) + values := make(map[string]json.RawMessage, len(p.Extra)+1) + + for key, value := range p.Extra { + keys = append(keys, key) + values[key] = value + } + if len(p.Tags) > 0 { + encoded, err := json.Marshal(p.Tags) + if err != nil { + return nil, err + } + if _, clash := values["tags"]; !clash { + keys = append(keys, "tags") + } + values["tags"] = encoded + } + sort.Strings(keys) + + var buf bytes.Buffer + buf.WriteByte('{') + for i, key := range keys { + if i > 0 { + buf.WriteByte(',') + } + encodedKey, err := json.Marshal(key) + if err != nil { + return nil, err + } + buf.Write(encodedKey) + buf.WriteByte(':') + buf.Write(values[key]) + } + buf.WriteByte('}') + return buf.Bytes(), nil +} diff --git a/cli/internal/sarif/property_bag_test.go b/cli/internal/sarif/property_bag_test.go new file mode 100644 index 000000000..23020a645 --- /dev/null +++ b/cli/internal/sarif/property_bag_test.go @@ -0,0 +1,100 @@ +package sarif + +import ( + "encoding/json" + "testing" +) + +func TestPropertyBagPreservesUnknownKeys(t *testing.T) { + const in = `{"tags":["CWE-89"],"precision":"high","confidence":0.75,"nested":{"a":[1,2]}}` + var bag PropertyBag + if err := json.Unmarshal([]byte(in), &bag); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(bag.Tags) != 1 || bag.Tags[0] != "CWE-89" { + t.Errorf("tags not decoded: %v", bag.Tags) + } + + out, err := json.Marshal(bag) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var before, after map[string]any + if err := json.Unmarshal([]byte(in), &before); err != nil { + t.Fatal(err) + } + if err := json.Unmarshal(out, &after); err != nil { + t.Fatal(err) + } + for k, v := range before { + got, ok := after[k] + if !ok { + t.Errorf("key %q was dropped", k) + continue + } + if toJSON(t, got) != toJSON(t, v) { + t.Errorf("key %q changed: %s -> %s", k, toJSON(t, v), toJSON(t, got)) + } + } +} + +func TestPropertyBagPreservesLargeIntegersExactly(t *testing.T) { + const in = `{"id":9007199254740993}` + var bag PropertyBag + if err := json.Unmarshal([]byte(in), &bag); err != nil { + t.Fatalf("unmarshal: %v", err) + } + out, err := json.Marshal(bag) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(out) != in { + t.Errorf("got %s, want %s", out, in) + } +} + +func TestPropertyBagWithOnlyTags(t *testing.T) { + bag := PropertyBag{Tags: []string{"a", "b"}} + out, err := json.Marshal(bag) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(out) != `{"tags":["a","b"]}` { + t.Errorf("got %s", out) + } +} + +func TestPropertyBagEmptyMarshalsToEmptyObject(t *testing.T) { + out, err := json.Marshal(PropertyBag{}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(out) != `{}` { + t.Errorf("got %s, want {}", out) + } +} + +func TestPropertyBagNonStringTagsAreNotLost(t *testing.T) { + // A malformed bag must still round-trip rather than silently dropping tags. + const in = `{"tags":"not-an-array"}` + var bag PropertyBag + if err := json.Unmarshal([]byte(in), &bag); err != nil { + t.Fatalf("unmarshal: %v", err) + } + out, err := json.Marshal(bag) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(out) != in { + t.Errorf("got %s, want %s", out, in) + } +} + +func toJSON(t *testing.T, v any) string { + t.Helper() + b, err := json.Marshal(v) + if err != nil { + t.Fatal(err) + } + return string(b) +} diff --git a/cli/internal/sarif/sarif.go b/cli/internal/sarif/sarif.go index 7553c909a..e609be5cb 100644 --- a/cli/internal/sarif/sarif.go +++ b/cli/internal/sarif/sarif.go @@ -217,9 +217,15 @@ type Address struct { // Key/value pairs that provide additional information about the special locations. // // Key/value pairs that provide additional information about the version control details. +// Property bags are the one open-ended part of the SARIF schema: any key is +// legal. Extra holds every key other than "tags" verbatim so that reading a +// report, modifying it and writing it back never discards tool metadata. See +// property_bag.go for the marshalling. type PropertyBag struct { // A set of distinct strings that provide additional information. Tags []string `json:"tags,omitempty"` + // Every other key in the bag, preserved as raw JSON. + Extra map[string]json.RawMessage `json:"-"` } // A single artifact. In some cases, this artifact might be nested within another artifact. diff --git a/cli/internal/sarif/save.go b/cli/internal/sarif/save.go new file mode 100644 index 000000000..09494e523 --- /dev/null +++ b/cli/internal/sarif/save.go @@ -0,0 +1,47 @@ +package sarif + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" +) + +// SaveReport writes report to path as indented JSON. The write goes to a +// temporary file in the destination directory and is then renamed over path, so +// a crash mid-write can never leave a truncated report behind — which matters +// because triage rewrites reports in place. +func SaveReport(report *Report, path string) error { + data, err := json.MarshalIndent(report, "", " ") + if err != nil { + return fmt.Errorf("failed to encode sarif report: %w", err) + } + data = append(data, '\n') + + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("failed to create output directory: %w", err) + } + + tmp, err := os.CreateTemp(dir, ".sarif-*.tmp") + if err != nil { + return fmt.Errorf("failed to create temporary report file: %w", err) + } + tmpName := tmp.Name() + defer os.Remove(tmpName) // no-op once the rename below succeeds + + if _, err := tmp.Write(data); err != nil { + tmp.Close() + return fmt.Errorf("failed to write sarif report: %w", err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("failed to write sarif report: %w", err) + } + if err := os.Chmod(tmpName, 0o644); err != nil { + return fmt.Errorf("failed to set report permissions: %w", err) + } + if err := os.Rename(tmpName, path); err != nil { + return fmt.Errorf("failed to replace sarif report: %w", err) + } + return nil +} diff --git a/cli/internal/sarif/save_test.go b/cli/internal/sarif/save_test.go new file mode 100644 index 000000000..cc74d92ba --- /dev/null +++ b/cli/internal/sarif/save_test.go @@ -0,0 +1,154 @@ +package sarif + +import ( + "encoding/json" + "os" + "path/filepath" + "reflect" + "testing" +) + +// A report shaped like real analyzer output: schema/version envelope, tool +// driver with rules, uri bases, a result with fingerprints and a code flow. +const realisticSarif = `{ + "$schema": "https://json.schemastore.org/sarif-2.1.0.json", + "version": "2.1.0", + "runs": [ + { + "tool": { + "driver": { + "name": "OpenTaint", + "version": "1.2.3", + "semanticVersion": "1.2.3", + "rules": [ + { + "id": "java.sqli", + "name": "java.sqli", + "shortDescription": {"text": "SQL injection"}, + "properties": {"tags": ["CWE-89"], "precision": "high"} + } + ] + } + }, + "originalUriBaseIds": {"%SRCROOT%": {"uri": "/project"}}, + "results": [ + { + "ruleId": "java.sqli", + "level": "error", + "message": {"text": "Tainted value reaches a SQL sink"}, + "partialFingerprints": { + "vulnerabilityWithTraceHash/v1": "trace-hash-aaa", + "vulnerabilitySourceSinkHash/v1": "src-sink-aaa" + }, + "locations": [ + { + "physicalLocation": { + "artifactLocation": {"uri": "src/Dao.java", "uriBaseId": "%SRCROOT%"}, + "region": {"startLine": 42, "startColumn": 9} + } + } + ], + "codeFlows": [ + { + "threadFlows": [ + { + "locations": [ + { + "location": { + "physicalLocation": { + "artifactLocation": {"uri": "src/Controller.java", "uriBaseId": "%SRCROOT%"}, + "region": {"startLine": 10} + }, + "logicalLocations": [{"fullyQualifiedName": "com.example.Controller#handle"}] + }, + "kinds": ["taint", "source"], + "executionOrder": 1 + } + ] + } + ] + } + ] + } + ] + } + ] +}` + +func TestSaveReportRoundTripsRealisticReport(t *testing.T) { + report, err := UnmarshalReport([]byte(realisticSarif)) + if err != nil { + t.Fatalf("unmarshal: %v", err) + } + + path := filepath.Join(t.TempDir(), "out.sarif") + if err := SaveReport(&report, path); err != nil { + t.Fatalf("save: %v", err) + } + + written, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read back: %v", err) + } + + // Compare as generic JSON so key order and indentation are irrelevant: the + // question is whether any field was dropped or altered by the round trip. + var before, after any + if err := json.Unmarshal([]byte(realisticSarif), &before); err != nil { + t.Fatalf("unmarshal expected: %v", err) + } + if err := json.Unmarshal(written, &after); err != nil { + t.Fatalf("unmarshal written: %v", err) + } + if !reflect.DeepEqual(before, after) { + t.Errorf("round trip lost or changed data\nbefore: %s\nafter: %s", realisticSarif, written) + } +} + +func TestSaveReportCreatesParentDirectories(t *testing.T) { + report := makeReport(makeResult("a", Error, "a.java", 1, nil)) + path := filepath.Join(t.TempDir(), "nested", "dir", "out.sarif") + if err := SaveReport(report, path); err != nil { + t.Fatalf("save: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Errorf("expected file at %s: %v", path, err) + } +} + +func TestSaveReportLeavesNoTempFileBehind(t *testing.T) { + dir := t.TempDir() + report := makeReport(makeResult("a", Error, "a.java", 1, nil)) + if err := SaveReport(report, filepath.Join(dir, "out.sarif")); err != nil { + t.Fatalf("save: %v", err) + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("readdir: %v", err) + } + if len(entries) != 1 || entries[0].Name() != "out.sarif" { + var names []string + for _, e := range entries { + names = append(names, e.Name()) + } + t.Errorf("expected only out.sarif, got %v", names) + } +} + +func TestSaveReportOverwritesAtomically(t *testing.T) { + path := filepath.Join(t.TempDir(), "out.sarif") + if err := os.WriteFile(path, []byte("stale contents"), 0o644); err != nil { + t.Fatalf("seed: %v", err) + } + report := makeReport(makeResult("a", Error, "a.java", 1, nil)) + if err := SaveReport(report, path); err != nil { + t.Fatalf("save: %v", err) + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read back: %v", err) + } + if _, err := UnmarshalReport(data); err != nil { + t.Errorf("overwritten file is not valid SARIF: %v", err) + } +} diff --git a/cli/internal/sarif/suppress.go b/cli/internal/sarif/suppress.go new file mode 100644 index 000000000..f4cba5a04 --- /dev/null +++ b/cli/internal/sarif/suppress.go @@ -0,0 +1,216 @@ +package sarif + +import ( + "fmt" + "strings" +) + +// Suppression semantics, per SARIF §3.35 and the read rule in the design: +// +// - status absent or "accepted" — suppressed. "accepted" is what triage +// --accept writes: the team will not fix this. +// - "underReview" — suppressed, and reported separately as deferred. This is +// what triage --defer writes: the team is not fixing it for now. +// - "rejected" — not suppressed. The suppression was explicitly denied, so +// reporting the finding is the whole point. +// - anything else — not suppressed, and counted so the report says so. +// +// Nothing but SARIF's own fields is written: kind, status, justification, guid. + +// honors reports whether a single suppression entry hides its result. +func honors(s *Suppression) bool { + if s.Status == nil { + return true + } + switch *s.Status { + case Accepted, UnderReview: + return true + default: + return false + } +} + +// IsSuppressed reports whether any suppression on the result is honored. +func IsSuppressed(r *Result) bool { + return honoredSuppression(r) != nil +} + +// honoredSuppression returns the first suppression entry that hides the result, +// or nil when none does. +func honoredSuppression(r *Result) *Suppression { + if r == nil { + return nil + } + for i := range r.Suppressions { + if honors(&r.Suppressions[i]) { + return &r.Suppressions[i] + } + } + return nil +} + +// IsDeferred reports whether the honored suppression is a deferral +// ("not fixing for now") rather than an acceptance ("won't fix"). +func IsDeferred(r *Result) bool { + s := honoredSuppression(r) + return s != nil && s.Status != nil && *s.Status == UnderReview +} + +// JustificationOf returns the justification of the honored suppression, or "" +// when the result is not suppressed or the entry carries no justification. +func JustificationOf(r *Result) string { + s := honoredSuppression(r) + if s == nil || s.Justification == nil { + return "" + } + return *s.Justification +} + +// StatusOf returns the honored suppression's status as a string, defaulting to +// "accepted" when the entry omits it (which is how the read rule treats it). +func StatusOf(r *Result) string { + s := honoredSuppression(r) + if s == nil { + return "" + } + if s.Status == nil { + return string(Accepted) + } + return string(*s.Status) +} + +// Accept records that the team will not fix this finding, writing an external +// suppression with status "accepted". Any suppression already on the result is +// replaced: a result carries one decision, the most recent one. +func Accept(r *Result, justification string) error { + return suppress(r, Accepted, justification) +} + +// Defer records that the team is not fixing this finding for now, writing an +// external suppression with status "underReview". +func Defer(r *Result, justification string) error { + return suppress(r, UnderReview, justification) +} + +func suppress(r *Result, status Status, justification string) error { + justification = strings.TrimSpace(justification) + if justification == "" { + return fmt.Errorf("a justification is required to suppress a finding") + } + guid := newUUIDv4() + statusValue := status + r.Suppressions = []Suppression{{ + Kind: External, + Status: &statusValue, + Justification: &justification, + GUID: &guid, + }} + return nil +} + +// Unsuppress removes every suppression from the result, reporting whether +// anything was removed. It only affects the report being triaged: if a baseline +// still carries the decision, the next scan inherits it again. +func Unsuppress(r *Result) bool { + if len(r.Suppressions) == 0 { + return false + } + r.Suppressions = nil + return true +} + +// InheritSuppressions copies honored suppressions from baseline results onto +// matching current results, and returns how many were copied. The copy is +// verbatim — same status, justification and guid — so a decision authored once +// stays attached to the finding across every later scan. +// +// Presence in the baseline is not acceptance: a baseline result without a +// suppression transmits nothing. A result that already carries its own +// suppression is left alone; its own decision is the newer one. +func InheritSuppressions(current, baseline *Report, key string) int { + byIdentity := make(map[string]*Suppression) + for _, r := range baseline.Results() { + id, ok := Identity(r, key) + if !ok { + continue + } + if _, seen := byIdentity[id]; seen { + continue + } + if s := honoredSuppression(r); s != nil { + byIdentity[id] = s + } + } + + inherited := 0 + for _, r := range current.Results() { + if len(r.Suppressions) > 0 { + continue + } + id, ok := Identity(r, key) + if !ok { + continue + } + source, found := byIdentity[id] + if !found { + continue + } + r.Suppressions = []Suppression{copySuppression(source)} + inherited++ + } + return inherited +} + +// copySuppression deep-copies the parts of a suppression we carry forward. +// Pointers are cloned so the two reports never share mutable state. +func copySuppression(s *Suppression) Suppression { + out := Suppression{Kind: s.Kind, Location: s.Location, Properties: s.Properties} + if s.Status != nil { + status := *s.Status + out.Status = &status + } + if s.Justification != nil { + justification := *s.Justification + out.Justification = &justification + } + if s.GUID != nil { + guid := *s.GUID + out.GUID = &guid + } + return out +} + +// SuppressionStats summarizes the suppression state of a report. +type SuppressionStats struct { + Total int // all results + Suppressed int // results hidden by an honored suppression + WontFix int // honored, status accepted (or absent) + Deferred int // honored, status underReview + NotHonored int // results carrying only rejected or unrecognised suppressions +} + +// Any reports whether the report contains any suppression at all, honored or +// not — the signal for whether to render the Suppressions summary group. +func (s SuppressionStats) Any() bool { + return s.Suppressed > 0 || s.NotHonored > 0 +} + +// CollectSuppressionStats walks the report and counts suppression states. +func CollectSuppressionStats(report *Report) SuppressionStats { + var stats SuppressionStats + for _, r := range report.Results() { + stats.Total++ + switch { + case IsSuppressed(r): + stats.Suppressed++ + if IsDeferred(r) { + stats.Deferred++ + } else { + stats.WontFix++ + } + case len(r.Suppressions) > 0: + stats.NotHonored++ + } + } + return stats +} diff --git a/cli/internal/sarif/suppress_test.go b/cli/internal/sarif/suppress_test.go new file mode 100644 index 000000000..11bca25d5 --- /dev/null +++ b/cli/internal/sarif/suppress_test.go @@ -0,0 +1,252 @@ +package sarif + +import ( + "strings" + "testing" +) + +func statusPtr(s Status) *Status { return &s } + +// suppressed builds a result carrying one external suppression with the given +// status ("" means the status property is absent). +func suppressed(ruleID, sourceSink string, status Status, justification string) Result { + r := makeResult(ruleID, Error, "a.java", 1, fp(sourceSink, "trace-"+sourceSink)) + s := Suppression{Kind: External, Justification: strptr(justification)} + if status != "" { + s.Status = statusPtr(status) + } + r.Suppressions = []Suppression{s} + return r +} + +func TestIsSuppressedReadRule(t *testing.T) { + cases := []struct { + name string + result Result + want bool + }{ + {"no suppressions", makeResult("a", Error, "a.java", 1, nil), false}, + {"status absent", suppressed("a", "id", "", "why"), true}, + {"accepted", suppressed("a", "id", Accepted, "why"), true}, + {"under review", suppressed("a", "id", UnderReview, "why"), true}, + {"rejected", suppressed("a", "id", Rejected, "why"), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := IsSuppressed(&tc.result); got != tc.want { + t.Errorf("got %v, want %v", got, tc.want) + } + }) + } +} + +func TestIsSuppressedUnknownStatusDoesNotHide(t *testing.T) { + r := suppressed("a", "id", Status("somethingElse"), "why") + if IsSuppressed(&r) { + t.Error("an unrecognised status must not hide a finding") + } +} + +func TestIsSuppressedAnyAcceptingEntryWins(t *testing.T) { + r := suppressed("a", "id", Rejected, "denied") + r.Suppressions = append(r.Suppressions, Suppression{ + Kind: External, + Status: statusPtr(Accepted), + }) + if !IsSuppressed(&r) { + t.Error("a result with one accepted suppression is suppressed") + } +} + +func TestAcceptWritesAcceptedStatus(t *testing.T) { + r := makeResult("a", Error, "a.java", 1, fp("id", "trace")) + if err := Accept(&r, "sink is a constant"); err != nil { + t.Fatalf("accept: %v", err) + } + if len(r.Suppressions) != 1 { + t.Fatalf("got %d suppressions, want 1", len(r.Suppressions)) + } + s := r.Suppressions[0] + if s.Kind != External { + t.Errorf("kind: got %q, want external", s.Kind) + } + if s.Status == nil || *s.Status != Accepted { + t.Errorf("status: got %v, want accepted", s.Status) + } + if s.Justification == nil || *s.Justification != "sink is a constant" { + t.Errorf("justification: got %v", s.Justification) + } + if s.GUID == nil || *s.GUID == "" { + t.Error("a guid must be generated") + } + if s.Properties != nil { + t.Error("no property bag should be written") + } + if s.Location != nil { + t.Error("an external suppression has no location") + } +} + +func TestDeferWritesUnderReviewStatus(t *testing.T) { + r := makeResult("a", Error, "a.java", 1, fp("id", "trace")) + if err := Defer(&r, "waiting on OT-412"); err != nil { + t.Fatalf("defer: %v", err) + } + s := r.Suppressions[0] + if s.Status == nil || *s.Status != UnderReview { + t.Errorf("status: got %v, want underReview", s.Status) + } + if !IsSuppressed(&r) { + t.Error("a deferred finding is suppressed") + } +} + +func TestAcceptRequiresJustification(t *testing.T) { + r := makeResult("a", Error, "a.java", 1, nil) + if err := Accept(&r, " "); err == nil { + t.Error("expected an error for a blank justification") + } + if len(r.Suppressions) != 0 { + t.Error("nothing should be written when validation fails") + } +} + +func TestAcceptReplacesAnExistingSuppression(t *testing.T) { + r := suppressed("a", "id", UnderReview, "deferred earlier") + if err := Accept(&r, "now decided: won't fix"); err != nil { + t.Fatalf("accept: %v", err) + } + if len(r.Suppressions) != 1 { + t.Fatalf("got %d suppressions, want 1", len(r.Suppressions)) + } + if *r.Suppressions[0].Status != Accepted { + t.Errorf("status not updated: %v", *r.Suppressions[0].Status) + } + if *r.Suppressions[0].Justification != "now decided: won't fix" { + t.Errorf("justification not updated: %v", *r.Suppressions[0].Justification) + } +} + +func TestUnsuppressRemovesTheEntry(t *testing.T) { + r := suppressed("a", "id", Accepted, "why") + if !Unsuppress(&r) { + t.Error("expected Unsuppress to report a change") + } + if len(r.Suppressions) != 0 { + t.Errorf("got %d suppressions, want 0", len(r.Suppressions)) + } + if Unsuppress(&r) { + t.Error("unsuppressing an unsuppressed result should report no change") + } +} + +func TestInheritCopiesSuppressionVerbatim(t *testing.T) { + guid := "11111111-2222-3333-4444-555555555555" + base := suppressed("a", "id-a", Accepted, "admin-only input") + base.Suppressions[0].GUID = &guid + baseline := makeReport(base) + current := makeReport( + makeResult("a", Error, "a.java", 1, fp("id-a", "trace-id-a")), + makeResult("b", Error, "b.java", 2, fp("id-b", "trace-id-b")), + ) + + n := InheritSuppressions(current, baseline, SourceSinkFingerprintKey) + if n != 1 { + t.Fatalf("inherited %d, want 1", n) + } + + got := current.Results()[0] + if len(got.Suppressions) != 1 { + t.Fatalf("got %d suppressions, want 1", len(got.Suppressions)) + } + s := got.Suppressions[0] + if s.GUID == nil || *s.GUID != guid { + t.Errorf("guid not inherited verbatim: %v", s.GUID) + } + if s.Justification == nil || *s.Justification != "admin-only input" { + t.Errorf("justification not inherited verbatim: %v", s.Justification) + } + if s.Status == nil || *s.Status != Accepted { + t.Errorf("status not inherited verbatim: %v", s.Status) + } + if len(current.Results()[1].Suppressions) != 0 { + t.Error("an unmatched result must not be suppressed") + } +} + +func TestInheritIgnoresBaselineEntriesWithoutSuppressions(t *testing.T) { + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + current := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + + if n := InheritSuppressions(current, baseline, SourceSinkFingerprintKey); n != 0 { + t.Errorf("inherited %d, want 0: presence in a baseline is not acceptance", n) + } + if IsSuppressed(current.Results()[0]) { + t.Error("a plain baseline entry must not suppress") + } +} + +func TestInheritDoesNotOverwriteAnExistingDecision(t *testing.T) { + baseline := makeReport(suppressed("a", "id-a", Accepted, "old decision")) + current := makeReport(suppressed("a", "id-a", UnderReview, "decided again just now")) + + if n := InheritSuppressions(current, baseline, SourceSinkFingerprintKey); n != 0 { + t.Errorf("inherited %d, want 0", n) + } + if *current.Results()[0].Suppressions[0].Justification != "decided again just now" { + t.Error("the result's own suppression was overwritten") + } +} + +func TestInheritSkipsRejectedBaselineEntries(t *testing.T) { + baseline := makeReport(suppressed("a", "id-a", Rejected, "denied")) + current := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + + if n := InheritSuppressions(current, baseline, SourceSinkFingerprintKey); n != 0 { + t.Errorf("inherited %d, want 0", n) + } + if IsSuppressed(current.Results()[0]) { + t.Error("a rejected suppression must not hide a finding") + } +} + +func TestSuppressionStatsBreakdown(t *testing.T) { + report := makeReport( + suppressed("a", "id-a", Accepted, "won't fix"), + suppressed("b", "id-b", Accepted, "won't fix either"), + suppressed("c", "id-c", UnderReview, "not now"), + suppressed("d", "id-d", Rejected, "denied"), + suppressed("e", "id-e", Status("weird"), "?"), + makeResult("f", Error, "f.java", 6, fp("id-f", "trace-f")), + ) + + stats := CollectSuppressionStats(report) + if stats.Total != 6 { + t.Errorf("total: got %d, want 6", stats.Total) + } + if stats.Suppressed != 3 { + t.Errorf("suppressed: got %d, want 3", stats.Suppressed) + } + if stats.WontFix != 2 { + t.Errorf("won't fix: got %d, want 2", stats.WontFix) + } + if stats.Deferred != 1 { + t.Errorf("deferred: got %d, want 1", stats.Deferred) + } + if stats.NotHonored != 2 { + t.Errorf("not honored: got %d, want 2 (rejected + unknown status)", stats.NotHonored) + } +} + +func TestJustificationOfReturnsTheHonoredEntry(t *testing.T) { + r := suppressed("a", "id", Rejected, "denied") + r.Suppressions = append(r.Suppressions, Suppression{ + Kind: External, + Status: statusPtr(Accepted), + Justification: strptr("the real reason"), + }) + got := JustificationOf(&r) + if !strings.Contains(got, "the real reason") { + t.Errorf("got %q, want the honored entry's justification", got) + } +} diff --git a/cli/internal/sarif/triage_summary_test.go b/cli/internal/sarif/triage_summary_test.go new file mode 100644 index 000000000..5792b7524 --- /dev/null +++ b/cli/internal/sarif/triage_summary_test.go @@ -0,0 +1,128 @@ +package sarif + +import ( + "bytes" + "strings" + "testing" + + "github.com/seqra/opentaint/internal/output" +) + +func renderSummary(t *testing.T, report *Report, view *TriageView) string { + t.Helper() + var buf bytes.Buffer + report.PrintSummary(output.NewWithWriter(&buf), "/tmp/report.sarif", view) + return buf.String() +} + +func TestSummaryWithoutTriageHasNoNewGroups(t *testing.T) { + out := renderSummary(t, makeReport(makeResult("a", Error, "a.java", 1, nil)), nil) + if strings.Contains(out, "Baseline") { + t.Errorf("unexpected Baseline group:\n%s", out) + } + if strings.Contains(out, "Suppressions") { + t.Errorf("unexpected Suppressions group:\n%s", out) + } + if strings.Contains(out, "Reported") { + t.Errorf("Reported line should only appear when something is suppressed:\n%s", out) + } +} + +func TestSummaryBaselineGroup(t *testing.T) { + baseline := makeReport( + makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a")), + makeResult("gone", Error, "c.java", 3, fp("id-gone", "trace-gone")), + ) + report := makeReport( + makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a")), + makeResult("fresh", Error, "b.java", 2, fp("id-fresh", "trace-fresh")), + ) + cmp, err := CompareToBaseline(report, baseline, SourceSinkFingerprintKey) + if err != nil { + t.Fatalf("compare: %v", err) + } + + out := renderSummary(t, report, &TriageView{ + BaselinePath: "reports/main.sarif", + Comparison: cmp, + }) + + for _, want := range []string{"Baseline", "reports/main.sarif", "New", "Unchanged", "Fixed"} { + if !strings.Contains(out, want) { + t.Errorf("missing %q in summary:\n%s", want, out) + } + } + if !strings.Contains(out, "Written to report") { + t.Errorf("summary must say whether states were persisted:\n%s", out) + } +} + +func TestSummaryBaselineGroupOmitsZeroUpdated(t *testing.T) { + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + report := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + cmp, _ := CompareToBaseline(report, baseline, SourceSinkFingerprintKey) + + out := renderSummary(t, report, &TriageView{BaselinePath: "b.sarif", Comparison: cmp}) + if strings.Contains(out, "Updated") { + t.Errorf("zero-valued Updated line should be omitted:\n%s", out) + } + if !strings.Contains(out, "Unchanged") { + t.Errorf("non-zero Unchanged should be shown:\n%s", out) + } +} + +func TestSummaryBaselineGroupReportsUnmatchable(t *testing.T) { + baseline := makeReport(makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a"))) + report := makeReport(makeResult("nofp", Error, "b.java", 2, nil)) + cmp, _ := CompareToBaseline(report, baseline, SourceSinkFingerprintKey) + + out := renderSummary(t, report, &TriageView{BaselinePath: "b.sarif", Comparison: cmp}) + if !strings.Contains(out, "Not comparable") { + t.Errorf("unmatchable findings must be surfaced:\n%s", out) + } +} + +func TestSummarySuppressionsGroup(t *testing.T) { + report := makeReport( + suppressed("a", "id-a", Accepted, "won't fix"), + suppressed("b", "id-b", UnderReview, "not now"), + makeResult("c", Error, "c.java", 3, fp("id-c", "trace-c")), + ) + + out := renderSummary(t, report, &TriageView{ + Suppressions: CollectSuppressionStats(report), + Inherited: 1, + }) + + for _, want := range []string{"Suppressions", "Suppressed", "Won't fix", "Deferred", "Inherited from baseline"} { + if !strings.Contains(out, want) { + t.Errorf("missing %q in summary:\n%s", want, out) + } + } + if !strings.Contains(out, "Reported") { + t.Errorf("Findings group should report the unsuppressed count:\n%s", out) + } +} + +func TestSummarySuppressionsGroupShowsAddedOnlyWhenRelevant(t *testing.T) { + report := makeReport(suppressed("a", "id-a", Accepted, "won't fix")) + stats := CollectSuppressionStats(report) + + out := renderSummary(t, report, &TriageView{Suppressions: stats}) + if strings.Contains(out, "Added this run") { + t.Errorf("Added line should be omitted when nothing was added:\n%s", out) + } + + out = renderSummary(t, report, &TriageView{Suppressions: stats, Added: 1}) + if !strings.Contains(out, "Added this run") { + t.Errorf("Added line expected:\n%s", out) + } +} + +func TestSummarySuppressionsGroupReportsNotHonored(t *testing.T) { + report := makeReport(suppressed("a", "id-a", Rejected, "denied")) + out := renderSummary(t, report, &TriageView{Suppressions: CollectSuppressionStats(report)}) + if !strings.Contains(out, "Not honored") { + t.Errorf("rejected suppressions must be surfaced:\n%s", out) + } +} diff --git a/cli/internal/sarif/triage_view.go b/cli/internal/sarif/triage_view.go new file mode 100644 index 000000000..044d87036 --- /dev/null +++ b/cli/internal/sarif/triage_view.go @@ -0,0 +1,107 @@ +package sarif + +import ( + "fmt" + + "github.com/seqra/opentaint/internal/output" +) + +// TriageView is the baseline and suppression state of a report, as computed by +// the command that is about to print it. A nil *TriageView means neither +// applies and the summary renders exactly as it did before triage existed. +type TriageView struct { + // BaselinePath is the baseline the report was compared against, shown so the + // reader can tell which report the counts are relative to. + BaselinePath string + // Comparison is the classification against that baseline, or nil when no + // baseline was supplied. + Comparison *Comparison + // StateWritten records whether baselineState was persisted into the report + // (--baseline-state) or only computed for display. + StateWritten bool + // ReadOnly means the command never writes the report, so reporting whether + // the state was persisted would be noise. + ReadOnly bool + + // Suppressions counts the suppression state of the report. + Suppressions SuppressionStats + // Inherited counts suppressions carried over from the baseline in this run. + Inherited int + // Added counts suppressions authored in this run (triage --accept/--defer). + Added int +} + +// baselineItems renders the Baseline group, or nil when no baseline applies. +// Zero-valued state counts are omitted so the group stays readable; the states +// that matter are the ones that happened. +func (v *TriageView) baselineItems(out *output.Printer) []any { + if v == nil || v.Comparison == nil { + return nil + } + + items := []any{} + if v.BaselinePath != "" { + items = append(items, out.FieldItem("Baseline", v.BaselinePath)) + } + for _, entry := range []struct { + label string + state BaselineState + }{ + {"New", New}, + {"Unchanged", Unchanged}, + {"Updated", Updated}, + } { + if count := v.Comparison.Counts[entry.state]; count > 0 { + items = append(items, out.FieldItem(entry.label, count)) + } + } + // "Fixed" reads better than SARIF's "absent" for a finding that is gone. + if count := v.Comparison.Counts[Absent]; count > 0 { + items = append(items, out.FieldItem("Fixed", count)) + } + if v.Comparison.Unmatchable > 0 { + items = append(items, out.FieldItem("Not comparable", v.Comparison.Unmatchable)) + } + + if v.ReadOnly { + return items + } + written := "no" + if v.StateWritten { + written = "yes" + } + return append(items, out.FieldItem("Written to report", written)) +} + +// suppressionItems renders the Suppressions group, or nil when the report +// carries no suppressions at all. +func (v *TriageView) suppressionItems(out *output.Printer) []any { + if v == nil || !v.Suppressions.Any() { + return nil + } + + stats := v.Suppressions + items := []any{ + out.FieldItem("Suppressed", suppressedOf(stats)), + } + if stats.WontFix > 0 { + items = append(items, out.FieldItem("Won't fix", stats.WontFix)) + } + if stats.Deferred > 0 { + items = append(items, out.FieldItem("Deferred", stats.Deferred)) + } + if stats.NotHonored > 0 { + items = append(items, out.FieldItem("Not honored", stats.NotHonored)) + } + if v.Inherited > 0 { + items = append(items, out.FieldItem("Inherited from baseline", v.Inherited)) + } + if v.Added > 0 { + items = append(items, out.FieldItem("Added this run", v.Added)) + } + return items +} + +func suppressedOf(stats SuppressionStats) string { + return fmt.Sprintf("%d of %d", stats.Suppressed, stats.Total) +} diff --git a/cli/internal/sarif/triage_view_test.go b/cli/internal/sarif/triage_view_test.go new file mode 100644 index 000000000..bd48ae3e5 --- /dev/null +++ b/cli/internal/sarif/triage_view_test.go @@ -0,0 +1,100 @@ +package sarif + +import ( + "strings" + "testing" +) + +func newState(s BaselineState) *BaselineState { return &s } + +func TestFilterByBaselineState(t *testing.T) { + a := makeResult("a", Error, "a.java", 1, fp("id-a", "trace-a")) + a.BaselineState = newState(New) + b := makeResult("b", Error, "b.java", 2, fp("id-b", "trace-b")) + b.BaselineState = newState(Unchanged) + c := makeResult("c", Error, "c.java", 3, fp("id-c", "trace-c")) + report := makeReport(a, b, c) + + got := report.Filter(Filters{BaselineStates: []string{"new"}}) + if len(got.Runs[0].Results) != 1 || *got.Runs[0].Results[0].RuleID != "a" { + t.Errorf("expected only the new finding, got %d results", len(got.Runs[0].Results)) + } + + got = report.Filter(Filters{BaselineStates: []string{"new", "unchanged"}}) + if len(got.Runs[0].Results) != 2 { + t.Errorf("expected 2 results, got %d", len(got.Runs[0].Results)) + } +} + +func TestFilterByBaselineStateIsCaseInsensitive(t *testing.T) { + a := makeResult("a", Error, "a.java", 1, nil) + a.BaselineState = newState(New) + got := makeReport(a).Filter(Filters{BaselineStates: []string{" NEW "}}) + if len(got.Runs[0].Results) != 1 { + t.Errorf("expected 1 result, got %d", len(got.Runs[0].Results)) + } +} + +func TestParseBaselineStatesValidatesValues(t *testing.T) { + if _, err := ParseBaselineStates([]string{"new", "absent"}); err != nil { + t.Errorf("unexpected error: %v", err) + } + if _, err := ParseBaselineStates([]string{"nope"}); err == nil { + t.Error("expected an error for an unknown baseline state") + } +} + +func TestPrintAllHidesSuppressedByDefault(t *testing.T) { + rendered := renderListing(t, makeReport( + suppressed("hidden.rule", "id-a", Accepted, "admin-only input"), + makeResult("shown.rule", Error, "b.java", 2, fp("id-b", "trace-b")), + ), ListingOptions{MaxNestingLevel: -1}) + + if strings.Contains(rendered, "hidden.rule") { + t.Errorf("suppressed finding should be hidden by default:\n%s", rendered) + } + if !strings.Contains(rendered, "shown.rule") { + t.Errorf("unsuppressed finding should be listed:\n%s", rendered) + } +} + +func TestPrintAllShowsSuppressedWithJustification(t *testing.T) { + rendered := renderListing(t, makeReport(suppressed("hidden.rule", "id-a", Accepted, "admin-only input")), ListingOptions{MaxNestingLevel: -1, ShowSuppressed: true}) + + if !strings.Contains(rendered, "hidden.rule") { + t.Errorf("finding should be listed with ShowSuppressed:\n%s", rendered) + } + if !strings.Contains(rendered, "admin-only input") { + t.Errorf("justification should be shown:\n%s", rendered) + } + if !strings.Contains(rendered, "accepted") { + t.Errorf("status should be shown:\n%s", rendered) + } +} + +func TestPrintAllShowsBaselineState(t *testing.T) { + r := makeResult("a.rule", Error, "a.java", 1, fp("id-a", "trace-a")) + r.BaselineState = newState(New) + rendered := renderListing(t, makeReport(r), ListingOptions{MaxNestingLevel: -1}) + + if !strings.Contains(rendered, "Baseline") || !strings.Contains(rendered, "new") { + t.Errorf("expected a baseline state field:\n%s", rendered) + } +} + +func TestPrintAllOmitsBaselineFieldWhenAbsent(t *testing.T) { + rendered := renderListing(t, makeReport(makeResult("a.rule", Error, "a.java", 1, nil)), + ListingOptions{MaxNestingLevel: -1}) + + if strings.Contains(rendered, "Baseline") { + t.Errorf("no baseline field expected without a comparison:\n%s", rendered) + } +} + +func TestPrintAllAllSuppressedRendersNothing(t *testing.T) { + rendered := renderListing(t, makeReport(suppressed("hidden.rule", "id-a", Accepted, "why")), + ListingOptions{MaxNestingLevel: -1}) + if strings.TrimSpace(rendered) != "" { + t.Errorf("expected no output, got:\n%s", rendered) + } +} diff --git a/cli/internal/sarif/utils.go b/cli/internal/sarif/utils.go index 9970f7f45..6617ac2b7 100644 --- a/cli/internal/sarif/utils.go +++ b/cli/internal/sarif/utils.go @@ -61,6 +61,10 @@ type RuleSummary struct { Notes int } +// LevelOf returns the result's SARIF level, defaulting to "note" when absent — +// the same reading the summary and the filters use. +func LevelOf(result *Result) Level { return findingLevel(result) } + func findingLevel(result *Result) Level { if result == nil || result.Level == nil || *result.Level == "" { return Note @@ -202,8 +206,11 @@ func pluralize(count int, singular string) string { return singular + "s" } -// PrintSummary prints a human-readable summary of the SARIF report -func (report *Report) PrintSummary(out *output.Printer, absSarifReportPath string) { +// PrintSummary prints a human-readable summary of the SARIF report. view is the +// baseline/suppression state to report alongside it, or nil when neither +// applies — in which case the output is exactly what it was before triage +// existed. +func (report *Report) PrintSummary(out *output.Printer, absSarifReportPath string, view *TriageView) { summary := GenerateSummary(report) ruleSummary := generateRuleSummary(report) @@ -244,17 +251,24 @@ func (report *Report) PrintSummary(out *output.Printer, absSarifReportPath strin rulesTriggered = out.FieldItem("Rules triggered", summary.TotalRulesTriggered) } - out.Section("Scan Summary"). - Group("Findings", - out.FieldItem("Total", totalLine), - out.FieldItem("Files affected", findingFiles(report)), - out.FieldItem("Rules executed", summary.TotalRulesExecuted), - rulesTriggered, - ). - Group("Output", - outputItems(out, absSarifReportPath)..., - ). - Render() + findings := []any{out.FieldItem("Total", totalLine)} + if view != nil && view.Suppressions.Suppressed > 0 { + findings = append(findings, out.FieldItem("Reported", view.Suppressions.Total-view.Suppressions.Suppressed)) + } + findings = append(findings, + out.FieldItem("Files affected", findingFiles(report)), + out.FieldItem("Rules executed", summary.TotalRulesExecuted), + rulesTriggered, + ) + + section := out.Section("Scan Summary").Group("Findings", findings...) + if items := view.baselineItems(out); len(items) > 0 { + section.Group("Baseline", items...) + } + if items := view.suppressionItems(out); len(items) > 0 { + section.Group("Suppressions", items...) + } + section.Group("Output", outputItems(out, absSarifReportPath)...).Render() } func outputItems(out *output.Printer, absSarifReportPath string) []any { diff --git a/cli/internal/triage/gate.go b/cli/internal/triage/gate.go new file mode 100644 index 000000000..61809c05a --- /dev/null +++ b/cli/internal/triage/gate.go @@ -0,0 +1,86 @@ +package triage + +import ( + "strings" + + "github.com/seqra/opentaint/internal/sarif" +) + +// Gate decides whether a report should fail the build. +// +// A finding counts when it is not suppressed and its level is in scope. With a +// baseline, only findings the comparison could not account for count: "new" +// ones, and ones it could not compare at all (no identity fingerprint), which +// fail closed rather than slipping through unnoticed. "unchanged" and "updated" +// findings existed before and do not fail the build. +type Gate struct { + // Enabled turns the gate on (--error-on-findings). Off by default, which + // keeps the historical behavior of never failing on findings. + Enabled bool + // Severities restricts which SARIF levels count. Empty means every level. + Severities []string +} + +// Evaluate returns the number of findings that count and whether the gate trips. +func (g Gate) Evaluate(report *sarif.Report, view *sarif.TriageView) (int, bool) { + if !g.Enabled { + return 0, false + } + + count := 0 + for _, r := range report.Results() { + if sarif.IsSuppressed(r) { + continue + } + if !g.inScope(r) { + continue + } + if !counts(r, view) { + continue + } + count++ + } + return count, count > 0 +} + +// counts reports whether a finding is one the gate should care about given the +// baseline comparison, if any. +func counts(r *sarif.Result, view *sarif.TriageView) bool { + if view == nil || view.Comparison == nil { + return true + } + switch view.Comparison.StateOf(r) { + case sarif.New: + return true + case "": + // Not comparable against the baseline: fail closed. + return true + default: + return false + } +} + +func (g Gate) inScope(r *sarif.Result) bool { + return len(g.Severities) == 0 || sarif.MatchesSeverity(r, g.Severities) +} + +// ParseGateSeverities validates --error-on-severity values. The flag is +// repeatable, and each value may also be a comma-separated list, so +// "--error-on-severity error,warning" and "--error-on-severity error +// --error-on-severity warning" mean the same thing. +func ParseGateSeverities(values []string) ([]string, error) { + var out []string + for _, v := range values { + for _, token := range strings.Split(v, ",") { + normalized := strings.ToLower(strings.TrimSpace(token)) + if normalized == "" { + continue + } + if err := sarif.ValidateSeverity(normalized); err != nil { + return nil, err + } + out = append(out, normalized) + } + } + return out, nil +} diff --git a/cli/internal/triage/gate_test.go b/cli/internal/triage/gate_test.go new file mode 100644 index 000000000..1fc9dcfae --- /dev/null +++ b/cli/internal/triage/gate_test.go @@ -0,0 +1,172 @@ +package triage + +import ( + "testing" + + "github.com/seqra/opentaint/internal/sarif" +) + +func warn(ruleID, identity string) sarif.Result { + r := result(ruleID, identity, "trace-"+identity) + r.Level = lvlptr(sarif.Warning) + return r +} + +func TestGateDisabledNeverTrips(t *testing.T) { + rep := report(result("a", "id-a", "trace-a")) + out, _ := Apply(rep, Options{}) + count, tripped := Gate{}.Evaluate(rep, out.View) + if tripped { + t.Error("a disabled gate must never trip") + } + if count != 0 { + t.Errorf("count: got %d, want 0", count) + } +} + +func TestGateCountsEveryFindingWithoutBaseline(t *testing.T) { + rep := report(result("a", "id-a", "trace-a"), warn("b", "id-b")) + out, _ := Apply(rep, Options{}) + count, tripped := Gate{Enabled: true}.Evaluate(rep, out.View) + if !tripped || count != 2 { + t.Errorf("got (%d, %v), want (2, true)", count, tripped) + } +} + +func TestGateIgnoresSuppressedFindings(t *testing.T) { + rep := report(result("a", "id-aaa", "trace-a"), result("b", "id-bbb", "trace-b")) + out, err := Apply(rep, Options{Accept: []string{"id-aaa"}, Justification: "why"}) + if err != nil { + t.Fatal(err) + } + count, tripped := Gate{Enabled: true}.Evaluate(rep, out.View) + if count != 1 || !tripped { + t.Errorf("got (%d, %v), want (1, true)", count, tripped) + } +} + +func TestGateIgnoresDeferredFindings(t *testing.T) { + rep := report(result("a", "id-aaa", "trace-a")) + out, err := Apply(rep, Options{Defer: []string{"id-aaa"}, Justification: "not now"}) + if err != nil { + t.Fatal(err) + } + if _, tripped := (Gate{Enabled: true}).Evaluate(rep, out.View); tripped { + t.Error("a deferred finding must not trip the gate") + } +} + +func TestGateWithBaselineCountsOnlyNewFindings(t *testing.T) { + baseline := report(result("old", "id-old", "trace-old")) + rep := report(result("old", "id-old", "trace-old"), result("new", "id-new", "trace-new")) + out, err := Apply(rep, Options{Baseline: baseline}) + if err != nil { + t.Fatal(err) + } + count, tripped := Gate{Enabled: true}.Evaluate(rep, out.View) + if count != 1 || !tripped { + t.Errorf("got (%d, %v), want (1, true): only the new finding counts", count, tripped) + } +} + +func TestGateWithBaselineDoesNotTripWhenNothingIsNew(t *testing.T) { + baseline := report(result("old", "id-old", "trace-old")) + rep := report(result("old", "id-old", "trace-old")) + out, err := Apply(rep, Options{Baseline: baseline}) + if err != nil { + t.Fatal(err) + } + if _, tripped := (Gate{Enabled: true}).Evaluate(rep, out.View); tripped { + t.Error("an unchanged report must not trip the gate") + } +} + +func TestGateDoesNotCountUpdatedFindings(t *testing.T) { + baseline := report(result("a", "id-a", "trace-old")) + rep := report(result("a", "id-a", "trace-new")) + out, err := Apply(rep, Options{Baseline: baseline}) + if err != nil { + t.Fatal(err) + } + if _, tripped := (Gate{Enabled: true}).Evaluate(rep, out.View); tripped { + t.Error("an updated finding is the same accepted vulnerability through a new path, not a new finding") + } +} + +func TestGateCountsUncomparableFindings(t *testing.T) { + // A finding with no identity fingerprint cannot be matched against the + // baseline. Fail closed: it is reported and it counts. + baseline := report(result("old", "id-old", "trace-old")) + nofp := sarif.Result{RuleID: strptr("nofp"), Level: lvlptr(sarif.Error)} + rep := report(result("old", "id-old", "trace-old")) + rep.Runs[0].Results = append(rep.Runs[0].Results, nofp) + + out, err := Apply(rep, Options{Baseline: baseline}) + if err != nil { + t.Fatal(err) + } + count, tripped := Gate{Enabled: true}.Evaluate(rep, out.View) + if count != 1 || !tripped { + t.Errorf("got (%d, %v), want (1, true)", count, tripped) + } +} + +func TestGateRestrictsToSeverities(t *testing.T) { + rep := report(result("a", "id-a", "trace-a"), warn("b", "id-b")) + out, _ := Apply(rep, Options{}) + + count, tripped := Gate{Enabled: true, Severities: []string{"error"}}.Evaluate(rep, out.View) + if count != 1 || !tripped { + t.Errorf("got (%d, %v), want (1, true)", count, tripped) + } + + count, tripped = Gate{Enabled: true, Severities: []string{"note"}}.Evaluate(rep, out.View) + if count != 0 || tripped { + t.Errorf("got (%d, %v), want (0, false)", count, tripped) + } +} + +func TestParseGateSeverities(t *testing.T) { + if _, err := ParseGateSeverities([]string{"error", "warning"}); err != nil { + t.Errorf("unexpected error: %v", err) + } + if _, err := ParseGateSeverities([]string{"critical"}); err == nil { + t.Error("expected an error for an unknown severity") + } +} + +func TestParseGateSeveritiesSplitsCommaSeparated(t *testing.T) { + got, err := ParseGateSeverities([]string{"error,warning"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 2 || got[0] != "error" || got[1] != "warning" { + t.Errorf("got %v, want [error warning]", got) + } +} + +func TestParseGateSeveritiesMixesCommaAndRepeatedFlags(t *testing.T) { + got, err := ParseGateSeverities([]string{"error, warning", "note"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 3 { + t.Errorf("got %v, want error warning note", got) + } +} + +func TestParseGateSeveritiesRejectsBadTokenInsideAList(t *testing.T) { + if _, err := ParseGateSeverities([]string{"error,bogus"}); err == nil { + t.Error("expected an error for a bad token in a comma list") + } +} + +func TestParseGateSeveritiesIgnoresEmptyTokens(t *testing.T) { + got, err := ParseGateSeverities([]string{"error,,warning,"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 2 { + t.Errorf("got %v, want [error warning]", got) + } +} diff --git a/cli/internal/triage/triage.go b/cli/internal/triage/triage.go new file mode 100644 index 000000000..02b6fb4f4 --- /dev/null +++ b/cli/internal/triage/triage.go @@ -0,0 +1,169 @@ +// Package triage applies baselines and suppressions to a SARIF report. It is +// the single implementation behind the `triage` command, the annotation step of +// `scan`, and the read-only view `summary` renders. +package triage + +import ( + "fmt" + + "github.com/seqra/opentaint/internal/sarif" +) + +// Options describes one triage pass over a report. +type Options struct { + // Baseline is the previously produced report to compare against, or nil. + Baseline *sarif.Report + // BaselinePath is that report's path, for display only. + BaselinePath string + // WriteBaselineState persists result.baselineState and run.baselineGuid. + // Without it the comparison only drives what is printed. + WriteBaselineState bool + // FingerprintKey selects the identity fingerprint ("" = default). + FingerprintKey string + // ReadOnly means the caller will never persist the report. The comparison is + // still applied to the in-memory copy so that --baseline-state can filter on + // it, but nothing is reported as written or changed. This is what summary + // uses. + ReadOnly bool + + // Accept, Defer and Unsuppress name findings by fingerprint prefix. + Accept []string + Defer []string + Unsuppress []string + // Justification is required whenever Accept or Defer is non-empty. + Justification string +} + +// suppressing reports whether the options author any new decision. +func (o Options) suppressing() bool { + return len(o.Accept) > 0 || len(o.Defer) > 0 +} + +// Outcome is what one triage pass produced. +type Outcome struct { + // View is the baseline and suppression state to print. + View *sarif.TriageView + // Changed reports whether the report was modified and needs writing back. + Changed bool +} + +// Apply runs a triage pass over report, mutating it in place. +// +// Order matters: suppressions are inherited from the baseline first, so that a +// decision made in a previous cycle is visible; then explicit accept/defer +// decisions from this run overwrite them; then the baseline comparison is +// computed over the final state. +func Apply(report *sarif.Report, opts Options) (*Outcome, error) { + key, err := sarif.ResolveIdentityKey(opts.FingerprintKey) + if err != nil { + return nil, err + } + if opts.suppressing() && opts.Justification == "" { + return nil, fmt.Errorf("a justification is required to suppress a finding: pass --justification") + } + + view := &sarif.TriageView{BaselinePath: opts.BaselinePath, ReadOnly: opts.ReadOnly} + changed := false + + if opts.Baseline != nil { + view.Inherited = sarif.InheritSuppressions(report, opts.Baseline, key) + changed = changed || view.Inherited > 0 + } + + added, err := applyDecisions(report, key, opts) + if err != nil { + return nil, err + } + view.Added = added + changed = changed || added > 0 + + removed, err := applyUnsuppressions(report, key, opts.Unsuppress) + if err != nil { + return nil, err + } + changed = changed || removed > 0 + + if opts.Baseline != nil { + comparison, err := sarif.CompareToBaseline(report, opts.Baseline, key) + if err != nil { + return nil, err + } + view.Comparison = comparison + if opts.WriteBaselineState || opts.ReadOnly { + comparison.Apply(report) + view.StateWritten = opts.WriteBaselineState && !opts.ReadOnly + changed = changed || view.StateWritten + } + } + + if opts.ReadOnly { + changed = false + } + if changed { + // A report the CLI has written must be citable as the next baseline. + sarif.EnsureRunGUIDs(report) + } + + view.Suppressions = sarif.CollectSuppressionStats(report) + return &Outcome{View: view, Changed: changed}, nil +} + +// applyDecisions resolves each accept/defer prefix and records the decision. +// Every prefix is resolved before anything is written, so a typo in the second +// of three prefixes leaves the report untouched rather than half-triaged. +func applyDecisions(report *sarif.Report, key string, opts Options) (int, error) { + type decision struct { + result *sarif.Result + accept bool + } + + var decisions []decision + for _, prefix := range opts.Accept { + r, err := sarif.ResolvePrefix(report, key, prefix) + if err != nil { + return 0, err + } + decisions = append(decisions, decision{result: r, accept: true}) + } + for _, prefix := range opts.Defer { + r, err := sarif.ResolvePrefix(report, key, prefix) + if err != nil { + return 0, err + } + decisions = append(decisions, decision{result: r}) + } + + for _, d := range decisions { + var err error + if d.accept { + err = sarif.Accept(d.result, opts.Justification) + } else { + err = sarif.Defer(d.result, opts.Justification) + } + if err != nil { + return 0, err + } + } + return len(decisions), nil +} + +// applyUnsuppressions resolves every prefix before removing anything, for the +// same all-or-nothing reason as applyDecisions. +func applyUnsuppressions(report *sarif.Report, key string, prefixes []string) (int, error) { + var targets []*sarif.Result + for _, prefix := range prefixes { + r, err := sarif.ResolvePrefix(report, key, prefix) + if err != nil { + return 0, err + } + targets = append(targets, r) + } + + removed := 0 + for _, r := range targets { + if sarif.Unsuppress(r) { + removed++ + } + } + return removed, nil +} diff --git a/cli/internal/triage/triage_test.go b/cli/internal/triage/triage_test.go new file mode 100644 index 000000000..bfca61293 --- /dev/null +++ b/cli/internal/triage/triage_test.go @@ -0,0 +1,258 @@ +package triage + +import ( + "strings" + "testing" + + "github.com/seqra/opentaint/internal/sarif" +) + +func strptr(s string) *string { return &s } +func lvlptr(l sarif.Level) *sarif.Level { return &l } + +func result(ruleID, identity string, trace string) sarif.Result { + return sarif.Result{ + RuleID: strptr(ruleID), + Level: lvlptr(sarif.Error), + Locations: []sarif.Location{{ + PhysicalLocation: &sarif.PhysicalLocation{ + ArtifactLocation: &sarif.ArtifactLocation{URI: strptr(ruleID + ".java")}, + }, + }}, + PartialFingerprints: map[string]string{ + sarif.SourceSinkFingerprintKey: identity, + sarif.TraceFingerprintKey: trace, + }, + } +} + +func report(results ...sarif.Result) *sarif.Report { + return &sarif.Report{Runs: []sarif.Run{{Results: results}}} +} + +func TestApplyWithNoOptionsChangesNothing(t *testing.T) { + r := report(result("a", "id-a", "trace-a")) + out, err := Apply(r, Options{}) + if err != nil { + t.Fatalf("apply: %v", err) + } + if out.Changed { + t.Error("expected no change") + } + if out.View.Comparison != nil { + t.Error("expected no comparison without a baseline") + } +} + +func TestApplyInheritsSuppressionsFromBaseline(t *testing.T) { + base := result("a", "id-a", "trace-a") + if err := sarif.Accept(&base, "admin-only input"); err != nil { + t.Fatal(err) + } + current := report(result("a", "id-a", "trace-a"), result("b", "id-b", "trace-b")) + + out, err := Apply(current, Options{Baseline: report(base)}) + if err != nil { + t.Fatalf("apply: %v", err) + } + if out.View.Inherited != 1 { + t.Errorf("inherited: got %d, want 1", out.View.Inherited) + } + if !sarif.IsSuppressed(current.Results()[0]) { + t.Error("matching finding should have inherited the suppression") + } + if !out.Changed { + t.Error("inheriting a suppression changes the report") + } +} + +func TestApplyComparesButDoesNotWriteStateByDefault(t *testing.T) { + current := report(result("a", "id-a", "trace-a"), result("b", "id-b", "trace-b")) + out, err := Apply(current, Options{Baseline: report(result("a", "id-a", "trace-a"))}) + if err != nil { + t.Fatalf("apply: %v", err) + } + if out.View.Comparison.Counts[sarif.New] != 1 { + t.Errorf("expected 1 new, got %d", out.View.Comparison.Counts[sarif.New]) + } + for _, r := range current.Results() { + if r.BaselineState != nil { + t.Error("baselineState must not be written without WriteBaselineState") + } + } + if out.View.StateWritten { + t.Error("StateWritten should be false") + } + if out.Changed { + t.Error("a comparison alone does not change the report") + } +} + +func TestApplyWritesStateWhenAsked(t *testing.T) { + current := report(result("a", "id-a", "trace-a")) + out, err := Apply(current, Options{ + Baseline: report(result("a", "id-a", "trace-a")), + WriteBaselineState: true, + }) + if err != nil { + t.Fatalf("apply: %v", err) + } + if current.Results()[0].BaselineState == nil { + t.Fatal("baselineState not written") + } + if !out.View.StateWritten || !out.Changed { + t.Error("writing state marks the report changed") + } + if current.RunGUID() == "" { + t.Error("a written report must be citable as a baseline: expected a run guid") + } +} + +func TestApplyAcceptsByFingerprintPrefix(t *testing.T) { + current := report(result("a", "id-aaa111", "trace-a"), result("b", "id-bbb222", "trace-b")) + out, err := Apply(current, Options{ + Accept: []string{"id-aaa"}, + Justification: "sink is a constant", + }) + if err != nil { + t.Fatalf("apply: %v", err) + } + if out.View.Added != 1 { + t.Errorf("added: got %d, want 1", out.View.Added) + } + first := current.Results()[0] + if !sarif.IsSuppressed(first) || sarif.StatusOf(first) != "accepted" { + t.Errorf("expected an accepted suppression, got %q", sarif.StatusOf(first)) + } + if sarif.IsSuppressed(current.Results()[1]) { + t.Error("the other finding must be untouched") + } +} + +func TestApplyDefersByFingerprintPrefix(t *testing.T) { + current := report(result("a", "id-aaa111", "trace-a")) + if _, err := Apply(current, Options{Defer: []string{"id-aaa"}, Justification: "waiting on OT-412"}); err != nil { + t.Fatalf("apply: %v", err) + } + if got := sarif.StatusOf(current.Results()[0]); got != "underReview" { + t.Errorf("status: got %q, want underReview", got) + } +} + +func TestApplyUnsuppresses(t *testing.T) { + r := result("a", "id-aaa111", "trace-a") + if err := sarif.Accept(&r, "was accepted"); err != nil { + t.Fatal(err) + } + current := report(r) + + out, err := Apply(current, Options{Unsuppress: []string{"id-aaa"}}) + if err != nil { + t.Fatalf("apply: %v", err) + } + if sarif.IsSuppressed(current.Results()[0]) { + t.Error("expected the suppression to be removed") + } + if !out.Changed { + t.Error("removing a suppression changes the report") + } +} + +func TestApplyRequiresJustificationForAccept(t *testing.T) { + current := report(result("a", "id-aaa111", "trace-a")) + _, err := Apply(current, Options{Accept: []string{"id-aaa"}}) + if err == nil || !strings.Contains(err.Error(), "justification") { + t.Errorf("expected a justification error, got %v", err) + } + if sarif.IsSuppressed(current.Results()[0]) { + t.Error("nothing should be suppressed when validation fails") + } +} + +func TestApplyRejectsUnknownFingerprint(t *testing.T) { + current := report(result("a", "id-aaa111", "trace-a")) + _, err := Apply(current, Options{Accept: []string{"zzz"}, Justification: "why"}) + if err == nil { + t.Error("expected an error for an unmatched fingerprint") + } +} + +func TestApplyRejectsAmbiguousFingerprint(t *testing.T) { + current := report(result("a", "id-aaa111", "trace-a"), result("b", "id-aaa222", "trace-b")) + _, err := Apply(current, Options{Accept: []string{"id-aaa"}, Justification: "why"}) + if err == nil || !strings.Contains(err.Error(), "ambiguous") { + t.Errorf("expected an ambiguity error, got %v", err) + } +} + +func TestApplyPropagatesBaselineKeyMismatch(t *testing.T) { + baseline := &sarif.Report{Runs: []sarif.Run{{Results: []sarif.Result{{ + RuleID: strptr("a"), + PartialFingerprints: map[string]string{"someOtherKey/v1": "x"}, + }}}}} + _, err := Apply(report(result("a", "id-a", "trace-a")), Options{Baseline: baseline}) + if err == nil { + t.Error("expected an error when the baseline lacks the identity key") + } +} + +func TestApplySuppressionStatsCoverTheWholeReport(t *testing.T) { + current := report(result("a", "id-aaa", "trace-a"), result("b", "id-bbb", "trace-b")) + out, err := Apply(current, Options{Accept: []string{"id-aaa"}, Justification: "why"}) + if err != nil { + t.Fatalf("apply: %v", err) + } + if out.View.Suppressions.Total != 2 || out.View.Suppressions.Suppressed != 1 { + t.Errorf("stats: got %+v", out.View.Suppressions) + } +} + +func TestApplyReadOnlyAnnotatesInMemoryWithoutClaimingToWrite(t *testing.T) { + // summary never writes the report, but it still needs baselineState on the + // in-memory copy so that --baseline-state can filter on it. + current := report(result("a", "id-a", "trace-a"), result("b", "id-b", "trace-b")) + out, err := Apply(current, Options{ + Baseline: report(result("a", "id-a", "trace-a")), + ReadOnly: true, + }) + if err != nil { + t.Fatalf("apply: %v", err) + } + states := []string{} + for _, r := range current.Results() { + if r.BaselineState == nil { + t.Fatal("read-only mode must still annotate the in-memory report") + } + states = append(states, string(*r.BaselineState)) + } + if states[0] != "unchanged" || states[1] != "new" { + t.Errorf("states: got %v", states) + } + if out.Changed { + t.Error("read-only mode must never mark the report as needing a write") + } + if out.View.StateWritten { + t.Error("read-only mode must not claim the state was persisted") + } + if !out.View.ReadOnly { + t.Error("the view should record that nothing will be written") + } +} + +func TestApplyReadOnlyStillInheritsSuppressions(t *testing.T) { + base := result("a", "id-a", "trace-a") + if err := sarif.Accept(&base, "admin-only"); err != nil { + t.Fatal(err) + } + current := report(result("a", "id-a", "trace-a")) + out, err := Apply(current, Options{Baseline: report(base), ReadOnly: true}) + if err != nil { + t.Fatal(err) + } + if !sarif.IsSuppressed(current.Results()[0]) { + t.Error("read-only display must still show inherited suppressions") + } + if out.Changed { + t.Error("read-only mode must not mark the report as changed") + } +} diff --git a/cli/internal/utils/opentaint_command_builder.go b/cli/internal/utils/opentaint_command_builder.go index 1356a557b..74f925186 100644 --- a/cli/internal/utils/opentaint_command_builder.go +++ b/cli/internal/utils/opentaint_command_builder.go @@ -389,3 +389,74 @@ func BuildScanCommandFromCompile(projectPath, projectModelPath string) string { WithOutput(outputPath). Build() } + +// WithBaseline sets the --baseline flag. +func (cb *OpentaintCommandBuilder) WithBaseline(path string) *OpentaintCommandBuilder { + if path != "" { + cb.flags["baseline"] = path + } + return cb +} + +// WithWriteBaselineState sets the --write-baseline-state flag (scan/triage). +func (cb *OpentaintCommandBuilder) WithWriteBaselineState(enabled bool) *OpentaintCommandBuilder { + if enabled { + cb.boolFlags["write-baseline-state"] = true + } + return cb +} + +// WithFingerprintKey sets the --fingerprint-key flag. +func (cb *OpentaintCommandBuilder) WithFingerprintKey(key string) *OpentaintCommandBuilder { + if key != "" { + cb.flags["fingerprint-key"] = key + } + return cb +} + +// WithErrorOnFindings sets the --error-on-findings flag. +func (cb *OpentaintCommandBuilder) WithErrorOnFindings(enabled bool) *OpentaintCommandBuilder { + if enabled { + cb.boolFlags["error-on-findings"] = true + } + return cb +} + +// WithErrorOnSeverity adds repeatable --error-on-severity filters. +func (cb *OpentaintCommandBuilder) WithErrorOnSeverity(severities []string) *OpentaintCommandBuilder { + for _, s := range severities { + if s != "" { + cb.arrayFlags["error-on-severity"] = append(cb.arrayFlags["error-on-severity"], s) + } + } + return cb +} + +// WithSuppressed sets the --suppressed flag. +func (cb *OpentaintCommandBuilder) WithSuppressed(enabled bool) *OpentaintCommandBuilder { + if enabled { + cb.boolFlags["suppressed"] = true + } + return cb +} + +// WithBaselineStateFilter adds repeatable --baseline-state selection values for +// the summary command, where the flag takes values rather than being a switch. +func (cb *OpentaintCommandBuilder) WithBaselineStateFilter(states []string) *OpentaintCommandBuilder { + for _, s := range states { + if s != "" { + cb.arrayFlags["baseline-state"] = append(cb.arrayFlags["baseline-state"], s) + } + } + return cb +} + +// WithExcludeRuleID adds repeatable --exclude-rule-id filters. +func (cb *OpentaintCommandBuilder) WithExcludeRuleID(ruleIDs []string) *OpentaintCommandBuilder { + for _, id := range ruleIDs { + if id != "" { + cb.arrayFlags["exclude-rule-id"] = append(cb.arrayFlags["exclude-rule-id"], id) + } + } + return cb +} diff --git a/docs/README.md b/docs/README.md index 760695b75..dfb16e3a5 100644 --- a/docs/README.md +++ b/docs/README.md @@ -16,6 +16,7 @@ - [Installation Guide](installation.md) - Full installation instructions - [Usage Guide](usage.md) - Comprehensive usage reference +- [Baselines & Suppressions](baselines-and-suppressions.md) - Baseline comparison, triage, and CI gating - [Configuration Guide](configuration.md) - All configuration options - [Docker](docker.md) - Run OpenTaint in containers and CI/CD pipelines - [Precompiled Classes and JARs Analysis](classes-and-jars-analysis.md) - Analyze pre-built artifacts when source compilation isn't available @@ -124,6 +125,7 @@ npx @seqra/opentaint scan # Run without installi opentaint scan --output results.sarif # Scan with explicit output path opentaint summary --show-findings results.sarif # View results opentaint summary --show-findings --verbose-flow --show-code-snippets results.sarif # Full detail +opentaint scan --baseline main.sarif --error-on-findings # Fail CI only on new findings ``` | Command | Description | @@ -132,6 +134,7 @@ opentaint summary --show-findings --verbose-flow --show-code-snippets results.sa | `opentaint compile` | Build project model separately | | `opentaint project` | Create model from precompiled JARs | | `opentaint summary` | View SARIF results | +| `opentaint triage` | Compare against a baseline and record suppressions | | `opentaint health` | Show resolved analyzer, autobuilder, rules, and runtime paths | | `opentaint test rule` | Scaffold, test, and debug detection rules | | `opentaint test approximation` | Scaffold and test dataflow approximations | @@ -166,6 +169,7 @@ For detailed configuration, see [Configuration Guide](configuration.md). - **GitHub Actions:** [seqra/opentaint/github](https://github.com/seqra/opentaint/tree/main/github) - **GitLab CI:** [seqra/opentaint/gitlab](https://github.com/seqra/opentaint/tree/main/gitlab) +- **Baseline gating** (fail only on *new* findings), triage, and copy-paste PR workflows: [Baselines & Suppressions](baselines-and-suppressions.md) --- diff --git a/docs/baselines-and-suppressions.md b/docs/baselines-and-suppressions.md new file mode 100644 index 000000000..14050d4b2 --- /dev/null +++ b/docs/baselines-and-suppressions.md @@ -0,0 +1,390 @@ +# Baselines, suppressions, and CI gating + +OpenTaint lets you adopt static analysis on an existing codebase without +drowning in the findings that were already there, and without hiding anything +silently. This guide covers three related capabilities: + +- **Baselines** — compare a scan against a previous report and tell what is new. +- **Suppressions** — record an explicit human decision to accept or defer a finding. +- **Gating** — fail a build on findings, optionally only on new ones. + +Everything is expressed in [SARIF 2.1.0](https://docs.oasis-open.org/sarif/sarif/v2.1.0/) +using the format's own fields, so any SARIF-aware tool (GitHub code scanning, +GitLab, IDEs) understands the output. + +## The mental model: two independent axes + +A finding sits on two axes that never interfere with each other. + +| Axis | Question | Where it lives | Set by | +|------|----------|----------------|--------| +| **Baseline state** | *Is this new?* | `result.baselineState` | `--baseline` comparison | +| **Suppression** | *Did a human accept this?* | `result.suppressions[]` | `opentaint triage` | + +The two are orthogonal. A finding can be old **and** unaccepted (it shows up as +`unchanged` and still counts). It can be new **and** already suppressed (rare, +but valid). Nothing about being in the baseline makes a finding "accepted" — only +a `triage` decision does that. + +Neither axis ever deletes a result. Suppressed and baselined findings stay in the +report, marked; the CLI filters them at display and gate time, not in the file. + +## Quick start + +```bash +# 1. Scan once. Keep the report — it is your baseline. +opentaint scan -o baseline.sarif . + +# 2. In CI, scan against it and fail only on new findings. +opentaint scan --baseline baseline.sarif --error-on-findings . +``` + +That is the whole ratchet: a codebase with 40 pre-existing findings does not turn +CI permanently red — only work introduced by the current change fails the build. + +## The lifecycle + +### 1. Establish a baseline + +A baseline is just a SARIF report you saved. There is no separate baseline +format and no suppressions file to maintain. + +```bash +opentaint scan -o baselines/main.sarif . +``` + +Commit that report (or store it as a CI artifact keyed to your default branch). + +### 2. Triage the findings you have reviewed + +`opentaint triage` records a decision about a finding directly in the report. +Two verdicts, each requiring a justification: + +```bash +# "Won't fix" — reviewed, accepted as not a real risk here. +opentaint triage baselines/main.sarif \ + --accept q3Vf9k --justification "MD5 is a cache key, not a secret hash" + +# "Not fixing yet" — real, but deferred. +opentaint triage baselines/main.sarif \ + --defer 8bc1d2 --justification "scheduled with the payments refactor (PAY-1420)" +``` + +A finding is named by a **fingerprint prefix**, git-style — the value shown as +`Fingerprint:` by `opentaint summary --show-findings`. An ambiguous or unknown +prefix is an error, never a guess. `--accept`, `--defer`, and `--unsuppress` are +repeatable; one `--justification` applies to every decision in the invocation. + +Each decision is written as a SARIF suppression (see +[Suppression reference](#suppression-reference)): + +```json +"suppressions": [{ + "kind": "external", + "status": "accepted", + "guid": "3f2a…", + "justification": "MD5 is a cache key, not a secret hash" +}] +``` + +### 3. Decisions travel forward + +When a later scan runs with `--baseline`, a current finding that matches a +baseline entry carrying a suppression **inherits it verbatim** — same status, +same justification, same guid. A decision is authored once and re-attached by +every scan afterwards, for as long as the finding's fingerprint still matches. +When the code is fixed and the finding disappears, its decision retires with it. + +This is why suppressions live in the report and not in a config file that would +accumulate dead entries forever. + +### 4. Gate CI on new findings + +```bash +opentaint scan --baseline baselines/main.sarif \ + --error-on-findings --error-on-severity error,warning -o scan.sarif . +``` + +With `--baseline`, the gate counts only findings that are **new** and **not +suppressed**. Without a baseline, it counts every reported (non-suppressed) +finding. See [The gate](#the-gate). + +### 5. Explain what changed + +```bash +opentaint summary scan.sarif --baseline baselines/main.sarif \ + --baseline-state new --show-findings +``` + +`--baseline-state` here is a **display filter** — it narrows the listing to the +findings in the states you name. This is a different flag from +`scan --write-baseline-state` (see the warning under +[Baseline reference](#baseline-reference)). + +## Baseline reference + +Given `--baseline old.sarif`, every current finding is classified: + +| State | Meaning | +|-------|---------| +| `new` | In this scan, not in the baseline | +| `unchanged` | In both, identical trace | +| `updated` | In both — same source and sink, but the path through the code changed | +| `absent` | In the baseline, gone now (i.e. fixed) | + +By default the comparison only affects **what is printed** — the SARIF file is +left byte-for-byte unchanged. Two flags control it: + +| Flag | Command | Effect | +|------|---------|--------| +| `--write-baseline-state` | `scan`, `triage` | **Switch.** Persists `result.baselineState` and `run.baselineGuid` into the output report. | +| `--baseline-state ` | `summary` | **Filter.** Shows only findings whose state is one of `new` \| `unchanged` \| `updated` \| `absent` (repeatable). | + +> **These are two different flags that share the word "baseline-state."** +> On `scan`/`triage` it is a boolean that *writes* the state into the file. +> On `summary` it takes a value and *filters* the listing. They do not overlap. + +`absent` (fixed) findings are counted and can be listed, but are never written +into the output report — surfacing a fixed finding as a live alert would be wrong. + +### Finding identity + +Findings are matched across reports by a **fingerprint**, not by line number, so +moving code around does not invent new findings. Two fingerprints exist: + +| Key | Hashes | Behavior | +|-----|--------|----------| +| `vulnerabilitySourceSinkHash/v1` | rule + source + sink | Survives edits to the call path between source and sink. **Default for baseline matching.** | +| `vulnerabilityWithTraceHash/v1` | rule + every step of every trace | Exact; changes if anything on the path moves. | + +`--fingerprint-key` overrides the identity key on `scan`, `triage`, and +`summary`. The source→sink hash is the default because a decision should survive +refactoring of an unrelated helper the flow happens to pass through. The finer +trace hash is what distinguishes `unchanged` from `updated`. + +Comparing reports built with different fingerprint keys is a hard error, not a +silent zero-match. Findings that carry no fingerprint at all (a report produced +without fingerprints) are reported as-is and counted as "not comparable." + +## Suppression reference + +A suppression is written only by `opentaint triage`, only with a justification, +and only ever as `kind: "external"` (the justification lives outside the source, +not in an in-source comment). The verdict is carried by the SARIF `status`: + +| `triage` flag | `suppression.status` | Meaning | Hidden from gate? | +|---------------|----------------------|---------|-------------------| +| `--accept` | `accepted` | The team will not fix this | Yes | +| `--defer` | `underReview` | The team is not fixing this for now | Yes | +| `--unsuppress` | *(removes the entry)* | Retract a decision | — | + +Both `--accept` and `--defer` hide the finding from the listing and from the +gate. A deferral does **not** expire on its own; the summary's `Deferred` count +keeps it visible so it can be revisited. + +`--unsuppress` removes the suppression from the report being triaged. It does not +"un-inherit": if the baseline still carries the decision, the next scan re-attaches +it. To retract a decision permanently, re-triage the baseline. + +### Reading suppressions conservatively + +When a baseline (or a report from another tool) is read, its suppressions are +interpreted defensively: + +| Status on the entry | Outcome | +|---------------------|---------| +| absent, or `accepted` | Suppressed | +| `underReview` | Suppressed, counted as deferred | +| `rejected` | **Not** suppressed — the suppression was explicitly denied | +| anything unrecognized | **Not** suppressed, counted under "Not honored" | + +A non-accepted or unknown status never hides a finding, and never disappears +silently — the summary surfaces it. + +> **Note on false positives.** SARIF 2.1.0 has no formal false-positive marker, +> and `status: "rejected"` means "the suppression request was rejected" (report +> it), not "this finding is wrong." Record that a finding is a false positive in +> the free-text `--justification`. + +### The summary Suppressions group + +``` +Suppressions +├─ Suppressed: 14 of 90 +├─ Won't fix: 9 (accepted) +├─ Deferred: 5 (under review) +├─ Inherited from baseline: 12 +└─ Added this run: 1 (triage only) +``` + +`opentaint summary --show-findings` hides suppressed findings by default; add +`--suppressed` to list them with their justification. + +## The gate + +| Flag | Meaning | +|------|---------| +| `--error-on-findings` | Enable the gate. Off by default — without it, scans never fail on findings. | +| `--error-on-severity ` | Restrict the gate to these levels: `error`, `warning`, `note`, `none`. Comma-separated or repeated; default is all reported levels. | + +A finding counts toward the gate when it is **not suppressed** and its level is +in scope. With `--baseline`, only **new** findings count (`unchanged` and +`updated` existed before). Findings that cannot be compared (no fingerprint) fail +closed — they count. + +### Exit codes + +| Code | Meaning | +|------|---------| +| `0` | Completed; gate not tripped | +| `2` | Findings remain and `--error-on-findings` was set | +| `1` | General failure (bad input, unreadable report) | +| `252`–`255` | Analyzer failure (exception, OOM, timeout, config error) | + +Exit `2` is deliberately distinct from `1` and from the analyzer codes, so CI can +tell "the scan found new problems" apart from "the scan itself broke." + +## Rule selection (a related scan-time control) + +Rule selection decides which rules the analyzer runs at all. It is **not** +suppression: an excluded rule never loads, so it produces nothing in the report +and there is nothing to review later. To hide a finding a rule *did* produce, +accept it with `triage` instead. + +Configure allow/deny lists in the config file: + +```yaml +rules: + only: # if set, only these rules run + - sql-injection # exact rule name + - java/security/** # glob over the full id + exclude: # these rules never run + - reflected-xss-in-servlet-app +``` + +Or on the command line: + +```bash +opentaint scan --exclude-rule-id java-jwt-decode-without-verify . +``` + +Each entry matches a full `path/to/file.yaml:rule-id`, a bare rule name, or a +doublestar glob over the full id — the same grammar as `summary --rule-id`. +`--rule-id` overrides the config lists; `--exclude-rule-id` overrides +`rules.exclude`. + +Notes: +- A pattern matching no rule produces a warning, so a typo cannot silently look + effective. +- A selection that ends up matching **no** rules is an error, not a silent scan + of nothing — `--dry-run` reports it without compiling. +- Excluding a library rule that a surviving rule joins against keeps working: the + reference still resolves, so removing a rule never quietly breaks another. + +## CI/CD recipes + +The official [GitHub Action](https://github.com/seqra/opentaint/tree/main/github) +and [GitLab template](https://github.com/seqra/opentaint/tree/main/gitlab) wrap +`opentaint scan`. Baseline gating is driven by the CLI directly, as shown below. + +### GitHub Actions + +Persist the default-branch report with `actions/cache`, restore it on pull +requests, and gate on new findings. A cache written on the default branch is +readable from pull-request runs via `restore-keys`, which makes it a simple, +official way to carry the baseline forward. (The first run has no baseline and +scans without gating; every later PR gates against the latest main report.) + +```yaml +name: opentaint +on: + push: + branches: [main] + pull_request: + +jobs: + scan: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - name: Install OpenTaint + run: curl -fsSL https://raw.githubusercontent.com/seqra/opentaint/main/scripts/install/install.sh | sh + + # Restore the most recent main baseline. On main, this key also becomes + # the save target below; on a PR, restore-keys falls back to it read-only. + - name: Restore baseline + uses: actions/cache@v4 + with: + path: baseline.sarif + key: opentaint-baseline-${{ github.run_id }} + restore-keys: opentaint-baseline- + + - name: Scan + run: | + if [ -f baseline.sarif ]; then + opentaint scan --baseline baseline.sarif \ + --error-on-findings --error-on-severity error,warning \ + -o opentaint.sarif . + else + opentaint scan -o opentaint.sarif . + fi + + # On main, the fresh report becomes the next baseline. + - name: Update baseline + if: github.ref == format('refs/heads/{0}', github.event.repository.default_branch) + run: cp opentaint.sarif baseline.sarif + + # Optional: send to GitHub code scanning (suppressions & states are honored). + - name: Upload SARIF + if: always() + uses: github/codeql-action/upload-sarif@v3 + with: + sarif_file: opentaint.sarif +``` + +The `cache@v4` step saves `baseline.sarif` under `opentaint-baseline-` +at job end, so each main run leaves a fresh baseline that the next PR restores +via the `opentaint-baseline-` prefix. + +### GitLab CI + +```yaml +opentaint: + script: + - curl -fsSL https://raw.githubusercontent.com/seqra/opentaint/main/scripts/install/install.sh | sh + - | + if [ -f baseline.sarif ]; then + opentaint scan --baseline baseline.sarif \ + --error-on-findings --error-on-severity error,warning \ + -o gl-opentaint.sarif . + else + opentaint scan -o gl-opentaint.sarif . + fi + artifacts: + when: always + paths: + - gl-opentaint.sarif +``` + +Keep the main-branch `gl-opentaint.sarif` as the `baseline.sarif` for later +pipelines (via the package registry, a cache key, or a committed artifact). + +## SARIF conformance + +Every annotation is a standard SARIF 2.1.0 field, so third-party tools ingest the +report without OpenTaint-specific knowledge: + +- **§3.35 `suppression`** — `kind` (`external`), `status` (`accepted` / + `underReview`), `justification`, `guid`. +- **§3.27.24 `result.baselineState`** — `new` / `unchanged` / `updated` / + `absent`, written under `--write-baseline-state`. +- **§3.14.5 `run.baselineGuid`** — cites the baseline run's + `automationDetails.guid`, so the report is itself citable as a future baseline. + +No property bag or vendor extension is required for any of it. + +## See also + +- [Usage Guide](usage.md) — full command and flag reference (`scan`, `triage`, `summary`). +- [Configuration Guide](configuration.md) — the `rules.only` / `rules.exclude` config keys. diff --git a/docs/configuration.md b/docs/configuration.md index 09381d8be..146a1c956 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -27,6 +27,11 @@ output: # Java runtime settings java: version: 23 + +# Which rules the analyzer runs +rules: + only: [] # if set, only these rules run + exclude: [cookie-missing-httponly] # these rules never run ``` ### Available Options @@ -39,6 +44,45 @@ java: | `output.color` | Color mode: `auto`, `always`, `never` | `auto` | | `output.quiet` | Suppress interactive console output (spinners, progress bars, JAR streaming) | `false` | | `java.version` | Java version for running the analyzer | `23` | +| `rules.only` | Run only the rules matching these patterns | all rules | +| `rules.exclude` | Never run the rules matching these patterns | none | + +### Selecting rules + +`rules.only` and `rules.exclude` control which rules the analyzer loads. They +are rule *selection*, not suppression: an excluded rule never runs, so it +produces nothing in the report and nothing to review later. To hide a finding a +rule did produce, accept it with `opentaint triage` instead. + +Each entry matches a full `path/to/file.yaml:rule-id` exactly, a bare rule name +exactly, or a doublestar glob over the full id — the same grammar as the +summary command's `--rule-id` filter. Globs never match the bare name alone: + +```yaml +rules: + only: + - sql-injection # exact rule name + - java/security/** # every rule under that directory + - java/security/sqli.yaml:* # every rule in that file + exclude: + - cookie-missing-httponly +``` + +A pattern that matches no rule in the active ruleset produces a warning, so a +typo'd exclusion cannot silently look effective. + +`exclude` is applied after `only`. An exclusion-only list is passed to the +analyzer as the excluded rule ids themselves — excluding one rule adds one +argument, not the whole ruleset's complement. A library rule that a selected +rule joins against always keeps working, even if a pattern excluded it, since +dropping it would leave a rule that can never match. A selection that ends up +matching no rules is an error rather than a scan that silently checks +nothing — `--dry-run` reports it without compiling. + +The `--rule-id` flag overrides both lists, and the `--exclude-rule-id` flag +overrides `rules.exclude`, following the usual rule that flags outrank the +configuration file. The two flags compose: `--rule-id` selects, then +`--exclude-rule-id` subtracts. The per-run log file (`~/.opentaint/logs//.log`) always captures full JAR subprocess output regardless of these flags. They control diff --git a/docs/usage.md b/docs/usage.md index 84f9359ed..8b8680c7f 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -82,6 +82,7 @@ Use [CodeChecker](https://github.com/Ericsson/codechecker) for advanced result m | `opentaint compile` | Build project model separately from scanning | | `opentaint project` | Create project model from precompiled JARs/classes | | `opentaint summary` | View SARIF analysis results | +| `opentaint triage` | Compare a report against a baseline and record suppressions | | `opentaint health` | Show resolved paths for the analyzer, autobuilder, rules, and Java runtime | | `opentaint test rule` | Create, run, and debug detection-rule tests | | `opentaint test approximation` | Create and run dataflow-approximation tests | @@ -106,6 +107,22 @@ On the first run, the compiled project model is cached in `~/.opentaint/cache/`. | `--ruleset` | YAML rules file or directory (default: `builtin`) | | `--dry-run` | Validate inputs and show what would run without compiling or scanning | | `--log-file` | Path to the log file (default: `/logs/.log`) | +| `--rule-id` | Run only rules with this ID (repeatable) | +| `--exclude-rule-id` | Never run rules matching this ID: full id, bare name, or glob over the full id — the same matching as summary's `--rule-id` filter (repeatable; overrides `rules.exclude` from the config, composes with `--rule-id`) | + +#### Baseline and gating flags + +| Flag | Description | +|------|-------------| +| `--baseline` | Previous SARIF report to compare against and inherit suppressions from | +| `--write-baseline-state` | Persist `result.baselineState` and `run.baselineGuid` into the output report (needs `--baseline`) | +| `--fingerprint-key` | partialFingerprints key identifying a finding across reports (default `vulnerabilitySourceSinkHash/v1`) | +| `--error-on-findings` | Exit with code 2 when findings remain; with `--baseline`, only new ones count | +| `--error-on-severity` | Restrict `--error-on-findings` to these levels: `error`, `warning`, `note`, `none` (repeatable, default all) | + +With `--baseline`, findings the baseline already accepted stay suppressed and +the summary reports how many are new, unchanged, updated, or fixed. See +[Baselines and suppressions](#baselines-and-suppressions). #### Rule-authoring flags @@ -208,9 +225,85 @@ reflects the full set the tool ran. | `--max-nesting-level` | Collapse code-flow steps deeper than this call-nesting level (`-1` = no cap). Best-effort: depth is derived from step kinds and method names, so flows lacking method info may over-collapse | | `--group-by` | Group the `--show-findings` listing by `severity`, `rule-id`, or `file-path` (default `file-path`) | | `--code-flow` | Render code flows: `all`, a 1-based index, or unset (first flow only). On multi-flow findings the listing also shows a `Code flows: ` field. | +| `--baseline` | Compare against this SARIF report and show new/unchanged/updated/fixed counts. The file is never modified. | +| `--baseline-state` | Show only findings whose state is one of `new` \| `unchanged` \| `updated` \| `absent` (repeatable, needs `--baseline`) | +| `--suppressed` | Include suppressed findings in the listing (hidden by default) | +| `--fingerprint-key` | partialFingerprints key identifying a finding across reports (default `vulnerabilitySourceSinkHash/v1`) | Filters combine as OR within a dimension and AND across dimensions. +### opentaint triage + +Compare a SARIF report against a baseline and record decisions about findings. +Nothing is ever deleted: an accepted or deferred finding stays in the report, +marked with a SARIF suppression recording what was decided and why. + +```bash +# What changed since the last release? Modifies nothing. +opentaint triage scan.sarif --baseline release.sarif + +# We will not fix this one +opentaint triage scan.sarif --accept q3Vf9k --justification "sink is a constant" + +# We are not fixing this one yet +opentaint triage scan.sarif --defer 8bc1d2 --justification "waiting on OT-412" +``` + +| Flag | Description | +|------|-------------| +| `--baseline` | Previous SARIF report to compare against and inherit suppressions from | +| `--write-baseline-state` | Persist `result.baselineState` and `run.baselineGuid` into the output report (needs `--baseline`) | +| `--accept` | Accept the finding with this fingerprint prefix — won't fix (repeatable) | +| `--defer` | Defer the finding with this fingerprint prefix — not fixing for now (repeatable) | +| `--unsuppress` | Remove the suppression from the finding with this fingerprint prefix (repeatable) | +| `--justification` | Why the finding is accepted or deferred (required with `--accept`/`--defer`) | +| `--output`, `-o` | Write the triaged report here (default: rewrite the input in place) | +| `--show-findings` | List the findings, not just the summary | +| `--suppressed` | Include suppressed findings in the listing | +| `--fingerprint-key` | partialFingerprints key identifying a finding across reports (default `vulnerabilitySourceSinkHash/v1`) | +| `--error-on-findings` | Exit with code 2 when findings remain; with `--baseline`, only new ones count | +| `--error-on-severity` | Restrict `--error-on-findings` to these levels (repeatable, default all) | + +A finding is named by a fingerprint prefix, git-style — the value shown as +`Fingerprint:` by `opentaint summary --show-findings`. An ambiguous or unknown +prefix is an error, never a guess. + +Exit codes: + +| Code | Meaning | +|------|---------| +| 0 | Triage completed | +| 1 | General failure (bad input, unreadable report) | +| 2 | Findings remain and `--error-on-findings` was set | + +## Baselines and suppressions + +A baseline is just a SARIF report you kept. Two independent axes are built on it: +`--baseline` answers *"is this new?"* (baseline state), and `opentaint triage` +answers *"did a human accept this?"* (suppression). Presence in a baseline is +**not** acceptance — an un-triaged baseline entry only makes a finding +`unchanged`, it does not hide it. + +```bash +# 1. Scan once; keep the report as the baseline. +opentaint scan -o baselines/main.sarif . + +# 2. Record decisions you've reviewed (writes SARIF suppressions). +opentaint triage baselines/main.sarif --accept q3Vf9k --justification "input is admin-only" + +# 3. In CI, gate on new, non-suppressed findings only. +opentaint scan --baseline baselines/main.sarif --error-on-findings --error-on-severity error,warning . +``` + +Decisions travel forward: a finding that matches a suppressed baseline entry +inherits the decision verbatim, so it's authored once and re-applied by every +later scan until the code is fixed and the finding retires. The gate exits `2` +when it trips — distinct from `1` (tool error) and `252`–`255` (analyzer). + +For the full model, the baseline-state and suppression-status reference, finding +identity, rule selection, and copy-paste GitHub Actions / GitLab recipes, see the +dedicated guide: **[Baselines, suppressions, and CI gating](baselines-and-suppressions.md)**. + ### opentaint project Create project models from precompiled JARs or classes when source code isn't available.