fix(images): bump the toolbox to Node 24 and upgrade npm in place (#2166) - #2168
Merged
Conversation
) The image scan reports 24 HIGH on insight-toolbox. Fifteen of them are npm's own bundled dependency tree -- tar, minimatch, glob, cross-spawn, sigstore, brace-expansion -- carried by the npm 10.8.2 that ships with the Node 20 distro package. Nothing in this repository depends on those packages; they come with the interpreter. Node 24 brings npm 11.16.0, which clears most of it; upgrading npm in place to 12.0.2 clears the rest. Measured on the real image, same flags as trivy-images.yml: Node 20, npm 10.8.2 24 HIGH Node 24, npm 11.16.0 6 HIGH (npm tree only, probe) Node 24, npm 12.0.2 10 HIGH (full image) What remains: 7 Go CVEs inside the kubectl and yq binaries, which already carry the newest upstream releases (kubectl v1.36.3, yq v4.53.3) and can only be fixed by an upstream rebuild; and 3 that npm 12.0.2 still bundles unfixed (brace-expansion 5.0.7, ip-address 10.2.0). The image keeps everything it had: node 24.18.1, npm 12.0.2, kubectl v1.36.3, yq v4.53.3, jq 1.7, envsubst 0.23.1, dbt 1.11.12, and it still runs as uid 1000. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe toolbox Dockerfile now uses Node.js 24, upgrades npm globally, cleans the npm cache, and retains package-list cleanup during image construction. ChangesToolbox runtime
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ingestion/tools/toolbox/Dockerfile`:
- Line 13: Update the Dockerfile’s global npm installation command to pin npm to
exactly version 12.0.2 instead of latest, then add a verification step that
asserts the installed npm version matches 12.0.2.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5792c19a-29c0-422b-b19a-ddcd722f1fdc
📒 Files selected for processing (1)
src/ingestion/tools/toolbox/Dockerfile
| # tree is what the image scan reports. | ||
| RUN curl -fsSL https://deb.nodesource.com/setup_24.x | bash - && \ | ||
| apt-get install -y --no-install-recommends nodejs && \ | ||
| npm install -g npm@latest && \ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/ingestion/tools/toolbox/Dockerfile"
if rg -nF 'npm install -g npm@latest' "$file"; then
echo "Floating npm tag is still present" >&2
exit 1
fi
rg -nF 'npm install -g npm@12.0.2' "$file"Repository: constructorfabric/insight
Length of output: 235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="src/ingestion/tools/toolbox/Dockerfile"
echo "== Dockerfile snippet =="
cat -n "$file" | sed -n '1,80p'
echo
echo "== npm version references relevant to toolbox or PR context =="
rg -n "npm(@|[: ]|version|12\.0\.2|latest)" . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' | sed -n '1,120p'Repository: constructorfabric/insight
Length of output: 3807
Pin npm to npm@12.0.2.
The Dockerfile comment says the bundled npm dependency tree is important for image scans, but line 13 installs the mutable latest tag. Use an exact npm version and assert the installed version after install.
Proposed fix
- npm install -g npm@latest && \
+ npm install -g npm@12.0.2 && \
+ test "$(npm --version)" = "12.0.2" && \
npm cache clean --force && \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/tools/toolbox/Dockerfile` at line 13, Update the Dockerfile’s
global npm installation command to pin npm to exactly version 12.0.2 instead of
latest, then add a verification step that asserts the installed npm version
matches 12.0.2.
ktursunov
approved these changes
Aug 4, 2026
Contributor
🤖 connectors-ddl snapshot driftThe committed snapshot does not match what this branch actually produces. The regenerated snapshot is waiting in #2169 — review the DDL diff there and merge it into this branch; the gate re-runs on your merge. Refreshed on every drifting gate run (the regen branch is force-pushed), so it reflects this branch as of the last completed run. |
cyberantonz
approved these changes
Aug 4, 2026
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Gregory91G
enabled auto-merge
August 5, 2026 09:46
8 tasks
cyberantonz
pushed a commit
to cyberantonz/insight
that referenced
this pull request
Aug 6, 2026
…ructorfabric#2018) Two gates, both on CRITICAL with --ignore-unfixed, matching what the issue asks for. Publish side: a new composite action scans the push-by-digest blobs in each merge job, before docker/metadata-action's tags are applied. A rejected build therefore keeps no build tag and does not move :latest, so no descriptor bump and no chart can reference it. It covers the seven jobs that produce the fourteen images the nightly scans; insight-ui-tests stays out, as it is out of that matrix. Drift side: trivy-images.yml gets a verdict step. It runs last and reads the JSON the scan already wrote, so the job summary and the SARIF upload still happen for a failing image -- putting --exit-code 1 on the scan itself would abort before `trivy convert` and lose both. Gating on Trivy's own severity rather than on the Code Scanning level is deliberate: GitHub derives its level from the CVSS score, so CVE-2026-59873 (constructorfabric#2014) was CRITICAL for Trivy and high in the Security tab. Note that `--severity` is undocumented on `trivy convert` yet honoured, which is one more reason the image stays pinned by digest. Baseline measured before enabling: insight-toolbox 0 critical after constructorfabric#2168, insight-gateway, insight-front and source-salesforce-insight also 0. Signed-off-by: Gregory Gogin <grigoriy.gogin@constructor.tech>
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2166. Takes
insight-toolboxfrom 24 HIGH to 10.The fifteen npm findings are not this repository's dependencies — npm bundles its own tree, and
the Node 20 distro package carries npm 10.8.2 against a current 12.0.2. Node 24 brings 11.16.0
and clears most of them; upgrading npm in place clears the rest.
Measured on the real image
docker build -f src/ingestion/tools/toolbox/Dockerfile src/ingestion, thentrivy imagewiththe flags
trivy-images.ymluses (--scanners vuln --severity CRITICAL,HIGH --ignore-unfixed):Cleared:
tar×8,minimatch×3,glob,cross-spawn,sigstore, and one of the threebrace-expansion.What remains, and why it cannot move here
golang.org/x/net×4,golang.org/x/text×2,stdlib— inusr/local/bin/kubectlandusr/local/bin/yqkubectl v1.36.3fromstable.txt,yq v4.53.3fromreleases/latest, both resolved at build time). The fixes need upstream to rebuild against newer Go libraries.brace-expansion 5.0.7×2,ip-address 10.2.0Test plan
trivy image, CI flags: 24 → 10node v24.18.1,npm 12.0.2,kubectl v1.36.3,yq v4.53.3,jq 1.7,envsubst 0.23.1,dbt 1.11.12uid=1000(appuser)— the non-root change from fix(images): run the tooling images as a non-root user #2089 is preservedgit diff --checkcleantrivy-images.ymlreports 10 forinsight-toolboxon its next nightly runSummary by CodeRabbit