1 Commits

Author SHA1 Message Date
clawbot
0384de4a7b Terminally fail retrying deliveries with a non-retry target type (closes #82)
All checks were successful
check / check (push) Successful in 4m4s
Restart recovery and the 60s retry sweep both looked an orphaned
`retrying` delivery's target up in the registry and silently returned
when it did not implement `rescheduler`. If a target's type was edited
from a retry type (`http`/`slack`) to a fire-and-forget type
(`database`/`log`) or an unknown one while a delivery was still
retrying, that delivery stayed `retrying` forever.

Both sites now hand the delivery to one shared helper,
`failUnretryableRetry`, which records a `DeliveryResult` naming the
current target type as the reason and marks the delivery `failed`. It
logs at warn, not error: this is operator-caused state, not a system
fault.

Re-dispatching under the new type was rejected as it would perform a
delivery the operator never asked for; the event itself stays in the
per-webhook event database, so manual redelivery can recover it
deliberately.

Fire-and-forget targets never set status `retrying` under normal
operation, so this path stays unreachable for them in practice.
2026-08-09 05:51:51 +00:00
13 changed files with 323 additions and 664 deletions

View File

@@ -636,6 +636,18 @@ This means:
durable fallback that ensures no retry is permanently lost, even under
extreme backpressure.
**Changing a target's type does not migrate in-flight deliveries.** Only
`http` and `slack` targets own durable retries; `database` and `log`
targets are fire-and-forget and never produce a `retrying` delivery. If a
target's `type` is edited from a retrying type to a non-retrying (or
unknown) one while one of its deliveries is still `retrying`, both
recovery paths above terminally mark that delivery `failed` and record a
`DeliveryResult` naming the current target type as the reason, logging it
at warn level. The delivery is not re-dispatched under the new type — the
operator never asked for that delivery — and the event itself remains
stored in the per-webhook event database, so it can be redelivered
manually.
### Circuit Breaker (HTTP Targets with Retries)
HTTP targets with `max_retries` > 0 are protected by a **per-target circuit breaker** that
@@ -867,17 +879,9 @@ Applied to all routes in this order:
8. **Sentry** — Error reporting to Sentry (if `SENTRY_DSN` is set;
configured with `Repanic: true` so panics still reach Recoverer)
Additionally, form endpoints (`/pages`, `/user/*`, `/sources`,
`/source/*`) apply a **MaxBodySize** middleware that limits
POST/PUT/PATCH request bodies to 1 MB. It is registered ahead of the
CSRF middleware in every one of those route groups, because
gorilla/csrf parses the form; if the cap were installed after it, form
parsing would run under net/http's 10 MB default and the 1 MB limit
would never apply. A request that declares a `Content-Length` over the
limit is answered with `413 Request Entity Too Large` before any other
middleware or handler runs; a chunked request, or one that lies about
its length, is hard-capped by `http.MaxBytesReader` and fails
downstream at form-parse time.
Additionally, form endpoints (`/pages`, `/sources`, `/source/*`) apply a
**MaxBodySize** middleware that limits POST/PUT/PATCH request bodies to
1 MB using `http.MaxBytesReader`, preventing oversized form submissions.
### Authentication
@@ -899,8 +903,7 @@ downstream at form-parse time.
- Production security headers on all responses: HSTS, X-Content-Type-Options
(`nosniff`), X-Frame-Options (`DENY`), Content-Security-Policy, Referrer-Policy,
and Permissions-Policy
- Request body size limits (1 MB) on all form POST endpoints, enforced
by middleware that runs before CSRF parses the form
- Request body size limits (1 MB) on all form POST endpoints
- **CSRF protection** via [gorilla/csrf](https://github.com/gorilla/csrf)
on all state-changing forms (cookie-based double-submit tokens with
HMAC authentication). Applied to `/pages`, `/sources`, `/source`, and

11
TODO.md
View File

@@ -28,13 +28,10 @@ databases currently grow without bound.
# Completed Steps
- 2026-08-09 Enforce the request body size limit before the CSRF
middleware parses the form (#90): `MaxBodySize` is now registered
ahead of `CSRF()` in every form route group, the `/user/{username}`
group gained the cap it never had (which is where `POST /password`
lives), the middleware rejects a declared-oversize body with a real
413 up front, and the redundant handler-local
`http.MaxBytesReader` calls were removed
- 2026-08-09 Restart recovery and the 60s retry sweep terminally fail an
orphaned `retrying` delivery whose target type no longer supports
retries, recording a `DeliveryResult` with the reason instead of
leaving the delivery stuck forever (#82)
- 2026-08-07 Update golangci-lint to v2.12.2 (Docker image digest in
`Dockerfile`, release-archive sha256 pins in `script/bootstrap`),
adopt the canonical `.golangci.yml` (v2 `linters.settings` layout so

View File

@@ -453,8 +453,9 @@ func (e *Engine) recoverRetryingDeliveries(
// recoverSingleRetry hands an orphaned retrying delivery back
// to its target to recompute the remaining backoff, then
// reschedules it. Targets that do not own durable retries
// (fire-and-forget) never produce retrying deliveries, so
// they are skipped.
// (fire-and-forget) never produce retrying deliveries, so a
// delivery found in that state has had its target's type
// changed underneath it and is terminally failed.
func (e *Engine) recoverSingleRetry(
webhookDB *gorm.DB,
webhookID string,
@@ -475,6 +476,10 @@ func (e *Engine) recoverSingleRetry(
rs, ok := e.targets[target.Type].(rescheduler)
if !ok {
e.failUnretryableRetry(
webhookDB, webhookID, d, &target,
)
return
}
@@ -649,8 +654,8 @@ func (e *Engine) sweepWebhookRetries(
// sweepSingleRetry re-enqueues an orphaned retrying delivery
// whose backoff window has elapsed, delegating the backoff
// decision to the delivery's target. Targets that do not own
// durable retries are skipped.
// decision to the delivery's target. A delivery whose target
// no longer owns durable retries is terminally failed.
func (e *Engine) sweepSingleRetry(
webhookDB *gorm.DB,
webhookID string,
@@ -670,6 +675,10 @@ func (e *Engine) sweepSingleRetry(
rs, ok := e.targets[target.Type].(rescheduler)
if !ok {
e.failUnretryableRetry(
webhookDB, webhookID, d, &target,
)
return
}
@@ -710,6 +719,59 @@ func (e *Engine) sweepSingleRetry(
}
}
// failUnretryableRetry terminally fails an orphaned retrying
// delivery whose target type no longer supports retries. Both
// restart recovery and the periodic sweep call it, so the
// terminal transition exists once.
//
// This is only reachable when a target's type has been changed
// out from under an in-flight retrying delivery (or the type is
// unknown to the registry): fire-and-forget targets never set
// status retrying themselves. Re-dispatching under the new type
// would be a delivery the operator never asked for, and leaving
// the row retrying strands it forever, so the delivery is
// failed with a recorded reason and can be redelivered
// manually. Logged at warn, not error: this is operator-caused
// state, not a system fault.
func (e *Engine) failUnretryableRetry(
webhookDB *gorm.DB,
webhookID string,
d *database.Delivery,
target *database.Target,
) {
e.log.Warn(
"failing orphaned retrying delivery: target "+
"type no longer supports retries",
"webhook_id", webhookID,
"delivery_id", d.ID,
"target_id", target.ID,
"target_name", target.Name,
"target_type", target.Type,
)
reason := fmt.Sprintf(
"target type %q does not support retries; "+
"delivery was left retrying by a previous "+
"target type and has been failed terminally",
target.Type,
)
e.recordResult(
webhookDB,
d,
e.countAttempts(webhookDB, d.ID)+1,
false,
0,
"",
reason,
0,
)
e.updateDeliveryStatus(
webhookDB, d, database.DeliveryStatusFailed,
)
}
// processDelivery dispatches a delivery to the target that
// owns its type. Unknown target types fail the delivery.
func (e *Engine) processDelivery(

View File

@@ -748,6 +748,193 @@ func TestRecoverWebhookDeliveries_RetryingDeliveries(
case <-time.After(5 * time.Second):
t.Fatal("expected retry task from recovery")
}
// Regression guard: a target that still supports retries
// must be rescheduled, never terminally failed, and must
// not gain a synthetic result row.
iAssertStatus(
t, s.WebhookDB, d.ID,
database.DeliveryStatusRetrying,
)
assert.Len(t, iResults(t, s.WebhookDB, d.ID), 1)
}
// --- Retrying deliveries whose target type changed ---
// iSeedRetryingWithType seeds a retrying delivery with one
// recorded failed attempt against a target of the given type,
// standing in for a target whose type was edited in the main
// database while the delivery was still retrying.
func iSeedRetryingWithType(
t *testing.T,
s iSetup,
targetType database.TargetType,
) string {
t.Helper()
targetID := uuid.New().String()
iCreateTarget(t, s.MainDB, targetID,
s.WebhookID, "mutated-target", targetType,
iHTTPConfig("http://example.com/hook"), 5,
)
event := iSeedEvent(
t, s.WebhookDB, s.WebhookID,
`{"orphaned":"retry"}`,
)
d := iSeedDelivery(
t, s.WebhookDB, event.ID, targetID,
database.DeliveryStatusRetrying,
)
iSeedFailedResult(t, s.WebhookDB, d.ID)
return d.ID
}
// iResults loads a delivery's results in attempt order.
func iResults(
t *testing.T, db *gorm.DB, deliveryID string,
) []database.DeliveryResult {
t.Helper()
var results []database.DeliveryResult
require.NoError(t, db.
Where("delivery_id = ?", deliveryID).
Order("attempt_num").
Find(&results).Error)
return results
}
// iAssertTerminallyFailed asserts the delivery ended failed
// with a result row recording why, and was not rescheduled.
func iAssertTerminallyFailed(
t *testing.T,
s iSetup,
deliveryID string,
targetType database.TargetType,
) {
t.Helper()
iAssertStatus(
t, s.WebhookDB, deliveryID,
database.DeliveryStatusFailed,
)
results := iResults(t, s.WebhookDB, deliveryID)
require.Len(t, results, 2)
last := results[1]
assert.False(t, last.Success)
assert.Equal(t, 2, last.AttemptNum)
assert.Contains(
t, last.Error, string(targetType),
)
assert.Contains(
t, last.Error, "does not support retries",
)
assert.Empty(t, s.Engine.ExportRetryCh())
}
func TestRecoverSingleRetry_TypeNoLongerRetries(
t *testing.T,
) {
t.Parallel()
s := newISetup(t)
iCreateWebhook(
t, s.MainDB, s.WebhookID, "mutated-type",
)
deliveryID := iSeedRetryingWithType(
t, s, database.TargetTypeLog,
)
s.Engine.ExportRecoverWebhookDeliveries(
context.Background(), s.WebhookID,
)
iAssertTerminallyFailed(
t, s, deliveryID, database.TargetTypeLog,
)
}
func TestSweepSingleRetry_TypeNoLongerRetries(
t *testing.T,
) {
t.Parallel()
s := newISetup(t)
iCreateWebhook(
t, s.MainDB, s.WebhookID, "mutated-type-sweep",
)
deliveryID := iSeedRetryingWithType(
t, s, database.TargetTypeDatabase,
)
s.Engine.ExportSweepWebhookRetries(
context.Background(), s.WebhookID,
)
iAssertTerminallyFailed(
t, s, deliveryID, database.TargetTypeDatabase,
)
}
func TestRecoverSingleRetry_UnknownTargetType(
t *testing.T,
) {
t.Parallel()
s := newISetup(t)
iCreateWebhook(
t, s.MainDB, s.WebhookID, "unknown-type",
)
unknown := database.TargetType("not-a-target-type")
deliveryID := iSeedRetryingWithType(t, s, unknown)
s.Engine.ExportRecoverWebhookDeliveries(
context.Background(), s.WebhookID,
)
iAssertTerminallyFailed(t, s, deliveryID, unknown)
}
func TestSweepSingleRetry_UnknownTargetType(
t *testing.T,
) {
t.Parallel()
s := newISetup(t)
iCreateWebhook(
t, s.MainDB, s.WebhookID, "unknown-type-sweep",
)
unknown := database.TargetType("not-a-target-type")
deliveryID := iSeedRetryingWithType(t, s, unknown)
s.Engine.ExportSweepWebhookRetries(
context.Background(), s.WebhookID,
)
iAssertTerminallyFailed(t, s, deliveryID, unknown)
}
// iSeedFailedResult creates a failed delivery result.

View File

@@ -188,6 +188,13 @@ func (e *Engine) ExportRecoverInFlight(
e.recoverInFlight(ctx)
}
// ExportSweepWebhookRetries exposes sweepWebhookRetries.
func (e *Engine) ExportSweepWebhookRetries(
ctx context.Context, webhookID string,
) {
e.sweepWebhookRetries(ctx, webhookID)
}
// ExportStart exposes start for testing.
func (e *Engine) ExportStart(ctx context.Context) {
e.start(ctx)

View File

@@ -29,8 +29,10 @@ func (h *Handlers) HandleLoginPage() http.HandlerFunc {
// HandleLoginSubmit handles the login form submission (POST)
func (h *Handlers) HandleLoginSubmit() http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
// Limit request body to prevent memory exhaustion
r.Body = http.MaxBytesReader(w, r.Body, 1<<maxBodyShift)
// Parse form data
err := r.ParseForm()
if err != nil {
h.log.Error("failed to parse form", "error", err)

View File

@@ -31,8 +31,9 @@ func (h *Handlers) HandlePasswordChange() http.HandlerFunc {
return
}
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
// Limit request body to prevent memory exhaustion.
r.Body = http.MaxBytesReader(w, r.Body, 1<<maxBodyShift)
err := r.ParseForm()
if err != nil {
h.log.Error("failed to parse form", "error", err)

View File

@@ -127,8 +127,10 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
return
}
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
err := r.ParseForm()
if err != nil {
http.Error(
@@ -384,8 +386,10 @@ func (h *Handlers) HandleSourceEditSubmit() http.HandlerFunc {
return
}
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
err = r.ParseForm()
if err != nil {
http.Error(
@@ -405,8 +409,10 @@ func (h *Handlers) applyWebhookEdit(
r *http.Request,
webhook *database.Webhook,
) {
// The body size cap is enforced by the MaxBodySize middleware,
// which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
name := r.FormValue("name")
if name == "" {
data := map[string]any{
@@ -719,8 +725,10 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc {
return
}
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
err = r.ParseForm()
if err != nil {
http.Error(
@@ -777,8 +785,10 @@ func (h *Handlers) HandleTargetCreate() http.HandlerFunc {
return
}
// The body size cap is enforced by the MaxBodySize
// middleware, which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
err = r.ParseForm()
if err != nil {
http.Error(
@@ -798,8 +808,10 @@ func (h *Handlers) processTargetCreate(
r *http.Request,
webhook database.Webhook,
) {
// The body size cap is enforced by the MaxBodySize middleware,
// which runs before CSRF parses the form.
r.Body = http.MaxBytesReader(
w, r.Body, 1<<maxBodyShift,
)
name := r.FormValue("name")
targetType := database.TargetType(r.FormValue("type"))
targetURL := r.FormValue("url")

View File

@@ -285,36 +285,10 @@ func (s *Middleware) NoCache() func(http.Handler) http.Handler {
}
}
// bodyLimitedMethod reports whether the request method carries a
// body that the MaxBodySize middleware should cap.
func bodyLimitedMethod(method string) bool {
return method == http.MethodPost ||
method == http.MethodPut ||
method == http.MethodPatch
}
// MaxBodySize returns middleware that limits the size of
// POST/PUT/PATCH request bodies to maxBytes. It must be registered
// before any middleware that parses the body — notably CSRF, which
// calls r.PostFormValue — so that form parsing happens under this
// cap rather than net/http's 10 MB default.
//
// Two enforcement paths exist, because http.MaxBytesReader alone
// cannot produce a 413: it reports the overflow as an error from
// Read, by which point the body parser downstream has already
// converted that error into its own response.
//
// - Declared oversize: the request announces a Content-Length
// greater than maxBytes. The middleware answers 413 Request
// Entity Too Large immediately and does not call the next
// handler, so neither CSRF nor the endpoint handler runs.
// - Undeclared oversize: the request is chunked (Content-Length
// of -1) or lies about its Content-Length. There is nothing to
// check up front, so http.MaxBytesReader hard-caps the body at
// maxBytes and the request fails downstream — the form parse
// errors out and CSRF rejects it with 403. The response is less
// precise than a 413, but the body is still never buffered
// beyond the cap, which is the property that matters.
// MaxBodySize returns middleware that limits the request body size
// for POST requests. If the body exceeds the given limit in
// bytes, the server returns 413 Request Entity Too Large. This
// prevents clients from sending arbitrarily large form bodies.
func (s *Middleware) MaxBodySize(
maxBytes int64,
) func(http.Handler) http.Handler {
@@ -323,31 +297,14 @@ func (s *Middleware) MaxBodySize(
w http.ResponseWriter,
r *http.Request,
) {
if !bodyLimitedMethod(r.Method) {
next.ServeHTTP(w, r)
return
}
if r.ContentLength > maxBytes {
s.log.Warn(
"request body exceeds limit",
"method", r.Method,
"path", r.URL.Path,
"content_length", r.ContentLength,
"limit", maxBytes,
if r.Method == http.MethodPost ||
r.Method == http.MethodPut ||
r.Method == http.MethodPatch {
r.Body = http.MaxBytesReader(
w, r.Body, maxBytes,
)
http.Error(
w,
"Request Entity Too Large",
http.StatusRequestEntityTooLarge,
)
return
}
r.Body = http.MaxBytesReader(w, r.Body, maxBytes)
next.ServeHTTP(w, r)
})
}

View File

@@ -3,12 +3,10 @@ package middleware_test
import (
"context"
"encoding/base64"
"io"
"log/slog"
"net/http"
"net/http/httptest"
"os"
"strings"
"testing"
"github.com/gorilla/sessions"
@@ -428,153 +426,6 @@ func TestNoCache_SetsHeaders(t *testing.T) {
)
}
// --- MaxBodySize Middleware Tests ---
const testBodyLimit int64 = 64
// maxBodySizeHandler wraps a sentinel handler in MaxBodySize with
// testBodyLimit. The sentinel records whether it ran and how much of
// the body it managed to read, so tests can distinguish "never
// reached" from "reached but truncated".
type maxBodySizeResult struct {
called bool
read int
readErr error
response *httptest.ResponseRecorder
}
func runMaxBodySize(
t *testing.T,
req *http.Request,
) *maxBodySizeResult {
t.Helper()
m, _ := testMiddleware(t, config.EnvironmentDev)
res := &maxBodySizeResult{response: httptest.NewRecorder()}
handler := m.MaxBodySize(testBodyLimit)(http.HandlerFunc(
func(w http.ResponseWriter, r *http.Request) {
res.called = true
body, err := io.ReadAll(r.Body)
res.read = len(body)
res.readErr = err
w.WriteHeader(http.StatusOK)
},
))
handler.ServeHTTP(res.response, req)
return res
}
// postWithBody builds a POST request whose Content-Length is
// accurate for the given payload size.
func postWithBody(size int) *http.Request {
req := httptest.NewRequestWithContext(
context.Background(),
http.MethodPost, "/pages/login",
strings.NewReader(strings.Repeat("a", size)),
)
req.Header.Set(
"Content-Type", "application/x-www-form-urlencoded",
)
return req
}
func TestMaxBodySize_DeclaredOversize_413AndHandlerNotReached(
t *testing.T,
) {
t.Parallel()
res := runMaxBodySize(t, postWithBody(int(testBodyLimit)+1))
assert.False(
t, res.called,
"handler must not be reached for an oversized body",
)
assert.Equal(
t, http.StatusRequestEntityTooLarge, res.response.Code,
)
}
func TestMaxBodySize_AtLimit_PassesThrough(t *testing.T) {
t.Parallel()
res := runMaxBodySize(t, postWithBody(int(testBodyLimit)))
assert.True(
t, res.called,
"handler should be reached for a body at the limit",
)
require.NoError(t, res.readErr)
assert.Equal(t, int(testBodyLimit), res.read)
assert.Equal(t, http.StatusOK, res.response.Code)
}
func TestMaxBodySize_UnderLimit_PassesThrough(t *testing.T) {
t.Parallel()
res := runMaxBodySize(t, postWithBody(1))
assert.True(t, res.called)
require.NoError(t, res.readErr)
assert.Equal(t, 1, res.read)
assert.Equal(t, http.StatusOK, res.response.Code)
}
func TestMaxBodySize_GetWithOversizeBody_NotCapped(t *testing.T) {
t.Parallel()
req := httptest.NewRequestWithContext(
context.Background(),
http.MethodGet, "/pages/login",
strings.NewReader(
strings.Repeat("a", int(testBodyLimit)+1),
),
)
res := runMaxBodySize(t, req)
assert.True(
t, res.called,
"GET requests are not subject to the POST body cap",
)
require.NoError(t, res.readErr)
assert.Equal(t, int(testBodyLimit)+1, res.read)
}
// TestMaxBodySize_UndeclaredOversize_TruncatedAtCap covers the
// chunked / lying-Content-Length case: there is nothing to check up
// front, so the request reaches the handler but MaxBytesReader
// hard-caps the body and the read fails at the limit.
func TestMaxBodySize_UndeclaredOversize_TruncatedAtCap(
t *testing.T,
) {
t.Parallel()
req := postWithBody(int(testBodyLimit) + 1)
// Simulate a chunked request: no declared length.
req.ContentLength = -1
res := runMaxBodySize(t, req)
assert.True(
t, res.called,
"an undeclared oversize body cannot be rejected up front",
)
require.Error(
t, res.readErr,
"reading past the cap must fail",
)
assert.Equal(
t, int(testBodyLimit), res.read,
"the handler must not see more than the cap",
)
}
// --- Helper Tests ---
func TestIpFromHostPort(t *testing.T) {

View File

@@ -1,36 +0,0 @@
package server
import (
"log/slog"
"net/http"
"sneak.berlin/go/webhooker/internal/config"
"sneak.berlin/go/webhooker/internal/handlers"
"sneak.berlin/go/webhooker/internal/middleware"
)
// MaxFormBodySizeForTest exposes the form body cap so tests can
// build requests that sit exactly at, below, and above it.
const MaxFormBodySizeForTest = maxFormBodySize
// NewRouterForTest builds the real route tree via SetupRoutes with
// the supplied middleware and handlers, bypassing the fx lifecycle
// and the HTTP listener. Tests use it so that route-group middleware
// registration order is exercised exactly as it ships, rather than
// against a hand-rebuilt chain that could drift from routes.go.
func NewRouterForTest(
log *slog.Logger,
cfg *config.Config,
mw *middleware.Middleware,
h *handlers.Handlers,
) http.Handler {
s := &Server{
log: log,
mw: mw,
h: h,
params: ServerParams{Config: cfg},
}
s.SetupRoutes()
return s.router
}

View File

@@ -90,11 +90,9 @@ func (s *Server) setupRoutes() {
func (s *Server) setupPageRoutes() {
s.router.Route("/pages", func(r chi.Router) {
// MaxBodySize must precede CSRF: gorilla/csrf parses the
// form, so the cap has to be installed before it runs.
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Use(s.mw.CSRF())
r.Use(s.mw.NoCache())
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Group(func(r chi.Router) {
r.Use(s.mw.LoginRateLimit())
@@ -108,9 +106,6 @@ func (s *Server) setupPageRoutes() {
func (s *Server) setupUserRoutes() {
s.router.Route("/user/{username}", func(r chi.Router) {
// MaxBodySize must precede CSRF: gorilla/csrf parses the
// form, so the cap has to be installed before it runs.
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Use(s.mw.CSRF())
r.Use(s.mw.NoCache())
r.Use(s.mw.RequireAuth())
@@ -123,24 +118,20 @@ func (s *Server) setupUserRoutes() {
func (s *Server) setupSourceRoutes() {
s.router.Route("/sources", func(r chi.Router) {
// MaxBodySize must precede CSRF: gorilla/csrf parses the
// form, so the cap has to be installed before it runs.
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Use(s.mw.CSRF())
r.Use(s.mw.NoCache())
r.Use(s.mw.RequireAuth())
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Get("/", s.h.HandleSourceList())
r.Get("/new", s.h.HandleSourceCreate())
r.Post("/new", s.h.HandleSourceCreateSubmit())
})
s.router.Route("/source/{sourceID}", func(r chi.Router) {
// MaxBodySize must precede CSRF: gorilla/csrf parses the
// form, so the cap has to be installed before it runs.
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Use(s.mw.CSRF())
r.Use(s.mw.NoCache())
r.Use(s.mw.RequireAuth())
r.Use(s.mw.MaxBodySize(maxFormBodySize))
r.Get("/", s.h.HandleSourceDetail())
r.Get("/edit", s.h.HandleSourceEdit())
r.Post("/edit", s.h.HandleSourceEditSubmit())

View File

@@ -1,375 +0,0 @@
package server_test
import (
"context"
"html"
"net/http"
"net/http/httptest"
"net/url"
"regexp"
"strings"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/fx"
"go.uber.org/fx/fxtest"
"sneak.berlin/go/webhooker/internal/config"
"sneak.berlin/go/webhooker/internal/database"
"sneak.berlin/go/webhooker/internal/delivery"
"sneak.berlin/go/webhooker/internal/globals"
"sneak.berlin/go/webhooker/internal/handlers"
"sneak.berlin/go/webhooker/internal/healthcheck"
"sneak.berlin/go/webhooker/internal/logger"
"sneak.berlin/go/webhooker/internal/middleware"
"sneak.berlin/go/webhooker/internal/server"
"sneak.berlin/go/webhooker/internal/session"
)
// csrfCookieName is the cookie gorilla/csrf issues when it runs. Its
// presence or absence on a response is how these tests tell whether
// the CSRF middleware executed.
const csrfCookieName = "_gorilla_csrf"
type noopNotifier struct{}
func (n *noopNotifier) Notify([]delivery.Task) {}
// testEnv is the real router from routes.go plus the collaborators
// tests need to seed users and forge sessions.
type testEnv struct {
router http.Handler
sess *session.Session
db *database.Database
}
// newTestEnv wires the dependency graph with fx and builds the
// production route tree, so middleware registration order is
// exercised exactly as it ships.
func newTestEnv(t *testing.T) *testEnv {
t.Helper()
var (
log *logger.Logger
cfg *config.Config
mw *middleware.Middleware
hnd *handlers.Handlers
sess *session.Session
db *database.Database
)
app := fxtest.New(
t,
fx.Provide(
globals.New,
logger.New,
func() *config.Config {
return &config.Config{
DataDir: t.TempDir(),
Environment: config.EnvironmentDev,
}
},
database.New,
database.NewWebhookDBManager,
healthcheck.New,
session.New,
func() delivery.Notifier { return &noopNotifier{} },
middleware.New,
handlers.New,
),
fx.Populate(&log, &cfg, &mw, &hnd, &sess, &db),
)
app.RequireStart()
t.Cleanup(app.RequireStop)
return &testEnv{
router: server.NewRouterForTest(log.Get(), cfg, mw, hnd),
sess: sess,
db: db,
}
}
// oversizeValue returns a form value one byte past the route-group
// body cap, so an encoded form containing it is guaranteed oversize.
func oversizeValue() string {
return strings.Repeat("a", int(server.MaxFormBodySizeForTest)+1)
}
// csrfCookieSet reports whether the response issued a gorilla/csrf
// cookie, which only happens if the CSRF middleware ran.
func csrfCookieSet(w *httptest.ResponseRecorder) bool {
for _, c := range w.Result().Cookies() {
if c.Name == csrfCookieName {
return true
}
}
return false
}
// get issues a GET through the router with the supplied cookies.
func (e *testEnv) get(
path string,
cookies []*http.Cookie,
) *httptest.ResponseRecorder {
req := httptest.NewRequestWithContext(
context.Background(), http.MethodGet, path, nil,
)
for _, c := range cookies {
req.AddCookie(c)
}
w := httptest.NewRecorder()
e.router.ServeHTTP(w, req)
return w
}
// post issues a urlencoded form POST through the router. The body is
// a strings.Reader, so the request carries an accurate
// Content-Length — the signal MaxBodySize checks up front.
func (e *testEnv) post(
path string,
form url.Values,
cookies []*http.Cookie,
) *httptest.ResponseRecorder {
req := httptest.NewRequestWithContext(
context.Background(), http.MethodPost, path,
strings.NewReader(form.Encode()),
)
req.Header.Set(
"Content-Type", "application/x-www-form-urlencoded",
)
for _, c := range cookies {
req.AddCookie(c)
}
w := httptest.NewRecorder()
e.router.ServeHTTP(w, req)
return w
}
// csrfFrom renders the page at path and returns the CSRF token from
// its form together with every cookie needed for the follow-up POST.
func (e *testEnv) csrfFrom(
t *testing.T,
path string,
cookies []*http.Cookie,
) (string, []*http.Cookie) {
t.Helper()
w := e.get(path, cookies)
require.Equal(t, http.StatusOK, w.Code)
pattern := regexp.MustCompile(
`name="csrf_token" value="([^"]+)"`,
)
match := pattern.FindStringSubmatch(w.Body.String())
require.Len(t, match, 2, "form must embed a CSRF token")
// html/template escapes "+" and "=" in attribute values, and
// gorilla/csrf tokens are standard base64, so the value read
// out of the markup has to be unescaped before it is submitted.
token := html.UnescapeString(match[1])
combined := make([]*http.Cookie, 0, len(cookies))
combined = append(combined, cookies...)
combined = append(combined, w.Result().Cookies()...)
return token, combined
}
// authCookies forges an authenticated session for the given user.
func (e *testEnv) authCookies(
t *testing.T,
userID, username string,
) []*http.Cookie {
t.Helper()
req := httptest.NewRequestWithContext(
context.Background(), http.MethodGet, "/setup", nil,
)
w := httptest.NewRecorder()
s, err := e.sess.Get(req)
require.NoError(t, err)
e.sess.SetUser(s, userID, username)
require.NoError(t, e.sess.Save(req, w, s))
cookies := w.Result().Cookies()
require.NotEmpty(t, cookies, "session cookie should be set")
return cookies
}
// seedUser creates a user with the given password and returns the
// stored hash so tests can assert whether it later changed.
func (e *testEnv) seedUser(
t *testing.T,
username, password string,
) (string, string) {
t.Helper()
hash, err := database.HashPassword(password)
require.NoError(t, err)
user := &database.User{Username: username, Password: hash}
require.NoError(t, e.db.DB().Create(user).Error)
return user.ID, hash
}
// storedHash reads the current password hash for a username.
func (e *testEnv) storedHash(t *testing.T, username string) string {
t.Helper()
var user database.User
require.NoError(t,
e.db.DB().Where("username = ?", username).
First(&user).Error,
)
return user.Password
}
// --- /pages group ---
// TestPagesLogin_OversizeBody_RejectedBeforeCSRF proves the cap runs
// ahead of gorilla/csrf: the response is a clean 413 and no CSRF
// cookie was issued, so neither the CSRF middleware nor the login
// handler ran.
func TestPagesLogin_OversizeBody_RejectedBeforeCSRF(t *testing.T) {
t.Parallel()
env := newTestEnv(t)
form := url.Values{}
form.Set("username", oversizeValue())
form.Set("password", "irrelevant")
w := env.post("/pages/login", form, nil)
assert.Equal(
t, http.StatusRequestEntityTooLarge, w.Code,
)
assert.False(
t, csrfCookieSet(w),
"CSRF middleware must not run for an oversized body",
)
}
// TestPagesLogin_UnderLimit_NoToken_CSRFRejects is the control for
// the test above: an identically shaped but under-limit POST does
// reach gorilla/csrf, which rejects it and issues its cookie. Without
// this, the missing-cookie assertion above would prove nothing.
func TestPagesLogin_UnderLimit_NoToken_CSRFRejects(t *testing.T) {
t.Parallel()
env := newTestEnv(t)
form := url.Values{}
form.Set("username", "someone")
form.Set("password", "irrelevant")
w := env.post("/pages/login", form, nil)
assert.Equal(t, http.StatusForbidden, w.Code)
assert.True(
t, csrfCookieSet(w),
"CSRF middleware should run for an under-limit body",
)
}
// TestPagesLogin_UnderLimit_ValidToken_ReachesHandler proves the
// reorder did not break CSRF token handling: a token harvested from
// the rendered login form is still accepted and the request lands in
// the handler.
func TestPagesLogin_UnderLimit_ValidToken_ReachesHandler(
t *testing.T,
) {
t.Parallel()
env := newTestEnv(t)
token, cookies := env.csrfFrom(t, "/pages/login", nil)
form := url.Values{}
form.Set("csrf_token", token)
form.Set("username", "nosuchuser")
form.Set("password", "wrongpassword")
w := env.post("/pages/login", form, cookies)
assert.Equal(t, http.StatusUnauthorized, w.Code)
assert.Contains(
t, w.Body.String(), "Invalid username or password",
"request should reach the login handler",
)
}
// --- /user/{username} group ---
// TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged
// covers the route that previously had no middleware body cap at
// all. The request carries a valid session and a valid CSRF token,
// so the only thing that can stop it is the size cap; the unchanged
// password hash is the observable proof the handler never ran.
func TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged(
t *testing.T,
) {
t.Parallel()
env := newTestEnv(t)
userID, originalHash := env.seedUser(t, "pwuser", "oldpassword")
cookies := env.authCookies(t, userID, "pwuser")
token, cookies := env.csrfFrom(t, "/user/pwuser/", cookies)
form := url.Values{}
form.Set("csrf_token", token)
form.Set("current_password", "oldpassword")
form.Set("new_password", oversizeValue())
form.Set("confirm_password", oversizeValue())
w := env.post("/user/pwuser/password", form, cookies)
assert.Equal(
t, http.StatusRequestEntityTooLarge, w.Code,
)
assert.Equal(
t, originalHash, env.storedHash(t, "pwuser"),
"handler must not run, so the password must be unchanged",
)
}
// TestPasswordChange_UnderLimit_Succeeds proves that adding the cap
// to the /user/{username} group did not break the route it guards.
func TestPasswordChange_UnderLimit_Succeeds(t *testing.T) {
t.Parallel()
env := newTestEnv(t)
userID, originalHash := env.seedUser(t, "okuser", "oldpassword")
cookies := env.authCookies(t, userID, "okuser")
token, cookies := env.csrfFrom(t, "/user/okuser/", cookies)
form := url.Values{}
form.Set("csrf_token", token)
form.Set("current_password", "oldpassword")
form.Set("new_password", "brandnewpassword")
form.Set("confirm_password", "brandnewpassword")
w := env.post("/user/okuser/password", form, cookies)
assert.Equal(t, http.StatusOK, w.Code)
assert.NotEqual(
t, originalHash, env.storedHash(t, "okuser"),
"an under-limit password change should still apply",
)
}