Merge pull request #4457 from Jabejixo/fix/hide-internal-500-error-details

fix: hide internal server error details from users

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
 	}
 }

server/deviceflowhandlers_test.go

@@ -222,8 +222,9 @@ func TestDeviceCallback(t *testing.T) {
 				code:  "somecode",
 				error: "Error Condition",
 			},
-			expectedResponseCode:   http.StatusBadRequest,
-			expectedServerResponse: "Error Condition: \n",
+			expectedResponseCode: http.StatusBadRequest,
+			// Note: Error details should NOT be displayed to user anymore.
+			// Instead, a safe generic message is shown.
 		},
 		{
 			testName: "Expired Auth Code",
@@ -352,8 +353,9 @@ func TestDeviceCallback(t *testing.T) {
 				code:  "somecode",
 				error: "<script>console.log(window);</script>",
 			},
-			expectedResponseCode:   http.StatusBadRequest,
-			expectedServerResponse: "&lt;script&gt;console.log(window);&lt;/script&gt;: \n",
+			expectedResponseCode: http.StatusBadRequest,
+			// Note: XSS data should NOT be displayed to user anymore.
+			// Instead, a safe generic message is shown.
 		},
 	}
 	for _, tc := range tests {
@@ -413,6 +415,29 @@ func TestDeviceCallback(t *testing.T) {
 					t.Errorf("%s: Unexpected Response.  Expected %q got %q", tc.testName, tc.expectedServerResponse, result)
 				}
 			}
+
+			// Special check for error message safety tests
+			if tc.testName == "Prevent cross-site scripting" || tc.testName == "Error During Authorization" {
+				result, _ := io.ReadAll(rr.Body)
+				responseBody := string(result)
+
+				// Error details should NOT be present in the response (for security)
+				if tc.testName == "Prevent cross-site scripting" {
+					if strings.Contains(responseBody, "<script>") || strings.Contains(responseBody, "console.log(window)") {
+						t.Errorf("%s: XSS script found in response, but should be blocked: %q", tc.testName, responseBody)
+					}
+				}
+				if tc.testName == "Error During Authorization" {
+					if strings.Contains(responseBody, "Error Condition") {
+						t.Errorf("%s: Error details found in response, but should be hidden: %q", tc.testName, responseBody)
+					}
+				}
+
+				// Safe message should be present
+				if !strings.Contains(responseBody, "Authorization failed. Please try again.") {
+					t.Errorf("%s: Safe error message not found in response: %q", tc.testName, responseBody)
+				}
+			}
 		})
 	}
 }

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."
+)

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:")
+}

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 {

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")

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)