From c57e4c2e570097a6a88294938581b6b16662f1fa Mon Sep 17 00:00:00 2001 From: silverwind <2021+silverwind@noreply.gitea.com> Date: Fri, 13 Mar 2026 17:45:59 +0000 Subject: [PATCH] fix: prevent silent write loss on 301 redirects (#154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- pkg/gitea/gitea.go | 17 +++++- pkg/gitea/redirect_test.go | 120 +++++++++++++++++++++++++++++++++++++ pkg/gitea/rest.go | 5 +- 3 files changed, 139 insertions(+), 3 deletions(-) create mode 100644 pkg/gitea/redirect_test.go diff --git a/pkg/gitea/gitea.go b/pkg/gitea/gitea.go index 3f3a4d7..a3adf4f 100644 --- a/pkg/gitea/gitea.go +++ b/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 { diff --git a/pkg/gitea/redirect_test.go b/pkg/gitea/redirect_test.go new file mode 100644 index 0000000..fc78bdb --- /dev/null +++ b/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) + } +} diff --git a/pkg/gitea/rest.go b/pkg/gitea/rest.go index 982dfef..af4a990 100644 --- a/pkg/gitea/rest.go +++ b/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, - Timeout: 60 * time.Second, + Transport: transport, + Timeout: 60 * time.Second, + CheckRedirect: checkRedirect, } }