From 7cd0edf7ffa40f38ba98d27b58dcb011dbcea53a Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 25 Oct 2026 13:36:00 +1100 Subject: [PATCH] fail closed on quarantine misconfig and first-seen errors - backfill quarantine_days<=0 to 3 with migration 0006 - reject quarantine_enabled with quarantine_days<1 on create and update - cap upstream Last-Modified at now - answer 503 with Retry-After when the first-seen lookup fails --- docs/common/caching.md | 4 ++- internal/api/v1/proxy.go | 3 ++ internal/api/v1/proxyerror_test.go | 16 ++++++++++ internal/api/v2/remotes.go | 13 +++++--- internal/api/v2/remotes_ttl_test.go | 28 +++++++++++++++++ internal/proxy/engine.go | 24 ++++++++++----- internal/proxy/quarantine_test.go | 48 +++++++++++++++++++++++++++++ migrations/0006_quarantine_days.sql | 1 + pkg/models/remote.go | 11 +++++++ 9 files changed, 136 insertions(+), 12 deletions(-) create mode 100644 internal/api/v1/proxyerror_test.go create mode 100644 migrations/0006_quarantine_days.sql diff --git a/docs/common/caching.md b/docs/common/caching.md index 34f45e0..5a88f3d 100644 --- a/docs/common/caching.md +++ b/docs/common/caching.md @@ -62,7 +62,9 @@ was published less than `quarantine_days` ago answers `403` and is not cached: "published": "2026-10-24T09:00:00Z", "available": "2026-10-27T09:00:00Z"} ``` -- The publish time is the upstream `Last-Modified` header, else the first time artifactapi was asked for the path. +- `quarantine_days` must be at least `1` when quarantine is enabled. +- The publish time is the upstream `Last-Modified` header (capped at now), else the first time artifactapi was asked for the path. +- If that first-seen lookup fails, the path answers `503` with `Retry-After` instead. - Mutable paths (indexes, metadata) are never quarantined. - Paths already cached keep serving, including ones cached before quarantine was enabled. - A virtual redirects to the member that owns the path, so its `403` reaches the client. A goproxy virtual probing for an unlisted version skips a quarantining member and only redirects to it when no other member serves the version. diff --git a/internal/api/v1/proxy.go b/internal/api/v1/proxy.go index f7b413f..a80f737 100644 --- a/internal/api/v1/proxy.go +++ b/internal/api/v1/proxy.go @@ -153,6 +153,9 @@ func (h *ProxyHandler) serveFetch(w http.ResponseWriter, r *http.Request, remote // writeProxyError answers a quarantine refusal as JSON and anything else as text. func writeProxyError(w http.ResponseWriter, pe *proxy.ProxyError) { + if pe.RetryAfter > 0 { + w.Header().Set("Retry-After", strconv.Itoa(pe.RetryAfter)) + } if pe.Published.IsZero() { http.Error(w, pe.Message, pe.Status) return diff --git a/internal/api/v1/proxyerror_test.go b/internal/api/v1/proxyerror_test.go new file mode 100644 index 0000000..009b9c5 --- /dev/null +++ b/internal/api/v1/proxyerror_test.go @@ -0,0 +1,16 @@ +package v1 + +import ( + "net/http/httptest" + "testing" + + "git.unkin.net/unkin/artifactapi/internal/proxy" +) + +func TestWriteProxyErrorRetryAfter(t *testing.T) { + w := httptest.NewRecorder() + writeProxyError(w, &proxy.ProxyError{Status: 503, Message: "quarantine check unavailable", RetryAfter: 30}) + if w.Code != 503 || w.Header().Get("Retry-After") != "30" { + t.Errorf("code %d, Retry-After %q", w.Code, w.Header().Get("Retry-After")) + } +} diff --git a/internal/api/v2/remotes.go b/internal/api/v2/remotes.go index e2d8f92..5eb530e 100644 --- a/internal/api/v2/remotes.go +++ b/internal/api/v2/remotes.go @@ -71,11 +71,8 @@ func (h *RemotesHandler) get(w http.ResponseWriter, r *http.Request) { // that omits mutable_ttl gets it, an explicit 0 still means never expire. const defaultMutableTTL = 3600 -// defaultQuarantineDays matches the remotes.quarantine_days column default. -const defaultQuarantineDays = 3 - func (h *RemotesHandler) create(w http.ResponseWriter, r *http.Request) { - remote := models.Remote{MutableTTL: defaultMutableTTL, QuarantineDays: defaultQuarantineDays} + remote := models.Remote{MutableTTL: defaultMutableTTL, QuarantineDays: models.DefaultQuarantineDays} if err := json.NewDecoder(r.Body).Decode(&remote); err != nil { http.Error(w, "invalid json", http.StatusBadRequest) return @@ -107,6 +104,10 @@ func (h *RemotesHandler) create(w http.ResponseWriter, r *http.Request) { http.Error(w, err.Error(), http.StatusBadRequest) return } + if err := remote.ValidateQuarantine(); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } if err := h.db.CreateRemote(r.Context(), &remote); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return @@ -148,6 +149,10 @@ func (h *RemotesHandler) update(w http.ResponseWriter, r *http.Request) { http.Error(w, err.Error(), http.StatusBadRequest) return } + if err := remote.ValidateQuarantine(); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } if err := h.db.UpdateRemote(r.Context(), &remote); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) return diff --git a/internal/api/v2/remotes_ttl_test.go b/internal/api/v2/remotes_ttl_test.go index d6a8058..8f55e97 100644 --- a/internal/api/v2/remotes_ttl_test.go +++ b/internal/api/v2/remotes_ttl_test.go @@ -91,3 +91,31 @@ func TestQuarantineDaysDefault(t *testing.T) { } } } + +func TestQuarantineDaysValidation(t *testing.T) { + if testDSN == "" { + t.Skip("Docker unavailable") + } + db, err := database.New(testDSN) + if err != nil { + t.Fatal(err) + } + defer db.Close() + h := NewRemotesHandler(db, nil, nil).Routes() + const body = `"package_type":"npm","base_url":"https://example.com"` + for i, s := range []struct { + method, path, json string + want int + }{ + {"POST", "/", `{"name":"q-val",` + body + `,"quarantine_enabled":true,"quarantine_days":0}`, 400}, + {"POST", "/", `{"name":"q-val",` + body + `,"quarantine_enabled":true,"quarantine_days":-1}`, 400}, + {"POST", "/", `{"name":"q-val",` + body + `,"quarantine_days":0}`, 201}, + {"PUT", "/q-val", `{` + body + `,"quarantine_enabled":true}`, 400}, + {"PUT", "/q-val", `{` + body + `,"quarantine_enabled":true,"quarantine_days":1}`, 200}, + {"PUT", "/q-val", `{` + body + `,"quarantine_enabled":true,"quarantine_days":0}`, 400}, + } { + if c := do(t, h, s.method, s.path, s.json); c != s.want { + t.Errorf("step %d %s %s = %d, want %d", i, s.method, s.path, c, s.want) + } + } +} diff --git a/internal/proxy/engine.go b/internal/proxy/engine.go index 9cab5a0..aef9118 100644 --- a/internal/proxy/engine.go +++ b/internal/proxy/engine.go @@ -674,24 +674,32 @@ func (e *Engine) checkUpstreamOnce(ctx context.Context, remote models.Remote, pa } // checkQuarantine refuses an uncached immutable artifact published less than -// quarantine_days ago. The publish time is the upstream Last-Modified, else -// the first time this path was seen. +// quarantine_days ago. The publish time is the upstream Last-Modified, capped +// at now, else the first time this path was seen. func (e *Engine) checkQuarantine(ctx context.Context, remote models.Remote, path string, upstream http.Header) error { - if !remote.QuarantineEnabled || remote.QuarantineDays <= 0 { + if !remote.QuarantineEnabled { return nil } if a, err := e.db.GetArtifact(ctx, remote.Name, path); err == nil && a != nil { return nil } - window := time.Duration(remote.QuarantineDays) * 24 * time.Hour + days := remote.QuarantineDays + if days < 1 { + days = models.DefaultQuarantineDays + } + window := time.Duration(days) * 24 * time.Hour + now := time.Now() published, err := http.ParseTime(upstream.Get("Last-Modified")) if err != nil { if published, err = e.cache.FirstSeen(ctx, remote.Name, path, window+24*time.Hour); err != nil { - slog.Warn("first-seen lookup failed, quarantining", "remote", remote.Name, "path", path, "error", err) - published = time.Now() + slog.Warn("first-seen lookup failed", "remote", remote.Name, "path", path, "error", err) + return &ProxyError{Status: http.StatusServiceUnavailable, Message: "quarantine check unavailable", RetryAfter: 30} } } - if until := published.Add(window); time.Now().Before(until) { + if published.After(now) { + published = now + } + if until := published.Add(window); now.Before(until) { return &ProxyError{ Status: http.StatusForbidden, Message: fmt.Sprintf("quarantined: published %s, available from %s", published.UTC().Format(time.RFC3339), until.UTC().Format(time.RFC3339)), @@ -863,6 +871,8 @@ type ProxyError struct { // Published and Available are set only on a quarantine refusal. Published time.Time Available time.Time + // RetryAfter, in seconds, is sent as Retry-After when set. + RetryAfter int } func (e *ProxyError) Error() string { return e.Message } diff --git a/internal/proxy/quarantine_test.go b/internal/proxy/quarantine_test.go index 1b91220..eb4c1c9 100644 --- a/internal/proxy/quarantine_test.go +++ b/internal/proxy/quarantine_test.go @@ -8,6 +8,7 @@ import ( "testing" "time" + "git.unkin.net/unkin/artifactapi/internal/cache" "git.unkin.net/unkin/artifactapi/pkg/models" goredis "github.com/redis/go-redis/v9" ) @@ -27,6 +28,8 @@ func quarantineUpstream(t *testing.T) *httptest.Server { w.Header().Set("Last-Modified", time.Now().Add(-time.Hour).UTC().Format(http.TimeFormat)) case "/old.tgz": w.Header().Set("Last-Modified", time.Now().Add(-30*24*time.Hour).UTC().Format(http.TimeFormat)) + case "/future.tgz": + w.Header().Set("Last-Modified", time.Now().Add(30*24*time.Hour).UTC().Format(http.TimeFormat)) case "/undated.tgz", "/pkg": default: http.NotFound(w, r) @@ -136,3 +139,48 @@ func TestQuarantineKeepsServingCached(t *testing.T) { t.Errorf("artifact cached before quarantine refused on refetch: %v", err) } } + +func TestQuarantineCapsFutureLastModified(t *testing.T) { + requireStack(t) + r := quarantineRemote(t, "q-future", true) + _, err := testEngine.Fetch(context.Background(), r, "future.tgz", prov(t, models.PackageNPM)) + pe := wantQuarantined(t, err) + if d := time.Until(pe.Available); d < 71*time.Hour || d > 72*time.Hour { + t.Errorf("available in %s, want ~72h", d) + } +} + +func TestQuarantineRedisErrorUnavailable(t *testing.T) { + requireStack(t) + ctx := context.Background() + r := quarantineRemote(t, "q-redis-down", true) + r.StaleOnError = true + down := must(cache.NewRedis(testRedis)) + _ = down.Close() + eng := NewEngine(testDB, down, testEngine.store) + _, err := eng.Fetch(ctx, r, "undated.tgz", prov(t, models.PackageNPM)) + var pe *ProxyError + if !errors.As(err, &pe) || pe.Status != http.StatusServiceUnavailable || pe.RetryAfter == 0 || Quarantined(err) { + t.Fatalf("want 503 with Retry-After, got %#v", err) + } + if a, _ := testDB.GetArtifact(ctx, r.Name, "undated.tgz"); a != nil { + t.Error("artifact cached while quarantine check unavailable") + } +} + +func TestQuarantineStaleOnErrorServesNothingUncached(t *testing.T) { + requireStack(t) + ctx := context.Background() + r := quarantineRemote(t, "q-stale", true) + r.StaleOnError = true + p := prov(t, models.PackageNPM) + _, err := testEngine.Fetch(ctx, r, "fresh.tgz", p) + _ = wantQuarantined(t, err) + r.BaseURL = "http://127.0.0.1:1" + if _, err := testEngine.Fetch(ctx, r, "fresh.tgz", p); err == nil { + t.Error("stale_on_error served an uncached quarantined path") + } + if a, _ := testDB.GetArtifact(ctx, r.Name, "fresh.tgz"); a != nil { + t.Error("quarantined artifact was cached") + } +} diff --git a/migrations/0006_quarantine_days.sql b/migrations/0006_quarantine_days.sql new file mode 100644 index 0000000..5a72bf5 --- /dev/null +++ b/migrations/0006_quarantine_days.sql @@ -0,0 +1 @@ +UPDATE remotes SET quarantine_days = 3 WHERE quarantine_days IS NULL OR quarantine_days <= 0; diff --git a/pkg/models/remote.go b/pkg/models/remote.go index fd27aa6..2a6ef95 100644 --- a/pkg/models/remote.go +++ b/pkg/models/remote.go @@ -149,6 +149,17 @@ func (r *Remote) ValidateMirrorStrategy() error { return nil } +// DefaultQuarantineDays matches the remotes.quarantine_days column default. +const DefaultQuarantineDays = 3 + +// ValidateQuarantine rejects an enabled quarantine with no window. +func (r *Remote) ValidateQuarantine() error { + if r.QuarantineEnabled && r.QuarantineDays < 1 { + return fmt.Errorf("quarantine_days must be at least 1 when quarantine_enabled is set") + } + return nil +} + // ValidatePatterns ensures every configured regex compiles. Storing an // invalid pattern would otherwise be silently dropped at match time, which // for the blocklist is a fail-open: a mistyped deny rule becomes a no-op.