From 8cb7b7d1c08812c70b6b361f3aae7adf0b8cbdbb Mon Sep 17 00:00:00 2001 From: Abhisek Datta Date: Mon, 25 May 2026 14:38:39 +0530 Subject: [PATCH] fix: npm Dependency Cooldown Select Stable Version (#291) * fix: npm Dependency Cooldown Select Stable Version * fix: Code review fixes --- proxy/interceptors/cooldown.go | 50 +++++++-- proxy/interceptors/cooldown_test.go | 96 ++++++++++++----- proxy/interceptors/npm_cooldown.go | 56 ++++++++-- proxy/interceptors/npm_cooldown_test.go | 135 +++++++++++++++++++++++- 4 files changed, 292 insertions(+), 45 deletions(-) diff --git a/proxy/interceptors/cooldown.go b/proxy/interceptors/cooldown.go index b6aa8e3..aed14f2 100644 --- a/proxy/interceptors/cooldown.go +++ b/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 diff --git a/proxy/interceptors/cooldown_test.go b/proxy/interceptors/cooldown_test.go index 87a2865..51fbbec 100644 --- a/proxy/interceptors/cooldown_test.go +++ b/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)) + }) + } } diff --git a/proxy/interceptors/npm_cooldown.go b/proxy/interceptors/npm_cooldown.go index 9a65be8..fa67c5e 100644 --- a/proxy/interceptors/npm_cooldown.go +++ b/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 ` 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@` 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) diff --git a/proxy/interceptors/npm_cooldown_test.go b/proxy/interceptors/npm_cooldown_test.go index 22016b2..4adf77d 100644 --- a/proxy/interceptors/npm_cooldown_test.go +++ b/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{