fix(tools): reject case-variant arg keys and ref-shaped branch names

parseArgs now refuses, before any request: two keys equal under strings.EqualFold in any object (exact duplicates included), and a top-level key that differs from a schema field only in case. Without this, encoding/json's case-insensitive last-wins match let {"branch":"feat/x","Branch":"main"} write to main while a client-side hook read feat/x. file_write_branch (branch) and tbd_ship (branch and base) validate the branch name: ^[A-Za-z0-9][A-Za-z0-9._/-]*$ plus no refs/ prefix, HEAD, "..", "//", "/.", trailing "/" or ".", or ".lock". refs/heads/main otherwise created refs/heads/refs/heads/main and wedged API writes to main. tbd_ship's base is the protection-probe key, so a refs/-prefixed base would also have read as unprotected. Closes #65 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-05 06:58:31 +02:00
parent 9396b4daaa
commit ae50501ba3
4 changed files with 243 additions and 2 deletions
@@ -63,6 +63,9 @@ func (t *FileWriteBranch) Call(ctx context.Context, raw json.RawMessage) (json.R
 	if args.Branch == "" {
 		return nil, fmt.Errorf("branch is required: %w", gitea.ErrValidation)
 	}
+	if err := ValidateBranchName(args.Branch); err != nil {
+		return nil, err
+	}
 	if args.Message == "" {
 		return nil, fmt.Errorf("message is required: %w", gitea.ErrValidation)
 	}
@@ -0,0 +1,110 @@
+package tools_test
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"sync/atomic"
+	"testing"
+
+	"git.d-ma.be/mathias/gitea-mcp/internal/allowlist"
+	"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
+	"git.d-ma.be/mathias/gitea-mcp/internal/tools"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// countingServer fails nothing by itself; it records how many requests reached
+// it so a test can assert that a rejected call never touched Gitea.
+func countingServer(t *testing.T) (*httptest.Server, *atomic.Int32) {
+	t.Helper()
+	var n atomic.Int32
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		n.Add(1)
+		w.WriteHeader(http.StatusTeapot)
+	}))
+	t.Cleanup(srv.Close)
+	return srv, &n
+}
+
+// #65 (1): encoding/json matches keys case-insensitively and the last one wins,
+// so a client-side check reading `branch` could see a different value from the
+// one the server acts on. Any two keys equal under strings.EqualFold, and any
+// key that differs from a schema field only in case, are rejected before a
+// request is made.
+func TestParseArgsRejectsCaseVariantKeys(t *testing.T) {
+	cases := map[string]string{
+		"issue repro: branch + Branch": `{"owner":"probe","repo":"t","name":"t","path":"case8.txt","content":"x","message":"m","branch":"feat/y8","Branch":"main"}`,
+		"exact duplicate key":          `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","branch":"feat/y8","branch":"main"}`,
+		"case-only schema mismatch":    `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","BRANCH":"main"}`,
+		"nested object duplicate":      `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","branch":"feat/y8","extra":{"a":1,"A":2}}`,
+	}
+	for name, raw := range cases {
+		t.Run(name, func(t *testing.T) {
+			srv, n := countingServer(t)
+			tool := tools.NewFileWriteBranch(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
+			_, err := tool.Call(context.Background(), json.RawMessage(raw))
+			require.Error(t, err)
+			assert.ErrorIs(t, err, gitea.ErrValidation)
+			assert.Zero(t, n.Load(), "no request may reach gitea")
+		})
+	}
+}
+
+// #65 (2): `refs/heads/main` as a branch makes Gitea create
+// refs/heads/refs/heads/main, after which every API write to main 500s.
+// Branch names that can create such refs are rejected before any request.
+func TestBranchNameValidationRejectsBeforeRequest(t *testing.T) {
+	bad := []string{
+		"refs/heads/main", "refs/tags/v1", "a..b", "feat x", "feat\tx", "feat/",
+		"/feat", "-feat", ".feat", "feat//x", "feat/.x", "feat.lock", "feat.", "HEAD", "feat@{1}", "",
+	}
+	for _, b := range bad {
+		t.Run("file_write_branch "+b, func(t *testing.T) {
+			srv, n := countingServer(t)
+			tool := tools.NewFileWriteBranch(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
+			args, _ := json.Marshal(map[string]string{
+				"owner": "probe", "repo": "t", "path": "p", "content": "x", "message": "m", "branch": b,
+			})
+			_, err := tool.Call(context.Background(), args)
+			require.Error(t, err)
+			assert.ErrorIs(t, err, gitea.ErrValidation)
+			assert.Zero(t, n.Load(), "no request may reach gitea")
+		})
+		if b == "" {
+			continue // tbd_ship derives a branch when empty
+		}
+		t.Run("tbd_ship "+b, func(t *testing.T) {
+			srv, n := countingServer(t)
+			tool := tools.NewTBDShip(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
+			args, _ := json.Marshal(map[string]string{
+				"owner": "probe", "repo": "t", "path": "p", "content": "x", "message": "m", "branch": b,
+			})
+			_, err := tool.Call(context.Background(), args)
+			require.Error(t, err)
+			assert.ErrorIs(t, err, gitea.ErrValidation)
+			assert.Zero(t, n.Load(), "no request may reach gitea")
+		})
+	}
+}
+
+// tbd_ship's base is the branch-protection probe key: `refs/heads/main` would
+// read as unprotected, so it is held to the same rule as the head branch.
+func TestTBDShipRejectsRefsPrefixedBase(t *testing.T) {
+	srv, n := countingServer(t)
+	tool := tools.NewTBDShip(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"probe"}))
+	_, err := tool.Call(context.Background(), json.RawMessage(
+		`{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","base":"refs/heads/main"}`))
+	require.Error(t, err)
+	assert.ErrorIs(t, err, gitea.ErrValidation)
+	assert.Zero(t, n.Load(), "no request may reach gitea")
+}
+
+func TestBranchNameValidationAcceptsOrdinaryNames(t *testing.T) {
+	for _, b := range []string{"main", "feat/y8", "tbd/fix-abc-1234abcd", "hold/mail-chat-infra-21", "v1.2_x"} {
+		t.Run(b, func(t *testing.T) {
+			assert.NoError(t, tools.ValidateBranchName(b))
+		})
+	}
+}
@@ -160,6 +160,13 @@ func (t *TBDShip) Call(ctx context.Context, raw json.RawMessage) (json.RawMessag
 	if branch == "" {
 		branch = deriveBranch(args.Message, args.Path, args.Content)
 	}
+	// Both names reach a ref: base is also the protection-probe key, so a
+	// `refs/heads/main` base would read as unprotected (#65).
+	for _, b := range []string{base, branch} {
+		if err := ValidateBranchName(b); err != nil {
+			return nil, err
+		}
+	}
 	deleteBranch := true
 	if args.DeleteBranch != nil {
 		deleteBranch = *args.DeleteBranch
@@ -1,9 +1,12 @@
 package tools

 import (
+	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"reflect"
+	"regexp"
 	"strings"

 	"git.d-ma.be/mathias/gitea-mcp/internal/gitea"
@@ -22,12 +25,130 @@ func parseArgs(raw json.RawMessage, dst any) error {
 		if err := json.Unmarshal([]byte("{}"), dst); err != nil {
 			return err
 		}
-	} else if err := json.Unmarshal(normalizeAliases(raw), dst); err != nil {
-		return err
+	} else {
+		// Reject ambiguity BEFORE alias normalisation: re-marshalling through a
+		// map would otherwise silently collapse exact duplicates (#65).
+		if err := rejectAmbiguousKeys(raw, dst); err != nil {
+			return err
+		}
+		if err := json.Unmarshal(normalizeAliases(raw), dst); err != nil {
+			return err
+		}
 	}
 	return validateRequiredIdentifiers(dst)
 }

+// rejectAmbiguousKeys refuses input whose meaning depends on encoding/json's
+// case-insensitive, last-key-wins matching (#65). A client-side check that reads
+// `branch` must see the value the server acts on, so: no object at any depth may
+// carry two keys equal under strings.EqualFold (exact duplicates included), and
+// no top-level key may differ from a field of dst only in case.
+func rejectAmbiguousKeys(raw json.RawMessage, dst any) error {
+	dec := json.NewDecoder(bytes.NewReader(raw))
+	if err := scanJSONValue(dec); err != nil {
+		if errors.Is(err, gitea.ErrValidation) {
+			return err
+		}
+		return nil // malformed JSON: let json.Unmarshal report it as before
+	}
+	var top map[string]json.RawMessage
+	if err := json.Unmarshal(raw, &top); err != nil {
+		return nil // not an object; nothing to compare against the schema
+	}
+	fields := jsonFieldNames(dst)
+	for k := range top {
+		for _, f := range fields {
+			if k != f && strings.EqualFold(k, f) {
+				return fmt.Errorf("argument %q does not match field %q exactly (keys are case-sensitive): %w", k, f, gitea.ErrValidation)
+			}
+		}
+	}
+	return nil
+}
+
+// scanJSONValue consumes one JSON value from dec, failing with ErrValidation if
+// any object within it has two keys that are equal under strings.EqualFold.
+func scanJSONValue(dec *json.Decoder) error {
+	tok, err := dec.Token()
+	if err != nil {
+		return err
+	}
+	d, ok := tok.(json.Delim)
+	if !ok {
+		return nil
+	}
+	switch d {
+	case '{':
+		seen := map[string]string{}
+		for dec.More() {
+			kt, err := dec.Token()
+			if err != nil {
+				return err
+			}
+			k, _ := kt.(string)
+			folded := strings.ToLower(k)
+			if prev, dup := seen[folded]; dup {
+				return fmt.Errorf("duplicate argument keys %q and %q (keys must be unique ignoring case): %w", prev, k, gitea.ErrValidation)
+			}
+			seen[folded] = k
+			if err := scanJSONValue(dec); err != nil {
+				return err
+			}
+		}
+	case '[':
+		for dec.More() {
+			if err := scanJSONValue(dec); err != nil {
+				return err
+			}
+		}
+	}
+	_, err = dec.Token() // closing delimiter
+	return err
+}
+
+// jsonFieldNames returns the json tag names of dst's top-level struct fields.
+func jsonFieldNames(dst any) []string {
+	v := reflect.ValueOf(dst)
+	if v.Kind() != reflect.Pointer || v.IsNil() || v.Elem().Kind() != reflect.Struct {
+		return nil
+	}
+	t := v.Elem().Type()
+	names := make([]string, 0, t.NumField())
+	for i := 0; i < t.NumField(); i++ {
+		name, _, _ := strings.Cut(t.Field(i).Tag.Get("json"), ",")
+		if name == "" || name == "-" {
+			name = t.Field(i).Name
+		}
+		names = append(names, name)
+	}
+	return names
+}
+
+// branchNameRe is the shape the skills mcp-destructive-guard hook enforces.
+var branchNameRe = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._/-]*$`)
+
+// ValidateBranchName rejects branch names that Gitea would turn into a ref
+// other than refs/heads/<name> as the caller meant it. `refs/heads/main` in
+// particular creates refs/heads/refs/heads/main, after which every API write to
+// main fails with "matches more than one" until the ref is deleted by hand
+// (#65). Beyond the shape regexp it applies git's own check-ref-format rules
+// that the regexp admits.
+func ValidateBranchName(b string) error {
+	bad := !branchNameRe.MatchString(b) ||
+		strings.HasPrefix(b, "refs/") ||
+		b == "HEAD" ||
+		strings.Contains(b, "..") ||
+		strings.Contains(b, "//") ||
+		strings.Contains(b, "/.") ||
+		strings.HasSuffix(b, "/") ||
+		strings.HasSuffix(b, ".") ||
+		strings.HasSuffix(b, ".lock")
+	if bad {
+		return fmt.Errorf("invalid branch name %q (want a short name like feat/x, no refs/ prefix): %w", b, gitea.ErrValidation)
+	}
+	return nil
+}
+
 // validateRequiredIdentifiers rejects an empty repo identifier at the tool layer
 // before it reaches the client, where it would build a trailing-empty path
 // segment (`/api/v1/repos/{owner}/`) and leak gitea's bare 404 (#37). `repo` and