Merge pull request 'Close redaction gaps + honor the poll-attempt cap' (#1) from fix/audit-redaction-poller into main

tasks/poller.go

@@ -54,11 +54,32 @@ func (p *Poller) pollAllTasks() {
 	}
 }
 
+// attemptsExhausted records one poll against the task and reports whether the
+// configured MaxPollAttempts cap has been reached. A cap of 0 means unlimited (the
+// default), so this is a no-op unless an operator sets a limit; when the cap is hit
+// the task is failed so GetActive stops returning it and the poll loop lets go.
+func (p *Poller) attemptsExhausted(task *Task) bool {
+	if p.config.MaxPollAttempts <= 0 {
+		return false
+	}
+	task.Attempts++
+	if task.Attempts < p.config.MaxPollAttempts {
+		return false
+	}
+	task.Status = TaskStatusFailed
+	task.StatusMessage = fmt.Sprintf("gave up polling after %d attempts without reaching a terminal state", task.Attempts)
+	p.store.Update(task)
+	return true
+}
+
 // pollJobTask polls a job-based task using core.get_jobs
 func (p *Poller) pollJobTask(task *Task) {
 	if task.JobID == nil {
 		return
 	}
+	if p.attemptsExhausted(task) {
+		return
+	}
 
 	// Query job status
 	result, err := p.client.Call("core.get_jobs", []interface{}{
@@ -86,6 +107,9 @@ func (p *Poller) pollStatusTask(task *Task) {
 	if task.StatusMethod == "" {
 		return
 	}
+	if p.attemptsExhausted(task) {
+		return
+	}
 
 	// Call the status method
 	result, err := p.client.Call(task.StatusMethod)

tasks/types.go

@@ -35,6 +35,7 @@ type Task struct {
 
 	// Internal fields (not exposed in JSON)
 	OperationType OperationType          `json:"-"`
+	Attempts      int                    `json:"-"` // Poll attempts so far (for MaxPollAttempts)
 	JobID         *int                   `json:"-"` // For job-based ops
 	StatusMethod  string                 `json:"-"` // For status-based ops
 	ToolName      string                 `json:"-"`

tools/redact.go

@@ -3,6 +3,7 @@ package tools
 import (
 	"bytes"
 	"encoding/json"
+	"regexp"
 	"strings"
 )
 
@@ -19,12 +20,13 @@ import (
 // legitimate reason for a credential to reach the model, and "the operator
 // remembered to field-filter" is not a control.
 //
-// KNOWN LIMIT, stated plainly: this masks by KEY NAME (plus the name/value pair
-// shape below). A secret embedded inside an opaque string -- e.g. a custom app's
-// docker-compose YAML returned as one blob, with `POSTGRES_PASSWORD: hunter2`
-// inside it -- is NOT caught, because the key holding the blob isn't credential-
-// shaped and we will not regex the interior of arbitrary strings. get_app_config on
-// a custom/compose app is therefore not fully safe; prefer query_apps for those.
+// Redaction works on three shapes: by KEY NAME, by the name/value pair shape (see
+// envPairSecret below), and -- for a secret embedded inside an opaque string, e.g. a
+// custom app's docker-compose YAML returned as one blob with `POSTGRES_PASSWORD:
+// hunter2` inside it -- by scanning the interior of string values line by line (see
+// redactStringBlob). The blob scan only touches lines shaped like `key: value` or
+// `key=value` whose key is credential-shaped, so ordinary prose values are left
+// byte-for-byte intact. get_app_config on a custom/compose app is covered by this.
 
 const redactedMarker = "***REDACTED***"
 
@@ -114,11 +116,43 @@ func redactValue(v interface{}) interface{} {
 			out[i] = redactValue(val)
 		}
 		return out
+	case string:
+		return redactStringBlob(node)
 	default:
 		return v
 	}
 }
 
+// blobAssignLine matches a single "key: value" or "key=value" assignment as found
+// inside a compose / env / YAML blob returned as one opaque string. The optional
+// leading "- " covers compose's list-form environment (`- POSTGRES_PASSWORD=...`).
+var blobAssignLine = regexp.MustCompile(`^(\s*-?\s*)([A-Za-z0-9_.\-]+)(\s*[:=]\s*)(.+?)(\s*)$`)
+
+// redactStringBlob masks secret-keyed assignments embedded inside a string value --
+// the docker-compose YAML that get_app_config hands back as one field is the case
+// that matters. A string with no secret-shaped assignment line is returned exactly
+// as given, so ordinary short values and prose are never mangled.
+func redactStringBlob(s string) string {
+	if !strings.ContainsAny(s, ":=") {
+		return s
+	}
+	lines := strings.Split(s, "\n")
+	changed := false
+	for i, line := range lines {
+		m := blobAssignLine.FindStringSubmatch(line)
+		if m == nil || !looksSecret(m[2]) {
+			continue
+		}
+		// Preserve indent (m[1]), key (m[2]), and separator (m[3]); drop the value.
+		lines[i] = m[1] + m[2] + m[3] + redactedMarker
+		changed = true
+	}
+	if !changed {
+		return s
+	}
+	return strings.Join(lines, "\n")
+}
+
 // RedactJSON masks credential-looking fields in a JSON document. Input that is
 // not valid JSON is returned unchanged -- redaction must never destroy a
 // response it does not understand.

tools/redact_gaps_test.go

@@ -75,13 +75,25 @@ func TestRedactJSONPreservesLargeIntegers(t *testing.T) {
 	}
 }
 
-// Honest documentation of the remaining gap: a secret inside an opaque string blob
-// (a custom app's compose YAML) is NOT caught. This test asserts the CURRENT
-// behaviour so nobody mistakes it for safety -- if someone later fixes it, this
-// test fails loudly and should be updated.
-func TestRedactJSONDoesNotCatchSecretsInsideStringBlobs(t *testing.T) {
-	got := RedactJSON(`{"custom_compose_config_string": "services:\n  db:\n    environment:\n      POSTGRES_PASSWORD: hunter2\n"}`)
-	if !strings.Contains(got, "hunter2") {
-		t.Skip("string-blob redaction now implemented -- update this test and the README's known-limit note")
+// Previously the audit's honestly-documented gap: a secret inside an opaque string
+// blob (a custom app's compose YAML) sailed through because redaction masked by key
+// name only. redactStringBlob now scans the interior of string values, so this is
+// covered -- a POSTGRES_PASSWORD embedded in a compose blob must be masked, while a
+// non-secret line in the same blob survives.
+func TestRedactJSONMasksSecretsInsideStringBlobs(t *testing.T) {
+	got := RedactJSON(`{"custom_compose_config_string": "services:\n  db:\n    environment:\n      POSTGRES_PASSWORD: hunter2\n      TZ: America/New_York\n    ports:\n      - 5432:5432\n"}`)
+	if strings.Contains(got, "hunter2") {
+		t.Errorf("secret inside a compose-YAML string blob survived:\n%s", got)
+	}
+	// The compose-list dotenv form (- KEY=value) must also be caught.
+	got2 := RedactJSON(`{"env_blob": "FOO=bar\nAPI_TOKEN=sk_live_deadbeef\n"}`)
+	if strings.Contains(got2, "sk_live_deadbeef") {
+		t.Errorf("secret inside a KEY=value env blob survived:\n%s", got2)
+	}
+	// Non-secret lines, and structural YAML, must be left intact.
+	for _, keep := range []string{"America/New_York", "services:", "5432:5432", "bar"} {
+		if !strings.Contains(got+got2, keep) {
+			t.Errorf("blob scan destroyed a non-secret line %q:\n%s\n%s", keep, got, got2)
+		}
 	}
 }

tools/registry.go

@@ -2024,7 +2024,7 @@ func handleQueryPools(client *truenas.Client, args map[string]interface{}) (stri
 
 	var pools []map[string]interface{}
 	if err := json.Unmarshal(result, &pools); err != nil {
-		return "", fmt.Errorf("failed to parse pools (raw response: %s): %w", string(result), err)
+		return "", fmt.Errorf("failed to parse pools (raw response: %s): %w", RedactJSON(string(result)), err)
 	}
 
 	formatted, err := json.MarshalIndent(pools, "", "  ")