Refuse unknown filter values on pipeline security findings

What does this MR do and why?

Pipeline.securityReportFindings takes its severity and reportType arguments as [String!] and hands them to Security::FindingsFinder unchecked, so the caller is never told that a value is one the finder does not know:

  • An unknown severity answers 500. The finder reads it with Security::Finding.severities.fetch_values(*params[:severity]), which raises KeyError for a key the enum does not have. The caller gets Internal server error, naming neither the argument nor the value. The lookup is case sensitive, so CRITICAL, the spelling the finding's own severity field answers with, is one of the values that fail.
  • An unknown report type is dropped and can empty the filter. The finder merges Security::Scan.by_scan_types(params[:report_type]), and Security::Scan.sanitize_scan_types keeps only the scan types it has (scan_types.keys & given_types). A filter made only of other values matches no scan, so the answer is an empty list that reads as a pipeline whose scans found nothing. generic and container_scanning_for_registry, which Project.vulnerabilities takes as GENERIC and CONTAINER_SCANNING_FOR_REGISTRY, a misspelling, and an uppercase SAST all read that way.

This MR validates both arguments in the resolver before the finder runs, against the same enums the finder reads (Security::Finding.severities and Security::Scan.scan_types), and answers an unknown value with a Gitlab::Graphql::Errors::ArgumentError that names the argument, the value and the values it takes:

Argument 'severity' has an invalid value ("CRITICAL"). Valid values are: info, unknown, low, medium, high, critical.

The argument descriptions now list the accepted values, read from the same enums, and the GraphQL reference and the introspection results under public/-/graphql are regenerated.

I put the check in the resolver rather than in the finder because the finder has a second caller. The REST route GET /projects/:id/vulnerability_findings (ee/lib/api/vulnerability_findings.rb) defaults report_type to every key of Vulnerabilities::Finding.report_types, generic and container_scanning_for_registry included, and relies on sanitize_scan_types to drop the two Security::Scan has no scan type for, so a check in the finder would refuse that route's own default. That route already refuses an unknown severity, through Grape's values:, but a caller who asks there for generic or container_scanning_for_registry alone still gets the same silently emptied filter. Narrowing its values: and its default to Security::Scan.scan_types would close that. I left it out to keep this MR to the GraphQL field, and can add it here or in a follow-up.

Two choices I would like your view on:

  • I kept the argument types as strings. Typing them with VulnerabilitySeverity and an enum of scan types is what #355840 asks for, but the deprecation process counts a changed argument type as a breaking change: every client that declares the variables as [String!], the pipeline security tab's own pipelineFindings query among them, would be refused. The enum would also need a type of its own for the report types, since VulnerabilityReportType has values Security::Scan has no scan type for. Validating the strings fixes both failures now and leaves that migration to #355840.
  • A list that mixes a known and an unknown report type is refused too. On master reportType: ["sast", "generic"] answers the SAST findings and says nothing about generic; with this MR it answers the argument error. I chose that because a partly applied filter misleads the same way, only less visibly, and it matches what the REST route that uses the same finder does for an unknown severity, which it refuses with 400 (ee/lib/api/vulnerability_findings.rb declares values: for severity). The pipeline security tab is not affected: it lowercases its filters before it sends them (pipeline_vulnerability_report.vue, normalizeForGraphQLQuery) and its report type token offers only scan types. Neither is the Duo Agent Platform's list_security_findings tool, which checks both filters against the same severities and scan types and lowercases them before it sends them (duo_workflow_service/tools/findings/list_security_findings.py in the AI Gateway). If you would rather keep mixed lists working, I can limit the refusal to a filter that would be left empty.

The values stay case sensitive, as they are today. Accepting CRITICAL as well would be friendlier, but it widens what the field accepts, so I left it out of a fix.

Where the code is, on master at 07c80d80
  • ee/app/graphql/resolvers/pipeline_security_report_findings_resolver.rb:9-15: the report_type and severity arguments, typed [GraphQL::Types::String], passed to the finder in resolve (line 30).
  • ee/app/finders/security/findings_finder.rb:174-180: severities, which calls Security::Finding.severities.fetch_values(*params[:severity]).
  • ee/app/finders/security/findings_finder.rb:140-144: by_report_types, which merges Security::Scan.by_scan_types(params[:report_type]).
  • ee/app/models/security/scan.rb:22-32 and :68-70: the scan_type enum and sanitize_scan_types.
  • ee/app/assets/javascripts/security_dashboard/components/pipeline/pipeline_vulnerability_report.vue:67-79: the frontend's comment that these two filters "need to be lower case", and the lowercasing.

References

Related to #355840, which proposes typing both arguments. This MR does not close it.

I searched the issues and merge requests of this project for the field, the resolver, the finder and sanitize_scan_types and found nothing else about these two failures. !246038 (merged) fixed another 500 in the same field, for an empty cursor.

#355840 is labeled for the Vulnerability Management group (Security Factory stage), and CODEOWNERS gives the finder and the models to Security Insights backend. Harrison Peters, who fixed the empty cursor 500 in this field in !246038 (merged), and Subashis Chakraborty, both in Security Insights backend, seem the right reviewers.

Where this comes from

I maintain gitlab-mcp-server, an MCP server that exposes the GitLab API to AI assistants. Its tool that lists a pipeline's security findings checks both filters itself before it sends anything, because GitLab answered an unknown severity with a 500 and an unknown report type with an empty list, and a model reads the second as a clean pipeline. The project keeps a record of what it finds in its dependencies in upstream-bugs.md (the severity entry and the report type entry), and this MR is the fix both entries call for.

Screenshots or screen recordings

The GraphQL response, for a pipeline with SAST findings:

Query Before After
securityReportFindings(severity: ["CRITICAL"]) 500, Internal server error 200, the argument error naming "CRITICAL", securityReportFindings: null
securityReportFindings(reportType: ["generic"]) 200, nodes: [] 200, the argument error naming "generic", securityReportFindings: null
securityReportFindings(reportType: ["sast", "generic"]) 200, the SAST findings 200, the argument error naming "generic", securityReportFindings: null
securityReportFindings(severity: ["critical"], reportType: ["sast"]) 200, the critical SAST findings unchanged

How to set up and validate locally

  1. In a project with Ultimate, run a pipeline with a SAST job (for example with the Jobs/SAST.gitlab-ci.yml template) and wait until its Security tab lists findings.
  2. In GraphiQL at http://gdk.test:3000/-/graphql-explorer, run:
    query {
      project(fullPath: "<project_path>") {
        pipeline(iid: "<pipeline_iid>") {
          securityReportFindings(severity: ["CRITICAL"]) {
            nodes { uuid severity reportType }
          }
        }
      }
    }
    On master this answers Internal server error. On this branch it answers the argument error listing the six severities.
  3. Replace the filter with reportType: ["generic"]. On master this answers an empty nodes list. On this branch it answers the argument error listing the scan types.
  4. Replace the filter with severity: ["critical", "high"], reportType: ["sast"]. Both branches answer the matching findings.
  5. Open the pipeline's Security tab and filter by severity and by report type. It behaves as on master.
  6. Run the specs:
    bin/rspec ee/spec/graphql/resolvers/pipeline_security_report_findings_resolver_spec.rb \
      ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb
Specs and checks I ran
  • New examples in ee/spec/graphql/resolvers/pipeline_security_report_findings_resolver_spec.rb: an unknown severity (%w[low CRITICAL]) and unknown report types (%w[sast generic sasst]) each raise the argument error with the expected message, and neither runs the finder.
  • New examples in ee/spec/requests/api/graphql/project/pipeline/security_report_findings_spec.rb: every severity with the report types the pipeline has still returns all 25 findings, and severity: ["CRITICAL"] and reportType: ["generic"] each answer 200 with the argument error and securityReportFindings: null.
  • Against master without the change, six of the seven new examples fail: the resolver examples get [] and see the finder run, the severity request gets 500 with key not found: "CRITICAL", and the report type request gets no error. The seventh, which sends every valid value, passes on both.
  • With the change, the two spec files: 22 examples, 0 failures. The type specs that query the field (pipeline_security_report_finding_type_spec.rb, vulnerability_evidence_type_spec.rb, vulnerability_location/coverage_fuzzing_type_spec.rb, vulnerability_request_type_spec.rb, asset_type_spec.rb): 89 examples, 0 failures, 1 pending, the existing xit for the dismissal N+1 example.
  • bundle exec rake gitlab:graphql:compile_docs regenerated doc/api/graphql/reference/_index.md (the two argument rows only), and bundle exec rake gitlab:graphql:generate_all_introspection_schemas regenerated public/-/graphql/introspection_result.json and public/-/graphql/introspection_result_no_deprecated.json (the two argument descriptions only); bundle exec rake gitlab:graphql:check_introspection_sync passes. RuboCop on the three changed Ruby files: no offenses. scripts/lint/commit_linter.rb: no problems. markdownlint-cli2 and Vale (the lint-markdown image) on the regenerated reference: no errors, and the one Vale warning is at line 23, which this MR does not touch.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by José M. Requena Plens

Merge request reports

Loading
Loading