From 8a34898ac4fd79bcaeaf520acda926803d484a59 Mon Sep 17 00:00:00 2001 From: Jonathon Leight Date: Sun, 5 Jul 2026 12:09:49 -0400 Subject: [PATCH] Input hardening --- internal/auth/account.go | 4 +- internal/auth/handlers.go | 14 ++++++ internal/auth/password_handlers.go | 4 +- internal/auth/service_test.go | 22 +++++++++- internal/core/endpoints_repeater_test.go | 56 ++++++++++++++++++++++++ internal/core/repeater_setup.go | 13 +++--- internal/core/repeaters.go | 28 +++++++++--- internal/web/env.go | 19 ++++++++ internal/web/limitbody_test.go | 36 +++++++++++++++ 9 files changed, 179 insertions(+), 17 deletions(-) create mode 100644 internal/web/limitbody_test.go diff --git a/internal/auth/account.go b/internal/auth/account.go index 8a1ce43..2cb666d 100644 --- a/internal/auth/account.go +++ b/internal/auth/account.go @@ -402,8 +402,8 @@ func (s *Handlers) handleChangePassword(w http.ResponseWriter, r *http.Request) } newPassword := r.FormValue("new_password") - if len(newPassword) < 8 { - accountRedirect(w, r, "error", "New password must be at least 8 characters.") + if !ValidPassword(newPassword) { + accountRedirect(w, r, "error", "New password must be 8–72 characters.") return } // When a password already exists, require the current one to change it. diff --git a/internal/auth/handlers.go b/internal/auth/handlers.go index 12d7c2c..5a4bb89 100644 --- a/internal/auth/handlers.go +++ b/internal/auth/handlers.go @@ -364,6 +364,20 @@ func NormalizePasskeyName(s string) string { return s } +// Password length bounds. The lower bound is a basic strength floor; the upper +// bound is bcrypt's hard limit — GenerateFromPassword rejects inputs longer than +// 72 bytes, so we reject them at the form boundary with a clear message rather +// than surfacing a generic hashing failure. +const ( + MinPasswordLen = 8 + MaxPasswordLen = 72 +) + +// ValidPassword reports whether p is within the accepted length bounds. +func ValidPassword(p string) bool { + return len(p) >= MinPasswordLen && len(p) <= MaxPasswordLen +} + // ValidUsername reports whether s is 3–32 chars of [a-z0-9_.-]. func ValidUsername(s string) bool { if len(s) < 3 || len(s) > 32 { diff --git a/internal/auth/password_handlers.go b/internal/auth/password_handlers.go index a982a5d..ca14e3b 100644 --- a/internal/auth/password_handlers.go +++ b/internal/auth/password_handlers.go @@ -13,8 +13,8 @@ func (s *Service) SignupPassword(w http.ResponseWriter, r *http.Request) { username := NormalizeUsername(r.FormValue("username")) displayName := NormalizeDisplayName(r.FormValue("display_name")) password := r.FormValue("password") - if !ValidUsername(username) || len(password) < 8 { - redirectErr(w, r, "/signup", "Choose a username (3–32 chars: letters, digits, _ . -) and a password of at least 8 characters.") + if !ValidUsername(username) || !ValidPassword(password) { + redirectErr(w, r, "/signup", "Choose a username (3–32 chars: letters, digits, _ . -) and a password of 8–72 characters.") return } diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index a0255a2..5822679 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -1,6 +1,26 @@ package auth -import "testing" +import ( + "strings" + "testing" +) + +// TestValidPassword pins the accepted length window: an 8-char floor and a +// 72-byte ceiling (bcrypt's hard input limit). Reject just outside both ends. +func TestValidPassword(t *testing.T) { + t.Parallel() + cases := []struct { + n int + want bool + }{ + {7, false}, {8, true}, {72, true}, {73, false}, + } + for _, c := range cases { + if got := ValidPassword(strings.Repeat("a", c.n)); got != c.want { + t.Errorf("ValidPassword(len=%d) = %v, want %v", c.n, got, c.want) + } + } +} func TestSafeLocalPath(t *testing.T) { t.Parallel() diff --git a/internal/core/endpoints_repeater_test.go b/internal/core/endpoints_repeater_test.go index 0afeaae..a1df3d8 100644 --- a/internal/core/endpoints_repeater_test.go +++ b/internal/core/endpoints_repeater_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/url" "strconv" + "strings" "testing" meshcore "github.com/meshcore-go/meshcore-go" @@ -13,6 +14,61 @@ import ( "github.com/jleight/meshtender/internal/store" ) +// TestCleanRepeaterName pins the firmware-derived byte bound: MeshCore stores a +// node name in a 32-byte buffer and NUL-terminates it, leaving 31 usable bytes. +func TestCleanRepeaterName(t *testing.T) { + t.Parallel() + cases := []struct { + in string + want string + wantOK bool + }{ + {" Repeater One ", "Repeater One", true}, + {"", "", false}, + {" ", "", false}, + {strings.Repeat("x", 31), strings.Repeat("x", 31), true}, + {strings.Repeat("x", 32), strings.Repeat("x", 32), false}, + {" " + strings.Repeat("x", 31) + " ", strings.Repeat("x", 31), true}, // trimmed to the limit + } + for _, c := range cases { + got, ok := cleanRepeaterName(c.in) + if got != c.want || ok != c.wantOK { + t.Errorf("cleanRepeaterName(%q) = (%q, %v), want (%q, %v)", c.in, got, ok, c.want, c.wantOK) + } + } +} + +// TestRepeaterNameLengthBound: a name over MeshCore's 31-byte limit is rejected +// at create (it would be silently truncated on the device); exactly-at-limit +// succeeds. Regression for the pre-release audit finding that names were +// unbounded. +func TestRepeaterNameLengthBound(t *testing.T) { + t.Parallel() + st, ctx, ts, h := splitServer(t) + _, sess := appLogin(t, ts, st, ctx, h.app, "namelen") + + form := func(name, key string) url.Values { + return url.Values{ + "name": {name}, "public_key": {key}, + "radio_freq_mhz": {"869.525"}, "radio_bw_khz": {"250"}, "radio_sf": {"11"}, "radio_cr": {"5"}, + } + } + + overID, _ := meshcore.GenerateLocalIdentity(rand.Reader) + over := post(t, ts, h.app, "/repeaters", form(strings.Repeat("x", maxRepeaterNameLen+1), overID.String()), sess) + over.Body.Close() + if loc, _ := url.Parse(over.Header.Get("Location")); over.StatusCode != http.StatusSeeOther || loc.Path != "/repeaters/add" || loc.Query().Get("error") == "" { + t.Fatalf("over-long create = %d %q, want 303 → /repeaters/add?error", over.StatusCode, over.Header.Get("Location")) + } + + atID, _ := meshcore.GenerateLocalIdentity(rand.Reader) + at := post(t, ts, h.app, "/repeaters", form(strings.Repeat("x", maxRepeaterNameLen), atID.String()), sess) + at.Body.Close() + if loc, _ := url.Parse(at.Header.Get("Location")); at.StatusCode != http.StatusSeeOther || !strings.HasSuffix(loc.Path, "/added") { + t.Fatalf("at-limit create = %d %q, want 303 → /repeaters/{id}/added", at.StatusCode, at.Header.Get("Location")) + } +} + // Black-box coverage for the repeater, sharing, invite, and admin POST endpoints. // All redirect (303) and do not render; each test asserts the redirect target and, // where cheap, a store side-effect. diff --git a/internal/core/repeater_setup.go b/internal/core/repeater_setup.go index 38505ff..e1f68d9 100644 --- a/internal/core/repeater_setup.go +++ b/internal/core/repeater_setup.go @@ -54,11 +54,12 @@ func (s *Handlers) handleSetupCommands(w http.ResponseWriter, r *http.Request) { http.Error(w, "bad request", http.StatusBadRequest) return } - req.Name = strings.TrimSpace(req.Name) - if req.Name == "" { - http.Error(w, "name is required", http.StatusBadRequest) + name, ok := cleanRepeaterName(req.Name) + if !ok { + http.Error(w, "name is required and must be 31 characters or fewer", http.StatusBadRequest) return } + req.Name = name var cmds []string cmds = append(cmds, "set name "+req.Name) @@ -223,10 +224,10 @@ func (s *Handlers) profileSteps(ctx context.Context, orgID int64, name string) ( func (s *Handlers) handleSetupComplete(w http.ResponseWriter, r *http.Request) { uid := s.Auth.CurrentUserID(r.Context()) - name := strings.TrimSpace(r.FormValue("name")) + name, ok := cleanRepeaterName(r.FormValue("name")) pubHex := strings.ToLower(strings.TrimSpace(r.FormValue("public_key"))) - if name == "" { - http.Error(w, "name is required", http.StatusBadRequest) + if !ok { + http.Error(w, "name is required and must be 31 characters or fewer", http.StatusBadRequest) return } if _, err := meshcore.NewIdentityFromHex(pubHex); err != nil { diff --git a/internal/core/repeaters.go b/internal/core/repeaters.go index 9fe84fd..7d3e565 100644 --- a/internal/core/repeaters.go +++ b/internal/core/repeaters.go @@ -161,14 +161,30 @@ func (s *Handlers) pageRepeaterAdded(w http.ResponseWriter, r *http.Request) { }) } +// maxRepeaterNameLen bounds a repeater name to what MeshCore firmware can store +// and advertise. A node's name lives in a 32-byte buffer (`node_name[32]` in +// CommonCLI.h) and `set name` copies into it with StrHelper::strncpy(..., +// sizeof(node_name)), whose loop runs while buf_sz > 1 and then writes the NUL +// terminator — so 31 usable bytes. A longer name is silently truncated on the +// device, so we reject it here rather than record a name the hardware won't +// actually advertise. (Verified against the MeshCore firmware source.) +const maxRepeaterNameLen = 31 + +// cleanRepeaterName trims s and reports whether it is a usable repeater name: +// non-empty and within the firmware's byte limit. +func cleanRepeaterName(s string) (string, bool) { + s = strings.TrimSpace(s) + return s, s != "" && len(s) <= maxRepeaterNameLen +} + // handleAddRepeater registers a new repeater (unconfirmed) for the current user. func (s *Handlers) handleAddRepeater(w http.ResponseWriter, r *http.Request) { uid := s.Auth.CurrentUserID(r.Context()) - name := strings.TrimSpace(r.FormValue("name")) + name, ok := cleanRepeaterName(r.FormValue("name")) pubHex := strings.ToLower(strings.TrimSpace(r.FormValue("public_key"))) - if name == "" { - addErr(w, r, "Repeater name is required.") + if !ok { + addErr(w, r, "Repeater name is required and must be 31 characters or fewer.") return } // Validate the public key is a real 32-byte MeshCore key. @@ -245,9 +261,9 @@ func (s *Handlers) handleEditRepeater(w http.ResponseWriter, r *http.Request) { editErr := func(msg string) { web.RedirectErr(w, r, "/repeaters/"+repeaterParam(r)+"/edit", msg) } - name := strings.TrimSpace(r.FormValue("name")) - if name == "" { - editErr("Repeater name is required.") + name, ok := cleanRepeaterName(r.FormValue("name")) + if !ok { + editErr("Repeater name is required and must be 31 characters or fewer.") return } freq, bw, sf, cr, valid := parseRadioForm(r) diff --git a/internal/web/env.go b/internal/web/env.go index 1a27e9d..eebe9be 100644 --- a/internal/web/env.go +++ b/internal/web/env.go @@ -239,9 +239,28 @@ func (e *Env) CommonMiddleware(r chi.Router) { r.Use(CaptureRemoteAddr) // preserve the true TCP peer before we resolve r.Use(e.resolveClientIP) // trusted-proxy-aware X-Forwarded-For resolution r.Use(e.securityHeaders) // CSP (+ per-request script nonce) and hardening headers + r.Use(limitBody) // cap request bodies before any handler reads them r.Use(middleware.Recoverer) } +// maxRequestBody caps the request body every surface will read. The app has no +// file uploads — the largest bodies are form posts and small JSON (serial-setup +// command lists, markdown docs) — so 1 MiB is generous while stopping a client +// from streaming an arbitrarily large body into memory before per-field limits +// can apply. WebSocket upgrades carry no request body, so this doesn't affect +// the console/confirm sockets. +const maxRequestBody = 1 << 20 // 1 MiB + +// limitBody caps r.Body so an oversized request fails fast — a read past the +// limit errors and the handler surfaces its generic 400/500 — instead of +// buffering unbounded data. +func limitBody(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r.Body = http.MaxBytesReader(w, r.Body, maxRequestBody) + next.ServeHTTP(w, r) + }) +} + // SharedRoutes registers endpoints every surface needs (health, static assets). func (e *Env) SharedRoutes(r chi.Router) { r.Get("/healthz", func(w http.ResponseWriter, _ *http.Request) { diff --git a/internal/web/limitbody_test.go b/internal/web/limitbody_test.go new file mode 100644 index 0000000..7a2a394 --- /dev/null +++ b/internal/web/limitbody_test.go @@ -0,0 +1,36 @@ +package web + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// TestLimitBody: a request body within the cap reads fully, one past the cap +// errors when the handler drains it. Regression for the pre-release audit +// finding that no request-body size limit existed. +func TestLimitBody(t *testing.T) { + t.Parallel() + + // A handler that drains the body and reports whether the read succeeded. + var readErr error + h := limitBody(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _, readErr = io.ReadAll(r.Body) + })) + + post := func(size int) { + req := httptest.NewRequest(http.MethodPost, "/", strings.NewReader(strings.Repeat("x", size))) + h.ServeHTTP(httptest.NewRecorder(), req) + } + + post(maxRequestBody) // exactly at the cap + if readErr != nil { + t.Fatalf("body at the cap should read cleanly, got %v", readErr) + } + post(maxRequestBody + 1) // one byte over + if readErr == nil { + t.Fatal("body over the cap should fail to read, got nil error") + } +}