Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: return invalid_grant error for invalid or expired auth codes #1952

Merged
merged 2 commits into from
Feb 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion server/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should check if the code is missing/empty first (and return invalid_request if so). WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like your assumption is right, but I need to check it first.

}
return
}
Expand Down
101 changes: 101 additions & 0 deletions server/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()
})
}
}