Repository: zitadel/oidc (go)

Issue:
panic in op.Authorize when request fails to parse
When `ParseAuthorizeRequest` return an error, `authReq` is `nil`:

https://github.com/zitadel/oidc/blob/de33faa2d89f79e30e19c265d549a0bac4cdb7b9/pkg/op/auth_request.go#L68-L72

However `AuthRequestError` takes an interface type for the `authReq` object. That means the test for `nil` always evaluates to `false`, resulting in a panic on line 22:

https://github.com/zitadel/oidc/blob/de33faa2d89f79e30e19c265d549a0bac4cdb7b9/pkg/op/error.go#L16-L25

I found this while writing test. The code to reproduce is rather complex. I can share the panic output:

<details>

```
Running tool: /usr/bin/go test -timeout 30s -coverprofile=/tmp/vscode-goPackQR/go-code-cover -covermode atomic -run ^TestRoutes$ github.com/zitadel/oidc/v2/pkg/op

--- FAIL: TestRoutes (0.00s)
    --- FAIL: TestRoutes//authorize (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x745b20]

goroutine 31 [running]:
testing.tRunner.func1.2({0x84b2a0, 0xbfbcd0})
	/usr/lib/go/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
	/usr/lib/go/src/testing/testing.go:1529 +0x39f
panic({0x84b2a0, 0xbfbcd0})
	/usr/lib/go/src/runtime/panic.go:884 +0x213
github.com/zitadel/oidc/v2/pkg/oidc.(*AuthRequest).GetRedirectURI(0x964880?)
	/home/timz/zitadel/oidc/pkg/oidc/authorization.go:91
github.com/zitadel/oidc/v2/pkg/op.AuthRequestError({0x9679b0, 0xc00014e580}, 0xc00007e0a0?, {0x966ed0?, 0x0?}, {0x964880, 0xc0000a4230}, {0x964840, 0xc00007e110})
	/home/timz/zitadel/oidc/pkg/op/error.go:22 +0x135
github.com/zitadel/oidc/v2/pkg/op.Authorize({0x9679b0, 0xc00014e580}, 0xc00050d400, {0x7f00244a21c0?, 0xc00045c000?})
	/home/timz/zitadel/oidc/pkg/op/auth_request.go:70 +0x9cb
github.com/zitadel/oidc/v2/pkg/op.CreateRouter.func2({0x9679b0?, 0xc00014e580?}, 0xbd8001?)
	/home/timz/zitadel/oidc/pkg/op/auth_request.go:55 +0x3c
net/http.HandlerFunc.ServeHTTP(0x967e20?, {0x9679b0?, 0xc00014e580?}, 0xbd80a0?)
	/usr/lib/go/src/net/http/server.go:2122 +0x2f
github.com/zitadel/oidc/v2/pkg/op.(*IssuerInterceptor).setIssuerCtx(0x0?, {0x9679b0, 0xc00014e580}, 0xc00050d300, {0x965880, 0xc000012540})
	/home/timz/zitadel/oidc/pkg/op/context.go:48 +0x1e2
github.com/zitadel/oidc/v2/pkg/op.(*IssuerInterceptor).Handler.func1({0x9679b0?, 0xc00014e580?}, 0xc0000f0e48?)
	/home/timz/zitadel/oidc/pkg/op/context.go:28 +0x4d
net/http.HandlerFunc.ServeHTTP(0xc00045c6c0?, {0x9679b0?, 0xc00014e580?}, 0xc00050d300?)
	/usr/lib/go/src/net/http/server.go:2122 +0x2f
github.com/rs/cors.(*Cors).Handler.func1({0x9679b0, 0xc00014e580}, 0xc00050d300)
	/home/timz/go/pkg/mod/github.com/rs/cors@v1.8.3/cors.go:236 +0x1c4
net/http.HandlerFunc.ServeHTTP(0xc00045c180?, {0x9679b0?, 0xc00014e580?}, 0xc00050d300?)
	/usr/lib/go/src/net/http/server.go:2122 +0x2f
github.com/rs/cors.(*Cors).Handler.func1({0x9679b0, 0xc00014e580}, 0xc00050d300)
	/home/timz/go/pkg/mod/github.com/rs/cors@v1.8.3/cors.go:236 +0x1c4
net/http.HandlerFunc.ServeHTTP(0xc00050d200?, {0x9679b0?, 0xc00014e580?}, 0x750919?)
	/usr/lib/go/src/net/http/server.go:2122 +0x2f
github.com/gorilla/mux.(*Router).ServeHTTP(0xc00045c0c0, {0x9679b0, 0xc00014e580}, 0xc00050d100)
	/home/timz/go/pkg/mod/github.com/gorilla/mux@v1.8.0/mux.go:210 +0x1cf
github.com/zitadel/oidc/v2/pkg/op_test.TestRoutes.func1(0xc000500ea0)
	/home/timz/zitadel/oidc/pkg/op/op_test.go:133 +0x3b0
testing.tRunner(0xc000500ea0, 0xc000090cc0)
	/usr/lib/go/src/testing/testing.go:1576 +0x10b
created by testing.(*T).Run
	/usr/lib/go/src/testing/testing.go:1629 +0x3ea
FAIL	github.com/zitadel/oidc/v2/pkg/op	0.228s
FAIL
```

</details>

Pull request:
fix: resolve nil pointer panic in Authorize
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

Patch:
diff --git a/pkg/op/auth_request.go b/pkg/op/auth_request.go
index 1f9fc45..c264605 100644
--- a/pkg/op/auth_request.go
+++ b/pkg/op/auth_request.go
@@ -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()
diff --git a/pkg/op/client.go b/pkg/op/client.go
index 9da44a7..32a3dd1 100644
--- a/pkg/op/client.go
+++ b/pkg/op/client.go
@@ -151,21 +151,30 @@ func ClientIDFromRequest(r *http.Request, p ClientProvider) (clientID string, au
 	}
 
 	data := new(clientData)
-	if err = p.Decoder().Decode(data, r.PostForm); err != nil {
+	if err = p.Decoder().Decode(data, r.Form); err != nil {
 		return "", false, err
 	}
 
 	JWTProfile, ok := p.(ClientJWTProfile)
-	if ok {
+	if ok && data.ClientAssertion != "" {
+		// if JWTProfile is supported and client sent an assertion, check it and use it as response
+		// regardless if it succeeded or failed
 		clientID, err = ClientJWTAuth(r.Context(), data.ClientAssertionParams, JWTProfile)
+		return clientID, err == nil, err
 	}
-	if !ok || errors.Is(err, ErrNoClientCredentials) {
-		clientID, err = ClientBasicAuth(r, p.Storage())
-	}
+	// try basic auth
+	clientID, err = ClientBasicAuth(r, p.Storage())
+	// if that succeeded, use it
 	if err == nil {
 		return clientID, true, nil
 	}
+	// if the client did not send a Basic Auth Header, ignore the `ErrNoClientCredentials`
+	// but return other errors immediately
+	if err != nil && !errors.Is(err, ErrNoClientCredentials) {
+		return "", false, err
+	}
 
+	// if the client did not authenticate (public clients) it must at least send a client_id
 	if data.ClientID == "" {
 		return "", false, oidc.ErrInvalidClient().WithParent(ErrMissingClientID)
 	}


Test patch:
diff --git a/pkg/op/auth_request_test.go b/pkg/op/auth_request_test.go
index 7a9701b..2bba4e7 100644
--- a/pkg/op/auth_request_test.go
+++ b/pkg/op/auth_request_test.go
@@ -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 {