⚽ Live Match: project copacetic/copacetic/pull/

robert-cronin · 2026-02-20T01:32:29Z

Copilot

Pull request overview

This PR addresses issue #1462 by fundamentally changing Copa's Node.js patching strategy from using npm install and npm overrides to directly downloading and replacing package tarballs from the npm registry. The original approach re-resolved the entire dependency tree during patching, which could introduce new vulnerabilities in transitive dependencies. The new approach surgically replaces only the targeted vulnerable packages by downloading their fixed versions as tarballs and extracting them directly into node_modules/.

Changes:

Added getPackageBaseName() helper function to extract base names from scoped packages for tarball URL construction
Replaced npm install commands with direct tarball downloads using wget and tar for both direct and transitive dependencies
Updated npm global package patching to use tarball replacement instead of npm install -g
Removed npm overrides strategy that was re-resolving entire dependency trees

Copilot · 2026-02-20T01:38:27Z

+	// Check if npm itself has vulnerabilities - if so, replace the specific vulnerable
+	// packages within npm's node_modules via direct tarball download, instead of
+	// upgrading npm entirely (which re-resolves npm's dependency tree and can
+	// introduce new vulnerabilities).
 	if hasNpmVulnerabilities(updates) {
-		log.Info("Detected vulnerabilities in npm dependencies. Upgrading npm to latest compatible version...")
+		log.Info("Detected vulnerabilities in npm dependencies. Replacing vulnerable packages within npm...")


The comments describe the new approach, but the old comment about npm/corepack behavior at the function level may be outdated. The function now uses direct tarball replacement instead of upgrading npm entirely. This fundamental change in approach means the previous npm upgrade strategy and its limitations (mentioned in line 812-813 original approach) are no longer applicable.

Consider verifying that the npm/corepack special handling logic still makes sense with the new tarball replacement approach, and update comments if needed.

Copilot · 2026-02-20T01:38:28Z

+					`if [ -d "$PKG_DIR" ]; then `+
+					`  echo "INFO: Replacing direct dependency %s with version %s" && `+
+					`  rm -rf "$PKG_DIR" && mkdir -p "$PKG_DIR" && `+
+					`  wget -qO- "$TARBALL_URL" | tar xz --strip-components=1 -C "$PKG_DIR"; `+
+					`else `+
+					`  echo "WARN: %s not found in node_modules, skipping"; `+
+					`fi'`,


The tarball URL construction assumes a specific npm registry URL format. If the package version doesn't exist or the tarball URL format is incorrect, wget will fail with a 404 error. However, without proper error handling, the script continues and creates an empty directory, breaking the application.

The current check only verifies if $PKG_DIR exists before attempting replacement, but doesn't verify the download succeeded or the extraction was successful. This is especially problematic because:

Non-existent versions will result in 404 errors

Network failures will be silent

The directory will be left in a corrupted state (deleted but not replaced)

Add verification after the wget/tar pipeline to ensure:

The download succeeded

The directory contains a valid package.json

The package.json has the expected name and version

Copilot · 2026-02-20T01:38:28Z

+
+			encodedName := u.Name
+			if strings.HasPrefix(u.Name, "@") {
+				encodedName = strings.Replace(u.Name, "/", "%2f", 1)


The tarball URL construction for scoped packages uses only the first occurrence of "/" when encoding. This could be problematic for edge cases where package names might contain multiple slashes, though this is unlikely given npm's naming rules. However, the encoding is case-sensitive and should use uppercase "%2F" instead of lowercase "%2f" for standards compliance with RFC 3986. While many servers accept both, using the standard uppercase form is more correct.

Suggested change

encodedName = strings.Replace(u.Name, "/", "%2f", 1)

encodedName = strings.ReplaceAll(u.Name, "/", "%2F")

Copilot · 2026-02-20T01:38:28Z

+						`if [ -d "$PKG_DIR" ]; then `+
+						`  echo "INFO: Replacing direct dependency %s with version %s" && `+
+						`  rm -rf "$PKG_DIR" && mkdir -p "$PKG_DIR" && `+
+						`  wget -qO- "$TARBALL_URL" | tar xz --strip-components=1 -C "$PKG_DIR"; `+


Similar error handling issue with the wget/tar pipeline for global package direct dependencies. Failures in download or extraction will not be properly detected, potentially breaking the global package.

Add error handling to ensure wget/tar failures are caught and propagated.

Suggested change

` wget -qO- "$TARBALL_URL" | tar xz --strip-components=1 -C "$PKG_DIR"; `+

` TMP_TARBALL="$(mktemp -t copa-npm-XXXXXX.tgz)" && `+

` wget -qO "$TMP_TARBALL" "$TARBALL_URL" && `+

` tar xz --strip-components=1 -C "$PKG_DIR" -f "$TMP_TARBALL" && `+

` rm -f "$TMP_TARBALL"; `+

Copilot · 2026-02-20T01:38:28Z

+func getPackageBaseName(name string) string {
+	if idx := strings.LastIndex(name, "/"); idx >= 0 {
+		return name[idx+1:]
+	}
+	return name
+}


The new getPackageBaseName helper function lacks unit tests. Given that it handles the critical logic of extracting the base name from scoped packages for tarball URL construction (e.g., "@babel/core" -> "core"), it should have tests to ensure correctness for:

Simple unscoped packages like "lodash" -> "lodash"

Scoped packages like "@babel/core" -> "core"

Edge cases like "@scope/name" -> "name"

Add unit tests for this function in nodejs_test.go to ensure reliable URL construction.

Copilot · 2026-02-20T01:38:30Z

+					`TARBALL_URL="https://registry.npmjs.org/%s/-/%s-%s.tgz" && `+
+					`PKG_NAME="%s" && `+
+					`FOUND=0 && `+
+					`for dir in $(find node_modules -type d -name "$PKG_NAME" -path "*/node_modules/$PKG_NAME" 2>/dev/null); do `+


The find command uses $(...) command substitution which could fail if there are too many matching directories (argument list too long error). Additionally, the pattern */node_modules/$PKG_NAME might match directories at the wrong depth or in unexpected locations.

Consider adding additional safeguards:

Limit the depth of the find command with -maxdepth to avoid searching too deeply

Use -print0 with xargs -0 instead of command substitution to handle large result sets

Add validation to ensure only actual package directories (with package.json) are processed

Copilot · 2026-02-20T01:38:31Z

+			// Scoped packages use @scope/name -> @scope%2fname in the URL
+			encodedName := u.Name
+			if strings.HasPrefix(u.Name, "@") {
+				encodedName = strings.Replace(u.Name, "/", "%2f", 1)


The URL encoding uses lowercase "%2f" here, while the RFC 3986 standard recommends uppercase percent-encoded characters "%2F". Though most servers accept both, using the standard uppercase form would be more correct and consistent with best practices.

Consider changing "%2f" to "%2F" throughout the file for RFC compliance.

Suggested change

// Scoped packages use @scope/name -> @scope%2fname in the URL

encodedName := u.Name

if strings.HasPrefix(u.Name, "@") {

encodedName = strings.Replace(u.Name, "/", "%2f", 1)

// Scoped packages use @scope/name -> @scope%2Fname in the URL

encodedName := u.Name

if strings.HasPrefix(u.Name, "@") {

encodedName = strings.Replace(u.Name, "/", "%2F", 1)

Copilot · 2026-02-20T01:38:31Z

-				escapedOverridesJSON := strings.ReplaceAll(overridesJSON, `"`, `\"`)
+				encodedName := u.Name
+				if strings.HasPrefix(u.Name, "@") {
+					encodedName = strings.Replace(u.Name, "/", "%2f", 1)


Similar URL encoding issue: using lowercase "%2f" instead of the RFC 3986 standard uppercase "%2F".

Consider using uppercase "%2F" for RFC compliance.

Suggested change

encodedName = strings.Replace(u.Name, "/", "%2f", 1)

encodedName = strings.Replace(u.Name, "/", "%2F", 1)

Copilot · 2026-02-20T01:38:31Z

+					`if [ -d "$PKG_DIR" ]; then `+
+					`  echo "INFO: Replacing direct dependency %s with version %s" && `+
+					`  rm -rf "$PKG_DIR" && mkdir -p "$PKG_DIR" && `+
+					`  wget -qO- "$TARBALL_URL" | tar xz --strip-components=1 -C "$PKG_DIR"; `+


The wget and tar commands in the pipeline lack explicit error handling. If either wget fails to download the tarball (e.g., 404 error, network issue) or tar fails to extract it, the shell command will silently continue without failing. This could result in an empty or corrupted package directory being left in place after the rm -rf, breaking the application.

Consider adding explicit error checking or using set -e in the shell command to ensure failures are caught. For example:

Add set -e at the beginning of the shell script

Or add explicit error checking after the wget/tar pipeline

Or ensure wget and tar return non-zero exit codes on failure by appending || exit 1

Suggested change

` wget -qO- "$TARBALL_URL" | tar xz --strip-components=1 -C "$PKG_DIR"; `+

` TMP_TGZ="$(mktemp)" && `+

` if ! wget -qO "$TMP_TGZ" "$TARBALL_URL"; then `+

` echo "ERROR: Failed to download $TARBALL_URL" && rm -f "$TMP_TGZ" && exit 1; `+

` fi && `+

` if ! tar xzf "$TMP_TGZ" --strip-components=1 -C "$PKG_DIR"; then `+

` echo "ERROR: Failed to extract package tarball for %s" && rm -f "$TMP_TGZ" && exit 1; `+

` fi && `+

` rm -f "$TMP_TGZ"; `+

Copilot · 2026-02-20T01:38:31Z

-		).Root()
+			encodedName := u.Name
+			if strings.HasPrefix(u.Name, "@") {
+				encodedName = strings.Replace(u.Name, "/", "%2f", 1)


Similar URL encoding issue: using lowercase "%2f" instead of the RFC 3986 standard uppercase "%2F".

Consider using uppercase "%2F" for RFC compliance.

Suggested change

encodedName = strings.Replace(u.Name, "/", "%2f", 1)

encodedName = strings.Replace(u.Name, "/", "%2F", 1)

…ntroducing new vulnerabilities Signed-off-by: robert-cronin <robert@robertcronin.com>

codecov · 2026-02-20T02:15:17Z

Codecov Report

❌ Patch coverage is 4.66321% with 184 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.39%. Comparing base (d213553) to head (768f0da).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/langmgr/nodejs.go	4.66%	184 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1479      +/-   ##
==========================================
- Coverage   38.77%   38.39%   -0.39%     
==========================================
  Files          53       53              
  Lines        8098     8202     +104     
==========================================
+ Hits         3140     3149       +9     
- Misses       4721     4816      +95     
  Partials      237      237

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ntroducing new vulnerabilities Signed-off-by: robert-cronin <robert@robertcronin.com>

…ty-only additions Merged upstream main (d13c641) into verity branch. Adopted from upstream (dropped verity duplicates): - Python venv patching (project-copacetic#1485) - Bulk skip detection with versioned tags (project-copacetic#1475) - Go binary patching (project-copacetic#1388) - RPM chroot validation (project-copacetic#1529) - npm tarball fix (project-copacetic#1479) - Security hardening (project-copacetic#1506, project-copacetic#1526) Kept verity-only features: - Helm chart patching (pkg/helm/, pkg/bulk/chart.go) - --dry-run and --output-json flags - Chart mode in bulk engine - Chart CLI flags (--chart, --chart-version, --chart-repo, --chart-registry) Conflict resolution: took upstream for upstreamed features, kept verity additions on top. Dropped GetProxy refactor (uses upstream utils.GetProxy). Signed-off-by: Omer <omer@descope.com>

Copilot AI review requested due to automatic review settings February 20, 2026 01:32

robert-cronin requested review from ashnamehrotra, jeremyrickard and sozercan as code owners February 20, 2026 01:32

github-project-automation Bot added this to Copacetic Workboard Feb 20, 2026

github-project-automation Bot moved this to 🆕 New in Copacetic Workboard Feb 20, 2026

robert-cronin added the bug Something isn't working label Feb 20, 2026

Copilot started reviewing on behalf of robert-cronin February 20, 2026 01:33 View session

Copilot AI reviewed Feb 20, 2026

View reviewed changes

fix: replace npm install with direct tarball replacement to prevent i…

e00420b

…ntroducing new vulnerabilities Signed-off-by: robert-cronin <robert@robertcronin.com>

robert-cronin force-pushed the fix/nodejs-npm-patching-regression branch from 1c2943b to e00420b Compare February 20, 2026 02:00

fix: replace npm install with direct tarball replacement to prevent i…

967c3e1

…ntroducing new vulnerabilities Signed-off-by: robert-cronin <robert@robertcronin.com>

robert-cronin force-pushed the fix/nodejs-npm-patching-regression branch from 291c32e to 967c3e1 Compare February 26, 2026 01:20

Merge branch 'main' into fix/nodejs-npm-patching-regression

1582427

ashnamehrotra approved these changes Mar 3, 2026

View reviewed changes

ashnamehrotra and others added 3 commits March 3, 2026 17:30

Merge branch 'main' into fix/nodejs-npm-patching-regression

151c7e5

Merge branch 'main' into fix/nodejs-npm-patching-regression

c185bbc

Merge branch 'main' into fix/nodejs-npm-patching-regression

768f0da

ashnamehrotra merged commit d934758 into project-copacetic:main Mar 6, 2026
75 of 76 checks passed

github-project-automation Bot moved this from 🆕 New to ✅ Done in Copacetic Workboard Mar 6, 2026

Navigation Menu

Uh oh!

fix: replace npm install with direct tarball replacement - #1479

fix: replace npm install with direct tarball replacement#1479
ashnamehrotra merged 6 commits into
project-copacetic:mainfrom
robert-cronin:fix/nodejs-npm-patching-regression

robert-cronin commented Feb 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

Copilot AI Feb 20, 2026

Uh oh!

codecov Bot commented Feb 20, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	encodedName = strings.Replace(u.Name, "/", "%2f", 1)
	encodedName = strings.ReplaceAll(u.Name, "/", "%2F")

-						`  wget -qO- "$TARBALL_URL" | tar xz --strip-components=1 -C "$PKG_DIR"; `+
+						`  TMP_TARBALL="$(mktemp -t copa-npm-XXXXXX.tgz)" && `+
+						`  wget -qO "$TMP_TARBALL" "$TARBALL_URL" && `+
+						`  tar xz --strip-components=1 -C "$PKG_DIR" -f "$TMP_TARBALL" && `+
+						`  rm -f "$TMP_TARBALL"; `+

-					`  wget -qO- "$TARBALL_URL" | tar xz --strip-components=1 -C "$PKG_DIR"; `+
+					`  TMP_TGZ="$(mktemp)" && `+
+					`  if ! wget -qO "$TMP_TGZ" "$TARBALL_URL"; then `+
+					`    echo "ERROR: Failed to download $TARBALL_URL" && rm -f "$TMP_TGZ" && exit 1; `+
+					`  fi && `+
+					`  if ! tar xzf "$TMP_TGZ" --strip-components=1 -C "$PKG_DIR"; then `+
+					`    echo "ERROR: Failed to extract package tarball for %s" && rm -f "$TMP_TGZ" && exit 1; `+
+					`  fi && `+
+					`  rm -f "$TMP_TGZ"; `+

Uh oh!

Conversation

robert-cronin commented Feb 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Feb 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Feb 20, 2026 •

edited

Loading