fix: npm Dependency Cooldown Select Stable Version (#291)

* fix: npm Dependency Cooldown Select Stable Version

* fix: Code review fixes

proxy/interceptors/cooldown.go

@@ -1,9 +1,11 @@
 package interceptors
 
 import (
+	"fmt"
 	"time"
 
 	packagev1 "buf.build/gen/go/safedep/api/protocolbuffers/go/safedep/messages/package/v1"
+	"github.com/Masterminds/semver"
 	"github.com/safedep/pmg/internal/audit"
 )
 
@@ -70,17 +72,51 @@ func recordCooldownStats(statsCollector *AnalysisStatsCollector, ecosystem packa
 	}
 }
 
-// cooldownLatestEligibleVersion returns the most recently published version not in tooNew.
-func cooldownLatestEligibleVersion(dates map[string]time.Time, tooNew map[string]bool) string {
+// cooldownHighestStableVersion returns the highest stable (non-prerelease) version
+// among candidates that does not exceed upperBound, ordered by semver. This mirrors
+// what npm treats as the "latest" dist-tag.
+//
+// Prerelease versions are excluded — semver classifies both alpha builds
+// (e.g. 1.2.0-alpha.1) and platform-specific builds (e.g. 1.2.0-win32-arm64) as
+// prereleases, so neither can be promoted to latest. Unparseable versions are skipped.
+//
+// upperBound is the version dist-tags.latest currently points to. Bounding by it keeps
+// a repaired latest on the lineage the maintainer marked as latest, rather than
+// promoting a higher major/minor published under a different channel (e.g. `next`).
+// An empty or unparseable upperBound applies no upper bound.
+func cooldownHighestStableVersion(candidates []string, upperBound string) string {
+	var bound *semver.Version
+	if upperBound != "" {
+		if b, err := semver.NewVersion(upperBound); err == nil {
+			// If latest itself points to a prerelease/platform build (e.g.
+			// 1.0.0-win32-arm64), bound to its base release. The bound represents a
+			// release line, and semver ranks 1.0.0 > 1.0.0-win32-arm64, so without
+			// this the stable counterpart on the same line would be wrongly excluded.
+			if b.Prerelease() != "" {
+				if base, err := semver.NewVersion(fmt.Sprintf("%d.%d.%d", b.Major(), b.Minor(), b.Patch())); err == nil {
+					b = base
+				}
+			}
+			bound = b
+		}
+	}
+
 	var latest string
-	var latestTime time.Time
-	for version, publishDate := range dates {
-		if tooNew[version] {
+	var latestVer *semver.Version
+	for _, version := range candidates {
+		ver, err := semver.NewVersion(version)
+		if err != nil {
 			continue
 		}
-		if publishDate.After(latestTime) {
+		if ver.Prerelease() != "" {
+			continue
+		}
+		if bound != nil && ver.GreaterThan(bound) {
+			continue
+		}
+		if latestVer == nil || ver.GreaterThan(latestVer) {
 			latest = version
-			latestTime = publishDate
+			latestVer = ver
 		}
 	}
 	return latest

proxy/interceptors/cooldown_test.go

@@ -131,34 +131,72 @@ func TestCooldownOldestVersion(t *testing.T) {
 	})
 }
 
-func TestCooldownLatestEligibleVersion(t *testing.T) {
-	now := time.Now()
-	day := 24 * time.Hour
+func TestCooldownHighestStableVersion(t *testing.T) {
+	tests := []struct {
+		name       string
+		candidates []string
+		upperBound string
+		want       string
+	}{
+		{
+			name:       "highest stable by semver, not lexical",
+			candidates: []string{"0.9.0", "0.10.0", "0.2.0"},
+			want:       "0.10.0",
+		},
+		{
+			name:       "excludes prerelease and platform builds",
+			candidates: []string{"0.132.0", "0.132.5-win32-arm64", "0.133.0-alpha.3", "0.131.0"},
+			want:       "0.132.0",
+		},
+		{
+			name:       "no stable version returns empty",
+			candidates: []string{"1.0.0-alpha.1", "1.0.0-win32-arm64"},
+			want:       "",
+		},
+		{
+			name:       "unparseable versions skipped",
+			candidates: []string{"latest", "not-a-version", "1.2.3"},
+			want:       "1.2.3",
+		},
+		{
+			name:       "single stable",
+			candidates: []string{"2.0.0"},
+			want:       "2.0.0",
+		},
+		{
+			name:       "empty input",
+			candidates: []string{},
+			want:       "",
+		},
+		{
+			name:       "upper bound excludes higher major from another channel",
+			candidates: []string{"1.4.0", "2.0.0"},
+			upperBound: "1.5.0",
+			want:       "1.4.0",
+		},
+		{
+			name:       "upper bound allows versions at or below it",
+			candidates: []string{"1.4.0", "1.5.0", "2.0.0"},
+			upperBound: "1.5.0",
+			want:       "1.5.0",
+		},
+		{
+			name:       "unparseable upper bound applies no bound",
+			candidates: []string{"1.4.0", "2.0.0"},
+			upperBound: "not-a-version",
+			want:       "2.0.0",
+		},
+		{
+			name:       "prerelease upper bound does not exclude its stable counterpart",
+			candidates: []string{"1.0.0", "0.9.0"},
+			upperBound: "1.0.0-win32-arm64",
+			want:       "1.0.0",
+		},
+	}
 
-	t.Run("returns most recently published non-blocked version", func(t *testing.T) {
-		dates := map[string]time.Time{
-			"1.0.0": now.Add(-30 * day),
-			"2.0.0": now.Add(-10 * day),
-			"3.0.0": now.Add(-1 * day),
-		}
-		tooNew := map[string]bool{"3.0.0": true}
-		ver := cooldownLatestEligibleVersion(dates, tooNew)
-		assert.Equal(t, "2.0.0", ver)
-	})
-
-	t.Run("all versions blocked returns empty string", func(t *testing.T) {
-		dates := map[string]time.Time{"1.0.0": now, "2.0.0": now.Add(-1 * day)}
-		tooNew := map[string]bool{"1.0.0": true, "2.0.0": true}
-		ver := cooldownLatestEligibleVersion(dates, tooNew)
-		assert.Empty(t, ver)
-	})
-
-	t.Run("empty tooNew returns latest version", func(t *testing.T) {
-		dates := map[string]time.Time{
-			"1.0.0": now.Add(-30 * day),
-			"2.0.0": now.Add(-10 * day),
-		}
-		ver := cooldownLatestEligibleVersion(dates, map[string]bool{})
-		assert.Equal(t, "2.0.0", ver)
-	})
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, cooldownHighestStableVersion(tt.candidates, tt.upperBound))
+		})
+	}
 }

proxy/interceptors/npm_cooldown.go

@@ -143,6 +143,13 @@ func (h *npmCooldownHandler) stripCooldownVersions(body []byte, dates map[string
 		return body, 0, remaining
 	}
 
+	// survivingVersions holds the version keys still present in the "versions" object
+	// after stripping. It is nil if the field is missing or unparseable, in which case
+	// dist-tag repair falls back to the publish-date set. When non-nil it bounds the
+	// repair candidates so a repaired dist-tag never points to a version absent from
+	// the packument (e.g. an unpublished version whose "time" entry lingers).
+	var survivingVersions map[string]bool
+
 	if raw, ok := metadata["versions"]; ok {
 		var versions map[string]json.RawMessage
 		if err := json.Unmarshal(raw, &versions); err != nil {
@@ -151,6 +158,10 @@ func (h *npmCooldownHandler) stripCooldownVersions(body []byte, dates map[string
 			for v := range tooNew {
 				delete(versions, v)
 			}
+			survivingVersions = make(map[string]bool, len(versions))
+			for v := range versions {
+				survivingVersions[v] = true
+			}
 			if updated, err := json.Marshal(versions); err != nil {
 				log.Warnf("Cooldown: failed to marshal updated versions: %v", err)
 			} else {
@@ -181,17 +192,48 @@ func (h *npmCooldownHandler) stripCooldownVersions(body []byte, dates map[string
 			log.Warnf("Cooldown: failed to unmarshal dist-tags field: %v", err)
 		} else {
 			changed := false
-			for tag, version := range distTags {
-				if tooNew[version] {
-					latest := cooldownLatestEligibleVersion(dates, tooNew)
-					if latest != "" {
-						distTags[tag] = latest
-					} else {
-						delete(distTags, tag)
+
+			// Repair the latest tag only when it points at a stripped version.
+			// The eligible-version scan and semver parsing are deferred to this
+			// branch so an unaffected latest tag costs nothing.
+			if latest, ok := distTags["latest"]; ok && tooNew[latest] {
+				eligible := make([]string, 0, len(dates))
+				for v := range dates {
+					if tooNew[v] {
+						continue
 					}
+					if survivingVersions != nil && !survivingVersions[v] {
+						continue // not in the packument's versions — would dangle
+					}
+					eligible = append(eligible, v)
+				}
+
+				if latestStable := cooldownHighestStableVersion(eligible, latest); latestStable != "" {
+					// Repair latest to the highest stable eligible version so
+					// `npm install <pkg>` resolves a real release — never a
+					// more-recently-published prerelease or platform-specific
+					// build (see #275).
+					log.Infof("Cooldown: repaired dist-tag latest %s -> %s for stripped version", latest, latestStable)
+					distTags["latest"] = latestStable
+				} else {
+					// No stable version survives — drop the tag so npm fails
+					// cleanly instead of mis-resolving.
+					log.Infof("Cooldown: removed dist-tag latest (was %s, no eligible stable version remains)", latest)
+					delete(distTags, "latest")
+				}
+				changed = true
+			}
+
+			// Drop any non-latest tag (beta, next, platform tags) whose target was
+			// stripped: an explicit `pkg@<tag>` request for a version in cooldown
+			// should fail cleanly, not resolve to an unrelated version.
+			for tag, version := range distTags {
+				if tag != "latest" && tooNew[version] {
+					delete(distTags, tag)
 					changed = true
 				}
 			}
+
 			if changed {
 				if updated, err := json.Marshal(distTags); err != nil {
 					log.Warnf("Cooldown: failed to marshal updated dist-tags: %v", err)

proxy/interceptors/npm_cooldown_test.go

@@ -198,13 +198,16 @@ func TestStripCooldownVersions_MixedVersions(t *testing.T) {
 
 	var resultDistTags map[string]string
 	require.NoError(t, json.Unmarshal(result["dist-tags"], &resultDistTags))
-	// latest should be updated to an older eligible version
-	assert.NotEqual(t, "1.0.2", resultDistTags["latest"])
+	// latest should be repaired to the highest stable eligible version
+	assert.Equal(t, "1.0.1", resultDistTags["latest"])
 
 	var resultTime map[string]string
 	require.NoError(t, json.Unmarshal(result["time"], &resultTime))
 	assert.Contains(t, resultTime, "created")
 	assert.Contains(t, resultTime, "modified")
+	assert.NotContains(t, resultTime, "1.0.2")
+	assert.Contains(t, resultTime, "1.0.0")
+	assert.Contains(t, resultTime, "1.0.1")
 }
 
 func TestStripCooldownVersions_AllVersionsTooNew(t *testing.T) {
@@ -279,6 +282,134 @@ func TestStripCooldownVersions_MalformedJSON(t *testing.T) {
 	assert.Equal(t, body, newBody)
 }
 
+// Regression for #275: when the stable version that dist-tags.latest points to is
+// stripped, latest must be repaired to the highest *stable* eligible version — never
+// a more-recently-published prerelease or platform-specific build (e.g. -win32-arm64).
+func TestStripCooldownVersions_LatestRepairedToStableNotPlatform(t *testing.T) {
+	handler := newNpmCooldownHandler(nil)
+	now := time.Now()
+	day := 24 * time.Hour
+	versions := map[string]time.Time{
+		"0.131.0":             now.Add(-40 * day), // old stable
+		"0.132.0":             now.Add(-30 * day), // old stable — expected latest after repair
+		"0.132.5-win32-arm64": now.Add(-6 * day),  // eligible platform build, newer than 0.132.0
+		"0.133.0":             now.Add(-1 * day),  // too new stable (current latest)
+		"0.133.0-win32-arm64": now.Add(-1 * day),  // too new platform build
+	}
+	distTags := map[string]string{"latest": "0.133.0"}
+	body := buildTestPackument(versions, distTags)
+
+	dates, err := handler.parseMetadataTime(body)
+	require.NoError(t, err)
+
+	newBody, _, _ := handler.stripCooldownVersions(body, dates, 5)
+
+	var result map[string]json.RawMessage
+	require.NoError(t, json.Unmarshal(newBody, &result))
+
+	var resultDistTags map[string]string
+	require.NoError(t, json.Unmarshal(result["dist-tags"], &resultDistTags))
+	assert.Equal(t, "0.132.0", resultDistTags["latest"],
+		"latest must be the highest stable eligible version, not a platform/prerelease build")
+}
+
+// Non-latest dist-tags whose target is stripped should be removed, not rewritten to
+// an unrelated version.
+func TestStripCooldownVersions_NonLatestTagRemovedWhenStripped(t *testing.T) {
+	handler := newNpmCooldownHandler(nil)
+	now := time.Now()
+	day := 24 * time.Hour
+	versions := map[string]time.Time{
+		"1.0.0":        now.Add(-30 * day), // eligible stable
+		"2.0.0-beta.1": now.Add(-1 * day),  // too new prerelease
+	}
+	distTags := map[string]string{"latest": "1.0.0", "next": "2.0.0-beta.1"}
+	body := buildTestPackument(versions, distTags)
+
+	dates, err := handler.parseMetadataTime(body)
+	require.NoError(t, err)
+
+	newBody, _, _ := handler.stripCooldownVersions(body, dates, 5)
+
+	var result map[string]json.RawMessage
+	require.NoError(t, json.Unmarshal(newBody, &result))
+
+	var resultDistTags map[string]string
+	require.NoError(t, json.Unmarshal(result["dist-tags"], &resultDistTags))
+	assert.Equal(t, "1.0.0", resultDistTags["latest"], "eligible latest tag should be untouched")
+	assert.NotContains(t, resultDistTags, "next", "stripped non-latest tag should be removed")
+}
+
+// A repaired latest must point to a version that still exists in the "versions"
+// object. A version present only in "time" (e.g. an unpublished version whose
+// timestamp lingers) must not be promoted to latest, or npm would get a dangling tag.
+func TestStripCooldownVersions_LatestRepairSkipsVersionsMissingFromPackument(t *testing.T) {
+	handler := newNpmCooldownHandler(nil)
+	old := time.Now().Add(-30 * 24 * time.Hour).Format(time.RFC3339)
+	tooNew := time.Now().Add(-1 * 24 * time.Hour).Format(time.RFC3339)
+
+	// "9.9.9" appears in time but NOT in versions; "2.0.0" (latest) is in cooldown.
+	body := []byte(`{
+		"name": "testpkg",
+		"dist-tags": {"latest": "2.0.0"},
+		"versions": {
+			"1.0.0": {"version": "1.0.0"},
+			"1.0.1": {"version": "1.0.1"},
+			"2.0.0": {"version": "2.0.0"}
+		},
+		"time": {
+			"1.0.0": "` + old + `",
+			"1.0.1": "` + old + `",
+			"9.9.9": "` + old + `",
+			"2.0.0": "` + tooNew + `"
+		}
+	}`)
+
+	dates, err := handler.parseMetadataTime(body)
+	require.NoError(t, err)
+
+	newBody, _, _ := handler.stripCooldownVersions(body, dates, 5)
+
+	var result map[string]json.RawMessage
+	require.NoError(t, json.Unmarshal(newBody, &result))
+
+	var resultDistTags map[string]string
+	require.NoError(t, json.Unmarshal(result["dist-tags"], &resultDistTags))
+	assert.Equal(t, "1.0.1", resultDistTags["latest"],
+		"latest must come from versions present in the packument, not a time-only entry")
+}
+
+// Repairing latest must respect the maintainer's dist-tag lineage: when latest is
+// pinned to an older line while a higher stable major lives under another channel
+// (e.g. next), stripping the fresh latest must fall back within the blessed line,
+// not promote the unrelated higher major.
+func TestStripCooldownVersions_LatestRepairStaysWithinBlessedLineage(t *testing.T) {
+	handler := newNpmCooldownHandler(nil)
+	now := time.Now()
+	day := 24 * time.Hour
+	versions := map[string]time.Time{
+		"1.4.0": now.Add(-40 * day), // eligible — previous blessed release
+		"1.5.0": now.Add(-1 * day),  // fresh — current latest, stripped
+		"2.0.0": now.Add(-30 * day), // eligible higher major, published under `next`
+	}
+	distTags := map[string]string{"latest": "1.5.0", "next": "2.0.0"}
+	body := buildTestPackument(versions, distTags)
+
+	dates, err := handler.parseMetadataTime(body)
+	require.NoError(t, err)
+
+	newBody, _, _ := handler.stripCooldownVersions(body, dates, 5)
+
+	var result map[string]json.RawMessage
+	require.NoError(t, json.Unmarshal(newBody, &result))
+
+	var resultDistTags map[string]string
+	require.NoError(t, json.Unmarshal(result["dist-tags"], &resultDistTags))
+	assert.Equal(t, "1.4.0", resultDistTags["latest"],
+		"latest must stay within the lineage it was pinned to, not jump to a higher major")
+	assert.Equal(t, "2.0.0", resultDistTags["next"], "eligible non-latest tag should be untouched")
+}
+
 func makeTestRequestContext(rawURL string) *proxy.RequestContext {
 	u := mustParseURL(rawURL)
 	return &proxy.RequestContext{