Upgrade Node version and optimize Dockerfile#313
Upgrade Node version and optimize Dockerfile#313niranjanmishra004 wants to merge 1 commit intoLondheShubham153:masterfrom
Conversation
Updated Node version to 18-alpine, improved caching by copying only package files first, and adjusted comments for clarity.
WalkthroughThe Dockerfile has been updated to use Node.js 18 instead of 12.2.0, with the working directory changed from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 16-17: Remove the RUN npm test step from the Dockerfile (the
literal "RUN npm test" line) so image builds are fast and deterministic; instead
run the test suite in your CI pipeline before building the image — if you need
an in-image test option for local debugging, replace the hard-coded RUN with a
build ARG or a separate debug stage (e.g., use an optional build-arg to
conditionally run tests) but do not leave RUN npm test in the production build
stage.
- Around line 1-2: You updated the Dockerfile base to FROM node:18-alpine but
the repo still has very old deps (package.json, e.g., sanitizer@^0.1.3) that may
break or be vulnerable; locally build the image using Dockerfile (Node 18), run
npm install inside that environment, execute the test suite, run npm audit (and
npm audit fix where safe), and iterate on package.json to upgrade or replace
outdated packages (update lockfile, bump versions, and re-run tests) until the
app builds and tests cleanly on Node 18.
- Around line 19-23: Add a non-root USER directive and optional HEALTHCHECK to
the Dockerfile: stop running the container as root by switching to the existing
node user (add a USER node or USER node:node after installing deps and before
EXPOSE/CMD) and include an optional HEALTHCHECK that probes the server (e.g.,
HTTP GET to port 8000) so orchestrators can detect unresponsive containers;
ensure the HEALTHCHECK uses a tool present in the image (wget or curl) or
install one before adding the HEALTHCHECK.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| # Use a more recent Node version (Node 12 is outdated) | ||
| FROM node:18-alpine |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for known vulnerabilities in the dependencies
echo "=== Checking package.json dependencies ==="
cat package.json | jq '.dependencies'
echo -e "\n=== Checking npm audit for known vulnerabilities ==="
# Check if package-lock.json exists
if [ -f "package-lock.json" ]; then
echo "package-lock.json found"
else
echo "No package-lock.json found"
fi
echo -e "\n=== Searching for sanitizer alternatives in codebase ==="
rg -n "require.*sanitizer|import.*sanitizer" --type jsRepository: LondheShubham153/node-todo-cicd
Length of output: 430
🌐 Web query:
Is the npm sanitizer package version 0.1.3 compatible with Node.js 18?
💡 Result:
Yes, the npm sanitizer package version 0.1.3 is compatible with Node.js 18.
Citations:
- 1: https://www.npmjs.com/sanitizer
- 2: https://www.npmjs.com/package/sanitizer?activeTab=dependencies
- 3: http://github.com/theSmaw/Caja-HTML-Sanitizer
- 4: https://registry.npmjs.org/express-sanitizer
Node 18 upgrade is appropriate, but test the build and consider dependency updates.
Upgrading from EOL Node 12 to Node 18 is appropriate. However, the package.json contains very old dependencies (dating to 2010-2017). While the sanitizer@^0.1.3 package is confirmed compatible with Node 18, these old dependencies may have unpatched security vulnerabilities and lack active maintenance.
Recommend:
- Test the build locally with Node 18 to ensure all dependencies install and work correctly
- Run
npm auditto identify any known vulnerabilities in the locked dependency tree - Plan to update dependencies to more recent versions for better security and compatibility going forward
🧰 Tools
🪛 Checkov (3.2.510)
[low] 1-23: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[low] 1-23: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🪛 Trivy (0.69.3)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
[info] 1-1: No HEALTHCHECK defined
Add HEALTHCHECK instruction in your Dockerfile
Rule: DS-0026
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 1 - 2, You updated the Dockerfile base to FROM
node:18-alpine but the repo still has very old deps (package.json, e.g.,
sanitizer@^0.1.3) that may break or be vulnerable; locally build the image using
Dockerfile (Node 18), run npm install inside that environment, execute the test
suite, run npm audit (and npm audit fix where safe), and iterate on package.json
to upgrade or replace outdated packages (update lockfile, bump versions, and
re-run tests) until the app builds and tests cleanly on Node 18.
| # Run tests (optional – remove if not needed in build) | ||
| RUN npm test |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Running tests during image build is unusual.
Embedding npm test in the Dockerfile means:
- Every image build runs the full test suite, increasing build time
- Tests may fail if they require external services (databases, APIs) unavailable during build
- Test failures will block image creation entirely
The comment says "(optional – remove if not needed in build)" which suggests uncertainty. Typically, tests run in CI/CD pipelines before building the Docker image, keeping the image build fast and deterministic.
Consider removing this step and running tests in your CI pipeline instead.
♻️ Proposed change to remove tests from build
-# Run tests (optional – remove if not needed in build)
-RUN npm test
-
# Expose the app port
EXPOSE 8000📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Run tests (optional – remove if not needed in build) | |
| RUN npm test |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 16 - 17, Remove the RUN npm test step from the
Dockerfile (the literal "RUN npm test" line) so image builds are fast and
deterministic; instead run the test suite in your CI pipeline before building
the image — if you need an in-image test option for local debugging, replace the
hard-coded RUN with a build ARG or a separate debug stage (e.g., use an optional
build-arg to conditionally run tests) but do not leave RUN npm test in the
production build stage.
| # Expose the app port | ||
| EXPOSE 8000 | ||
|
|
||
| #Run the code | ||
| CMD ["node","app.js"] | ||
| # Start the app | ||
| CMD ["node", "app.js"] |
There was a problem hiding this comment.
Add a non-root USER directive for security.
Static analysis correctly flags that the container runs as root, which is a security risk. The node:18-alpine image includes a node user. Running the application as a non-root user limits the impact of container escape vulnerabilities.
Optionally, adding a HEALTHCHECK improves orchestration reliability by letting Docker/Kubernetes detect unresponsive containers.
🛡️ Proposed fix to add USER and optional HEALTHCHECK
# Expose the app port
EXPOSE 8000
+# Run as non-root user for security
+USER node
+
+# Optional: Add healthcheck for container orchestration
+HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \
+ CMD wget --no-verbose --tries=1 --spider http://localhost:8000/ || exit 1
+
# Start the app
CMD ["node", "app.js"]Note: wget is available in Alpine. Alternatively, use curl if you add it to the image.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Expose the app port | |
| EXPOSE 8000 | |
| #Run the code | |
| CMD ["node","app.js"] | |
| # Start the app | |
| CMD ["node", "app.js"] | |
| # Expose the app port | |
| EXPOSE 8000 | |
| # Run as non-root user for security | |
| USER node | |
| # Optional: Add healthcheck for container orchestration | |
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | |
| CMD wget --no-verbose --tries=1 --spider http://localhost:8000/ || exit 1 | |
| # Start the app | |
| CMD ["node", "app.js"] |
🧰 Tools
🪛 Checkov (3.2.510)
[low] 1-23: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[low] 1-23: Ensure that a user for the container has been created
(CKV_DOCKER_3)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 19 - 23, Add a non-root USER directive and optional
HEALTHCHECK to the Dockerfile: stop running the container as root by switching
to the existing node user (add a USER node or USER node:node after installing
deps and before EXPOSE/CMD) and include an optional HEALTHCHECK that probes the
server (e.g., HTTP GET to port 8000) so orchestrators can detect unresponsive
containers; ensure the HEALTHCHECK uses a tool present in the image (wget or
curl) or install one before adding the HEALTHCHECK.
1 participant
Updated Node version to 18-alpine, improved caching by copying only package files first, and adjusted comments for clarity.
Summary by CodeRabbit