From 710a2c731c1d185c8a3d6318a1244719e5ccc3ff Mon Sep 17 00:00:00 2001 From: naiba Date: Mon, 18 May 2026 15:16:59 +0000 Subject: [PATCH] fix(controller): hide foreign server IDs from forceUpdateServer response MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous loop only checked ownership inside the \`server != nil && server.TaskStream != nil\` branch. A foreign online server returned permission denied for the whole batch while foreign offline / unknown IDs silently went into the Offline bucket — the response-shape delta let a RoleMember enumerate other users' online server IDs by submitting them in batches. Drop both foreign and unknown IDs silently into the Offline bucket (without dispatching the upgrade task) so the response is indistinguishable across those three states. Co-authored-by: naiba/CloudCode --- .../controller/force_update_ownership_test.go | 149 ++++++++++++++++++ cmd/dashboard/controller/server.go | 14 +- 2 files changed, 159 insertions(+), 4 deletions(-) create mode 100644 cmd/dashboard/controller/force_update_ownership_test.go diff --git a/cmd/dashboard/controller/force_update_ownership_test.go b/cmd/dashboard/controller/force_update_ownership_test.go new file mode 100644 index 00000000..d501b8e8 --- /dev/null +++ b/cmd/dashboard/controller/force_update_ownership_test.go @@ -0,0 +1,149 @@ +package controller + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "gorm.io/driver/sqlite" + "gorm.io/gorm" + + "github.com/nezhahq/nezha/model" + "github.com/nezhahq/nezha/pkg/i18n" + pb "github.com/nezhahq/nezha/proto" + "github.com/nezhahq/nezha/service/singleton" +) + +// fakeTaskStream is the minimum stub of pb.NezhaService_RequestTaskServer +// required to make a server look "online" to forceUpdateServer. Only Send is +// called; we capture its argument so the test can verify the upgrade task is +// NOT dispatched for foreign IDs. +type fakeTaskStream struct { + pb.NezhaService_RequestTaskServer + sentTasks []*pb.Task +} + +func (f *fakeTaskStream) Send(t *pb.Task) error { + f.sentTasks = append(f.sentTasks, t) + return nil +} + +// setupServerOwnershipFixture seeds an in-memory DB with alice's server +// (UserID=100, ID=1). The returned stream is wired in so the server is +// "online" — i.e. exercises the path that previously returned permission +// denied for foreign callers (the actual leak channel). +func setupServerOwnershipFixture(t *testing.T) (stream *fakeTaskStream, reset func()) { + t.Helper() + if singleton.Localizer == nil { + singleton.Localizer = i18n.NewLocalizer("en_US", "nezha", "translations", i18n.Translations) + } + originalDB := singleton.DB + originalShared := singleton.ServerShared + + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + assert.NoError(t, err) + assert.NoError(t, db.AutoMigrate(&model.Server{})) + assert.NoError(t, db.Create(&model.Server{ + Common: model.Common{ID: 1, UserID: 100}, + Name: "alice-online", + }).Error) + singleton.DB = db + singleton.ServerShared = singleton.NewServerClass() + + alice, _ := singleton.ServerShared.Get(1) + stream = &fakeTaskStream{} + alice.TaskStream = stream + + return stream, func() { + singleton.DB = originalDB + singleton.ServerShared = originalShared + } +} + +func runForceUpdate(t *testing.T, callerID uint64, ids []uint64) []byte { + t.Helper() + r := gin.New() + r.Use(func(c *gin.Context) { + setAuthUser(c, callerID, model.RoleMember) + c.Next() + }) + r.POST("/force-update/server", commonHandler(forceUpdateServer)) + + body, _ := json.Marshal(ids) + w := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/force-update/server", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + r.ServeHTTP(w, req) + return w.Body.Bytes() +} + +type forceUpdateBody struct { + Success bool `json:"success"` + Error string `json:"error"` + Data struct { + Offline []uint64 `json:"offline"` + Success []uint64 `json:"success"` + Failure []uint64 `json:"failure"` + } `json:"data"` +} + +func decodeForceUpdate(t *testing.T, body []byte) forceUpdateBody { + t.Helper() + var resp forceUpdateBody + assert.NoError(t, json.Unmarshal(body, &resp)) + return resp +} + +// Core regression: bob submitting alice's online server ID must NOT produce +// a distinct response from bob submitting an unknown ID. The original code +// returned "permission denied" for the former and a structured success/Offline +// response for the latter — that delta is the enumeration oracle for server +// IDs and online state. +func TestForceUpdateServerOnlineForeignIDIndistinguishableFromUnknown(t *testing.T) { + gin.SetMode(gin.TestMode) + _, reset := setupServerOwnershipFixture(t) + defer reset() + + const bobID = uint64(200) + foreignResp := decodeForceUpdate(t, runForceUpdate(t, bobID, []uint64{1})) // alice's online + unknownResp := decodeForceUpdate(t, runForceUpdate(t, bobID, []uint64{9999})) // does not exist + + assert.Equal(t, foreignResp.Success, unknownResp.Success, + "top-level success flag must not differ between foreign-online and unknown IDs") + assert.Equal(t, foreignResp.Error, unknownResp.Error, + "error string must not differ — distinct error reveals existence/state of foreign servers") + assert.Equal(t, foreignResp.Data.Success, unknownResp.Data.Success) + assert.Equal(t, foreignResp.Data.Failure, unknownResp.Data.Failure) +} + +// Submitting a foreign online server must NOT actually trigger the upgrade +// task on it — that would be a write primitive on someone else's machine. +func TestForceUpdateServerForeignOnlineDoesNotDispatchUpgrade(t *testing.T) { + gin.SetMode(gin.TestMode) + stream, reset := setupServerOwnershipFixture(t) + defer reset() + + _ = runForceUpdate(t, 200, []uint64{1}) // bob hits alice's online server + assert.Empty(t, stream.sentTasks, + "foreign server must not receive the upgrade task even when online") +} + +// Sanity: owner submitting their own online server must still get the upgrade +// dispatched and a structured success response — the hardening must not +// regress the legitimate case. +func TestForceUpdateServerOwnerOnlineStillDispatches(t *testing.T) { + gin.SetMode(gin.TestMode) + stream, reset := setupServerOwnershipFixture(t) + defer reset() + + resp := decodeForceUpdate(t, runForceUpdate(t, 100, []uint64{1})) // alice on her own server + assert.True(t, resp.Success) + assert.Equal(t, []uint64{1}, resp.Data.Success) + assert.Empty(t, resp.Data.Offline) + assert.Empty(t, resp.Data.Failure) + assert.Len(t, stream.sentTasks, 1, "owner's own server must receive the upgrade task exactly once") +} diff --git a/cmd/dashboard/controller/server.go b/cmd/dashboard/controller/server.go index ec9bad59..4f3e015a 100644 --- a/cmd/dashboard/controller/server.go +++ b/cmd/dashboard/controller/server.go @@ -178,10 +178,16 @@ func forceUpdateServer(c *gin.Context) (*model.ServerTaskResponse, error) { for _, sid := range forceUpdateServers { server, _ := singleton.ServerShared.Get(sid) - if server != nil && server.TaskStream != nil { - if !server.HasPermission(c) { - return nil, singleton.Localizer.ErrorT("permission denied") - } + // Per-ID ownership check. Foreign servers (online or offline) and + // unknown IDs MUST be indistinguishable in the response — otherwise the + // response shape leaks server-ID existence/online-state, letting a + // RoleMember enumerate other users' machines. We drop them into the + // Offline bucket without actually dispatching the upgrade task. + if server == nil || !server.HasPermission(c) { + forceUpdateResp.Offline = append(forceUpdateResp.Offline, sid) + continue + } + if server.TaskStream != nil { if err := server.TaskStream.Send(&pb.Task{ Type: model.TaskTypeUpgrade, }); err != nil {