Conversation
Docker image by removing gosu and fixing entrypoint Remove gosu from the final image since the container now runs as the app user via USER directive. Simplify the entrypoint to run migrations directly and exec the server as PID 1 for proper signal handling.
📝 WalkthroughWalkthroughThe pull request simplifies Docker container execution by removing the gosu dependency and refactoring how the application runs. The Dockerfile now explicitly sets the container to run as a non-root user, while the entrypoint script is streamlined to remove permission management and user switching logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile (1)
48-49: Use a fixed UID/GID for theappuser to make volume permission management predictable
useradd -rauto-assigns a UID from the system range (typically < 1000), and the exact value depends on which other system accounts already exist in thenode:25-slimbase image. If the base image ever adds or reorders its own system users, the UID shifts silently. Operators who pre-configure host directory permissions (e.g.,chown 999:999 /srv/data) have no stable UID to target.Prefer an explicit, documented UID — ideally exposed as a build-arg for flexibility:
🔧 Proposed fix with stable UID
+ARG APP_UID=10001 +ARG APP_GID=10001 + RUN apt-get update && apt-get install -y wget openssl ca-certificates && rm -rf /var/lib/apt/lists/* && \ - groupadd -r app && useradd -r -g app -m -d /home/app app + groupadd -g ${APP_GID} app && \ + useradd -u ${APP_UID} -g app -m -d /home/app appThis gives operators a predictable UID/GID to set on their host volume directories and lets them override it at build time if needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 48 - 49, The Dockerfile currently creates the app group/user with auto-assigned IDs; change to use explicit, overridable UID/GID build args so host volume ownership is predictable: add ARGs like APP_UID and APP_GID with sensible defaults (e.g., 999), then replace the groupadd -r and useradd -r invocation with groupadd -g "$APP_GID" app and useradd -u "$APP_UID" -g app -m -d /home/app app (keeping the same home and name), and document the build-arg so operators can override the UID/GID at build time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/docker-entrypoint.sh`:
- Around line 4-6: The entrypoint now runs as the non-root "app" user and fails
when runtime-mounted volumes under /app/database or /app/uploads are owned by
the host; modify scripts/docker-entrypoint.sh to perform a conditional
chown/chgrp of /app/database and /app/uploads when running as root (or detect
mismatched ownership) before running "npx prisma migrate deploy", then drop
privileges to the "app" user to run "exec npx tsx server.ts" (use a lightweight
su-exec or gosu wrapper like su-exec to switch from root to the app user);
alternatively, if you prefer not to change permissions at runtime, add
documentation and Dockerfile build args exposing APP_UID/APP_GID so operators
can pre-set host directory ownership to match the "app" user.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 48-49: The Dockerfile currently creates the app group/user with
auto-assigned IDs; change to use explicit, overridable UID/GID build args so
host volume ownership is predictable: add ARGs like APP_UID and APP_GID with
sensible defaults (e.g., 999), then replace the groupadd -r and useradd -r
invocation with groupadd -g "$APP_GID" app and useradd -u "$APP_UID" -g app -m
-d /home/app app (keeping the same home and name), and document the build-arg so
operators can override the UID/GID at build time.
| # Run migrations and start app | ||
| npx prisma migrate deploy | ||
| exec npx tsx server.ts |
There was a problem hiding this comment.
Runtime-mounted volume permissions may break database writes and file uploads
The old entrypoint ran as root first, allowing it to chown the data directories before switching to app. Now that the container starts directly as app, any volume mounted at /app/database or /app/uploads at runtime retains the host directory's ownership. If that ownership doesn't match the app user's UID/GID, npx prisma migrate deploy (and subsequent DB writes / file uploads) will fail with permission errors — a silent regression for anyone using named or bind-mount volumes.
Common deployment pattern that will break:
docker run -v /srv/hemmelig/data:/app/database \
-v /srv/hemmelig/uploads:/app/uploads \
hemmeligIf /srv/hemmelig/data is owned by root:root or any UID that doesn't match the app system user, the migration step will immediately error out.
Mitigation options (pick one):
- Keep a lightweight root → user handoff for volume fixup (most backward-compatible):
🔧 Proposed entrypoint with conditional volume permission fix
#!/bin/sh
set -e
-# Run migrations and start app
-npx prisma migrate deploy
-exec npx tsx server.ts
+# Fix ownership of mounted volumes if running as root (e.g. via docker run --user)
+# or if invoked by an orchestrator that maps host UIDs onto the volume.
+if [ "$(id -u)" = "0" ]; then
+ chown -R app:app /app/database /app/uploads
+ exec su-exec app "$0" "$@"
+fi
+
+# Run migrations and start app
+npx prisma migrate deploy
+exec npx tsx server.ts(requires adding su-exec — a tiny, static binary — to the final image instead of the heavier gosu)
- Document the required host permissions and expose the UID/GID as build args so operators can match their host directories (see Dockerfile comment below).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/docker-entrypoint.sh` around lines 4 - 6, The entrypoint now runs as
the non-root "app" user and fails when runtime-mounted volumes under
/app/database or /app/uploads are owned by the host; modify
scripts/docker-entrypoint.sh to perform a conditional chown/chgrp of
/app/database and /app/uploads when running as root (or detect mismatched
ownership) before running "npx prisma migrate deploy", then drop privileges to
the "app" user to run "exec npx tsx server.ts" (use a lightweight su-exec or
gosu wrapper like su-exec to switch from root to the app user); alternatively,
if you prefer not to change permissions at runtime, add documentation and
Dockerfile build args exposing APP_UID/APP_GID so operators can pre-set host
directory ownership to match the "app" user.
|
@2franix no worries :D I can close whenever something is ready. To merge this, I need to write the release notes, as it will break the setup for the ones already running Hemmelig. |
2 participants
#492
Summary by CodeRabbit