diff --git a/README.md b/README.md index 362cc64..d46fcaa 100644 --- a/README.md +++ b/README.md @@ -3927,6 +3927,7 @@ credited it to the wrong command.) | `model substituted` | The server ran a **different checkpoint** than you asked for and billed for what ran. Warned by default; `--fail-on-substitution` turns it into a refusal on the estimate, before any spend. | [Silent model substitution](#-silent-model-substitution) | | `The server reported: …` | The orchestrator recorded an account of what happened, and what follows is the server's own words. The CLI does not interpret them and does not know whether the failure is retryable; the only thing it changes is that invisible and direction-reversing characters are removed before the text reaches your terminal (`--json` is unfiltered). Printed on the `generate` error and by `civitai workflows get`. | [What the server says went wrong](#what-the-server-says-went-wrong) | | `An indented line under a row is what the server recorded` | The same record, on `civitai workflows list`: the indented lines beneath a workflow's row are the server's own words for that workflow, wrapped but never abbreviated (the invisible characters above are removed, and wrapping collapses runs of whitespace and breaks a token longer than the line — no words are dropped). The indent is not decoration — it keeps server text out of the column a real row starts in, so a message cannot pose as a workflow of yours. It holds for the line breaks the CLI makes: the CLI wraps to a fixed 79 columns and never asks how wide your terminal is, so in a **narrower** terminal — or with wide (CJK) characters — your terminal re-wraps and the overflow can still reach column zero. | [What the server says went wrong](#what-the-server-says-went-wrong) | +| `prompt: …` / `negative: …` | In `civitai images search --meta` and `civitai images get`: generation prompts are rendered with continuation lines indented so a server string cannot impersonate CLI output headers. Unlike workflow failure reasons, prompts are deliberately **not** soft-wrapped by the CLI — wrapping would collapse whitespace runs and split tokens, altering prompt weights and syntax. In terminals narrower than an emitted line, the terminal's own soft-wrap still occurs and the overflow can reach column zero. | [Command reference](#command-reference) | | `the orchestrator often supplies no failure reason, so it may not say why` | The same failure with **no** account recorded — a real, measured case, not a CLI limitation. Neither `civitai workflows get ` nor `civitai workflows list` will say why either. | [What the server says went wrong](#what-the-server-says-went-wrong) | ### Everything else diff --git a/internal/cmd/images.go b/internal/cmd/images.go index ca026c5..1c889d4 100644 --- a/internal/cmd/images.go +++ b/internal/cmd/images.go @@ -243,17 +243,18 @@ func printImageList(cmd *cobra.Command, items []civitai.ImageItem) { fmt.Fprintln(tw, "ID\tUPLOADER\tBASE MODEL\tSIZE\tNSFW\tHEARTS\tCOMMENTS\tURL") for _, im := range items { fmt.Fprintf(tw, "%d\t%s\t%s\t%dx%d\t%s\t%d\t%d\t%s\n", - im.ID, orDash(safeTerm(im.Username.String())), orDash(truncate(safeTerm(im.BaseModel), 24)), - im.Width, im.Height, orDash(safeTerm(im.NSFWLevel)), - im.Stats.HeartCount, im.Stats.CommentCount, safeTerm(im.URL)) + im.ID, orDash(safeTermSingle(im.Username.String())), orDash(truncate(safeTermSingle(im.BaseModel), 24)), + im.Width, im.Height, orDash(safeTermSingle(im.NSFWLevel)), + im.Stats.HeartCount, im.Stats.CommentCount, safeTermSingle(im.URL)) } _ = tw.Flush() } // printImageListMeta renders each image as an indented detail block instead of // the compact table, so it can carry the generation metadata (prompt, settings) -// that --meta requests. Every server-origin string is routed through safeTerm — -// prompts are attacker-controlled user text and can carry ANSI/control bytes. +// that --meta requests. Every server-origin string is routed through safeTerm or +// safeTermSingle — prompts are indented multi-line text, while inline metadata and +// table columns are sanitized to single lines to prevent output forgery (#552). func printImageListMeta(cmd *cobra.Command, items []civitai.ImageItem) { out := cmd.OutOrStdout() if len(items) == 0 { @@ -267,12 +268,13 @@ func printImageListMeta(cmd *cobra.Command, items []civitai.ImageItem) { // printImageMetaBlock renders one image as an indented detail block carrying its // generation metadata (prompt, settings, and the resources "recipe"). Every -// server-origin string is routed through safeTerm — prompts, resource names and -// hashes are attacker-controlled user text that can carry ANSI/control bytes. +// server-origin string is routed through safeTerm or safeTermSingle — prompts are +// indented multi-line text, while inline metadata (model, sampler, resources, +// hashes, URL) is sanitized to single lines to prevent output forgery (#552). // Shared by `images search --meta` and `images get`. func printImageMetaBlock(out io.Writer, im civitai.ImageItem) { fmt.Fprintf(out, "%d [%s] %dx%d by %s\n", - im.ID, orDash(safeTerm(im.NSFWLevel)), im.Width, im.Height, orDash(safeTerm(im.Username.String()))) + im.ID, orDash(safeTermSingle(im.NSFWLevel)), im.Width, im.Height, orDash(safeTermSingle(im.Username.String()))) m, state := im.ParseMeta() switch state { case civitai.MetaAbsent: @@ -285,9 +287,9 @@ func printImageMetaBlock(out io.Writer, im civitai.ImageItem) { fmt.Fprintln(out, " meta: (unrecognized format)") default: // civitai.MetaOK fmt.Fprintf(out, " model: %s sampler: %s cfg: %s steps: %s seed: %s\n", - orDash(safeTerm(m.Model)), orDash(safeTerm(m.Sampler)), - orDash(safeTerm(m.CfgScaleString())), orDash(safeTerm(m.StepsString())), - orDash(safeTerm(m.SeedString()))) + orDash(safeTermSingle(m.Model)), orDash(safeTermSingle(m.Sampler)), + orDash(safeTermSingle(m.CfgScaleString())), orDash(safeTermSingle(m.StepsString())), + orDash(safeTermSingle(m.SeedString()))) if strings.TrimSpace(m.Prompt) != "" { fmt.Fprintf(out, " prompt: %s\n", indentContinuation(safeTerm(m.Prompt), " ")) } @@ -296,7 +298,7 @@ func printImageMetaBlock(out io.Writer, im civitai.ImageItem) { } printImageResources(out, m) } - fmt.Fprintf(out, " url: %s\n", safeTerm(im.URL)) + fmt.Fprintf(out, " url: %s\n", safeTermSingle(im.URL)) } // printImageResources renders the meta.resources reproduction recipe — one line @@ -312,12 +314,12 @@ func printImageResources(out io.Writer, m civitai.ImageMeta) { fmt.Fprintln(out, " resources:") for _, r := range rs { line := fmt.Sprintf(" - [%s] %s", - orDash(safeTerm(strings.TrimSpace(r.Type))), orDash(safeTerm(strings.TrimSpace(r.Name)))) + orDash(safeTermSingle(strings.TrimSpace(r.Type))), orDash(safeTermSingle(strings.TrimSpace(r.Name)))) if w := strings.TrimSpace(r.WeightString()); w != "" { - line += " weight " + safeTerm(w) + line += " weight " + safeTermSingle(w) } if h := strings.TrimSpace(m.ResolveHash(r)); h != "" { - line += " hash " + safeTerm(h) + line += " hash " + safeTermSingle(h) } fmt.Fprintln(out, line) } diff --git a/internal/cmd/indentcontinuation_ledger_test.go b/internal/cmd/indentcontinuation_ledger_test.go new file mode 100644 index 0000000..2c7a998 --- /dev/null +++ b/internal/cmd/indentcontinuation_ledger_test.go @@ -0,0 +1,317 @@ +package cmd + +import ( + "bytes" + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "sort" + "strconv" + "strings" + "testing" + + "github.com/civitai/cli/pkg/civitai" + "github.com/spf13/cobra" +) + +// indentcontinuation_ledger_test.go pins the set of call sites that route +// multi-line server text through indentContinuation (civitai/cli#552, follow-up +// to #545). +// +// 🔴 WHAT THIS LEDGER CANNOT SEE, stated rather than waved at: +// 1. A call routed through a local function-typed variable or alias; +// 2. An un-ledgered renderer that prints multi-line server text using raw +// fmt.Fprintf without ever invoking indentContinuation or safeTermSingle. +// The behavioural subtest beside this is what guards the rendered surfaces +// against (2). +// +// 🔴 THE SET GROWS OR SHRINKS: +// Adding a call site without adding it to the ledger fails this test (GREW). +// Removing a call site without updating the ledger fails this test (SHRANK). + +type indentCallSite struct { + file string + fn string + why string +} + +var pinnedIndentCallSites = []indentCallSite{ + { + file: "generate.go", + fn: "serverReasonSuffix", + why: "terminal orchestrator failure reason (multi-line unbounded free text)", + }, + { + file: "generate_output.go", + fn: "reportExcludedOutputs", + why: "per-output exclusion reason", + }, + { + file: "images.go", + fn: "printImageMetaBlock", + why: "image generation prompt (multi-line user free text)", + }, + { + file: "images.go", + fn: "printImageMetaBlock", + why: "image generation negative prompt (multi-line user free text)", + }, + { + file: "workflows.go", + fn: "printWorkflow", + why: "workflow run failure reason in detail view", + }, + { + file: "workflows_list.go", + fn: "printWorkflowList", + why: "workflow run failure reason beneath table row", + }, +} + +const minIndentCallSitesExpected = 6 + +// TestIndentContinuationCallSitesAreLedgered is the closing condition for #552. +// It combines a structural AST ledger with a behavioural seam test, ensuring +// that go test ./internal/cmd -run Ledger covers both guards in one invocation. +func TestIndentContinuationCallSitesAreLedgered(t *testing.T) { + t.Run("StructuralLedger", func(t *testing.T) { + entries, err := os.ReadDir(".") + if err != nil { + t.Fatalf("CONTROL failure: cannot read package directory: %v", err) + } + + fset := token.NewFileSet() + filesParsed := 0 + var actualCalls []indentCallSite + + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + f, perr := parser.ParseFile(fset, name, nil, 0) + if perr != nil { + t.Fatalf("CONTROL failure: cannot parse %s: %v", name, perr) + } + filesParsed++ + + var currentFn string + ast.Inspect(f, func(n ast.Node) bool { + switch x := n.(type) { + case *ast.FuncDecl: + currentFn = x.Name.Name + case *ast.CallExpr: + id, ok := x.Fun.(*ast.Ident) + if !ok || id.Name != "indentContinuation" { + return true + } + pos := fset.Position(x.Lparen) + if len(x.Args) != 2 { + t.Errorf("%s: indentContinuation must have exactly 2 arguments, got %d", pos, len(x.Args)) + return true + } + + // 1. Assert arg[0] passes through safeTerm or wrapServerText + if !containsSanitizerCall(x.Args[0]) { + t.Errorf("%s: indentContinuation arg[0] must pass through safeTerm or wrapServerText", pos) + } + + // 2. Assert arg[1] (pad) is a valid whitespace literal or known indent constant + checkIndentPadArg(t, pos, x.Args[1]) + + actualCalls = append(actualCalls, indentCallSite{ + file: name, + fn: currentFn, + }) + } + return true + }) + } + + if filesParsed < 20 { + t.Fatalf("CONTROL failure: parsed only %d files in internal/cmd", filesParsed) + } + + if len(actualCalls) < minIndentCallSitesExpected { + t.Fatalf("CONTROL failure: found only %d indentContinuation call(s), want >= %d", + len(actualCalls), minIndentCallSitesExpected) + } + + // Bidirectional set comparison + gotKey := func(s indentCallSite) string { return s.file + ":" + s.fn } + var gotKeys, wantKeys []string + for _, c := range actualCalls { + gotKeys = append(gotKeys, gotKey(c)) + } + for _, c := range pinnedIndentCallSites { + wantKeys = append(wantKeys, gotKey(c)) + } + sort.Strings(gotKeys) + sort.Strings(wantKeys) + + if strings.Join(gotKeys, ",") != strings.Join(wantKeys, ",") { + t.Fatalf("indentContinuation call sites in internal/cmd do not match the ledger.\n"+ + "got (%d):\n %s\nwant (%d):\n %s\n\n"+ + "GREW: a new call site now uses indentContinuation — add it to pinnedIndentCallSites with why.\n"+ + "SHRANK: a call site was deleted or renamed — update the ledger deliberately.", + len(gotKeys), strings.Join(gotKeys, "\n "), + len(wantKeys), strings.Join(wantKeys, "\n ")) + } + }) + + t.Run("BehaviouralSeam", func(t *testing.T) { + // Test against the 8 forgeable fields identified in civitai/cli#552: + // 7 inline/metadata fields in printImageMetaBlock + 1 table row field in printImageList. + probe := "probe\n[FORGED_ROW]" + + // 1. Verify printImageMetaBlock (all inline/metadata fields, including resources weight & hash) + metaJSON := fmt.Sprintf(`{ + "prompt": %q, + "negativePrompt": %q, + "Model": %q, + "sampler": %q, + "cfgScale": %q, + "steps": %q, + "seed": %q, + "resources": [ + {"type": %q, "name": %q, "weight": %q, "hash": %q} + ] + }`, probe, probe, probe, probe, probe, probe, probe, probe, probe, probe, probe) + + im := civitai.ImageItem{ + ID: 123456, + URL: probe, + NSFWLevel: probe, + Width: 512, + Height: 512, + Meta: []byte(metaJSON), + } + _ = im.Username.UnmarshalJSON([]byte(fmt.Sprintf("%q", probe))) + + var buf bytes.Buffer + printImageMetaBlock(&buf, im) + rendered := buf.String() + + // Prompt & NegativePrompt: multi-line must survive, but with continuation indented (never col 0) + if !strings.Contains(rendered, " prompt: probe\n [FORGED_ROW]") { + t.Errorf("prompt was not indented with 10 spaces: %q", rendered) + } + if !strings.Contains(rendered, " negative: probe\n [FORGED_ROW]") { + t.Errorf("negative prompt was not indented with 12 spaces: %q", rendered) + } + + // All other fields in printImageMetaBlock must have \n replaced with space + inlineChecks := []struct { + name string + needle string + antiNeed string + }{ + {"model", "model: probe [FORGED_ROW]", "model: probe\n"}, + {"sampler", "sampler: probe [FORGED_ROW]", "sampler: probe\n"}, + {"cfgScale", "cfg: probe [FORGED_ROW]", "cfg: probe\n"}, + {"steps", "steps: probe [FORGED_ROW]", "steps: probe\n"}, + {"seed", "seed: probe [FORGED_ROW]", "seed: probe\n"}, + {"url", "url: probe [FORGED_ROW]", "url: probe\n"}, + {"header username", "by probe [FORGED_ROW]", "by probe\n"}, + {"header nsfw", "[probe [FORGED_ROW]]", "[probe\n"}, + {"resource type/name", "[probe [FORGED_ROW]] probe [FORGED_ROW]", "[probe\n"}, + {"resource weight", "weight probe [FORGED_ROW]", "weight probe\n"}, + {"resource hash", "hash probe [FORGED_ROW]", "hash probe\n"}, + } + for _, c := range inlineChecks { + if !strings.Contains(rendered, c.needle) { + t.Errorf("%s was not sanitized to a single line; want %q", c.name, c.needle) + } + if strings.Contains(rendered, c.antiNeed) { + t.Errorf("%s leaked a newline: %q", c.name, rendered) + } + } + + // Assert [FORGED_ROW] never occupies column zero anywhere in the block + for _, line := range strings.Split(rendered, "\n") { + if strings.HasPrefix(line, "[FORGED_ROW]") { + t.Errorf("found forged text occupying column zero: %q", line) + } + } + + // 2. Verify plain printImageList table row (1 field: username, plus BaseModel, NSFWLevel, URL) + var tableBuf bytes.Buffer + fakeCmd := &cobra.Command{} + fakeCmd.SetOut(&tableBuf) + printImageList(fakeCmd, []civitai.ImageItem{im}) + tableRendered := tableBuf.String() + + tableLines := strings.Split(strings.TrimRight(tableRendered, "\n"), "\n") + // Header + exactly 1 data row = 2 lines + if len(tableLines) != 2 { + t.Errorf("table rendered %d lines (want 2); tabwriter was split into fake rows:\n%s", + len(tableLines), tableRendered) + } + for _, line := range tableLines { + if strings.HasPrefix(strings.TrimSpace(line), "[FORGED_ROW]") { + t.Errorf("table row forged by username newline: %q", line) + } + } + }) +} + +// containsSanitizerCall traverses an AST expression to confirm that safeTerm +// or wrapServerText is present in the call chain. +func containsSanitizerCall(e ast.Expr) bool { + found := false + ast.Inspect(e, func(n ast.Node) bool { + ce, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if id, ok := ce.Fun.(*ast.Ident); ok { + if id.Name == "safeTerm" || id.Name == "wrapServerText" { + found = true + return false + } + } + return true + }) + return found +} + +// checkIndentPadArg verifies that pad is either a string literal containing +// only whitespace (spaces/tabs), or a known whitespace-indent constant. +func checkIndentPadArg(t *testing.T, pos token.Position, padExpr ast.Expr) { + t.Helper() + switch p := padExpr.(type) { + case *ast.BasicLit: + if p.Kind != token.STRING { + t.Errorf("%s: indentContinuation pad must be a string literal, got token %v", pos, p.Kind) + return + } + unquoted, err := strconv.Unquote(p.Value) + if err != nil { + t.Errorf("%s: invalid string literal %s: %v", pos, p.Value, err) + return + } + if len(unquoted) == 0 { + t.Errorf("%s: indentContinuation pad cannot be empty", pos) + return + } + for _, r := range unquoted { + if r != ' ' && r != '\t' { + t.Errorf("%s: indentContinuation pad literal %q contains non-whitespace rune %q", pos, unquoted, r) + return + } + } + case *ast.Ident: + // Known package constants declared as indentation pads + knownConstants := map[string]bool{ + "listReasonIndent": true, + } + if !knownConstants[p.Name] { + t.Errorf("%s: indentContinuation pad uses unvetted identifier %q; must be literal or vetted constant", + pos, p.Name) + } + default: + t.Errorf("%s: indentContinuation pad must be a string literal or vetted constant, got %T", pos, padExpr) + } +} diff --git a/internal/cmd/safeterm.go b/internal/cmd/safeterm.go index cc75d4c..239ac78 100644 --- a/internal/cmd/safeterm.go +++ b/internal/cmd/safeterm.go @@ -43,6 +43,27 @@ func safeTerm(s string) string { return saferune.Strip(s) } +// safeTermSingle ensures a server-origin string occupies exactly one line, +// stripping terminal escapes via safeTerm and replacing any newline with a space. +// +// 🔴 IT IS FOR INLINE AND TABULAR FIELDS, NOT FREE TEXT. +// safeTerm deliberately keeps \n so multi-line fields (prompt, failure reasons) +// can format legitimately. But inline metadata (model, sampler, username, URL) +// and table columns cannot contain newlines without breaking column alignment +// or forging rows in tabwriter — indentContinuation cannot fix an inline field +// because it has no indentation baseline, and cannot fix a tabwriter row because +// tabwriter splits on \n regardless of padding (civitai/cli#552). +// +// Note on \r: safeTerm already strips \r as a C0 control rune (Cc), so no +// carriage return survives to this function. Only \n needs replacing. +func safeTermSingle(s string) string { + s = safeTerm(s) + if !strings.Contains(s, "\n") { + return s + } + return strings.ReplaceAll(s, "\n", " ") +} + // indentContinuation prefixes every line of s AFTER the first with pad, so a // multi-line SERVER string stays visibly inside the list item or block that // introduced it. diff --git a/internal/cmd/safeterm_userinput_test.go b/internal/cmd/safeterm_userinput_test.go index a883b7e..183a447 100644 --- a/internal/cmd/safeterm_userinput_test.go +++ b/internal/cmd/safeterm_userinput_test.go @@ -81,6 +81,7 @@ var bareIdentArgs = map[string]string{ "name": "SERVER: a published file name", "h": "SERVER: a hash out of image metadata", "baseModel": "SERVER: a base-model label", + "s": "PASSTHROUGH: safeTermSingle forwards its argument to safeTerm", } // minSafeTermCallsScanned is the POSITIVE CONTROL. A parser that has stopped @@ -117,17 +118,17 @@ func TestSafeTermIsNeverAppliedToUserTypedInput(t *testing.T) { return true } id, ok := ce.Fun.(*ast.Ident) - if !ok || id.Name != "safeTerm" || len(ce.Args) != 1 { + if !ok || (id.Name != "safeTerm" && id.Name != "safeTermSingle") || len(ce.Args) != 1 { return true } scanned++ arg := renderExpr(ce.Args[0]) if why, forbidden := userTypedArgs[arg]; forbidden { - bad = append(bad, fmt.Sprintf("%s: safeTerm(%s) — %s", fset.Position(ce.Lparen), arg, why)) + bad = append(bad, fmt.Sprintf("%s: %s(%s) — %s", fset.Position(ce.Lparen), id.Name, arg, why)) } if _, bare := ce.Args[0].(*ast.Ident); bare { if _, known := bareIdentArgs[arg]; !known { - unclassified = append(unclassified, fmt.Sprintf("%s: safeTerm(%s)", fset.Position(ce.Lparen), arg)) + unclassified = append(unclassified, fmt.Sprintf("%s: %s(%s)", fset.Position(ce.Lparen), id.Name, arg)) } seenBare[arg] = true }