From 167eeaa27103090f02e18a3ae7a1b3ba0ec9d946 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sun, 16 Aug 2026 23:11:51 +0200 Subject: [PATCH] fix(docker): address external review of the all-in-one image (#1042) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings, all verified against the code before applying. - The built-in backup system wrote nowhere. Migrations 029/030 seed /backup/picpeak and /backup/database as destinations, and this image neither created nor mounted /backup — so backups failed, and anything written there would have died with the container. /backup is now a symlink into /data/backup, so the seeded defaults work and the archives land on the volume like everything else. - Database backups need the sqlite3 CLI. DatabaseBackupService spawns it for .backup and PRAGMA integrity_check; the npm module does not provide the binary. The compose image omits it because it always runs Postgres — this one defaults to SQLite, so it failed with ENOENT. Added. - Every AIO install served literal ${BRAND_TITLE}. index.html carries placeholders that frontend/docker-entrypoint.sh substitutes at start, and this image runs no nginx and never invoked it. Rendered at build with the same defaults that entrypoint applies, and the build now fails if any ${BRAND_*} token survives. Browser testing missed this because the SPA rewrites document.title at runtime — the og:/twitter: cards and view-source still showed the raw token. - The SPA fallback swallowed backend file-route 404s. nginx gives /api, /photos, /thumbnails and /health their own location blocks, so try_files never applies to them; excluding only /api/ made the fallback strictly broader than the behaviour it claimed parity with, turning a missing photo into a 200 HTML body under an image URL. Now excludes the same set. - Docs claimed DB_* alone switches the engine. It does not: the image declares DATABASE_CLIENT=sqlite3 and the resolver treats a declared client as explicit, so DATABASE_CLIENT=pg is required. Corrected, and /data/backup added to the documented layout. - The AIO Trivy upload reused the backend's SARIF category, so the two scans replaced each other's results instead of both being retained. - The AIO build reused the backend's buildx cache scope, so two concurrent jobs wrote the same cache object from different Dockerfiles. Verified on a rebuilt image: sqlite3 3.53.2 present, PicPeak, /backup -> /data/backup with both seeded subdirectories, /photos + /thumbnails + /api back to 404 while /setup /impressum /gallery/x /admin/login stay 200. --- .github/workflows/docker-build.yml | 6 +++--- Dockerfile.aio | 25 +++++++++++++++++++++++-- backend/server.js | 8 +++++++- docs/single-container.md | 4 +++- 4 files changed, 36 insertions(+), 7 deletions(-) diff --git a/.github/workflows/docker-build.yml b/.github/workflows/docker-build.yml index 9d1c48ea..97a92ec2 100644 --- a/.github/workflows/docker-build.yml +++ b/.github/workflows/docker-build.yml @@ -436,11 +436,11 @@ jobs: file: ./Dockerfile.aio platforms: ${{ matrix.platform }} labels: ${{ steps.meta-backend.outputs.labels }} - cache-from: type=gha,scope=backend-${{ env.PLATFORM_PAIR }} + cache-from: type=gha,scope=aio-${{ env.PLATFORM_PAIR }} # ignore-error: a flaky GitHub Actions cache write ("error writing # layer blob: not_found") must not fail an otherwise-successful build # that already pushed the image. - cache-to: type=gha,mode=max,scope=backend-${{ env.PLATFORM_PAIR }},ignore-error=true + cache-to: type=gha,mode=max,scope=aio-${{ env.PLATFORM_PAIR }},ignore-error=true outputs: ${{ steps.push-decision.outputs.push == 'true' && format('type=image,name={0}/{1},push-by-digest=true,name-canonical=true,push=true', env.REGISTRY, env.AIO_IMAGE_NAME) || 'type=cacheonly' }} build-args: | CACHEBUST=${{ github.run_number }} @@ -505,7 +505,7 @@ jobs: # Distinct category per arch so the Security tab surfaces # per-platform findings independently — an amd64-only CVE in # a base layer doesn't get masked by the arm64 scan. - category: 'backend-vulnerabilities-${{ env.PLATFORM_PAIR }}' + category: 'aio-vulnerabilities-${{ env.PLATFORM_PAIR }}' merge-aio: needs: build-aio diff --git a/Dockerfile.aio b/Dockerfile.aio index 0e5be7a5..a1a92728 100644 --- a/Dockerfile.aio +++ b/Dockerfile.aio @@ -90,7 +90,12 @@ RUN rm -rf /usr/local/lib/node_modules/npm /usr/local/bin/npm /usr/local/bin/npx # exiftool — pulls the embedded preview out of RAW/DNG uploads # dumb-init — PID 1 signal handling # su-exec — the root → nodejs privilege drop in wait-for-db.sh (#484) -RUN apk add --no-cache dumb-init postgresql-client ffmpeg su-exec \ +# sqlite — DatabaseBackupService.createSQLiteBackup() spawns the `sqlite3` +# CLI for `.backup` and PRAGMA integrity_check. The npm sqlite3 module does +# 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 \ fontconfig ttf-dejavu ttf-liberation poppler-utils exiftool && \ fc-cache -f @@ -102,6 +107,16 @@ COPY --chown=nodejs:nodejs backend/ ./ # The built SPA. SERVE_FRONTEND/FRONTEND_DIR below point server.js at it. 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 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 + RUN chmod -R a+r /app && chmod +x wait-for-db.sh RUN printf '<?xml version="1.0"?>\n<!DOCTYPE fontconfig SYSTEM "fonts.dtd">\n<fontconfig>\n <dir>/app/assets/fonts</dir>\n</fontconfig>\n' > /etc/fonts/conf.d/99-picpeak-fonts.conf && \ @@ -129,8 +144,14 @@ ENV SERVE_FRONTEND=true \ LOG_DIR=/data/logs \ PORT=3000 +# /backup is where migrations 029 + 030 seed the built-in backup destinations +# (/backup/picpeak and /backup/database). Nothing mounts it here, so it is +# symlinked into the volume: the seeded defaults keep working AND the archives +# land on /data instead of inside a container that gets replaced on upgrade. RUN mkdir -p /data/db /data/storage/events/active /data/storage/events/archived \ - /data/storage/thumbnails /data/logs && \ + /data/storage/thumbnails /data/logs \ + /data/backup/picpeak /data/backup/database && \ + ln -s /data/backup /backup && \ chown -R nodejs:nodejs /data VOLUME ["/data"] diff --git a/backend/server.js b/backend/server.js index 06b06d1e..3c29eb5e 100644 --- a/backend/server.js +++ b/backend/server.js @@ -962,8 +962,14 @@ app.use('/api', notFoundHandler); // which is exactly what try_files means. GET/HEAD only: a stray POST should // still 404 rather than be handed an HTML page. if (typeof serveFrontendIndexPath === 'string') { + // nginx carves these out as their own `location` blocks, so try_files never + // applies to them. The fallback has to mirror that: /photos, /thumbnails and + // /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']; app.get('*', (req, res, next) => { - if (req.path.startsWith('/api/')) return next(); + if (BACKEND_OWNED.some((prefix) => req.path.startsWith(prefix))) return next(); return res.sendFile(serveFrontendIndexPath); }); } diff --git a/docs/single-container.md b/docs/single-container.md index eed6c815..0775e6d1 100644 --- a/docs/single-container.md +++ b/docs/single-container.md @@ -62,6 +62,7 @@ Everything that must survive a container replacement lives under `/data`: | `/data/db` | `picpeak.db` (+ `-wal`/`-shm`) and `SETUP_TOKEN` | | `/data/storage` | originals, thumbnails, archives | | `/data/logs` | application logs | +| `/data/backup` | built-in backup output (`/backup` is symlinked here) | One mount point is the whole point. Back up `/data` and you have backed up the install. @@ -80,7 +81,8 @@ Only `JWT_SECRET` is required. Everything else has a working default. | `PORT` | `3000` | Listen port inside the container. | | `FRONTEND_URL` | — | Public URL. Set it once you are behind a domain, so emails and share links point at the right host. | | `SMTP_*` | — | Outbound email. Without it, PicPeak runs fine but sends nothing. | -| `DB_HOST`, `DB_USER`, `DB_PASSWORD`, `DB_NAME` | — | Point at an **external** PostgreSQL. Setting these switches the engine off SQLite. | +| `DATABASE_CLIENT` | `sqlite3` | Set to `pg` to use an external PostgreSQL. Required — the image declares `sqlite3`, and the boot resolver treats a declared client as an explicit instruction, so `DB_*` alone will **not** switch engines. | +| `DB_HOST`, `DB_USER`, `DB_PASSWORD`, `DB_NAME` | — | Connection details, used when `DATABASE_CLIENT=pg`. | ### Using an external PostgreSQL