From 344e785253a359778b52d8f029793674c3fff79d Mon Sep 17 00:00:00 2001 From: Joey L Date: Wed, 5 Aug 2026 06:57:25 +0000 Subject: [PATCH 1/7] repo-based consider all branches --- go/cmd/worker/main.go | 1 + go/internal/database/datastore/models.go | 7 + go/internal/database/datastore/repo_cab.go | 156 +++++++++++++++ .../database/datastore/repo_cab_test.go | 186 ++++++++++++++++++ go/internal/models/repo_cab.go | 10 + go/internal/worker/affectedcommits.go | 29 ++- go/internal/worker/affectedcommits_test.go | 81 ++++++++ go/internal/worker/worker.go | 1 + 8 files changed, 466 insertions(+), 5 deletions(-) create mode 100644 go/internal/database/datastore/repo_cab.go create mode 100644 go/internal/database/datastore/repo_cab_test.go create mode 100644 go/internal/models/repo_cab.go diff --git a/go/cmd/worker/main.go b/go/cmd/worker/main.go index 1edf4553542..64580986246 100644 --- a/go/cmd/worker/main.go +++ b/go/cmd/worker/main.go @@ -84,6 +84,7 @@ func run() error { stores := worker.Stores{ SourceRepo: db.NewSourceRepositoryStore(dsClient), + RepoCAB: db.NewRepoCABStore(dsClient), Vulnerability: db.NewVulnerabilityStore(db.VulnStoreConfig{ Client: dsClient, GCS: clients.NewGCSClient(gcsClient, vulnBucket), diff --git a/go/internal/database/datastore/models.go b/go/internal/database/datastore/models.go index 42b37c9e5a0..be2826260b5 100644 --- a/go/internal/database/datastore/models.go +++ b/go/internal/database/datastore/models.go @@ -109,6 +109,13 @@ type AliasDenyListEntry struct { VulnID string `datastore:"bug_id"` } +// RepoConsiderAllBranchesAllowList holds a repository URL or regex pattern +// for repo-based consider-all-branches git enumeration. +type RepoConsiderAllBranchesAllowList struct { + Type string `datastore:"type"` + Value string `datastore:"value"` +} + type Severity struct { Type string `datastore:"type"` Score string `datastore:"score"` diff --git a/go/internal/database/datastore/repo_cab.go b/go/internal/database/datastore/repo_cab.go new file mode 100644 index 00000000000..b1bbdd5ca35 --- /dev/null +++ b/go/internal/database/datastore/repo_cab.go @@ -0,0 +1,156 @@ +package datastore + +import ( + "context" + "fmt" + "regexp" + "sync" + "time" + + "cloud.google.com/go/datastore" + "github.com/google/osv.dev/go/internal/models" +) + +const defaultCABCacheTTL = 5 * time.Minute + +// RepoCABStore handles Datastore persistence and caching for the repository Consider All Branches allowlist. +type RepoCABStore struct { + client *datastore.Client + cacheTTL time.Duration + + mu sync.RWMutex + urlCache map[string]struct{} + regexCache []*regexp.Regexp + lastFetched time.Time +} + +var _ models.RepoCABStore = (*RepoCABStore)(nil) + +// NewRepoCABStore returns a new RepoCABStore instance with default cache TTL (5 minutes). +func NewRepoCABStore(client *datastore.Client) *RepoCABStore { + return NewRepoCABStoreWithTTL(client, defaultCABCacheTTL) +} + +// NewRepoCABStoreWithTTL returns a new RepoCABStore instance with a specified cache TTL. +func NewRepoCABStoreWithTTL(client *datastore.Client, cacheTTL time.Duration) *RepoCABStore { + return &RepoCABStore{ + client: client, + cacheTTL: cacheTTL, + } +} + +// ShouldConsiderAllBranches returns true if the repoURL matches any pattern or url in the cab allowlist. +func (s *RepoCABStore) ShouldConsiderAllBranches(ctx context.Context, repoURL string) (bool, error) { + if repoURL == "" || s.client == nil { + return false, nil + } + + normalized := normalizeRepo(repoURL) + if normalized == "" { + return false, nil + } + + // 1. URL matching + matchedURL, err := s.matchURL(ctx, normalized) + if err != nil { + return false, err + } + if matchedURL { + return true, nil + } + + // 2. Regex matching + matchedPattern, err := s.matchPattern(ctx, repoURL, normalized) + if err != nil { + return false, err + } + + return matchedPattern, nil +} + +// matchURL checks if the repo URL matches an exact URL allowlist entry in Datastore cache. +func (s *RepoCABStore) matchURL(ctx context.Context, repo string) (bool, error) { + if err := s.loadCache(ctx); err != nil { + return false, err + } + + s.mu.RLock() + defer s.mu.RUnlock() + + _, ok := s.urlCache[repo] + + return ok, nil +} + +// matchPattern checks if the repo URL matches any cached compiled regex pattern. +func (s *RepoCABStore) matchPattern(ctx context.Context, repoURL, normalizedRepo string) (bool, error) { + if err := s.loadCache(ctx); err != nil { + return false, err + } + + s.mu.RLock() + defer s.mu.RUnlock() + + for _, re := range s.regexCache { + if re.MatchString(repoURL) || (normalizedRepo != "" && re.MatchString(normalizedRepo)) { + return true, nil + } + } + + return false, nil +} + +// loadCache retrieves all allowlist entries from Datastore, using in-memory caching for URLs and regexes. +func (s *RepoCABStore) loadCache(ctx context.Context) error { + // Fast path: check cache validity under read lock. + s.mu.RLock() + if s.urlCache != nil && s.regexCache != nil && time.Since(s.lastFetched) < s.cacheTTL { + s.mu.RUnlock() + return nil + } + s.mu.RUnlock() + + s.mu.Lock() + defer s.mu.Unlock() + + // Re-check if cache is valid in case another goroutine refreshed it while we are waiting for write lock + if s.urlCache != nil && s.regexCache != nil && time.Since(s.lastFetched) < s.cacheTTL { + return nil + } + + var entries []RepoConsiderAllBranchesAllowList + query := datastore.NewQuery("RepoConsiderAllBranchesAllowList") + if _, err := s.client.GetAll(ctx, query, &entries); err != nil { + return fmt.Errorf("failed fetching RepoConsiderAllBranchesAllowList entities: %w", err) + } + + urlCache := make(map[string]struct{}) + regexCache := make([]*regexp.Regexp, 0) + + for _, entry := range entries { + switch entry.Type { + case "regex": + if entry.Value == "" { + continue + } + re, err := regexp.Compile(entry.Value) + if err != nil { + // Skip invalid regex entries. + continue + } + regexCache = append(regexCache, re) + + default: // URL exact match entries + if entry.Value == "" { + continue + } + urlCache[entry.Value] = struct{}{} + } + } + + s.urlCache = urlCache + s.regexCache = regexCache + s.lastFetched = time.Now() + + return nil +} diff --git a/go/internal/database/datastore/repo_cab_test.go b/go/internal/database/datastore/repo_cab_test.go new file mode 100644 index 00000000000..d86f76a30ac --- /dev/null +++ b/go/internal/database/datastore/repo_cab_test.go @@ -0,0 +1,186 @@ +package datastore + +import ( + "context" + "testing" + "time" + + "cloud.google.com/go/datastore" + "github.com/google/osv.dev/go/testutils" +) + +func TestRepoCABStore_ShouldConsiderAllBranches(t *testing.T) { + ctx := context.Background() + dsClient := testutils.MustNewDatastoreClientForTesting(t) + store := NewRepoCABStore(dsClient) + + // Seed test data in Datastore with exact URLs and regexes for Consider All Branches (CAB) + testEntries := []RepoConsiderAllBranchesAllowList{ + {Type: "url", Value: "github.com/google/osv.dev"}, + {Type: "url", Value: "github.com/foo/bar"}, + {Type: "regex", Value: "github\\.com/org-glob/.*"}, + {Type: "regex", Value: "^https?://github\\.com/regex-org/.*$"}, + {Type: "regex", Value: "github\\.com/test/repo-."}, + } + + keys := []*datastore.Key{ + datastore.NameKey("RepoConsiderAllBranchesAllowList", "github.com/google/osv.dev", nil), + datastore.NameKey("RepoConsiderAllBranchesAllowList", "github.com/foo/bar", nil), + datastore.NameKey("RepoConsiderAllBranchesAllowList", "github\\.com/org-glob/.*", nil), + datastore.NameKey("RepoConsiderAllBranchesAllowList", "^https?://github\\.com/regex-org/.*$", nil), + datastore.NameKey("RepoConsiderAllBranchesAllowList", "github\\.com/test/repo-.", nil), + } + + if _, err := dsClient.PutMulti(ctx, keys, testEntries); err != nil { + t.Fatalf("Failed to setup test data: %v", err) + } + + tests := []struct { + name string + repoURL string + want bool + }{ + { + name: "Empty repo URL", + repoURL: "", + want: false, + }, + { + name: "Exact URL match", + repoURL: "https://github.com/google/osv.dev.git", + want: true, + }, + { + name: "Normalized lookup without .git suffix", + repoURL: "https://github.com/google/osv.dev", + want: true, + }, + { + name: "Normalized lookup with trailing slash", + repoURL: "github.com/foo/bar/", + want: true, + }, + { + name: "Normalized lookup with scheme matching host+path key", + repoURL: "https://github.com/foo/bar", + want: true, + }, + { + name: "Regex pattern matching repo in org", + repoURL: "https://github.com/org-glob/sub-repo.git", + want: true, + }, + { + name: "Regex pattern matching another repo in org", + repoURL: "github.com/org-glob/another-repo", + want: true, + }, + { + name: "Anchored regex pattern matching", + repoURL: "https://github.com/regex-org/my-project", + want: true, + }, + { + name: "Character class regex matching", + repoURL: "https://github.com/test/repo-1", + want: true, + }, + + { + name: "Unlisted repo", + repoURL: "https://github.com/unlisted/repo", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := store.ShouldConsiderAllBranches(ctx, tt.repoURL) + if err != nil { + t.Fatalf("ShouldConsiderAllBranches(%q) unexpected error: %v", tt.repoURL, err) + } + if got != tt.want { + t.Errorf("ShouldConsiderAllBranches(%q) = %v, want %v", tt.repoURL, got, tt.want) + } + }) + } +} + +func TestRepoCABStore_Caching(t *testing.T) { + ctx := context.Background() + dsClient := testutils.MustNewDatastoreClientForTesting(t) + // Create store with very short TTL for testing refresh behavior + store := NewRepoCABStoreWithTTL(dsClient, 100*time.Millisecond) + + regexEntry := RepoConsiderAllBranchesAllowList{ + Type: "regex", + Value: "github\\.com/cached-org/.*", + } + regexKey := datastore.NameKey("RepoConsiderAllBranchesAllowList", "github\\.com/cached-org/.*", nil) + + urlEntry := RepoConsiderAllBranchesAllowList{ + Type: "url", + Value: "github.com/cached-url/repo", + } + urlKey := datastore.NameKey("RepoConsiderAllBranchesAllowList", "github.com/cached-url/repo", nil) + + if _, err := dsClient.PutMulti(ctx, []*datastore.Key{regexKey, urlKey}, []RepoConsiderAllBranchesAllowList{regexEntry, urlEntry}); err != nil { + t.Fatalf("Failed setup: %v", err) + } + + // First match populates cache + got, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-org/repo1") + if err != nil || !got { + t.Fatalf("Initial ShouldConsiderAllBranches failed for regex: got %v, err %v", got, err) + } + + gotURL, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-url/repo") + if err != nil || !gotURL { + t.Fatalf("Initial ShouldConsiderAllBranches failed for url: got %v, err %v", gotURL, err) + } + + // Check that cache is populated + store.mu.RLock() + if len(store.regexCache) != 1 { + t.Errorf("expected 1 cached regex, got %d", len(store.regexCache)) + } + if _, ok := store.urlCache["github.com/cached-url/repo"]; !ok { + t.Errorf("expected urlCache to contain github.com/cached-url/repo") + } + store.mu.RUnlock() + + // Delete from Datastore to test that cache hit still succeeds before TTL expires + if err := dsClient.DeleteMulti(ctx, []*datastore.Key{regexKey, urlKey}); err != nil { + t.Fatalf("Failed deleting keys: %v", err) + } + + gotCached, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-org/repo2") + if err != nil || !gotCached { + t.Errorf("Expected cache hit for regex to succeed even after DB deletion, got %v, err %v", gotCached, err) + } + + gotCachedURL, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-url/repo") + if err != nil || !gotCachedURL { + t.Errorf("Expected cache hit for url to succeed even after DB deletion, got %v, err %v", gotCachedURL, err) + } + + // Wait for TTL to expire + time.Sleep(150 * time.Millisecond) + + // After TTL expiry, store should re-query DB and find no matches + gotExpired, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-org/repo3") + if err != nil { + t.Fatalf("Unexpected error after cache expiry: %v", err) + } + if gotExpired { + t.Errorf("Expected ShouldConsiderAllBranches for regex to return false after TTL expiry and DB deletion, got true") + } + + gotExpiredURL, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-url/repo") + if err != nil { + t.Fatalf("Unexpected error after cache expiry for url: %v", err) + } + if gotExpiredURL { + t.Errorf("Expected ShouldConsiderAllBranches for url to return false after TTL expiry and DB deletion, got true") + } +} diff --git a/go/internal/models/repo_cab.go b/go/internal/models/repo_cab.go new file mode 100644 index 00000000000..4125d833b13 --- /dev/null +++ b/go/internal/models/repo_cab.go @@ -0,0 +1,10 @@ +// Package models contains the domain types for the OSV database. +package models + +import "context" + +// RepoCABStore is the repository allowlist store for the Consider All Branches (CAB) feature. +type RepoCABStore interface { + // ShouldConsiderAllBranches returns true if the repository URL matches the consider all branches (CAB) allowlist. + ShouldConsiderAllBranches(ctx context.Context, repoURL string) (bool, error) +} diff --git a/go/internal/worker/affectedcommits.go b/go/internal/worker/affectedcommits.go index ff6c2af4fad..19ced3a1d82 100644 --- a/go/internal/worker/affectedcommits.go +++ b/go/internal/worker/affectedcommits.go @@ -31,7 +31,8 @@ func (e *Engine) populateAffectedCommitsAndTags(ctx context.Context, vuln *osvsc if aRange.GetType() != osvschema.Range_GIT || repo == "" { continue } - resp, err := fetchAffectedCommits(ctx, e.GitterClient, e.GitterHost, aRange, sourceRepo.GitAnalysis, vuln.GetId()) + considerAllBranches := e.shouldConsiderAllBranches(ctx, repo, sourceRepo) + resp, err := fetchAffectedCommits(ctx, e.GitterClient, e.GitterHost, aRange, sourceRepo.GitAnalysis, vuln.GetId(), considerAllBranches) if err != nil { return models.AffectedCommitsResult{}, err } @@ -54,8 +55,8 @@ func (e *Engine) populateAffectedCommitsAndTags(ctx context.Context, vuln *osvsc }, nil } -func fetchAffectedCommits(ctx context.Context, client *http.Client, gitterHost string, aRange *osvschema.Range, gitAnalysis *models.GitAnalysisConfig, refID string) (*gitterpb.AffectedCommitsResponse, error) { - req, err := newAffectedCommitsRequest(aRange, gitAnalysis, refID) +func fetchAffectedCommits(ctx context.Context, client *http.Client, gitterHost string, aRange *osvschema.Range, gitAnalysis *models.GitAnalysisConfig, refID string, considerAllBranches bool) (*gitterpb.AffectedCommitsResponse, error) { + req, err := newAffectedCommitsRequest(aRange, gitAnalysis, refID, considerAllBranches) if err != nil { return nil, fmt.Errorf("failed constructing gitter request: %w", err) } @@ -163,10 +164,10 @@ func applyAffectedCommitsAndTags(resp *gitterpb.AffectedCommitsResponse, affecte } } -func newAffectedCommitsRequest(affectedRange *osvschema.Range, gitAnalysis *models.GitAnalysisConfig, refID string) (*gitterpb.AffectedCommitsRequest, error) { +func newAffectedCommitsRequest(affectedRange *osvschema.Range, gitAnalysis *models.GitAnalysisConfig, refID string, considerAllBranches bool) (*gitterpb.AffectedCommitsRequest, error) { gitterReq := &gitterpb.AffectedCommitsRequest{ Url: affectedRange.GetRepo(), - ConsiderAllBranches: gitAnalysis.ConsiderAllBranches, + ConsiderAllBranches: considerAllBranches, DetectCherrypicksIntroduced: gitAnalysis.DetectCherrypicks, DetectCherrypicksFixed: gitAnalysis.DetectCherrypicks, DetectCherrypicksLimit: gitAnalysis.DetectCherrypicks, @@ -197,3 +198,21 @@ func newAffectedCommitsRequest(affectedRange *osvschema.Range, gitAnalysis *mode return gitterReq, nil } + +func (e *Engine) shouldConsiderAllBranches(ctx context.Context, repo string, sourceRepo *models.SourceRepository) bool { + // If source repository level consider_all_branches is enabled, use it. + if sourceRepo.GitAnalysis.ConsiderAllBranches { + return true + } + + // Otherwise, check if the specific git repository is in the per-repo allowlist. + if e.Stores.RepoCAB == nil { + return false + } + inAllowlist, err := e.Stores.RepoCAB.ShouldConsiderAllBranches(ctx, repo) + if err != nil { + return false + } + + return inAllowlist +} diff --git a/go/internal/worker/affectedcommits_test.go b/go/internal/worker/affectedcommits_test.go index 34b4bd2c6fb..f4d47d3b3c3 100644 --- a/go/internal/worker/affectedcommits_test.go +++ b/go/internal/worker/affectedcommits_test.go @@ -86,3 +86,84 @@ func TestPopulateAffectedCommitsAndTags(t *testing.T) { t.Errorf("expected 3 events, got %d", len(aRange.GetEvents())) } } + +type mockRepoCABStore struct { + allowedRepos map[string]bool +} + +func (m *mockRepoCABStore) ShouldConsiderAllBranches(_ context.Context, repoURL string) (bool, error) { + if m.allowedRepos[repoURL] { + return true, nil + } + + return false, nil +} + +func TestShouldConsiderAllBranches(t *testing.T) { + ctx := context.Background() + mockStore := &mockRepoCABStore{ + allowedRepos: map[string]bool{ + "https://github.com/test-org/test-repo": true, + }, + } + + engineWithAllowlist := &Engine{ + Stores: Stores{ + RepoCAB: mockStore, + }, + } + engineWithoutAllowlist := &Engine{} + + sourceRepoCABFalse := &models.SourceRepository{ + GitAnalysis: &models.GitAnalysisConfig{ConsiderAllBranches: false}, + } + sourceRepoCABTrue := &models.SourceRepository{ + GitAnalysis: &models.GitAnalysisConfig{ConsiderAllBranches: true}, + } + + tests := []struct { + name string + engine *Engine + repo string + sourceRepo *models.SourceRepository + want bool + }{ + { + name: "Source repo CAB true overrides allowlist", + engine: engineWithoutAllowlist, + repo: "https://github.com/not-relevant/not-relevant.git", + sourceRepo: sourceRepoCABTrue, + want: true, + }, + { + name: "Source repo CAB false, in repo-based allowlist", + engine: engineWithAllowlist, + repo: "https://github.com/test-org/test-repo", + sourceRepo: sourceRepoCABFalse, + want: true, + }, + { + name: "Source repo CAB false, not in allowlist", + engine: engineWithAllowlist, + repo: "https://github.com/unlisted/repo.git", + sourceRepo: sourceRepoCABFalse, + want: false, + }, + { + name: "Source repo CAB false, nil allowlist store", + engine: engineWithoutAllowlist, + repo: "https://github.com/apache/hadoop.git", + sourceRepo: sourceRepoCABFalse, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.engine.shouldConsiderAllBranches(ctx, tt.repo, tt.sourceRepo) + if got != tt.want { + t.Errorf("shouldConsiderAllBranches(%q) = %v, want %v", tt.repo, got, tt.want) + } + }) + } +} diff --git a/go/internal/worker/worker.go b/go/internal/worker/worker.go index 03b15b2b155..9acb1e83a79 100644 --- a/go/internal/worker/worker.go +++ b/go/internal/worker/worker.go @@ -35,5 +35,6 @@ type Stores struct { Vulnerability models.VulnerabilityStore Relations models.RelationsStore ImportFindings models.ImportFindingsStore + RepoCAB models.RepoCABStore PyPIPublisher clients.Publisher } From 5efce4a2942d28db5cfb04fdff2a1ca929ffde1f Mon Sep 17 00:00:00 2001 From: Joey L Date: Thu, 6 Aug 2026 05:58:30 +0000 Subject: [PATCH 2/7] Address some comments --- go/internal/database/datastore/repo_cab.go | 4 +++- go/internal/worker/affectedcommits.go | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/go/internal/database/datastore/repo_cab.go b/go/internal/database/datastore/repo_cab.go index b1bbdd5ca35..ff143740919 100644 --- a/go/internal/database/datastore/repo_cab.go +++ b/go/internal/database/datastore/repo_cab.go @@ -3,12 +3,14 @@ package datastore import ( "context" "fmt" + "log/slog" "regexp" "sync" "time" "cloud.google.com/go/datastore" "github.com/google/osv.dev/go/internal/models" + "github.com/google/osv.dev/go/logger" ) const defaultCABCacheTTL = 5 * time.Minute @@ -135,7 +137,7 @@ func (s *RepoCABStore) loadCache(ctx context.Context) error { } re, err := regexp.Compile(entry.Value) if err != nil { - // Skip invalid regex entries. + logger.WarnContext(ctx, "Failed to compile RepoConsiderAllBranchesAllowList regex entry", slog.String("value", entry.Value), slog.Any("error", err)) continue } regexCache = append(regexCache, re) diff --git a/go/internal/worker/affectedcommits.go b/go/internal/worker/affectedcommits.go index 19ced3a1d82..5ef1635d07e 100644 --- a/go/internal/worker/affectedcommits.go +++ b/go/internal/worker/affectedcommits.go @@ -211,6 +211,7 @@ func (e *Engine) shouldConsiderAllBranches(ctx context.Context, repo string, sou } inAllowlist, err := e.Stores.RepoCAB.ShouldConsiderAllBranches(ctx, repo) if err != nil { + logger.ErrorContext(ctx, "Failed to check RepoCAB allowlist", slog.String("repo", repo), slog.Any("error", err)) return false } From b2ba368fdad377c9f6d714d44964b856f0362b61 Mon Sep 17 00:00:00 2001 From: Joey L Date: Thu, 6 Aug 2026 06:55:32 +0000 Subject: [PATCH 3/7] make cache global and only compile new regexes --- go/internal/database/datastore/repo_cab.go | 97 +++++++++++-------- .../database/datastore/repo_cab_test.go | 21 ++-- 2 files changed, 66 insertions(+), 52 deletions(-) diff --git a/go/internal/database/datastore/repo_cab.go b/go/internal/database/datastore/repo_cab.go index ff143740919..4440bbbccc6 100644 --- a/go/internal/database/datastore/repo_cab.go +++ b/go/internal/database/datastore/repo_cab.go @@ -13,32 +13,39 @@ import ( "github.com/google/osv.dev/go/logger" ) -const defaultCABCacheTTL = 5 * time.Minute - -// RepoCABStore handles Datastore persistence and caching for the repository Consider All Branches allowlist. -type RepoCABStore struct { - client *datastore.Client - cacheTTL time.Duration +const defaultCacheTTL = 5 * time.Minute +type allowlistCacheData struct { mu sync.RWMutex urlCache map[string]struct{} - regexCache []*regexp.Regexp + regexCache map[string]*regexp.Regexp lastFetched time.Time } -var _ models.RepoCABStore = (*RepoCABStore)(nil) +var allowlistCache = &allowlistCacheData{ + urlCache: make(map[string]struct{}), + regexCache: make(map[string]*regexp.Regexp), +} -// NewRepoCABStore returns a new RepoCABStore instance with default cache TTL (5 minutes). -func NewRepoCABStore(client *datastore.Client) *RepoCABStore { - return NewRepoCABStoreWithTTL(client, defaultCABCacheTTL) +// For testing purposes +func resetAllowlistCache() { + allowlistCache.mu.Lock() + defer allowlistCache.mu.Unlock() + allowlistCache.urlCache = make(map[string]struct{}) + allowlistCache.regexCache = make(map[string]*regexp.Regexp) + allowlistCache.lastFetched = time.Time{} } -// NewRepoCABStoreWithTTL returns a new RepoCABStore instance with a specified cache TTL. -func NewRepoCABStoreWithTTL(client *datastore.Client, cacheTTL time.Duration) *RepoCABStore { - return &RepoCABStore{ - client: client, - cacheTTL: cacheTTL, - } +// RepoCABStore handles Datastore persistence and caching for the repository Consider All Branches allowlist. +type RepoCABStore struct { + client *datastore.Client +} + +var _ models.RepoCABStore = (*RepoCABStore)(nil) + +// NewRepoCABStore returns a new RepoCABStore instance. +func NewRepoCABStore(client *datastore.Client) *RepoCABStore { + return &RepoCABStore{client: client} } // ShouldConsiderAllBranches returns true if the repoURL matches any pattern or url in the cab allowlist. @@ -76,10 +83,10 @@ func (s *RepoCABStore) matchURL(ctx context.Context, repo string) (bool, error) return false, err } - s.mu.RLock() - defer s.mu.RUnlock() + allowlistCache.mu.RLock() + defer allowlistCache.mu.RUnlock() - _, ok := s.urlCache[repo] + _, ok := allowlistCache.urlCache[repo] return ok, nil } @@ -90,10 +97,10 @@ func (s *RepoCABStore) matchPattern(ctx context.Context, repoURL, normalizedRepo return false, err } - s.mu.RLock() - defer s.mu.RUnlock() + allowlistCache.mu.RLock() + defer allowlistCache.mu.RUnlock() - for _, re := range s.regexCache { + for _, re := range allowlistCache.regexCache { if re.MatchString(repoURL) || (normalizedRepo != "" && re.MatchString(normalizedRepo)) { return true, nil } @@ -102,21 +109,21 @@ func (s *RepoCABStore) matchPattern(ctx context.Context, repoURL, normalizedRepo return false, nil } -// loadCache retrieves all allowlist entries from Datastore, using in-memory caching for URLs and regexes. +// loadCache retrieves all allowlist entries from Datastore, using in-memory global caching for URLs and regexes. func (s *RepoCABStore) loadCache(ctx context.Context) error { // Fast path: check cache validity under read lock. - s.mu.RLock() - if s.urlCache != nil && s.regexCache != nil && time.Since(s.lastFetched) < s.cacheTTL { - s.mu.RUnlock() + allowlistCache.mu.RLock() + if allowlistCache.urlCache != nil && allowlistCache.regexCache != nil && time.Since(allowlistCache.lastFetched) < defaultCacheTTL { + allowlistCache.mu.RUnlock() return nil } - s.mu.RUnlock() + allowlistCache.mu.RUnlock() - s.mu.Lock() - defer s.mu.Unlock() + allowlistCache.mu.Lock() + defer allowlistCache.mu.Unlock() - // Re-check if cache is valid in case another goroutine refreshed it while we are waiting for write lock - if s.urlCache != nil && s.regexCache != nil && time.Since(s.lastFetched) < s.cacheTTL { + // Re-check if cache is valid in case another goroutine refreshed it while we were waiting for write lock + if allowlistCache.urlCache != nil && allowlistCache.regexCache != nil && time.Since(allowlistCache.lastFetched) < defaultCacheTTL { return nil } @@ -126,8 +133,8 @@ func (s *RepoCABStore) loadCache(ctx context.Context) error { return fmt.Errorf("failed fetching RepoConsiderAllBranchesAllowList entities: %w", err) } - urlCache := make(map[string]struct{}) - regexCache := make([]*regexp.Regexp, 0) + newURLCache := make(map[string]struct{}) + newRegexCache := make(map[string]*regexp.Regexp) for _, entry := range entries { switch entry.Type { @@ -135,24 +142,28 @@ func (s *RepoCABStore) loadCache(ctx context.Context) error { if entry.Value == "" { continue } - re, err := regexp.Compile(entry.Value) - if err != nil { - logger.WarnContext(ctx, "Failed to compile RepoConsiderAllBranchesAllowList regex entry", slog.String("value", entry.Value), slog.Any("error", err)) - continue + re, ok := allowlistCache.regexCache[entry.Value] + if !ok { + var err error + re, err = regexp.Compile(entry.Value) + if err != nil { + logger.WarnContext(ctx, "Failed to compile RepoConsiderAllBranchesAllowList regex entry", slog.String("value", entry.Value), slog.Any("error", err)) + continue + } } - regexCache = append(regexCache, re) + newRegexCache[entry.Value] = re default: // URL exact match entries if entry.Value == "" { continue } - urlCache[entry.Value] = struct{}{} + newURLCache[entry.Value] = struct{}{} } } - s.urlCache = urlCache - s.regexCache = regexCache - s.lastFetched = time.Now() + allowlistCache.urlCache = newURLCache + allowlistCache.regexCache = newRegexCache + allowlistCache.lastFetched = time.Now() return nil } diff --git a/go/internal/database/datastore/repo_cab_test.go b/go/internal/database/datastore/repo_cab_test.go index d86f76a30ac..acdcf1d2de3 100644 --- a/go/internal/database/datastore/repo_cab_test.go +++ b/go/internal/database/datastore/repo_cab_test.go @@ -10,6 +10,7 @@ import ( ) func TestRepoCABStore_ShouldConsiderAllBranches(t *testing.T) { + resetAllowlistCache() ctx := context.Background() dsClient := testutils.MustNewDatastoreClientForTesting(t) store := NewRepoCABStore(dsClient) @@ -107,10 +108,10 @@ func TestRepoCABStore_ShouldConsiderAllBranches(t *testing.T) { } func TestRepoCABStore_Caching(t *testing.T) { + resetAllowlistCache() ctx := context.Background() dsClient := testutils.MustNewDatastoreClientForTesting(t) - // Create store with very short TTL for testing refresh behavior - store := NewRepoCABStoreWithTTL(dsClient, 100*time.Millisecond) + store := NewRepoCABStore(dsClient) regexEntry := RepoConsiderAllBranchesAllowList{ Type: "regex", @@ -140,14 +141,14 @@ func TestRepoCABStore_Caching(t *testing.T) { } // Check that cache is populated - store.mu.RLock() - if len(store.regexCache) != 1 { - t.Errorf("expected 1 cached regex, got %d", len(store.regexCache)) + allowlistCache.mu.RLock() + if len(allowlistCache.regexCache) != 1 { + t.Errorf("expected 1 cached regex, got %d", len(allowlistCache.regexCache)) } - if _, ok := store.urlCache["github.com/cached-url/repo"]; !ok { + if _, ok := allowlistCache.urlCache["github.com/cached-url/repo"]; !ok { t.Errorf("expected urlCache to contain github.com/cached-url/repo") } - store.mu.RUnlock() + allowlistCache.mu.RUnlock() // Delete from Datastore to test that cache hit still succeeds before TTL expires if err := dsClient.DeleteMulti(ctx, []*datastore.Key{regexKey, urlKey}); err != nil { @@ -164,8 +165,10 @@ func TestRepoCABStore_Caching(t *testing.T) { t.Errorf("Expected cache hit for url to succeed even after DB deletion, got %v, err %v", gotCachedURL, err) } - // Wait for TTL to expire - time.Sleep(150 * time.Millisecond) + // Simulate TTL expiry by setting lastFetched into the past + allowlistCache.mu.Lock() + allowlistCache.lastFetched = time.Now().Add(-6 * time.Minute) + allowlistCache.mu.Unlock() // After TTL expiry, store should re-query DB and find no matches gotExpired, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-org/repo3") From 6e3fafaa0f73b048e034ed9616c3fac4ff4e9913 Mon Sep 17 00:00:00 2001 From: Joey L Date: Fri, 7 Aug 2026 01:54:47 +0000 Subject: [PATCH 4/7] Expand allowlist to cherrypicks and some sensible renames --- go/cmd/worker/main.go | 4 +- go/internal/database/datastore/models.go | 13 +- .../database/datastore/repo_allowlist.go | 169 ++++++++++++++ .../database/datastore/repo_allowlist_test.go | 213 ++++++++++++++++++ go/internal/database/datastore/repo_cab.go | 169 -------------- .../database/datastore/repo_cab_test.go | 189 ---------------- go/internal/models/repo_allowlist.go | 18 ++ go/internal/models/repo_cab.go | 10 - go/internal/worker/affectedcommits.go | 56 +++-- go/internal/worker/affectedcommits_test.go | 126 ++++++++--- go/internal/worker/worker.go | 2 +- 11 files changed, 533 insertions(+), 436 deletions(-) create mode 100644 go/internal/database/datastore/repo_allowlist.go create mode 100644 go/internal/database/datastore/repo_allowlist_test.go delete mode 100644 go/internal/database/datastore/repo_cab.go delete mode 100644 go/internal/database/datastore/repo_cab_test.go create mode 100644 go/internal/models/repo_allowlist.go delete mode 100644 go/internal/models/repo_cab.go diff --git a/go/cmd/worker/main.go b/go/cmd/worker/main.go index 64580986246..2b9bbaf7ebb 100644 --- a/go/cmd/worker/main.go +++ b/go/cmd/worker/main.go @@ -83,8 +83,8 @@ func run() error { defer psClient.Close() stores := worker.Stores{ - SourceRepo: db.NewSourceRepositoryStore(dsClient), - RepoCAB: db.NewRepoCABStore(dsClient), + SourceRepo: db.NewSourceRepositoryStore(dsClient), + RepoAllowList: db.NewRepoAllowListStore(dsClient), Vulnerability: db.NewVulnerabilityStore(db.VulnStoreConfig{ Client: dsClient, GCS: clients.NewGCSClient(gcsClient, vulnBucket), diff --git a/go/internal/database/datastore/models.go b/go/internal/database/datastore/models.go index be2826260b5..285b41aecb7 100644 --- a/go/internal/database/datastore/models.go +++ b/go/internal/database/datastore/models.go @@ -109,11 +109,14 @@ type AliasDenyListEntry struct { VulnID string `datastore:"bug_id"` } -// RepoConsiderAllBranchesAllowList holds a repository URL or regex pattern -// for repo-based consider-all-branches git enumeration. -type RepoConsiderAllBranchesAllowList struct { - Type string `datastore:"type"` - Value string `datastore:"value"` +// RepoAllowList holds repository URL or regex pattern and repo-based git enumeration flags. +type RepoAllowList struct { + Type string `datastore:"type"` + Value string `datastore:"value"` + ConsiderAllBranches bool `datastore:"consider_all_branches"` + CherrypicksIntroduced bool `datastore:"cherrypicks_introduced"` + CherrypicksFixed bool `datastore:"cherrypicks_fixed"` + CherrypicksLimit bool `datastore:"cherrypicks_limit"` } type Severity struct { diff --git a/go/internal/database/datastore/repo_allowlist.go b/go/internal/database/datastore/repo_allowlist.go new file mode 100644 index 00000000000..497d7b1990e --- /dev/null +++ b/go/internal/database/datastore/repo_allowlist.go @@ -0,0 +1,169 @@ +package datastore + +import ( + "context" + "fmt" + "log/slog" + "regexp" + "sync" + "time" + + "cloud.google.com/go/datastore" + "github.com/google/osv.dev/go/internal/models" + "github.com/google/osv.dev/go/logger" +) + +const defaultCacheTTL = 5 * time.Minute + +type regexCacheEntry struct { + pattern *regexp.Regexp + flags models.RepoAllowListFlags +} + +type allowListCache struct { + mu sync.RWMutex + urlCache map[string]models.RepoAllowListFlags + regexCache map[string]regexCacheEntry + lastFetched time.Time +} + +var cache = &allowListCache{ + urlCache: make(map[string]models.RepoAllowListFlags), + regexCache: make(map[string]regexCacheEntry), +} + +// For testing purposes +func resetCache() { + cache.mu.Lock() + defer cache.mu.Unlock() + cache.urlCache = make(map[string]models.RepoAllowListFlags) + cache.regexCache = make(map[string]regexCacheEntry) + cache.lastFetched = time.Time{} +} + +// RepoAllowListStore handles Datastore persistence and caching for the repository allowlist. +type RepoAllowListStore struct { + client *datastore.Client +} + +var _ models.RepoAllowListStore = (*RepoAllowListStore)(nil) + +// NewRepoAllowListStore returns a new RepoAllowListStore instance. +func NewRepoAllowListStore(client *datastore.Client) *RepoAllowListStore { + return &RepoAllowListStore{client: client} +} + +// GetFlags returns the combined feature flags for the given repoURL. +func (s *RepoAllowListStore) GetFlags(ctx context.Context, repoURL string) (models.RepoAllowListFlags, error) { + if repoURL == "" || s.client == nil { + return models.RepoAllowListFlags{}, nil + } + + normalized := normalizeRepo(repoURL) + if normalized == "" { + return models.RepoAllowListFlags{}, nil + } + + if err := s.loadCache(ctx); err != nil { + return models.RepoAllowListFlags{}, err + } + + cache.mu.RLock() + defer cache.mu.RUnlock() + + // 1. Exact URL match takes precedence over regex patterns + if flags, ok := cache.urlCache[normalized]; ok { + return flags, nil + } + + var res models.RepoAllowListFlags + + // 2. Regex pattern match (used when no exact URL match exists) + for _, re := range cache.regexCache { + // Try to match both the actual and normalized repoURL + if re.pattern.MatchString(repoURL) || (normalized != "" && re.pattern.MatchString(normalized)) { + res.ConsiderAllBranches = res.ConsiderAllBranches || re.flags.ConsiderAllBranches + res.CherrypicksIntroduced = res.CherrypicksIntroduced || re.flags.CherrypicksIntroduced + res.CherrypicksFixed = res.CherrypicksFixed || re.flags.CherrypicksFixed + res.CherrypicksLimit = res.CherrypicksLimit || re.flags.CherrypicksLimit + } + } + + return res, nil +} + +// loadCache retrieves all allowlist entries from Datastore, caches the url and regexes. +func (s *RepoAllowListStore) loadCache(ctx context.Context) error { + // Fast path: check cache validity under read lock. + cache.mu.RLock() + if cache.urlCache != nil && cache.regexCache != nil && time.Since(cache.lastFetched) < defaultCacheTTL { + cache.mu.RUnlock() + return nil + } + cache.mu.RUnlock() + + cache.mu.Lock() + defer cache.mu.Unlock() + + // Re-check if cache is valid in case another goroutine refreshed it while we were waiting for write lock + if cache.urlCache != nil && cache.regexCache != nil && time.Since(cache.lastFetched) < defaultCacheTTL { + return nil + } + + var entries []RepoAllowList + query := datastore.NewQuery("RepoAllowList") + if _, err := s.client.GetAll(ctx, query, &entries); err != nil { + return fmt.Errorf("failed fetching RepoAllowList entities: %w", err) + } + + newURLCache := make(map[string]models.RepoAllowListFlags) + newRegexCache := make(map[string]regexCacheEntry) + + for _, entry := range entries { + flags := models.RepoAllowListFlags{ + ConsiderAllBranches: entry.ConsiderAllBranches, + CherrypicksIntroduced: entry.CherrypicksIntroduced, + CherrypicksFixed: entry.CherrypicksFixed, + CherrypicksLimit: entry.CherrypicksLimit, + } + + switch entry.Type { + case "regex": + if entry.Value == "" { + continue + } + cached, ok := cache.regexCache[entry.Value] + var re *regexp.Regexp + if ok && cached.pattern != nil { + re = cached.pattern + } else { + var err error + re, err = regexp.Compile(entry.Value) + if err != nil { + logger.WarnContext(ctx, "Failed to compile RepoAllowList regex entry", slog.String("value", entry.Value), slog.Any("error", err)) + continue + } + } + newRegexCache[entry.Value] = regexCacheEntry{ + pattern: re, + flags: flags, + } + + default: // URL exact match entries + if entry.Value == "" { + continue + } + newURLCache[entry.Value] = flags + normValue := normalizeRepo(entry.Value) + if normValue != "" { + newURLCache[normValue] = flags + } + } + } + + cache.urlCache = newURLCache + cache.regexCache = newRegexCache + cache.lastFetched = time.Now() + + return nil +} diff --git a/go/internal/database/datastore/repo_allowlist_test.go b/go/internal/database/datastore/repo_allowlist_test.go new file mode 100644 index 00000000000..87bbb486d8a --- /dev/null +++ b/go/internal/database/datastore/repo_allowlist_test.go @@ -0,0 +1,213 @@ +package datastore + +import ( + "context" + "testing" + "time" + + "cloud.google.com/go/datastore" + "github.com/google/osv.dev/go/internal/models" + "github.com/google/osv.dev/go/testutils" +) + +func TestRepoAllowListStore_GetFlags(t *testing.T) { + resetCache() + ctx := context.Background() + dsClient := testutils.MustNewDatastoreClientForTesting(t) + store := NewRepoAllowListStore(dsClient) + + // Seed test data in Datastore with exact URLs and regexes for RepoAllowList + testEntries := []RepoAllowList{ + {Type: "url", Value: "github.com/google/osv.dev", ConsiderAllBranches: true, CherrypicksIntroduced: true, CherrypicksFixed: false, CherrypicksLimit: false}, + {Type: "url", Value: "github.com/foo/bar", ConsiderAllBranches: false, CherrypicksIntroduced: false, CherrypicksFixed: true, CherrypicksLimit: true}, + {Type: "url", Value: "github.com/all/flags", ConsiderAllBranches: true, CherrypicksIntroduced: true, CherrypicksFixed: true, CherrypicksLimit: true}, + {Type: "regex", Value: "github\\.com/org-glob/.*", ConsiderAllBranches: true, CherrypicksIntroduced: false, CherrypicksFixed: true, CherrypicksLimit: false}, + {Type: "regex", Value: "^https?://github\\.com/regex-intro/.*$", ConsiderAllBranches: false, CherrypicksIntroduced: true, CherrypicksFixed: false, CherrypicksLimit: true}, + {Type: "url", Value: "github.com/org-glob/override-repo", ConsiderAllBranches: false, CherrypicksIntroduced: true, CherrypicksFixed: false, CherrypicksLimit: false}, + {Type: "url", Value: "https://github.com/unnorm/repo.git", ConsiderAllBranches: false, CherrypicksIntroduced: false, CherrypicksFixed: false, CherrypicksLimit: true}, + } + + keys := []*datastore.Key{ + datastore.NameKey("RepoAllowList", "github.com/google/osv.dev", nil), + datastore.NameKey("RepoAllowList", "github.com/foo/bar", nil), + datastore.NameKey("RepoAllowList", "github.com/all/flags", nil), + datastore.NameKey("RepoAllowList", "github\\.com/org-glob/.*", nil), + datastore.NameKey("RepoAllowList", "^https?://github\\.com/regex-intro/.*$", nil), + datastore.NameKey("RepoAllowList", "github.com/org-glob/override-repo", nil), + datastore.NameKey("RepoAllowList", "https://github.com/unnorm/repo.git", nil), + } + + if _, err := dsClient.PutMulti(ctx, keys, testEntries); err != nil { + t.Fatalf("Failed to setup test data: %v", err) + } + + tests := []struct { + name string + repoURL string + wantFlags models.RepoAllowListFlags + }{ + { + name: "Empty repo URL", + repoURL: "", + wantFlags: models.RepoAllowListFlags{}, + }, + { + name: "Exact URL match (CAB and Intro enabled)", + repoURL: "https://github.com/google/osv.dev.git", + wantFlags: models.RepoAllowListFlags{ + ConsiderAllBranches: true, + CherrypicksIntroduced: true, + }, + }, + { + name: "Normalized lookup (Fixed and Limit enabled)", + repoURL: "https://github.com/foo/bar", + wantFlags: models.RepoAllowListFlags{ + CherrypicksFixed: true, + CherrypicksLimit: true, + }, + }, + { + name: "All flags enabled", + repoURL: "https://github.com/all/flags", + wantFlags: models.RepoAllowListFlags{ + ConsiderAllBranches: true, + CherrypicksIntroduced: true, + CherrypicksFixed: true, + CherrypicksLimit: true, + }, + }, + { + name: "Regex pattern match (CAB and Fixed enabled)", + repoURL: "https://github.com/org-glob/sub-repo.git", + wantFlags: models.RepoAllowListFlags{ + ConsiderAllBranches: true, + CherrypicksFixed: true, + }, + }, + { + name: "Exact URL match takes precedence over regex match", + repoURL: "https://github.com/org-glob/override-repo", + wantFlags: models.RepoAllowListFlags{ + CherrypicksIntroduced: true, + }, + }, + { + name: "Un-normalized Datastore URL entry matches normalized query", + repoURL: "https://github.com/unnorm/repo", + wantFlags: models.RepoAllowListFlags{ + CherrypicksLimit: true, + }, + }, + { + name: "Regex pattern match (Intro and Limit enabled)", + repoURL: "https://github.com/regex-intro/my-project", + wantFlags: models.RepoAllowListFlags{ + CherrypicksIntroduced: true, + CherrypicksLimit: true, + }, + }, + { + name: "Unlisted repo", + repoURL: "https://github.com/unlisted/repo", + wantFlags: models.RepoAllowListFlags{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotFlags, err := store.GetFlags(ctx, tt.repoURL) + if err != nil { + t.Fatalf("GetFlags(%q) unexpected error: %v", tt.repoURL, err) + } + if gotFlags != tt.wantFlags { + t.Errorf("GetFlags(%q) = %+v, want %+v", tt.repoURL, gotFlags, tt.wantFlags) + } + }) + } +} + +func TestRepoAllowListStore_Caching(t *testing.T) { + resetCache() + ctx := context.Background() + dsClient := testutils.MustNewDatastoreClientForTesting(t) + store := NewRepoAllowListStore(dsClient) + + regexEntry := RepoAllowList{ + Type: "regex", + Value: "github\\.com/cached-org/.*", + ConsiderAllBranches: true, + CherrypicksIntroduced: true, + } + regexKey := datastore.NameKey("RepoAllowList", "github\\.com/cached-org/.*", nil) + + urlEntry := RepoAllowList{ + Type: "url", + Value: "github.com/cached-url/repo", + CherrypicksFixed: true, + CherrypicksLimit: true, + } + urlKey := datastore.NameKey("RepoAllowList", "github.com/cached-url/repo", nil) + + if _, err := dsClient.PutMulti(ctx, []*datastore.Key{regexKey, urlKey}, []RepoAllowList{regexEntry, urlEntry}); err != nil { + t.Fatalf("Failed setup: %v", err) + } + + // First match populates cache + flagsRegex, err := store.GetFlags(ctx, "https://github.com/cached-org/repo1") + if err != nil || !flagsRegex.ConsiderAllBranches || !flagsRegex.CherrypicksIntroduced { + t.Fatalf("Initial GetFlags failed for regex: got %+v, err %v", flagsRegex, err) + } + + flagsURL, err := store.GetFlags(ctx, "https://github.com/cached-url/repo") + if err != nil || !flagsURL.CherrypicksFixed || !flagsURL.CherrypicksLimit { + t.Fatalf("Initial GetFlags failed for url: got %+v, err %v", flagsURL, err) + } + + // Check that cache is populated + cache.mu.RLock() + if len(cache.regexCache) != 1 { + t.Errorf("expected 1 cached regex, got %d", len(cache.regexCache)) + } + if flags, ok := cache.urlCache["github.com/cached-url/repo"]; !ok || !flags.CherrypicksFixed || !flags.CherrypicksLimit { + t.Errorf("expected urlCache to contain github.com/cached-url/repo with CherrypicksFixed=true, CherrypicksLimit=true") + } + cache.mu.RUnlock() + + // Delete from Datastore to test that cache hit still succeeds before TTL expires + if err := dsClient.DeleteMulti(ctx, []*datastore.Key{regexKey, urlKey}); err != nil { + t.Fatalf("Failed deleting keys: %v", err) + } + + flagsCachedRegex, err := store.GetFlags(ctx, "https://github.com/cached-org/repo2") + if err != nil || !flagsCachedRegex.ConsiderAllBranches { + t.Errorf("Expected cache hit for regex to succeed even after DB deletion, got %+v, err %v", flagsCachedRegex, err) + } + + flagsCachedURL, err := store.GetFlags(ctx, "https://github.com/cached-url/repo") + if err != nil || !flagsCachedURL.CherrypicksFixed { + t.Errorf("Expected cache hit for url to succeed even after DB deletion, got %+v, err %v", flagsCachedURL, err) + } + + // Simulate TTL expiry by setting lastFetched into the past + cache.mu.Lock() + cache.lastFetched = time.Now().Add(-6 * time.Minute) + cache.mu.Unlock() + + // After TTL expiry, store should re-query DB and find no matches + flagsExpired, err := store.GetFlags(ctx, "https://github.com/cached-org/repo3") + if err != nil { + t.Fatalf("Unexpected error after cache expiry: %v", err) + } + if flagsExpired.ConsiderAllBranches || flagsExpired.CherrypicksIntroduced { + t.Errorf("Expected GetFlags for regex to return zero flags after TTL expiry and DB deletion, got %+v", flagsExpired) + } + + flagsExpiredURL, err := store.GetFlags(ctx, "https://github.com/cached-url/repo") + if err != nil { + t.Fatalf("Unexpected error after cache expiry for url: %v", err) + } + if flagsExpiredURL.CherrypicksFixed || flagsExpiredURL.CherrypicksLimit { + t.Errorf("Expected GetFlags for url to return zero flags after TTL expiry and DB deletion, got %+v", flagsExpiredURL) + } +} diff --git a/go/internal/database/datastore/repo_cab.go b/go/internal/database/datastore/repo_cab.go deleted file mode 100644 index 4440bbbccc6..00000000000 --- a/go/internal/database/datastore/repo_cab.go +++ /dev/null @@ -1,169 +0,0 @@ -package datastore - -import ( - "context" - "fmt" - "log/slog" - "regexp" - "sync" - "time" - - "cloud.google.com/go/datastore" - "github.com/google/osv.dev/go/internal/models" - "github.com/google/osv.dev/go/logger" -) - -const defaultCacheTTL = 5 * time.Minute - -type allowlistCacheData struct { - mu sync.RWMutex - urlCache map[string]struct{} - regexCache map[string]*regexp.Regexp - lastFetched time.Time -} - -var allowlistCache = &allowlistCacheData{ - urlCache: make(map[string]struct{}), - regexCache: make(map[string]*regexp.Regexp), -} - -// For testing purposes -func resetAllowlistCache() { - allowlistCache.mu.Lock() - defer allowlistCache.mu.Unlock() - allowlistCache.urlCache = make(map[string]struct{}) - allowlistCache.regexCache = make(map[string]*regexp.Regexp) - allowlistCache.lastFetched = time.Time{} -} - -// RepoCABStore handles Datastore persistence and caching for the repository Consider All Branches allowlist. -type RepoCABStore struct { - client *datastore.Client -} - -var _ models.RepoCABStore = (*RepoCABStore)(nil) - -// NewRepoCABStore returns a new RepoCABStore instance. -func NewRepoCABStore(client *datastore.Client) *RepoCABStore { - return &RepoCABStore{client: client} -} - -// ShouldConsiderAllBranches returns true if the repoURL matches any pattern or url in the cab allowlist. -func (s *RepoCABStore) ShouldConsiderAllBranches(ctx context.Context, repoURL string) (bool, error) { - if repoURL == "" || s.client == nil { - return false, nil - } - - normalized := normalizeRepo(repoURL) - if normalized == "" { - return false, nil - } - - // 1. URL matching - matchedURL, err := s.matchURL(ctx, normalized) - if err != nil { - return false, err - } - if matchedURL { - return true, nil - } - - // 2. Regex matching - matchedPattern, err := s.matchPattern(ctx, repoURL, normalized) - if err != nil { - return false, err - } - - return matchedPattern, nil -} - -// matchURL checks if the repo URL matches an exact URL allowlist entry in Datastore cache. -func (s *RepoCABStore) matchURL(ctx context.Context, repo string) (bool, error) { - if err := s.loadCache(ctx); err != nil { - return false, err - } - - allowlistCache.mu.RLock() - defer allowlistCache.mu.RUnlock() - - _, ok := allowlistCache.urlCache[repo] - - return ok, nil -} - -// matchPattern checks if the repo URL matches any cached compiled regex pattern. -func (s *RepoCABStore) matchPattern(ctx context.Context, repoURL, normalizedRepo string) (bool, error) { - if err := s.loadCache(ctx); err != nil { - return false, err - } - - allowlistCache.mu.RLock() - defer allowlistCache.mu.RUnlock() - - for _, re := range allowlistCache.regexCache { - if re.MatchString(repoURL) || (normalizedRepo != "" && re.MatchString(normalizedRepo)) { - return true, nil - } - } - - return false, nil -} - -// loadCache retrieves all allowlist entries from Datastore, using in-memory global caching for URLs and regexes. -func (s *RepoCABStore) loadCache(ctx context.Context) error { - // Fast path: check cache validity under read lock. - allowlistCache.mu.RLock() - if allowlistCache.urlCache != nil && allowlistCache.regexCache != nil && time.Since(allowlistCache.lastFetched) < defaultCacheTTL { - allowlistCache.mu.RUnlock() - return nil - } - allowlistCache.mu.RUnlock() - - allowlistCache.mu.Lock() - defer allowlistCache.mu.Unlock() - - // Re-check if cache is valid in case another goroutine refreshed it while we were waiting for write lock - if allowlistCache.urlCache != nil && allowlistCache.regexCache != nil && time.Since(allowlistCache.lastFetched) < defaultCacheTTL { - return nil - } - - var entries []RepoConsiderAllBranchesAllowList - query := datastore.NewQuery("RepoConsiderAllBranchesAllowList") - if _, err := s.client.GetAll(ctx, query, &entries); err != nil { - return fmt.Errorf("failed fetching RepoConsiderAllBranchesAllowList entities: %w", err) - } - - newURLCache := make(map[string]struct{}) - newRegexCache := make(map[string]*regexp.Regexp) - - for _, entry := range entries { - switch entry.Type { - case "regex": - if entry.Value == "" { - continue - } - re, ok := allowlistCache.regexCache[entry.Value] - if !ok { - var err error - re, err = regexp.Compile(entry.Value) - if err != nil { - logger.WarnContext(ctx, "Failed to compile RepoConsiderAllBranchesAllowList regex entry", slog.String("value", entry.Value), slog.Any("error", err)) - continue - } - } - newRegexCache[entry.Value] = re - - default: // URL exact match entries - if entry.Value == "" { - continue - } - newURLCache[entry.Value] = struct{}{} - } - } - - allowlistCache.urlCache = newURLCache - allowlistCache.regexCache = newRegexCache - allowlistCache.lastFetched = time.Now() - - return nil -} diff --git a/go/internal/database/datastore/repo_cab_test.go b/go/internal/database/datastore/repo_cab_test.go deleted file mode 100644 index acdcf1d2de3..00000000000 --- a/go/internal/database/datastore/repo_cab_test.go +++ /dev/null @@ -1,189 +0,0 @@ -package datastore - -import ( - "context" - "testing" - "time" - - "cloud.google.com/go/datastore" - "github.com/google/osv.dev/go/testutils" -) - -func TestRepoCABStore_ShouldConsiderAllBranches(t *testing.T) { - resetAllowlistCache() - ctx := context.Background() - dsClient := testutils.MustNewDatastoreClientForTesting(t) - store := NewRepoCABStore(dsClient) - - // Seed test data in Datastore with exact URLs and regexes for Consider All Branches (CAB) - testEntries := []RepoConsiderAllBranchesAllowList{ - {Type: "url", Value: "github.com/google/osv.dev"}, - {Type: "url", Value: "github.com/foo/bar"}, - {Type: "regex", Value: "github\\.com/org-glob/.*"}, - {Type: "regex", Value: "^https?://github\\.com/regex-org/.*$"}, - {Type: "regex", Value: "github\\.com/test/repo-."}, - } - - keys := []*datastore.Key{ - datastore.NameKey("RepoConsiderAllBranchesAllowList", "github.com/google/osv.dev", nil), - datastore.NameKey("RepoConsiderAllBranchesAllowList", "github.com/foo/bar", nil), - datastore.NameKey("RepoConsiderAllBranchesAllowList", "github\\.com/org-glob/.*", nil), - datastore.NameKey("RepoConsiderAllBranchesAllowList", "^https?://github\\.com/regex-org/.*$", nil), - datastore.NameKey("RepoConsiderAllBranchesAllowList", "github\\.com/test/repo-.", nil), - } - - if _, err := dsClient.PutMulti(ctx, keys, testEntries); err != nil { - t.Fatalf("Failed to setup test data: %v", err) - } - - tests := []struct { - name string - repoURL string - want bool - }{ - { - name: "Empty repo URL", - repoURL: "", - want: false, - }, - { - name: "Exact URL match", - repoURL: "https://github.com/google/osv.dev.git", - want: true, - }, - { - name: "Normalized lookup without .git suffix", - repoURL: "https://github.com/google/osv.dev", - want: true, - }, - { - name: "Normalized lookup with trailing slash", - repoURL: "github.com/foo/bar/", - want: true, - }, - { - name: "Normalized lookup with scheme matching host+path key", - repoURL: "https://github.com/foo/bar", - want: true, - }, - { - name: "Regex pattern matching repo in org", - repoURL: "https://github.com/org-glob/sub-repo.git", - want: true, - }, - { - name: "Regex pattern matching another repo in org", - repoURL: "github.com/org-glob/another-repo", - want: true, - }, - { - name: "Anchored regex pattern matching", - repoURL: "https://github.com/regex-org/my-project", - want: true, - }, - { - name: "Character class regex matching", - repoURL: "https://github.com/test/repo-1", - want: true, - }, - - { - name: "Unlisted repo", - repoURL: "https://github.com/unlisted/repo", - want: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := store.ShouldConsiderAllBranches(ctx, tt.repoURL) - if err != nil { - t.Fatalf("ShouldConsiderAllBranches(%q) unexpected error: %v", tt.repoURL, err) - } - if got != tt.want { - t.Errorf("ShouldConsiderAllBranches(%q) = %v, want %v", tt.repoURL, got, tt.want) - } - }) - } -} - -func TestRepoCABStore_Caching(t *testing.T) { - resetAllowlistCache() - ctx := context.Background() - dsClient := testutils.MustNewDatastoreClientForTesting(t) - store := NewRepoCABStore(dsClient) - - regexEntry := RepoConsiderAllBranchesAllowList{ - Type: "regex", - Value: "github\\.com/cached-org/.*", - } - regexKey := datastore.NameKey("RepoConsiderAllBranchesAllowList", "github\\.com/cached-org/.*", nil) - - urlEntry := RepoConsiderAllBranchesAllowList{ - Type: "url", - Value: "github.com/cached-url/repo", - } - urlKey := datastore.NameKey("RepoConsiderAllBranchesAllowList", "github.com/cached-url/repo", nil) - - if _, err := dsClient.PutMulti(ctx, []*datastore.Key{regexKey, urlKey}, []RepoConsiderAllBranchesAllowList{regexEntry, urlEntry}); err != nil { - t.Fatalf("Failed setup: %v", err) - } - - // First match populates cache - got, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-org/repo1") - if err != nil || !got { - t.Fatalf("Initial ShouldConsiderAllBranches failed for regex: got %v, err %v", got, err) - } - - gotURL, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-url/repo") - if err != nil || !gotURL { - t.Fatalf("Initial ShouldConsiderAllBranches failed for url: got %v, err %v", gotURL, err) - } - - // Check that cache is populated - allowlistCache.mu.RLock() - if len(allowlistCache.regexCache) != 1 { - t.Errorf("expected 1 cached regex, got %d", len(allowlistCache.regexCache)) - } - if _, ok := allowlistCache.urlCache["github.com/cached-url/repo"]; !ok { - t.Errorf("expected urlCache to contain github.com/cached-url/repo") - } - allowlistCache.mu.RUnlock() - - // Delete from Datastore to test that cache hit still succeeds before TTL expires - if err := dsClient.DeleteMulti(ctx, []*datastore.Key{regexKey, urlKey}); err != nil { - t.Fatalf("Failed deleting keys: %v", err) - } - - gotCached, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-org/repo2") - if err != nil || !gotCached { - t.Errorf("Expected cache hit for regex to succeed even after DB deletion, got %v, err %v", gotCached, err) - } - - gotCachedURL, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-url/repo") - if err != nil || !gotCachedURL { - t.Errorf("Expected cache hit for url to succeed even after DB deletion, got %v, err %v", gotCachedURL, err) - } - - // Simulate TTL expiry by setting lastFetched into the past - allowlistCache.mu.Lock() - allowlistCache.lastFetched = time.Now().Add(-6 * time.Minute) - allowlistCache.mu.Unlock() - - // After TTL expiry, store should re-query DB and find no matches - gotExpired, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-org/repo3") - if err != nil { - t.Fatalf("Unexpected error after cache expiry: %v", err) - } - if gotExpired { - t.Errorf("Expected ShouldConsiderAllBranches for regex to return false after TTL expiry and DB deletion, got true") - } - - gotExpiredURL, err := store.ShouldConsiderAllBranches(ctx, "https://github.com/cached-url/repo") - if err != nil { - t.Fatalf("Unexpected error after cache expiry for url: %v", err) - } - if gotExpiredURL { - t.Errorf("Expected ShouldConsiderAllBranches for url to return false after TTL expiry and DB deletion, got true") - } -} diff --git a/go/internal/models/repo_allowlist.go b/go/internal/models/repo_allowlist.go new file mode 100644 index 00000000000..01b33fc4bbc --- /dev/null +++ b/go/internal/models/repo_allowlist.go @@ -0,0 +1,18 @@ +// Package models contains the domain types for the OSV database. +package models + +import "context" + +// RepoAllowListFlags holds boolean feature flags for repository git analysis. +type RepoAllowListFlags struct { + ConsiderAllBranches bool + CherrypicksIntroduced bool + CherrypicksFixed bool + CherrypicksLimit bool +} + +// RepoAllowListStore is the repository allowlist store for repository configuration overrides. +type RepoAllowListStore interface { + // GetFlags returns the combined feature flags for the given repository URL. + GetFlags(ctx context.Context, repoURL string) (RepoAllowListFlags, error) +} diff --git a/go/internal/models/repo_cab.go b/go/internal/models/repo_cab.go deleted file mode 100644 index 4125d833b13..00000000000 --- a/go/internal/models/repo_cab.go +++ /dev/null @@ -1,10 +0,0 @@ -// Package models contains the domain types for the OSV database. -package models - -import "context" - -// RepoCABStore is the repository allowlist store for the Consider All Branches (CAB) feature. -type RepoCABStore interface { - // ShouldConsiderAllBranches returns true if the repository URL matches the consider all branches (CAB) allowlist. - ShouldConsiderAllBranches(ctx context.Context, repoURL string) (bool, error) -} diff --git a/go/internal/worker/affectedcommits.go b/go/internal/worker/affectedcommits.go index 5ef1635d07e..156c09e9bb0 100644 --- a/go/internal/worker/affectedcommits.go +++ b/go/internal/worker/affectedcommits.go @@ -31,8 +31,8 @@ func (e *Engine) populateAffectedCommitsAndTags(ctx context.Context, vuln *osvsc if aRange.GetType() != osvschema.Range_GIT || repo == "" { continue } - considerAllBranches := e.shouldConsiderAllBranches(ctx, repo, sourceRepo) - resp, err := fetchAffectedCommits(ctx, e.GitterClient, e.GitterHost, aRange, sourceRepo.GitAnalysis, vuln.GetId(), considerAllBranches) + flags := e.getGitAnalysisFlags(ctx, sourceRepo.GitAnalysis, repo) + resp, err := fetchAffectedCommits(ctx, e.GitterClient, e.GitterHost, aRange, vuln.GetId(), flags) if err != nil { return models.AffectedCommitsResult{}, err } @@ -55,8 +55,8 @@ func (e *Engine) populateAffectedCommitsAndTags(ctx context.Context, vuln *osvsc }, nil } -func fetchAffectedCommits(ctx context.Context, client *http.Client, gitterHost string, aRange *osvschema.Range, gitAnalysis *models.GitAnalysisConfig, refID string, considerAllBranches bool) (*gitterpb.AffectedCommitsResponse, error) { - req, err := newAffectedCommitsRequest(aRange, gitAnalysis, refID, considerAllBranches) +func fetchAffectedCommits(ctx context.Context, client *http.Client, gitterHost string, aRange *osvschema.Range, refID string, flags models.RepoAllowListFlags) (*gitterpb.AffectedCommitsResponse, error) { + req, err := newAffectedCommitsRequest(aRange, flags, refID) if err != nil { return nil, fmt.Errorf("failed constructing gitter request: %w", err) } @@ -164,13 +164,13 @@ func applyAffectedCommitsAndTags(resp *gitterpb.AffectedCommitsResponse, affecte } } -func newAffectedCommitsRequest(affectedRange *osvschema.Range, gitAnalysis *models.GitAnalysisConfig, refID string, considerAllBranches bool) (*gitterpb.AffectedCommitsRequest, error) { +func newAffectedCommitsRequest(affectedRange *osvschema.Range, gitAnalysisFlags models.RepoAllowListFlags, refID string) (*gitterpb.AffectedCommitsRequest, error) { gitterReq := &gitterpb.AffectedCommitsRequest{ Url: affectedRange.GetRepo(), - ConsiderAllBranches: considerAllBranches, - DetectCherrypicksIntroduced: gitAnalysis.DetectCherrypicks, - DetectCherrypicksFixed: gitAnalysis.DetectCherrypicks, - DetectCherrypicksLimit: gitAnalysis.DetectCherrypicks, + ConsiderAllBranches: gitAnalysisFlags.ConsiderAllBranches, + DetectCherrypicksIntroduced: gitAnalysisFlags.CherrypicksIntroduced, + DetectCherrypicksFixed: gitAnalysisFlags.CherrypicksFixed, + DetectCherrypicksLimit: gitAnalysisFlags.CherrypicksLimit, Events: make([]*gitterpb.Event, 0, len(affectedRange.GetEvents())), RefId: refID, } @@ -199,21 +199,29 @@ func newAffectedCommitsRequest(affectedRange *osvschema.Range, gitAnalysis *mode return gitterReq, nil } -func (e *Engine) shouldConsiderAllBranches(ctx context.Context, repo string, sourceRepo *models.SourceRepository) bool { - // If source repository level consider_all_branches is enabled, use it. - if sourceRepo.GitAnalysis.ConsiderAllBranches { - return true - } - - // Otherwise, check if the specific git repository is in the per-repo allowlist. - if e.Stores.RepoCAB == nil { - return false - } - inAllowlist, err := e.Stores.RepoCAB.ShouldConsiderAllBranches(ctx, repo) - if err != nil { - logger.ErrorContext(ctx, "Failed to check RepoCAB allowlist", slog.String("repo", repo), slog.Any("error", err)) - return false +// getGitAnalysisFlags determines the gitter flags for a given repo. +// If the flag is true at source repo level, we follow that. Otherwise, we check the repo allowlist. +func (e *Engine) getGitAnalysisFlags(ctx context.Context, gitAnalysis *models.GitAnalysisConfig, repo string) models.RepoAllowListFlags { + var flags models.RepoAllowListFlags + + if gitAnalysis != nil { + flags.ConsiderAllBranches = gitAnalysis.ConsiderAllBranches + flags.CherrypicksIntroduced = gitAnalysis.DetectCherrypicks + flags.CherrypicksFixed = gitAnalysis.DetectCherrypicks + flags.CherrypicksLimit = gitAnalysis.DetectCherrypicks + } + + if e.Stores.RepoAllowList != nil { + storeFlags, err := e.Stores.RepoAllowList.GetFlags(ctx, repo) + if err != nil { + logger.ErrorContext(ctx, "Failed to check RepoAllowList", slog.String("repo", repo), slog.Any("error", err)) + } else { + flags.ConsiderAllBranches = flags.ConsiderAllBranches || storeFlags.ConsiderAllBranches + flags.CherrypicksIntroduced = flags.CherrypicksIntroduced || storeFlags.CherrypicksIntroduced + flags.CherrypicksFixed = flags.CherrypicksFixed || storeFlags.CherrypicksFixed + flags.CherrypicksLimit = flags.CherrypicksLimit || storeFlags.CherrypicksLimit + } } - return inAllowlist + return flags } diff --git a/go/internal/worker/affectedcommits_test.go b/go/internal/worker/affectedcommits_test.go index f4d47d3b3c3..170e53893d2 100644 --- a/go/internal/worker/affectedcommits_test.go +++ b/go/internal/worker/affectedcommits_test.go @@ -87,38 +87,42 @@ func TestPopulateAffectedCommitsAndTags(t *testing.T) { } } -type mockRepoCABStore struct { - allowedRepos map[string]bool +type mockRepoAllowListStore struct { + flags map[string]models.RepoAllowListFlags } -func (m *mockRepoCABStore) ShouldConsiderAllBranches(_ context.Context, repoURL string) (bool, error) { - if m.allowedRepos[repoURL] { - return true, nil +func (m *mockRepoAllowListStore) GetFlags(_ context.Context, repoURL string) (models.RepoAllowListFlags, error) { + if m.flags == nil { + return models.RepoAllowListFlags{}, nil } - return false, nil + return m.flags[repoURL], nil } -func TestShouldConsiderAllBranches(t *testing.T) { +func TestGetGitAnalysisFlags(t *testing.T) { ctx := context.Background() - mockStore := &mockRepoCABStore{ - allowedRepos: map[string]bool{ - "https://github.com/test-org/test-repo": true, + mockStore := &mockRepoAllowListStore{ + flags: map[string]models.RepoAllowListFlags{ + "https://github.com/test-org/store-enabled-repo": { + ConsiderAllBranches: true, + CherrypicksFixed: true, + }, + "https://github.com/test-org/store-all-false-repo": {}, }, } engineWithAllowlist := &Engine{ Stores: Stores{ - RepoCAB: mockStore, + RepoAllowList: mockStore, }, } engineWithoutAllowlist := &Engine{} - sourceRepoCABFalse := &models.SourceRepository{ - GitAnalysis: &models.GitAnalysisConfig{ConsiderAllBranches: false}, + sourceRepoAllFalse := &models.SourceRepository{ + GitAnalysis: &models.GitAnalysisConfig{ConsiderAllBranches: false, DetectCherrypicks: false}, } - sourceRepoCABTrue := &models.SourceRepository{ - GitAnalysis: &models.GitAnalysisConfig{ConsiderAllBranches: true}, + sourceRepoAllTrue := &models.SourceRepository{ + GitAnalysis: &models.GitAnalysisConfig{ConsiderAllBranches: true, DetectCherrypicks: true}, } tests := []struct { @@ -126,43 +130,93 @@ func TestShouldConsiderAllBranches(t *testing.T) { engine *Engine repo string sourceRepo *models.SourceRepository - want bool + wantFlags models.RepoAllowListFlags }{ { - name: "Source repo CAB true overrides allowlist", - engine: engineWithoutAllowlist, - repo: "https://github.com/not-relevant/not-relevant.git", - sourceRepo: sourceRepoCABTrue, - want: true, + name: "Source repo true, store unlisted (all false) -> returns source repo flags", + engine: engineWithAllowlist, + repo: "https://github.com/unlisted/repo", + sourceRepo: sourceRepoAllTrue, + wantFlags: models.RepoAllowListFlags{ + ConsiderAllBranches: true, + CherrypicksIntroduced: true, + CherrypicksFixed: true, + CherrypicksLimit: true, + }, + }, + { + name: "Source repo false, store unlisted (all false) -> returns all false", + engine: engineWithAllowlist, + repo: "https://github.com/unlisted/repo", + sourceRepo: sourceRepoAllFalse, + wantFlags: models.RepoAllowListFlags{}, + }, + { + name: "Source repo false, store repo explicitly all false -> returns all false", + engine: engineWithAllowlist, + repo: "https://github.com/test-org/store-all-false-repo", + sourceRepo: sourceRepoAllFalse, + wantFlags: models.RepoAllowListFlags{}, }, { - name: "Source repo CAB false, in repo-based allowlist", + name: "Source repo false, store repo has flags true -> returns store flags", engine: engineWithAllowlist, - repo: "https://github.com/test-org/test-repo", - sourceRepo: sourceRepoCABFalse, - want: true, + repo: "https://github.com/test-org/store-enabled-repo", + sourceRepo: sourceRepoAllFalse, + wantFlags: models.RepoAllowListFlags{ + ConsiderAllBranches: true, + CherrypicksFixed: true, + }, + }, + { + name: "Source repo true, store repo has different flags true -> returns merged flags", + engine: engineWithAllowlist, + repo: "https://github.com/test-org/store-enabled-repo", + sourceRepo: &models.SourceRepository{ + GitAnalysis: &models.GitAnalysisConfig{ConsiderAllBranches: false, DetectCherrypicks: true}, + }, + wantFlags: models.RepoAllowListFlags{ + ConsiderAllBranches: true, + CherrypicksIntroduced: true, + CherrypicksFixed: true, + CherrypicksLimit: true, + }, }, { - name: "Source repo CAB false, not in allowlist", + name: "Nil source repo, store repo has flags true -> returns store flags", engine: engineWithAllowlist, - repo: "https://github.com/unlisted/repo.git", - sourceRepo: sourceRepoCABFalse, - want: false, + repo: "https://github.com/test-org/store-enabled-repo", + sourceRepo: nil, + wantFlags: models.RepoAllowListFlags{ + ConsiderAllBranches: true, + CherrypicksFixed: true, + }, }, { - name: "Source repo CAB false, nil allowlist store", + name: "Nil source repo, store unlisted -> returns all false", + engine: engineWithAllowlist, + repo: "https://github.com/unlisted/repo", + sourceRepo: nil, + wantFlags: models.RepoAllowListFlags{}, + }, + { + name: "Nil allowlist store, source repo false -> returns all false", engine: engineWithoutAllowlist, - repo: "https://github.com/apache/hadoop.git", - sourceRepo: sourceRepoCABFalse, - want: false, + repo: "https://github.com/test-org/store-enabled-repo", + sourceRepo: sourceRepoAllFalse, + wantFlags: models.RepoAllowListFlags{}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got := tt.engine.shouldConsiderAllBranches(ctx, tt.repo, tt.sourceRepo) - if got != tt.want { - t.Errorf("shouldConsiderAllBranches(%q) = %v, want %v", tt.repo, got, tt.want) + var gitAnalysis *models.GitAnalysisConfig + if tt.sourceRepo != nil { + gitAnalysis = tt.sourceRepo.GitAnalysis + } + got := tt.engine.getGitAnalysisFlags(ctx, gitAnalysis, tt.repo) + if got != tt.wantFlags { + t.Errorf("getGitAnalysisFlags(%q) = %+v, want %+v", tt.repo, got, tt.wantFlags) } }) } diff --git a/go/internal/worker/worker.go b/go/internal/worker/worker.go index 9acb1e83a79..8182deb7495 100644 --- a/go/internal/worker/worker.go +++ b/go/internal/worker/worker.go @@ -35,6 +35,6 @@ type Stores struct { Vulnerability models.VulnerabilityStore Relations models.RelationsStore ImportFindings models.ImportFindingsStore - RepoCAB models.RepoCABStore + RepoAllowList models.RepoAllowListStore PyPIPublisher clients.Publisher } From 887d9a0e6500c7f173544774a244a085b68a7192 Mon Sep 17 00:00:00 2001 From: Joey L Date: Fri, 7 Aug 2026 05:45:11 +0000 Subject: [PATCH 5/7] Update comments --- go/internal/worker/affectedcommits.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/go/internal/worker/affectedcommits.go b/go/internal/worker/affectedcommits.go index 156c09e9bb0..fd6f7ae2e23 100644 --- a/go/internal/worker/affectedcommits.go +++ b/go/internal/worker/affectedcommits.go @@ -199,8 +199,7 @@ func newAffectedCommitsRequest(affectedRange *osvschema.Range, gitAnalysisFlags return gitterReq, nil } -// getGitAnalysisFlags determines the gitter flags for a given repo. -// If the flag is true at source repo level, we follow that. Otherwise, we check the repo allowlist. +// getGitAnalysisFlags determines the gitter flags for a given repo by combining the source repo level and allowlist configs func (e *Engine) getGitAnalysisFlags(ctx context.Context, gitAnalysis *models.GitAnalysisConfig, repo string) models.RepoAllowListFlags { var flags models.RepoAllowListFlags From 877faedf0b3f8b4af19e306ecd5d62768c0fe8e0 Mon Sep 17 00:00:00 2001 From: Joey L Date: Fri, 7 Aug 2026 06:11:48 +0000 Subject: [PATCH 6/7] address comments --- go/internal/database/datastore/models.go | 13 +++++++------ go/internal/database/datastore/repo_allowlist.go | 2 +- go/internal/worker/affectedcommits.go | 12 ++++++------ 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/go/internal/database/datastore/models.go b/go/internal/database/datastore/models.go index 285b41aecb7..1cba1dfaafe 100644 --- a/go/internal/database/datastore/models.go +++ b/go/internal/database/datastore/models.go @@ -111,12 +111,13 @@ type AliasDenyListEntry struct { // RepoAllowList holds repository URL or regex pattern and repo-based git enumeration flags. type RepoAllowList struct { - Type string `datastore:"type"` - Value string `datastore:"value"` - ConsiderAllBranches bool `datastore:"consider_all_branches"` - CherrypicksIntroduced bool `datastore:"cherrypicks_introduced"` - CherrypicksFixed bool `datastore:"cherrypicks_fixed"` - CherrypicksLimit bool `datastore:"cherrypicks_limit"` + Type string `datastore:"type"` // `url` or `regex` + Value string `datastore:"value"` // normalized URL or regex pattern + // The following corresponds to git analysis flags for gitter's affected-commits endpoint. + ConsiderAllBranches bool `datastore:"consider_all_branches"` + CherrypicksIntroduced bool `datastore:"cherrypicks_introduced"` + CherrypicksFixed bool `datastore:"cherrypicks_fixed"` + CherrypicksLimit bool `datastore:"cherrypicks_limit"` } type Severity struct { diff --git a/go/internal/database/datastore/repo_allowlist.go b/go/internal/database/datastore/repo_allowlist.go index 497d7b1990e..2b46aa6a2c4 100644 --- a/go/internal/database/datastore/repo_allowlist.go +++ b/go/internal/database/datastore/repo_allowlist.go @@ -81,7 +81,7 @@ func (s *RepoAllowListStore) GetFlags(ctx context.Context, repoURL string) (mode // 2. Regex pattern match (used when no exact URL match exists) for _, re := range cache.regexCache { // Try to match both the actual and normalized repoURL - if re.pattern.MatchString(repoURL) || (normalized != "" && re.pattern.MatchString(normalized)) { + if re.pattern.MatchString(repoURL) || re.pattern.MatchString(normalized) { res.ConsiderAllBranches = res.ConsiderAllBranches || re.flags.ConsiderAllBranches res.CherrypicksIntroduced = res.CherrypicksIntroduced || re.flags.CherrypicksIntroduced res.CherrypicksFixed = res.CherrypicksFixed || re.flags.CherrypicksFixed diff --git a/go/internal/worker/affectedcommits.go b/go/internal/worker/affectedcommits.go index fd6f7ae2e23..bd80aba184d 100644 --- a/go/internal/worker/affectedcommits.go +++ b/go/internal/worker/affectedcommits.go @@ -200,14 +200,14 @@ func newAffectedCommitsRequest(affectedRange *osvschema.Range, gitAnalysisFlags } // getGitAnalysisFlags determines the gitter flags for a given repo by combining the source repo level and allowlist configs -func (e *Engine) getGitAnalysisFlags(ctx context.Context, gitAnalysis *models.GitAnalysisConfig, repo string) models.RepoAllowListFlags { +func (e *Engine) getGitAnalysisFlags(ctx context.Context, sourceRepoGitConfig *models.GitAnalysisConfig, repo string) models.RepoAllowListFlags { var flags models.RepoAllowListFlags - if gitAnalysis != nil { - flags.ConsiderAllBranches = gitAnalysis.ConsiderAllBranches - flags.CherrypicksIntroduced = gitAnalysis.DetectCherrypicks - flags.CherrypicksFixed = gitAnalysis.DetectCherrypicks - flags.CherrypicksLimit = gitAnalysis.DetectCherrypicks + if sourceRepoGitConfig != nil { + flags.ConsiderAllBranches = sourceRepoGitConfig.ConsiderAllBranches + flags.CherrypicksIntroduced = sourceRepoGitConfig.DetectCherrypicks + flags.CherrypicksFixed = sourceRepoGitConfig.DetectCherrypicks + flags.CherrypicksLimit = sourceRepoGitConfig.DetectCherrypicks } if e.Stores.RepoAllowList != nil { From 91510661c59e764569dab90ca5f4a151d99da43a Mon Sep 17 00:00:00 2001 From: Joey L Date: Fri, 7 Aug 2026 07:06:32 +0000 Subject: [PATCH 7/7] address more comments (regex cache) --- go/internal/database/datastore/repo_allowlist.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/go/internal/database/datastore/repo_allowlist.go b/go/internal/database/datastore/repo_allowlist.go index 2b46aa6a2c4..9f278e98a6f 100644 --- a/go/internal/database/datastore/repo_allowlist.go +++ b/go/internal/database/datastore/repo_allowlist.go @@ -116,8 +116,7 @@ func (s *RepoAllowListStore) loadCache(ctx context.Context) error { return fmt.Errorf("failed fetching RepoAllowList entities: %w", err) } - newURLCache := make(map[string]models.RepoAllowListFlags) - newRegexCache := make(map[string]regexCacheEntry) + newURLCache := make(map[string]models.RepoAllowListFlags, len(entries)) for _, entry := range entries { flags := models.RepoAllowListFlags{ @@ -144,7 +143,7 @@ func (s *RepoAllowListStore) loadCache(ctx context.Context) error { continue } } - newRegexCache[entry.Value] = regexCacheEntry{ + cache.regexCache[entry.Value] = regexCacheEntry{ pattern: re, flags: flags, } @@ -162,7 +161,6 @@ func (s *RepoAllowListStore) loadCache(ctx context.Context) error { } cache.urlCache = newURLCache - cache.regexCache = newRegexCache cache.lastFetched = time.Now() return nil