diff --git a/docs/migration-1.0.md b/docs/migration-1.0.md index 3d69c7cc..fe430d02 100644 --- a/docs/migration-1.0.md +++ b/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 diff --git a/internal/app/api.go b/internal/app/api.go index 42fb1c9e..b5d97df8 100644 --- a/internal/app/api.go +++ b/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") diff --git a/internal/app/api_test.go b/internal/app/api_test.go index 4e8e3052..9fe42cdc 100644 --- a/internal/app/api_test.go +++ b/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", `; 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") +} diff --git a/internal/app/download.go b/internal/app/download.go index f8913f3c..a2526068 100644 --- a/internal/app/download.go +++ b/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:] diff --git a/internal/app/download_test.go b/internal/app/download_test.go index 49c10163..37dcca21 100644 --- a/internal/app/download_test.go +++ b/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) { diff --git a/internal/services/api.go b/internal/services/api.go index 3f946d8d..0bdf3184 100644 --- a/internal/services/api.go +++ b/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 "" +} diff --git a/internal/services/api_test.go b/internal/services/api_test.go index 309c93eb..9f6c52d5 100644 --- a/internal/services/api_test.go +++ b/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", `; 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") +} diff --git a/internal/transport/api.go b/internal/transport/api.go index 22911535..c3a012c9 100644 --- a/internal/transport/api.go +++ b/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 diff --git a/internal/transport/api_test.go b/internal/transport/api_test.go index f04c26ab..d70e2fa5 100644 --- a/internal/transport/api_test.go +++ b/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)) + } + } +} diff --git a/internal/transport/client_test.go b/internal/transport/client_test.go index 5212d219..6683a12a 100644 --- a/internal/transport/client_test.go +++ b/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") +} diff --git a/internal/transport/debug.go b/internal/transport/debug.go index f46bc342..63cb7536 100644 --- a/internal/transport/debug.go +++ b/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 {