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 raisesKeyErrorfor a key the enum does not have. The caller getsInternal server error, naming neither the argument nor the value. The lookup is case sensitive, soCRITICAL, the spelling the finding's ownseverityfield 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]), andSecurity::Scan.sanitize_scan_typeskeeps 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.genericandcontainer_scanning_for_registry, whichProject.vulnerabilitiestakes asGENERICandCONTAINER_SCANNING_FOR_REGISTRY, a misspelling, and an uppercaseSASTall 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
VulnerabilitySeverityand 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 ownpipelineFindingsquery among them, would be refused. The enum would also need a type of its own for the report types, sinceVulnerabilityReportTypehas valuesSecurity::Scanhas 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 aboutgeneric; 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 with400(ee/lib/api/vulnerability_findings.rbdeclaresvalues:forseverity). 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'slist_security_findingstool, 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.pyin 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: thereport_typeandseverityarguments, typed[GraphQL::Types::String], passed to the finder inresolve(line 30).ee/app/finders/security/findings_finder.rb:174-180:severities, which callsSecurity::Finding.severities.fetch_values(*params[:severity]).ee/app/finders/security/findings_finder.rb:140-144:by_report_types, which mergesSecurity::Scan.by_scan_types(params[:report_type]).ee/app/models/security/scan.rb:22-32and:68-70: thescan_typeenum andsanitize_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
- In a project with Ultimate, run a pipeline with a SAST job (for example with the
Jobs/SAST.gitlab-ci.ymltemplate) and wait until its Security tab lists findings. - In GraphiQL at
http://gdk.test:3000/-/graphql-explorer, run:On master this answersquery { project(fullPath: "<project_path>") { pipeline(iid: "<pipeline_iid>") { securityReportFindings(severity: ["CRITICAL"]) { nodes { uuid severity reportType } } } } }Internal server error. On this branch it answers the argument error listing the six severities. - Replace the filter with
reportType: ["generic"]. On master this answers an emptynodeslist. On this branch it answers the argument error listing the scan types. - Replace the filter with
severity: ["critical", "high"], reportType: ["sast"]. Both branches answer the matching findings. - Open the pipeline's Security tab and filter by severity and by report type. It behaves as on master.
- 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, andseverity: ["CRITICAL"]andreportType: ["generic"]each answer200with the argument error andsecurityReportFindings: 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 gets500withkey 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 existingxitfor the dismissal N+1 example. bundle exec rake gitlab:graphql:compile_docsregenerateddoc/api/graphql/reference/_index.md(the two argument rows only), andbundle exec rake gitlab:graphql:generate_all_introspection_schemasregeneratedpublic/-/graphql/introspection_result.jsonandpublic/-/graphql/introspection_result_no_deprecated.json(the two argument descriptions only);bundle exec rake gitlab:graphql:check_introspection_syncpasses. RuboCop on the three changed Ruby files: no offenses.scripts/lint/commit_linter.rb: no problems. markdownlint-cli2 and Vale (thelint-markdownimage) 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.