P1: Harden Docker image — tini PID 1, HEALTHCHECK, pinned digest (#171) - #189
Conversation
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Verdict: MERGEABLE ✅ Image-layer only; no runtime code, no schema, no CORS/error-masking change — so nothing here can affect What I checked
Non-blocking nits
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api. |
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving on the basis of the second-pass review comment above: no mid-to-high severity security, compatibility, or degradation issue found, and the downstream contract with mina-explorer / mina-explorer-api holds — GraphQL validation error text reaches errors[].message verbatim, the browser SPA's cross-origin access is preserved, and the real consumer query shapes (including the 2000-block analytics query and the 500-row page crawl) still pass.
Two things this approval does not mean:
- It does not close the non-blocking items in the review comment. Several are worth fixing before or shortly after merge; they are written up there with patches.
- It does not by itself mean the branch is ready to merge.
mainrequires branches to be up to date, so this needs an update-branch (or a rebase, if the branch is conflicting) first, and a few PRs in this series have cross-PR ordering constraints called out in their review comments.
Automated second-pass review — focus: downstream compatibility with mina-explorer / mina-explorer-api.
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
889ae63 to
c5d0ef2
Compare
SanabriaRusso
left a comment
There was a problem hiding this comment.
Approving. I verified the two claims that would have been hard blockers by building and running the image rather than reading it.
tini is genuinely PID 1 and genuinely forwards SIGTERM. I built an image reproducing Dockerfile:16, :24-27, :32-33, :37-38 verbatim and ran it:
PID COMMAND
1 tini
3 node
then sent SIGTERM → container log NODE GOT SIGTERM pid=3, exit code 0. The exec-form ENTRYPOINT ["/sbin/tini", "--"] + CMD ["node", "build/src/index.js"] split at :37-38 is what makes this work; a shell-form CMD would interpose /bin/sh and swallow the signal. tini's default single-child forwarding is sufficient because node is a direct child — no -g needed. #188's graceful shutdown will actually receive the signal, which is the whole point of this PR. apk add --no-cache tini does install to /sbin/tini (confirmed: -rwxr-xr-x root root 23576).
The HEALTHCHECK works — wget exists and /healthcheck is real. This was my main hard-blocker candidate, since a HEALTHCHECK calling a missing binary marks a container unhealthy forever. It's clean: BusyBox wget is at /usr/bin/wget in the alpine base (BusyBox v1.37.0), the combined -qO- short-option form parses correctly, and running the exact command from :33 against a stub server in the container gave exit=0 with the body. inspect reported Health.Status: healthy after two probes — the first fails during startup, which is what --start-period=15s is for.
It targets /healthcheck, which already exists on main at src/server/server.ts:20 — so no dependency on #187's /readiness, and importantly it's yoga's DB-free liveness endpoint, so unlike a readiness ping it won't flap during DB slowness. HEALTHCHECK shell-form runs via /bin/sh -c and bypasses ENTRYPOINT, so tini isn't in that path; loopback wget as nodeuser is fine since the app binds all interfaces.
The digest is real. sha256:fb4cd12c… resolves on Docker Hub to a genuine multi-arch node:20-alpine index (annotation org.opencontainers.image.version: 20-alpine, base alpine:3.23, created 2026-04-15; amd64/arm/arm64/ppc64le/s390x). Same digest on both stages, which is correct.
The APP_COMMAND removal is correct and complete. I grepped the repo — APP_COMMAND="npm run start" in both .env.example.* plus command: ${APP_COMMAND} in docker-compose.yml:88 are the only three occurrences; no doc, script, or workflow references it. It's behaviour-preserving (package.json's start is literally node build/src/index.js, identical to the new CMD) and it's necessary: leaving command: ${APP_COMMAND} would override the image CMD and reinstate npm as an intermediate process, defeating the direct-child signal delivery this PR exists to establish. c5d0ef29 is the right fix.
.dockerignore is safe. The Dockerfile's only COPY sources are package*.json, src, tsconfig.json, schema.graphql; none are matched by any of the ten patterns, and package-lock.json is matched by package*.json and not excluded, so npm ci still works.
One thing I'd like added — non-blocking, but I'd take it in this PR
Add .npmrc to .dockerignore. .github/workflows/build.yaml writes a live GCP access token into .npmrc (//europe-southwest1-npm.pkg.dev/…/:_authToken=$(gcloud auth print-access-token)) before the Docker build, and the rm -f .npmrc cleanup step only runs when needs_version_update == 'true' — so on tag/release builds that file is still on disk when docker/build-push-action runs with context: .. There's no leak today because the Dockerfile never COPYs it and package*.json doesn't glob it, but a future COPY . . would bake a registry token into a published layer. This is exactly what a .dockerignore is for:
.env*
+.npmrc
dbMinor: --start-period=15s may be tight on a cold Compose start where checkSQLSchema() waits on a loading snapshot. Harmless — Docker doesn't act on unhealthy by itself, and retries=3 × interval=30s gives ~105 s before the status flips.
Merge ordering — this PR conflicts with #194, and I'd merge #194 first. Both rewrite the same two FROM lines. #194 moves to node:22-alpine@sha256:16e22a55…; this one pins node:20-alpine@sha256:fb4cd12c…. Beyond the textual conflict there's a semantic one: Node 20 reached EOL on 2026-04-30, so landing this after #194 with a naive resolution would silently roll the runtime back to an unsupported base — and #194 also adds engines: ">=22.12.0", which this base wouldn't satisfy.
Suggest merging #194 first, then rebasing this onto it. The resolution is mechanical — keep #194's digest and layer this PR's hardening on top:
FROM node:22-alpine@sha256:16e22a550f3863206a3f701448c45f7912c6896a62de43add43bb9c86130c3e2 AS build
WORKDIR /app
COPY package*.json ./
RUN npm ci
COPY src ./src
COPY tsconfig.json ./
RUN npm run build
FROM node:22-alpine@sha256:16e22a550f3863206a3f701448c45f7912c6896a62de43add43bb9c86130c3e2
WORKDIR /app
# tini as PID 1: forwards SIGTERM to node (so graceful shutdown runs) and reaps zombies.
RUN apk add --no-cache tini
COPY --from=build /app/node_modules ./node_modules
COPY --from=build /app/build ./build
COPY package*.json ./
COPY schema.graphql ./
RUN addgroup -g 1001 -S nodejs \
&& adduser -S nodeuser -u 1001 \
&& chown -R nodeuser:nodejs /app
USER nodeuser
EXPOSE 8080
HEALTHCHECK --interval=30s --timeout=5s --start-period=15s --retries=3 \
CMD wget -qO- "http://127.0.0.1:${PORT:-8080}/healthcheck" || exit 1
ENTRYPOINT ["/sbin/tini", "--"]
CMD ["node", "build/src/index.js"]I confirmed the node:22 digest is a real multi-arch node:22-alpine (v22.23.1) and re-ran both the PID-1 signal test and the BusyBox-wget HEALTHCHECK test on that base — they behave identically, so the combination is safe.
Also note .env.example.compose / .env.example.lightnet will conflict with #188, which inserts SHUTDOWN_TIMEOUT_MS immediately after PORT while this PR deletes APP_COMMAND immediately before it. Overlapping hunks; both changes should survive.
Downstream: none. No application code, schema, resolver, error-text, or HTTP-behaviour change — purely image packaging. Indirectly positive for both consumers, since tini forwarding SIGTERM is the precondition for #188's drain actually running.
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e938cb9 to
a85112a
Compare
The runtime container ran `npm start` as PID 1, which forwards signals poorly — so SIGTERM didn't cleanly reach node and the new graceful shutdown couldn't run. It also had no container healthcheck and used a floating `node:20-alpine` tag. - Run `node build/src/index.js` directly under `tini` as PID 1, so the process receives SIGTERM and shuts down gracefully (and zombies are reaped). - Add a `HEALTHCHECK` hitting the built-in `/healthcheck` endpoint (honours $PORT). - Pin both stages to the `node:20-alpine` image digest for reproducible, tamper-evident builds (bump via Dependabot — #175). - Add a `.dockerignore` to keep the build context lean. Verified locally: the image builds; runs as non-root (uid 1001); tini 0.19.0 is PID 1; `node build/src/index.js` is the entrypoint chain (reaches buildContext). Closes #171. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QSuak9smCHbp4N17xjjLF6
a85112a to
9b6bc69
Compare
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pins both Dockerfile stages to node:22-alpine@sha256:16e22a55… (v22.23.1, verified by pull and build). Without this, moving to a floating 22 tag would silently drop the digest pin #189 adds, reverting its reproducible/tamper-evident build hardening depending on merge order. Once #192 lands, Dependabot's docker ecosystem keeps the digest fresh. Bumps @types/node to ^22 so the types match the runtime rather than staying on 20. Adds tests pinning the GraphQL-over-HTTP behaviour the mina-explorer depends on: validation errors return 200 (its client throws on non-2xx before reading the body) and carry the literal "Cannot query field" (its fallbacks key on that string). Both were verified by hand across the 4 → 5 upgrade and are unchanged, but they rest on an implicit content-negotiation default — yoga only returns 400 under an Accept header that client never sends — so a later bump could flip them unnoticed. Now a standing guard rather than a one-time check. Addresses review feedback on #194. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What & why
Part of the production-readiness epic (#163). Closes #171.
The runtime container ran
npm startas PID 1, which forwards signals poorly — so SIGTERM didn't cleanly reach node and the new graceful shutdown (#170) couldn't run. It also had no container healthcheck and used a floatingnode:20-alpinetag.Changes
tinias PID 1: runnode build/src/index.jsdirectly under tini, so node receives SIGTERM (graceful shutdown) and zombies are reaped.HEALTHCHECK: hits the built-in/healthcheckendpoint (honours$PORT, defaults to 8080).node:20-alpine@sha256:fb4cd12c…) for reproducible, tamper-evident builds — bump via Dependabot (P1: Supply chain — Dependabot, npm audit gate, image scan, SBOM #175)..dockerignoreto keep the build context lean.Verification (built and run locally)
uid=1001 nodeuser).node build/src/index.jsunder tini — reachesbuildContext(errors only on the expected missingPG_CONN), confirming the entrypoint chain.No app code changed; lint and
prettier --debug-check .pass.🤖 Generated with Claude Code