fix: address verified review findings (api-transport-download-1)

Signed-off-by: bircni <bircni@icloud.com>

docs/migration-1.0.md

@@ -261,7 +261,9 @@ changed meaning:
 - `api`: `--field` (`-f`) sent a string and `--Field` (`-F`) a typed value. Now
   `--raw-field` / `-f` sends a string and `--field` / `-F` a typed value, so the
   shorthands behave as before. `--data` / `-d` became `--input FILE` (or `-` for
-  stdin), and `--output` / `-o` became `--output-file`.
+  stdin), and `--output` / `-o` became `--output-file`. `api` exits 1 on a response outside
+  2xx (0.16 printed the body and exited 0), `-F key=@FILE` keeps the file's trailing newline,
+  and `--output-file` refuses to replace an existing file.
 
 ## Changed positional arguments
 

internal/app/api.go

@@ -303,6 +303,9 @@ func (o apiResponse) write(w io.Writer, r *http.Response) error {
 			return err
 		}
 		if o.query != "" && succeeded {
+			if len(data) == 0 {
+				return nil
+			}
 			if !json.Valid(data) {
 				return fmt.Errorf("--jq needs a JSON response, but the response is %s", cmp.Or(mediaType, "not JSON"))
 			}
@@ -363,7 +366,7 @@ func writeRequestHead(w io.Writer, r *http.Request) error {
 
 func writeHead(w io.Writer, prefix, first string, headers http.Header) error {
 	var b strings.Builder
-	b.WriteString(prefix + first + "\n")
+	b.WriteString(prefix + terminal.SafeLine(first) + "\n")
 	for _, key := range slices.Sorted(maps.Keys(headers)) {
 		for _, value := range headers.Values(key) {
 			b.WriteString(prefix + terminal.SafeLine(key+": "+value) + "\n")

internal/app/api_test.go

@@ -219,6 +219,8 @@ func TestAPIFiltersAndFormatsJSONResponses(t *testing.T) {
 		case "/api/v1/repos/alice/nope":
 			w.WriteHeader(http.StatusNotFound)
 			_, _ = w.Write([]byte(`{"message":"not found"}`))
+		case "/api/v1/repos/alice/gone":
+			w.WriteHeader(http.StatusNoContent)
 		case "/api/v1/repos":
 			if r.URL.Query().Get("page") != "2" {
 				w.Header().Set("Link", `<?page=2>; rel="next"`)
@@ -247,6 +249,7 @@ func TestAPIFiltersAndFormatsJSONResponses(t *testing.T) {
 		{name: "error body ends with a newline", args: []string{"repos/alice/nope"}, want: "{\"message\":\"not found\"}\n", err: "HTTP 404"},
 		{name: "error body is not filtered", args: []string{"repos/alice/nope", "--jq", ".message"}, want: "{\"message\":\"not found\"}\n", err: "HTTP 404"},
 		{name: "jq needs JSON", args: []string{"text", "--jq", "."}, err: "--jq needs a JSON response, but the response is text/plain"},
+		{name: "jq on 204 No Content", args: []string{"repos/alice/gone", "-X", "DELETE", "--jq", "."}},
 		{name: "silent", args: []string{"version", "--silent"}},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
@@ -288,3 +291,20 @@ func TestAPIHelpShowsJQ(t *testing.T) {
 	require.Contains(t, out.String(), "\n  -q, --jq expression ")
 	require.NotContains(t, out.String(), "--json fields")
 }
+
+func TestAPIIncludeSanitizesTheStatusLine(t *testing.T) {
+	server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		conn, rw, err := w.(http.Hijacker).Hijack()
+		if err != nil {
+			return
+		}
+		defer conn.Close()
+		_, _ = rw.WriteString("HTTP/1.1 200 OK\x1b]0;PWNED\x07\r\nContent-Length: 2\r\nConnection: close\r\n\r\n{}")
+		_ = rw.Flush()
+	}))
+	defer server.Close()
+	deps, _, diagnostics := serverDeps(t, server.URL)
+	require.NoError(t, Execute(context.Background(), deps, []string{"api", "version", "--include"}))
+	require.NotContains(t, diagnostics.String(), "\x1b")
+	require.NotContains(t, diagnostics.String(), "\x07")
+}

internal/app/download.go

@@ -14,6 +14,7 @@ import (
 	"path/filepath"
 	"strconv"
 	"strings"
+	"unicode"
 	"unicode/utf8"
 
 	"gitea.dev/tea/internal/clierr"
@@ -53,10 +54,13 @@ func (f downloadFlags) destination(name string) (path string, skip bool, err err
 	}
 	path = f.output
 	if path == "" || isDirectory(path) {
-		if name == "" || name == "." || name == ".." || strings.ContainsAny(name, `/\`) {
+		if name == "" || name == "." || name == ".." || strings.ContainsAny(name, `/\`) || strings.ContainsFunc(name, func(r rune) bool { return unicode.IsControl(r) || unicode.Is(unicode.Cf, r) }) {
 			return "", false, clierr.Wrap(clierr.Usage, fmt.Errorf("cannot use %q as a file name; pass --output", name))
 		}
 		path = filepath.Join(cmp.Or(path, f.dir, "."), name)
+		if path == "-" {
+			path = "." + string(filepath.Separator) + path // a file named "-", not standard output
+		}
 	}
 	if _, err := os.Lstat(path); err == nil {
 		if f.skipExisting {
@@ -115,8 +119,11 @@ func writeTerminalSafe(w io.Writer, tty bool, body io.Reader) error {
 // rune cut off at the end.
 func binary(data []byte) bool {
 	for len(data) > 0 {
+		if !utf8.FullRune(data) {
+			return false
+		}
 		r, size := utf8.DecodeRune(data)
-		if r == 0 || (r == utf8.RuneError && size == 1 && (len(data) >= utf8.UTFMax || utf8.FullRune(data))) {
+		if r == 0 || (r == utf8.RuneError && size == 1) {
 			return true
 		}
 		data = data[size:]

internal/app/download_test.go

@@ -98,6 +98,9 @@ func TestDownloadDestinations(t *testing.T) {
 		{name: "--skip-existing", flags: downloadFlags{skipExisting: true}, file: "existing.txt", want: "existing.txt", skip: true},
 		{name: "parent directory name", file: "../escape.txt", err: `cannot use "../escape.txt" as a file name; pass --output`},
 		{name: "dot dot", file: "..", err: "cannot use"},
+		{name: "escape sequence in the name", file: "\x1b]0;PWNED\x07.txt", err: "cannot use"},
+		{name: "bidi override in the name", file: "a\u202etxt.exe", err: "cannot use"},
+		{name: "a file named dash stays a file", file: "-", want: "." + string(filepath.Separator) + "-"},
 		{name: "--output and --dir", flags: downloadFlags{output: "x", dir: "dl"}, file: "notes.txt", err: "--output and --dir cannot be combined"},
 		{name: "--clobber and --skip-existing", flags: downloadFlags{clobber: true, skipExisting: true}, file: "notes.txt", err: "--clobber and --skip-existing cannot be combined"},
 	} {
@@ -147,6 +150,8 @@ func TestDownloadsToATerminalRefuseBinaryData(t *testing.T) {
 		{name: "terminal text loses escapes", body: "title\x1b]0;PWNED\x07\n", want: "title]0;PWNED\n", tty: true},
 		{name: "terminal binary", body: "PK\x03\x04\x00\x01", tty: true, err: "refusing to write binary data to the terminal"},
 		{name: "terminal invalid UTF-8", body: "abc\xff\xfedef", tty: true, err: "refusing to write binary data"},
+		{name: "terminal 3-byte rune cut after 2 bytes", body: strings.Repeat("a", binarySniffLength-2) + "\u2026", want: strings.Repeat("a", binarySniffLength-2) + "\u2026", tty: true},
+		{name: "terminal 4-byte rune cut after 2 bytes", body: strings.Repeat("a", binarySniffLength-2) + "\U0001F600", want: strings.Repeat("a", binarySniffLength-2) + "\U0001F600", tty: true},
 		{name: "terminal UTF-8 cut at the sniff boundary", body: strings.Repeat("a", binarySniffLength-1) + "é", want: strings.Repeat("a", binarySniffLength-1) + "é", tty: true},
 	} {
 		t.Run(tc.name, func(t *testing.T) {

internal/services/api.go

@@ -6,6 +6,7 @@ package services
 import (
 	"context"
 	"errors"
+	"fmt"
 	"io"
 	"mime"
 	"net/http"
@@ -110,7 +111,8 @@ func (s API) Visit(ctx context.Context, options APIOptions, visit func(*http.Res
 		}
 		seen[absolute] = true
 		current, _ := url.Parse(absolute)
-		response, err := s.Client.Send(ctx, options.Method, endpoint, options.Body, options.Headers)
+		captureCtx, failure := transport.CaptureErrors(ctx)
+		response, err := s.Client.Send(captureCtx, options.Method, endpoint, options.Body, options.Headers)
 		if err != nil {
 			return err
 		}
@@ -124,6 +126,9 @@ func (s API) Visit(ctx context.Context, options APIOptions, visit func(*http.Res
 			return closeErr
 		}
 		if response.StatusCode < 200 || response.StatusCode >= 300 {
+			if captured := failure(); captured != nil {
+				return captured // carries the server message even when the visitor wrote the body to a file
+			}
 			return &transport.APIError{Status: response.StatusCode, Method: options.Method, Endpoint: current.Path}
 		}
 		if !options.Paginate {
@@ -146,13 +151,28 @@ func (s API) Visit(ctx context.Context, options APIOptions, visit func(*http.Res
 		if page+1 >= options.MaxPages {
 			return ErrMaxPages
 		}
-		if _, err := s.Client.URL(next); err != nil {
-			return err
-		}
 		relative, err := url.Parse(next)
 		if err != nil || strings.Contains(next, "\\") {
 			return errors.New("API returned an invalid pagination link")
 		}
+		if _, err := s.Client.URL(next); err != nil {
+			// Gitea builds links from its ROOT_URL, which can name the instance differently than the
+			// configured host. Take the page from the link's query and keep the request on this instance.
+			if !relative.IsAbs() || apiTail(relative.Path) == "" || apiTail(relative.Path) != apiTail(current.Path) {
+				return fmt.Errorf("%w (next page link points to %s, the request went to %s)", err, relative.Host, current.Host)
+			}
+			same := *current
+			same.RawQuery = relative.RawQuery
+			relative = &same
+		}
 		endpoint = current.ResolveReference(relative).String()
 	}
 }
+
+// apiTail returns the part of a path from its /api/ segment on.
+func apiTail(path string) string {
+	if i := strings.Index(path, "/api/"); i >= 0 {
+		return path[i:]
+	}
+	return ""
+}

internal/services/api_test.go

@@ -39,3 +39,43 @@ func TestAPIPaginationRejectsUnsafeAndRepeatingLinks(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, []string{"https://example.org/items?a=1,2"}, links)
 }
+
+func TestAPIPaginationFollowsLinksNamingTheInstanceDifferently(t *testing.T) {
+	var pages []string
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		pages = append(pages, r.Host+r.URL.RequestURI())
+		if r.URL.Query().Get("page") != "2" {
+			// Gitea builds Link headers from its ROOT_URL rather than from the request host.
+			w.Header().Set("Link", `<https://git.example.test/api/v1/items?limit=1&page=2>; rel="next"`)
+		}
+		_, _ = w.Write([]byte(`[]`))
+	}))
+	defer server.Close()
+	visit := func(response *http.Response) error { _, err := io.Copy(io.Discard, response.Body); return err }
+	err := (API{Client: transport.New(server.URL, "secret", false)}).Visit(context.Background(), APIOptions{Endpoint: "items", Method: "GET", Paginate: true}, visit)
+	require.NoError(t, err)
+	require.Len(t, pages, 2)
+	require.Contains(t, pages[1], "/api/v1/items?limit=1&page=2")
+	require.Contains(t, pages[1], server.Listener.Addr().String(), "the second page still goes to the configured host")
+
+	err = (API{Client: transport.New(server.URL, "secret", false)}).Visit(context.Background(), APIOptions{Endpoint: "other", Method: "GET", Paginate: true}, visit)
+	require.ErrorContains(t, err, "git.example.test")
+}
+
+func TestAPIVisitReturnsTheServerMessageOfAFailedResponse(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusUnprocessableEntity)
+		_, _ = w.Write([]byte(`{"message":"invalid color: #nothex"}`))
+	}))
+	defer server.Close()
+	var body []byte
+	err := (API{Client: transport.New(server.URL, "secret", false)}).Visit(context.Background(), APIOptions{Endpoint: "labels", Method: "POST"}, func(response *http.Response) error {
+		var err error
+		body, err = io.ReadAll(response.Body)
+		return err
+	})
+	var apiErr *transport.APIError
+	require.ErrorAs(t, err, &apiErr)
+	require.Equal(t, "invalid color: #nothex", apiErr.Message)
+	require.Contains(t, string(body), "nothex", "the visitor still sees the whole body")
+}

internal/transport/api.go

@@ -14,7 +14,8 @@ import (
 )
 
 // API returns an HTTP client for requests to this instance. It attaches the credential and Sudo
-// header to every attempt, retries only body-free reads, never follows redirects, and records
+// header to every attempt, retries only body-free reads, follows redirects (a renamed or
+// transferred repository) only within this instance's API, and records
 // the sanitized error of a failed response for CaptureErrors.
 func (c *Client) API() *http.Client {
 	base := c.HTTP.Transport
@@ -27,12 +28,29 @@ func (c *Client) API() *http.Client {
 	}
 	return &http.Client{
 		Transport:     chain,
-		CheckRedirect: func(_ *http.Request, _ []*http.Request) error { return http.ErrUseLastResponse },
+		CheckRedirect: c.checkRedirect,
 		Jar:           c.HTTP.Jar,
 		Timeout:       c.HTTP.Timeout,
 	}
 }
 
+// maxRedirects bounds the redirects followed for one request.
+const maxRedirects = 5
+
+// checkRedirect follows a redirect when it stays within the API of this instance and keeps the
+// method, as the 301 of a renamed repository does for a read. Anything else returns the 3xx
+// response, so a credential is never sent elsewhere and a POST that Go would turn into a GET is
+// not silently dropped.
+func (c *Client) checkRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) > maxRedirects || req.Method != via[0].Method {
+		return http.ErrUseLastResponse
+	}
+	if _, err := c.URL(req.URL.String()); err != nil {
+		return http.ErrUseLastResponse
+	}
+	return nil
+}
+
 type capturedErrorKey struct{}
 
 // CaptureErrors returns a context under which API clients record the sanitized error of each

internal/transport/api_test.go

@@ -206,3 +206,43 @@ func TestCaptureErrorsIgnoresSuccessAndUncapturedRequests(t *testing.T) {
 	require.NoError(t, resp.Body.Close())
 	require.JSONEq(t, `{"message":"ignored"}`, string(data))
 }
+
+func TestAPIFollowsRedirectsWithinTheInstanceAPI(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/repos/alice/old/issues":
+			http.Redirect(w, r, "/api/v1/repos/alice/new/issues", http.StatusMovedPermanently)
+		case "/api/v1/loop":
+			http.Redirect(w, r, "/api/v1/loop", http.StatusMovedPermanently)
+		case "/web":
+			http.Redirect(w, r, "/user/login", http.StatusFound)
+		default:
+			require.Equal(t, "token secret", r.Header.Get("Authorization"))
+			_, _ = w.Write([]byte(r.Method + " " + r.URL.Path))
+		}
+	}))
+	defer server.Close()
+	client := New(server.URL, "secret", false)
+	for _, tc := range []struct {
+		method, path, body string
+		status             int
+	}{
+		{http.MethodGet, "/api/v1/repos/alice/old/issues", "GET /api/v1/repos/alice/new/issues", http.StatusOK},
+		{http.MethodHead, "/api/v1/repos/alice/old/issues", "", http.StatusOK},
+		{http.MethodPost, "/api/v1/repos/alice/old/issues", "", http.StatusMovedPermanently},
+		{http.MethodGet, "/web", "", http.StatusFound},
+		{http.MethodGet, "/api/v1/loop", "", http.StatusMovedPermanently},
+	} {
+		req, err := http.NewRequest(tc.method, server.URL+tc.path, nil)
+		require.NoError(t, err)
+		resp, err := client.API().Do(req)
+		require.NoError(t, err, tc.path)
+		data, err := io.ReadAll(resp.Body)
+		require.NoError(t, err)
+		require.NoError(t, resp.Body.Close())
+		require.Equal(t, tc.status, resp.StatusCode, tc.method+" "+tc.path)
+		if tc.status == http.StatusOK {
+			require.Equal(t, tc.body, string(data))
+		}
+	}
+}

internal/transport/client_test.go

@@ -213,3 +213,22 @@ func TestDebugRedactsOnlySensitiveQueryValues(t *testing.T) {
 		require.Equal(t, tc.redacted, sensitiveQuery(tc.path, tc.key), "%s %s", tc.path, tc.key)
 	}
 }
+
+func TestDebugStripsControlsFromTheStatusLine(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		conn, rw, err := w.(http.Hijacker).Hijack()
+		if err != nil {
+			return
+		}
+		defer conn.Close()
+		_, _ = rw.WriteString("HTTP/1.1 200 OK\x1b]0;PWNED\x07\r\nContent-Length: 0\r\nConnection: close\r\n\r\n")
+		_ = rw.Flush()
+	}))
+	defer server.Close()
+	var log bytes.Buffer
+	_, err := New(server.URL, "t", false).Debug(&log).Do(context.Background(), http.MethodGet, "version", nil, nil)
+	require.NoError(t, err)
+	require.NotContains(t, log.String(), "\x1b")
+	require.NotContains(t, log.String(), "\x07")
+	require.Contains(t, log.String(), "-> 200 OK")
+}

internal/transport/debug.go

@@ -11,6 +11,7 @@ import (
 	"net/url"
 	"strings"
 	"time"
+	"unicode"
 )
 
 type debugTransport struct {
@@ -25,7 +26,7 @@ func (t debugTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	if err != nil {
 		outcome = "error: " + err.Error()
 	} else {
-		outcome = resp.Status
+		outcome = plainText(resp.Status)
 		message = failureMessage(req, resp)
 	}
 	line := fmt.Sprintf("[debug] %s %s -> %s (%s)", req.Method, redactURL(req.URL), outcome, time.Since(start).Round(time.Millisecond))
@@ -36,6 +37,12 @@ func (t debugTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	return resp, err
 }
 
+// plainText drops control and format characters, which a server can put in the reason phrase
+// to drive the terminal the log is printed to.
+func plainText(s string) string {
+	return strings.Join(strings.FieldsFunc(s, func(r rune) bool { return unicode.IsControl(r) || unicode.IsSpace(r) || unicode.Is(unicode.Cf, r) }), " ")
+}
+
 // failureMessage returns the sanitized server message of a failed response and keeps its body
 // readable for the caller.
 func failureMessage(req *http.Request, resp *http.Response) string {