From dba6261c8ed0dfd00ffde6193910b1405b17432f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Mon, 10 Aug 2026 10:24:42 +0000 Subject: [PATCH] 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 --- internal/api/oidc/userinfo.go | 250 ++++++++++++++++++----------- internal/api/oidc/userinfo_test.go | 237 +++++++++++++++++++++++++++ 2 files changed, 391 insertions(+), 96 deletions(-) diff --git a/internal/api/oidc/userinfo.go b/internal/api/oidc/userinfo.go index ca3607f3d0..3a53ee850d 100644 --- a/internal/api/oidc/userinfo.go +++ b/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 { diff --git a/internal/api/oidc/userinfo_test.go b/internal/api/oidc/userinfo_test.go index 2bb8556b2b..e63d3b48e3 100644 --- a/internal/api/oidc/userinfo_test.go +++ b/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") +}