From ae50501ba3f64ce601d712f684241f9d6130c36b Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 06:58:31 +0200 Subject: [PATCH 1/3] 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 From 31a3dd3e0e13af895be18f3cd22beb706250d28e Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 07:02:27 +0200 Subject: [PATCH 2/3] fix(tools): fail closed when the key scan cannot read the input PR #66 review: json.Decoder.Token() fails on a number outside float64 range (1e999), which json.Unmarshal ignores in an unknown key, so a decoy key ended the scan and the #65 repro wrote to main. The decoder now uses UseNumber, and any scan error is a validation error. The duplicate check folds with strings.EqualFold (as encoding/json does) instead of ToLower, so long s and the Kelvin sign are caught in nested objects too. Refs #65 Co-Authored-By: Claude Opus 5.5 --- internal/tools/strict_args_test.go | 6 ++++++ internal/tools/tool.go | 20 ++++++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/internal/tools/strict_args_test.go b/internal/tools/strict_args_test.go index f858134..0b8b906 100644 --- a/internal/tools/strict_args_test.go +++ b/internal/tools/strict_args_test.go @@ -39,6 +39,12 @@ func TestParseArgsRejectsCaseVariantKeys(t *testing.T) { "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}}`, + // PR #66 review: a number the token scanner cannot parse as float64 must + // not make the scan give up and let the duplicate through. + "1e999 decoy before duplicate": `{"owner":"probe","repo":"t","name":"t","path":"case8.txt","content":"x","message":"m","z":1e999,"branch":"feat/y8","Branch":"main"}`, + "-1e309 decoy before exact dup": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","z":-1e309,"branch":"feat/y8","branch":"main"}`, + // encoding/json folds like strings.EqualFold: long s (U+017F) == s. + "nested long-s duplicate": `{"owner":"probe","repo":"t","path":"p","content":"x","message":"m","branch":"feat/y8","extra":{"s":1,"\u017f":2}}`, } for name, raw := range cases { t.Run(name, func(t *testing.T) { diff --git a/internal/tools/tool.go b/internal/tools/tool.go index bf547ce..52eb640 100644 --- a/internal/tools/tool.go +++ b/internal/tools/tool.go @@ -45,11 +45,16 @@ func parseArgs(raw json.RawMessage, dst any) error { // 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)) + // UseNumber: without it Token() fails on a number outside float64 range, + // which json.Unmarshal ignores in an unknown key — a decoy that would end + // the scan before the duplicate (PR #66 review). + dec.UseNumber() 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 + // Fail closed: input the scanner cannot read is input it cannot vouch for. + return fmt.Errorf("arguments are not valid JSON: %v: %w", err, gitea.ErrValidation) } var top map[string]json.RawMessage if err := json.Unmarshal(raw, &top); err != nil { @@ -79,18 +84,21 @@ func scanJSONValue(dec *json.Decoder) error { } switch d { case '{': - seen := map[string]string{} + var seen []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) + // EqualFold, not ToLower: encoding/json folds the same way, so + // long s (U+017F) matches s and the Kelvin sign matches k. + for _, prev := range seen { + if strings.EqualFold(prev, k) { + return fmt.Errorf("duplicate argument keys %q and %q (keys must be unique ignoring case): %w", prev, k, gitea.ErrValidation) + } } - seen[folded] = k + seen = append(seen, k) if err := scanJSONValue(dec); err != nil { return err } From cd612394513e5edbc736c26079dcb19349eab4b9 Mon Sep 17 00:00:00 2001 From: Mathias Date: Mon, 5 Oct 2026 07:05:00 +0200 Subject: [PATCH 3/3] perf(tools): linear duplicate-key scan keyed on SimpleFold orbit PR #66 review round 2: the pairwise EqualFold loop was quadratic, 7s of CPU for ~90k keys under the 1 MiB body cap. foldKey maps each rune to the smallest member of its unicode.SimpleFold orbit, equal exactly when strings.EqualFold is, so the scan is a map lookup again. The scaling test fails on the previous code (7.1s). Refs #65 Co-Authored-By: Claude Opus 5.5 --- internal/tools/foldkey_internal_test.go | 42 +++++++++++++++++++++++++ internal/tools/tool.go | 33 ++++++++++++++----- 2 files changed, 67 insertions(+), 8 deletions(-) create mode 100644 internal/tools/foldkey_internal_test.go diff --git a/internal/tools/foldkey_internal_test.go b/internal/tools/foldkey_internal_test.go new file mode 100644 index 0000000..da7d057 --- /dev/null +++ b/internal/tools/foldkey_internal_test.go @@ -0,0 +1,42 @@ +package tools + +import ( + "fmt" + "strings" + "testing" + "time" +) + +// foldKey must agree with strings.EqualFold, which is how encoding/json matches. +func TestFoldKeyAgreesWithEqualFold(t *testing.T) { + pairs := [][2]string{{"branch", "Branch"}, {"sha", "ſha"}, {"k", "K"}, {"ss", "ß"}, {"a", "b"}, {"Σ", "ς"}} + for _, p := range pairs { + if got, want := foldKey(p[0]) == foldKey(p[1]), strings.EqualFold(p[0], p[1]); got != want { + t.Errorf("%q vs %q: foldKey equal=%v, EqualFold=%v", p[0], p[1], got, want) + } + } +} + +// The duplicate scan is linear in the number of keys: 90k keys (about the +// 1 MiB body cap) must scan well under a second. +func TestRejectAmbiguousKeysScalesLinearly(t *testing.T) { + var b strings.Builder + b.WriteString("{") + for i := 0; i < 90_000; i++ { + if i > 0 { + b.WriteString(",") + } + fmt.Fprintf(&b, `"k%d":1`, i) + } + b.WriteString("}") + var dst struct { + Owner string `json:"owner"` + } + start := time.Now() + if err := rejectAmbiguousKeys([]byte(b.String()), &dst); err != nil { + t.Fatal(err) + } + if d := time.Since(start); d > 2*time.Second { + t.Errorf("scan of 90k keys took %v", d) + } +} diff --git a/internal/tools/tool.go b/internal/tools/tool.go index 52eb640..ca79e9e 100644 --- a/internal/tools/tool.go +++ b/internal/tools/tool.go @@ -8,6 +8,7 @@ import ( "reflect" "regexp" "strings" + "unicode" "git.d-ma.be/mathias/gitea-mcp/internal/gitea" "git.d-ma.be/mathias/gitea-mcp/internal/registry" @@ -84,21 +85,18 @@ func scanJSONValue(dec *json.Decoder) error { } switch d { case '{': - var seen []string + seen := map[string]string{} for dec.More() { kt, err := dec.Token() if err != nil { return err } k, _ := kt.(string) - // EqualFold, not ToLower: encoding/json folds the same way, so - // long s (U+017F) matches s and the Kelvin sign matches k. - for _, prev := range seen { - if strings.EqualFold(prev, k) { - return fmt.Errorf("duplicate argument keys %q and %q (keys must be unique ignoring case): %w", prev, k, gitea.ErrValidation) - } + fk := foldKey(k) + if prev, dup := seen[fk]; dup { + return fmt.Errorf("duplicate argument keys %q and %q (keys must be unique ignoring case): %w", prev, k, gitea.ErrValidation) } - seen = append(seen, k) + seen[fk] = k if err := scanJSONValue(dec); err != nil { return err } @@ -114,6 +112,25 @@ func scanJSONValue(dec *json.Decoder) error { return err } +// foldKey maps every rune to the smallest member of its unicode.SimpleFold +// orbit, so foldKey(a) == foldKey(b) exactly when strings.EqualFold(a, b) — +// the folding encoding/json uses (long s == s, Kelvin sign == k). A map keyed +// on it keeps the duplicate scan linear (PR #66 review: a pairwise EqualFold +// loop cost 7s of CPU on ~90k keys). +func foldKey(s string) string { + var b strings.Builder + for _, r := range s { + min := r + for f := unicode.SimpleFold(r); f != r; f = unicode.SimpleFold(f) { + if f < min { + min = f + } + } + b.WriteRune(min) + } + return b.String() +} + // jsonFieldNames returns the json tag names of dst's top-level struct fields. func jsonFieldNames(dst any) []string { v := reflect.ValueOf(dst)