fix(docker): address external review of the all-in-one image (#1042)
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, <title>PicPeak</title>,
/backup -> /data/backup with both seeded subdirectories, /photos + /thumbnails
+ /api back to 404 while /setup /impressum /gallery/x /admin/login stay 200.
This commit is contained in:
@@ -436,11 +436,11 @@ jobs:
|
|||||||
file: ./Dockerfile.aio
|
file: ./Dockerfile.aio
|
||||||
platforms: ${{ matrix.platform }}
|
platforms: ${{ matrix.platform }}
|
||||||
labels: ${{ steps.meta-backend.outputs.labels }}
|
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
|
# ignore-error: a flaky GitHub Actions cache write ("error writing
|
||||||
# layer blob: not_found") must not fail an otherwise-successful build
|
# layer blob: not_found") must not fail an otherwise-successful build
|
||||||
# that already pushed the image.
|
# 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' }}
|
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: |
|
build-args: |
|
||||||
CACHEBUST=${{ github.run_number }}
|
CACHEBUST=${{ github.run_number }}
|
||||||
@@ -505,7 +505,7 @@ jobs:
|
|||||||
# Distinct category per arch so the Security tab surfaces
|
# Distinct category per arch so the Security tab surfaces
|
||||||
# per-platform findings independently — an amd64-only CVE in
|
# per-platform findings independently — an amd64-only CVE in
|
||||||
# a base layer doesn't get masked by the arm64 scan.
|
# 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:
|
merge-aio:
|
||||||
needs: build-aio
|
needs: build-aio
|
||||||
|
|||||||
+23
-2
@@ -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
|
# exiftool — pulls the embedded preview out of RAW/DNG uploads
|
||||||
# dumb-init — PID 1 signal handling
|
# dumb-init — PID 1 signal handling
|
||||||
# su-exec — the root → nodejs privilege drop in wait-for-db.sh (#484)
|
# 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 && \
|
fontconfig ttf-dejavu ttf-liberation poppler-utils exiftool && \
|
||||||
fc-cache -f
|
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.
|
# The built SPA. SERVE_FRONTEND/FRONTEND_DIR below point server.js at it.
|
||||||
COPY --from=frontend-builder --chown=nodejs:nodejs /build/dist ./public
|
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
|
||||||
|
|
||||||
RUN chmod -R a+r /app && chmod +x wait-for-db.sh
|
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 && \
|
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 \
|
LOG_DIR=/data/logs \
|
||||||
PORT=3000
|
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 \
|
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
|
chown -R nodejs:nodejs /data
|
||||||
|
|
||||||
VOLUME ["/data"]
|
VOLUME ["/data"]
|
||||||
|
|||||||
+7
-1
@@ -962,8 +962,14 @@ app.use('/api', notFoundHandler);
|
|||||||
// which is exactly what try_files means. GET/HEAD only: a stray POST should
|
// which is exactly what try_files means. GET/HEAD only: a stray POST should
|
||||||
// still 404 rather than be handed an HTML page.
|
// still 404 rather than be handed an HTML page.
|
||||||
if (typeof serveFrontendIndexPath === 'string') {
|
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) => {
|
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);
|
return res.sendFile(serveFrontendIndexPath);
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -62,6 +62,7 @@ Everything that must survive a container replacement lives under `/data`:
|
|||||||
| `/data/db` | `picpeak.db` (+ `-wal`/`-shm`) and `SETUP_TOKEN` |
|
| `/data/db` | `picpeak.db` (+ `-wal`/`-shm`) and `SETUP_TOKEN` |
|
||||||
| `/data/storage` | originals, thumbnails, archives |
|
| `/data/storage` | originals, thumbnails, archives |
|
||||||
| `/data/logs` | application logs |
|
| `/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
|
One mount point is the whole point. Back up `/data` and you have backed up the
|
||||||
install.
|
install.
|
||||||
@@ -80,7 +81,8 @@ Only `JWT_SECRET` is required. Everything else has a working default.
|
|||||||
| `PORT` | `3000` | Listen port inside the container. |
|
| `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. |
|
| `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. |
|
| `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
|
### Using an external PostgreSQL
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user