From f8e8e66f4d86b66af1c9720c60a6566ec2d66921 Mon Sep 17 00:00:00 2001 From: bussyjd Date: Thu, 16 Jul 2026 20:54:24 +0400 Subject: [PATCH] fix(discovery): harden upstream-OpenAPI discovery (size, cache, SSRF, pricing, ERC-8004) #777's upstream-OpenAPI discovery feature had six issues: - getJSONMap accepted up to 8MiB and re-marshaled it into the shared "obol-skill-md" ConfigMap; anything over k8s's ~1MiB ConfigMap limit bricked static-site publishing for every offer. Cap fetch + re-marshal at 200KiB and fall back to buildOfferScopedOpenAPI when exceeded. - tryUpstreamOpenAPI ran inline (up to 3x3s GETs, uncached) inside buildOfferBundles, itself called under staticSiteMu on every offer's reconcile. A slow/flapping upstream serialized reconciles and flip-flopped the content hash, rolling the shared discovery pod on every flap. Added upstreamOpenAPICache, keyed by offer UID+generation: refresh() runs from each offer's own reconcile outside staticSiteMu, at most once per generation; buildOfferBundles now takes a lookup func and only ever reads the cache. - fetchUpstreamOpenAPI followed redirects and let Upstream.Namespace (offer-author-controlled) pick an arbitrary namespace, with none of the ReferenceGrant checks the Gateway API data path requires for cross-namespace targets. Disabled redirect following, pinned the probe to the offer's own namespace, and rejected non-simple openapiPath overrides (traversal / embedded scheme). - buildOfferWellKnownX402FromOpenAPI priced every paid op at the offer's default, ignoring spec.routes[] price overrides the non-upstream path already honors. Added routePriceOverride, matching the verifier's own pattern semantics. - buildOfferAgentRegistration never set registrations[], which ERC-8004 requires once an offer is on-chain; buyers using --expected-agent-id failed closed. Populated from status.AgentID + the resolved payment network's registry. - Services[].Endpoint origin scoping used HasPrefix(ep, origin), which admits an evil-suffix host like origin+".evil.tld". Fixed to exact match or origin+"/". - Deleted an unreachable duplicate security-empty check in openAPIOpIsPaid. Every fix has a test that fails without it (verified by reverting each change in isolation and re-running its test). Claude-Session: https://claude.ai/code/session_014YjPMViNrZ7zBVgUQzwEKk --- internal/serviceoffercontroller/controller.go | 16 +- .../serviceoffercontroller/hostoffer_test.go | 29 +- .../serviceoffercontroller/offerbundle.go | 13 +- .../upstream_openapi.go | 224 +++++++++++-- .../upstream_openapi_test.go | 302 ++++++++++++++++++ 5 files changed, 545 insertions(+), 39 deletions(-) create mode 100644 internal/serviceoffercontroller/upstream_openapi_test.go diff --git a/internal/serviceoffercontroller/controller.go b/internal/serviceoffercontroller/controller.go index 4d836045..9002ad04 100644 --- a/internal/serviceoffercontroller/controller.go +++ b/internal/serviceoffercontroller/controller.go @@ -78,6 +78,12 @@ type Controller struct { agentQueue workqueue.TypedRateLimitingInterface[string] staticSiteMu sync.Mutex + // upstreamOpenAPICache is populated from each offer's own reconcile + // (refresh, outside staticSiteMu) and only ever read by + // reconcileStaticSite's buildOfferBundles call (under staticSiteMu) — + // see upstream_openapi.go. + upstreamOpenAPICache upstreamOpenAPICache + pendingAuths sync.Map // key: "ns/name" → []map[string]string httpClient *http.Client @@ -470,6 +476,7 @@ func (c *Controller) reconcileOffer(ctx context.Context, key string) error { if err := c.reconcileStaticSite(ctx, &tombstone); err != nil { return err } + c.upstreamOpenAPICache.forget(offer.UID) return c.removeFinalizer(ctx, raw, serviceOfferFinalizer) } @@ -661,6 +668,13 @@ func (c *Controller) reconcileOffer(ctx context.Context, key string) error { // requiring a spec mutation or unrelated ConfigMap update. c.offerQueue.AddAfter(offer.Namespace+"/"+offer.Name, 5*time.Second) } + // Refresh this offer's upstream OpenAPI cache from its own reconcile, + // outside staticSiteMu, at most once per generation — see + // upstreamOpenAPICache and buildOfferBundles for why the rebuild below + // must never fetch live. + if offer.Spec.Hostname != "" { + c.upstreamOpenAPICache.refresh(offer, tryUpstreamOpenAPI) + } // Rebuild the static site on every reconcile so tunnel URL changes and // offer status updates propagate immediately. reconcileStaticSite skips // ConfigMap/Deployment writes when the rendered hash is unchanged. @@ -1339,7 +1353,7 @@ func (c *Controller) reconcileStaticSite(ctx context.Context, override *monetize // when tunnelURL changes — see enqueueDiscoveryRefresh). openAPIJSON := buildOpenAPIDocument(offers, baseURL, resolvedProfile) apiDocsHTML := scalarHTML(resolvedProfile) - bundles := buildOfferBundles(offers, resolvedProfile) + bundles := buildOfferBundles(offers, resolvedProfile, c.upstreamOpenAPICache.get) contentHash := computeStaticSiteContentHash(content, servicesJSON, openAPIJSON, apiDocsHTML, bundles) unchanged, err := c.staticSiteContentUnchanged(ctx, content, servicesJSON, openAPIJSON, apiDocsHTML, bundles) diff --git a/internal/serviceoffercontroller/hostoffer_test.go b/internal/serviceoffercontroller/hostoffer_test.go index d3164d1c..249f2faa 100644 --- a/internal/serviceoffercontroller/hostoffer_test.go +++ b/internal/serviceoffercontroller/hostoffer_test.go @@ -18,6 +18,10 @@ func hostnameOffer() *monetizeapi.ServiceOffer { return offer } +// noUpstreamOpenAPI is the buildOfferBundles cache-lookup stub for tests +// that don't exercise the upstream-OpenAPI path. +func noUpstreamOpenAPI(*monetizeapi.ServiceOffer) map[string]any { return nil } + // TestBuildHostHTTPRoute pins the dedicated-origin route topology: Exact // discovery rules rewriting into the offer's bundle files on the catalog // httpd, and a PathPrefix / rule rewriting the public path-world into @@ -105,15 +109,12 @@ func TestBuildHostHTTPRoute(t *testing.T) { func TestBuildOfferBundles(t *testing.T) { profile := schemas.StorefrontProfile{DisplayName: "Acme", ContactEmail: "ops@acme.example"} offer := hostnameOffer() - prev := tryUpstreamOpenAPI - tryUpstreamOpenAPI = func(*monetizeapi.ServiceOffer) map[string]any { return nil } - defer func() { tryUpstreamOpenAPI = prev }() - if got := buildOfferBundles([]*monetizeapi.ServiceOffer{routeTableOffer()}, profile); len(got) != 0 { + if got := buildOfferBundles([]*monetizeapi.ServiceOffer{routeTableOffer()}, profile, noUpstreamOpenAPI); len(got) != 0 { t.Fatalf("path-only offer produced bundles: %v", got) } - bundles := buildOfferBundles([]*monetizeapi.ServiceOffer{offer}, profile) + bundles := buildOfferBundles([]*monetizeapi.ServiceOffer{offer}, profile, noUpstreamOpenAPI) if len(bundles) != 4 { t.Fatalf("len(bundles) = %d, want 4", len(bundles)) } @@ -186,9 +187,6 @@ func TestBuildOfferBundles(t *testing.T) { // spec.branding fields override the storefront profile on the dedicated // origin's surfaces, empty fields inherit. func TestBuildOfferBundles_BrandingOverride(t *testing.T) { - prev := tryUpstreamOpenAPI - tryUpstreamOpenAPI = func(*monetizeapi.ServiceOffer) map[string]any { return nil } - defer func() { tryUpstreamOpenAPI = prev }() profile := storefront.ResolvePublished(&schemas.StorefrontProfile{ DisplayName: "Acme", ContactEmail: "ops@acme.example", @@ -202,7 +200,7 @@ func TestBuildOfferBundles_BrandingOverride(t *testing.T) { Description: "**Deep** audits by AuditCo.", } - bundles := buildOfferBundles([]*monetizeapi.ServiceOffer{offer}, profile) + bundles := buildOfferBundles([]*monetizeapi.ServiceOffer{offer}, profile, noUpstreamOpenAPI) byPath := map[string]string{} for _, f := range bundles { byPath[f.Path] = f.Content @@ -371,10 +369,7 @@ func TestStaticSiteServesChatWidget(t *testing.T) { // Per-offer page: agent offers gain a chat.html bundle file carrying // the landing page's theme tokens and title; non-agent offers do not. profile := schemas.StorefrontProfile{DisplayName: "Acme"} - prev := tryUpstreamOpenAPI - tryUpstreamOpenAPI = func(*monetizeapi.ServiceOffer) map[string]any { return nil } - defer func() { tryUpstreamOpenAPI = prev }() - plain := buildOfferBundles([]*monetizeapi.ServiceOffer{hostnameOffer()}, profile) + plain := buildOfferBundles([]*monetizeapi.ServiceOffer{hostnameOffer()}, profile, noUpstreamOpenAPI) for _, f := range plain { if strings.HasSuffix(f.Path, "chat.html") { t.Fatalf("non-agent offer rendered a chat page: %s", f.Path) @@ -382,7 +377,7 @@ func TestStaticSiteServesChatWidget(t *testing.T) { } agent := hostnameOffer() agent.Spec.Type = "agent" - bundles := buildOfferBundles([]*monetizeapi.ServiceOffer{agent}, profile) + bundles := buildOfferBundles([]*monetizeapi.ServiceOffer{agent}, profile, noUpstreamOpenAPI) var chat string for _, f := range bundles { if f.Path == "offers/sec/audit/chat.html" { @@ -427,8 +422,7 @@ func TestBuildOfferBundles_UpstreamOpenAPI(t *testing.T) { offer := hostnameOffer() offer.Spec.Registration.Name = "Hyperliquid Trading Intelligence" offer.Spec.Registration.Description = "Full first-party catalog." - prev := tryUpstreamOpenAPI - tryUpstreamOpenAPI = func(*monetizeapi.ServiceOffer) map[string]any { + upstream := func(*monetizeapi.ServiceOffer) map[string]any { return map[string]any{ "openapi": "3.1.0", "info": map[string]any{"title": "upstream-title", "version": "1.1.0"}, @@ -446,8 +440,7 @@ func TestBuildOfferBundles_UpstreamOpenAPI(t *testing.T) { }, } } - defer func() { tryUpstreamOpenAPI = prev }() - bundles := buildOfferBundles([]*monetizeapi.ServiceOffer{offer}, profile) + bundles := buildOfferBundles([]*monetizeapi.ServiceOffer{offer}, profile, upstream) byPath := map[string]string{} for _, f := range bundles { byPath[f.Path] = f.Content diff --git a/internal/serviceoffercontroller/offerbundle.go b/internal/serviceoffercontroller/offerbundle.go index 4449d495..3d60127a 100644 --- a/internal/serviceoffercontroller/offerbundle.go +++ b/internal/serviceoffercontroller/offerbundle.go @@ -43,7 +43,16 @@ func offerBundleKey(offer *monetizeapi.ServiceOffer, file string) string { // buildOfferBundles renders the discovery bundle for every hostname-bound, // operationally-included offer. Deterministic order (bundle keys sorted) so // content hashing is stable. -func buildOfferBundles(offers []*monetizeapi.ServiceOffer, profile schemas.StorefrontProfile) []offerBundleFile { +// +// upstreamOpenAPI supplies each offer's (possibly nil) upstream OpenAPI +// document. It must be a cache read, never a live fetch: this function runs +// under staticSiteMu on every offer's reconcile (see reconcileStaticSite), +// so a live HTTP call here would serialize every reconcile behind the +// slowest upstream and — on a flapping upstream — flip-flop the rebuilt +// content hash and roll the shared discovery pod. The production caller +// passes the Controller's upstreamOpenAPICache.get, refreshed independently +// from each offer's own reconcile. +func buildOfferBundles(offers []*monetizeapi.ServiceOffer, profile schemas.StorefrontProfile, upstreamOpenAPI func(*monetizeapi.ServiceOffer) map[string]any) []offerBundleFile { var bundles []offerBundleFile for _, offer := range offers { if offer == nil || offer.Spec.Hostname == "" { @@ -53,7 +62,7 @@ func buildOfferBundles(offers []*monetizeapi.ServiceOffer, profile schemas.Store // branding block overrides the storefront profile field-wise // (empty fields inherit). originProfile := storefront.MergeProfile(profile, offer.Spec.Branding.ProfilePatch()) - upstreamDoc := tryUpstreamOpenAPI(offer) + upstreamDoc := upstreamOpenAPI(offer) openapiContent := buildOfferScopedOpenAPI(offer, originProfile) x402Content := buildOfferWellKnownX402(offer) if upstreamDoc != nil { diff --git a/internal/serviceoffercontroller/upstream_openapi.go b/internal/serviceoffercontroller/upstream_openapi.go index dacd73ec..e6266432 100644 --- a/internal/serviceoffercontroller/upstream_openapi.go +++ b/internal/serviceoffercontroller/upstream_openapi.go @@ -5,18 +5,41 @@ import ( "fmt" "io" "net/http" + "path" "sort" "strings" + "sync" "time" "github.com/ObolNetwork/obol-stack/internal/erc8004" "github.com/ObolNetwork/obol-stack/internal/monetizeapi" "github.com/ObolNetwork/obol-stack/internal/schemas" + "k8s.io/apimachinery/pkg/types" ) -var upstreamOpenAPIClient = &http.Client{Timeout: 3 * time.Second} +// maxUpstreamOpenAPIBytes caps both the fetched body and the re-marshaled +// document. The rewritten doc lands in the shared "obol-skill-md" ConfigMap +// alongside every other offer's bundle, which is subject to Kubernetes' +// ~1MiB ConfigMap size limit; a single oversized upstream doc would brick +// static-site publishing for every offer, not just its own. 200KiB leaves +// ample headroom for many offers to coexist. +const maxUpstreamOpenAPIBytes = 200 * 1024 -// tryUpstreamOpenAPI is the seam tests replace. +var upstreamOpenAPIClient = &http.Client{ + Timeout: 3 * time.Second, + // Never follow redirects: the target is offer-author-controlled + // (service/namespace/port/path) and the fetched document is republished + // publicly, so a redirect must not be able to silently retarget the + // fetch. + CheckRedirect: func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + }, +} + +// tryUpstreamOpenAPI is the seam tests replace. It performs the actual +// network fetch; callers that need determinism across a slow/flapping +// upstream should go through upstreamOpenAPICache instead of calling this +// directly. var tryUpstreamOpenAPI = fetchUpstreamOpenAPI func fetchUpstreamOpenAPI(offer *monetizeapi.ServiceOffer) map[string]any { @@ -26,11 +49,7 @@ func fetchUpstreamOpenAPI(offer *monetizeapi.ServiceOffer) map[string]any { if strings.TrimSpace(offer.Spec.Upstream.Service) == "" { return nil } - base := fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", - offer.Spec.Upstream.Service, - offer.EffectiveNamespace(), - offer.EffectivePort(), - ) + base := upstreamOpenAPIBase(offer) for _, path := range upstreamOpenAPIPathCandidates(offer) { doc, err := getJSONMap(base + path) if err != nil || doc == nil { @@ -45,13 +64,24 @@ func fetchUpstreamOpenAPI(offer *monetizeapi.ServiceOffer) map[string]any { return nil } +// upstreamOpenAPIBase builds the fetch target for one offer's own upstream +// service. Deliberately offer.Namespace, not EffectiveNamespace(): +// Upstream.Namespace is offer-author-controlled, and the Gateway API data +// path only trusts a cross-namespace target once a ReferenceGrant +// authorizes it. This controller-side probe has no such check, so it must +// never leave the offer's own namespace. +func upstreamOpenAPIBase(offer *monetizeapi.ServiceOffer) string { + return fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", + offer.Spec.Upstream.Service, + offer.Namespace, + offer.EffectivePort(), + ) +} + func upstreamOpenAPIPathCandidates(offer *monetizeapi.ServiceOffer) []string { var out []string if offer.Spec.Registration.Metadata != nil { - if p := strings.TrimSpace(offer.Spec.Registration.Metadata["openapiPath"]); p != "" { - if !strings.HasPrefix(p, "/") { - p = "/" + p - } + if p := strings.TrimSpace(offer.Spec.Registration.Metadata["openapiPath"]); p != "" && isSimpleUpstreamPath(p) { out = append(out, p) } } @@ -59,6 +89,21 @@ func upstreamOpenAPIPathCandidates(offer *monetizeapi.ServiceOffer) []string { return out } +// isSimpleUpstreamPath rejects anything but a plain, '/'-prefixed path: no +// traversal segments and no embedded scheme/authority that could redirect +// the request off the intended upstream once concatenated onto base. +func isSimpleUpstreamPath(p string) bool { + if !strings.HasPrefix(p, "/") || strings.Contains(p, "://") { + return false + } + for _, seg := range strings.Split(p, "/") { + if seg == ".." { + return false + } + } + return true +} + func getJSONMap(url string) (map[string]any, error) { resp, err := upstreamOpenAPIClient.Get(url) if err != nil { @@ -68,10 +113,13 @@ func getJSONMap(url string) (map[string]any, error) { if resp.StatusCode < 200 || resp.StatusCode >= 300 { return nil, fmt.Errorf("HTTP %d", resp.StatusCode) } - body, err := io.ReadAll(io.LimitReader(resp.Body, 8<<20)) + body, err := io.ReadAll(io.LimitReader(resp.Body, maxUpstreamOpenAPIBytes+1)) if err != nil { return nil, err } + if len(body) > maxUpstreamOpenAPIBytes { + return nil, fmt.Errorf("upstream openapi document exceeds %d bytes", maxUpstreamOpenAPIBytes) + } var doc map[string]any if err := json.Unmarshal(body, &doc); err != nil { return nil, err @@ -79,6 +127,64 @@ func getJSONMap(url string) (map[string]any, error) { return doc, nil } +// upstreamOpenAPICacheEntry is one offer's last-fetched result. +type upstreamOpenAPICacheEntry struct { + generation int64 + doc map[string]any +} + +// upstreamOpenAPICache holds the last-good upstream fetch per offer, keyed +// by UID. refresh is called from an offer's own reconcile (outside +// staticSiteMu) at most once per observed generation; get is read-only and +// is all buildOfferBundles ever calls. This keeps a slow or flapping +// upstream from blocking, or flip-flopping the content hash of, the shared +// static-site rebuild that runs for every offer on every offer's reconcile. +type upstreamOpenAPICache struct { + mu sync.Mutex + entries map[types.UID]upstreamOpenAPICacheEntry +} + +// get returns the cached doc, or nil if no fetch has completed yet for this +// offer's current generation. +func (c *upstreamOpenAPICache) get(offer *monetizeapi.ServiceOffer) map[string]any { + if offer == nil { + return nil + } + c.mu.Lock() + defer c.mu.Unlock() + return c.entries[offer.UID].doc +} + +// refresh fetches (via fetch) and caches the result, but only when the +// offer's generation has moved on from what's cached — a requeue with no +// spec change (e.g. the 5s convergence retry, a tunnel URL change) reuses +// the last-good result instead of hitting the upstream again. +func (c *upstreamOpenAPICache) refresh(offer *monetizeapi.ServiceOffer, fetch func(*monetizeapi.ServiceOffer) map[string]any) { + if offer == nil { + return + } + c.mu.Lock() + cached, ok := c.entries[offer.UID] + c.mu.Unlock() + if ok && cached.generation == offer.Generation { + return + } + doc := fetch(offer) + c.mu.Lock() + if c.entries == nil { + c.entries = map[types.UID]upstreamOpenAPICacheEntry{} + } + c.entries[offer.UID] = upstreamOpenAPICacheEntry{generation: offer.Generation, doc: doc} + c.mu.Unlock() +} + +// forget drops a cache entry (offer deleted). +func (c *upstreamOpenAPICache) forget(uid types.UID) { + c.mu.Lock() + delete(c.entries, uid) + c.mu.Unlock() +} + func rewriteUpstreamOpenAPI(doc map[string]any, offer *monetizeapi.ServiceOffer, profile schemas.StorefrontProfile) (string, bool) { if doc == nil { return "", false @@ -116,7 +222,7 @@ func rewriteUpstreamOpenAPI(doc map[string]any, offer *monetizeapi.ServiceOffer, } } encoded, err := json.MarshalIndent(out, "", " ") - if err != nil { + if err != nil || len(encoded) > maxUpstreamOpenAPIBytes { return "", false } return string(encoded), true @@ -161,13 +267,19 @@ func buildOfferWellKnownX402FromOpenAPI(offer *monetizeapi.ServiceOffer, doc map if desc == "" { desc = offerDescription(offer, "x402 payment-gated service.") } + accepts := defaultAccepts + if price, ok := routePriceOverride(offer, m, p); ok { + payment := offer.EffectivePayments()[0] + payment.Price = price + accepts = []any{wellKnownAccept(payment)} + } resources = append(resources, map[string]any{ "resource": origin + joinOpenAPIPath("/", p), "type": "http", "method": strings.ToUpper(m), "description": desc, "x402Version": 2, - "accepts": defaultAccepts, + "accepts": accepts, }) } } @@ -184,6 +296,72 @@ func buildOfferWellKnownX402FromOpenAPI(offer *monetizeapi.ServiceOffer, doc map return string(encoded) } +// routePriceOverride returns the price of the most specific spec.routes[] +// entry that both matches the upstream path p and method, and carries a +// price override, mirroring how buildOfferWellKnownX402 (offerbundle.go) +// honors rt.HasPriceOverride()/rt.Price for the non-upstream document. +// Routes without an override are skipped so lookups fall through to the +// offer's default payments instead of pinning to a route that has nothing +// to override. +func routePriceOverride(offer *monetizeapi.ServiceOffer, method, p string) (monetizeapi.ServiceOfferPriceTable, bool) { + routes := append([]monetizeapi.ServiceOfferRoute(nil), offer.EffectiveRoutes()...) + sortRouteTableBySpecificity(routes) + for _, rt := range routes { + if !rt.HasPriceOverride() || !routeMethodMatches(rt.Methods, method) { + continue + } + if openAPIRoutePatternMatches(rt.Path, p) { + return rt.Price, true + } + } + return monetizeapi.ServiceOfferPriceTable{}, false +} + +// routeMethodMatches reports whether method (any case) is among the route's +// declared methods; an empty list means the route applies to every method. +func routeMethodMatches(methods []string, method string) bool { + if len(methods) == 0 { + return true + } + for _, m := range methods { + if strings.EqualFold(m, method) { + return true + } + } + return false +} + +// openAPIRoutePatternMatches mirrors matchPattern in internal/x402/matcher.go +// (unexported there, so duplicated here): exact match, a trailing "/*" +// greedy prefix, or path.Match segment globs. Both packages interpret +// spec.routes[].path against the same offer-rooted path-world, so this must +// agree with what the verifier actually gates. +func openAPIRoutePatternMatches(pattern, p string) bool { + if !strings.Contains(pattern, "*") { + return pattern == p + } + if prefix, ok := strings.CutSuffix(pattern, "/*"); ok && !strings.Contains(prefix, "*") { + return p == prefix || strings.HasPrefix(p, prefix+"/") + } + patParts := strings.Split(strings.TrimPrefix(pattern, "/"), "/") + pParts := strings.Split(strings.TrimPrefix(p, "/"), "/") + if len(pParts) < len(patParts) { + return false + } + for i, pp := range patParts { + if i == len(patParts)-1 && pp == "*" { + return true + } + if i >= len(pParts) { + return false + } + if matched, err := path.Match(pp, pParts[i]); err != nil || !matched { + return false + } + } + return len(pParts) == len(patParts) +} + func openAPIOpIsPaid(op map[string]any) bool { if op == nil { return false @@ -199,9 +377,6 @@ func openAPIOpIsPaid(op map[string]any) bool { } if responses, ok := op["responses"].(map[string]any); ok { if _, has402 := responses["402"]; has402 { - if sec, ok := op["security"].([]any); ok && len(sec) == 0 { - return false - } return true } } @@ -234,7 +409,7 @@ func buildOfferAgentRegistration(offer *monetizeapi.ServiceOffer, profile schema if ep == "" { continue } - if strings.Contains(ep, "://") && !strings.HasPrefix(ep, origin) { + if strings.Contains(ep, "://") && ep != origin && !strings.HasPrefix(ep, origin+"/") { continue } scoped = append(scoped, erc8004.ServiceDef{ @@ -258,6 +433,19 @@ func buildOfferAgentRegistration(offer *monetizeapi.ServiceOffer, profile schema Active: offer.Spec.Registration.Enabled, X402Support: true, Services: services, SupportedTrust: offer.Spec.Registration.SupportedTrust, } + // ERC-8004 requires registrations[] to have >=1 entry once the offer is + // on-chain (status.AgentID). Buyers verifying --expected-agent-id + // (internal/buy/discover.go) fail closed without it. + if agentID := strings.TrimSpace(offer.Status.AgentID); agentID != "" { + registry := fmt.Sprintf("eip155:%d:%s", erc8004.BaseSepoliaChainID, erc8004.IdentityRegistryBaseSepolia) + if net, err := erc8004.ResolveNetwork(offer.Spec.Payment.Network); err == nil { + registry = net.CAIP10Registry() + } + doc.Registrations = []erc8004.OnChainReg{{ + AgentID: parseInt64(agentID), + AgentRegistry: registry, + }} + } if meta := nonEmptyStringMap(offer.Spec.Registration.Metadata); len(meta) > 0 { doc.Metadata = meta } diff --git a/internal/serviceoffercontroller/upstream_openapi_test.go b/internal/serviceoffercontroller/upstream_openapi_test.go new file mode 100644 index 00000000..08bc2507 --- /dev/null +++ b/internal/serviceoffercontroller/upstream_openapi_test.go @@ -0,0 +1,302 @@ +package serviceoffercontroller + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/ObolNetwork/obol-stack/internal/erc8004" + "github.com/ObolNetwork/obol-stack/internal/monetizeapi" + "github.com/ObolNetwork/obol-stack/internal/schemas" +) + +// TestGetJSONMap_SizeCap pins the BLOCKER fix: an upstream body over the +// cap must be rejected outright, not read in full and re-marshaled into the +// shared "obol-skill-md" ConfigMap where it would blow the ~1MiB k8s limit +// for every offer's bundle, not just its own. +func TestGetJSONMap_SizeCap(t *testing.T) { + big := `{"paths":{"/x":{}},"pad":"` + strings.Repeat("a", maxUpstreamOpenAPIBytes) + `"}` + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(big)) + })) + defer srv.Close() + + if _, err := getJSONMap(srv.URL); err == nil { + t.Fatal("getJSONMap accepted an oversized body, want a size-cap error") + } + + small := `{"paths":{"/x":{}}}` + srv2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(small)) + })) + defer srv2.Close() + if _, err := getJSONMap(srv2.URL); err != nil { + t.Fatalf("getJSONMap rejected a small body: %v", err) + } +} + +// TestRewriteUpstreamOpenAPI_SizeCapFallsBack pins the re-marshal side of +// the same cap: even a document that arrived under budget must not be +// republished if rewriting (adding servers/info/x-discovery) pushes it over. +func TestRewriteUpstreamOpenAPI_SizeCapFallsBack(t *testing.T) { + offer := hostnameOffer() + oversized := map[string]any{ + "paths": map[string]any{"/x": map[string]any{}}, + "pad": strings.Repeat("a", maxUpstreamOpenAPIBytes), + } + if _, ok := rewriteUpstreamOpenAPI(oversized, offer, schemas.StorefrontProfile{}); ok { + t.Fatal("rewriteUpstreamOpenAPI accepted a document that re-marshals over the size cap") + } + + // End-to-end: buildOfferBundles must fall back to the offer-scoped + // openapi.json (buildOfferScopedOpenAPI) instead of failing the whole + // static site. + fallback := buildOfferScopedOpenAPI(offer, schemas.StorefrontProfile{}) + upstream := func(*monetizeapi.ServiceOffer) map[string]any { return oversized } + bundles := buildOfferBundles([]*monetizeapi.ServiceOffer{offer}, schemas.StorefrontProfile{}, upstream) + var openapiContent string + for _, f := range bundles { + if f.Path == "offers/sec/audit/openapi.json" { + openapiContent = f.Content + } + } + if openapiContent != fallback { + t.Fatalf("oversized upstream doc leaked into the bundle instead of falling back to buildOfferScopedOpenAPI") + } +} + +// TestUpstreamOpenAPICache_DeterministicAcrossFlappingFetch pins the second +// BLOCKER fix: buildOfferBundles must never re-fetch live, and the cache +// that feeds it must fetch at most once per offer generation — a flapping +// upstream (different content on every call) must not change what the +// cache serves between reconciles of unrelated offers. +func TestUpstreamOpenAPICache_DeterministicAcrossFlappingFetch(t *testing.T) { + offer := hostnameOffer() + offer.UID = "offer-uid-1" + offer.Generation = 1 + + fetchCount := 0 + flapping := func(*monetizeapi.ServiceOffer) map[string]any { + fetchCount++ + return map[string]any{"paths": map[string]any{"/x": map[string]any{}}, "call": fetchCount} + } + + var cache upstreamOpenAPICache + cache.refresh(offer, flapping) + first := cache.get(offer) + if fetchCount != 1 { + t.Fatalf("fetchCount after first refresh = %d, want 1", fetchCount) + } + + // Same generation: refresh again (simulating a requeue with no spec + // change while the upstream is flapping) must not fetch again, and the + // bundle-facing read must be byte-identical to the first fetch. + cache.refresh(offer, flapping) + if fetchCount != 1 { + t.Fatalf("fetchCount after same-generation refresh = %d, want 1 (no re-fetch)", fetchCount) + } + second := cache.get(offer) + firstJSON, _ := json.Marshal(first) + secondJSON, _ := json.Marshal(second) + if string(firstJSON) != string(secondJSON) { + t.Fatalf("cached doc changed across a same-generation refresh: %s vs %s", firstJSON, secondJSON) + } + + // A real generation bump (spec change) does fetch again. + offer.Generation = 2 + cache.refresh(offer, flapping) + if fetchCount != 2 { + t.Fatalf("fetchCount after generation bump = %d, want 2", fetchCount) + } + + // buildOfferBundles only ever reads the cache — never triggers a fetch + // itself, however many times it's called (the static-site rebuild that + // happens on every offer's reconcile). + for i := 0; i < 3; i++ { + buildOfferBundles([]*monetizeapi.ServiceOffer{offer}, schemas.StorefrontProfile{}, cache.get) + } + if fetchCount != 2 { + t.Fatalf("fetchCount after 3 bundle rebuilds = %d, want 2 (buildOfferBundles must not fetch)", fetchCount) + } +} + +// TestBuildOfferWellKnownX402FromOpenAPI_PerRoutePriceOverride pins the +// price-override fix: a paid upstream operation whose path matches a +// spec.routes[] entry with a price override must be priced at that +// override, not the offer's default price for every op. +func TestBuildOfferWellKnownX402FromOpenAPI_PerRoutePriceOverride(t *testing.T) { + offer := hostnameOffer() // routeTableOffer: /submit POST overridden to 0.5, offer default 0.1 + doc := map[string]any{ + "paths": map[string]any{ + "/submit": map[string]any{ + "post": map[string]any{ + "security": []any{map[string]any{"x402": []any{}}}, + "x-payment-info": map[string]any{}, + "responses": map[string]any{"402": map[string]any{}}, + }, + }, + "/v1/other": map[string]any{ + "get": map[string]any{ + "security": []any{map[string]any{"x402": []any{}}}, + "x-payment-info": map[string]any{}, + "responses": map[string]any{"402": map[string]any{}}, + }, + }, + }, + } + var wk struct { + Resources []struct { + Resource string `json:"resource"` + Method string `json:"method"` + Accepts []map[string]any `json:"accepts"` + } `json:"resources"` + } + if err := json.Unmarshal([]byte(buildOfferWellKnownX402FromOpenAPI(offer, doc)), &wk); err != nil { + t.Fatal(err) + } + byResource := map[string]map[string]any{} + for _, r := range wk.Resources { + byResource[r.Resource] = r.Accepts[0] + } + submit := byResource["https://audit.v1337.example/submit"] + if submit["amount"] != "500000" { + t.Errorf("/submit accepts = %v, want the 0.5 route override (500000 atomic units)", submit) + } + other := byResource["https://audit.v1337.example/v1/other"] + if other["amount"] != "100000" { + t.Errorf("/v1/other accepts = %v, want the offer default (0.1 => 100000 atomic units)", other) + } +} + +// TestBuildOfferAgentRegistration_RegistrationsField pins the ERC-8004 +// registrations[] fix: the field must be populated once the offer carries +// an on-chain agentId (status.AgentID), and absent otherwise — buyers using +// --expected-agent-id fail closed on an empty registrations[] +// (internal/buy/discover.go verifyAgentID). +func TestBuildOfferAgentRegistration_RegistrationsField(t *testing.T) { + offer := hostnameOffer() + offer.Spec.Registration.Enabled = true + + var unregistered map[string]any + if err := json.Unmarshal([]byte(buildOfferAgentRegistration(offer, schemas.StorefrontProfile{})), &unregistered); err != nil { + t.Fatal(err) + } + if _, has := unregistered["registrations"]; has { + t.Errorf("unregistered offer already carries registrations[]: %v", unregistered["registrations"]) + } + + offer.Status.AgentID = "42" + var reg map[string]any + if err := json.Unmarshal([]byte(buildOfferAgentRegistration(offer, schemas.StorefrontProfile{})), ®); err != nil { + t.Fatal(err) + } + regs, ok := reg["registrations"].([]any) + if !ok || len(regs) != 1 { + t.Fatalf("registrations = %v, want one entry", reg["registrations"]) + } + entry := regs[0].(map[string]any) + if entry["agentId"] != float64(42) { + t.Errorf("agentId = %v, want 42", entry["agentId"]) + } + if entry["agentRegistry"] != erc8004.BaseSepolia.CAIP10Registry() { + t.Errorf("agentRegistry = %v, want %s (offer's base-sepolia payment network)", entry["agentRegistry"], erc8004.BaseSepolia.CAIP10Registry()) + } +} + +// TestBuildOfferAgentRegistration_OriginScope pins the origin-scope fix: +// an https(s) service endpoint must equal the offer's own origin, or be a +// sub-path of it — a same-prefix different-host origin like +// "https://audit.v1337.example.evil.tld" must not pass HasPrefix. +func TestBuildOfferAgentRegistration_OriginScope(t *testing.T) { + offer := hostnameOffer() + offer.Spec.Registration.Services = []monetizeapi.ServiceOfferService{ + {Name: "web", Endpoint: "https://audit.v1337.example.evil.tld/steal"}, + {Name: "a2a", Endpoint: "https://audit.v1337.example/a2a"}, + {Name: "root", Endpoint: "https://audit.v1337.example"}, + } + var reg map[string]any + if err := json.Unmarshal([]byte(buildOfferAgentRegistration(offer, schemas.StorefrontProfile{})), ®); err != nil { + t.Fatal(err) + } + services, _ := reg["services"].([]any) + var endpoints []string + for _, s := range services { + endpoints = append(endpoints, s.(map[string]any)["endpoint"].(string)) + } + for _, want := range endpoints { + if strings.Contains(want, "evil.tld") { + t.Fatalf("services leaked the evil-suffix origin: %v", endpoints) + } + } + joined := strings.Join(endpoints, ",") + if !strings.Contains(joined, "https://audit.v1337.example/a2a") || !strings.Contains(joined, "https://audit.v1337.example") { + t.Errorf("services missing legitimately-scoped endpoints: %v", endpoints) + } +} + +// TestUpstreamOpenAPIBase_NamespaceConstraint pins the SSRF fix's namespace +// constraint: the openapi probe must target the offer's OWN namespace, not +// an offer-author-controlled spec.upstream.namespace override — unlike the +// Gateway API data path, this controller-side fetch has no ReferenceGrant +// check gating a cross-namespace target. +func TestUpstreamOpenAPIBase_NamespaceConstraint(t *testing.T) { + offer := hostnameOffer() + offer.Namespace = "tenant-a" + offer.Spec.Upstream.Namespace = "tenant-b-secrets" + + base := upstreamOpenAPIBase(offer) + if !strings.Contains(base, "tenant-a.svc.cluster.local") { + t.Errorf("base = %q, want the offer's own namespace (tenant-a)", base) + } + if strings.Contains(base, "tenant-b") { + t.Errorf("base = %q, leaked spec.upstream.namespace override", base) + } +} + +// TestGetJSONMap_NoRedirectFollow pins the SSRF fix's redirect guard: a +// republished document must never come from wherever a 3xx points, since +// the offer author controls the fetch target and the result is served +// publicly. +func TestGetJSONMap_NoRedirectFollow(t *testing.T) { + var hitTarget bool + target := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hitTarget = true + w.Write([]byte(`{"paths":{"/x":{}}}`)) + })) + defer target.Close() + + redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target.URL, http.StatusFound) + })) + defer redirector.Close() + + if _, err := getJSONMap(redirector.URL); err == nil { + t.Fatal("getJSONMap followed a redirect, want it rejected as a non-2xx response") + } + if hitTarget { + t.Fatal("getJSONMap followed the redirect to a different host") + } +} + +// TestIsSimpleUpstreamPath pins the path-validation half of the SSRF fix: +// spec.registration.metadata["openapiPath"] is offer-author-controlled and +// gets string-concatenated onto the fetch base, so it must stay a plain +// '/'-prefixed path. +func TestIsSimpleUpstreamPath(t *testing.T) { + cases := map[string]bool{ + "/v1/openapi.json": true, + "/openapi.json": true, + "openapi.json": false, // not '/'-prefixed + "/../../etc/passwd": false, // traversal + "/v1/../../../secrets": false, // traversal + "http://evil.tld/x": false, // embedded scheme + "/x?y=http://evil.tld": false, // embedded scheme, even mid-path + } + for path, want := range cases { + if got := isSimpleUpstreamPath(path); got != want { + t.Errorf("isSimpleUpstreamPath(%q) = %v, want %v", path, got, want) + } + } +}