diff --git a/internal/adapter/proxy/core/common.go b/internal/adapter/proxy/core/common.go index 280e389..bcae197 100644 --- a/internal/adapter/proxy/core/common.go +++ b/internal/adapter/proxy/core/common.go @@ -61,12 +61,11 @@ func CopyHeaders(proxyReq, originalReq *http.Request) { proxyReq.Header[header] = values } - // SCOUT-581: Host header missing for some requests - // preserve the original host header which is critical for virtual hosting - // many backend services rely on this to route requests correctly - if originalReq.Host != "" { - proxyReq.Host = originalReq.Host - } + // Deprecates SCOUT-581, superseded by OLLA-135: + // https://github.com/thushan/olla/issues/135 + // Do not propagate the inbound Host onto the outbound request. + // req.URL.Host is authoritative for the backend; X-Forwarded-Host (set below) carries + // the original value for backends that need it. // Add proxy identification headers proxyReq.Header.Set(constants.HeaderXProxiedBy, GetProxiedByHeader()) diff --git a/internal/adapter/proxy/core/common_test.go b/internal/adapter/proxy/core/common_test.go index 94495d5..1929bab 100644 --- a/internal/adapter/proxy/core/common_test.go +++ b/internal/adapter/proxy/core/common_test.go @@ -776,6 +776,29 @@ func TestSetStickySessionHeaders(t *testing.T) { } } +// TestCopyHeaders_DoesNotPropagateInboundHost verifies that the inbound client Host is never +// forwarded to the backend. Go's transport uses req.URL.Host when req.Host is empty, which is +// the correct backend address. Propagating the inbound Host breaks HTTPS backends behind strict +// nginx server_name rules and is a Host-header injection vector. +func TestCopyHeaders_DoesNotPropagateInboundHost(t *testing.T) { + t.Parallel() + + originalReq := httptest.NewRequest("POST", "http://olla.example.com/api/generate", nil) + originalReq.Host = "olla.example.com" + + proxyReq := httptest.NewRequest("POST", "http://backend.internal:11434/api/generate", nil) + proxyReq.Host = "" // Transport will use URL.Host when this is empty + + CopyHeaders(proxyReq, originalReq) + + // Host must remain unset so Go's transport derives it from URL.Host (the backend address). + assert.Empty(t, proxyReq.Host, "outbound Host must not be overridden with the inbound client Host") + + // The original host must still be available to backends via X-Forwarded-Host. + assert.Equal(t, "olla.example.com", proxyReq.Header.Get("X-Forwarded-Host"), + "X-Forwarded-Host must carry the original inbound Host") +} + // BenchmarkSetResponseHeaders benchmarks the SetResponseHeaders function func BenchmarkSetResponseHeaders(b *testing.B) { stats := &ports.RequestStats{ diff --git a/internal/adapter/proxy/proxy_headers_test.go b/internal/adapter/proxy/proxy_headers_test.go index e6c162a..113d1f1 100644 --- a/internal/adapter/proxy/proxy_headers_test.go +++ b/internal/adapter/proxy/proxy_headers_test.go @@ -5,6 +5,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/thushan/olla/internal/adapter/proxy/olla" @@ -143,3 +144,192 @@ func TestProxyResponseHeaders_NoOverride(t *testing.T) { assert.NotEmpty(t, w.Header().Get(constants.HeaderXServedBy)) assert.Contains(t, w.Header().Get(constants.HeaderXServedBy), "Olla/") } + +// hostHeaderSuites returns both proxy engines paired with a constructor so tests +// can iterate over both without duplicating setup. +func hostHeaderSuites() []struct { + name string + createProxy func(upstream *httptest.Server) ports.ProxyService +} { + make := func(name string, fn func(upstream *httptest.Server) ports.ProxyService) struct { + name string + createProxy func(upstream *httptest.Server) ports.ProxyService + } { + return struct { + name string + createProxy func(upstream *httptest.Server) ports.ProxyService + }{name, fn} + } + + return []struct { + name string + createProxy func(upstream *httptest.Server) ports.ProxyService + }{ + make("Sherpa", func(upstream *httptest.Server) ports.ProxyService { + ep := createTestEndpoint("be", upstream.URL, domain.StatusHealthy) + disc := &mockDiscoveryService{endpoints: []*domain.Endpoint{ep}} + sel := &mockEndpointSelector{endpoint: ep} + cfg := &sherpa.Configuration{} // zero value is fine for unit tests + svc, _ := sherpa.NewService(disc, sel, cfg, createTestStatsCollector(), nil, createTestLogger()) + return svc + }), + make("Olla", func(upstream *httptest.Server) ports.ProxyService { + ep := createTestEndpoint("be", upstream.URL, domain.StatusHealthy) + disc := &mockDiscoveryService{endpoints: []*domain.Endpoint{ep}} + sel := &mockEndpointSelector{endpoint: ep} + cfg := &olla.Configuration{} + svc, _ := olla.NewService(disc, sel, cfg, createTestStatsCollector(), nil, createTestLogger()) + return svc + }), + } +} + +// TestProxy_OutboundHostMatchesBackend asserts that the Host seen by the backend +// is its own address, not the inbound client Host. Regression for issue #135. +func TestProxy_OutboundHostMatchesBackend(t *testing.T) { + t.Parallel() + + for _, tc := range hostHeaderSuites() { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var capturedHost string + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedHost = r.Host + w.WriteHeader(http.StatusOK) + })) + defer upstream.Close() + + proxy := tc.createProxy(upstream) + + req := httptest.NewRequest("GET", "/test", nil) + req.Host = "olla.example.com" // inbound client host — must NOT reach the backend + + stats := &ports.RequestStats{RequestID: "host-test", StartTime: time.Now()} + w := httptest.NewRecorder() + _ = proxy.ProxyRequest(req.Context(), w, req, stats, createTestLogger()) + + // The backend's listener address is the authoritative host. + assert.Equal(t, upstream.Listener.Addr().String(), capturedHost, + "backend must receive its own address as Host, not the inbound client Host") + }) + } +} + +// TestProxy_XForwardedHostPreservesInbound asserts that backends can still recover the +// original client Host via X-Forwarded-Host even though we don't propagate it as Host. +func TestProxy_XForwardedHostPreservesInbound(t *testing.T) { + t.Parallel() + + for _, tc := range hostHeaderSuites() { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var capturedXFH string + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedXFH = r.Header.Get("X-Forwarded-Host") + w.WriteHeader(http.StatusOK) + })) + defer upstream.Close() + + proxy := tc.createProxy(upstream) + + req := httptest.NewRequest("GET", "/test", nil) + req.Host = "olla.example.com" + + stats := &ports.RequestStats{RequestID: "xfh-test", StartTime: time.Now()} + w := httptest.NewRecorder() + _ = proxy.ProxyRequest(req.Context(), w, req, stats, createTestLogger()) + + assert.Equal(t, "olla.example.com", capturedXFH, + "X-Forwarded-Host must carry the original inbound Host for backends that need it") + }) + } +} + +// TestProxy_HostHeaderInjectionIsNeutralised is the security regression for issue #135. +// An attacker supplying a crafted Host must not influence which virtual host the backend serves. +func TestProxy_HostHeaderInjectionIsNeutralised(t *testing.T) { + t.Parallel() + + for _, tc := range hostHeaderSuites() { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var capturedHost string + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedHost = r.Host + w.WriteHeader(http.StatusOK) + })) + defer upstream.Close() + + proxy := tc.createProxy(upstream) + + req := httptest.NewRequest("GET", "/test", nil) + req.Host = "attacker.evil.com" // injected host — must be neutralised + + stats := &ports.RequestStats{RequestID: "inject-test", StartTime: time.Now()} + w := httptest.NewRecorder() + _ = proxy.ProxyRequest(req.Context(), w, req, stats, createTestLogger()) + + assert.NotEqual(t, "attacker.evil.com", capturedHost, + "injected Host must not reach the backend") + assert.Equal(t, upstream.Listener.Addr().String(), capturedHost, + "backend must receive its own address as Host regardless of inbound Host value") + }) + } +} + +// TestProxy_MultipleBackends_HostIsPerBackend verifies that when the selector routes to +// different backends, each request carries the correct backend's own Host — not a host +// value leaked from a prior request or the inbound client. +// +// Note: the mock selector always returns the pre-configured endpoint, so we use two separate +// proxy instances (one per backend) to simulate per-backend routing rather than within-proxy +// round-robin, which requires a real balancer. The invariant being tested — that URL.Host +// drives the outbound Host — is identical regardless of how the endpoint was selected. +func TestProxy_MultipleBackends_HostIsPerBackend(t *testing.T) { + t.Parallel() + + var capturedHost1, capturedHost2 string + + upstream1 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedHost1 = r.Host + w.WriteHeader(http.StatusOK) + })) + defer upstream1.Close() + + upstream2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedHost2 = r.Host + w.WriteHeader(http.StatusOK) + })) + defer upstream2.Close() + + suites := hostHeaderSuites() + proxy1 := suites[0].createProxy(upstream1) // Sherpa → upstream1 + proxy2 := suites[0].createProxy(upstream2) // Sherpa → upstream2 + + inboundHost := "olla.example.com" + + req1 := httptest.NewRequest("GET", "/test", nil) + req1.Host = inboundHost + _ = proxy1.ProxyRequest(req1.Context(), httptest.NewRecorder(), req1, + &ports.RequestStats{RequestID: "mb1", StartTime: time.Now()}, createTestLogger()) + + req2 := httptest.NewRequest("GET", "/test", nil) + req2.Host = inboundHost + _ = proxy2.ProxyRequest(req2.Context(), httptest.NewRecorder(), req2, + &ports.RequestStats{RequestID: "mb2", StartTime: time.Now()}, createTestLogger()) + + assert.Equal(t, upstream1.Listener.Addr().String(), capturedHost1, + "backend 1 must receive its own address as Host") + assert.Equal(t, upstream2.Listener.Addr().String(), capturedHost2, + "backend 2 must receive its own address as Host") + assert.NotEqual(t, capturedHost1, capturedHost2, + "each backend must see a distinct, correct Host") + + // HTTP/2: Go's httptest.NewServer is HTTP/1. The h2 :authority pseudo-header is derived + // from the same req.Host field on the server side, so the HTTP/1 assertions above are a + // sufficient proxy — the plumbing is identical. An h2 variant would require + // httptest.NewUnstartedServer + TLS, which adds noise without extra coverage. +}