From c479c3e79c35501214e64304e32f48d6263fbd65 Mon Sep 17 00:00:00 2001 From: Daniel Jeremiah <121419973+daniyyell-dev@users.noreply.github.com> Date: Sat, 17 Oct 2026 18:38:17 +0100 Subject: [PATCH] fix: prevent ownership reassignment through record updates (#2632) Co-authored-by: henrygd --- internal/hub/api_settings_ownership_test.go | 155 ++++++++++++++++++++ internal/hub/collections.go | 25 +++- internal/hub/collections_test.go | 12 +- 3 files changed, 185 insertions(+), 7 deletions(-) create mode 100644 internal/hub/api_settings_ownership_test.go diff --git a/internal/hub/api_settings_ownership_test.go b/internal/hub/api_settings_ownership_test.go new file mode 100644 index 000000000..7096793ec --- /dev/null +++ b/internal/hub/api_settings_ownership_test.go @@ -0,0 +1,155 @@ +//go:build testing + +package hub_test + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + beszelTests "github.com/henrygd/beszel/internal/tests" + "github.com/stretchr/testify/require" +) + +func TestSettingsOwnershipUpdates(t *testing.T) { + for _, shareAll := range []bool{false, true} { + t.Run(fmt.Sprintf("share_all_systems=%t", shareAll), func(t *testing.T) { + t.Setenv("SHARE_ALL_SYSTEMS", fmt.Sprint(shareAll)) + hub, handler := firstUserTestMux(t) + defer hub.Cleanup() + a, err := beszelTests.CreateUser(hub, "owner@example.invalid", "password123") + require.NoError(t, err) + b, err := beszelTests.CreateUser(hub, "other@example.invalid", "password123") + require.NoError(t, err) + empty, err := beszelTests.CreateUser(hub, "empty@example.invalid", "password123") + require.NoError(t, err) + auth, err := a.NewAuthToken() + require.NoError(t, err) + superuser, err := beszelTests.CreateSuperuser(hub, "maintenance@example.invalid", "password123") + require.NoError(t, err) + superAuth, err := superuser.NewAuthToken() + require.NoError(t, err) + now := time.Now().UTC() + + type testCase struct { + collection string + field string + original string + target string + create map[string]any + edit map[string]any + } + systems, err := beszelTests.CreateSystems(hub, 1, a.Id, "paused") + require.NoError(t, err) + targets, err := beszelTests.CreateSystems(hub, 1, b.Id, "paused") + require.NoError(t, err) + // The settings recipient has no settings record, and the target system has + // no fingerprint, so uniqueness constraints cannot mask an ownership bypass. + _, err = beszelTests.CreateRecord(hub, "quiet_hours", map[string]any{ + "user": b.Id, "type": "one-time", + "start": now.Add(time.Hour).Format(time.RFC3339), "end": now.Add(2 * time.Hour).Format(time.RFC3339), + }) + require.NoError(t, err) + cases := []testCase{ + { + collection: "user_settings", field: "user", original: a.Id, target: empty.Id, + create: map[string]any{"user": a.Id, "settings": map[string]any{"chartTime": "1h"}}, + edit: map[string]any{"settings": map[string]any{"chartTime": "12h"}}, + }, + { + collection: "quiet_hours", field: "user", original: a.Id, target: b.Id, + create: map[string]any{ + "user": a.Id, "type": "one-time", "start": now.Add(-time.Minute).Format(time.RFC3339), + "end": now.Add(time.Hour).Format(time.RFC3339), + }, + edit: map[string]any{"end": now.Add(3 * time.Hour).Format(time.RFC3339)}, + }, + { + collection: "fingerprints", field: "system", original: systems[0].Id, target: targets[0].Id, + create: map[string]any{"system": systems[0].Id, "token": "initial-dummy-token", "fingerprint": "dummy-fingerprint"}, + edit: map[string]any{"token": "rotated-dummy-token", "fingerprint": ""}, + }, + } + for _, tc := range cases { + t.Run(tc.collection, func(t *testing.T) { + record, err := beszelTests.CreateRecord(hub, tc.collection, tc.create) + require.NoError(t, err) + path := "/api/collections/" + tc.collection + "/records/" + record.Id + patch := func(body map[string]any, token string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodPatch, path, jsonReader(body)) + req.Header.Set("Authorization", token) + req.Header.Set("Content-Type", "application/json") + response := httptest.NewRecorder() + handler.ServeHTTP(response, req) + return response + } + t.Run("normal edit with unchanged relationship", func(t *testing.T) { + body := map[string]any{tc.field: tc.original} + for key, value := range tc.edit { + body[key] = value + } + response := patch(body, auth) + require.Equal(t, http.StatusOK, response.Code, response.Body.String()) + }) + t.Run("normal edit without relationship", func(t *testing.T) { + response := patch(tc.edit, auth) + require.Equal(t, http.StatusOK, response.Code, response.Body.String()) + }) + before, err := hub.FindRecordById(tc.collection, record.Id) + require.NoError(t, err) + for _, modification := range []struct { + name string + body map[string]any + }{ + {"reassign", map[string]any{tc.field: tc.target}}, + {"clear", map[string]any{tc.field: ""}}, + {"append relation", map[string]any{tc.field + "+": tc.target}}, + {"remove relation", map[string]any{tc.field + "-": tc.original}}, + } { + t.Run(modification.name, func(t *testing.T) { + response := patch(modification.body, auth) + require.Contains(t, []int{400, 403, 404}, response.Code, response.Body.String()) + after, err := hub.FindRecordById(tc.collection, record.Id) + require.NoError(t, err) + require.Equal(t, tc.original, after.GetString(tc.field)) + for field := range tc.edit { + require.Equal(t, before.Get(field), after.Get(field)) + } + }) + } + t.Run("batch reassignment", func(t *testing.T) { + req := httptest.NewRequest(http.MethodPost, "/api/batch", jsonReader(map[string]any{ + "requests": []map[string]any{{"method": "PATCH", "url": path, "body": map[string]any{tc.field: tc.target}}}, + })) + req.Header.Set("Authorization", auth) + req.Header.Set("Content-Type", "application/json") + response := httptest.NewRecorder() + handler.ServeHTTP(response, req) + require.Equal(t, http.StatusBadRequest, response.Code, response.Body.String()) + after, err := hub.FindRecordById(tc.collection, record.Id) + require.NoError(t, err) + require.Equal(t, tc.original, after.GetString(tc.field)) + }) + if tc.collection == "quiet_hours" { + t.Run("recipient notification state remains unchanged", func(t *testing.T) { + require.False(t, hub.IsNotificationSilenced(b.Id, "")) + response := patch(map[string]any{"user": b.Id}, auth) + require.Equal(t, http.StatusNotFound, response.Code) + require.False(t, hub.IsNotificationSilenced(b.Id, "")) + }) + } + t.Run("superuser maintenance", func(t *testing.T) { + response := patch(map[string]any{tc.field: tc.target}, superAuth) + require.Equal(t, http.StatusOK, response.Code, response.Body.String()) + var data map[string]any + require.NoError(t, json.Unmarshal(response.Body.Bytes(), &data)) + require.Equal(t, tc.target, data[tc.field]) + }) + }) + } + }) + } +} diff --git a/internal/hub/collections.go b/internal/hub/collections.go index 41a7d989d..1c4a52172 100644 --- a/internal/hub/collections.go +++ b/internal/hub/collections.go @@ -98,11 +98,13 @@ func setCollectionAuthSettings(app core.App) error { return err } + // The system of an existing fingerprint cannot be changed through the API. + fingerprintsUpdateRule := systemScopedWriteRule + " && @request.body.system:changed = false" if err := applyCollectionRules(app, []string{"fingerprints"}, collectionRules{ list: &systemScopedWriteRule, view: &systemScopedWriteRule, create: &systemScopedWriteRule, - update: &systemScopedWriteRule, + update: &fingerprintsUpdateRule, delete: &systemScopedWriteRule, }); err != nil { return err @@ -144,6 +146,27 @@ func setCollectionAuthSettings(app core.App) error { return err } + // User settings and quiet hours belong to their user, and the user of an + // existing record cannot be changed through the API. + userOwnedRule := authenticatedRule + " && user = @request.auth.id" + userOwnedUpdateRule := userOwnedRule + " && @request.body.user:changed = false" + if err := applyCollectionRules(app, []string{"user_settings"}, collectionRules{ + list: &userOwnedRule, + create: &userOwnedRule, + update: &userOwnedUpdateRule, + }); err != nil { + return err + } + if err := applyCollectionRules(app, []string{"quiet_hours"}, collectionRules{ + list: &userOwnedRule, + view: &userOwnedRule, + create: &userOwnedRule, + update: &userOwnedUpdateRule, + delete: &userOwnedRule, + }); err != nil { + return err + } + return nil } diff --git a/internal/hub/collections_test.go b/internal/hub/collections_test.go index 280a583bc..2acd245c0 100644 --- a/internal/hub/collections_test.go +++ b/internal/hub/collections_test.go @@ -91,7 +91,7 @@ func TestCollectionRulesDefault(t *testing.T) { assert.Equal(t, isUserInSystemUsersNotReadonly, *fingerprintsCollection.ListRule) assert.Equal(t, isUserInSystemUsersNotReadonly, *fingerprintsCollection.ViewRule) assert.Equal(t, isUserInSystemUsersNotReadonly, *fingerprintsCollection.CreateRule) - assert.Equal(t, isUserInSystemUsersNotReadonly, *fingerprintsCollection.UpdateRule) + assert.Equal(t, isUserInSystemUsersNotReadonly+` && @request.body.system:changed = false`, *fingerprintsCollection.UpdateRule) assert.Equal(t, isUserInSystemUsersNotReadonly, *fingerprintsCollection.DeleteRule) // quiet_hours collection @@ -100,7 +100,7 @@ func TestCollectionRulesDefault(t *testing.T) { assert.Equal(t, isUserMatchesUser, *quietHoursCollection.ListRule) assert.Equal(t, isUserMatchesUser, *quietHoursCollection.ViewRule) assert.Equal(t, isUserMatchesUser, *quietHoursCollection.CreateRule) - assert.Equal(t, isUserMatchesUser, *quietHoursCollection.UpdateRule) + assert.Equal(t, isUserMatchesUser+` && @request.body.user:changed = false`, *quietHoursCollection.UpdateRule) assert.Equal(t, isUserMatchesUser, *quietHoursCollection.DeleteRule) // smart_devices collection @@ -163,7 +163,7 @@ func TestCollectionRulesDefault(t *testing.T) { assert.Equal(t, isUserMatchesUser, *userSettingsCollection.ListRule) assert.Nil(t, userSettingsCollection.ViewRule) assert.Equal(t, isUserMatchesUser, *userSettingsCollection.CreateRule) - assert.Equal(t, isUserMatchesUser, *userSettingsCollection.UpdateRule) + assert.Equal(t, isUserMatchesUser+` && @request.body.user:changed = false`, *userSettingsCollection.UpdateRule) assert.Nil(t, userSettingsCollection.DeleteRule) } @@ -219,7 +219,7 @@ func TestCollectionRulesShareAllSystems(t *testing.T) { assert.Equal(t, isUserNotReadonly, *fingerprintsCollection.ListRule) assert.Equal(t, isUserNotReadonly, *fingerprintsCollection.ViewRule) assert.Equal(t, isUserNotReadonly, *fingerprintsCollection.CreateRule) - assert.Equal(t, isUserNotReadonly, *fingerprintsCollection.UpdateRule) + assert.Equal(t, isUserNotReadonly+` && @request.body.system:changed = false`, *fingerprintsCollection.UpdateRule) assert.Equal(t, isUserNotReadonly, *fingerprintsCollection.DeleteRule) // quiet_hours collection @@ -228,7 +228,7 @@ func TestCollectionRulesShareAllSystems(t *testing.T) { assert.Equal(t, isUserMatchesUser, *quietHoursCollection.ListRule) assert.Equal(t, isUserMatchesUser, *quietHoursCollection.ViewRule) assert.Equal(t, isUserMatchesUser, *quietHoursCollection.CreateRule) - assert.Equal(t, isUserMatchesUser, *quietHoursCollection.UpdateRule) + assert.Equal(t, isUserMatchesUser+` && @request.body.user:changed = false`, *quietHoursCollection.UpdateRule) assert.Equal(t, isUserMatchesUser, *quietHoursCollection.DeleteRule) // smart_devices collection @@ -291,7 +291,7 @@ func TestCollectionRulesShareAllSystems(t *testing.T) { assert.Equal(t, isUserMatchesUser, *userSettingsCollection.ListRule) assert.Nil(t, userSettingsCollection.ViewRule) assert.Equal(t, isUserMatchesUser, *userSettingsCollection.CreateRule) - assert.Equal(t, isUserMatchesUser, *userSettingsCollection.UpdateRule) + assert.Equal(t, isUserMatchesUser+` && @request.body.user:changed = false`, *userSettingsCollection.UpdateRule) assert.Nil(t, userSettingsCollection.DeleteRule) }