Repository: zitadel/oidc
Pull request #551 (2024-02-25 10:40:48)

feat(op): Add response_mode: form_post
As mentioned in this issue(https://github.com/zitadel/oidc/issues/515), the implementation of `form_post` is being requested. Additionally, as mentioned in this issue(#550) as well, there are issues with the current implementation of response_mode. 
It is crucial to establish a secure method of returning Authorization Response in the implicit flow.

According to the [OAuth 2.0 Form Post Response Mode](https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html) specification, it is considered safe to return Authorization Response parameters using the form_post Response Mode even if the parameter's default response mode is set to the query or the fragment.

> 4.  Security Considerations
As described in [OAuth 2.0 Multiple Response Type Encoding Practices](https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html#OAuth.Responses) [OAuth.Responses], there are security implications to encoding response values in the query string and in the fragment value. Some of these concerns can be addressed by using the Form Post Response Mode. In particular, it is safe to return Authorization Response parameters whose default Response Modes are the query encoding or the fragment encoding using the form_post Response Mode.

Therefore I have implemented response_mode=form_post as it is considered beneficial.
Any feedback on this implementation would be greatly appreciated.

### Definition of Ready

- [x] I am happy with the code
- [x] Short description of the feature/issue is added in the pr description
- [ ] PR is linked to the corresponding user story
- [ ] Acceptance criteria are met
- [ ] All open todos and follow ups are defined in a new ticket and justified
- [ ] Deviations from the acceptance criteria and design are agreed with the PO and documented.
- [x] No debug or dead code
- [x] My code has no repetitions
- [ ] Critical parts are tested automatically
- [ ] Where possible E2E tests are implemented
- [ ] Documentation/examples are up-to-date
- [ ] All non-functional requirements are met
- [ ] Functionality of the acceptance criteria is checked manually on the dev system.



muhlemmer: The approach looks good to me. I see the PR is in draft, is there still work to do?

At a first glance I would comment:

1. Parse the template ahead of time in a global variable using template.Must. No need to waste resources on every request.
2. Render the template in a buffer, so that the error can be handled correctly. As the code is now, Execute can return an error after partial output (and so a 200) is already written.

Once you mark the PR ready for review I will do a final review.

Cheers!
ay4toh5i: @muhlemmer 
Thank you for the initial review!
I made this PR is ready. And I've addressed the feedback, so please take a look.

muhlemmer: I get a bunch of compile errors when I build your branch:

```
# github.com/zitadel/oidc/v3/pkg/op_test [github.com/zitadel/oidc/v3/pkg/op.test]
pkg/op/auth_request_test.go:12:2: "time" imported and not used
pkg/op/auth_request_test.go:1122:6: unknown field ResponseMode in struct literal of type storage.AuthRequest
```
muhlemmer: Another comment, I just reread the [OAuth 2.0 Form Post Response Mode, section 2.  Form Post Response Mode](https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html#FormPostResponseMode) and there is also the following requirement:

> Because the Authorization Response is intended to be used only once, the Authorization Server MUST instruct the User Agent (and any intermediaries) not to store or reuse the content of the response.

So a [`Cache-Control: no-store`](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#response_directives) header must be sent along with the response.
ay4toh5i: @muhlemmer 
Thank you for your review!

I've addressed your feedback. Could you please review it once?
Also, regarding the tests, I've added `RESPONSE_MODE` to the example app for testing, and I've also tested it with the following URLs:
In both cases, the client's implementation is still pending, so I'm checking the request contents in the console.

`http://localhost:9998/auth?client_id=web&prompt=Welcome back!&redirect_uri=http://localhost:9999/auth/callback&response_type=id_token&response_mode=form_post&scope=openid&state=751c4627-0a02-47e3-8630-bde7611964ad`

`http://localhost:9998/auth?client_id=web&prompt=Welcome back!&redirect_uri=http://localhost:9999/auth/callback&response_type=id_token+token&response_mode=form_post&scope=openid&state=751c4627-0a02-47e3-8630-bde7611964ad`
muhlemmer: The client implementation is quite simple. But you disabled push access to your PR. I've send a PR against this branch. Once merged, the examples work fine with `RESPONSE_MODE="form_post"` set for the app.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/zitadel/oidc/pull/551?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) Report
Attention: Patch coverage is `54.54545%` with `15 lines` in your changes are missing coverage. Please review.
> Project coverage is 61.29%. Comparing base [(`0992c5f`)](https://app.codecov.io/gh/zitadel/oidc/commit/0992c5f3ce793c265c82993cf17fcb7c5a7e6cdf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) to head [(`6d9aaf7`)](https://app.codecov.io/gh/zitadel/oidc/pull/551?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel).
> Report is 17 commits behind head on main.

| [Files](https://app.codecov.io/gh/zitadel/oidc/pull/551?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) | Patch % | Lines |
|---|---|---|
| [pkg/op/auth\_request.go](https://app.codecov.io/gh/zitadel/oidc/pull/551?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29wL2F1dGhfcmVxdWVzdC5nbw==) | 54.54% | [11 Missing and 4 partials :warning: ](https://app.codecov.io/gh/zitadel/oidc/pull/551?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) |

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #551      +/-   ##
==========================================
+ Coverage   60.06%   61.29%   +1.23%     
==========================================
  Files          80       80              
  Lines        6998     5958    -1040     
==========================================
- Hits         4203     3652     -551     
+ Misses       2498     2005     -493     
- Partials      297      301       +4     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/zitadel/oidc/pull/551?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel).

ay4toh5i: @muhlemmer Thanks a bunch! I've merged your PR.

Patch:
diff --git a/example/client/app/app.go b/example/client/app/app.go
index a779169..9b43b8d 100644
--- a/example/client/app/app.go
+++ b/example/client/app/app.go
@@ -32,6 +32,7 @@ func main() {
 	issuer := os.Getenv("ISSUER")
 	port := os.Getenv("PORT")
 	scopes := strings.Split(os.Getenv("SCOPES"), " ")
+	responseMode := os.Getenv("RESPONSE_MODE")
 
 	redirectURI := fmt.Sprintf("http://localhost:%v%v", port, callbackPath)
 	cookieHandler := httphelper.NewCookieHandler(key, key, httphelper.WithUnsecure())
@@ -77,12 +78,24 @@ func main() {
 		return uuid.New().String()
 	}
 
+	urlOptions := []rp.URLParamOpt{
+		rp.WithPromptURLParam("Welcome back!"),
+	}
+
+	if responseMode != "" {
+		urlOptions = append(urlOptions, rp.WithResponseModeURLParam(oidc.ResponseMode(responseMode)))
+	}
+
 	// register the AuthURLHandler at your preferred path.
 	// the AuthURLHandler creates the auth request and redirects the user to the auth server.
 	// including state handling with secure cookie and the possibility to use PKCE.
 	// Prompts can optionally be set to inform the server of
 	// any messages that need to be prompted back to the user.
-	http.Handle("/login", rp.AuthURLHandler(state, provider, rp.WithPromptURLParam("Welcome back!")))
+	http.Handle("/login", rp.AuthURLHandler(
+		state,
+		provider,
+		urlOptions...,
+	))
 
 	// for demonstration purposes the returned userinfo response is written as JSON object onto response
 	marshalUserinfo := func(w http.ResponseWriter, r *http.Request, tokens *oidc.Tokens[*oidc.IDTokenClaims], state string, rp rp.RelyingParty, info *oidc.UserInfo) {
diff --git a/example/server/storage/client.go b/example/server/storage/client.go
index 2e57cc5..010b9ce 100644
--- a/example/server/storage/client.go
+++ b/example/server/storage/client.go
@@ -184,10 +184,10 @@ func WebClient(id, secret string, redirectURIs ...string) *Client {
 		applicationType:                op.ApplicationTypeWeb,
 		authMethod:                     oidc.AuthMethodBasic,
 		loginURL:                       defaultLoginURL,
-		responseTypes:                  []oidc.ResponseType{oidc.ResponseTypeCode},
+		responseTypes:                  []oidc.ResponseType{oidc.ResponseTypeCode, oidc.ResponseTypeIDTokenOnly, oidc.ResponseTypeIDToken},
 		grantTypes:                     []oidc.GrantType{oidc.GrantTypeCode, oidc.GrantTypeRefreshToken, oidc.GrantTypeTokenExchange},
 		accessTokenType:                op.AccessTokenTypeBearer,
-		devMode:                        false,
+		devMode:                        true,
 		idTokenUserinfoClaimsAssertion: false,
 		clockSkew:                      0,
 	}
diff --git a/example/server/storage/oidc.go b/example/server/storage/oidc.go
index 2509f77..9cd08d9 100644
--- a/example/server/storage/oidc.go
+++ b/example/server/storage/oidc.go
@@ -35,6 +35,7 @@ type AuthRequest struct {
 	UserID        string
 	Scopes        []string
 	ResponseType  oidc.ResponseType
+	ResponseMode  oidc.ResponseMode
 	Nonce         string
 	CodeChallenge *OIDCCodeChallenge
 
@@ -100,7 +101,7 @@ func (a *AuthRequest) GetResponseType() oidc.ResponseType {
 }
 
 func (a *AuthRequest) GetResponseMode() oidc.ResponseMode {
-	return "" // we won't handle response mode in this example
+	return a.ResponseMode
 }
 
 func (a *AuthRequest) GetScopes() []string {
@@ -154,6 +155,7 @@ func authRequestToInternal(authReq *oidc.AuthRequest, userID string) *AuthReques
 		UserID:        userID,
 		Scopes:        authReq.Scopes,
 		ResponseType:  authReq.ResponseType,
+		ResponseMode:  authReq.ResponseMode,
 		Nonce:         authReq.Nonce,
 		CodeChallenge: &OIDCCodeChallenge{
 			Challenge: authReq.CodeChallenge,
diff --git a/pkg/client/rp/relying_party.go b/pkg/client/rp/relying_party.go
index 72270fe..74da71e 100644
--- a/pkg/client/rp/relying_party.go
+++ b/pkg/client/rp/relying_party.go
@@ -494,9 +494,8 @@ func CodeExchangeHandler[C oidc.IDClaims](callback CodeExchangeCallback[C], rp R
 			unauthorizedError(w, r, "failed to get state: "+err.Error(), state, rp)
 			return
 		}
-		params := r.URL.Query()
-		if params.Get("error") != "" {
-			rp.ErrorHandler()(w, r, params.Get("error"), params.Get("error_description"), state)
+		if errValue := r.FormValue("error"); errValue != "" {
+			rp.ErrorHandler()(w, r, errValue, r.FormValue("error_description"), state)
 			return
 		}
 		codeOpts := make([]CodeExchangeOpt, len(urlParam))
@@ -521,7 +520,7 @@ func CodeExchangeHandler[C oidc.IDClaims](callback CodeExchangeCallback[C], rp R
 			}
 			codeOpts = append(codeOpts, WithClientAssertionJWT(assertion))
 		}
-		tokens, err := CodeExchange[C](r.Context(), params.Get("code"), rp, codeOpts...)
+		tokens, err := CodeExchange[C](r.Context(), r.FormValue("code"), rp, codeOpts...)
 		if err != nil {
 			unauthorizedError(w, r, "failed to exchange token: "+err.Error(), state, rp)
 			return
diff --git a/pkg/oidc/authorization.go b/pkg/oidc/authorization.go
index 89139ba..fa37dbf 100644
--- a/pkg/oidc/authorization.go
+++ b/pkg/oidc/authorization.go
@@ -48,6 +48,7 @@ const (
 
 	ResponseModeQuery    ResponseMode = "query"
 	ResponseModeFragment ResponseMode = "fragment"
+	ResponseModeFormPost ResponseMode = "form_post"
 
 	// PromptNone (`none`) disallows the Authorization Server to display any authentication or consent user interface pages.
 	// An error (login_required, interaction_required, ...) will be returned if the user is not already authenticated or consent is needed
diff --git a/pkg/op/auth_request.go b/pkg/op/auth_request.go
index d570e25..18d8826 100644
--- a/pkg/op/auth_request.go
+++ b/pkg/op/auth_request.go
@@ -1,9 +1,12 @@
 package op
 
 import (
+	"bytes"
 	"context"
+	_ "embed"
 	"errors"
 	"fmt"
+	"html/template"
 	"log/slog"
 	"net"
 	"net/http"
@@ -464,6 +467,17 @@ func AuthResponseCode(w http.ResponseWriter, r *http.Request, authReq AuthReques
 		Code:  code,
 		State: authReq.GetState(),
 	}
+
+	if authReq.GetResponseMode() == oidc.ResponseModeFormPost {
+		err := AuthResponseFormPost(w, authReq.GetRedirectURI(), &codeResponse, authorizer.Encoder())
+		if err != nil {
+			AuthRequestError(w, r, authReq, err, authorizer)
+			return
+		}
+
+		return
+	}
+
 	callback, err := AuthResponseURL(authReq.GetRedirectURI(), authReq.GetResponseType(), authReq.GetResponseMode(), &codeResponse, authorizer.Encoder())
 	if err != nil {
 		AuthRequestError(w, r, authReq, err, authorizer)
@@ -484,6 +498,17 @@ func AuthResponseToken(w http.ResponseWriter, r *http.Request, authReq AuthReque
 		AuthRequestError(w, r, authReq, err, authorizer)
 		return
 	}
+
+	if authReq.GetResponseMode() == oidc.ResponseModeFormPost {
+		err := AuthResponseFormPost(w, authReq.GetRedirectURI(), resp, authorizer.Encoder())
+		if err != nil {
+			AuthRequestError(w, r, authReq, err, authorizer)
+			return
+		}
+
+		return
+	}
+
 	callback, err := AuthResponseURL(authReq.GetRedirectURI(), authReq.GetResponseType(), authReq.GetResponseMode(), resp, authorizer.Encoder())
 	if err != nil {
 		AuthRequestError(w, r, authReq, err, authorizer)
@@ -535,6 +560,43 @@ func AuthResponseURL(redirectURI string, responseType oidc.ResponseType, respons
 	return mergeQueryParams(uri, params), nil
 }
 
+//go:embed form_post.html.tmpl
+var formPostHtmlTemplate string
+
+var formPostTmpl = template.Must(template.New("form_post").Parse(formPostHtmlTemplate))
+
+// AuthResponseFormPost responds a html page that automatically submits the form which contains the auth response parameters
+func AuthResponseFormPost(res http.ResponseWriter, redirectURI string, response any, encoder httphelper.Encoder) error {
+	values := make(map[string][]string)
+	err := encoder.Encode(response, values)
+	if err != nil {
+		return oidc.ErrServerError().WithParent(err)
+	}
+
+	params := &struct {
+		RedirectURI string
+		Params      any
+	}{
+		RedirectURI: redirectURI,
+		Params:      values,
+	}
+
+	var buf bytes.Buffer
+	err = formPostTmpl.Execute(&buf, params)
+	if err != nil {
+		return oidc.ErrServerError().WithParent(err)
+	}
+
+	res.Header().Set("Cache-Control", "no-store")
+	res.WriteHeader(http.StatusOK)
+	_, err = buf.WriteTo(res)
+	if err != nil {
+		return oidc.ErrServerError().WithParent(err)
+	}
+
+	return nil
+}
+
 func setFragment(uri *url.URL, params url.Values) string {
 	uri.Fragment = params.Encode()
 	return uri.String()
diff --git a/pkg/op/form_post.html.tmpl b/pkg/op/form_post.html.tmpl
new file mode 100644
index 0000000..7bc9ab3
--- /dev/null
+++ b/pkg/op/form_post.html.tmpl
@@ -0,0 +1,14 @@
+<!doctype html>
+<html>
+<head><meta charset="UTF-8" /></head>
+<body onload="javascript:document.forms[0].submit()">
+<form method="post" action="{{ .RedirectURI }}">
+{{with .Params.state}}<input type="hidden" name="state" value="{{ index . 0 }}"/>{{end}}
+{{with .Params.code}}<input type="hidden" name="code" value="{{ index . 0 }}" />{{end}}
+{{with .Params.id_token}}<input type="hidden" name="id_token" value="{{ index . 0 }}"/>{{end}}
+{{with .Params.access_token}}<input type="hidden" name="access_token" value="{{ index . 0 }}" />{{end}}
+{{with .Params.token_type}}<input type="hidden" name="token_type" value="{{ index . 0 }}" />{{end}}
+{{with .Params.expires_in}}<input type="hidden" name="expires_in" value="{{ index . 0 }}" />{{end}}
+</form>
+</body>
+</html>
\ No newline at end of file


Test patch:
diff --git a/pkg/op/auth_request_test.go b/pkg/op/auth_request_test.go
index 2e7c75c..76cb00d 100644
--- a/pkg/op/auth_request_test.go
+++ b/pkg/op/auth_request_test.go
@@ -1027,9 +1027,10 @@ func TestAuthResponseCode(t *testing.T) {
 		authorizer func(*testing.T) op.Authorizer
 	}
 	type res struct {
-		wantCode           int
-		wantLocationHeader string
-		wantBody           string
+		wantCode               int
+		wantLocationHeader     string
+		wantCacheControlHeader string
+		wantBody               string
 	}
 	tests := []struct {
 		name string
@@ -1111,6 +1112,33 @@ func TestAuthResponseCode(t *testing.T) {
 				wantBody:           "",
 			},
 		},
+		{
+			name: "success form_post",
+			args: args{
+				authReq: &storage.AuthRequest{
+					ID:            "id1",
+					CallbackURI:   "https://example.com/callback",
+					TransferState: "state1",
+					ResponseMode:  "form_post",
+				},
+				authorizer: func(t *testing.T) op.Authorizer {
+					ctrl := gomock.NewController(t)
+					storage := mock.NewMockStorage(ctrl)
+					storage.EXPECT().SaveAuthCode(context.Background(), "id1", "id1")
+
+					authorizer := mock.NewMockAuthorizer(ctrl)
+					authorizer.EXPECT().Storage().Return(storage)
+					authorizer.EXPECT().Crypto().Return(&mockCrypto{})
+					authorizer.EXPECT().Encoder().Return(schema.NewEncoder())
+					return authorizer
+				},
+			},
+			res: res{
+				wantCode:               http.StatusOK,
+				wantCacheControlHeader: "no-store",
+				wantBody:               "<!doctype html>\n<html>\n<head><meta charset=\"UTF-8\" /></head>\n<body onload=\"javascript:document.forms[0].submit()\">\n<form method=\"post\" action=\"https://example.com/callback\">\n<input type=\"hidden\" name=\"state\" value=\"state1\"/>\n<input type=\"hidden\" name=\"code\" value=\"id1\" />\n\n\n\n\n</form>\n</body>\n</html>",
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -1121,6 +1149,7 @@ func TestAuthResponseCode(t *testing.T) {
 			defer resp.Body.Close()
 			assert.Equal(t, tt.res.wantCode, resp.StatusCode)
 			assert.Equal(t, tt.res.wantLocationHeader, resp.Header.Get("Location"))
+			assert.Equal(t, tt.res.wantCacheControlHeader, resp.Header.Get("Cache-Control"))
 			body, err := io.ReadAll(resp.Body)
 			require.NoError(t, err)
 			assert.Equal(t, tt.res.wantBody, string(body))