From 507980a347eb68ca021947f98d5280c6818fa1ee Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Thu, 1 Oct 2026 21:38:48 +0200 Subject: [PATCH] Bound username length at creation (closes #184) Usernames are limited to 1024 bytes, so no account can exist that is unable to log in. The username rides in the session cookie, which browsers and securecookie refuse past about 4 KB, leaving room for roughly 2000 bytes of username; the limit is about half that. User.BeforeSave returns ErrUsernameTooLong when a whole User is created or saved. A byte-counting check constraint on users.username catches every other write, including a column update. The limit appears in the constant and in the struct tag; a test fails if they disagree. Model: opus-5-5 --- README.md | 18 ++++---- internal/database/model_user.go | 49 ++++++++++++++++++++- internal/database/model_user_test.go | 65 ++++++++++++++++++++++++++++ internal/handlers/auth_test.go | 30 +++++++++++++ internal/handlers/logbound_test.go | 8 ++-- 5 files changed, 154 insertions(+), 16 deletions(-) create mode 100644 internal/database/model_user_test.go diff --git a/README.md b/README.md index c1bb19b..f6a682d 100644 --- a/README.md +++ b/README.md @@ -1439,7 +1439,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. @@ -2379,14 +2379,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..2c95040 100644 --- a/internal/database/model_user.go +++ b/internal/database/model_user.go @@ -1,13 +1,58 @@ 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 when a whole +// User is created or saved, so those calls get ErrUsernameTooLong rather +// than the database's constraint error. A column update such as +// Update("username", ...) is caught only by the check constraint, as is +// 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.