Fix a JSON-escaping gap in redaction, remove OCI dependency from a test

Found by an independent, adversarial pre-submission review agent with no
prior context of this branch's work - the highest-value kind of finding this
whole audit process was designed to surface.

The redaction gap
------------------
redactSensitiveValues does a literal strings.ReplaceAll of the raw configured
secret value against the already-JSON-marshaled manifest/resources text. Any
value containing a character JSON escapes - a double quote, a backslash, or a
control character such as a newline - never appears in that text as its raw
bytes, so the search matched nothing and the secret (readable, merely
re-escaped) shipped into state and plan output unredacted. Multi-line secrets
- the canonical case being a PEM private key or certificate - were affected on
every single apply, silently, independent of the map-orientation bug fixed in
the previous commit.

Reproduced directly before fixing: a value containing embedded newlines,
quotes and backslashes survived "redaction" fully readable in the manifest
attribute.

Fix: jsonEscapedForm returns what a value looks like once embedded in a JSON
string field - json.Marshal's own encoding, quotes stripped - and
redactSensitiveValues now searches for that instead of the raw value. For a
plain alphanumeric secret (everything the existing test suite already
covered) the escaped form is byte-identical to the raw value, so this is
purely additive. Verified the fix is real, not tautological, by reintroducing
the raw-value search and confirming all 7 new special-character test cases
fail against it before restoring the fix.

Confirmed every JSON-producing call site in this codebase (manifest,
resources, and this new lookup) uses plain json.Marshal, not a custom
HTML-safe-escaping-disabled encoder, so the escaping rules the lookup assumes
are exactly the rules the producers actually use - including Go's
HTML-escaping of <, >, & by default, which is otherwise easy to miss.

Also updated .changelog/1879.txt, which the review correctly flagged as
overclaiming ("Both are now fixed") before this commit closed the remaining
gap - it's accurate now.

The OCI-chart test dependency
-------------------------------
The same review flagged TestAccResourceRelease_manifestOCIChartUpgrade's
dependency on a third-party OCI registry (ghcr.io/berriai/litellm-helm) as
worth a second look. Investigating: this repo's own test-chart can't actually
exercise the ownership-metadata fix at all, because its _helpers.tpl already
sets app.kubernetes.io/managed-by itself - the dry-run and live sides agree
for a completely unrelated reason (both read the same chart template), so the
bug this fix addresses never has a chance to surface. That's exactly why the
real litellm-helm chart was needed in the first place: its ConfigMap has no
labels block of its own.

Added testdata/charts/bare-metadata, a two-file fixture chart whose ConfigMap
deliberately declares no labels, and a new fully-local, offline-safe
acceptance test using it. Confirmed non-tautological the same way: reverting
setDryRunOwnershipMetadata's label injection reproduces the exact original
"Provider produced inconsistent result after apply" failure against this
local chart; restoring it passes. The OCI-based test is kept alongside it
(still gated behind testing.Short(), so it never runs by default) for the
additional real-world-fidelity coverage a synthetic chart can't provide - not
a replacement, an addition.

Verification
------------
go build / vet / test -race clean, gofmt clean. All 23 acceptance tests from
this session's work pass together.
Claude-Session: https://claude.ai/code/session_01SNanSDUQNVBmGcTWZiQV1A

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: johannesgesenhues

.changelog/1879.txt

@@ -1,3 +1,3 @@
 ```release-note:bug
-`resource/helm_release`: Fix `set_sensitive` values never actually being redacted from the `manifest` and `resources` attributes when `experiments.manifest` is enabled. `redactSensitiveValues` was handed a map keyed by attribute name with the real value discarded or replaced by a placeholder, so it searched the stored manifest for literal attribute names like `dbPassword` and never touched the actual secret text - which then flowed into Terraform state and plan output in the clear, on every apply, silently (no error, nothing visible without inspecting the state file directly). The `resources` attribute additionally had no redaction at all, independent of this bug. Both are now fixed: the real configured value is what gets searched for and hashed out, on both attributes, on every code path (read, and both create/update dry runs).
+`resource/helm_release`: Fix `set_sensitive` values never actually being redacted from the `manifest` and `resources` attributes when `experiments.manifest` is enabled. `redactSensitiveValues` was handed a map keyed by attribute name with the real value discarded or replaced by a placeholder, so it searched the stored manifest for literal attribute names like `dbPassword` and never touched the actual secret text - which then flowed into Terraform state and plan output in the clear, on every apply, silently (no error, nothing visible without inspecting the state file directly). The `resources` attribute additionally had no redaction at all, independent of this bug. A separate gap in the same code path: the search compared the raw configured value against the already-JSON-encoded manifest text, so any value containing a character JSON escapes - a quote, a backslash, or a control character such as a newline, as in a multi-line PEM private key or certificate - was compared against the wrong bytes and left unredacted too. All three are now fixed: the real configured value, in the form it actually appears in the JSON text, is what gets searched for and hashed out, on both attributes, on every code path (read, and both create/update dry runs).
 ```

helm/manifest_json.go

@@ -246,12 +246,50 @@ func redactSensitiveValues(text string, sensitiveValues []string) string {
 		if value == "" {
 			continue
 		}
-		masked = strings.ReplaceAll(masked, value, hashSensitiveValue(value))
+
+		// text is always JSON (the manifest/resources attributes are always
+		// produced by json.Marshal), so a value containing a character JSON
+		// escapes - a double quote, a backslash, or a control character such
+		// as a newline - never appears in text as its raw bytes. It appears
+		// as its JSON-escaped form instead: a multi-line secret like a PEM
+		// private key or certificate, or any secret containing a quote or
+		// backslash, would otherwise survive "redaction" fully readable,
+		// just with backslash-n / backslash-quote / double-backslash in
+		// place of the original control characters. jsonEscapedForm is
+		// exactly what value looks like once embedded in the JSON string
+		// field that holds it, so search for that instead of the raw value.
+		//
+		// For a value with no JSON-special characters (the common case -
+		// plain alphanumeric secrets), the escaped form is byte-identical to
+		// the raw value, so this changes nothing for the values every
+		// existing test already covers.
+		escaped, ok := jsonEscapedForm(value)
+		if !ok || escaped == "" {
+			continue
+		}
+		masked = strings.ReplaceAll(masked, escaped, hashSensitiveValue(value))
 	}
 
 	return masked
 }
 
+// jsonEscapedForm returns value as it appears inside a JSON string field -
+// json.Marshal's quoted encoding with the surrounding quotes stripped - or
+// false if value cannot be encoded as a JSON string (never true for a Go
+// string, which is always valid UTF-8 input to json.Marshal; the check
+// exists so a theoretical encoding failure skips that one value instead of
+// panicking or silently matching nothing).
+func jsonEscapedForm(value string) (string, bool) {
+	b, err := json.Marshal(value)
+	if err != nil {
+		return "", false
+	}
+	if len(b) < 2 || b[0] != '"' || b[len(b)-1] != '"' {
+		return "", false
+	}
+	return string(b[1 : len(b)-1]), true
+}
+
 func redactSecretData(secret *corev1.Secret) {
 	for k, v := range secret.Data {
 		h := hashSensitiveValue(string(v))

helm/manifest_redaction_test.go

@@ -5,6 +5,7 @@ package helm
 
 import (
 	"context"
+	"encoding/json"
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-framework/attr"
@@ -140,3 +141,79 @@ spec:
 		assert.Contains(t, redacted, "(sensitive value", "keyed=%v: hash marker must be present", keyed)
 	}
 }
+
+// TestJSONEscapedForm pins jsonEscapedForm's contract directly: it must
+// return exactly what json.Marshal would put inside a JSON string field for
+// this value, with the surrounding quotes stripped - the plain-value case is
+// a byte-identical no-op, everything else is genuinely transformed.
+func TestJSONEscapedForm(t *testing.T) {
+	for name, tc := range map[string]struct {
+		value string
+		want  string
+	}{
+		"plain alphanumeric":               {value: "correct-horse-battery-staple", want: "correct-horse-battery-staple"},
+		"empty string":                     {value: "", want: ""},
+		"contains a newline":               {value: "line one\nline two", want: `line one\nline two`},
+		"contains a quote":                 {value: `say "hello"`, want: `say \"hello\"`},
+		"contains a backslash":             {value: `C:\path\to\thing`, want: `C:\\path\\to\\thing`},
+		"contains tab and carriage return": {value: "a\tb\rc", want: `a\tb\rc`},
+		"contains a null byte":             {value: "a\x00b", want: `a\u0000b`},
+		"contains angle brackets and amp":  {value: "<script>&amp;</script>", want: `\u003cscript\u003e\u0026amp;\u003c/script\u003e`},
+		"unicode":                          {value: "héllo wörld 日本語 🚀", want: "héllo wörld 日本語 🚀"},
+		"multi-line secret with mixed special characters": {
+			value: "line-one-canary\nline-two-quo\"te\nline-three-back\\slash",
+			want:  `line-one-canary\nline-two-quo\"te\nline-three-back\\slash`,
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			got, ok := jsonEscapedForm(tc.value)
+			require.True(t, ok)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}
+
+// TestRedactSensitiveValues_MultiLineAndSpecialCharacters is a regression
+// test for a real gap found during pre-submission review: redactSensitiveValues
+// used to search the JSON manifest text for the RAW configured secret value.
+// Since that text is always JSON, any value containing a character JSON
+// escapes - a quote, a backslash, or a control character such as a newline -
+// never appears in the text as its raw bytes, so the raw-value search found
+// nothing and the secret (in its readable, merely re-escaped form) shipped
+// into state/plan output unredacted. A multi-line secret - a PEM private key
+// or certificate being the obvious real-world case - was affected on every
+// single apply, silently, regardless of the map-orientation bug fixed
+// alongside this one.
+func TestRedactSensitiveValues_MultiLineAndSpecialCharacters(t *testing.T) {
+	for name, secret := range map[string]string{
+		"multi-line with embedded newlines":                                     "line-one-canary\nline-two-indented\nline-three-end",
+		"contains a double quote":                                               `value with a "quoted phrase" inside it`,
+		"contains a backslash":                                                  `C:\Users\canary\secret.txt`,
+		"contains a backslash immediately before a quote":                       `path\"escaped`,
+		"contains tab and carriage return":                                      "a\tcanary\rb",
+		"contains a null byte":                                                  "before\x00after-canary",
+		"contains angle brackets (HTML-unsafe under Go's default json.Marshal)": "<canary>&value</canary>",
+	} {
+		t.Run(name, func(t *testing.T) {
+			manifest := podEnv(secret)
+			for _, keyed := range []bool{false, true} {
+				jsonManifest, err := convertYAMLManifestToJSON(manifest, keyed)
+				require.NoError(t, err)
+
+				redacted := redactSensitiveValues(jsonManifest, []string{secret})
+
+				assert.NotContains(t, redacted, "canary", "keyed=%v: secret content must not survive redaction: %s", keyed, redacted)
+				assert.Contains(t, redacted, "(sensitive value", "keyed=%v: hash marker must be present", keyed)
+			}
+		})
+	}
+}
+
+// podEnv renders a manifest whose single env value, once through YAML's
+// block-scalar handling, round-trips secret byte-for-byte (YAML block
+// literals preserve embedded quotes/backslashes/control characters exactly,
+// which is what this test needs to actually exercise the escaping gap).
+func podEnv(value string) string {
+	b, _ := json.Marshal(value) // reuse Go's own JSON string encoding as a safe way to embed an arbitrary string inside YAML too
+	return "apiVersion: v1\nkind: Pod\nmetadata:\n  name: p\nspec:\n  containers:\n  - name: app\n    env:\n    - name: TLS_KEY\n      value: " + string(b) + "\n"
+}

helm/resource_helm_release_test.go

@@ -3373,3 +3373,135 @@ func testAccHelmReleaseConfigKeyedResources(resource, ns, name string) string {
 		}
 	`, resource, name, ns, testRepositoryURL, "1.2.3")
 }
+
+// TestAccResourceRelease_manifestRedactsMultiLineSetSensitive is a real
+// end-to-end regression test for a gap found during pre-submission review of
+// the fix above: redactSensitiveValues searched the JSON manifest text for
+// the RAW configured secret value, but that text is always JSON, so a value
+// containing a character JSON escapes - a quote, a backslash, or a control
+// character such as a newline - never appears in it as raw bytes. Secrets
+// containing such characters (a PEM private key/certificate being the
+// canonical multi-line case) therefore survived "redaction" fully readable,
+// merely re-escaped, on every single apply. This is the same
+// state-inspection technique as the sibling test above, applied to a secret
+// shaped like the ones that were actually affected.
+func TestAccResourceRelease_manifestRedactsQuoteContainingSetSensitive(t *testing.T) {
+	namespace := createRandomNamespace(t)
+	defer deleteNamespace(t, namespace)
+	name := randName("redact-esc")
+
+	// A quote alone is enough to exercise the JSON-escaping gap ("→\") without
+	// tripping over set_sensitive's own, unrelated strvals.ParseInto value
+	// parsing (Helm's --set-style mini-language, which treats backslash as
+	// its own escape character and can't carry a raw embedded newline through
+	// a single "key=value" argument at all) - a value containing a literal
+	// newline or backslash gets mangled by THAT parser before it ever reaches
+	// the chart, which is a real, separate, pre-existing set_sensitive
+	// behavior unrelated to redaction. The unit-level tests in
+	// manifest_redaction_test.go cover the newline/backslash cases directly
+	// against redactSensitiveValues, bypassing strvals entirely, which is
+	// where those cases are actually meaningful to test.
+	const secretValue = `canary-with-a-"quoted-phrase"-inside-it`
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: protoV6ProviderFactories(),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccHelmReleaseConfigSetSensitiveManifest(testResourceName, namespace, name, secretValue),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					resource.TestCheckResourceAttrSet("helm_release.test", "manifest"),
+					func(s *terraform.State) error {
+						res := s.RootModule().Resources["helm_release.test"]
+						if res == nil || res.Primary == nil {
+							return fmt.Errorf("helm_release.test not found in state")
+						}
+
+						foundMarker := false
+						for key, value := range res.Primary.Attributes {
+							if key != "manifest" && !strings.HasPrefix(key, "resources.") {
+								continue
+							}
+							if strings.Contains(value, "canary-with-a") {
+								return fmt.Errorf("quote-containing set_sensitive value leaked into computed attribute %q, readable: %s", key, value)
+							}
+							if strings.Contains(value, "(sensitive value") {
+								foundMarker = true
+							}
+						}
+						if !foundMarker {
+							return fmt.Errorf("expected the redaction hash marker in manifest or resources[...]; attribute may not have been populated")
+						}
+						return nil
+					},
+				),
+			},
+		},
+	})
+}
+
+// TestAccResourceRelease_ownershipMetadataLocalChart is a fully local,
+// offline-safe regression test for the setDryRunOwnershipMetadata fix,
+// covering the same property TestAccResourceRelease_manifestOCIChartUpgrade
+// proves against a real third-party chart, without any external dependency.
+// Uses ./testdata/charts/bare-metadata, whose ConfigMap deliberately declares
+// no labels of its own - most real charts (including this repo's own
+// test-chart, via _helpers.tpl) set app.kubernetes.io/managed-by themselves,
+// which means the label agrees on both the dry-run and live sides for a
+// completely unrelated reason (both read it from the same chart template)
+// and never actually exercises the code path this fix touches. Without the
+// fix, this fails the create step outright with "Provider produced
+// inconsistent result after apply" on resources[...].
+func TestAccResourceRelease_ownershipMetadataLocalChart(t *testing.T) {
+	namespace := createRandomNamespace(t)
+	defer deleteNamespace(t, namespace)
+	name := randName("bare-metadata")
+
+	resource.Test(t, resource.TestCase{
+		ProtoV6ProviderFactories: protoV6ProviderFactories(),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccHelmReleaseConfigBareMetadata(testResourceName, namespace, name),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					func(s *terraform.State) error {
+						res := s.RootModule().Resources["helm_release.test"]
+						if res == nil || res.Primary == nil {
+							return fmt.Errorf("helm_release.test not found in state")
+						}
+						for key, value := range res.Primary.Attributes {
+							if strings.HasPrefix(key, "resources.") && strings.Contains(key, "configmap") {
+								if !strings.Contains(value, `"app.kubernetes.io/managed-by":"Helm"`) {
+									return fmt.Errorf("resources[%s] is missing the Helm-injected managed-by label the fix is supposed to predict: %s", key, value)
+								}
+								return nil
+							}
+						}
+						return fmt.Errorf("no configmap found under resources[...] to check")
+					},
+				),
+			},
+			{
+				// A second apply (an Update, not just a Create) with a
+				// deliberately unrelated change, to prove the fix holds on
+				// the update dry-run path too, not just install.
+				Config: testAccHelmReleaseConfigBareMetadata(testResourceName, namespace, name),
+				Check:  resource.TestCheckResourceAttrSet("helm_release.test", "manifest"),
+			},
+		},
+	})
+}
+
+func testAccHelmReleaseConfigBareMetadata(resource, ns, name string) string {
+	return fmt.Sprintf(`
+		provider helm {
+			experiments = {
+				manifest = true
+			}
+		}
+
+		resource "helm_release" "%s" {
+			name      = %q
+			namespace = %q
+			chart     = "./testdata/charts/bare-metadata"
+		}
+	`, resource, name, ns)
+}

helm/testdata/charts/bare-metadata/Chart.yaml

@@ -0,0 +1,14 @@
+apiVersion: v2
+name: bare-metadata
+description: >-
+  A minimal fixture chart whose ConfigMap declares no labels of its own -
+  deliberately, so app.kubernetes.io/managed-by only ever appears via Helm's
+  own metadata injection (setMetadataVisitor), never via the chart template.
+  Most real charts declare this label themselves through a common labels
+  helper, which means they can never expose a dry-run/live discrepancy in
+  that label - this chart exists specifically so a discrepancy there is
+  reproducible locally, without depending on a third-party chart whose
+  templates happen to omit it.
+type: application
+version: 1.0.0
+appVersion: "1.0"

helm/testdata/charts/bare-metadata/templates/configmap.yaml

@@ -0,0 +1,9 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: {{ .Release.Name }}-bare
+  # Deliberately no `labels:` block - no app.kubernetes.io/managed-by, no
+  # helm.sh/chart, nothing. Every one of those comes solely from Helm's own
+  # setMetadataVisitor at apply time, not from this template.
+data:
+  marker: bare-metadata