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

Reduce vuln and changed docker images #4590

Closed
wants to merge 9 commits into from

Conversation

bruhwhyamisobad
Copy link

⚠️⚠️⚠️ Since we do not accept all types of pull requests and do not want to waste your time. Please be sure that you have read pull request rules:
https://github.com/louislam/uptime-kuma/blob/master/CONTRIBUTING.md#can-i-create-a-pull-request-for-uptime-kuma

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes some vulnerabilities (by snyk), changed golang docker image to "bookworm", and changed debian based docker image to "bookworm-slim".

Type of change

Please delete any options that are not relevant.

  • Other

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)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

dependabot bot and others added 9 commits March 16, 2024 22:50
Bumps [jsonata](https://github.com/jsonata-js/jsonata) from 2.0.3 to 2.0.4.
- [Release notes](https://github.com/jsonata-js/jsonata/releases)
- [Changelog](https://github.com/jsonata-js/jsonata/blob/master/CHANGELOG.md)
- [Commits](jsonata-js/jsonata@v2.0.3...v2.0.4)

---
updated-dependencies:
- dependency-name: jsonata
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [nodemailer](https://github.com/nodemailer/nodemailer) from 6.6.5 to 6.9.9.
- [Release notes](https://github.com/nodemailer/nodemailer/releases)
- [Changelog](https://github.com/nodemailer/nodemailer/blob/master/CHANGELOG.md)
- [Commits](nodemailer/nodemailer@v6.6.5...v6.9.9)

---
updated-dependencies:
- dependency-name: nodemailer
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [ip](https://github.com/indutny/node-ip) from 2.0.0 to 2.0.1.
- [Commits](indutny/node-ip@v2.0.0...v2.0.1)

---
updated-dependencies:
- dependency-name: ip
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [follow-redirects](https://github.com/follow-redirects/follow-redirects) from 1.15.5 to 1.15.6.
- [Release notes](https://github.com/follow-redirects/follow-redirects/releases)
- [Commits](follow-redirects/follow-redirects@v1.15.5...v1.15.6)

---
updated-dependencies:
- dependency-name: follow-redirects
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [jose](https://github.com/panva/jose) from 4.15.4 to 4.15.5.
- [Release notes](https://github.com/panva/jose/releases)
- [Changelog](https://github.com/panva/jose/blob/v4.15.5/CHANGELOG.md)
- [Commits](panva/jose@v4.15.4...v4.15.5)

---
updated-dependencies:
- dependency-name: jose
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [undici](https://github.com/nodejs/undici) from 5.28.2 to 5.28.3.
- [Release notes](https://github.com/nodejs/undici/releases)
- [Commits](nodejs/undici@v5.28.2...v5.28.3)

---
updated-dependencies:
- dependency-name: undici
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
@Zaid-maker
Copy link
Contributor

Zaid-maker commented Mar 17, 2024

Please refer to the SECURITY before making Pull Requests for Vulnerabilities.

These first needs to be discussed and the fix will be implemented if there is breaking change then it will put on hold!

Copy link
Collaborator

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

As said by @Zaid-maker if there is an actual (=at least somewhat exploitable) vulnerability, please report them via the appropriate channel.
I am not convinced that is the case here ⇒ I am interpreting this as a general bumping of dependencies.

For this, please take more care and actually read the changelog of the packages.
Bumping dependencys is not just updating a version number.

I will be a bit less subtle:
Your PR is from my point quite careless (without a second thought if something else needs to change or explanation why some changes have been made) in bumping versions of dependencies and the impact of that.
Like this, this is not getting merged!

@@ -2,7 +2,7 @@
# Build in Golang
# Run npm run build-healthcheck-armv7 in the host first, another it will be super slow where it is building the armv7 healthcheck
############################################
FROM golang:1.19-buster
FROM golang:bookworm
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't remove the go version.
Updating the go version should be fine as by their stability guarantee, but don't remove it

Copy link
Contributor

Choose a reason for hiding this comment

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

It can updated to 1.21.x

@@ -1,5 +1,5 @@
# If the image changed, the second stage image should be changed too
FROM node:20-bookworm-slim AS base2-slim
FROM node:bookworm-slim AS base2-slim
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't remove the node version here. It is here for a reason

@@ -79,8 +79,8 @@
"@louislam/sqlite3": "15.1.6",
"@vvo/tzdb": "^6.125.0",
"args-parser": "~1.3.0",
"axios": "~0.28.0",
"axios-ntlm": "1.3.0",
"axios": "~1.6.4",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Until axios provides a migration guide or documents what (breaking) changes have been made, just bumping the version is too risky for us.

They litterally only state the folling in their changelog:

There are multiple deprecations, refactors and fixes provided in this release. Please read through the full release notes to see how this may impact your project and use case.

"axios": "~0.28.0",
"axios-ntlm": "1.3.0",
"axios": "~1.6.4",
"axios-ntlm": "1.3.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at said changelog, bumping to 1.3.2 should be fine too

@@ -106,7 +106,7 @@
"iconv-lite": "~0.6.3",
"isomorphic-ws": "^5.0.0",
"jsesc": "~3.0.2",
"jsonata": "^2.0.3",
"jsonata": "^2.0.4",
Copy link
Collaborator

Choose a reason for hiding this comment

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

changelog looks fine for this one

@@ -115,13 +115,13 @@
"liquidjs": "^10.7.0",
"mitt": "~3.0.1",
"mongodb": "~4.17.1",
"mqtt": "~4.3.7",
"mqtt": "~5.3.5",
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are doing a major version bump here with a lot of intermediate versions.

Have you carefully read through the changelog? Have you tested that this feature still works?

"mssql": "~8.1.4",
"mysql2": "~3.6.2",
"nanoid": "~3.3.4",
"node-cloudflared-tunnel": "~1.0.9",
"node-radius-client": "~1.0.0",
"nodemailer": "~6.6.5",
"nodemailer": "~6.9.9",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changelog looks fine for this one

@@ -186,7 +186,7 @@
"qrcode": "~1.5.0",
"rollup-plugin-visualizer": "^5.6.0",
"sass": "~1.42.1",
"stylelint": "^15.10.1",
"stylelint": "^16.1.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a Breaking change!

See https://stylelint.io/changelog/#1600

Removed: Node.js less than 18.12.0 support

There are also some deprecation warnings in the current stle configuration => these need to be migrated first, as they are removed in v16

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea and check the check-linter run it going crazy

@CommanderStorm CommanderStorm marked this pull request as draft March 17, 2024 17:49
@louislam louislam added the question Further information is requested label Mar 18, 2024
@Saibamen
Copy link
Contributor

The safer method will be using NCU https://www.npmjs.com/package/npm-check-updates

And update only PATCH:
ncu --target patch -u

And maybe MINOR, but this requires reading changelogs and tests:
ncu --target MINOR -u

@CommanderStorm
Copy link
Collaborator

The safer method will be

@Saibamen
What do you mean by safer in this context? Am I missing something?

@Saibamen
Copy link
Contributor

If you want to update a lot of packages, the first step is to install the latest patch release, and then minor releases.

See semantic versioning: https://semver.org/

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Mar 20, 2024

the first step is ..

Disagree:
The first step is to read the changelog for every update you do.
I have seen (not in the dependencies of this repo) package authors ship bugfixes which ended up being breaking in patch releases.

@CommanderStorm
Copy link
Collaborator

I am going to close this PR for now. We can reopen if the points noted in #4590 (review) are resolved.

@CommanderStorm CommanderStorm mentioned this pull request Apr 4, 2024
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants