From 123185c456018de6c68f6d3638f201323c2464d9 Mon Sep 17 00:00:00 2001 From: "m.nabokikh" Date: Thu, 21 Jan 2021 01:31:38 +0400 Subject: [PATCH 1/2] fix: return invalid_grant error for invalid or expired auth codes Signed-off-by: m.nabokikh --- server/handlers.go | 2 +- server/handlers_test.go | 70 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/server/handlers.go b/server/handlers.go index ec056a76..df348f9a 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -811,7 +811,7 @@ func (s *Server) handleAuthCode(w http.ResponseWriter, r *http.Request, client s s.logger.Errorf("failed to get auth code: %v", err) s.tokenErrHelper(w, errServerError, "", http.StatusInternalServerError) } else { - s.tokenErrHelper(w, errInvalidRequest, "Invalid or expired code parameter.", http.StatusBadRequest) + s.tokenErrHelper(w, errInvalidGrant, "Invalid or expired code parameter.", http.StatusBadRequest) } return } diff --git a/server/handlers_test.go b/server/handlers_test.go index df83e866..07af8b95 100644 --- a/server/handlers_test.go +++ b/server/handlers_test.go @@ -10,7 +10,10 @@ import ( "testing" "time" + "github.com/coreos/go-oidc/v3/oidc" "github.com/gorilla/mux" + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" "github.com/dexidp/dex/storage" "github.com/dexidp/dex/storage/memory" @@ -204,3 +207,70 @@ func TestConnectorLoginDoesNotAllowToChangeConnectorForAuthRequest(t *testing.T) t.Error("attempt to overwrite connector on auth request should fail") } } + +// TestHandleCodeReuse checks that it is forbidden to use same code twice +func TestHandleCodeReuse(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + httpServer, s := newTestServer(ctx, t, func(c *Config) { c.Issuer += "/non-root-path" }) + defer httpServer.Close() + + p, err := oidc.NewProvider(ctx, httpServer.URL) + require.NoError(t, err) + + var oauth2Client oauth2Client + oauth2Client.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/callback" { + http.Redirect(w, r, oauth2Client.config.AuthCodeURL(""), http.StatusSeeOther) + return + } + + q := r.URL.Query() + require.Equal(t, q.Get("error"), "", q.Get("error_description")) + + if code := q.Get("code"); code != "" { + _, err := oauth2Client.config.Exchange(ctx, code) + require.NoError(t, err) + + _, err = oauth2Client.config.Exchange(ctx, code) + require.Error(t, err) + + oauth2Err, ok := err.(*oauth2.RetrieveError) + require.True(t, ok) + + var errResponse struct{ Error string } + err = json.Unmarshal(oauth2Err.Body, &errResponse) + require.NoError(t, err) + + // invalid_grant must be returned for invalid values + // https://tools.ietf.org/html/rfc6749#section-5.2 + require.Equal(t, errInvalidGrant, errResponse.Error) + } + + w.WriteHeader(http.StatusOK) + })) + defer oauth2Client.server.Close() + + redirectURL := oauth2Client.server.URL + "/callback" + client := storage.Client{ + ID: "testclient", + Secret: "testclientsecret", + RedirectURIs: []string{redirectURL}, + } + err = s.storage.CreateClient(client) + require.NoError(t, err) + + oauth2Client.config = &oauth2.Config{ + ClientID: client.ID, + ClientSecret: client.Secret, + Endpoint: p.Endpoint(), + Scopes: []string{oidc.ScopeOpenID, "email", "offline_access"}, + RedirectURL: redirectURL, + } + + resp, err := http.Get(oauth2Client.server.URL + "/login") + require.NoError(t, err) + + resp.Body.Close() +} From d6b5105d9b2e3a1481705c808c2d598d44afc342 Mon Sep 17 00:00:00 2001 From: "m.nabokikh" Date: Mon, 25 Jan 2021 18:50:36 +0400 Subject: [PATCH 2/2] fix: check code presence Signed-off-by: m.nabokikh --- server/handlers.go | 5 ++ server/handlers_test.go | 141 ++++++++++++++++++++++++---------------- 2 files changed, 91 insertions(+), 55 deletions(-) diff --git a/server/handlers.go b/server/handlers.go index df348f9a..3c402a07 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -805,6 +805,11 @@ func (s *Server) handleAuthCode(w http.ResponseWriter, r *http.Request, client s code := r.PostFormValue("code") redirectURI := r.PostFormValue("redirect_uri") + if code == "" { + s.tokenErrHelper(w, errInvalidRequest, `Required param: code.`, http.StatusBadRequest) + return + } + authCode, err := s.storage.GetAuthCode(code) if err != nil || s.now().After(authCode.Expiry) || authCode.ClientID != client.ID { if err != storage.ErrNotFound { diff --git a/server/handlers_test.go b/server/handlers_test.go index 07af8b95..4ca182f2 100644 --- a/server/handlers_test.go +++ b/server/handlers_test.go @@ -208,69 +208,100 @@ func TestConnectorLoginDoesNotAllowToChangeConnectorForAuthRequest(t *testing.T) } } -// TestHandleCodeReuse checks that it is forbidden to use same code twice -func TestHandleCodeReuse(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() +// TestHandleAuthCode checks that it is forbidden to use same code twice +func TestHandleAuthCode(t *testing.T) { + tests := []struct { + name string + handleCode func(*testing.T, context.Context, *oauth2.Config, string) + }{ + { + name: "Code Reuse should return invalid_grant", + handleCode: func(t *testing.T, ctx context.Context, oauth2Config *oauth2.Config, code string) { + _, err := oauth2Config.Exchange(ctx, code) + require.NoError(t, err) - httpServer, s := newTestServer(ctx, t, func(c *Config) { c.Issuer += "/non-root-path" }) - defer httpServer.Close() + _, err = oauth2Config.Exchange(ctx, code) + require.Error(t, err) - p, err := oidc.NewProvider(ctx, httpServer.URL) - require.NoError(t, err) + oauth2Err, ok := err.(*oauth2.RetrieveError) + require.True(t, ok) - var oauth2Client oauth2Client - oauth2Client.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/callback" { - http.Redirect(w, r, oauth2Client.config.AuthCodeURL(""), http.StatusSeeOther) - return - } + var errResponse struct{ Error string } + err = json.Unmarshal(oauth2Err.Body, &errResponse) + require.NoError(t, err) - q := r.URL.Query() - require.Equal(t, q.Get("error"), "", q.Get("error_description")) + // invalid_grant must be returned for invalid values + // https://tools.ietf.org/html/rfc6749#section-5.2 + require.Equal(t, errInvalidGrant, errResponse.Error) + }, + }, + { + name: "No Code should return invalid_request", + handleCode: func(t *testing.T, ctx context.Context, oauth2Config *oauth2.Config, _ string) { + _, err := oauth2Config.Exchange(ctx, "") + require.Error(t, err) - if code := q.Get("code"); code != "" { - _, err := oauth2Client.config.Exchange(ctx, code) - require.NoError(t, err) + oauth2Err, ok := err.(*oauth2.RetrieveError) + require.True(t, ok) - _, err = oauth2Client.config.Exchange(ctx, code) - require.Error(t, err) + var errResponse struct{ Error string } + err = json.Unmarshal(oauth2Err.Body, &errResponse) + require.NoError(t, err) - oauth2Err, ok := err.(*oauth2.RetrieveError) - require.True(t, ok) - - var errResponse struct{ Error string } - err = json.Unmarshal(oauth2Err.Body, &errResponse) - require.NoError(t, err) - - // invalid_grant must be returned for invalid values - // https://tools.ietf.org/html/rfc6749#section-5.2 - require.Equal(t, errInvalidGrant, errResponse.Error) - } - - w.WriteHeader(http.StatusOK) - })) - defer oauth2Client.server.Close() - - redirectURL := oauth2Client.server.URL + "/callback" - client := storage.Client{ - ID: "testclient", - Secret: "testclientsecret", - RedirectURIs: []string{redirectURL}, - } - err = s.storage.CreateClient(client) - require.NoError(t, err) - - oauth2Client.config = &oauth2.Config{ - ClientID: client.ID, - ClientSecret: client.Secret, - Endpoint: p.Endpoint(), - Scopes: []string{oidc.ScopeOpenID, "email", "offline_access"}, - RedirectURL: redirectURL, + require.Equal(t, errInvalidRequest, errResponse.Error) + }, + }, } - resp, err := http.Get(oauth2Client.server.URL + "/login") - require.NoError(t, err) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() - resp.Body.Close() + httpServer, s := newTestServer(ctx, t, func(c *Config) { c.Issuer += "/non-root-path" }) + defer httpServer.Close() + + p, err := oidc.NewProvider(ctx, httpServer.URL) + require.NoError(t, err) + + var oauth2Client oauth2Client + oauth2Client.server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/callback" { + http.Redirect(w, r, oauth2Client.config.AuthCodeURL(""), http.StatusSeeOther) + return + } + + q := r.URL.Query() + require.Equal(t, q.Get("error"), "", q.Get("error_description")) + + code := q.Get("code") + tc.handleCode(t, ctx, oauth2Client.config, code) + + w.WriteHeader(http.StatusOK) + })) + defer oauth2Client.server.Close() + + redirectURL := oauth2Client.server.URL + "/callback" + client := storage.Client{ + ID: "testclient", + Secret: "testclientsecret", + RedirectURIs: []string{redirectURL}, + } + err = s.storage.CreateClient(client) + require.NoError(t, err) + + oauth2Client.config = &oauth2.Config{ + ClientID: client.ID, + ClientSecret: client.Secret, + Endpoint: p.Endpoint(), + Scopes: []string{oidc.ScopeOpenID, "email", "offline_access"}, + RedirectURL: redirectURL, + } + + resp, err := http.Get(oauth2Client.server.URL + "/login") + require.NoError(t, err) + + resp.Body.Close() + }) + } }