From a362d037f135118776398070d391ebcc19fdcd70 Mon Sep 17 00:00:00 2001 From: jjuanrivvera99 Date: Wed, 10 Jun 2026 11:23:42 -0500 Subject: [PATCH 1/3] fix: harden the emit idempotency cache and surface silent truncations The emitted.json guard protecting against double electronic emission had three silent failure modes, all fixed: - A corrupt or unreadable cache was ignored and read as empty, which would re-emit already-stamped invoices. emit now aborts with a remediation hint. - The cache was persisted once after all batches; a crash mid-run lost every stamped id. It is now saved after each successful batch, and emission stops if the save fails (listing the affected ids). - The cache file was written in place; a crash mid-write could tear it. Writes now go through temp-file + rename. Also addressed from the same review: - output: auto-detected column sets capped at 10 now print a stderr note with the dropped count and a --columns/--output json hint (stdout stays clean for piping). - generic: list filters dropped over a flag collision or empty definition are recorded and asserted empty by a registry test, so a bad resource definition fails CI instead of silently losing the filter. - api: ID decoding tries Int64 before the float64 normalization path, so ids above 2^53 keep full precision. - api: AsAPIError delegates to errors.As, which also walks errors.Join trees the manual unwrap loop missed. - api: removed the unreachable lastErr branch after the retry loop. - api: ListAll's page-cap warning now includes a remediation hint. --- commands/emit.go | 53 +++++++++++++++++++++++++++---- commands/generic.go | 16 ++++++++-- commands/generic_more_test.go | 8 +++++ commands/helpers_more_test.go | 24 ++++++++++++-- commands/integration_more_test.go | 16 ++++++++++ internal/api/client.go | 7 ++-- internal/api/errors.go | 15 +++------ internal/api/errors_more_test.go | 17 ++++++++++ internal/api/resource.go | 3 +- internal/api/types.go | 6 ++++ internal/api/types_test.go | 3 ++ internal/output/output.go | 34 +++++++++++++++----- internal/output/render_test.go | 18 ++++++++--- 13 files changed, 180 insertions(+), 40 deletions(-) diff --git a/commands/emit.go b/commands/emit.go index 98fb6639..8e08829e 100644 --- a/commands/emit.go +++ b/commands/emit.go @@ -2,6 +2,7 @@ package commands import ( "encoding/json" + "errors" "fmt" "os" "path/filepath" @@ -60,9 +61,14 @@ emission is NOT idempotent on Alegra's side, so this prevents duplicates.`, return fmt.Errorf("no invoices to emit (pass ids or --all)") } - // 2. Idempotency guard. + // 2. Idempotency guard. A cache that exists but cannot be read is a + // hard stop: proceeding with an empty guard could re-emit invoices + // that were already stamped (a fiscal duplicate, not undoable). profile := currentProfileName() - cache, _ := loadEmitCache() + cache, cerr := loadEmitCache() + if cerr != nil { + return fmt.Errorf("cannot read the emission idempotency cache: %w\nInspect or remove %s, then retry", cerr, emitCachePath()) + } todo, skipped := filterEmitted(ids, cache, profile, force) out := cmd.OutOrStdout() if len(skipped) > 0 { @@ -92,9 +98,14 @@ emission is NOT idempotent on Alegra's side, so this prevents duplicates.`, emitted++ } fmt.Fprintf(out, "stamped: %v\n", batch) + // Persist after every batch, and stop on failure: stamping more + // invoices the guard cannot record would risk re-emission on the + // next run. + if werr := saveEmitCache(cache); werr != nil { + return fmt.Errorf("stamped %v but could not record them in the idempotency cache: %w\nRecord these ids in %s before re-running, or a re-run may emit them twice", batch, werr, emitCachePath()) + } } if !flagDryRun { - _ = saveEmitCache(cache) fmt.Fprintf(out, "Emitted %d invoice(s); %d batch(es) failed.\n", emitted, failedChunks) } if failedChunks > 0 { @@ -146,26 +157,54 @@ func emitCachePath() string { return filepath.Join(filepath.Dir(config.DefaultPath()), "emitted.json") } +// loadEmitCache reads the emitted-ids cache. A missing file is a fresh start; +// an unreadable or corrupt file is an error — silently treating it as empty +// would drop the idempotency guard and allow double emission. func loadEmitCache() (map[string]bool, error) { cache := map[string]bool{} data, err := os.ReadFile(emitCachePath()) //nolint:gosec // path under config dir + if errors.Is(err, os.ErrNotExist) { + return cache, nil + } if err != nil { - return cache, err + return nil, err + } + if err := json.Unmarshal(data, &cache); err != nil { + return nil, fmt.Errorf("corrupt cache %s: %w", emitCachePath(), err) } - _ = json.Unmarshal(data, &cache) return cache, nil } +// saveEmitCache persists atomically (write temp + rename): a crash mid-write +// must never leave a torn emitted.json, which would read as corrupt and block +// (or, worse, lose) the guard. func saveEmitCache(cache map[string]bool) error { path := emitCachePath() - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { return err } data, err := json.Marshal(cache) if err != nil { return err } - return os.WriteFile(path, data, 0o600) + tmp, err := os.CreateTemp(dir, "emitted-*.json") + if err != nil { + return err + } + defer func() { _ = os.Remove(tmp.Name()) }() // no-op once renamed + if err := tmp.Chmod(0o600); err != nil { + _ = tmp.Close() + return err + } + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(tmp.Name(), path) } // currentProfileName returns the active profile name for cache scoping. diff --git a/commands/generic.go b/commands/generic.go index 8a799a8b..fecdf858 100644 --- a/commands/generic.go +++ b/commands/generic.go @@ -131,6 +131,12 @@ func buildResourceCmd[T any](sp resourceSpec[T]) *cobra.Command { // --- list --- +// droppedListFilters records resource filters that were skipped at registration +// because of an empty definition or a flag collision. The CLI must never panic +// at init over a bad resource definition, so the mistake is surfaced by a +// registry test (TestNoListFiltersAreSilentlyDropped) failing CI instead. +var droppedListFilters []string + // reservedListFlags are the built-in flag names on every `list` subcommand; // resource-specific filters that collide with these are skipped. var reservedListFlags = map[string]bool{ @@ -245,9 +251,15 @@ func newListCmd[T any](sp resourceSpec[T]) *cobra.Command { fs.StringVar(&lf.orderDir, "order-direction", "", "Sort direction: ASC or DESC") // Register resource-specific filters, skipping any that would collide with a // built-in list flag or a previously declared filter (defensive: a bad - // resource definition must never panic the whole CLI at init). + // resource definition must never panic the whole CLI at init). Skips are + // recorded so the registry test fails CI instead of losing filters silently. for _, f := range sp.ListFilters { - if f.Flag == "" || f.Query == "" || reservedListFlags[f.Flag] || fs.Lookup(f.Flag) != nil { + if f.Flag == "" || f.Query == "" { + droppedListFilters = append(droppedListFilters, fmt.Sprintf("%s: filter %+v has an empty flag or query", sp.Use, f)) + continue + } + if reservedListFlags[f.Flag] || fs.Lookup(f.Flag) != nil { + droppedListFilters = append(droppedListFilters, fmt.Sprintf("%s: --%s collides with a built-in or duplicate flag", sp.Use, f.Flag)) continue } filterVals[f.Query] = fs.String(f.Flag, "", f.Usage) diff --git a/commands/generic_more_test.go b/commands/generic_more_test.go index dd6285d8..f848dad2 100644 --- a/commands/generic_more_test.go +++ b/commands/generic_more_test.go @@ -87,3 +87,11 @@ func TestConfirm(t *testing.T) { assert.False(t, confirm(cmd, "proceed"), "input %q", in) } } + +// Every registered resource builds its list command at init, so by the time +// tests run any filter dropped over an empty definition or a flag collision is +// already recorded. An entry here means a resource silently lost a filter — +// rename the flag in the resource definition. +func TestNoListFiltersAreSilentlyDropped(t *testing.T) { + assert.Empty(t, droppedListFilters) +} diff --git a/commands/helpers_more_test.go b/commands/helpers_more_test.go index 7b66daa0..ed7606c1 100644 --- a/commands/helpers_more_test.go +++ b/commands/helpers_more_test.go @@ -5,6 +5,7 @@ import ( "context" "net/http" "net/http/httptest" + "os" "path/filepath" "testing" @@ -58,9 +59,9 @@ func TestEmitCacheRoundTrip(t *testing.T) { dir := t.TempDir() t.Setenv(config.EnvConfig, filepath.Join(dir, "config.yaml")) - // Missing cache: empty map + a read error. + // Missing cache is a fresh start, not an error. cache, err := loadEmitCache() - assert.Error(t, err) + require.NoError(t, err) assert.Empty(t, cache) require.NoError(t, saveEmitCache(map[string]bool{"prod:5": true})) @@ -69,6 +70,25 @@ func TestEmitCacheRoundTrip(t *testing.T) { cache, err = loadEmitCache() require.NoError(t, err) assert.True(t, cache["prod:5"]) + + // The atomic write must not leave temp files behind. + entries, rerr := os.ReadDir(dir) + require.NoError(t, rerr) + for _, e := range entries { + assert.NotContains(t, e.Name(), "emitted-", "leftover temp file %s", e.Name()) + } +} + +func TestLoadEmitCache_CorruptFileIsAnError(t *testing.T) { + dir := t.TempDir() + t.Setenv(config.EnvConfig, filepath.Join(dir, "config.yaml")) + require.NoError(t, os.WriteFile(filepath.Join(dir, "emitted.json"), []byte("{not json"), 0o600)) + + // A corrupt cache must never read as empty: that would drop the + // idempotency guard and allow double emission. + _, err := loadEmitCache() + require.Error(t, err) + assert.Contains(t, err.Error(), "corrupt cache") } func TestRootCmd(t *testing.T) { diff --git a/commands/integration_more_test.go b/commands/integration_more_test.go index ec752eb8..e26d14d6 100644 --- a/commands/integration_more_test.go +++ b/commands/integration_more_test.go @@ -169,3 +169,19 @@ func TestIntegration_ExtendedCommands(t *testing.T) { require.NoError(t, err) }) } + +// A corrupt emitted.json must stop emission before any API call: proceeding +// with an empty guard could double-emit already-stamped invoices. +func TestEmit_AbortsOnCorruptCache(t *testing.T) { + dir := t.TempDir() + t.Setenv(config.EnvBaseURL, "http://127.0.0.1:1") + t.Setenv(config.EnvEmail, "e@x.com") + t.Setenv(config.EnvToken, "tok") + t.Setenv(config.EnvProfile, "") + t.Setenv(config.EnvConfig, filepath.Join(dir, "config.yaml")) + require.NoError(t, os.WriteFile(filepath.Join(dir, "emitted.json"), []byte("{corrupt"), 0o600)) + + _, err := runRoot(t, "invoices", "emit", "7") + require.Error(t, err) + assert.Contains(t, err.Error(), "idempotency cache") +} diff --git a/internal/api/client.go b/internal/api/client.go index 0e3800c9..a1e4ec34 100644 --- a/internal/api/client.go +++ b/internal/api/client.go @@ -217,7 +217,6 @@ func (c *Client) do(ctx context.Context, method, path string, query url.Values, return nil, errDryRun } - var lastErr error for attempt := 0; attempt <= c.retryPolicy.MaxRetries; attempt++ { if err := c.rateLimiter.Wait(ctx); err != nil { return nil, err @@ -254,7 +253,6 @@ func (c *Client) do(ctx context.Context, method, path string, query url.Values, return nil, ctx.Err() case <-time.After(wait): } - lastErr = err continue } @@ -276,9 +274,8 @@ func (c *Client) do(ctx context.Context, method, path string, query url.Values, return respBody, nil } - if lastErr != nil { - return nil, fmt.Errorf("alegra: exhausted retries: %w", lastErr) - } + // Unreachable with a sane policy (MaxRetries >= 0): the final attempt always + // returns inside the loop. Kept as a guard against a negative MaxRetries. return nil, errors.New("alegra: exhausted retries") } diff --git a/internal/api/errors.go b/internal/api/errors.go index 72578968..19348f2c 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -2,6 +2,7 @@ package api import ( "encoding/json" + "errors" "fmt" "net/http" "regexp" @@ -124,16 +125,8 @@ func parseAPIError(statusCode int, body []byte) *APIError { // AsAPIError extracts an *APIError from an error chain, if present. func AsAPIError(err error) (*APIError, bool) { var apiErr *APIError - for err != nil { - if e, ok := err.(*APIError); ok { - return e, true - } - type unwrapper interface{ Unwrap() error } - u, ok := err.(unwrapper) - if !ok { - break - } - err = u.Unwrap() + if errors.As(err, &apiErr) { + return apiErr, true } - return apiErr, false + return nil, false } diff --git a/internal/api/errors_more_test.go b/internal/api/errors_more_test.go index 7b2a7f0f..c04f324e 100644 --- a/internal/api/errors_more_test.go +++ b/internal/api/errors_more_test.go @@ -60,3 +60,20 @@ func TestAsAPIError(t *testing.T) { _, ok = AsAPIError(nil) assert.False(t, ok) } + +func TestAsAPIError_UnwrapsChainsAndJoins(t *testing.T) { + apiErr := &APIError{StatusCode: 404, Message: "not found"} + + got, ok := AsAPIError(fmt.Errorf("wrapped: %w", apiErr)) + assert.True(t, ok) + assert.Same(t, apiErr, got) + + // errors.Join produces a multi-error tree; only errors.As walks it. + got, ok = AsAPIError(errors.Join(errors.New("other"), apiErr)) + assert.True(t, ok) + assert.Same(t, apiErr, got) + + got, ok = AsAPIError(errors.New("plain")) + assert.False(t, ok) + assert.Nil(t, got) +} diff --git a/internal/api/resource.go b/internal/api/resource.go index b28a8723..e531d561 100644 --- a/internal/api/resource.go +++ b/internal/api/resource.go @@ -109,7 +109,8 @@ func (r *Resource[T]) ListAll(ctx context.Context, params ListParams, maxPages i } // Reached the page cap with a full final page: more records may exist. r.client.logger.Warn("alegra: --all stopped at the page cap; results may be truncated", - "resource", r.path, "fetched", len(all), "max_pages", maxPages) + "resource", r.path, "fetched", len(all), "max_pages", maxPages, + "hint", "narrow the query with filters or a date range") return all, nil } diff --git a/internal/api/types.go b/internal/api/types.go index 931ddd15..c0b0cbb4 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -31,6 +31,12 @@ func (i *ID) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &n); err != nil { return err } + // Int64 first: round-tripping through float64 would silently lose + // precision for ids above 2^53. + if v, err := n.Int64(); err == nil { + *i = ID(strconv.FormatInt(v, 10)) + return nil + } if f, err := n.Float64(); err == nil && f == float64(int64(f)) { *i = ID(strconv.FormatInt(int64(f), 10)) return nil diff --git a/internal/api/types_test.go b/internal/api/types_test.go index 6819c293..1d203d8a 100644 --- a/internal/api/types_test.go +++ b/internal/api/types_test.go @@ -17,6 +17,9 @@ func TestID_Unmarshal(t *testing.T) { `null`: "", `""`: "", `"abc1"`: "abc1", + // above 2^53: must not round-trip through float64 + `9007199254740993`: "9007199254740993", + `9223372036854775807`: "9223372036854775807", } for in, want := range cases { var id ID diff --git a/internal/output/output.go b/internal/output/output.go index 8860f8a2..b1f6c290 100644 --- a/internal/output/output.go +++ b/internal/output/output.go @@ -8,6 +8,7 @@ import ( "encoding/json" "fmt" "io" + "os" "sort" "strconv" "strings" @@ -83,7 +84,8 @@ func renderCSV(w io.Writer, data any, columns []string) error { if len(rows) == 0 { return nil } - cols := resolveColumns(rows, columns) + cols, dropped := resolveColumns(rows, columns) + warnDroppedColumns(dropped) cw := csv.NewWriter(w) defer cw.Flush() if err := cw.Write(cols); err != nil { @@ -134,7 +136,8 @@ func renderTable(w io.Writer, data any, columns []string) error { if len(rows) == 1 { return renderKeyValue(w, rows[0], columns) } - cols := resolveColumns(rows, columns) + cols, dropped := resolveColumns(rows, columns) + warnDroppedColumns(dropped) tw := tabwriter.NewWriter(w, 0, 2, 2, ' ', 0) fmt.Fprintln(tw, strings.Join(upper(cols), "\t")) for _, row := range rows { @@ -206,11 +209,16 @@ func toObject(data any) (map[string]any, bool) { return m, ok } +// maxAutoColumns caps the auto-detected column set so wide resources stay +// readable; explicit --columns selections are never capped. +const maxAutoColumns = 10 + // resolveColumns returns the explicit columns (filtered to ones present) or -// derives a scalar-only column set ordered by preference. -func resolveColumns(rows []map[string]any, columns []string) []string { +// derives a scalar-only column set ordered by preference, plus how many +// detected columns were dropped by the cap. +func resolveColumns(rows []map[string]any, columns []string) ([]string, int) { if len(columns) > 0 { - return columns + return columns, 0 } seen := map[string]bool{} for _, row := range rows { @@ -221,10 +229,20 @@ func resolveColumns(rows []map[string]any, columns []string) []string { } } cols := orderKeys(seen) - if len(cols) > 10 { - cols = cols[:10] + dropped := 0 + if len(cols) > maxAutoColumns { + dropped = len(cols) - maxAutoColumns + cols = cols[:maxAutoColumns] + } + return cols, dropped +} + +// warnDroppedColumns surfaces the auto-detect cap on stderr; stdout must stay +// clean for piping (a note inside CSV/table data would corrupt it). +func warnDroppedColumns(dropped int) { + if dropped > 0 { + fmt.Fprintf(os.Stderr, "note: %d more column(s) detected but not shown; use --columns to choose, or --output json for everything\n", dropped) } - return cols } func orderedKeys(obj map[string]any, includeAll bool) []string { diff --git a/internal/output/render_test.go b/internal/output/render_test.go index eda70e16..0680dff2 100644 --- a/internal/output/render_test.go +++ b/internal/output/render_test.go @@ -84,11 +84,14 @@ func TestRenderUnsupportedFormat(t *testing.T) { func TestResolveColumns(t *testing.T) { rows := []map[string]any{{"id": "1", "name": "A", "nested": map[string]any{"x": 1}}} // Explicit columns pass through unchanged. - assert.Equal(t, []string{"name", "id"}, resolveColumns(rows, []string{"name", "id"})) + cols, dropped := resolveColumns(rows, []string{"name", "id"}) + assert.Equal(t, []string{"name", "id"}, cols) + assert.Zero(t, dropped) // Derived columns: scalars only, preferred order first; nested map excluded. - got := resolveColumns(rows, nil) + got, dropped := resolveColumns(rows, nil) assert.Equal(t, []string{"id", "name"}, got) + assert.Zero(t, dropped) assert.NotContains(t, got, "nested") } @@ -97,8 +100,15 @@ func TestResolveColumns_CapsAtTen(t *testing.T) { for _, k := range []string{"c01", "c02", "c03", "c04", "c05", "c06", "c07", "c08", "c09", "c10", "c11", "c12"} { row[k] = "v" } - got := resolveColumns([]map[string]any{row}, nil) - assert.Len(t, got, 10) + got, dropped := resolveColumns([]map[string]any{row}, nil) + assert.Len(t, got, maxAutoColumns) + assert.Equal(t, 2, dropped) + + // Explicit selection is never capped. + explicit := []string{"c01", "c02", "c03", "c04", "c05", "c06", "c07", "c08", "c09", "c10", "c11", "c12"} + got, dropped = resolveColumns([]map[string]any{row}, explicit) + assert.Len(t, got, 12) + assert.Zero(t, dropped) } func TestScalarStringAndValueString(t *testing.T) { From bd56a679a52850167bb8aa190f4af8b84903fa85 Mon Sep 17 00:00:00 2001 From: jjuanrivvera99 Date: Wed, 10 Jun 2026 13:20:38 -0500 Subject: [PATCH 2/3] fix: reject NaN/Inf in Money and Int, keep Int precision, make config save atomic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found while hardening the test suite (the new fuzz properties surfaced the first two within seconds of fuzzing): - Money accepted the string "NaN"/"Inf" via ParseFloat; the resulting value poisons any later json.Marshal of the containing document (JSON cannot represent NaN). Both decoders now reject non-finite values. - Int round-tripped bare and quoted integers through float64, losing precision above 2^53 — same bug just fixed in ID; now tries int64 first. - import dry-run and create --draft ignored json.Marshal errors, which could print garbage or send a stale body; both now fail the row/command. - auth status ignored the config.Load error and would dereference a nil config if the file were unreadable; it now propagates the error. - config.Save writes via temp + rename (same crash-safety treatment as emitted.json); country auto-detect's best-effort save is now documented as deliberate. --- commands/auth.go | 5 ++++- commands/country.go | 2 ++ commands/generic.go | 16 +++++++++++----- internal/api/types.go | 29 +++++++++++++++++++++++++++-- internal/config/config.go | 20 +++++++++++++++++++- 5 files changed, 63 insertions(+), 9 deletions(-) diff --git a/commands/auth.go b/commands/auth.go index 600c8289..0a24c4a0 100644 --- a/commands/auth.go +++ b/commands/auth.go @@ -109,7 +109,10 @@ func newAuthStatusCmd() *cobra.Command { if err != nil { return err } - cfg, _ := config.Load() + cfg, err := config.Load() + if err != nil { + return err + } profile := cfg.ActiveProfileName(flagProfile) r := cfg.Resolve(profile) out := cmd.OutOrStdout() diff --git a/commands/country.go b/commands/country.go index 8bcbb218..fe031e2c 100644 --- a/commands/country.go +++ b/commands/country.go @@ -36,5 +36,7 @@ func cacheCountry(cfg *config.Config, profile, country string) { } p.Country = country cfg.SetProfile(p) + // Best-effort by design: the cache only saves a future detection call, so + // a failed write must never break the command that triggered it. _ = cfg.Save() } diff --git a/commands/generic.go b/commands/generic.go index fecdf858..69fd7d02 100644 --- a/commands/generic.go +++ b/commands/generic.go @@ -427,18 +427,21 @@ failures are reported and do not stop the run.`, setDotPath(body, field, inferValue(cell)) } if flagDryRun { - raw, _ := json.Marshal(body) + raw, merr := json.Marshal(body) + if merr != nil { + failed++ + fmt.Fprintf(cmd.ErrOrStderr(), "[row %d] FAILED: cannot encode body: %v\n", i+1, merr) + continue + } fmt.Fprintf(out, "[row %d] would create: %s\n", i+1, raw) continue } - item, cerr := res.Create(cmd.Context(), body) - if cerr != nil { + if _, cerr := res.Create(cmd.Context(), body); cerr != nil { failed++ fmt.Fprintf(cmd.ErrOrStderr(), "[row %d] FAILED: %v\n", i+1, cerr) continue } created++ - _ = item fmt.Fprintf(out, "[row %d] created\n", i+1) } if !flagDryRun { @@ -553,7 +556,10 @@ func newCreateCmd[T any](sp resourceSpec[T]) *cobra.Command { if draft { if m, ok := bodyToMap(body); ok { delete(m, "stamp") - body, _ = json.Marshal(m) + body, err = json.Marshal(m) + if err != nil { + return fmt.Errorf("re-encoding body after stripping stamp: %w", err) + } } } if !noValidate { diff --git a/internal/api/types.go b/internal/api/types.go index c0b0cbb4..638d3f2d 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -3,6 +3,8 @@ package api import ( "bytes" "encoding/json" + "fmt" + "math" "strconv" ) @@ -77,15 +79,33 @@ func (i *Int) UnmarshalJSON(data []byte) error { *i = 0 return nil } + // ParseInt first: the float64 fallback (for "30.0"-style strings) + // would lose precision above 2^53. + if v, err := strconv.ParseInt(s, 10, 64); err == nil { + *i = Int(v) + return nil + } f, err := strconv.ParseFloat(s, 64) if err != nil { return err } + // int64(NaN/Inf) is implementation-defined; reject instead. + if math.IsNaN(f) || math.IsInf(f, 0) { + return fmt.Errorf("invalid integer value %q", s) + } *i = Int(int64(f)) return nil } - var f float64 - if err := json.Unmarshal(data, &f); err != nil { + var n json.Number + if err := json.Unmarshal(data, &n); err != nil { + return err + } + if v, err := n.Int64(); err == nil { + *i = Int(v) + return nil + } + f, err := n.Float64() + if err != nil { return err } *i = Int(int64(f)) @@ -155,6 +175,11 @@ func (m *Money) UnmarshalJSON(data []byte) error { if err != nil { return err } + // ParseFloat accepts "NaN"/"Inf", which JSON cannot represent: the + // value would poison any later re-marshal of the document. + if math.IsNaN(f) || math.IsInf(f, 0) { + return fmt.Errorf("invalid money value %q", s) + } *m = Money(f) return nil } diff --git a/internal/config/config.go b/internal/config/config.go index 86fa46f7..fb90c744 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -118,6 +118,8 @@ func Load() (*Config, error) { func (c *Config) Path() string { return c.path } // Save writes the config to disk (0600), creating the directory as needed. +// The write is atomic (temp + rename) so a crash mid-write can never leave a +// torn config.yaml behind. func (c *Config) Save() error { dir := filepath.Dir(c.path) if err := os.MkdirAll(dir, 0o700); err != nil { @@ -127,7 +129,23 @@ func (c *Config) Save() error { if err != nil { return fmt.Errorf("encoding config: %w", err) } - if err := os.WriteFile(c.path, data, 0o600); err != nil { + tmp, err := os.CreateTemp(dir, "config-*.yaml") + if err != nil { + return fmt.Errorf("writing config: %w", err) + } + defer func() { _ = os.Remove(tmp.Name()) }() // no-op once renamed + if err := tmp.Chmod(0o600); err != nil { + _ = tmp.Close() + return fmt.Errorf("writing config: %w", err) + } + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return fmt.Errorf("writing config: %w", err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("writing config: %w", err) + } + if err := os.Rename(tmp.Name(), c.path); err != nil { return fmt.Errorf("writing config: %w", err) } return nil From c15f85d5815028ef6b20a070f4213d5ce6bc72c6 Mon Sep 17 00:00:00 2001 From: jjuanrivvera99 Date: Wed, 10 Jun 2026 13:21:00 -0500 Subject: [PATCH 3/3] =?UTF-8?q?test:=20strengthen=20suite=20effectiveness?= =?UTF-8?q?=20=E2=80=94=20failure=20paths,=20fuzz=20properties,=20negative?= =?UTF-8?q?=20inputs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The suite was overwhelmingly happy-path (26 error assertions across 241 test functions; integration fake servers only ever returned 200/404). This adds the failure-path coverage a recent review showed was missing: - Fuzzers now assert value-level properties instead of "doesn't panic": integer literals survive ID/Int decoding verbatim, Money matches ParseFloat exactly, StringOrSlice keeps single strings intact. The Money property found the NaN bug within seconds; its counterexample is committed as a permanent corpus regression. - commands: partial import fails loudly with per-row errors and non-zero exit; emit keeps failed batches out of the idempotency cache; a cache save failure stops emission before the next batch; delete without confirmation never sends the DELETE; auth login never persists the token into config.yaml. - internal/api: malformed JSON on 200, HTML error bodies, context cancellation mid-backoff, the 429 throttle/restore cycle, HTTP-date Retry-After fallback, missing rate-limit headers, the subscriptions list wrapper, and empty wrapper arrays. - internal/config: corrupt YAML is an error, save leaves no temp files behind. - internal/auth: a locked keyring backend degrades to env/config fallback. - internal/output: key/value column filtering and ordering; pinned the drop-non-map-rows normalization so changing it is a conscious decision. - CONTRIBUTING: resource tests should cover what is unique to the resource, not re-test generic CRUD; documented the failure-path and fuzzing expectations. TestCurrentProfileName now pins flagProfile: runRoot resets globals before each run, not after, so a prior test's --profile leaked into direct helper calls depending on file order. --- CONTRIBUTING.md | 16 +- commands/failures_test.go | 165 ++++++++++++++++++ commands/helpers_more_test.go | 5 + internal/api/client_failures_test.go | 113 ++++++++++++ internal/api/fuzz_test.go | 66 ++++++- internal/api/resource_test.go | 17 +- .../testdata/fuzz/FuzzMoney/c0c1d566e1d675d2 | 2 + internal/api/types_test.go | 16 ++ internal/auth/auth_test.go | 16 ++ internal/config/config_test.go | 33 ++++ internal/output/render_test.go | 19 ++ 11 files changed, 455 insertions(+), 13 deletions(-) create mode 100644 commands/failures_test.go create mode 100644 internal/api/client_failures_test.go create mode 100644 internal/api/testdata/fuzz/FuzzMoney/c0c1d566e1d675d2 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 820e3e6a..d84f2f33 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -58,7 +58,11 @@ A resource needs three files and no edits to shared code: ``` 3. `internal/api/_test.go` — an httptest-based service test (reuse the - `newTestClient` helper). + `newTestClient` helper). Test what is **unique** to the resource — special + field types, custom actions, odd response shapes — not the generic CRUD + plumbing, which is already covered once by the `Resource[T]` tests + (`resource_test.go`, `client_failures_test.go`). A List/Get happy-path pair + adds volume, not signal. Custom actions (e.g. `void`, `email`) go through the `Extra` hook using `NewActionCmd` / `NewCollectionActionCmd`. Non-CRUD resources (singletons, @@ -82,6 +86,16 @@ reports) build a plain cobra command using `client.GetInto/PostInto/PutInto`. `internal/api`). - Prefer `require` for fatal assertions, `assert` for the rest. - Keep coverage healthy — the suite sits above 80%; new code should ship tests. +- **Test failure paths, not just happy paths.** Every parse of external state + (API bodies, config files, caches) needs a test with corrupt input; every + batch operation needs a partial-failure test asserting counts and a non-zero + exit. Coverage measures execution, not assertion quality — a swallowed error + can be 100% "covered" and still hide a bug. +- The flexible JSON types have fuzzers with value-level properties + (`internal/api/fuzz_test.go`); run them after touching a decoder: + `go test ./internal/api -fuzz '^FuzzID$' -fuzztime 30s` (likewise `FuzzInt`, + `FuzzMoney`, `FuzzStringOrSlice`). Counterexamples land in `testdata/fuzz/` + and become permanent regression cases — commit them. ## Reporting bugs & security issues diff --git a/commands/failures_test.go b/commands/failures_test.go new file mode 100644 index 00000000..ccedb495 --- /dev/null +++ b/commands/failures_test.go @@ -0,0 +1,165 @@ +package commands + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "strings" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/zalando/go-keyring" + + "github.com/jjuanrivvera/alegra-cli/internal/config" +) + +// These tests exercise the failure paths the integration suite never hit: the +// fake servers previously only returned 200/404, so partial batch failures, +// API errors mid-operation, and persistence failures were all untested. + +// failureTestEnv points the CLI at srv with env-only credentials and an +// isolated config dir, returning that dir. +func failureTestEnv(t *testing.T, srv *httptest.Server) string { + t.Helper() + dir := t.TempDir() + t.Setenv(config.EnvBaseURL, srv.URL) + t.Setenv(config.EnvEmail, "e@x.com") + t.Setenv(config.EnvToken, "tok") + t.Setenv(config.EnvProfile, "") + t.Setenv(config.EnvConfig, filepath.Join(dir, "config.yaml")) + return dir +} + +func TestImport_PartialFailureFailsLoudly(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + body, _ := io.ReadAll(r.Body) + var m map[string]any + _ = json.Unmarshal(body, &m) + if m["name"] == "Bad" { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"message":"name is invalid","code":400}`)) + return + } + _, _ = w.Write([]byte(`{"id":"9","name":"ok"}`)) + })) + t.Cleanup(srv.Close) + dir := failureTestEnv(t, srv) + + csvFile := filepath.Join(dir, "rows.csv") + require.NoError(t, os.WriteFile(csvFile, []byte("name\nGood\nBad\nAlsoGood\n"), 0o600)) + + out, err := runRoot(t, "contacts", "import", "-f", csvFile) + // A partial import must fail the command (exit code), report which row + // broke, and still account for the rows that were created. + require.Error(t, err, "partial failure must not look like success to pipelines") + assert.Contains(t, err.Error(), "1 row(s) failed") + assert.Contains(t, out, "[row 2] FAILED") + assert.Contains(t, out, "Imported 2, failed 1") +} + +func TestEmit_BatchAPIErrorFailsAndKeepsCacheClean(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if strings.HasSuffix(r.URL.Path, "/invoices/stamp") { + w.WriteHeader(http.StatusBadRequest) // 4xx: not retried, fails fast + _, _ = w.Write([]byte(`{"message":"EPR001: certificado vencido"}`)) + return + } + _, _ = w.Write([]byte(`{}`)) + })) + t.Cleanup(srv.Close) + dir := failureTestEnv(t, srv) + + out, err := runRoot(t, "invoices", "emit", "7", "--force") + require.Error(t, err) + assert.Contains(t, err.Error(), "1 batch(es) failed") + assert.Contains(t, out, "FAILED") + + // A failed batch must never be recorded as emitted. + data, rerr := os.ReadFile(filepath.Join(dir, "emitted.json")) + if rerr == nil { + assert.NotContains(t, string(data), ":7", "failed invoice must not be marked emitted") + } +} + +func TestEmit_CacheSaveFailureStopsEmission(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("read-only directory permissions are not enforced on Windows") + } + var stamps int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if strings.HasSuffix(r.URL.Path, "/invoices/stamp") { + atomic.AddInt32(&stamps, 1) + _, _ = w.Write([]byte(`{"stamped":true}`)) + return + } + _, _ = w.Write([]byte(`{}`)) + })) + t.Cleanup(srv.Close) + dir := failureTestEnv(t, srv) + + // Make the config dir unwritable so the post-batch cache save fails. + require.NoError(t, os.Chmod(dir, 0o500)) + t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) + + // 12 ids → 2 batches. The save failure after batch 1 must stop the run + // before batch 2 is stamped, and the error must name the affected ids. + ids := []string{"1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12"} + out, err := runRoot(t, append([]string{"invoices", "emit", "--force"}, ids...)...) + require.Error(t, err) + assert.Contains(t, err.Error(), "could not record them in the idempotency cache") + assert.Contains(t, err.Error(), "1") // affected ids are listed + assert.Contains(t, out, "stamped") + assert.Equal(t, int32(1), atomic.LoadInt32(&stamps), "must stop stamping once the guard cannot record progress") +} + +func TestDelete_AbortsWithoutConfirmation(t *testing.T) { + var deletes int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodDelete { + atomic.AddInt32(&deletes, 1) + } + _, _ = w.Write([]byte(`{}`)) + })) + t.Cleanup(srv.Close) + failureTestEnv(t, srv) + + // Piped/empty stdin without --yes: the prompt cannot be answered, so the + // delete must abort — and the DELETE request must never be sent. + rootCmd.SetIn(strings.NewReader("")) + t.Cleanup(func() { rootCmd.SetIn(nil) }) + + _, err := runRoot(t, "contacts", "delete", "1") + require.Error(t, err) + assert.Contains(t, err.Error(), "aborted") + assert.Zero(t, atomic.LoadInt32(&deletes), "no DELETE may be sent without confirmation") +} + +func TestAuthLogin_NeverPersistsTokenToConfig(t *testing.T) { + keyring.MockInit() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"name":"Tester","applicationVersion":"colombia"}`)) + })) + t.Cleanup(srv.Close) + dir := failureTestEnv(t, srv) + t.Setenv(config.EnvToken, "") // force login to use the flag token + + const secret = "super-secret-token" + _, err := runRoot(t, "auth", "login", "--email", "a@x.com", "--token", secret, "--profile", "p1") + require.NoError(t, err) + + // The token belongs in the keyring only; the YAML must never contain it. + data, rerr := os.ReadFile(filepath.Join(dir, "config.yaml")) + require.NoError(t, rerr) + assert.NotContains(t, string(data), secret, "plaintext token leaked into config.yaml") +} diff --git a/commands/helpers_more_test.go b/commands/helpers_more_test.go index ed7606c1..a4a2c892 100644 --- a/commands/helpers_more_test.go +++ b/commands/helpers_more_test.go @@ -131,6 +131,11 @@ func TestCurrentProfileName(t *testing.T) { dir := t.TempDir() t.Setenv(config.EnvConfig, filepath.Join(dir, "config.yaml")) t.Setenv(config.EnvProfile, "") + // runRoot resets flag globals before each run, not after — a prior test's + // --profile would otherwise leak into this direct helper call. + prev := flagProfile + flagProfile = "" + t.Cleanup(func() { flagProfile = prev }) cfg := config.New() cfg.DefaultProfile = "prod" diff --git a/internal/api/client_failures_test.go b/internal/api/client_failures_test.go new file mode 100644 index 00000000..186ae919 --- /dev/null +++ b/internal/api/client_failures_test.go @@ -0,0 +1,113 @@ +package api + +import ( + "context" + "net/http" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// These tests pin the client's behavior under hostile responses: malformed +// bodies, non-JSON error pages, cancellation mid-backoff, and the 429 +// throttle/restore cycle. None of them were previously exercised — the fake +// servers only ever spoke well-formed JSON. + +func TestDoJSON_MalformedJSONOn200(t *testing.T) { + c := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"id": "trunca`)) // 200 OK, broken body + }) + + var out map[string]any + err := c.GetInto(context.Background(), "contacts/1", nil, &out) + require.Error(t, err, "a 200 with a broken body must not be reported as success") + assert.NotContains(t, err.Error(), "panic") +} + +func TestDo_HTMLErrorBodyProducesUsableError(t *testing.T) { + c := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/html") + w.WriteHeader(http.StatusBadRequest) // 4xx: not retried + _, _ = w.Write([]byte(`

400 Bad Request

`)) + }) + + var out map[string]any + err := c.GetInto(context.Background(), "contacts", nil, &out) + require.Error(t, err) + apiErr, ok := AsAPIError(err) + require.True(t, ok, "non-JSON error bodies must still yield an APIError") + assert.Equal(t, http.StatusBadRequest, apiErr.StatusCode) + assert.Contains(t, apiErr.Body, "Bad Request", "raw body must be preserved for diagnosis") +} + +func TestDo_ContextCancelDuringBackoffAborts(t *testing.T) { + c := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) // always retryable + }) + // A long backoff makes the wait the dominant phase; cancellation must cut + // it short instead of sleeping out the full window. + c.retryPolicy = &RetryPolicy{MaxRetries: 3, InitialBackoff: 30 * time.Second, MaxBackoff: 30 * time.Second} + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + start := time.Now() + var out map[string]any + err := c.GetInto(ctx, "contacts", nil, &out) + require.Error(t, err) + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.Less(t, time.Since(start), 5*time.Second, "cancellation must abort the backoff wait") +} + +func TestDo_429ThrottlesThenRestores(t *testing.T) { + var calls int32 + c := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + if atomic.AddInt32(&calls, 1) == 1 { + w.WriteHeader(http.StatusTooManyRequests) + return + } + _, _ = w.Write([]byte(`{"id":"1"}`)) + }) + c.retryPolicy = fastRetry(2) + + before := c.rateLimiter.current + var out map[string]any + require.NoError(t, c.GetInto(context.Background(), "contacts/1", nil, &out)) + + // The 429 halves the rate; the subsequent success restores toward base + // (1.5x) — so the net effect of halve-then-restore lands below the + // starting rate but above the floor. + after := c.rateLimiter.current + assert.Less(t, after, before, "a 429 must leave the limiter slower than it started") + assert.Greater(t, after, before/2, "the success after the 429 must have restored part of the rate") +} + +func TestBackoff_RetryAfterHTTPDateFallsBack(t *testing.T) { + p := &RetryPolicy{MaxRetries: 3, InitialBackoff: time.Second, MaxBackoff: 8 * time.Second} + resp := &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: http.Header{"Retry-After": []string{"Wed, 21 Oct 2026 07:28:00 GMT"}}, + } + // HTTP-date is a valid RFC 9110 Retry-After form the parser doesn't + // understand; it must fall back to exponential backoff, not panic or + // return zero/negative. + d := p.backoff(0, resp) + assert.GreaterOrEqual(t, d, time.Second) + assert.LessOrEqual(t, d, 8*time.Second) +} + +func TestDo_NoRateLimitHeadersKeepsSnapshotUnset(t *testing.T) { + c := newTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"id":"1"}`)) // no X-Rate-Limit-* headers + }) + + var out map[string]any + require.NoError(t, c.GetInto(context.Background(), "contacts/1", nil, &out)) + limit, remaining, reset := c.rateLimiter.Snapshot() + assert.Equal(t, -1, limit) + assert.Equal(t, -1, remaining) + assert.Equal(t, -1, reset) +} diff --git a/internal/api/fuzz_test.go b/internal/api/fuzz_test.go index f1cf2eee..c159ccd6 100644 --- a/internal/api/fuzz_test.go +++ b/internal/api/fuzz_test.go @@ -1,17 +1,27 @@ package api import ( + "bytes" "encoding/json" + "regexp" + "strconv" "testing" ) // The flexible JSON types parse values Alegra serializes inconsistently (string -// vs number vs null). These fuzz tests assert the decoders never panic and that -// successfully-decoded values re-marshal cleanly. Without `-fuzz` they run the -// seed corpus as regular tests. +// vs number vs null). Beyond never panicking, the fuzzers assert value-level +// properties: weak "doesn't crash" checks let a real bug through once (ids +// above 2^53 were silently rounded through float64), so each decoder pins the +// strongest property that holds for its input class. Without `-fuzz` they run +// the seed corpus as regular tests. + +// jsonIntLiteral matches a bare JSON integer token (no exponent, no fraction, +// no leading zeros — shapes json.Unmarshal would reject anyway). "-0" is +// excluded: normalizing it to "0" is intended, not precision loss. +var jsonIntLiteral = regexp.MustCompile(`^(-?[1-9][0-9]*|0)$`) func FuzzID(f *testing.F) { - for _, s := range []string{`"12"`, `12`, `12.0`, `12.5`, `null`, `""`, `"abc"`, `true`, `[]`} { + for _, s := range []string{`"12"`, `12`, `12.0`, `12.5`, `null`, `""`, `"abc"`, `true`, `[]`, `9007199254740993`} { f.Add([]byte(s)) } f.Fuzz(func(t *testing.T, data []byte) { @@ -19,15 +29,21 @@ func FuzzID(f *testing.F) { if json.Unmarshal(data, &id) != nil { return } + // An integer literal must survive decoding verbatim: any difference + // means precision was lost on the way through. + if tok := bytes.TrimSpace(data); jsonIntLiteral.Match(tok) { + if id.String() != string(tok) { + t.Fatalf("integer literal %s decoded to %q", tok, id) + } + } if _, err := json.Marshal(id); err != nil { t.Fatalf("re-marshal failed for %q: %v", data, err) } - _ = id.String() }) } func FuzzInt(f *testing.F) { - for _, s := range []string{`30`, `"30"`, `""`, `null`, `12.9`, `"abc"`, `{}`} { + for _, s := range []string{`30`, `"30"`, `""`, `null`, `12.9`, `"abc"`, `{}`, `9007199254740993`, `"9007199254740993"`} { f.Add([]byte(s)) } f.Fuzz(func(t *testing.T, data []byte) { @@ -35,6 +51,20 @@ func FuzzInt(f *testing.F) { if json.Unmarshal(data, &n) != nil { return } + // Bare or quoted integer literals within int64 range must decode to + // the exact value, not a float64 approximation. + tok := bytes.TrimSpace(data) + if tok[0] == '"' { + var s string + if json.Unmarshal(tok, &s) == nil && jsonIntLiteral.MatchString(s) { + tok = []byte(s) + } + } + if jsonIntLiteral.Match(tok) { + if want, err := strconv.ParseInt(string(tok), 10, 64); err == nil && int64(n) != want { + t.Fatalf("integer literal %s decoded to %d, want %d", tok, int64(n), want) + } + } if _, err := json.Marshal(n); err != nil { t.Fatalf("re-marshal failed for %q: %v", data, err) } @@ -42,7 +72,7 @@ func FuzzInt(f *testing.F) { } func FuzzMoney(f *testing.F) { - for _, s := range []string{`1000`, `"1000.50"`, `""`, `null`, `"abc"`, `[1]`} { + for _, s := range []string{`1000`, `"1000.50"`, `""`, `null`, `"abc"`, `[1]`, `-42.5`} { f.Add([]byte(s)) } f.Fuzz(func(t *testing.T, data []byte) { @@ -50,6 +80,14 @@ func FuzzMoney(f *testing.F) { if json.Unmarshal(data, &m) != nil { return } + // A bare JSON number must decode to exactly what ParseFloat yields — + // Money is a float64, so this is the strongest property available. + tok := bytes.TrimSpace(data) + if len(tok) > 0 && tok[0] != '"' && string(tok) != "null" { + if want, err := strconv.ParseFloat(string(tok), 64); err == nil && float64(m) != want { + t.Fatalf("number %s decoded to %v, want %v", tok, float64(m), want) + } + } if _, err := json.Marshal(m); err != nil { t.Fatalf("re-marshal failed for %q: %v", data, err) } @@ -62,6 +100,18 @@ func FuzzStringOrSlice(f *testing.F) { } f.Fuzz(func(t *testing.T, data []byte) { var s StringOrSlice - _ = json.Unmarshal(data, &s) // must not panic regardless of input + if json.Unmarshal(data, &s) != nil { + return + } + // A single JSON string must decode to exactly that one element. + tok := bytes.TrimSpace(data) + if len(tok) > 0 && tok[0] == '"' { + var want string + if json.Unmarshal(tok, &want) == nil { + if len(s) != 1 || s[0] != want { + t.Fatalf("string %s decoded to %v", tok, []string(s)) + } + } + } }) } diff --git a/internal/api/resource_test.go b/internal/api/resource_test.go index e7253b2f..bf647a74 100644 --- a/internal/api/resource_test.go +++ b/internal/api/resource_test.go @@ -16,10 +16,11 @@ func TestDecodeList_Shapes(t *testing.T) { } cases := map[string]string{ - "bare array": `[{"id":"1","name":"a"},{"id":"2","name":"b"}]`, - "data wrapper": `{"metadata":{"total":2},"data":[{"id":"1","name":"a"},{"id":"2","name":"b"}]}`, - "results wrapper": `{"total":"2","results":[{"id":"1","name":"a"},{"id":"2","name":"b"}]}`, - "rows wrapper": `{"rows":[{"id":"1","name":"a"},{"id":"2","name":"b"}]}`, + "bare array": `[{"id":"1","name":"a"},{"id":"2","name":"b"}]`, + "data wrapper": `{"metadata":{"total":2},"data":[{"id":"1","name":"a"},{"id":"2","name":"b"}]}`, + "results wrapper": `{"total":"2","results":[{"id":"1","name":"a"},{"id":"2","name":"b"}]}`, + "rows wrapper": `{"rows":[{"id":"1","name":"a"},{"id":"2","name":"b"}]}`, + "subscriptions wrapper": `{"subscriptions":[{"id":"1","name":"a"},{"id":"2","name":"b"}]}`, } for name, body := range cases { t.Run(name, func(t *testing.T) { @@ -36,6 +37,14 @@ func TestDecodeList_Empty(t *testing.T) { out, err := decodeList[Contact](nil) require.NoError(t, err) assert.Empty(t, out) + + // Empty wrapper arrays must yield an empty slice, not an error or a + // fall-through to the bare-array parser. + for _, body := range []string{`{"data":[]}`, `{"results":[]}`, `{"rows":[]}`, `{"data":null}`} { + out, err := decodeList[Contact]([]byte(body)) + require.NoError(t, err, "body %s", body) + assert.Empty(t, out, "body %s", body) + } } func TestCount_TotalShapes(t *testing.T) { diff --git a/internal/api/testdata/fuzz/FuzzMoney/c0c1d566e1d675d2 b/internal/api/testdata/fuzz/FuzzMoney/c0c1d566e1d675d2 new file mode 100644 index 00000000..e215eaa2 --- /dev/null +++ b/internal/api/testdata/fuzz/FuzzMoney/c0c1d566e1d675d2 @@ -0,0 +1,2 @@ +go test fuzz v1 +[]byte("\"nAn\"") diff --git a/internal/api/types_test.go b/internal/api/types_test.go index 1d203d8a..9a5e5fd3 100644 --- a/internal/api/types_test.go +++ b/internal/api/types_test.go @@ -45,6 +45,9 @@ func TestInt_RoundTrip(t *testing.T) { `""`: 0, `null`: 0, `12.9`: 12, + // above 2^53: must not round-trip through float64 + `9007199254740993`: 9007199254740993, + `"9007199254740993"`: 9007199254740993, } for in, want := range cases { var n Int @@ -54,6 +57,12 @@ func TestInt_RoundTrip(t *testing.T) { b, err := json.Marshal(Int(7)) require.NoError(t, err) assert.Equal(t, "7", string(b)) + + // ParseFloat accepts these; the decoder must not (int64(NaN) is undefined). + for _, in := range []string{`"NaN"`, `"Inf"`, `"-Inf"`} { + var n Int + assert.Error(t, json.Unmarshal([]byte(in), &n), "input %s", in) + } } func TestMoney_RoundTrip(t *testing.T) { @@ -72,6 +81,13 @@ func TestMoney_RoundTrip(t *testing.T) { b, err := json.Marshal(Money(1234.5)) require.NoError(t, err) assert.Equal(t, "1234.5", string(b)) + + // ParseFloat accepts these; a NaN/Inf Money would poison any re-marshal + // of the containing document (JSON cannot represent them). + for _, in := range []string{`"NaN"`, `"nAn"`, `"Inf"`, `"-Infinity"`} { + var m Money + assert.Error(t, json.Unmarshal([]byte(in), &m), "input %s", in) + } } func TestStringOrSlice_Unmarshal(t *testing.T) { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index d64fa7ff..3b4d5e29 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -1,6 +1,7 @@ package auth import ( + "errors" "testing" "github.com/stretchr/testify/assert" @@ -36,3 +37,18 @@ func TestKeyringStore_DeleteMissing(t *testing.T) { err := NewKeyringStore().Delete("nope") assert.ErrorIs(t, err, ErrNotFound) } + +func TestLookup_KeyringBackendFailureDegradesGracefully(t *testing.T) { + // A locked or unavailable keyring (not just "no entry") must not break + // auth resolution: Lookup returns "" so callers fall back to env/config. + keyring.MockInitWithError(errors.New("keyring backend locked")) + t.Cleanup(keyring.MockInit) + + assert.Equal(t, "", Lookup("prof")) + + // The raw store surfaces the backend error untouched for callers that + // need to distinguish it from not-found. + _, err := NewKeyringStore().Get("prof") + require.Error(t, err) + assert.NotErrorIs(t, err, ErrNotFound) +} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index dc08d55b..5c20f9f8 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "os" "path/filepath" "testing" @@ -119,3 +120,35 @@ func TestParseRPS(t *testing.T) { assert.Equal(t, 5.0, ParseRPS("nope", 5.0), "invalid falls back") assert.Equal(t, 5.0, ParseRPS("-3", 5.0), "non-positive falls back") } + +func TestLoadCorruptYAMLIsAnError(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.yaml") + t.Setenv(EnvConfig, path) + require.NoError(t, os.WriteFile(path, []byte("profiles: [not: a: map\n\t"), 0o600)) + + // A corrupt config must surface a parse error, never be silently + // replaced by defaults (that would drop every profile). + _, err := Load() + require.Error(t, err) + assert.Contains(t, err.Error(), "parsing config") +} + +func TestSaveIsAtomicAndLeavesNoTempFiles(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + t.Setenv(EnvConfig, path) + + c := New() + c.SetProfile(&Profile{Name: "p", Email: "a@b.c"}) + require.NoError(t, c.Save()) + require.NoError(t, c.Save()) // overwrite path: rename over existing file + + entries, err := os.ReadDir(dir) + require.NoError(t, err) + require.Len(t, entries, 1, "temp files must not survive a save") + assert.Equal(t, "config.yaml", entries[0].Name()) + + got, err := Load() + require.NoError(t, err) + assert.Equal(t, "a@b.c", got.Profile("p").Email) +} diff --git a/internal/output/render_test.go b/internal/output/render_test.go index 0680dff2..15c27d03 100644 --- a/internal/output/render_test.go +++ b/internal/output/render_test.go @@ -122,3 +122,22 @@ func TestScalarStringAndValueString(t *testing.T) { assert.Equal(t, `{"x":1}`, valueString(map[string]any{"x": float64(1)})) assert.Equal(t, "plain", valueString("plain")) } + +func TestRenderKeyValue_FiltersAndOrdersByColumns(t *testing.T) { + var buf bytes.Buffer + obj := map[string]any{"id": "1", "name": "Acme", "secret": "hide-me"} + // Single objects render as key/value; --columns must filter and order it. + require.NoError(t, Render(&buf, obj, FormatTable, []string{"name", "id"})) + out := buf.String() + assert.Contains(t, out, "Acme") + assert.Contains(t, out, "1") + assert.NotContains(t, out, "hide-me", "columns filter must apply to key/value output") + assert.Less(t, strings.Index(out, "NAME"), strings.Index(out, "ID"), "explicit column order must be respected") +} + +func TestToRows_DropsNonMapItems(t *testing.T) { + // Heterogeneous arrays keep only object rows — pinned so a future change + // to surface (or error on) dropped items is a conscious decision. + rows := toRows([]any{map[string]any{"id": "1"}, "stray-string", 42, map[string]any{"id": "2"}}) + assert.Len(t, rows, 2) +}