From 2ee28203ab89f41bf3122d678c7384e47f586a21 Mon Sep 17 00:00:00 2001 From: Florian Sylvain Date: Thu, 24 Sep 2026 15:39:50 +0200 Subject: [PATCH] fix: API responses never include a user's password, verification code or reset code hashes, or their expiry dates; verifying an account returns only the public user fields --- api/handlers/auth/authHandler.go | 26 +- features/user_response_field_hygiene.feature | 61 +++++ features/user_response_field_hygiene.qa.md | 60 +++++ test/user_response_field_hygiene_test.go | 243 +++++++++++++++++++ 4 files changed, 389 insertions(+), 1 deletion(-) create mode 100644 features/user_response_field_hygiene.feature create mode 100644 features/user_response_field_hygiene.qa.md create mode 100644 test/user_response_field_hygiene_test.go diff --git a/api/handlers/auth/authHandler.go b/api/handlers/auth/authHandler.go index 8a485bc..8c797f4 100644 --- a/api/handlers/auth/authHandler.go +++ b/api/handlers/auth/authHandler.go @@ -52,6 +52,26 @@ type ResetPasswordCredentials struct { Password string `json:"password" validate:"required,min=8"` } +type PublicUser struct { + ID uint32 `json:"id"` + Username string `json:"username"` + Email string `json:"email"` + IsVerified bool `json:"is_verified"` + CreatedAt time.Time `json:"created_at"` + UpdatedAt time.Time `json:"updated_at"` +} + +func newPublicUser(u user.User) PublicUser { + return PublicUser{ + ID: u.ID, + Username: u.Username, + Email: u.Email, + IsVerified: u.IsVerified, + CreatedAt: u.CreatedAt, + UpdatedAt: u.UpdatedAt, + } +} + func (h *Handler) removeJwtCookie(w http.ResponseWriter) { http.SetCookie(w, &http.Cookie{ Name: "jwt", @@ -205,7 +225,11 @@ func (h *Handler) Verify(w http.ResponseWriter, r *http.Request) { return } - body, _ := json.Marshal(verifiedUser) + body, err := json.Marshal(newPublicUser(verifiedUser)) + if err != nil { + http.Error(w, "Internal server error", http.StatusInternalServerError) + return + } _, _ = w.Write(body) } diff --git a/features/user_response_field_hygiene.feature b/features/user_response_field_hygiene.feature new file mode 100644 index 0000000..a09283e --- /dev/null +++ b/features/user_response_field_hygiene.feature @@ -0,0 +1,61 @@ +Feature: API responses never expose a user's secret fields + + As the maintainer of RenewCMS + I want every HTTP response that includes user data to carry only public fields, never a + password hash, a verification or password-reset code hash, or their expiry timestamps + So that a bug, a future endpoint, or a client inspecting a response body can never recover a + credential or a still-valid reset/verification secret + + Background: + Given the public shape of a user in any API response is exactly six fields: id, username, + email, is_verified, created_at and updated_at + And password, password_reset_code, verification_code, verification_expiration and + password_reset_expiration never appear in any response body, under any key name, regardless + of their value or whether they are set + + Scenario: A successful verification response carries only public fields + Given a user repository containing user id 7, username "alice", email "alice@example.com", + is_verified false, whose stored VerificationCode is the bcrypt hash of "abc123" and whose + VerificationExpiration is one hour from now + And a valid jwt cookie for user id 7 + When the client sends POST /v1/auth/verify with body {"code": "abc123"} + Then the response status is 200 + And the response body is a JSON object with exactly the keys id, username, email, + is_verified, created_at and updated_at + And the response body does not contain the keys password, password_reset_code, + verification_code, verification_expiration or password_reset_expiration + And the response body's is_verified is true + + Scenario: A rejected verification attempt never echoes any user field + Given a user repository containing user id 7, username "alice", is_verified false, whose + stored VerificationCode is the bcrypt hash of "abc123" and whose VerificationExpiration is + one hour from now + And a valid jwt cookie for user id 7 + When the client sends POST /v1/auth/verify with body {"code": "wrong-code"} + Then the response status is 403 + And the response body is the plain-text apperr.VerificationFailedErrorMessage, not JSON, and + contains no user field at all + + Scenario: Registering while no verified account exists never echoes any user field + Given the user repository is empty + When the client sends POST /v1/auth/register with username "bob", password "hunter2example12", + email "bob@example.com" + Then the response status is 200 + And the response body contains no key named password, password_reset_code, + verification_code, verification_expiration or password_reset_expiration + + Scenario Outline: No other auth endpoint under /v1/auth leaks a user field + Given a user repository containing user id 7, username "alice", email "alice@example.com", + is_verified true, with a non-empty PasswordResetCode or VerificationCode where the endpoint + under test would set one + When the client sends with + Then the response status is + And the response body contains no key named password, password_reset_code, + verification_code, verification_expiration or password_reset_expiration + + Examples: + | method | path | body | status | + | POST | /v1/auth/login | {"username":"alice","password":"hunter2example12"} | 200 | + | POST | /v1/auth/logout | {} | 200 | + | POST | /v1/auth/password-reset | {"email":"alice@example.com"} | 200 | + | POST | /v1/auth/password-reset/confirm | {"email":"alice@example.com","code":"wrong","password":"hunter2example12"} | 403 | diff --git a/features/user_response_field_hygiene.qa.md b/features/user_response_field_hygiene.qa.md new file mode 100644 index 0000000..037acf1 --- /dev/null +++ b/features/user_response_field_hygiene.qa.md @@ -0,0 +1,60 @@ +# QA procedure: user_response_field_hygiene + +Package: `test` (black-box), extending `test/account_verification_http_test.go`'s +`newAccountVerificationRouter` / `newAccountVerificationTestUserRepo` helpers and +`test/user_test.go`'s `mockUserRepo` / `stubHasher`. + +## Setup shared by every case + +1. Call `validator.InitValidator()` once. +2. Reuse `newAccountVerificationRouter(userRepo, mailRepo, token)` from + `test/account_verification_http_test.go`, which wires `auth.NewHandler(...)` with fakes over + a fresh `chi.NewRouter()` mirroring `api/router.go`'s `/v1/auth` group (public routes plus the + `jwtauth.Verifier` + `Authenticator`-wrapped `verify` route). No live server, no real SQLite, + no real SMTP. +3. `newAccountVerificationTestUserRepo()` seeds user id 7 with a non-empty `VerificationCode` + (`"hashed:abc123"`) and `VerificationExpiration` one hour out, matching the existing fixture. + +## Response-shape check for POST /v1/auth/verify (the fix) + +4. Successful verify: `POST /v1/auth/verify` with `{"code":"abc123"}` and a valid jwt cookie for + user id 7. Decode the response body into `map[string]json.RawMessage` (not into `user.User`) + and assert: + - the key set is exactly `{"id", "username", "email", "is_verified", "created_at", + "updated_at"}`; + - none of `password`, `password_reset_code`, `verification_code`, + `verification_expiration`, `password_reset_expiration` is present; + - `is_verified` unmarshals to `true`. +5. Rejected verify: same seed, `{"code":"wrong-code"}` → assert status `403` and that the raw + response body bytes, treated as a string, contain none of the substrings `password`, + `verification_code`, `password_reset_code`, `verification_expiration`, + `password_reset_expiration` (the body is plain text from `http.Error`, so this is a substring + check, not a JSON-key check). + +## Registration check (its own case, correctly gated) + +6. `POST /v1/auth/register` against an **empty** `mockUserRepo` (the registration gate only + allows register while no verified account exists) with + `{"username":"bob","password":"hunter2example12","email":"bob@example.com"}` → assert status + `200` and that the raw response body string contains none of the six sensitive substrings + from step 5. + +## Table-driven sweep of the other auth endpoints (regression guard, not the fix) + +7. For each of `POST /v1/auth/login`, `/logout`, `/password-reset`, `/password-reset/confirm`, + seed a `mockUserRepo`/`mockMailRepo` pair with a verified user "alice" (`IsVerified: true`, + matching the Scenario Outline's Given, since these endpoints don't depend on the registration + gate), send the request, and assert the raw response body string contains none of the six + sensitive substrings from step 5. This should already pass today since these handlers only + ever write a `{"message": ...}` body; the point of the test is to pin that fact down so a + future change can't quietly regress it. + +## Manual verification (not exercised by the Go test) + +- The SPA's post-verification screen (`web/src/`) only ever needs `username`/`is_verified` to + render; confirm by hand that no SPA code reads `password`, `verification_code` or + `password_reset_code` off the verify response, since removing them from the wire format could + otherwise silently break a screen that isn't covered by this Go test suite. +- Confirm the new response type lives in `api/handlers/auth` (not a change to + `internal/domain/user/userDomain.go`'s json tags), per the project's stated preference to keep + the domain struct's tags intact and shape the public response in the handler layer instead. diff --git a/test/user_response_field_hygiene_test.go b/test/user_response_field_hygiene_test.go new file mode 100644 index 0000000..25b8989 --- /dev/null +++ b/test/user_response_field_hygiene_test.go @@ -0,0 +1,243 @@ +package test + +import ( + "RenewCMS/api/handlers/auth" + "RenewCMS/api/pkg/apperr" + validator "RenewCMS/api/pkg/validator" + "RenewCMS/internal/domain/user" + "RenewCMS/internal/infrastructure/useCases" + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/go-chi/chi/v5" + "github.com/go-chi/jwtauth/v5" +) + +func newUserFieldHygieneRouter(userRepo *mockUserRepo, mailRepo *mockMailRepo, token *jwtauth.JWTAuth) *chi.Mux { + hasher := &stubHasher{} + authHandler := auth.NewHandler( + token, + useCases.NewGetUserUseCase(userRepo), + useCases.NewListUsersUseCase(userRepo), + useCases.NewCreateUserUseCase(userRepo, hasher, mailRepo, "https://cms.example.com"), + useCases.NewAuthenticateUserUseCase(userRepo, hasher), + useCases.NewVerifyUserUseCase(userRepo, hasher), + useCases.NewCancelRegistrationUseCase(userRepo), + useCases.NewRequestPasswordResetUseCase(userRepo, hasher, mailRepo, "https://cms.example.com"), + useCases.NewResetPasswordUseCase(userRepo, hasher), + true, + ) + + r := chi.NewRouter() + r.Route("/v1", func(r chi.Router) { + r.Route("/auth", func(r chi.Router) { + r.Post("/register", authHandler.Register) + r.Post("/login", authHandler.Login) + r.Post("/logout", authHandler.Logout) + r.Post("/password-reset", authHandler.RequestPasswordReset) + r.Post("/password-reset/confirm", authHandler.ResetPassword) + r.Group(func(r chi.Router) { + r.Use(jwtauth.Verifier(token)) + r.Use(jwtauth.Authenticator(token)) + r.Post("/verify", authHandler.Verify) + }) + }) + }) + return r +} + +func newFieldHygieneVerifiedAliceRepo() *mockUserRepo { + repo := newMockUserRepo() + repo.users[7] = user.User{ + ID: 7, + Username: "alice", + Email: "alice@example.com", + Password: "hashed:hunter2example12", + IsVerified: true, + PasswordResetCode: "hashed:some-reset-code", + PasswordResetExpiration: time.Now().Add(time.Hour), + VerificationCode: "hashed:some-verification-code", + VerificationExpiration: time.Now().Add(time.Hour), + } + repo.nextID = 7 + return repo +} + +var forbiddenUserFieldKeys = []string{ + "password", + "password_reset_code", + "verification_code", + "verification_expiration", + "password_reset_expiration", +} + +func assertNoForbiddenKeys(t *testing.T, body []byte) { + t.Helper() + var decoded map[string]json.RawMessage + if err := json.Unmarshal(body, &decoded); err != nil { + return + } + for _, key := range forbiddenUserFieldKeys { + if _, present := decoded[key]; present { + t.Errorf("expected no key %q in response body, got %s", key, string(body)) + } + } +} + +func TestUserResponseFieldHygiene(t *testing.T) { + validator.InitValidator() + token := jwtauth.New("HS256", []byte("test-secret"), nil) + + t.Run("A successful verification response carries only public fields", func(t *testing.T) { + repo := newMockUserRepo() + repo.users[7] = user.User{ + ID: 7, + Username: "alice", + Email: "alice@example.com", + IsVerified: false, + VerificationCode: "hashed:abc123", + VerificationExpiration: time.Now().Add(time.Hour), + } + repo.nextID = 7 + router := newUserFieldHygieneRouter(repo, newMockMailRepo(), token) + + body, _ := json.Marshal(map[string]string{"code": "abc123"}) + req := httptest.NewRequest(http.MethodPost, "/v1/auth/verify", bytes.NewReader(body)) + _, tokenString, _ := token.Encode(map[string]any{"user_id": uint32(7)}) + req.AddCookie(&http.Cookie{Name: "jwt", Value: tokenString}) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected status 200, got %d: %s", rec.Code, rec.Body.String()) + } + + var decoded map[string]json.RawMessage + if err := json.Unmarshal(rec.Body.Bytes(), &decoded); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + + expectedKeys := []string{"id", "username", "email", "is_verified", "created_at", "updated_at"} + if len(decoded) != len(expectedKeys) { + t.Errorf("expected exactly %d keys, got %d: %v", len(expectedKeys), len(decoded), decoded) + } + for _, key := range expectedKeys { + if _, present := decoded[key]; !present { + t.Errorf("expected key %q in response body", key) + } + } + assertNoForbiddenKeys(t, rec.Body.Bytes()) + + var isVerified bool + if err := json.Unmarshal(decoded["is_verified"], &isVerified); err != nil { + t.Fatalf("failed to decode is_verified: %v", err) + } + if !isVerified { + t.Errorf("expected is_verified true") + } + }) + + t.Run("A rejected verification attempt never echoes any user field", func(t *testing.T) { + repo := newMockUserRepo() + repo.users[7] = user.User{ + ID: 7, + Username: "alice", + IsVerified: false, + VerificationCode: "hashed:abc123", + VerificationExpiration: time.Now().Add(time.Hour), + } + repo.nextID = 7 + router := newUserFieldHygieneRouter(repo, newMockMailRepo(), token) + + body, _ := json.Marshal(map[string]string{"code": "wrong-code"}) + req := httptest.NewRequest(http.MethodPost, "/v1/auth/verify", bytes.NewReader(body)) + _, tokenString, _ := token.Encode(map[string]any{"user_id": uint32(7)}) + req.AddCookie(&http.Cookie{Name: "jwt", Value: tokenString}) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("expected status 403, got %d: %s", rec.Code, rec.Body.String()) + } + if rec.Body.String() != apperr.VerificationFailedErrorMessage+"\n" { + t.Errorf("expected plain-text apperr.VerificationFailedErrorMessage body, got %q", rec.Body.String()) + } + var decoded map[string]json.RawMessage + if err := json.Unmarshal(rec.Body.Bytes(), &decoded); err == nil { + t.Errorf("expected non-JSON body, but it decoded as JSON: %v", decoded) + } + }) + + t.Run("Registering while no verified account exists never echoes any user field", func(t *testing.T) { + repo := newMockUserRepo() + router := newUserFieldHygieneRouter(repo, newMockMailRepo(), token) + + body := `{"username":"bob","password":"hunter2example12","email":"bob@example.com"}` + req := httptest.NewRequest(http.MethodPost, "/v1/auth/register", bytes.NewBufferString(body)) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("expected status 200, got %d: %s", rec.Code, rec.Body.String()) + } + assertNoForbiddenKeys(t, rec.Body.Bytes()) + }) + + outlineCases := []struct { + name string + method string + path string + body string + status int + }{ + { + name: "login", + method: http.MethodPost, + path: "/v1/auth/login", + body: `{"username":"alice","password":"hunter2example12"}`, + status: http.StatusOK, + }, + { + name: "logout", + method: http.MethodPost, + path: "/v1/auth/logout", + body: `{}`, + status: http.StatusOK, + }, + { + name: "password-reset", + method: http.MethodPost, + path: "/v1/auth/password-reset", + body: `{"email":"alice@example.com"}`, + status: http.StatusOK, + }, + { + name: "password-reset/confirm", + method: http.MethodPost, + path: "/v1/auth/password-reset/confirm", + body: `{"email":"alice@example.com","code":"wrong","password":"hunter2example12"}`, + status: http.StatusForbidden, + }, + } + + for _, tc := range outlineCases { + t.Run("No other auth endpoint under /v1/auth leaks a user field: "+tc.name, func(t *testing.T) { + repo := newFieldHygieneVerifiedAliceRepo() + router := newUserFieldHygieneRouter(repo, newMockMailRepo(), token) + + req := httptest.NewRequest(tc.method, tc.path, bytes.NewBufferString(tc.body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != tc.status { + t.Fatalf("expected status %d, got %d: %s", tc.status, rec.Code, rec.Body.String()) + } + assertNoForbiddenKeys(t, rec.Body.Bytes()) + }) + } +}