From c2b22d395572f0a690706558bdb7fe67f34b14ce Mon Sep 17 00:00:00 2001 From: Greg Date: Sun, 25 Oct 2026 15:43:27 -0400 Subject: [PATCH] feat(draft): catch split text settings and owner-editable game ports Two problems a draft can boot fine with but break once an owner changes a setting, both in the builder's Valheim draft: - splitSettings: text settings used unquoted on the command line, directly or through ARGS="-name ${SERVER_NAME}"; ./server $ARGS, so "GBRHosting - My Server" reaches the game as several arguments. Suggests quoting or a bash array. - portSettings: game-port settings whose default isn't in defaultPorts, or that owners can change although nodes always publish the same container port. Internal ports (RCON, REST) and {PUBLIC_PORT_n} values are left out. Both run on drafts only; AUTHORING.md gets the matching rules. Co-Authored-By: Claude Opus 5.5 --- AUTHORING.md | 6 + cmd/gbrgames/draft.go | 2 + cmd/gbrgames/draftchecks.go | 250 +++++++++++++++++++++++++++++++ cmd/gbrgames/draftchecks_test.go | 107 +++++++++++++ 4 files changed, 365 insertions(+) create mode 100644 cmd/gbrgames/draftchecks.go create mode 100644 cmd/gbrgames/draftchecks_test.go diff --git a/AUTHORING.md b/AUTHORING.md index b595a5d..e21e185 100644 --- a/AUTHORING.md +++ b/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_}`. ### The container user - Run `gbrgames inspect-image -game `. 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). diff --git a/cmd/gbrgames/draft.go b/cmd/gbrgames/draft.go index 986ab96..ecde53c 100644 --- a/cmd/gbrgames/draft.go +++ b/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 diff --git a/cmd/gbrgames/draftchecks.go b/cmd/gbrgames/draftchecks.go new file mode 100644 index 0000000..5db39a0 --- /dev/null +++ b/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 +} diff --git a/cmd/gbrgames/draftchecks_test.go b/cmd/gbrgames/draftchecks_test.go new file mode 100644 index 0000000..148ea9f --- /dev/null +++ b/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) + } +}