diff --git a/go.mod b/go.mod index 6a1a832b..874f4b5e 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,7 @@ require ( github.com/gomarkdown/markdown v0.0.0-20260824154242-13c5cf49db8d github.com/google/uuid v1.6.0 github.com/hashicorp/go-retryablehttp v0.7.8 - github.com/labstack/echo/v4 v4.15.4 + github.com/labstack/echo/v5 v5.3.1 github.com/labstack/gommon v0.5.0 github.com/mholt/archives v0.1.5 github.com/microcosm-cc/bluemonday v1.0.27 @@ -114,8 +114,6 @@ require ( github.com/tklauser/go-sysconf v0.4.0 // indirect github.com/tklauser/numcpus v0.12.0 // indirect github.com/ulikunitz/xz v0.5.16 // indirect - github.com/valyala/bytebufferpool v1.0.0 // indirect - github.com/valyala/fasttemplate v1.2.2 // indirect github.com/yusufpapurcu/wmi v1.2.4 // indirect go.opentelemetry.io/auto/sdk v1.2.1 // indirect go.opentelemetry.io/contrib/bridges/prometheus v0.71.0 // indirect diff --git a/go.sum b/go.sum index a98eeef4..ae94f7ea 100644 --- a/go.sum +++ b/go.sum @@ -131,8 +131,8 @@ github.com/klauspost/pgzip v1.2.6 h1:8RXeL5crjEUFnR2/Sn6GJNWtSQ3Dk8pq4CL3jvdDyjU github.com/klauspost/pgzip v1.2.6/go.mod h1:Ch1tH69qFZu15pkjo5kYi6mth2Zzwzt50oCQKQE9RUs= github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc= github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= -github.com/labstack/echo/v4 v4.15.4 h1:DL45vVYa+BWE+XuW+zZNd9H0YEdZ80UAWJGcTVW4EVs= -github.com/labstack/echo/v4 v4.15.4/go.mod h1:CuMetKIRwsuO/qlAgMq+KTAalwGoB/h4tC+yPdrTj1g= +github.com/labstack/echo/v5 v5.3.1 h1:75maCxkQVGualckLc/5s/ihgpH1a1Dc6AuGWNVNs6bw= +github.com/labstack/echo/v5 v5.3.1/go.mod h1:4iEGNQiPPZnkfYpNR/L6fINd3NLiGWUD5+eBotFALas= github.com/labstack/gommon v0.5.0 h1:6VSQ2NOzsnEJ5W6+84E0RbcaDDmgB6NIAzWCczTEe6c= github.com/labstack/gommon v0.5.0/go.mod h1:Rzlg7HHy1maLfzBYGg9NZcVuz1sA68HHhLjhcEllYE0= github.com/ledongthuc/pdf v0.0.0-20220302134840-0c2507a12d80 h1:6Yzfa6GP0rIo/kULo2bwGEkFvCePZ3qHDDTC3/J9Swo= @@ -231,10 +231,6 @@ github.com/tklauser/numcpus v0.12.0/go.mod h1:ABHeXzJnr/qqwguhClkZKT1/8VABcYrsyU github.com/ulikunitz/xz v0.5.8/go.mod h1:nbz6k7qbPmH4IRqmfOplQw/tblSgqTqBwxkY0oWt/14= github.com/ulikunitz/xz v0.5.16 h1:ld6NyySjx5lowVKwJvMRLnW5nxKX/xnpSiFYZ/Lxur0= github.com/ulikunitz/xz v0.5.16/go.mod h1:H9Rt/W6/Qj27PGauhQc6nfCDy7vHpzsOThBSaYDoEhw= -github.com/valyala/bytebufferpool v1.0.0 h1:GqA5TC/0021Y/b9FG4Oi9Mr3q7XYx6KllzawFIhcdPw= -github.com/valyala/bytebufferpool v1.0.0/go.mod h1:6bBcMArwyJ5K/AmCkWv1jt77kVWyCJ6HpOuEn7z0Csc= -github.com/valyala/fasttemplate v1.2.2 h1:lxLXG0uE3Qnshl9QyaK6XJxMXlQZELvChBOCmQD0Loo= -github.com/valyala/fasttemplate v1.2.2/go.mod h1:KHLXt3tVN2HBp8eijSv/kGJopbvo7S+qRAEEKiv+SiQ= github.com/xyproto/randomstring v1.0.5 h1:YtlWPoRdgMu3NZtP45drfy1GKoojuR7hmRcnhZqKjWU= github.com/xyproto/randomstring v1.0.5/go.mod h1:rgmS5DeNXLivK7YprL0pY+lTuhNQW3iGxZ18UQApw/E= github.com/yusufpapurcu/wmi v1.2.4 h1:zFUKzehAFReQwLys1b/iSMl+JQGSCSjtVqQn9bBrPo0= diff --git a/pkg/modules/api/api.go b/pkg/modules/api/api.go index 5fdb3371..1b50283d 100644 --- a/pkg/modules/api/api.go +++ b/pkg/modules/api/api.go @@ -13,9 +13,8 @@ import ( "github.com/alexliesenfeld/health" "github.com/dlclark/regexp2" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" flag "github.com/spf13/pflag" - "golang.org/x/net/http2" "golang.org/x/sync/errgroup" "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" @@ -58,6 +57,12 @@ type Api struct { fs *gotenberg.FileSystem logger *slog.Logger srv *echo.Echo + + // shutdownCancel triggers the graceful shutdown of the server started by + // [echo.StartConfig]. Echo v5 drives shutdown from a context instead of an + // Echo.Shutdown method. serveDone closes once that shutdown completed. + shutdownCancel context.CancelFunc + serveDone chan struct{} } type downloadFromConfig struct { @@ -134,7 +139,7 @@ const ( // middleware := Middleware{ // Handler: func() echo.MiddlewareFunc { // return func(next echo.HandlerFunc) echo.HandlerFunc { -// return func(c echo.Context) error { +// return func(c *echo.Context) error { // rootPath := c.Get("rootPath").(string) // healthURI := fmt.Sprintf("%shealth", rootPath) // @@ -536,16 +541,29 @@ func (a *Api) Validate() error { return nil } +// newEchoServer builds the [echo.Echo] instance with the settings Gotenberg +// relies on, independently of the routes and middlewares added by [Api.Start]. +func newEchoServer() *echo.Echo { + srv := echo.New() + srv.HTTPErrorHandler = httpErrorHandler() + // Echo v5 serves files through Echo.Filesystem, an [fs.FS] rooted at the + // working directory, and [fs.FS] rejects absolute names. Every output file + // lives under the request's temporary directory, so Context.Attachment gets + // an absolute path and the default filesystem answers 404 for every + // conversion. Rooting at "/" restores the v4 behavior, where the path was + // opened as-is. + srv.Filesystem = echo.NewDefaultFS("/") + // Echo v5 dropped the X-Forwarded-For and X-Real-IP fallbacks from + // Context.RealIP. Keep the previous behavior so that the access log still + // reports the client IP when Gotenberg sits behind a reverse proxy. + srv.IPExtractor = echo.LegacyIPExtractor() + + return srv +} + // Start starts the HTTP server. func (a *Api) Start() error { - a.srv = echo.New() - a.srv.HideBanner = true - a.srv.HidePort = true - a.srv.Server.ReadTimeout = a.timeout - a.srv.Server.IdleTimeout = a.timeout - // See https://github.com/gotenberg/gotenberg/issues/396. - a.srv.Server.WriteTimeout = a.timeout + a.timeout - a.srv.HTTPErrorHandler = httpErrorHandler() + a.srv = newEchoServer() // Let's prepare the modules' routes. var disableTelemetryForPaths []string @@ -609,7 +627,7 @@ func (a *Api) Start() error { securityMiddleware = oidcAuthMiddleware(verifier) default: securityMiddleware = func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { + return func(c *echo.Context) error { return next(c) } } @@ -641,7 +659,7 @@ func (a *Api) Start() error { // Root route. a.srv.GET( a.rootPath, - func(c echo.Context) error { + func(c *echo.Context) error { return c.HTML(http.StatusOK, `Hey, Gotenberg has no UI, it's an API. Head to the documentation to learn how to interact with it 🚀`) }, securityMiddleware, @@ -650,7 +668,7 @@ func (a *Api) Start() error { // Favicon route. a.srv.GET( fmt.Sprintf("%s%s", a.rootPath, "favicon.ico"), - func(c echo.Context) error { + func(c *echo.Context) error { return c.NoContent(http.StatusNoContent) }, securityMiddleware, @@ -681,7 +699,7 @@ func (a *Api) Start() error { // ...the version route. a.srv.GET( fmt.Sprintf("%s%s", a.rootPath, "version"), - func(c echo.Context) error { + func(c *echo.Context) error { return c.String(http.StatusOK, gotenberg.Version) }, securityMiddleware, @@ -691,7 +709,7 @@ func (a *Api) Start() error { if a.enableDebugRoute { a.srv.GET( fmt.Sprintf("%s%s", a.rootPath, "debug"), - func(c echo.Context) error { + func(c *echo.Context) error { return c.JSONPretty(http.StatusOK, gotenberg.Debug(), " ") }, securityMiddleware, @@ -712,18 +730,50 @@ func (a *Api) Start() error { return fmt.Errorf("waiting for modules readiness: %w", err) } + startConfig := echo.StartConfig{ + Address: fmt.Sprintf("%s:%d", a.bindIp, a.port), + HideBanner: true, + HidePort: true, + BeforeServeFunc: func(s *http.Server) error { + s.ReadTimeout = a.timeout + s.IdleTimeout = a.timeout + // See https://github.com/gotenberg/gotenberg/issues/396. + s.WriteTimeout = a.timeout + a.timeout + + if a.tlsCertFile == "" || a.tlsKeyFile == "" { + // Serve HTTP/2 Cleartext (h2c). Echo v5 dropped + // StartH2CServer and golang.org/x/net/http2/h2c is deprecated, + // so the standard library serves h2c through Server.Protocols + // instead. + protocols := new(http.Protocols) + protocols.SetHTTP1(true) + protocols.SetUnencryptedHTTP2(true) + s.Protocols = protocols + } + + return nil + }, + } + + // Not named cancel: that would reassign the readiness timeout's cancel from + // above, whose deferred call is already bound to the old value. + serveCtx, serveCancel := context.WithCancel(context.Background()) + a.shutdownCancel = serveCancel + a.serveDone = make(chan struct{}) + // As the following code is blocking, run it in a goroutine. go func() { + defer close(a.serveDone) + var err error if a.tlsCertFile != "" && a.tlsKeyFile != "" { // Start an HTTPS server (supports HTTP/2). - err = a.srv.StartTLS(fmt.Sprintf("%s:%d", a.bindIp, a.port), a.tlsCertFile, a.tlsKeyFile) + err = startConfig.StartTLS(serveCtx, a.srv, a.tlsCertFile, a.tlsKeyFile) } else { // Start an HTTP/2 Cleartext (non-HTTPS) server. - server := &http2.Server{} - err = a.srv.StartH2CServer(fmt.Sprintf("%s:%d", a.bindIp, a.port), server) + err = startConfig.Start(serveCtx, a.srv) } - if !errors.Is(err, http.ErrServerClosed) { + if err != nil && !errors.Is(err, http.ErrServerClosed) { a.logger.ErrorContext(context.Background(), err.Error()) } }() @@ -740,6 +790,26 @@ func (a *Api) StartupMessage() string { return fmt.Sprintf("server started on %s:%d", ip, a.port) } +// shutdown triggers the server's graceful shutdown and waits for it to +// complete, or for ctx to be done. Cancelling the serve context is what Echo v5 +// exposes in place of Echo.Shutdown: [echo.StartConfig] shuts the server down +// on its own deadline, so the shutdown proceeds even when ctx is already done. +func (a *Api) shutdown(ctx context.Context) error { + if a.shutdownCancel == nil { + // Start never ran, so there is nothing to shut down. + return nil + } + + a.shutdownCancel() + + select { + case <-a.serveDone: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} + // Stop stops the HTTP server. func (a *Api) Stop(ctx context.Context) error { for { @@ -749,7 +819,7 @@ func (a *Api) Stop(ctx context.Context) error { } select { case <-ctx.Done(): - return a.srv.Shutdown(ctx) + return a.shutdown(ctx) default: a.logger.DebugContext(ctx, fmt.Sprintf("%d asynchronous requests", count)) if count > 0 { @@ -757,7 +827,7 @@ func (a *Api) Stop(ctx context.Context) error { continue } a.logger.DebugContext(ctx, "no more asynchronous requests, continue with shutdown") - err := a.srv.Shutdown(ctx) + err := a.shutdown(ctx) if err != nil { return fmt.Errorf("shutdown: %w", err) } diff --git a/pkg/modules/api/context.go b/pkg/modules/api/context.go index 1e1cc476..ae2a8b9b 100644 --- a/pkg/modules/api/context.go +++ b/pkg/modules/api/context.go @@ -18,7 +18,7 @@ import ( "github.com/google/uuid" "github.com/hashicorp/go-retryablehttp" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "github.com/mholt/archives" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/codes" @@ -70,7 +70,7 @@ type Context struct { outputFilename string logger *slog.Logger - echoCtx echo.Context + echoCtx *echo.Context mkdirAll gotenberg.MkdirAll pathRename gotenberg.PathRename context.Context @@ -158,7 +158,7 @@ type downloadFrom struct { } // newContext returns a [Context] by parsing a "multipart/form-data" request. -func newContext(echoCtx echo.Context, logger *slog.Logger, fs *gotenberg.FileSystem, timeout time.Duration, bodyLimit int64, downloadFromCfg downloadFromConfig) (*Context, context.CancelFunc, error) { +func newContext(echoCtx *echo.Context, logger *slog.Logger, fs *gotenberg.FileSystem, timeout time.Duration, bodyLimit int64, downloadFromCfg downloadFromConfig) (*Context, context.CancelFunc, error) { processCtx, processCancel := context.WithTimeout(echoCtx.Request().Context(), timeout) // We want to make sure the multipart/form-data does not exceed a given diff --git a/pkg/modules/api/context_test.go b/pkg/modules/api/context_test.go index 8857f657..e1e361b2 100644 --- a/pkg/modules/api/context_test.go +++ b/pkg/modules/api/context_test.go @@ -20,7 +20,7 @@ import ( "time" "github.com/dlclark/regexp2" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" ) diff --git a/pkg/modules/api/middlewares.go b/pkg/modules/api/middlewares.go index 337e2310..c06c543d 100644 --- a/pkg/modules/api/middlewares.go +++ b/pkg/modules/api/middlewares.go @@ -12,8 +12,8 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/google/uuid" - "github.com/labstack/echo/v4" - "github.com/labstack/echo/v4/middleware" + "github.com/labstack/echo/v5" + "github.com/labstack/echo/v5/middleware" "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" @@ -37,10 +37,12 @@ var ( // ParseError parses an error and returns the corresponding HTTP status and // HTTP message. func ParseError(err error) (int, string) { - var echoErr *echo.HTTPError - ok := errors.As(err, &echoErr) - if ok { - return echoErr.Code, http.StatusText(echoErr.Code) + // [echo.StatusCode] also matches the router's ErrNotFound and + // ErrMethodNotAllowed sentinels, which Echo v5 no longer models as + // [echo.HTTPError]. Matching that type alone would let every unrouted + // request fall through to a 500. + if code := echo.StatusCode(err); code != 0 { + return code, http.StatusText(code) } if errors.Is(err, context.DeadlineExceeded) { @@ -100,14 +102,14 @@ const statusClientClosedRequest = 499 // A server-side timeout is [context.DeadlineExceeded], mapped to 503 by // [ParseError], and is deliberately not treated as a client abort. // See https://github.com/gotenberg/gotenberg/issues/1627. -func requestCanceled(c echo.Context, err error) bool { +func requestCanceled(c *echo.Context, err error) bool { return errors.Is(err, context.Canceled) && errors.Is(c.Request().Context().Err(), context.Canceled) } // httpErrorHandler is the centralized HTTP error handler. It parses the error, // returns a response as "text/plain; charset=UTF-8". func httpErrorHandler() echo.HTTPErrorHandler { - return func(err error, c echo.Context) { + return func(c *echo.Context, err error) { logger := c.Get("logger").(*slog.Logger) if requestCanceled(c, err) { @@ -134,7 +136,7 @@ func httpErrorHandler() echo.HTTPErrorHandler { // startTime := c.Get("startTime").(time.Time) func latencyMiddleware() echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { + return func(c *echo.Context) error { // First piece for calculating the latency. startTime := time.Now() c.Set("startTime", startTime) @@ -159,7 +161,7 @@ func latencyMiddleware() echo.MiddlewareFunc { // } func rootPathMiddleware(rootPath string) echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { + return func(c *echo.Context) error { c.Set("rootPath", rootPath) // Call the next middleware in the chain. return next(c) @@ -173,7 +175,7 @@ func rootPathMiddleware(rootPath string) echo.MiddlewareFunc { // outputFilename := c.Get("outputFilename").(string) func outputFilenameMiddleware() echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { + return func(c *echo.Context) error { filename := c.Request().Header.Get("Gotenberg-Output-Filename") // Keep only the last path segment, so that a caller cannot name an // output file after a path. @@ -203,10 +205,18 @@ func telemetryMiddleware(logger *slog.Logger, serverName, correlationIdHeader st semconvSrv := semconvutil.NewHTTPServer(meter) return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { + return func(c *echo.Context) error { startTime := c.Get("startTime").(time.Time) rootPath := c.Get("rootPath").(string) + // Echo v5 returns the bare [http.ResponseWriter] from + // Context.Response, so unwrap it to read the recorded status and + // size below. + response, errUnwrap := echo.UnwrapResponse(c.Response()) + if errUnwrap != nil { + return fmt.Errorf("unwrap response: %w", errUnwrap) + } + request := c.Request() savedCtx := request.Context() defer func() { @@ -239,7 +249,7 @@ func telemetryMiddleware(logger *slog.Logger, serverName, correlationIdHeader st err := next(c) if err != nil { - c.Error(err) + c.Echo().HTTPErrorHandler(c, err) } return nil } @@ -288,7 +298,7 @@ func telemetryMiddleware(logger *slog.Logger, serverName, correlationIdHeader st err := next(c) finishTime := time.Now() - status := c.Response().Status + status := response.Status canceled := false if err != nil { canceled = requestCanceled(c, err) @@ -300,13 +310,13 @@ func telemetryMiddleware(logger *slog.Logger, serverName, correlationIdHeader st } span.SetAttributes(attribute.String("error", err.Error())) - c.Error(err) + c.Echo().HTTPErrorHandler(c, err) } span.SetStatus(semconvSrv.Status(status)) span.SetAttributes(semconvSrv.ResponseTraceAttrs(semconvutil.ResponseTelemetry{ StatusCode: status, - WriteBytes: c.Response().Size, + WriteBytes: response.Size, })...) // Pick the level and message before building the record: err.Error @@ -343,11 +353,11 @@ func telemetryMiddleware(logger *slog.Logger, serverName, correlationIdHeader st slog.String("path", routePath), slog.String("referer", c.Request().Referer()), slog.String("user_agent", c.Request().UserAgent()), - slog.Int("status", c.Response().Status), + slog.Int("status", response.Status), slog.Int64("latency", int64(latency)), slog.String("latency_human", latency.String()), slog.Int64("bytes_in", c.Request().ContentLength), - slog.Int64("bytes_out", c.Response().Size), + slog.Int64("bytes_out", response.Size), ) additionalAttributes := []attribute.KeyValue{ @@ -356,7 +366,7 @@ func telemetryMiddleware(logger *slog.Logger, serverName, correlationIdHeader st semconvSrv.RecordMetrics(ctx, semconvutil.ServerMetricData{ ServerName: serverName, - ResponseSize: c.Response().Size, + ResponseSize: response.Size, Req: request, StatusCode: status, AdditionalAttributes: additionalAttributes, @@ -371,7 +381,7 @@ func telemetryMiddleware(logger *slog.Logger, serverName, correlationIdHeader st // basicAuthMiddleware manages basic authentication. func basicAuthMiddleware(username, password string) echo.MiddlewareFunc { - return middleware.BasicAuth(func(u string, p string, e echo.Context) (bool, error) { + return middleware.BasicAuth(func(c *echo.Context, u string, p string) (bool, error) { if subtle.ConstantTimeCompare([]byte(u), []byte(username)) == 1 && subtle.ConstantTimeCompare([]byte(p), []byte(password)) == 1 { return true, nil @@ -417,7 +427,7 @@ func (a *Api) buildOidcVerifier() (*oidc.IDTokenVerifier, error) { // it to the client. func oidcAuthMiddleware(verifier *oidc.IDTokenVerifier) echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { + return func(c *echo.Context) error { rawToken, ok := strings.CutPrefix(c.Request().Header.Get("Authorization"), "Bearer ") if !ok || rawToken == "" { return echo.NewHTTPError(http.StatusUnauthorized, "a Bearer token is required in the Authorization header") @@ -446,7 +456,7 @@ func oidcAuthMiddleware(verifier *oidc.IDTokenVerifier) echo.MiddlewareFunc { // cancel := c.Get("cancel").(context.CancelFunc) func contextMiddleware(fs *gotenberg.FileSystem, timeout time.Duration, bodyLimit int64, downloadFromCfg downloadFromConfig) echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { + return func(c *echo.Context) error { logger, _ := c.Get("logger").(*slog.Logger) if logger == nil { return errors.New("no logger in context (possible pool reuse)") @@ -507,7 +517,7 @@ func contextMiddleware(fs *gotenberg.FileSystem, timeout time.Duration, bodyLimi // handler fails to timeout as expected. func hardTimeoutMiddleware(hardTimeout time.Duration) echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { - return func(c echo.Context) error { + return func(c *echo.Context) error { // Guard the type assertion so a pooled [echo.Context] whose // store has been recycled under us does not crash the process. // See the webhook async handler for the race this protects diff --git a/pkg/modules/api/middlewares_test.go b/pkg/modules/api/middlewares_test.go index 4bc23d72..d2caa456 100644 --- a/pkg/modules/api/middlewares_test.go +++ b/pkg/modules/api/middlewares_test.go @@ -1,6 +1,7 @@ package api import ( + "bytes" "context" "crypto/rand" "crypto/rsa" @@ -9,13 +10,15 @@ import ( "log/slog" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" "time" "github.com/coreos/go-oidc/v3/oidc" "github.com/coreos/go-oidc/v3/oidc/oidctest" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" ) // TestRequestCanceled pins the client-abort discriminator: only a @@ -73,7 +76,7 @@ func TestHttpErrorHandler_ClientClosedRequest(t *testing.T) { c := echo.New().NewContext(req, rec) c.Set("logger", slog.New(slog.DiscardHandler)) - httpErrorHandler()(tc.err, c) + httpErrorHandler()(c, tc.err) if rec.Code != tc.wantStatus { t.Fatalf("status = %d, want %d", rec.Code, tc.wantStatus) @@ -105,7 +108,7 @@ func TestOutputFilenameMiddleware(t *testing.T) { {"control characters", "fo\x01o\x7f", "foo"}, } { t.Run(tc.name, func(t *testing.T) { - handler := outputFilenameMiddleware()(func(c echo.Context) error { return nil }) + handler := outputFilenameMiddleware()(func(c *echo.Context) error { return nil }) req := httptest.NewRequest(http.MethodPost, "/", nil) if tc.header != "" { @@ -131,7 +134,7 @@ func TestOutputFilenameMiddleware(t *testing.T) { func TestHardTimeoutMiddleware_MissingLoggerReturnsErrorInsteadOfPanicking(t *testing.T) { mw := hardTimeoutMiddleware(100 * time.Millisecond) - handler := mw(func(c echo.Context) error { return nil }) + handler := mw(func(c *echo.Context) error { return nil }) e := echo.New() req := httptest.NewRequest(http.MethodGet, "/", nil) @@ -220,7 +223,7 @@ func TestOidcAuthMiddleware(t *testing.T) { } c := echo.New().NewContext(req, httptest.NewRecorder()) - handler := oidcAuthMiddleware(verifier)(func(c echo.Context) error { + handler := oidcAuthMiddleware(verifier)(func(c *echo.Context) error { return c.NoContent(http.StatusOK) }) @@ -243,3 +246,114 @@ func TestOidcAuthMiddleware(t *testing.T) { }) } } + +// TestParseError_StatusMapping pins the statuses [ParseError] derives from the +// errors Echo and Gotenberg produce. +// +// Echo v5 models the router's ErrNotFound and ErrMethodNotAllowed as an +// unexported type rather than [echo.HTTPError], so matching that type alone +// would turn every unrouted request into a 500. It also guards the ordering: +// Gotenberg's own [SentinelHttpError] carries a client-facing message and must +// not be shadowed by the generic status lookup. +func TestParseError_StatusMapping(t *testing.T) { + for _, tc := range []struct { + scenario string + err error + wantStatus int + wantMessage string + }{ + {"router not found", echo.ErrNotFound, http.StatusNotFound, http.StatusText(http.StatusNotFound)}, + {"router method not allowed", echo.ErrMethodNotAllowed, http.StatusMethodNotAllowed, http.StatusText(http.StatusMethodNotAllowed)}, + {"explicit HTTP error", echo.NewHTTPError(http.StatusUnauthorized, "nope"), http.StatusUnauthorized, http.StatusText(http.StatusUnauthorized)}, + {"wrapped HTTP error", fmt.Errorf("authenticate request: %w", echo.NewHTTPError(http.StatusUnauthorized, "nope")), http.StatusUnauthorized, http.StatusText(http.StatusUnauthorized)}, + {"sentinel keeps its message", NewSentinelHttpError(http.StatusBadRequest, "Invalid 'foo' form field value"), http.StatusBadRequest, "Invalid 'foo' form field value"}, + {"unknown error", errors.New("boom"), http.StatusInternalServerError, http.StatusText(http.StatusInternalServerError)}, + } { + t.Run(tc.scenario, func(t *testing.T) { + status, message := ParseError(tc.err) + if status != tc.wantStatus { + t.Fatalf("status = %d, want %d", status, tc.wantStatus) + } + if message != tc.wantMessage { + t.Fatalf("message = %q, want %q", message, tc.wantMessage) + } + }) + } +} + +// TestNewEchoServer_RealIP pins the client IP extraction that the access log's +// "remote_ip" field depends on. +// +// Echo v5.1.0 dropped the X-Forwarded-For and X-Real-IP fallbacks from +// Context.RealIP, so without an explicit extractor a Gotenberg behind a reverse +// proxy would log the proxy's address for every request. [newEchoServer] +// restores the previous behavior. +func TestNewEchoServer_RealIP(t *testing.T) { + srv := newEchoServer() + if srv.IPExtractor == nil { + t.Fatal("no IPExtractor configured: remote_ip would report the proxy address") + } + + for _, tc := range []struct { + scenario string + headers map[string]string + want string + }{ + {"x-forwarded-for keeps the client, not the proxy", map[string]string{"X-Forwarded-For": "203.0.113.7, 70.41.3.18"}, "203.0.113.7"}, + {"single x-forwarded-for", map[string]string{"X-Forwarded-For": "203.0.113.7"}, "203.0.113.7"}, + {"bracketed IPv6 is unwrapped", map[string]string{"X-Forwarded-For": "[2001:db8::1], 70.41.3.18"}, "2001:db8::1"}, + {"x-real-ip when no x-forwarded-for", map[string]string{"X-Real-IP": "203.0.113.9"}, "203.0.113.9"}, + {"no headers falls back to the remote address", nil, "192.0.2.1"}, + } { + t.Run(tc.scenario, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "192.0.2.1:1234" + for key, value := range tc.headers { + req.Header.Set(key, value) + } + + c := srv.NewContext(req, httptest.NewRecorder()) + + if got := c.RealIP(); got != tc.want { + t.Fatalf("RealIP = %q, want %q", got, tc.want) + } + }) + } +} + +// TestNewEchoServer_AttachmentServesAbsolutePath pins the filesystem that every +// conversion response is sent through. +// +// Echo v5 serves files through Echo.Filesystem, an [fs.FS] rooted at the working +// directory, and [fs.FS] rejects absolute names. Gotenberg builds every output +// file under the request's temporary directory and hands Context.Attachment an +// absolute path, so with the default filesystem every conversion route answers +// 404 while still reading the whole upload. +func TestNewEchoServer_AttachmentServesAbsolutePath(t *testing.T) { + path := filepath.Join(t.TempDir(), "output.pdf") + want := []byte("%PDF-1.7 not really a PDF") + + err := os.WriteFile(path, want, 0o600) + if err != nil { + t.Fatalf("write fixture: %v", err) + } + + srv := newEchoServer() + rec := httptest.NewRecorder() + c := srv.NewContext(httptest.NewRequest(http.MethodPost, "/", nil), rec) + + err = c.Attachment(path, "output.pdf") + if err != nil { + t.Fatalf("Attachment(%q) = %v, want nil", path, err) + } + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK) + } + if got := rec.Body.Bytes(); !bytes.Equal(got, want) { + t.Fatalf("body = %q, want %q", got, want) + } + if got := rec.Header().Get("Content-Disposition"); !strings.Contains(got, `filename="output.pdf"`) { + t.Fatalf("Content-Disposition = %q, want it to carry filename=\"output.pdf\"", got) + } +} diff --git a/pkg/modules/api/mocks.go b/pkg/modules/api/mocks.go index b9bdc4fc..a7bf7903 100644 --- a/pkg/modules/api/mocks.go +++ b/pkg/modules/api/mocks.go @@ -4,7 +4,7 @@ import ( "log/slog" "github.com/alexliesenfeld/health" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" ) @@ -83,7 +83,7 @@ func (ctx *ContextMock) SetLogger(logger *slog.Logger) { // // ctx := &api.ContextMock{Context: &api.Context{}} // ctx.setEchoContext(c) -func (ctx *ContextMock) SetEchoContext(c echo.Context) { +func (ctx *ContextMock) SetEchoContext(c *echo.Context) { ctx.echoCtx = c } diff --git a/pkg/modules/chromium/routes.go b/pkg/modules/chromium/routes.go index 705935d5..e960dd38 100644 --- a/pkg/modules/chromium/routes.go +++ b/pkg/modules/chromium/routes.go @@ -16,7 +16,7 @@ import ( "github.com/dlclark/regexp2" "github.com/gomarkdown/markdown" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "github.com/microcosm-cc/bluemonday" "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" @@ -466,7 +466,7 @@ func convertUrlRoute(chromium Api, engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/chromium/convert/url", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form, options := FormDataChromiumPdfOptions(ctx) mode := pdfengines.FormDataPdfSplitMode(form, false) @@ -528,7 +528,7 @@ func screenshotUrlRoute(chromium Api) api.Route { Method: http.MethodPost, Path: "/forms/chromium/screenshot/url", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form, options := FormDataChromiumScreenshotOptions(ctx) @@ -562,7 +562,7 @@ func convertHtmlRoute(chromium Api, engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/chromium/convert/html", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form, options := FormDataChromiumPdfOptions(ctx) mode := pdfengines.FormDataPdfSplitMode(form, false) @@ -621,7 +621,7 @@ func screenshotHtmlRoute(chromium Api) api.Route { Method: http.MethodPost, Path: "/forms/chromium/screenshot/html", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form, options := FormDataChromiumScreenshotOptions(ctx) @@ -652,7 +652,7 @@ func convertMarkdownRoute(chromium Api, engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/chromium/convert/markdown", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form, options := FormDataChromiumPdfOptions(ctx) mode := pdfengines.FormDataPdfSplitMode(form, false) @@ -720,7 +720,7 @@ func screenshotMarkdownRoute(chromium Api) api.Route { Method: http.MethodPost, Path: "/forms/chromium/screenshot/markdown", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form, options := FormDataChromiumScreenshotOptions(ctx) diff --git a/pkg/modules/libreoffice/routes.go b/pkg/modules/libreoffice/routes.go index 0ed5f9c7..51c0b895 100644 --- a/pkg/modules/libreoffice/routes.go +++ b/pkg/modules/libreoffice/routes.go @@ -7,7 +7,7 @@ import ( "slices" "strconv" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" "github.com/gotenberg/gotenberg/v8/pkg/modules/api" @@ -27,7 +27,7 @@ func convertRoute(libreOffice libreofficeapi.Uno, engine gotenberg.PdfEngine) ap Method: http.MethodPost, Path: "/forms/libreoffice/convert", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) defaultOptions := libreofficeapi.DefaultOptions() diff --git a/pkg/modules/libreoffice/routes_test.go b/pkg/modules/libreoffice/routes_test.go index f67b801b..6a6c2aa4 100644 --- a/pkg/modules/libreoffice/routes_test.go +++ b/pkg/modules/libreoffice/routes_test.go @@ -12,7 +12,7 @@ import ( "path/filepath" "testing" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" "github.com/gotenberg/gotenberg/v8/pkg/modules/api" diff --git a/pkg/modules/pdfengines/routes.go b/pkg/modules/pdfengines/routes.go index d019b14d..cb6a96eb 100644 --- a/pkg/modules/pdfengines/routes.go +++ b/pkg/modules/pdfengines/routes.go @@ -11,7 +11,7 @@ import ( "strings" "github.com/google/uuid" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" "github.com/gotenberg/gotenberg/v8/pkg/modules/api" @@ -1011,7 +1011,7 @@ func mergeRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/merge", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1212,7 +1212,7 @@ func splitRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/split", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1364,7 +1364,7 @@ func flattenRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/flatten", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1399,7 +1399,7 @@ func optimizeRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/optimize", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1435,7 +1435,7 @@ func convertRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/convert", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1492,7 +1492,7 @@ func readMetadataRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/metadata/read", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) var inputPaths []string @@ -1546,7 +1546,7 @@ func writeMetadataRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/metadata/write", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1581,7 +1581,7 @@ func readBookmarksRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/bookmarks/read", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) var inputPaths []string @@ -1634,7 +1634,7 @@ func writeBookmarksRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/bookmarks/write", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1669,7 +1669,7 @@ func encryptRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/encrypt", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1714,7 +1714,7 @@ func embedRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/embed", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1776,7 +1776,7 @@ func watermarkRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/watermark", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1833,7 +1833,7 @@ func stampRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/stamp", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1892,7 +1892,7 @@ func rotateRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/rotate", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() @@ -1929,7 +1929,7 @@ func facturXRoute(engine gotenberg.PdfEngine) api.Route { Method: http.MethodPost, Path: "/forms/pdfengines/factur-x", IsMultipart: true, - Handler: func(c echo.Context) error { + Handler: func(c *echo.Context) error { ctx := c.Get("context").(*api.Context) form := ctx.FormData() diff --git a/pkg/modules/prometheus/prometheus.go b/pkg/modules/prometheus/prometheus.go index 469cbbd3..b8049d21 100644 --- a/pkg/modules/prometheus/prometheus.go +++ b/pkg/modules/prometheus/prometheus.go @@ -7,7 +7,7 @@ import ( "net/http" "time" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" flag "github.com/spf13/pflag" diff --git a/pkg/modules/webhook/client.go b/pkg/modules/webhook/client.go index 6a32b639..e809643a 100644 --- a/pkg/modules/webhook/client.go +++ b/pkg/modules/webhook/client.go @@ -11,7 +11,7 @@ import ( "time" "github.com/hashicorp/go-retryablehttp" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/codes" "go.opentelemetry.io/otel/propagation" diff --git a/pkg/modules/webhook/middleware.go b/pkg/modules/webhook/middleware.go index 3943850b..71ef5154 100644 --- a/pkg/modules/webhook/middleware.go +++ b/pkg/modules/webhook/middleware.go @@ -14,7 +14,7 @@ import ( "time" "github.com/hashicorp/go-retryablehttp" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "github.com/gotenberg/gotenberg/v8/pkg/gotenberg" "github.com/gotenberg/gotenberg/v8/pkg/modules/api" @@ -95,7 +95,7 @@ func webhookMiddleware(w *Webhook) api.Middleware { }) } - return func(c echo.Context) error { + return func(c *echo.Context) error { webhookUrl := c.Request().Header.Get("Gotenberg-Webhook-Url") if webhookUrl == "" { // No webhook URL, call the next middleware in the chain. @@ -208,7 +208,7 @@ func webhookMiddleware(w *Webhook) api.Middleware { } } - // Retrieve values from echo.Context before it gets recycled. + // Retrieve values from [echo.Context] before it gets recycled. // See https://github.com/gotenberg/gotenberg/issues/1000. startTime := c.Get("startTime").(time.Time) correlationIdHeader := c.Get("correlationIdHeader").(string) @@ -323,16 +323,16 @@ func webhookMiddleware(w *Webhook) api.Middleware { // As a webhook URL has been given, we handle the request in a // goroutine and return immediately. // - // Echo returns the echo.Context back to its sync.Pool as + // Echo returns the [echo.Context] back to its sync.Pool as // soon as this synchronous handler returns ErrAsyncProcess. // A concurrent request can then claim the recycled context // and c.Reset() wipes the shared store, which would cause // any c.Get("...").(T) assertion downstream of the webhook // goroutine to panic on a nil value and crash the process. // Snapshot the keys downstream reads onto a detached - // wrapper before spawning the goroutine so pool reuse + // context before spawning the goroutine so pool reuse // cannot reach into our async work. - detached := newPoolSafeContext(c, "logger", "context", "correlationId", "correlationIdHeader", "startTime") + detached := newDetachedContext(c, "logger", "context", "correlationId", "correlationIdHeader", "startTime") w.asyncCount.Add(1) go func() { diff --git a/pkg/modules/webhook/pool_safe_context.go b/pkg/modules/webhook/pool_safe_context.go index 0848df38..2dd6f6a1 100644 --- a/pkg/modules/webhook/pool_safe_context.go +++ b/pkg/modules/webhook/pool_safe_context.go @@ -1,58 +1,34 @@ package webhook import ( - "sync" - - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" ) -// poolSafeContext wraps an [echo.Context] and keeps a private snapshot of -// the values that downstream middleware and route handlers read from the -// store. Echo returns an [echo.Context] to its sync.Pool as soon as the -// synchronous handler returns, including when the webhook middleware -// returns [api.ErrAsyncProcess]. A concurrent request can then claim the -// recycled context and c.Reset() wipes the shared store out from under -// the webhook goroutine, which causes any -// `c.Get("logger").(*slog.Logger)`-style assertion further down the -// chain to panic on a nil value. +// newDetachedContext returns an [echo.Context] carrying a snapshot of the given +// keys, detached from Echo's context pool. // -// Wrapping c before handing it to the goroutine insulates the async work -// from pool reuse: Get/Set read and write the private store while every -// other [echo.Context] method delegates to the embedded context for -// anything the downstream might still need. -type poolSafeContext struct { - echo.Context - mu sync.RWMutex - store map[string]any -} - -// newPoolSafeContext snapshots the given keys from c into a detached -// store and returns a wrapper whose Get/Set operate on that store -// exclusively. Keys absent from c are omitted; the wrapper still +// Echo returns an [echo.Context] to its sync.Pool as soon as the synchronous +// handler returns, including when the webhook middleware returns +// [api.ErrAsyncProcess]. A concurrent request can then claim the recycled +// context and c.Reset() wipes the shared store out from under the webhook +// goroutine, which causes any `c.Get("logger").(*slog.Logger)`-style assertion +// further down the chain to panic on a nil value. +// +// [echo.NewContext] allocates outside the pool, so recycling cannot reach the +// returned context. Keys absent from c are omitted; the returned context still // returns nil for them, matching [echo.Context.Get] behavior. -func newPoolSafeContext(c echo.Context, keys ...string) *poolSafeContext { - store := make(map[string]any, len(keys)) +// +// Only the asynchronous path uses this. Nothing downstream of the webhook +// middleware writes to the response: contextMiddleware sits upstream and has +// already answered 204 by the time the goroutine runs. +func newDetachedContext(c *echo.Context, keys ...string) *echo.Context { + detached := echo.NewContext(c.Request(), c.Response(), c.Echo()) + for _, key := range keys { if v := c.Get(key); v != nil { - store[key] = v + detached.Set(key, v) } } - return &poolSafeContext{Context: c, store: store} -} -// Get returns the value stored in the detached store, not the embedded -// context's pooled store. -func (p *poolSafeContext) Get(key string) any { - p.mu.RLock() - defer p.mu.RUnlock() - return p.store[key] -} - -// Set writes to the detached store, not the embedded context's pooled -// store. This prevents downstream middleware writes from leaking into a -// later request that claims the same pooled context. -func (p *poolSafeContext) Set(key string, val any) { - p.mu.Lock() - defer p.mu.Unlock() - p.store[key] = val + return detached } diff --git a/pkg/modules/webhook/pool_safe_context_test.go b/pkg/modules/webhook/pool_safe_context_test.go index 7e87a981..e9525d06 100644 --- a/pkg/modules/webhook/pool_safe_context_test.go +++ b/pkg/modules/webhook/pool_safe_context_test.go @@ -6,10 +6,10 @@ import ( "net/http/httptest" "testing" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" ) -func TestPoolSafeContext_SurvivesUnderlyingReset(t *testing.T) { +func TestNewDetachedContext_SurvivesUnderlyingReset(t *testing.T) { e := echo.New() req := httptest.NewRequest(http.MethodPost, "/", nil) rec := httptest.NewRecorder() @@ -19,10 +19,10 @@ func TestPoolSafeContext_SurvivesUnderlyingReset(t *testing.T) { c.Set("logger", logger) c.Set("correlationId", "abc-123") - detached := newPoolSafeContext(c, "logger", "correlationId", "missing") + detached := newDetachedContext(c, "logger", "correlationId", "missing") - // Simulate Echo recycling c for a concurrent request. Reset wipes the - // shared store, which is exactly the crash scenario the wrapper + // Simulate Echo recycling c for a concurrent request. Reset clears the + // pooled store, which is exactly the crash scenario the detached context // guards against. c.Reset(httptest.NewRequest(http.MethodGet, "/", nil), httptest.NewRecorder()) @@ -38,17 +38,17 @@ func TestPoolSafeContext_SurvivesUnderlyingReset(t *testing.T) { // Underlying c must remain clean. if c.Get("logger") != nil { - t.Fatalf("underlying c.Get(\"logger\") leaked wrapper state after reset") + t.Fatalf("underlying c.Get(\"logger\") leaked detached state after reset") } } -func TestPoolSafeContext_SetDoesNotTouchUnderlying(t *testing.T) { +func TestNewDetachedContext_SetDoesNotTouchUnderlying(t *testing.T) { e := echo.New() req := httptest.NewRequest(http.MethodPost, "/", nil) rec := httptest.NewRecorder() c := e.NewContext(req, rec) - detached := newPoolSafeContext(c) + detached := newDetachedContext(c) detached.Set("foo", "bar") if got, _ := detached.Get("foo").(string); got != "bar" { diff --git a/test/integration/scenario/server.go b/test/integration/scenario/server.go index a9c4a65f..0417e4a5 100644 --- a/test/integration/scenario/server.go +++ b/test/integration/scenario/server.go @@ -15,7 +15,7 @@ import ( "github.com/cucumber/godog" "github.com/google/uuid" - "github.com/labstack/echo/v4" + "github.com/labstack/echo/v5" "github.com/mholt/archives" ) @@ -26,12 +26,17 @@ type server struct { errChan chan error eventBody []byte eventMu sync.Mutex + + shutdown context.CancelFunc + done chan struct{} } func newServer(ctx context.Context, workdir string) (*server, error) { srv := echo.New() - srv.HideBanner = true - srv.HidePort = true + // The static file handlers below serve absolute paths, which Echo v5's + // default working-directory filesystem rejects. See newEchoServer in + // pkg/modules/api. + srv.Filesystem = echo.NewDefaultFS("/") s := &server{ srv: srv, errChan: make(chan error, 1), @@ -47,7 +52,7 @@ func newServer(ctx context.Context, workdir string) (*server, error) { return err } - webhookHandler := func(c echo.Context) error { + webhookHandler := func(c *echo.Context) error { s.req = c.Request() body, err := io.ReadAll(s.req.Body) @@ -131,7 +136,7 @@ func newServer(ctx context.Context, workdir string) (*server, error) { return webhookErr(c.String(http.StatusOK, http.StatusText(http.StatusOK))) } - webhookErrorHandler := func(c echo.Context) error { + webhookErrorHandler := func(c *echo.Context) error { s.req = c.Request() body, err := io.ReadAll(s.req.Body) if err != nil { @@ -148,7 +153,7 @@ func newServer(ctx context.Context, workdir string) (*server, error) { srv.PATCH("/webhook/error", webhookErrorHandler) srv.PUT("/webhook/error", webhookErrorHandler) - webhookEventsHandler := func(c echo.Context) error { + webhookEventsHandler := func(c *echo.Context) error { body, err := io.ReadAll(c.Request().Body) if err != nil { return c.String(http.StatusInternalServerError, err.Error()) @@ -159,7 +164,7 @@ func newServer(ctx context.Context, workdir string) (*server, error) { return c.String(http.StatusOK, http.StatusText(http.StatusOK)) } srv.POST("/webhook/events", webhookEventsHandler) - srv.GET("/static/:path", func(c echo.Context) error { + srv.GET("/static/:path", func(c *echo.Context) error { s.req = c.Request() path := c.Param("path") if strings.Contains(path, "teststore") { @@ -167,7 +172,7 @@ func newServer(ctx context.Context, workdir string) (*server, error) { } return c.Attachment(fmt.Sprintf("%s/%s", wd, path), filepath.Base(path)) }) - srv.GET("/html/:path", func(c echo.Context) error { + srv.GET("/html/:path", func(c *echo.Context) error { s.req = c.Request() path := fmt.Sprintf("%s/%s", wd, c.Param("path")) f, err := os.Open(path) @@ -181,7 +186,7 @@ func newServer(ctx context.Context, workdir string) (*server, error) { } return c.HTML(http.StatusOK, string(b)) }) - srv.GET("/redirect-to-private", func(c echo.Context) error { + srv.GET("/redirect-to-private", func(c *echo.Context) error { s.req = c.Request() // Redirect the browser to a non-public address so the outbound filter // is exercised on the redirected request rather than on this URL. @@ -206,9 +211,20 @@ func (s *server) start(ctx context.Context) (int, error) { port := ln.Addr().(*net.TCPAddr).Port + startConfig := echo.StartConfig{ + Listener: ln, + HideBanner: true, + HidePort: true, + } + + serveCtx, cancel := context.WithCancel(context.Background()) + s.shutdown = cancel + s.done = make(chan struct{}) + go func() { - s.srv.Listener = ln - err = s.srv.Start("") + defer close(s.done) + + err := startConfig.Start(serveCtx, s.srv) if err != nil && !errors.Is(err, http.ErrServerClosed) { godog.Log(ctx, err.Error()) } @@ -219,5 +235,17 @@ func (s *server) start(ctx context.Context) (int, error) { func (s *server) stop(ctx context.Context) error { close(s.errChan) - return s.srv.Shutdown(ctx) + + if s.shutdown == nil { + return nil + } + + s.shutdown() + + select { + case <-s.done: + return nil + case <-ctx.Done(): + return ctx.Err() + } }