From ae50501ba3f64ce601d712f684241f9d6130c36b Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 06:58:31 +0200 Subject: [PATCH] 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 --- internal/tools/file_write_branch.go | 3 + internal/tools/strict_args_test.go | 110 ++++++++++++++++++++++++ internal/tools/tbd_ship.go | 7 ++ internal/tools/tool.go | 125 +++++++++++++++++++++++++++- 4 files changed, 243 insertions(+), 2 deletions(-) create mode 100644 internal/tools/strict_args_test.go diff --git a/internal/tools/file_write_branch.go b/internal/tools/file_write_branch.go index f98976e..8d0b6ff 100644 --- a/internal/tools/file_write_branch.go +++ b/internal/tools/file_write_branch.go @@ -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) } diff --git a/internal/tools/strict_args_test.go b/internal/tools/strict_args_test.go new file mode 100644 index 0000000..f858134 --- /dev/null +++ b/internal/tools/strict_args_test.go @@ -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)) + }) + } +} diff --git a/internal/tools/tbd_ship.go b/internal/tools/tbd_ship.go index e94941e..de7a7e8 100644 --- a/internal/tools/tbd_ship.go +++ b/internal/tools/tbd_ship.go @@ -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 diff --git a/internal/tools/tool.go b/internal/tools/tool.go index 8ceebdc..bf547ce 100644 --- a/internal/tools/tool.go +++ b/internal/tools/tool.go @@ -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/ 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