Dependency update by Saibamen · Pull Request #4647 · louislam/uptime-kuma

Saibamen · 2024-04-03T20:50:09Z

Tick the checkbox if you understand [x]:

I have read and understand the pull request rules.

Description

Fix vulnerabilities + update already installed versions in package.json (ncu --target patch -u - see commit 1c0280f and ncu --target semver -u 7c07f97)

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist

My code follows the style guidelines of this project
I ran ESLint and other linters for modified files
I have performed a self-review of my own code and tested it
I have commented my code, particularly in hard-to-understand areas (including JSDoc for methods)
My changes generates no new warnings
My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Before

After

Saibamen · 2024-04-03T20:59:23Z

npm audit report:

express  <4.19.2
Severity: moderate

*Currently used version in this PR*: 4.17.3

hoek  *
Severity: high

nodemailer  <=6.9.8
Severity: moderate

*Currently used version in this PR*: 6.6.5

vite  4.0.0 - 4.5.2
Severity: high

*Currently used version in this PR*: 4.4.12

CommanderStorm

Let me reiterate my point from #4590 (comment):
Have you read through the changelogs of each update you are proposing carefully?
If not, this is basically just as careless as #4590

Just executing some command is not really a PR, that is just dumping work which needs fixing.
If you have a look at axios-ntlm, they ship their breaking changes in patch releases.

Note

This took me 3.5 hours to review.
Please submit smaller PRs in smaller, thematically linked PRs in the future.
Like this, it is really not managable

CommanderStorm · 2024-04-03T22:32:33Z

        "args-parser": "~1.3.0",
-        "axios": "~0.27.0",
-        "axios-ntlm": "1.3.0",
+        "axios": "~0.28.1",


Regarding 0.28.0
Have you looked at if
axios/axios#4624 + axios/axios#4718 make the output different and if this is better/worse

Not looked, but better stack trace is always good

It might not look good or require additional work to look decent.
Please have a look if they are displayed properly

CommanderStorm · 2024-04-03T22:36:58Z

-        "axios": "~0.27.0",
-        "axios-ntlm": "1.3.0",
+        "axios": "~0.28.1",
+        "axios-ntlm": "1.3.1",


Includes Catbuttes/axios-ntlm#15 which bumps axios to 1.X.

This likely breaks something
=> change needs to be done in step with bumping axios once migration is possible.

Suggested change

"axios-ntlm": "1.3.1",

"axios-ntlm": "1.3.0",

axios-ntlm will use its own axios, and we will still use axios 0.X

This would be still to risky given what this libary does. There might be breaking changes which happened and might make this incompatible:

This is a helper library for NTLM Authentication using the Axios HTTP library on Node. It attaches interceptors to an axios instance to authenticate using NTLM for any resources that offer it.

CommanderStorm · 2024-04-03T23:24:17Z

-        "mqtt": "~4.3.7",
+        "liquidjs": "^10.10.2",
+        "mongodb": "~4.17.2",
+        "mqtt": "~4.3.8",


4.3.8 is neither documented in the changelog nor has an associated commit tag.
=> what are we migrating to here?

4.3.8 was already installed in package-lock before my PR:

That was not the point. I was asking what 4.3.8 is.

CommanderStorm · 2024-04-04T00:36:27Z

+        "@babel/eslint-parser": "^7.24.1",
+        "@babel/preset-env": "^7.24.4",


For these dependencies is somewhat hard to audit what has changed in this bump.
How did you audit this?

These dependencies had MINOR update ranges, so it is automatically downloaded. See: https://docs.npmjs.com/about-semantic-versioning#using-semantic-versioning-to-specify-update-types-your-package-can-accept

And we already had installed 7.23.7 in package-lock.json 4 months ago by @louislam in 8d847ab:

So you are suggesting to just not search for breaking changes here?
Have you audited the changes?

CommanderStorm · 2024-04-04T01:42:53Z

-        "qrcode": "~1.5.0",
-        "rollup-plugin-visualizer": "^5.6.0",
+        "qrcode": "~1.5.3",
+        "rollup-plugin-visualizer": "^5.12.0",


5.7 adds the emitFile (default: false) option.

We use this plugin to generate a file.
Have you checked that the file is still being generated?

We already have rollup-plugin-visualizer installed with version 5.12.0 - no changes for this package in package-lock:

Have you checked that the file is still being generated?
Automatically bumping this in the package-lock might have been a mistake.

Saibamen · 2024-04-04T07:35:39Z

Have you read through the changelogs of each update you are proposing carefully?

Yes. Only 2 packages were updated in my PR: axios and axios-ntlm. See commits: https://github.com/louislam/uptime-kuma/pull/4647/commits

CommanderStorm · 2024-04-04T10:54:57Z

Only 2 packages were updated in my PR

Why is this PR then

Looking through the changelog is still an important part of updating.
There might have been breaking changes that have been missed, despite this package being in the package-lock.
I really don't like these responses ("I don't need to look into this, because package-lock") to potentially breaking changes that you are giving.
=> Please actually check the cases I raised instead of handwaving them away.

Saibamen added 2 commits April 3, 2024 21:13

Recreate package-lock.json without code changes (delete and npm install)

12bad08

Fix axios vulnerable version

b2f1636

Saibamen added 2 commits April 3, 2024 23:07

ncu --target patch -u. No changes in package-lock

1c0280f

ncu --target semver -u

7c07f97

CommanderStorm requested changes Apr 4, 2024

View reviewed changes

CommanderStorm changed the title ~~Fix: Axios update - fix vulnerabilities [1.23.X]~~ Dependency update Apr 4, 2024

Update @actions/github and dompurify

3aeb213

Saibamen closed this Apr 4, 2024

Saibamen deleted the axios_update branch April 4, 2024 16:05

This was referenced Apr 4, 2024

fix: NPM update 4 packages (and AXIOS) [1.23.X] #4651

Closed

fix: Update axios, @actions/github and dompurify [1.23.X] #4652

Merged

		"@babel/eslint-parser": "^7.24.1",
		"@babel/preset-env": "^7.24.4",

Uh oh!

Conversation

Saibamen commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist

Screenshots (if any)

Before

After

Uh oh!

Saibamen commented Apr 3, 2024

Uh oh!

CommanderStorm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CommanderStorm Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Saibamen commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CommanderStorm commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Saibamen commented Apr 3, 2024 •

edited

Loading

CommanderStorm left a comment •

edited

Loading

CommanderStorm Apr 4, 2024 •

edited

Loading

Saibamen commented Apr 4, 2024 •

edited

Loading

CommanderStorm commented Apr 4, 2024 •

edited

Loading