AUTHORING.md

@@ -41,6 +41,9 @@ Know this before writing anything:
 - `defaultPorts` lists **only what players connect to** (game and query ports). RCON, REST and admin
   ports are **never** published.
 - Use `"7777/udp"` or `"8211:8211/udp"`. Every port needs its protocol.
+- Nodes always publish these container ports (only the host side moves when servers share a node).
+  So the game's own port is **not a setting**: write it into the startup command or config file. A
+  game that must announce the port players use gets `{PUBLIC_PORT_<port>}`.
 
 ### The container user
 - Run `gbrgames inspect-image -game <id>`. If the image is built around its own user (it declares
@@ -51,6 +54,9 @@ Know this before writing anything:
 - Read settings under the names the agent gives them (see above). `${RCON_PORT}` when the setting is
   `RCONPort` silently ignores the owner's value; it arrives as `RCONPORT`.
 - Use `${VAR:-default}` for anything that may be unset.
+- Quote text settings where they're used: `-name "${SERVER_NAME}"`. `ARGS="-name ${SERVER_NAME}"; ./server $ARGS`
+  splits "GBRHosting - My Server" into words; build optional arguments in a bash array instead
+  (`args=(-name "$SERVER_NAME"); [ -n "$PASSWORD" ] && args+=(-password "$PASSWORD"); ./server "${args[@]}"`).
 - If the image installs the game itself on start, make sure a failed install is retried rather than
   starting a server that isn't there (see ARK's template).
 

cmd/gbrgames/draft.go

@@ -198,6 +198,8 @@ func (d *drafter) check(answer string) []string {
 			problems = append(problems, draftCompleteness(g, t)...)
 			problems = append(problems, unreadSettings(g, d.facts)...)
 			problems = append(problems, savesOutsideVolumes(g, d.job.Report)...)
+			problems = append(problems, splitSettings(g)...)
+			problems = append(problems, portSettings(g)...)
 		}
 	}
 	return problems

cmd/gbrgames/draftchecks.go

@@ -0,0 +1,250 @@
+package main
+
+import (
+	"encoding/json"
+	"fmt"
+	"regexp"
+	"sort"
+	"strconv"
+	"strings"
+)
+
+// Checks a booting draft can still fail: things that only go wrong once an owner changes a setting, so
+// neither the validator nor a boot test with the defaults sees them.
+
+// splitSettings finds text settings that reach the server's command line unquoted, so a value with spaces
+// ("GBRHosting - My Server") arrives as several arguments and the game only sees the first word. Covers
+// direct use (-name ${SERVER_NAME}) and the common ARGS="-name ${SERVER_NAME}"; ./server $ARGS, where the
+// quotes in the assignment don't survive the unquoted $ARGS.
+func splitSettings(game map[string]any) []string {
+	startup, _ := game["defaultStartupCommand"].(string)
+	if startup == "" {
+		return nil
+	}
+	textVars := map[string]string{} // env name -> setting key
+	for key, p := range schemaProperties(game) {
+		if prop, _ := p.(map[string]any); prop["type"] == "string" {
+			textVars[envName(key)] = key
+		}
+	}
+	if len(textVars) == 0 {
+		return nil
+	}
+	s := scanShell(unwrapShell(startup))
+
+	split := map[string]string{} // setting key -> how it gets split
+	var reach func(holder string, seen map[string]bool) []string
+	reach = func(holder string, seen map[string]bool) []string {
+		if seen[holder] {
+			return nil
+		}
+		seen[holder] = true
+		var out []string
+		for _, v := range s.held[holder] {
+			if _, ok := textVars[v]; ok {
+				out = append(out, v)
+			}
+			out = append(out, reach(v, seen)...)
+		}
+		return out
+	}
+	for _, e := range s.uses {
+		if e.quoted {
+			continue
+		}
+		if key, ok := textVars[e.name]; ok {
+			split[key] = "${" + e.name + "} is not in double quotes"
+		}
+		if len(s.held[e.name]) > 0 {
+			for _, v := range reach(e.name, map[string]bool{}) {
+				if _, done := split[textVars[v]]; !done {
+					split[textVars[v]] = fmt.Sprintf("${%s} is inside $%s, which is used without double quotes", v, e.name)
+				}
+			}
+		}
+	}
+	var problems []string
+	for _, key := range sortedKeysString(split) {
+		problems = append(problems, fmt.Sprintf("setting %s is split into words on the command line (%s): a value with spaces like "+
+			"\"GBRHosting - My Server\" reaches the game as several arguments. Quote it where it is used (-name \"${%s}\"), or build "+
+			"optional arguments in a bash array: args=(-name \"$%s\"); [ -n \"$X\" ] && args+=(-x \"$X\"); ./server \"${args[@]}\"",
+			key, split[key], envName(key), envName(key)))
+	}
+	return problems
+}
+
+var shellWrap = regexp.MustCompile(`^\s*(?:ba)?sh\s+-c\s+'(.*)'\s*$`)
+
+// unwrapShell looks inside a startup command that is one bash -c '...' (ARK's), which would otherwise be
+// all single-quoted text.
+func unwrapShell(cmd string) string {
+	if m := shellWrap.FindStringSubmatch(cmd); m != nil && !strings.Contains(m[1], "'") {
+		return m[1]
+	}
+	return cmd
+}
+
+type shellUse struct {
+	name   string
+	quoted bool
+}
+
+type shellScan struct {
+	uses []shellUse          // expansions where the value is used: words, arguments
+	held map[string][]string // variable -> variables its assigned value contains
+}
+
+var (
+	assignWord = regexp.MustCompile(`^([A-Za-z_][A-Za-z0-9_]*)\+?=`)
+	varName    = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]*`)
+	nestedVar  = regexp.MustCompile(`\$\{?([A-Za-z_][A-Za-z0-9_]*)`)
+)
+
+// scanShell is just enough of a shell parser for splitSettings: quotes, $VAR / ${VAR...} expansions, and
+// assignments (NAME=..., NAME+=..., NAME=(...) arrays).
+func scanShell(cmd string) *shellScan {
+	s := &shellScan{held: map[string][]string{}}
+	var single, double, wordStart = false, false, true
+	assign, inArray := "", false
+	record := func(name string, quoted bool) {
+		switch {
+		case assign != "" && !inArray:
+			// NAME="...$X..." : no splitting yet, but $NAME carries X along
+			s.held[assign] = append(s.held[assign], name)
+		case inArray && quoted:
+			s.held[assign] = append(s.held[assign], name) // safe unless the array is expanded unquoted
+		default:
+			s.uses = append(s.uses, shellUse{name, quoted})
+		}
+	}
+	for i := 0; i < len(cmd); i++ {
+		c := cmd[i]
+		if wordStart && !single && !double {
+			if m := assignWord.FindString(cmd[i:]); m != "" {
+				assign = strings.TrimRight(strings.TrimSuffix(m, "="), "+")
+				i += len(m)
+				if i < len(cmd) && cmd[i] == '(' {
+					inArray = true
+				} else {
+					i--
+				}
+				wordStart = false
+				continue
+			}
+		}
+		wordStart = false
+		switch {
+		case c == '\\' && !single:
+			i++
+		case c == '\'' && !double:
+			single = !single
+		case c == '"' && !single:
+			double = !double
+		case c == '$' && !single && i+1 < len(cmd):
+			if cmd[i+1] == '{' {
+				end := matchingBrace(cmd, i+1)
+				body := cmd[i+2 : end]
+				name := varName.FindString(strings.TrimLeft(body, "#!"))
+				if name != "" {
+					record(name, double)
+				}
+				// Defaults can hold more expansions: ${A:-${B}}
+				for _, m := range nestedVar.FindAllStringSubmatch(body[len(name):], -1) {
+					record(m[1], double)
+				}
+				i = end
+			} else if name := varName.FindString(cmd[i+1:]); name != "" {
+				record(name, double)
+				i += len(name)
+			}
+		case single || double:
+		case inArray && c == ')':
+			inArray, assign = false, ""
+		case inArray:
+		case c == ' ' || c == '\t' || c == '\n' || c == ';' || c == '&' || c == '|' || c == '(' || c == ')':
+			assign, wordStart = "", true
+		}
+	}
+	return s
+}
+
+func matchingBrace(s string, open int) int {
+	depth := 0
+	for i := open; i < len(s); i++ {
+		switch s[i] {
+		case '{':
+			depth++
+		case '}':
+			depth--
+			if depth == 0 {
+				return i
+			}
+		}
+	}
+	return len(s) - 1
+}
+
+var (
+	portSetting = regexp.MustCompile(`(?i)(port$|(^|_)port(_|$))`)
+	notAPort    = regexp.MustCompile(`(?i)(sup|re|ex|im|trans)port$`)
+	// Reached over the Docker network, never published (AUTHORING.md)
+	internalPort = regexp.MustCompile(`(?i)rcon|rest|api|admin|telnet|web|http|metrics`)
+)
+
+// portSettings checks settings that set a port players connect to against defaultPorts. The node always
+// publishes defaultPorts' container ports (moving only the host side when servers share a node), so a
+// game listening anywhere else can't be reached: a default that isn't published is broken from the
+// start, and an editable one breaks the moment an owner changes it.
+func portSettings(game map[string]any) []string {
+	published := map[int]bool{}
+	ports, _ := game["defaultPorts"].([]any)
+	for _, p := range ports {
+		n, _ := strconv.Atoi(strings.Split(containerPort(fmt.Sprint(p)), "/")[0])
+		published[n] = true
+	}
+	defaults, _ := game["defaultConfig"].(map[string]any)
+	props := schemaProperties(game)
+	var problems []string
+	keys := make([]string, 0, len(defaults))
+	for k := range defaults {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+	for _, key := range keys {
+		if !portSetting.MatchString(key) || notAPort.MatchString(key) || internalPort.MatchString(key) {
+			continue
+		}
+		port, ok := portNumber(defaults[key])
+		if !ok {
+			continue // {PUBLIC_PORT_n} and other placeholders are filled per server
+		}
+		_, editable := props[key]
+		switch {
+		case !published[port]:
+			problems = append(problems, fmt.Sprintf("setting %s defaults to port %d, which defaultPorts does not publish: players can't reach it. "+
+				"Use one of the published ports, or add this port to defaultPorts if players connect to it", key, port))
+		case editable:
+			problems = append(problems, fmt.Sprintf("setting %s lets owners change the port, but the node always publishes container port %d "+
+				"(defaultPorts), so any other value makes the server unreachable. Don't make it a setting: remove %s from "+
+				"defaultConfig and configSchema and write %d directly in the startup command or config file. If the game must "+
+				"announce the port players use, give it {PUBLIC_PORT_%d} instead", key, port, key, port, port))
+		}
+	}
+	return problems
+}
+
+func portNumber(v any) (int, bool) {
+	var s string
+	switch n := v.(type) {
+	case json.Number:
+		s = n.String()
+	case string:
+		s = n
+	case int, int64, float64:
+		s = fmt.Sprint(n)
+	default:
+		return 0, false
+	}
+	p, err := strconv.Atoi(strings.TrimSpace(s))
+	return p, err == nil && p > 0 && p < 65536
+}

cmd/gbrgames/draftchecks_test.go

@@ -0,0 +1,107 @@
+package main
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+)
+
+func textSettings(keys ...string) map[string]any {
+	props := map[string]any{}
+	for _, k := range keys {
+		props[k] = map[string]any{"type": "string"}
+	}
+	return map[string]any{"properties": props}
+}
+
+func TestSplitSettingsCatchesTheValheimDraft(t *testing.T) {
+	// What the builder drafted for Valheim: quoted in the assignment, split by the unquoted $ARGS
+	game := map[string]any{
+		"defaultStartupCommand": `ARGS="-name ${SERVER_NAME:-Valheim Server} -port 2456 -world ${WORLD_NAME:-Dedicated}"; ` +
+			`[ -n "$PASSWORD" ] && ARGS="$ARGS -password $PASSWORD"; [ "$CROSSPLAY" = "true" ] && ARGS="$ARGS -crossplay"; ` +
+			`HOME=/server ./valheim_server.x86_64 $ARGS`,
+		"configSchema": textSettings("ServerName", "WorldName", "Password"),
+	}
+	problems := splitSettings(game)
+	got := strings.Join(problems, "\n")
+	if len(problems) != 3 {
+		t.Fatalf("want ServerName, WorldName and Password, got:\n%s", got)
+	}
+	for _, key := range []string{"setting Password", "setting ServerName", "setting WorldName", "inside $ARGS"} {
+		if !strings.Contains(got, key) {
+			t.Errorf("missing %q in:\n%s", key, got)
+		}
+	}
+}
+
+func TestSplitSettingsAcceptsQuotedAndArrays(t *testing.T) {
+	for _, startup := range []string{
+		`./valheim_server.x86_64 -name "${SERVER_NAME}" -world "$WORLD_NAME" -port 2456`,
+		`args=(-name "$SERVER_NAME" -world "${WORLD_NAME:-Dedicated}"); [ -n "$PASSWORD" ] && args+=(-password "$PASSWORD"); ./valheim_server.x86_64 "${args[@]}"`,
+		`bash -c 'exec ./server -name "$SERVER_NAME"'`,
+	} {
+		game := map[string]any{"defaultStartupCommand": startup, "configSchema": textSettings("ServerName", "WorldName", "Password")}
+		if p := splitSettings(game); len(p) != 0 {
+			t.Errorf("%s\n  flagged: %v", startup, p)
+		}
+	}
+}
+
+func TestSplitSettingsDirectAndArrayMisuse(t *testing.T) {
+	cases := map[string]string{
+		`./server -name ${SERVER_NAME}`:                     "is not in double quotes",
+		`bash -c 'exec ./server -name $SERVER_NAME'`:        "is not in double quotes",
+		`args=(-name "$SERVER_NAME"); ./server ${args[@]}`:  "inside $args",
+		`args=(-name $SERVER_NAME); ./server "${args[@]}"`:  "is not in double quotes",
+		`export OPTS="-name ${SERVER_NAME}" && ./run $OPTS`: "inside $OPTS",
+	}
+	for startup, want := range cases {
+		game := map[string]any{"defaultStartupCommand": startup, "configSchema": textSettings("ServerName")}
+		got := strings.Join(splitSettings(game), "\n")
+		if !strings.Contains(got, want) {
+			t.Errorf("%s\n  want %q, got %q", startup, want, got)
+		}
+	}
+	// Numbers and booleans can't contain spaces
+	game := map[string]any{"defaultStartupCommand": `./server -port ${GAME_PORT}`, "configSchema": map[string]any{
+		"properties": map[string]any{"GamePort": map[string]any{"type": "integer"}}}}
+	if p := splitSettings(game); len(p) != 0 {
+		t.Errorf("integer setting flagged: %v", p)
+	}
+}
+
+func TestPortSettings(t *testing.T) {
+	game := map[string]any{
+		"defaultPorts": []any{"2456/udp", "2457/udp"},
+		"defaultConfig": map[string]any{
+			"GamePort":   json.Number("2456"),
+			"QueryPort":  json.Number("27015"),
+			"RCONPort":   json.Number("25575"),
+			"PublicPort": "{PUBLIC_PORT_2456}",
+			"Transport":  "udp",
+			"Export":     json.Number("9"),
+		},
+		"configSchema": map[string]any{"properties": map[string]any{
+			"GamePort": map[string]any{"type": "integer"}, "QueryPort": map[string]any{"type": "integer"},
+		}},
+	}
+	got := strings.Join(portSettings(game), "\n")
+	if !strings.Contains(got, "setting GamePort lets owners change the port") || !strings.Contains(got, "write 2456 directly") {
+		t.Errorf("editable published port not caught:\n%s", got)
+	}
+	if !strings.Contains(got, "setting QueryPort defaults to port 27015, which defaultPorts does not publish") {
+		t.Errorf("unpublished port not caught:\n%s", got)
+	}
+	for _, key := range []string{"RCONPort", "PublicPort", "Transport", "Export"} {
+		if strings.Contains(got, "setting "+key+" ") {
+			t.Errorf("%s should be left alone:\n%s", key, got)
+		}
+	}
+
+	// A published port written by the template itself, not offered to owners, is fine
+	delete(game["configSchema"].(map[string]any)["properties"].(map[string]any), "GamePort")
+	delete(game["defaultConfig"].(map[string]any), "QueryPort")
+	if p := portSettings(game); len(p) != 0 {
+		t.Errorf("got %v", p)
+	}
+}