From 62c009b8374b717590228bf96068e2645baa55c8 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 25 Oct 2026 13:23:30 +1100 Subject: [PATCH 1/2] enforce remote quarantine for fresh immutable artifacts - refuse uncached immutable paths published under quarantine_days ago with a JSON 403 - take publish time from Last-Modified, else a Redis first-seen key - let goproxy virtual probes skip a quarantining member - default quarantine_days to 3 on create and keep it on update - document quarantine in caching.md --- docs/common/caching.md | 18 +++- internal/api/v1/proxy.go | 21 +++- internal/api/v2/remotes.go | 6 +- internal/api/v2/remotes_ttl_test.go | 36 +++++++ internal/cache/redis.go | 14 +++ internal/proxy/engine.go | 56 +++++++++- internal/proxy/engine_test.go | 2 +- internal/proxy/quarantine_test.go | 138 ++++++++++++++++++++++++ internal/virtual/goproxy_merger.go | 13 ++- internal/virtual/goproxy_merger_test.go | 33 ++++++ 10 files changed, 327 insertions(+), 10 deletions(-) create mode 100644 internal/proxy/quarantine_test.go diff --git a/docs/common/caching.md b/docs/common/caching.md index 3b32604..34f45e0 100644 --- a/docs/common/caching.md +++ b/docs/common/caching.md @@ -15,7 +15,7 @@ Every proxied request on a remote is classified as **denied**, **immutable** or | `mutable_ttl` | Seconds a mutable path stays fresh. `0` also means never expires. Omitted on create, it defaults to `3600`; omitted on update, the stored value is kept. | | `check_mutable` | When a mutable path's `mutable_ttl` expires, sends a conditional `HEAD` (`If-None-Match`) with the stored upstream ETag. A `304` re-arms the TTL and serves the stored copy; anything else refetches in full. The ETag is kept for `max(10 × mutable_ttl, 7 days)`, re-armed on each `304`, so idle paths age out. It is only stored when `check_mutable` is set and `mutable_ttl` is non-zero, and never for immutable paths. | | `stale_on_error` | On an upstream network error, serve the stored copy instead of failing. | -| `quarantine_enabled` / `quarantine_days` | Stored on the remote but not enforced by the proxy yet. | +| `quarantine_enabled` / `quarantine_days` | Refuses immutable artifacts published less than `quarantine_days` (default `3`) ago. See [Quarantine](#quarantine). | | `mirrorlist` / `mirror_strategy` | rpm/deb/alpine only. Upstream pool is `[base_url] + mirrorlist`; `round_robin` (default) rotates the starting mirror, `least_conn` starts at the one with the fewest in-flight requests. Network errors and 5xx fail over to the next mirror; other statuses return as-is. | ## Precedence @@ -52,6 +52,22 @@ So `blocklist` beats the allowlist, and `immutable_patterns` beats Concurrent misses on one path share a fetch lock. Waiters poll the store for up to 5s instead of all going upstream. +## Quarantine + +With `quarantine_enabled`, an uncached **immutable** path whose upstream copy +was published less than `quarantine_days` ago answers `403` and is not cached: + +```json +{"error": "quarantined: published 2026-10-24T09:00:00Z, available from 2026-10-27T09:00:00Z", + "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. +- 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. +- Indexes still list fresh versions, so a client resolving "latest" may pick one and get the `403`. + ## Eviction ```sh diff --git a/internal/api/v1/proxy.go b/internal/api/v1/proxy.go index 9c6ad33..f7b413f 100644 --- a/internal/api/v1/proxy.go +++ b/internal/api/v1/proxy.go @@ -2,6 +2,7 @@ package v1 import ( "bytes" + "encoding/json" "errors" "fmt" "io" @@ -11,6 +12,7 @@ import ( "net/url" "strconv" "strings" + "time" "github.com/go-chi/chi/v5" @@ -121,7 +123,7 @@ func (h *ProxyHandler) serveFetch(w http.ResponseWriter, r *http.Request, remote if err != nil { var proxyErr *proxy.ProxyError if errors.As(err, &proxyErr) { - http.Error(w, proxyErr.Message, proxyErr.Status) + writeProxyError(w, proxyErr) return } slog.Error("proxy fetch failed", "remote", remote.Name, "path", path, "error", err) @@ -149,6 +151,21 @@ func (h *ProxyHandler) serveFetch(w http.ResponseWriter, r *http.Request, remote io.Copy(w, body) } +// writeProxyError answers a quarantine refusal as JSON and anything else as text. +func writeProxyError(w http.ResponseWriter, pe *proxy.ProxyError) { + if pe.Published.IsZero() { + http.Error(w, pe.Message, pe.Status) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(pe.Status) + _ = json.NewEncoder(w).Encode(map[string]string{ + "error": pe.Message, + "published": pe.Published.Format(time.RFC3339), + "available": pe.Available.Format(time.RFC3339), + }) +} + func rewritesMutable(prov provider.Provider, path string) bool { _, sr := prov.(provider.ServeRewriter) _, rr := prov.(provider.ResponseRewriter) @@ -176,7 +193,7 @@ func (h *ProxyHandler) handleProxyHead(w http.ResponseWriter, r *http.Request) { if err != nil { var proxyErr *proxy.ProxyError if errors.As(err, &proxyErr) { - http.Error(w, proxyErr.Message, proxyErr.Status) + writeProxyError(w, proxyErr) return } slog.Error("proxy head failed", "remote", remote.Name, "path", path, "error", err) diff --git a/internal/api/v2/remotes.go b/internal/api/v2/remotes.go index b5c3a82..e2d8f92 100644 --- a/internal/api/v2/remotes.go +++ b/internal/api/v2/remotes.go @@ -71,8 +71,11 @@ 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} + remote := models.Remote{MutableTTL: defaultMutableTTL, QuarantineDays: defaultQuarantineDays} if err := json.NewDecoder(r.Body).Decode(&remote); err != nil { http.Error(w, "invalid json", http.StatusBadRequest) return @@ -126,6 +129,7 @@ func (h *RemotesHandler) update(w http.ResponseWriter, r *http.Request) { var remote models.Remote if oldKnown { remote.MutableTTL = existing.MutableTTL + remote.QuarantineDays = existing.QuarantineDays } if err := json.NewDecoder(r.Body).Decode(&remote); err != nil { http.Error(w, "invalid json", http.StatusBadRequest) diff --git a/internal/api/v2/remotes_ttl_test.go b/internal/api/v2/remotes_ttl_test.go index 7beb859..d6a8058 100644 --- a/internal/api/v2/remotes_ttl_test.go +++ b/internal/api/v2/remotes_ttl_test.go @@ -55,3 +55,39 @@ func TestMutableTTLOmittedVsZero(t *testing.T) { t.Errorf("explicit 0 stored as %d", got) } } + +func TestQuarantineDaysDefault(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() + days := func() int { + t.Helper() + r, err := db.GetRemote(context.Background(), "q-days") + if err != nil { + t.Fatal(err) + } + return r.QuarantineDays + } + const body = `"package_type":"npm","base_url":"https://example.com","quarantine_enabled":true` + for i, s := range []struct { + method, path, json string + want int + }{ + {"POST", "/", `{"name":"q-days",` + body + `}`, 3}, + {"PUT", "/q-days", `{` + body + `,"quarantine_days":14}`, 14}, + {"PUT", "/q-days", `{` + body + `}`, 14}, + } { + if c := do(t, h, s.method, s.path, s.json); c/100 != 2 { + t.Fatalf("step %d = %d", i, c) + } + if got := days(); got != s.want { + t.Errorf("step %d: quarantine_days = %d, want %d", i, got, s.want) + } + } +} diff --git a/internal/cache/redis.go b/internal/cache/redis.go index c120745..844d939 100644 --- a/internal/cache/redis.go +++ b/internal/cache/redis.go @@ -84,6 +84,20 @@ func (r *Redis) GetETag(ctx context.Context, remote, path string) (string, error return val, err } +// FirstSeen records now as the first sighting of path unless one is already +// stored, and returns the stored sighting. +func (r *Redis) FirstSeen(ctx context.Context, remote, path string, ttl time.Duration) (time.Time, error) { + key := fmt.Sprintf("firstseen:%s:%s", remote, path) + if err := r.client.SetNX(ctx, key, time.Now().Unix(), ttl).Err(); err != nil { + return time.Time{}, err + } + sec, err := r.client.Get(ctx, key).Int64() + if err != nil { + return time.Time{}, err + } + return time.Unix(sec, 0), nil +} + func (r *Redis) GetToken(ctx context.Context, key string) (string, error) { val, err := r.client.Get(ctx, "token:"+key).Result() if err == redis.Nil { diff --git a/internal/proxy/engine.go b/internal/proxy/engine.go index 779bec9..9cab5a0 100644 --- a/internal/proxy/engine.go +++ b/internal/proxy/engine.go @@ -325,12 +325,12 @@ func (e *Engine) Head(ctx context.Context, remote models.Remote, path string, pr return &HeadResult{ContentType: info.ContentType, Size: info.Size, Source: "cache"}, nil } - return e.headUpstream(ctx, remote, path, prov) + return e.headUpstream(ctx, remote, path, prov, class) } // headUpstream issues an upstream HEAD, load-balancing across the remote's base // URLs and failing over to the next mirror on a network error or 5xx. -func (e *Engine) headUpstream(ctx context.Context, remote models.Remote, path string, prov provider.Provider) (*HeadResult, error) { +func (e *Engine) headUpstream(ctx context.Context, remote models.Remote, path string, prov provider.Provider, class Classification) (*HeadResult, error) { order := e.baseURLAttemptOrder(remote) if len(order) == 0 { return nil, &ProxyError{Status: http.StatusBadGateway, Message: "no upstream base_url configured"} @@ -338,7 +338,7 @@ func (e *Engine) headUpstream(ctx context.Context, remote models.Remote, path st var lastErr error for i, url := range order { ctr := e.beginAttempt(remote, url) - result, err := e.headUpstreamOnce(ctx, withBaseURL(remote, url), path, prov) + result, err := e.headUpstreamOnce(ctx, withBaseURL(remote, url), path, prov, class) endAttempt(ctr) if err == nil { return result, nil @@ -353,7 +353,7 @@ func (e *Engine) headUpstream(ctx context.Context, remote models.Remote, path st return nil, lastErr } -func (e *Engine) headUpstreamOnce(ctx context.Context, remote models.Remote, path string, prov provider.Provider) (*HeadResult, error) { +func (e *Engine) headUpstreamOnce(ctx context.Context, remote models.Remote, path string, prov provider.Provider, class Classification) (*HeadResult, error) { url := prov.UpstreamURL(remote, path) authHeaders, err := prov.AuthHeaders(ctx, remote) @@ -400,6 +400,11 @@ func (e *Engine) headUpstreamOnce(ctx context.Context, remote models.Remote, pat if resp.StatusCode != http.StatusOK { return nil, &ProxyError{Status: resp.StatusCode, Message: fmt.Sprintf("upstream returned %d", resp.StatusCode)} } + if class == ClassImmutable { + if err := e.checkQuarantine(ctx, remote, path, resp.Header); err != nil { + return nil, err + } + } contentType := prov.ContentType(path) if ct := resp.Header.Get("Content-Type"); ct != "" { @@ -527,6 +532,11 @@ func (e *Engine) fetchFromUpstreamOnce(ctx context.Context, remote models.Remote }, nil } + if err := e.checkQuarantine(ctx, remote, path, resp.Header); err != nil { + _ = resp.Body.Close() + return nil, err + } + // Immutable blobs are streamed through the content-addressable store // (tempfile -> sha256 -> S3) so arbitrarily large artifacts never sit // fully in memory. Immutable content is never rewritten in the proxy path. @@ -663,6 +673,35 @@ func (e *Engine) checkUpstreamOnce(ctx context.Context, remote models.Remote, pa return resp.StatusCode == http.StatusNotModified, nil } +// 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. +func (e *Engine) checkQuarantine(ctx context.Context, remote models.Remote, path string, upstream http.Header) error { + if !remote.QuarantineEnabled || remote.QuarantineDays <= 0 { + 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 + 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() + } + } + if until := published.Add(window); time.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)), + Published: published.UTC(), + Available: until.UTC(), + } + } + return nil +} + // etagTTL keeps a mutable path's ETag well past its freshness key so an expired // path revalidates with a conditional HEAD, while idle paths still age out. func etagTTL(ttl time.Duration) time.Duration { @@ -821,10 +860,19 @@ func fetchBearerToken(ctx context.Context, wwwAuth string, remote models.Remote) type ProxyError struct { Status int Message string + // Published and Available are set only on a quarantine refusal. + Published time.Time + Available time.Time } func (e *ProxyError) Error() string { return e.Message } +// Quarantined reports whether err is a quarantine refusal. +func Quarantined(err error) bool { + var pe *ProxyError + return errors.As(err, &pe) && !pe.Published.IsZero() +} + type UpstreamError struct { Err error } diff --git a/internal/proxy/engine_test.go b/internal/proxy/engine_test.go index 447de34..25371a0 100644 --- a/internal/proxy/engine_test.go +++ b/internal/proxy/engine_test.go @@ -227,7 +227,7 @@ func TestHeadUpstreamUsesRemoteTimeout(t *testing.T) { r := models.Remote{Name: "slow-head", PackageType: models.PackageGeneric, BaseURL: slow.URL, UpstreamResponseHeaderTimeout: 1} start := time.Now() - _, err := (&Engine{}).headUpstreamOnce(context.Background(), r, "blob.bin", prov(t, models.PackageGeneric)) + _, err := (&Engine{}).headUpstreamOnce(context.Background(), r, "blob.bin", prov(t, models.PackageGeneric), ClassImmutable) if err == nil || time.Since(start) > 2*time.Second { t.Fatalf("HEAD ignored the remote's response header timeout: err=%v after %s", err, time.Since(start)) } diff --git a/internal/proxy/quarantine_test.go b/internal/proxy/quarantine_test.go new file mode 100644 index 0000000..1b91220 --- /dev/null +++ b/internal/proxy/quarantine_test.go @@ -0,0 +1,138 @@ +package proxy + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" + + "git.unkin.net/unkin/artifactapi/pkg/models" + goredis "github.com/redis/go-redis/v9" +) + +func must[T any](v T, err error) T { + if err != nil { + panic(err) + } + return v +} + +func quarantineUpstream(t *testing.T) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/fresh.tgz": + 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 "/undated.tgz", "/pkg": + default: + http.NotFound(w, r) + return + } + _, _ = w.Write([]byte("body " + r.URL.Path)) + })) + t.Cleanup(srv.Close) + return srv +} + +func quarantineRemote(t *testing.T, name string, enabled bool) models.Remote { + r := models.Remote{Name: name, PackageType: models.PackageNPM, RepoType: models.RepoTypeRemote, + BaseURL: quarantineUpstream(t).URL, QuarantineEnabled: enabled, QuarantineDays: 3} + return seed(t, r) +} + +func wantQuarantined(t *testing.T, err error) *ProxyError { + t.Helper() + var pe *ProxyError + if !errors.As(err, &pe) || pe.Status != http.StatusForbidden || !Quarantined(err) { + t.Fatalf("want quarantine 403, got %v", err) + } + return pe +} + +func TestQuarantineBlocksFreshArtifact(t *testing.T) { + requireStack(t) + ctx := context.Background() + r := quarantineRemote(t, "q-fresh", true) + pe := wantQuarantined(t, func() error { _, err := testEngine.Fetch(ctx, r, "fresh.tgz", prov(t, models.PackageNPM)); return err }()) + if d := time.Until(pe.Available); d < 70*time.Hour || d > 72*time.Hour { + t.Errorf("available in %s, want ~71h", d) + } + if a, _ := testDB.GetArtifact(ctx, r.Name, "fresh.tgz"); a != nil { + t.Error("quarantined artifact was cached") + } + _, err := testEngine.Head(ctx, r, "fresh.tgz", prov(t, models.PackageNPM)) + _ = wantQuarantined(t, err) +} + +func TestQuarantineServesOldArtifact(t *testing.T) { + requireStack(t) + r := quarantineRemote(t, "q-old", true) + res, err := testEngine.Fetch(context.Background(), r, "old.tgz", prov(t, models.PackageNPM)) + if err != nil { + t.Fatal(err) + } + if got := readAll(t, res); got != "body /old.tgz" { + t.Errorf("body = %q", got) + } +} + +func TestQuarantineFallsBackToFirstSeen(t *testing.T) { + requireStack(t) + ctx := context.Background() + r := quarantineRemote(t, "q-undated", true) + p := prov(t, models.PackageNPM) + _, err := testEngine.Fetch(ctx, r, "undated.tgz", p) + first := wantQuarantined(t, err).Published + if time.Since(first) > time.Minute { + t.Fatalf("first seen %s, want now", first) + } + _, err = testEngine.Fetch(ctx, r, "undated.tgz", p) + if again := wantQuarantined(t, err).Published; !again.Equal(first) { + t.Errorf("first seen moved: %s -> %s", first, again) + } + + r.QuarantineDays = 1 + c := goredis.NewClient(must(goredis.ParseURL(testRedis))) + defer func() { _ = c.Close() }() + if err := c.Set(ctx, "firstseen:q-undated:undated.tgz", time.Now().Add(-48*time.Hour).Unix(), time.Hour).Err(); err != nil { + t.Fatal(err) + } + if _, err := testEngine.Fetch(ctx, r, "undated.tgz", p); err != nil { + t.Errorf("seen 48h ago with 1 day quarantine: %v", err) + } +} + +func TestQuarantineSkipsMutable(t *testing.T) { + requireStack(t) + r := quarantineRemote(t, "q-mutable", true) + if _, err := testEngine.Fetch(context.Background(), r, "pkg", prov(t, models.PackageNPM)); err != nil { + t.Errorf("mutable index blocked: %v", err) + } +} + +func TestQuarantineDisabled(t *testing.T) { + requireStack(t) + r := quarantineRemote(t, "q-off", false) + if _, err := testEngine.Fetch(context.Background(), r, "fresh.tgz", prov(t, models.PackageNPM)); err != nil { + t.Errorf("disabled quarantine blocked: %v", err) + } +} + +func TestQuarantineKeepsServingCached(t *testing.T) { + requireStack(t) + ctx := context.Background() + r := quarantineRemote(t, "q-cached", false) + p := prov(t, models.PackageNPM) + if _, err := testEngine.Fetch(ctx, r, "fresh.tgz", p); err != nil { + t.Fatal(err) + } + r.QuarantineEnabled = true + expire(t, r.Name, "fresh.tgz") + if _, err := testEngine.Fetch(ctx, r, "fresh.tgz", p); err != nil { + t.Errorf("artifact cached before quarantine refused on refetch: %v", err) + } +} diff --git a/internal/virtual/goproxy_merger.go b/internal/virtual/goproxy_merger.go index f502631..634cfcc 100644 --- a/internal/virtual/goproxy_merger.go +++ b/internal/virtual/goproxy_merger.go @@ -194,10 +194,18 @@ func (e *Engine) goList(ctx context.Context, virt models.Virtual, escMod string) return nil, ErrNotFound } -// goProbe returns the first member, in member order, that serves path. +// goProbe returns the first member, in member order, that serves path, else +// the first member quarantining it so the client sees that refusal. func (e *Engine) goProbe(ctx context.Context, virt models.Virtual, path string) (string, error) { + held := "" for _, name := range virt.Members { m, err := e.goMember(ctx, name, path) + if proxy.Quarantined(err) { + if held == "" { + held = name + } + continue + } if err != nil { return "", fmt.Errorf("virtual %q: member %q: %v", virt.Name, name, err) } @@ -205,6 +213,9 @@ func (e *Engine) goProbe(ctx context.Context, virt models.Virtual, path string) return name, nil } } + if held != "" { + return held, nil + } return "", ErrNotFound } diff --git a/internal/virtual/goproxy_merger_test.go b/internal/virtual/goproxy_merger_test.go index 0d99d19..088f9d7 100644 --- a/internal/virtual/goproxy_merger_test.go +++ b/internal/virtual/goproxy_merger_test.go @@ -3,11 +3,15 @@ package virtual import ( "context" "errors" + "fmt" + "net/http" + "strings" "sync" "testing" "time" "git.unkin.net/unkin/artifactapi/internal/database" + "git.unkin.net/unkin/artifactapi/internal/proxy" "git.unkin.net/unkin/artifactapi/internal/testsupport" "git.unkin.net/unkin/artifactapi/pkg/models" ) @@ -181,3 +185,32 @@ func TestGoProxyLocalMemberNotFound(t *testing.T) { t.Fatalf("module only absent from a local member must be not-found, got %v", err) } } + +func TestGoProbeFallsThroughQuarantine(t *testing.T) { + quarantined := &proxy.ProxyError{Status: http.StatusForbidden, Message: "quarantined", Published: time.Now()} + served := map[string]bool{} + e := &Engine{ + mergeTTL: time.Minute, + getRemote: func(_ context.Context, name string) (*models.Remote, error) { + return &models.Remote{Name: name, RepoType: models.RepoTypeRemote}, nil + }, + goMember: func(_ context.Context, name, path string) (*GoMember, error) { + if name == "internal" && strings.HasSuffix(path, ".info") { + return nil, fmt.Errorf("fetch: %w", quarantined) + } + if served[name] { + return &GoMember{RemoteName: name, Body: []byte("{}")}, nil + } + return &GoMember{RemoteName: name}, nil + }, + } + zip := "example.com/mod/@v/v0.0.0-20260101-abcdef.zip" + served["goproxy"] = true + if _, loc, err := e.GoProxy(context.Background(), goVirt(), zip, ""); err != nil || loc != "/api/v1/remote/goproxy/"+zip { + t.Errorf("next member serving: %q %v", loc, err) + } + served["goproxy"] = false + if _, loc, err := e.GoProxy(context.Background(), goVirt(), zip, ""); err != nil || loc != "/api/v1/remote/internal/"+zip { + t.Errorf("only quarantined member: %q %v, want redirect to its refusal", loc, err) + } +} -- 2.47.3 From 7cd0edf7ffa40f38ba98d27b58dcb011dbcea53a Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Sun, 25 Oct 2026 13:36:00 +1100 Subject: [PATCH 2/2] 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. -- 2.47.3