From 9b7f53bdda857cfccf6707bbcbf8111db3cefcb6 Mon Sep 17 00:00:00 2001 From: Mathias Date: Fri, 3 Jul 2026 22:57:24 +0200 Subject: [PATCH] feat(auth): document caller header precedence + warn on conflict (#10) CallerMiddleware silently preferred X-Auth-Request-User over X-Forwarded-User with no explanation and no signal when both were set. Documented the precedence (X-Auth-Request-User is the verified OIDC identity oauth2-proxy sets, so it is authoritative; X-Forwarded-User is a fallback), and it now takes a *slog.Logger and warns when both headers are present and disagree, so a proxy misconfiguration is visible instead of silently resolved. Table-driven tests cover precedence (both/single/none) and the conflict-warning path. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/gitea-mcp/main.go | 2 +- internal/auth/caller.go | 29 +++++++++++-- internal/auth/caller_test.go | 74 +++++++++++++++++++++++++++----- internal/tools/pr_create_test.go | 2 +- 4 files changed, 92 insertions(+), 15 deletions(-) diff --git a/cmd/gitea-mcp/main.go b/cmd/gitea-mcp/main.go index 9594c6b..ecf9998 100644 --- a/cmd/gitea-mcp/main.go +++ b/cmd/gitea-mcp/main.go @@ -56,7 +56,7 @@ func main() { mux := http.NewServeMux() mux.Handle("/mcp", mcp.OriginAllowlist(cfg.OriginAllowlist)( chassisauth.BearerMiddleware(cfg.StaticToken, jwtValidator, "gitea", resourceMetadataURL, - auth.CallerMiddleware(mcpSrv), + auth.CallerMiddleware(logger, mcpSrv), ), )) mux.Handle("/healthz", newHealthzHandler(cfg.DexIssuerURL != "", jwtValidator != nil, jwtInitErr)) diff --git a/internal/auth/caller.go b/internal/auth/caller.go index bcffed3..77a2005 100644 --- a/internal/auth/caller.go +++ b/internal/auth/caller.go @@ -2,17 +2,40 @@ package auth import ( "context" + "log/slog" "net/http" ) type ctxKey struct{} -func CallerMiddleware(next http.Handler) http.Handler { +// CallerMiddleware extracts the authenticated username from the reverse-proxy +// identity headers and stashes it in the request context for Caller(). +// +// Header precedence: X-Auth-Request-User takes priority over X-Forwarded-User. +// X-Auth-Request-User is the header oauth2-proxy sets from the *verified* OIDC +// identity, so it is authoritative. X-Forwarded-User is a weaker, proxy-set +// convention some setups populate instead; it is used only as a fallback when +// X-Auth-Request-User is absent. If a proxy sets BOTH and they disagree, the +// verified X-Auth-Request-User still wins and we log a warning so the +// misconfiguration is visible rather than silently resolved (#10). +// +// logger may be nil, in which case the conflict warning is skipped. +func CallerMiddleware(logger *slog.Logger, next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - user := r.Header.Get("X-Auth-Request-User") + authUser := r.Header.Get("X-Auth-Request-User") + fwdUser := r.Header.Get("X-Forwarded-User") + + user := authUser if user == "" { - user = r.Header.Get("X-Forwarded-User") + user = fwdUser } + + if logger != nil && authUser != "" && fwdUser != "" && authUser != fwdUser { + logger.Warn("conflicting caller identity headers; using X-Auth-Request-User", + "x_auth_request_user", authUser, + "x_forwarded_user", fwdUser) + } + ctx := context.WithValue(r.Context(), ctxKey{}, user) next.ServeHTTP(w, r.WithContext(ctx)) }) diff --git a/internal/auth/caller_test.go b/internal/auth/caller_test.go index c4a70ee..ac4df88 100644 --- a/internal/auth/caller_test.go +++ b/internal/auth/caller_test.go @@ -1,7 +1,9 @@ package auth_test import ( + "bytes" "context" + "log/slog" "net/http" "net/http/httptest" "testing" @@ -10,17 +12,69 @@ import ( "github.com/stretchr/testify/assert" ) -func TestCallerFromContext(t *testing.T) { - called := false - h := auth.CallerMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { - called = true - assert.Equal(t, "mathiasbq", auth.Caller(r.Context())) - })) +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(bytes.NewBuffer(nil), nil)) +} + +// Header precedence: X-Auth-Request-User (verified OIDC identity) wins over +// X-Forwarded-User, and X-Forwarded-User is only a fallback when the former is +// absent. +func TestCallerHeaderPrecedence(t *testing.T) { + tests := []struct { + name string + authReq string + forwarded string + wantCaller string + }{ + {"auth-request only", "mathiasbq", "", "mathiasbq"}, + {"forwarded fallback", "", "fwduser", "fwduser"}, + {"both present, same", "same", "same", "same"}, + {"both present, differ → auth-request wins", "authuser", "fwduser", "authuser"}, + {"neither", "", "", ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var got string + h := auth.CallerMiddleware(discardLogger(), http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + got = auth.Caller(r.Context()) + })) + req := httptest.NewRequest(http.MethodPost, "/", nil) + if tc.authReq != "" { + req.Header.Set("X-Auth-Request-User", tc.authReq) + } + if tc.forwarded != "" { + req.Header.Set("X-Forwarded-User", tc.forwarded) + } + h.ServeHTTP(httptest.NewRecorder(), req) + assert.Equal(t, tc.wantCaller, got) + }) + } +} + +// When both headers are present and disagree, a warning is logged so the proxy +// misconfiguration is visible rather than silent. +func TestCallerConflictingHeadersLogsWarning(t *testing.T) { + var buf bytes.Buffer + logger := slog.New(slog.NewJSONHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn})) + + h := auth.CallerMiddleware(logger, http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {})) req := httptest.NewRequest(http.MethodPost, "/", nil) - req.Header.Set("X-Auth-Request-User", "mathiasbq") - rr := httptest.NewRecorder() - h.ServeHTTP(rr, req) - assert.True(t, called) + req.Header.Set("X-Auth-Request-User", "authuser") + req.Header.Set("X-Forwarded-User", "fwduser") + h.ServeHTTP(httptest.NewRecorder(), req) + + logged := buf.String() + assert.Contains(t, logged, "conflicting") + assert.Contains(t, logged, "authuser") + assert.Contains(t, logged, "fwduser") + + // No warning when they agree. + buf.Reset() + req2 := httptest.NewRequest(http.MethodPost, "/", nil) + req2.Header.Set("X-Auth-Request-User", "same") + req2.Header.Set("X-Forwarded-User", "same") + h.ServeHTTP(httptest.NewRecorder(), req2) + assert.Empty(t, buf.String(), "no warning expected when headers agree") } func TestCallerEmptyWhenHeaderMissing(t *testing.T) { diff --git a/internal/tools/pr_create_test.go b/internal/tools/pr_create_test.go index 8afc328..1d42d0c 100644 --- a/internal/tools/pr_create_test.go +++ b/internal/tools/pr_create_test.go @@ -30,7 +30,7 @@ const prFixture = `{ func callerContext(user string) context.Context { var capturedCtx context.Context - h := auth.CallerMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + h := auth.CallerMiddleware(nil, http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { capturedCtx = r.Context() })) req := httptest.NewRequest("POST", "/", nil)