Correct the stop timeout comments on the archive writers

The comments on the engine's stop and on the timeout test gave false
reasons for leaving archive writers open when the stop budget runs
out. Closing them would wait for any write in progress, and a worker
still running would then open new writers that nothing closes, so
closing gains nothing over a kill.

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
+- **It cannot open link-local, or a cloud metadata endpoint that
-  non-public address that discloses credentials or user data.** An
+  discloses credentials or user data.** An address is on the list below
-  address is on the list below when it is not a public address and both
+  when both of these hold: the provider fixes it, so it cannot collide
-  of these hold: the provider fixes it, so it cannot collide with
+  with anything you run; and reaching it hands out credentials, user
-  anything you run; and reaching it hands out credentials, user data or
+  data or bootstrap material. Those stay blocked no matter what you
-  bootstrap material. Those stay blocked no matter what you list,
+  list, including when you list them outright or list a supernet such
-  including when you list them outright or list a supernet such as
+  as `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as
-  `0.0.0.0/0`, `::/0`, `fd00::/8` or `100.64.0.0/10`. Treat this as best
+  best effort rather than a guarantee — it is a hand-maintained list
-  effort rather than a guarantee — it is a hand-maintained list and the
+  and the caveat below the table applies:
-  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
+  escape hatch at all.
-  blocklist instead, as described above.
 
   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
+at each of these — 1 KB at `invalid password`, whose accounts are
-accounts are shared with the successful-login line and so must stay
+shared with the successful-login line, where a username past 4 KB
-within the 1024-byte username limit — through both handlers, and
+overflows the session cookie and answers 500 before that line is
-through seven fills: plain text as the baseline, and then the
+written — through both handlers, and through seven fills: plain text
-quotation mark, backslash, tab, newline, C0 control and astral
+as the baseline, and then the quotation mark, backslash, tab, newline,
-non-printable, six characters the wider of the two handlers spends
+C0 control and astral non-printable, six characters the wider of the
-more on than the client spent sending them. Every case holds each
+two handlers spends more on than the client spent sending them. Every
-line to the 2,560-byte ceiling. That per-line ceiling
+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
+	// at a provider-fixed address; it is not exhaustive of every
-	// exhaustive of every cloud's metadata address. See
+	// cloud's metadata address. See alwaysBlockedNetworks for the
-	// alwaysBlockedNetworks for the authoritative list and the
+	// authoritative list and the criterion it is built from.
-	// 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 "+
+			"otherwise-blocked private/reserved networks. Anyone "+
-			"delivery target can now make this process issue "+
+			"who can create a delivery target can now make this "+
-			"requests into them, and read back the response. Only "+
+			"process issue requests into them, and read back the "+
-			"the addresses the README lists as blocked "+
+			"response. Link-local and the known cloud instance "+
-			"unconditionally stay blocked regardless of what is "+
+			"metadata endpoints outside it stay blocked "+
-			"listed here; a public cloud metadata address such as "+
+			"regardless of what is listed here.",
-			"168.63.129.16 is reachable once it, or a block "+
-			"covering it, is listed.",
 		"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
+			// What stays shut. Asserted on the clause naming the
-			// link-local alone; a public metadata address is not in
+			// wider set rather than on "Link-local" alone, so the
-			// it, so a listed block covering it opens it.
+			// string cannot narrow back to link-local only while
-			assert.Contains(t, logged, "blocked unconditionally")
+			// the always-blocked set covers ULA, CGNAT and two
-			assert.Contains(t, logged, "168.63.129.16 is reachable")
+			// public metadata addresses as well.
-			// The listed blocks need not be private or reserved.
+			assert.Contains(t, logged, "metadata endpoints outside it")
-			assert.NotContains(t, logged, "private/reserved")
 		})
 	}
 }

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"`
+	Username string `gorm:"uniqueIndex;not null" json:"username"`
-	Password string `gorm:"not null"                                                          json:"-"` // Argon2 hashed
+	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/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
+// blockedNetworks contains all private/reserved IP ranges
-// reserved IP ranges, plus the public cloud metadata addresses,
+// that should be blocked to prevent SSRF attacks. An operator
-// that are blocked to prevent SSRF attacks. An operator can
+// can permit specific blocks out of this set with
-// 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
+// any blocked private/reserved network range, before any
-// considered.
+// 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/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
+// session cookie a successful login writes stays inside
-// database.MaxUsernameBytes.
+// securecookie's 4 KB limit: the cookie is written BEFORE the
-const storedFillBytes = 960
+// "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
+// so a second WriteHeader would change nothing the client sees and
-// client sees and would draw net/http's "superfluous
+// would draw net/http's "superfluous response.WriteHeader" report.
-// 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"])