From f90fa0e6a5e45967be7e066ade7a83f4efa89b2a Mon Sep 17 00:00:00 2001 From: Sahil Bansal Date: Tue, 3 Feb 2026 13:17:07 +0530 Subject: [PATCH] Generalise Proxy Mode Registry Config (#149) * refactor: generalise registry config * add test cases & add defensive check for domain match * fix linter --- proxy/interceptors/npm_registry.go | 41 ++-- proxy/interceptors/npm_registry_config.go | 32 --- proxy/interceptors/npm_url_parser.go | 95 ++++++--- proxy/interceptors/npm_url_parser_test.go | 22 +- proxy/interceptors/registry_config.go | 73 +++++++ proxy/interceptors/registry_config_test.go | 234 +++++++++++++++++++++ 6 files changed, 397 insertions(+), 100 deletions(-) delete mode 100644 proxy/interceptors/npm_registry_config.go create mode 100644 proxy/interceptors/registry_config.go create mode 100644 proxy/interceptors/registry_config_test.go diff --git a/proxy/interceptors/npm_registry.go b/proxy/interceptors/npm_registry.go index 01e7d95..addf73f 100644 --- a/proxy/interceptors/npm_registry.go +++ b/proxy/interceptors/npm_registry.go @@ -1,34 +1,32 @@ package interceptors import ( - "strings" - packagev1 "buf.build/gen/go/safedep/api/protocolbuffers/go/safedep/messages/package/v1" "github.com/safedep/dry/log" "github.com/safedep/pmg/analyzer" "github.com/safedep/pmg/proxy" ) -var npmRegistryDomains = map[string]*npmRegistryConfig{ +var npmRegistryDomains = registryConfigMap{ "registry.npmjs.org": { Host: "registry.npmjs.org", SupportedForAnalysis: true, - RegistryParser: npmParser{}, + Parser: npmParser{}, }, "registry.yarnpkg.com": { Host: "registry.yarnpkg.com", SupportedForAnalysis: true, - RegistryParser: npmParser{}, + Parser: npmParser{}, }, "npm.pkg.github.com": { Host: "npm.pkg.github.com", SupportedForAnalysis: false, // Skip analysis for now (private packages, auth complexity) - RegistryParser: githubParser{}, + Parser: npmGithubParser{}, }, "pkg-npm.githubusercontent.com": { Host: "pkg-npm.githubusercontent.com", SupportedForAnalysis: false, // Skip analysis (blob storage, redirected downloads) - RegistryParser: githubBlobParser{}, + Parser: npmGithubBlobParser{}, }, } @@ -64,18 +62,7 @@ func (i *NpmRegistryInterceptor) Name() string { // ShouldIntercept determines if this interceptor should handle the given request func (i *NpmRegistryInterceptor) ShouldIntercept(ctx *proxy.RequestContext) bool { - if _, exists := npmRegistryDomains[ctx.Hostname]; exists { - return true - } - - // Check subdomain match - for domain := range npmRegistryDomains { - if strings.HasSuffix(ctx.Hostname, "."+domain) { - return true - } - } - - return false + return npmRegistryDomains.ContainsHostname(ctx.Hostname) } // HandleRequest processes the request and returns response action @@ -84,7 +71,7 @@ func (i *NpmRegistryInterceptor) HandleRequest(ctx *proxy.RequestContext) (*prox log.Debugf("[%s] Handling NPM registry request: %s", ctx.RequestID, ctx.URL.Path) // Get registry configuration - config := getNpmRegistryConfigForHostname(ctx.Hostname) + config := npmRegistryDomains.GetConfigForHostname(ctx.Hostname) if config == nil { // Shouldn't happen if ShouldIntercept is working correctly log.Warnf("[%s] No registry config found for hostname: %s", ctx.RequestID, ctx.Hostname) @@ -99,7 +86,7 @@ func (i *NpmRegistryInterceptor) HandleRequest(ctx *proxy.RequestContext) (*prox } // Parse URL using registry-specific strategy - pkgInfo, err := config.RegistryParser.ParseURL(ctx.URL.Path) + pkgInfo, err := config.Parser.ParseURL(ctx.URL.Path) if err != nil { log.Warnf("[%s] Failed to parse NPM registry URL %s for %s: %v", ctx.RequestID, ctx.URL.Path, config.Host, err) @@ -108,21 +95,21 @@ func (i *NpmRegistryInterceptor) HandleRequest(ctx *proxy.RequestContext) (*prox // Only analyze tarball downloads (these have a specific version) // Metadata requests (without version) are allowed through - if !pkgInfo.IsTarball { - log.Debugf("[%s] Skipping analysis for metadata request: %s", ctx.RequestID, pkgInfo.Name) + if !pkgInfo.IsFileDownload() { + log.Debugf("[%s] Skipping analysis for metadata request: %s", ctx.RequestID, pkgInfo.GetName()) return &proxy.InterceptorResponse{Action: proxy.ActionAllow}, nil } result, err := i.baseRegistryInterceptor.analyzePackage( ctx, packagev1.Ecosystem_ECOSYSTEM_NPM, - pkgInfo.Name, - pkgInfo.Version, + pkgInfo.GetName(), + pkgInfo.GetVersion(), ) if err != nil { - log.Errorf("[%s] Failed to analyze package %s@%s: %v", ctx.RequestID, pkgInfo.Name, pkgInfo.Version, err) + log.Errorf("[%s] Failed to analyze package %s@%s: %v", ctx.RequestID, pkgInfo.GetName(), pkgInfo.GetVersion(), err) return &proxy.InterceptorResponse{Action: proxy.ActionAllow}, nil } - return i.baseRegistryInterceptor.handleAnalysisResult(ctx, packagev1.Ecosystem_ECOSYSTEM_NPM, pkgInfo.Name, pkgInfo.Version, result) + return i.handleAnalysisResult(ctx, packagev1.Ecosystem_ECOSYSTEM_NPM, pkgInfo.GetName(), pkgInfo.GetVersion(), result) } diff --git a/proxy/interceptors/npm_registry_config.go b/proxy/interceptors/npm_registry_config.go deleted file mode 100644 index 3a9e1da..0000000 --- a/proxy/interceptors/npm_registry_config.go +++ /dev/null @@ -1,32 +0,0 @@ -package interceptors - -import "strings" - -// npmRegistryConfig defines configuration for npm registry endpoints -type npmRegistryConfig struct { - // Hostname - Host string - - // Whether this registry is supported for malware analysis - SupportedForAnalysis bool - - // Parser for the registry - RegistryParser npmRegistryURLParser -} - -// getNpmRegistryConfigForHostname returns the configuration for a hostname (with subdomain matching) -func getNpmRegistryConfigForHostname(hostname string) *npmRegistryConfig { - // Check exact match first - if config, exists := npmRegistryDomains[hostname]; exists { - return config - } - - // Check subdomain match: hostname could be "cdn.registry.npmjs.org" matching "registry.npmjs.org" - for endpoint, config := range npmRegistryDomains { - if strings.HasSuffix(hostname, "."+endpoint) { - return config - } - } - - return nil -} diff --git a/proxy/interceptors/npm_url_parser.go b/proxy/interceptors/npm_url_parser.go index cfe8d0d..013bcee 100644 --- a/proxy/interceptors/npm_url_parser.go +++ b/proxy/interceptors/npm_url_parser.go @@ -7,30 +7,51 @@ import ( // npmPackageInfo represents parsed package information from an NPM registry URL type npmPackageInfo struct { - Name string - Version string - IsTarball bool - IsScoped bool + name string + version string + isTarball bool + isScoped bool } -// npmRegistryURLParser defines the interface for parsing registry-specific URLs -type npmRegistryURLParser interface { - ParseURL(urlPath string) (*npmPackageInfo, error) +// Ensure npmPackageInfo implements packageInfo interface +var _ packageInfo = (*npmPackageInfo)(nil) + +// GetName returns the package name +func (n *npmPackageInfo) GetName() string { + return n.name } +// GetVersion returns the package version +func (n *npmPackageInfo) GetVersion() string { + return n.version +} + +// IsFileDownload returns true if this is a tarball download +func (n *npmPackageInfo) IsFileDownload() bool { + return n.isTarball +} + +// IsScoped returns true if this is a scoped package (@scope/name) +func (n *npmPackageInfo) IsScoped() bool { + return n.isScoped +} + +// npmParser parses standard NPM registry URL paths (registry.npmjs.org, registry.yarnpkg.com) type npmParser struct{} -// parseNpmRegistryURL parses standard NPM registry URL paths (registry.npmjs.org, registry.yarnpkg.com) -// This function handles the standard npm registry URL format. +// Ensure npmParser implements RegistryURLParser interface +var _ registryURLParser = npmParser{} + +// ParseURL parses standard NPM registry URL paths // // Supported URL patterns: -// - /package -> {Name: "package", Version: ""} -// - /package/1.0.0 -> {Name: "package", Version: "1.0.0"} -// - /@scope/package -> {Name: "@scope/package", Version: "", IsScoped: true} -// - /@scope/package/1.0.0 -> {Name: "@scope/package", Version: "1.0.0", IsScoped: true} -// - /package/-/package-1.0.0.tgz -> {Name: "package", Version: "1.0.0", IsTarball: true} -// - /@scope/package/-/@scope-package-1.0.0.tgz -> {Name: "@scope/package", Version: "1.0.0", IsTarball: true, IsScoped: true} -func (n npmParser) ParseURL(urlPath string) (*npmPackageInfo, error) { +// - /package -> {name: "package", version: ""} +// - /package/1.0.0 -> {name: "package", version: "1.0.0"} +// - /@scope/package -> {name: "@scope/package", version: "", isScoped: true} +// - /@scope/package/1.0.0 -> {name: "@scope/package", version: "1.0.0", isScoped: true} +// - /package/-/package-1.0.0.tgz -> {name: "package", version: "1.0.0", isTarball: true} +// - /@scope/package/-/@scope-package-1.0.0.tgz -> {name: "@scope/package", version: "1.0.0", isTarball: true, isScoped: true} +func (n npmParser) ParseURL(urlPath string) (packageInfo, error) { // Remove leading and trailing slashes urlPath = strings.Trim(urlPath, "/") @@ -51,29 +72,37 @@ func (n npmParser) ParseURL(urlPath string) (*npmPackageInfo, error) { return parseUnscopedPackageURL(segments) } -type githubParser struct{} +// npmGithubParser parses GitHub npm registry URLs +type npmGithubParser struct{} + +// Ensure npmGithubParser implements RegistryURLParser interface +var _ registryURLParser = npmGithubParser{} // ParseURL implements RegistryURLParser for GitHub npm registry -func (g githubParser) ParseURL(urlPath string) (*npmPackageInfo, error) { +func (g npmGithubParser) ParseURL(urlPath string) (packageInfo, error) { // For now, just allow all GitHub npm registry requests through without analysis // TODO: Implement proper GitHub npm registry URL parsing when analysis is enabled // GitHub URLs follow patterns: - // - /download/@owner/package/version/hash.tgz -> {Name: "package", Version: "1.0.0", IsTarball: true} + // - /download/@owner/package/version/hash.tgz -> {name: "package", version: "1.0.0", isTarball: true} // - /@owner/package (metadata requests) return &npmPackageInfo{ - IsTarball: false, // Mark as non-tarball to skip analysis + isTarball: false, // Mark as non-tarball to skip analysis }, nil } -type githubBlobParser struct{} +// npmGithubBlobParser parses GitHub blob storage URLs +type npmGithubBlobParser struct{} + +// Ensure npmGithubBlobParser implements RegistryURLParser interface +var _ registryURLParser = npmGithubBlobParser{} // ParseURL implements RegistryURLParser for GitHub blob storage -func (g githubBlobParser) ParseURL(urlPath string) (*npmPackageInfo, error) { +func (g npmGithubBlobParser) ParseURL(urlPath string) (packageInfo, error) { // For now, just allow all GitHub blob storage requests through without analysis // TODO: Implement proper GitHub blob storage URL parsing when analysis is enabled // Pattern: /npmregistryv2prod/blobs/{blob_id}/{package_name}/{version}/*** return &npmPackageInfo{ - IsTarball: false, // Mark as non-tarball to skip analysis + isTarball: false, // Mark as non-tarball to skip analysis }, nil } @@ -92,8 +121,8 @@ func parseScopedPackageURL(segments []string) (*npmPackageInfo, error) { fullName := scope + "/" + packageName info := &npmPackageInfo{ - Name: fullName, - IsScoped: true, + name: fullName, + isScoped: true, } // Just the scoped package name: /@scope/package @@ -112,14 +141,14 @@ func parseScopedPackageURL(segments []string) (*npmPackageInfo, error) { return nil, fmt.Errorf("failed to extract version from tarball %s: %w", tarballName, err) } - info.Version = version - info.IsTarball = true + info.version = version + info.isTarball = true return info, nil } // Version metadata: /@scope/package/1.0.0 if len(segments) == 3 { - info.Version = segments[2] + info.version = segments[2] return info, nil } @@ -139,8 +168,8 @@ func parseUnscopedPackageURL(segments []string) (*npmPackageInfo, error) { packageName := segments[0] info := &npmPackageInfo{ - Name: packageName, - IsScoped: false, + name: packageName, + isScoped: false, } // Just the package name: /package @@ -159,14 +188,14 @@ func parseUnscopedPackageURL(segments []string) (*npmPackageInfo, error) { return nil, fmt.Errorf("failed to extract version from tarball %s: %w", tarballName, err) } - info.Version = version - info.IsTarball = true + info.version = version + info.isTarball = true return info, nil } // Version metadata: /package/1.0.0 if len(segments) == 2 { - info.Version = segments[1] + info.version = segments[1] return info, nil } diff --git a/proxy/interceptors/npm_url_parser_test.go b/proxy/interceptors/npm_url_parser_test.go index 1109765..bca9974 100644 --- a/proxy/interceptors/npm_url_parser_test.go +++ b/proxy/interceptors/npm_url_parser_test.go @@ -274,10 +274,16 @@ func TestParseNpmRegistryURL(t *testing.T) { } assert.NoError(t, err) - assert.Equal(t, tt.wantName, got.Name) - assert.Equal(t, tt.wantVersion, got.Version) - assert.Equal(t, tt.wantIsTarball, got.IsTarball) - assert.Equal(t, tt.wantIsScoped, got.IsScoped) + assert.Equal(t, tt.wantName, got.GetName()) + assert.Equal(t, tt.wantVersion, got.GetVersion()) + assert.Equal(t, tt.wantIsTarball, got.IsFileDownload()) + + // Check scoped status via type assertion - must succeed for npm packages + npmInfo, ok := got.(*npmPackageInfo) + assert.True(t, ok, "expected *npmPackageInfo type") + if ok { + assert.Equal(t, tt.wantIsScoped, npmInfo.IsScoped()) + } }) } } @@ -305,7 +311,7 @@ func TestGithubParser_ParseURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - parser := githubParser{} + parser := npmGithubParser{} got, err := parser.ParseURL(tt.urlPath) if tt.wantErr { @@ -314,7 +320,7 @@ func TestGithubParser_ParseURL(t *testing.T) { } assert.NoError(t, err) - assert.Equal(t, tt.wantIsTarball, got.IsTarball) + assert.Equal(t, tt.wantIsTarball, got.IsFileDownload()) }) } } @@ -336,7 +342,7 @@ func TestGithubBlobParser_ParseURL(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - parser := githubBlobParser{} + parser := npmGithubBlobParser{} got, err := parser.ParseURL(tt.urlPath) if tt.wantErr { @@ -345,7 +351,7 @@ func TestGithubBlobParser_ParseURL(t *testing.T) { } assert.NoError(t, err) - assert.Equal(t, tt.wantIsTarball, got.IsTarball) + assert.Equal(t, tt.wantIsTarball, got.IsFileDownload()) }) } } diff --git a/proxy/interceptors/registry_config.go b/proxy/interceptors/registry_config.go new file mode 100644 index 0000000..fa98b06 --- /dev/null +++ b/proxy/interceptors/registry_config.go @@ -0,0 +1,73 @@ +package interceptors + +import "strings" + +// packageInfo represents parsed package information from a registry URL. +// All ecosystem-specific package info types must implement this interface. +type packageInfo interface { + // GetName returns the package name + GetName() string + + // GetVersion returns the package version (may be empty for metadata requests) + GetVersion() string + + // IsFileDownload returns true if this is a file download request (tarball, wheel, etc.) + // Returns false for metadata requests (package index, version info, etc.) + IsFileDownload() bool +} + +// registryURLParser parses registry-specific URLs to extract package information. +// Each registry (npm, pypi, etc.) implements this interface with its own URL parsing logic. +type registryURLParser interface { + // ParseURL parses a URL path and returns package information. + // Returns an error if the URL cannot be parsed. + ParseURL(urlPath string) (packageInfo, error) +} + +// registryConfig defines configuration for a package registry endpoint. +// This is the common configuration structure used by all ecosystem interceptors. +type registryConfig struct { + // Host is the hostname of the registry + Host string + + // SupportedForAnalysis indicates whether this registry supports malware analysis. + // Some registries (like private registries or test instances) may not support analysis. + SupportedForAnalysis bool + + // Parser is the URL parser for this registry + Parser registryURLParser +} + +// registryConfigMap is a map of hostname to registry configuration +type registryConfigMap map[string]*registryConfig + +// GetConfigForHostname returns the configuration for a hostname with subdomain matching support. +// It first checks for an exact match, then checks if the hostname is a subdomain of any configured registry. +func (m registryConfigMap) GetConfigForHostname(hostname string) *registryConfig { + // Check exact match first + if config, exists := m[hostname]; exists { + return config + } + + // Check subdomain match: hostname could be "cdn.registry.example.org" matching "registry.example.org". + // Defensive: Since Go map iteration order is non-deterministic, if multiple endpoints could match + // (e.g., both "example.org" and "registry.example.org"), we select the longest (most specific) one + // to ensure consistent behavior. In practice, our configured endpoints don't overlap. + var bestConfig *registryConfig + bestLen := 0 + for endpoint, config := range m { + if strings.HasSuffix(hostname, "."+endpoint) { + if len(endpoint) > bestLen { + bestLen = len(endpoint) + bestConfig = config + } + } + } + + return bestConfig +} + +// ContainsHostname checks if the hostname matches any configured registry (exact or subdomain match) +func (m registryConfigMap) ContainsHostname(hostname string) bool { + return m.GetConfigForHostname(hostname) != nil +} diff --git a/proxy/interceptors/registry_config_test.go b/proxy/interceptors/registry_config_test.go new file mode 100644 index 0000000..6d62167 --- /dev/null +++ b/proxy/interceptors/registry_config_test.go @@ -0,0 +1,234 @@ +package interceptors + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// mockParser is a simple parser for testing +type mockParser struct{} + +func (m mockParser) ParseURL(urlPath string) (packageInfo, error) { + return nil, nil +} + +func TestRegistryConfigMap_GetConfigForHostname_ExactMatch(t *testing.T) { + configMap := registryConfigMap{ + "registry.example.org": { + Host: "registry.example.org", + SupportedForAnalysis: true, + Parser: mockParser{}, + }, + "other.example.org": { + Host: "other.example.org", + SupportedForAnalysis: false, + Parser: mockParser{}, + }, + } + + tests := []struct { + name string + hostname string + wantHost string + wantExists bool + }{ + { + name: "exact match first registry", + hostname: "registry.example.org", + wantHost: "registry.example.org", + wantExists: true, + }, + { + name: "exact match second registry", + hostname: "other.example.org", + wantHost: "other.example.org", + wantExists: true, + }, + { + name: "no match", + hostname: "unknown.example.org", + wantHost: "", + wantExists: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config := configMap.GetConfigForHostname(tt.hostname) + if !tt.wantExists { + assert.Nil(t, config) + return + } + assert.NotNil(t, config) + assert.Equal(t, tt.wantHost, config.Host) + }) + } +} + +func TestRegistryConfigMap_GetConfigForHostname_SubdomainMatch(t *testing.T) { + configMap := registryConfigMap{ + "registry.example.org": { + Host: "registry.example.org", + SupportedForAnalysis: true, + Parser: mockParser{}, + }, + } + + tests := []struct { + name string + hostname string + wantHost string + wantExists bool + }{ + { + name: "subdomain match", + hostname: "cdn.registry.example.org", + wantHost: "registry.example.org", + wantExists: true, + }, + { + name: "multi-level subdomain match", + hostname: "a.b.c.registry.example.org", + wantHost: "registry.example.org", + wantExists: true, + }, + { + name: "partial match should not work", + hostname: "fakeregistry.example.org", + wantHost: "", + wantExists: false, + }, + { + name: "different domain should not match", + hostname: "registry.other.org", + wantHost: "", + wantExists: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config := configMap.GetConfigForHostname(tt.hostname) + if !tt.wantExists { + assert.Nil(t, config) + return + } + assert.NotNil(t, config) + assert.Equal(t, tt.wantHost, config.Host) + }) + } +} + +func TestRegistryConfigMap_GetConfigForHostname_LongestMatchPrecedence(t *testing.T) { + // Test that when multiple endpoints could match, the longest (most specific) is selected + configMap := registryConfigMap{ + "example.org": { + Host: "example.org", + SupportedForAnalysis: false, + Parser: mockParser{}, + }, + "registry.example.org": { + Host: "registry.example.org", + SupportedForAnalysis: true, + Parser: mockParser{}, + }, + } + + tests := []struct { + name string + hostname string + wantHost string + }{ + { + name: "should match longer endpoint", + hostname: "cdn.registry.example.org", + wantHost: "registry.example.org", + }, + { + name: "should match shorter when longer doesn't apply", + hostname: "other.example.org", + wantHost: "example.org", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + config := configMap.GetConfigForHostname(tt.hostname) + assert.NotNil(t, config) + assert.Equal(t, tt.wantHost, config.Host) + }) + } +} + +func TestRegistryConfigMap_GetConfigForHostname_ExactMatchTakesPrecedence(t *testing.T) { + // Exact match should always take precedence over subdomain match + configMap := registryConfigMap{ + "example.org": { + Host: "example.org", + SupportedForAnalysis: false, + Parser: mockParser{}, + }, + "cdn.example.org": { + Host: "cdn.example.org", + SupportedForAnalysis: true, + Parser: mockParser{}, + }, + } + + config := configMap.GetConfigForHostname("cdn.example.org") + assert.NotNil(t, config) + assert.Equal(t, "cdn.example.org", config.Host) + assert.True(t, config.SupportedForAnalysis, "exact match should be selected, not subdomain match") +} + +func TestRegistryConfigMap_ContainsHostname(t *testing.T) { + configMap := registryConfigMap{ + "registry.example.org": { + Host: "registry.example.org", + SupportedForAnalysis: true, + Parser: mockParser{}, + }, + } + + tests := []struct { + name string + hostname string + want bool + }{ + { + name: "exact match", + hostname: "registry.example.org", + want: true, + }, + { + name: "subdomain match", + hostname: "cdn.registry.example.org", + want: true, + }, + { + name: "no match", + hostname: "unknown.org", + want: false, + }, + { + name: "partial match should not work", + hostname: "fakeregistry.example.org", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := configMap.ContainsHostname(tt.hostname) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestRegistryConfigMap_EmptyMap(t *testing.T) { + configMap := registryConfigMap{} + + assert.Nil(t, configMap.GetConfigForHostname("any.host.org")) + assert.False(t, configMap.ContainsHostname("any.host.org")) +}