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.
270 lines
6.6 KiB
Go
270 lines
6.6 KiB
Go
package config
|
|
|
|
import (
|
|
"fmt"
|
|
"os"
|
|
"path/filepath"
|
|
"strings"
|
|
|
|
"github.com/spf13/viper"
|
|
"gopkg.in/yaml.v3"
|
|
)
|
|
|
|
type Config struct {
|
|
Hosts map[string]HostConfig `yaml:"hosts"`
|
|
}
|
|
|
|
// explicitConfigPath, when non-empty, overrides the default config file
|
|
// location for both Load() and Save(). It's set by cmd/root.initConfig when
|
|
// the user passes --config <path>. Stored at package scope so existing
|
|
// call sites of config.Load()/c.Save() continue to work without each one
|
|
// having to know about the flag.
|
|
var explicitConfigPath string
|
|
|
|
// SetExplicitConfigPath wires a user-supplied --config path through to
|
|
// Load/Save. Pass "" to clear.
|
|
func SetExplicitConfigPath(p string) { explicitConfigPath = p }
|
|
|
|
type HostConfig struct {
|
|
Hostname string `yaml:"hostname"`
|
|
Token string `yaml:"token"`
|
|
User string `yaml:"user,omitempty"`
|
|
GitProtocol string `yaml:"git_protocol,omitempty"`
|
|
MatchDirs []string `yaml:"match_dirs,omitempty"`
|
|
Order int `yaml:"-"` // config file order, set at load time
|
|
}
|
|
|
|
func GetConfigDir() (string, error) {
|
|
if xdgConfigHome := os.Getenv("XDG_CONFIG_HOME"); xdgConfigHome != "" {
|
|
return filepath.Join(xdgConfigHome, "fj"), nil
|
|
}
|
|
home, err := os.UserHomeDir()
|
|
if err != nil {
|
|
return "", err
|
|
}
|
|
return filepath.Join(home, ".config", "fj"), nil
|
|
}
|
|
|
|
func GetConfigPath() (string, error) {
|
|
if explicitConfigPath != "" {
|
|
return explicitConfigPath, nil
|
|
}
|
|
dir, err := GetConfigDir()
|
|
if err != nil {
|
|
return "", err
|
|
}
|
|
return filepath.Join(dir, "config.yaml"), nil
|
|
}
|
|
|
|
func Load() (*Config, error) {
|
|
path, err := GetConfigPath()
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
return LoadFromPath(path)
|
|
}
|
|
|
|
func LoadFromPath(path string) (*Config, error) {
|
|
data, err := os.ReadFile(path)
|
|
if err != nil {
|
|
if os.IsNotExist(err) {
|
|
return &Config{Hosts: make(map[string]HostConfig)}, nil
|
|
}
|
|
return nil, err
|
|
}
|
|
|
|
var cfg Config
|
|
if err := yaml.Unmarshal(data, &cfg); err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
if cfg.Hosts == nil {
|
|
cfg.Hosts = make(map[string]HostConfig)
|
|
}
|
|
|
|
// Parse again with yaml.Node to capture config file order for hosts
|
|
assignHostOrder(&cfg, data)
|
|
|
|
return &cfg, nil
|
|
}
|
|
|
|
// assignHostOrder walks the YAML document tree to find the "hosts" mapping
|
|
// and stamps each HostConfig.Order with its position in the file.
|
|
func assignHostOrder(cfg *Config, data []byte) {
|
|
var doc yaml.Node
|
|
if err := yaml.Unmarshal(data, &doc); err != nil || len(doc.Content) == 0 {
|
|
return
|
|
}
|
|
root := doc.Content[0] // mapping node
|
|
if root.Kind != yaml.MappingNode {
|
|
return
|
|
}
|
|
for i := 0; i+1 < len(root.Content); i += 2 {
|
|
if root.Content[i].Value == "hosts" {
|
|
hostsNode := root.Content[i+1]
|
|
if hostsNode.Kind != yaml.MappingNode {
|
|
return
|
|
}
|
|
order := 0
|
|
for j := 0; j+1 < len(hostsNode.Content); j += 2 {
|
|
key := hostsNode.Content[j].Value
|
|
if h, ok := cfg.Hosts[key]; ok {
|
|
h.Order = order
|
|
cfg.Hosts[key] = h
|
|
order++
|
|
}
|
|
}
|
|
return
|
|
}
|
|
}
|
|
}
|
|
|
|
func (c *Config) Save() error {
|
|
path, err := GetConfigPath()
|
|
if err != nil {
|
|
return err
|
|
}
|
|
return c.SaveToPath(path)
|
|
}
|
|
|
|
func (c *Config) SaveToPath(path string) error {
|
|
dir := filepath.Dir(path)
|
|
if err := os.MkdirAll(dir, 0755); err != nil {
|
|
return err
|
|
}
|
|
|
|
data, err := yaml.Marshal(c)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
|
|
return os.WriteFile(path, data, 0600)
|
|
}
|
|
|
|
// GetHost resolves the hostname to use for API client creation.
|
|
// Priority order:
|
|
// 1. Explicitly provided hostname parameter
|
|
// 2. CLI flag (--hostname)
|
|
// 3. Environment variable (FJ_HOST, with FGJ_HOST fallback)
|
|
// 4. Auto-detected hostname from git remote
|
|
// 5. match_dirs lookup (longest prefix match)
|
|
// 6. Default to codeberg.org
|
|
func (c *Config) GetHost(hostname string, detectedHost string, cwd string) (HostConfig, error) {
|
|
if hostname == "" {
|
|
hostname = viper.GetString("hostname")
|
|
}
|
|
|
|
if hostname == "" {
|
|
hostname = EnvWithFallback("FJ_HOST", "FGJ_HOST")
|
|
}
|
|
|
|
if hostname == "" {
|
|
hostname = detectedHost
|
|
}
|
|
|
|
if hostname == "" {
|
|
hostname = c.ResolveHostByPath(cwd)
|
|
}
|
|
|
|
if hostname == "" {
|
|
hostname = "codeberg.org"
|
|
}
|
|
|
|
host, ok := c.Hosts[hostname]
|
|
if !ok {
|
|
return HostConfig{}, fmt.Errorf("no configuration found for host %s", hostname)
|
|
}
|
|
|
|
return host, nil
|
|
}
|
|
|
|
// ResolveHostByPath finds the host whose match_dirs entry is the longest
|
|
// prefix of cwd. Returns "" if no match is found.
|
|
// Both cwd and match_dirs entries are resolved through filepath.EvalSymlinks
|
|
// to handle symlinks (e.g. macOS /tmp → /private/tmp).
|
|
// On ties (same prefix length from multiple hosts), the host appearing first
|
|
// in the config file wins and a warning is printed to stderr.
|
|
func (c *Config) ResolveHostByPath(cwd string) string {
|
|
if cwd == "" {
|
|
return ""
|
|
}
|
|
|
|
// Resolve symlinks in cwd so /tmp becomes /private/tmp on macOS, etc.
|
|
if resolved, err := filepath.EvalSymlinks(cwd); err == nil {
|
|
cwd = resolved
|
|
}
|
|
|
|
bestHost := ""
|
|
bestLen := 0
|
|
bestOrder := 0
|
|
tied := false
|
|
|
|
for hostname, host := range c.Hosts {
|
|
for _, dir := range host.MatchDirs {
|
|
if dir == "" {
|
|
continue
|
|
}
|
|
// Expand ~ to home directory
|
|
dir = expandHome(dir)
|
|
// Resolve symlinks in the configured dir as well
|
|
if resolved, err := filepath.EvalSymlinks(dir); err == nil {
|
|
dir = resolved
|
|
}
|
|
// Normalize: ensure trailing slash for prefix matching
|
|
prefix := dir
|
|
if !strings.HasSuffix(prefix, "/") {
|
|
prefix += "/"
|
|
}
|
|
// Match if cwd equals dir exactly or is under it
|
|
if cwd == dir || strings.HasPrefix(cwd, prefix) {
|
|
if len(dir) > bestLen {
|
|
bestLen = len(dir)
|
|
bestHost = hostname
|
|
bestOrder = host.Order
|
|
tied = false
|
|
} else if len(dir) == bestLen && hostname != bestHost {
|
|
// Tie — pick the host with the lower Order (earlier in config)
|
|
if host.Order < bestOrder {
|
|
bestHost = hostname
|
|
bestOrder = host.Order
|
|
}
|
|
tied = true
|
|
}
|
|
}
|
|
}
|
|
}
|
|
|
|
if tied {
|
|
fmt.Fprintf(os.Stderr, "warning: multiple hosts match directory %q with the same specificity; using %s (first in config)\n", cwd, bestHost)
|
|
}
|
|
|
|
return bestHost
|
|
}
|
|
|
|
// expandHome replaces a leading ~ with the user's home directory.
|
|
// EnvWithFallback returns the value of the primary env var, falling back to
|
|
// the legacy name if the primary is unset. This eases the FGJ_ → FJ_ rename.
|
|
func EnvWithFallback(primary, legacy string) string {
|
|
if v := os.Getenv(primary); v != "" {
|
|
return v
|
|
}
|
|
return os.Getenv(legacy)
|
|
}
|
|
|
|
func expandHome(path string) string {
|
|
if path == "~" || strings.HasPrefix(path, "~/") {
|
|
home, err := os.UserHomeDir()
|
|
if err != nil {
|
|
return path
|
|
}
|
|
return filepath.Join(home, path[1:])
|
|
}
|
|
return path
|
|
}
|
|
|
|
func (c *Config) SetHost(hostname string, host HostConfig) {
|
|
if c.Hosts == nil {
|
|
c.Hosts = make(map[string]HostConfig)
|
|
}
|
|
c.Hosts[hostname] = host
|
|
}
|