From fa3fad84e045be0ff5c7d19c190a1bd5d9daafec Mon Sep 17 00:00:00 2001 From: James Elliott Date: Mon, 21 Nov 2022 12:29:22 +1100 Subject: [PATCH] fix: write authorize error returns 303 for get method This presumably fixes an issue where the WriteAuthorizeError function sets a 303 See Other when it should return a 302 Found instead when the method of the original HTTP request used the method verb 'GET'. See https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/302 and https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/303. The 303 See Other is typically only used when the client is being instructed to change the request method verb to 'GET' which is not relevant if it is already 'GET'. --- authorize_error.go | 8 +++++- authorize_error_test.go | 46 +++++++++++++++++++++++------------ authorize_request.go | 5 ++++ authorize_request_handler.go | 3 +++ internal/authorize_request.go | 14 +++++++++++ oauth2.go | 3 +++ 6 files changed, 63 insertions(+), 16 deletions(-) diff --git a/authorize_error.go b/authorize_error.go index 3870e4472..3ac9b6d1f 100644 --- a/authorize_error.go +++ b/authorize_error.go @@ -83,5 +83,11 @@ func (f *Fosite) WriteAuthorizeError(ctx context.Context, rw http.ResponseWriter } rw.Header().Set("Location", redirectURIString) - rw.WriteHeader(http.StatusSeeOther) + + switch ar.GetRequestMethod() { + case http.MethodGet: + rw.WriteHeader(http.StatusFound) + default: + rw.WriteHeader(http.StatusSeeOther) + } } diff --git a/authorize_error_test.go b/authorize_error_test.go index 3c5cf1d11..132d7400d 100644 --- a/authorize_error_test.go +++ b/authorize_error_test.go @@ -95,7 +95,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code"})) req.EXPECT().GetResponseMode().Return(ResponseModeQuery).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?error=invalid_request&error_debug=with-debug&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -117,6 +118,7 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code"})) req.EXPECT().GetResponseMode().Return(ResponseModeQuery).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) + req.EXPECT().GetRequestMethod().Return(http.MethodPost) rw.EXPECT().WriteHeader(http.StatusSeeOther) }, checkHeader: func(t *testing.T, k int) { @@ -138,7 +140,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code"})) req.EXPECT().GetResponseMode().Return(ResponseModeQuery).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?error=invalid_request&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.+Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -158,7 +161,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code"})) req.EXPECT().GetResponseMode().Return(ResponseModeDefault).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?error=invalid_request&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -178,7 +182,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code"})) req.EXPECT().GetResponseMode().Return(ResponseModeQuery).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?error=invalid_request&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&foo=bar&state=foostate") @@ -198,7 +203,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"foobar"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?foo=bar#error=unsupported_grant_type&error_description=The+authorization+grant+type+is+not+supported+by+the+authorization+server.&state=foostate") @@ -218,7 +224,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"token"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/#error=invalid_request&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -238,7 +245,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"token"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?foo=bar#error=invalid_request&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -258,7 +266,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code", "token"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/#error=invalid_request&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -279,7 +288,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code", "token"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/#error=invalid_request&error_debug=with-debug&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -301,7 +311,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code", "token"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/#error=invalid_request&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.+Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.+with-debug&state=foostate") @@ -324,7 +335,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code", "token"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/#error=invalid_request&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.+Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -347,7 +359,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code", "token"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?foo=bar#error=invalid_request&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -368,7 +381,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"code", "token"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?foo=bar#error=invalid_request&error_debug=with-debug&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -389,7 +403,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"id_token"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?foo=bar#error=invalid_request&error_debug=with-debug&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") @@ -410,7 +425,8 @@ func TestWriteAuthorizeError(t *testing.T) { req.EXPECT().GetResponseTypes().AnyTimes().Return(Arguments([]string{"token"})) req.EXPECT().GetResponseMode().Return(ResponseModeFragment).AnyTimes() rw.EXPECT().Header().Times(3).Return(header) - rw.EXPECT().WriteHeader(http.StatusSeeOther) + req.EXPECT().GetRequestMethod().Return(http.MethodGet) + rw.EXPECT().WriteHeader(http.StatusFound) }, checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?foo=bar#error=invalid_request&error_debug=with-debug&error_description=The+request+is+missing+a+required+parameter%2C+includes+an+invalid+parameter+value%2C+includes+a+parameter+more+than+once%2C+or+is+otherwise+malformed.&error_hint=Make+sure+that+the+various+parameters+are+correct%2C+be+aware+of+case+sensitivity+and+trim+your+parameters.+Make+sure+that+the+client+you+are+using+has+exactly+whitelisted+the+redirect_uri+you+specified.&state=foostate") diff --git a/authorize_request.go b/authorize_request.go index dd5f0a571..863751c1a 100644 --- a/authorize_request.go +++ b/authorize_request.go @@ -42,6 +42,7 @@ type AuthorizeRequest struct { HandledResponseTypes Arguments `json:"handledResponseTypes" gorethink:"handledResponseTypes"` ResponseMode ResponseModeType `json:"ResponseModes" gorethink:"ResponseModes"` DefaultResponseMode ResponseModeType `json:"DefaultResponseMode" gorethink:"DefaultResponseMode"` + RequestMethod string `json:"requestMethod" gorethink:"requestMethod"` Request } @@ -114,3 +115,7 @@ func (d *AuthorizeRequest) SetDefaultResponseMode(defaultResponseMode ResponseMo func (d *AuthorizeRequest) GetDefaultResponseMode() ResponseModeType { return d.DefaultResponseMode } + +func (d *AuthorizeRequest) GetRequestMethod() string { + return d.RequestMethod +} diff --git a/authorize_request_handler.go b/authorize_request_handler.go index b0984982f..e27aa6731 100644 --- a/authorize_request_handler.go +++ b/authorize_request_handler.go @@ -343,6 +343,9 @@ func (f *Fosite) newAuthorizeRequest(ctx context.Context, r *http.Request, isPAR // Save state to the request to be returned in error conditions (https://github.com/ory/hydra/issues/1642) request.State = request.Form.Get("state") + // Save the original request method in order to use in later potions of the flow. + request.RequestMethod = r.Method + // Check if this is a continuation from a pushed authorization request if !isPARRequest { if isPAR, err := f.authorizeRequestFromPAR(ctx, r, request); err != nil { diff --git a/internal/authorize_request.go b/internal/authorize_request.go index a643e04e8..66359fc8d 100644 --- a/internal/authorize_request.go +++ b/internal/authorize_request.go @@ -231,6 +231,20 @@ func (mr *MockAuthorizeRequesterMockRecorder) GetResponseTypes() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetResponseTypes", reflect.TypeOf((*MockAuthorizeRequester)(nil).GetResponseTypes)) } +// GetRequestMethod mocks base method. +func (m *MockAuthorizeRequester) GetRequestMethod() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRequestMethod") + ret0, _ := ret[0].(string) + return ret0 +} + +// GetRequestMethod indicates an expected call of GetRequestMethod. +func (mr *MockAuthorizeRequesterMockRecorder) GetRequestMethod() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRequestMethod", reflect.TypeOf((*MockAuthorizeRequester)(nil).GetRequestMethod)) +} + // GetSession mocks base method. func (m *MockAuthorizeRequester) GetSession() fosite.Session { m.ctrl.T.Helper() diff --git a/oauth2.go b/oauth2.go index 66a539b06..cd7a77644 100644 --- a/oauth2.go +++ b/oauth2.go @@ -302,6 +302,9 @@ type AuthorizeRequester interface { // GetDefaultResponseMode gets default response mode for a response type in a flow GetDefaultResponseMode() ResponseModeType + // GetRequestMethod returns the authorize requests HTTP method. + GetRequestMethod() string + Requester }