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.

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

Upgrade superagent #813

Closed
wants to merge 2 commits into from
Closed

Upgrade superagent #813

wants to merge 2 commits into from

Conversation

Jaykingamez
Copy link

Checklist

#812

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

@pixelfuture
Copy link

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

@cupofjoakim
Copy link

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

@cupofjoakim
Copy link

Maybe @yunnysunny or @lamweili ?

@yunnysunny
Copy link
Contributor

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

@lamweili
Copy link
Contributor

lamweili commented Jan 13, 2024

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
Copy link

@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
Copy link
Contributor

lamweili commented Jan 14, 2024

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.


"superagent": "^8.0.5"

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
Copy link
Contributor

lamweili commented Jan 14, 2024

@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
Copy link
Collaborator

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

@titanism titanism closed this Jan 14, 2024
@lamweili
Copy link
Contributor

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

@titanism, thanks for the release!

@cupofjoakim
Copy link

That did it, thank you very much!

@lamweili
Copy link
Contributor

That did it, thank you very much!

Great to hear!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants