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: unsupported request parameter error #1956

Merged
merged 1 commit into from
Jan 23, 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: 7 additions & 0 deletions server/oauth2.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ const (
errUnauthorizedClient = "unauthorized_client"
errAccessDenied = "access_denied"
errUnsupportedResponseType = "unsupported_response_type"
errRequestNotSupported = "request_not_supported"
errInvalidScope = "invalid_scope"
errServerError = "server_error"
errTemporarilyUnavailable = "temporarily_unavailable"
Expand Down Expand Up @@ -453,6 +454,12 @@ func (s *Server) parseAuthorizationRequest(r *http.Request) (*storage.AuthReques
return &authErr{state, redirectURI, typ, fmt.Sprintf(format, a...)}
}

// dex doesn't support request parameter and must return request_not_supported error
// https://openid.net/specs/openid-connect-core-1_0.html#6.1
if q.Get("request") != "" {
return nil, newErr(errRequestNotSupported, "Server does not support request parameter.")
Copy link
Member

Choose a reason for hiding this comment

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

Do I recall correctly that the first implementation involved some redirections? I briefly looked at that version and I didn't really understand it.

Copy link
Member Author

@nabokihms nabokihms Jan 23, 2021

Choose a reason for hiding this comment

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

I realized that Dex already does the necessary redirect if an error is returned from the parseAuthorizationRequest method.

}

if codeChallengeMethod != CodeChallengeMethodS256 && codeChallengeMethod != CodeChallengeMethodPlain {
description := fmt.Sprintf("Unsupported PKCE challenge method (%q).", codeChallengeMethod)
return nil, newErr(errInvalidRequest, description)
Expand Down
35 changes: 29 additions & 6 deletions server/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/kylelemons/godebug/pretty"
"github.com/prometheus/client_golang/prometheus"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/require"
"golang.org/x/crypto/bcrypt"
"golang.org/x/oauth2"
jose "gopkg.in/square/go-jose.v2"
Expand Down Expand Up @@ -223,6 +224,9 @@ type test struct {
// extra parameters to pass when retrieving id token
retrieveTokenOptions []oauth2.AuthCodeOption

// define an error response, when the test expects an error on the auth endpoint
authError *OAuth2ErrorResponse

// define an error response, when the test expects an error on the token endpoint
tokenError ErrorResponse
}
Expand Down Expand Up @@ -607,6 +611,19 @@ func makeOAuth2Tests(clientID string, clientSecret string, now func() time.Time)
StatusCode: http.StatusBadRequest,
},
},
{
name: "Request parameter in authorization query",
authCodeOptions: []oauth2.AuthCodeOption{
oauth2.SetAuthURLParam("request", "anything"),
},
authError: &OAuth2ErrorResponse{
Error: errRequestNotSupported,
ErrorDescription: "Server does not support request parameter.",
},
handleToken: func(ctx context.Context, p *oidc.Provider, config *oauth2.Config, token *oauth2.Token, conn *mock.Callback) error {
return nil
},
},
},
}
}
Expand Down Expand Up @@ -665,7 +682,7 @@ func TestOAuth2CodeFlow(t *testing.T) {
state = "a_state"
)
defer func() {
if !gotCode {
if !gotCode && tc.authError == nil {
t.Errorf("never got a code in callback\n%s\n%s", reqDump, respDump)
}
}()
Expand All @@ -684,12 +701,18 @@ func TestOAuth2CodeFlow(t *testing.T) {

// Did dex return an error?
if errType := q.Get("error"); errType != "" {
if desc := q.Get("error_description"); desc != "" {
t.Errorf("got error from server %s: %s", errType, desc)
} else {
t.Errorf("got error from server %s", errType)
description := q.Get("error_description")

if tc.authError == nil {
if description != "" {
t.Errorf("got error from server %s: %s", errType, description)
} else {
t.Errorf("got error from server %s", errType)
}
w.WriteHeader(http.StatusInternalServerError)
return
}
w.WriteHeader(http.StatusInternalServerError)
require.Equal(t, *tc.authError, OAuth2ErrorResponse{Error: errType, ErrorDescription: description})
return
}

Expand Down