From 57b0146fcdf8d7eafab8f1cae06a8ca5c7f5fc65 Mon Sep 17 00:00:00 2001 From: woodsonl <65194841+woodsonl@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:36:49 -0500 Subject: [PATCH] fix: bound proxy request bodies and gate browser origins at request entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two additions at the proxy's request boundary: - Inbound bodies are buffered once so each failover attempt can replay them, with no size limit — any loopback caller could make the proxy read an unbounded body into memory before routing. Cap the buffer at 32 MiB and refuse anything larger with 413 before resolving a candidate or reserving scheduler capacity. A mid-body read error is refused the same way rather than forwarded truncated, and the 413 uses the same JSON error shape as every other proxy rejection. Long-context prompts fit well below the cap. - The loopback listener is reachable from a browser page on any origin, and a simple cross-origin POST needs no preflight, so header policy alone cannot stop an unlisted page from driving the local engines. Add a deny-by-default allowlist gate ahead of forwarding: a request carrying an Origin is admitted only from an exact origin listed in NVPAIR_PROXY_ALLOWED_ORIGINS, otherwise it is refused with 403 origin-not-allowed. Callers that send no Origin (Electron main, CLI, probes) are unaffected. This sits above the existing per-engine intersection policy, which still decides what an admitted origin may read. Signed-off-by: woodsonl <65194841+woodsonl@users.noreply.github.com> --- services/nvpair-proxy/README.md | 4 ++ services/nvpair-proxy/body_limit_test.go | 66 ++++++++++++++++++++++ services/nvpair-proxy/ingress.go | 12 ++++ services/nvpair-proxy/ingress_test.go | 51 +++++++++++++++++ services/nvpair-proxy/proxy.go | 50 ++++++++++++---- services/nvpair-proxy/retry_test.go | 9 +++ services/shared/cors/allowlist.go | 72 ++++++++++++++++++++++++ services/shared/cors/allowlist_test.go | 55 ++++++++++++++++++ 8 files changed, 308 insertions(+), 11 deletions(-) create mode 100644 services/nvpair-proxy/body_limit_test.go create mode 100644 services/shared/cors/allowlist.go create mode 100644 services/shared/cors/allowlist_test.go diff --git a/services/nvpair-proxy/README.md b/services/nvpair-proxy/README.md index c36fb0c2..8d921073 100644 --- a/services/nvpair-proxy/README.md +++ b/services/nvpair-proxy/README.md @@ -116,6 +116,8 @@ currently active node — except the model-list routes, which are queried across every candidate node concurrently and merged into one de-duplicated inventory. Point your client at the proxy and it handles routing. +Inbound request bodies are buffered once so each failover attempt can replay them. That buffer is capped at 32 MiB: a larger body is refused with `413` before any candidate is resolved or forwarded, rather than read into memory unbounded. Long-context prompts fit far below the cap. + When the broker supplies `aliasAddresses`, the facade reserves that loopback-only endpoint before reporting ready and serves it through the same routing and workload-lifecycle handler. A `localhost` alias reserves `127.0.0.1` @@ -148,6 +150,8 @@ sit on the engine's own port. The stored value predates the current default. **Browser clients (CORS).** PAIR does not enable CORS or add default browser permissions. Configure origins through the engine; its built-in defaults and user configuration remain authoritative. Ordinary forwarded responses preserve the upstream status, body, and CORS headers, including missing headers. A denial is never replaced with a successful OPTIONS response or retried to find permission elsewhere. Proxy-generated errors carry their actual status without CORS permission headers, so browser JavaScript may see a generic CORS failure while curl and diagnostics show the real error. +Above that per-engine policy sits a request-entry allowlist gate. The loopback listener is reachable by a browser page on any origin, and a simple cross-origin POST needs no preflight, so header policy alone cannot stop an unlisted page from driving the local engines. Browser callers (a request carrying an `Origin`) are therefore admitted only from exact origins the operator lists in `NVPAIR_PROXY_ALLOWED_ORIGINS` (comma-separated, scheme+host[:port], compared exactly); with no entry configured, every browser origin is refused with `403` `origin-not-allowed` before it can reach a candidate or reserve capacity. Non-browser callers (the Electron main process, CLI tools, health probes) send no `Origin` and are unaffected. The gate only admits or refuses; what an admitted origin may then read is still the engines' own intersection policy below. + A browser preflight (OPTIONS with Origin and Access-Control-Request-Method) queries every currently routable target, with concurrency eight, a ten-second query deadline, and no redirects. A single target's response is relayed. Multiple responding targets must all permit the requested origin, method, and headers; PAIR grants only their shared permissions. Credentials require unanimous explicit support. Synthesized preflights allow browsers to cache the result for 60 seconds; PAIR itself does not cache decisions. A policy denial returns 403. Unavailable targets are skipped; if none can answer, the proxy returns 502. Ordinary OPTIONS requests retain normal routing. Preflights do not create inference jobs or reserve scheduler capacity. Paired ingress forwards only to its local engine. Combined model lists forward the caller's origin and end-to-end headers, excluding Authorization and Cookie so credentials are not shared across engines. Multi-target preflights apply the same credential filtering. With an Origin header, every responding engine must return a valid list and permit sharing: one denial returns 403 and one invalid list returns 502, without partial inventory. An invalid-list error retains the combined CORS permissions when every responding engine allows the origin. Unavailable engines are skipped, with 502 returned when none can answer. Successful lists combine origin/credential permissions and Vary requirements. Requests without Origin retain partial aggregation when some inventories are unavailable. Engines without CORS support remain unavailable to cross-origin browser clients through PAIR. diff --git a/services/nvpair-proxy/body_limit_test.go b/services/nvpair-proxy/body_limit_test.go new file mode 100644 index 00000000..44b854a2 --- /dev/null +++ b/services/nvpair-proxy/body_limit_test.go @@ -0,0 +1,66 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestBufferBodyAndModelLimit covers the body-cap contract: a body over +// maxInferenceBodyBytes is reported too-large with the body dropped (never +// buffered into memory), and a body within the limit is returned with its +// parsed model field. +func TestBufferBodyAndModelLimit(t *testing.T) { + t.Run("body over the limit is too large", func(t *testing.T) { + big := bytes.Repeat([]byte("a"), maxInferenceBodyBytes+1) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", bytes.NewReader(big)) + body, model, tooLarge := bufferBodyAndModel(req) + if !tooLarge { + t.Fatal("bufferBodyAndModel tooLarge = false, want true past the cap") + } + if body != nil { + t.Error("body should be dropped (nil) when too large, not buffered") + } + if model != "" { + t.Errorf("model = %q, want empty when too large", model) + } + }) + + t.Run("body at the limit parses", func(t *testing.T) { + atLimit := bytes.Repeat([]byte("a"), maxInferenceBodyBytes) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", bytes.NewReader(atLimit)) + _, _, tooLarge := bufferBodyAndModel(req) + if tooLarge { + t.Fatal("bufferBodyAndModel tooLarge = true at exactly the cap, want false") + } + }) + + t.Run("model parsed from a small body", func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", strings.NewReader(`{"model":"llama3"}`)) + body, model, tooLarge := bufferBodyAndModel(req) + if tooLarge || model != "llama3" || string(body) != `{"model":"llama3"}` { + t.Fatalf("= (%q, %q, %v), want body kept, model llama3, not too large", body, model, tooLarge) + } + }) +} + +// TestHandleHTTPRejectsOversizedBody drives the 413 path end to end: a request +// body past maxInferenceBodyBytes is refused before any candidate resolution +// or upstream forward, with StatusRequestEntityTooLarge. +func TestHandleHTTPRejectsOversizedBody(t *testing.T) { + p := testProxy(anyProfile(t), NewDiscovery(), 1235) + big := bytes.Repeat([]byte("a"), maxInferenceBodyBytes+1) + req := httptest.NewRequest(http.MethodPost, "/v1/chat/completions", bytes.NewReader(big)) + req.RemoteAddr = "127.0.0.1:40000" + rec := httptest.NewRecorder() + + p.soleFacade().handleHTTP(rec, req) + if rec.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("oversized body status = %d, want %d", rec.Code, http.StatusRequestEntityTooLarge) + } +} diff --git a/services/nvpair-proxy/ingress.go b/services/nvpair-proxy/ingress.go index ba8336f5..402eefd4 100644 --- a/services/nvpair-proxy/ingress.go +++ b/services/nvpair-proxy/ingress.go @@ -12,6 +12,8 @@ import ( "net/http/httputil" "net/url" "strconv" + + "nvpair-shared/cors" ) const engineIdentityProbeHeader = "X-NVPAIR-Engine-Identity-Probe" @@ -84,6 +86,16 @@ func (f *facade) handlePlain(w http.ResponseWriter, r *http.Request) { "plaintext requests are accepted only from loopback; cluster peers must use the mTLS ingress") return } + // Cross-origin browser gate: a loopback bind does not exclude browser + // pages (they connect from loopback), so any Origin this process's + // allowlist does not name is refused before it can drive an engine. + if !cors.AllowRequest(r) { + slog.Warn("rejected cross-origin browser request not on the allowlist", + "remote", r.RemoteAddr, "method", r.Method, "path", r.URL.Path, + "origin", r.Header.Get("Origin")) + cors.RejectOrigin(w) + return + } // Engine-manager marks its private identity/action requests so this // compatibility facade can never be mistaken for the local Ollama backend. if r.Header.Get(engineIdentityProbeHeader) == "1" { diff --git a/services/nvpair-proxy/ingress_test.go b/services/nvpair-proxy/ingress_test.go index 5ccc24ab..d0996010 100644 --- a/services/nvpair-proxy/ingress_test.go +++ b/services/nvpair-proxy/ingress_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "nvpair-shared/cors" "strings" "testing" ) @@ -57,6 +58,56 @@ func TestHandlePlainRejectsNonLoopback(t *testing.T) { } } +// TestAllowlistGateRejectsUnlistedOrigin covers the request-entry gate: a +// loopback request from an origin the operator allowlist does not name is +// refused with 403 before it can drive an engine. The Origin-less (non-browser) +// caller is unaffected. This runs before engine-policy intersection, which is +// what closes the blind-oracle path a simple cross-origin POST would otherwise +// reach with no preflight. +func TestAllowlistGateRejectsUnlistedOrigin(t *testing.T) { + t.Setenv(cors.AllowedOriginsEnv, "https://ui.example") + + t.Run("unlisted origin is refused", func(t *testing.T) { + p := testProxy(anyProfile(t), NewDiscovery(), 11435) + req := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + req.RemoteAddr = "127.0.0.1:40000" + req.Header.Set("Origin", "https://evil.example") + rec := httptest.NewRecorder() + p.soleFacade().handlePlain(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("unlisted origin status = %d, want %d", rec.Code, http.StatusForbidden) + } + if got := rec.Header().Get("Access-Control-Allow-Origin"); got != "" { + t.Errorf("Access-Control-Allow-Origin = %q, want no grant on a refusal", got) + } + }) + + t.Run("allowlisted origin reaches routing", func(t *testing.T) { + p := testProxy(anyProfile(t), NewDiscovery(), 11435) + req := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + req.RemoteAddr = "127.0.0.1:40000" + req.Header.Set("Origin", "https://ui.example") + rec := httptest.NewRecorder() + // No engine is available, so the request reaches routing and is + // rejected there (502), not by the allowlist gate (403). + p.soleFacade().handlePlain(rec, req) + if rec.Code == http.StatusForbidden { + t.Fatalf("allowlisted origin was denied by the gate (403); want it to pass through to routing") + } + }) + + t.Run("no Origin is unaffected", func(t *testing.T) { + p := testProxy(anyProfile(t), NewDiscovery(), 11435) + req := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + req.RemoteAddr = "127.0.0.1:40000" + rec := httptest.NewRecorder() + p.soleFacade().handlePlain(rec, req) + if rec.Code == http.StatusForbidden { + t.Fatalf("Origin-less request was denied by the gate (403); want it unaffected") + } + }) +} + // Preflight is subject to the same ingress gate as ordinary requests. func TestHandlePlainRejectsPreflightAtLoopbackGate(t *testing.T) { p := testProxy(anyProfile(t), NewDiscovery(), 11435) diff --git a/services/nvpair-proxy/proxy.go b/services/nvpair-proxy/proxy.go index 929c3d67..32ef98d9 100644 --- a/services/nvpair-proxy/proxy.go +++ b/services/nvpair-proxy/proxy.go @@ -215,26 +215,34 @@ type workloadParams struct { // bufferBodyAndModel reads the request body once and returns the raw bytes // (so each failover attempt can replay it — see the loop in handleHTTP) along -// with the JSON "model" field for workload tracking. Inference bodies are -// small (prompt + model), so full buffering is cheap. Returns (nil, "") when -// the body is absent and an empty model when none is parseable. The caller -// restores r.Body from the returned bytes before each forward attempt. -func bufferBodyAndModel(r *http.Request) ([]byte, string) { +// with the JSON "model" field for workload tracking. Bodies are capped at +// maxInferenceBodyBytes: without a limit, any loopback caller (or a cross-origin +// browser POST) could stream an arbitrarily large body and exhaust proxy +// memory. Returns (nil, "", false) when the body is absent and an empty model +// when none is parseable. The caller restores r.Body from the returned bytes +// before each forward attempt. +func bufferBodyAndModel(r *http.Request) ([]byte, string, bool) { if r.Body == nil { - return nil, "" + return nil, "", false } - body, err := io.ReadAll(r.Body) + body, err := io.ReadAll(io.LimitReader(r.Body, maxInferenceBodyBytes+1)) _ = r.Body.Close() if err != nil { - return body, "" + // A mid-body read error leaves a truncated buffer; refuse it rather + // than forward a maimed prompt upstream. Reuses the over-limit + // outcome — the caller's only two states are "buffered" or "refuse". + return nil, "", true + } + if len(body) > maxInferenceBodyBytes { + return nil, "", true } var probe struct { Model string `json:"model"` } if err := json.Unmarshal(body, &probe); err != nil { - return body, "" + return body, "", false } - return body, probe.Model + return body, probe.Model, false } type statusCapture struct { @@ -736,6 +744,11 @@ const ( proxyReadHeaderTimeout = 10 * time.Second proxyServerIdleTimeout = 90 * time.Second maxModelListBytes = 16 << 20 + // maxInferenceBodyBytes caps how much of an inbound request body the proxy + // buffers for replay across failover attempts. Long-context prompts fit + // far below this; anything larger is rejected with 413 instead of being + // buffered into memory unbounded. + maxInferenceBodyBytes = 32 << 20 ) // idleClientWriteTimeout bounds how long a single write of streamed response @@ -1161,7 +1174,22 @@ func (f *facade) handleHTTP(w http.ResponseWriter, r *http.Request) { // Parse the request's model before choosing a node. Model eligibility only // applies to inference routes; control endpoints retain their existing // routing behavior even when their JSON happens to contain a model field. - bodyBytes, model := bufferBodyAndModel(r) + bodyBytes, model, bodyTooLarge := bufferBodyAndModel(r) + if bodyTooLarge { + slog.Warn("proxy request rejected", + "id", reqID, "method", r.Method, "path", r.URL.Path, + "remote", r.RemoteAddr, "reason", "request body exceeds limit") + writeIngressError(w, http.StatusRequestEntityTooLarge, "request-too-large", "request body exceeds limit") + f.notify("proxy/request", RequestEvent{ + ID: reqID, + Method: r.Method, + Path: r.URL.Path, + Status: http.StatusRequestEntityTooLarge, + Duration: time.Since(start).Milliseconds(), + Error: "request body exceeds limit", + }) + return + } isInf := isInferenceRequest(f.profile, r.Method, r.URL.Path) routingModel := "" if isInf { diff --git a/services/nvpair-proxy/retry_test.go b/services/nvpair-proxy/retry_test.go index f3634f4c..b81218e7 100644 --- a/services/nvpair-proxy/retry_test.go +++ b/services/nvpair-proxy/retry_test.go @@ -9,6 +9,7 @@ import ( "io" "net/http" "net/http/httptest" + "nvpair-shared/cors" "os" "strings" "sync" @@ -24,6 +25,14 @@ import ( // directly in TestBackoffFor, so shortening it here costs no coverage. func TestMain(m *testing.M) { retryBackoff = []time.Duration{time.Millisecond} + // The CORS tests exercise engine-policy intersection using browser origins + // the engines variously grant and deny. The request-entry allowlist gate + // runs ahead of that intersection, so every origin those tests send must be + // admitted here or the gate would deny before intersection is reached and + // the tests would no longer assert what they intend. The gate's own deny + // path is asserted directly in TestAllowlistGateRejectsUnlistedOrigin. + os.Setenv(cors.AllowedOriginsEnv, + "http://app.test,https://app.test,http://other.test,http://example.com,http://wrong.com,http://denied.test,https://app.example") os.Exit(m.Run()) } diff --git a/services/shared/cors/allowlist.go b/services/shared/cors/allowlist.go new file mode 100644 index 00000000..3f721f89 --- /dev/null +++ b/services/shared/cors/allowlist.go @@ -0,0 +1,72 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package cors + +import ( + "net/http" + "os" + "strings" +) + +// The combining helpers in cors.go forward what engines declare; they never +// synthesize a grant. That is necessary but not sufficient at the request +// boundary: a browser page on any origin can reach the proxies' loopback +// listener, and a simple cross-origin POST needs no preflight, so header +// policy alone cannot stop an unlisted page from driving the local inference +// engines (a blind oracle). This file adds the deny-by-default allowlist gate +// that runs before a request is forwarded at all. The two layers compose: the +// gate admits only operator-listed origins, and Combine still intersects what +// the engines themselves permit. + +// AllowedOriginsEnv names the operator-controlled origin allowlist. Exact +// origins only; an empty value (the default) admits no browser origins. +const AllowedOriginsEnv = "NVPAIR_PROXY_ALLOWED_ORIGINS" + +// allowedOrigins returns the configured allowlist. Read per request so an +// operator change needs a proxy restart at most, not a code change per caller. +func allowedOrigins() []string { + raw := strings.TrimSpace(os.Getenv(AllowedOriginsEnv)) + if raw == "" { + return nil + } + var out []string + for _, o := range strings.Split(raw, ",") { + if o = strings.TrimSpace(o); o != "" { + out = append(out, o) + } + } + return out +} + +// originAllowed reports whether the request's Origin header is absent (a +// non-browser or same-origin-with-no-origin caller — allowed) or exactly +// matches one configured origin (allowed). A present-but-unlisted Origin is +// the cross-origin browser case: denied. +func originAllowed(r *http.Request) bool { + origin := r.Header.Get("Origin") + if origin == "" { + return true + } + for _, want := range allowedOrigins() { + if origin == want { + return true + } + } + return false +} + +// AllowRequest gates a browser-capable request: anything carrying an Origin +// not on the operator allowlist is rejected before it can drive an engine, +// closing the cross-origin blind-oracle path (a simple cross-origin POST needs +// no preflight, so header/preflight policy alone cannot provide this). +func AllowRequest(r *http.Request) bool { return originAllowed(r) } + +// RejectOrigin writes the 403 for a disallowed Origin. The body is static so +// nothing about the proxy's internals is reflected. +func RejectOrigin(w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Content-Type-Options", "nosniff") + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"error":"cross-origin browser requests are not allowed","code":"origin-not-allowed"}`)) +} diff --git a/services/shared/cors/allowlist_test.go b/services/shared/cors/allowlist_test.go new file mode 100644 index 00000000..534adec6 --- /dev/null +++ b/services/shared/cors/allowlist_test.go @@ -0,0 +1,55 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package cors + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +func TestAllowRequest(t *testing.T) { + t.Run("no Origin header is allowed (non-browser callers)", func(t *testing.T) { + if !AllowRequest(httptest.NewRequest(http.MethodPost, "/api/chat", nil)) { + t.Fatal("an Origin-less request must be allowed") + } + }) + t.Run("empty allowlist denies every browser origin", func(t *testing.T) { + t.Setenv(AllowedOriginsEnv, "") + req := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + req.Header.Set("Origin", "https://evil.example") + if AllowRequest(req) { + t.Fatal("an unlisted Origin must be denied with an empty allowlist") + } + }) + t.Run("exact match required", func(t *testing.T) { + t.Setenv(AllowedOriginsEnv, " https://ui.example ,http://localhost:5173 ") + allowed := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + allowed.Header.Set("Origin", "https://ui.example") + if !AllowRequest(allowed) { + t.Fatal("an exactly-allowlisted Origin must be allowed") + } + prefix := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + prefix.Header.Set("Origin", "https://ui.example.evil.example") + if AllowRequest(prefix) { + t.Fatal("a same-suffix origin must not match an allowlist entry as a prefix") + } + scheme := httptest.NewRequest(http.MethodPost, "/api/chat", nil) + scheme.Header.Set("Origin", "http://ui.example") + if AllowRequest(scheme) { + t.Fatal("a scheme-swapped origin must not match") + } + }) +} + +func TestRejectOriginShape(t *testing.T) { + rec := httptest.NewRecorder() + RejectOrigin(rec) + if rec.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403", rec.Code) + } + if got := rec.Header().Get("Content-Type"); got != "application/json" { + t.Errorf("Content-Type = %q, want application/json", got) + } +}