fj/cmd/json.go
sid 0c181df1d1 fix(cmd): correctness + audit hardening across cmd/ + internal/
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.
2026-05-02 15:41:48 -06:00

156 lines
4.3 KiB
Go

package cmd
import (
"encoding/json"
"fmt"
"strings"
"github.com/itchyny/gojq"
"github.com/spf13/cobra"
)
// addJSONFlags adds --json, --json-fields, and --jq flags to a command.
//
// Flag design (BREAKING CHANGE — the previous --json was a string with
// NoOptDefVal=" " so `--json=fields` projected and `--json` alone meant
// "everything". That sentinel produced a `--json string[=" "]` in --help
// and left users guessing about the equals sign). Now:
//
// - --json : Bool. "Output the response as JSON." (all fields)
// - --json-fields … : String. Comma-separated projection.
// - --jq … : String. jq expression filter.
//
// --json and --json-fields are mutually exclusive — pick one. --jq composes
// with either (or neither, in which case it implies "as JSON").
func addJSONFlags(cmd *cobra.Command, jsonDesc string) {
f := cmd.Flags()
f.Bool("json", false, jsonDesc)
f.String("json-fields", "", "Output as JSON, projecting only these comma-separated fields")
f.String("jq", "", "Filter JSON output using a jq expression")
cmd.MarkFlagsMutuallyExclusive("json", "json-fields")
}
// wantJSON returns true if the user requested JSON output via --json,
// --json-fields, or --jq.
func wantJSON(cmd *cobra.Command) bool {
if b, _ := cmd.Flags().GetBool("json"); b {
return true
}
if f, _ := cmd.Flags().GetString("json-fields"); f != "" {
return true
}
if jq, _ := cmd.Flags().GetString("jq"); jq != "" {
return true
}
return false
}
// outputJSON writes a value as JSON, respecting --json-fields and --jq.
// --json (the bool) is the "no projection, no filter" signal handled
// implicitly: when neither --json-fields nor --jq is set, the whole value
// is emitted.
func outputJSON(cmd *cobra.Command, value any) error {
fields, _ := cmd.Flags().GetString("json-fields")
jqExpr, _ := cmd.Flags().GetString("jq")
return writeJSONFiltered(value, fields, jqExpr)
}
// writeJSON writes a value as pretty-printed JSON to ios.Out.
func writeJSON(value any) error {
enc := json.NewEncoder(ios.Out)
enc.SetIndent("", " ")
return enc.Encode(value)
}
// writeJSONFiltered writes a value as JSON, optionally selecting specific fields
// and/or applying a jq expression. If fields is empty and jqExpr is empty, it
// writes the full value.
func writeJSONFiltered(value any, fields string, jqExpr string) error {
// If no filtering, just write the full JSON.
if fields == "" && jqExpr == "" {
return writeJSON(value)
}
// Convert value to a generic interface via JSON round-trip so we can
// manipulate it with maps/slices.
raw, err := json.Marshal(value)
if err != nil {
return fmt.Errorf("marshaling JSON: %w", err)
}
var data any
if err := json.Unmarshal(raw, &data); err != nil {
return fmt.Errorf("unmarshaling JSON: %w", err)
}
// Apply field selection if specified.
if fields != "" {
fieldList := strings.Split(fields, ",")
for i, f := range fieldList {
fieldList[i] = strings.TrimSpace(f)
}
data = selectFields(data, fieldList)
}
// Apply jq expression if specified.
if jqExpr != "" {
return applyJQ(data, jqExpr)
}
return writeJSON(data)
}
// selectFields filters a JSON value to only include the specified fields.
// Works on both single objects and arrays of objects.
func selectFields(data any, fields []string) any {
switch v := data.(type) {
case []any:
result := make([]any, len(v))
for i, item := range v {
result[i] = selectFields(item, fields)
}
return result
case map[string]any:
result := make(map[string]any)
for _, field := range fields {
if val, ok := v[field]; ok {
result[field] = val
}
}
return result
default:
return data
}
}
// applyJQ applies a jq expression to data and writes each output value.
func applyJQ(data any, expr string) error {
query, err := gojq.Parse(expr)
if err != nil {
return fmt.Errorf("invalid jq expression: %w", err)
}
iter := query.Run(data)
enc := json.NewEncoder(ios.Out)
enc.SetIndent("", " ")
for {
v, ok := iter.Next()
if !ok {
break
}
if err, isErr := v.(error); isErr {
return fmt.Errorf("jq error: %w", err)
}
// For string values, print raw (no JSON encoding) to match jq behavior.
if s, ok := v.(string); ok {
fmt.Fprintln(ios.Out, s)
} else {
if err := enc.Encode(v); err != nil {
return err
}
}
}
return nil
}