Derive dependency malware status from malware advisories

Bala Kumarrequested to merge
feat/malware-status-from-advisories into master

What does this MR do and why?

Closes #623539 (closed).

Sbom::Occurrence#malware_status asked whether any of the occurrence's vulnerabilities carried a GLAM- identifier. That makes malware a property of a project's scan history rather than of the package, and it has three consequences.

Group aggregations were wrong. Sbom::AggregationsFinder collapses a component version to one MIN(id) representative occurrence. A GLAM finding recorded against any of that component's other occurrences in the group was invisible, so a package could be malware in one project and report false on the group list.

Nothing could be flagged before a scan ran. A known-malicious package sat unflagged until a security scan produced a vulnerability record for it.

Withdrawn advisories never cleared. The GLAM- vulnerability record is created once, at scan time, from the advisory that was active then. Nothing deletes or re-evaluates that record when the upstream advisory is later retracted, so has_glam_identifier? keeps returning true and the dependency list keeps reporting the package as malware indefinitely. There is no retraction path at all.

This asks the advisories instead. Malware becomes a function of (purl_type, name, version), so every occurrence of a version gets the same answer and the aggregation problem disappears — no cross-occurrence reconciliation, no representative to work around.

How

Sbom::MalwareAdvisoriesPreloader resolves a page in one query, and a larger batch in one query per 1,000 distinct packages. PackageMetadata::MalwareAffectedPackage.for_occurrences already accepts an array and builds a VALUES CTE joined on (purl_type, name), so the batching needs no new query plumbing — the existing MalwareAdvisoriesFinder passes a single-element array only because the Dependency Firewall checks one package at a time. Version ranges are matched in Ruby with the same Gitlab::VulnerabilityScanning::AdvisoryUtils matcher that finder uses, including its treatment of an unparseable range as a match — but not a missing version, which the preloader treats differently from the finder; see Semantics change below.

Sbom::MalwareAdvisoriesPreloader::BATCH_SIZE = 1_000 slices the component list before it reaches the VALUES CTE, so the CTE can't grow unbounded. Dependency list pages sit far below that limit and still cost one query; the constant exists because Sbom::Exporters::DependencyListService renders through DependencyListEntity and hands the preloader a whole export rather than a page.

Occurrences whose purl_type is one of Enums::Sbom::CONTAINER_SCANNING_PURL_TYPES (apk, deb, rpm, cbl-mariner, wolfi, and the rest) are skipped. Gitlab::VulnerabilityScanning::AdvisoryUtils#build_matcher returns a container-scanning matcher for those types whose affected? takes (distro, source, version); an SBOM occurrence has no distro or source to supply, so calling it with a bare version would raise ArgumentError. Malware advisories describe registry packages anyway, so excluding them is also correct on the merits.

Sbom::Occurrence keeps a malware_status writer for the batched result and falls back to a single lookup when nothing preloaded it. Every consumer is therefore correct whether or not it batches; batching is purely about query count.

The preloader also preloads component and component_version for any occurrence that lacks them, since purl_type, name and version delegate through those. Callers that already preloaded pay nothing.

Withdrawn advisories

This is a behaviour fix, not just a rename: the withdrawn-advisory exclusion now actually reaches the dependency list, which it never did before, because the list wasn't reading the advisory table at all under the vulnerability-derived approach.

PackageMetadata::MalwareAffectedPackage.not_withdrawn (which merges PackageMetadata::MalwareAdvisory.not_withdrawn) and its use in with_advisory already exist on master. This MR renames the scope to active and calls it from the new preloader (see the scope note below for what else that rename touches). Pure rename, no behaviour change to the scope itself — the behaviour change is that the dependency list now consults it at all.

Scope note: the rename reaches beyond this MR

  • PackageMetadata::MalwareAffectedPackage.not_withdrawn is renamed to active. The rename is not confined to the new code path: the scope's only other caller is with_advisory, which is used by the Dependency Firewall (Security::DependencyFirewall::FetchPackageVulnerabilitiesService and PackageMetadata::MalwareAdvisoriesFinder) and by the CI ingestion path (Gitlab::VulnerabilityScanning::SecurityReportBuilder).
  • This is a pure rename: the scope body is unchanged, the emitted SQL is identical, and no behaviour on those paths changes.
  • Under a strict minimal-change reading this belongs in a separate MR, since it touches callers unrelated to deriving malware status. It's included here because it was requested during review, and because splitting a pure rename across two MRs leaves the codebase with two names for one concept in the interim.
  • Reviewers who'd rather see it split should say so; it will be extracted.

Surfaces

Surface Batching
Project and group dependency list JSON DependencyListEntity, once per rendered page
GET /api/v4/projects/:id/dependencies after paginate, once per page
GraphQL Dependency BatchLoader::GraphQL, once per rendered page
Elasticsearch occurrence_ref index inside the existing preload_indexing_data(refs) hook

A new concern, DependencyMalwareGating (ee/app/serializers/concerns/dependency_malware_gating.rb), provides render_malware_field?, which requires both read_security_resource on the project or group and dependency_malware_detection_feature_flag_enabled?. DependencyEntity (decides whether to render the field) and DependencyListEntity (decides whether to run the batch lookup) both include it, so the two can't drift — previously the list entity could pay for a lookup whose field the item entity then omitted. GET /api/v4/projects/:id/dependencies mirrors the same pair of conditions inline before preloading, matching what API::Entities::Dependency checks.

The malware field description on DependencyEntity and API::Entities::Dependency now says the value is true when the package version matches a malware advisory, rather than when a GLAM identifier is present.

What this removes

Malware is now package-derived, so the group-level workaround merged in !251901 (merged) to cope with the MIN(id) representative is unnecessary. This MR removes it:

  • ee/app/models/sbom/malware_status_preloader.rb (Sbom::MalwareStatusPreloader) and its spec
  • Groups::DependenciesController#preload_malware_status!
  • Sbom::Occurrence.vulnerability_ids_by_component_version and its spec
  • Vulnerability.with_vulnerability_read (its only caller was the deleted preloader)
  • Vulnerability.malware_status_for

All of these existed only to reconcile GLAM findings across the occurrences that AggregationsFinder's MIN(id) representative hides; a package-derived answer has nothing to reconcile.

!251903 (GraphQL group aggregation) is built on top of the preloader this MR deletes, and is still an open Draft. It owns the DependencyAggregationType feature-flag-actor change and the context[:sbom_dependency_group] resolver stamp — this MR carries neither. !251903 will need rework on top of this MR, whichever order the two land in.

Feature flag

Unchanged. dependency_malware_detection, beta, off by default, resolved through dependency_malware_detection_feature_flag_enabled? on Group and Project so it inherits down the namespace hierarchy.

Semantics change

This is the part worth reviewing carefully. malware: true now means the package version matches a malware advisory, not that a scan produced a vulnerability record.

  • A dependency can report true with no corresponding vulnerability in the vulnerability report, if no scan has run.
  • Vulnerability lifecycle no longer applies. Dismissal and resolution have no effect on the field. Note the previous implementation did not honour them either — malware_status_for was any?(&:has_glam_identifier?) with no state filter — so nothing regresses, but the option to make it state-aware goes away.
  • The vulnerability report still uses has_glam_identifier?, so the two surfaces can disagree for a package with no scan. #623539 (closed) records this as an accepted consequence.
  • The advanced-search malware filter can now lag the rendered field in a way it didn't before — see Known risk: stale Elasticsearch malware filter below.
  • A missing version no longer counts as a match. Sbom::MalwareAdvisoriesPreloader#version_affected? still treats an unparseable range as a match, mirroring PackageMetadata::MalwareAdvisoriesFinder, but it no longer extends that to a missing version — an occurrence with no component_version used to match every non-withdrawn advisory for that package name. The finder's behaviour is right for the Dependency Firewall, which is making a blocking decision where "some version of this package is malicious" is reason enough to refuse it; the dependency list instead makes a claim about one specific package version, so with no version there is nothing to test the range against and the row is left unflagged. Known blind spot: an advisory whose range covers every version (for example >=0.0.0) will not flag an occurrence whose version is unknown. Raised in review by GitLab Duo.

Known risk: stale Elasticsearch malware filter

  • The rendered malware field is computed live from the advisory table on every request. The Elasticsearch malware boolean is written once, at index time.
  • Under the old vulnerability-derived behaviour these stayed roughly in step, because a GLAM vulnerability was created by a security scan and that same scan writes the occurrence, which triggers reindexing.
  • That coupling is gone now. An advisory arrives from the package metadata sync with no corresponding occurrence write, so nothing triggers reindexing. The indexed value can stay wrong indefinitely — until something unrelated happens to touch that occurrence.
  • The consequence is user-visible and specific: on group dependency lists backed by advanced search, the malware filter reads the stale indexed value while the malware badge on the very same page is computed live. Filtering by malware can therefore hide a package the page would flag, or list one it would not.
  • This is a pre-existing issue class, tracked in #623538, but this MR makes it materially worse. 623538 was written against the milder vulnerability-derived version of the problem and should be re-scoped accordingly.
  • Blast radius is currently limited because the feature is behind dependency_malware_detection (beta, off by default). This should be resolved before the flag is enabled broadly.

Database review

This MR adds exactly one new query shape. It runs once per rendered page of the dependency list, replacing the vulnerability and vulnerability_read preloads the old GLAM-derived implementation needed. There are no migrations and no writes.

WITH occurrences_cte(purl_type, name) AS (VALUES (6, 'chalk'), (6, 'debug'), (8, 'requests'))
SELECT "pm_malware_affected_packages".*
FROM "pm_malware_affected_packages"
INNER JOIN occurrences_cte
  ON occurrences_cte.purl_type = pm_malware_affected_packages.purl_type
 AND occurrences_cte.name = pm_malware_affected_packages.package_name
INNER JOIN "pm_malware_advisories"
  ON "pm_malware_advisories"."id" = "pm_malware_affected_packages"."pm_malware_advisory_id"
WHERE "pm_malware_advisories"."withdrawn_date" IS NULL

The VALUES list has one tuple per distinct (purl_type, package_name) on the page: at most 20 for a group aggregation page (Sbom::AggregationsFinder::MAX_PAGE_SIZE), and at most 1,000 per statement (Sbom::MalwareAdvisoriesPreloader::BATCH_SIZE) for the one caller that can exceed a page, Sbom::Exporters::DependencyListService.

Production data shape

Measured on a gitlab-production-sec Database Lab clone:

  • pm_malware_affected_packages: 237,620 rows, 22 MB table, 44 MB indexes.
  • pm_malware_advisories: 237,620 rows, 231 MB table, 21 MB indexes.
  • 358 advisories are withdrawn, 0.15%. So withdrawn_date IS NULL is a correctness filter, not a selective one — it cannot be used to reduce work and does not warrant an index.
  • (purl_type, package_name) is 1:1 in current data: per purl_type the row count equals the distinct package-name count exactly (npm 221,455/221,455; pypi 11,722/11,722; gem 3,627/3,627; nuget 777/777; cargo 19; golang 18; maven 2). So a page of N packages returns at most N rows — there is no join fan-out. This is an observed property of the data, not enforced by a constraint: the unique index is on (pm_malware_advisory_id, purl_type, package_name), not on (purl_type, package_name) alone.
  • No container-scanning purl types (apk, rpm, deb, cbl-mariner, wolfi) exist in the data at all, so the container-type skip in the preloader is defensive and currently a no-op.

Query plans

Ordered by cost, ascending. We're reading buffers rather than timings, since timings vary run to run on a clone.

Case Tuples Rows returned Buffers Link
Un-preloaded fallback (Sbom::Occurrence#malware_status, nothing batched) 1 1 11 (4 hit + 7 read, ~88 KiB) https://console.postgres.ai/gitlab/projects/gitlab-production-sec/sessions/56669/commands/161002
Typical page, 20 real component names that match nothing 20 0 63 (42 hit + 21 read, ~504 KiB) https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/56669/commands/161007
Worst realistic page, 20 real package names that all match 20 20 163 (95 hit + 68 read, ~1.27 MiB) https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/56669/commands/161004
BATCH_SIZE ceiling, 1,000 tuples that all match 1,000 1,000 8,016 (7,924 hit + 92 read, ~62.6 MiB) https://postgres.ai/console/gitlab/gitlab-production-sec/sessions/56669/commands/161009

What the plans show

  • Both joins are index scans in every plan. Index Cond uses i_pm_malware_affected_packages_on_purl_type_and_package_name on (purl_type, package_name), which is exactly the pair the CTE joins on, with Index Searches equal to the tuple count — one probe per package. The advisory join uses pm_malware_advisories_pkey. No sequential scan appears at any point.
  • The VALUES literals are typed integer while purl_type is smallint. This was worth checking because that cast can suppress index use; it does not — the plans confirm the composite index is chosen.
  • withdrawn_date IS NULL is applied as a filter after the primary-key lookup, only on rows that already matched a package name, never across the table. On the typical page the advisory index scan is reported as never executed, because nothing matched to join to.
  • Cost is linear in page size and flat in table size: about 8 buffers per matching package (163/20 and 8,016/1,000 agree) and about 3 per non-matching one (63/20). The 100-buffer difference between the all-match and no-match 20-tuple pages is the primary-key lookup into the 231 MB advisories table, roughly 5 buffers per match.
  • Planner estimates are off: the top node estimates rows=1 against 20 and 1,000 actual, because the planner cannot know how many of the VALUES tuples will match. Benign here, since the result is bounded by the input tuple count and the query is standalone rather than feeding a larger plan, but flagging it explicitly rather than leaving a reviewer to find it.

Caveats, stated plainly

  • The 1,000-tuple plan sources its names from pm_malware_affected_packages through a MATERIALIZED CTE rather than as literals, because a 1,000-literal statement was impractical to run through the console. The join shape and volume are the same; the plan carries a small extra source scan production would not have, which is negligible against 8,016 buffers.
  • That 1,000-tuple case also assumes every package matches, which never happens in practice. A realistic export batch looks like the no-match page scaled up, roughly 3,000 buffers per 1,000 packages. The 8,016 figure is the ceiling, not the expectation.
  • Both tables are on the sec database, the same as sbom_occurrences, so there is no cross-database access.
  • The not_withdrawn to active rename does not change any SQL. Its only other caller, with_advisory, keeps an identical body, so the Dependency Firewall and CI ingestion paths are unaffected.

How to set up and validate locally

Local testing steps

  1. Seed an advisory matching something in your SBOM:

    project    = Project.find_by_full_path('your-group/your-project')
occurrence = project.sbom_occurrences.first

advisory = create(:pm_malware_advisory)
create(:pm_malware_affected_package,
  malware_advisory: advisory,
  purl_type: occurrence.purl_type,
  package_name: occurrence.name,
  affected_range: "=#{occurrence.version}")

  2. Feature.enable(:dependency_malware_detection, project.root_ancestor)

  3. Check each surface:

    curl --header "PRIVATE-TOKEN: <token>" \
  "http://gdk.test:3000/api/v4/projects/<id>/dependencies" | jq '[.[] | {name, malware}]'

    and the group list, which is the case that was broken before:

    curl --header "PRIVATE-TOKEN: <token>" \
  "http://gdk.test:3000/groups/<group-path>/-/dependencies.json" \
  | jq '[.dependencies[] | {name, malware}]'

Before: !251901 (merged) already fixed the MIN(id) representative gap on master, so the group list reports true when a GLAM vulnerability exists anywhere in the group for that component. Nothing is flagged when no scan has run at all.

After: both report true for the seeded package, with no scan required.

  1. Withdraw the advisory and re-request:

    advisory.update!(withdrawn_date: Date.current)

    Both surfaces return to false.

Advisory matches version Withdrawn Flag enabled malware
yes no yes true
yes yes yes false
no — yes false
yes no no omitted / null

Local verification

Everything below was run on a GDK synced with the real GLAM advisory data (227,326 advisories), not seeded fixtures, and driven through the real routing, controller and serializer stack as a logged-in user.

  • Both dependency list pages were exercised end to end and return HTTP 200: the group list at /groups/bala-test-group/-/dependencies.json and the project list at /bala-test-group/malware-sbom-verification/-/dependencies.json.
  • The full group list was swept across every page: 28 unique rows, exactly 6 flagged — chalk 5.6.1, debug 4.4.2, base65-85x 5.0.1, db-convertor 1.2.0, polymarket-risk-manager 3.5.2, vitest-agent 0.3.1. chalk 5.6.1 and debug 4.4.2 are the real September 2025 npm compromise versions.
  • Every row on the group list was then cross-checked against a direct advisory lookup performed independently of the preloader (resolving the purl type and canonical name, applying the active scope, and running the version matcher directly). Every row agreed — zero mismatches, so no false positives and no false negatives.
  • Version precision: chalk 5.6.1 reports true while chalk 5.3.0 reports false, against an advisory whose range is =5.6.1. Same package, different version.
  • The project list flags 6 of 11; the group aggregation flags the same 6 across its pages.
  • With the feature flag off, the malware key is absent from every row on both pages; with it on, the group page flags 3 on the first page of 20 and the project page flags 6.
  • Withdrawing the matching advisory flipped the field to false; restoring it flipped it back.
  • REST and GraphQL agree exactly.
  • Query counts, measured per surface at two page sizes: the advisory lookup stays at exactly one query in all cases — project dependency list (3 vs 11 dependencies, 8 total queries either way), group aggregation (5 vs 20 rows, 4 total either way), GraphQL Dependency BatchLoader (3 vs 11 nodes, 4 total either way), the exporter, and the preloader alone measured from 1 to 70 occurrences.
  • The exporter's total query count does still grow by roughly one per occurrence, but that's sbom_graph_paths from has_dependency_paths? — the pre-existing N+1 tracked in #624732, not the advisory lookup.
  • ee/app/serializers/concerns/ is a new directory: a GDK that was already running needs gdk restart rails-web before the dependency list loads; a fresh boot registers it as an autoload root normally.
  • The HTML shell of both pages could not be rendered on this GDK: its asset pipeline is configured with webpack.enabled: false while the rspack dev server is running and serves manifest.rspack.json, so every HTML page returns a Gitlab::Webpack::Manifest::ManifestLoadError regardless of this branch. The same failure appears on the HTML-format request specs. It predates this MR and is unrelated to it; the JSON the page consumes, and every layer beneath it, is verified above.

Testing

  • ee/spec/models/sbom/malware_advisories_preloader_spec.rb (new): 11 examples covering a matching advisory, a non-matching package, a version outside the affected range, a versionless occurrence (regression coverage for the missing-version semantics change above), a withdrawn advisory (regression coverage for the withdrawn-advisory bug above), a container-scanning purl type, batch slicing, the no-dependencies short circuit, an already-preloaded read, an N+1 guard, and the un-preloaded fallback on Sbom::Occurrence#malware_status.
  • ee/spec/requests/groups/dependencies_controller_spec.rb: the malware section is rewritten around advisories, including a withdrawn-advisory context that is regression coverage for the same bug. The project_ids case used to assert that a per-occurrence row must not inherit a sibling project's finding; it now asserts the opposite — the filtered list reports the same status as the aggregated list, because malware is a property of the package version, not of which project scanned it.
  • ee/spec/requests/api/dependencies_spec.rb and ee/spec/requests/api/graphql/project/sbom/dependencies_spec.rb each gained a withdrawn-advisory case — regression coverage for the same bug — collapsed into an RSpec::Parameterized::TableSyntax table in both files, per a Duo review comment preferring table-based tests over contexts that differ only in their let values.
  • ee/spec/models/concerns/vulnerabilities/malware_detection_spec.rb loses its .malware_status_for block; ee/spec/models/sbom/occurrence_spec.rb loses its .vulnerability_ids_by_component_version block; ee/spec/models/sbom/malware_status_preloader_spec.rb is deleted along with the class it covered.
  • The existing specs verify the values the malware field returns. None of them would have failed if the batching were removed, because correctness doesn't depend on it — only query count does. Three surfaces had no assertion that would notice:
    • ee/spec/requests/api/graphql/project/sbom/dependencies_spec.rb — new test on the BatchLoader::GraphQL path on the malware field.
    • ee/spec/services/sbom/exporters/dependency_list_service_spec.rb — new test on the exporter, which renders through DependencyListEntity.
    • ee/spec/lib/search/elastic/references/sbom/occurrence_ref_spec.rb — new test on the preload_indexing_data batch.
  • Each new test asserts exactly one query against pm_malware_affected_packages, and each asserts the field actually rendered first, so the count can't pass vacuously. Each was verified by disabling the corresponding mechanism and confirming the test fails: removing the GraphQL BatchLoader produced 5 queries for 5 nodes, removing the DependencyListEntity page preload produced 5 for 5 occurrences, and removing the Elasticsearch batch preload produced 3 for 3 references.
  • The exporter already had a generic N+1 test, but its with_threshold(3) allowance absorbs a per-occurrence advisory query when only one occurrence is added, so it couldn't have caught this.
  • Local runs so far: 191 examples, 0 failures, across the model and serializer specs. RuboCop clean.

Related

Edited by Bala Kumar

Merge request reports