refactor(oidc): split userinfoFlows and fix uncaught panic/context leak

userinfoFlows mixed two unrelated flows in one large function with no
tests. A panic while building goja context fields (e.g. from a
non-marshalable claim) could escape unrecovered and skip the per-action
context cancel. Split the function into focused pieces, recover panics
per action with the context always canceled via defer, and add tests.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

internal/api/oidc/userinfo.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"maps"
 	"net/http"
@@ -21,6 +22,7 @@ import (
 	"github.com/zitadel/zitadel/internal/api/authz"
 	"github.com/zitadel/zitadel/internal/domain"
 	"github.com/zitadel/zitadel/internal/execution"
+	target_domain "github.com/zitadel/zitadel/internal/execution/target"
 	"github.com/zitadel/zitadel/internal/query"
 	exec_repo "github.com/zitadel/zitadel/internal/repository/execution"
 	"github.com/zitadel/zitadel/internal/telemetry/tracing"
@@ -309,16 +311,28 @@ func setUserInfoUserGroups(userGroups []query.UserInfoUserGroup, out *oidc.UserI
 	out.AppendClaims(ClaimUserGroups, groups)
 }
 
-//nolint:gocognit
 func (s *Server) userinfoFlows(ctx context.Context, qu *query.OIDCUserInfo, userInfo *oidc.UserInfo, triggerType domain.TriggerType, clientID string) (err error) {
 	ctx, span := tracing.NewSpan(ctx)
 	defer func() { span.EndWithError(err) }()
 
+	if err := s.runUserinfoActionFlows(ctx, qu, userInfo, triggerType, clientID); err != nil {
+		return err
+	}
+	return s.runUserinfoExecutionFlow(ctx, qu, userInfo, triggerType, clientID)
+}
+
+// runUserinfoActionFlows loads the legacy, DB-configured Actions for triggerType and runs them.
+func (s *Server) runUserinfoActionFlows(ctx context.Context, qu *query.OIDCUserInfo, userInfo *oidc.UserInfo, triggerType domain.TriggerType, clientID string) error {
 	queriedActions, err := s.query.GetActiveActionsByFlowAndTriggerType(ctx, domain.FlowTypeCustomiseToken, triggerType, qu.User.ResourceOwner)
 	if err != nil {
 		return err
 	}
+	return s.runUserinfoActions(ctx, qu, userInfo, clientID, queriedActions)
+}
 
+// runUserinfoActions runs the given, already queried Actions. It is kept separate from
+// runUserinfoActionFlows so it can be unit tested without a DB by injecting queriedActions directly.
+func (s *Server) runUserinfoActions(ctx context.Context, qu *query.OIDCUserInfo, userInfo *oidc.UserInfo, clientID string, queriedActions []*query.Action) error {
 	ctxFields := actions.SetContextFields(
 		actions.SetFields("v1",
 			actions.SetFields("claims", userinfoClaims(userInfo)),
@@ -355,102 +369,148 @@ func (s *Server) userinfoFlows(ctx context.Context, qu *query.OIDCUserInfo, user
 	)
 
 	for _, action := range queriedActions {
-		actionCtx, cancel := context.WithTimeout(ctx, action.Timeout())
-		claimLogs := []string{}
+		// Actions.contextFields/apiFields are unexported types in package actions, so ctxFields
+		// can't be passed as a typed parameter to a separate function - it's captured by this
+		// closure instead. The closure also gives each iteration its own deferred cancel() and
+		// recover(), so a timeout-bound context never outlives its iteration and a panic from
+		// building or running the script (whether from a goja-exposed closure or from
+		// constructing ctxFields/apiFields itself) is converted into a returned error instead
+		// of escaping to the caller.
+		runAction := func(action *query.Action) (err error) {
+			actionCtx, cancel := context.WithTimeout(ctx, action.Timeout())
+			defer cancel()
+			defer func() {
+				if r := recover(); r != nil {
+					err = errorFromRecover(r)
+				}
+			}()
 
-		apiFields := actions.WithAPIFields(
-			actions.SetFields("v1",
-				actions.SetFields("userinfo",
-					actions.SetFields("setClaim", func(key string, value interface{}) {
-						if strings.HasPrefix(key, ClaimPrefix) {
-							return
-						}
-						if userInfo.Claims[key] == nil {
-							userInfo.AppendClaims(key, value)
-							return
-						}
-						claimLogs = append(claimLogs, fmt.Sprintf("key %q already exists", key))
-					}),
-					actions.SetFields("appendLogIntoClaims", func(entry string) {
-						claimLogs = append(claimLogs, entry)
-					}),
+			claimLogs := []string{}
+			setClaim := func(key string, value interface{}) {
+				appendOrLogClaim(userInfo, key, value, &claimLogs)
+			}
+			appendLogIntoClaims := func(entry string) {
+				claimLogs = append(claimLogs, entry)
+			}
+
+			apiFields := actions.WithAPIFields(
+				actions.SetFields("v1",
+					actions.SetFields("userinfo",
+						actions.SetFields("setClaim", setClaim),
+						actions.SetFields("appendLogIntoClaims", appendLogIntoClaims),
+					),
+					actions.SetFields("claims",
+						actions.SetFields("setClaim", setClaim),
+						actions.SetFields("appendLogIntoClaims", appendLogIntoClaims),
+					),
+					actions.SetFields("user",
+						actions.SetFields("setMetadata", func(call goja.FunctionCall) goja.Value {
+							if len(call.Arguments) != 2 {
+								panic("exactly 2 (key, value) arguments expected")
+							}
+							key := call.Arguments[0].Export().(string)
+							val := call.Arguments[1].Export()
+
+							value, err := json.Marshal(val)
+							if err != nil {
+								logging.WithError(err).Debug("unable to marshal")
+								panic(err)
+							}
+
+							metadata := &domain.Metadata{
+								Key:   key,
+								Value: value,
+							}
+							if _, err = s.command.SetUserMetadata(ctx, metadata, userInfo.Subject, qu.User.ResourceOwner, nil); err != nil {
+								logging.WithError(err).Info("unable to set md in action")
+								panic(err)
+							}
+							return nil
+						}),
+					),
 				),
-				actions.SetFields("claims",
-					actions.SetFields("setClaim", func(key string, value interface{}) {
-						if strings.HasPrefix(key, ClaimPrefix) {
-							return
-						}
-						if userInfo.Claims[key] == nil {
-							userInfo.AppendClaims(key, value)
-							return
-						}
-						claimLogs = append(claimLogs, fmt.Sprintf("key %q already exists", key))
-					}),
-					actions.SetFields("appendLogIntoClaims", func(entry string) {
-						claimLogs = append(claimLogs, entry)
-					}),
-				),
-				actions.SetFields("user",
-					actions.SetFields("setMetadata", func(call goja.FunctionCall) goja.Value {
-						if len(call.Arguments) != 2 {
-							panic("exactly 2 (key, value) arguments expected")
-						}
-						key := call.Arguments[0].Export().(string)
-						val := call.Arguments[1].Export()
+			)
 
-						value, err := json.Marshal(val)
-						if err != nil {
-							logging.WithError(err).Debug("unable to marshal")
-							panic(err)
-						}
+			if err := actions.Run(
+				actionCtx,
+				ctxFields,
+				apiFields,
+				action.Script,
+				action.Name,
+				append(actions.ActionToOptions(action), actions.WithHTTP(actionCtx, s.httpClient), actions.WithUUID(actionCtx))...,
+			); err != nil {
+				return err
+			}
+			if len(claimLogs) > 0 {
+				userInfo.AppendClaims(fmt.Sprintf(ClaimActionLogFormat, action.Name), claimLogs)
+			}
+			return nil
+		}
 
-						metadata := &domain.Metadata{
-							Key:   key,
-							Value: value,
-						}
-						if _, err = s.command.SetUserMetadata(ctx, metadata, userInfo.Subject, qu.User.ResourceOwner, nil); err != nil {
-							logging.WithError(err).Info("unable to set md in action")
-							panic(err)
-						}
-						return nil
-					}),
-				),
-			),
-		)
-
-		err = actions.Run(
-			actionCtx,
-			ctxFields,
-			apiFields,
-			action.Script,
-			action.Name,
-			append(actions.ActionToOptions(action), actions.WithHTTP(actionCtx, s.httpClient), actions.WithUUID(actionCtx))...,
-		)
-		cancel()
-		if err != nil {
+		if err := runAction(action); err != nil {
 			return err
 		}
-		if len(claimLogs) > 0 {
-			userInfo.AppendClaims(fmt.Sprintf(ClaimActionLogFormat, action.Name), claimLogs)
-		}
 	}
+	return nil
+}
 
-	var function string
-	switch triggerType {
-	case domain.TriggerTypePreUserinfoCreation:
-		function = exec_repo.ID(domain.ExecutionTypeFunction, domain.ActionFunctionPreUserinfo.LocalizationKey())
-	case domain.TriggerTypePreAccessTokenCreation:
-		function = exec_repo.ID(domain.ExecutionTypeFunction, domain.ActionFunctionPreAccessToken.LocalizationKey())
-	case domain.TriggerTypeUnspecified, domain.TriggerTypePostAuthentication, domain.TriggerTypePreCreation, domain.TriggerTypePostCreation, domain.TriggerTypePreSAMLResponseCreation:
-		// added for linting, there should never be any trigger type be used here besides PreUserinfo and PreAccessToken
+// appendOrLogClaim adds value under key unless key is reserved (ClaimPrefix) or already set,
+// in which case a conflict message is appended to logs instead. Shared by the setClaim
+// goja closures and the execution-response AppendClaims merge.
+func appendOrLogClaim(userInfo *oidc.UserInfo, key string, value any, logs *[]string) {
+	if strings.HasPrefix(key, ClaimPrefix) {
+		return
+	}
+	if userInfo.Claims[key] == nil {
+		userInfo.AppendClaims(key, value)
+		return
+	}
+	*logs = append(*logs, fmt.Sprintf("key %q already exists", key))
+}
+
+// errorFromRecover converts a recovered panic value into an error, mirroring the idiom
+// internal/actions.executeFn already uses internally for panics from goja-exposed closures.
+func errorFromRecover(r any) error {
+	if err, ok := r.(error); ok {
 		return err
 	}
+	if s, ok := r.(string); ok {
+		return errors.New(s)
+	}
+	return fmt.Errorf("unknown error occurred: %v", r)
+}
 
+// functionForTriggerType resolves the Execution function key for a trigger type, or ""
+// if no Execution runs for that trigger type. The switch is kept exhaustive (no default)
+// so the `exhaustive` linter forces a conscious decision whenever a new domain.TriggerType
+// is added, rather than silently falling through.
+func functionForTriggerType(triggerType domain.TriggerType) string {
+	switch triggerType {
+	case domain.TriggerTypePreUserinfoCreation:
+		return exec_repo.ID(domain.ExecutionTypeFunction, domain.ActionFunctionPreUserinfo.LocalizationKey())
+	case domain.TriggerTypePreAccessTokenCreation:
+		return exec_repo.ID(domain.ExecutionTypeFunction, domain.ActionFunctionPreAccessToken.LocalizationKey())
+	case domain.TriggerTypeUnspecified, domain.TriggerTypePostAuthentication, domain.TriggerTypePreCreation, domain.TriggerTypePostCreation, domain.TriggerTypePreSAMLResponseCreation:
+		// there should never be any trigger type used here besides PreUserinfo and PreAccessToken
+		return ""
+	}
+	return ""
+}
+
+// runUserinfoExecutionFlow resolves and runs the new-style Executions (webhook targets) for triggerType.
+func (s *Server) runUserinfoExecutionFlow(ctx context.Context, qu *query.OIDCUserInfo, userInfo *oidc.UserInfo, triggerType domain.TriggerType, clientID string) error {
+	function := functionForTriggerType(triggerType)
 	if function == "" {
 		return nil
 	}
-
 	executionTargets := execution.QueryExecutionTargetsForFunction(ctx, function)
+	return s.runUserinfoExecutionTargets(ctx, qu, userInfo, clientID, function, executionTargets)
+}
+
+// runUserinfoExecutionTargets calls the given, already resolved Execution targets. It is kept
+// separate from runUserinfoExecutionFlow so it can be unit tested without authz/DB access by
+// injecting executionTargets directly (e.g. pointing at httptest servers).
+func (s *Server) runUserinfoExecutionTargets(ctx context.Context, qu *query.OIDCUserInfo, userInfo *oidc.UserInfo, clientID, function string, executionTargets []target_domain.Target) error {
 	info := &ContextInfo{
 		Function:     function,
 		UserInfo:     userInfo,
@@ -469,28 +529,26 @@ func (s *Server) userinfoFlows(ctx context.Context, qu *query.OIDCUserInfo, user
 	if !ok || contextInfoResponse == nil {
 		return nil
 	}
+	s.applyContextInfoResponse(ctx, qu, userInfo, function, contextInfoResponse)
+	return nil
+}
+
+// applyContextInfoResponse merges an Execution's response back into userInfo: applying
+// requested user metadata writes and appending claims, logging any conflicts along the way.
+func (s *Server) applyContextInfoResponse(ctx context.Context, qu *query.OIDCUserInfo, userInfo *oidc.UserInfo, function string, resp *ContextInfoResponse) {
 	claimLogs := make([]string, 0)
-	for _, metadata := range contextInfoResponse.SetUserMetadata {
-		if _, err = s.command.SetUserMetadata(ctx, metadata, userInfo.Subject, qu.User.ResourceOwner, nil); err != nil {
+	for _, metadata := range resp.SetUserMetadata {
+		if _, err := s.command.SetUserMetadata(ctx, metadata, userInfo.Subject, qu.User.ResourceOwner, nil); err != nil {
 			claimLogs = append(claimLogs, fmt.Sprintf("failed to set user metadata key %q", metadata.Key))
 		}
 	}
-	for _, claim := range contextInfoResponse.AppendClaims {
-		if strings.HasPrefix(claim.Key, ClaimPrefix) {
-			continue
-		}
-		if userInfo.Claims[claim.Key] == nil {
-			userInfo.AppendClaims(claim.Key, claim.Value)
-			continue
-		}
-		claimLogs = append(claimLogs, fmt.Sprintf("key %q already exists", claim.Key))
+	for _, claim := range resp.AppendClaims {
+		appendOrLogClaim(userInfo, claim.Key, claim.Value, &claimLogs)
 	}
-	claimLogs = append(claimLogs, contextInfoResponse.AppendLogClaims...)
+	claimLogs = append(claimLogs, resp.AppendLogClaims...)
 	if len(claimLogs) > 0 {
 		userInfo.AppendClaims(fmt.Sprintf(ClaimActionLogFormat, function), claimLogs)
 	}
-
-	return nil
 }
 
 type ContextInfo struct {

internal/api/oidc/userinfo_test.go

@@ -3,15 +3,25 @@ package oidc
 import (
 	"context"
 	"encoding/base64"
+	"errors"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"github.com/zitadel/oidc/v3/pkg/oidc"
 	"golang.org/x/text/language"
 
+	"github.com/zitadel/zitadel/internal/actions"
 	"github.com/zitadel/zitadel/internal/domain"
+	target_domain "github.com/zitadel/zitadel/internal/execution/target"
+	"github.com/zitadel/zitadel/internal/logstore"
+	"github.com/zitadel/zitadel/internal/logstore/record"
 	"github.com/zitadel/zitadel/internal/query"
+	exec_repo "github.com/zitadel/zitadel/internal/repository/execution"
 )
 
 func Test_prepareRoles(t *testing.T) {
@@ -558,3 +568,230 @@ func Test_userInfoToOIDC(t *testing.T) {
 		})
 	}
 }
+
+func Test_functionForTriggerType(t *testing.T) {
+	tests := []struct {
+		name        string
+		triggerType domain.TriggerType
+		want        string
+	}{
+		{
+			name:        "pre userinfo creation",
+			triggerType: domain.TriggerTypePreUserinfoCreation,
+			want:        exec_repo.ID(domain.ExecutionTypeFunction, domain.ActionFunctionPreUserinfo.LocalizationKey()),
+		},
+		{
+			name:        "pre access token creation",
+			triggerType: domain.TriggerTypePreAccessTokenCreation,
+			want:        exec_repo.ID(domain.ExecutionTypeFunction, domain.ActionFunctionPreAccessToken.LocalizationKey()),
+		},
+		{
+			name:        "unspecified",
+			triggerType: domain.TriggerTypeUnspecified,
+			want:        "",
+		},
+		{
+			name:        "post authentication",
+			triggerType: domain.TriggerTypePostAuthentication,
+			want:        "",
+		},
+		{
+			name:        "pre creation",
+			triggerType: domain.TriggerTypePreCreation,
+			want:        "",
+		},
+		{
+			name:        "post creation",
+			triggerType: domain.TriggerTypePostCreation,
+			want:        "",
+		},
+		{
+			name:        "pre saml response creation",
+			triggerType: domain.TriggerTypePreSAMLResponseCreation,
+			want:        "",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, functionForTriggerType(tt.triggerType))
+		})
+	}
+}
+
+func Test_appendOrLogClaim(t *testing.T) {
+	tests := []struct {
+		name       string
+		userInfo   *oidc.UserInfo
+		key        string
+		value      any
+		wantClaims map[string]any
+		wantLogs   []string
+	}{
+		{
+			name:       "new claim is added",
+			userInfo:   &oidc.UserInfo{Claims: map[string]any{}},
+			key:        "foo",
+			value:      "bar",
+			wantClaims: map[string]any{"foo": "bar"},
+		},
+		{
+			name:       "reserved prefix is skipped",
+			userInfo:   &oidc.UserInfo{Claims: map[string]any{}},
+			key:        ClaimPrefix + ":something",
+			value:      "bar",
+			wantClaims: map[string]any{},
+		},
+		{
+			name:       "existing claim is not overwritten and logs a conflict",
+			userInfo:   &oidc.UserInfo{Claims: map[string]any{"foo": "existing"}},
+			key:        "foo",
+			value:      "bar",
+			wantClaims: map[string]any{"foo": "existing"},
+			wantLogs:   []string{`key "foo" already exists`},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var logs []string
+			appendOrLogClaim(tt.userInfo, tt.key, tt.value, &logs)
+			assert.Equal(t, tt.wantClaims, tt.userInfo.Claims)
+			assert.Equal(t, tt.wantLogs, logs)
+		})
+	}
+}
+
+func Test_errorFromRecover(t *testing.T) {
+	tests := []struct {
+		name    string
+		r       any
+		wantMsg string
+	}{
+		{
+			name:    "error value is passed through",
+			r:       errors.New("boom"),
+			wantMsg: "boom",
+		},
+		{
+			name:    "string value is wrapped",
+			r:       "boom",
+			wantMsg: "boom",
+		},
+		{
+			name:    "other value falls back to a generic message",
+			r:       42,
+			wantMsg: "unknown error occurred: 42",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := errorFromRecover(tt.r)
+			require.Error(t, err)
+			assert.Equal(t, tt.wantMsg, err.Error())
+		})
+	}
+}
+
+func Test_runUserinfoActions(t *testing.T) {
+	actions.SetLogstoreService(logstore.New[*record.ExecutionLog](nil, nil))
+
+	t.Run("happy path sets claim from script", func(t *testing.T) {
+		s := &Server{}
+		qu := &query.OIDCUserInfo{User: &query.User{ID: "user1", ResourceOwner: "org1"}}
+		userInfo := &oidc.UserInfo{Subject: "user1", Claims: map[string]any{}}
+		queriedActions := []*query.Action{
+			{
+				Name: "testFunc",
+				Script: `function testFunc(ctx, api) {
+					api.v1.userinfo.setClaim("foo", "bar")
+				}`,
+			},
+		}
+
+		err := s.runUserinfoActions(context.Background(), qu, userInfo, "clientID", queriedActions)
+		require.NoError(t, err)
+		assert.Equal(t, "bar", userInfo.Claims["foo"])
+	})
+
+	t.Run("panic while building ctxFields is recovered, not left uncaught", func(t *testing.T) {
+		s := &Server{}
+		qu := &query.OIDCUserInfo{User: &query.User{ID: "user1", ResourceOwner: "org1"}}
+		// A non-JSON-marshalable claim value makes userinfoClaims' eager json.Marshal panic
+		// while ctxFields are being built for this (or a later) action - a phase that runs
+		// before internal/actions.executeScript's own recover is armed. Without our own
+		// recover in the loop, this panic would escape uncaught and skip cancel().
+		userInfo := &oidc.UserInfo{Subject: "user1", Claims: map[string]any{"bad": make(chan int)}}
+		queriedActions := []*query.Action{
+			{Name: "testFunc", Script: `function testFunc(ctx, api) {}`},
+		}
+
+		err := s.runUserinfoActions(context.Background(), qu, userInfo, "clientID", queriedActions)
+		require.Error(t, err)
+	})
+
+	t.Run("panic from setMetadata bad arg count is recovered", func(t *testing.T) {
+		s := &Server{}
+		qu := &query.OIDCUserInfo{User: &query.User{ID: "user1", ResourceOwner: "org1"}}
+		userInfo := &oidc.UserInfo{Subject: "user1", Claims: map[string]any{}}
+		queriedActions := []*query.Action{
+			{
+				Name: "testFunc",
+				Script: `function testFunc(ctx, api) {
+					api.v1.user.setMetadata("onlyonearg")
+				}`,
+			},
+		}
+
+		err := s.runUserinfoActions(context.Background(), qu, userInfo, "clientID", queriedActions)
+		require.Error(t, err)
+		assert.Contains(t, err.Error(), "exactly 2")
+	})
+
+	t.Run("error from a script stops the loop and is returned", func(t *testing.T) {
+		s := &Server{}
+		qu := &query.OIDCUserInfo{User: &query.User{ID: "user1", ResourceOwner: "org1"}}
+		userInfo := &oidc.UserInfo{Subject: "user1", Claims: map[string]any{}}
+		queriedActions := []*query.Action{
+			{
+				Name:   "testFunc",
+				Script: `function testFunc(ctx, api) { throw new Error("script failure") }`,
+			},
+			{
+				Name: "shouldNotRun",
+				Script: `function shouldNotRun(ctx, api) {
+					api.v1.userinfo.setClaim("unreachable", "value")
+				}`,
+			},
+		}
+
+		err := s.runUserinfoActions(context.Background(), qu, userInfo, "clientID", queriedActions)
+		require.Error(t, err)
+		assert.NotContains(t, userInfo.Claims, "unreachable")
+	})
+}
+
+func Test_runUserinfoExecutionTargets(t *testing.T) {
+	respBody := []byte(`{"append_claims":[{"key":"foo","value":"bar"},{"key":"foo","value":"baz"}],"append_log_claims":["extra log entry"]}`)
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write(respBody)
+	}))
+	defer server.Close()
+
+	s := &Server{httpClient: server.Client()}
+	qu := &query.OIDCUserInfo{User: &query.User{ID: "user1", ResourceOwner: "org1"}}
+	userInfo := &oidc.UserInfo{Subject: "user1", Claims: map[string]any{}}
+	targets := []target_domain.Target{
+		{TargetType: target_domain.TargetTypeCall, Endpoint: server.URL, Timeout: 5 * time.Second},
+	}
+
+	err := s.runUserinfoExecutionTargets(context.Background(), qu, userInfo, "clientID", "function/test", targets)
+	require.NoError(t, err)
+
+	assert.Equal(t, "bar", userInfo.Claims["foo"])
+	logClaim, ok := userInfo.Claims[fmt.Sprintf(ClaimActionLogFormat, "function/test")]
+	require.True(t, ok)
+	logs, ok := logClaim.([]string)
+	require.True(t, ok)
+	assert.Contains(t, logs, `key "foo" already exists`)
+	assert.Contains(t, logs, "extra log entry")
+}