Files
gitea-mcp/pkg/gitea/redirect_test.go
silverwind c57e4c2e57 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>
2026-03-13 17:45:59 +00:00

121 lines
4.2 KiB
Go

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)
}
}