fix(docker): harden image (0 crit/high) and halve size - #377
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ed54d02. Configure here.
KIvanow
changed the title
…nd GNU tar Docker Scout/grype flagged ~45 fixable CVEs in the published images that traced to bundled artifacts rather than application code: - RedisShake: the upstream prebuilt binary is compiled with Go 1.21.13 (EOL), whose embedded stdlib carries dozens of CVEs (2 Critical, 11 High, ...) that 'apk upgrade' cannot patch. Build it from source in a golang:1.26-alpine stage instead, pinned to the v4.6.1 release commit. - npm CLI bundled in node:*-alpine is unused at runtime (deps installed via pnpm); remove it to drop its vendored-dep CVEs (tar, brace- expansion, picomatch, sigstore, ip-address). - GNU tar package (unfixable CVEs) is dropped; busybox already provides tar for the RedisShake extraction path. Also switch the healthcheck from the full wget package to a node one-liner (removes the wget package and its unfixed CVEs), and drop the prod stage's 'apk del wget' which had been silently removing the wget the healthcheck depended on. Verified locally: grype on the rebuilt Dockerfile.prod image vs published latest shows 138 -> 93 findings (Critical 4->1, High 52->34), 45 removed / 0 added; stdlib findings 26->0; container boots, /health returns 200, docker healthcheck reports healthy.
The production stage ended with 'RUN chown -R betterdb:nodejs /app', which rewrites ownership on every file in node_modules. Under overlayfs that copies the entire ~550MB (no-ai) / ~1GB (ai) node_modules UP into a new layer, roughly doubling the dependency footprint on disk purely to change ownership bits. Create the non-root user before the COPYs and set ownership at copy time with 'COPY --chown=betterdb:nodejs' instead, so the files are written once, already owned correctly - no second layer. The @proprietary symlinks stay root-owned (read-only at runtime; the app only reads them). Verified locally (uncompressed 'docker images' sizes): no-ai: 1.34GB -> 744MB (-596MB) ai: 2.22GB -> 1.17GB (-1.05GB) Container still boots, /health returns 200, runs as uid 1001, node_modules owned by betterdb:nodejs, proprietary modules load.
Move the base image node:25-alpine -> node:26-alpine. node 25 is a non-LTS line with no patch for CVE-2026-56848/58043/etc; node 26 (LTS) carries the fixes. Also bump golang.org/x/text embedded in the from-source redis-shake build 0.14.0 -> 0.39.0 (CVE-2026-56852). The npm dependency CVEs this branch originally patched via pnpm.overrides are now covered by upstream (#364/#366) - upstream's lockfile already resolves the patched versions (incl. nanoid 3.3.17) - so no package.json or lockfile changes remain here.
The COPY --chown size fix set ownership on the copied files but left the /app WORKDIR itself root-owned, so the non-root runtime user could not create new paths under /app. RedisShake is spawned with cwd=/app and creates its default relative `data` dir there, so migrations failed with EACCES; the same affected sqlite/license paths under /app. The /health smoke test did not exercise these writes. Restore writability with a non-recursive `chown betterdb:nodejs /app` plus a pre-created /app/data. Only the /app dir node changes owner, so it does not reintroduce the ~550MB node_modules duplicate. Verified: runtime user can create /app/__w and /app/data/*; image size unchanged (746MB).
ee8fb0c to
5b8ab70
Compare
KIvanow
changed the title
There was a problem hiding this comment.
The core mechanisms of the PR held up well under adversarial checks: the pinned commit matches the real v4.6.1 release commit, all runtime write paths resolve to the pre-created /app/data, and the wget-removal/npm-removal rationale is sound. Inline comments carry one-click suggestions where a concrete fix exists (posted on both Dockerfiles).
| # compiled with Go 1.21.13 (EOL), whose stdlib carries dozens of CVEs (incl. | ||
| # criticals) that `apk upgrade` cannot fix - they are baked into the static | ||
| # binary, not provided by an Alpine package. | ||
| FROM golang:1.26-alpine AS redisshake-builder |
There was a problem hiding this comment.
[Confirmed] arm64 leg of this stage compiles under QEMU emulation. docker-publish.yml sets up QEMU and builds linux/amd64,linux/arm64 for both Dockerfiles; without --platform=$BUILDPLATFORM the arm64 pass pulls the arm64 golang image and runs the whole clone + go build under qemu-user, even though the stage is already a pure cross-compile (CGO_ENABLED=0 GOOS=linux GOARCH=${TARGETARCH}). On every cache miss that's typically 5-10x slower (minutes per release, twice - the two builds use disjoint cache scopes), and the Go toolchain under qemu-user is a known source of spurious SIGSEGV, i.e. intermittently red tag builds. The existing GOOS/GOARCH env already produces the correct target binary when the stage runs natively.
| FROM golang:1.26-alpine AS redisshake-builder | |
| FROM --platform=$BUILDPLATFORM golang:1.26-alpine AS redisshake-builder |
(Same applies to Dockerfile.prod line 151 - twin suggestion posted there.)
| # compiled with Go 1.21.13 (EOL), whose stdlib carries dozens of CVEs (incl. | ||
| # criticals) that `apk upgrade` cannot fix - they are baked into the static | ||
| # binary, not provided by an Alpine package. | ||
| FROM golang:1.26-alpine AS redisshake-builder |
There was a problem hiding this comment.
[Confirmed] Twin of the Dockerfile line 76 comment: add --platform=$BUILDPLATFORM so the arm64 leg cross-compiles natively instead of running the Go toolchain under QEMU.
| FROM golang:1.26-alpine AS redisshake-builder | |
| FROM --platform=$BUILDPLATFORM golang:1.26-alpine AS redisshake-builder |
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | ||
| CMD wget --no-verbose --tries=1 --spider http://localhost:${PORT}/api/health || exit 1 | ||
| CMD node -e "require('http').get('http://127.0.0.1:'+(process.env.PORT||3001)+'/api/health',r=>process.exit(r.statusCode===200?0:1)).on('error',()=>process.exit(1))" |
There was a problem hiding this comment.
[Confirmed] Node-based healthcheck can exceed the kept 3s timeout under CPU pressure - and busybox wget makes the node trick unnecessary. Two things verified empirically:
- busybox wget ships in
node:26-alpine(/usr/bin/wget -> /bin/busybox) with no package install - the oldapk add wgetwas always unnecessary, and busybox--spiderhas the right semantics (200 -> exit 0; 404 / connection refused -> exit 1). - The comment's rationale aside,
sh -> node -eboots a full interpreter every 30s. Under a saturated CPU-capped cgroup (exactly what a RedisShake full sync causes -migration-execution.service.ts:152spawns it in this same container) the PR's command measured 2.3s / 5.2s / 6.0s at--cpus=0.2, vs 0.2-0.4s for busybox wget. Three misses (~90s of a full sync) flip the container unhealthy and restart-on-unhealthy orchestrators kill it mid-migration.
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | |
| CMD wget --no-verbose --tries=1 --spider http://localhost:${PORT}/api/health || exit 1 | |
| CMD node -e "require('http').get('http://127.0.0.1:'+(process.env.PORT||3001)+'/api/health',r=>process.exit(r.statusCode===200?0:1)).on('error',()=>process.exit(1))" | |
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | |
| CMD wget -T 2 -q --spider "http://127.0.0.1:${PORT:-3001}/api/health" || exit 1 |
The ${PORT:-3001} default also covers the fallback the node one-liner added. If you take this, the comment above ("Uses node ... no extra wget package") should be updated - no package is needed either way.
(Same applies to Dockerfile.prod lines 262-263 - twin suggestion posted there.)
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | ||
| CMD wget --no-verbose --tries=1 --spider http://localhost:${PORT}/api/health || exit 1 | ||
| CMD node -e "require('http').get('http://127.0.0.1:'+(process.env.PORT||3001)+'/api/health',r=>process.exit(r.statusCode===200?0:1)).on('error',()=>process.exit(1))" |
There was a problem hiding this comment.
[Confirmed] Twin of the Dockerfile lines 184-185 comment: busybox wget is already in the base image and avoids node's cold-start blowing the 3s timeout under CPU pressure (measured 2.3-6.0s at --cpus=0.2 vs 0.2-0.4s for wget).
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | |
| CMD wget --no-verbose --tries=1 --spider http://localhost:${PORT}/api/health || exit 1 | |
| CMD node -e "require('http').get('http://127.0.0.1:'+(process.env.PORT||3001)+'/api/health',r=>process.exit(r.statusCode===200?0:1)).on('error',()=>process.exit(1))" | |
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | |
| CMD wget -T 2 -q --spider "http://127.0.0.1:${PORT:-3001}/api/health" || exit 1 |
| WORKDIR /build | ||
| RUN git clone --depth 1 --branch "v${REDISSHAKE_VERSION}" https://github.com/tair-opensource/RedisShake.git . && \ | ||
| test "$(git rev-parse HEAD)" = "${REDISSHAKE_COMMIT}" && \ | ||
| go get golang.org/x/text@v0.39.0 && \ |
There was a problem hiding this comment.
[Confirmed] This exact-version pin will silently downgrade x/text on a future RedisShake bump. go get pkg@version explicitly downgrades when the module's requirement is already higher (go help get: "To upgrade or downgrade a package to a specific version"). When REDISSHAKE_VERSION is next bumped to a release whose go.mod already requires x/text >= v0.40.0, this line silently drops it back to v0.39.0 - reintroducing whatever CVEs the newer version fixed, with no build failure to alert anyone (or a confusing conflict error if another module strictly needs newer). The pin is also duplicated in Dockerfile.prod line 160 and nothing documents which advisory it serves, so nobody will know when it can be deleted. At minimum, document the intent and the downgrade hazard next to the line:
| go get golang.org/x/text@v0.39.0 && \ | |
| # x/text >= 0.39.0 clears the Go CVE wall (v4.6.1's go.mod pins 0.14.0). | |
| # NB: exact-version `go get` also DOWNGRADES - delete this line once upstream requires >= v0.39.0. | |
| go get golang.org/x/text@v0.39.0 && \ |
(Comment lines inside RUN continuations are stripped by the Dockerfile parser, so this is build-safe. Twin suggestion posted on Dockerfile.prod.)
| COPY --from=redisshake-builder /out/redis-shake /usr/local/bin/redis-shake | ||
| RUN chmod +x /usr/local/bin/redis-shake |
There was a problem hiding this comment.
[Confirmed] This RUN chmod +x is a no-op that ships the binary twice. go build emits the binary 0755 and COPY --from preserves the mode - verified empirically, including the layer cost: overlay2 copy-up triggers on the setattr itself even when the mode value doesn't change (a no-op chmod on a 20MB test file produced a 21MB layer). So every published image carries the ~15-20MB redis-shake binary in two layers - the exact duplication cost the COPY --chown comments in this PR were written to avoid. One instruction does both:
| COPY --from=redisshake-builder /out/redis-shake /usr/local/bin/redis-shake | |
| RUN chmod +x /usr/local/bin/redis-shake | |
| COPY --chmod=755 --from=redisshake-builder /out/redis-shake /usr/local/bin/redis-shake |
(Same applies to Dockerfile.prod lines 243-244 - twin suggestion posted there.)
| COPY --from=redisshake-builder /out/redis-shake /usr/local/bin/redis-shake | ||
| RUN chmod +x /usr/local/bin/redis-shake |
There was a problem hiding this comment.
[Confirmed] Twin of the Dockerfile lines 165-166 comment: the chmod is a no-op that duplicates the binary into a second layer; COPY --chmod=755 does both in one instruction.
| COPY --from=redisshake-builder /out/redis-shake /usr/local/bin/redis-shake | |
| RUN chmod +x /usr/local/bin/redis-shake | |
| COPY --chmod=755 --from=redisshake-builder /out/redis-shake /usr/local/bin/redis-shake |
| RUN apk add --no-cache git | ||
| WORKDIR /build | ||
| RUN git clone --depth 1 --branch "v${REDISSHAKE_VERSION}" https://github.com/tair-opensource/RedisShake.git . && \ | ||
| test "$(git rev-parse HEAD)" = "${REDISSHAKE_COMMIT}" && \ |
There was a problem hiding this comment.
[Observation - non-blocking] The integrity story is weaker than the sha256-of-artifact it replaces. The commit check on this line verifies the cloned tree, but the very next step (go get golang.org/x/text@v0.39.0) mutates go.mod/go.sum after the tamper check, and the final binary hash is pinned nowhere - while the workflow's provenance: true / sbom: true attestations record an artifact whose dependency set no longer matches the pinned upstream commit. Practical residual risk is modest (the commit pin + Go's checksum DB make module tampering fail closed), but the shipped binary is a 4.6.1 + x/text-0.39.0 combination upstream never released or tested, and an SBOM audit against RedisShake v4.6.1 will show a mismatch. Worth a note in the stage comment, and ideally an upstream PR bumping x/text so the pin (and the mismatch) can go away.
| ARG REDISSHAKE_COMMIT=a2a7e4e46d15708b6ab203e1c10a108aa405a638 | ||
| RUN apk add --no-cache git | ||
| WORKDIR /build | ||
| RUN git clone --depth 1 --branch "v${REDISSHAKE_VERSION}" https://github.com/tair-opensource/RedisShake.git . && \ |
There was a problem hiding this comment.
[Observation - non-blocking] This stage's failure modes are only ever exercised on release day. docker-publish.yml triggers on v* tags/dispatch only, and no PR-time job builds either Dockerfile - so a moved/deleted tag (fails closed at the commit check, by design), a proxy.golang.org outage, or a golang:1.26-alpine patch regression is discovered for the first time mid-release. The blind spot predates this PR (the old wget lived in the same path), but the PR widens it from one artifact download to a full toolchain build. A cheap guard: a PR-time or scheduled job that builds just this stage (docker build --target redisshake-builder .).
There was a problem hiding this comment.
Fair point — this is fine for now. Depending on usage we can add a PR-time or scheduled docker build --target redisshake-builder guard later. Leaving as-is for this PR.
| # compiled with Go 1.21.13 (EOL), whose stdlib carries dozens of CVEs (incl. | ||
| # criticals) that `apk upgrade` cannot fix - they are baked into the static | ||
| # binary, not provided by an Alpine package. | ||
| FROM golang:1.26-alpine AS redisshake-builder |
There was a problem hiding this comment.
[Cleanup] The hand-maintained duplication between the two Dockerfiles grows from 131 to 160 byte-identical lines in this PR - this whole stage (with four coupled pins: golang:1.26, REDISSHAKE_VERSION, REDISSHAKE_COMMIT, the x/text version), the apk/npm-removal block, the user/chown/chmod blocks, and the 200-char healthcheck line are all character-identical with Dockerfile, with no CI build-arg as a single source of truth. Drift between exactly these two files already caused one real regression (commit 9708786 patched only Dockerfile to re-sync). The CI cache scopes are also disjoint (with-ai/without-ai), so this identical stage compiles 4x per release with zero sharing. docker/build-push-action@v6 supports target:, so a single multi-target Dockerfile (shared builder + redisshake-builder stages, terminal production / production-no-ai targets) would need only a target: line per existing CI step - happy to leave that for a follow-up, but flagging it while the surface is growing.
There was a problem hiding this comment.
Great point — the duplication is real and this makes sense to refactor into a single multi-target Dockerfile (shared builder + redisshake-builder stages, terminal production / production-no-ai targets). Doing it in a separate PR to keep this one focused on the security/build fixes.
…ck, version stamp, single binary layer Applies Petar's review findings on both Dockerfiles: 1. FROM --platform=$BUILDPLATFORM on the redisshake-builder stage so the arm64 leg cross-compiles natively (GOARCH) instead of running the Go toolchain under QEMU (5-10x slower, spurious SIGSEGV). Verified: building with TARGETARCH=arm64 on amd64 produces an aarch64 binary in seconds. 2. Healthcheck back to the base image's busybox wget (--spider) instead of a node one-liner. busybox wget ships in node:26-alpine and supports --spider/-T/-q; node's interpreter cold-start can exceed the 3s timeout under the CPU pressure of a running migration and flip the container unhealthy mid-sync. 4. Inject -X main.Version / -X main.GitCommit ldflags so the binary reports 'v4.6.1 (Git SHA ...)' instead of 'unknown' in migration logs. 5. COPY --chmod=755 instead of COPY + RUN chmod +x — the chmod was a no-op (go build already emits 0755) that duplicated the ~10MB binary into a second overlay layer. Also documents (3) the go get x/text exact-pin downgrade hazard and (6) the SBOM/integrity delta the x/text bump introduces, in the stage comment. Verified locally: builds on node 26, 0 Critical/High (grype), container healthy via wget, /api/health 200, redis-shake reports v4.6.1.
2 participants
Summary
Harden and shrink the Docker image. Rebased onto latest
master, so this now sits on top of the Dependabot dependency fixes (#364/#366) and touches only the two Dockerfiles — no dependency/lockfile changes here.Result (grype, no-ai image): 0 Critical, 0 High (7 Medium/Low remain — OS packages with no fix + suite-locked mediums). Image 1.34 GB → 739 MB.
Changes (Dockerfile + Dockerfile.prod)
golang.org/x/textbumped 0.14.0 → 0.39.0 — clears the Go CVE wallnodeinstead of the wget package (removes wget CVEs); uses the correct/api/healthpath; also removes the prod-stageapk del wgetthat had been breaking the checkCOPY --chowninstead ofchown -R /app— removes a duplicated node_modules layer (~600 MB), then a non-recursive chown restores/appwritability for the runtime user (RedisShake data dir, sqlite/license)node:25-alpine→node:26-alpine(25 is non-LTS with no patch line)Verified locally: builds on node 26 (native modules recompile), container boots,
/api/healthreturns real JSON 200, docker healthcheck healthy, static UI serves, redis-shake runs,/appwritable.Checklist
Note
Medium Risk
Touches production image build, runtime ownership, and the RedisShake migration binary (now a from-source build with a patched dependency). Failures here can break container boots, healthchecks, or migrations.
Overview
Hardens and shrinks both
DockerfileandDockerfile.prod: base image moves tonode:26-alpine, unused runtime packages are dropped, and image size roughly halves by replacing recursivechown -R /appwithCOPY --chownplus a non-recursive/app//app/datachown.RedisShake is no longer downloaded as a prebuilt release. It is built from pinned
v4.6.1source on Go 1.26, withgolang.org/x/textbumped to clear EOL-stdlib CVEs, then copied into the runtime image.Also removes the bundled npm CLI (CVE surface), avoids installing full wget/GNU tar, and switches the healthcheck to busybox
wgetagainst127.0.0.1:${PORT:-3001}/api/health.Reviewed by Cursor Bugbot for commit 1680e43. Bugbot is set up for automated code reviews on this repo. Configure here.