From b507932d69647ed6ca90b68606a48e65e3e01cbc Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sun, 16 Aug 2026 23:25:48 +0200 Subject: [PATCH] fix(docker): address round-2 review of the all-in-one image (#1042) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reverted the engine-resolution reordering. Resolving before the PostgreSQL wait broke a real upgrade path: probePgData deliberately reports "occupied" when Postgres is unreachable, and the target database may not exist until the wait loop creates it — so an install that declares no DATABASE_CLIENT but has a populated legacy SQLite file would resolve to ambiguous-both-populated and refuse to boot. The resolver is back in its original position and the wait is now skipped only when sqlite3 is EXPLICITLY declared, which is what this image does. Installs that declare nothing keep today's ordering byte for byte. - Backup subdirectories are created at startup. Creating /data/backup alone was not enough: on a bind mount the subdirectories baked into the image are hidden, and the backup services do not create them, so /backup/picpeak and /backup/database failed with ENOENT. - BACKUP_DIR points at the real path, not the /backup symlink. `chown -R` on a symlink argument acts on the link, so the target under a bind-mounted /data was never adopted and the writability preflight failed as UID 1001. - Branding is rendered at startup, not baked at build. A build-time sed fixed the literal-token bug but silently made BRAND_TITLE/BRAND_DESCRIPTION inert for this image. docker-entrypoint.aio.sh keeps index.html as a template and renders it with envsubst before exec'ing wait-for-db.sh — same contract, same defaults and same two-variable allowlist as the frontend image. - /fonts/ added to the SPA fallback exclusions. Both font secureStatic mounts call next() on a miss, so a missing font was answering 200 text/html. Not changed: the workflow_dispatch push opt-out. That push-decision block is byte-identical in build-backend, build-aio and build-frontend, so it is pre-existing behaviour rather than something this job introduces; the reviewer agreed on being shown the evidence that fixing it only here would make AIO the odd one out. Verified on a rebuilt image, named volume and bind mount: both healthy, default title "PicPeak" and BRAND_TITLE="Studio Nord Galerie" rendering in and og:title, /backup/{picpeak,database} present and writable on the host side. --- Dockerfile.aio | 27 +++++++----- backend/server.js | 2 +- backend/wait-for-db.sh | 92 +++++++++++++++++++++++----------------- docker-entrypoint.aio.sh | 35 +++++++++++++++ 4 files changed, 106 insertions(+), 50 deletions(-) create mode 100644 docker-entrypoint.aio.sh diff --git a/Dockerfile.aio b/Dockerfile.aio index a1a92728..ca4d0746 100644 --- a/Dockerfile.aio +++ b/Dockerfile.aio @@ -95,7 +95,8 @@ RUN rm -rf /usr/local/lib/node_modules/npm /usr/local/bin/npm /usr/local/bin/npx # not provide that executable. The compose backend image omits it because # it always runs Postgres; this image defaults to SQLite, so without it the # built-in database backup fails with ENOENT. -RUN apk add --no-cache dumb-init postgresql-client sqlite ffmpeg su-exec \ +# gettext — envsubst, used by docker-entrypoint.aio.sh to render index.html +RUN apk add --no-cache dumb-init postgresql-client sqlite ffmpeg su-exec gettext \ fontconfig ttf-dejavu ttf-liberation poppler-utils exiftool && \ fc-cache -f @@ -108,14 +109,15 @@ COPY --chown=nodejs:nodejs backend/ ./ COPY --from=frontend-builder --chown=nodejs:nodejs /build/dist ./public # index.html ships with ${BRAND_TITLE} / ${BRAND_DESCRIPTION} placeholders that -# frontend/docker-entrypoint.sh substitutes at container start. This image runs -# no nginx and therefore never runs that entrypoint, so without this every AIO -# install would serve a literal "${BRAND_TITLE}" as its <title> and in every -# og:/twitter: card. Render them here with the same defaults that entrypoint -# applies when the vars are unset. -RUN sed -i 's/\${BRAND_TITLE}/PicPeak/g; s/\${BRAND_DESCRIPTION}/Photo gallery shared with PicPeak./g' \ - ./public/index.html && \ - ! grep -q '\${BRAND_' ./public/index.html +# frontend/docker-entrypoint.sh renders at container start. This image runs no +# nginx and never invokes that entrypoint, so keep the built file as a template +# and render it on startup instead (docker-entrypoint.aio.sh) — baking the +# defaults at build time would fix the literal-token bug but silently make +# BRAND_TITLE/BRAND_DESCRIPTION inert for this image. +RUN mv ./public/index.html ./public/index.html.template + +COPY --chown=nodejs:nodejs docker-entrypoint.aio.sh /usr/local/bin/docker-entrypoint.aio.sh +RUN chmod +x /usr/local/bin/docker-entrypoint.aio.sh RUN chmod -R a+r /app && chmod +x wait-for-db.sh @@ -142,6 +144,11 @@ ENV SERVE_FRONTEND=true \ DATABASE_PATH=/data/db/picpeak.db \ STORAGE_PATH=/data/storage \ LOG_DIR=/data/logs \ + # Point the startup chown/preflight at the REAL directory, not the /backup + # symlink: `chown -R` on a symlink argument acts on the link itself, so the + # target under a bind-mounted /data would never be adopted and the + # writability preflight would fail as UID 1001. + BACKUP_DIR=/data/backup \ PORT=3000 # /backup is where migrations 029 + 030 seed the built-in backup destinations @@ -169,4 +176,4 @@ HEALTHCHECK --interval=30s --timeout=5s --start-period=60s --retries=3 \ # No USER directive: the container starts as root so wait-for-db.sh can chown a # bind-mounted /data to UID 1001 before dropping privileges via su-exec (#484). ENTRYPOINT ["dumb-init", "--"] -CMD ["./wait-for-db.sh", "node", "server.js"] +CMD ["/usr/local/bin/docker-entrypoint.aio.sh", "node", "server.js"] diff --git a/backend/server.js b/backend/server.js index 3c29eb5e..928388d2 100644 --- a/backend/server.js +++ b/backend/server.js @@ -967,7 +967,7 @@ if (typeof serveFrontendIndexPath === 'string') { // /uploads are backend-owned file routes whose middleware calls next() when // the file is missing, and swallowing that into a 200 index.html would turn // a missing photo into an HTML body served under an image URL. - const BACKEND_OWNED = ['/api/', '/photos/', '/thumbnails/', '/uploads/', '/health']; + const BACKEND_OWNED = ['/api/', '/photos/', '/thumbnails/', '/uploads/', '/fonts/', '/health']; app.get('*', (req, res, next) => { if (BACKEND_OWNED.some((prefix) => req.path.startsWith(prefix))) return next(); return res.sendFile(serveFrontendIndexPath); diff --git a/backend/wait-for-db.sh b/backend/wait-for-db.sh index 02f3ba35..417b0e14 100755 --- a/backend/wait-for-db.sh +++ b/backend/wait-for-db.sh @@ -29,7 +29,7 @@ unset _pair _var _file _cur # single-container image (#1042) points all three under one mounted volume, # so these must follow the same env vars the app itself reads rather than # hard-coding /app — otherwise the checks below guard directories nothing uses. -DATA_DIRS="${STORAGE_PATH:-/app/storage} ${DATA_DIR:-/app/data} ${LOG_DIR:-/app/logs}" +DATA_DIRS="${STORAGE_PATH:-/app/storage} ${DATA_DIR:-/app/data} ${LOG_DIR:-/app/logs} ${BACKUP_DIR:-/backup}" # Create the roots before touching them. With the compose layout each of the # three is its own mount point, so they always exist — but the single-container @@ -72,46 +72,20 @@ for _dir in $DATA_DIRS; do fi done -# Resolve which database engine this boot should use (#1038) BEFORE migrations -# run — and, since #1042, before the PostgreSQL readiness wait below, so a -# SQLite install never blocks on a Postgres that will never answer. +# Everything below this point is PostgreSQL readiness. On SQLite the database is +# a file this process opens itself — there is no daemon to wait for — so the +# loop would block forever on a host that will never answer. That is the +# single-container case (#1042), where the image DECLARES sqlite3. # -# An install that has been unknowingly running on SQLite (the image used to -# leave NODE_ENV unset, so -# knexfile.js fell back to sqlite3 and ignored DB_HOST/DB_USER/DB_PASSWORD) -# keeps serving from its SQLite file instead of coming up against an empty -# Postgres. The exported value survives the `exec` below, so the migration -# runner and the server agree on the engine. -RESOLVED_DB_CLIENT="$(node scripts/resolve-db-engine.js)" -RESOLVER_STATUS=$? -# Exit 3 means two populated databases with no record of which is current -# (#1038). Starting either would hide the other's data, so stop here — the -# resolver has already printed what to do. -if [ "$RESOLVER_STATUS" = "3" ]; then - exit 1 -fi -# Validate rather than trust: anything unexpected on stdout (a stray log line -# from a library that writes to the console) must not become DATABASE_CLIENT, -# which would break knexfile for every process that follows. -case "$RESOLVED_DB_CLIENT" in - pg|sqlite3) - export DATABASE_CLIENT="$RESOLVED_DB_CLIENT" - ;; - "") - >&2 echo "Database engine resolver returned nothing; falling back to the configured client." - ;; - *) - >&2 echo "Database engine resolver returned an unexpected value; ignoring it and falling back to the configured client." - ;; -esac - -# Everything below this point is PostgreSQL readiness. On SQLite the -# database is a file this process opens itself — there is no daemon to -# wait for — so the wait loop would block forever on a host that will -# never answer. That is exactly the single-container case (#1042), where -# SQLite is the documented default and no DB_HOST is set. +# The test is deliberately the raw env var, not the resolver's answer. Resolving +# first would change behaviour for installs that declare nothing: probePgData +# reports "occupied" when Postgres is unreachable, and the target database may +# not exist until the loop below creates it — so an unset-client install with a +# populated legacy SQLite file would resolve to ambiguous-both-populated and +# refuse to start. Those installs keep today's ordering exactly: wait, create, +# then resolve. if [ "${DATABASE_CLIENT:-}" = "sqlite3" ]; then - echo "Database engine: sqlite3 — skipping the PostgreSQL readiness wait." + echo "Database engine: sqlite3 declared — skipping the PostgreSQL readiness wait." else host="${DB_HOST:-postgres}" @@ -191,9 +165,49 @@ echo "Ensuring storage directories exist..." STORAGE_BASE="${STORAGE_PATH:-/app/storage}" mkdir -p "$STORAGE_BASE/events/active" "$STORAGE_BASE/events/archived" "$STORAGE_BASE/thumbnails" 2>/dev/null || true +# Backup destinations seeded by migrations 029 + 030 (/backup/picpeak and +# /backup/database). Creating the root alone is not enough: on a bind mount the +# subdirectories baked into the image are hidden, and the backup services do not +# create them, so a scheduled or manual backup fails with ENOENT. +BACKUP_BASE="${BACKUP_DIR:-/backup}" +mkdir -p "$BACKUP_BASE/picpeak" "$BACKUP_BASE/database" 2>/dev/null || true + # Resolve which database engine this boot should use (#1038) BEFORE migrations # run, while the Postgres target is still untouched. An install that has been +# Resolve which database engine this boot should use (#1038) BEFORE migrations +# run — and, since #1042, before the PostgreSQL readiness wait below, so a +# SQLite install never blocks on a Postgres that will never answer. +# +# An install that has been unknowingly running on SQLite (the image used to +# leave NODE_ENV unset, so +# knexfile.js fell back to sqlite3 and ignored DB_HOST/DB_USER/DB_PASSWORD) +# keeps serving from its SQLite file instead of coming up against an empty +# Postgres. The exported value survives the `exec` below, so the migration +# runner and the server agree on the engine. +RESOLVED_DB_CLIENT="$(node scripts/resolve-db-engine.js)" +RESOLVER_STATUS=$? +# Exit 3 means two populated databases with no record of which is current +# (#1038). Starting either would hide the other's data, so stop here — the +# resolver has already printed what to do. +if [ "$RESOLVER_STATUS" = "3" ]; then + exit 1 +fi +# Validate rather than trust: anything unexpected on stdout (a stray log line +# from a library that writes to the console) must not become DATABASE_CLIENT, +# which would break knexfile for every process that follows. +case "$RESOLVED_DB_CLIENT" in + pg|sqlite3) + export DATABASE_CLIENT="$RESOLVED_DB_CLIENT" + ;; + "") + >&2 echo "Database engine resolver returned nothing; falling back to the configured client." + ;; + *) + >&2 echo "Database engine resolver returned an unexpected value; ignoring it and falling back to the configured client." + ;; +esac + # Run migrations (use safe runner in production). Invoked via node directly — # the runtime image no longer ships npm (see Dockerfile: its bundled deps kept # tripping CVE scanners while npm itself never runs in production). diff --git a/docker-entrypoint.aio.sh b/docker-entrypoint.aio.sh new file mode 100644 index 00000000..b86166ca --- /dev/null +++ b/docker-entrypoint.aio.sh @@ -0,0 +1,35 @@ +#!/bin/sh +# All-in-one entrypoint (#1042). +# +# The compose deployment renders index.html in the frontend container via +# frontend/docker-entrypoint.sh. This image runs no nginx, so that never +# happens — and Vite does not substitute ${BRAND_TITLE} at build time, so the +# built index.html ships the placeholders verbatim. Without this step every +# AIO install would serve a literal "${BRAND_TITLE}" as its <title> and in +# every og:/twitter: card. +# +# Kept as a startup step rather than a build-time sed so BRAND_TITLE and +# BRAND_DESCRIPTION behave the same way they do for the frontend image: +# set the env var, restart the container, new branding. Baking them at build +# would silently make those variables inert. +set -e + +TEMPLATE=/app/public/index.html.template +RENDERED=/app/public/index.html + +if [ -f "$TEMPLATE" ]; then + # Same defaults frontend/docker-entrypoint.sh applies when the vars are unset. + : "${BRAND_TITLE:=PicPeak}" + : "${BRAND_DESCRIPTION:=Photo gallery shared with PicPeak.}" + + # Substitute ONLY these two, exactly like the frontend entrypoint: letting + # envsubst expand every ${...} it finds would mangle unrelated content. + export BRAND_TITLE BRAND_DESCRIPTION + envsubst '${BRAND_TITLE} ${BRAND_DESCRIPTION}' < "$TEMPLATE" > "$RENDERED" + + # The app runs as UID 1001 after the privilege drop below; the render happens + # while we are still root, so hand the result over explicitly. + chown nodejs:nodejs "$RENDERED" 2>/dev/null || true +fi + +exec ./wait-for-db.sh "$@"