From 5934e0796063e02a15e68b3a8d3abdd114678758 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Tue, 3 Mar 2026 12:55:17 +0100 Subject: [PATCH] fix: recover from request panics (#11713) # Which Problems Are Solved Panics may cause a service discruption by unexpectedly closing an request's connection. Or in the case of gRPC completely killing the service. Allthough panics are still individual bugs that need to be solved, this PR makes sure a panic is gracefully handled and an understandable error is returned to the client. # How the Problems Are Solved - Recover in the middleware interceptors for the 3 API protocols (HTTP, gRPC, connect). - HTTP middleware uses formatted responses for: - OIDC errors (JSON formatted response) - UI (error page rendering) - SCIM - Upon recovery an alert level log is printed (ERROR+4 for stdlib log handlers) # Additional Context - internal observation --------- Co-authored-by: Livio Spring Co-authored-by: Livio Spring --- backend/v3/instrumentation/slog.go | 13 +++ backend/v3/instrumentation/slog_test.go | 48 ++++++++++ cmd/start/start.go | 7 +- internal/api/grpc/gerrors/zitadel_errors.go | 4 + .../connect_middleware/error_interceptor.go | 22 ++++- .../error_interceptor_test.go | 68 ++++++++------ .../server/connect_middleware/mock_test.go | 10 +- .../server/middleware/error_interceptor.go | 22 ++++- .../middleware/error_interceptor_test.go | 69 ++++++++------ .../api/grpc/server/middleware/mock_test.go | 12 ++- .../http/middleware/recover_interceptor.go | 82 +++++++++++++++++ .../middleware/recover_interceptor_test.go | 91 +++++++++++++++++++ internal/api/oidc/error.go | 6 ++ internal/api/oidc/op.go | 1 + internal/api/scim/server.go | 5 +- internal/api/ui/login/renderer.go | 25 ++--- internal/api/ui/login/router.go | 6 ++ internal/zerrors/zerror.go | 3 + 18 files changed, 418 insertions(+), 76 deletions(-) create mode 100644 internal/api/http/middleware/recover_interceptor.go create mode 100644 internal/api/http/middleware/recover_interceptor_test.go diff --git a/backend/v3/instrumentation/slog.go b/backend/v3/instrumentation/slog.go index c4847908b..51c73123e 100644 --- a/backend/v3/instrumentation/slog.go +++ b/backend/v3/instrumentation/slog.go @@ -2,6 +2,7 @@ package instrumentation import ( "context" + "errors" "log/slog" "os" "slices" @@ -159,6 +160,7 @@ func setLogger(provider *log.LoggerProvider, cfg LogConfig) { Prependers: []slogctx.AttrExtractor{ instanceExtractor, requestIDExtractor, + causeExtractor, slogotel.ExtractTraceSpanID, slogctx.ExtractPrepended, }, @@ -224,6 +226,17 @@ func requestIDExtractor(ctx context.Context, _ time.Time, _ slog.Level, _ string return nil } +// causeExtractor sets the cause of a canceled context to a log entry. +func causeExtractor(ctx context.Context, _ time.Time, _ slog.Level, _ string) []slog.Attr { + err := context.Cause(ctx) + if err != nil && !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded) { + return []slog.Attr{ + slog.String("cause", err.Error()), + } + } + return nil +} + type replacer func(groups []string, a slog.Attr) slog.Attr func chainReplacers(replacers ...replacer) replacer { diff --git a/backend/v3/instrumentation/slog_test.go b/backend/v3/instrumentation/slog_test.go index 077fa8279..9f86a5fe5 100644 --- a/backend/v3/instrumentation/slog_test.go +++ b/backend/v3/instrumentation/slog_test.go @@ -1,8 +1,11 @@ package instrumentation import ( + "context" + "errors" "log/slog" "testing" + "time" "github.com/stretchr/testify/assert" old_logging "github.com/zitadel/logging" //nolint:staticcheck @@ -164,3 +167,48 @@ func TestLogConfig_replacer(t *testing.T) { }) } } + +func Test_causeExtractor(t *testing.T) { + background := context.Background() + canceled, cancel := context.WithCancel(background) + cancel() + timedOut, cancel := context.WithTimeout(background, -1) + defer cancel() + canceledCause, cause := context.WithCancelCause(background) + cause(errors.New("oops")) + + tests := []struct { + name string // description of this test case + ctx context.Context + want []slog.Attr + }{ + { + name: "valid background, no attributes", + ctx: background, + want: nil, + }, + { + name: "regular canceled context", + ctx: canceled, + want: nil, + }, + { + name: "regular timed out context", + ctx: timedOut, + want: nil, + }, + { + name: "canceled context with cause", + ctx: canceledCause, + want: []slog.Attr{ + slog.String("cause", "oops"), + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := causeExtractor(tt.ctx, time.Time{}, 0, "") + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/cmd/start/start.go b/cmd/start/start.go index bb06ac8a1..e6d25ac8b 100644 --- a/cmd/start/start.go +++ b/cmd/start/start.go @@ -434,8 +434,11 @@ func startAPIs( queries, } oidcPrefixes := []string{"/.well-known/openid-configuration", "/oidc/v1", "/oauth/v2"} - // always set the origin in the context if available in the http headers, no matter for what protocol - router.Use(middleware.WithOrigin(config.ExternalSecure, config.HTTP1HostHeader, config.HTTP2HostHeader, config.InstanceHostHeaders, config.PublicHostHeaders)) + router.Use( + middleware.FallbackRecoverHandler(), + // always set the origin in the context if available in the http headers, no matter for what protocol + middleware.WithOrigin(config.ExternalSecure, config.HTTP1HostHeader, config.HTTP2HostHeader, config.InstanceHostHeaders, config.PublicHostHeaders), + ) systemTokenVerifier, err := internal_authz.StartSystemTokenVerifierFromConfig(http_util.BuildHTTP(config.ExternalDomain, config.ExternalPort, config.ExternalSecure), config.SystemAPIUsers) if err != nil { return nil, err diff --git a/internal/api/grpc/gerrors/zitadel_errors.go b/internal/api/grpc/gerrors/zitadel_errors.go index 3a9c6d7a2..d0bddf17b 100644 --- a/internal/api/grpc/gerrors/zitadel_errors.go +++ b/internal/api/grpc/gerrors/zitadel_errors.go @@ -7,6 +7,7 @@ import ( "connectrpc.com/connect" "github.com/jackc/pgx/v5/pgconn" + "github.com/zitadel/sloggcp" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" @@ -119,6 +120,9 @@ func extractError(err error) (c codes.Code, msg, id string, lvl slog.Level) { default: c, lvl = codes.Unknown, slog.LevelError } + if id == zerrors.IDRecover { + lvl = sloggcp.LevelAlert + } return c, msg, id, lvl } diff --git a/internal/api/grpc/server/connect_middleware/error_interceptor.go b/internal/api/grpc/server/connect_middleware/error_interceptor.go index 43d61c2d1..fe5fe983d 100644 --- a/internal/api/grpc/server/connect_middleware/error_interceptor.go +++ b/internal/api/grpc/server/connect_middleware/error_interceptor.go @@ -2,11 +2,13 @@ package connect_middleware import ( "context" + "fmt" "connectrpc.com/connect" "github.com/zitadel/zitadel/internal/api/grpc/gerrors" _ "github.com/zitadel/zitadel/internal/statik" + "github.com/zitadel/zitadel/internal/zerrors" ) func ErrorHandler() connect.UnaryInterceptorFunc { @@ -17,7 +19,21 @@ func ErrorHandler() connect.UnaryInterceptorFunc { } } -func toConnectError(ctx context.Context, req connect.AnyRequest, handler connect.UnaryFunc) (connect.AnyResponse, error) { - resp, err := handler(ctx, req) - return resp, gerrors.ZITADELToConnectError(ctx, err) +func toConnectError(ctx context.Context, req connect.AnyRequest, handler connect.UnaryFunc) (_ connect.AnyResponse, err error) { + ctx, cancel := context.WithCancelCause(ctx) + defer func() { + if rec := recover(); rec != nil { + recErr, ok := rec.(error) + if !ok { + recErr = fmt.Errorf("%v", rec) + } + if recErr != nil { + err = zerrors.ThrowInternal(recErr, zerrors.IDRecover, "Errors.Internal") + } + } + cause := err // avoid passing the transport error as cancel cause. + err = gerrors.ZITADELToConnectError(ctx, err) + cancel(cause) + }() + return handler(ctx, req) } diff --git a/internal/api/grpc/server/connect_middleware/error_interceptor_test.go b/internal/api/grpc/server/connect_middleware/error_interceptor_test.go index 954f2fd58..28a8b259e 100644 --- a/internal/api/grpc/server/connect_middleware/error_interceptor_test.go +++ b/internal/api/grpc/server/connect_middleware/error_interceptor_test.go @@ -2,64 +2,80 @@ package connect_middleware import ( "context" - "reflect" + "errors" "testing" "connectrpc.com/connect" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/zitadel/zitadel/internal/api/authz" ) -func Test_toGRPCError(t *testing.T) { +func Test_toConnectError(t *testing.T) { type args struct { ctx context.Context req connect.AnyRequest handler func(t *testing.T) connect.UnaryFunc } - type res struct { - want interface{} - wantErr bool - } tests := []struct { - name string - args args - res res + name string + args args + want any + wantCode connect.Code }{ { - "no error", - args{ + name: "no error", + args: args{ ctx: context.Background(), req: &mockReq[struct{}]{}, handler: emptyMockHandler(&connect.Response[struct{}]{}, authz.CtxData{}), }, - res{ - &connect.Response[struct{}]{}, - false, - }, + want: &connect.Response[struct{}]{}, }, { - "error", - args{ + name: "error", + args: args{ ctx: context.Background(), req: &mockReq[struct{}]{}, handler: errorMockHandler(), }, - res{ - nil, - true, + want: nil, + wantCode: connect.CodeFailedPrecondition, + }, + { + name: "panic with string", + args: args{ + ctx: context.Background(), + req: &mockReq[struct{}]{}, + handler: panicMockHandler("test panic"), }, + want: nil, + wantCode: connect.CodeInternal, + }, + { + name: "panic with error", + args: args{ + ctx: context.Background(), + req: &mockReq[struct{}]{}, + handler: panicMockHandler(errors.New("oops")), + }, + want: nil, + wantCode: connect.CodeInternal, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := toConnectError(tt.args.ctx, tt.args.req, tt.args.handler(t)) - if (err != nil) != tt.res.wantErr { - t.Errorf("toGRPCError() error = %v, wantErr %v", err, tt.res.wantErr) - return - } - if !reflect.DeepEqual(got, tt.res.want) { - t.Errorf("toGRPCError() got = %v, want %v", got, tt.res.want) + if tt.wantCode != 0 { + var connectErr *connect.Error + require.ErrorAs(t, err, &connectErr) + assert.Equal(t, tt.wantCode, connectErr.Code()) + } else { + require.NoError(t, err) } + assert.Equal(t, tt.want, got) }) } } diff --git a/internal/api/grpc/server/connect_middleware/mock_test.go b/internal/api/grpc/server/connect_middleware/mock_test.go index abd996b01..a691129fb 100644 --- a/internal/api/grpc/server/connect_middleware/mock_test.go +++ b/internal/api/grpc/server/connect_middleware/mock_test.go @@ -24,7 +24,15 @@ func emptyMockHandler(resp connect.AnyResponse, expectedCtxData authz.CtxData) f func errorMockHandler() func(*testing.T) connect.UnaryFunc { return func(t *testing.T) connect.UnaryFunc { return func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) { - return nil, zerrors.ThrowInternal(nil, "test", "error") + return nil, zerrors.ThrowPreconditionFailed(nil, "test", "error") + } + } +} + +func panicMockHandler(payload any) func(*testing.T) connect.UnaryFunc { + return func(t *testing.T) connect.UnaryFunc { + return func(ctx context.Context, req connect.AnyRequest) (connect.AnyResponse, error) { + panic(payload) } } } diff --git a/internal/api/grpc/server/middleware/error_interceptor.go b/internal/api/grpc/server/middleware/error_interceptor.go index 9c4c86526..70b050319 100644 --- a/internal/api/grpc/server/middleware/error_interceptor.go +++ b/internal/api/grpc/server/middleware/error_interceptor.go @@ -2,11 +2,13 @@ package middleware import ( "context" + "fmt" "google.golang.org/grpc" "github.com/zitadel/zitadel/internal/api/grpc/gerrors" _ "github.com/zitadel/zitadel/internal/statik" + "github.com/zitadel/zitadel/internal/zerrors" ) func ErrorHandler() grpc.UnaryServerInterceptor { @@ -15,7 +17,21 @@ func ErrorHandler() grpc.UnaryServerInterceptor { } } -func toGRPCError(ctx context.Context, req interface{}, handler grpc.UnaryHandler) (interface{}, error) { - resp, err := handler(ctx, req) - return resp, gerrors.ZITADELToGRPCError(ctx, err) +func toGRPCError(ctx context.Context, req interface{}, handler grpc.UnaryHandler) (_ interface{}, err error) { + ctx, cancel := context.WithCancelCause(ctx) + defer func() { + if rec := recover(); rec != nil { + recErr, ok := rec.(error) + if !ok { + recErr = fmt.Errorf("%v", rec) + } + if recErr != nil { + err = zerrors.ThrowInternal(recErr, zerrors.IDRecover, "Errors.Internal") + } + } + cause := err // avoid passing the transport error as cancel cause. + err = gerrors.ZITADELToGRPCError(ctx, err) + cancel(cause) + }() + return handler(ctx, req) } diff --git a/internal/api/grpc/server/middleware/error_interceptor_test.go b/internal/api/grpc/server/middleware/error_interceptor_test.go index 82dc35f64..04fc8e35f 100644 --- a/internal/api/grpc/server/middleware/error_interceptor_test.go +++ b/internal/api/grpc/server/middleware/error_interceptor_test.go @@ -2,62 +2,79 @@ package middleware import ( "context" - "reflect" + "errors" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) func Test_toGRPCError(t *testing.T) { type args struct { ctx context.Context - req interface{} + req any handler grpc.UnaryHandler } - type res struct { - want interface{} - wantErr bool - } tests := []struct { - name string - args args - res res + name string + args args + want any + wantCode codes.Code }{ { - "no error", - args{ + name: "no error", + args: args{ ctx: context.Background(), req: &mockReq{}, handler: emptyMockHandler, }, - res{ - &mockReq{}, - false, - }, + want: &mockReq{}, }, { - "error", - args{ + name: "error", + args: args{ ctx: context.Background(), req: &mockReq{}, handler: errorMockHandler, }, - res{ - nil, - true, + want: nil, + wantCode: codes.FailedPrecondition, + }, + { + name: "panic with string", + args: args{ + ctx: context.Background(), + req: &mockReq{}, + handler: panicMockHandler("test panic"), }, + want: nil, + wantCode: codes.Internal, + }, + { + name: "panic with error", + args: args{ + ctx: context.Background(), + req: &mockReq{}, + handler: panicMockHandler(errors.New("oops")), + }, + want: nil, + wantCode: codes.Internal, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got, err := toGRPCError(tt.args.ctx, tt.args.req, tt.args.handler) - if (err != nil) != tt.res.wantErr { - t.Errorf("toGRPCError() error = %v, wantErr %v", err, tt.res.wantErr) - return - } - if !reflect.DeepEqual(got, tt.res.want) { - t.Errorf("toGRPCError() got = %v, want %v", got, tt.res.want) + if tt.wantCode != 0 { + status, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, tt.wantCode, status.Code()) + } else { + require.NoError(t, err) } + assert.Equal(t, tt.want, got) }) } } diff --git a/internal/api/grpc/server/middleware/mock_test.go b/internal/api/grpc/server/middleware/mock_test.go index 498f45082..e3424531d 100644 --- a/internal/api/grpc/server/middleware/mock_test.go +++ b/internal/api/grpc/server/middleware/mock_test.go @@ -8,12 +8,18 @@ import ( "github.com/zitadel/zitadel/internal/zerrors" ) -func emptyMockHandler(_ context.Context, req interface{}) (interface{}, error) { +func emptyMockHandler(_ context.Context, req any) (any, error) { return req, nil } -func errorMockHandler(_ context.Context, req interface{}) (interface{}, error) { - return nil, zerrors.ThrowInternal(nil, "test", "error") +func errorMockHandler(_ context.Context, req any) (any, error) { + return nil, zerrors.ThrowPreconditionFailed(nil, "test", "error") +} + +func panicMockHandler(payload any) func(context.Context, any) (any, error) { + return func(context.Context, any) (any, error) { + panic(payload) + } } type mockReq struct{} diff --git a/internal/api/http/middleware/recover_interceptor.go b/internal/api/http/middleware/recover_interceptor.go new file mode 100644 index 000000000..2f730f9ea --- /dev/null +++ b/internal/api/http/middleware/recover_interceptor.go @@ -0,0 +1,82 @@ +package middleware + +import ( + "context" + "fmt" + "net/http" + + "github.com/zitadel/sloggcp" + + "github.com/zitadel/zitadel/backend/v3/instrumentation/logging" + "github.com/zitadel/zitadel/internal/zerrors" +) + +// RecoverHandler recovers from panics in the HTTP handler chain +// and calls the provided writeResponse function to write an appropriate response to the client. +// +// The request context is canceled with the panic error as the cause. +func RecoverHandler(writeResponse func(w http.ResponseWriter, r *http.Request, err error)) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx, cancel := context.WithCancelCause(r.Context()) + r = r.WithContext(ctx) + + defer func() { + var err error + if rec := recover(); rec != nil { + recErr, ok := rec.(error) + if !ok { + recErr = fmt.Errorf("%v", rec) + } + err = zerrors.ThrowInternal(recErr, zerrors.IDRecover, "Errors.Internal") + logRecovered(ctx, err) + writeResponse(w, r, err) + } + cancel(err) + }() + + next.ServeHTTP(w, r) + }) + } +} + +// FallbackRecoverHandler recovers from panics in the HTTP handler chain +// and returns a 500 Internal Server Error response. +// The request context is canceled with the panic error as the cause, +// so that any ongoing operations can be stopped and cleaned up. +// +// The response is sent as a text/plain response. +// It is used as a last line of defense to prevent the server from crashing +// due to panics in the handlers. +// Protocols (OIDC, SAML, HTML etc.) should use [RecoverHandler] to write +// properly formatted error responses to the clients. +func FallbackRecoverHandler() func(http.Handler) http.Handler { + return RecoverHandler(func(w http.ResponseWriter, r *http.Request, _ error) { + http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) + }) +} + +// RecoverHandlerWithError is similar to [RecoverHandler] but returns an error instead of writing a response directly. +func RecoverHandlerWithError(next HandlerFuncWithError) HandlerFuncWithError { + return func(w http.ResponseWriter, r *http.Request) (err error) { + ctx, cancel := context.WithCancelCause(r.Context()) + r = r.WithContext(ctx) + defer func() { + if rec := recover(); rec != nil { + recErr, ok := rec.(error) + if !ok { + recErr = fmt.Errorf("%v", rec) + } + err = zerrors.ThrowInternal(recErr, zerrors.IDRecover, "Errors.Internal") + logRecovered(ctx, err) + } + cancel(err) + }() + return next(w, r) + } +} + +func logRecovered(ctx context.Context, err error) { + logger := logging.FromCtx(ctx) + logger.Log(ctx, sloggcp.LevelAlert, "recovered from panic", "err", err) +} diff --git a/internal/api/http/middleware/recover_interceptor_test.go b/internal/api/http/middleware/recover_interceptor_test.go new file mode 100644 index 000000000..3a636f561 --- /dev/null +++ b/internal/api/http/middleware/recover_interceptor_test.go @@ -0,0 +1,91 @@ +package middleware + +import ( + "errors" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/zitadel/zitadel/internal/zerrors" +) + +func TestRecoverHandler(t *testing.T) { + tests := []struct { + name string + handler http.HandlerFunc + wantStatus int + wantBody string + }{ + { + name: "no panic", + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + io.WriteString(w, "ok") //nolint:errcheck + }, + wantStatus: http.StatusOK, + wantBody: "ok", + }, + { + name: "panic with error", + handler: func(w http.ResponseWriter, r *http.Request) { + panic(errors.New("oops")) + }, + wantStatus: http.StatusInternalServerError, + wantBody: "ID=RECOVER Message=Errors.Internal Parent=(oops)", + }, + { + name: "panic with string", + handler: func(w http.ResponseWriter, r *http.Request) { + panic("something went wrong") + }, + wantStatus: http.StatusInternalServerError, + wantBody: "ID=RECOVER Message=Errors.Internal Parent=(something went wrong)", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + handler := RecoverHandler(writeResponse)(tt.handler) + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/", nil) + handler.ServeHTTP(w, r) + + res := w.Result() + defer res.Body.Close() + body, _ := io.ReadAll(res.Body) + assert.Equal(t, tt.wantStatus, res.StatusCode) + assert.Equal(t, tt.wantBody, string(body)) + }) + } +} + +func TestFallbackRecoverHandler(t *testing.T) { + handler := FallbackRecoverHandler()(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + panic("unexpected error") + })) + + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/", nil) + handler.ServeHTTP(w, r) + + res := w.Result() + defer res.Body.Close() + body, _ := io.ReadAll(res.Body) + assert.Equal(t, http.StatusInternalServerError, res.StatusCode) + assert.Equal(t, "Internal Server Error\n", string(body)) +} + +func TestRecoverHandlerWithError(t *testing.T) { + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, "/", nil) + // Panic on nil handler. + err := RecoverHandlerWithError(nil)(w, r) + assert.ErrorIs(t, err, zerrors.ThrowInternal(nil, zerrors.IDRecover, "Errors.Internal")) +} + +func writeResponse(w http.ResponseWriter, _ *http.Request, err error) { + w.WriteHeader(http.StatusInternalServerError) + io.WriteString(w, err.Error()) //nolint:errcheck +} diff --git a/internal/api/oidc/error.go b/internal/api/oidc/error.go index 5cba33044..7ed7644dc 100644 --- a/internal/api/oidc/error.go +++ b/internal/api/oidc/error.go @@ -3,10 +3,12 @@ package oidc import ( "context" "errors" + "net/http" "github.com/zitadel/oidc/v3/pkg/oidc" "github.com/zitadel/oidc/v3/pkg/op" + "github.com/zitadel/zitadel/backend/v3/instrumentation/logging" http_util "github.com/zitadel/zitadel/internal/api/http" "github.com/zitadel/zitadel/internal/zerrors" ) @@ -47,3 +49,7 @@ func oidcError(ctx context.Context, err error) error { oidcErr.Description = zError.GetMessage() return op.NewStatusError(oidcErr, statusCode) } + +func writeRecoverError(w http.ResponseWriter, r *http.Request, err error) { + op.WriteError(w, r, err, logging.FromCtx(r.Context())) +} diff --git a/internal/api/oidc/op.go b/internal/api/oidc/op.go index dae87964d..35576774c 100644 --- a/internal/api/oidc/op.go +++ b/internal/api/oidc/op.go @@ -186,6 +186,7 @@ func NewServer( middleware.MetricsHandler(metricTypes), middleware.TraceHandler(), middleware.LogHandler("oidc"), + middleware.RecoverHandler(writeRecoverError), middleware.NoCacheInterceptor().Handler, instanceHandler, userAgentCookie, diff --git a/internal/api/scim/server.go b/internal/api/scim/server.go index 38b7128e8..ae2228f36 100644 --- a/internal/api/scim/server.go +++ b/internal/api/scim/server.go @@ -70,7 +70,10 @@ func buildMiddleware( middlewares []zhttp_middlware.MiddlewareWithErrorFunc, ) zhttp_middlware.ErrorHandlerFunc { // content type middleware needs to run at the very beginning to correctly set content types of errors - middlewares = append([]zhttp_middlware.MiddlewareWithErrorFunc{smiddleware.ContentTypeMiddleware}, middlewares...) + middlewares = append([]zhttp_middlware.MiddlewareWithErrorFunc{ + smiddleware.ContentTypeMiddleware, + zhttp_middlware.RecoverHandlerWithError, + }, middlewares...) middlewares = append(middlewares, smiddleware.ScimContextMiddleware(query)) scimMiddleware := zhttp_middlware.ChainedWithErrorHandler(serrors.ErrorHandler(translator), middlewares...) return func(handler zhttp_middlware.HandlerFuncWithError) http.Handler { diff --git a/internal/api/ui/login/renderer.go b/internal/api/ui/login/renderer.go index d0238ce6e..edd733a41 100644 --- a/internal/api/ui/login/renderer.go +++ b/internal/api/ui/login/renderer.go @@ -11,8 +11,9 @@ import ( "time" "github.com/gorilla/csrf" - "github.com/zitadel/logging" + old_logging "github.com/zitadel/logging" //nolint:staticcheck + "github.com/zitadel/zitadel/backend/v3/instrumentation/logging" "github.com/zitadel/zitadel/cmd/build" "github.com/zitadel/zitadel/internal/api/authz" http_mw "github.com/zitadel/zitadel/internal/api/http/middleware" @@ -249,7 +250,7 @@ func CreateRenderer(pathPrefix string, staticStorage static.Storage, cookieName tmplMapping, funcs, cookieName, ) - logging.New().OnError(err).WithError(err).Panic("error creating renderer") + old_logging.New().OnError(err).WithError(err).Panic("error creating renderer") return r } @@ -342,18 +343,20 @@ func (l *Login) chooseNextStep(w http.ResponseWriter, r *http.Request, authReq * } func (l *Login) renderInternalError(w http.ResponseWriter, r *http.Request, authReq *domain.AuthRequest, err error) { + ctx := r.Context() if err != nil { - log := logging.WithError(err) + log := logging.FromCtx(ctx).With("err", err) if authReq != nil { - log = log.WithField("auth_req_id", authReq.ID) + log = log.With("auth_req_id", authReq.ID) } + const msg = "render internal error" if zerrors.IsInternal(err) { - log.Error() + log.ErrorContext(ctx, msg) } else { - log.Info() + log.InfoContext(ctx, msg) } } - translator := l.getTranslator(r.Context(), authReq) + translator := l.getTranslator(ctx, authReq) data := l.getBaseData(r, authReq, translator, "Errors.Internal", "", err) l.renderer.RenderTemplate(w, r, translator, l.renderer.Templates[tmplError], data, nil) } @@ -568,7 +571,7 @@ func (l *Login) getOrgPrimaryDomain(r *http.Request, authReq *domain.AuthRequest } org, err := l.query.OrgByID(r.Context(), orgID) if err != nil { - logging.New().WithError(err).Error("cannot get default org") + old_logging.New().WithError(err).Error("cannot get default org") return "" } return org.Domain @@ -591,7 +594,7 @@ func (l *Login) addLoginTranslations(translator *i18n.Translator, customTexts [] Text: text.Text, } err := l.renderer.AddMessages(translator, text.Language, msg) - logging.OnError(err).Warn("could no add message to translator") + old_logging.OnError(err).Warn("could no add message to translator") } } @@ -599,7 +602,7 @@ func (l *Login) customTexts(ctx context.Context, translator *i18n.Translator, or instanceID := authz.GetInstance(ctx).InstanceID() instanceTexts, err := l.query.CustomTextListByTemplate(ctx, instanceID, domain.LoginCustomText, false) if err != nil { - logging.WithFields("instanceID", instanceID).Warn("unable to load custom texts for instance") + old_logging.WithFields("instanceID", instanceID).Warn("unable to load custom texts for instance") return } l.addLoginTranslations(translator, query.CustomTextsToDomain(instanceTexts)) @@ -608,7 +611,7 @@ func (l *Login) customTexts(ctx context.Context, translator *i18n.Translator, or } orgTexts, err := l.query.CustomTextListByTemplate(ctx, orgID, domain.LoginCustomText, false) if err != nil { - logging.WithFields("instanceID", instanceID, "org", orgID).Warn("unable to load custom texts for org") + old_logging.WithFields("instanceID", instanceID, "org", orgID).Warn("unable to load custom texts for org") return } l.addLoginTranslations(translator, query.CustomTextsToDomain(orgTexts)) diff --git a/internal/api/ui/login/router.go b/internal/api/ui/login/router.go index 459a0aab5..6f8ab5537 100644 --- a/internal/api/ui/login/router.go +++ b/internal/api/ui/login/router.go @@ -4,6 +4,7 @@ import ( "net/http" "github.com/gorilla/mux" + "github.com/zitadel/zitadel/internal/api/http/middleware" ) const ( @@ -71,6 +72,7 @@ var ( func CreateRouter(login *Login, interceptors ...mux.MiddlewareFunc) *mux.Router { router := mux.NewRouter() router.Use(interceptors...) + router.Use(middleware.RecoverHandler(login.writeRecoverError)) router.HandleFunc(EndpointRoot, login.handleLogin).Methods(http.MethodGet) router.HandleFunc(EndpointHealthz, login.handleHealthz).Methods(http.MethodGet) router.HandleFunc(EndpointReadiness, login.handleReadiness).Methods(http.MethodGet) @@ -131,3 +133,7 @@ func CreateRouter(login *Login, interceptors ...mux.MiddlewareFunc) *mux.Router router.HandleFunc(EndpointDeviceAuthAction, login.handleDeviceAuthAction).Methods(http.MethodGet, http.MethodPost) return router } + +func (l *Login) writeRecoverError(w http.ResponseWriter, r *http.Request, err error) { + l.renderError(w, r, nil, err) +} diff --git a/internal/zerrors/zerror.go b/internal/zerrors/zerror.go index bb3b6ba58..040e27025 100644 --- a/internal/zerrors/zerror.go +++ b/internal/zerrors/zerror.go @@ -80,6 +80,9 @@ const ( KindUnauthenticated Kind = 16 ) +// IDRecover is the error ID used for errors created after recovery from panics. +const IDRecover = "RECOVER" + // Because errors are created through singletons, config is global. var ( enableReportLocation atomic.Bool