Close archive writers when the delivery engine stops (closes #280)

The engine cached archive writers and never closed them at shutdown,
so after a clean stop an archive's rows could sit in its -wal while
the .db held no table. The engine's stop hook now evicts every cached
writer once its workers have returned, the same way deleting a webhook
does, so a clean stop leaves each archive as one file and a late write
is refused. If the workers do not return within the stop budget, the
writers are left open as a kill would leave them.

The README no longer says archives keep their sidecars across a clean
stop.

Model: opus-5-5

README.md

@@ -157,11 +157,6 @@ private and reserved ranges — RFC 1918, loopback, CGNAT, link-local and
 the rest — are refused, which stops a target from being used to make
 webhooker probe the network it sits in.
 
-Besides the private and reserved ranges, the default blocklist refuses
-public cloud metadata addresses: currently only `168.63.129.16`, Azure's
-WireServer, which serves an Azure VM its credentials. Because it is a
-public address, listing it in `ALLOWED_EGRESS_CIDRS` reopens it.
-
 That default is also inconvenient for the thing webhooker is mostly
 for: taking a public webhook and forwarding it to something on your own
 network. A container on the same Docker network, a box on `10.x`, a
@@ -200,16 +195,15 @@ Two things this setting cannot do:
   the list is always an allowlist; an empty list (the default) means
   every private and reserved range stays refused. Note that
   `0.0.0.0/0` gets you most of the way there anyway, per above.
-- **It cannot open link-local, or a cloud metadata endpoint at a
-  non-public address that discloses credentials or user data.** An
-  address is on the list below when it is not a public address and both
-  of these hold: the provider fixes it, so it cannot collide with
-  anything you run; and reaching it hands out credentials, user data or
-  bootstrap material. Those stay blocked no matter what you list,
-  including when you list them outright or list a supernet such as
-  `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as best
-  effort rather than a guarantee — it is a hand-maintained list and the
-  caveat below the table applies:
+- **It cannot open link-local, or a cloud metadata endpoint that
+  discloses credentials or user data.** An address is on the list below
+  when both of these hold: the provider fixes it, so it cannot collide
+  with anything you run; and reaching it hands out credentials, user
+  data or bootstrap material. Those stay blocked no matter what you
+  list, including when you list them outright or list a supernet such
+  as `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as
+  best effort rather than a guarantee — it is a hand-maintained list
+  and the caveat below the table applies:
 
   | Blocked unconditionally | What it is |
   | ----------------------- | ---------- |
@@ -248,8 +242,7 @@ Two things this setting cannot do:
   encodings, which the default blocklist does not match. A publicly
   routable metadata address is not listed here, because nothing on this
   list can be reopened and blocking one that way would leave you no
-  escape hatch at all; Azure's `168.63.129.16` is refused by the default
-  blocklist instead, as described above.
+  escape hatch at all.
 
   This list is not exhaustive of every cloud's metadata address — if
   yours is not here, do not allowlist the block that contains it.
@@ -1472,7 +1465,7 @@ A registered user of the webhooker service.
 | Field      | Type     | Description |
 | ---------- | -------- | ----------- |
 | `id`       | UUID     | Primary key |
-| `username` | string   | Unique login name, at most 1024 bytes so that it fits in the session cookie |
+| `username` | string   | Unique login name |
 | `password` | string   | Argon2id hash (never exposed via API) |
 
 **Relations:** Has many Webhooks. Has many APIKeys.
@@ -2412,14 +2405,14 @@ Removing either cap fails 14 subtests.
 
 `internal/middleware/logbound_test.go` and
 `internal/handlers/logbound_test.go` drive 8 KB of client-chosen text
-at each of these — just under 1 KB at `invalid password`, whose
-accounts are shared with the successful-login line and so must stay
-within the 1024-byte username limit — through both handlers, and
-through seven fills: plain text as the baseline, and then the
-quotation mark, backslash, tab, newline, C0 control and astral
-non-printable, six characters the wider of the two handlers spends
-more on than the client spent sending them. Every case holds each
-line to the 2,560-byte ceiling. That per-line ceiling
+at each of these — 1 KB at `invalid password`, whose accounts are
+shared with the successful-login line, where a username past 4 KB
+overflows the session cookie and answers 500 before that line is
+written — through both handlers, and through seven fills: plain text
+as the baseline, and then the quotation mark, backslash, tab, newline,
+C0 control and astral non-printable, six characters the wider of the
+two handlers spends more on than the client spent sending them. Every
+case holds each line to the 2,560-byte ceiling. That per-line ceiling
 is what the figure above states, and every row establishes it.
 
 Three of the sites go further and bound the whole flood's output — the

internal/config/config.go

@@ -192,10 +192,9 @@ type Config struct {
 	// alwaysBlockedNetworks stays blocked no matter what is listed
 	// here. That set is link-local plus the cloud metadata
 	// endpoints outside it that disclose credentials or user data
-	// at a provider-fixed, non-public address; it is not
-	// exhaustive of every cloud's metadata address. See
-	// alwaysBlockedNetworks for the authoritative list and the
-	// criterion it is built from.
+	// at a provider-fixed address; it is not exhaustive of every
+	// cloud's metadata address. See alwaysBlockedNetworks for the
+	// authoritative list and the criterion it is built from.
 	AllowedEgressCIDRs []netip.Prefix
 
 	params *ConfigParams
@@ -747,14 +746,12 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) {
 
 	log.Warn(
 		"ALLOWED_EGRESS_CIDRS lets delivery targets reach these "+
-			"otherwise-blocked networks. Anyone who can create a "+
-			"delivery target can now make this process issue "+
-			"requests into them, and read back the response. Only "+
-			"the addresses the README lists as blocked "+
-			"unconditionally stay blocked regardless of what is "+
-			"listed here; a public cloud metadata address such as "+
-			"168.63.129.16 is reachable once it, or a block "+
-			"covering it, is listed.",
+			"otherwise-blocked private/reserved networks. Anyone "+
+			"who can create a delivery target can now make this "+
+			"process issue requests into them, and read back the "+
+			"response. Link-local and the known cloud instance "+
+			"metadata endpoints outside it stay blocked "+
+			"regardless of what is listed here.",
 		"allowedEgressCIDRs",
 		strings.Join(PrefixStrings(c.AllowedEgressCIDRs), ","),
 	)

internal/config/config_test.go

@@ -834,13 +834,12 @@ func TestEgressAllowlistWarning(t *testing.T) {
 			// to be able to read back which networks are open.
 			assert.Contains(t, logged, "10.0.0.0/8")
 			assert.Contains(t, logged, "127.0.0.0/8")
-			// What stays shut is the whole unconditional set, not
-			// link-local alone; a public metadata address is not in
-			// it, so a listed block covering it opens it.
-			assert.Contains(t, logged, "blocked unconditionally")
-			assert.Contains(t, logged, "168.63.129.16 is reachable")
-			// The listed blocks need not be private or reserved.
-			assert.NotContains(t, logged, "private/reserved")
+			// What stays shut. Asserted on the clause naming the
+			// wider set rather than on "Link-local" alone, so the
+			// string cannot narrow back to link-local only while
+			// the always-blocked set covers ULA, CGNAT and two
+			// public metadata addresses as well.
+			assert.Contains(t, logged, "metadata endpoints outside it")
 		})
 	}
 }

internal/database/model_user.go

@@ -1,57 +1,13 @@
 package database
 
-import (
-	"errors"
-	"fmt"
-
-	"gorm.io/gorm"
-)
-
-// MaxUsernameBytes is the longest username, in bytes, that a user may
-// have. The same number appears in the check constraint on
-// User.Username, because a struct tag cannot reference a constant.
-//
-// A login stores the username in the session cookie, and both
-// securecookie and browsers refuse a cookie value past about 4096
-// bytes. That value is the session base64-encoded twice, so it holds
-// 4096 × 3/4 × 3/4 = 2304 bytes of session, and the signature,
-// timestamp and the session's other values take about 270 of those: a
-// username longer than about 2030 bytes can never log in. The limit is
-// about half that, so the session can carry more values later without
-// locking out an account whose username is already at the limit.
-const MaxUsernameBytes = 1024
-
-// ErrUsernameTooLong is returned when a user is saved with a username
-// longer than MaxUsernameBytes.
-var ErrUsernameTooLong = errors.New("username is too long")
-
 // User represents a user of the webhooker service
-//
-//nolint:lll // a struct tag cannot wrap
 type User struct {
 	BaseModel
 
-	Username string `gorm:"uniqueIndex;not null;check:length(CAST(username AS BLOB)) <= 1024" json:"username"`
-	Password string `gorm:"not null"                                                          json:"-"` // Argon2 hashed
+	Username string `gorm:"uniqueIndex;not null" json:"username"`
+	Password string `gorm:"not null"             json:"-"` // Argon2 hashed
 
 	// Relations
 	Webhooks []Webhook `json:"webhooks,omitempty"`
 	APIKeys  []APIKey  `json:"apiKeys,omitempty"`
 }
-
-// BeforeSave rejects a username longer than MaxUsernameBytes, so every
-// path that saves a user through GORM gets ErrUsernameTooLong rather
-// than the database's constraint error. The check constraint behind it
-// holds for any path that writes the table without this model.
-func (u *User) BeforeSave(_ *gorm.DB) error {
-	if len(u.Username) > MaxUsernameBytes {
-		return fmt.Errorf(
-			"%w: %d bytes, limit is %d",
-			ErrUsernameTooLong,
-			len(u.Username),
-			MaxUsernameBytes,
-		)
-	}
-
-	return nil
-}

internal/database/model_user_test.go

@@ -1,65 +0,0 @@
-package database_test
-
-import (
-	"strings"
-	"testing"
-
-	"github.com/google/uuid"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/database"
-)
-
-// usernameAtLimit is exactly MaxUsernameBytes long, built from a
-// two-byte character. A check that counted characters rather than bytes
-// would see half the length and let the one-byte-longer name through.
-func usernameAtLimit() string {
-	return strings.Repeat("é", database.MaxUsernameBytes/2)
-}
-
-func TestUserCreate_RejectsOverlongUsername(t *testing.T) {
-	t.Parallel()
-
-	db := startedTestDB(t)
-
-	err := db.Create(&database.User{
-		Username: usernameAtLimit() + "x",
-		Password: "hash",
-	}).Error
-
-	require.ErrorIs(t, err, database.ErrUsernameTooLong)
-}
-
-func TestUserCreate_AcceptsUsernameAtLimit(t *testing.T) {
-	t.Parallel()
-
-	db := startedTestDB(t)
-
-	require.NoError(t, db.Create(&database.User{
-		Username: usernameAtLimit(),
-		Password: "hash",
-	}).Error)
-}
-
-// TestUsersTable_EnforcesUsernameLimitWithoutTheModel inserts with raw
-// SQL, as a path that bypassed User.BeforeSave would, so only the
-// table's check constraint stands between it and an over-long
-// username. Accepting the name at the limit and refusing the next byte
-// also pins the constraint's number to MaxUsernameBytes.
-func TestUsersTable_EnforcesUsernameLimitWithoutTheModel(t *testing.T) {
-	t.Parallel()
-
-	db := startedTestDB(t)
-
-	insert := "INSERT INTO users (id, username, password) VALUES (?, ?, ?)"
-
-	require.NoError(t, db.Exec(
-		insert, uuid.New().String(), usernameAtLimit(), "hash",
-	).Error)
-
-	err := db.Exec(
-		insert, uuid.New().String(), usernameAtLimit()+"x", "hash",
-	).Error
-	require.Error(t, err)
-	assert.Contains(t, err.Error(), "CHECK constraint failed")
-}

internal/delivery/engine.go

@@ -368,9 +368,9 @@ func (e *Engine) start() {
 // writer for long by then: the archive sweeper stops before the
 // engine, and deleting a webhook only closes one. If the pool did
 // not drain in time, the writers are left open, as a kill would
-// leave them. Closing them would wait for any write in progress,
-// and a worker still running would then open new writers that
-// nothing closes, so it gains nothing over a kill.
+// leave them: a worker still running may be mid-write, and
+// closing its writer would wait on that write and then fail the
+// next delivery the worker archives.
 func (e *Engine) stop(ctx context.Context) error {
 	e.log.Info("delivery engine stopping")
 

internal/delivery/engine_lifecycle_test.go

@@ -327,10 +327,10 @@ func TestEngine_StopHookClosesArchives(t *testing.T) {
 }
 
 // TestEngine_StopHookTimeoutLeavesArchivesOpen covers a stop whose
-// budget runs out while a worker is still running. The archive
-// writers are left open, as a kill would leave them: closing them
-// would wait for any write in progress, and that worker would then
-// open new writers that nothing closes.
+// budget runs out while a worker is still running. That worker may
+// be in the middle of an archive write, so the archive writers are
+// left open, as a kill would leave them, rather than closed
+// underneath it.
 func TestEngine_StopHookTimeoutLeavesArchivesOpen(t *testing.T) {
 	t.Parallel()
 

internal/delivery/engine_test.go

@@ -1166,10 +1166,6 @@ func TestIsForwardableHeader(t *testing.T) {
 	assert.False(t,
 		delivery.ExportIsForwardableHeader("Content-Length"),
 	)
-
-	assert.False(t,
-		delivery.ExportIsForwardableHeader("Content-Type"),
-	)
 }
 
 func TestTruncate(t *testing.T) {
@@ -1251,81 +1247,6 @@ func TestDoHTTPRequest_ForwardsHeaders(t *testing.T) {
 	)
 }
 
-// The event's stored inbound headers carry the same Content-Type the
-// receiver saved as the event's ContentType, so a delivery could send
-// it twice. It must go out exactly once, with a Content-Type configured
-// on the target winning, then the event's ContentType.
-func TestApplyRequestHeaders_SendsOneContentType(t *testing.T) {
-	t.Parallel()
-
-	cases := map[string]struct {
-		inbound    string
-		event      string
-		configured string
-		want       []string
-	}{
-		"inbound and event agree": {
-			inbound: testContentType,
-			event:   testContentType,
-			want:    []string{testContentType},
-		},
-		"inbound and event disagree": {
-			inbound: "text/plain",
-			event:   testContentType,
-			want:    []string{testContentType},
-		},
-		"event has none": {
-			inbound: testContentType,
-			want:    nil,
-		},
-		"target configures its own": {
-			inbound:    testContentType,
-			event:      testContentType,
-			configured: "application/xml",
-			want:       []string{"application/xml"},
-		},
-	}
-
-	for name, tc := range cases {
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
-
-			inbound, err := json.Marshal(map[string][]string{
-				headerContentType: {tc.inbound},
-			})
-			require.NoError(t, err)
-
-			cfg := &delivery.HTTPTargetConfig{}
-			if tc.configured != "" {
-				cfg.Headers = map[string]string{
-					headerContentType: tc.configured,
-				}
-			}
-
-			req, err := http.NewRequestWithContext(
-				context.Background(),
-				http.MethodPost,
-				"https://target.example.com/hook",
-				http.NoBody,
-			)
-			require.NoError(t, err)
-
-			delivery.ExportApplyRequestHeaders(
-				req,
-				&database.Event{
-					Headers:     string(inbound),
-					ContentType: tc.event,
-				},
-				cfg,
-			)
-
-			assert.Equal(t,
-				tc.want, req.Header.Values(headerContentType),
-			)
-		})
-	}
-}
-
 func TestProcessDelivery_RoutesToCorrectHandler(
 	t *testing.T,
 ) {

internal/delivery/redirect_test.go

@@ -339,11 +339,10 @@ func TestRedirectPolicy_StopsAtHopCap(t *testing.T) {
 // The set the redirect policy strips is whatever the delivery path
 // actually put on the wire, so a header added to the forward set is
 // covered without a second edit. A header the event never carried
-// is not in the set, and neither is the inbound Content-Type, because
-// it is not forwarded. Two more are deliberately excluded: a
-// Content-Type configured on the target describes the body, which a
-// 307 carries across hosts, and the inbound User-Agent every real
-// sender supplies is overwritten before the request goes out.
+// is not in the set, and the delivery path's own two are deliberately
+// excluded: Content-Type describes the body, which a 307 carries
+// across hosts, and the inbound User-Agent every real sender supplies
+// is overwritten before the request goes out.
 func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 	t.Parallel()
 
@@ -372,7 +371,6 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 		&delivery.HTTPTargetConfig{
 			Headers: map[string]string{
 				probeHeaderName: probeHeaderValue,
-				"Content-Type":  testContentType,
 			},
 		},
 	)
@@ -380,11 +378,7 @@ func TestApplyRequestHeaders_ReportsOriginScopedNames(t *testing.T) {
 	assert.Equal(t,
 		[]string{probeHeaderName, inboundHeaderName}, names,
 		"both header classes are reported, and only those: "+
-			"Host and the inbound Content-Type are never "+
-			"forwarded, User-Agent is the delivery path's own",
-	)
-	assert.NotContains(t, names, "Content-Type",
-		"a Content-Type configured on the target must survive "+
-			"a cross-origin 307/308 with the body it describes",
+			"Host is never forwarded, Content-Type and "+
+			"User-Agent are the delivery path's own",
 	)
 }

internal/delivery/ssrf.go

@@ -26,7 +26,7 @@ var (
 		"hostname resolved to no IP addresses",
 	)
 	errBlockedIP = errors.New(
-		"blocked private, reserved or cloud metadata address",
+		"blocked private/reserved IP range",
 	)
 	errBlockedMetadata = errors.New(
 		"blocked link-local or cloud instance metadata " +
@@ -37,10 +37,9 @@ var (
 	)
 )
 
-// blockedNetworks is the default blocklist: the private and
-// reserved IP ranges, plus the public cloud metadata addresses,
-// that are blocked to prevent SSRF attacks. An operator can
-// permit specific blocks out of this set with
+// blockedNetworks contains all private/reserved IP ranges
+// that should be blocked to prevent SSRF attacks. An operator
+// can permit specific blocks out of this set with
 // ALLOWED_EGRESS_CIDRS; see Guard.
 //
 //nolint:gochecknoglobals // package-level network list is appropriate here
@@ -123,8 +122,6 @@ func init() {
 		"::1/128",
 		"fc00::/7",
 		"fe80::/10",
-		// Azure WireServer, a public address that serves VM credentials.
-		"168.63.129.16/32",
 	})
 
 	// Every entry is named. The set must not grow or shrink
@@ -219,8 +216,8 @@ func matchesAny(networks []*net.IPNet, ip net.IP) bool {
 }
 
 // isBlockedIP checks whether an IP address falls within
-// the default blocklist, before any operator allowlist is
-// considered.
+// any blocked private/reserved network range, before any
+// operator allowlist is considered.
 func isBlockedIP(ip net.IP) bool {
 	return matchesAny(blockedNetworks, ip)
 }
@@ -323,7 +320,7 @@ func (g *Guard) allows(ip net.IP) bool {
 //
 //  1. alwaysBlockedNetworks is refused before the allowlist is
 //     consulted, so no configured CIDR reaches link-local or a
-//     cloud metadata endpoint at a non-public address.
+//     cloud instance metadata endpoint.
 //  2. The allowlist is consulted next, so a listed private
 //     network becomes reachable.
 //  3. Everything else keeps the default blocklist's answer.

internal/delivery/ssrf_allowlist_test.go

@@ -390,41 +390,6 @@ func TestGuardAllowlist_PublicUnaffected(t *testing.T) {
 	}
 }
 
-// TestGuardAllowlist_AzureWireServerReopenable covers Azure's
-// WireServer, a public address that serves VM credentials. The
-// default guard refuses it, but because it is public it sits in
-// the default blocklist rather than the unconditional set, so an
-// operator who lists it can reach it.
-func TestGuardAllowlist_AzureWireServerReopenable(t *testing.T) {
-	t.Parallel()
-
-	const wireServerIP = "168.63.129.16"
-
-	target := "http://" + wireServerIP + "/?comp=versions"
-
-	defaultGuard := delivery.NewTestGuard()
-
-	err := defaultGuard.ValidateTargetURL(context.Background(), target)
-	require.Error(t, err,
-		"WireServer must be refused with no allowlist set",
-	)
-	assert.NotContains(t, err.Error(), metadataRefusalClause,
-		"WireServer must be refused by the default blocklist, "+
-			"which an allowlist can override",
-	)
-
-	assertDialRefused(t, defaultGuard, target)
-
-	listed := delivery.NewTestGuard(
-		netip.MustParsePrefix(wireServerIP + "/32"),
-	)
-
-	assert.NoError(t,
-		listed.ValidateTargetURL(context.Background(), target),
-		"an operator who lists WireServer must be able to reach it",
-	)
-}
-
 // TestGuardCheckIP_BothPathsShareOneDecision asserts that the
 // validator and the dialer are not two policies that happen to
 // agree: both are defined in terms of checkIP, so the exported

internal/delivery/target_headers_test.go

@@ -11,11 +11,10 @@ import (
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
-// Literals these tests repeat, named so that the header names and the
+// Literals these tests repeat, named so that the header name and the
 // keep-forever archive config each have one definition.
 const (
 	headerAuthorization = "Authorization"
-	headerContentType   = "Content-Type"
 	bearerValue         = "Bearer abc"
 	archiveConfigNever  = "{\"expiry\":\"never\"}"
 )

internal/delivery/target_http.go

@@ -541,11 +541,6 @@ func isForwardableHeader(name string) bool {
 		"Upgrade", "Proxy-Authorization",
 		"Proxy-Connection", "Content-Length":
 		return false
-	case "Content-Type":
-		// applyRequestHeaders sets Content-Type itself. The receiver
-		// already stored this inbound value as the event's
-		// ContentType, so forwarding it too would send it twice.
-		return false
 	default:
 		return true
 	}
@@ -558,10 +553,6 @@ func isForwardableHeader(name string) bool {
 // policy strips exactly that set on a hop that leaves the origin,
 // so the forward set is decided here and only here — a header added
 // to it is covered off-origin without a second edit elsewhere.
-//
-// Content-Type goes out once: a Content-Type configured on the target
-// wins, otherwise the event's ContentType, otherwise none. The inbound
-// Content-Type in the event's headers is never forwarded.
 func applyRequestHeaders(
 	req *http.Request,
 	event *database.Event,
@@ -582,10 +573,10 @@ func applyRequestHeaders(
 
 	req.Header.Set("User-Agent", "webhooker/1.0")
 
-	// A Content-Type configured on the target describes the body
-	// being sent rather than the sender. A 307/308 preserves the
-	// body across hosts, so stripping it would send that body
-	// untyped.
+	// Content-Type describes the body being sent rather than the
+	// sender, and the delivery path sets it from the event itself.
+	// A 307/308 preserves the body across hosts, so stripping it
+	// would send that body untyped.
 	delete(originScoped, "Content-Type")
 
 	// User-Agent is overwritten just above, so an inbound one never

internal/handlers/auth_test.go

@@ -453,33 +453,3 @@ func TestLogin_SuccessCreatesSession(t *testing.T) {
 		"the issued cookie must carry an authenticated session",
 	)
 }
-
-// TestLogin_UsernameAtLimitCanLogIn shows that a username of exactly
-// database.MaxUsernameBytes still fits in the session cookie. Past
-// what the cookie can carry, a correct login answers 500.
-func TestLogin_UsernameAtLimitCanLogIn(t *testing.T) {
-	t.Parallel()
-
-	var (
-		h  *handlers.Handlers
-		db *database.Database
-	)
-
-	app := newTestApp(t, &h, &db)
-	app.RequireStart()
-
-	t.Cleanup(app.RequireStop)
-
-	username := strings.Repeat("a", database.MaxUsernameBytes)
-
-	hash, err := database.HashPassword(operatorPassword)
-	require.NoError(t, err)
-	require.NoError(t, db.DB().Create(&database.User{
-		Username: username,
-		Password: hash,
-	}).Error)
-
-	w := submitLogin(h, sharedProxyPeer, username, operatorPassword)
-
-	assert.Equal(t, http.StatusSeeOther, w.Code)
-}

internal/handlers/logbound_test.go

@@ -339,9 +339,11 @@ const storedUserPassword = "correct-horse-battery-staple"
 // storedFillBytes is the raw length of the client-chosen value in
 // those accounts' usernames. It is well past the 512-byte field
 // budget, so the line is still truncated, but short enough that the
-// whole username, markers and fill name included, stays within
-// database.MaxUsernameBytes.
-const storedFillBytes = 960
+// session cookie a successful login writes stays inside
+// securecookie's 4 KB limit: the cookie is written BEFORE the
+// "user logged in" line, so an 8 KB username answers 500 and never
+// reaches it.
+const storedFillBytes = 1024
 
 // storedFill builds a username fill of storedFillBytes raw bytes out
 // of repetitions of ch, with both markers at its far end.

internal/middleware/recoverer.go

@@ -133,11 +133,6 @@ func (w *recoverResponseWriter) Unwrap() http.ResponseWriter {
 // what the access log records and the metrics count, and outside the
 // sentryhttp handler, whose Repanic option depends on something
 // further out recovering what it re-raises.
-//
-// Unlike http.Error on its own, it deletes any Set-Cookie the handler
-// set before panicking, because a request that failed must not hand
-// the client a credential; every other header is left to http.Error.
-// See https://git.eeqj.de/sneak/webhooker/issues/193.
 func (s *Middleware) Recoverer() func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(
@@ -169,8 +164,6 @@ func (s *Middleware) Recoverer() func(http.Handler) http.Handler {
 					return
 				}
 
-				rw.Header().Del("Set-Cookie")
-
 				http.Error(
 					rw,
 					http.StatusText(

internal/middleware/recoverer_test.go

@@ -304,44 +304,16 @@ func TestRecovererRepanicsErrAbortHandler(t *testing.T) {
 	)
 }
 
-// TestRecovererDropsSetCookieFromTheRecovered500 covers a handler that
-// sets a cookie and a redirect target and then panics before sending
-// anything. A request that failed must not hand the client a
-// credential, so the 500 carries no cookie; Location is left alone.
-func TestRecovererDropsSetCookieFromTheRecovered500(t *testing.T) {
-	t.Parallel()
-
-	probe := newRecovererProbe(
-		t, false,
-		func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Set-Cookie", "session=x")
-			w.Header().Set("Location", "/after")
-
-			panic(panicMarker)
-		},
-	)
-
-	resp, err := probe.get(t)
-	require.NoError(t, err)
-	require.NoError(t, resp.Body.Close())
-
-	assert.Equal(t, http.StatusInternalServerError, resp.StatusCode)
-	assert.Empty(t, resp.Cookies())
-	assert.Equal(t, "/after", resp.Header.Get("Location"))
-}
-
 // TestRecovererKeepsAnAlreadyCommittedResponse covers a handler that
 // panics after sending its status. The bytes are already on the wire,
-// cookie included, so a second WriteHeader would change nothing the
-// client sees and would draw net/http's "superfluous
-// response.WriteHeader" report.
+// so a second WriteHeader would change nothing the client sees and
+// would draw net/http's "superfluous response.WriteHeader" report.
 func TestRecovererKeepsAnAlreadyCommittedResponse(t *testing.T) {
 	t.Parallel()
 
 	probe := newRecovererProbe(
 		t, false,
 		func(w http.ResponseWriter, _ *http.Request) {
-			w.Header().Set("Set-Cookie", "session=x")
 			w.WriteHeader(committedStatus)
 			_, _ = w.Write([]byte("partial"))
 
@@ -359,7 +331,6 @@ func TestRecovererKeepsAnAlreadyCommittedResponse(t *testing.T) {
 
 	assert.Equal(t, committedStatus, resp.StatusCode)
 	assert.Equal(t, "partial", string(body))
-	assert.Len(t, resp.Cookies(), 1)
 
 	record := probe.panicRecord(t)
 	assert.Equal(t, panicMarker, record["panic"])