Validate Minecraft resource keys (minekube#892)

Author: robinbraemer

pkg/edition/java/cookie/internal.go

@@ -4,14 +4,14 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"strings"
 	"sync"
 
 	"github.com/robinbraemer/event"
 	"go.minekube.com/common/minecraft/key"
 	"go.minekube.com/gate/pkg/edition/java/internal/methods"
 	pkt "go.minekube.com/gate/pkg/edition/java/proto/packet/cookie"
 	"go.minekube.com/gate/pkg/edition/java/proto/state/states"
+	protoutils "go.minekube.com/gate/pkg/edition/java/proto/util"
 
 	"go.minekube.com/gate/pkg/edition/java/proto/version"
 	"go.minekube.com/gate/pkg/edition/java/proxy"
@@ -25,13 +25,7 @@ func validate(c *Cookie, cli Client) error {
 }
 
 func validateKey(key key.Key) error {
-	if key == nil {
-		return errors.New("key is nil")
-	}
-	if strings.TrimSpace(key.String()) == "" {
-		return errors.New("empty key")
-	}
-	return nil
+	return protoutils.ValidateKey(key)
 }
 
 func validateCookie(c *Cookie) error {

pkg/edition/java/cookie/internal_test.go

@@ -0,0 +1,22 @@
+package cookie
+
+import (
+	"testing"
+
+	"go.minekube.com/common/minecraft/key"
+)
+
+func TestValidateKeyRejectsInvalidResourceLocations(t *testing.T) {
+	for _, invalid := range []key.Key{
+		key.New("MineKube", "cookie"),
+		key.New("minecraft", "BadCookie"),
+		key.New("mine kube", "cookie"),
+		key.New("..", "cookie"),
+	} {
+		t.Run(invalid.String(), func(t *testing.T) {
+			if err := validateKey(invalid); err == nil {
+				t.Fatalf("validateKey(%q) succeeded, want error", invalid)
+			}
+		})
+	}
+}

pkg/edition/java/proto/util/reader.go

@@ -437,23 +437,50 @@ func ReadUnixMilli(rd io.Reader) (time.Time, error) {
 //
 //
 
-const defaultKeySeparator = ":"
+// ValidateKey verifies that a key is a valid Minecraft resource location.
+func ValidateKey(k key.Key) error {
+	if k == nil {
+		return errors.New("key is nil")
+	}
+	if k.Namespace() == ".." {
+		return fmt.Errorf("invalid key %q: namespace must not be ..", k.String())
+	}
+	if !key.NamespaceValid(k.Namespace()) || !key.ValueValid(k.Value()) {
+		return fmt.Errorf("invalid key %q", k.String())
+	}
+	return nil
+}
 
 // ReadKey reads a standard Mojang Text namespaced:key from the reader.
 func ReadKey(rd io.Reader) (key.Key, error) {
 	str, err := ReadString(rd)
 	if err != nil {
 		return nil, err
 	}
-	parts := strings.SplitN(str, defaultKeySeparator, 2)
-	if len(parts) != 2 {
-		return nil, errors.New("invalid key format")
+	k := parseIdentifierKey(str)
+	if err := ValidateKey(k); err != nil {
+		return nil, err
+	}
+	return k, nil
+}
+
+func parseIdentifierKey(str string) key.Key {
+	namespace := key.MinecraftNamespace
+	value := str
+	if separatorIndex := strings.IndexByte(str, ':'); separatorIndex >= 0 {
+		value = str[separatorIndex+1:]
+		if separatorIndex != 0 {
+			namespace = str[:separatorIndex]
+		}
 	}
-	return key.New(parts[0], parts[1]), nil
+	return key.New(namespace, value)
 }
 
 // WriteKey writes a standard Mojang Text namespaced:key to the writer.
 func WriteKey(wr io.Writer, k key.Key) error {
+	if err := ValidateKey(k); err != nil {
+		return err
+	}
 	return WriteString(wr, k.String())
 }
 

pkg/edition/java/proto/util/writer_test.go

@@ -4,8 +4,68 @@ import (
 	"bytes"
 	"math"
 	"testing"
+
+	"go.minekube.com/common/minecraft/key"
 )
 
+func TestReadKeyRejectsInvalidResourceLocations(t *testing.T) {
+	for _, raw := range []string{
+		"MineKube:cookie",
+		"minecraft:BadCookie",
+		"mine kube:cookie",
+		"..:cookie",
+	} {
+		t.Run(raw, func(t *testing.T) {
+			var buf bytes.Buffer
+			if err := WriteString(&buf, raw); err != nil {
+				t.Fatalf("failed to write raw key string: %v", err)
+			}
+
+			if got, err := ReadKey(&buf); err == nil {
+				t.Fatalf("ReadKey(%q) = %q, want error", raw, got)
+			}
+		})
+	}
+}
+
+func TestReadKeyDefaultsMissingNamespaceToMinecraft(t *testing.T) {
+	for raw, want := range map[string]key.Key{
+		"cookie":  key.New(key.MinecraftNamespace, "cookie"),
+		":cookie": key.New(key.MinecraftNamespace, "cookie"),
+	} {
+		t.Run(raw, func(t *testing.T) {
+			var buf bytes.Buffer
+			if err := WriteString(&buf, raw); err != nil {
+				t.Fatalf("failed to write raw key string: %v", err)
+			}
+
+			got, err := ReadKey(&buf)
+			if err != nil {
+				t.Fatalf("ReadKey(%q) returned error: %v", raw, err)
+			}
+			if got.String() != want.String() {
+				t.Fatalf("ReadKey(%q) = %q, want %q", raw, got, want)
+			}
+		})
+	}
+}
+
+func TestWriteKeyRejectsInvalidResourceLocations(t *testing.T) {
+	for _, invalid := range []key.Key{
+		key.New("MineKube", "cookie"),
+		key.New("minecraft", "BadCookie"),
+		key.New("mine kube", "cookie"),
+		key.New("..", "cookie"),
+	} {
+		t.Run(invalid.String(), func(t *testing.T) {
+			var buf bytes.Buffer
+			if err := WriteKey(&buf, invalid); err == nil {
+				t.Fatalf("WriteKey(%q) succeeded, want error", invalid)
+			}
+		})
+	}
+}
+
 // TestWriteBytes17_NonExtended tests WriteBytes17 with allowExtended=false
 // This is the case that was failing for 1.7.x clients with encryption requests
 func TestWriteBytes17_NonExtended(t *testing.T) {