Adds ability to forward hints and debug messages to clients (ory#242)

HISTORY.md

@@ -39,6 +39,17 @@ bumps (`0.1.0` -> `0.2.0`).
 
 <!-- END doctoc generated TOC please keep comment here to allow auto update -->
 
+## 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.

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)

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(&params)
+			require.NoError(t, err)
 
-		require.NotNil(t, rw.Body, "(%d) %s: nil body", k, c.code)
-		err := json.NewDecoder(rw.Body).Decode(&params)
-		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)
+			}
+		})
 	}
 }

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()

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,135 +52,180 @@ 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")
 				req.EXPECT().GetResponseTypes().MaxTimes(2).Return(Arguments([]string{"code"}))
 				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")
 				req.EXPECT().GetResponseTypes().MaxTimes(2).Return(Arguments([]string{"code"}))
 				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")
 				req.EXPECT().GetResponseTypes().MaxTimes(2).Return(Arguments([]string{"token"}))
 				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")
 				req.EXPECT().GetResponseTypes().MaxTimes(2).Return(Arguments([]string{"token"}))
 				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")
 				req.EXPECT().GetResponseTypes().MaxTimes(2).Return(Arguments([]string{"code", "token"}))
 				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")
 				req.EXPECT().GetResponseTypes().MaxTimes(2).Return(Arguments([]string{"code", "token"}))
 				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
 }

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 {

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
 }