Skip to content

Commit

Permalink
Minor bugfix to handle cases where the package name doesn't resolve p…
Browse files Browse the repository at this point in the history
…roperly (#305)

* Minor bugfix to handle cases where the package name doesn't resolve to a proper URL

* Remove accidental changes

* PR review fixes

* Pass the trustedURLs directly instead of ignoring the npm configured URL

* Updated the test cases
  • Loading branch information
insaaniManav authored Jan 14, 2025
1 parent 6b050fe commit 01396c1
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 17 deletions.
38 changes: 22 additions & 16 deletions pkg/analyzer/lfp_npm.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func (npm *npmLockfilePoisoningAnalyzer) Analyze(manifest *models.PackageManifes

trustedRegistryUrls := []string{npmRegistryTrustedUrlBase}
trustedRegistryUrls = append(trustedRegistryUrls, npm.config.TrustedRegistryUrls...)

userTrustUrls := npm.config.TrustedRegistryUrls
logger.Debugf("npmLockfilePoisoningAnalyzer: Analyzing package [%s] with %d trusted registry URLs in config",
packageName, len(trustedRegistryUrls))

Expand Down Expand Up @@ -147,7 +147,7 @@ func (npm *npmLockfilePoisoningAnalyzer) Analyze(manifest *models.PackageManifes
})
}

if !npmIsUrlFollowsPathConvention(lockfilePackage.Resolved, packageName, trustedRegistryUrls) {
if !npmIsUrlFollowsPathConvention(lockfilePackage.Resolved, packageName, trustedRegistryUrls, userTrustUrls) {
logger.Debugf("npmLockfilePoisoningAnalyzer: Package [%s] resolved to an unconventional URL [%s]",
packageName, lockfilePackage.Resolved)

Expand Down Expand Up @@ -247,12 +247,11 @@ func npmNodeModulesPackagePathToName(path string) string {

// Test if URL follows the pkg name path convention as per NPM package registry
// specification https://docs.npmjs.com/cli/v10/configuring-npm/package-lock-json
func npmIsUrlFollowsPathConvention(sourceUrl string, pkg string, trustedUrls []string) bool {
// Example: https://registry.npmjs.org/express/-/express-4.17.1.tgz
func npmIsUrlFollowsPathConvention(sourceUrl string, pkg string, trustedUrls []string, userTrustedUrls []string) bool {
// Parse the source URL
parsedUrl, err := npmParseSourceUrl(sourceUrl)
if err != nil {
logger.Errorf("npmIsUrlFollowsPathConvention: Failed to parse URL %s: %v",
sourceUrl, err)
logger.Errorf("npmIsUrlFollowsPathConvention: Failed to parse URL %s: %v", sourceUrl, err)
return false
}

Expand All @@ -265,25 +264,32 @@ func npmIsUrlFollowsPathConvention(sourceUrl string, pkg string, trustedUrls []s
path = path[1:]
}

// Build a list of acceptable package names
acceptablePackageNames := []string{pkg}
for _, trustedUrl := range trustedUrls {
parsedTrustedUrl, err := npmParseSourceUrl(trustedUrl)
if err != nil {
logger.Errorf("npmIsUrlFollowsPathConvention: Failed to parse trusted URL %s: %v",
trustedUrl, err)
logger.Errorf("npmIsUrlFollowsPathConvention: Failed to parse trusted URL %s: %v", trustedUrl, err)
continue
}

trustedBase := parsedTrustedUrl.Path
trustedBase = strings.TrimPrefix(trustedBase, "/")
trustedBase = strings.TrimSuffix(trustedBase, "/")

acceptablePackageNames = append(acceptablePackageNames,
fmt.Sprintf("%s/%s", trustedBase, pkg))
trustedBase := strings.Trim(parsedTrustedUrl.Path, "/")
acceptablePackageNames = append(acceptablePackageNames, fmt.Sprintf("%s/%s", trustedBase, pkg))
}

// Example: @angular/core from https://registry.npmjs.org/@angular/core/-/core-1.0.0.tgz
// Extract the scoped package name
scopedPackageName := strings.Split(path, "/-/")[0]
if slices.Contains(acceptablePackageNames, scopedPackageName) {
return true
}

return slices.Contains(acceptablePackageNames, scopedPackageName)
// Check if the source URL starts with any trusted URL except the NPM trusted base URL
for _, trustedUrl := range userTrustedUrls {
if strings.HasPrefix(sourceUrl, trustedUrl) {
return true
}
}

// Default fallback
return false
}
9 changes: 8 additions & 1 deletion pkg/analyzer/lfp_npm_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -160,11 +160,18 @@ func TestNpmIsUrlFollowsPathConvention(t *testing.T) {
[]string{"https://registry.npmjs.org/base", "https://registry.npmjs.org/base1"},
true,
},
{
"strip_ansi_cjs package path matches trusted url path",
"https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz",
"strip-ansi-cjs",
[]string{"https://registry.npmjs.org/strip-ansi"},
true,
},
}

for _, test := range cases {
t.Run(test.name, func(t *testing.T) {
actual := npmIsUrlFollowsPathConvention(test.url, test.pkgName, test.trustedUrls)
actual := npmIsUrlFollowsPathConvention(test.url, test.pkgName, test.trustedUrls, test.trustedUrls)
assert.Equal(t, test.expected, actual)
})
}
Expand Down

0 comments on commit 01396c1

Please sign in to comment.