feat(api): adopt enforcing rest.Timeout, drop local cooperative timeout
go-pkgz/rest v1.22.0 ships an enforcing Timeout middleware (net/http.TimeoutHandler style): it runs the handler with a deadline and returns 504 at the deadline even if the handler ignores the context - unlike the local cooperative timeout, which only cancelled the context and never actually stopped a stuck handler. Replace the local timeout with rest.Timeout on every route with a bounded response. The streaming and long-polling routes are deliberately left without it, since the enforcing timeout buffers the whole response in memory and aborts at the deadline: - GET /api/v1/userdata and GET /api/v1/admin/export stream gzipped exports - GET /api/v1/admin/wait long-polls for up to 15m - POST /api/v1/admin/import[/form] and /remap ingest large uploads Delete the local timeout middleware and its test; the enforcing behaviour is covered by go-pkgz/rest. TestRouteTimeout locks the enforcing-vs-exempt contract in this build.
This commit is contained in:
committed by
Umputun
parent
c48254a994
commit
b33025a76f
@@ -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
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user