cim/zitadel-oidc
5
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix: resolve nil pointer panic in Authorize (#358)

Browse source 
When ParseAuthorizeRequest received an invalid URL,
for example containing a semi-colon `;`,
AuthRequestError used to panic.
This was because a typed nil was passed as a interface argument.
The nil check inside AuthRequestError always resulted in false,
allowing access through the nil pointer.

Fixes #315
This commit is contained in:
Tim Möhlmann 2023-04-05 10:02:37 +02:00 • committed by GitHub
parent c72aa8f9a1
commit 057538d555
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 30 additions and 55 deletions
Download patch file Download diff file Expand all files Collapse all files

2
pkg/op/auth_request.go
View file

@ -68,7 +68,7 @@ func authorizeCallbackHandler(authorizer Authorizer) func(http.ResponseWriter, *
func Authorize(w http.ResponseWriter, r *http.Request, authorizer Authorizer) {
authReq, err := ParseAuthorizeRequest(r, authorizer.Decoder())
if err != nil {
AuthRequestError(w, r, authReq, err, authorizer.Encoder())
AuthRequestError(w, r, nil, err, authorizer.Encoder())
return
}
ctx := r.Context()

83
pkg/op/auth_request_test.go
View file

@ -9,6 +9,7 @@ import (
"reflect"
"testing"
"github.com/golang/mock/gomock"
"github.com/gorilla/schema"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
@ -19,60 +20,34 @@ import (
"github.com/zitadel/oidc/v2/pkg/op/mock"
)
//
// TOOD: tests will be implemented in branch for service accounts
// func TestAuthorize(t *testing.T) {
// // testCallback := func(t *testing.T, clienID string) callbackHandler {
// // return func(authReq *oidc.AuthRequest, client oidc.Client, w http.ResponseWriter, r *http.Request) {
// // // require.Equal(t, clientID, client.)
// // }
// // }
// // testErr := func(t *testing.T, expected error) errorHandler {
// // return func(w http.ResponseWriter, r *http.Request, authReq *oidc.AuthRequest, err error) {
// // require.Equal(t, expected, err)
// // }
// // }
// type args struct {
// w http.ResponseWriter
// r *http.Request
// authorizer op.Authorizer
// }
// tests := []struct {
// name string
// args args
// }{
// {
// "parsing fails",
// args{
// httptest.NewRecorder(),
// &http.Request{Method: "POST", Body: nil},
// mock.NewAuthorizerExpectValid(t, true),
// // testCallback(t, ""),
// // testErr(t, ErrInvalidRequest("cannot parse form")),
// },
// },
// {
// "decoding fails",
// args{
// httptest.NewRecorder(),
// func() *http.Request {
// r := httptest.NewRequest("POST", "/authorize", strings.NewReader("client_id=foo"))
// r.Header.Set("Content-Type", "application/x-www-form-urlencoded")
// return r
// }(),
// mock.NewAuthorizerExpectValid(t, true),
// // testCallback(t, ""),
// // testErr(t, ErrInvalidRequest("cannot parse auth request")),
// },
// },
// // {"decoding fails", args{httptest.NewRecorder(), &http.Request{}, mock.NewAuthorizerExpectValid(t), nil, testErr(t, nil)}},
// }
// for _, tt := range tests {
// t.Run(tt.name, func(t *testing.T) {
// op.Authorize(tt.args.w, tt.args.r, tt.args.authorizer)
// })
// }
//}
func TestAuthorize(t *testing.T) {
tests := []struct {
name string
req *http.Request
expect func(a *mock.MockAuthorizerMockRecorder)
}{
{
name: "parse error", // used to panic, see issue #315
req: httptest.NewRequest(http.MethodPost, "/?;", nil),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
w := httptest.NewRecorder()
authorizer := mock.NewMockAuthorizer(gomock.NewController(t))
expect := authorizer.EXPECT()
expect.Decoder().Return(schema.NewDecoder())
expect.Encoder().Return(schema.NewEncoder())
if tt.expect != nil {
tt.expect(expect)
}
op.Authorize(w, tt.req, authorizer)
})
}
}
func TestParseAuthorizeRequest(t *testing.T) {
type args struct {