diff --git a/internal/gitea/client.go b/internal/gitea/client.go index 22378fa..fa8da6d 100644 --- a/internal/gitea/client.go +++ b/internal/gitea/client.go @@ -3,8 +3,10 @@ package gitea import ( "bytes" "context" + "fmt" "io" "net/http" + "strings" "time" "github.com/hashicorp/golang-lru/v2/expirable" @@ -40,7 +42,21 @@ func (c *Client) DefaultBranch(ctx context.Context, owner, name string) (string, return repo.DefaultBranch, nil } +// hasEmptySegment reports whether the path portion (before any query string) +// contains an empty segment ("//"), which means an owner or repo path +// parameter was empty. Forwarding it upstream yields gitea's opaque +// /api/swagger 404 (#36), so callers reject it locally instead. +func hasEmptySegment(path string) bool { + if i := strings.IndexByte(path, '?'); i >= 0 { + path = path[:i] + } + return strings.Contains(path, "//") +} + func (c *Client) doOnce(ctx context.Context, method, path string, body []byte) ([]byte, int, error) { + if hasEmptySegment(path) { + return nil, 0, fmt.Errorf("%w: upstream path %q has an empty owner or repo segment", ErrValidation, path) + } var reader io.Reader if body != nil { reader = bytes.NewReader(body) @@ -107,6 +123,9 @@ type rawResponse struct { } func (c *Client) doRaw(ctx context.Context, method, path string, body []byte) (*rawResponse, error) { + if hasEmptySegment(path) { + return nil, fmt.Errorf("%w: upstream path %q has an empty owner or repo segment", ErrValidation, path) + } var reader io.Reader if body != nil { reader = bytes.NewReader(body) diff --git a/internal/gitea/client_guard_test.go b/internal/gitea/client_guard_test.go new file mode 100644 index 0000000..8571212 --- /dev/null +++ b/internal/gitea/client_guard_test.go @@ -0,0 +1,54 @@ +package gitea_test + +import ( + "context" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + + "gitea.d-ma.be/mathias/gitea-mcp/internal/gitea" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// #36 second line of defence: when owner or repo is empty the path contains an +// empty segment ("//"). Rather than forward it upstream — where gitea answers +// with its opaque /api/swagger 404 — the client must reject it locally with a +// validation error and never touch the network. +func TestEmptyPathSegmentRejectedBeforeNetwork(t *testing.T) { + var hits int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&hits, 1) + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + c := gitea.NewClient(srv.URL, "tok") + + paths := []string{ + "/api/v1/repos/mathias//issues", // empty repo + "/api/v1/repos//gitea-mcp/contents/", // empty owner + "/api/v1/repos/mathias//issues?state=open", // empty repo before query + } + for _, p := range paths { + _, _, err := c.GetJSON(context.Background(), p) + require.Error(t, err, "path %q should be rejected", p) + assert.ErrorIs(t, err, gitea.ErrValidation) + } + assert.Equal(t, int32(0), atomic.LoadInt32(&hits), "guard must short-circuit before any HTTP call") +} + +// A well-formed path with a query string must still pass the guard. +func TestWellFormedPathPasses(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[]`)) + })) + defer srv.Close() + + c := gitea.NewClient(srv.URL, "tok") + _, status, err := c.GetJSON(context.Background(), "/api/v1/repos/mathias/gitea-mcp/issues?state=open") + require.NoError(t, err) + assert.Equal(t, 200, status) +} diff --git a/internal/tools/alias_test.go b/internal/tools/alias_test.go new file mode 100644 index 0000000..76719e3 --- /dev/null +++ b/internal/tools/alias_test.go @@ -0,0 +1,49 @@ +package tools_test + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "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" +) + +// #36: every caller (claude.ai connector, gitea's own convention) sends the +// repo identifier as `repo` and issue/PR index as `index`, but the tools +// declare them as `name` and `number`. Unmatched fields zero-valued the path +// segment and produced gitea's misleading /api/swagger 404. parseArgs now +// aliases repo->name and index->number so the idiomatic call works. +func TestRepoAndIndexAliasesResolve(t *testing.T) { + tests := []struct { + name string + args string + wantPath string + }{ + {"repo+index aliases", `{"owner":"mathias","repo":"infra","index":7}`, "/api/v1/repos/mathias/infra/issues/7"}, + {"canonical name+number", `{"owner":"mathias","name":"infra","number":7}`, "/api/v1/repos/mathias/infra/issues/7"}, + {"explicit name wins over repo", `{"owner":"mathias","name":"infra","repo":"ignored","number":7}`, "/api/v1/repos/mathias/infra/issues/7"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var gotPath string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotPath = r.URL.Path + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"number":7,"title":"t"}`)) + })) + defer srv.Close() + + tool := tools.NewIssueGet(gitea.NewClient(srv.URL, "tok"), allowlist.New([]string{"mathias"})) + out, err := tool.Call(context.Background(), json.RawMessage(tc.args)) + require.NoError(t, err) + assert.Equal(t, tc.wantPath, gotPath) + assert.Contains(t, string(out), `"number":7`) + }) + } +} diff --git a/internal/tools/tool.go b/internal/tools/tool.go index c4979b7..28132cc 100644 --- a/internal/tools/tool.go +++ b/internal/tools/tool.go @@ -2,6 +2,7 @@ package tools import ( "encoding/json" + "strings" "gitea.d-ma.be/mathias/gitea-mcp/internal/registry" ) @@ -17,7 +18,54 @@ func parseArgs(raw json.RawMessage, dst any) error { if len(raw) == 0 { return json.Unmarshal([]byte("{}"), dst) } - return json.Unmarshal(raw, dst) + return json.Unmarshal(normalizeAliases(raw), dst) +} + +// normalizeAliases maps the near-universal gitea/GitHub argument names onto the +// idiosyncratic ones these tools declare: `repo` -> `name`, `index` -> `number`. +// Every MCP caller (the claude.ai connector, LLMs primed on gitea's own API) +// reaches for `repo`/`index`; without this the unmatched fields zero-valued the +// upstream path segment and produced gitea's misleading /api/swagger 404 (#36). +// The alias key is left intact so a tool whose real field IS `index` +// (e.g. pr_merge) is unaffected. +func normalizeAliases(raw json.RawMessage) json.RawMessage { + var m map[string]json.RawMessage + if err := json.Unmarshal(raw, &m); err != nil { + return raw // not a JSON object — leave untouched + } + changed := aliasInto(m, "name", "repo") + changed = aliasInto(m, "number", "index") || changed + if !changed { + return raw + } + if patched, err := json.Marshal(m); err == nil { + return patched + } + return raw +} + +// aliasInto copies m[alias] into m[canonical] when canonical is absent or an +// empty/zero JSON value and alias carries a usable one. An explicit canonical +// value always wins. Returns true if it wrote canonical. +func aliasInto(m map[string]json.RawMessage, canonical, alias string) bool { + av, ok := m[alias] + if !ok || isEmptyJSON(av) { + return false + } + if cv, ok := m[canonical]; ok && !isEmptyJSON(cv) { + return false + } + m[canonical] = av + return true +} + +func isEmptyJSON(v json.RawMessage) bool { + switch strings.TrimSpace(string(v)) { + case "", `""`, "null", "0": + return true + default: + return false + } } // capLimit returns a sane page size: 0 or negative → def, > 50 → 50.