Skip to content

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

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

Conversation

OiYouYeahYou
Copy link

The most important part of these changes is deprivileging the user from root to node 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.

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
Copy link
Author

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
Copy link
Author

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

Copy link
Author

@OiYouYeahYou OiYouYeahYou left a 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]

@brookslybrand brookslybrand requested a review from jacob-ebey May 1, 2025 21:52
@brookslybrand
Copy link
Contributor

@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 Dockerfiles in the repo though? If we're gonna make this change I'd want it to be reflected in all of them

@MichaelDeBoey
Copy link
Member

CC/ @rubys Do we need any of these changes in https://github.com/fly-apps/dockerfile-node as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants