v0.4.0: close five untrusted-input paths, and bring the docs back in line

CHANGELOG.md

@@ -4,6 +4,56 @@ All notable changes to PIACE are recorded here. The format follows
 [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and the project
 follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [0.4.0] - 2026-09-04
+
+### Changed
+
+- The impact-estimate section is labelled **Potential impact estimate** with a
+  capital P in every output format.
+- A `differences_allowed` run is coloured green in the HTML report. It shares
+  exit `0` with `clean`, and yellow is reserved for the advisory medium-risk
+  indication rather than for a run that succeeded.
+
+### Fixed
+
+- **The task prompt states the response shape unconditionally.** Anthropic's
+  OpenAI-compatible endpoint documents itself as ignoring `response_format`
+  rather than rejecting it, so a request relying on structured output alone
+  asked for nothing. `Interpret` also unwraps a Markdown code fence around a
+  whole reply, recording a warning, rather than discarding a complete
+  assessment over its packaging.
+- **An oversized inference response is reported as oversized** rather than as
+  "not a chat completion", which sent an operator looking at the wrong thing.
+
+### Security
+
+- **A File `source` value can no longer steer the file-content request.** A
+  `source` reached the compiler's `file_content` endpoint as an unescaped path
+  concatenation, so `puppet:///../../pdb/query/v4/catalogs/<node>` was
+  attacker-shaped input, compiled from the change under review, aiming an
+  mTLS-authenticated GET at another path on a host PIACE is already authorized
+  against. `parsePuppetSourceURI` now refuses an empty, `.` or `..` segment and
+  a NUL byte.
+- **The inference client has a redirect policy.** `net/http` keeps an
+  `Authorization` header across a redirect to the same host, so an endpoint
+  answering `302 Location: http://<same host>` received the bearer token in
+  cleartext. Redirects that leave `https` or change authority are now refused,
+  matching what `internal/transport` already enforced for the credential-free
+  clients.
+- **The text report escapes control characters** in every interpolated value.
+  A resource title or diagnostic message carrying `ESC[2K\r` followed by a
+  forged outcome line could make a run that exits `30` read as clean in a CI
+  log. C0, DEL and C1 are replaced with a printable escape; the JSON and HTML
+  reports are unchanged, since JSON escaping already makes a control character
+  inert and `html/template` covers the HTML.
+- **Refs reaching git are validated.** `change-context` refuses a ref beginning
+  with `-` before any git command runs and passes `--end-of-options`, so a base
+  or head ref chosen by whoever opened a fork pull request cannot become an
+  option to `git diff`.
+- **`--debug-dump-dir` no longer writes through a symlink** already sitting at
+  a dump path, and applies `0600` to the file it writes rather than only to one
+  it creates.
+
 ## [0.3.0] - 2026-09-01
 
 ### Added
@@ -41,8 +91,6 @@ follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   Hub, from the same build. An anonymous Docker Hub pull from a shared CI
   runner IP is exactly what Docker Hub rate-limits, and a pipeline failing for
   that reason fails for a reason unrelated to this project.
-
-
 - **`piace compare --candidate-environment ENVIRONMENT`**: compiles every
   target's candidate catalog from ENVIRONMENT, overriding
   `candidate.environment` in both the `defaults:` block and any per-target
@@ -55,7 +103,6 @@ follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   `candidate.environment` entirely. `capture catalog --environment` is
   unchanged and unrelated: it names the environment to snapshot, not the
   candidate environment under test.
-
 - **`piace explain --debug` / `--debug-dump-dir DIR`**: the two observation
   options `compare` and `capture` already accept now work on `explain` too, so
   a rejected inference request can be diagnosed without guessing. `--debug`
@@ -67,14 +114,12 @@ follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   4xx is where a provider names the field it rejected. Neither the returned
   error nor any log line ever carries a response-body value, and the bearer
   token is a header so it reaches no dump file.
-
 - **`services.inference.token_limit_param`**: selects the request field that
   carries the output-token bound, `max_tokens` (the default) or
   `max_completion_tokens`. OpenAI's GPT-5 family rejects `max_tokens` outright
   and requires `max_completion_tokens`; OpenAI-compatible servers other than
   current OpenAI (Ollama, vLLM, llama.cpp) only understand `max_tokens`. The
   value in `max_tokens` is unchanged; only the wire field name differs.
-
 - **`services.inference.temperature`**: optional sampling temperature, sent
   only when set.
 
@@ -105,7 +150,6 @@ follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   it, reasoning models reject it), so the `seed` field is gone. Pinning them
   never made a model-generated assessment reproducible in the first place: a
   provider-side model revision still moves the bytes.
-
 - **`piace explain`**: dependency-graph edge groups are no longer sent to the
   inference service. An edge change is a consequence of the resource changes
   around it, carries no before/after pair to reason about, and a run's edges
@@ -295,14 +339,14 @@ than what changed.
 ### Known limitations
 
 - **`catalog_api: v3` with `baseline.source: puppetdb` is not rejected.**
-  requirements.md 1.8 requires a v3 target to use a file baseline, but config
-  validation does not yet enforce it. The configuration loads and the run
-  overwrites the baseline it just read; the symptom on the next run is a
+  A v3 target is required to use a file baseline, but config validation does
+  not yet enforce it. The configuration loads and the run overwrites the
+  baseline it just read; the symptom on the next run is a
   baseline-environment mismatch naming the candidate environment. Set
   `baseline.source: file` yourself. See the README's
   "If you must use v3, compare against a captured file".
 - **Two PuppetDB impact-endpoint behaviours are unconfirmed against a
-  deployment**: that design §8's PQL text is accepted at the root
+  deployment**: that the impact PQL text is accepted at the root
   `/pdb/query/v4`, and that `limit`/`order_by` are honoured there. If
   `order_by` is not honoured, a *truncated* impact sample is not reproducible.
 - **The Puppet `Sensitive` wire shape** (`{"__ptype":"Sensitive","__pvalue":…}`)
@@ -313,6 +357,7 @@ than what changed.
 The last two are recorded as skipped tests carrying their confirmation
 procedures in `cmd/piace/acceptance_assumptions_test.go`.
 
+[0.4.0]: https://github.com/example42/piace/releases/tag/v0.4.0
 [0.3.0]: https://github.com/example42/piace/releases/tag/v0.3.0
 [0.2.1]: https://github.com/example42/piace/releases/tag/v0.2.1
 [0.2.0]: https://github.com/example42/piace/releases/tag/v0.2.0

CONTEXT.md

@@ -131,18 +131,16 @@ _Avoid_: anonymized, masked, redacted
 ### One path rule
 
 Every relative path named in a config file resolves against the directory of
-the file that names it. `facts.file` and `baseline.file` resolve against the
-target file; the TLS paths, `token_file` and `policy_notes_file` resolve
-against the services file. An absolute path is taken as written, and nothing
-resolves against the process working directory, so moving a file takes its
-paths with it.
+the file that names it, never against the process working directory, so moving
+a config file takes its paths with it. The [README](README.md#configuration)
+lists which key resolves against which file.
 
-A credential may instead be named rather than located: `ca_bundle_env`,
+A credential may be named rather than located: `ca_bundle_env`,
 `client_cert_env`, `private_key_env` and `token_env` each name an environment
-variable holding a path, which must be absolute. Naming both forms of one
-credential is an error rather than a precedence rule nobody remembers. This is
-what lets a services file be committed and read in place by a CI job whose
-credential directory did not exist when the file was written.
+variable holding an absolute path. Naming both forms of one credential is an
+error rather than a precedence rule nobody remembers. Together the two rules
+are what let a services file be committed and read in place, unmodified, by a
+CI job whose credential directory did not exist when the file was written.
 
 ### Candidate catalogs come from an existing compiler
 

README.md

@@ -124,18 +124,13 @@ piace change-context (--base-ref REF | --base-ref-env VAR) \
 | `--debug` | compare, capture, explain | One metadata line per service request to stderr |
 | `--debug-dump-dir` | compare, capture, explain | Also write raw bodies to `0600` files in DIR |
 
-`compare --candidate-environment ENV` compiles every target against `ENV`,
-overriding `candidate.environment` in both the `defaults:` block and any
-per-target `candidate:` block. The environment CI deployed is a per-pipeline
-value, so passing it at the invocation keeps the target file reviewable policy
-that no job has to rewrite; with the flag, the file may omit
-`candidate.environment` entirely.
-
-`capture catalog --environment ENV` is a different flag with a different
-meaning: it requests the catalog for `ENV`, typically the production or default
-environment, captured after merge so development-branch runs baseline against a
-frozen catalog rather than a later one from another environment. It never
-overrides `candidate.environment`.
+The two environment flags are not the same flag.
+`compare --candidate-environment` names the environment CI deployed and
+overrides `candidate.environment` for every target, so the target file may omit
+the field entirely; see
+[docs/ci.md](docs/ci.md#why-the-targets-file-is-not-a-template).
+`capture catalog --environment` names the environment to *snapshot*, typically
+the production baseline, and never overrides `candidate.environment`.
 
 ### Reports
 
@@ -478,10 +473,8 @@ is diffed.
 
 Optional and advisory. It reads a JSON report `compare` already wrote, sends
 **one** request to a configured **inference service**, and writes a separately
-versioned assessment artifact plus a re-rendered HTML report. It never
-re-compiles anything, never contacts a compiler or PuppetDB, and never rewrites
-the result document. `compare`, for its part, never contacts an inference
-service, and nothing the assessment says can change an outcome or an exit code.
+versioned assessment artifact plus a re-rendered HTML report, never rewriting
+the result document. Nothing it says can change an outcome or an exit code.
 
 ```sh
 piace change-context --base-ref origin/main \

cmd/piace/acceptance_changecontext_test.go

@@ -162,6 +162,7 @@ func TestAcceptance_ChangeContextUsageErrors(t *testing.T) {
 	repo := changeContextRepo(t)
 	t.Setenv("PR_TITLE", "a title")
 	t.Setenv("BASE", "main")
+	t.Setenv("HOSTILE_BASE", "--output=/tmp/piace-should-not-exist")
 
 	tests := []struct {
 		name    string
@@ -193,6 +194,26 @@ func TestAcceptance_ChangeContextUsageErrors(t *testing.T) {
 			args:    []string{"--base-ref", "no-such-branch"},
 			wantMsg: "git merge-base",
 		},
+		// A ref reaches git as a positional argument, so one beginning
+		// with "-" is read as an option rather than a branch name. On a
+		// fork pull request the head ref is a branch name whoever opened
+		// the change chose, which is exactly why the refs take an `-env`
+		// form in the first place. Refused before any git command runs.
+		{
+			name:    "base ref looks like an option",
+			args:    []string{"--base-ref", "--output=/tmp/piace-should-not-exist"},
+			wantMsg: `a ref may not begin with "-"`,
+		},
+		{
+			name:    "head ref looks like an option",
+			args:    []string{"--base-ref", "main", "--head-ref", "--output=/tmp/piace-should-not-exist"},
+			wantMsg: `a ref may not begin with "-"`,
+		},
+		{
+			name:    "base ref from the environment looks like an option",
+			args:    []string{"--base-ref-env", "HOSTILE_BASE"},
+			wantMsg: `a ref may not begin with "-"`,
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {

cmd/piace/changecontext.go

@@ -113,12 +113,18 @@ func runChangeContext(args []string, stdout, stderr *os.File) exitcode.Code {
 func changeContextFromGit(baseRef, headRef string) (assess.ChangeContext, error) {
 	cc := assess.ChangeContext{Present: true, BaseRef: baseRef}
 
-	mergeBase, err := gitOutput("merge-base", baseRef, headRef)
+	mergeBase, err := gitOutput("merge-base", endOfOptions, baseRef, headRef)
 	if err != nil {
 		return cc, err
 	}
 	mergeBase = strings.TrimSpace(mergeBase)
 
+	// The one call with no endOfOptions marker: `git rev-parse` echoes
+	// arguments it cannot interpret, and the marker itself is one of them,
+	// so passing it here puts a literal "--end-of-options" line above the
+	// branch name in the output this reads. validateRef, which refuses a
+	// leading "-" before any git command runs, is the control that covers
+	// this call.
 	name, err := gitOutput("rev-parse", "--abbrev-ref", headRef)
 	if err != nil {
 		return cc, err
@@ -128,7 +134,7 @@ func changeContextFromGit(baseRef, headRef string) (assess.ChangeContext, error)
 	// -z so records are NUL-separated and \x1f between fields: a commit
 	// subject may legally contain a tab or a newline, and splitting on
 	// either would invent commits that do not exist.
-	log, err := gitOutput("log", "-z", "--format=%H%x1f%s%x1f%an", mergeBase+".."+headRef)
+	log, err := gitOutput("log", "-z", "--format=%H%x1f%s%x1f%an", endOfOptions, mergeBase+".."+headRef)
 	if err != nil {
 		return cc, err
 	}
@@ -142,7 +148,7 @@ func changeContextFromGit(baseRef, headRef string) (assess.ChangeContext, error)
 
 	// -z again, for the same reason: a path may contain a newline, and
 	// without it git would quote and escape such a path instead.
-	paths, err := gitOutput("diff", "--name-only", "-z", mergeBase, headRef)
+	paths, err := gitOutput("diff", "--name-only", "-z", endOfOptions, mergeBase, headRef)
 	if err != nil {
 		return cc, err
 	}
@@ -151,6 +157,28 @@ func changeContextFromGit(baseRef, headRef string) (assess.ChangeContext, error)
 	return cc, nil
 }
 
+// endOfOptions is passed to the git invocations above that accept it,
+// immediately before the first ref. It tells git's revision parser that nothing
+// after it is an option, however it is spelled.
+//
+// The refs here are attacker-supplied in the case this command exists
+// for: on a fork pull request the base and head refs are branch names
+// whoever opened the change chose. A ref of `--output=<path>` reaching
+// `git diff` as a positional argument is an arbitrary file write on the
+// runner that holds the catalog-reader identity. As it happens the
+// `merge-base` call runs first and rejects an unknown option, so that
+// particular value dead-ends before `diff` is reached, but a chain that
+// depends on the argument order of the first of four commands is not a
+// control. validateRef refuses a leading "-" outright and this refuses
+// what a future reordering might otherwise let through.
+//
+// It requires git 2.24 (November 2019). A `piace change-context` run
+// against anything older fails with git's own "unknown option" text
+// rather than silently dropping the guard, which is the right way round:
+// `explain --change` reads a file produced by any means, so a site on an
+// older git writes the same YAML without this subcommand.
+const endOfOptions = "--end-of-options"
+
 func splitNUL(s string) []string {
 	var out []string
 	for _, field := range strings.Split(s, "\x00") {
@@ -181,7 +209,8 @@ func gitOutput(args ...string) (string, error) {
 }
 
 // readRef reads a ref from exactly one of the two references the caller
-// may give, the same rule the services file follows for a credential.
+// may give, the same rule the services file follows for a credential,
+// and validates it before it can become a git argument.
 func readRef(flagName, value, env string) (string, error) {
 	switch {
 	case value != "" && value != "HEAD" && env != "":
@@ -191,10 +220,38 @@ func readRef(flagName, value, env string) (string, error) {
 		if v == "" {
 			return "", fmt.Errorf("--%s-env: environment variable %s is unset or empty", flagName, env)
 		}
-		return v, nil
+		return v, validateRef(flagName, v)
 	default:
-		return value, nil
+		return value, validateRef(flagName, value)
+	}
+}
+
+// validateRef refuses the ref shapes that would be read as something
+// other than a ref by the git commands changeContextFromGit runs.
+//
+// A leading "-" is the whole point: a ref is passed to git as a
+// positional argument, and git's option parser does not care that the
+// caller meant a branch name. Git itself forbids a ref name beginning
+// with "-", so nothing legitimate is refused here.
+//
+// A NUL byte is refused because exec would fail on it anyway, with a
+// message that says nothing about which flag was wrong. Everything else
+// a branch name may legally contain, semicolons, dollar signs and
+// backticks among it, is passed through untouched: this command never
+// builds a shell string, which is why those characters are safe here and
+// are exactly what makes a `--title` flag unsafe (see the doc comment on
+// runChangeContext).
+func validateRef(flagName, ref string) error {
+	if ref == "" {
+		return nil
+	}
+	if strings.HasPrefix(ref, "-") {
+		return fmt.Errorf("--%s: a ref may not begin with \"-\"", flagName)
+	}
+	if strings.ContainsRune(ref, 0) {
+		return fmt.Errorf("--%s: a ref may not contain a NUL byte", flagName)
 	}
+	return nil
 }
 
 // readCallerText reads one untrusted free-text field by reference. An

cmd/piace/debug.go

@@ -136,6 +136,20 @@ func (s *debugSink) observe(ev transport.Event) {
 // dump writes one body to a 0600 file. A dump failure is reported to
 // stderr but never fails the run: observation must not change a
 // comparison's or capture's outcome.
+//
+// Unlink first, then create exclusively, rather than os.WriteFile,
+// because these bodies are the unredacted ones. os.WriteFile opens an
+// existing path for writing: it follows a symlink already sitting there
+// and leaves whatever mode that file already had, applying 0600 only to
+// a file it creates itself. A dump directory is often /tmp or a CI
+// workspace, and neither is somewhere to take that on trust with raw
+// catalog values.
+//
+// os.Remove unlinks a symlink rather than its target, so the create that
+// follows always makes a fresh 0600 regular file. Removing first rather
+// than only creating exclusively keeps a second run against the same
+// dump directory working the way the first one did; the sequence numbers
+// restart at 001 each run, so O_EXCL alone would refuse every file.
 func (s *debugSink) dump(base string, body []byte) {
 	if len(body) == 0 {
 		return
@@ -145,7 +159,18 @@ func (s *debugSink) dump(base string, body []byte) {
 		name = base + ".json"
 	}
 	path := filepath.Join(s.dumpDir, name)
-	if err := os.WriteFile(path, body, 0o600); err != nil {
+	// A path that does not exist is the ordinary case; any other failure
+	// surfaces from the create below, with the message that names it.
+	_ = os.Remove(path)
+	f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600)
+	if err != nil {
+		fmt.Fprintf(s.stderr, "piace %s: debug: writing %s: %v\n", s.label, path, err)
+		return
+	}
+	if _, err := f.Write(body); err != nil {
+		fmt.Fprintf(s.stderr, "piace %s: debug: writing %s: %v\n", s.label, path, err)
+	}
+	if err := f.Close(); err != nil {
 		fmt.Fprintf(s.stderr, "piace %s: debug: writing %s: %v\n", s.label, path, err)
 	}
 }

cmd/piace/main.go

@@ -652,9 +652,7 @@ func runExplain(args []string, stdout, stderr *os.File) exitcode.Code {
 		fmt.Fprintf(stderr, "piace explain: %s\n", err)
 		return exitcode.OperationalError
 	}
-	if inferenceHTTPClient != nil {
-		client.HTTPClient = inferenceHTTPClient
-	}
+	client.SetHTTPClient(inferenceHTTPClient)
 
 	assessment, diagnostics := assess.Produce(context.Background(), client, result, changeContext, in.Assess, assess.Meta{
 		GeneratedAt:          clock().UTC().Format(time.RFC3339),