Merge b8a981fea9 into 14af2951e5

CHANGELOG.md

@@ -8,6 +8,8 @@
 
 ## Changes since v7.15.3
 
+- [#3453](https://github.com/oauth2-proxy/oauth2-proxy/pull/3453) fix: return 403 instead of 500 for malformed OAuth2 state on the callback (@Elaine-cls)
+
 # V7.15.3
 
 ## Release Highlights

oauthproxy.go

@@ -905,8 +905,15 @@ func (p *OAuthProxy) OAuthCallback(rw http.ResponseWriter, req *http.Request) {
 
 	nonce, appRedirect, err := decodeState(req.Form.Get("state"), p.encodeState)
 	if err != nil {
+		// A malformed or missing state parameter is a client/auth failure, not a
+		// server error: the request did not originate from a valid /oauth2/start
+		// flow (e.g. crawlers and scanners hitting the callback directly). Return
+		// 403 instead of 500 so these do not surface as server errors / trigger
+		// 5xx alerting, leaking minimal information to the caller. This restores
+		// the pre-v7.7.1 behaviour and is consistent with the other auth-failure
+		// paths below (CSRF cookie missing, CSRF mismatch), which also return 403.
 		logger.Errorf("Error while parsing OAuth2 state: %v", err)
-		p.ErrorPage(rw, req, http.StatusInternalServerError, err.Error())
+		p.ErrorPage(rw, req, http.StatusForbidden, err.Error(), "Login Failed: invalid or missing state parameter.")
 		return
 	}
 

oauthproxy_test.go

@@ -2087,6 +2087,43 @@ func Test_noCacheHeaders(t *testing.T) {
 	})
 }
 
+func TestOAuthCallbackMalformedStateIsClientError(t *testing.T) {
+	// A crawler/scanner that hits /oauth2/callback directly sends a missing or
+	// malformed state parameter (no "nonce:redirect" form). This must be
+	// reported as a 4xx auth failure (403), not a 5xx server error, otherwise it
+	// pollutes 5xx-based alerting even though the proxy is healthy.
+	opts := baseTestOptions()
+	err := validation.Validate(opts)
+	require.NoError(t, err)
+
+	proxy, err := NewOAuthProxy(opts, func(string) bool { return true })
+	require.NoError(t, err)
+
+	testCases := []struct {
+		name  string
+		state string
+	}{
+		{name: "EmptyState", state: ""},
+		{name: "NoColonSeparator", state: "garbage"},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			rec := httptest.NewRecorder()
+			req := httptest.NewRequest(
+				http.MethodGet,
+				fmt.Sprintf("/oauth2/callback?code=abc&state=%s", url.QueryEscape(tc.state)),
+				nil,
+			)
+			proxy.ServeHTTP(rec, req)
+
+			assert.Equal(t, http.StatusForbidden, rec.Code)
+			assert.Less(t, rec.Code, http.StatusInternalServerError,
+				"malformed state must not be reported as a 5xx server error")
+		})
+	}
+}
+
 func TestSignOutCallsBackendLogoutURL(t *testing.T) {
 	const testIDToken = "test-id-token-12345"
 	var receivedURL string