From 6f92b9396dfbacaf71c20444d175af78e0517409 Mon Sep 17 00:00:00 2001 From: henrygd Date: Fri, 21 Aug 2026 17:25:09 -0400 Subject: [PATCH] fix(hub): user alerts idor fixes very unlikely scenario where user guesses another user's 15 character random system id and adds alerts for it --- internal/alerts/alerts_api.go | 16 ++++++++++ internal/alerts/alerts_api_test.go | 49 ++++++++++++++++++++++++++++++ internal/hub/api_test.go | 44 ++++++++++++++++++++++++++- internal/hub/collections_test.go | 31 +++++++++++++++++++ 4 files changed, 139 insertions(+), 1 deletion(-) diff --git a/internal/alerts/alerts_api.go b/internal/alerts/alerts_api.go index 8c9e92ef..feb0587c 100644 --- a/internal/alerts/alerts_api.go +++ b/internal/alerts/alerts_api.go @@ -9,6 +9,7 @@ import ( "slices" "strings" + "github.com/henrygd/beszel/internal/hub/utils" "github.com/pocketbase/dbx" "github.com/pocketbase/pocketbase/core" ) @@ -37,6 +38,9 @@ func UpsertUserAlerts(e *core.RequestEvent) error { err = e.App.RunInTransaction(func(txApp core.App) error { for _, systemId := range reqData.Systems { + if !userHasSystem(txApp, userID, systemId) { + continue + } // find existing matching alert alertRecord, err := txApp.FindFirstRecordByFilter(alertsCollection, "system={:system} && name={:name} && user={:user}", @@ -94,6 +98,9 @@ func DeleteUserAlerts(e *core.RequestEvent) error { err = e.App.RunInTransaction(func(txApp core.App) error { for _, systemId := range reqData.Systems { + if !userHasSystem(txApp, userID, systemId) { + continue + } // Find existing alert to delete alertRecord, err := txApp.FindFirstRecordByFilter("alerts", "system={:system} && name={:name} && user={:user}", @@ -122,6 +129,15 @@ func DeleteUserAlerts(e *core.RequestEvent) error { return e.JSON(http.StatusOK, map[string]any{"success": true, "count": numDeleted}) } +func userHasSystem(app core.App, userID, systemID string) bool { + system, err := app.FindRecordById("systems", systemID) + if err != nil { + return false + } + shareAll, _ := utils.GetEnv("SHARE_ALL_SYSTEMS") + return shareAll == "true" || slices.Contains(system.GetStringSlice("users"), userID) +} + // SendTestNotification handles API request to send a test notification to a specified Shoutrrr URL func (am *AlertManager) SendTestNotification(e *core.RequestEvent) error { var data struct { diff --git a/internal/alerts/alerts_api_test.go b/internal/alerts/alerts_api_test.go index dccb667f..326e8f35 100644 --- a/internal/alerts/alerts_api_test.go +++ b/internal/alerts/alerts_api_test.go @@ -190,6 +190,30 @@ func TestUserAlertsApi(t *testing.T) { assert.EqualValues(t, 3, user1Alerts, "should have 3 alerts") }, }, + { + Name: "POST ignores systems the user cannot access", + Method: http.MethodPost, + URL: "/api/beszel/user-alerts", + Headers: map[string]string{ + "Authorization": user2Token, + }, + ExpectedStatus: 200, + ExpectedContent: []string{"\"success\":true"}, + TestAppFactory: testAppFactory, + Body: jsonReader(map[string]any{ + "name": "CPU", + "systems": []string{system1.Id}, + "value": 90, + "min": 10, + }), + BeforeTestFunc: func(t testing.TB, app *pbTests.TestApp, e *core.ServeEvent) { + beszelTests.ClearCollection(t, app, "alerts") + }, + AfterTestFunc: func(t testing.TB, app *pbTests.TestApp, res *http.Response) { + alerts, _ := app.CountRecords("alerts") + assert.Zero(t, alerts) + }, + }, { Name: "Overwrite: false, should not overwrite existing alert", Method: http.MethodPost, @@ -347,6 +371,31 @@ func TestUserAlertsApi(t *testing.T) { assert.Zero(t, alerts, "should have 0 alerts") }, }, + { + Name: "DELETE ignores systems the user cannot access", + Method: http.MethodDelete, + URL: "/api/beszel/user-alerts", + Headers: map[string]string{ + "Authorization": user2Token, + }, + ExpectedStatus: 200, + ExpectedContent: []string{"\"count\":0", "\"success\":true"}, + TestAppFactory: testAppFactory, + Body: jsonReader(map[string]any{ + "name": "CPU", + "systems": []string{system1.Id}, + }), + BeforeTestFunc: func(t testing.TB, app *pbTests.TestApp, e *core.ServeEvent) { + beszelTests.ClearCollection(t, app, "alerts") + beszelTests.CreateRecord(app, "alerts", map[string]any{ + "name": "CPU", "system": system1.Id, "user": user2.Id, "value": 80, + }) + }, + AfterTestFunc: func(t testing.TB, app *pbTests.TestApp, res *http.Response) { + alerts, _ := app.CountRecords("alerts") + assert.EqualValues(t, 1, alerts) + }, + }, { Name: "User 2 should not be able to delete alert of user 1", Method: http.MethodDelete, diff --git a/internal/hub/api_test.go b/internal/hub/api_test.go index 15bb7445..bbe3ff0c 100644 --- a/internal/hub/api_test.go +++ b/internal/hub/api_test.go @@ -11,6 +11,7 @@ import ( beszelTests "github.com/henrygd/beszel/internal/tests" "github.com/henrygd/beszel/internal/migrations" + "github.com/pocketbase/dbx" "github.com/pocketbase/pocketbase/core" pbTests "github.com/pocketbase/pocketbase/tests" "github.com/stretchr/testify/require" @@ -55,7 +56,7 @@ func TestApiRoutesAuthentication(t *testing.T) { // Create test system system, err := beszelTests.CreateRecord(hub, "systems", map[string]any{ "name": "test-system", - "users": []string{user.Id}, + "users": []string{user.Id, readOnlyUser.Id}, "host": "127.0.0.1", }) require.NoError(t, err, "Failed to create test system") @@ -277,6 +278,24 @@ func TestApiRoutesAuthentication(t *testing.T) { "systems": []string{system.Id}, }), }, + { + Name: "POST /user-alerts - readonly user can create own alert", + Method: http.MethodPost, + URL: "/api/beszel/user-alerts", + Headers: map[string]string{ + "Authorization": readOnlyUserToken, + }, + ExpectedStatus: 200, + ExpectedContent: []string{"\"success\":true"}, + TestAppFactory: testAppFactory, + Body: jsonReader(map[string]any{ + "name": "CPU", "value": 80, "min": 10, "systems": []string{system.Id}, + }), + AfterTestFunc: func(t testing.TB, app *pbTests.TestApp, res *http.Response) { + alerts, _ := app.CountRecords("alerts", dbx.HashExp{"user": readOnlyUser.Id}) + require.EqualValues(t, 1, alerts) + }, + }, { Name: "DELETE /user-alerts - no auth should fail", Method: http.MethodDelete, @@ -314,6 +333,29 @@ func TestApiRoutesAuthentication(t *testing.T) { }) }, }, + { + Name: "DELETE /user-alerts - readonly user can delete own alert", + Method: http.MethodDelete, + URL: "/api/beszel/user-alerts", + Headers: map[string]string{ + "Authorization": readOnlyUserToken, + }, + ExpectedStatus: 200, + ExpectedContent: []string{"\"count\":1", "\"success\":true"}, + TestAppFactory: testAppFactory, + Body: jsonReader(map[string]any{ + "name": "CPU", "systems": []string{system.Id}, + }), + BeforeTestFunc: func(t testing.TB, app *pbTests.TestApp, e *core.ServeEvent) { + beszelTests.CreateRecord(app, "alerts", map[string]any{ + "name": "CPU", "system": system.Id, "user": readOnlyUser.Id, "value": 80, + }) + }, + AfterTestFunc: func(t testing.TB, app *pbTests.TestApp, res *http.Response) { + alerts, _ := app.CountRecords("alerts", dbx.HashExp{"user": readOnlyUser.Id}) + require.Zero(t, alerts) + }, + }, { Name: "GET /containers/logs - no auth should fail", Method: http.MethodGet, diff --git a/internal/hub/collections_test.go b/internal/hub/collections_test.go index eb583006..054fdde1 100644 --- a/internal/hub/collections_test.go +++ b/internal/hub/collections_test.go @@ -357,6 +357,13 @@ func TestApiCollectionsAuthRules(t *testing.T) { "host": "127.0.0.2", }) + userOneAlert, _ := beszelTests.CreateRecord(hub, "alerts", map[string]any{ + "name": "CPU", "system": userOneSystem.Id, "user": user1.Id, "value": 80, + }) + userTwoAlert, _ := beszelTests.CreateRecord(hub, "alerts", map[string]any{ + "name": "CPU", "system": userTwoSystem.Id, "user": user2.Id, "value": 80, + }) + userRecords, _ := hub.CountRecords("users") assert.EqualValues(t, 3, userRecords, "all users should be created") @@ -368,6 +375,30 @@ func TestApiCollectionsAuthRules(t *testing.T) { } scenarios := []beszelTests.ApiScenario{ + { + Name: "Users can only list their own alerts", + Method: http.MethodGet, + URL: "/api/collections/alerts/records", + Headers: map[string]string{ + "Authorization": user1Token, + }, + ExpectedStatus: 200, + ExpectedContent: []string{userOneAlert.Id}, + NotExpectedContent: []string{userTwoAlert.Id}, + TestAppFactory: testAppFactory, + }, + { + Name: "Users cannot view another user's alert by id", + Method: http.MethodGet, + URL: fmt.Sprintf("/api/collections/alerts/records/%s", userTwoAlert.Id), + Headers: map[string]string{ + "Authorization": user1Token, + }, + ExpectedStatus: 403, + ExpectedContent: []string{"Only superusers"}, + NotExpectedContent: []string{userTwoAlert.Id}, + TestAppFactory: testAppFactory, + }, { Name: "Unauthorized user cannot list systems", Method: http.MethodGet,