Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix NPM packages name validation #26595

Merged
merged 7 commits into from
Aug 20, 2023
Merged

Fix NPM packages name validation #26595

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c4bab2c
Added tests to parse npm package names
TimberBro Aug 17, 2023
0bb5520
Merge branch 'main' into timberbro-npm-patch-1
TimberBro Aug 19, 2023
cdad9f0
Replaced regex to parse npm packages names.
TimberBro Aug 19, 2023
b82d53b
Added test to cover names contains tilde.
TimberBro Aug 19, 2023
87789ae
Excluded tilde from regex, as suggested in PR
TimberBro Aug 19, 2023
f1fbb42
Merge branch 'main' into timberbro-npm-patch-1
GiteaBot Aug 20, 2023
712ac64
Merge branch 'main' into timberbro-npm-patch-1
GiteaBot Aug 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/packages/npm/creator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ var (
ErrInvalidIntegrity = util.NewInvalidArgumentErrorf("failed to validate integrity")
)

var nameMatch = regexp.MustCompile(`\A((@[^\s\/~'!\(\)\*]+?)[\/])?([^_.][^\s\/~'!\(\)\*]+)\z`)
var nameMatch = regexp.MustCompile(`^(@[a-z0-9-~][a-z0-9-._~]*/)?[a-z0-9-~][a-z0-9-._~]*$`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

package name should not contain any of the following characters: ~)('!*

Suggested change
var nameMatch = regexp.MustCompile(`^(@[a-z0-9-~][a-z0-9-._~]*/)?[a-z0-9-~][a-z0-9-._~]*$`)
var nameMatch = regexp.MustCompile(`^(@[a-z0-9-][a-z0-9-._]*/)?[a-z0-9-][a-z0-9-._]*$`)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Added test for that case and applied suggested solution.


// Package represents a npm package
type Package struct {
Expand Down
17 changes: 17 additions & 0 deletions modules/packages/npm/creator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,16 @@ func TestParsePackage(t *testing.T) {
test(t, " test")
test(t, "test ")
test(t, "te st")
test(t, "Test")
test(t, "_test")
test(t, ".test")
test(t, "^test")
test(t, "te^st")
test(t, "te|st")
test(t, "te)(st")
test(t, "te'st")
test(t, "te!st")
test(t, "te*st")
test(t, "invalid/scope")
test(t, "@invalid/_name")
test(t, "@invalid/.name")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also test for

package name cannot be the same as a node.js/io.js core module nor a reserved/blacklisted name. For example, the following names are invalid:
http
stream
node_modules
favicon.ico

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could keep a map of reserved names. Would be cleaner then extending the regex.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One doesn't exclude the other?
The test method does not test the regex directly, but the upload.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we merge this PR without reserved names inspection feature?
Can I create separated issue for that, and I think, I'm able to implement it, but need some time to dive into codebase.

Expand All @@ -93,6 +103,13 @@ func TestParsePackage(t *testing.T) {

test(t, "test")
test(t, "@scope/name")
test(t, "@scope/q")
test(t, "q")
test(t, "@scope/package-name")
test(t, "@scope/package.name")
test(t, "@scope/package_name")
test(t, "123name")
test(t, "----")
test(t, packageFullName)
})

Expand Down