Merge pull request #1952 from flant/auth-code-iinvalid-grant

fix: return invalid_grant error for invalid or expired auth codes

server/handlers.go

@@ -805,13 +805,18 @@ 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 {
 			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
 	}

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,101 @@ func TestConnectorLoginDoesNotAllowToChangeConnectorForAuthRequest(t *testing.T)
 		t.Error("attempt to overwrite connector on auth request should fail")
 	}
 }
+
+// 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)
+
+				_, err = oauth2Config.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)
+			},
+		},
+		{
+			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)
+
+				oauth2Err, ok := err.(*oauth2.RetrieveError)
+				require.True(t, ok)
+
+				var errResponse struct{ Error string }
+				err = json.Unmarshal(oauth2Err.Body, &errResponse)
+				require.NoError(t, err)
+
+				require.Equal(t, errInvalidRequest, errResponse.Error)
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(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"))
+
+				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()
+		})
+	}
+}