Skip to content

Commit 77896dd

Browse files
geokatgeokat
authored
fix: only trust x-forwarded-host from configured trusted proxies (#26204) (#26316)
Backport of #26204 (commit b5ef700) to `release/2.29` Replace RequestHost with httpmw.EffectiveHost, which honors X-Forwarded-Host only when the original socket peer is a configured trusted origin, otherwise falling back to the received Host header. <details> <summary>The following cherry-pick conflicts were resolved</summary> This release branch predates several things the fix was built on: the agentchat and agentproc packages, the OBSERVABILITY.md doc, the cdr.dev/slog v3 migration, and the user_agent request-log field. Rebasing produced six conflicts. Dropped (absent on this branch, so not part of the backport): - .claude/docs/OBSERVABILITY.md - agent/agentchat/log_test.go (entire agentchat package absent) - agent/agentproc/api_test.go (entire agentproc package absent) Adapted: - agent/api.go — kept the release-branch version and added only the required , nil for the new two-argument loggermw.Logger signature; did not pull in agentchat.Middleware or its import. - coderd/httpmw/loggermw/logger.go — kept cdr.dev/slog v1, removed the now-unused httpapi import, and applied the security change (log the trust-aware effective host plus received_host). Did not introduce the user_agent field, which this branch doesn't log. - coderd/httpmw/loggermw/logger_internal_test.go — updated requiredFields to match (added received_host, kept user_agent out) and rewrote the new TestLoggerMiddleware_HostFields to use the branch's local fakeSink helper, since the fix's testutil.NewFakeSink/Entries() API doesn't exist here. </details> > [!NOTE] > Breaking change. See the original PR for the breaking-change details.
1 parent 4aa84f2 commit 77896dd

17 files changed

Lines changed: 290 additions & 30 deletions

File tree

‎agent/api.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ func (a *agent) apiHandler() http.Handler {
1919
r.Use(
2020
httpmw.Recover(a.logger),
2121
tracing.StatusWriterMiddleware,
22-
loggermw.Logger(a.logger),
22+
loggermw.Logger(a.logger, nil),
2323
)
2424
r.Get("/", func(rw http.ResponseWriter, r *http.Request) {
2525
httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{

‎cli/testdata/coder_server_--help.golden‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -295,8 +295,8 @@ NETWORKING OPTIONS:
295295
True-Client-Ip, X-Forwarded-For.
296296

297297
--proxy-trusted-origins string-array, $CODER_PROXY_TRUSTED_ORIGINS
298-
Origin addresses to respect "proxy-trusted-headers". e.g.
299-
192.168.1.0/24.
298+
Origin addresses to respect "proxy-trusted-headers" and
299+
X-Forwarded-Host for subdomain app routing. e.g. 192.168.1.0/24.
300300

301301
--redirect-to-access-url bool, $CODER_REDIRECT_TO_ACCESS_URL
302302
Specifies whether to redirect requests that do not match the access

‎cli/testdata/server-config.yaml.golden‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,8 @@ networking:
172172
# True-Client-Ip, X-Forwarded-For.
173173
# (default: <unset>, type: string-array)
174174
proxyTrustedHeaders: []
175-
# Origin addresses to respect "proxy-trusted-headers". e.g. 192.168.1.0/24.
175+
# Origin addresses to respect "proxy-trusted-headers" and X-Forwarded-Host for
176+
# subdomain app routing. e.g. 192.168.1.0/24.
176177
# (default: <unset>, type: string-array)
177178
proxyTrustedOrigins: []
178179
# Controls if the 'Secure' property is set on browser session cookies.

‎coderd/coderd.go‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -868,7 +868,9 @@ func New(options *Options) *API {
868868
tracing.Middleware(api.TracerProvider),
869869
httpmw.AttachRequestID,
870870
httpmw.ExtractRealIP(api.RealIPConfig),
871-
loggermw.Logger(api.Logger),
871+
loggermw.Logger(api.Logger, func(r *http.Request) string {
872+
return httpmw.EffectiveHost(api.RealIPConfig, r)
873+
}),
872874
singleSlashMW,
873875
rolestore.CustomRoleMW,
874876
prometheusMW,

‎coderd/httpapi/request.go‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,6 @@ const (
88
XForwardedHostHeader = "X-Forwarded-Host"
99
)
1010

11-
// RequestHost returns the name of the host from the request. It prioritizes
12-
// 'X-Forwarded-Host' over r.Host since most requests are being proxied.
13-
func RequestHost(r *http.Request) string {
14-
host := r.Header.Get(XForwardedHostHeader)
15-
if host != "" {
16-
return host
17-
}
18-
19-
return r.Host
20-
}
21-
2211
func IsWebsocketUpgrade(r *http.Request) bool {
2312
vs := r.Header.Values("Upgrade")
2413
for _, v := range vs {

‎coderd/httpmw/loggermw/logger.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import (
1212
"github.com/go-chi/chi/v5"
1313

1414
"cdr.dev/slog"
15-
"github.com/coder/coder/v2/coderd/httpapi"
1615
"github.com/coder/coder/v2/coderd/tracing"
1716
)
1817

@@ -69,7 +68,7 @@ func safeQueryParams(params url.Values) []slog.Field {
6968
return fields
7069
}
7170

72-
func Logger(log slog.Logger) func(next http.Handler) http.Handler {
71+
func Logger(log slog.Logger, hostResolver func(*http.Request) string) func(next http.Handler) http.Handler {
7372
return func(next http.Handler) http.Handler {
7473
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
7574
start := time.Now()
@@ -79,8 +78,14 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler {
7978
panic(fmt.Sprintf("ResponseWriter not a *tracing.StatusWriter; got %T", rw))
8079
}
8180

81+
host := r.Host
82+
if hostResolver != nil {
83+
host = hostResolver(r)
84+
}
85+
8286
httplog := log.With(
83-
slog.F("host", httpapi.RequestHost(r)),
87+
slog.F("host", host),
88+
slog.F("received_host", r.Host),
8489
slog.F("path", r.URL.Path),
8590
slog.F("proto", r.Proto),
8691
slog.F("remote_addr", r.RemoteAddr),

‎coderd/httpmw/loggermw/logger_internal_test.go‎

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) {
6868
})
6969

7070
// Wrap the test handler with the Logger middleware
71-
loggerMiddleware := Logger(logger)
71+
loggerMiddleware := Logger(logger, nil)
7272
wrappedHandler := loggerMiddleware(testHandler)
7373

7474
// Create a test HTTP request
@@ -90,7 +90,7 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) {
9090
}
9191

9292
// Check that the log contains the expected fields
93-
requiredFields := []string{"host", "path", "proto", "remote_addr", "start", "took", "status_code", "latency_ms"}
93+
requiredFields := []string{"host", "received_host", "path", "proto", "remote_addr", "start", "took", "status_code", "latency_ms"}
9494
for _, field := range requiredFields {
9595
_, exists := fieldsMap[field]
9696
require.True(t, exists, "field %q is missing in log fields", field)
@@ -102,6 +102,38 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) {
102102
require.Equal(t, fieldsMap["status_code"], http.StatusOK)
103103
}
104104

105+
func TestLoggerMiddleware_HostFields(t *testing.T) {
106+
t.Parallel()
107+
108+
sink := &fakeSink{}
109+
logger := slog.Make(sink)
110+
logger = logger.Leveled(slog.LevelDebug)
111+
112+
testHandler := http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
113+
rw.WriteHeader(http.StatusOK)
114+
})
115+
116+
loggerMiddleware := Logger(logger, func(_ *http.Request) string {
117+
return "effective.test"
118+
})
119+
wrappedHandler := loggerMiddleware(testHandler)
120+
121+
req := httptest.NewRequest(http.MethodGet, "http://received.test/path", nil)
122+
123+
sw := &tracing.StatusWriter{ResponseWriter: httptest.NewRecorder()}
124+
wrappedHandler.ServeHTTP(sw, req)
125+
126+
require.Len(t, sink.entries, 1, "expected exactly one log entry")
127+
128+
fieldsMap := make(map[string]any)
129+
for _, field := range sink.entries[0].Fields {
130+
fieldsMap[field.Name] = field.Value
131+
}
132+
133+
require.Equal(t, "effective.test", fieldsMap["host"])
134+
require.Equal(t, "received.test", fieldsMap["received_host"])
135+
}
136+
105137
func TestLoggerMiddleware_WebSocket(t *testing.T) {
106138
t.Parallel()
107139
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
@@ -129,7 +161,7 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) {
129161
})
130162

131163
// Wrap the test handler with the Logger middleware
132-
loggerMiddleware := Logger(logger)
164+
loggerMiddleware := Logger(logger, nil)
133165
wrappedHandler := loggerMiddleware(testHandler)
134166

135167
// RequestLogger expects the ResponseWriter to be *tracing.StatusWriter
@@ -184,7 +216,7 @@ func TestRequestLogger_HTTPRouteParams(t *testing.T) {
184216
})
185217

186218
// Wrap the test handler with the Logger middleware
187-
loggerMiddleware := Logger(logger)
219+
loggerMiddleware := Logger(logger, nil)
188220
wrappedHandler := loggerMiddleware(testHandler)
189221

190222
// Create a test HTTP request

‎coderd/httpmw/realip.go‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,35 @@ func FilterUntrustedOriginHeaders(config *RealIPConfig, req *http.Request) {
105105
}
106106
}
107107

108+
// EffectiveHost returns the host Coder should trust for request handling.
109+
// It uses X-Forwarded-Host only when the immediate peer is a configured
110+
// trusted proxy. Otherwise it uses the received Host header.
111+
func EffectiveHost(config *RealIPConfig, r *http.Request) string {
112+
if config == nil {
113+
config = &RealIPConfig{
114+
TrustedOrigins: nil,
115+
TrustedHeaders: nil,
116+
}
117+
}
118+
119+
// When ExtractRealIP has run, r.RemoteAddr may hold the forwarded
120+
// client IP, and we should use the original socket peer for proxy
121+
// trust decisions.
122+
remoteAddr := r.RemoteAddr
123+
state := RealIP(r.Context())
124+
if state != nil && state.OriginalRemoteAddr != "" {
125+
remoteAddr = state.OriginalRemoteAddr
126+
}
127+
128+
if isContainedIn(config.TrustedOrigins, getRemoteAddress(remoteAddr)) {
129+
if host := r.Header.Get(httpapi.XForwardedHostHeader); host != "" {
130+
return host
131+
}
132+
}
133+
134+
return r.Host
135+
}
136+
108137
// EnsureXForwardedForHeader ensures that the request has an X-Forwarded-For
109138
// header. It uses the following logic:
110139
//

‎coderd/httpmw/realip_test.go‎

Lines changed: 107 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111

1212
"github.com/stretchr/testify/require"
1313

14+
"github.com/coder/coder/v2/coderd/httpapi"
1415
"github.com/coder/coder/v2/coderd/httpmw"
1516
)
1617

@@ -472,6 +473,112 @@ func TestFilterUntrusted(t *testing.T) {
472473
}
473474
}
474475

476+
func TestEffectiveHost(t *testing.T) {
477+
t.Parallel()
478+
479+
cidr32 := func(t *testing.T, ip string) *net.IPNet {
480+
t.Helper()
481+
482+
return &net.IPNet{
483+
IP: net.ParseIP(ip),
484+
Mask: net.CIDRMask(32, 32),
485+
}
486+
}
487+
488+
t.Run("UntrustedPeerFallsBackToReceivedHost", func(t *testing.T) {
489+
t.Parallel()
490+
491+
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
492+
r.RemoteAddr = "17.18.19.20:1234"
493+
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
494+
495+
require.Equal(t, "received.test", httpmw.EffectiveHost(nil, r))
496+
})
497+
498+
t.Run("TrustedPeerUsesOriginalRemoteAddrForTrust", func(t *testing.T) {
499+
t.Parallel()
500+
501+
config := &httpmw.RealIPConfig{
502+
TrustedOrigins: []*net.IPNet{cidr32(t, "17.18.19.20")},
503+
TrustedHeaders: []string{"X-Real-Ip"},
504+
}
505+
506+
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
507+
r.RemoteAddr = "17.18.19.20:1234"
508+
// X-Real-Ip causes ExtractRealIP to rewrite r.RemoteAddr, so
509+
// this test can verify trust still uses OriginalRemoteAddr,
510+
// the actual socket peer.
511+
r.Header.Set("X-Real-Ip", "99.88.77.66")
512+
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
513+
514+
middleware := httpmw.ExtractRealIP(config)
515+
next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
516+
require.Equal(t, "99.88.77.66", r.RemoteAddr)
517+
require.Equal(t, "app.test.coder.com", httpmw.EffectiveHost(config, r))
518+
})
519+
520+
middleware(next).ServeHTTP(httptest.NewRecorder(), r)
521+
})
522+
523+
t.Run("UntrustedPeerDoesNotHonorForwardedHost", func(t *testing.T) {
524+
t.Parallel()
525+
526+
config := &httpmw.RealIPConfig{
527+
TrustedOrigins: []*net.IPNet{cidr32(t, "99.88.77.66")},
528+
TrustedHeaders: []string{"X-Real-Ip"},
529+
}
530+
531+
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
532+
r.RemoteAddr = "17.18.19.20:1234"
533+
r.Header.Set("X-Real-Ip", "99.88.77.66")
534+
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
535+
536+
middleware := httpmw.ExtractRealIP(config)
537+
nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
538+
require.Equal(t, "17.18.19.20", r.RemoteAddr)
539+
require.Equal(t, "received.test", httpmw.EffectiveHost(config, r))
540+
})
541+
542+
middleware(nextHandler).ServeHTTP(httptest.NewRecorder(), r)
543+
})
544+
545+
t.Run("TrustedPeerWithoutForwardedHostFallsBackToReceivedHost", func(t *testing.T) {
546+
t.Parallel()
547+
548+
config := &httpmw.RealIPConfig{
549+
TrustedOrigins: []*net.IPNet{cidr32(t, "17.18.19.20")},
550+
TrustedHeaders: []string{"X-Real-Ip"},
551+
}
552+
553+
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
554+
r.RemoteAddr = "17.18.19.20:1234"
555+
556+
middleware := httpmw.ExtractRealIP(config)
557+
nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
558+
require.Equal(t, "received.test", httpmw.EffectiveHost(config, r))
559+
})
560+
561+
middleware(nextHandler).ServeHTTP(httptest.NewRecorder(), r)
562+
})
563+
564+
t.Run("MalformedRemoteAddrFallsBackToReceivedHost", func(t *testing.T) {
565+
t.Parallel()
566+
567+
config := &httpmw.RealIPConfig{
568+
TrustedOrigins: []*net.IPNet{cidr32(t, "17.18.19.20")},
569+
TrustedHeaders: []string{"X-Real-Ip"},
570+
}
571+
572+
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
573+
// A RemoteAddr that cannot be parsed into an IP must be treated as
574+
// untrusted, so the forwarded host is ignored.
575+
r.RemoteAddr = "garbage"
576+
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
577+
578+
require.Equal(t, "received.test", httpmw.EffectiveHost(config, r))
579+
})
580+
}
581+
475582
// TestApplicationProxy checks headers passed to DevURL services are as expected.
476583
func TestApplicationProxy(t *testing.T) {
477584
t.Parallel()

‎coderd/workspaceapps/proxy.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,7 @@ func (s *Server) HandleSubdomain(middlewares ...func(http.Handler) http.Handler)
417417
}
418418

419419
// Step 2: Get the request Host.
420-
host := httpapi.RequestHost(r)
420+
host := httpmw.EffectiveHost(s.RealIPConfig, r)
421421
if host == "" {
422422
if r.URL.Path == "/derp" {
423423
// The /derp endpoint is used by wireguard clients to tunnel

0 commit comments

Comments
 (0)