Jaykingamez commented 1 year ago

Checklist

812

[x] I have ensured my pull request is not behind the main or master branch of the original repository.
[x] I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
[x] I have written a commit message that passes commitlint linting.
[x] I have ensured that my code changes pass linting tests.
[x] I have ensured that my code changes pass unit tests.
[x] I have described my pull request and the reasons for code changes along with context if necessary.

pixelfuture commented 1 year ago

Can we get this one merged soon? It looks good.

cupofjoakim commented 10 months ago

@titanism Any chance we could get this merged? It closes CVE-2022-25901 which has been up for way too long.

cupofjoakim commented 9 months ago

Maybe @yunnysunny or @lamweili ?

yunnysunny commented 9 months ago

I do not have the permission. And the bumped dependencies is development dependencies. So will they make danger on production environment?

lamweili commented 9 months ago

I do not have the permission. And the bumped dependencies is development dependencies. So will they make danger on production environment?

I agree with @yunnysunny.

The CVE-2022-25901 affects cookiejar@<=2.1.3 and it was fixed in cookiejar@2.1.4. Being a targeted PR which fixes this issue, there's no need bump the version of other devDependencies.

supertest is using superagent@^8.0.5 which requires cookiejar@^2.1.3. (source: https://github.com/ladjs/superagent/blob/v8.0.5/package.json#L23)

    "cookiejar": "^2.1.3",

So there shouldn't be any vulnerabilities, as the versioning is not fixed at cookiejar@2.1.3 but cookiejar@^2.1.3. Thus, at the time of writing, cookiejar@2.1.4 will be installed in accordance with semver.

So I am unsure why there is any urgency for this version bump. Yes, it might be good defensively. But there shouldn't be any urgency as it has no impact on current use.

cupofjoakim commented 9 months ago

@lamweili In our case we need to have no CVE's in our code repository for compliancy reasons with a local regulatory body. It's no biggie though, we'll just move off of supertest.

lamweili commented 9 months ago

I don't quite understand how the CVE exist in your code repository. Following semver, the vulnerable version of cookiejar@<=2.1.3 will not be installed. If you do npm install now, cookiejar@2.1.4 will be installed, as the package.json specifies cookiejar@^2.1.3.

I'm guessing your package-lock.json/npm-shrinkwrap.json locks it at cookiejar@2.1.3? If that's the case, you can remove your package-lock.json/npm-shrinkwrap.json and do a npm install. It should update all your dependencies and generate new package-lock.json/npm-shrinkwrap.json.

https://github.com/ladjs/supertest/blob/ffb96df43b1d5c2bad7f6b9534ecf948e87b9452/package.json#L9

It's strange why superagent@^8.0.5 (which uses cookiejar@^2.1.3) doesn't resolve to the latest version. They should all resolve to the same final versions below:

package.json	Resolved
`superagent@^8.0.5` (`cookiejar@^2.1.3`)	`superagent@8.1.2` (`cookiejar@2.1.4`)
`superagent@^8.0.6` (`cookiejar@^2.1.3`)	`superagent@8.1.2` (`cookiejar@2.1.4`)
`superagent@^8.0.7` (`cookiejar@^2.1.4`)	`superagent@8.1.2` (`cookiejar@2.1.4`)
...	...
`superagent@^8.1.2` (`cookiejar@^2.1.4`)	`superagent@8.1.2` (`cookiejar@2.1.4`)

lamweili commented 9 months ago

@titanism, what's your take on this?

Maybe we should remove yarn-lock.json from this repository, since superagentrepository doesn't include lock files too. Or to accept PR #811 which bumps cookiejar in yarn-lock.json.

For the weird resolution issue, we can also defensively up to superagent@^8.0.7 (which bumps to cookiejar@^2.1.4). That should be a one-liner change in package.json and not the number of changes (especially the devDependencies) in this PR. Unless, we also need to update yard-lock.json, then it might be more than a one-liner change.

titanism commented 9 months ago

https://github.com/ladjs/supertest/releases/tag/v6.3.4

lamweili commented 9 months ago

@cupofjoakim, your issue should have been resolved. Please help to verify and get back.

@titanism, thanks for the release!

cupofjoakim commented 9 months ago

That did it, thank you very much!

lamweili commented 9 months ago

That did it, thank you very much!

Great to hear!

ladjs / supertest

Upgrade superagent #813

Checklist

812