Skip to content

Fix the npm package already taken validator

David Fernandez requested to merge 37364-fix-npm-validator into master

🦐 Context

When pulling packages from the npm package registry, it can be accessed on two different levels:

  • The project level
    • Basically, users can pull packages from a given project
  • The instance level
    • Users can target an instance url
    • Only packages that follow the naming convention are available at this level. Basically the package scope must be the root group (or user namespace) path.

Uploading an npm package can only be done at the project level.

In addition of this, we have an additional restriction on uploads:

You cannot publish a package if a package of the same name and version already exists.

At the project level, the above is implemented as a unique model validation.

At the instance level, we have a custom validation function that will look for duplicate package names in all the projects of the root namespace (except the current project).

🔥 The ~bug

That custom validation function looking for duplicates has a few issues:

  1. When looking for other packages within the root group, all package types are considered. The validation is for npm packages only
  2. The validation is executed even for packages that don't follow the naming convention.
    • Those packages will never be available at the instance level. There is no point in checking packages outside of the given project.

Those issues leads to npm package uploads getting rejected with 400 Bad Request. That's issue #37364 (closed).

🚒 What does this MR do?

  • Change how the package_already_taken? validator works:
    • Don't run when the package name doesn't follow the naming convention.
    • When looking for duplicates from the root namespace, only consider npm packages.
  • Update the related specs

📷 Screenshots or Screencasts (strongly suggested)

n / a

How to setup and validate locally (strongly suggested)

  1. Create a group (public or private, it doesn't matter). Note the path. (path example: fruits)
  2. Create a project with whatever visibility and whatever name. Note the id. (example: Project1)
  3. Create a second project with whatever visibility and whatever name. Note the id. (example: Project2)
  4. Have a personal access token ready with the api scope.
  5. We're going to install a small util project that will manage npm commands for us: checkout https://gitlab.com/10io/gl_pru.
  6. cd into gl_pru

Case: Package not following the naming convention

  1. Let's create a random package on the first project: $ bundle exec thor package:push --package-type=npm --user=<user> --token="<pat token>" --url=http://<base url of gitlab>/api/v4/projects/<first project id>/packages/npm/ --name=test --version=1.2.3 --npm-package-scope=@test
    • The package scope MUST NOT be the group path. (example: fruits)
  2. Make sure that the upload succeeds. You should see + @test/test@1.2.3
  3. Let's create a package with the same name and version on the second project $ bundle exec thor package:push --package-type=npm --user=<user> --token="<pat token>" --url=http://<base url of gitlab>/api/v4/projects/<second project id>/packages/npm/ --name=test --version=1.2.3 --npm-package-scope=@test

Using master: this second upload will fail with

npm ERR! code E400
npm ERR! 400 Bad Request - PUT <url>

Using this MR branch: the second upload succeeds 🚀

Case: Package of different type with same name

  1. Let's create a random nuget package on the first project. Not everyone has $ nuget installed so let's do it with a rails console:
    project = Project.find(<first project id>)
    project.packages.nuget.create!(name: "bananas", version: "1.2.3")
  2. Let's try to upload an npm package on the second project. $ bundle exec thor package:push --package-type=npm --user=<user> --token="<pat token>" --url=http://<base url of gitlab>/api/v4/projects/<second project id>/packages/npm/ --name=bananas --version=1.2.3

Using master, this second upload will fail with

npm ERR! code E400
npm ERR! 400 Bad Request - PUT <url>

Using this MR branch: the second upload succeeds 🚀

📐 Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

  • [-] Label as security and @ mention @gitlab-com/gl-security/appsec
  • [-] The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • [-] Security reports checked/validated by a reviewer from the AppSec team

💾 Database review

Query updated: !67107 (comment 638647444)

Edited by David Fernandez

Merge request reports