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

#2052: update vulnerable npm dependencies (npm audit fix) #2087

Merged
merged 10 commits into from
Oct 19, 2024

Conversation

kcaran
Copy link

@kcaran kcaran commented May 21, 2024

Hi. In this branch I updated all the node packages to the very latest versions that I could. Now the trivy scans pass and github actions no longer warn about using node 16.

The biggest issue I had was stylelint, which checks the default Sass files. It changed significantly, but older versions rely on postcss, and that had a vulnerability in the older versions.

I didn't touch Docker but I think everything else is working fine.

@nodiscc
Copy link
Member

nodiscc commented May 22, 2024

Thanks!

How did you manage? I was stuck with npm audit fix errors in #2052

Your patch seems to break Docker builds though https://github.com/shaarli/Shaarli/actions/runs/9180779024/job/25245995113?pr=2087

#25 [node 3/3] RUN cd shaarli     && yarnpkg install     && yarnpkg run build     && rm -rf node_modules
#25 0.222 yarn install v1.22.18
#25 0.319 [1/4] Resolving packages...
#25 0.538 warning Resolution field "string-width@4.2.3" is incompatible with requested version "string-width@^5.1.2"
#25 0.541 warning Resolution field "string-width@4.2.3" is incompatible with requested version "string-width@^5.0.1"
#25 0.651 [2/4] Fetching packages...
#25 7.997 error babel-loader@9.1.3: The engine "node" is incompatible with this module. Expected version ">= 14.15.0". Got "12.22.12"
#25 8.004 error Found incompatible module.
#25 8.004 info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
#25 ERROR: process "/bin/sh -c cd shaarli     && yarnpkg install     && yarnpkg run build     && rm -rf node_modules" did not complete successfully: exit code: 1
------
 > [node 3/3] RUN cd shaarli     && yarnpkg install     && yarnpkg run build     && rm -rf node_modules:
0.222 yarn install v1.22.18
0.319 [1/4] Resolving packages...
0.538 warning Resolution field "string-width@4.2.3" is incompatible with requested version "string-width@^5.1.2"
0.541 warning Resolution field "string-width@4.2.3" is incompatible with requested version "string-width@^5.0.1"
0.651 [2/4] Fetching packages...
7.997 error babel-loader@9.1.3: The engine "node" is incompatible with this module. Expected version ">= 14.15.0". Got "12.22.12"
8.004 error Found incompatible module.
8.004 info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
------
WARNING: No output specified with docker-container driver. Build result will only remain in the build cache. To push result image into registry use --push or to load image into docker use --load
Dockerfile:21
--------------------
  20 |     COPY --from=composer /app/shaarli shaarli
  21 | >>> RUN cd shaarli \
  22 | >>>     && yarnpkg install \
  23 | >>>     && yarnpkg run build \
  24 | >>>     && rm -rf node_modules
  25 |     
--------------------
ERROR: failed to solve: process "/bin/sh -c cd shaarli     && yarnpkg install     && yarnpkg run build     && rm -rf node_modules" did not complete successfully: exit code: 1

@nodiscc nodiscc added security dependencies Pull requests that update a dependency file labels May 22, 2024
@nodiscc nodiscc added this to the 0.14.0 milestone May 22, 2024
@kcaran
Copy link
Author

kcaran commented May 22, 2024

Hi. I think I fixed the docker builds by upgrading node (and alpine which I saw you did in another branch).

For the dependencies, I upgraded everything manually. First I edited package.json with all asterisks to get the latest versions of everything. Then I downgraded anything that was broken (and I couldn't fix). Then I put the versions that worked back into package.json.

I used Node 18 because that is what I'm using on my servers, but I'm pretty sure everything would work with Node 20.

@nodiscc
Copy link
Member

nodiscc commented May 23, 2024

👍

I think your last changes broke the PR, there are now 35 commits in this PR, some from 2020... https://github.com/shaarli/Shaarli/pull/2087/commits. Can you please check and/or rebase on master?

@kcaran
Copy link
Author

kcaran commented May 27, 2024

Sorry about that. I did a rebase and I think it looks ok now.

@nodiscc nodiscc self-requested a review July 2, 2024 11:37
nodiscc
nodiscc previously approved these changes Aug 18, 2024
@nodiscc nodiscc removed the in review label Aug 18, 2024
@nodiscc nodiscc dismissed their stale review August 18, 2024 08:24

something wrong with fonts

@nodiscc
Copy link
Member

nodiscc commented Aug 18, 2024

I just tried building and running the Docker image, it seems there is something wrong with icon fonts, they are missing from the image:

~/GIT/Shaarli (npm-audit-fix =)$ docker build -t shaarli:npm-audit-fix .
...
Successfully tagged localhost/shaarli:npm-audit-fix
~/GIT/Shaarli (npm-audit-fix =)$ docker run --name shaarli --publish 8000:80 localhost/shaarli:npm-audit-fix
  • Access http://localhost:8000 in a browser
  • Fill in the setup page
  • All font-based icons/glyphs are missing

image

image

~/GIT/Shaarli (npm-audit-fix =)$ docker exec -it shaarli sh
Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
/var/www # ls shaarli/tpl/default/css/
markdown.min.css  shaarli.min.css

Edit: can confirm that icons are properly displayed when building from master

Copy link
Member

@nodiscc nodiscc left a comment

Choose a reason for hiding this comment

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

^ see comment above

@kcaran kcaran requested a review from nodiscc August 19, 2024 19:30
@kcaran
Copy link
Author

kcaran commented Aug 19, 2024

I updated webpack.config.js, which I think broke with version 5. I was able to run the default and vintage themes both locally and through docker. I'll try to run a docker instance next time: it helps ensure that the environment is completely clean.

Copy link
Member

@nodiscc nodiscc left a comment

Choose a reason for hiding this comment

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

successfully tested (installed from scratch, tested, imported data from another shaarli instance, tested most features)

Copy link
Member

@nodiscc nodiscc left a comment

Choose a reason for hiding this comment

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

There are two unrelated commits in this PR

image

@kcaran Is it ok to rebase this branch and drop them?

@kcaran
Copy link
Author

kcaran commented Oct 18, 2024

Of course, it's your show. Those changes are from an early pull request:

#2086

@nodiscc nodiscc merged commit 8a541c7 into shaarli:master Oct 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file in review security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants