diff --git a/CHANGELOG.md b/CHANGELOG.md index 788e82c2..4ad2bed4 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/oauthproxy.go b/oauthproxy.go index f8dc5471..165c82ae 100644 --- a/oauthproxy.go +++ b/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 } diff --git a/oauthproxy_test.go b/oauthproxy_test.go index b3271e5b..35b17997 100644 --- a/oauthproxy_test.go +++ b/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