mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix!: only trust x-forwarded-host from configured trusted proxies (#26204)
Subdomain app routing derived the app identity from httpapi.RequestHost, which returned the client-supplied X-Forwarded-Host header verbatim. No middleware validated or stripped that header, so a request from an untrusted peer could forge it. Since the application_connect cookie is scoped to the wildcard apps domain, JavaScript in a share=authenticated app could fetch() with a forged X-Forwarded-Host pointing at a victim's owner-only app; coderd routed and authorized the request as the victim and returned the private app response same-origin to the attacker. 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. This ties host trust to the same RealIPConfig model already used for X-Forwarded-For and -Proto. Wire it into HandleSubdomain for both coderd and wsproxy, and log both the effective host and the raw received_host. Add coverage: EffectiveHost unit tests assert the trust decision uses the socket peer rather than the spoofable forwarded client IP, and a HandleSubdomain test confirms a forged X-Forwarded-Host from an untrusted peer never reaches token resolution. Refs: https://linear.app/codercom/issue/PLAT-259
This commit is contained in:
+3
-1
@@ -1012,7 +1012,9 @@ func New(options *Options) *API {
|
||||
tracing.Middleware(api.TracerProvider),
|
||||
httpmw.AttachRequestID,
|
||||
httpmw.ExtractRealIP(api.RealIPConfig),
|
||||
loggermw.Logger(api.Logger),
|
||||
loggermw.Logger(api.Logger, func(r *http.Request) string {
|
||||
return httpmw.EffectiveHost(api.RealIPConfig, r)
|
||||
}),
|
||||
singleSlashMW,
|
||||
rolestore.CustomRoleMW,
|
||||
// Validate API key on every request (if present) and store
|
||||
|
||||
@@ -8,17 +8,6 @@ const (
|
||||
XForwardedHostHeader = "X-Forwarded-Host"
|
||||
)
|
||||
|
||||
// RequestHost returns the name of the host from the request. It prioritizes
|
||||
// 'X-Forwarded-Host' over r.Host since most requests are being proxied.
|
||||
func RequestHost(r *http.Request) string {
|
||||
host := r.Header.Get(XForwardedHostHeader)
|
||||
if host != "" {
|
||||
return host
|
||||
}
|
||||
|
||||
return r.Host
|
||||
}
|
||||
|
||||
func IsWebsocketUpgrade(r *http.Request) bool {
|
||||
vs := r.Header.Values("Upgrade")
|
||||
for _, v := range vs {
|
||||
|
||||
@@ -12,7 +12,6 @@ import (
|
||||
"github.com/go-chi/chi/v5"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"github.com/coder/coder/v2/coderd/httpapi"
|
||||
"github.com/coder/coder/v2/coderd/tracing"
|
||||
)
|
||||
|
||||
@@ -69,7 +68,7 @@ func safeQueryParams(params url.Values) []slog.Field {
|
||||
return fields
|
||||
}
|
||||
|
||||
func Logger(log slog.Logger) func(next http.Handler) http.Handler {
|
||||
func Logger(log slog.Logger, hostResolver func(*http.Request) string) func(next http.Handler) http.Handler {
|
||||
return func(next http.Handler) http.Handler {
|
||||
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
|
||||
start := time.Now()
|
||||
@@ -79,9 +78,15 @@ func Logger(log slog.Logger) func(next http.Handler) http.Handler {
|
||||
panic(fmt.Sprintf("ResponseWriter not a *tracing.StatusWriter; got %T", rw))
|
||||
}
|
||||
|
||||
host := r.Host
|
||||
if hostResolver != nil {
|
||||
host = hostResolver(r)
|
||||
}
|
||||
|
||||
httplog := log.With(
|
||||
slog.F("user_agent", r.Header.Get("User-Agent")),
|
||||
slog.F("host", httpapi.RequestHost(r)),
|
||||
slog.F("host", host),
|
||||
slog.F("received_host", r.Host),
|
||||
slog.F("path", r.URL.Path),
|
||||
slog.F("proto", r.Proto),
|
||||
slog.F("remote_addr", r.RemoteAddr),
|
||||
|
||||
@@ -68,7 +68,7 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) {
|
||||
})
|
||||
|
||||
// Wrap the test handler with the Logger middleware
|
||||
loggerMiddleware := Logger(logger)
|
||||
loggerMiddleware := Logger(logger, nil)
|
||||
wrappedHandler := loggerMiddleware(testHandler)
|
||||
|
||||
// Create a test HTTP request
|
||||
@@ -91,7 +91,7 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) {
|
||||
}
|
||||
|
||||
// Check that the log contains the expected fields
|
||||
requiredFields := []string{"host", "path", "proto", "remote_addr", "start", "took", "status_code", "user_agent", "latency_ms"}
|
||||
requiredFields := []string{"host", "received_host", "path", "proto", "remote_addr", "start", "took", "status_code", "user_agent", "latency_ms"}
|
||||
for _, field := range requiredFields {
|
||||
_, exists := fieldsMap[field]
|
||||
require.True(t, exists, "field %q is missing in log fields", field)
|
||||
@@ -103,6 +103,38 @@ func TestLoggerMiddleware_SingleRequest(t *testing.T) {
|
||||
require.Equal(t, fieldsMap["status_code"], http.StatusOK)
|
||||
}
|
||||
|
||||
func TestLoggerMiddleware_HostFields(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
sink := testutil.NewFakeSink(t)
|
||||
logger := sink.Logger()
|
||||
|
||||
testHandler := http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
|
||||
rw.WriteHeader(http.StatusOK)
|
||||
})
|
||||
|
||||
loggerMiddleware := Logger(logger, func(_ *http.Request) string {
|
||||
return "effective.test"
|
||||
})
|
||||
wrappedHandler := loggerMiddleware(testHandler)
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, "http://received.test/path", nil)
|
||||
|
||||
sw := &tracing.StatusWriter{ResponseWriter: httptest.NewRecorder()}
|
||||
wrappedHandler.ServeHTTP(sw, req)
|
||||
|
||||
entries := sink.Entries()
|
||||
require.Len(t, entries, 1, "expected exactly one log entry")
|
||||
|
||||
fieldsMap := make(map[string]any)
|
||||
for _, field := range entries[0].Fields {
|
||||
fieldsMap[field.Name] = field.Value
|
||||
}
|
||||
|
||||
require.Equal(t, "effective.test", fieldsMap["host"])
|
||||
require.Equal(t, "received.test", fieldsMap["received_host"])
|
||||
}
|
||||
|
||||
func TestLoggerMiddleware_WebSocket(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitShort)
|
||||
@@ -129,7 +161,7 @@ func TestLoggerMiddleware_WebSocket(t *testing.T) {
|
||||
})
|
||||
|
||||
// Wrap the test handler with the Logger middleware
|
||||
loggerMiddleware := Logger(logger)
|
||||
loggerMiddleware := Logger(logger, nil)
|
||||
wrappedHandler := loggerMiddleware(testHandler)
|
||||
|
||||
// RequestLogger expects the ResponseWriter to be *tracing.StatusWriter
|
||||
@@ -186,7 +218,7 @@ func TestRequestLogger_HTTPRouteParams(t *testing.T) {
|
||||
})
|
||||
|
||||
// Wrap the test handler with the Logger middleware
|
||||
loggerMiddleware := Logger(logger)
|
||||
loggerMiddleware := Logger(logger, nil)
|
||||
wrappedHandler := loggerMiddleware(testHandler)
|
||||
|
||||
// Create a test HTTP request
|
||||
|
||||
@@ -105,6 +105,35 @@ func FilterUntrustedOriginHeaders(config *RealIPConfig, req *http.Request) {
|
||||
}
|
||||
}
|
||||
|
||||
// EffectiveHost returns the host Coder should trust for request handling.
|
||||
// It uses X-Forwarded-Host only when the immediate peer is a configured
|
||||
// trusted proxy. Otherwise it uses the received Host header.
|
||||
func EffectiveHost(config *RealIPConfig, r *http.Request) string {
|
||||
if config == nil {
|
||||
config = &RealIPConfig{
|
||||
TrustedOrigins: nil,
|
||||
TrustedHeaders: nil,
|
||||
}
|
||||
}
|
||||
|
||||
// When ExtractRealIP has run, r.RemoteAddr may hold the forwarded
|
||||
// client IP, and we should use the original socket peer for proxy
|
||||
// trust decisions.
|
||||
remoteAddr := r.RemoteAddr
|
||||
state := RealIP(r.Context())
|
||||
if state != nil && state.OriginalRemoteAddr != "" {
|
||||
remoteAddr = state.OriginalRemoteAddr
|
||||
}
|
||||
|
||||
if isContainedIn(config.TrustedOrigins, getRemoteAddress(remoteAddr)) {
|
||||
if host := r.Header.Get(httpapi.XForwardedHostHeader); host != "" {
|
||||
return host
|
||||
}
|
||||
}
|
||||
|
||||
return r.Host
|
||||
}
|
||||
|
||||
// EnsureXForwardedForHeader ensures that the request has an X-Forwarded-For
|
||||
// header. It uses the following logic:
|
||||
//
|
||||
|
||||
@@ -11,6 +11,7 @@ import (
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
"github.com/coder/coder/v2/coderd/httpapi"
|
||||
"github.com/coder/coder/v2/coderd/httpmw"
|
||||
)
|
||||
|
||||
@@ -472,6 +473,112 @@ func TestFilterUntrusted(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestEffectiveHost(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
cidr32 := func(t *testing.T, ip string) *net.IPNet {
|
||||
t.Helper()
|
||||
|
||||
return &net.IPNet{
|
||||
IP: net.ParseIP(ip),
|
||||
Mask: net.CIDRMask(32, 32),
|
||||
}
|
||||
}
|
||||
|
||||
t.Run("UntrustedPeerFallsBackToReceivedHost", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
|
||||
r.RemoteAddr = "17.18.19.20:1234"
|
||||
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
|
||||
|
||||
require.Equal(t, "received.test", httpmw.EffectiveHost(nil, r))
|
||||
})
|
||||
|
||||
t.Run("TrustedPeerUsesOriginalRemoteAddrForTrust", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
config := &httpmw.RealIPConfig{
|
||||
TrustedOrigins: []*net.IPNet{cidr32(t, "17.18.19.20")},
|
||||
TrustedHeaders: []string{"X-Real-Ip"},
|
||||
}
|
||||
|
||||
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
|
||||
r.RemoteAddr = "17.18.19.20:1234"
|
||||
// X-Real-Ip causes ExtractRealIP to rewrite r.RemoteAddr, so
|
||||
// this test can verify trust still uses OriginalRemoteAddr,
|
||||
// the actual socket peer.
|
||||
r.Header.Set("X-Real-Ip", "99.88.77.66")
|
||||
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
|
||||
|
||||
middleware := httpmw.ExtractRealIP(config)
|
||||
next := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
|
||||
require.Equal(t, "99.88.77.66", r.RemoteAddr)
|
||||
require.Equal(t, "app.test.coder.com", httpmw.EffectiveHost(config, r))
|
||||
})
|
||||
|
||||
middleware(next).ServeHTTP(httptest.NewRecorder(), r)
|
||||
})
|
||||
|
||||
t.Run("UntrustedPeerDoesNotHonorForwardedHost", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
config := &httpmw.RealIPConfig{
|
||||
TrustedOrigins: []*net.IPNet{cidr32(t, "99.88.77.66")},
|
||||
TrustedHeaders: []string{"X-Real-Ip"},
|
||||
}
|
||||
|
||||
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
|
||||
r.RemoteAddr = "17.18.19.20:1234"
|
||||
r.Header.Set("X-Real-Ip", "99.88.77.66")
|
||||
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
|
||||
|
||||
middleware := httpmw.ExtractRealIP(config)
|
||||
nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
|
||||
require.Equal(t, "17.18.19.20", r.RemoteAddr)
|
||||
require.Equal(t, "received.test", httpmw.EffectiveHost(config, r))
|
||||
})
|
||||
|
||||
middleware(nextHandler).ServeHTTP(httptest.NewRecorder(), r)
|
||||
})
|
||||
|
||||
t.Run("TrustedPeerWithoutForwardedHostFallsBackToReceivedHost", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
config := &httpmw.RealIPConfig{
|
||||
TrustedOrigins: []*net.IPNet{cidr32(t, "17.18.19.20")},
|
||||
TrustedHeaders: []string{"X-Real-Ip"},
|
||||
}
|
||||
|
||||
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
|
||||
r.RemoteAddr = "17.18.19.20:1234"
|
||||
|
||||
middleware := httpmw.ExtractRealIP(config)
|
||||
nextHandler := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
|
||||
require.Equal(t, "received.test", httpmw.EffectiveHost(config, r))
|
||||
})
|
||||
|
||||
middleware(nextHandler).ServeHTTP(httptest.NewRecorder(), r)
|
||||
})
|
||||
|
||||
t.Run("MalformedRemoteAddrFallsBackToReceivedHost", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
config := &httpmw.RealIPConfig{
|
||||
TrustedOrigins: []*net.IPNet{cidr32(t, "17.18.19.20")},
|
||||
TrustedHeaders: []string{"X-Real-Ip"},
|
||||
}
|
||||
|
||||
r := httptest.NewRequest(http.MethodGet, "http://received.test", nil)
|
||||
// A RemoteAddr that cannot be parsed into an IP must be treated as
|
||||
// untrusted, so the forwarded host is ignored.
|
||||
r.RemoteAddr = "garbage"
|
||||
r.Header.Set(httpapi.XForwardedHostHeader, "app.test.coder.com")
|
||||
|
||||
require.Equal(t, "received.test", httpmw.EffectiveHost(config, r))
|
||||
})
|
||||
}
|
||||
|
||||
// TestApplicationProxy checks headers passed to DevURL services are as expected.
|
||||
func TestApplicationProxy(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
@@ -438,7 +438,7 @@ func (s *Server) HandleSubdomain(middlewares ...func(http.Handler) http.Handler)
|
||||
}
|
||||
|
||||
// Step 2: Get the request Host.
|
||||
host := httpapi.RequestHost(r)
|
||||
host := httpmw.EffectiveHost(s.RealIPConfig, r)
|
||||
if host == "" {
|
||||
if r.URL.Path == "/derp" {
|
||||
// The /derp endpoint is used by wireguard clients to tunnel
|
||||
|
||||
@@ -1,3 +1,90 @@
|
||||
package workspaceapps_test
|
||||
|
||||
// App tests can be found in the apptest package.
|
||||
|
||||
import (
|
||||
"context"
|
||||
"net"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
"github.com/coder/coder/v2/coderd/httpapi"
|
||||
"github.com/coder/coder/v2/coderd/httpmw"
|
||||
"github.com/coder/coder/v2/coderd/workspaceapps"
|
||||
"github.com/coder/coder/v2/coderd/workspaceapps/appurl"
|
||||
"github.com/coder/coder/v2/testutil"
|
||||
)
|
||||
|
||||
type fakeSignedTokenProvider struct {
|
||||
fromRequestCalls int
|
||||
issueCalls int
|
||||
}
|
||||
|
||||
func (s *fakeSignedTokenProvider) FromRequest(_ *http.Request) (*workspaceapps.SignedToken, bool) {
|
||||
s.fromRequestCalls++
|
||||
return nil, false
|
||||
}
|
||||
|
||||
func (s *fakeSignedTokenProvider) Issue(_ context.Context, _ http.ResponseWriter, _ *http.Request, _ workspaceapps.IssueTokenRequest) (*workspaceapps.SignedToken, string, bool) {
|
||||
s.issueCalls++
|
||||
return nil, "", false
|
||||
}
|
||||
|
||||
func TestHandleSubdomain_IgnoresUntrustedForwardedHost(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
hostnamePattern := "*--apps.test.coder.com"
|
||||
hostnameRegex, err := appurl.CompileHostnamePattern(hostnamePattern)
|
||||
require.NoError(t, err)
|
||||
|
||||
dashboardURL, err := url.Parse("https://dashboard.test.coder.com")
|
||||
require.NoError(t, err)
|
||||
|
||||
provider := &fakeSignedTokenProvider{}
|
||||
srv := workspaceapps.NewServer(workspaceapps.ServerOptions{
|
||||
Logger: testutil.Logger(t),
|
||||
DashboardURL: dashboardURL,
|
||||
AccessURL: dashboardURL,
|
||||
Hostname: hostnamePattern,
|
||||
HostnameRegex: hostnameRegex,
|
||||
RealIPConfig: &httpmw.RealIPConfig{
|
||||
TrustedOrigins: []*net.IPNet{{
|
||||
IP: net.ParseIP("10.0.0.1"),
|
||||
Mask: net.CIDRMask(32, 32),
|
||||
}},
|
||||
},
|
||||
SignedTokenProvider: provider,
|
||||
})
|
||||
|
||||
forgedHost := appurl.ApplicationURL{
|
||||
AppSlugOrPort: "app",
|
||||
WorkspaceName: "workspace",
|
||||
Username: "victim",
|
||||
}.String() + "--apps.test.coder.com"
|
||||
|
||||
nextCalled := false
|
||||
next := http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) {
|
||||
nextCalled = true
|
||||
})
|
||||
|
||||
// Given: a request with a forged X-Forwarded-Host set to a valid
|
||||
// app hostname, and an immediate peer outside the trusted proxy
|
||||
// config.
|
||||
req := httptest.NewRequest(http.MethodGet, "https://dashboard.test.coder.com/", nil)
|
||||
req.Header.Set(httpapi.XForwardedHostHeader, forgedHost)
|
||||
req.RemoteAddr = "17.18.19.20:1234"
|
||||
|
||||
// When: HandleSubdomain runs.
|
||||
srv.HandleSubdomain()(next).ServeHTTP(httptest.NewRecorder(), req)
|
||||
|
||||
// Then: it ignores untrusted X-Forwarded-Host, so the received
|
||||
// dashboard host is used, the request falls through to the next
|
||||
// handler, and the signed app token provider is never called.
|
||||
require.True(t, nextCalled)
|
||||
require.Zero(t, provider.fromRequestCalls)
|
||||
require.Zero(t, provider.issueCalls)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user