dependabot / dependabot-core

🤖 Dependabot's core logic for creating update PRs.
https://docs.github.com/en/code-security/dependabot
MIT License
4.55k stars 955 forks source link

Issue when updating npm github packages #3813

Closed ImRodry closed 3 years ago

ImRodry commented 3 years ago

Package ecosystem

npm

Package manager version

6.14.13

Language version

Node 14.17.0

Manifest location and content prior to update

You can find the diff on this PR.

{
  "name": "hypixel-translators",
  "version": "3.0.0",
  "main": "dist/index",
  "license": "MIT",
  "scripts": {
    "start": "node --trace-warnings dist/index.js",
    "test": "node --unhandled-rejections=strict dist/index.js",
    "compile": "rimraf dist/ && tsc -w",
    "postinstall": "tsc"
  },
  "repository": {
    "type": "git",
    "url": "git://github.com/Hypixel-Translators/hypixel-translators-bot.git"
  },
  "dependencies": {
    "canvas": "^2.8.0",
    "country-emoji": "^1.5.6",
    "countryjs": "^1.8.0",
    "discord.js": "github:discordjs/discord.js",
    "dotenv": "^9.0.2",
    "mongodb": "^3.6.6",
    "node-fetch": "^2.6.1",
    "puppeteer": "^9.1.1",
    "source-map-support": "^0.5.19",
    "typescript": "^4.2.4",
    "uuid": "^8.3.2"
  },
  "engines": {
    "node": "14.x"
  },
  "devDependencies": {
    "@types/countryjs": "^1.8.1",
    "@types/mongodb": "^3.6.12",
    "@types/node-fetch": "^2.5.10",
    "@types/uuid": "^8.3.0",
    "@types/ws": "^7.4.2",
    "discord-api-types": "^0.18.1",
    "rimraf": "^3.0.2"
  }
}

package.json

dependabot.yml content

version: 2
updates:
- package-ecosystem: npm
  directory: "/"
  schedule:
    interval: daily
    time: "04:00"
    timezone: Europe/Lisbon
  open-pull-requests-limit: 10
  assignees:
  - imrodry

dependabot.yml

Updated dependency

github:discordjs/discord.js#ab82cafcde0ee259a32ef14303c1b4a64dea8fae to github:discordjs/discord.js#c8d20a456b635ce6081ed8ad17315a9a0c0244bc (github repository package)

What you expected to see, versus what you actually saw

I expected the value in the "version" parameter to be changed, while "from" would be kept the same as I had the repository set as a dependency, and not a specific commit. The "from" parameter should only be changed between commits when the value in package.json has a commit hash specified

Actual:

      "version": "github:discordjs/discord.js#c8d20a456b635ce6081ed8ad17315a9a0c0244bc",
      "from": "github:discordjs/discord.js#c8d20a456b635ce6081ed8ad17315a9a0c0244bc",

Expected:

      "version": "github:discordjs/discord.js#c8d20a456b635ce6081ed8ad17315a9a0c0244bc",
      "from": "github:discordjs/discord.js",

Native package manager behavior

The behavior described above, "version" is updated while "from" is kept the same, except if a commit hash is specified on package.json or installed through npm i owner/repository#commitHash

Images of the diff or a link to the PR, issue or logs

https://github.com/Hypixel-Translators/hypixel-translators-bot/pull/327

ImRodry commented 3 years ago

Forgot to mention but this slight change makes it impossible to install dependencies/makes them unusable and crashes on startup.

ImRodry commented 3 years ago

Seems to me that the issue is on these lines: (737 and 740) https://github.com/dependabot/dependabot-core/blob/955eccebe71140cf00d5fde3e6b5a8cb2ff3be70/npm_and_yarn/lib/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater.rb#L735-L742 It should say :from instead of :version but I'm not entirely sure

xlgmokha commented 3 years ago

I think I see the problem. When we attempt to parse the semver version from a version constraint that specifies a git repo we made the assumption that a semver version will always be present. However, the regular expression that we use makes it optional. The docs also seems to suggest that this is optional.

e.g.

With the given input:

{
  "name": "foo",
  "version": "0.0.0",
  "dependencies": {
    "express": "expressjs/express",
    "mocha": "mochajs/mocha#4727d357ea",
    "module": "user/repo#feature\/branch"
  }

Each of the dependencies above will match:

https://github.com/dependabot/dependabot-core/blob/83c7f99968cc174a88500fc5dc9fc48be0b5497c/npm_and_yarn/lib/dependabot/npm_and_yarn/file_parser.rb#L23-L32

However, the semver capture group is optional and therefore will return nil here:

https://github.com/dependabot/dependabot-core/blob/83c7f99968cc174a88500fc5dc9fc48be0b5497c/npm_and_yarn/lib/dependabot/npm_and_yarn/file_parser.rb#L255-L260

We then end up with a nil requirement and this is what we pass down to the npm helper script.

https://github.com/dependabot/dependabot-core/blob/83c7f99968cc174a88500fc5dc9fc48be0b5497c/npm_and_yarn/helpers/lib/npm6/updater.js#L56

npm doesn't know what the original version constraint was and therefore assumes it is safest to pin to an explicit version and that is why the from gets pinned to a hash instead of using the original version constraint that was specified in the package.json.

I have a PR that appears to be working https://github.com/dependabot/dependabot-core/pull/3837. I need to do a little more testing and get some feedback from others.

ImRodry commented 3 years ago

Thank you for such a detailed response! I have since stopped using the github dependency so I will not be able to check if this fix works once it’s merged but I’m glad it’s being looked into!

xlgmokha commented 3 years ago

Thank you for such a detailed response! I have since stopped using the github dependency so I will not be able to check if this fix works once it’s merged but I’m glad it’s being looked into!

No problem. Thank you for providing a public repo and for taking the time to write up this issue. This is very helpful.

xlgmokha commented 3 years ago

I was able to regenerate the original PR but with the latest version of the dependabot-core runtime. https://github.com/xlgmokha/hypixel-translators-bot/pull/1

The latest version version produced a diff that did not modify the from field in the lock file.

    "discord.js": {
-      "version": "github:discordjs/discord.js#ab82cafcde0ee259a32ef14303c1b4a64dea8fae",
+      "version": "github:discordjs/discord.js#e980948de55e91e59c9e3293ac76bc645a058a53",
      "from": "github:discordjs/discord.js",

I believe this is now working as expected.

ImRodry commented 3 years ago

Yep that should work as expected, thank you so much!