Security hardening fixes

api/jsonrpc/server.go

@@ -262,6 +262,18 @@ func (j *JSONRPCServer) SimulateActions(
 	ctx, span := j.vm.Tracer().Start(req.Context(), "JSONRPCServer.SimulateActions")
 	defer span.End()
 
+	currentTime := time.Now().UnixMilli()
+	ruleFactory := j.vm.GetRuleFactory()
+	rules := ruleFactory.GetRules(currentTime)
+
+	// Limit the number of actions to simulate to avoid DoS attacks
+	if len(args.Actions) == 0 {
+		return errSimulateZeroActions
+	}
+	if maxActionsPerTx := int(rules.GetMaxActionsPerTx()); len(args.Actions) > maxActionsPerTx {
+		return fmt.Errorf("exceeded max actions per simulation: %d", maxActionsPerTx)
+	}
+
 	txParser := j.vm.GetParser()
 	actions := make([]chain.Action, 0, len(args.Actions))
 	for _, actionBytes := range args.Actions {
@@ -271,9 +283,6 @@ func (j *JSONRPCServer) SimulateActions(
 		}
 		actions = append(actions, action)
 	}
-	if len(actions) == 0 {
-		return errSimulateZeroActions
-	}
 	currentState, err := j.vm.ImmutableState(ctx)
 	if err != nil {
 		return err
@@ -287,9 +296,6 @@ func (j *JSONRPCServer) SimulateActions(
 		0,
 	)
 
-	currentTime := time.Now().UnixMilli()
-	ruleFactory := j.vm.GetRuleFactory()
-	rules := ruleFactory.GetRules(currentTime)
 	for _, action := range actions {
 		actionOutput, err := action.Execute(
 			ctx,

api/jsonrpc/server_test.go

@@ -0,0 +1,142 @@
+// Copyright (C) 2024, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package jsonrpc
+
+import (
+	"context"
+	"net/http"
+	"testing"
+
+	"github.com/ava-labs/avalanchego/trace"
+	"github.com/stretchr/testify/require"
+
+	"github.com/ava-labs/hypersdk/api"
+	"github.com/ava-labs/hypersdk/chain"
+	"github.com/ava-labs/hypersdk/chain/chaintest"
+	"github.com/ava-labs/hypersdk/codec"
+	"github.com/ava-labs/hypersdk/genesis"
+	"github.com/ava-labs/hypersdk/state"
+)
+
+// stubVM implements only the methods the simulation endpoints touch. Any other
+// call panics on the embedded nil interface, which keeps the stub honest.
+type stubVM struct {
+	api.VM
+
+	ruleFactory chain.RuleFactory
+	parsed      int
+}
+
+func (*stubVM) Tracer() trace.Tracer { return trace.Noop }
+
+func (s *stubVM) GetRuleFactory() chain.RuleFactory { return s.ruleFactory }
+
+func (s *stubVM) GetParser() chain.Parser {
+	s.parsed++
+	return chaintest.NewTestParser()
+}
+
+func (*stubVM) ImmutableState(context.Context) (state.Immutable, error) {
+	return state.ImmutableStorage(map[string][]byte{}), nil
+}
+
+func (*stubVM) ReadState(_ context.Context, keys [][]byte) ([][]byte, []error) {
+	return make([][]byte, len(keys)), make([]error, len(keys))
+}
+
+func newTestServer(rules chain.Rules) (*JSONRPCServer, *stubVM) {
+	v := &stubVM{ruleFactory: &genesis.ImmutableRuleFactory{Rules: rules}}
+	return NewJSONRPCServer(v), v
+}
+
+func dummyActionBytes(t *testing.T) []byte {
+	t.Helper()
+
+	return chaintest.NewDummyTestAction().Bytes()
+}
+
+// SimulateActions is unauthenticated and unmetered, so it must bound the action
+// count the same way ExecuteActions does. Without this, one request could ask
+// the node to parse and execute an arbitrarily long action list against live
+// state.
+func TestSimulateActionsBoundsActionCount(t *testing.T) {
+	rules := genesis.NewDefaultRules()
+	maxActions := int(rules.GetMaxActionsPerTx())
+
+	t.Run("over the limit is rejected before parsing", func(t *testing.T) {
+		r := require.New(t)
+
+		server, v := newTestServer(rules)
+
+		actionBytes := dummyActionBytes(t)
+		actions := make([]codec.Bytes, maxActions+1)
+		for i := range actions {
+			actions[i] = actionBytes
+		}
+
+		err := server.SimulateActions(
+			&http.Request{},
+			&SimulatActionsArgs{Actions: actions},
+			&SimulateActionsReply{},
+		)
+		r.ErrorContains(err, "exceeded max actions per simulation")
+		r.Zero(v.parsed, "the request should be rejected before any action is parsed")
+	})
+
+	t.Run("zero actions is rejected", func(t *testing.T) {
+		r := require.New(t)
+
+		server, _ := newTestServer(rules)
+
+		err := server.SimulateActions(
+			&http.Request{},
+			&SimulatActionsArgs{},
+			&SimulateActionsReply{},
+		)
+		r.ErrorIs(err, errSimulateZeroActions)
+	})
+
+	t.Run("at the limit is accepted", func(t *testing.T) {
+		r := require.New(t)
+
+		server, _ := newTestServer(rules)
+
+		actionBytes := dummyActionBytes(t)
+		actions := make([]codec.Bytes, maxActions)
+		for i := range actions {
+			actions[i] = actionBytes
+		}
+
+		reply := &SimulateActionsReply{}
+		err := server.SimulateActions(
+			&http.Request{},
+			&SimulatActionsArgs{Actions: actions},
+			reply,
+		)
+		r.NoError(err)
+		r.Len(reply.ActionResults, maxActions)
+	})
+}
+
+// ExecuteActions already had this bound; keep it covered so the two endpoints
+// cannot drift apart again.
+func TestExecuteActionsBoundsActionCount(t *testing.T) {
+	r := require.New(t)
+
+	rules := genesis.NewDefaultRules()
+	server, _ := newTestServer(rules)
+
+	actionBytes := dummyActionBytes(t)
+	actions := make([][]byte, int(rules.GetMaxActionsPerTx())+1)
+	for i := range actions {
+		actions[i] = actionBytes
+	}
+
+	err := server.ExecuteActions(
+		&http.Request{},
+		&ExecuteActionArgs{Actions: actions},
+		&ExecuteActionReply{},
+	)
+	r.ErrorContains(err, "exceeded max actions per simulation")
+}

api/ws/server.go

@@ -5,7 +5,9 @@ package ws
 
 import (
 	"context"
+	"errors"
 	"sync"
+	"time"
 
 	"github.com/ava-labs/avalanchego/ids"
 	"github.com/ava-labs/avalanchego/trace"
@@ -124,8 +126,8 @@ func NewWebSocketServer(
 	return w, w.s
 }
 
-// Note: no need to have a tx listener removal, this will happen when all
-// submitted transactions are cleared.
+// AddTxListener registers [c] to be notified about the result of [tx].
+// Note: cleanup relies on expiry eviction; never register a tx expiring past the validity window.
 func (w *WebSocketServer) AddTxListener(tx *chain.Transaction, c *pubsub.Connection) {
 	w.txL.Lock()
 	defer w.txL.Unlock()
@@ -141,6 +143,22 @@ func (w *WebSocketServer) AddTxListener(tx *chain.Transaction, c *pubsub.Connect
 	w.expiringTxs.Add([]*chain.Transaction{tx})
 }
 
+// RemoveTxListener unregisters [c] from the listeners for [txID], dropping the
+// entry entirely once no connection is waiting on it.
+func (w *WebSocketServer) RemoveTxListener(txID ids.ID, c *pubsub.Connection) {
+	w.txL.Lock()
+	defer w.txL.Unlock()
+
+	connections, ok := w.txListeners[txID]
+	if !ok {
+		return
+	}
+	connections.Remove(c)
+	if connections.Len() == 0 {
+		delete(w.txListeners, txID)
+	}
+}
+
 func (w *WebSocketServer) expireTx(txID ids.ID) {
 	listeners, ok := w.txListeners[txID]
 	if !ok {
@@ -228,11 +246,28 @@ func (w *WebSocketServer) MessageCallback() pubsub.Callback {
 				return
 			}
 
+			txID := tx.GetID()
+
+			// Drop txs expiring past the validity window
+			now := time.Now().UnixMilli()
+			rules := w.vm.GetRuleFactory().GetRules(now)
+			if tx.Base.Timestamp > now+rules.GetValidityWindow() {
+				w.logger.Debug("dropping tx with expiry beyond the validity window",
+					zap.Stringer("txID", txID),
+					zap.Int64("timestamp", tx.Base.Timestamp),
+				)
+				return
+			}
+
+			// Registered before Submit so that a tx accepted into a block
+			// cannot be reported before the listener exists.
 			w.AddTxListener(tx, c)
 
-			// Submit will remove from [txListeners] if it is not added
-			txID := tx.GetID()
 			if err := w.vm.Submit(ctx, []*chain.Transaction{tx})[0]; err != nil {
+				// If the tx is already in the mempool, the listener must be retained
+				if !errors.Is(err, vm.ErrNotAdded) {
+					w.RemoveTxListener(txID, c)
+				}
 				w.logger.Error("failed to submit tx",
 					zap.Stringer("txID", txID),
 					zap.Error(err),