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.

Sign up for GitHub

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

Jump to bottom

Docker tweaks and node update #2911

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Docker tweaks and node update #2911

wants to merge 1 commit into from

Conversation

kingthorin
Copy link
Member

Tested successfully on a Kali VM.

@psiinon
Copy link
Member

psiinon commented Dec 31, 2024 •
edited
Loading

Logo
Checkmarx One – Scan Summary & Details – a9c06f97-a713-4a63-bd30-138615af3e06

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
HIGH Missing User Instruction /Dockerfile: 1
detailsA user should be specified in the dockerfile, otherwise the image will run as root
LOW Healthcheck Instruction Missing /Dockerfile: 1
detailsEnsure that HEALTHCHECK is being used. The HEALTHCHECK instruction tells Docker how to test a container to check that it is still working
Fixed Issues (5)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
HIGH CVE-2024-21538 Npm-cross-spawn-7.0.3
HIGH Missing User Instruction /Dockerfile: 1
MEDIUM CVE-2024-4067 Npm-micromatch-4.0.5
MEDIUM Not Using JSON In CMD And ENTRYPOINT Arguments /Dockerfile: 21
LOW Healthcheck Instruction Missing /Dockerfile: 1

@kingthorin
Copy link
Member Author

Establishing/using a non-root user has proven to be a P.I.T.A. so I abandon that part. This image is only meant for local testing so should be less of an issue.

@psiinon
Copy link
Member

psiinon commented Dec 31, 2024

Thanks! Testing this on my Mac right now...

@psiinon
Copy link
Member

psiinon commented Dec 31, 2024

Works for me 😁
I'll getting a load of errors like: WARN Raw HTML omitted while rendering "/app/site/content/blog/2023-05-23-authentication-tester/index.md" but they could just be things we should have spotted before?

@kingthorin
Copy link
Member Author

kingthorin commented Dec 31, 2024 •
edited
Loading

I suspect it's because of a more complete build with the changes. They've likely been there for a while.

Edit: In fact if you go back to main and build again you'll have a comparison point.

thc202
thc202 reviewed Dec 31, 2024
View reviewed changes
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated
COPY package-lock.json package.json /app/
# Create app directory
Copy link
Member

Choose a reason for hiding this comment

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

Most of these changes are unnecessary and comments are redundant.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can drop the comments.

Copy link
Member

Choose a reason for hiding this comment

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

The COPY of line 9 should be reverted too. I'd also suggest reverting the package lock changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

The build doesn't succeed without the dependency updates.

@kingthorin kingthorin force-pushed the docker-tweaks branch 5 times, most recently from 65dee93 to 3fc336d Compare December 31, 2024 15:53
@kingthorin
Copy link
Member Author

Hopefully that looks better?

kingthorin
kingthorin commented Dec 31, 2024
View reviewed changes
Dockerfile
Comment on lines -9 to +14
COPY .babelrc \
.eslintrc.yml \
.nvmrc \
postcss.config.js \
webpack.common.js \
webpack.dev.js \
COPY .babelrc \
.eslintrc.yml \
.nvmrc \
postcss.config.js \
webpack.common.js \
webpack.dev.js \
Copy link
Member Author

@kingthorin kingthorin Dec 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The file seems to have had mixed line endings (or something)

@kingthorin kingthorin force-pushed the docker-tweaks branch 2 times, most recently from af1a696 to 66e4e69 Compare December 31, 2024 17:08
@kingthorin
Docker tweaks and node update
2df99cf 
Signed-off-by: kingthorin <[email protected]>

Node 22 everywhere

Signed-off-by: kingthorin <[email protected]>

# Conflicts:
#	package-lock.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thc202 thc202 thc202 left review comments

@psiinon psiinon psiinon approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kingthorin @psiinon @thc202