-
Notifications
You must be signed in to change notification settings - Fork 85
Set node
as user in Dockerfile + some refactoring
#91
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
base: main
Are you sure you want to change the base?
Conversation
The most important part of these changes is deprivileging the user from `root` to `node` to mitigate escalation attacks. https://github.com/nodejs/docker-node/blob/main/docs/BestPractices.md#non-root-user Also, this refactors for simplicity by reducing much of the repetition
@@ -1,22 +1,17 @@ | |||
FROM node:20-alpine AS development-dependencies-env | |||
COPY . /app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm uncertain if this was a typo or intentional. I can see that maybe in some cases there's a postbuild in the install, but this copy busts the cache basically every time
# Build app and prepare `node_modules` for production | ||
# This second install will remove the omitted deps automatically | ||
RUN npm run build \ | ||
&& npm ci --omit=dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting this in a the same stage as the previous one to reduce network, disk and time usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[obligatory message post code comments]
@OiYouYeahYou I'm no Docker expert, but I'll try to get someone on the team to take a look Would you update all of the |
CC/ @rubys Do we need any of these changes in https://github.com/fly-apps/dockerfile-node as well? |
The most important part of these changes is deprivileging the user from
root
tonode
to mitigate escalation attacks.The refactoring I've done is in the attempt of being a simple but solid Dockerfile for new and novice users by making it a little easier to understand with fewer repeated commands. Any resultant changes to built images should be trivial at best, which I've also checked with Go Dive to confirm.
There are other Dockerfiles in this repo which could be updated, but I want to open up with this first to test the waters.