feat(token_hook): filter out sensitive params instead of only allowin…

…g certain params feat(token_hook): pass client metadata in hook payload
ory · Jul 2, 2024 · 7f342c5 · 7f342c5
1 parent d19e847
commit 7f342c5
Show file tree

Hide file tree

Showing 11 changed files with 88 additions and 7 deletions.
diff --git a/...ass_request_if_strategy_passes-should_call_refresh_token_hook_if_configured-hook=new.json b/...ass_request_if_strategy_passes-should_call_refresh_token_hook_if_configured-hook=new.json
@@ -35,6 +35,7 @@
   },
   "request": {
     "client_id": "app-client",
+    "client_metadata": {},
     "granted_scopes": [
       "offline",
       "openid",

diff --git a/...max_age_is_less_than_auth_time-should_call_refresh_token_hook_if_configured-hook=new.json b/...max_age_is_less_than_auth_time-should_call_refresh_token_hook_if_configured-hook=new.json
@@ -35,6 +35,7 @@
   },
   "request": {
     "client_id": "app-client",
+    "client_metadata": {},
     "granted_scopes": [
       "offline",
       "openid",

diff --git a/..._authentication_time_is_recent-should_call_refresh_token_hook_if_configured-hook=new.json b/..._authentication_time_is_recent-should_call_refresh_token_hook_if_configured-hook=new.json
@@ -35,6 +35,7 @@
   },
   "request": {
     "client_id": "app-client",
+    "client_metadata": {},
     "granted_scopes": [
       "offline",
       "openid",

diff --git a/...ass_request_if_strategy_passes-should_call_refresh_token_hook_if_configured-hook=new.json b/...ass_request_if_strategy_passes-should_call_refresh_token_hook_if_configured-hook=new.json
@@ -35,6 +35,7 @@
   },
   "request": {
     "client_id": "app-client",
+    "client_metadata": {},
     "granted_scopes": [
       "offline",
       "openid",

diff --git a/...max_age_is_less_than_auth_time-should_call_refresh_token_hook_if_configured-hook=new.json b/...max_age_is_less_than_auth_time-should_call_refresh_token_hook_if_configured-hook=new.json
@@ -35,6 +35,7 @@
   },
   "request": {
     "client_id": "app-client",
+    "client_metadata": {},
     "granted_scopes": [
       "offline",
       "openid",

diff --git a/..._authentication_time_is_recent-should_call_refresh_token_hook_if_configured-hook=new.json b/..._authentication_time_is_recent-should_call_refresh_token_hook_if_configured-hook=new.json
@@ -35,6 +35,7 @@
   },
   "request": {
     "client_id": "app-client",
+    "client_metadata": {},
     "granted_scopes": [
       "offline",
       "openid",

diff --git a/oauth2/handler.go b/oauth2/handler.go
@@ -63,6 +63,10 @@ const (
 	DeleteTokensPath = "/oauth2/tokens" // #nosec G101
 )
 
+func GrantTypesSupported() []string {
+	return []string{"authorization_code", "implicit", "client_credentials", "refresh_token"}
+}
+
 type Handler struct {
 	r InternalRegistry
 	c *config.DefaultProvider
@@ -496,7 +500,7 @@ func (h *Handler) discoverOidcConfiguration(w http.ResponseWriter, r *http.Reque
 		IDTokenSigningAlgValuesSupported:       []string{key.Algorithm},
 		IDTokenSignedResponseAlg:               []string{key.Algorithm},
 		UserinfoSignedResponseAlg:              []string{key.Algorithm},
-		GrantTypesSupported:                    []string{"authorization_code", "implicit", "client_credentials", "refresh_token"},
+		GrantTypesSupported:                    GrantTypesSupported(),
 		ResponseModesSupported:                 []string{"query", "fragment"},
 		UserinfoSigningAlgValuesSupported:      []string{"none", key.Algorithm},
 		RequestParameterSupported:              true,

diff --git a/oauth2/oauth2_auth_code_test.go b/oauth2/oauth2_auth_code_test.go
@@ -1347,6 +1347,7 @@ func TestAuthCodeWithMockStrategy(t *testing.T) {
 				RedirectURL: ts.URL + "/callback",
 				Scopes:      []string{"hydra.*", "offline", "openid"},
 			}
+			clientMetadata := json.RawMessage("{}")
 
 			var code string
 			for k, tc := range []struct {
@@ -1665,6 +1666,7 @@ func TestAuthCodeWithMockStrategy(t *testing.T) {
 										require.Equal(t, hookReq.Session.ClientID, oauthConfig.ClientID)
 										require.NotEmpty(t, hookReq.Request)
 										require.Equal(t, hookReq.Request.ClientID, oauthConfig.ClientID)
+										require.Equal(t, hookReq.Request.ClientMetadata, clientMetadata)
 										require.ElementsMatch(t, hookReq.Request.GrantedScopes, expectedGrantedScopes)
 										require.ElementsMatch(t, hookReq.Request.GrantedAudience, []string{})
 										require.Equal(t, hookReq.Request.Payload, map[string][]string{"grant_type": {"refresh_token"}})

diff --git a/oauth2/oauth2_client_credentials_test.go b/oauth2/oauth2_client_credentials_test.go
@@ -264,14 +264,15 @@ func TestClientCredentials(t *testing.T) {
 					var hookReq hydraoauth2.TokenHookRequest
 					require.NoError(t, json.NewDecoder(r.Body).Decode(&hookReq))
 					require.NotEmpty(t, hookReq.Session)
-					require.Equal(t, hookReq.Session.Extra, map[string]interface{}{})
+					require.Equal(t, map[string]interface{}{}, hookReq.Session.Extra)
 					require.NotEmpty(t, hookReq.Request)
 					require.ElementsMatch(t, hookReq.Request.GrantedScopes, expectedGrantedScopes)
 					require.ElementsMatch(t, hookReq.Request.GrantedAudience, expectedGrantedAudience)
-					require.Equal(t, hookReq.Request.Payload, map[string][]string{
+					require.Equal(t, map[string][]string{
+						"audience":   {"https://api.ory.sh/"},
 						"grant_type": {"client_credentials"},
 						"scope":      {"foobar"},
-					})
+					}, hookReq.Request.Payload)
 
 					claims := map[string]interface{}{
 						"hooked": true,

diff --git a/oauth2/token_hook.go b/oauth2/token_hook.go
@@ -7,11 +7,15 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"fmt"
+	"maps"
 	"net/http"
+	"slices"
 
+	"github.com/hashicorp/go-retryablehttp"
 	"github.com/pkg/errors"
 
-	"github.com/hashicorp/go-retryablehttp"
+	"github.com/ory/hydra/v2/client"
 
 	"github.com/ory/hydra/v2/flow"
 	"github.com/ory/hydra/v2/x"
@@ -29,7 +33,8 @@ type AccessRequestHook func(ctx context.Context, requester fosite.AccessRequeste
 // swagger:ignore
 type Request struct {
 	// ClientID is the identifier of the OAuth 2.0 client.
-	ClientID string `json:"client_id"`
+	ClientID       string          `json:"client_id"`
+	ClientMetadata json.RawMessage `json:"client_metadata"`
 	// GrantedScopes is the list of scopes granted to the OAuth 2.0 client.
 	GrantedScopes []string `json:"granted_scopes"`
 	// GrantedAudience is the list of audiences granted to the OAuth 2.0 client.
@@ -150,6 +155,31 @@ func executeHookAndUpdateSession(ctx context.Context, reg x.HTTPClientProvider,
 	return nil
 }
 
+func sensitiveAccessRequestParametersForGrantTypes(grantTypes ...string) []string {
+	var parameters []string
+
+	for i := range grantTypes {
+		switch fosite.GrantType(grantTypes[i]) {
+		case fosite.GrantTypeClientCredentials:
+			parameters = append(parameters, "client_secret")
+		case fosite.GrantTypeAuthorizationCode:
+			parameters = append(parameters, "code", "code_verifier")
+		case fosite.GrantTypeRefreshToken:
+			parameters = append(parameters, "refresh_token")
+		case fosite.GrantTypePassword:
+			parameters = append(parameters, "password")
+		case fosite.GrantTypeImplicit:
+			// Implicit grant doesn't utilize the token endpoint.
+		case fosite.GrantTypeJWTBearer:
+			// Assertion isn't considered sensitive.
+		default:
+			panic(fmt.Sprintf("Can't execute hook for grant type %s without sensitive parameters configured. This is a hydra bug.", grantTypes[i]))
+		}
+	}
+
+	return parameters
+}
+
 // TokenHook is an AccessRequestHook called for all grant types.
 func TokenHook(reg interface {
 	config.Provider
@@ -166,12 +196,28 @@ func TokenHook(reg interface {
 			return nil
 		}
 
+		var clientMeta json.RawMessage
+		if hydraClient, ok := requester.GetClient().(*client.Client); ok {
+			clientMeta = json.RawMessage(hydraClient.Metadata)
+		} else {
+			clientMeta = []byte("null")
+		}
+
+		// Always include client authentication parameters, and any sensitive parameters for the requested grant types.
+		sensitiveParameters := append([]string{"client_secret", "client_assertion"}, sensitiveAccessRequestParametersForGrantTypes(requester.GetGrantTypes()...)...)
+
+		payload := maps.Clone(requester.GetRequestForm())
+		// Delete all sensitive parameters from the access request form before sending it to the hook.
+		maps.DeleteFunc(payload, func(s string, _ []string) bool {
+			return slices.Contains(sensitiveParameters, s)
+		})
 		request := Request{
 			ClientID:        requester.GetClient().GetID(),
+			ClientMetadata:  clientMeta,
 			GrantedScopes:   requester.GetGrantedScopes(),
 			GrantedAudience: requester.GetGrantedAudience(),
 			GrantTypes:      requester.GetGrantTypes(),
-			Payload:         requester.Sanitize([]string{"assertion"}).GetRequestForm(),
+			Payload:         payload,
 		}
 
 		reqBody := TokenHookRequest{

diff --git a/oauth2/token_hook_test.go b/oauth2/token_hook_test.go
@@ -0,0 +1,22 @@
+// Copyright © 2024 Ory Corp
+// SPDX-License-Identifier: Apache-2.0
+
+package oauth2
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_sensitiveAccessRequestParametersForGrantTypes_is_exhaustive(t *testing.T) {
+	for _, grantType := range GrantTypesSupported() {
+		grantType := grantType
+		t.Run(fmt.Sprintf("grant type %s handled", grantType), func(t *testing.T) {
+			assert.NotPanics(t, func() {
+				_ = sensitiveAccessRequestParametersForGrantTypes(grantType)
+			})
+		})
+	}
+}