fix(oidc): default response mode not validated (#5129)

This fixes an issue where the default response mode (i.e. if the mode is omitted) would skip the validations against the allowed response modes.

Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>

internal/handlers/handler_oidc_authorization.go

@@ -63,6 +63,28 @@ func OpenIDConnectAuthorization(ctx *middlewares.AutheliaCtx, rw http.ResponseWr
 		return
 	}
 
+	if !oidc.IsPushedAuthorizedRequest(requester, ctx.Providers.OpenIDConnect.GetPushedAuthorizeRequestURIPrefix(ctx)) {
+		if err = client.ValidatePKCEPolicy(requester); err != nil {
+			rfc := fosite.ErrorToRFC6749Error(err)
+
+			ctx.Logger.Errorf("Authorization Request with id '%s' on client with id '%s' failed to validate the PKCE policy: %s", requester.GetID(), client.GetID(), rfc.WithExposeDebug(true).GetDescription())
+
+			ctx.Providers.OpenIDConnect.WriteAuthorizeError(ctx, rw, requester, err)
+
+			return
+		}
+
+		if err = client.ValidateResponseModePolicy(requester); err != nil {
+			rfc := fosite.ErrorToRFC6749Error(err)
+
+			ctx.Logger.Errorf("Authorization Request with id '%s' on client with id '%s' failed to validate the Response Mode: %s", requester.GetID(), client.GetID(), rfc.WithExposeDebug(true).GetDescription())
+
+			ctx.Providers.OpenIDConnect.WriteAuthorizeError(ctx, rw, requester, err)
+
+			return
+		}
+	}
+
 	if err = client.ValidatePKCEPolicy(requester); err != nil {
 		rfc := fosite.ErrorToRFC6749Error(err)
 
@@ -175,9 +197,19 @@ func OpenIDConnectPushedAuthorizationRequest(ctx *middlewares.AutheliaCtx, rw ht
 	if err = client.ValidatePKCEPolicy(requester); err != nil {
 		rfc := fosite.ErrorToRFC6749Error(err)
 
-		ctx.Logger.Errorf("Pushed Authorization Request with id '%s' on client with id '%s' failed to validate the PKCE policy: %s", requester.GetID(), clientID, rfc.WithExposeDebug(true).GetDescription())
+		ctx.Logger.Errorf("Pushed Authorization Request with id '%s' on client with id '%s' failed to validate the PKCE policy: %s", requester.GetID(), client.GetID(), rfc.WithExposeDebug(true).GetDescription())
 
-		ctx.Providers.OpenIDConnect.WritePushedAuthorizeError(ctx, rw, requester, err)
+		ctx.Providers.OpenIDConnect.WriteAuthorizeError(ctx, rw, requester, err)
+
+		return
+	}
+
+	if err = client.ValidateResponseModePolicy(requester); err != nil {
+		rfc := fosite.ErrorToRFC6749Error(err)
+
+		ctx.Logger.Errorf("Pushed Authorization Request with id '%s' on client with id '%s' failed to validate the Response Mode: %s", requester.GetID(), client.GetID(), rfc.WithExposeDebug(true).GetDescription())
+
+		ctx.Providers.OpenIDConnect.WriteAuthorizeError(ctx, rw, requester, err)
 
 		return
 	}

internal/oidc/client.go

@@ -1,8 +1,6 @@
 package oidc
 
 import (
-	"strings"
-
 	"github.com/ory/fosite"
 	"github.com/ory/x/errorsx"
 
@@ -30,7 +28,7 @@ func NewClient(config schema.OpenIDConnectClientConfiguration) (client *Client)
 		RedirectURIs:  config.RedirectURIs,
 		GrantTypes:    config.GrantTypes,
 		ResponseTypes: config.ResponseTypes,
-		ResponseModes: []fosite.ResponseModeType{fosite.ResponseModeDefault},
+		ResponseModes: []fosite.ResponseModeType{},
 
 		EnforcePAR: config.EnforcePAR,
 
@@ -73,21 +71,44 @@ func (c *Client) ValidatePKCEPolicy(r fosite.Requester) (err error) {
 
 // ValidatePARPolicy is a helper function to validate additional policy constraints on a per-client basis.
 func (c *Client) ValidatePARPolicy(r fosite.Requester, prefix string) (err error) {
-	form := r.GetRequestForm()
-
 	if c.EnforcePAR {
-		if requestURI := form.Get(FormParameterRequestURI); !strings.HasPrefix(requestURI, prefix) {
+		if !IsPushedAuthorizedRequest(r, prefix) {
-			if requestURI == "" {
+			switch requestURI := r.GetRequestForm().Get(FormParameterRequestURI); requestURI {
+			case "":
 				return errorsx.WithStack(ErrPAREnforcedClientMissingPAR.WithDebug("The request_uri parameter was empty."))
+			default:
+				return errorsx.WithStack(ErrPAREnforcedClientMissingPAR.WithDebugf("The request_uri parameter '%s' is malformed.", requestURI))
 			}
-
-			return errorsx.WithStack(ErrPAREnforcedClientMissingPAR.WithDebugf("The request_uri parameter '%s' is malformed.", requestURI))
 		}
 	}
 
 	return nil
 }
 
+// ValidateResponseModePolicy is an additional check to the response mode parameter to ensure if it's omitted that the
+// default response mode for the fosite.AuthorizeRequester is permitted.
+func (c *Client) ValidateResponseModePolicy(r fosite.AuthorizeRequester) (err error) {
+	if r.GetResponseMode() != fosite.ResponseModeDefault {
+		return nil
+	}
+
+	m := r.GetDefaultResponseMode()
+
+	modes := c.GetResponseModes()
+
+	if len(modes) == 0 {
+		return nil
+	}
+
+	for _, mode := range modes {
+		if m == mode {
+			return nil
+		}
+	}
+
+	return errorsx.WithStack(fosite.ErrUnsupportedResponseMode.WithHintf(`The request omitted the response_mode making the default response_mode "%s" based on the other authorization request parameters but registered OAuth 2.0 client doesn't support this response_mode`, m))
+}
+
 // IsAuthenticationLevelSufficient returns if the provided authentication.Level is sufficient for the client of the AutheliaClient.
 func (c *Client) IsAuthenticationLevelSufficient(level authentication.Level) bool {
 	if level == authentication.NotAuthenticated {

internal/oidc/client_test.go

@@ -1,6 +1,7 @@
 package oidc
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/ory/fosite"
@@ -19,8 +20,7 @@ func TestNewClient(t *testing.T) {
 	assert.Equal(t, "", blankClient.ID)
 	assert.Equal(t, "", blankClient.Description)
 	assert.Equal(t, "", blankClient.Description)
-	require.Len(t, blankClient.ResponseModes, 1)
+	assert.Len(t, blankClient.ResponseModes, 0)
-	assert.Equal(t, fosite.ResponseModeDefault, blankClient.ResponseModes[0])
 
 	exampleConfig := schema.OpenIDConnectClientConfiguration{
 		ID:            "myapp",
@@ -36,11 +36,10 @@ func TestNewClient(t *testing.T) {
 
 	exampleClient := NewClient(exampleConfig)
 	assert.Equal(t, "myapp", exampleClient.ID)
-	require.Len(t, exampleClient.ResponseModes, 4)
+	require.Len(t, exampleClient.ResponseModes, 3)
-	assert.Equal(t, fosite.ResponseModeDefault, exampleClient.ResponseModes[0])
+	assert.Equal(t, fosite.ResponseModeFormPost, exampleClient.ResponseModes[0])
-	assert.Equal(t, fosite.ResponseModeFormPost, exampleClient.ResponseModes[1])
+	assert.Equal(t, fosite.ResponseModeQuery, exampleClient.ResponseModes[1])
-	assert.Equal(t, fosite.ResponseModeQuery, exampleClient.ResponseModes[2])
+	assert.Equal(t, fosite.ResponseModeFragment, exampleClient.ResponseModes[2])
-	assert.Equal(t, fosite.ResponseModeFragment, exampleClient.ResponseModes[3])
 	assert.Equal(t, authorization.TwoFactor, exampleClient.Policy)
 }
 
@@ -226,6 +225,7 @@ func TestNewClientPKCE(t *testing.T) {
 		expected                           string
 		r                                  *fosite.Request
 		err                                string
+		desc                               string
 	}{
 		{
 			"ShouldNotEnforcePKCEAndNotErrorOnNonPKCERequest",
@@ -235,6 +235,7 @@ func TestNewClientPKCE(t *testing.T) {
 			"",
 			&fosite.Request{},
 			"",
+			"",
 		},
 		{
 			"ShouldEnforcePKCEAndErrorOnNonPKCERequest",
@@ -244,6 +245,7 @@ func TestNewClientPKCE(t *testing.T) {
 			"",
 			&fosite.Request{},
 			"invalid_request",
+			"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. Clients must include a code_challenge when performing the authorize code flow, but it is missing. The server is configured in a way that enforces PKCE for this client.",
 		},
 		{
 			"ShouldEnforcePKCEAndNotErrorOnPKCERequest",
@@ -253,6 +255,7 @@ func TestNewClientPKCE(t *testing.T) {
 			"",
 			&fosite.Request{Form: map[string][]string{"code_challenge": {"abc"}}},
 			"",
+			"",
 		},
 		{"ShouldEnforcePKCEFromChallengeMethodAndErrorOnNonPKCERequest",
 			schema.OpenIDConnectClientConfiguration{PKCEChallengeMethod: "S256"},
@@ -261,6 +264,7 @@ func TestNewClientPKCE(t *testing.T) {
 			"S256",
 			&fosite.Request{},
 			"invalid_request",
+			"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. Clients must include a code_challenge when performing the authorize code flow, but it is missing. The server is configured in a way that enforces PKCE for this client.",
 		},
 		{"ShouldEnforcePKCEFromChallengeMethodAndErrorOnInvalidChallengeMethod",
 			schema.OpenIDConnectClientConfiguration{PKCEChallengeMethod: "S256"},
@@ -269,6 +273,7 @@ func TestNewClientPKCE(t *testing.T) {
 			"S256",
 			&fosite.Request{Form: map[string][]string{"code_challenge": {"abc"}}},
 			"invalid_request",
+			"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. Client must use code_challenge_method=S256,  is not allowed. The server is configured in a way that enforces PKCE S256 as challenge method for this client.",
 		},
 		{"ShouldEnforcePKCEFromChallengeMethodAndNotErrorOnValidRequest",
 			schema.OpenIDConnectClientConfiguration{PKCEChallengeMethod: "S256"},
@@ -277,6 +282,7 @@ func TestNewClientPKCE(t *testing.T) {
 			"S256",
 			&fosite.Request{Form: map[string][]string{"code_challenge": {"abc"}, "code_challenge_method": {"S256"}}},
 			"",
+			"",
 		},
 	}
 
@@ -292,7 +298,136 @@ func TestNewClientPKCE(t *testing.T) {
 				err := client.ValidatePKCEPolicy(tc.r)
 
 				if tc.err != "" {
+					require.NotNil(t, err)
 					assert.EqualError(t, err, tc.err)
+					assert.Equal(t, tc.desc, fosite.ErrorToRFC6749Error(err).WithExposeDebug(true).GetDescription())
+				} else {
+					assert.NoError(t, err)
+				}
+			}
+		})
+	}
+}
+
+func TestNewClientPAR(t *testing.T) {
+	testCases := []struct {
+		name     string
+		have     schema.OpenIDConnectClientConfiguration
+		expected bool
+		r        *fosite.Request
+		err      string
+		desc     string
+	}{
+		{
+			"ShouldNotEnforcEPARAndNotErrorOnNonPARRequest",
+			schema.OpenIDConnectClientConfiguration{},
+			false,
+			&fosite.Request{},
+			"",
+			"",
+		},
+		{
+			"ShouldEnforcePARAndErrorOnNonPARRequest",
+			schema.OpenIDConnectClientConfiguration{EnforcePAR: true},
+			true,
+			&fosite.Request{},
+			"invalid_request",
+			"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. Pushed Authorization Requests are enforced for this client but no such request was sent. The request_uri parameter was empty.",
+		},
+		{
+			"ShouldEnforcePARAndErrorOnNonPARRequest",
+			schema.OpenIDConnectClientConfiguration{EnforcePAR: true},
+			true,
+			&fosite.Request{Form: map[string][]string{FormParameterRequestURI: {"https://example.com"}}},
+			"invalid_request",
+			"The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed. Pushed Authorization Requests are enforced for this client but no such request was sent. The request_uri parameter 'https://example.com' is malformed."},
+		{
+			"ShouldEnforcePARAndNotErrorOnPARRequest",
+			schema.OpenIDConnectClientConfiguration{EnforcePAR: true},
+			true,
+			&fosite.Request{Form: map[string][]string{FormParameterRequestURI: {fmt.Sprintf("%sabc", urnPARPrefix)}}},
+			"",
+			"",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			client := NewClient(tc.have)
+
+			assert.Equal(t, tc.expected, client.EnforcePAR)
+
+			if tc.r != nil {
+				err := client.ValidatePARPolicy(tc.r, urnPARPrefix)
+
+				if tc.err != "" {
+					require.NotNil(t, err)
+					assert.EqualError(t, err, tc.err)
+					assert.Equal(t, tc.desc, fosite.ErrorToRFC6749Error(err).WithExposeDebug(true).GetDescription())
+				} else {
+					assert.NoError(t, err)
+				}
+			}
+		})
+	}
+}
+
+func TestNewClientResponseModes(t *testing.T) {
+	testCases := []struct {
+		name     string
+		have     schema.OpenIDConnectClientConfiguration
+		expected []fosite.ResponseModeType
+		r        *fosite.AuthorizeRequest
+		err      string
+		desc     string
+	}{
+		{
+			"ShouldEnforceResponseModePolicyAndAllowDefaultModeQuery",
+			schema.OpenIDConnectClientConfiguration{ResponseModes: []string{ResponseModeQuery}},
+			[]fosite.ResponseModeType{fosite.ResponseModeQuery},
+			&fosite.AuthorizeRequest{DefaultResponseMode: fosite.ResponseModeQuery, ResponseMode: fosite.ResponseModeDefault, Request: fosite.Request{Form: map[string][]string{FormParameterResponseMode: nil}}},
+			"",
+			"",
+		},
+		{
+			"ShouldEnforceResponseModePolicyAndFailOnDefaultMode",
+			schema.OpenIDConnectClientConfiguration{ResponseModes: []string{ResponseModeFormPost}},
+			[]fosite.ResponseModeType{fosite.ResponseModeFormPost},
+			&fosite.AuthorizeRequest{DefaultResponseMode: fosite.ResponseModeQuery, ResponseMode: fosite.ResponseModeDefault, Request: fosite.Request{Form: map[string][]string{FormParameterResponseMode: nil}}},
+			"unsupported_response_mode",
+			"The authorization server does not support obtaining a response using this response mode. The request omitted the response_mode making the default response_mode 'query' based on the other authorization request parameters but registered OAuth 2.0 client doesn't support this response_mode",
+		},
+		{
+			"ShouldNotEnforceConfiguredResponseMode",
+			schema.OpenIDConnectClientConfiguration{ResponseModes: []string{ResponseModeFormPost}},
+			[]fosite.ResponseModeType{fosite.ResponseModeFormPost},
+			&fosite.AuthorizeRequest{DefaultResponseMode: fosite.ResponseModeQuery, ResponseMode: fosite.ResponseModeQuery, Request: fosite.Request{Form: map[string][]string{FormParameterResponseMode: {ResponseModeQuery}}}},
+			"",
+			"",
+		},
+		{
+			"ShouldNotEnforceUnconfiguredResponseMode",
+			schema.OpenIDConnectClientConfiguration{ResponseModes: []string{}},
+			[]fosite.ResponseModeType{},
+			&fosite.AuthorizeRequest{DefaultResponseMode: fosite.ResponseModeQuery, ResponseMode: fosite.ResponseModeDefault, Request: fosite.Request{Form: map[string][]string{FormParameterResponseMode: {ResponseModeQuery}}}},
+			"",
+			"",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			client := NewClient(tc.have)
+
+			assert.Equal(t, tc.expected, client.GetResponseModes())
+
+			if tc.r != nil {
+				err := client.ValidateResponseModePolicy(tc.r)
+
+				if tc.err != "" {
+					require.NotNil(t, err)
+					assert.EqualError(t, err, tc.err)
+					assert.Equal(t, tc.desc, fosite.ErrorToRFC6749Error(err).WithExposeDebug(true).GetDescription())
 				} else {
 					assert.NoError(t, err)
 				}

internal/oidc/const.go

@@ -112,6 +112,7 @@ const (
 
 const (
 	FormParameterRequestURI          = "request_uri"
+	FormParameterResponseMode        = "response_mode"
 	FormParameterCodeChallenge       = "code_challenge"
 	FormParameterCodeChallengeMethod = "code_challenge_method"
 )

internal/oidc/util.go

@@ -0,0 +1,12 @@
+package oidc
+
+import (
+	"strings"
+
+	"github.com/ory/fosite"
+)
+
+// IsPushedAuthorizedRequest returns true if the requester has a PushedAuthorizationRequest redirect_uri value.
+func IsPushedAuthorizedRequest(r fosite.Requester, prefix string) bool {
+	return strings.HasPrefix(r.GetRequestForm().Get(FormParameterRequestURI), prefix)
+}

internal/templates/src/oidc/AuthorizeResponseFormPost.html

@@ -10,10 +10,10 @@
 	</head>
 	<body>
 		<form method="post" action="{{ .RedirURL }}">
-			{{ range $key,$value := .Parameters }}
+			{{ range $key, $value := .Parameters }}
-			{{ range $parameter:= $value}}
+			{{ range $parameter := $value }}
-			<input type="hidden" name="{{$key}}" value="{{$parameter}}"/>
+			<input type="hidden" name="{{ $key }}" value="{{ $parameter }}"/>
-			{{end}}
+			{{ end }}
 			{{ end }}
 		</form>
 	</body>