Addresses audit findings from a tri-partite review (codex + 2 Claude agents).
Multiple distinct fixes here because they touched overlapping files; happy
to split via interactive rebase if a reviewer prefers.
## Correctness bugs (HIGH)
* `--config` is now actually honored. cmd/root.initConfig fed Viper but
every command that mattered loaded config via `internal/config.Load()`
which always read the default path. Added `config.SetExplicitConfigPath`
consulted by `GetConfigPath`; `--config other.yaml auth login` now writes
to other.yaml.
- internal/config/config.go, cmd/root.go
* `--json` now works on `fj run …`, `fj workflow …`, and `fj wiki view`.
cmd/aliases.go registered `--json` as a Bool but the handlers call
`wantJSON()` which does `GetString("json")` and silently ignores the
type-error return. cmd/wiki.go did the inverse (`GetBool("json")` against
a string-registered flag). Both now use `addJSONFlags`/`wantJSON`/
`outputJSON` consistently.
- cmd/aliases.go, cmd/wiki.go
* `fj api` no longer lets endpoints escape the /api/v1 base via
path-traversal. `fj api '/../admin/users'` previously normalized to
`/admin/users` because `http.NewRequest` resolves `..` segments —
silently sending authenticated traffic to non-API routes. Endpoint is
now parsed, `..` segments are rejected, and JoinPath is used.
- cmd/api.go
## Design rework (BREAKING — gets rid of the `--json=fields` quirk)
* `--json` flag rebuilt from a string-with-NoOptDefVal=" " sentinel into a
plain Bool. `--json-fields` keeps comma-separated projection. The two
are mutually exclusive (`MarkFlagsMutuallyExclusive`). `--jq` composes
with either or neither. The previous design produced a `--json string[=" "]`
in --help and required `--json=fields` (with literal "=") because
`--json fields` was parsed as the bare flag plus a positional. Gone.
- cmd/json.go: addJSONFlags / wantJSON / outputJSON
- cmd/api.go: example block reflects the new shape
Migration: `--json=fields` → `--json-fields fields`. Bare `--json` still
means "everything as JSON".
* `fj api` now uses `internal/api.SharedHTTPClient` (30 s timeout, pooled)
instead of constructing a zero-value `&http.Client{}` with no timeout.
A hung Forgejo no longer pins the CLI indefinitely. Response body is
also bounded by `io.LimitReader` at 64 MB to prevent OOM-on-self.
- internal/api/client.go (export SharedHTTPClient), cmd/api.go
* `--hostname` declared as a persistent flag on rootCmd is now the only
declaration. cmd/auth.go re-declared `--hostname` on three subcommands,
shadowing the persistent flag — meaning `fj --hostname=X auth login`
and `fj auth login --hostname=X` went through different code paths
(viper read vs. local flag read). Local declarations removed.
- cmd/auth.go
## Hardening (MEDIUM/LOW)
* `--token` on `auth login` now emits a stderr warning when used, since
it puts the PAT on argv (visible in `ps auxe`/shell history). Flag not
removed — too disruptive — but discoverable now.
- cmd/auth.go
* Error handling no longer regex-matches "401"/"403" against rendered
error strings (would have triggered "auth login" hint for an error
that just mentioned issue #403). Now relies on typed `*api.APIError`.
Hints moved to a separate `Hint` field on `CLIError`, so JSON-error
consumers get clean structure and the human renderer still appends
"\nHint: …".
- cmd/errors.go
* `migrateConfigDir` now opens dst with `O_TRUNC` instead of just
`O_CREATE|O_WRONLY`. Previously a partially-pre-existing dst file
would have legacy contents overwrite a prefix and leave stale tail
bytes — silent YAML/token corruption.
- cmd/root.go (extracted into copyOneConfigFile with proper close handling)
* Config dir created with mode 0700 instead of 0755. `initConfig` warns
on stderr if the resolved config file is world/group readable
(`mode & 0o077 != 0`); doesn't fail-close.
- cmd/root.go
* Network errors (`no such host`, `connection refused`, `i/o timeout`)
now return a structured `CLIError` with code `ErrNetworkError` and a
hint, instead of a fmt.Errorf chain.
- cmd/errors.go
Verified: `go build ./...` and `go test ./...` clean. Live integration
tested against forgejo.zerova.net.
Out of scope, deferred to follow-up commits:
- Pagination unification across `repo list`/`pr list`/`issue list` (only
`release list` walks pages today; others silently truncate).
- `fj api --paginate` to follow pages like `gh api --paginate`.
- De-duplicating cmd/aliases.go ↔ cmd/actions.go subtrees.
308 lines
9.4 KiB
Go
308 lines
9.4 KiB
Go
package cmd
|
|
|
|
import (
|
|
"bytes"
|
|
"encoding/json"
|
|
"fmt"
|
|
"io"
|
|
"net/http"
|
|
"net/url"
|
|
"os"
|
|
"strconv"
|
|
"strings"
|
|
|
|
"forgejo.zerova.net/public/fj/internal/api"
|
|
"forgejo.zerova.net/public/fj/internal/config"
|
|
"forgejo.zerova.net/public/fj/internal/git"
|
|
"github.com/spf13/cobra"
|
|
)
|
|
|
|
// maxAPIResponseBytes caps response bodies for `fj api`. Forgejo responses
|
|
// are normally <1 MB; 64 MB is enough for any sane payload while preventing
|
|
// a runaway body from OOMing the CLI when combined with the 30 s client
|
|
// timeout.
|
|
const maxAPIResponseBytes = 64 << 20
|
|
|
|
var apiCmd = &cobra.Command{
|
|
Use: "api <endpoint> [flags]",
|
|
Short: "Make an authenticated API request",
|
|
Long: `Makes an authenticated HTTP request to the Forgejo API and prints the response.
|
|
|
|
The endpoint argument should be a path like "/repos/{owner}/{repo}/pulls".
|
|
Placeholders {owner} and {repo} are automatically replaced with values
|
|
detected from the current git repository.
|
|
|
|
If --field is used and no --method is specified, the method defaults to POST.`,
|
|
Example: ` # List pull requests for the current repository
|
|
fj api /repos/{owner}/{repo}/pulls
|
|
|
|
# Create an issue
|
|
fj api /repos/{owner}/{repo}/issues --method POST --field title=Bug --field body="It broke"
|
|
|
|
# Get a specific user
|
|
fj api /users/johndoe
|
|
|
|
# Use raw body from stdin
|
|
echo '{"title":"test"}' | fj api /repos/{owner}/{repo}/issues --input -
|
|
|
|
# Filter the response with a jq expression
|
|
fj api /repos/{owner}/{repo}/issues --jq '.[].title'
|
|
|
|
# Project the response down to specific fields
|
|
fj api /repos/{owner}/{repo} --json-fields full_name,description,private`,
|
|
Args: cobra.ExactArgs(1),
|
|
RunE: runAPI,
|
|
}
|
|
|
|
func init() {
|
|
rootCmd.AddCommand(apiCmd)
|
|
|
|
apiCmd.Flags().StringP("method", "X", "", "HTTP method (default: GET, or POST if --field is used)")
|
|
apiCmd.Flags().StringArrayP("field", "f", nil, "Add a typed field to the request body (key=value)")
|
|
apiCmd.Flags().StringArrayP("raw-field", "F", nil, "Add a string field to the request body (key=value)")
|
|
apiCmd.Flags().String("input", "", "Read request body from file (use \"-\" for stdin)")
|
|
apiCmd.Flags().StringArrayP("header", "H", nil, "Add an HTTP request header (key:value)")
|
|
apiCmd.Flags().Bool("silent", false, "Do not print the response body")
|
|
apiCmd.Flags().BoolP("include", "i", false, "Include HTTP response headers in the output")
|
|
addJSONFlags(apiCmd, "Output the response as JSON; pass a comma-separated field list to project specific keys")
|
|
}
|
|
|
|
func runAPI(cmd *cobra.Command, args []string) error {
|
|
endpoint := args[0]
|
|
|
|
method, _ := cmd.Flags().GetString("method")
|
|
fields, _ := cmd.Flags().GetStringArray("field")
|
|
rawFields, _ := cmd.Flags().GetStringArray("raw-field")
|
|
inputFile, _ := cmd.Flags().GetString("input")
|
|
headers, _ := cmd.Flags().GetStringArray("header")
|
|
hostname, _ := cmd.Flags().GetString("hostname")
|
|
silent, _ := cmd.Flags().GetBool("silent")
|
|
include, _ := cmd.Flags().GetBool("include")
|
|
|
|
// Resolve hostname and token from config
|
|
cfg, err := config.Load()
|
|
if err != nil {
|
|
return err
|
|
}
|
|
|
|
detectedHost := getDetectedHost()
|
|
|
|
host, err := cfg.GetHost(hostname, detectedHost, getCwd())
|
|
if err != nil {
|
|
return err
|
|
}
|
|
|
|
// Interpolate {owner} and {repo} placeholders
|
|
if strings.Contains(endpoint, "{owner}") || strings.Contains(endpoint, "{repo}") {
|
|
owner, repo, detectErr := git.DetectRepo()
|
|
if detectErr != nil {
|
|
return fmt.Errorf("cannot determine repository for path interpolation: %w", detectErr)
|
|
}
|
|
endpoint = strings.ReplaceAll(endpoint, "{owner}", owner)
|
|
endpoint = strings.ReplaceAll(endpoint, "{repo}", repo)
|
|
}
|
|
|
|
// Determine HTTP method
|
|
hasBody := len(fields) > 0 || len(rawFields) > 0 || inputFile != ""
|
|
if method == "" {
|
|
if hasBody {
|
|
method = http.MethodPost
|
|
} else {
|
|
method = http.MethodGet
|
|
}
|
|
}
|
|
method = strings.ToUpper(method)
|
|
|
|
// Build request body
|
|
var body io.Reader
|
|
if inputFile != "" {
|
|
if len(fields) > 0 || len(rawFields) > 0 {
|
|
return fmt.Errorf("--input cannot be combined with --field or --raw-field")
|
|
}
|
|
if inputFile == "-" {
|
|
body = os.Stdin
|
|
} else {
|
|
f, openErr := os.Open(inputFile)
|
|
if openErr != nil {
|
|
return fmt.Errorf("failed to open input file: %w", openErr)
|
|
}
|
|
defer func() { _ = f.Close() }()
|
|
body = f
|
|
}
|
|
} else if len(fields) > 0 || len(rawFields) > 0 {
|
|
bodyMap := make(map[string]any)
|
|
|
|
for _, f := range fields {
|
|
key, value, parseErr := parseField(f, false)
|
|
if parseErr != nil {
|
|
return parseErr
|
|
}
|
|
bodyMap[key] = value
|
|
}
|
|
for _, f := range rawFields {
|
|
key, value, parseErr := parseField(f, true)
|
|
if parseErr != nil {
|
|
return parseErr
|
|
}
|
|
bodyMap[key] = value
|
|
}
|
|
|
|
bodyBytes, marshalErr := json.Marshal(bodyMap)
|
|
if marshalErr != nil {
|
|
return fmt.Errorf("failed to marshal request body: %w", marshalErr)
|
|
}
|
|
body = bytes.NewReader(bodyBytes)
|
|
}
|
|
|
|
// Build the request URL safely. Naive concatenation lets endpoints like
|
|
// "/../admin/users" escape the /api/v1 base via Go's URL normalization
|
|
// of `..` segments — silently sending authenticated traffic to non-API
|
|
// paths. Parse the endpoint, reject `..`, then JoinPath onto the base.
|
|
endpointURL, err := url.Parse(endpoint)
|
|
if err != nil {
|
|
return fmt.Errorf("invalid endpoint %q: %w", endpoint, err)
|
|
}
|
|
if endpointURL.Scheme != "" || endpointURL.Host != "" {
|
|
return fmt.Errorf("endpoint must be a path, not a full URL: %s", endpoint)
|
|
}
|
|
for _, seg := range strings.Split(strings.Trim(endpointURL.Path, "/"), "/") {
|
|
if seg == ".." {
|
|
return fmt.Errorf("endpoint contains forbidden '..' segment: %s", endpoint)
|
|
}
|
|
}
|
|
base := &url.URL{Scheme: "https", Host: host.Hostname, Path: "/api/v1"}
|
|
final := base.JoinPath(endpointURL.Path)
|
|
final.RawQuery = endpointURL.RawQuery
|
|
|
|
// Create HTTP request
|
|
req, err := http.NewRequest(method, final.String(), body)
|
|
if err != nil {
|
|
return fmt.Errorf("failed to create request: %w", err)
|
|
}
|
|
|
|
// Set auth header
|
|
if host.Token != "" {
|
|
req.Header.Set("Authorization", "token "+host.Token)
|
|
}
|
|
req.Header.Set("Accept", "application/json")
|
|
if hasBody {
|
|
req.Header.Set("Content-Type", "application/json")
|
|
}
|
|
|
|
// Apply custom headers
|
|
for _, h := range headers {
|
|
key, value, found := strings.Cut(h, ":")
|
|
if !found {
|
|
return fmt.Errorf("invalid header format %q (expected key:value)", h)
|
|
}
|
|
req.Header.Set(strings.TrimSpace(key), strings.TrimSpace(value))
|
|
}
|
|
|
|
// Execute request via the shared client (30 s timeout, pooled
|
|
// connections). Previous zero-value http.Client{} had no timeout, which
|
|
// pinned the CLI on a hung Forgejo indefinitely.
|
|
ios.StartSpinner("Requesting...")
|
|
resp, err := api.SharedHTTPClient.Do(req)
|
|
ios.StopSpinner()
|
|
if err != nil {
|
|
return fmt.Errorf("failed to perform request: %w", err)
|
|
}
|
|
defer func() { _ = resp.Body.Close() }()
|
|
|
|
// Print response headers if requested
|
|
if include {
|
|
fmt.Fprintf(ios.Out, "%s %s\n", resp.Proto, resp.Status)
|
|
for key, values := range resp.Header {
|
|
for _, v := range values {
|
|
fmt.Fprintf(ios.Out, "%s: %s\n", key, v)
|
|
}
|
|
}
|
|
fmt.Fprintln(ios.Out)
|
|
}
|
|
|
|
// Read response body with a hard ceiling so a runaway upstream can't OOM
|
|
// the CLI. Read maxAPIResponseBytes+1 to detect overflow.
|
|
respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxAPIResponseBytes+1))
|
|
if err != nil {
|
|
return fmt.Errorf("failed to read response body: %w", err)
|
|
}
|
|
if int64(len(respBody)) > maxAPIResponseBytes {
|
|
return fmt.Errorf("response body exceeded %d bytes (use a different tool for bulk transfers)", maxAPIResponseBytes)
|
|
}
|
|
|
|
// Handle non-2xx status codes
|
|
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
|
if !silent {
|
|
fmt.Fprint(ios.ErrOut, string(respBody))
|
|
if len(respBody) > 0 && respBody[len(respBody)-1] != '\n' {
|
|
fmt.Fprintln(ios.ErrOut)
|
|
}
|
|
}
|
|
return fmt.Errorf("API request failed with status %d", resp.StatusCode)
|
|
}
|
|
|
|
if silent || len(respBody) == 0 {
|
|
return nil
|
|
}
|
|
|
|
contentType := resp.Header.Get("Content-Type")
|
|
isJSON := strings.Contains(contentType, "json") || json.Valid(respBody)
|
|
|
|
// If the user asked for JSON projection or jq filtering, route through
|
|
// the shared JSON output helpers so the API command is consistent with
|
|
// `fj repo list`, `fj pr list`, etc.
|
|
if wantJSON(cmd) {
|
|
if !isJSON {
|
|
return fmt.Errorf("--json/--json-fields/--jq requires a JSON response, but the server returned %s", contentType)
|
|
}
|
|
var parsed any
|
|
if err := json.Unmarshal(respBody, &parsed); err != nil {
|
|
return fmt.Errorf("response is not valid JSON: %w", err)
|
|
}
|
|
return outputJSON(cmd, parsed)
|
|
}
|
|
|
|
// Pretty-print JSON by default, otherwise emit raw bytes.
|
|
if isJSON {
|
|
var parsed any
|
|
if err := json.Unmarshal(respBody, &parsed); err == nil {
|
|
return writeJSON(parsed)
|
|
}
|
|
}
|
|
|
|
_, err = ios.Out.Write(respBody)
|
|
return err
|
|
}
|
|
|
|
// parseField parses a "key=value" string. When rawString is true, the value is
|
|
// always treated as a string. Otherwise, the function attempts JSON type
|
|
// inference: booleans ("true"/"false"), null, numbers, and falls back to string.
|
|
func parseField(field string, rawString bool) (string, any, error) {
|
|
key, value, found := strings.Cut(field, "=")
|
|
if !found {
|
|
return "", nil, fmt.Errorf("invalid field format %q (expected key=value)", field)
|
|
}
|
|
|
|
if rawString {
|
|
return key, value, nil
|
|
}
|
|
|
|
// JSON type inference
|
|
switch {
|
|
case value == "true":
|
|
return key, true, nil
|
|
case value == "false":
|
|
return key, false, nil
|
|
case value == "null":
|
|
return key, nil, nil
|
|
default:
|
|
// Try number
|
|
if n, err := strconv.ParseInt(value, 10, 64); err == nil {
|
|
return key, n, nil
|
|
}
|
|
if f, err := strconv.ParseFloat(value, 64); err == nil {
|
|
return key, f, nil
|
|
}
|
|
return key, value, nil
|
|
}
|
|
}
|