feat(issue): add attachment read downloads (#218)

- Add a read-only `attachment_read` tool for issue and issue comment attachments. - Use the existing Gitea SDK attachment metadata APIs for `list` and `get`. - Add authenticated `/attachments/{uuid}` downloads in the MCP server for `download`, with small image inline results and streamed file downloads for larger content. - Add `-max-inline-attachment-bytes` / `GITEA_MAX_INLINE_ATTACHMENT_BYTES` to configure inline image limits. - Update the tool tables in all README variants. Refs: #209 Reviewed-on: https://gitea.com/gitea/gitea-mcp/pulls/218 Reviewed-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: yp05327 <576951401@qq.com>
2026-08-03 07:39:22 +02:00 · 2026-07-26 11:53:02 +00:00
parent 18fcd663e0
commit c97c38996c
10 changed files with 968 additions and 62 deletions
@@ -7,6 +7,8 @@ var (
 	Version string
 	Mode    string

+	MaxInlineAttachmentBytes int
+
 	Insecure     bool
 	ReadOnly     bool
 	Debug        bool
@@ -0,0 +1,208 @@
+package gitea
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"gitea.com/gitea/gitea-mcp/pkg/flag"
+)
+
+func TestBuildAttachmentURLRejectsExternalURL(t *testing.T) {
+	origHost := flag.Host
+	flag.Host = "https://example.com"
+	defer func() { flag.Host = origHost }()
+
+	if _, err := buildAttachmentURL("https://evil.example.com/attachments/1"); err == nil {
+		t.Fatal("expected error for full URL attachment path")
+	}
+}
+
+func TestBuildAttachmentURLPreservesHostPathPrefix(t *testing.T) {
+	origHost := flag.Host
+	flag.Host = "https://example.com/gitea"
+	defer func() { flag.Host = origHost }()
+
+	got, err := buildAttachmentURL("/attachments/uuid-1")
+	if err != nil {
+		t.Fatalf("buildAttachmentURL() error = %v", err)
+	}
+	if got != "https://example.com/gitea/attachments/uuid-1" {
+		t.Fatalf("buildAttachmentURL() = %q", got)
+	}
+}
+
+func TestDownloadAttachmentUsesTokenAndReturnsContentType(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/attachments/uuid-1" {
+			t.Fatalf("path = %s", r.URL.Path)
+		}
+		if got := r.Header.Get("Authorization"); got != "token secret-token" {
+			t.Fatalf("Authorization = %q", got)
+		}
+		if got := r.Header.Get("Accept"); got != "*/*" {
+			t.Fatalf("Accept = %q", got)
+		}
+		w.Header().Set("Content-Type", "image/png")
+		_, _ = w.Write([]byte("png-bytes"))
+	}))
+	defer server.Close()
+
+	origHost, origToken := flag.Host, flag.Token
+	flag.Host, flag.Token = server.URL, "secret-token"
+	defer func() { flag.Host, flag.Token = origHost, origToken }()
+
+	raw, contentType, status, err := DownloadAttachment(context.Background(), "/attachments/uuid-1", "*/*")
+	if err != nil {
+		t.Fatalf("DownloadAttachment() error = %v", err)
+	}
+	if status != http.StatusOK {
+		t.Fatalf("DownloadAttachment() status = %d, want %d", status, http.StatusOK)
+	}
+	if contentType != "image/png" {
+		t.Fatalf("contentType = %q", contentType)
+	}
+	if string(raw) != "png-bytes" {
+		t.Fatalf("body = %q", string(raw))
+	}
+}
+
+func TestDownloadAttachmentRejectsCrossOriginRedirect(t *testing.T) {
+	redirected := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatalf("cross-origin redirect should not be followed: %s", r.URL.String())
+	}))
+	defer redirected.Close()
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, redirected.URL+"/attachments/uuid-1", http.StatusFound)
+	}))
+	defer server.Close()
+
+	origHost, origToken := flag.Host, flag.Token
+	flag.Host, flag.Token = server.URL, "secret-token"
+	defer func() { flag.Host, flag.Token = origHost, origToken }()
+
+	_, _, _, err := DownloadAttachment(context.Background(), "/attachments/uuid-1", "*/*")
+	if err == nil {
+		t.Fatal("expected error for cross-origin redirect")
+	}
+	want := "do request: Get \"" + redirected.URL + "/attachments/uuid-1\": attachment redirect changed origin"
+	if err.Error() != want {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestDownloadAttachmentRejectsSameOriginRedirectLoopAtHopLimit(t *testing.T) {
+	var serverURL string
+	redirects := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		redirects++
+		http.Redirect(w, r, serverURL+"/attachments/uuid-1", http.StatusFound)
+	}))
+	defer server.Close()
+	serverURL = server.URL
+
+	origHost := flag.Host
+	flag.Host = server.URL
+	defer func() { flag.Host = origHost }()
+
+	_, _, _, err := DownloadAttachment(context.Background(), "/attachments/uuid-1", "*/*")
+	if err == nil {
+		t.Fatal("expected redirect limit error")
+	}
+	if !strings.Contains(err.Error(), "stopped after 10 redirects") {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if redirects != 10 {
+		t.Fatalf("redirects = %d, want 10", redirects)
+	}
+}
+
+func TestWriteAttachmentStreamsBodyToFile(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "attachment.bin")
+	written, err := WriteAttachment(strings.NewReader(strings.Repeat("z", 64)), path)
+	if err != nil {
+		t.Fatalf("WriteAttachment() error = %v", err)
+	}
+	if written != 64 {
+		t.Fatalf("written = %d, want 64", written)
+	}
+	data, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("ReadFile() error = %v", err)
+	}
+	if string(data) != strings.Repeat("z", 64) {
+		t.Fatalf("body mismatch")
+	}
+}
+
+func TestWriteAttachmentCreatesPrivateFile(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "attachment.bin")
+
+	if _, err := WriteAttachment(strings.NewReader("secret"), path); err != nil {
+		t.Fatalf("WriteAttachment() error = %v", err)
+	}
+
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("Stat() error = %v", err)
+	}
+	if got := info.Mode().Perm() & 0o077; got != 0 {
+		t.Fatalf("permissions = %03o, want no group/other bits", info.Mode().Perm())
+	}
+}
+
+func TestWriteAttachmentTightensExistingFilePermissions(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "attachment.bin")
+	if err := os.WriteFile(path, []byte("public"), 0o644); err != nil {
+		t.Fatalf("WriteFile() error = %v", err)
+	}
+
+	if _, err := WriteAttachment(strings.NewReader("secret"), path); err != nil {
+		t.Fatalf("WriteAttachment() error = %v", err)
+	}
+
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("Stat() error = %v", err)
+	}
+	if got := info.Mode().Perm(); got != 0o600 {
+		t.Fatalf("permissions = %03o, want 600", got)
+	}
+}
+
+func TestDownloadAttachmentErrorsOnNon2xx(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, "nope", http.StatusForbidden)
+	}))
+	defer server.Close()
+
+	origHost := flag.Host
+	flag.Host = server.URL
+	defer func() { flag.Host = origHost }()
+
+	_, _, status, err := DownloadAttachment(context.Background(), "/attachments/uuid-1", "*/*")
+	if status != http.StatusForbidden {
+		t.Fatalf("status = %d, want %d", status, http.StatusForbidden)
+	}
+	var httpErr *HTTPError
+	if !errors.As(err, &httpErr) {
+		t.Fatalf("expected HTTPError, got %T", err)
+	}
+}
+
+func TestAttachmentHTTPClientHasNoTotalTimeout(t *testing.T) {
+	origin := &url.URL{Scheme: "https", Host: "example.com"}
+
+	client := attachmentHTTPClient(origin)
+
+	if client.Timeout != 0 {
+		t.Fatalf("Timeout = %v, want 0", client.Timeout)
+	}
+}
@@ -9,6 +9,8 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"os"
+	"path"
 	"strings"
 	"sync"
 	"time"
@@ -34,6 +36,12 @@ func (e *HTTPError) Error() string {
 	return fmt.Sprintf("request failed with status %d: %s", e.StatusCode, e.Body)
 }

+type AttachmentResponse struct {
+	Body        io.ReadCloser
+	ContentType string
+	StatusCode  int
+}
+
 func tokenFromContext(ctx context.Context) string {
 	if ctx != nil {
 		if token, ok := ctx.Value(mcpContext.TokenContextKey).(string); ok && token != "" {
@@ -75,6 +83,40 @@ func buildAPIURL(path string, query url.Values) (string, error) {
 	return u.String(), nil
 }

+func buildAttachmentURL(attachmentPath string) (string, error) {
+	host := strings.TrimRight(flag.Host, "/")
+	if host == "" {
+		return "", errors.New("gitea host is empty")
+	}
+	baseURL, err := url.Parse(host)
+	if err != nil {
+		return "", err
+	}
+	if attachmentPath == "" {
+		return "", errors.New("attachment path is empty")
+	}
+	if strings.Contains(attachmentPath, "://") {
+		return "", errors.New("attachment path must not be a URL")
+	}
+	if !strings.HasPrefix(attachmentPath, "/attachments/") {
+		return "", errors.New("attachment path must start with /attachments/")
+	}
+	cleanPath := path.Clean(attachmentPath)
+	if !strings.HasPrefix(cleanPath, "/attachments/") {
+		return "", errors.New("attachment path must stay within /attachments/")
+	}
+	if cleanPath == "/attachments" || cleanPath == "/attachments/" {
+		return "", errors.New("attachment uuid is required")
+	}
+	joinedPath := strings.TrimRight(baseURL.Path, "/") + cleanPath
+	if joinedPath == "" {
+		joinedPath = cleanPath
+	}
+	baseURL.Path = joinedPath
+	baseURL.RawPath = joinedPath
+	return baseURL.String(), nil
+}
+
 // DoJSON performs an API request and decodes a JSON response into respOut (if non-nil).
 // It returns the HTTP status code.
 func DoJSON(ctx context.Context, method, path string, query url.Values, body, respOut any) (int, error) {
@@ -91,45 +133,142 @@ func DoJSON(ctx context.Context, method, path string, query url.Values, body, re
 	if err != nil {
 		return 0, err
 	}
+
 	req, err := http.NewRequestWithContext(ctx, method, u, bodyReader)
 	if err != nil {
 		return 0, fmt.Errorf("create request: %w", err)
 	}
-
 	token := tokenFromContext(ctx)
 	if token != "" {
 		req.Header.Set("Authorization", "token "+token)
 	}
-	req.Header.Set("Accept", "application/json")
 	if body != nil {
 		req.Header.Set("Content-Type", "application/json")
 	}
+	if respOut != nil {
+		req.Header.Set("Accept", "application/json")
+	}

-	client := restHTTPClient()
-	resp, err := client.Do(req)
+	resp, err := restHTTPClient().Do(req)
 	if err != nil {
 		return 0, fmt.Errorf("do request: %w", err)
 	}
 	defer resp.Body.Close()

 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		bodySnippet, _ := io.ReadAll(io.LimitReader(resp.Body, errBodySnippetSize))
+		bodySnippet, err := io.ReadAll(io.LimitReader(resp.Body, errBodySnippetSize+1))
+		if err != nil {
+			return resp.StatusCode, fmt.Errorf("read response: %w", err)
+		}
+		if len(bodySnippet) > errBodySnippetSize {
+			bodySnippet = bodySnippet[:errBodySnippetSize]
+		}
 		return resp.StatusCode, &HTTPError{StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(bodySnippet))}
 	}

 	if respOut == nil {
-		_, _ = io.Copy(io.Discard, resp.Body) // best-effort
+		_, _ = io.Copy(io.Discard, resp.Body)
 		return resp.StatusCode, nil
 	}

 	if err := json.NewDecoder(resp.Body).Decode(respOut); err != nil {
 		return resp.StatusCode, fmt.Errorf("decode response: %w", err)
 	}
+
 	return resp.StatusCode, nil
 }

+func attachmentHTTPClient(origin *url.URL) *http.Client {
+	return &http.Client{
+		Transport: sharedTransport(),
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {
+			if err := checkRedirect(req, via); err != nil {
+				return err
+			}
+			if req.URL.Scheme != origin.Scheme || req.URL.Host != origin.Host {
+				return errors.New("attachment redirect changed origin")
+			}
+			return nil
+		},
+	}
+}
+
+func OpenAttachment(ctx context.Context, attachmentPath, accept string) (*AttachmentResponse, error) {
+	u, err := buildAttachmentURL(attachmentPath)
+	if err != nil {
+		return nil, err
+	}
+	origin, err := url.Parse(u)
+	if err != nil {
+		return nil, err
+	}
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil)
+	if err != nil {
+		return nil, fmt.Errorf("create request: %w", err)
+	}
+	token := tokenFromContext(ctx)
+	if token != "" {
+		req.Header.Set("Authorization", "token "+token)
+	}
+	if accept != "" {
+		req.Header.Set("Accept", accept)
+	}
+
+	resp, err := attachmentHTTPClient(origin).Do(req)
+	if err != nil {
+		return nil, fmt.Errorf("do request: %w", err)
+	}
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+		defer resp.Body.Close()
+		bodySnippet, readErr := io.ReadAll(io.LimitReader(resp.Body, errBodySnippetSize+1))
+		if readErr != nil {
+			return nil, fmt.Errorf("read response: %w", readErr)
+		}
+		if len(bodySnippet) > errBodySnippetSize {
+			bodySnippet = bodySnippet[:errBodySnippetSize]
+		}
+		return nil, &HTTPError{StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(bodySnippet))}
+	}
+	return &AttachmentResponse{Body: resp.Body, ContentType: resp.Header.Get("Content-Type"), StatusCode: resp.StatusCode}, nil
+}
+
+// DownloadAttachment downloads attachment bytes from a same-host attachment path.
+func DownloadAttachment(ctx context.Context, attachmentPath, accept string) ([]byte, string, int, error) {
+	resp, err := OpenAttachment(ctx, attachmentPath, accept)
+	if err != nil {
+		var httpErr *HTTPError
+		if errors.As(err, &httpErr) {
+			return nil, "", httpErr.StatusCode, err
+		}
+		return nil, "", 0, err
+	}
+	defer resp.Body.Close()
+
+	respBytes, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return nil, "", resp.StatusCode, fmt.Errorf("read response: %w", err)
+	}
+	return respBytes, resp.ContentType, resp.StatusCode, nil
+}
+
+func WriteAttachment(body io.Reader, outputPath string) (int64, error) {
+	file, err := os.OpenFile(outputPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600)
+	if err != nil {
+		return 0, err
+	}
+	defer file.Close()
+	if err := file.Chmod(0o600); err != nil {
+		return 0, err
+	}
+
+	written, err := io.Copy(file, body)
+	if err != nil {
+		return written, err
+	}
+	return written, file.Close()
+}
+
 // DoBytes performs an API request and returns the raw response bytes.
-// It returns the HTTP status code.
 func DoBytes(ctx context.Context, method, path string, query url.Values, body any, accept string) ([]byte, int, error) {
 	var bodyReader io.Reader
 	if body != nil {
@@ -144,24 +283,23 @@ func DoBytes(ctx context.Context, method, path string, query url.Values, body an
 	if err != nil {
 		return nil, 0, err
 	}
+
 	req, err := http.NewRequestWithContext(ctx, method, u, bodyReader)
 	if err != nil {
 		return nil, 0, fmt.Errorf("create request: %w", err)
 	}
-
 	token := tokenFromContext(ctx)
 	if token != "" {
 		req.Header.Set("Authorization", "token "+token)
 	}
-	if accept != "" {
-		req.Header.Set("Accept", accept)
-	}
 	if body != nil {
 		req.Header.Set("Content-Type", "application/json")
 	}
+	if accept != "" {
+		req.Header.Set("Accept", accept)
+	}

-	client := restHTTPClient()
-	resp, err := client.Do(req)
+	resp, err := restHTTPClient().Do(req)
 	if err != nil {
 		return nil, 0, fmt.Errorf("do request: %w", err)
 	}
@@ -171,7 +309,6 @@ func DoBytes(ctx context.Context, method, path string, query url.Values, body an
 	if err != nil {
 		return nil, resp.StatusCode, fmt.Errorf("read response: %w", err)
 	}
-
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
 		bodySnippet := respBytes
 		if len(bodySnippet) > errBodySnippetSize {
@@ -179,6 +316,5 @@ func DoBytes(ctx context.Context, method, path string, query url.Values, body an
 		}
 		return nil, resp.StatusCode, &HTTPError{StatusCode: resp.StatusCode, Body: strings.TrimSpace(string(bodySnippet))}
 	}
-
 	return respBytes, resp.StatusCode, nil
 }
@@ -2,6 +2,11 @@ package gitea

 import (
 	"context"
+	"errors"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
 	"testing"

 	mcpContext "gitea.com/gitea/gitea-mcp/pkg/context"
@@ -28,3 +33,32 @@ func TestTokenFromContext(t *testing.T) {
 		}
 	})
 }
+
+func TestDoJSON_LimitsErrorResponseBody(t *testing.T) {
+	payload := strings.Repeat("x", errBodySnippetSize+100)
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusBadRequest)
+		_, _ = io.WriteString(w, payload)
+	}))
+	defer srv.Close()
+
+	origHost := flag.Host
+	defer func() { flag.Host = origHost }()
+	flag.Host = srv.URL
+
+	status, err := DoJSON(context.Background(), http.MethodGet, "repos/owner/repo", nil, nil, nil)
+	if err == nil {
+		t.Fatal("expected error")
+	}
+	if status != http.StatusBadRequest {
+		t.Fatalf("expected status %d, got %d", http.StatusBadRequest, status)
+	}
+
+	var httpErr *HTTPError
+	if !errors.As(err, &httpErr) {
+		t.Fatalf("expected HTTPError, got %T", err)
+	}
+	if len(httpErr.Body) != errBodySnippetSize {
+		t.Fatalf("expected body length %d, got %d", errBodySnippetSize, len(httpErr.Body))
+	}
+}