Skip to content

fix(keyvalue): skip nil results in CertifyBad and CertifyGood full-scan queries#3117

Open
infernus01 wants to merge 1 commit into
guacsec:mainfrom
infernus01:fix/keyvalue-certifybad-nil-results
Open

fix(keyvalue): skip nil results in CertifyBad and CertifyGood full-scan queries#3117
infernus01 wants to merge 1 commit into
guacsec:mainfrom
infernus01:fix/keyvalue-certifybad-nil-results

Conversation

@infernus01

Copy link
Copy Markdown

Description of the PR

  • Fix nil elements being appended to query results in the keyvalue backend's CertifyBad() and CertifyGood() functions
  • The bug is in the full-scan path (the else branch) — when no artifact or source filter matches, the code scans all entries and appends results from CBIfMatch/CGIfMatch without checking for nil
  • The foundOne path (the if branch) already has the nil guard — this PR adds the same guard to the else branch in both files

Problem

When you query CertifyBad or CertifyGood with a package filter (not artifact or source), the backend can't do a targeted lookup — the comment in the code says "Cant really search for an exact Pkg, as these can be linked to either names or versions, and version could be empty." So it falls through to a full scan of all entries.

During that scan, CBIfMatch/CGIfMatch returns (nil, nil) for entries that don't match the filter. But the code appends these nil values to the output slice anyway. The GraphQL layer then tries to serialize an array containing nil elements, which violates the schema ([CertifyBad!]! — the ! inside the brackets means elements must be non-null), and the client gets:

the requested element is null which the schema does not allow

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If GraphQL schema is changed, GraphQL client updates/additions have been made
  • If OpenAPI spec is changed, make generate has been run
  • If ent schema is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

@gaganhr94

Copy link
Copy Markdown
Contributor

Thanks for the contribution @infernus01. These changes look good and can be merged.

…an queries

Signed-off-by: infernus01 <sbmbhardwaj01@gmail.com>
@infernus01
infernus01 force-pushed the fix/keyvalue-certifybad-nil-results branch from f601595 to 60d19e2 Compare July 18, 2026 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants