Repository: zitadel/oidc (go)

Issue:
missing state in redirect of implicit flow
**Describe the bug**

as described here: https://discord.com/channels/927474939156643850/927866013545025566/981120399658082334

> if I create the default quickstart react app, and using either Zitadel v1, or v2, setup a client to allow id_token, then configure the app to use id_token response type instead of code, the app doesn't work because the redirect from Zitadel doesn't include the state that the client 

**Expected behaviour**

The redirect should include the state field.


Pull request:
fix: state and auth code response encoding
close #183 

Patch:
diff --git a/go.mod b/go.mod
index 570a3ba..d313764 100644
--- a/go.mod
+++ b/go.mod
@@ -13,7 +13,7 @@ require (
 	github.com/gorilla/securecookie v1.1.1
 	github.com/niemeyer/pretty v0.0.0-20200227124842-a10e7caefd8e // indirect
 	github.com/sirupsen/logrus v1.8.1
-	github.com/stretchr/testify v1.7.1
+	github.com/stretchr/testify v1.7.4
 	github.com/zitadel/logging v0.3.3
 	golang.org/x/oauth2 v0.0.0-20200902213428-5d25da1a8d43
 	golang.org/x/text v0.3.7
diff --git a/go.sum b/go.sum
index 92f343e..f887641 100644
--- a/go.sum
+++ b/go.sum
@@ -134,11 +134,13 @@ github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFR
 github.com/sirupsen/logrus v1.8.1 h1:dJKuHgqk1NNQlqoA6BTlM1Wf9DOH3NBjQyu0h9+AZZE=
 github.com/sirupsen/logrus v1.8.1/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic61uBYv0=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
+github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
 github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
-github.com/stretchr/testify v1.7.1 h1:5TQK59W5E3v0r2duFAb7P95B6hEeOyEnHRa8MjYSMTY=
 github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg=
+github.com/stretchr/testify v1.7.4 h1:wZRexSlwd7ZXfKINDLsO4r7WBt3gTKONc6K/VesHvHM=
+github.com/stretchr/testify v1.7.4/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU=
 github.com/yuin/goldmark v1.1.25/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
 github.com/yuin/goldmark v1.1.32/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74=
@@ -405,8 +407,9 @@ gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.8 h1:obN1ZagJSUGI0Ek/LBmuj4SNLPfIny3KsKFopxRdj10=
 gopkg.in/yaml.v2 v2.2.8/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
-gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b h1:h8qDotaEPuJATrMmW04NCwg7v22aHH28wwpauUhK9Oo=
 gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
+gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
+gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 honnef.co/go/tools v0.0.0-20190102054323-c2f93a96b099/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.0-20190106161140-3f1c8253044a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
 honnef.co/go/tools v0.0.0-20190418001031-e561f6794a2a/go.mod h1:rf3lG4BRIbNafJWhAfAdb/ePZxsR/4RtNHQocxwk9r4=
diff --git a/pkg/http/http.go b/pkg/http/http.go
index 2512707..b0d1ef7 100644
--- a/pkg/http/http.go
+++ b/pkg/http/http.go
@@ -77,14 +77,13 @@ func HttpRequest(client *http.Client, req *http.Request, response interface{}) e
 	return nil
 }
 
-func URLEncodeResponse(resp interface{}, encoder Encoder) (string, error) {
+func URLEncodeParams(resp interface{}, encoder Encoder) (url.Values, error) {
 	values := make(map[string][]string)
 	err := encoder.Encode(resp, values)
 	if err != nil {
-		return "", err
+		return nil, err
 	}
-	v := url.Values(values)
-	return v.Encode(), nil
+	return values, nil
 }
 
 func StartServer(ctx context.Context, port string) {
diff --git a/pkg/oidc/token.go b/pkg/oidc/token.go
index 7b5b812..9c8c48b 100644
--- a/pkg/oidc/token.go
+++ b/pkg/oidc/token.go
@@ -396,6 +396,7 @@ type AccessTokenResponse struct {
 	RefreshToken string `json:"refresh_token,omitempty" schema:"refresh_token,omitempty"`
 	ExpiresIn    uint64 `json:"expires_in,omitempty" schema:"expires_in,omitempty"`
 	IDToken      string `json:"id_token,omitempty" schema:"id_token,omitempty"`
+	State        string `json:"state,omitempty" schema:"state,omitempty"`
 }
 
 type JWTProfileAssertionClaims interface {
diff --git a/pkg/op/auth_request.go b/pkg/op/auth_request.go
index 8a2b8b9..2ebedb5 100644
--- a/pkg/op/auth_request.go
+++ b/pkg/op/auth_request.go
@@ -465,18 +465,41 @@ func BuildAuthRequestCode(authReq AuthRequest, crypto Crypto) (string, error) {
 //AuthResponseURL encodes the authorization response (successful and error) and sets it as query or fragment values
 //depending on the response_mode and response_type
 func AuthResponseURL(redirectURI string, responseType oidc.ResponseType, responseMode oidc.ResponseMode, response interface{}, encoder httphelper.Encoder) (string, error) {
-	params, err := httphelper.URLEncodeResponse(response, encoder)
+	uri, err := url.Parse(redirectURI)
 	if err != nil {
 		return "", oidc.ErrServerError().WithParent(err)
 	}
+	params, err := httphelper.URLEncodeParams(response, encoder)
+	if err != nil {
+		return "", oidc.ErrServerError().WithParent(err)
+	}
+	//return explicitly requested mode
 	if responseMode == oidc.ResponseModeQuery {
-		return redirectURI + "?" + params, nil
+		return mergeQueryParams(uri, params), nil
 	}
 	if responseMode == oidc.ResponseModeFragment {
-		return redirectURI + "#" + params, nil
+		return setFragment(uri, params), nil
 	}
-	if responseType == "" || responseType == oidc.ResponseTypeCode {
-		return redirectURI + "?" + params, nil
+	//implicit must use fragment mode is not specified by client
+	if responseType == oidc.ResponseTypeIDToken || responseType == oidc.ResponseTypeIDTokenOnly {
+		return setFragment(uri, params), nil
+	}
+	//if we get here it's code flow: defaults to query
+	return mergeQueryParams(uri, params), nil
+}
+
+func setFragment(uri *url.URL, params url.Values) string {
+	uri.Fragment = params.Encode()
+	return uri.String()
+}
+
+func mergeQueryParams(uri *url.URL, params url.Values) string {
+	queries := uri.Query()
+	for param, values := range params {
+		for _, value := range values {
+			queries.Add(param, value)
+		}
 	}
-	return redirectURI + "#" + params, nil
+	uri.RawQuery = queries.Encode()
+	return uri.String()
 }
diff --git a/pkg/op/token.go b/pkg/op/token.go
index 7f6f599..3a72261 100644
--- a/pkg/op/token.go
+++ b/pkg/op/token.go
@@ -37,11 +37,13 @@ func CreateTokenResponse(ctx context.Context, request IDTokenRequest, client Cli
 		return nil, err
 	}
 
+	var state string
 	if authRequest, ok := request.(AuthRequest); ok {
 		err = creator.Storage().DeleteAuthRequest(ctx, authRequest.GetID())
 		if err != nil {
 			return nil, err
 		}
+		state = authRequest.GetState()
 	}
 
 	exp := uint64(validity.Seconds())
@@ -51,6 +53,7 @@ func CreateTokenResponse(ctx context.Context, request IDTokenRequest, client Cli
 		RefreshToken: newRefreshToken,
 		TokenType:    oidc.BearerToken,
 		ExpiresIn:    exp,
+		State:        state,
 	}, nil
 }
 


Test patch:
diff --git a/pkg/op/auth_request_test.go b/pkg/op/auth_request_test.go
index f9ba4de..9023011 100644
--- a/pkg/op/auth_request_test.go
+++ b/pkg/op/auth_request_test.go
@@ -793,6 +793,90 @@ func TestAuthResponseURL(t *testing.T) {
 				nil,
 			},
 		},
+		{
+			"with query",
+			args{
+				"uri?param=value",
+				oidc.ResponseTypeCode,
+				"",
+				map[string][]string{"test": {"test"}},
+				&mockEncoder{},
+			},
+			res{
+				"uri?param=value&test=test",
+				nil,
+			},
+		},
+		{
+			"with query response type id token",
+			args{
+				"uri?param=value",
+				oidc.ResponseTypeIDToken,
+				"",
+				map[string][]string{"test": {"test"}},
+				&mockEncoder{},
+			},
+			res{
+				"uri?param=value#test=test",
+				nil,
+			},
+		},
+		{
+			"with existing query",
+			args{
+				"uri?test=value",
+				oidc.ResponseTypeCode,
+				"",
+				map[string][]string{"test": {"test"}},
+				&mockEncoder{},
+			},
+			res{
+				"uri?test=value&test=test",
+				nil,
+			},
+		},
+		{
+			"with existing query response type id token",
+			args{
+				"uri?test=value",
+				oidc.ResponseTypeIDToken,
+				"",
+				map[string][]string{"test": {"test"}},
+				&mockEncoder{},
+			},
+			res{
+				"uri?test=value#test=test",
+				nil,
+			},
+		},
+		{
+			"with existing query and multiple values",
+			args{
+				"uri?test=value",
+				oidc.ResponseTypeCode,
+				"",
+				map[string][]string{"test": {"test", "test2"}},
+				&mockEncoder{},
+			},
+			res{
+				"uri?test=value&test=test&test=test2",
+				nil,
+			},
+		},
+		{
+			"with existing query and multiple values response type id token",
+			args{
+				"uri?test=value",
+				oidc.ResponseTypeIDToken,
+				"",
+				map[string][]string{"test": {"test", "test2"}},
+				&mockEncoder{},
+			},
+			res{
+				"uri?test=value#test=test&test=test2",
+				nil,
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {