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.
137 lines
3.7 KiB
Go
137 lines
3.7 KiB
Go
package cmd
|
|
|
|
import (
|
|
"encoding/json"
|
|
"errors"
|
|
"strings"
|
|
|
|
"forgejo.zerova.net/public/fj/internal/api"
|
|
)
|
|
|
|
// Error codes for structured error output.
|
|
const (
|
|
ErrAuthRequired = "auth_required"
|
|
ErrNotFound = "not_found"
|
|
ErrAPIError = "api_error"
|
|
ErrInvalidInput = "invalid_input"
|
|
ErrGitDetectionFailed = "git_detection_failed"
|
|
ErrNetworkError = "network_error"
|
|
)
|
|
|
|
// CLIError is a structured error type for machine-readable output.
|
|
type CLIError struct {
|
|
Code string `json:"code"`
|
|
Message string `json:"message"`
|
|
Detail string `json:"detail,omitempty"`
|
|
Status int `json:"status,omitempty"`
|
|
// Hint is a separate field so JSON consumers get clean structure and
|
|
// the human renderer can append "Hint: ..." without polluting Message.
|
|
Hint string `json:"hint,omitempty"`
|
|
}
|
|
|
|
func (e *CLIError) Error() string {
|
|
if e.Hint != "" {
|
|
return e.Message + "\nHint: " + e.Hint
|
|
}
|
|
return e.Message
|
|
}
|
|
|
|
// NewCLIError creates a new CLIError with the given code and message.
|
|
func NewCLIError(code, message string) *CLIError {
|
|
return &CLIError{Code: code, Message: message}
|
|
}
|
|
|
|
// NewAPIError creates a CLIError from an HTTP status and message.
|
|
func NewAPIError(status int, message string) *CLIError {
|
|
return &CLIError{Code: ErrAPIError, Message: message, Status: status}
|
|
}
|
|
|
|
// ContextualError wraps common errors with helpful hints.
|
|
//
|
|
// Auth/404 hints come exclusively from a typed *api.APIError now — we used
|
|
// to substring-match "401"/"403" against the rendered error string, which
|
|
// would trigger an "auth login" hint for any error mentioning issue #403.
|
|
// If the API client doesn't surface APIError, no hint is added; that's a
|
|
// signal to fix the client wrapper, not to layer regex on top.
|
|
func ContextualError(err error) error {
|
|
if err == nil {
|
|
return nil
|
|
}
|
|
|
|
// If the error chain already holds a CLIError, leave it — it owns its
|
|
// Code/Hint already.
|
|
var cErr *CLIError
|
|
if errors.As(err, &cErr) {
|
|
return err
|
|
}
|
|
|
|
var apiErr *api.APIError
|
|
if errors.As(err, &apiErr) {
|
|
c := &CLIError{
|
|
Code: ErrAPIError,
|
|
Message: err.Error(),
|
|
Status: apiErr.StatusCode,
|
|
Detail: apiErr.Body,
|
|
}
|
|
switch apiErr.StatusCode {
|
|
case 401, 403:
|
|
c.Code = ErrAuthRequired
|
|
c.Hint = "Try authenticating with: fj auth login"
|
|
case 404:
|
|
c.Code = ErrNotFound
|
|
c.Hint = "Resource not found. Check the repository and number are correct."
|
|
}
|
|
return c
|
|
}
|
|
|
|
// Plain network errors come back as fmt.Errorf strings from net/http.
|
|
msg := err.Error()
|
|
switch {
|
|
case strings.Contains(msg, "no such host"),
|
|
strings.Contains(msg, "connection refused"),
|
|
strings.Contains(msg, "i/o timeout"):
|
|
return &CLIError{
|
|
Code: ErrNetworkError,
|
|
Message: msg,
|
|
Hint: "Check your internet connection and that the host is correct.",
|
|
}
|
|
}
|
|
|
|
return err
|
|
}
|
|
|
|
// WriteJSONError writes a structured JSON error to stderr.
|
|
// It is exported for use from main.go.
|
|
func WriteJSONError(err error) {
|
|
cliErr := &CLIError{
|
|
Code: ErrAPIError,
|
|
Message: err.Error(),
|
|
}
|
|
|
|
// Try to extract structured info from the error chain. Prefer CLIError
|
|
// (which carries Hint cleanly) over APIError so a wrapped CLIError
|
|
// keeps its structured fields.
|
|
var apiErr *api.APIError
|
|
var cErr *CLIError
|
|
|
|
switch {
|
|
case errors.As(err, &cErr):
|
|
cliErr = cErr
|
|
case errors.As(err, &apiErr):
|
|
cliErr.Status = apiErr.StatusCode
|
|
cliErr.Detail = apiErr.Body
|
|
switch {
|
|
case apiErr.StatusCode == 401 || apiErr.StatusCode == 403:
|
|
cliErr.Code = ErrAuthRequired
|
|
case apiErr.StatusCode == 404:
|
|
cliErr.Code = ErrNotFound
|
|
}
|
|
}
|
|
|
|
enc := json.NewEncoder(ios.ErrOut)
|
|
enc.SetIndent("", " ")
|
|
_ = enc.Encode(cliErr)
|
|
}
|
|
|
|
// Compile-time check that CLIError satisfies the standard error interface.
|
|
var _ error = (*CLIError)(nil)
|