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

fix: hide internal server error details from users

Author: sagikazarmark

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