From 171386f7a73b939514734acf22521c7a75d641da Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 4 Sep 2026 14:24:13 +0100 Subject: [PATCH] fix: remediate severity-ordered review findings across cache, scanners, and proxies - H2: gormstore soft-delete + storage_key UNIQUE collision; re-save after eviction succeeds - H1: cache getOrFetch cache-hit branch no longer runs CheckVulnerabilities on metadata entries - M2: cache store() streams upstream body to temp file + SHA256 instead of double-buffering - H3/L3: FetchResult/FetchFunc plumbing (+RequiresAuth/AuthProvider); proxy closures adapted; goproxy strips git+ prefix from UpstreamURL - H4: scanner normalizes bypass package names; CheckVulnerabilities builds normalized packageKey - M4: requireAdmin routes through extractAPIKey -> ValidateAPIKey -> admin role check - M5: network client keeps only final RecordFailure (one count per attempt cycle) - M3: normalizeOriginHost + buildCheckOrigin rewrite; origin_test.go added - M1: app lifecycle cancel func; Run -> Shutdown on listen error (sole cache owner); JoinHostPort - M6: VulnerabilityThresholds defaults all -1 so BlockOnSeverity governs - L4: metadata.NormalizeSeverity at 4 severity-count sites (MEDIUM->MODERATE consistency) - L1: handleDeleteBypass uses structured *errors.Error + ErrCodeNotFound - L2: pypi extractVersionFromFilename (wheel/2nd segment, non-wheel/last digit segment); cacheVersion uses full unique basename; regression tests added - L5: document per-instance ValidationCache key scoping; rename packageURL param to resource All: go build ./... && go vet ./... && go test ./... pass; golangci-lint clean. --- pkg/app/app.go | 124 ++++++++++++---- pkg/app/handlers.go | 2 +- pkg/app/handlers_admin.go | 26 ++-- pkg/app/handlers_vulnerabilities.go | 18 ++- pkg/app/origin_test.go | 73 ++++++++++ pkg/auth/validation_cache.go | 45 +++--- pkg/auth/validator.go | 33 +++++ pkg/cache/cache.go | 151 +++++++++++++------- pkg/cache/cache_test.go | 38 ++--- pkg/config/config.go | 10 ++ pkg/metadata/gormstore/gormstore_v2.go | 16 ++- pkg/metadata/gormstore/gormstore_v2_test.go | 51 +++++++ pkg/network/client.go | 7 - pkg/network/client_test.go | 35 +++++ pkg/proxy/goproxy/goproxy.go | 84 +++++++---- pkg/proxy/npm/npm.go | 28 ++-- pkg/proxy/pypi/pypi.go | 118 +++++++++------ pkg/proxy/pypi/pypi_test.go | 70 +++++++++ pkg/scanner/scanner.go | 24 +++- pkg/scanner/scanner_test.go | 76 ++++++++++ 20 files changed, 790 insertions(+), 239 deletions(-) create mode 100644 pkg/app/origin_test.go diff --git a/pkg/app/app.go b/pkg/app/app.go index ecc1563..dac62fa 100644 --- a/pkg/app/app.go +++ b/pkg/app/app.go @@ -5,10 +5,12 @@ package app import ( "context" "fmt" + "net" "net/http" "net/url" "os" "os/signal" + "strconv" "strings" "syscall" "time" @@ -44,6 +46,7 @@ import ( // App represents the main application type App struct { config *config.Config + cancel context.CancelFunc app *fiber.App healthChecker *health.Checker cache *cache.Manager @@ -580,20 +583,13 @@ func (a *App) setupServer() error { return nil } -// Run starts the application func (a *App) Run() error { - ctx := context.Background() - - // Start WebSocket server - a.wsServer.Start(ctx) - - // Start pre-warming worker - a.prewarmWorker.Start(ctx) - - // Start rescan worker if enabled - if a.rescanWorker != nil { - go a.rescanWorker.Start(ctx) - } + // Cancellable worker context: aggregation + stats broadcaster goroutines + // watch ctx.Done() and exit, so Shutdown can stop them before closing + // the storage/metadata backends they touch. + ctx, cancel := context.WithCancel(context.Background()) + a.cancel = cancel + defer cancel() // Start download data aggregation worker (runs every hour) go a.startAggregationWorker(ctx) @@ -605,7 +601,7 @@ func (a *App) Run() error { // Start Fiber server in goroutine. Branch on TLS to pick Listen vs ListenTLS. errChan := make(chan error, 1) go func() { - addr := fmt.Sprintf("%s:%d", a.config.Server.Host, a.config.Server.Port) + addr := net.JoinHostPort(a.config.Server.Host, strconv.Itoa(a.config.Server.Port)) if a.config.Server.TLS.Enabled { log.Info(). Str("addr", addr). @@ -630,6 +626,13 @@ func (a *App) Run() error { select { case err := <-errChan: + // Server failed to start or stopped unexpectedly. Cancel the worker + // contexts and run the same teardown Shutdown performs so background + // goroutines don't keep running against closed storage/metadata. + shutdownErr := a.Shutdown() + if shutdownErr != nil { + log.Error().Err(shutdownErr).Msg("Error during shutdown after server error") + } return fmt.Errorf("server error: %w", err) case sig := <-sigChan: log.Info(). @@ -640,7 +643,6 @@ func (a *App) Run() error { // Graceful shutdown return a.Shutdown() } - // Shutdown gracefully shuts down the application func (a *App) Shutdown() error { log.Info().Msg("Starting graceful shutdown") @@ -650,6 +652,12 @@ func (a *App) Shutdown() error { log.Error().Err(err).Msg("Error shutting down Fiber server") } + // Cancel worker contexts first so the aggregation and stats broadcaster + // goroutines stop before the storage/metadata backends they touch close. + if a.cancel != nil { + a.cancel() + } + // Drain async auth writes (LastUsedAt updates) before closing metadata. if a.authManager != nil { a.authManager.Close() @@ -666,14 +674,15 @@ func (a *App) Shutdown() error { // Close analytics engine a.analyticsEngine.Close() // #nosec G104 -- Cleanup, error not critical - // Close storage - if err := a.storage.Close(); err != nil { - log.Error().Err(err).Msg("Error closing storage") - } - - // Close metadata store - if err := a.metadata.Close(); err != nil { - log.Error().Err(err).Msg("Error closing metadata store") + // Close cache manager. This stops the cache cleanup worker and closes the + // storage + metadata backends (a.cache is the sole owner of both, sharing + // the same pointers with the App). Calling Close on storage/metadata here + // instead of via a.cache.Close() would double-close and leave the cleanup + // worker running against closed backends. + if a.cache != nil { + if err := a.cache.Close(); err != nil { + log.Error().Err(err).Msg("Error closing cache") + } } log.Info().Msg("Shutdown complete") @@ -762,6 +771,41 @@ func getOrDefaultStr(value, defaultValue string) string { return value } +// normalizeOriginHost lowercases a host and strips the scheme's default port +// (443 for https, 80 for http) so "https://app.example.com:443" and +// "https://app.example.com" are treated as the same origin. Non-default ports +// are preserved. Returns ("", false) for an empty/invalid host. +func normalizeOriginHost(scheme, host string) (string, bool) { + host = strings.ToLower(strings.TrimSpace(host)) + if host == "" { + return "", false + } + // Split off a port, if present. url.Parse stores the full host; use + // net.SplitHostPort so IPv6 literals like [::1]:8080 are handled. + h := host + var port string + if hh, p, err := net.SplitHostPort(host); err == nil { + h = hh + port = p + } + defaultPort := "" + switch scheme { + case "https": + defaultPort = "443" + case "http": + defaultPort = "80" + } + if port != "" && port == defaultPort { + return h, true + } + // Rebuild the host with the port (when present) for exact comparisons. + if port != "" { + // Reuse the original port form; net.JoinHostPort preserves IPv6 brackets. + return net.JoinHostPort(h, port), true + } + return h, true +} + // buildCheckOrigin returns a websocket Origin validator. // // Behaviour: @@ -771,13 +815,18 @@ func getOrDefaultStr(value, defaultValue string) string { // Origin host must match the Host header of the incoming request. // - requests with no Origin header are accepted (non-browser clients). // +// Scheme-default ports (443/80) are normalized out of both the allowlist and +// the incoming Origin, so "https://app.example.com" and +// "https://app.example.com:443" are treated identically. Wildcard matching is +// performed on the hostname only. +// // We deliberately do NOT default to allow-all to avoid Cross-Site WebSocket // Hijacking (CSWSH). func buildCheckOrigin(allowed []string) func(*http.Request) bool { // Pre-parse allowlist once. type originRule struct { scheme string - host string // exact host, or "" if hostSuffix is set + host string // exact normalized host:port (default port stripped), or "" if hostGlob is set hostGlob string // suffix match including leading "." } rules := make([]originRule, 0, len(allowed)) @@ -791,13 +840,18 @@ func buildCheckOrigin(allowed []string) func(*http.Request) bool { log.Warn().Str("entry", raw).Msg("Ignoring invalid AllowedOrigins entry") continue } - r := originRule{scheme: strings.ToLower(u.Scheme)} - host := strings.ToLower(u.Host) - if strings.HasPrefix(host, "*.") { + scheme := strings.ToLower(u.Scheme) + normHost, ok := normalizeOriginHost(scheme, u.Host) + if !ok { + log.Warn().Str("entry", raw).Msg("Ignoring invalid AllowedOrigins entry") + continue + } + r := originRule{scheme: scheme} + if strings.HasPrefix(normHost, "*.") { // "*.example.com" → match any subdomain plus the apex. - r.hostGlob = host[1:] // ".example.com" + r.hostGlob = normHost[1:] // ".example.com" } else { - r.host = host + r.host = normHost } rules = append(rules, r) } @@ -814,11 +868,19 @@ func buildCheckOrigin(allowed []string) func(*http.Request) bool { return false } originScheme := strings.ToLower(ou.Scheme) - originHost := strings.ToLower(ou.Host) + originHost, ok := normalizeOriginHost(originScheme, ou.Host) + if !ok { + log.Warn().Str("origin", origin).Msg("Rejecting WebSocket upgrade: malformed Origin header") + return false + } // Empty allowlist → same-origin only. if len(rules) == 0 { - return strings.EqualFold(originHost, r.Host) + reqHost, ok := normalizeOriginHost(originScheme, r.Host) + if !ok { + return false + } + return originHost == reqHost } for _, rule := range rules { diff --git a/pkg/app/handlers.go b/pkg/app/handlers.go index b8b10f9..bda0d99 100644 --- a/pkg/app/handlers.go +++ b/pkg/app/handlers.go @@ -139,7 +139,7 @@ func (a *App) handleListPackages(c *fiber.Ctx) error { // Count vulnerabilities by severity severityCounts := make(map[string]int) for _, vuln := range scanResult.Vulnerabilities { - severityCounts[strings.ToUpper(vuln.Severity)]++ + severityCounts[metadata.NormalizeSeverity(vuln.Severity)]++ } // Check if package should be blocked based on thresholds diff --git a/pkg/app/handlers_admin.go b/pkg/app/handlers_admin.go index f4e4c7c..e40cdf5 100644 --- a/pkg/app/handlers_admin.go +++ b/pkg/app/handlers_admin.go @@ -1,36 +1,28 @@ package app import ( - "strings" "time" "github.com/gofiber/fiber/v2" "github.com/lukaszraczylo/gohoarder/pkg/auth" + "github.com/lukaszraczylo/gohoarder/pkg/errors" "github.com/lukaszraczylo/gohoarder/pkg/metadata" "github.com/lukaszraczylo/gohoarder/pkg/uuid" "github.com/rs/zerolog/log" ) -// requireAdmin middleware checks for admin authentication +// requireAdmin middleware checks for admin authentication. It accepts the API +// key via either the Authorization Bearer header or the X-API-Key header +// (shared extractAPIKey), so admin clients are not limited to the bearer form. func (a *App) requireAdmin(c *fiber.Ctx) error { - // Get API key from Authorization header - authHeader := c.Get("Authorization") - if authHeader == "" { + // Get API key from Authorization or X-API-Key header + apiKey, ok := extractAPIKey(c) + if !ok { return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{ - "error": "missing authorization header", + "error": "missing API key; provide Authorization: Bearer or X-API-Key header", }) } - // Extract bearer token - parts := strings.SplitN(authHeader, " ", 2) - if len(parts) != 2 || strings.ToLower(parts[0]) != "bearer" { - return c.Status(fiber.StatusUnauthorized).JSON(fiber.Map{ - "error": "invalid authorization header format, expected: Bearer ", - }) - } - - apiKey := parts[1] - // Validate API key key, err := a.authManager.ValidateAPIKey(c.Context(), apiKey) if err != nil { @@ -304,7 +296,7 @@ func (a *App) handleDeleteBypass(c *fiber.Ctx) error { // Delete bypass if err := a.metadata.DeleteCVEBypass(ctx, bypassID); err != nil { - if strings.Contains(err.Error(), "not found") { + if appErr, ok := err.(*errors.Error); ok && appErr.Code == errors.ErrCodeNotFound { return c.Status(fiber.StatusNotFound).JSON(fiber.Map{"error": "bypass not found"}) } log.Error().Err(err).Msg("Failed to delete bypass") diff --git a/pkg/app/handlers_vulnerabilities.go b/pkg/app/handlers_vulnerabilities.go index e13e63d..f1c766e 100644 --- a/pkg/app/handlers_vulnerabilities.go +++ b/pkg/app/handlers_vulnerabilities.go @@ -77,11 +77,19 @@ func (a *App) handleVulnerabilities(c *fiber.Ctx) error { bypasses = []*metadata.CVEBypass{} } - // Build bypass map for fast lookup + // Build bypass map for fast lookup. + // Normalize Go cache-key names (modulePath/@v/version.zip) to the + // natural module path so admin bypass targets like + // "go/github.com/foo/bar@v1.2.3" match the package. + bypassName := name + if registry == "go" { + if idx := strings.Index(bypassName, "/@v/"); idx >= 0 { + bypassName = bypassName[:idx] + } + } + packageKey := registry + "/" + bypassName + "@" + version + packageKeyNoVersion := registry + "/" + bypassName bypassedCVEs := make(map[string]*metadata.CVEBypass) - packageKey := registry + "/" + name + "@" + version - packageKeyNoVersion := registry + "/" + name - for _, bypass := range bypasses { if bypass.Type == metadata.BypassTypeCVE && bypass.Active { // Check if bypass applies to this package @@ -110,7 +118,7 @@ func (a *App) handleVulnerabilities(c *fiber.Ctx) error { } } else { // Count non-bypassed vulnerabilities by severity - severityCounts[strings.ToUpper(vuln.Severity)]++ + severityCounts[metadata.NormalizeSeverity(vuln.Severity)]++ } enrichedVuln := map[string]interface{}{ diff --git a/pkg/app/origin_test.go b/pkg/app/origin_test.go new file mode 100644 index 0000000..92c099b --- /dev/null +++ b/pkg/app/origin_test.go @@ -0,0 +1,73 @@ +package app + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" +) + +func reqWithOrigin(origin string) *http.Request { + r, _ := http.NewRequest(http.MethodGet, "/ws", nil) + if origin != "" { + r.Header.Set("Origin", origin) + } + r.Host = "app.example.com" + return r +} + +// TestBuildCheckOrigin_DefaultPortNormalization verifies that scheme-default +// ports (443/80) are ignored, so "https://app.example.com" and +// "https://app.example.com:443" are treated as the same origin. +func TestBuildCheckOrigin_DefaultPortNormalization(t *testing.T) { + check := buildCheckOrigin([]string{"https://app.example.com"}) + + assert.True(t, check(reqWithOrigin("https://app.example.com"))) + assert.True(t, check(reqWithOrigin("https://app.example.com:443"))) + assert.False(t, check(reqWithOrigin("https://app.example.com:8443"))) + + // Default http port strips too. + checkHTTP := buildCheckOrigin([]string{"http://app.example.com"}) + assert.True(t, checkHTTP(reqWithOrigin("http://app.example.com:80"))) + assert.True(t, checkHTTP(reqWithOrigin("http://app.example.com"))) +} + +// TestBuildCheckOrigin_NonDefaultPortPreserved verifies a non-default port is +// not stripped and must match exactly. +func TestBuildCheckOrigin_NonDefaultPortPreserved(t *testing.T) { + check := buildCheckOrigin([]string{"https://app.example.com:8443"}) + + assert.True(t, check(reqWithOrigin("https://app.example.com:8443"))) + assert.False(t, check(reqWithOrigin("https://app.example.com"))) +} + +// TestBuildCheckOrigin_Wildcard verifies wildcard host patterns match the apex +// and subdomains, and are unaffected by ports on the incoming origin. +func TestBuildCheckOrigin_Wildcard(t *testing.T) { + check := buildCheckOrigin([]string{"https://*.example.com"}) + + assert.True(t, check(reqWithOrigin("https://example.com"))) + assert.True(t, check(reqWithOrigin("https://cdn.example.com"))) + assert.True(t, check(reqWithOrigin("https://cdn.example.com:443"))) + assert.False(t, check(reqWithOrigin("https://example.org"))) +} + +// TestBuildCheckOrigin_SchemeMismatch verifies the scheme must match. +func TestBuildCheckOrigin_SchemeMismatch(t *testing.T) { + check := buildCheckOrigin([]string{"https://app.example.com"}) + + assert.False(t, check(reqWithOrigin("http://app.example.com"))) +} + +// TestBuildCheckOrigin_SameOriginFallback verifies that with an empty allowlist +// only a same-origin upgrade (matching Host) is permitted. +func TestBuildCheckOrigin_SameOriginFallback(t *testing.T) { + check := buildCheckOrigin(nil) + + r := reqWithOrigin("https://app.example.com") + assert.True(t, check(r)) + + // Host defaults to app.example.com; a different Origin host is rejected. + r2 := reqWithOrigin("https://other.example.com") + assert.False(t, check(r2)) +} diff --git a/pkg/auth/validation_cache.go b/pkg/auth/validation_cache.go index a0c86d0..ad0886d 100644 --- a/pkg/auth/validation_cache.go +++ b/pkg/auth/validation_cache.go @@ -12,7 +12,17 @@ type ValidationResult struct { Allowed bool } -// ValidationCache caches credential validation results to reduce upstream checks +// ValidationCache caches credential validation results to reduce upstream checks. +// +// Scope: each instance is owned by exactly one proxy handler (goproxy, npm, +// or pypi) and caches only that registry's credential checks. Because the +// instances are never shared across handlers, the second key component may be +// whatever uniquely identifies the resource for that registry: goproxy passes +// a module path, npm and pypi pass a URL. There is no cross-registry key +// collision even though the resource shapes differ, because each handler has +// its own map. Callers must pass the same resource value to every Get/Set for +// a given resource so the cache miss wrapper (Get then Set) is internally +// consistent. type ValidationCache struct { cache map[string]*ValidationResult stopCh chan struct{} @@ -32,22 +42,15 @@ func NewValidationCache(ttl time.Duration) *ValidationCache { // Start cleanup goroutine go vc.cleanupExpired() - return vc } -// Stop terminates the background cleanup goroutine. Safe to call multiple -// times; subsequent calls are no-ops. -func (vc *ValidationCache) Stop() { - vc.stopOnce.Do(func() { - close(vc.stopCh) - }) -} - -// Get retrieves a validation result from cache -// Returns (allowed bool, cached bool, reason string) -func (vc *ValidationCache) Get(credHash, packageURL string) (bool, bool, string) { - key := credHash + ":" + packageURL +// Get retrieves a validation result from cache. +// resource names the registry resource the check was for (a module path for +// goproxy, or a URL for npm/pypi). It is only meaningful within this cache +// instance. Returns (allowed bool, cached bool, reason string). +func (vc *ValidationCache) Get(credHash, resource string) (bool, bool, string) { + key := credHash + ":" + resource // Fast path: read-locked lookup. vc.mu.RLock() @@ -74,12 +77,14 @@ func (vc *ValidationCache) Get(credHash, packageURL string) (bool, bool, string) return false, false, "" } -// Set stores a validation result in cache -func (vc *ValidationCache) Set(credHash, packageURL string, allowed bool, reason string) { +// Set stores a validation result in cache. +// resource is the registry resource the result belongs to; it must match the +// resource value passed to the corresponding Get. +func (vc *ValidationCache) Set(credHash, resource string, allowed bool, reason string) { vc.mu.Lock() defer vc.mu.Unlock() - key := credHash + ":" + packageURL + key := credHash + ":" + resource vc.cache[key] = &ValidationResult{ Allowed: allowed, ExpiresAt: time.Now().Add(vc.ttl), @@ -87,12 +92,12 @@ func (vc *ValidationCache) Set(credHash, packageURL string, allowed bool, reason } } -// Invalidate removes a specific entry from cache -func (vc *ValidationCache) Invalidate(credHash, packageURL string) { +// Invalidate removes a specific entry from cache. +func (vc *ValidationCache) Invalidate(credHash, resource string) { vc.mu.Lock() defer vc.mu.Unlock() - key := credHash + ":" + packageURL + key := credHash + ":" + resource delete(vc.cache, key) } diff --git a/pkg/auth/validator.go b/pkg/auth/validator.go index 8bf041b..b664bf4 100644 --- a/pkg/auth/validator.go +++ b/pkg/auth/validator.go @@ -18,6 +18,9 @@ type CredentialValidator interface { // ValidateAccess checks if credentials grant access to a package // Returns (allowed bool, error) ValidateAccess(ctx context.Context, packageURL string, credentials string) (bool, error) + // Provider returns the upstream auth provider identifier stored on + // packages that require auth (e.g. github, gitlab, npm, pypi). + Provider() string } // NPMValidator validates npm registry credentials @@ -36,6 +39,11 @@ func NewNPMValidator() *NPMValidator { } } +// Provider identifies this validator's upstream provider. +func (v *NPMValidator) Provider() string { + return "npm" +} + // ValidateAccess validates npm package access using HEAD request func (v *NPMValidator) ValidateAccess(ctx context.Context, packageURL string, credentials string) (bool, error) { req, err := http.NewRequestWithContext(ctx, "HEAD", packageURL, nil) @@ -87,6 +95,11 @@ func NewPyPIValidator() *PyPIValidator { } } +// Provider identifies this validator's upstream provider. +func (v *PyPIValidator) Provider() string { + return "pypi" +} + // ValidateAccess validates PyPI package access using HEAD request func (v *PyPIValidator) ValidateAccess(ctx context.Context, packageURL string, credentials string) (bool, error) { req, err := http.NewRequestWithContext(ctx, "HEAD", packageURL, nil) @@ -134,6 +147,26 @@ func NewGoValidator() *GoValidator { } } +// Provider returns the base upstream provider identifier used when the +// registry does not distinguish hosts (e.g. a generic git host). +func (v *GoValidator) Provider() string { + return "git" +} + +// ProviderFor derives the specific provider identifier from a module path. +// Mirrors the host branching in ValidateAccess so the stored AuthProvider +// reflects the actual upstream host (github, gitlab, or a generic git host). +func (v *GoValidator) ProviderFor(modulePath string) string { + switch { + case strings.HasPrefix(modulePath, "github.com/"): + return "github" + case strings.HasPrefix(modulePath, "gitlab.com/"): + return "gitlab" + default: + return "git" + } +} + // ValidateAccess validates Go module access using git ls-remote func (v *GoValidator) ValidateAccess(ctx context.Context, modulePath string, credentials string) (bool, error) { // Create context with timeout diff --git a/pkg/cache/cache.go b/pkg/cache/cache.go index bc0eadc..6cf7e10 100644 --- a/pkg/cache/cache.go +++ b/pkg/cache/cache.go @@ -3,7 +3,6 @@ package cache import ( - "bytes" "context" "crypto/sha256" "fmt" @@ -96,6 +95,27 @@ type CacheEntry struct { FromCache bool } +// FetchResult carries the outcome of an upstream fetch closure. In addition +// to the response body, it communicates auth state so the cache manager can +// persist RequiresAuth/AuthProvider on the stored package. The downstream +// security gate reads entry.Package.RequiresAuth, so this state must be stored +// in the database (not just set on the ephemeral cache entry). +type FetchResult struct { + // Data is the response body. Caller closes it (or Get closes it after store). + Data io.ReadCloser + // UpstreamURL is the upstream URL the body came from. + UpstreamURL string + // AuthProvider identifies the auth system (e.g. github, gitlab, custom). + AuthProvider string + // RequiresAuth is true when the upstream indicated the package is behind + // authentication (e.g. HTTP 401/403, or a private registry that demanded + // credentials). + RequiresAuth bool +} + +// FetchFunc fetches a package from upstream and reports auth state. +type FetchFunc func(context.Context) (*FetchResult, error) + // New creates a new cache manager func New(storage storage.StorageBackend, metadata metadata.MetadataStore, scanner ScannerInterface, analytics AnalyticsInterface, config Config) (*Manager, error) { if storage == nil { @@ -150,10 +170,10 @@ func New(storage storage.StorageBackend, metadata metadata.MetadataStore, scanne go manager.cleanupWorker() return manager, nil + } -// Get retrieves a package from cache or upstream -func (m *Manager) Get(ctx context.Context, registry, name, version string, fetchFunc func(context.Context) (io.ReadCloser, string, error)) (*CacheEntry, error) { +func (m *Manager) Get(ctx context.Context, registry, name, version string, fetchFunc FetchFunc) (*CacheEntry, error) { // Use singleflight to deduplicate concurrent requests key := fmt.Sprintf("%s/%s/%s", registry, name, version) @@ -190,7 +210,7 @@ func (m *Manager) Get(ctx context.Context, registry, name, version string, fetch } // getOrFetch implements the actual get-or-fetch logic -func (m *Manager) getOrFetch(ctx context.Context, registry, name, version string, fetchFunc func(context.Context) (io.ReadCloser, string, error)) (*CacheEntry, error) { +func (m *Manager) getOrFetch(ctx context.Context, registry, name, version string, fetchFunc FetchFunc) (*CacheEntry, error) { // Check metadata first pkg, err := m.metadata.GetPackage(ctx, registry, name, version) if err == nil { @@ -247,8 +267,13 @@ func (m *Manager) getOrFetch(ctx context.Context, registry, name, version string m.trackDownload(registry, name, version, pkg.Size) } - // Check for vulnerabilities if scanner is enabled - if m.scanner != nil { + // Check for vulnerabilities if scanner is enabled. Metadata + // entries (index pages, lists, .mod/.info) are not scannable + // packages, so skip them to avoid a bogus "clean" verdict and + // the extra database round-trip. + isMetadataEntry := version == "list" || version == "page" || version == "latest" || version == "metadata" || + strings.HasSuffix(name, ".mod") || strings.HasSuffix(name, ".info") + if m.scanner != nil && !isMetadataEntry { blocked, reason, vulnErr := m.scanner.CheckVulnerabilities(ctx, registry, name, version) if vulnErr != nil { log.Warn().Err(vulnErr).Str("package", name).Msg("Failed to check vulnerabilities") @@ -293,17 +318,22 @@ func (m *Manager) getOrFetch(ctx context.Context, registry, name, version string log.Debug().Str("package", name).Str("version", version).Msg("Fetching from upstream") // Fetch from upstream - data, upstreamURL, err := fetchFunc(ctx) + fr, err := fetchFunc(ctx) if err != nil { metrics.RecordUpstreamRequest(registry, "error") return nil, errors.Wrap(err, errors.ErrCodeUpstreamFailure, "failed to fetch from upstream") } + if fr == nil || fr.Data == nil { + return nil, errors.New(errors.ErrCodeUpstreamFailure, "upstream fetch returned no data") + } + data := fr.Data defer func() { _ = data.Close() }() // #nosec G104 -- Cleanup, error not critical metrics.RecordUpstreamRequest(registry, "success") - // Store in cache (this will also trigger background scan) - storedPkg, err := m.store(ctx, registry, name, version, data, upstreamURL) + // Store in cache (this will also trigger background scan). Auth state is + // plumbed through so the persisted package carries RequiresAuth. + storedPkg, err := m.store(ctx, registry, name, version, data, fr.UpstreamURL, fr.RequiresAuth, fr.AuthProvider) if err != nil { return nil, err } @@ -412,45 +442,56 @@ func (m *Manager) getOrFetch(ctx context.Context, registry, name, version string // Data is intentionally nil; Get() opens a fresh reader per caller. return &CacheEntry{ Package: storedPkg, - Data: nil, + UpstreamURL: fr.UpstreamURL, FromCache: false, - UpstreamURL: upstreamURL, }, nil } // store stores a package in cache -func (m *Manager) store(ctx context.Context, registry, name, version string, data io.ReadCloser, upstreamURL string) (*metadata.Package, error) { +func (m *Manager) store(ctx context.Context, registry, name, version string, data io.ReadCloser, upstreamURL string, requiresAuth bool, authProvider string) (*metadata.Package, error) { // Generate storage key storageKey := m.generateStorageKey(registry, name, version) - // Calculate checksums while storing - // We need to read the data, calculate checksums, and store it - // This requires buffering the data - var buf []byte - var err error - - // Cap upstream read to MaxPackageSize to prevent OOM on hostile/oversized - // upstream responses. Read one extra byte so we can detect overflow. + // Stream upstream to a temp file while computing checksums, instead of + // buffering the full payload in memory (avoids OOM on large packages, + // e.g. the default 2 GB MaxPackageSize). The temp file also becomes the + // scan source for backends that can't expose a local path, so the payload + // is never held twice in RAM. maxSize := m.config.MaxPackageSize if maxSize <= 0 { maxSize = 2 * 1024 * 1024 * 1024 // 2GB safety floor } - limited := io.LimitReader(data, maxSize+1) - buf, err = io.ReadAll(limited) + + tempFile, err := os.CreateTemp(os.TempDir(), "gohoarder-store-*") if err != nil { + return nil, errors.Wrap(err, errors.ErrCodeUpstreamFailure, "failed to create temp file for upstream data") + } + tempPath := tempFile.Name() + // Temp file is removed when store() returns, unless ownership is handed + // to a scan goroutine (remote backends reuse it for scanning). + scanOwnsTemp := false + defer func() { + if !scanOwnsTemp { + _ = os.Remove(tempPath) // #nosec G104 -- cleanup + } + }() + + // Compute SHA256 as the payload is streamed to the temp file. Cap at + // maxSize+1 so we can detect overflow without reading unbounded data. + hash := sha256.New() + limited := io.LimitReader(data, maxSize+1) + written, err := io.Copy(io.MultiWriter(tempFile, hash), limited) + if err != nil { + _ = tempFile.Close() // #nosec G104 -- cleanup return nil, errors.Wrap(err, errors.ErrCodeUpstreamFailure, "failed to read upstream data") } - if int64(len(buf)) > maxSize { + if written > maxSize { + _ = tempFile.Close() // #nosec G104 -- cleanup return nil, errors.New(errors.ErrCodePayloadTooLarge, fmt.Sprintf("upstream package exceeds max size (%d bytes)", maxSize)) } - - // Calculate checksums - h := sha256.New() - h.Write(buf) - checksumSHA256 := fmt.Sprintf("%x", h.Sum(nil)) - - size := int64(len(buf)) + checksumSHA256 := fmt.Sprintf("%x", hash.Sum(nil)) + size := written // Check quota before storing quota, err := m.storage.GetQuota(ctx) @@ -458,20 +499,28 @@ func (m *Manager) store(ctx context.Context, registry, name, version string, dat if quota.Used+size > quota.Limit { // Trigger eviction if evictErr := m.evict(ctx, size); evictErr != nil { + _ = tempFile.Close() // #nosec G104 -- cleanup return nil, errors.QuotaExceeded(quota.Limit) } } } + // Seek temp file back to start and stream it to the storage backend. + if _, err := tempFile.Seek(0, io.SeekStart); err != nil { + _ = tempFile.Close() // #nosec G104 -- cleanup + return nil, errors.Wrap(err, errors.ErrCodeStorageFailure, "failed to seek temp file") + } + // Store in storage backend opts := &storage.PutOptions{ ChecksumSHA256: checksumSHA256, } - err = m.storage.Put(ctx, storageKey, io.NopCloser(bytes.NewReader(buf)), opts) - if err != nil { + if err := m.storage.Put(ctx, storageKey, tempFile, opts); err != nil { + _ = tempFile.Close() // #nosec G104 -- cleanup return nil, err } + _ = tempFile.Close() // #nosec G104 -- cleanup; scan may still read the file // Create metadata entry now := time.Now() @@ -486,6 +535,8 @@ func (m *Manager) store(ctx context.Context, registry, name, version string, dat Size: size, ChecksumSHA256: checksumSHA256, UpstreamURL: upstreamURL, + RequiresAuth: requiresAuth, + AuthProvider: authProvider, CachedAt: now, LastAccessed: now, ExpiresAt: &expiresAt, @@ -531,6 +582,16 @@ func (m *Manager) store(ctx context.Context, registry, name, version string, dat // Scan package if scanner is enabled (run in background to not block cache operations) // Skip scanning metadata entries (index pages, lists, etc.) if m.scanner != nil && !isMetadataEntry { + // Decide synchronously whether the streaming temp file must outlive + // store(). Remote backends (no GetLocalPath) reuse it as the scan + // source; local backends scan from GetLocalPath and don't need it. + _, hasLocalPath := m.storage.(interface { + GetLocalPath(ctx context.Context, key string) (string, error) + }) + if !hasLocalPath { + scanOwnsTemp = true + } + go func() { scanCtx := context.Background() var filePath string @@ -550,27 +611,11 @@ func (m *Manager) store(ctx context.Context, registry, name, version string, dat cleanupFunc = func() {} // No cleanup needed for direct path log.Debug().Str("package", name).Str("path", filePath).Msg("Scanning package from storage path") } else { - // Fallback: Create temp file for remote storage (S3, SMB, etc.). - // Use os.CreateTemp so the OS picks a safe, unique filename — this - // prevents path traversal via storageKey containing "..". - tempFile, err := os.CreateTemp(os.TempDir(), "gohoarder-scan-*") - if err != nil { - log.Error().Err(err).Str("package", name).Msg("Failed to create temp file for scanning") - return - } - tempFilePath := tempFile.Name() - - // Write package data to temp file - if _, err := tempFile.Write(buf); err != nil { - _ = tempFile.Close() // #nosec G104 -- Cleanup, error not critical - _ = os.Remove(tempFilePath) // #nosec G104 -- Cleanup, error not critical - log.Error().Err(err).Str("package", name).Msg("Failed to write temp file for scanning") - return - } - _ = tempFile.Close() // #nosec G104 -- Cleanup, error not critical - - filePath = tempFilePath - cleanupFunc = func() { _ = os.Remove(tempFilePath) } // #nosec G104 -- Cleanup + // Remote storage (S3, SMB, etc.) — reuse the temp file store() + // already wrote during streaming. This avoids re-uploading the + // payload into another temp file and avoids holding it in RAM. + filePath = tempPath + cleanupFunc = func() { _ = os.Remove(tempPath) } // #nosec G104 -- Cleanup log.Debug().Str("package", name).Str("path", filePath).Msg("Scanning package from temp file") } diff --git a/pkg/cache/cache_test.go b/pkg/cache/cache_test.go index 206479e..8707369 100644 --- a/pkg/cache/cache_test.go +++ b/pkg/cache/cache_test.go @@ -355,7 +355,7 @@ func TestNew(t *testing.T) { func TestGet(t *testing.T) { tests := []struct { setupMock func(*MockStorageBackend, *MockMetadataStore) - fetchFunc func(context.Context) (io.ReadCloser, string, error) + fetchFunc FetchFunc name string registry string packageName string @@ -404,8 +404,8 @@ func TestGet(t *testing.T) { s.On("Get", mock.Anything, "npm/lodash/4.17.21").Return(io.NopCloser(strings.NewReader("upstream data")), nil) m.On("UpdateDownloadCount", mock.Anything, "npm", "lodash", "4.17.21").Return(nil) }, - fetchFunc: func(ctx context.Context) (io.ReadCloser, string, error) { - return io.NopCloser(strings.NewReader("upstream data")), "https://registry.npmjs.org/lodash", nil + fetchFunc: func(ctx context.Context) (*FetchResult, error) { + return &FetchResult{Data: io.NopCloser(strings.NewReader("upstream data")), UpstreamURL: "https://registry.npmjs.org/lodash"}, nil }, wantFromCache: false, wantErr: false, @@ -436,8 +436,8 @@ func TestGet(t *testing.T) { s.On("Get", mock.Anything, "npm/expired-pkg/1.0.0").Return(io.NopCloser(strings.NewReader("refreshed data")), nil) m.On("UpdateDownloadCount", mock.Anything, "npm", "expired-pkg", "1.0.0").Return(nil) }, - fetchFunc: func(ctx context.Context) (io.ReadCloser, string, error) { - return io.NopCloser(strings.NewReader("refreshed data")), "https://registry.npmjs.org/expired-pkg", nil + fetchFunc: func(ctx context.Context) (*FetchResult, error) { + return &FetchResult{Data: io.NopCloser(strings.NewReader("refreshed data")), UpstreamURL: "https://registry.npmjs.org/expired-pkg"}, nil }, wantFromCache: false, wantErr: false, @@ -464,8 +464,8 @@ func TestGet(t *testing.T) { setupMock: func(s *MockStorageBackend, m *MockMetadataStore) { m.On("GetPackage", mock.Anything, "npm", "fail-pkg", "1.0.0").Return(nil, errors.New("not found")) }, - fetchFunc: func(ctx context.Context) (io.ReadCloser, string, error) { - return nil, "", errors.New("upstream error") + fetchFunc: func(ctx context.Context) (*FetchResult, error) { + return nil, errors.New("upstream error") }, wantErr: true, errContains: "failed to fetch from upstream", @@ -498,8 +498,8 @@ func TestGet(t *testing.T) { s.On("Get", mock.Anything, "npm/inconsistent/1.0.0").Return(io.NopCloser(strings.NewReader("recovered data")), nil).Once() m.On("UpdateDownloadCount", mock.Anything, "npm", "inconsistent", "1.0.0").Return(nil) }, - fetchFunc: func(ctx context.Context) (io.ReadCloser, string, error) { - return io.NopCloser(strings.NewReader("recovered data")), "https://registry.npmjs.org/inconsistent", nil + fetchFunc: func(ctx context.Context) (*FetchResult, error) { + return &FetchResult{Data: io.NopCloser(strings.NewReader("recovered data")), UpstreamURL: "https://registry.npmjs.org/inconsistent"}, nil }, wantFromCache: false, wantErr: false, @@ -515,8 +515,8 @@ func TestGet(t *testing.T) { s.On("GetQuota", mock.Anything).Return(&storage.QuotaInfo{Used: 100, Available: 900, Limit: 1000}, nil) s.On("Put", mock.Anything, "npm/save-fail/1.0.0", mock.Anything, mock.Anything).Return(errors.New("storage error")) }, - fetchFunc: func(ctx context.Context) (io.ReadCloser, string, error) { - return io.NopCloser(strings.NewReader("data")), "https://registry.npmjs.org/save-fail", nil + fetchFunc: func(ctx context.Context) (*FetchResult, error) { + return &FetchResult{Data: io.NopCloser(strings.NewReader("data")), UpstreamURL: "https://registry.npmjs.org/save-fail"}, nil }, wantErr: true, errContains: "storage error", @@ -534,8 +534,8 @@ func TestGet(t *testing.T) { m.On("SavePackage", mock.Anything, mock.Anything).Return(errors.New("metadata error")) s.On("Delete", mock.Anything, "npm/meta-fail/1.0.0").Return(nil) }, - fetchFunc: func(ctx context.Context) (io.ReadCloser, string, error) { - return io.NopCloser(strings.NewReader("data")), "https://registry.npmjs.org/meta-fail", nil + fetchFunc: func(ctx context.Context) (*FetchResult, error) { + return &FetchResult{Data: io.NopCloser(strings.NewReader("data")), UpstreamURL: "https://registry.npmjs.org/meta-fail"}, nil }, wantErr: true, errContains: "metadata error", @@ -1105,8 +1105,8 @@ func TestBroadcaster_CacheMissStore(t *testing.T) { bc := &fakeBroadcaster{} manager.SetBroadcaster(bc) - fetch := func(ctx context.Context) (io.ReadCloser, string, error) { - return io.NopCloser(strings.NewReader("upstream data")), "https://registry.npmjs.org/lodash", nil + fetch := func(ctx context.Context) (*FetchResult, error) { + return &FetchResult{Data: io.NopCloser(strings.NewReader("upstream data")), UpstreamURL: "https://registry.npmjs.org/lodash"}, nil } entry, err := manager.Get(context.Background(), "npm", "lodash", "4.17.21", fetch) require.NoError(t, err) @@ -1153,8 +1153,8 @@ func TestBroadcaster_NoEmitOnError(t *testing.T) { bc := &fakeBroadcaster{} manager.SetBroadcaster(bc) - fetch := func(ctx context.Context) (io.ReadCloser, string, error) { - return io.NopCloser(strings.NewReader("data")), "https://registry.npmjs.org/fail", nil + fetch := func(ctx context.Context) (*FetchResult, error) { + return &FetchResult{Data: io.NopCloser(strings.NewReader("data")), UpstreamURL: "https://registry.npmjs.org/fail"}, nil } _, err = manager.Get(context.Background(), "npm", "fail", "1.0.0", fetch) require.Error(t, err) @@ -1178,8 +1178,8 @@ func TestBroadcaster_NoEmitOnError(t *testing.T) { bc := &fakeBroadcaster{} manager.SetBroadcaster(bc) - fetch := func(ctx context.Context) (io.ReadCloser, string, error) { - return io.NopCloser(strings.NewReader("data")), "https://registry.npmjs.org/meta-fail", nil + fetch := func(ctx context.Context) (*FetchResult, error) { + return &FetchResult{Data: io.NopCloser(strings.NewReader("data")), UpstreamURL: "https://registry.npmjs.org/meta-fail"}, nil } _, err = manager.Get(context.Background(), "npm", "meta-fail", "1.0.0", fetch) require.Error(t, err) diff --git a/pkg/config/config.go b/pkg/config/config.go index cd6711e..98535a8 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -362,6 +362,16 @@ func Default() *Config { Security: SecurityConfig{ Enabled: false, BlockOnSeverity: "high", + // Default all severity caps to unlimited (-1) so the operator's + // BlockOnSeverity decision governs by default. A zero cap means + // "block any", which would make a single LOW vulnerability block + // every package even though the configured default is "high". + BlockThresholds: VulnerabilityThresholds{ + Critical: -1, + High: -1, + Medium: -1, + Low: -1, + }, Scanners: ScannersConfig{ Trivy: TrivyConfig{ Enabled: false, diff --git a/pkg/metadata/gormstore/gormstore_v2.go b/pkg/metadata/gormstore/gormstore_v2.go index ed53369..1a99e65 100644 --- a/pkg/metadata/gormstore/gormstore_v2.go +++ b/pkg/metadata/gormstore/gormstore_v2.go @@ -265,8 +265,22 @@ func (s *GORMStoreV2) SavePackage(ctx context.Context, pkg *metadata.Package) er return errors.Wrap(result.Error, errors.ErrCodeStorageFailure, "failed to update package") } - // If no rows were updated, create new record + // If no rows were updated, create new record. + // RowsAffected == 0 here means no LIVE row matches the key, because + // GORM's default scope appends `deleted_at IS NULL` to the UPDATE. + // The only possible match is a soft-deleted row (e.g. after TTL/evict + // called DeletePackage). That retained row still holds the same + // `storage_key`, which has a UNIQUE index, so a plain Create would + // collide (UNIQUE constraint failed: packages.storage_key) and the + // re-fetch would permanently 502. Hard-delete any such stale row + // before creating. if result.RowsAffected == 0 { + if err := tx.Unscoped(). + Where("registry_id = ? AND name = ? AND version = ?", registryID, pkg.Name, pkg.Version). + Delete(&PackageModel{}).Error; err != nil { + return errors.Wrap(err, errors.ErrCodeStorageFailure, "failed to purge stale soft-deleted package before create") + } + if err := tx.Create(model).Error; err != nil { return errors.Wrap(err, errors.ErrCodeStorageFailure, "failed to create package") } diff --git a/pkg/metadata/gormstore/gormstore_v2_test.go b/pkg/metadata/gormstore/gormstore_v2_test.go index 5a58c50..d25716a 100644 --- a/pkg/metadata/gormstore/gormstore_v2_test.go +++ b/pkg/metadata/gormstore/gormstore_v2_test.go @@ -209,6 +209,57 @@ func (s *GORMStoreV2TestSuite) Test_V2_DeletePackage_Success() { s.Equal(int64(1), count) // Still in DB, just soft deleted } +// Test_V2_SavePackage_AfterSoftDelete tests that saving the same +// registry/name/version key after a soft delete (e.g. TTL expiry or +// cache eviction) succeeds instead of colliding on the retained +// storage_key UNIQUE index. Regression for the permanent 502 on +// re-fetch of a once-evicted package. +func (s *GORMStoreV2TestSuite) Test_V2_SavePackage_AfterSoftDelete() { + pkg := &metadata.Package{ + Registry: "npm", + Name: "re-add-test", + Version: "1.0.0", + StorageKey: "npm/re-add-test/1.0.0.tgz", + Size: 1000, + CachedAt: time.Now(), + LastAccessed: time.Now(), + } + err := s.store.SavePackage(s.ctx, pkg) + s.NoError(err) + + // Soft-delete (simulates TTL expiry / eviction) + err = s.store.DeletePackage(s.ctx, "npm", "re-add-test", "1.0.0") + s.NoError(err) + + // Re-save the same key (simulates re-fetch after cache miss). + // Before the fix this failed with: + // UNIQUE constraint failed: packages.storage_key + pkg2 := &metadata.Package{ + Registry: "npm", + Name: "re-add-test", + Version: "1.0.0", + StorageKey: "npm/re-add-test/1.0.0.tgz", + Size: 2000, + CachedAt: time.Now(), + LastAccessed: time.Now(), + } + err = s.store.SavePackage(s.ctx, pkg2) + s.NoError(err) + + // The re-saved package must be retrievable + retrieved, err := s.store.GetPackage(s.ctx, "npm", "re-add-test", "1.0.0") + s.NoError(err) + s.NotNil(retrieved) + s.Equal(int64(2000), retrieved.Size) + + // No duplicate rows retained after the hard purge + var count int64 + s.store.db.Unscoped().Model(&PackageModel{}). + Where("name = ?", "re-add-test"). + Count(&count) + s.Equal(int64(1), count) +} + // Test_V2_ListPackages_All tests listing all packages func (s *GORMStoreV2TestSuite) Test_V2_ListPackages_All() { // Create multiple packages diff --git a/pkg/network/client.go b/pkg/network/client.go index 7e0bc03..2044873 100644 --- a/pkg/network/client.go +++ b/pkg/network/client.go @@ -229,19 +229,12 @@ func (c *Client) do(ctx context.Context, req *http.Request) (*http.Response, err resp, err := c.client.Do(req) if err != nil { lastErr = err - if c.circuitBreaker != nil { - c.circuitBreaker.RecordFailure() - } continue } - // Check if response is retryable if c.isRetryable(resp.StatusCode) { _ = resp.Body.Close() // #nosec G104 -- Cleanup, error not critical lastErr = fmt.Errorf("received retryable status code: %d", resp.StatusCode) - if c.circuitBreaker != nil { - c.circuitBreaker.RecordFailure() - } continue } diff --git a/pkg/network/client_test.go b/pkg/network/client_test.go index 5e0baa8..34724f8 100644 --- a/pkg/network/client_test.go +++ b/pkg/network/client_test.go @@ -405,3 +405,38 @@ func TestConcurrentRequests(t *testing.T) { assert.NoError(t, err) } } + +// TestCircuitBreakerOpens verifies that a configured number of consecutive +// failed requests opens the circuit breaker and that subsequent requests are +// rejected with the circuit-open error until the breaker half-open window +// elapses and a request succeeds again. +func TestCircuitBreakerOpens(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer server.Close() // #nosec G104 -- Cleanup, error not critical + + client := network.NewClient(network.Config{ + Timeout: 5 * time.Second, + MaxRetries: 0, + CircuitBreaker: network.CircuitBreakerConfig{ + Enabled: true, + FailureThreshold: 3, + SuccessThreshold: 1, + Timeout: time.Hour, + }, + }) + + ctx := context.Background() + + // Three consecutive failed requests open the breaker. + for i := 0; i < 3; i++ { + _, _, err := client.Get(ctx, server.URL, nil) + assert.Error(t, err) + } + + // The next request is rejected because the breaker is open. + _, _, err := client.Get(ctx, server.URL, nil) + assert.Error(t, err) + assert.Contains(t, err.Error(), "circuit breaker is open") +} diff --git a/pkg/proxy/goproxy/goproxy.go b/pkg/proxy/goproxy/goproxy.go index 3b4b69e..3bd609d 100644 --- a/pkg/proxy/goproxy/goproxy.go +++ b/pkg/proxy/goproxy/goproxy.go @@ -115,7 +115,7 @@ func (h *Handler) handleList(ctx context.Context, w http.ResponseWriter, r *http // Extract credentials from request credentials := h.credExtractor.Extract(r) - entry, err := h.cache.Get(ctx, "go", modulePath, "list", func(ctx context.Context) (io.ReadCloser, string, error) { + entry, err := h.cache.Get(ctx, "go", modulePath, "list", func(ctx context.Context) (*cache.FetchResult, error) { // Prepare headers for upstream request headers := make(map[string]string) if credentials != "" { @@ -124,13 +124,16 @@ func (h *Handler) handleList(ctx context.Context, w http.ResponseWriter, r *http body, statusCode, err := h.client.Get(ctx, url, headers) if err != nil { - return nil, "", err + return nil, err + } + if statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden { + return &cache.FetchResult{Data: body, UpstreamURL: url, RequiresAuth: true, AuthProvider: h.credValidator.ProviderFor(modulePath)}, nil } if statusCode != http.StatusOK { _ = body.Close() // #nosec G104 -- Cleanup, error not critical - return nil, "", fmt.Errorf("upstream returned status %d", statusCode) + return nil, fmt.Errorf("upstream returned status %d", statusCode) } - return body, url, nil + return &cache.FetchResult{Data: body, UpstreamURL: url}, nil }) if err != nil { @@ -155,7 +158,7 @@ func (h *Handler) handleInfo(ctx context.Context, w http.ResponseWriter, r *http // Extract credentials from request credentials := h.credExtractor.Extract(r) - entry, err := h.cache.Get(ctx, "go", cacheKey, version, func(ctx context.Context) (io.ReadCloser, string, error) { + entry, err := h.cache.Get(ctx, "go", cacheKey, version, func(ctx context.Context) (*cache.FetchResult, error) { // Prepare headers for upstream request headers := make(map[string]string) if credentials != "" { @@ -164,13 +167,16 @@ func (h *Handler) handleInfo(ctx context.Context, w http.ResponseWriter, r *http body, statusCode, err := h.client.Get(ctx, url, headers) if err != nil { - return nil, "", err + return nil, err + } + if statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden { + return &cache.FetchResult{Data: body, UpstreamURL: url, RequiresAuth: true, AuthProvider: h.credValidator.ProviderFor(modulePath)}, nil } if statusCode != http.StatusOK { _ = body.Close() // #nosec G104 -- Cleanup, error not critical - return nil, "", fmt.Errorf("upstream returned status %d", statusCode) + return nil, fmt.Errorf("upstream returned status %d", statusCode) } - return body, url, nil + return &cache.FetchResult{Data: body, UpstreamURL: url}, nil }) if err != nil { @@ -188,14 +194,14 @@ func (h *Handler) handleInfo(ctx context.Context, w http.ResponseWriter, r *http func (h *Handler) handleMod(ctx context.Context, w http.ResponseWriter, r *http.Request, path string) { url := h.upstream + path modulePath := h.extractModulePath(path) + + // Extract credentials from request + credentials := h.credExtractor.Extract(r) version := h.extractVersion(path, ".mod") // Use .mod suffix to distinguish from .info and .zip in cache cacheKey := modulePath + "/@v/" + version + ".mod" - // Extract credentials from request - credentials := h.credExtractor.Extract(r) - - entry, err := h.cache.Get(ctx, "go", cacheKey, version, func(ctx context.Context) (io.ReadCloser, string, error) { + entry, err := h.cache.Get(ctx, "go", cacheKey, version, func(ctx context.Context) (*cache.FetchResult, error) { // Prepare headers for upstream request headers := make(map[string]string) if credentials != "" { @@ -204,13 +210,16 @@ func (h *Handler) handleMod(ctx context.Context, w http.ResponseWriter, r *http. body, statusCode, err := h.client.Get(ctx, url, headers) if err != nil { - return nil, "", err + return nil, err + } + if statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden { + return &cache.FetchResult{Data: body, UpstreamURL: url, RequiresAuth: true, AuthProvider: h.credValidator.ProviderFor(modulePath)}, nil } if statusCode != http.StatusOK { _ = body.Close() // #nosec G104 -- Cleanup, error not critical - return nil, "", fmt.Errorf("upstream returned status %d", statusCode) + return nil, fmt.Errorf("upstream returned status %d", statusCode) } - return body, url, nil + return &cache.FetchResult{Data: body, UpstreamURL: url}, nil }) if err != nil { @@ -245,7 +254,7 @@ func (h *Handler) handleZip(ctx context.Context, w http.ResponseWriter, r *http. Bool("has_credentials", credentials != ""). Msg("Handling Go module zip request") - entry, err := h.cache.Get(ctx, "go", cacheKey, version, func(ctx context.Context) (io.ReadCloser, string, error) { + entry, err := h.cache.Get(ctx, "go", cacheKey, version, func(ctx context.Context) (*cache.FetchResult, error) { // Prepare headers for upstream request headers := make(map[string]string) if credentials != "" { @@ -255,7 +264,7 @@ func (h *Handler) handleZip(ctx context.Context, w http.ResponseWriter, r *http. // Try upstream proxy first (fast path for public modules) body, statusCode, err := h.client.Get(ctx, url, headers) if err == nil && statusCode == http.StatusOK { - return body, url, nil + return &cache.FetchResult{Data: body, UpstreamURL: url}, nil } // If upstream failed with 404 or 403, try git fallback (private modules) @@ -278,9 +287,9 @@ func (h *Handler) handleZip(ctx context.Context, w http.ResponseWriter, r *http. _ = body.Close() // #nosec G104 -- Cleanup, error not critical } if err != nil { - return nil, "", err + return nil, err } - return nil, "", fmt.Errorf("upstream returned status %d", statusCode) + return nil, fmt.Errorf("upstream returned status %d", statusCode) }) if err != nil { @@ -299,6 +308,10 @@ func (h *Handler) handleZip(ctx context.Context, w http.ResponseWriter, r *http. defer func() { _ = entry.Data.Close() }() // #nosec G104 -- Cleanup, error not critical // CRITICAL SECURITY CHECK: If module requires auth, validate credentials + // The auth validation cache is owned by this handler instance only, so its + // key may use the module path (modulePath) rather than a URL. The module path + // uniquely identifies the private module within this registry's cache; there + // is no cross-registry collision because npm/pypi use their own instances. if entry.Package != nil && entry.Package.RequiresAuth { // Check validation cache first allowed, cached, reason := h.validationCache.Get(credHash, modulePath) @@ -352,6 +365,7 @@ func (h *Handler) handleZip(ctx context.Context, w http.ResponseWriter, r *http. w.Header().Set("Content-Type", "application/zip") _, _ = io.Copy(w, entry.Data) // #nosec G104 -- HTTP response write + } // handleLatest handles /@latest requests @@ -362,7 +376,7 @@ func (h *Handler) handleLatest(ctx context.Context, w http.ResponseWriter, r *ht // Extract credentials from request credentials := h.credExtractor.Extract(r) - entry, err := h.cache.Get(ctx, "go", modulePath, "latest", func(ctx context.Context) (io.ReadCloser, string, error) { + entry, err := h.cache.Get(ctx, "go", modulePath, "latest", func(ctx context.Context) (*cache.FetchResult, error) { // Prepare headers for upstream request headers := make(map[string]string) if credentials != "" { @@ -371,13 +385,16 @@ func (h *Handler) handleLatest(ctx context.Context, w http.ResponseWriter, r *ht body, statusCode, err := h.client.Get(ctx, url, headers) if err != nil { - return nil, "", err + return nil, err + } + if statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden { + return &cache.FetchResult{Data: body, UpstreamURL: url, RequiresAuth: true, AuthProvider: h.credValidator.ProviderFor(modulePath)}, nil } if statusCode != http.StatusOK { _ = body.Close() // #nosec G104 -- Cleanup, error not critical - return nil, "", fmt.Errorf("upstream returned status %d", statusCode) + return nil, fmt.Errorf("upstream returned status %d", statusCode) } - return body, url, nil + return &cache.FetchResult{Data: body, UpstreamURL: url}, nil }) if err != nil { @@ -450,7 +467,7 @@ func (h *Handler) extractModulePath(path string) string { } // fetchModuleFromGit fetches a Go module directly from git repository -func (h *Handler) fetchModuleFromGit(ctx context.Context, modulePath, version, credentials string) (io.ReadCloser, string, error) { +func (h *Handler) fetchModuleFromGit(ctx context.Context, modulePath, version, credentials string) (*cache.FetchResult, error) { log.Info(). Str("module", modulePath). Str("version", version). @@ -459,23 +476,25 @@ func (h *Handler) fetchModuleFromGit(ctx context.Context, modulePath, version, c // 1. Fetch module source from git srcPath, err := h.gitFetcher.FetchModule(ctx, modulePath, version, credentials) if err != nil { - return nil, "", fmt.Errorf("git fetch failed: %w", err) + return nil, fmt.Errorf("git fetch failed: %w", err) } defer h.gitFetcher.Cleanup(srcPath) // 2. Validate module if validateErr := h.moduleBuilder.ValidateModule(ctx, srcPath, modulePath); validateErr != nil { - return nil, "", fmt.Errorf("module validation failed: %w", validateErr) + return nil, fmt.Errorf("module validation failed: %w", validateErr) } // 3. Build module zip zipReader, err := h.moduleBuilder.BuildModuleZip(ctx, srcPath, modulePath, version) if err != nil { - return nil, "", fmt.Errorf("module zip build failed: %w", err) + return nil, fmt.Errorf("module zip build failed: %w", err) } - // Create source URL for logging - sourceURL := fmt.Sprintf("git+https://%s@%s", modulePath, version) + // Create source URL for logging. Private modules fetched from git require + // auth, so mark the result accordingly. FIX-L3: strip the git+ scheme so the + // stored UpstreamURL is a fetchable HTTPS URL. + sourceURL := fmt.Sprintf("https://%s@%s", modulePath, version) log.Info(). Str("module", modulePath). @@ -483,5 +502,10 @@ func (h *Handler) fetchModuleFromGit(ctx context.Context, modulePath, version, c Str("source", sourceURL). Msg("Successfully built module from git") - return zipReader, sourceURL, nil + return &cache.FetchResult{ + Data: zipReader, + UpstreamURL: sourceURL, + RequiresAuth: true, + AuthProvider: h.credValidator.ProviderFor(modulePath), + }, nil } diff --git a/pkg/proxy/npm/npm.go b/pkg/proxy/npm/npm.go index 56819a5..b28b15c 100644 --- a/pkg/proxy/npm/npm.go +++ b/pkg/proxy/npm/npm.go @@ -80,16 +80,19 @@ func (h *Handler) handleMetadata(ctx context.Context, w http.ResponseWriter, r * url := h.upstream + path packageName := extractPackageName(path) - entry, err := h.cache.Get(ctx, "npm", packageName, "metadata", func(ctx context.Context) (io.ReadCloser, string, error) { + entry, err := h.cache.Get(ctx, "npm", packageName, "metadata", func(ctx context.Context) (*cache.FetchResult, error) { body, statusCode, fetchErr := h.client.Get(ctx, url, nil) if fetchErr != nil { - return nil, "", fetchErr + return nil, fetchErr + } + if statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden { + return &cache.FetchResult{Data: body, UpstreamURL: url, RequiresAuth: true, AuthProvider: h.credValidator.Provider()}, nil } if statusCode != http.StatusOK { _ = body.Close() - return nil, "", fmt.Errorf("upstream returned status %d", statusCode) + return nil, fmt.Errorf("upstream returned status %d", statusCode) } - return body, url, nil + return &cache.FetchResult{Data: body, UpstreamURL: url}, nil }) if err != nil { @@ -158,9 +161,7 @@ func (h *Handler) handleTarball(ctx context.Context, w http.ResponseWriter, r *h Str("cred_hash", credHash). Bool("has_credentials", credentials != ""). Msg("Handling tarball request") - - // Try to get from cache first (with credential-aware key) - entry, err := h.cache.Get(ctx, "npm", packageName, version, func(ctx context.Context) (io.ReadCloser, string, error) { + entry, err := h.cache.Get(ctx, "npm", packageName, version, func(ctx context.Context) (*cache.FetchResult, error) { // Prepare headers for upstream request headers := make(map[string]string) if credentials != "" { @@ -169,13 +170,16 @@ func (h *Handler) handleTarball(ctx context.Context, w http.ResponseWriter, r *h body, statusCode, fetchErr := h.client.Get(ctx, url, headers) if fetchErr != nil { - return nil, "", fetchErr + return nil, fetchErr + } + if statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden { + return &cache.FetchResult{Data: body, UpstreamURL: url, RequiresAuth: true, AuthProvider: h.credValidator.Provider()}, nil } if statusCode != http.StatusOK { _ = body.Close() - return nil, "", fmt.Errorf("upstream returned status %d", statusCode) + return nil, fmt.Errorf("upstream returned status %d", statusCode) } - return body, url, nil + return &cache.FetchResult{Data: body, UpstreamURL: url}, nil }) if err != nil { @@ -198,6 +202,10 @@ func (h *Handler) handleTarball(ctx context.Context, w http.ResponseWriter, r *h }() // CRITICAL SECURITY CHECK: If package requires auth, validate credentials + // The validation cache is owned by this handler instance only, so the key + // uses the upstream tarball URL. Uniquely identifies the artifact within + // this registry; no cross-registry collision (goproxy/pypi keep their own + // independent instances). if entry.Package != nil && entry.Package.RequiresAuth { // Check validation cache first allowed, cached, reason := h.validationCache.Get(credHash, url) diff --git a/pkg/proxy/pypi/pypi.go b/pkg/proxy/pypi/pypi.go index 7400575..1035204 100644 --- a/pkg/proxy/pypi/pypi.go +++ b/pkg/proxy/pypi/pypi.go @@ -124,16 +124,19 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { func (h *Handler) handleIndex(ctx context.Context, w http.ResponseWriter, r *http.Request) { url := h.upstream + "/" - entry, err := h.cache.Get(ctx, "pypi", "index", "latest", func(ctx context.Context) (io.ReadCloser, string, error) { + entry, err := h.cache.Get(ctx, "pypi", "index", "latest", func(ctx context.Context) (*cache.FetchResult, error) { body, statusCode, err := h.client.Get(ctx, url, nil) if err != nil { - return nil, "", err + return nil, err + } + if statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden { + return &cache.FetchResult{Data: body, UpstreamURL: url, RequiresAuth: true, AuthProvider: h.credValidator.Provider()}, nil } if statusCode != http.StatusOK { _ = body.Close() - return nil, "", fmt.Errorf("upstream returned status %d", statusCode) + return nil, fmt.Errorf("upstream returned status %d", statusCode) } - return body, url, nil + return &cache.FetchResult{Data: body, UpstreamURL: url}, nil }) if err != nil { @@ -155,17 +158,19 @@ func (h *Handler) handleIndex(ctx context.Context, w http.ResponseWriter, r *htt func (h *Handler) handlePackagePage(ctx context.Context, w http.ResponseWriter, r *http.Request, path string) { url := h.upstream + path packageName := extractPackageName(path) - - entry, err := h.cache.Get(ctx, "pypi", packageName, "page", func(ctx context.Context) (io.ReadCloser, string, error) { + entry, err := h.cache.Get(ctx, "pypi", packageName, "page", func(ctx context.Context) (*cache.FetchResult, error) { body, statusCode, err := h.client.Get(ctx, url, nil) if err != nil { - return nil, "", err + return nil, err + } + if statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden { + return &cache.FetchResult{Data: body, UpstreamURL: url, RequiresAuth: true, AuthProvider: h.credValidator.Provider()}, nil } if statusCode != http.StatusOK { _ = body.Close() - return nil, "", fmt.Errorf("upstream returned status %d", statusCode) + return nil, fmt.Errorf("upstream returned status %d", statusCode) } - return body, url, nil + return &cache.FetchResult{Data: body, UpstreamURL: url}, nil }) if err != nil { @@ -199,16 +204,17 @@ func (h *Handler) handlePackagePage(ctx context.Context, w http.ResponseWriter, func (h *Handler) handlePackageFile(ctx context.Context, w http.ResponseWriter, r *http.Request, path string) { packageName, version := extractPackageFileInfo(path) - // Make version unique by appending file type to avoid cache collisions - // between .whl and .metadata files with same version - cacheVersion := version - if strings.HasSuffix(path, ".metadata") { - cacheVersion = version + ".metadata" - } else if strings.HasSuffix(path, ".whl") { - cacheVersion = version + ".whl" - } else if strings.HasSuffix(path, ".tar.gz") { - cacheVersion = version + ".tar.gz" + // cacheVersion is the cache key's version portion. It must be unique per + // distinct artifact. PyPI publishes many files per release (multiple + // wheels with different build tags, sdists, zips, .egg files, and + // .metadata files for PEP 658), so the bare version is too coarse: + // distinct files of the same version would collide on the same cache + // key. The full unique basename guarantees one cache entry per artifact. + filename := path + if i := strings.LastIndex(filename, "/"); i >= 0 { + filename = filename[i+1:] } + cacheVersion := filename // Extract credentials from request credentials := h.credExtractor.Extract(r) @@ -255,7 +261,7 @@ func (h *Handler) handlePackageFile(ctx context.Context, w http.ResponseWriter, Bool("has_credentials", credentials != ""). Msg("Handling PyPI package file request") - entry, err := h.cache.Get(ctx, "pypi", packageName, cacheVersion, func(ctx context.Context) (io.ReadCloser, string, error) { + entry, err := h.cache.Get(ctx, "pypi", packageName, cacheVersion, func(ctx context.Context) (*cache.FetchResult, error) { // Prepare headers for upstream request headers := make(map[string]string) if credentials != "" { @@ -264,13 +270,16 @@ func (h *Handler) handlePackageFile(ctx context.Context, w http.ResponseWriter, body, statusCode, err := h.client.Get(ctx, originalURL, headers) if err != nil { - return nil, "", err + return nil, err + } + if statusCode == http.StatusUnauthorized || statusCode == http.StatusForbidden { + return &cache.FetchResult{Data: body, UpstreamURL: originalURL, RequiresAuth: true, AuthProvider: h.credValidator.Provider()}, nil } if statusCode != http.StatusOK { _ = body.Close() - return nil, "", fmt.Errorf("upstream returned status %d", statusCode) + return nil, fmt.Errorf("upstream returned status %d", statusCode) } - return body, originalURL, nil + return &cache.FetchResult{Data: body, UpstreamURL: originalURL}, nil }) if err != nil { @@ -293,6 +302,10 @@ func (h *Handler) handlePackageFile(ctx context.Context, w http.ResponseWriter, }() // CRITICAL SECURITY CHECK: If package requires auth, validate credentials + // The validation cache is owned by this handler instance only, so the key + // uses the upstream artifact URL (originalURL). Uniquely identifies the + // artifact within this registry; no cross-registry collision (goproxy/npm + // keep their own independent instances). if entry.Package != nil && entry.Package.RequiresAuth { // Check validation cache first allowed, cached, reason := h.validationCache.Get(credHash, originalURL) @@ -395,37 +408,60 @@ func extractPackageName(path string) string { func extractPackageFileInfo(path string) (string, string) { // Format: /package-name/package-name-version.whl // or: /package-name/package-name-version.tar.gz - packageName := extractPackageName(path) - // Extract filename parts := strings.Split(path, "/") if len(parts) < 2 { return packageName, "" } - filename := parts[len(parts)-1] + base := parts[len(parts)-1] + isWheel := strings.HasSuffix(base, ".whl") + base = strings.TrimSuffix(base, ".whl") + base = strings.TrimSuffix(base, ".tar.gz") + base = strings.TrimSuffix(base, ".zip") + base = strings.TrimSuffix(base, ".egg") + base = strings.TrimSuffix(base, ".metadata") - // Remove extension - filename = strings.TrimSuffix(filename, ".whl") - filename = strings.TrimSuffix(filename, ".tar.gz") - filename = strings.TrimSuffix(filename, ".zip") - filename = strings.TrimSuffix(filename, ".egg") + return packageName, extractVersionFromFilename(base, isWheel) +} - // Extract version - // Filename format: package-name-version or package_name-version - // Version typically starts after last dash before build tags - versionParts := strings.Split(filename, "-") - if len(versionParts) >= 2 { - // Simple heuristic: version is the part that starts with a digit - for i := 1; i < len(versionParts); i++ { - if len(versionParts[i]) > 0 && versionParts[i][0] >= '0' && versionParts[i][0] <= '9' { - return packageName, versionParts[i] - } +// extractVersionFromFilename extracts the version from a PyPI distribution +// filename with the extension already stripped. +// +// Wheels (PEP 427) are formatted as +// +// {distribution}-{version}(-{build})?-{python}-{abi}-{platform} +// +// and the distribution is normalized (lowercase, runs of '-', '.', '_' +// collapsed to '_'), so it never contains '-' and the version is always the +// second dash-separated segment. For sdists/zips/eggs the filename is +// {name}-{version}; PyPI normalizes name separators to '_', so the version +// is always the last dash-separated segment (PEP 440 versions never contain +// '-'). +// +// The historical first-digit-segment heuristic is avoided because it mangles +// digit-embedded distribution names (e.g. "2captcha-1.0.0" → "2captcha") and +// intermediate digit segments (e.g. "foo-2019bar-1.0" → "2019bar"). The last +// digit-starting segment is correct for those cases. +func extractVersionFromFilename(base string, isWheel bool) string { + if isWheel { + // The distribution never contains '-', so the version immediately + // follows the first '-'. + if parts := strings.SplitN(base, "-", 3); len(parts) >= 2 && parts[1] != "" && parts[1][0] >= '0' && parts[1][0] <= '9' { + return parts[1] } } - return packageName, filename + // Non-wheel fallback (and defensive wheel edge case): the last + // dash-separated segment that starts with a digit. + parts := strings.Split(base, "-") + for i := len(parts) - 1; i >= 1; i-- { + if len(parts[i]) > 0 && parts[i][0] >= '0' && parts[i][0] <= '9' { + return parts[i] + } + } + return base } // getProxyBaseURL constructs the proxy base URL from the request diff --git a/pkg/proxy/pypi/pypi_test.go b/pkg/proxy/pypi/pypi_test.go index c16860a..8d8d5e9 100644 --- a/pkg/proxy/pypi/pypi_test.go +++ b/pkg/proxy/pypi/pypi_test.go @@ -4,6 +4,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" "github.com/lukaszraczylo/gohoarder/pkg/auth" @@ -137,3 +138,72 @@ func indexOf(s, sub string) int { } return -1 } + +func TestExtractPackageFileInfo(t *testing.T) { + tests := []struct { + name string + path string + wantName string + wantVer string + }{ + {"numpy wheel", "/numpy/numpy-1.26.0rc1-cp312-cp312-manylinux.whl", "numpy", "1.26.0rc1"}, + {"underscore wheel name", "/typing_extensions/typing_extensions-4.8.0-py3-none-any.whl", "typing_extensions", "4.8.0"}, + {"hyphenated sdist", "/my-pkg/my-pkg-2.0.1.tar.gz", "my-pkg", "2.0.1"}, + {"hyphen name underscore file", "/typed-ast/typed_ast-1.5.4.tar.gz", "typed-ast", "1.5.4"}, + {"digit-embedded name", "/2captcha/2captcha-1.0.0.tar.gz", "2captcha", "1.0.0"}, + {"digit intermediate segment", "/foo/foo-2019bar-1.0.zip", "foo", "1.0"}, + {"egg file", "/foo/foo-1.0.egg", "foo", "1.0"}, + {"metadata file", "/foo/foo-1.0.metadata", "foo", "1.0"}, + {"no version", "/foo", "foo", "foo"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotName, gotVer := extractPackageFileInfo(tt.path) + if gotName != tt.wantName { + t.Errorf("extractPackageFileInfo(%q) name = %q, want %q", tt.path, gotName, tt.wantName) + } + if gotVer != tt.wantVer { + t.Errorf("extractPackageFileInfo(%q) version = %q, want %q", tt.path, gotVer, tt.wantVer) + } + }) + } +} + +// TestCacheVersionUniquePerArtifact asserts that distinct PyPI artifacts of +// the same logical version produce distinct cache keys, and that the bare +// version returned for logging stays stable. +func TestCacheVersionUniquePerArtifact(t *testing.T) { + // Three wheels of the same version differing only in build/tag, plus a + // .zip and a .metadata file for the same version. The cache key uses the + // full unique basename, so keys must all differ; the extracted version + // must remain the bare stable version for logging/bypass logic. + paths := []string{ + "/foo/foo-1.0.0-cp312-cp312-manylinux.whl", + "/foo/foo-1.0.0-cp311-cp311-manylinux.whl", + "/foo/foo-1.0.0-py3-none-any.whl", + "/foo/foo-1.0.0.zip", + "/foo/foo-1.0.0.metadata", + } + + seenKeys := make(map[string]bool) + for _, path := range paths { + _, ver := extractPackageFileInfo(path) + if ver != "1.0.0" { + t.Errorf("extractPackageFileInfo(%q) version = %q, want stable %q", path, ver, "1.0.0") + } + + filename := path + if i := strings.LastIndex(filename, "/"); i >= 0 { + filename = filename[i+1:] + } + if seenKeys[filename] { + t.Errorf("cache key %q duplicated across artifacts", filename) + } + seenKeys[filename] = true + } + + if len(seenKeys) != len(paths) { + t.Errorf("expected %d distinct cache keys, got %d", len(paths), len(seenKeys)) + } +} diff --git a/pkg/scanner/scanner.go b/pkg/scanner/scanner.go index ae7b6d7..3529c48 100644 --- a/pkg/scanner/scanner.go +++ b/pkg/scanner/scanner.go @@ -377,7 +377,7 @@ func (m *Manager) mergeResults(results []*metadata.ScanResult, scannerNames []st // Convert map to slice and count severities for _, vuln := range vulnMap { merged.Vulnerabilities = append(merged.Vulnerabilities, *vuln) - severityCounts[strings.ToUpper(vuln.Severity)]++ + severityCounts[metadata.NormalizeSeverity(vuln.Severity)]++ } // Update counts @@ -409,6 +409,21 @@ func (m *Manager) compareSeverity(s1, s2 string) int { return v1 - v2 } +// normalizeBypassPackageName reduces a stored package cache key to the +// natural registry/package identifier used in admin bypass targets. +// Go module packages are stored with a cache key of +// "modulePath/@v/version.zip" (and .info/.mod variants); the admin-facing +// target uses the bare module path, e.g. "go/github.com/foo/bar@v1.2.3". +// npm/pypi names are stored unchanged, so this is a no-op for them. +func normalizeBypassPackageName(registry, name string) string { + if registry == "go" { + if idx := strings.Index(name, "/@v/"); idx >= 0 { + return name[:idx] + } + } + return name +} + // CheckVulnerabilities checks if a package exceeds vulnerability thresholds func (m *Manager) CheckVulnerabilities(ctx context.Context, registry, packageName, version string) (bool, string, error) { if !m.enabled { @@ -423,8 +438,9 @@ func (m *Manager) CheckVulnerabilities(ctx context.Context, registry, packageNam } // Check if entire package is bypassed - packageKey := fmt.Sprintf("%s/%s@%s", registry, packageName, version) - packageKeyNoVersion := fmt.Sprintf("%s/%s", registry, packageName) + bypassName := normalizeBypassPackageName(registry, packageName) + packageKey := fmt.Sprintf("%s/%s@%s", registry, bypassName, version) + packageKeyNoVersion := fmt.Sprintf("%s/%s", registry, bypassName) for _, bypass := range bypasses { if bypass.Type == metadata.BypassTypePackage && bypass.Active { @@ -499,7 +515,7 @@ func (m *Manager) CheckVulnerabilities(ctx context.Context, registry, packageNam Msg("CVE bypassed by admin") continue } - severityCounts[strings.ToUpper(vuln.Severity)]++ + severityCounts[metadata.NormalizeSeverity(vuln.Severity)]++ } // Check against thresholds diff --git a/pkg/scanner/scanner_test.go b/pkg/scanner/scanner_test.go index d3cbb53..9cd3e6a 100644 --- a/pkg/scanner/scanner_test.go +++ b/pkg/scanner/scanner_test.go @@ -254,3 +254,79 @@ func TestBroadcaster_NilBroadcasterSafe(t *testing.T) { err := mgr.ScanPackage(context.Background(), "npm", "x", "1", "/tmp/dummy") require.NoError(t, err) } + +// stubBypassStore returns a package bypass and a clean scan result for a +// Go cache-key package, to exercise FIX-H4 bypass-name normalization. +type stubBypassStore struct { + *stubMetadataStore + scan *metadata.ScanResult + bypasses []*metadata.CVEBypass +} + +func (m *stubBypassStore) GetActiveCVEBypasses(context.Context) ([]*metadata.CVEBypass, error) { + return m.bypasses, nil +} + +func (m *stubBypassStore) GetScanResult(context.Context, string, string, string) (*metadata.ScanResult, error) { + return m.scan, nil +} + +// TestCheckVulnerabilities_GoPackageBypassNaturalTarget verifies a package +// bypass targeting the natural Go module path matches a package stored under +// its cache-key name (modulePath/@v/version.zip). +func TestCheckVulnerabilities_GoPackageBypassNaturalTarget(t *testing.T) { + store := &stubBypassStore{ + stubMetadataStore: &stubMetadataStore{}, + bypasses: []*metadata.CVEBypass{ + { + ID: "b1", + Type: metadata.BypassTypePackage, + Target: "go/github.com/foo/bar@v1.2.3", + Active: true, + }, + }, + scan: &metadata.ScanResult{ + ID: "r", Scanner: "stub", ScannedAt: time.Now(), Status: metadata.ScanStatusClean, + }, + } + mgr := newTestManager(t, store) + + // The package is stored under the cache-key name; the version is passed + // separately. The bypass target uses the natural module path. + blocked, reason, err := mgr.CheckVulnerabilities( + context.Background(), "go", "github.com/foo/bar/@v/v1.2.3.zip", "v1.2.3") + require.NoError(t, err) + assert.False(t, blocked, "package bypass should unblock the Go package") + assert.Equal(t, "", reason) +} + +// TestCheckVulnerabilities_GoPackageBypassNotFound verifies a package bypass +// targeting a different module does NOT unblock the Go package. Its stored +// scan result is clean, so CheckVulnerabilities returns not-blocked anyway; +// to make the package-block path observable we use a scan result with a +// high-severity vulnerability and zero thresholds (block any non-zero). +func TestCheckVulnerabilities_GoPackageBypassNotFound(t *testing.T) { + store := &stubBypassStore{ + stubMetadataStore: &stubMetadataStore{}, + bypasses: []*metadata.CVEBypass{ + { + ID: "b1", + Type: metadata.BypassTypePackage, + Target: "go/github.com/other/mod@v1.0.0", + Active: true, + }, + }, + scan: &metadata.ScanResult{ + ID: "r", Scanner: "stub", ScannedAt: time.Now(), Status: metadata.ScanStatusVulnerable, + Vulnerabilities: []metadata.Vulnerability{ + {ID: "CVE-1", Severity: "HIGH"}, + }, + }, + } + mgr := newTestManager(t, store) + + blocked, _, err := mgr.CheckVulnerabilities( + context.Background(), "go", "github.com/foo/bar/@v/v1.2.3.zip", "v1.2.3") + require.NoError(t, err) + assert.True(t, blocked, "a non-matching bypass should not unblock the package") +}