fix: prevent silent write loss on 301 redirects (#154)

When a Gitea repo is renamed, the API returns a 301 redirect. Go's default `http.Client` follows 301/302/303 redirects by changing the HTTP method from PATCH/POST/PUT to GET and dropping the request body. This causes mutating API calls (edit PR, create issue, etc.) to silently appear to succeed while no write actually occurs — the client receives the current resource data via the redirected GET and returns it as if the edit worked.

## Fix

Add a `CheckRedirect` function to both HTTP clients (SDK client in `gitea.go` and REST client in `rest.go`) that returns `http.ErrUseLastResponse` for non-GET/HEAD methods. This surfaces the redirect as an error instead of silently downgrading the request. GET/HEAD reads continue to follow redirects normally.

## Tests

- `TestCheckRedirect`: table-driven unit tests for all HTTP methods + redirect limit
- `TestDoJSON_RepoRenameRedirect`: regression test with `httptest` server proving PATCH to a 301 endpoint returns an error instead of silently succeeding
- `TestDoJSON_GETRedirectFollowed`: verifies GET reads still follow 301 redirects

*This PR was authored by Claude.*

Reviewed-on: https://gitea.com/gitea/gitea-mcp/pulls/154
Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com>

operation/operation.go

@@ -2,6 +2,7 @@ package operation
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"os"
@@ -136,7 +137,7 @@ func Run() error {
 			close(shutdownDone)
 		}()
 
-		if err := httpServer.Start(fmt.Sprintf(":%d", flag.Port)); err != nil {
+		if err := httpServer.Start(fmt.Sprintf(":%d", flag.Port)); err != nil && !errors.Is(err, http.ErrServerClosed) {
 			return err
 		}
 		<-shutdownDone // Wait for shutdown to finish

operation/repo/repo.go

@@ -2,6 +2,7 @@ package repo
 
 import (
 	"context"
+	"errors"
 	"fmt"
 
 	"gitea.com/gitea/gitea-mcp/pkg/gitea"
@@ -18,9 +19,10 @@ import (
 var Tool = tool.New()
 
 const (
-	CreateRepoToolName  = "create_repo"
+	CreateRepoToolName   = "create_repo"
-	ForkRepoToolName    = "fork_repo"
+	ForkRepoToolName     = "fork_repo"
-	ListMyReposToolName = "list_my_repos"
+	ListMyReposToolName  = "list_my_repos"
+	ListOrgReposToolName = "list_org_repos"
 )
 
 var (
@@ -55,6 +57,14 @@ var (
 		mcp.WithNumber("page", mcp.Required(), mcp.Description("Page number"), mcp.DefaultNumber(1), mcp.Min(1)),
 		mcp.WithNumber("perPage", mcp.Required(), mcp.Description("results per page"), mcp.DefaultNumber(30), mcp.Min(1)),
 	)
+
+	ListOrgReposTool = mcp.NewTool(
+		ListOrgReposToolName,
+		mcp.WithDescription("List repositories of an organization"),
+		mcp.WithString("org", mcp.Required(), mcp.Description("Organization name")),
+		mcp.WithNumber("page", mcp.Required(), mcp.Description("Page number"), mcp.DefaultNumber(1), mcp.Min(1)),
+		mcp.WithNumber("pageSize", mcp.Required(), mcp.Description("Page size number"), mcp.DefaultNumber(100), mcp.Min(1)),
+	)
 )
 
 func init() {
@@ -70,6 +80,10 @@ func init() {
 		Tool:    ListMyReposTool,
 		Handler: ListMyReposFn,
 	})
+	Tool.RegisterRead(server.ServerTool{
+		Tool:    ListOrgReposTool,
+		Handler: ListOrgReposFn,
+	})
 }
 
 func CreateRepoFn(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
@@ -178,3 +192,34 @@ func ListMyReposFn(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolR
 
 	return to.TextResult(slimRepos(repos))
 }
+
+func ListOrgReposFn(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) {
+	log.Debugf("Called ListOrgReposFn")
+	org, ok := req.GetArguments()["org"].(string)
+	if !ok {
+		return to.ErrorResult(errors.New("organization name is required"))
+	}
+	page, ok := req.GetArguments()["page"].(float64)
+	if !ok {
+		page = 1
+	}
+	pageSize, ok := req.GetArguments()["pageSize"].(float64)
+	if !ok {
+		pageSize = 100
+	}
+	opt := gitea_sdk.ListOrgReposOptions{
+		ListOptions: gitea_sdk.ListOptions{
+			Page:     int(page),
+			PageSize: int(pageSize),
+		},
+	}
+	client, err := gitea.ClientFromContext(ctx)
+	if err != nil {
+		return to.ErrorResult(fmt.Errorf("get gitea client err: %v", err))
+	}
+	repos, _, err := client.ListOrgRepos(org, opt)
+	if err != nil {
+		return to.ErrorResult(fmt.Errorf("list organization '%s' repositories error: %v", org, err))
+	}
+	return to.TextResult(repos)
+}

pkg/gitea/gitea.go

@@ -3,6 +3,7 @@ package gitea
 import (
 	"context"
 	"crypto/tls"
+	"errors"
 	"fmt"
 	"net/http"
 
@@ -13,7 +14,8 @@ import (
 
 func NewClient(token string) (*gitea.Client, error) {
 	httpClient := &http.Client{
-		Transport: http.DefaultTransport,
+		Transport:     http.DefaultTransport,
+		CheckRedirect: checkRedirect,
 	}
 
 	opts := []gitea.ClientOption{
@@ -38,6 +40,19 @@ func NewClient(token string) (*gitea.Client, error) {
 	return client, nil
 }
 
+// checkRedirect prevents Go from silently changing mutating requests (POST, PATCH, etc.)
+// to GET when following 301/302/303 redirects, which would drop the request body and
+// make writes appear to succeed when they didn't.
+func checkRedirect(_ *http.Request, via []*http.Request) error {
+	if len(via) >= 10 {
+		return errors.New("stopped after 10 redirects")
+	}
+	if via[0].Method != http.MethodGet && via[0].Method != http.MethodHead {
+		return http.ErrUseLastResponse
+	}
+	return nil
+}
+
 func ClientFromContext(ctx context.Context) (*gitea.Client, error) {
 	token, ok := ctx.Value(mcpContext.TokenContextKey).(string)
 	if !ok {

pkg/gitea/redirect_test.go

@@ -0,0 +1,120 @@
+package gitea
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"gitea.com/gitea/gitea-mcp/pkg/flag"
+)
+
+func TestCheckRedirect(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		method  string
+		wantErr error
+	}{
+		{"allows GET", http.MethodGet, nil},
+		{"allows HEAD", http.MethodHead, nil},
+		{"blocks PATCH", http.MethodPatch, http.ErrUseLastResponse},
+		{"blocks POST", http.MethodPost, http.ErrUseLastResponse},
+		{"blocks PUT", http.MethodPut, http.ErrUseLastResponse},
+		{"blocks DELETE", http.MethodDelete, http.ErrUseLastResponse},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			via := []*http.Request{{Method: tc.method}}
+			err := checkRedirect(nil, via)
+			if err != tc.wantErr {
+				t.Fatalf("expected %v, got %v", tc.wantErr, err)
+			}
+		})
+	}
+
+	t.Run("stops after 10 redirects", func(t *testing.T) {
+		via := make([]*http.Request, 10)
+		for i := range via {
+			via[i] = &http.Request{Method: http.MethodGet}
+		}
+		err := checkRedirect(nil, via)
+		if err == nil || err == http.ErrUseLastResponse {
+			t.Fatalf("expected redirect limit error, got %v", err)
+		}
+	})
+}
+
+// TestDoJSON_RepoRenameRedirect is a regression test for the bug where a PATCH
+// request to a renamed repo got a 301 redirect, Go's http.Client silently
+// changed the method to GET, and the write appeared to succeed without error.
+func TestDoJSON_RepoRenameRedirect(t *testing.T) {
+	// Simulate a Gitea API that returns 301 for the old repo name (like a renamed repo).
+	mux := http.NewServeMux()
+	mux.HandleFunc("PATCH /api/v1/repos/owner/old-name/pulls/1", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/api/v1/repos/owner/new-name/pulls/1", http.StatusMovedPermanently)
+	})
+	mux.HandleFunc("PATCH /api/v1/repos/owner/new-name/pulls/1", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusOK)
+		fmt.Fprint(w, `{"id":1,"title":"updated"}`)
+	})
+	mux.HandleFunc("GET /api/v1/repos/owner/new-name/pulls/1", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusOK)
+		fmt.Fprint(w, `{"id":1,"title":"not-updated"}`)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	origHost := flag.Host
+	defer func() { flag.Host = origHost }()
+	flag.Host = srv.URL
+
+	var result map[string]any
+	status, err := DoJSON(context.Background(), http.MethodPatch, "repos/owner/old-name/pulls/1", nil, map[string]string{"title": "updated"}, &result)
+	if err != nil {
+		// The redirect should be blocked, returning the 301 response directly.
+		// DoJSON treats non-2xx as an error, which is the correct behavior.
+		if status != http.StatusMovedPermanently {
+			t.Fatalf("expected status 301, got %d (err: %v)", status, err)
+		}
+		return
+	}
+
+	// If we reach here without error, the redirect was followed. Verify the
+	// method was preserved (title should be "updated", not "not-updated").
+	title, _ := result["title"].(string)
+	if title == "not-updated" {
+		t.Fatal("PATCH was silently converted to GET on 301 redirect — write was lost")
+	}
+}
+
+// TestDoJSON_GETRedirectFollowed verifies that GET requests still follow redirects normally.
+func TestDoJSON_GETRedirectFollowed(t *testing.T) {
+	mux := http.NewServeMux()
+	mux.HandleFunc("GET /api/v1/repos/owner/old-name/pulls/1", func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, "/api/v1/repos/owner/new-name/pulls/1", http.StatusMovedPermanently)
+	})
+	mux.HandleFunc("GET /api/v1/repos/owner/new-name/pulls/1", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(http.StatusOK)
+		json.NewEncoder(w).Encode(map[string]any{"id": 1, "title": "found"})
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	origHost := flag.Host
+	defer func() { flag.Host = origHost }()
+	flag.Host = srv.URL
+
+	var result map[string]any
+	status, err := DoJSON(context.Background(), http.MethodGet, "repos/owner/old-name/pulls/1", nil, nil, &result)
+	if err != nil {
+		t.Fatalf("GET redirect should be followed, got error: %v (status %d)", err, status)
+	}
+	title, _ := result["title"].(string)
+	if title != "found" {
+		t.Fatalf("expected title 'found', got %q", title)
+	}
+}

pkg/gitea/rest.go

@@ -44,8 +44,9 @@ func newRESTHTTPClient() *http.Client {
 		transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec // user-requested insecure mode
 	}
 	return &http.Client{
-		Transport: transport,
+		Transport:     transport,
-		Timeout:   60 * time.Second,
+		Timeout:       60 * time.Second,
+		CheckRedirect: checkRedirect,
 	}
 }