diff --git a/middleware.go b/middleware.go index ac872c6..176bb09 100644 --- a/middleware.go +++ b/middleware.go @@ -99,11 +99,23 @@ func (t *TraefikOidc) applyBypassUserHeaders(req *http.Request, reason string) b return false } - req.Header.Set("X-Forwarded-User", userIdentifier) - if !t.minimalHeaders { - req.Header.Set("X-Auth-Request-User", userIdentifier) + // Sanitize the claim-derived identifier before it is injected as a + // header, matching forwardAuthorized (which uses safeIdentifier). The + // value originates from an IdP-controlled claim and can contain CRLF + // or delimiter characters that would otherwise inject or confuse + // downstream header parsing. On failure, drop the header but still + // honor the bypass (the identity headers are decoration here). + safeIdentifier, ok := sanitizeHeaderClaimValue(userIdentifier, t.headerClaimMaxLen()) + if !ok { + t.logger.Debugf("%s bypass: dropping unsafe user-identifier header: %s", reason, headerClaimValueReason(userIdentifier, t.headerClaimMaxLen())) + return true } - t.logger.Debugf("%s bypass: forwarded user %s from session", reason, userIdentifier) + + req.Header.Set("X-Forwarded-User", safeIdentifier) + if !t.minimalHeaders { + req.Header.Set("X-Auth-Request-User", safeIdentifier) + } + t.logger.Debugf("%s bypass: forwarded user %s from session", reason, safeIdentifier) return true } diff --git a/review_r48_regression_test.go b/review_r48_regression_test.go new file mode 100644 index 0000000..56d1303 --- /dev/null +++ b/review_r48_regression_test.go @@ -0,0 +1,92 @@ +package traefikoidc + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// TestApplyBypassUserHeaders_SanitizesIdentifier is a regression test for the +// auth-bypass header path (applyBypassUserHeaders). The claim-derived user +// identifier is injected into X-Forwarded-User / X-Auth-Request-User without +// sanitization, unlike forwardAuthorized which uses sanitizeHeaderClaimValue. +// An IdP-controlled identifier containing CRLF (or a delimiter) would +// otherwise inject or confuse downstream header parsing. Now the bypass path +// sanitizes the identifier and drops the header on failure. +func TestApplyBypassUserHeaders_SanitizesIdentifier(t *testing.T) { + sm := createTestSessionManager(t) + + base := httptest.NewRequest(http.MethodGet, "/protected", nil) + baseRec := httptest.NewRecorder() + + // Build an authenticated session whose user identifier contains CRLF. + session, err := sm.GetSession(base) + if err != nil { + t.Fatalf("GetSession: %v", err) + } + if err := session.SetAuthenticated(true); err != nil { + t.Fatalf("SetAuthenticated: %v", err) + } + session.SetUserIdentifier("user\nX-Injected: yes") + if err := session.Save(base, baseRec); err != nil { + t.Fatalf("Save: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + for _, c := range baseRec.Result().Cookies() { + req.AddCookie(c) + } + + oidc := &TraefikOidc{ + sessionManager: sm, + logger: NewLogger("debug"), + } + + if ok := oidc.applyBypassUserHeaders(req, "test"); !ok { + t.Fatalf("applyBypassUserHeaders returned false; bypass should be honored") + } + if v := req.Header.Get("X-Forwarded-User"); v != "" { + t.Fatalf("X-Forwarded-User = %q, want empty (unsafe identifier dropped)", v) + } + if v := req.Header.Get("X-Auth-Request-User"); v != "" { + t.Fatalf("X-Auth-Request-User = %q, want empty (unsafe identifier dropped)", v) + } +} + +// TestApplyBypassUserHeaders_KeepsCleanIdentifier ensures a normal identifier +// still flows through, and is trimmed/sanitized. +func TestApplyBypassUserHeaders_KeepsCleanIdentifier(t *testing.T) { + sm := createTestSessionManager(t) + + base := httptest.NewRequest(http.MethodGet, "/protected", nil) + baseRec := httptest.NewRecorder() + + session, err := sm.GetSession(base) + if err != nil { + t.Fatalf("GetSession: %v", err) + } + if err := session.SetAuthenticated(true); err != nil { + t.Fatalf("SetAuthenticated: %v", err) + } + session.SetUserIdentifier(" user@company.com ") + if err := session.Save(base, baseRec); err != nil { + t.Fatalf("Save: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/protected", nil) + for _, c := range baseRec.Result().Cookies() { + req.AddCookie(c) + } + + oidc := &TraefikOidc{ + sessionManager: sm, + logger: NewLogger("debug"), + } + + if ok := oidc.applyBypassUserHeaders(req, "test"); !ok { + t.Fatalf("applyBypassUserHeaders returned false; bypass should be honored") + } + if v := req.Header.Get("X-Forwarded-User"); v != "user@company.com" { + t.Fatalf("X-Forwarded-User = %q, want %q (trimmed)", v, "user@company.com") + } +}