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

Update axios package to > 0.21.1 to solve security vulnerability #231

Closed
kannapples opened this issue Sep 21, 2021 · 14 comments
Closed

Update axios package to > 0.21.1 to solve security vulnerability #231

kannapples opened this issue Sep 21, 2021 · 14 comments

Comments

@kannapples
Copy link

One of my repos uses devour-client as a dependency, and we received a dependabot warning about a security vulnerability for axios package versions <= 0.21.1.

image

Please upgrade the axios version to at least 0.21.2 (current is 0.21.4).

@tijn
Copy link
Collaborator

tijn commented Sep 22, 2021

Hi @kajaaldias, can you make a pull request for this?

@tijn
Copy link
Collaborator

tijn commented Oct 5, 2021

#232 solved this

@tijn tijn closed this as completed Oct 5, 2021
@TheElter
Copy link

TheElter commented Nov 3, 2022

Hi @tijn did you noticed that with the upgrade of axios you raised node.js requirement from 6.x to 13.x ? Why such a breaking change in a patch?

@tijn
Copy link
Collaborator

tijn commented Nov 3, 2022

Hi @tijn did you noticed that with the upgrade of axios you raised node.js requirement from 6.x to 13.x ? Why such a breaking change in a patch?

Oh. I did not notice that! I actually had a conversation with a colleague about the impact of the next release but neither of us realised that this requirement had changed too. (But it explains our own troubles with upgrading devour in our own app).

So, basically, I messed it up. I will try to release a fix later this evening when I get home.

@tijn
Copy link
Collaborator

tijn commented Nov 3, 2022

Hi @TheElter ,

This happened a month ago. Is this really the thing you're talking about, or was it #243 ?

@tijn tijn pinned this issue Nov 3, 2022
@tijn tijn unpinned this issue Nov 3, 2022
@TheElter
Copy link

TheElter commented Nov 4, 2022

Hi @tijn maybe I'm commenting the wrong issue, but the jump of axios is the cause of node version issue, if you want I can test the patch when you are ready (actually I'm fixing the dependency to old version on our code that runs on node 6)

@tijn
Copy link
Collaborator

tijn commented Nov 4, 2022

@TheElter

  1. okay, so the updated Axios needs a newer node version. This looks totally reasonable to me; you wouldn't expect them to keep supporting an old node-js forever. Anyhow, can you point out the commit in Axios where this happened?

  2. What patch exactly do you want to test?

@TheElter
Copy link

TheElter commented Nov 4, 2022

@tijn for point 2 depends on the goal: you want to keep node 6 compatibility or not for devour client ? :)
If YES I'm available to help you testing an eventual patch
if NO I presume that it has to be wrote somewhere here on github and/or a different version has to done for devour (to avoid getting the incompatible version with a caret dependency in package Json)

For point 1 I can look in Axios history BUT I presume that first the goal on point 2 must be clear.

@tijn
Copy link
Collaborator

tijn commented Nov 4, 2022

@TheElter I don't want to keep compatibility with version 6 forever. The current LTS version is 18.12.0. 6 seems unreasonably old.

@TheElter
Copy link

TheElter commented Nov 4, 2022

@tjin I agree with your evaluation, so if the compatibility with version 6 is forgettable looking where axios is no more compatible is unuseful. The error I got from my test was the use of import keyword that if I looked properly is supported officially from node 13 (is experimental in some previous ones).

If it was my public project a breaking change (for any reason) will have caused a MAJOR version (semver state that), I presume is something unclear due to the fact I saw other project that for the node version doesn't do that and do a MINOR version (not a patch for sure).

These are my two cents as user uf the devour-client. For my personal case means lock the version to the last one compatible and add more priority to the migration of old code that is still running on node 6 :)

@tijn
Copy link
Collaborator

tijn commented Nov 4, 2022

@TheElter yeah, it's not really a breaking change because our API didn't change. We don't have any new functions to call, no different parameters, nothing deleted. But there is a big difference for users: those using node 18 will probably have a smooth upgrade while node 6-users will be stuck. And yeah, that sucks, but nevertheless, a change in dependencies is not a major version change in semver as far as I know.

Note that even if we marked this as a major version bump you'd still be doing exactly what you're doing now. You'd lock to the current version and add more priority to migrating old code on node 6, which is obviously the correct thing to do. 🤷

@TheElter
Copy link

TheElter commented Nov 4, 2022

I see that the point on semver is effectively discussed around (this is on thread as sample semver/semver#526 ). I suggest you wrote at least a note that you bumped the minimum node version.

@tijn
Copy link
Collaborator

tijn commented Nov 6, 2022

I see that the point on semver is effectively discussed around (this is on thread as sample semver/semver#526 ). I suggest you wrote at least a note that you bumped the minimum node version.

@TheElter I would've, if only I had a way to know about it before we released this version. Anyhow, please provide a PR with the note that you wished you had had. It's open source after all.

@auvipy
Copy link
Collaborator

auvipy commented Nov 7, 2022

in any case, we should support only supported LTS and other versions which are supported. current minimum LTS ia 16.x right?

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

No branches or pull requests

4 participants