From 3fb20797c9b4e65943bcbf1a11a620f6160e62cb Mon Sep 17 00:00:00 2001
From: Jonathan Moss <2729151+jwmoss@users.noreply.github.com>
Date: Fri, 11 Sep 2026 21:08:35 -0400
Subject: [PATCH] fix: neutralize untrusted workspace strings and redact
 credential secrets

Sanitize progress lines, the auth status table, and HCL comment text;
extend terminalText to format and line separator characters; redact
storage credential secret fields from inventory output; cap grant
pagination; and allowlist the workspace_error and render_error
telemetry categories.
---
 CHANGELOG.md                         |  5 ++++
 docs/telemetry.md                    |  3 +-
 internal/cli/auth.go                 |  8 +++---
 internal/cli/errors.go               |  4 ++-
 internal/cli/errors_test.go          | 26 +++++++++++++++++
 internal/cli/root.go                 |  7 ++++-
 internal/cli/telemetry_codes_test.go | 36 ++++++++++++++++++++++++
 internal/contract/hcl.go             |  6 +++-
 internal/inventory/redact.go         | 20 +++++++++++++
 internal/inventory/redact_test.go    | 42 ++++++++++++++++++++++++++++
 internal/inventory/unitycatalog.go   | 13 +++++++--
 internal/telemetry/event.go          |  1 +
 12 files changed, 161 insertions(+), 10 deletions(-)
 create mode 100644 internal/cli/telemetry_codes_test.go
 create mode 100644 internal/inventory/redact.go
 create mode 100644 internal/inventory/redact_test.go

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 06504f6..0e57961 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,11 @@ This project uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Security
 
+- Progress lines, the `auth status` table, and generated HCL comments neutralize control,
+  format, and line separator characters that come from workspace object names.
+- `inventory.json` and `inventory --json` no longer serialize storage credential secret fields.
+- Grant pagination stops with an issue after 1000 pages or a repeated page token.
+- Telemetry records `workspace_error` and `render_error` instead of collapsing them to `other`.
 - The release job no longer receives the winget token or runs tests with release credentials.
 - Release builds use the commit timestamp for the build date and module timestamps.
 - The installers require HTTPS and TLS 1.2, and match checksum entries exactly.
diff --git a/docs/telemetry.md b/docs/telemetry.md
index 9466b32..7a4f63e 100644
--- a/docs/telemetry.md
+++ b/docs/telemetry.md
@@ -96,7 +96,8 @@ Every event has these fields and no others:
 Error categories are `none`, `other`, `invalid_usage`, `configuration_error`,
 `authentication_failed`, `permission_denied`, `not_found`, `resource_exhausted`,
 `service_error`, `api_error`, `connection_failed`, `operation_failed`, `timeout`,
-`canceled`, `partial_result`, and `output_error`. Unknown categories become `other`.
+`canceled`, `partial_result`, `output_error`, `workspace_error`, and `render_error`.
+Unknown categories become `other`.
 
 `--allow-partial` still reports `partial`, even when the command exits successfully.
 Default resource groups also count as selected types.
diff --git a/internal/cli/auth.go b/internal/cli/auth.go
index 6c3529d..2e3f332 100644
--- a/internal/cli/auth.go
+++ b/internal/cli/auth.go
@@ -45,11 +45,11 @@ Use inventory --json to check the visible resource selection.`,
 			}
 			rc.out.Success("authenticated")
 			rc.out.Table([]string{"KEY", "VALUE"}, [][]string{
-				{"host", id.Host},
-				{"user", id.UserName},
-				{"auth_type", id.AuthType},
+				{"host", terminalText(id.Host)},
+				{"user", terminalText(id.UserName)},
+				{"auth_type", terminalText(id.AuthType)},
 				{"workspace_id", fmt.Sprint(id.WorkspaceID)},
-				{"metastore_id", id.MetastoreID},
+				{"metastore_id", terminalText(id.MetastoreID)},
 			})
 			return nil
 		},
diff --git a/internal/cli/errors.go b/internal/cli/errors.go
index 0800061..728e459 100644
--- a/internal/cli/errors.go
+++ b/internal/cli/errors.go
@@ -86,10 +86,12 @@ func outputError(err error) error {
 		"Check the destination and disk access. Use --out with a new directory. Keep existing files.")
 }
 
+// terminalText escapes control, format, and line separator characters so untrusted
+// workspace strings cannot rewrite, hide, or reorder terminal output.
 func terminalText(value string) string {
 	var text strings.Builder
 	for _, char := range value {
-		if unicode.IsControl(char) {
+		if unicode.IsControl(char) || unicode.Is(unicode.Cf, char) || char == '\u2028' || char == '\u2029' {
 			quoted := strconv.QuoteRune(char)
 			text.WriteString(quoted[1 : len(quoted)-1])
 		} else {
diff --git a/internal/cli/errors_test.go b/internal/cli/errors_test.go
index 4829010..3b82989 100644
--- a/internal/cli/errors_test.go
+++ b/internal/cli/errors_test.go
@@ -193,3 +193,29 @@ func TestOutputFailureDiagnostic(t *testing.T) {
 		t.Fatalf("unexpected diagnostic: %v", failure)
 	}
 }
+
+func TestTerminalTextNeutralizesControlAndFormatCharacters(t *testing.T) {
+	for _, hostile := range []string{
+		"a\x1b[2Kb", "a\rb", "a\u202eb", "a\u2066b", "a\u2028b", "a\u200fb",
+	} {
+		safe := terminalText(hostile)
+		if strings.ContainsAny(safe, "\x1b\r\u202e\u2066\u2028\u200f") {
+			t.Fatalf("%q was not neutralized: %q", hostile, safe)
+		}
+		if !strings.HasPrefix(safe, "a") || !strings.HasSuffix(safe, "b") {
+			t.Fatalf("%q lost visible text: %q", hostile, safe)
+		}
+	}
+	if terminalText("Analytics WH") != "Analytics WH" {
+		t.Fatal("plain text changed")
+	}
+}
+
+func TestProgressOutputNeutralizesWorkspaceNames(t *testing.T) {
+	var stderr strings.Builder
+	rc := &runtime{stderr: &stderr, g: &globals{}}
+	rc.progress()("  Warehouse: %s", "prod\x1b[2K\rok")
+	if got := stderr.String(); strings.Contains(got, "\x1b") || strings.Contains(got, "\r") {
+		t.Fatalf("progress leaked control characters: %q", got)
+	}
+}
diff --git a/internal/cli/root.go b/internal/cli/root.go
index 81ec104..3f4867d 100644
--- a/internal/cli/root.go
+++ b/internal/cli/root.go
@@ -217,7 +217,12 @@ func (rc *runtime) progress() func(string, ...any) {
 		return nil
 	}
 	return func(format string, args ...any) {
-		_, _ = fmt.Fprintf(rc.stderr, format+"\n", args...)
+		// Workspace object names are untrusted. Neutralize terminal control sequences at the sink.
+		safe := make([]any, len(args))
+		for i, arg := range args {
+			safe[i] = terminalText(fmt.Sprint(arg))
+		}
+		_, _ = fmt.Fprintf(rc.stderr, format+"\n", safe...)
 	}
 }
 
diff --git a/internal/cli/telemetry_codes_test.go b/internal/cli/telemetry_codes_test.go
new file mode 100644
index 0000000..e5dd3f2
--- /dev/null
+++ b/internal/cli/telemetry_codes_test.go
@@ -0,0 +1,36 @@
+package cli
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/536tech/datatf/internal/telemetry"
+)
+
+// Every error code the CLI attaches with withHint, plus every diagnostic category, must be
+// an allowlisted telemetry category. Otherwise the collector records it as "other".
+func TestTelemetryAllowsEveryCLIErrorCode(t *testing.T) {
+	for _, code := range []string{
+		"invalid_usage", "configuration_error", "authentication_failed", "permission_denied",
+		"not_found", "resource_exhausted", "service_error", "api_error", "connection_failed",
+		"operation_failed", "timeout", "canceled", "partial_result", "output_error",
+		"workspace_error", "render_error",
+	} {
+		payload, err := telemetry.Payload(telemetry.Run{
+			Version: "1.0.0", OS: "linux", Arch: "amd64", Command: "export", Scope: "workspace",
+			Outcome: "error", ErrorCode: code,
+		})
+		if err != nil {
+			t.Fatal(err)
+		}
+		var event struct {
+			ErrorCode string `json:"error_code"`
+		}
+		if err := json.Unmarshal(payload, &event); err != nil {
+			t.Fatal(err)
+		}
+		if event.ErrorCode != code {
+			t.Errorf("%s recorded as %s", code, event.ErrorCode)
+		}
+	}
+}
diff --git a/internal/contract/hcl.go b/internal/contract/hcl.go
index 5aa9805..b717d9a 100644
--- a/internal/contract/hcl.go
+++ b/internal/contract/hcl.go
@@ -186,9 +186,13 @@ func appendTraversalIndex(tokens, index hclwrite.Tokens) hclwrite.Tokens {
 	return append(tokens, &hclwrite.Token{Type: hclsyntax.TokenCBrack, Bytes: []byte("]")})
 }
 
+// commentLine is the only text that bypasses hclwrite. A line break would end the comment,
+// so it is never allowed through.
+var commentLine = strings.NewReplacer("\r\n", " ", "\r", " ", "\n", " ", "\u2028", " ", "\u2029", " ")
+
 func writeComments(buf *bytes.Buffer, lines []string) {
 	for _, line := range lines {
-		buf.WriteString("# " + line + "\n")
+		buf.WriteString("# " + commentLine.Replace(line) + "\n")
 	}
 }
 
diff --git a/internal/inventory/redact.go b/internal/inventory/redact.go
new file mode 100644
index 0000000..9d7fb8d
--- /dev/null
+++ b/internal/inventory/redact.go
@@ -0,0 +1,20 @@
+package inventory
+
+import "github.com/databricks/databricks-sdk-go/service/catalog"
+
+// redactStorageCredential drops credential secret fields before the SDK struct is
+// serialized into inventory files or JSON output. Unity Catalog redacts these values on
+// read today; DataTF never relies on that.
+func redactStorageCredential(info catalog.StorageCredentialInfo) catalog.StorageCredentialInfo {
+	if info.AzureServicePrincipal != nil {
+		sp := *info.AzureServicePrincipal
+		sp.ClientSecret = ""
+		info.AzureServicePrincipal = &sp
+	}
+	if info.CloudflareApiToken != nil {
+		token := *info.CloudflareApiToken
+		token.SecretAccessKey = ""
+		info.CloudflareApiToken = &token
+	}
+	return info
+}
diff --git a/internal/inventory/redact_test.go b/internal/inventory/redact_test.go
new file mode 100644
index 0000000..4d78e64
--- /dev/null
+++ b/internal/inventory/redact_test.go
@@ -0,0 +1,42 @@
+package inventory
+
+import (
+	"encoding/json"
+	"strings"
+	"testing"
+
+	"github.com/databricks/databricks-sdk-go/service/catalog"
+)
+
+func TestRedactStorageCredentialDropsSecrets(t *testing.T) {
+	info := catalog.StorageCredentialInfo{
+		Name: "adls",
+		AzureServicePrincipal: &catalog.AzureServicePrincipal{
+			ApplicationId: "app", DirectoryId: "dir", ClientSecret: "canary-secret",
+		},
+		CloudflareApiToken: &catalog.CloudflareApiToken{
+			AccessKeyId: "key", AccountId: "acct", SecretAccessKey: "canary-token",
+		},
+	}
+	original, err := json.Marshal(info)
+	if err != nil {
+		t.Fatal(err)
+	}
+	redacted, err := json.Marshal(redactStorageCredential(info))
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, canary := range []string{"canary-secret", "canary-token"} {
+		if strings.Contains(string(redacted), canary) {
+			t.Fatalf("secret survived redaction: %s", redacted)
+		}
+	}
+	for _, kept := range []string{`"application_id":"app"`, `"directory_id":"dir"`, `"access_key_id":"key"`} {
+		if !strings.Contains(string(redacted), kept) {
+			t.Fatalf("non-secret field dropped: %s", redacted)
+		}
+	}
+	if !strings.Contains(string(original), "canary-secret") {
+		t.Fatal("redaction mutated the caller's value")
+	}
+}
diff --git a/internal/inventory/unitycatalog.go b/internal/inventory/unitycatalog.go
index f5ada74..6efa132 100644
--- a/internal/inventory/unitycatalog.go
+++ b/internal/inventory/unitycatalog.go
@@ -68,11 +68,14 @@ func sortedWorkspaceBindings(bindings []catalog.WorkspaceBinding) []catalog.Work
 	return out
 }
 
+// maxGrantPages bounds a hostile or looping paginated response.
+const maxGrantPages = 1000
+
 // grants reads direct privilege assignments on a securable, following pages.
 func (r *Reader) grants(ctx context.Context, securableType, fullName string) ([]Grant, bool) {
 	out := []Grant{}
 	pageToken := ""
-	for {
+	for page := 1; ; page++ {
 		resp, err := r.ws.Grants.Get(ctx, catalog.GetGrantRequest{
 			SecurableType: securableType,
 			FullName:      fullName,
@@ -93,6 +96,11 @@ func (r *Reader) grants(ctx context.Context, securableType, fullName string) ([]
 		if resp.NextPageToken == "" {
 			break
 		}
+		if resp.NextPageToken == pageToken || page >= maxGrantPages {
+			r.issue("unity_catalog", fullName, securableType+" grants",
+				fmt.Errorf("pagination did not finish after %d pages", page))
+			return nil, false
+		}
 		pageToken = resp.NextPageToken
 	}
 	sort.SliceStable(out, func(i, j int) bool { return out[i].Principal < out[j].Principal })
@@ -241,7 +249,8 @@ func (r *Reader) readStorageCredentials(ctx context.Context) {
 			)
 			r.logf("  Storage credential: %s (%s)", name, ownership)
 			sc := &StorageCredential{
-				Info: *info, Ownership: ownership, WorkspaceIDs: ids, WorkspaceBindings: bindings,
+				Info: redactStorageCredential(*info), Ownership: ownership,
+				WorkspaceIDs: ids, WorkspaceBindings: bindings,
 			}
 			sc.Grants, sc.GrantsRead = r.grants(gctx, "storage_credential", name)
 			if sc.Grants == nil {
diff --git a/internal/telemetry/event.go b/internal/telemetry/event.go
index 128b60f..fb81273 100644
--- a/internal/telemetry/event.go
+++ b/internal/telemetry/event.go
@@ -40,6 +40,7 @@ var codes = []string{
 	"none", "other", "invalid_usage", "configuration_error", "authentication_failed",
 	"permission_denied", "not_found", "resource_exhausted", "service_error", "api_error",
 	"connection_failed", "operation_failed", "timeout", "canceled", "partial_result", "output_error",
+	"workspace_error", "render_error",
 }
 
 var releaseVersion = regexp.MustCompile(