diff --git a/internal/auth/account.go b/internal/auth/account.go index dcb3ef5..bd99259 100644 --- a/internal/auth/account.go +++ b/internal/auth/account.go @@ -3,6 +3,7 @@ package auth import ( "encoding/hex" "errors" + "fmt" "net/http" "net/mail" "strconv" @@ -87,12 +88,15 @@ func (s *Handlers) renderAccount(w http.ResponseWriter, r *http.Request, uid int "Platforms": store.UserLinkPlatforms(), "PlatformsJS": web.LinkPlatformsJS(store.UserLinkPlatforms()), "HasPassword": u.PasswordHash != nil, - "Passkeys": views, - "NextRename": nextRename, // nil when a rename is allowed now - "Error": r.URL.Query().Get("error"), - "OK": r.URL.Query().Get("ok"), - "PKMsg": r.URL.Query().Get("pk"), - "PKErr": r.URL.Query().Get("pkerr"), + // Stated in the change-password form and used as minlength, so the client + // hint can't drift from the server's floor. + "MinPasswordLen": MinPasswordLen, + "Passkeys": views, + "NextRename": nextRename, // nil when a rename is allowed now + "Error": r.URL.Query().Get("error"), + "OK": r.URL.Query().Get("ok"), + "PKMsg": r.URL.Query().Get("pk"), + "PKErr": r.URL.Query().Get("pkerr"), } for k, v := range overrides { data[k] = v @@ -421,7 +425,8 @@ func (s *Handlers) handleChangePassword(w http.ResponseWriter, r *http.Request) newPassword := r.FormValue("new_password") if !ValidPassword(newPassword) { - accountRedirect(w, r, "error", "New password must be at least 8 characters.") + accountRedirect(w, r, "error", fmt.Sprintf( + "New password must be at least %d characters.", MinPasswordLen)) 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 e5b43dd..53110c1 100644 --- a/internal/auth/handlers.go +++ b/internal/auth/handlers.go @@ -411,7 +411,17 @@ func NormalizePasskeyName(s string) string { // MinPasswordLen is a basic strength floor. There is deliberately no maximum: // passwords are pre-hashed before bcrypt (see hashPassword), so bcrypt's 72-byte // input limit never reaches the user. -const MinPasswordLen = 8 +// +// It is enforced only where a password is *set* (signup and the account page's +// change-password form), never on the verify path — so raising it doesn't lock out +// accounts whose existing password predates the increase. They keep signing in, and +// only have to meet the current floor if they choose a new password. +// +// 12 rather than 8 because length is the only strength signal here (no complexity +// rules, no breach-list check) and there is no account recovery to fall back on if a +// weak password is guessed — see the audit's B1. Passkeys remain the preferred path +// and are offered first on both the sign-in and sign-up forms. +const MinPasswordLen = 12 // ValidPassword reports whether p meets the minimum length. func ValidPassword(p string) bool { diff --git a/internal/auth/password_handlers.go b/internal/auth/password_handlers.go index 51c4f37..1876e4b 100644 --- a/internal/auth/password_handlers.go +++ b/internal/auth/password_handlers.go @@ -2,6 +2,7 @@ package auth import ( "errors" + "fmt" "net/http" "net/url" @@ -14,7 +15,9 @@ func (s *Service) SignupPassword(w http.ResponseWriter, r *http.Request) { displayName := NormalizeDisplayName(r.FormValue("display_name")) password := r.FormValue("password") if !ValidUsername(username) || !ValidPassword(password) { - redirectErr(w, r, "/signup", "Choose a username (3–32 chars: letters, digits, _ . -) and a password of at least 8 characters.") + redirectErr(w, r, "/signup", fmt.Sprintf( + "Choose a username (3–32 chars: letters, digits, _ . -) and a password of at least %d characters.", + MinPasswordLen)) return } diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index 2238ff4..308f022 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -7,21 +7,39 @@ import ( "golang.org/x/crypto/bcrypt" ) -// TestValidPassword pins the 8-char floor and confirms there is no upper bound: -// pre-hashing removes bcrypt's 72-byte input limit, so long passwords are valid. +// TestValidPassword pins the floor at MinPasswordLen and confirms there is no upper +// bound: pre-hashing removes bcrypt's 72-byte input limit, so long passwords are +// valid. +// +// The boundary cases are expressed relative to the constant rather than hardcoded, so +// changing the floor doesn't require editing the expectations — only the explicit +// "old floor" case below, which is deliberate. func TestValidPassword(t *testing.T) { t.Parallel() cases := []struct { n int want bool }{ - {7, false}, {8, true}, {72, true}, {73, true}, {5000, true}, + {MinPasswordLen - 1, false}, + {MinPasswordLen, true}, + {72, true}, // bcrypt's raw input limit — pre-hashing means it isn't ours + {73, true}, // past it + {5000, true}, // no maximum at all } 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) } } + // The floor was raised from 8 to 12 (audit S7); 8 must now be rejected when a + // password is *set*. Existing shorter passwords keep working because the floor is + // never applied on the verify path — see TestFloorNotAppliedToExistingPasswords. + if ValidPassword("12345678") { + t.Error("an 8-character password is still accepted; the floor didn't move") + } + if MinPasswordLen < 12 { + t.Errorf("MinPasswordLen = %d, want at least 12", MinPasswordLen) + } } // TestPasswordHashRoundTrip: a password far past bcrypt's 72-byte input limit diff --git a/internal/auth/templates/account.html b/internal/auth/templates/account.html index 677198e..6d3c9a7 100644 --- a/internal/auth/templates/account.html +++ b/internal/auth/templates/account.html @@ -236,9 +236,9 @@ {{end}}
- +
- + {{template "icon-eye" ""}}
diff --git a/internal/auth/templates/signup.html b/internal/auth/templates/signup.html index 7842cd9..33bf6e4 100644 --- a/internal/auth/templates/signup.html +++ b/internal/auth/templates/signup.html @@ -25,9 +25,9 @@
or use a password
- +
- + {{template "icon-eye" ""}} diff --git a/internal/auth/web.go b/internal/auth/web.go index ffbe5c0..a2e0e32 100644 --- a/internal/auth/web.go +++ b/internal/auth/web.go @@ -149,6 +149,9 @@ func (s *Handlers) pageSignup(w http.ResponseWriter, r *http.Request) { "Layout": "authbase", "Error": r.URL.Query().Get("error"), "Next": r.URL.Query().Get("next"), + // Stated in the form and used as minlength, so the client hint can't drift + // from the server's floor. + "MinPasswordLen": MinPasswordLen, }) } diff --git a/internal/core/endpoints_auth_test.go b/internal/core/endpoints_auth_test.go index ccee99e..93a1e5b 100644 --- a/internal/core/endpoints_auth_test.go +++ b/internal/core/endpoints_auth_test.go @@ -21,7 +21,7 @@ import ( // session cookie — the automatable way to mint an authenticated auth-host session. func authSSO(t *testing.T, ts *httptest.Server, h hostEnv, username string) *http.Cookie { t.Helper() - resp := post(t, ts, h.auth, "/signup/password", url.Values{"username": {username}, "password": {"supersecret"}}) + resp := post(t, ts, h.auth, "/signup/password", url.Values{"username": {username}, "password": {testPassword}}) resp.Body.Close() c := cookieByName(resp, "meshtender_session") if c == nil { @@ -43,7 +43,7 @@ func TestPasswordSignupAndLogin(t *testing.T) { } // #19 signup, valid → creates the account + an SSO session and hands off. - ok := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"pwuser"}, "password": {"supersecret"}}) + ok := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"pwuser"}, "password": {testPassword}}) ok.Body.Close() if ok.StatusCode != http.StatusSeeOther || cookieByName(ok, "meshtender_session") == nil { t.Fatalf("valid signup = %d, session=%v; want 303 + session cookie", ok.StatusCode, cookieByName(ok, "meshtender_session")) @@ -60,7 +60,7 @@ func TestPasswordSignupAndLogin(t *testing.T) { } // #18 login, correct credentials → session + handoff. - good := post(t, ts, h.auth, "/login/password", url.Values{"username": {"pwuser"}, "password": {"supersecret"}}) + good := post(t, ts, h.auth, "/login/password", url.Values{"username": {"pwuser"}, "password": {testPassword}}) good.Body.Close() if good.StatusCode != http.StatusSeeOther || cookieByName(good, "meshtender_session") == nil { t.Fatalf("good login = %d, session=%v; want 303 + session cookie", good.StatusCode, cookieByName(good, "meshtender_session")) @@ -260,7 +260,7 @@ func TestAccountProfilePosts(t *testing.T) { assertAccountOK(t, links, "links") pw := post(t, ts, h.auth, "/account/password", - url.Values{"new_password": {"anothersecret"}, "current_password": {"supersecret"}}, sso) + url.Values{"new_password": {"anothersecret"}, "current_password": {testPassword}}, sso) pw.Body.Close() assertAccountOK(t, pw, "password") diff --git a/internal/core/handoff_test.go b/internal/core/handoff_test.go index 6dc87e4..947e502 100644 --- a/internal/core/handoff_test.go +++ b/internal/core/handoff_test.go @@ -27,6 +27,20 @@ type hostEnv struct{ auth, app, root, www string } // splitServer stands up a server in full split-host mode (root/www/auth/app) // against the test database, returning the store and the per-surface host:port. +// testPassword is the fixture password for sign-up/sign-in tests. It must satisfy +// auth.MinPasswordLen — asserted below — so raising the floor doesn't quietly break +// every credential test with a confusing "password too short" redirect. +const testPassword = "correct-horse-battery-staple" + +// TestFixturePasswordMeetsFloor keeps the fixture honest if the floor moves again. +func TestFixturePasswordMeetsFloor(t *testing.T) { + t.Parallel() + if len(testPassword) < auth.MinPasswordLen { + t.Fatalf("testPassword is %d chars, below the %d-char floor — credential tests "+ + "would fail with a misleading validation error", len(testPassword), auth.MinPasswordLen) + } +} + func splitServer(t *testing.T) (*store.Store, context.Context, *httptest.Server, hostEnv) { t.Helper() st, ctx := coreStore(t) @@ -251,7 +265,7 @@ func TestSingleLogout(t *testing.T) { // Sign up on the auth host: creates login row L and the auth SSO session, then // hands off to the app with a single-use code carrying L. - su := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"sharuser"}, "password": {"supersecret"}}) + su := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"sharuser"}, "password": {testPassword}}) su.Body.Close() sso := cookieByName(su, "meshtender_session") if sso == nil { @@ -301,7 +315,7 @@ func TestAuthLogoutClearsSSO(t *testing.T) { t.Parallel() _, _, ts, h := splitServer(t) - su := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"ssoonly"}, "password": {"supersecret"}}) + su := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"ssoonly"}, "password": {testPassword}}) su.Body.Close() sso := cookieByName(su, "meshtender_session") if sso == nil { @@ -329,7 +343,7 @@ func TestLogoutRejectsGet(t *testing.T) { t.Parallel() _, _, ts, h := splitServer(t) - su := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"getlogout"}, "password": {"supersecret"}}) + su := post(t, ts, h.auth, "/signup/password", url.Values{"username": {"getlogout"}, "password": {testPassword}}) su.Body.Close() sso := cookieByName(su, "meshtender_session") if sso == nil { @@ -382,7 +396,7 @@ func TestAccountOnAuthHost(t *testing.T) { t.Fatal("expected a session cookie carrying the auth-local flag") } fin := post(t, ts, h.auth, "/signup/password", - url.Values{"username": {"acctlocal"}, "password": {"supersecret"}}, sess) + url.Values{"username": {"acctlocal"}, "password": {testPassword}}, sess) fin.Body.Close() if got := fin.Header.Get("Location"); got != "/account" { t.Fatalf("post-login redirect = %q, want local /account (no handoff)", got) @@ -391,7 +405,7 @@ func TestAccountOnAuthHost(t *testing.T) { t.Run("auth /account with SSO session renders", func(t *testing.T) { su := post(t, ts, h.auth, "/signup/password", - url.Values{"username": {"acctsso"}, "password": {"supersecret"}}) + url.Values{"username": {"acctsso"}, "password": {testPassword}}) su.Body.Close() sso := cookieByName(su, "meshtender_session") if sso == nil { diff --git a/internal/core/password_floor_test.go b/internal/core/password_floor_test.go new file mode 100644 index 0000000..a343e78 --- /dev/null +++ b/internal/core/password_floor_test.go @@ -0,0 +1,164 @@ +package core + +import ( + "io" + "net/http" + "net/http/cookiejar" + "net/url" + "strconv" + "strings" + "testing" + + "github.com/jleight/meshtender/internal/auth" +) + +// TestPasswordFloorAppliesOnlyWhenSetting is the guarantee behind raising the +// minimum from 8 to 12 (audit S7): the floor is checked where a password is *set*, +// never where one is verified. An account whose password predates the increase keeps +// signing in; it only has to meet the current floor if the owner chooses a new +// password. +// +// This is the test that would catch someone "helpfully" adding ValidPassword to the +// login path, which would lock out every account with an older, shorter password. +func TestPasswordFloorAppliesOnlyWhenSetting(t *testing.T) { + t.Parallel() + st, ctx, ts, h := splitServer(t) + + // A password that was fine under the old floor and is too short under the new one. + const legacyPassword = "old8char" // 8 characters + if len(legacyPassword) >= auth.MinPasswordLen { + t.Fatalf("fixture password must be shorter than the floor (%d)", auth.MinPasswordLen) + } + + // Establish it the way an existing account's hash exists today: through the auth + // service's own setter, bypassing the form-level length check. + svc, err := auth.New(st, st.Pool(), auth.Config{ + RPID: "localhost", RPDisplayName: "test", + RPOrigins: []string{"http://auth.localhost"}, + AppHost: testAppHost, AuthHost: testAuthHost, RootHost: testRootHost, + }) + if err != nil { + t.Fatalf("auth service: %v", err) + } + legacyUser, err := st.CreateUser(ctx, "legacypw", "") + if err != nil { + t.Fatalf("create user: %v", err) + } + if err := svc.SetPassword(ctx, legacyUser.ID, legacyPassword); err != nil { + t.Fatalf("set legacy password: %v", err) + } + + t.Run("existing short password still signs in", func(t *testing.T) { + resp := post(t, ts, h.auth, "/login/password", url.Values{ + "username": {"legacypw"}, + "password": {legacyPassword}, + }) + defer resp.Body.Close() + if resp.StatusCode != http.StatusSeeOther { + t.Fatalf("login = %d, want 303", resp.StatusCode) + } + loc := resp.Header.Get("Location") + if strings.Contains(loc, "error=") { + t.Fatalf("login failed for a pre-existing short password: %s", loc) + } + // A successful sign-in hands off with a code rather than bouncing back to /login. + if !strings.Contains(loc, "/session/callback") { + t.Fatalf("login didn't hand off; Location = %q", loc) + } + }) + + t.Run("verification is unchanged for a wrong password", func(t *testing.T) { + resp := post(t, ts, h.auth, "/login/password", url.Values{ + "username": {"legacypw"}, + "password": {"wrongwrongwrong"}, + }) + defer resp.Body.Close() + if loc := resp.Header.Get("Location"); !strings.Contains(loc, "error=") { + t.Fatalf("wrong password was accepted; Location = %q", loc) + } + }) + + t.Run("signup rejects a password under the floor", func(t *testing.T) { + resp := post(t, ts, h.auth, "/signup/password", url.Values{ + "username": {"tooshortpw"}, + "password": {strings.Repeat("a", auth.MinPasswordLen-1)}, + }) + defer resp.Body.Close() + loc := resp.Header.Get("Location") + if !strings.Contains(loc, "/signup?error=") { + t.Fatalf("short signup password was accepted; Location = %q", loc) + } + if !strings.Contains(loc, strconv.Itoa(auth.MinPasswordLen)) { + t.Errorf("the error should tell the user the required length (%d); Location = %q", + auth.MinPasswordLen, loc) + } + // And no account may have been created. + if _, err := st.GetUserByUsername(ctx, "tooshortpw"); err == nil { + t.Error("a user was created despite the rejected password") + } + }) + + t.Run("signup accepts a password at the floor", func(t *testing.T) { + resp := post(t, ts, h.auth, "/signup/password", url.Values{ + "username": {"longenoughpw"}, + "password": {strings.Repeat("a", auth.MinPasswordLen)}, + }) + defer resp.Body.Close() + if loc := resp.Header.Get("Location"); strings.Contains(loc, "error=") { + t.Fatalf("a password at exactly the floor was rejected: %s", loc) + } + }) +} + +// TestPasswordFormsStateTheFloor checks the signup and account forms advertise the +// same minimum the server enforces — both in the human-readable hint and in the +// client-side minlength, which is what stops the browser submitting a doomed form. +func TestPasswordFormsStateTheFloor(t *testing.T) { + t.Parallel() + st, ctx, ts, h := splitServer(t) + + want := strconv.Itoa(auth.MinPasswordLen) + + t.Run("signup form", func(t *testing.T) { + resp := do(t, ts, h.auth, "/signup") + body, err := io.ReadAll(resp.Body) + resp.Body.Close() + if err != nil { + t.Fatalf("read body: %v", err) + } + page := string(body) + if !strings.Contains(page, "min "+want+" characters") { + t.Errorf("signup form doesn't state a %s-character minimum", want) + } + if !strings.Contains(page, `minlength="`+want+`"`) { + t.Errorf("signup password field lacks minlength=%q", want) + } + }) + + t.Run("account change-password form", func(t *testing.T) { + jar, err := cookiejar.New(nil) + if err != nil { + t.Fatalf("cookiejar: %v", err) + } + seedSession(t, ts, st, ctx, jar, "pwformuser") + // The account page lives on the auth host and needs that host's own session, + // so drive it through the same handoff the app uses. + resp := do(t, ts, h.auth, "/account", jar.Cookies(mustURL(t, ts.URL))...) + body, err := io.ReadAll(resp.Body) + resp.Body.Close() + if err != nil { + t.Fatalf("read body: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Fatalf("account page = %d, want 200 — the handoff cookie should carry the "+ + "auth-host session", resp.StatusCode) + } + page := string(body) + if !strings.Contains(page, "min "+want+" characters") { + t.Errorf("account form doesn't state a %s-character minimum", want) + } + if !strings.Contains(page, `minlength="`+want+`"`) { + t.Errorf("account password field lacks minlength=%q", want) + } + }) +}