From 41a1ecae8db976a739b43432eba89b27859ca9fc Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 08:35:25 +0000 Subject: [PATCH] Bound username length at creation (closes #184) A login stores the username in the session cookie, which cannot carry a value past about 4096 bytes, so a username of about 2 KB or more could never log in and the login answered 500. Usernames are now limited to 1024 bytes, about half of what the cookie can carry. The User model rejects a longer one with ErrUsernameTooLong, and a check constraint on the users table rejects it for any path that bypasses the model. The comment on MaxUsernameBytes gives the arithmetic. Model: opus-5-5 --- README.md | 18 ++++---- internal/database/model_user.go | 48 +++++++++++++++++++- internal/database/model_user_test.go | 65 ++++++++++++++++++++++++++++ internal/handlers/auth_test.go | 30 +++++++++++++ internal/handlers/logbound_test.go | 8 ++-- 5 files changed, 153 insertions(+), 16 deletions(-) create mode 100644 internal/database/model_user_test.go diff --git a/README.md b/README.md index 5729606..c841d59 100644 --- a/README.md +++ b/README.md @@ -1472,7 +1472,7 @@ A registered user of the webhooker service. | Field | Type | Description | | ---------- | -------- | ----------- | | `id` | UUID | Primary key | -| `username` | string | Unique login name | +| `username` | string | Unique login name, at most 1024 bytes so that it fits in the session cookie | | `password` | string | Argon2id hash (never exposed via API) | **Relations:** Has many Webhooks. Has many APIKeys. @@ -2412,14 +2412,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 — 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 +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 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 diff --git a/internal/database/model_user.go b/internal/database/model_user.go index ec2ca1e..08db8e6 100644 --- a/internal/database/model_user.go +++ b/internal/database/model_user.go @@ -1,13 +1,57 @@ 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" json:"username"` - Password string `gorm:"not null" json:"-"` // Argon2 hashed + Username string `gorm:"uniqueIndex;not null;check:length(CAST(username AS BLOB)) <= 1024" 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 +} diff --git a/internal/database/model_user_test.go b/internal/database/model_user_test.go new file mode 100644 index 0000000..9d82b8b --- /dev/null +++ b/internal/database/model_user_test.go @@ -0,0 +1,65 @@ +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") +} diff --git a/internal/handlers/auth_test.go b/internal/handlers/auth_test.go index 98c64c8..1fdbcb1 100644 --- a/internal/handlers/auth_test.go +++ b/internal/handlers/auth_test.go @@ -453,3 +453,33 @@ 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) +} diff --git a/internal/handlers/logbound_test.go b/internal/handlers/logbound_test.go index abb78f4..bddb149 100644 --- a/internal/handlers/logbound_test.go +++ b/internal/handlers/logbound_test.go @@ -339,11 +339,9 @@ 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 -// 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 +// whole username, markers and fill name included, stays within +// database.MaxUsernameBytes. +const storedFillBytes = 960 // storedFill builds a username fill of storedFillBytes raw bytes out // of repetitions of ch, with both markers at its far end.