fix(tools): reject empty repo/name identifier at tool layer (#37)
An empty required repo identifier built a trailing-empty path segment
(`/api/v1/repos/{owner}/`) and leaked gitea's bare 404. parseArgs now
validates, via reflection, that `repo`/`name` string args are non-empty and
returns a typed ErrValidation naming the field. Optional identifiers (e.g.
code_search's owner-wide fan-out `repo`) opt out with `,omitempty`. `owner` is
already enforced by the allowlist check.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -49,7 +49,7 @@ func (t *CodeSearch) Descriptor() registry.ToolDescriptor {
|
||||
type codeSearchArgs struct {
|
||||
Q string `json:"q"`
|
||||
Owner string `json:"owner"`
|
||||
Repo string `json:"repo"`
|
||||
Repo string `json:"repo,omitempty"` // optional: empty => owner-wide fan-out (#37 opt-out)
|
||||
Page int `json:"page"`
|
||||
Limit int `json:"limit"`
|
||||
}
|
||||
|
||||
+42
-2
@@ -2,8 +2,11 @@ package tools
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"reflect"
|
||||
"strings"
|
||||
|
||||
"gitea.d-ma.be/mathias/gitea-mcp/internal/gitea"
|
||||
"gitea.d-ma.be/mathias/gitea-mcp/internal/registry"
|
||||
)
|
||||
|
||||
@@ -16,9 +19,46 @@ func textOK(v any) (json.RawMessage, error) {
|
||||
|
||||
func parseArgs(raw json.RawMessage, dst any) error {
|
||||
if len(raw) == 0 {
|
||||
return json.Unmarshal([]byte("{}"), dst)
|
||||
if err := json.Unmarshal([]byte("{}"), dst); err != nil {
|
||||
return err
|
||||
}
|
||||
return json.Unmarshal(normalizeAliases(raw), dst)
|
||||
} else if err := json.Unmarshal(normalizeAliases(raw), dst); err != nil {
|
||||
return err
|
||||
}
|
||||
return validateRequiredIdentifiers(dst)
|
||||
}
|
||||
|
||||
// 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
|
||||
// `name` are always required where present — the per-repo identifier tools and
|
||||
// the two create tools respectively; `owner` is already enforced by the
|
||||
// allowlist check. Returns a typed ErrValidation naming the missing field.
|
||||
func validateRequiredIdentifiers(dst any) error {
|
||||
v := reflect.ValueOf(dst)
|
||||
if v.Kind() != reflect.Ptr || v.IsNil() {
|
||||
return nil
|
||||
}
|
||||
v = v.Elem()
|
||||
if v.Kind() != reflect.Struct {
|
||||
return nil
|
||||
}
|
||||
t := v.Type()
|
||||
for i := 0; i < t.NumField(); i++ {
|
||||
base, opts, _ := strings.Cut(t.Field(i).Tag.Get("json"), ",")
|
||||
if base != "repo" && base != "name" {
|
||||
continue
|
||||
}
|
||||
// An optional identifier (e.g. code_search's `repo`, empty => owner-wide
|
||||
// fan-out) opts out by carrying `,omitempty`.
|
||||
if strings.Contains(opts, "omitempty") {
|
||||
continue
|
||||
}
|
||||
if v.Field(i).Kind() == reflect.String && v.Field(i).String() == "" {
|
||||
return fmt.Errorf("%q is required: %w", base, gitea.ErrValidation)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// normalizeAliases reconciles the two spellings of the repo identifier so a tool
|
||||
|
||||
@@ -0,0 +1,32 @@
|
||||
package tools_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"testing"
|
||||
|
||||
"gitea.d-ma.be/mathias/gitea-mcp/internal/allowlist"
|
||||
"gitea.d-ma.be/mathias/gitea-mcp/internal/gitea"
|
||||
"gitea.d-ma.be/mathias/gitea-mcp/internal/tools"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
// #37: an empty required repo identifier must fail fast at the tool layer with a
|
||||
// typed ErrValidation naming the field, rather than building a trailing-empty
|
||||
// path segment (`/api/v1/repos/{owner}/`) that leaks gitea's bare 404.
|
||||
func TestEmptyRepoRejectedAsValidation(t *testing.T) {
|
||||
c := gitea.NewClient("http://unused", "")
|
||||
a := allowlist.New([]string{"mathias"})
|
||||
|
||||
_, err := tools.NewRepoGet(c, a).Call(context.Background(), json.RawMessage(`{"owner":"mathias","repo":""}`))
|
||||
require.Error(t, err)
|
||||
assert.ErrorIs(t, err, gitea.ErrValidation)
|
||||
assert.Contains(t, err.Error(), "repo")
|
||||
|
||||
// same for an empty `name` on a create tool (name = required new-repo name)
|
||||
_, err = tools.NewRepoCreate(c, a).Call(context.Background(), json.RawMessage(`{"owner":"mathias","name":""}`))
|
||||
require.Error(t, err)
|
||||
assert.ErrorIs(t, err, gitea.ErrValidation)
|
||||
assert.Contains(t, err.Error(), "name")
|
||||
}
|
||||
Reference in New Issue
Block a user