diff --git a/internal/web/config.go b/internal/web/config.go index 5e9b5bd..5d25e01 100644 --- a/internal/web/config.go +++ b/internal/web/config.go @@ -76,6 +76,7 @@ func (s *Server) handleUpdateConfig(c *gin.Context) { return } + // Best-effort multi-key write (no multi-Set transaction on ConfigRepo). for i := range reqBody.KeyValuePairs { kv := reqBody.KeyValuePairs[i] if !isEditableConfigKey(kv.Key) { diff --git a/internal/web/pagemodel.go b/internal/web/pagemodel.go index dc163d1..6bb1a68 100644 --- a/internal/web/pagemodel.go +++ b/internal/web/pagemodel.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/renorris/openfsd/internal/auth" + "github.com/renorris/openfsd/internal/db" "github.com/renorris/openfsd/pkg/protocol" ) @@ -165,11 +166,18 @@ func networkRatingLabel(val int) string { } } -// allRatingOptions returns every network rating for select elements. -func allRatingOptions(selected int) []ratingOption { - vals := []int{-1, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12} - out := make([]ratingOption, 0, len(vals)) - for _, v := range vals { +// ratingOptionsUpTo returns network rating select options from Inactive (−1) +// through maxInclusive (clamped to Administrator). Actors only see ratings +// they are allowed to assign (server still enforces the ceiling). +func ratingOptionsUpTo(maxInclusive int, selected int) []ratingOption { + if maxInclusive > int(protocol.NetworkRatingAdministator) { + maxInclusive = int(protocol.NetworkRatingAdministator) + } + if maxInclusive < int(protocol.NetworkRatingInactive) { + maxInclusive = int(protocol.NetworkRatingInactive) + } + out := make([]ratingOption, 0, maxInclusive-int(protocol.NetworkRatingInactive)+1) + for v := int(protocol.NetworkRatingInactive); v <= maxInclusive; v++ { out = append(out, ratingOption{ Value: v, Label: networkRatingLabel(v), @@ -180,6 +188,7 @@ func allRatingOptions(selected int) []ratingOption { } // editableConfigKeys is the allowlist of config keys shown/mutated via the form UI. +// Keys use db.Config* constants so the form allowlist cannot drift from the repository. var editableConfigKeys = []struct { Key string Label string @@ -187,31 +196,31 @@ var editableConfigKeys = []struct { Placeholder string }{ { - Key: "WELCOME_MESSAGE", + Key: db.ConfigWelcomeMessage, Label: "Welcome Message", Description: "Welcome message sent to FSD clients after they connect", Placeholder: "Welcome to my FSD server!", }, { - Key: "FSD_SERVER_HOSTNAME", + Key: db.ConfigFsdServerHostname, Label: "FSD Server Hostname", Description: "Server hostname advertised to clients", Placeholder: "myfsdserver.com", }, { - Key: "FSD_SERVER_IDENT", + Key: db.ConfigFsdServerIdent, Label: "FSD Server Ident", Description: "Server ident advertised to clients", Placeholder: "MY-FSD-SERVER", }, { - Key: "FSD_SERVER_LOCATION", + Key: db.ConfigFsdServerLocation, Label: "FSD Server Location", Description: "Geographical server location advertised to clients", Placeholder: "East US", }, { - Key: "API_SERVER_BASE_URL", + Key: db.ConfigApiServerBaseURL, Label: "API Server Base URL", Description: "API server base URL advertised to clients", Placeholder: "https://example.com", diff --git a/internal/web/pages_config.go b/internal/web/pages_config.go index 4c0a632..5b51069 100644 --- a/internal/web/pages_config.go +++ b/internal/web/pages_config.go @@ -47,12 +47,6 @@ func (s *Server) handleFrontendConfigEditor(c *gin.Context) { switch c.Query("flash") { case "saved": page.FlashSuccess = "Configuration saved" - case "secret_reset": - page.FlashSuccess = "JWT secret key reset. All sessions and API tokens are invalidated." - case "token_created": - // Token value is not put in the query string (too long / sensitive in logs). - // The create-token POST re-renders with CreatedToken instead of redirecting. - page.FlashSuccess = "API token created" } s.writeTemplate(c, "configeditor", page) } @@ -79,6 +73,9 @@ func (s *Server) handleFrontendConfigUpdate(c *gin.Context) { page.Fields[i].Value = c.PostForm("cfg_" + key) } + // Best-effort multi-key save: ConfigRepo has no multi-Set transaction, so a + // mid-loop failure can leave earlier keys already written. Keys are allowlisted + // and low-risk; operators can re-submit the form to retry. for i := range page.Fields { f := page.Fields[i] if !isEditableConfigKey(f.Key) { diff --git a/internal/web/pages_user.go b/internal/web/pages_user.go index 8da77ae..887f81c 100644 --- a/internal/web/pages_user.go +++ b/internal/web/pages_user.go @@ -14,18 +14,31 @@ import ( func (s *Server) newUserEditorPage(c *gin.Context) userEditorPage { claims := getJwtContext(c) + maxRating := int(claims.NetworkRating) + defaultRating := int(protocol.NetworkRatingObserver) + if defaultRating > maxRating { + defaultRating = maxRating + } return userEditorPage{ basePage: basePage{ User: pageUserFromClaims(claims), CSRFToken: s.issueCSRFToken(c), }, Create: userForm{ - NetworkRating: int(protocol.NetworkRatingObserver), + NetworkRating: defaultRating, }, - RatingOptions: allRatingOptions(int(protocol.NetworkRatingObserver)), + // Only offer ratings the actor may assign (server still enforces ceiling). + RatingOptions: ratingOptionsUpTo(maxRating, defaultRating), } } +func actorMaxRating(page *userEditorPage) int { + if page.User == nil { + return int(protocol.NetworkRatingObserver) + } + return page.User.NetworkRating +} + // handleFrontendUserEditor renders the supervisor user editor. // GET /usereditor?cid=N loads that user into the edit form (server-rendered). func (s *Server) handleFrontendUserEditor(c *gin.Context) { @@ -68,7 +81,9 @@ func (s *Server) loadUserIntoEditForm(page *userEditorPage, cidStr string) { LastName: safeStr(user.LastName), NetworkRating: user.NetworkRating, } - page.RatingOptions = allRatingOptions(user.NetworkRating) + // Keep create/edit selects capped at actor max; selected value is the loaded rating + // (may appear only via value compare in template if above max — rare for higher targets). + page.RatingOptions = ratingOptionsUpTo(actorMaxRating(page), user.NetworkRating) } // handleFrontendUserCreate processes POST /usereditor/create (no-JS form path). @@ -90,15 +105,17 @@ func (s *Server) handleFrontendUserCreate(c *gin.Context) { page.Create.LastName = lastName page.Create.Password = "" // never re-render password + maxRating := int(claims.NetworkRating) rating, err := strconv.Atoi(ratingStr) if err != nil { page.Create.RatingError = "Select a network rating" page.Create.NetworkRating = int(protocol.NetworkRatingObserver) + page.RatingOptions = ratingOptionsUpTo(maxRating, page.Create.NetworkRating) s.writeTemplate(c, "usereditor", page) return } page.Create.NetworkRating = rating - page.RatingOptions = allRatingOptions(rating) + page.RatingOptions = ratingOptionsUpTo(maxRating, rating) if len(password) < 8 { page.Create.PasswordError = "Password must be at least 8 characters" @@ -115,7 +132,7 @@ func (s *Server) handleFrontendUserCreate(c *gin.Context) { s.writeTemplate(c, "usereditor", page) return } - if claims.NetworkRating < protocol.NetworkRatingSupervisor || rating > int(claims.NetworkRating) { + if claims.NetworkRating < protocol.NetworkRatingSupervisor || rating > maxRating { page.Create.Error = "You cannot create a user with that rating" s.writeTemplate(c, "usereditor", page) return @@ -175,16 +192,17 @@ func (s *Server) handleFrontendUserUpdate(c *gin.Context) { return } + maxRating := int(claims.NetworkRating) rating, err := strconv.Atoi(ratingStr) if err != nil || rating < -1 || rating > 12 { page.Edit.RatingError = "Invalid network rating" page.Edit.NetworkRating = int(protocol.NetworkRatingObserver) - page.RatingOptions = allRatingOptions(page.Edit.NetworkRating) + page.RatingOptions = ratingOptionsUpTo(maxRating, page.Edit.NetworkRating) s.writeTemplate(c, "usereditor", page) return } page.Edit.NetworkRating = rating - page.RatingOptions = allRatingOptions(rating) + page.RatingOptions = ratingOptionsUpTo(maxRating, rating) if password != "" { if len(password) < 8 { diff --git a/internal/web/pe_admin_test.go b/internal/web/pe_admin_test.go index f9f2daa..b841a61 100644 --- a/internal/web/pe_admin_test.go +++ b/internal/web/pe_admin_test.go @@ -407,3 +407,180 @@ func TestConfigUpdateViaAPIStillRequiresAuth(t *testing.T) { t.Fatalf("status %d want 401, body %s", w.Code, body) } } + +// TestAPIUpdateUserCannotElevateRatingAboveActor is the Issue 1 regression: +// supervisor cannot PATCH an observer to Administrator via the JSON API. +func TestAPIUpdateUserCannotElevateRatingAboveActor(t *testing.T) { + ts := newTestServer(t) + sup := createTestUser(t, ts, "sup-pass", int(protocol.NetworkRatingSupervisor)) + target := createTestUser(t, ts, "obs-pass", int(protocol.NetworkRatingObserver)) + + // Obtain Bearer access token + loginBody := `{"cid":` + itoa(sup.CID) + `,"password":"sup-pass"}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/login", strings.NewReader(loginBody)) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + ts.engine.ServeHTTP(w, req) + if w.Code != http.StatusOK { + t.Fatalf("auth login %d %s", w.Code, w.Body.String()) + } + respBody := w.Body.String() + marker := `"access_token":"` + i := strings.Index(respBody, marker) + if i < 0 { + t.Fatalf("no access token in %s", respBody) + } + rest := respBody[i+len(marker):] + j := strings.Index(rest, `"`) + token := rest[:j] + + // Attempt privilege escalation: set rating to Administrator (12) + payload := `{"cid":` + itoa(target.CID) + `,"network_rating":12}` + req = httptest.NewRequest(http.MethodPatch, "/api/v1/user/update", strings.NewReader(payload)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Authorization", "Bearer "+token) + w = httptest.NewRecorder() + ts.engine.ServeHTTP(w, req) + if w.Code != http.StatusForbidden { + t.Fatalf("elevate via API status %d want 403, body %s", w.Code, w.Body.String()) + } + + // DB must be unchanged + u, err := ts.dbRepo.UserRepo.GetUserByCID(target.CID) + if err != nil { + t.Fatal(err) + } + if u.NetworkRating != int(protocol.NetworkRatingObserver) { + t.Fatalf("target rating = %d, want still Observer", u.NetworkRating) + } +} + +// Cookie+CSRF path of the same elevation hole. +func TestAPIUpdateUserCannotElevateViaCookieSession(t *testing.T) { + ts := newTestServer(t) + sup := createTestUser(t, ts, "sup-pass", int(protocol.NetworkRatingSupervisor)) + target := createTestUser(t, ts, "obs-pass", int(protocol.NetworkRatingObserver)) + cookies := formLogin(t, ts, sup.CID, "sup-pass") + + // Ensure CSRF cookie + _, cookies = authedGET(t, ts, "/dashboard", cookies) + csrf := csrfFromCookies(cookies) + if csrf == "" { + t.Fatal("missing csrf after dashboard") + } + + payload := `{"cid":` + itoa(target.CID) + `,"network_rating":12}` + req := httptest.NewRequest(http.MethodPatch, "/api/v1/user/update", strings.NewReader(payload)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Cookie", cookieHeader(cookies)) + req.Header.Set(csrfHeaderName, csrf) + // No Authorization — cookie session only + w := httptest.NewRecorder() + ts.engine.ServeHTTP(w, req) + if w.Code != http.StatusForbidden { + t.Fatalf("cookie elevate status %d want 403, body %s", w.Code, w.Body.String()) + } + + u, err := ts.dbRepo.UserRepo.GetUserByCID(target.CID) + if err != nil { + t.Fatal(err) + } + if u.NetworkRating != int(protocol.NetworkRatingObserver) { + t.Fatalf("target rating = %d after cookie elevate attempt", u.NetworkRating) + } +} + +func TestSupervisorCannotCreateAdminViaForm(t *testing.T) { + ts := newTestServer(t) + sup := createTestUser(t, ts, "sup-pass", int(protocol.NetworkRatingSupervisor)) + cookies := formLogin(t, ts, sup.CID, "sup-pass") + + // UI should not offer Administrator option for supervisor + w, cookies := authedGET(t, ts, "/usereditor", cookies) + if w.Code != http.StatusOK { + t.Fatalf("GET usereditor %d", w.Code) + } + body := w.Body.String() + if strings.Contains(body, `value="12"`) { + t.Fatal("supervisor usereditor should not list Administrator (12) option") + } + + form := url.Values{} + form.Set("first_name", "Elevated") + form.Set("password", "password99") + form.Set("network_rating", "12") // forced POST bypassing UI + w, _ = formPOST(t, ts, "/usereditor/create", form, cookies) + // Server re-renders with error (200), not redirect success + if w.Code == http.StatusSeeOther { + t.Fatalf("create admin must not redirect success, Location=%s", w.Header().Get("Location")) + } + if w.Code != http.StatusOK { + t.Fatalf("status %d want 200 re-render", w.Code) + } + if !strings.Contains(w.Body.String(), "You cannot create a user with that rating") { + t.Fatalf("expected rating ceiling error, body=%s", clip(w.Body.String(), 500)) + } +} + +func TestSupervisorCannotPromoteToAdminViaForm(t *testing.T) { + ts := newTestServer(t) + sup := createTestUser(t, ts, "sup-pass", int(protocol.NetworkRatingSupervisor)) + target := createTestUser(t, ts, "obs-pass", int(protocol.NetworkRatingObserver)) + cookies := formLogin(t, ts, sup.CID, "sup-pass") + + form := url.Values{} + form.Set("cid", itoa(target.CID)) + form.Set("first_name", "Still") + form.Set("last_name", "Observer") + form.Set("network_rating", "12") + form.Set("password", "") + w, _ := formPOST(t, ts, "/usereditor/update", form, cookies) + if w.Code == http.StatusSeeOther { + t.Fatalf("promote to admin must not succeed, Location=%s", w.Header().Get("Location")) + } + if w.Code != http.StatusOK { + t.Fatalf("status %d want 200 re-render", w.Code) + } + if !strings.Contains(w.Body.String(), "Cannot set rating above your own") { + t.Fatalf("expected ceiling error, body=%s", clip(w.Body.String(), 500)) + } + + u, err := ts.dbRepo.UserRepo.GetUserByCID(target.CID) + if err != nil { + t.Fatal(err) + } + if u.NetworkRating != int(protocol.NetworkRatingObserver) { + t.Fatalf("DB rating = %d, want Observer still", u.NetworkRating) + } +} + +func TestSupervisorCannotUpdateHigherRatedUserViaForm(t *testing.T) { + ts := newTestServer(t) + // Create admin first so CID ordering is fine; login as supervisor + admin := createTestUser(t, ts, "admin-pass", int(protocol.NetworkRatingAdministator)) + sup := createTestUser(t, ts, "sup-pass", int(protocol.NetworkRatingSupervisor)) + cookies := formLogin(t, ts, sup.CID, "sup-pass") + + form := url.Values{} + form.Set("cid", itoa(admin.CID)) + form.Set("first_name", "Hacked") + form.Set("last_name", "Admin") + form.Set("network_rating", "11") + form.Set("password", "") + w, _ := formPOST(t, ts, "/usereditor/update", form, cookies) + if w.Code == http.StatusSeeOther { + t.Fatalf("must not update higher-rated user, Location=%s", w.Header().Get("Location")) + } + body := w.Body.String() + if !strings.Contains(body, "Cannot update user with higher network rating") { + t.Fatalf("expected higher-target error, body=%s", clip(body, 500)) + } + + u, err := ts.dbRepo.UserRepo.GetUserByCID(admin.CID) + if err != nil { + t.Fatal(err) + } + if safeStr(u.FirstName) == "Hacked" { + t.Fatal("admin first name must not change") + } +} diff --git a/internal/web/user.go b/internal/web/user.go index 951fae4..59b7fec 100644 --- a/internal/web/user.go +++ b/internal/web/user.go @@ -104,7 +104,14 @@ func (s *Server) updateUser(c *gin.Context) { if reqBody.LastName != nil { targetUser.LastName = reqBody.LastName } + // Ceiling matches createUser and the no-JS form path: cannot assign a + // network_rating above the actor's own rating (privilege escalation). if reqBody.NetworkRating != nil { + if *reqBody.NetworkRating > int(claims.NetworkRating) { + res := newAPIV1Failure("cannot set rating above your own") + writeAPIV1Response(c, http.StatusForbidden, &res) + return + } targetUser.NetworkRating = *reqBody.NetworkRating }