diff --git a/backend/app/rest/api/middleware.go b/backend/app/rest/api/middleware.go index 8746c240..e075a6c0 100644 --- a/backend/app/rest/api/middleware.go +++ b/backend/app/rest/api/middleware.go @@ -2,7 +2,6 @@ package api import ( - "context" "fmt" "net/http" "net/mail" @@ -33,26 +32,6 @@ func corsMiddleware() func(http.Handler) http.Handler { ) } -// timeout returns a middleware matching chi's middleware.Timeout: it sets a -// deadline on the request context and writes 504 Gateway Timeout if the -// deadline is exceeded. The 504 is sent once the downstream handler returns -// after observing the canceled context; a handler that ignores r.Context() -// is not aborted. -func timeout(d time.Duration) 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.WithTimeout(r.Context(), d) - defer func() { - cancel() - if ctx.Err() == context.DeadlineExceeded { - w.WriteHeader(http.StatusGatewayTimeout) - } - }() - next.ServeHTTP(w, r.WithContext(ctx)) - }) - } -} - // rejectHead rejects HEAD requests with 405, advertising the given allowed methods in // the Allow header. net/http.ServeMux routes HEAD to a "GET ..." handler, but per RFC // 9110 GET/HEAD are safe methods; this guard is applied to the few GET routes whose diff --git a/backend/app/rest/api/middleware_test.go b/backend/app/rest/api/middleware_test.go index f0ae65bc..de0b06e0 100644 --- a/backend/app/rest/api/middleware_test.go +++ b/backend/app/rest/api/middleware_test.go @@ -9,35 +9,44 @@ import ( "time" "github.com/go-pkgz/auth/v2/token" + R "github.com/go-pkgz/rest" + "github.com/go-pkgz/routegroup" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/umputun/remark42/backend/app/rest" "github.com/umputun/remark42/backend/app/store" ) -func TestTimeout(t *testing.T) { - t.Run("fast handler passes through and gets a deadline", func(t *testing.T) { - var gotDeadline bool - h := timeout(time.Second)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - _, gotDeadline = r.Context().Deadline() - w.WriteHeader(http.StatusCreated) - _, _ = w.Write([]byte("ok")) - })) - rec := httptest.NewRecorder() - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/", http.NoBody)) - assert.True(t, gotDeadline, "request context should carry a deadline") - assert.Equal(t, http.StatusCreated, rec.Code) - assert.Equal(t, "ok", rec.Body.String()) - }) +// routes() wraps bounded routes with the enforcing rest.Timeout and deliberately leaves the +// streaming/long-polling routes (GET /export, /userdata, /wait) without it. This checks that +// contract holds against the vendored middleware: a slow handler under R.Timeout is aborted with +// 504 at the deadline, while a route left without it runs to completion. +func TestRouteTimeout(t *testing.T) { + slow := func(d time.Duration) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + select { + case <-r.Context().Done(): // return promptly once the enforcing timeout cancels the context + case <-time.After(d): + } + w.WriteHeader(http.StatusOK) + } + } - t.Run("deadline exceeded writes 504", func(t *testing.T) { - h := timeout(10 * time.Millisecond)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { - <-r.Context().Done() // honor the context: return only once the deadline fires - })) - rec := httptest.NewRecorder() - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/", http.NoBody)) - assert.Equal(t, http.StatusGatewayTimeout, rec.Code) - }) + router := routegroup.New(http.NewServeMux()) + router.With(R.Timeout(20*time.Millisecond)).HandleFunc("GET /bounded", slow(time.Second)) + router.HandleFunc("GET /streaming", slow(30*time.Millisecond)) // no timeout, like /export and /wait + ts := httptest.NewServer(router) + defer ts.Close() + + resp, err := http.Get(ts.URL + "/bounded") + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, http.StatusGatewayTimeout, resp.StatusCode, "route under R.Timeout is aborted at the deadline") + + resp, err = http.Get(ts.URL + "/streaming") + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + assert.Equal(t, http.StatusOK, resp.StatusCode, "route without R.Timeout runs to completion") } func TestRest_rejectAnonUser(t *testing.T) { diff --git a/backend/app/rest/api/rest.go b/backend/app/rest/api/rest.go index 2aacb5ac..9053c980 100644 --- a/backend/app/rest/api/rest.go +++ b/backend/app/rest/api/rest.go @@ -236,14 +236,14 @@ func (s *Rest) routes() http.Handler { authHandler, avatarHandler := s.Authenticator.Handlers() router.Route(func(r *routegroup.Bundle) { - r.Use(timeout(5 * time.Second)) + r.Use(R.Timeout(5 * time.Second)) r.Use(logInfoWithBody, rateLimiter(2), R.NoCache) r.Use(validEmailAuth()) // reject suspicious email logins r.Handle("/auth/", authHandler) }) router.Route(func(r *routegroup.Bundle) { - r.Use(timeout(5 * time.Second)) + r.Use(R.Timeout(5 * time.Second)) r.Use(rateLimiter(100)) r.Handle("/avatar/", avatarHandler) }) @@ -255,14 +255,14 @@ func (s *Rest) routes() http.Handler { rapi.Use(apiCSPMiddleware) rapi.Group().Route(func(rava *routegroup.Bundle) { - rava.Use(timeout(5 * time.Second)) + rava.Use(R.Timeout(5 * time.Second)) rava.Use(rateLimiter(100)) rava.Handle("/avatar/", avatarHandler) }) // open routes rapi.Group().Route(func(ropen *routegroup.Bundle) { - ropen.Use(timeout(30 * time.Second)) + ropen.Use(R.Timeout(30 * time.Second)) ropen.Use(rateLimiter(s.openRouteLimiter)) ropen.Use(authMiddleware.Trace, R.NoCache, logInfoWithBody) ropen.HandleFunc("GET /config", s.configCtrl) @@ -289,7 +289,7 @@ func (s *Rest) routes() http.Handler { // invalidation on revalidation); error responses get Cache-Control: no-store // so transient failures aren't pinned in the cache. rapi.Group().Route(func(ropen *routegroup.Bundle) { - ropen.Use(timeout(30 * time.Second)) + ropen.Use(R.Timeout(30 * time.Second)) ropen.Use(rateLimiter(10)) ropen.Use(authMiddleware.Trace, logInfoWithBody) ropen.HandleFunc("GET /img", s.ImageProxy.Handler) @@ -299,32 +299,45 @@ func (s *Rest) routes() http.Handler { // protected routes, require auth rapi.Group().Route(func(rauth *routegroup.Bundle) { - rauth.Use(timeout(30 * time.Second)) rauth.Use(rateLimiter(10)) rauth.Use(authMiddleware.Auth, matchSiteID, R.NoCache, logInfoWithBody) - rauth.HandleFunc("GET /user", s.privRest.userInfoCtrl) + + // GET /userdata streams a gzipped export of the user's data straight to the client, so it + // deliberately runs without R.Timeout: that middleware buffers the whole response in memory + // before sending and aborts at the deadline, which would hold a full export in RAM and truncate it. rauth.HandleFunc("GET /userdata", s.privRest.userAllDataCtrl) + + rauth.Group().Route(func(r *routegroup.Bundle) { + r.Use(R.Timeout(30 * time.Second)) + r.HandleFunc("GET /user", s.privRest.userInfoCtrl) + }) }) // admin routes, require auth and admin users only rapi.Mount("/admin").Route(func(radmin *routegroup.Bundle) { - radmin.Use(timeout(30 * time.Second)) radmin.Use(rateLimiter(10)) radmin.Use(authMiddleware.Auth, authMiddleware.AdminOnly, matchSiteID) radmin.Use(R.NoCache, logInfoWithBody) - radmin.HandleFunc("DELETE /comment/{id}", s.adminRest.deleteCommentCtrl) - radmin.HandleFunc("PUT /user/{userid}", s.adminRest.setBlockCtrl) - radmin.HandleFunc("DELETE /user/{userid}", s.adminRest.deleteUserCtrl) - radmin.HandleFunc("GET /user/{userid}", s.adminRest.getUserInfoCtrl) - radmin.With(rejectHead("GET")).HandleFunc("GET /deleteme", s.adminRest.deleteMeRequestCtrl) - radmin.HandleFunc("PUT /verify/{userid}", s.adminRest.setVerifyCtrl) - radmin.HandleFunc("PUT /pin/{id}", s.adminRest.setPinCtrl) - radmin.HandleFunc("GET /blocked", s.adminRest.blockedUsersCtrl) - radmin.HandleFunc("PUT /readonly", s.adminRest.setReadOnlyCtrl) - radmin.HandleFunc("PUT /title/{id}", s.adminRest.setTitleCtrl) + // bounded admin operations return small responses and get the enforcing request timeout + radmin.Group().Route(func(r *routegroup.Bundle) { + r.Use(R.Timeout(30 * time.Second)) + r.HandleFunc("DELETE /comment/{id}", s.adminRest.deleteCommentCtrl) + r.HandleFunc("PUT /user/{userid}", s.adminRest.setBlockCtrl) + r.HandleFunc("DELETE /user/{userid}", s.adminRest.deleteUserCtrl) + r.HandleFunc("GET /user/{userid}", s.adminRest.getUserInfoCtrl) + r.With(rejectHead("GET")).HandleFunc("GET /deleteme", s.adminRest.deleteMeRequestCtrl) + r.HandleFunc("PUT /verify/{userid}", s.adminRest.setVerifyCtrl) + r.HandleFunc("PUT /pin/{id}", s.adminRest.setPinCtrl) + r.HandleFunc("GET /blocked", s.adminRest.blockedUsersCtrl) + r.HandleFunc("PUT /readonly", s.adminRest.setReadOnlyCtrl) + r.HandleFunc("PUT /title/{id}", s.adminRest.setTitleCtrl) + }) - // migrator + // migrator routes deliberately run without R.Timeout: GET /export streams a full-site + // backup, GET /wait long-polls for up to 15m, and import/remap ingest large uploads. The + // enforcing timeout buffers the whole response and aborts at the deadline, which would + // truncate backups, break waiting, and reject large imports. radmin.HandleFunc("GET /export", s.adminRest.migrator.exportCtrl) radmin.HandleFunc("POST /import", s.adminRest.migrator.importCtrl) radmin.HandleFunc("POST /import/form", s.adminRest.migrator.importFormCtrl) @@ -334,7 +347,7 @@ func (s *Rest) routes() http.Handler { // protected routes, throttled to 10/s by default, controlled by external UpdateLimiter param rapi.Group().Route(func(rauth *routegroup.Bundle) { - rauth.Use(timeout(10 * time.Second)) + rauth.Use(R.Timeout(10 * time.Second)) rauth.Use(rateLimiter(s.updateLimiter())) rauth.Use(authMiddleware.Auth, matchSiteID, subscribersOnly(s.SubscribersOnly)) rauth.Use(R.NoCache, logInfoWithBody) @@ -354,7 +367,7 @@ func (s *Rest) routes() http.Handler { // protected routes, anonymous rejected rapi.Group().Route(func(rauth *routegroup.Bundle) { - rauth.Use(timeout(10 * time.Second)) + rauth.Use(R.Timeout(10 * time.Second)) rauth.Use(rateLimiter(s.updateLimiter())) rauth.Use(authMiddleware.Auth, rejectAnonUser, matchSiteID) rauth.Use(logger.New(logger.Log(log.Default()), logger.Prefix("[DEBUG]"), logger.IPfn(ipFn)).Handler) @@ -363,7 +376,7 @@ func (s *Rest) routes() http.Handler { // open routes on root level router.Route(func(rroot *routegroup.Bundle) { - rroot.Use(timeout(10 * time.Second)) + rroot.Use(R.Timeout(10 * time.Second)) rroot.Use(rateLimiter(50)) rroot.HandleFunc("GET /robots.txt", s.pubRest.robotsCtrl) rroot.With(rejectHead("GET, POST")).HandleFunc("GET /email/unsubscribe.html", s.privRest.emailUnsubscribeCtrl) @@ -501,7 +514,7 @@ func addFileServer(r *routegroup.Bundle, embedFS embed.FS, webRoot, version stri r.HandleFunc("GET /web", http.RedirectHandler("/web/", http.StatusMovedPermanently).ServeHTTP) r.With(rateLimiter(20), - timeout(10*time.Second), + R.Timeout(10*time.Second), cacheControl(time.Hour, version), ).HandleFunc("GET /web/", func(w http.ResponseWriter, r *http.Request) { // don't show dirs, just serve files diff --git a/backend/app/rest/api/ssl.go b/backend/app/rest/api/ssl.go index 4e791c6e..9ab08f78 100644 --- a/backend/app/rest/api/ssl.go +++ b/backend/app/rest/api/ssl.go @@ -44,7 +44,7 @@ func (s *Rest) httpToHTTPSRouter() http.Handler { log.Printf("[DEBUG] create http-to-https redirect routes") router := routegroup.New(http.NewServeMux()) router.Use(R.Recoverer(log.Default())) - router.Use(R.Throttle(1000), timeout(60*time.Second)) + router.Use(R.Throttle(1000), R.Timeout(60*time.Second)) router.Handle("/", s.redirectHandler()) return router @@ -58,7 +58,7 @@ func (s *Rest) httpChallengeRouter(m *autocert.Manager) http.Handler { log.Printf("[DEBUG] create http-challenge routes") router := routegroup.New(http.NewServeMux()) router.Use(R.Recoverer(log.Default())) - router.Use(R.Throttle(1000), timeout(60*time.Second)) + router.Use(R.Throttle(1000), R.Timeout(60*time.Second)) router.Handle("/", m.HTTPHandler(s.redirectHandler())) return router