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

On Hold: Update Axios to latest version: 1.6.2 #338

Merged
merged 4 commits into from
Feb 20, 2024
Merged

On Hold: Update Axios to latest version: 1.6.2 #338

merged 4 commits into from
Feb 20, 2024

Conversation

janpaepke
Copy link
Collaborator

@janpaepke janpaepke commented Dec 13, 2023

UPDATE
It seems this PR fails with node 6.14. See discussion below for more info.


This PR updates the Axios dependency to 1.6.2, since the version currently in use (0.27.2) contains a reported vulnerability (Cross-Site Request Forgery).

Even though this is actually of no consequence in the context of the usage of this library, this results in warning messages for all consumers, who will have to then research if a risk exists.
This resolves: #337

The update is technically pretty straight forward, since there appears to be no breaking change in behaviour within the scope of this library.
The only changes to the library are type related.

Most of the work spent on this PR was getting the tests to work.
Since changes to the tests are obviously a delicate matter, I broke the updates down into individual commits for better reviewability:

  1. upgrade axios and update types:
    This includes the version update of axios and the only necessary changes to get the build to compile. As mentioned before, these are only type related.
  2. upgrade jest and fix uncontroversial tests:
    This contains three fairly uncontroversial changes:
    • Update jest: One major difference in axios is that it points to an es6 file as the main entry point. Since tests are run on dist files, only cjs is supported and the old jest version was unable to properly resolve the axios cjs lib source. This is fixed in jest 29. (actually I think since 28).
    • Snapshot updates: The snapshot format in the jest 29 does not include the prototype name by default.
      There are no snapshot changes other than the removal of the prototype names.
    • Axios http adapter definition: The http adapter is not imported anymore, but referenced by string.
  3. fixed tests, which require some explanation:
    • Type mismatch: Jest no longer exposes a global fail method, even though DefinitelyTyped still thinks so. This can be easily fixed by importing the fail method from node:assert, which has the same behavior in our context.
    • Promise OR done(): Jest 29 forces users to either use done OR return a promise. That's an easy fix!
  4. fix axios-timing related tests: This issue was quite time consuming to identify, but it seems the new axios version does not play nice with jest.useFakeTimers().
    I suspect the reason to be a changed logic for their internal request timeout management.
    As a consequence the initial request would not be processed, when simply awaiting tick (). What I did find to work was using await jest.advanceTimersByTimeAsync(0); instead, which should have the same effect.
    Additionally this is how we awaited subsequent requests:
    jest.advanceTimersByTime(2e3); await tick();
    The only method I found to work reliably now was this:
    await tick(); await jest.advanceTimersByTimeAsync(2e3);
    Swapping the order and making the timer async should still not change the intention of this test.

In conclusion, this should be an appropriate upgrade with no breaking changes for the lib.
The only part that might deserve a closer look are the changes describe in #4 above.

@janpaepke janpaepke mentioned this pull request Dec 13, 2023
@janpaepke janpaepke changed the title Update Axios to latest version: 1.6.2 Draft: Update Axios to latest version: 1.6.2 Dec 13, 2023
@janpaepke
Copy link
Collaborator Author

janpaepke commented Dec 13, 2023

All right, so in contrast to my naive initial conclusion, this seems to be a less trivial update.

The reason is that this sdk currently supports node 6, which is dropped in jest 25.

I investigated further, if there is a way to get jest 24 to understand how to resolve the correct axios cjs files and got it to work by updating the jest.config.js like this:

function createProject(displayName, testRegex, rest) {
  return Object.assign({}, rest, {
    
    moduleNameMapper: {
      axios: 'axios/dist/node/axios.cjs',
    },});
}

While this does make jest resolve axios correctly, it still breaks, because axios.cjs seems to contain the spread operator, which was only supported starting node 8.6.
image

So not only is axios apparently incompatible with node 6 (there is no minimum node version marked in the package), but even if it was, it's likely that any user actually still using node 6 would run into the import issue, which makes this a bad solution.

The way I see it there are three possible ways forward:

  1. rip out axios and replace it or write a custom http client implementation.
  2. try to include axios in the output bundle rather than making it a dependency, transpiling accordingly.
  3. drop support for node 6 and use node 14 as minimum supported version instead.

Personally I would recommend 3, which would make this a breaking change.
Both 1 AND 2 require more work, where the effort is most likely unjustified to support an ancient node version.
Node 14 was released almost 4 years ago and is currently used by most libraries (including jest 29) as the minimum supported version.
I have confirmed this PR does work on node 14.

To decide, it would be interesting to get some usage stats from the MollieAPI maintainers.
For now this PR is on hold, until we decide on a path forward.

@janpaepke janpaepke changed the title Draft: Update Axios to latest version: 1.6.2 On Hold: Update Axios to latest version: 1.6.2 Dec 13, 2023
@DHFW
Copy link

DHFW commented Dec 27, 2023

I would vote for dropping support for the very old NodeJS versions (I am on Node 18). I really want my payment provider to not have CVE's... Release a version 4 so that it is clear this is a major upgrade.

@janpaepke
Copy link
Collaborator Author

janpaepke commented Jan 4, 2024

As an additional side note:
You'd think another option would be to "only" increase the node requirement to node 8.6 to support the spread operator.

Well I tried that, but unfortunately the axios.cjs dist also includes an async iterator:
image

Support for this was only introduced in node 10.

Using node 10 and the jest "fix" above, axios-mock-adapter breaks. Same for node 12.
This is likely because it fails to resolve its peer deependency for the same reason we require the jest config update.

Maybe there's a way to get it to work with node 10, but is the effort worth it? I mean if we're doing a breaking change anyway, why not go 14?

Looking forward to some maintainer insights on this.

best Jan

@fjbender
Copy link
Contributor

fjbender commented Jan 4, 2024

@maria-swierblewska Can you look into the numbers a bit and then come up with a recommendation around dropping version support?

@maria-swierblewska
Copy link

Hello @janpaepke I've done some analysis on past year's data on Node versions used versus the volume of payments they support and it is clear that the support should be maintained for v14 and higher (v14 supports almost 30% of all volume). In the past year all versions under v14 account for 2.94% of all payments volume so indeed we think there is no need to support the versions below 14.
Let me know if you have any other questions on this!

@janpaepke
Copy link
Collaborator Author

janpaepke commented Jan 8, 2024

@maria-swierblewska thank you for your info.

@fjbender
We could either prepare a separate branch with the v14 update ONLY (package json, yaml etc.) to update everything and then rebase this one, or keep working in this branch.
How do you propose we proceed?

@maria-swierblewska
Copy link

@janpaepke for the sake of transparency we'd suggest to proceed with a separate branch with the v14 update and rebasing this one

@janpaepke
Copy link
Collaborator Author

janpaepke commented Jan 15, 2024

Added the node update here: #341

This should be pretty straight forward.

Copy link
Collaborator

@edorivai edorivai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Went over all changes with @janpaepke and tests are passing now.

@edorivai edorivai merged commit 342cedd into mollie:master Feb 20, 2024
5 checks passed
@edorivai edorivai mentioned this pull request Feb 20, 2024
@DHFW
Copy link

DHFW commented Mar 22, 2024

Thanks for all the work! When will this be released?
Edit: nvm, I see I need to install the 4.0.0 beta.

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.

Axios CVE
5 participants