diff --git a/HISTORY.md b/HISTORY.md index e7f9c1dc4..8fd3df9bb 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -39,6 +39,17 @@ bumps (`0.1.0` -> `0.2.0`). +## 0.16.0 + +This patch introduces `SendDebugMessagesToClients` to the Fosite struct which enables/disables sending debug information to +clients. Debug information may contain sensitive information as it forwards error messages from, for example, storage +implementations. For this reason, `RevealDebugPayloads` defaults to false. Keep in mind that the information may be +very helpful when specific OAuth 2.0 requests fail and we generally recommend displaying debug information. + +Additionally, error keys for JSON changed which caused a new minor version, speicifically +[`statusCode` was changed to `status_code`](https://github.com/ory/fosite/pull/242/files#diff-dd25e0e0a594c3f3592c1c717039b85eR221). + + ## 0.15.0 This release focuses on improving compatibility with OpenID Connect Certification and better error context. diff --git a/access_error.go b/access_error.go index 09ce8c4bb..f54b7c633 100644 --- a/access_error.go +++ b/access_error.go @@ -21,13 +21,17 @@ import ( ) func (c *Fosite) WriteAccessError(rw http.ResponseWriter, _ AccessRequester, err error) { - writeJsonError(rw, err) + c.writeJsonError(rw, err) } -func writeJsonError(rw http.ResponseWriter, err error) { +func (c *Fosite) writeJsonError(rw http.ResponseWriter, err error) { rw.Header().Set("Content-Type", "application/json;charset=UTF-8") rfcerr := ErrorToRFC6749Error(err) + if !c.SendDebugMessagesToClients { + rfcerr.Debug = "" + } + js, err := json.Marshal(rfcerr) if err != nil { http.Error(rw, fmt.Sprintf(`{"error": "%s"}`, err.Error()), http.StatusInternalServerError) diff --git a/access_error_test.go b/access_error_test.go index 90b47623d..79840b2ef 100644 --- a/access_error_test.go +++ b/access_error_test.go @@ -20,6 +20,8 @@ import ( "net/http/httptest" "testing" + "fmt" + "github.com/golang/mock/gomock" . "github.com/ory/fosite" . "github.com/ory/fosite/internal" @@ -47,29 +49,43 @@ func TestWriteAccessError_RFC6749(t *testing.T) { f := &Fosite{} for k, c := range []struct { - err *RFC6749Error - code string + err *RFC6749Error + code string + debug bool }{ - {ErrInvalidRequest, "invalid_request"}, - {ErrInvalidClient, "invalid_client"}, - {ErrInvalidGrant, "invalid_grant"}, - {ErrInvalidScope, "invalid_scope"}, - {ErrUnauthorizedClient, "unauthorized_client"}, - {ErrUnsupportedGrantType, "unsupported_grant_type"}, + {ErrInvalidRequest.WithDebug("some-debug"), "invalid_request", false}, + {ErrInvalidClient.WithDebug("some-debug"), "invalid_client", false}, + {ErrInvalidGrant.WithDebug("some-debug"), "invalid_grant", false}, + {ErrInvalidScope.WithDebug("some-debug"), "invalid_scope", false}, + {ErrUnauthorizedClient.WithDebug("some-debug"), "unauthorized_client", false}, + {ErrUnsupportedGrantType.WithDebug("some-debug"), "unsupported_grant_type", false}, } { - rw := httptest.NewRecorder() - f.WriteAccessError(rw, nil, c.err) + t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { + f.SendDebugMessagesToClients = c.debug + + rw := httptest.NewRecorder() + f.WriteAccessError(rw, nil, c.err) + + var params struct { + Error string `json:"error"` // specified by RFC, required + Description string `json:"error_description"` // specified by RFC, optional + Debug string `json:"error_debug"` + Hint string `json:"error_hint"` + } - var params struct { - Error string `json:"error"` // specified by RFC, required - Description string `json:"error_description"` // specified by RFC, optional - } + require.NotNil(t, rw.Body) + err := json.NewDecoder(rw.Body).Decode(¶ms) + require.NoError(t, err) - require.NotNil(t, rw.Body, "(%d) %s: nil body", k, c.code) - err := json.NewDecoder(rw.Body).Decode(¶ms) - require.NoError(t, err, "(%d) %s", k, c.code) + assert.Equal(t, c.code, params.Error) + assert.Equal(t, c.err.Description, params.Description) + assert.Equal(t, c.err.Hint, params.Hint) - assert.Equal(t, c.code, params.Error, "(%d) %s: error", k, c.code) - assert.Equal(t, c.err.Description, params.Description, "(%d) %s: description", k, c.code) + if !c.debug { + assert.Empty(t, params.Debug) + } else { + assert.Equal(t, "some-debug", params.Debug) + } + }) } } diff --git a/authorize_error.go b/authorize_error.go index eaa41d480..5cca4a236 100644 --- a/authorize_error.go +++ b/authorize_error.go @@ -23,6 +23,10 @@ import ( func (c *Fosite) WriteAuthorizeError(rw http.ResponseWriter, ar AuthorizeRequester, err error) { rfcerr := ErrorToRFC6749Error(err) if !ar.IsRedirectURIValid() { + if !c.SendDebugMessagesToClients { + rfcerr.Debug = "" + } + js, err := json.MarshalIndent(rfcerr, "", "\t") if err != nil { http.Error(rw, err.Error(), http.StatusInternalServerError) @@ -40,6 +44,13 @@ func (c *Fosite) WriteAuthorizeError(rw http.ResponseWriter, ar AuthorizeRequest query.Add("error", rfcerr.Name) query.Add("error_description", rfcerr.Description) query.Add("state", ar.GetState()) + if c.SendDebugMessagesToClients && rfcerr.Debug != "" { + query.Add("error_debug", rfcerr.Debug) + } + + if rfcerr.Hint != "" { + query.Add("error_hint", rfcerr.Hint) + } if ar.GetResponseTypes().Exact("token") || len(ar.GetResponseTypes()) > 1 { redirectURI.Fragment = query.Encode() diff --git a/authorize_error_test.go b/authorize_error_test.go index 1eb51fda8..c39d1bb01 100644 --- a/authorize_error_test.go +++ b/authorize_error_test.go @@ -19,6 +19,8 @@ import ( "net/url" "testing" + "fmt" + "github.com/golang/mock/gomock" . "github.com/ory/fosite" . "github.com/ory/fosite/internal" @@ -40,11 +42,6 @@ import ( // additional query parameters. The endpoint URI MUST NOT include a // fragment component. func TestWriteAuthorizeError(t *testing.T) { - ctrl := gomock.NewController(t) - rw := NewMockResponseWriter(ctrl) - req := NewMockAuthorizeRequester(ctrl) - defer ctrl.Finish() - var urls = []string{ "https://foobar.com/", "https://foobar.com/?foo=bar", @@ -55,28 +52,29 @@ func TestWriteAuthorizeError(t *testing.T) { purls = append(purls, purl) } - oauth2 := &Fosite{} header := http.Header{} for k, c := range []struct { err error - mock func() - checkHeader func(int) + debug bool + mock func(*MockResponseWriter, *MockAuthorizeRequester) + checkHeader func(*testing.T, int) }{ { err: ErrInvalidGrant, - mock: func() { + mock: func(rw *MockResponseWriter, req *MockAuthorizeRequester) { req.EXPECT().IsRedirectURIValid().Return(false) rw.EXPECT().Header().Return(header) rw.EXPECT().WriteHeader(http.StatusBadRequest) rw.EXPECT().Write(gomock.Any()) }, - checkHeader: func(k int) { - assert.Equal(t, "application/json", header.Get("Content-Type"), "%d", k) + checkHeader: func(t *testing.T, k int) { + assert.Equal(t, "application/json", header.Get("Content-Type")) }, }, { - err: ErrInvalidRequest, - mock: func() { + debug: true, + err: ErrInvalidRequest.WithDebug("with-debug"), + mock: func(rw *MockResponseWriter, req *MockAuthorizeRequester) { req.EXPECT().IsRedirectURIValid().Return(true) req.EXPECT().GetRedirectURI().Return(copyUrl(purls[0])) req.EXPECT().GetState().Return("foostate") @@ -84,15 +82,31 @@ func TestWriteAuthorizeError(t *testing.T) { rw.EXPECT().Header().Return(header) rw.EXPECT().WriteHeader(http.StatusFound) }, - checkHeader: func(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&state=foostate") + 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") b, _ := url.Parse(header.Get("Location")) - assert.Equal(t, a, b, "%d", k) + assert.Equal(t, a, b) + }, + }, + { + err: ErrInvalidRequest.WithDebug("with-debug"), + mock: func(rw *MockResponseWriter, req *MockAuthorizeRequester) { + req.EXPECT().IsRedirectURIValid().Return(true) + req.EXPECT().GetRedirectURI().Return(copyUrl(purls[0])) + req.EXPECT().GetState().Return("foostate") + req.EXPECT().GetResponseTypes().MaxTimes(2).Return(Arguments([]string{"code"})) + rw.EXPECT().Header().Return(header) + 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") + b, _ := url.Parse(header.Get("Location")) + assert.Equal(t, a, b) }, }, { err: ErrInvalidRequest, - mock: func() { + mock: func(rw *MockResponseWriter, req *MockAuthorizeRequester) { req.EXPECT().IsRedirectURIValid().Return(true) req.EXPECT().GetRedirectURI().Return(copyUrl(purls[1])) req.EXPECT().GetState().Return("foostate") @@ -100,15 +114,15 @@ func TestWriteAuthorizeError(t *testing.T) { rw.EXPECT().Header().Return(header) rw.EXPECT().WriteHeader(http.StatusFound) }, - checkHeader: func(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&foo=bar&state=foostate") + 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") b, _ := url.Parse(header.Get("Location")) - assert.Equal(t, a, b, "%d", k) + assert.Equal(t, a, b) }, }, { err: ErrInvalidRequest, - mock: func() { + mock: func(rw *MockResponseWriter, req *MockAuthorizeRequester) { req.EXPECT().IsRedirectURIValid().Return(true) req.EXPECT().GetRedirectURI().Return(copyUrl(purls[0])) req.EXPECT().GetState().Return("foostate") @@ -116,16 +130,16 @@ func TestWriteAuthorizeError(t *testing.T) { rw.EXPECT().Header().Return(header) rw.EXPECT().WriteHeader(http.StatusFound) }, - checkHeader: func(k int) { + checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/") - a.Fragment = "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&state=foostate" + a.Fragment = "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" b, _ := url.Parse(header.Get("Location")) - assert.Equal(t, a, b, "%d", k) + assert.Equal(t, a, b) }, }, { err: ErrInvalidRequest, - mock: func() { + mock: func(rw *MockResponseWriter, req *MockAuthorizeRequester) { req.EXPECT().IsRedirectURIValid().Return(true) req.EXPECT().GetRedirectURI().Return(copyUrl(purls[1])) req.EXPECT().GetState().Return("foostate") @@ -133,16 +147,16 @@ func TestWriteAuthorizeError(t *testing.T) { rw.EXPECT().Header().Return(header) rw.EXPECT().WriteHeader(http.StatusFound) }, - checkHeader: func(k int) { + checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?foo=bar") - a.Fragment = "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&state=foostate" + a.Fragment = "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" b, _ := url.Parse(header.Get("Location")) - assert.Equal(t, a.String(), b.String(), "%d", k) + assert.Equal(t, a.String(), b.String()) }, }, { err: ErrInvalidRequest, - mock: func() { + mock: func(rw *MockResponseWriter, req *MockAuthorizeRequester) { req.EXPECT().IsRedirectURIValid().Return(true) req.EXPECT().GetRedirectURI().Return(copyUrl(purls[0])) req.EXPECT().GetState().Return("foostate") @@ -150,16 +164,16 @@ func TestWriteAuthorizeError(t *testing.T) { rw.EXPECT().Header().Return(header) rw.EXPECT().WriteHeader(http.StatusFound) }, - checkHeader: func(k int) { + checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/") - a.Fragment = "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&state=foostate" + a.Fragment = "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" b, _ := url.Parse(header.Get("Location")) - assert.Equal(t, a, b, "%d", k) + assert.Equal(t, a, b) }, }, { - err: ErrInvalidRequest, - mock: func() { + err: ErrInvalidRequest.WithDebug("with-debug"), + mock: func(rw *MockResponseWriter, req *MockAuthorizeRequester) { req.EXPECT().IsRedirectURIValid().Return(true) req.EXPECT().GetRedirectURI().Return(copyUrl(purls[1])) req.EXPECT().GetState().Return("foostate") @@ -167,23 +181,51 @@ func TestWriteAuthorizeError(t *testing.T) { rw.EXPECT().Header().Return(header) rw.EXPECT().WriteHeader(http.StatusFound) }, - checkHeader: func(k int) { + checkHeader: func(t *testing.T, k int) { a, _ := url.Parse("https://foobar.com/?foo=bar") - a.Fragment = "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&state=foostate" + a.Fragment = "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" b, _ := url.Parse(header.Get("Location")) - assert.Equal(t, a.String(), b.String(), "%d", k) + assert.Equal(t, a.String(), b.String()) + }, + }, + { + debug: true, + err: ErrInvalidRequest.WithDebug("with-debug"), + mock: func(rw *MockResponseWriter, req *MockAuthorizeRequester) { + req.EXPECT().IsRedirectURIValid().Return(true) + req.EXPECT().GetRedirectURI().Return(copyUrl(purls[1])) + req.EXPECT().GetState().Return("foostate") + req.EXPECT().GetResponseTypes().MaxTimes(2).Return(Arguments([]string{"code", "token"})) + rw.EXPECT().Header().Return(header) + rw.EXPECT().WriteHeader(http.StatusFound) + }, + checkHeader: func(t *testing.T, k int) { + a, _ := url.Parse("https://foobar.com/?foo=bar") + a.Fragment = "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" + b, _ := url.Parse(header.Get("Location")) + assert.Equal(t, a.String(), b.String()) }, }, } { - c.mock() - oauth2.WriteAuthorizeError(rw, req, c.err) - c.checkHeader(k) - header = http.Header{} - t.Logf("Passed test case %d", k) + t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { + oauth2 := &Fosite{ + SendDebugMessagesToClients: c.debug, + } + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + rw := NewMockResponseWriter(ctrl) + req := NewMockAuthorizeRequester(ctrl) + + c.mock(rw, req) + oauth2.WriteAuthorizeError(rw, req, c.err) + c.checkHeader(t, k) + header = http.Header{} + }) } } func copyUrl(u *url.URL) *url.URL { - url, _ := url.Parse(u.String()) - return url + u2, _ := url.Parse(u.String()) + return u2 } diff --git a/errors.go b/errors.go index 70e24e62f..224d2017a 100644 --- a/errors.go +++ b/errors.go @@ -218,9 +218,9 @@ func ErrorToRFC6749Error(err error) *RFC6749Error { type RFC6749Error struct { Name string `json:"error"` Description string `json:"error_description"` - Hint string `json:"-"` - Code int `json:"statusCode"` - Debug string `json:"-"` + Hint string `json:"error_hint,omitempty"` + Code int `json:"status_code,omitempty"` + Debug string `json:"error_debug,omitempty"` } func (e *RFC6749Error) Status() string { diff --git a/fosite.go b/fosite.go index ccc913818..bb5c3c6f7 100644 --- a/fosite.go +++ b/fosite.go @@ -83,4 +83,9 @@ type Fosite struct { RevocationHandlers RevocationHandlers Hasher Hasher ScopeStrategy ScopeStrategy + + // SendDebugMessagesToClients if set to true, includes error debug messages in response payloads. Be aware that sensitive + // data may be exposed, depending on your implementation of Fosite. Such sensitive data might include database error + // codes or other information. Proceed with caution! + SendDebugMessagesToClients bool } diff --git a/introspection_response_writer.go b/introspection_response_writer.go index 1d937511c..ebaceb44d 100644 --- a/introspection_response_writer.go +++ b/introspection_response_writer.go @@ -50,7 +50,7 @@ func (f *Fosite) WriteIntrospectionError(rw http.ResponseWriter, err error) { switch errors.Cause(err) { case ErrInvalidRequest, ErrRequestUnauthorized: - writeJsonError(rw, err) + f.writeJsonError(rw, err) return }