From debcb5c8f9112fd524995013ed04770009e6d245 Mon Sep 17 00:00:00 2001 From: Ivan Zvyagintsev Date: Tue, 23 Dec 2025 11:42:47 +0300 Subject: [PATCH] fix: hide internal server error details from users Signed-off-by: Ivan Zvyagintsev --- server/deviceflowhandlers.go | 13 +- server/errors.go | 26 +++ server/errors_test.go | 300 +++++++++++++++++++++++++++++++++ server/handlers.go | 15 +- server/introspectionhandler.go | 4 +- server/oauth2.go | 6 +- 6 files changed, 349 insertions(+), 15 deletions(-) create mode 100644 server/errors.go create mode 100644 server/errors_test.go diff --git a/server/deviceflowhandlers.go b/server/deviceflowhandlers.go index 3cbfbf16..ec5fb52b 100644 --- a/server/deviceflowhandlers.go +++ b/server/deviceflowhandlers.go @@ -11,8 +11,6 @@ import ( "strings" "time" - "golang.org/x/net/html" - "github.com/dexidp/dex/storage" ) @@ -300,9 +298,11 @@ func (s *Server) handleDeviceCallback(w http.ResponseWriter, r *http.Request) { // Authorization redirect callback from OAuth2 auth flow. if errMsg := r.FormValue("error"); errMsg != "" { - // escape the message to prevent cross-site scripting - msg := html.EscapeString(errMsg + ": " + r.FormValue("error_description")) - http.Error(w, msg, http.StatusBadRequest) + // Log the error details but don't expose them to the user + s.logger.ErrorContext(r.Context(), "OAuth2 authorization error", + "error", errMsg, + "error_description", r.FormValue("error_description")) + s.renderError(r, w, http.StatusBadRequest, "Authorization failed. Please try again.") return } @@ -392,7 +392,8 @@ func (s *Server) handleDeviceCallback(w http.ResponseWriter, r *http.Request) { } default: - http.Error(w, fmt.Sprintf("method not implemented: %s", r.Method), http.StatusBadRequest) + s.logger.ErrorContext(r.Context(), "unsupported method in device callback", "method", r.Method) + s.renderError(r, w, http.StatusBadRequest, ErrMsgMethodNotAllowed) return } } diff --git a/server/errors.go b/server/errors.go new file mode 100644 index 00000000..c0b9d425 --- /dev/null +++ b/server/errors.go @@ -0,0 +1,26 @@ +package server + +// Safe error messages for user-facing responses. +// These messages are intentionally generic to avoid leaking internal details. +// All actual error details should be logged server-side. + +const ( + // ErrMsgLoginError is a generic login error message shown to users. + // Used when authentication fails due to internal server errors. + ErrMsgLoginError = "Login error. Please contact your administrator or try again later." + + // ErrMsgAuthenticationFailed is shown when callback/SAML authentication fails. + ErrMsgAuthenticationFailed = "Authentication failed. Please contact your administrator or try again later." + + // ErrMsgInternalServerError is a generic internal server error message. + ErrMsgInternalServerError = "Internal server error. Please contact your administrator or try again later." + + // ErrMsgDatabaseError is shown when database operations fail. + ErrMsgDatabaseError = "A database error occurred. Please try again later." + + // ErrMsgInvalidRequest is shown when request parsing fails. + ErrMsgInvalidRequest = "Invalid request. Please try again." + + // ErrMsgMethodNotAllowed is shown when an unsupported HTTP method is used. + ErrMsgMethodNotAllowed = "Method not allowed." +) diff --git a/server/errors_test.go b/server/errors_test.go new file mode 100644 index 00000000..17d2b1f5 --- /dev/null +++ b/server/errors_test.go @@ -0,0 +1,300 @@ +package server + +import ( + "bytes" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestErrorMessagesDoNotLeakInternalDetails verifies that error responses +// do not contain internal error details that could be exploited by attackers. +func TestErrorMessagesDoNotLeakInternalDetails(t *testing.T) { + // List of sensitive patterns that should never appear in user-facing errors + sensitivePatterns := []string{ + "panic", + "runtime error", + "nil pointer", + "stack trace", + "goroutine", + ".go:", // file paths like "server.go:123" + "sql:", // SQL errors + "connection", // Connection errors + "timeout", // Unless it's a user-friendly timeout message + "ECONNREFUSED", + "EOF", + "broken pipe", + } + + tests := []struct { + name string + path string + method string + body string + contentType string + setupFunc func(t *testing.T, s *Server) + checkFunc func(t *testing.T, resp *http.Response, body string) + }{ + { + name: "Invalid authorization request parse error", + path: "/auth", + method: "POST", + body: "invalid%body", + contentType: "application/x-www-form-urlencoded", + checkFunc: func(t *testing.T, resp *http.Response, body string) { + // Should return a safe error message, not the parse error details + for _, pattern := range sensitivePatterns { + require.NotContains(t, body, pattern, + "Response should not contain sensitive pattern: %s", pattern) + } + }, + }, + { + name: "Invalid callback state", + path: "/callback?state=invalid_state", + method: "GET", + checkFunc: func(t *testing.T, resp *http.Response, body string) { + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + // Should not leak storage error details + require.NotContains(t, body, "storage") + require.NotContains(t, body, "not found") + }, + }, + { + name: "Invalid token request", + path: "/token", + method: "POST", + body: "grant_type=authorization_code&code=invalid", + contentType: "application/x-www-form-urlencoded", + checkFunc: func(t *testing.T, resp *http.Response, body string) { + // Token endpoint returns JSON errors which is correct OAuth2 behavior + // Just verify no internal details leak + for _, pattern := range sensitivePatterns { + require.NotContains(t, body, pattern, + "Response should not contain sensitive pattern: %s", pattern) + } + }, + }, + { + name: "Invalid introspection request - no token", + path: "/token/introspect", + method: "POST", + body: "", + contentType: "application/x-www-form-urlencoded", + checkFunc: func(t *testing.T, resp *http.Response, body string) { + for _, pattern := range sensitivePatterns { + require.NotContains(t, body, pattern, + "Response should not contain sensitive pattern: %s", pattern) + } + }, + }, + { + name: "Device flow invalid user code", + path: "/device/auth/verify_code", + method: "POST", + body: "user_code=INVALID", + checkFunc: func(t *testing.T, resp *http.Response, body string) { + for _, pattern := range sensitivePatterns { + require.NotContains(t, body, pattern, + "Response should not contain sensitive pattern: %s", pattern) + } + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + httpServer, s := newTestServer(t, nil) + defer httpServer.Close() + + if tc.setupFunc != nil { + tc.setupFunc(t, s) + } + + var reqBody io.Reader + if tc.body != "" { + reqBody = strings.NewReader(tc.body) + } + + req := httptest.NewRequest(tc.method, tc.path, reqBody) + if tc.contentType != "" { + req.Header.Set("Content-Type", tc.contentType) + } + + rr := httptest.NewRecorder() + s.ServeHTTP(rr, req) + + resp := rr.Result() + defer resp.Body.Close() + + bodyBytes, err := io.ReadAll(resp.Body) + require.NoError(t, err) + body := string(bodyBytes) + + if tc.checkFunc != nil { + tc.checkFunc(t, resp, body) + } + }) + } +} + +// TestLoginErrorMessageIsSafe verifies that the login error page +// shows a safe, user-friendly message. +func TestLoginErrorMessageIsSafe(t *testing.T) { + httpServer, s := newTestServer(t, nil) + defer httpServer.Close() + + // Create a request that will trigger a login error + rr := httptest.NewRecorder() + req := httptest.NewRequest("GET", "/auth/nonexistent/login?state=test", nil) + s.ServeHTTP(rr, req) + + resp := rr.Result() + defer resp.Body.Close() + + body, _ := io.ReadAll(resp.Body) + bodyStr := string(body) + + // Should not contain error stack traces or internal details + require.NotContains(t, bodyStr, "panic") + require.NotContains(t, bodyStr, ".go:") + require.NotContains(t, bodyStr, "goroutine") +} + +// TestCallbackErrorMessageIsSafe verifies that callback errors +// do not leak internal details. +func TestCallbackErrorMessageIsSafe(t *testing.T) { + httpServer, s := newTestServer(t, nil) + defer httpServer.Close() + + // Test OAuth2 callback with invalid state + rr := httptest.NewRecorder() + req := httptest.NewRequest("GET", "/callback?code=test&state=invalid", nil) + s.ServeHTTP(rr, req) + + resp := rr.Result() + defer resp.Body.Close() + + body, _ := io.ReadAll(resp.Body) + bodyStr := string(body) + + // Should not contain storage error details + require.NotContains(t, bodyStr, "storage.ErrNotFound") + require.NotContains(t, bodyStr, "database") +} + +// TestDeviceCallbackMethodError verifies that unsupported methods +// return safe error messages. +func TestDeviceCallbackMethodError(t *testing.T) { + httpServer, s := newTestServer(t, nil) + defer httpServer.Close() + + // Test with unsupported method + rr := httptest.NewRecorder() + req := httptest.NewRequest("PUT", "/device/callback", nil) + s.ServeHTTP(rr, req) + + resp := rr.Result() + defer resp.Body.Close() + + body, _ := io.ReadAll(resp.Body) + bodyStr := string(body) + + // Should not expose the method name in error + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.NotContains(t, bodyStr, "PUT") + require.NotContains(t, bodyStr, "method not implemented") +} + +// TestRenderErrorSafeMessages tests that renderError uses safe messages +func TestRenderErrorSafeMessages(t *testing.T) { + tests := []struct { + name string + statusCode int + message string + expectedInBody []string + notInBody []string + }{ + { + name: "Login error message", + statusCode: http.StatusInternalServerError, + message: ErrMsgLoginError, + expectedInBody: []string{"Login error", "administrator"}, + notInBody: []string{"stack", "panic", ".go:"}, + }, + { + name: "Authentication failed message", + statusCode: http.StatusInternalServerError, + message: ErrMsgAuthenticationFailed, + expectedInBody: []string{"Authentication failed", "administrator"}, + notInBody: []string{"stack", "panic", ".go:"}, + }, + { + name: "Database error message", + statusCode: http.StatusInternalServerError, + message: ErrMsgDatabaseError, + expectedInBody: []string{"database error"}, + notInBody: []string{"sql:", "connection", "timeout"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + httpServer, s := newTestServer(t, nil) + defer httpServer.Close() + + rr := httptest.NewRecorder() + req := httptest.NewRequest("GET", "/", nil) + + s.renderError(req, rr, tc.statusCode, tc.message) + + resp := rr.Result() + defer resp.Body.Close() + + body, _ := io.ReadAll(resp.Body) + bodyStr := string(body) + + require.Equal(t, tc.statusCode, resp.StatusCode) + + for _, expected := range tc.expectedInBody { + require.Contains(t, bodyStr, expected, + "Response should contain: %s", expected) + } + + for _, notExpected := range tc.notInBody { + require.NotContains(t, bodyStr, notExpected, + "Response should not contain: %s", notExpected) + } + }) + } +} + +// TestTokenErrorDoesNotLeakDetails tests that token errors don't leak internal details +func TestTokenErrorDoesNotLeakDetails(t *testing.T) { + httpServer, s := newTestServer(t, nil) + defer httpServer.Close() + + // Create a token request with invalid credentials + body := bytes.NewBufferString("grant_type=authorization_code&code=invalid_code") + req := httptest.NewRequest("POST", "/token", body) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetBasicAuth("invalid_client", "invalid_secret") + + rr := httptest.NewRecorder() + s.ServeHTTP(rr, req) + + resp := rr.Result() + defer resp.Body.Close() + + respBody, _ := io.ReadAll(resp.Body) + bodyStr := string(respBody) + + // Should not contain internal error details + require.NotContains(t, bodyStr, "storage") + require.NotContains(t, bodyStr, "not found") + require.NotContains(t, bodyStr, ".go:") +} diff --git a/server/handlers.go b/server/handlers.go index f8d0ed64..e46c7b8f 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -141,7 +141,7 @@ func (s *Server) handleAuthorization(w http.ResponseWriter, r *http.Request) { if err := r.ParseForm(); err != nil { s.logger.ErrorContext(r.Context(), "failed to parse arguments", "err", err) - s.renderError(r, w, http.StatusBadRequest, err.Error()) + s.renderError(r, w, http.StatusBadRequest, ErrMsgInvalidRequest) return } @@ -374,7 +374,7 @@ func (s *Server) handlePasswordLogin(w http.ResponseWriter, r *http.Request) { identity, ok, err := pwConn.Login(r.Context(), scopes, username, password) if err != nil { s.logger.ErrorContext(r.Context(), "failed to login user", "err", err) - s.renderError(r, w, http.StatusInternalServerError, fmt.Sprintf("Login error: %v", err)) + s.renderError(r, w, http.StatusInternalServerError, ErrMsgLoginError) return } if !ok { @@ -480,7 +480,7 @@ func (s *Server) handleConnectorCallback(w http.ResponseWriter, r *http.Request) if err != nil { s.logger.ErrorContext(r.Context(), "failed to authenticate", "err", err) - s.renderError(r, w, http.StatusInternalServerError, fmt.Sprintf("Failed to authenticate: %v", err)) + s.renderError(r, w, http.StatusInternalServerError, ErrMsgAuthenticationFailed) return } @@ -1102,13 +1102,15 @@ func (s *Server) handleUserInfo(w http.ResponseWriter, r *http.Request) { verifier := oidc.NewVerifier(s.issuerURL.String(), &storageKeySet{s.storage}, &oidc.Config{SkipClientIDCheck: true}) idToken, err := verifier.Verify(ctx, rawIDToken) if err != nil { - s.tokenErrHelper(w, errAccessDenied, err.Error(), http.StatusForbidden) + s.logger.ErrorContext(r.Context(), "failed to verify ID token", "err", err) + s.tokenErrHelper(w, errAccessDenied, "Invalid bearer token.", http.StatusForbidden) return } var claims json.RawMessage if err := idToken.Claims(&claims); err != nil { - s.tokenErrHelper(w, errServerError, err.Error(), http.StatusInternalServerError) + s.logger.ErrorContext(r.Context(), "failed to decode ID token claims", "err", err) + s.tokenErrHelper(w, errServerError, "", http.StatusInternalServerError) return } @@ -1149,7 +1151,8 @@ func (s *Server) handlePasswordGrant(w http.ResponseWriter, r *http.Request, cli isTrusted, err := s.validateCrossClientTrust(ctx, client.ID, peerID) if err != nil { - s.tokenErrHelper(w, errInvalidClient, fmt.Sprintf("Error validating cross client trust %v.", err), http.StatusBadRequest) + s.logger.ErrorContext(r.Context(), "error validating cross client trust", "client_id", client.ID, "peer_id", peerID, "err", err) + s.tokenErrHelper(w, errInvalidClient, "Error validating cross client trust.", http.StatusBadRequest) return } if !isTrusted { diff --git a/server/introspectionhandler.go b/server/introspectionhandler.go index 42ad1b3c..4b0073db 100644 --- a/server/introspectionhandler.go +++ b/server/introspectionhandler.go @@ -318,7 +318,9 @@ func (s *Server) handleIntrospect(w http.ResponseWriter, r *http.Request) { rawJSON, jsonErr := json.Marshal(introspect) if jsonErr != nil { - s.introspectErrHelper(w, errServerError, jsonErr.Error(), 500) + s.logger.ErrorContext(r.Context(), "failed to marshal introspection response", "err", jsonErr) + s.introspectErrHelper(w, errServerError, "", http.StatusInternalServerError) + return } w.Header().Set("Content-Type", "application/json") diff --git a/server/oauth2.go b/server/oauth2.go index bd0b0530..7268bcfd 100644 --- a/server/oauth2.go +++ b/server/oauth2.go @@ -481,14 +481,16 @@ func (s *Server) parseAuthorizationRequest(r *http.Request) (*storage.AuthReques client, err := s.storage.GetClient(ctx, clientID) if err != nil { if err == storage.ErrNotFound { - return nil, newDisplayedErr(http.StatusNotFound, "Invalid client_id (%q).", clientID) + s.logger.ErrorContext(r.Context(), "invalid client_id provided", "client_id", clientID) + return nil, newDisplayedErr(http.StatusNotFound, "Invalid client_id.") } s.logger.ErrorContext(r.Context(), "failed to get client", "err", err) return nil, newDisplayedErr(http.StatusInternalServerError, "Database error.") } if !validateRedirectURI(client, redirectURI) { - return nil, newDisplayedErr(http.StatusBadRequest, "Unregistered redirect_uri (%q).", redirectURI) + s.logger.ErrorContext(r.Context(), "unregistered redirect_uri", "redirect_uri", redirectURI, "client_id", clientID) + return nil, newDisplayedErr(http.StatusBadRequest, "Unregistered redirect_uri.") } if redirectURI == deviceCallbackURI && client.Public { redirectURI = s.absPath(deviceCallbackURI)