Repository: nats-io/nats-server (go)

Issue:
`server_name` with Space characters breaks reply routing for leaf nodes.
### Observed behavior

When setting `server_name` to a string with a " " in it, like "My Server", results in replies not being delivered to a requestor, when the requestor lives on the hub and the replier living on a leaf node.

### Expected behavior

I would expect it to work as intended or throw a validation error/warning when trying to connect as a leaf node.

### Server and client version

`nats-server` - 2.10.17
`nats-cli` - (devel) `main`

### Host environment

MacOS on M1 Mac studio for leaf node, Synadia Cloud for hub.

### Steps to reproduce

1. Create a leaf node that connects into a hub (any sort of auth works fine here)
2. Set the `server_name` of the leaf node to "My Server" (or anything with a space on it)
3. Add a replier to the leaf node `nats reply hello.world --echo --context leaf`
4. Make a request from the hub `nats req hello.world hihi --context hub`
5. Observe that the request is received by the leaf node client, but the reply is never sent.

When the `server_name` has no spaces, I don't encounter this issue

Pull request:
[FIXED/CHANGED] Reject server/cluster/gateway names with spaces
Having cluster name with spaces in the context of a leaf node could cause problems when a subscription on the leaf node would be propagated in the cluster's hub.

Since when a gateway is specified the name of the gateway needs to match the name of the cluster, the restriction of "no spaces" is added to gateway name.

Finally, in the case of the leafnode, if no cluster name is specified the server name is used, so also applying the restriction to the server name.

Resolves #5633

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

Patch:
diff --git a/server/errors.go b/server/errors.go
index 8efa7ac0..62c054cd 100644
--- a/server/errors.go
+++ b/server/errors.go
@@ -153,6 +153,9 @@ var (
 	// Gateway's name.
 	ErrWrongGateway = errors.New("wrong gateway")
 
+	// ErrGatewayNameHasSpaces signals that the gateway name contains spaces, which is not allowed.
+	ErrGatewayNameHasSpaces = errors.New("gateway name cannot contain spaces")
+
 	// ErrNoSysAccount is returned when an attempt to publish or subscribe is made
 	// when there is no internal system account defined.
 	ErrNoSysAccount = errors.New("system account not setup")
@@ -163,6 +166,9 @@ var (
 	// ErrServerNotRunning is used to signal an error that a server is not running.
 	ErrServerNotRunning = errors.New("server is not running")
 
+	// ErrServerNameHasSpaces signals that the server name contains spaces, which is not allowed.
+	ErrServerNameHasSpaces = errors.New("server name cannot contain spaces")
+
 	// ErrBadMsgHeader signals the parser detected a bad message header
 	ErrBadMsgHeader = errors.New("bad message header detected")
 
@@ -180,6 +186,9 @@ var (
 	// ErrClusterNameRemoteConflict signals that a remote server has a different cluster name.
 	ErrClusterNameRemoteConflict = errors.New("cluster name from remote server conflicts")
 
+	// ErrClusterNameHasSpaces signals that the cluster name contains spaces, which is not allowed.
+	ErrClusterNameHasSpaces = errors.New("cluster name cannot contain spaces")
+
 	// ErrMalformedSubject is returned when a subscription is made with a subject that does not conform to subject rules.
 	ErrMalformedSubject = errors.New("malformed subject")
 
diff --git a/server/gateway.go b/server/gateway.go
index d8d434a2..be175467 100644
--- a/server/gateway.go
+++ b/server/gateway.go
@@ -18,12 +18,14 @@ import (
 	"crypto/sha256"
 	"crypto/tls"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"math/rand"
 	"net"
 	"net/url"
 	"sort"
 	"strconv"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -299,17 +301,20 @@ func (r *RemoteGatewayOpts) clone() *RemoteGatewayOpts {
 
 // Ensure that gateway is properly configured.
 func validateGatewayOptions(o *Options) error {
-	if o.Gateway.Name == "" && o.Gateway.Port == 0 {
+	if o.Gateway.Name == _EMPTY_ && o.Gateway.Port == 0 {
 		return nil
 	}
-	if o.Gateway.Name == "" {
-		return fmt.Errorf("gateway has no name")
+	if o.Gateway.Name == _EMPTY_ {
+		return errors.New("gateway has no name")
+	}
+	if strings.Contains(o.Gateway.Name, " ") {
+		return ErrGatewayNameHasSpaces
 	}
 	if o.Gateway.Port == 0 {
 		return fmt.Errorf("gateway %q has no port specified (select -1 for random port)", o.Gateway.Name)
 	}
 	for i, g := range o.Gateway.Gateways {
-		if g.Name == "" {
+		if g.Name == _EMPTY_ {
 			return fmt.Errorf("gateway in the list %d has no name", i)
 		}
 		if len(g.URLs) == 0 {
diff --git a/server/leafnode.go b/server/leafnode.go
index f8624406..61ba2207 100644
--- a/server/leafnode.go
+++ b/server/leafnode.go
@@ -1288,6 +1288,13 @@ func (c *client) processLeafnodeInfo(info *Info) {
 			c.closeConnection(WrongPort)
 			return
 		}
+		// Reject a cluster that contains spaces.
+		if info.Cluster != _EMPTY_ && strings.Contains(info.Cluster, " ") {
+			c.mu.Unlock()
+			c.sendErrAndErr(ErrClusterNameHasSpaces.Error())
+			c.closeConnection(ProtocolViolation)
+			return
+		}
 		// Capture a nonce here.
 		c.nonce = []byte(info.Nonce)
 		if info.TLSRequired && didSolicit {
@@ -1779,6 +1786,13 @@ func (c *client) processLeafNodeConnect(s *Server, arg []byte, lang string) erro
 		return err
 	}
 
+	// Reject a cluster that contains spaces.
+	if proto.Cluster != _EMPTY_ && strings.Contains(proto.Cluster, " ") {
+		c.sendErrAndErr(ErrClusterNameHasSpaces.Error())
+		c.closeConnection(ProtocolViolation)
+		return ErrClusterNameHasSpaces
+	}
+
 	// Check for cluster name collisions.
 	if cn := s.cachedClusterName(); cn != _EMPTY_ && proto.Cluster != _EMPTY_ && proto.Cluster == cn {
 		c.sendErrAndErr(ErrLeafNodeHasSameClusterName.Error())
diff --git a/server/opts.go b/server/opts.go
index e717f26e..08212f43 100644
--- a/server/opts.go
+++ b/server/opts.go
@@ -917,7 +917,13 @@ func (o *Options) processConfigFileLine(k string, v any, errors *[]error, warnin
 	case "port":
 		o.Port = int(v.(int64))
 	case "server_name":
-		o.ServerName = v.(string)
+		sn := v.(string)
+		if strings.Contains(sn, " ") {
+			err := &configErr{tk, ErrServerNameHasSpaces.Error()}
+			*errors = append(*errors, err)
+			return
+		}
+		o.ServerName = sn
 	case "host", "net":
 		o.Host = v.(string)
 	case "debug":
@@ -1690,7 +1696,13 @@ func parseCluster(v any, opts *Options, errors *[]error, warnings *[]error) erro
 		tk, mv = unwrapValue(mv, &lt)
 		switch strings.ToLower(mk) {
 		case "name":
-			opts.Cluster.Name = mv.(string)
+			cn := mv.(string)
+			if strings.Contains(cn, " ") {
+				err := &configErr{tk, ErrClusterNameHasSpaces.Error()}
+				*errors = append(*errors, err)
+				continue
+			}
+			opts.Cluster.Name = cn
 		case "listen":
 			hp, err := parseListen(mv)
 			if err != nil {
@@ -1920,7 +1932,13 @@ func parseGateway(v any, o *Options, errors *[]error, warnings *[]error) error {
 		tk, mv = unwrapValue(mv, &lt)
 		switch strings.ToLower(mk) {
 		case "name":
-			o.Gateway.Name = mv.(string)
+			gn := mv.(string)
+			if strings.Contains(gn, " ") {
+				err := &configErr{tk, ErrGatewayNameHasSpaces.Error()}
+				*errors = append(*errors, err)
+				continue
+			}
+			o.Gateway.Name = gn
 		case "listen":
 			hp, err := parseListen(mv)
 			if err != nil {
diff --git a/server/server.go b/server/server.go
index be21987b..0479f319 100644
--- a/server/server.go
+++ b/server/server.go
@@ -1004,6 +1004,9 @@ func (s *Server) ClientURL() string {
 }
 
 func validateCluster(o *Options) error {
+	if o.Cluster.Name != _EMPTY_ && strings.Contains(o.Cluster.Name, " ") {
+		return ErrClusterNameHasSpaces
+	}
 	if o.Cluster.Compression.Mode != _EMPTY_ {
 		if err := validateAndNormalizeCompressionOption(&o.Cluster.Compression, CompressionS2Fast); err != nil {
 			return err
@@ -1013,8 +1016,9 @@ func validateCluster(o *Options) error {
 		return fmt.Errorf("cluster: %v", err)
 	}
 	// Check that cluster name if defined matches any gateway name.
-	if o.Gateway.Name != "" && o.Gateway.Name != o.Cluster.Name {
-		if o.Cluster.Name != "" {
+	// Note that we have already verified that the gateway name does not have spaces.
+	if o.Gateway.Name != _EMPTY_ && o.Gateway.Name != o.Cluster.Name {
+		if o.Cluster.Name != _EMPTY_ {
 			return ErrClusterNameConfigConflict
 		}
 		// Set this here so we do not consider it dynamic.
@@ -1055,6 +1059,9 @@ func validateOptions(o *Options) error {
 		return fmt.Errorf("max_payload (%v) cannot be higher than max_pending (%v)",
 			o.MaxPayload, o.MaxPending)
 	}
+	if o.ServerName != _EMPTY_ && strings.Contains(o.ServerName, " ") {
+		return errors.New("server name cannot contain spaces")
+	}
 	// Check that the trust configuration is correct.
 	if err := validateTrustedOperators(o); err != nil {
 		return err


Test patch:
diff --git a/server/events_test.go b/server/events_test.go
index 0d52ca20..f27fe86b 100644
--- a/server/events_test.go
+++ b/server/events_test.go
@@ -103,14 +103,14 @@ func runTrustedCluster(t *testing.T) (*Server, *Options, *Server, *Options, nkey
 	mr.Store(apub, jwt)
 
 	optsA := DefaultOptions()
-	optsA.Cluster.Name = "TEST CLUSTER 22"
+	optsA.Cluster.Name = "TEST_CLUSTER_22"
 	optsA.Cluster.Host = "127.0.0.1"
 	optsA.TrustedKeys = []string{pub}
 	optsA.AccountResolver = mr
 	optsA.SystemAccount = apub
 	optsA.ServerName = "A_SRV"
 	// Add in dummy gateway
-	optsA.Gateway.Name = "TEST CLUSTER 22"
+	optsA.Gateway.Name = "TEST_CLUSTER_22"
 	optsA.Gateway.Host = "127.0.0.1"
 	optsA.Gateway.Port = -1
 	optsA.gatewaysSolicitDelay = 30 * time.Second
@@ -1876,7 +1876,7 @@ func TestServerEventsStatsZ(t *testing.T) {
 	if m.Server.ID != sa.ID() {
 		t.Fatalf("Did not match IDs")
 	}
-	if m.Server.Cluster != "TEST CLUSTER 22" {
+	if m.Server.Cluster != "TEST_CLUSTER_22" {
 		t.Fatalf("Did not match cluster name")
 	}
 	if m.Server.Version != VERSION {
@@ -2978,11 +2978,11 @@ func TestServerEventsPingMonitorz(t *testing.T) {
 			[]string{"now", "routes"}},
 		{"ROUTEZ", json.RawMessage(`{"name":""}`), &Routez{},
 			[]string{"now", "routes"}},
-		{"ROUTEZ", json.RawMessage(`{"cluster":"TEST CLUSTER 22"}`), &Routez{},
+		{"ROUTEZ", json.RawMessage(`{"cluster":"TEST_CLUSTER_22"}`), &Routez{},
 			[]string{"now", "routes"}},
 		{"ROUTEZ", json.RawMessage(`{"cluster":"CLUSTER"}`), &Routez{},
 			[]string{"now", "routes"}},
-		{"ROUTEZ", json.RawMessage(`{"cluster":"TEST CLUSTER 22", "subscriptions":true}`), &Routez{},
+		{"ROUTEZ", json.RawMessage(`{"cluster":"TEST_CLUSTER_22", "subscriptions":true}`), &Routez{},
 			[]string{"now", "routes"}},
 
 		{"JSZ", nil, &JSzOptions{}, []string{"now", "disabled"}},
@@ -3083,8 +3083,8 @@ func TestGatewayNameClientInfo(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Could not parse INFO json: %v\n", err)
 	}
-	if info.Cluster != "TEST CLUSTER 22" {
-		t.Fatalf("Expected a cluster name of 'TEST CLUSTER 22', got %q", info.Cluster)
+	if info.Cluster != "TEST_CLUSTER_22" {
+		t.Fatalf("Expected a cluster name of 'TEST_CLUSTER_22', got %q", info.Cluster)
 	}
 }
 
diff --git a/server/server_test.go b/server/server_test.go
index 08789a20..45ea9f93 100644
--- a/server/server_test.go
+++ b/server/server_test.go
@@ -2150,3 +2150,53 @@ func TestServerConfigLastLineComments(t *testing.T) {
 	require_NoError(t, err)
 	defer nc.Close()
 }
+
+func TestServerClusterAndGatewayNameNoSpace(t *testing.T) {
+	conf := createConfFile(t, []byte(`
+		port: -1
+		server_name: "my server"
+	`))
+	_, err := ProcessConfigFile(conf)
+	require_Error(t, err, ErrServerNameHasSpaces)
+
+	o := DefaultOptions()
+	o.ServerName = "my server"
+	_, err = NewServer(o)
+	require_Error(t, err, ErrServerNameHasSpaces)
+
+	conf = createConfFile(t, []byte(`
+		port: -1
+		server_name: "myserver"
+		cluster {
+			port: -1
+			name: "my cluster"
+		}
+	`))
+	_, err = ProcessConfigFile(conf)
+	require_Error(t, err, ErrClusterNameHasSpaces)
+
+	o = DefaultOptions()
+	o.Cluster.Name = "my cluster"
+	o.Cluster.Port = -1
+	_, err = NewServer(o)
+	require_Error(t, err, ErrClusterNameHasSpaces)
+
+	conf = createConfFile(t, []byte(`
+		port: -1
+		server_name: "myserver"
+		gateway {
+			port: -1
+			name: "my gateway"
+		}
+	`))
+	_, err = ProcessConfigFile(conf)
+	require_Error(t, err, ErrGatewayNameHasSpaces)
+
+	o = DefaultOptions()
+	o.Cluster.Name = _EMPTY_
+	o.Cluster.Port = 0
+	o.Gateway.Name = "my gateway"
+	o.Gateway.Port = -1
+	_, err = NewServer(o)
+	require_Error(t, err, ErrGatewayNameHasSpaces)
+}
diff --git a/test/leafnode_test.go b/test/leafnode_test.go
index b1bfb132..fb3dff89 100644
--- a/test/leafnode_test.go
+++ b/test/leafnode_test.go
@@ -4426,3 +4426,27 @@ func TestLeafnodeHeaders(t *testing.T) {
 		t.Fatalf("leaf msg header is empty")
 	}
 }
+
+func TestLeafNodeClusterNameWithSpacesRejected(t *testing.T) {
+	s, opts := runLeafServer()
+	defer s.Shutdown()
+
+	lc := createLeafConn(t, opts.LeafNode.Host, opts.LeafNode.Port)
+	defer lc.Close()
+
+	checkInfoMsg(t, lc)
+	sendProto(t, lc, "CONNECT {\"cluster\":\"my cluster\"}\r\n")
+	expect := expectCommand(t, lc)
+	expect(errRe)
+	expectDisconnect(t, lc)
+
+	lc = createLeafConn(t, opts.LeafNode.Host, opts.LeafNode.Port)
+	defer lc.Close()
+	leafSend, leafExpect := setupLeaf(t, lc, 1)
+	// The accept side does expect an INFO from an handshake,
+	// so it will "ignore" the first one.
+	leafSend("INFO {\"server\":\"server\"}\r\n")
+	leafSend("INFO {\"cluster\":\"my cluster\"}\r\n")
+	leafExpect(errRe)
+	expectDisconnect(t, lc)
+}