fix: Docker security hardening and CI version alignment#712
fix: Docker security hardening and CI version alignment#7122witstudios merged 4 commits intomasterfrom
Conversation
📝 WalkthroughWalkthroughThe PR modernizes CI/CD infrastructure by upgrading Node.js to version 22 and PostgreSQL to 17-alpine, enforces frozen lockfiles in the processor build, restructures the realtime service into a multi-stage Dockerfile, disables Next.js telemetry, removes embedded .env files from utility Dockerfiles, and replaces the worker healthcheck with actual process verification. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cace0687fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.dockerignore (1)
31-35: Good security improvement — consider also adding a wildcard.env*pattern.Adding
.envhere correctly prevents secrets from leaking into Docker images. However, the explicit list still misses variants like.env.stagingor.env.production. A single.env*glob (with a!.env.exampleoverride if you want to keep the template available) would be more robust.This is optional since the current explicit list covers the standard Next.js variants.
♻️ Optional: replace explicit list with a glob
# Environment files -.env -.env.local -.env.development.local -.env.test.local -.env.production.local +.env* +!.env.example🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.dockerignore around lines 31 - 35, Replace the explicit .env variants in the .dockerignore with a glob to catch all env files: update the lines that currently list ".env", ".env.local", ".env.development.local", ".env.test.local", ".env.production.local" to include a single ".env*" entry (and optionally add an override like "!.env.example" if you want to keep the template in images); this ensures variants like .env.staging or .env.production are also ignored.apps/realtime/Dockerfile (1)
8-11:gitand custom npm timeout config may be unnecessary baggage in the final image.
apk add gitand the npm timeout settings were likely carried over from the old single-stage Dockerfile. Sincegitis only needed during dependency installation (deps stage), and these npm config lines apply to npm (not pnpm), consider whether they're still needed. The runner stage inherits none of this, but the deps stage could be leaner without git if no git-based dependencies exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/realtime/Dockerfile` around lines 8 - 11, Remove unnecessary tooling and npm config from the final runner image: if your project uses pnpm and has no git-based dependencies, delete the "apk add --no-cache git" and the "npm config set fetch-..." lines from this RUN instruction, or move them into the deps/install stage only (keep them in the stage that runs package installation). Check for any git-based dependencies in package.json and whether pnpm is used; if pnpm is used, drop the npm config lines entirely. Update the RUN that performs installation (and the deps stage) to include git only when required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/realtime/Dockerfile`:
- Around line 36-47: The runner stage currently copies the full build output
(including node_modules with devDependencies) via COPY --from=builder /app ., so
set up the runner to perform a production-only dependency step: in the runner
stage (the Dockerfile's "runner" stage and the lines around COPY --from=builder
/app . and ENV NODE_ENV=production) copy only package.json /pnpm-lock.yaml (and
any built dist output from the builder for `@pagespace/db` and `@pagespace/lib`),
run corepack enable and a production install or prune (e.g., pnpm install --prod
or pnpm prune --prod) so only production deps are present, and then switch to
USER node; ensure the builder stage still emits built artifacts for shared
packages so the runner can install without dev toolchains.
- Around line 24-34: The builder stage currently only copies source/deps and
never compiles packages, so add explicit build steps in the builder stage to
compile shared packages and the realtime app: run pnpm --filter `@pagespace/db`
build, pnpm --filter `@pagespace/lib` build, and pnpm --filter realtime build (or
pnpm build for the workspace target) after enabling corepack and copying
sources, ensuring the produced artifacts are available for the final runtime
stage and devDependencies are not carried into the runtime image; reference the
builder stage and the runner/production copy to confirm artifacts are copied
from the builder.
In `@apps/web/Dockerfile.worker`:
- Around line 42-44: The HEALTHCHECK uses pgrep ("pgrep -f \"file-processor\"")
but the base image (node:22.17.0-alpine) doesn't include pgrep; install the
package that provides it before the HEALTHCHECK. Add a RUN apk add --no-cache
procps-ng prior to the HEALTHCHECK directive so pgrep is available when the
HEALTHCHECK (pgrep -f "file-processor") runs.
---
Nitpick comments:
In @.dockerignore:
- Around line 31-35: Replace the explicit .env variants in the .dockerignore
with a glob to catch all env files: update the lines that currently list ".env",
".env.local", ".env.development.local", ".env.test.local",
".env.production.local" to include a single ".env*" entry (and optionally add an
override like "!.env.example" if you want to keep the template in images); this
ensures variants like .env.staging or .env.production are also ignored.
In `@apps/realtime/Dockerfile`:
- Around line 8-11: Remove unnecessary tooling and npm config from the final
runner image: if your project uses pnpm and has no git-based dependencies,
delete the "apk add --no-cache git" and the "npm config set fetch-..." lines
from this RUN instruction, or move them into the deps/install stage only (keep
them in the stage that runs package installation). Check for any git-based
dependencies in package.json and whether pnpm is used; if pnpm is used, drop the
npm config lines entirely. Update the RUN that performs installation (and the
deps stage) to include git only when required.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.dockerignore.github/workflows/test.ymlapps/processor/Dockerfileapps/realtime/Dockerfileapps/web/Dockerfileapps/web/Dockerfile.migrateapps/web/Dockerfile.seedapps/web/Dockerfile.worker
💤 Files with no reviewable changes (2)
- apps/web/Dockerfile.migrate
- apps/web/Dockerfile.seed
Remove .env COPY from realtime, migrate, and seed Dockerfiles — env vars are provided at runtime via docker-compose env_file. Add .env to .dockerignore. Convert realtime Dockerfile to multi-stage build with USER node. Enable --frozen-lockfile in processor build. Replace no-op worker healthcheck with pgrep process check. Enable NEXT_TELEMETRY_DISABLED in web Dockerfile. Align CI to Node 22 and Postgres 17 matching production. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…check Realtime Dockerfile: - Add build steps for shared packages and realtime service (tsc) - Production-only install in runner stage (--prod) excludes devDependencies - Remove git/npm config (only pnpm is used, no git-based deps) - Runner now executes compiled JS via node instead of tsx Worker healthcheck: - Replace pgrep with node-based PID 1 liveness check - Avoids procps-ng dependency and ancestor shell match false-positives .dockerignore: - Use .env* glob with !.env.example override for broader coverage Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2witstudios
force-pushed
the
ppg/docker-security
branch
from
9c4edb8 to
3ae8e3e
Compare
Result: ag-1rno7f2aPR(Review-only — no code changes made, no PR created) SummaryComprehensive code review of PR #712: "fix: Docker security hardening and CI version alignment" ChangesReview-only. No files were modified. NotesSee full review below. 🔬 COMPREHENSIVE CODE REVIEW: Docker Security Hardening & CI Version Alignment (PR #712)PR Scope AnalysisChanges span 8 files (+44 / -40):
Functional Requirements: PR description lists 8 objectives — all addressed in the diff. 1. Code Structure & Organization ✅🎯 Consistent Docker patterns across services Strengths:
2. Security Analysis (OWASP Top 10 Scan) ✅ with notesExplicit review of each OWASP Top 10 category against the PR changes:
Security-specific findings: ✅
|
| Category | Score | Notes |
|---|---|---|
| Security Improvement | ✅ 100% | .env removal, non-root, frozen lockfile |
| Docker Best Practices | ✅ 95% | Solid multi-stage, minor docs opportunity |
| CI Alignment | ✅ 100% | Node + Postgres versions matched |
| Backwards Compatibility | ✅ 100% | No behavioral changes to running services |
| Completeness | ✅ 95% | All 8 stated objectives met; dead dotenv noted |
Critical Findings Summary
Strengths (Outstanding)
- Secrets removed from images — eliminates a common Docker anti-pattern
- Non-root execution — follows container security best practices
- Multi-stage build — proper separation of build and runtime concerns
- Frozen lockfile enforcement — reproducible builds across all services
- Healthcheck improvement — meaningful liveness probe vs. no-op
- CI/production parity — version alignment reduces environment drift
Areas for Improvement (None Blocking)
- Dead
dotenv.config()in realtime — benign but should be cleaned up in follow-up - Missing build verification — the realtime multi-stage build should be smoke-tested (
docker compose build realtime && docker compose up realtime) - Comment for web package.json stub — would help future maintainers understand the pnpm workspace requirement
Final Assessment
Overall Score: 94/100 (Excellent)
Breakdown:
- Security: ✅ 100% — Significant hardening across all Dockerfiles
- Correctness: ✅ 90% — High confidence but the CJS runtime switch should be verified with a build
- Best Practices: ✅ 95% — Strong adherence to Docker multi-stage patterns
- CI Alignment: ✅ 100% — Clean version bump with rationale
- Completeness: ✅ 95% — All stated objectives addressed
Production Readiness: ✅ APPROVED with verification
Recommendation: Merge after verifying:
docker compose buildsucceeds for all services (especially realtime)docker compose up— realtime service starts and accepts WebSocket connectionsdocker exec <realtime-container> whoamireturnsnode
The PR achieves meaningful security hardening with zero behavioral changes to running services. Clean work.
- Realtime Dockerfile: copy only dist/ and package.json from packages instead of full packages/ directory (smaller, cleaner image) - Realtime Dockerfile: add HEALTHCHECK for consistency with worker and processor services - Realtime service: add /health HTTP endpoint to support HEALTHCHECK - CI test.yml: pin Node 22.17.0 and postgres:17.5-alpine to match docker-compose - CI load-test.yml: align Node 22.17.0 and postgres:17.5-alpine with test.yml and docker-compose Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…st artifacts Builder stage: replace broad `COPY packages` with targeted copies of packages/db, packages/lib, and types (needed for build-time type roots). Runner stage: copy only compiled dist/ directories instead of full package trees — package.json files are already present from the prod install step, so Node module resolution via pnpm workspace links works correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
COPY .env ./from realtime, migrate, and seed Dockerfiles — env vars are injected at runtime via docker-composeenv_file:. Added.env*glob to.dockerignoreas defense-in-depth.USER nodeinstruction matching the web Dockerfile pattern.@pagespace/db,@pagespace/lib) and the realtime service viatsc. Runner installs production-only dependencies (--prod) and copies only compileddist/artifacts — no devDependencies or TypeScript source in the final image.packages/db,packages/lib, andtypes(not the fullpackages/tree). Runner copies onlydist/directories from the builder —package.jsonfiles are already present from the prod install step for pnpm workspace resolution.--no-frozen-lockfileto--frozen-lockfilefor reproducible builds.console.loghealthcheck with node-based PID 1 liveness check (process.kill(1, 0)) — avoidsprocps-ngdependency andpgrepancestor shell match false-positives.NEXT_TELEMETRY_DISABLED=1in both builder and runner stages of the web Dockerfile.Test plan
docker compose buildsucceeds for all servicesdocker compose upstarts all services correctly with env vars fromenv_filedocker exec <container> whoami→node).envfile exists inside built images🤖 Generated with Claude Code