diff --git a/internal/database/database_test.go b/internal/database/database_test.go index bb2b195..a48d82f 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -1099,3 +1099,56 @@ func BenchmarkMigrateSchemaFullyMigrated(b *testing.B) { } } } + +func TestVersionPublishedAtPreserved(t *testing.T) { + runWithBothDatabases(t, func(t *testing.T, db *DB) { + publishedAt := time.Date(2020, 1, 2, 3, 4, 5, 0, time.UTC) + + if err := db.SetVersionPublishedAt("pkg:npm/leftpad@1.0.0", "pkg:npm/leftpad", publishedAt); err != nil { + t.Fatalf("SetVersionPublishedAt failed: %v", err) + } + + got, err := db.GetVersionByPURL("pkg:npm/leftpad@1.0.0") + if err != nil || got == nil { + t.Fatalf("GetVersionByPURL failed: %v", err) + } + if !got.PublishedAt.Valid || !got.PublishedAt.Time.Equal(publishedAt) { + t.Fatalf("PublishedAt = %v, want %v", got.PublishedAt, publishedAt) + } + + // An upsert that carries no publish time (the artifact cache path) + // must not erase the stored value. + if err := db.UpsertVersion(&Version{ + PURL: "pkg:npm/leftpad@1.0.0", + PackagePURL: "pkg:npm/leftpad", + }); err != nil { + t.Fatalf("UpsertVersion failed: %v", err) + } + + got, err = db.GetVersionByPURL("pkg:npm/leftpad@1.0.0") + if err != nil || got == nil { + t.Fatalf("GetVersionByPURL after upsert failed: %v", err) + } + if !got.PublishedAt.Valid || !got.PublishedAt.Time.Equal(publishedAt) { + t.Fatalf("PublishedAt after null upsert = %v, want %v preserved", got.PublishedAt, publishedAt) + } + + // An upsert that does carry a publish time still updates it. + later := publishedAt.Add(24 * time.Hour) + if err := db.UpsertVersion(&Version{ + PURL: "pkg:npm/leftpad@1.0.0", + PackagePURL: "pkg:npm/leftpad", + PublishedAt: sql.NullTime{Time: later, Valid: true}, + }); err != nil { + t.Fatalf("UpsertVersion with publish time failed: %v", err) + } + + got, err = db.GetVersionByPURL("pkg:npm/leftpad@1.0.0") + if err != nil || got == nil { + t.Fatalf("GetVersionByPURL after second upsert failed: %v", err) + } + if !got.PublishedAt.Valid || !got.PublishedAt.Time.Equal(later) { + t.Fatalf("PublishedAt after valued upsert = %v, want %v", got.PublishedAt, later) + } + }) +} diff --git a/internal/database/queries.go b/internal/database/queries.go index 9fa5381..21ececa 100644 --- a/internal/database/queries.go +++ b/internal/database/queries.go @@ -141,7 +141,7 @@ func (db *DB) UpsertVersion(v *Version) error { ON CONFLICT(purl) DO UPDATE SET license = EXCLUDED.license, integrity = EXCLUDED.integrity, - published_at = EXCLUDED.published_at, + published_at = COALESCE(EXCLUDED.published_at, versions.published_at), yanked = EXCLUDED.yanked, enriched_at = EXCLUDED.enriched_at, updated_at = EXCLUDED.updated_at @@ -154,7 +154,7 @@ func (db *DB) UpsertVersion(v *Version) error { ON CONFLICT(purl) DO UPDATE SET license = excluded.license, integrity = excluded.integrity, - published_at = excluded.published_at, + published_at = COALESCE(excluded.published_at, published_at), yanked = excluded.yanked, enriched_at = excluded.enriched_at, updated_at = excluded.updated_at @@ -171,6 +171,38 @@ func (db *DB) UpsertVersion(v *Version) error { return nil } +// SetVersionPublishedAt records a version's publish time, creating the +// versions row if the proxy has not seen the version yet. It only writes +// published_at, so it never disturbs enrichment data on an existing row. +func (db *DB) SetVersionPublishedAt(versionPURL, packagePURL string, publishedAt time.Time) error { + now := time.Now() + var query string + + if db.dialect == DialectPostgres { + query = ` + INSERT INTO versions (purl, package_purl, published_at, created_at, updated_at) + VALUES ($1, $2, $3, $4, $5) + ON CONFLICT(purl) DO UPDATE SET + published_at = EXCLUDED.published_at, + updated_at = EXCLUDED.updated_at + ` + } else { + query = ` + INSERT INTO versions (purl, package_purl, published_at, created_at, updated_at) + VALUES (?, ?, ?, ?, ?) + ON CONFLICT(purl) DO UPDATE SET + published_at = excluded.published_at, + updated_at = excluded.updated_at + ` + } + + _, err := db.Exec(query, versionPURL, packagePURL, publishedAt, now, now) + if err != nil { + return fmt.Errorf("setting version publish time: %w", err) + } + return nil +} + // Artifact queries func (db *DB) GetArtifact(versionPURL, filename string) (*Artifact, error) { diff --git a/internal/handler/handler.go b/internal/handler/handler.go index a4f75bf..66da2ae 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -71,6 +71,12 @@ func canonicalPackagePURL(ecosystem, name string) string { return purl.MakePURLString(ecosystem, name, "") } +// canonicalVersionPURL returns a versioned PURL in canonical form, matching +// the keys the artifact cache writes to the versions table. +func canonicalVersionPURL(ecosystem, name, version string) string { + return purl.MakePURLString(ecosystem, name, version) +} + const contentTypeJSON = "application/json" const headerAcceptEncoding = "Accept-Encoding" diff --git a/internal/handler/npm.go b/internal/handler/npm.go index b7d96a3..8903189 100644 --- a/internal/handler/npm.go +++ b/internal/handler/npm.go @@ -302,14 +302,22 @@ func (h *NPMHandler) handleDownload(w http.ResponseWriter, r *http.Request) { // predictable and lockfiles record them directly, so `npm ci` reaches the // download path without ever requesting metadata. // -// The packument is served from the metadata cache, so this normally costs no -// extra upstream request. A version with no usable publish time is allowed -// through, matching how applyCooldownFiltering treats it. +// A version's publish time is immutable, so the check reads the stored +// versions row first and only falls back to the packument for a version the +// proxy has never seen, persisting the parsed time so the packument is +// fetched and parsed at most once per version. A version with no usable +// publish time is allowed through, matching how applyCooldownFiltering +// treats it. func (h *NPMHandler) versionInCooldown(r *http.Request, packageName, version string) bool { if h.proxy.Cooldown == nil || !h.proxy.Cooldown.Enabled() { return false } + versionPURL := canonicalVersionPURL("npm", packageName, version) + if ver, err := h.proxy.DB.GetVersionByPURL(versionPURL); err == nil && ver != nil && ver.PublishedAt.Valid { + return !h.proxy.Cooldown.IsAllowed("npm", canonicalPackagePURL("npm", packageName), ver.PublishedAt.Time) + } + upstreamURL := fmt.Sprintf("%s/%s", h.upstreamURL, url.PathEscape(packageName)) body, _, err := h.proxy.FetchOrCacheMetadata(r.Context(), "npm", packageName, upstreamURL, contentTypeJSON) @@ -338,6 +346,11 @@ func (h *NPMHandler) versionInCooldown(r *http.Request, packageName, version str return false } + if err := h.proxy.DB.SetVersionPublishedAt(versionPURL, canonicalPackagePURL("npm", packageName), publishedAt); err != nil { + h.proxy.Logger.Warn("cooldown: could not store npm publish time", + "package", packageName, "version", version, "error", err) + } + return !h.proxy.Cooldown.IsAllowed("npm", canonicalPackagePURL("npm", packageName), publishedAt) } diff --git a/internal/handler/npm_test.go b/internal/handler/npm_test.go index 07da9c3..85cef74 100644 --- a/internal/handler/npm_test.go +++ b/internal/handler/npm_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" @@ -532,3 +533,102 @@ func TestNPMDownloadCooldownDisabled(t *testing.T) { t.Error("versionInCooldown = true, want false when cooldown is not configured") } } + +func TestNPMDownloadCooldownUsesStoredPublishTime(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + t.Error("metadata must not be fetched when the publish time is already stored") + w.WriteHeader(http.StatusInternalServerError) + })) + defer upstream.Close() + + tests := []struct { + name string + version string + publishedAt time.Time + wantStatus int + }{ + {"stored time before the window serves the tarball", testVersion100, time.Now().Add(-30 * 24 * time.Hour), http.StatusOK}, + {"stored time inside the window is withheld", "2.0.0", time.Now().Add(-1 * time.Hour), http.StatusNotFound}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + proxy, db, _, fetcher := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + proxy.Cooldown = &cooldown.Config{Default: "7d"} + fetcher.artifact = &fetch.Artifact{ + Body: io.NopCloser(strings.NewReader("tarball data")), + ContentType: "application/octet-stream", + } + + if err := db.SetVersionPublishedAt("pkg:npm/leftpad@"+tt.version, "pkg:npm/leftpad", tt.publishedAt); err != nil { + t.Fatalf("seeding publish time failed: %v", err) + } + + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL) + srv := httptest.NewServer(h.Routes()) + defer srv.Close() + + resp, err := http.Get(srv.URL + "/leftpad/-/leftpad-" + tt.version + ".tgz") + if err != nil { + t.Fatalf("request failed: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != tt.wantStatus { + t.Errorf("status = %d, want %d", resp.StatusCode, tt.wantStatus) + } + }) + } +} + +func TestNPMDownloadCooldownFetchesMetadataOnce(t *testing.T) { + now := time.Now() + packument := `{ + "name": "leftpad", + "dist-tags": {"latest": "1.0.0"}, + "time": { + "1.0.0": "` + now.Add(-30*24*time.Hour).Format(time.RFC3339) + `" + }, + "versions": {"1.0.0": {}} + }` + + var metadataRequests atomic.Int64 + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + metadataRequests.Add(1) + w.Header().Set("Content-Type", contentTypeJSON) + _, _ = io.WriteString(w, packument) + })) + defer upstream.Close() + + proxy, _, _, fetcher := setupTestProxy(t) + proxy.HTTPClient = upstream.Client() + proxy.Cooldown = &cooldown.Config{Default: "7d"} + + h := NewNPMHandler(proxy, "http://proxy.test", upstream.URL) + srv := httptest.NewServer(h.Routes()) + defer srv.Close() + + // The first download parses the packument once and persists the publish + // time; caching the artifact afterwards upserts the versions row without a + // publish time, which must not erase the stored value. The second download + // must answer from the stored time alone. + for i := 0; i < 2; i++ { + fetcher.artifact = &fetch.Artifact{ + Body: io.NopCloser(strings.NewReader("tarball data")), + ContentType: "application/octet-stream", + } + resp, err := http.Get(srv.URL + "/leftpad/-/leftpad-" + testVersion100 + ".tgz") + if err != nil { + t.Fatalf("request %d failed: %v", i+1, err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("request %d status = %d, want %d", i+1, resp.StatusCode, http.StatusOK) + } + } + + if got := metadataRequests.Load(); got != 1 { + t.Errorf("metadata requests = %d, want 1", got) + } +}