fix: harden dashboard security boundaries

This commit is contained in:
naiba committed 2026-10-01 04:28:34 +00:00
1 parent c726930833
commit 36b742a7e9
12 files changed
+357 -46

No files matched your search

+35 -2
View File
@@ -152,6 +152,8 @@ type ServerTransferClass struct {
subMu sync.Mutex
subs map[uint64]chan *model.ServerTransfer
subOwners map[uint64]uint64
subCounts map[uint64]int
nextSubID uint64
timeout time.Duration
@@ -174,6 +176,18 @@ var ErrServerAlreadyTransferring = errors.New("server already has an in-flight t
// deferred to OnAgentReconnect / PushIfOnline.
var ErrAgentTooOldForTransfer = fmt.Errorf("agent build older than %s does not support server transfer (TaskTypeServerTransferApply)", MinServerTransferAgentVersion)
const (
// A normal dashboard needs one subscription per open transfer page. Keep a
// small per-user allowance for tabs/devices plus a process-wide hard stop.
maxServerTransferSubscribersPerUser = 8
maxServerTransferSubscribersGlobal = 1024
)
var (
errTooManyServerTransferSubscribersForUser = errors.New("too many concurrent server transfer subscribers for this user")
errTooManyServerTransferSubscribersGlobal = errors.New("too many concurrent server transfer subscribers")
)
// agentSupportsTransfer reports whether s has reported a build version >=
// MinServerTransferAgentVersion. Returns true when version is unknown (agent
// never reported) so callers can defer the decision; PushIfOnline re-checks
@@ -210,6 +224,8 @@ func NewServerTransferClass() *ServerTransferClass {
verifiedHandshakes: make(map[uint64]string),
initiating: make(map[uint64]bool),
subs: make(map[uint64]chan *model.ServerTransfer),
subOwners: make(map[uint64]uint64),
subCounts: make(map[uint64]int),
timeout: defaultServerTransferTimeout,
stopCh: make(chan struct{}),
}
@@ -1410,20 +1426,37 @@ func (c *ServerTransferClass) sweepTimeouts() {
// Subscribe registers a channel that will receive every transfer transition
// event from this point forward. The caller MUST Unsubscribe when done or
// the broker will block forever if the channel is unbuffered or full.
func (c *ServerTransferClass) Subscribe() (uint64, <-chan *model.ServerTransfer) {
func (c *ServerTransferClass) Subscribe(userID uint64) (uint64, <-chan *model.ServerTransfer, error) {
c.subMu.Lock()
defer c.subMu.Unlock()
if len(c.subs) >= maxServerTransferSubscribersGlobal {
return 0, nil, errTooManyServerTransferSubscribersGlobal
}
if c.subCounts[userID] >= maxServerTransferSubscribersPerUser {
return 0, nil, errTooManyServerTransferSubscribersForUser
}
id := atomic.AddUint64(&c.nextSubID, 1)
ch := make(chan *model.ServerTransfer, 16)
c.subs[id] = ch
return id, ch
c.subOwners[id] = userID
c.subCounts[userID]++
return id, ch, nil
}
func (c *ServerTransferClass) Unsubscribe(id uint64) {
c.subMu.Lock()
ch, ok := c.subs[id]
ownerID, ownerOK := c.subOwners[id]
delete(c.subs, id)
delete(c.subOwners, id)
if ownerOK {
if c.subCounts[ownerID] <= 1 {
delete(c.subCounts, ownerID)
} else {
c.subCounts[ownerID]--
}
}
c.subMu.Unlock()
if ok {
close(ch)
+56 -3
View File
@@ -455,7 +455,8 @@ func TestServerTransferBroadcastReachesSubscribers(t *testing.T) {
defer cleanup()
seedServerForTransfer(t, 1, 100)
id, ch := c.Subscribe()
id, ch, err := c.Subscribe(100)
require.NoError(t, err)
defer c.Unsubscribe(id)
tr := initiateAndRegister(t, c, 1, 100, 200, 1)
@@ -469,7 +470,7 @@ func TestServerTransferBroadcastReachesSubscribers(t *testing.T) {
t.Fatal("expected Pending broadcast within 1s")
}
_, _, err := markPendingVerified(t, c, 1)
_, _, err = markPendingVerified(t, c, 1)
require.NoError(t, err)
select {
case ev := <-ch:
@@ -479,6 +480,57 @@ func TestServerTransferBroadcastReachesSubscribers(t *testing.T) {
}
}
func TestServerTransferSubscribeEnforcesPerUserLimitAndReleasesQuota(t *testing.T) {
c, cleanup := setupTransferFixture(t)
defer cleanup()
ids := make([]uint64, 0, maxServerTransferSubscribersPerUser)
for range maxServerTransferSubscribersPerUser {
id, ch, err := c.Subscribe(100)
require.NoError(t, err)
require.NotNil(t, ch)
ids = append(ids, id)
}
_, _, err := c.Subscribe(100)
require.ErrorIs(t, err, errTooManyServerTransferSubscribersForUser)
otherID, _, err := c.Subscribe(200)
require.NoError(t, err, "one user's quota must not block another user")
c.Unsubscribe(otherID)
c.Unsubscribe(ids[0])
replacementID, _, err := c.Subscribe(100)
require.NoError(t, err, "disconnecting must release the user's quota")
c.Unsubscribe(replacementID)
for _, id := range ids[1:] {
c.Unsubscribe(id)
}
}
func TestServerTransferSubscribeEnforcesGlobalLimit(t *testing.T) {
c, cleanup := setupTransferFixture(t)
defer cleanup()
ids := make([]uint64, 0, maxServerTransferSubscribersGlobal)
for userID := uint64(1); len(ids) < maxServerTransferSubscribersGlobal; userID++ {
for range maxServerTransferSubscribersPerUser {
if len(ids) == maxServerTransferSubscribersGlobal {
break
}
id, _, err := c.Subscribe(userID)
require.NoError(t, err)
ids = append(ids, id)
}
}
_, _, err := c.Subscribe(999999)
require.ErrorIs(t, err, errTooManyServerTransferSubscribersGlobal)
for _, id := range ids {
c.Unsubscribe(id)
}
}
// The "one active transfer per server" invariant must hold under concurrent
// callers (two operators batch-moving the same server, or batch-move racing
// retry). The old flow had a TOCTOU between HasPending() and Initiate() that
@@ -1626,7 +1678,8 @@ func TestOnServersDeletedTerminatesPendingTransfersAndClearsIndexes(t *testing.T
tr := initiateAndRegister(t, c, 1, 100, 200, 1)
require.True(t, c.HasPending(1))
subID, ch := c.Subscribe()
subID, ch, err := c.Subscribe(100)
require.NoError(t, err)
defer c.Unsubscribe(subID)
require.NoError(t, DB.Unscoped().Delete(&model.Server{}, tr.ServerID).Error)