Redact secret values from conversion-failure exceptions (S1)

FIX-PLAN.md

@@ -7,8 +7,11 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform
 - **Q1–Q4 proven** via isolated micro-benchmarks (macro `ScanBenchmark` can't resolve them): Q1 2.7× / 64 KB→88 B · Q3 2.65× / 152 B→0 · Q4 32× / 224 B→0. **Q5 was already resolved by B4.** **M1** (code-review finding): namespace-qualify the generated impl name in the generator only — `GetNormalizeInterfaceName` also backs the section name. Suite → **56 per TFM**.
 - **Merged since:** **benchmark-tracking CI (#22)** — BDN on push/PR, gates PRs on **allocation** regressions (>10%) via github-action-benchmark on `gh-pages`; time informational. · **session-wrap docs (#23)** · **P3 — cached "settings plan" (#24)** — `SettingsPlan` per type (section name once+lazy, key/default/converter precomputed, `[SettingsProperty]` read once). Warm re-populate **−55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. Reviewed by code+perf agents; emitted/compiled setter reverted (regressed the gated cold scan for **no** warm gain — net10 `SetValue` is already alloc-free). New gated `PlanPopulateBenchmark` tracks the warm path.
 - **P4 merged (#25)** — de-reflect + DRY the array/enumerable converters: shared `CollectionTypeConverter` (`Array.CreateInstance` + indexed fill, manual `LinkedList` walk); `CreateNullResult` de-reflected. Gated `ConvertArrayBenchmark`: **1.33 KB→688 B (−49%), 5.7×**. Suite 68/TFM. Included a user modernization pass (collection expressions across ~19 files).
-- **In flight:** **P5 — resolve config section once per type** = **implemented + reviewed clean on branch `perf/p5-resolve-section-once`** (awaiting commit/PR at time of writing). `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`). Plan reviewed by architect+perf+security (chose internal cache over a contract change — layering); code reviewed via `/code-review`. Gated `ConfigBinderBenchmark`: **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)**. Suite **71/TFM** (+3 parity/live-view tests). Also gated P4's `ConvertArrayBenchmark` (was never in the CI filter). **Security review surfaced a pre-existing leak → new item S1.**
-- **Next:** S1 (redact secret from exception message) · optional P3b (tiered/lazy compiled setter, only if set *time* shows in a profile).
+- **P5 merged (#26)** — resolve config section once per type: `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`). Plan reviewed by architect+perf+security (chose internal cache over a contract change — layering); code reviewed via `/code-review`. Gated `ConfigBinderBenchmark`: **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)**. Suite **71/TFM**. Also gated P4's `ConvertArrayBenchmark` (was never in the CI filter). **Security review surfaced a pre-existing leak → new item S1.**
+- **Perf track P0–P5 = COMPLETE + merged.** `master` @ `498fc81`.
+- **S1 shipped** (branch `security/s1-redact-exception-value`, PR open) — **full fix (Rec 1)**: conversion-failure exceptions no longer carry the bound value **or** chain the value-bearing framework inner (only the failure's type name is surfaced); the value-free "required value missing" case split into its own `SettingsPropertyNullException` so its useful message survives. Three-specialist plan review (security ENDORSE-WITH-CHANGES; architect/perf in-context — kit agents misfired) + `/code-review` (clean, 0 antipatterns). Suite **76 net10** (was 71; +5 redaction tests). Detail in §S1 below.
+- **In flight:** S1 PR open (branch `security/s1-redact-exception-value`) — carries this fix-plan + handoff refresh.
+- **Next:** engine tests (T4 `ValuesPopulator` / T5 `TypeConverter` / T7 generator concurrency) **or** batch the breaking cleanups (C2 public exception base / A5 / C1 / A6 / A3 / A4) while still pre-stable · optional P3b (tiered/lazy compiled setter, only if set *time* shows in a profile).
 - **C3 — DECIDED (option 2):** cache in the provider only; Core `SettingsBuilder.GetSettings` unchanged; no reload. See #17.
 - **Held — do NOT delete (feature work coming):** D1 Validations (reconcile with the `validate-settings` branch) · D2 EqualityCompererCreator.
 - Running status lives in `SESSION-HANDOFF.md`.
@@ -60,8 +63,8 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform
 - [x] P3 · Cached “settings plan” — hoist section (lazy) + keys, precompute/cache converters, plan per type. Reflective `SetValue` kept; compiled setter deferred (regressed the gated cold scan for no warm gain) · Sev High · Eff L
 - [x] Q1–Q5 · Quick wins (GetEnumerator, OrdinalIgnoreCase, env-binder, type-cache; Q5 dead ctor checks already done by B4)
 - [x] P4 · De-reflect + DRY array/enumerable converters (shared `CollectionTypeConverter`; `Array.CreateInstance` + manual converter walk; `CreateNullResult` de-reflected) — **1.33 KB→688 B, 5.7×**; merged #25 · Sev Med · Eff M
-- [x] P5 · Resolve config section once per type — `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`) — **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)**; branch `perf/p5-resolve-section-once` · Sev Med · Eff M
-- [ ] S1 · Redact secret values from `SettingsPropertyValueException` message (`Resources.cs:34-36` interpolates the raw bound value → leaks secrets into logs); log type/length, not the value · Sev Med · Eff S · **found in P5 security review**
+- [x] P5 · Resolve config section once per type — `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`) — **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)**; merged #26 · Sev Med · Eff M
+- [x] S1 · Redact secret values from exception messages — **full fix (Rec 1)**: redacted message + value-bearing inner no longer chained + null-not-allowed split into `SettingsPropertyNullException` · Sev Med · Eff S · **found in P5 security review** *(detail below)*
 
 **Phase 6 — Architecture strategy**
 - [ ] A1 · Decide AOT/trim story; annotate `[RequiresDynamicCode]`/`[RequiresUnreferencedCode]` and/or plan a source generator · Sev High · Eff M–L
@@ -226,10 +229,20 @@ The populate loop (`src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:36-55
 Was: `TypeConverter.cs` (empty enumerable via `Enumerable.Empty` `MakeGenericMethod().Invoke()`), `ArrayTypeConverter` (`Activator.CreateInstance(List<>)` + reflected `Enumerable.ToArray` `Invoke`), `EnumerableTypeConverter` (`Activator.CreateInstance(List<>)`), both selecting the element converter with LINQ `First` (boxes the `LinkedList` enumerator + a closure).
 Now: a shared `CollectionTypeConverter` base implements `Convert` once — normalize the value to an array (split delimited string / passthrough / wrap scalar), select the element converter by walking the concrete `LinkedList` (struct enumerator, no boxing/closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` are now thin subclasses differing only in `CanConvert` + element-type extraction; both return `T[]` (safe — `IsEnumerable()` matches only `IEnumerable<T>`, which a `T[]` satisfies). `CreateNullResult` uses `Array.CreateInstance(t,0)` instead of the `Enumerable.Empty<T>()` reflection. **Proof (`ConvertArrayBenchmark`, gated): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×).** 12 parity tests in `Conversion/CollectionConversionTests.cs` (int/string/enum/DateTime/Uri elements, empty-entry removal, custom delimiter, default passthrough, unbound→empty `T[]`, `T[]`-not-`List` guard, bad-element negative). Residual 688 B = irreducible split-substrings + element boxing + result array (shared by old & new). **Code-reviewed clean** (dotnet code-reviewer verified all parity claims; the enum/DateTime/Uri + null-path + negative tests were its suggestions — partially closes T6).
 
-### P5 · Resolve config section once per type  — Sev Med · Eff M · **DONE (branch `perf/p5-resolve-section-once`)**
+### P5 · Resolve config section once per type  — Sev Med · Eff M · **DONE (merged #26)**
 Was: `ConfigurationBinder.BindPropertySettings` called `_configuration.GetSection(...)` per property (a fresh `ConfigurationSection` alloc each time) + a `$"{RootSection}:{Section}"` interpolation per property when a root is set — though the section is constant per type.
 Now: the binder caches the resolved `IConfigurationSection` per section name in a `private readonly ConcurrentDictionary<string, IConfigurationSection>`, via a **zero-capture** `GetOrAdd(context.Section, static (name, self) => self.ResolveSection(name), this)` (a capturing lambda would allocate a 64 B delegate per call — measured). Kept the internal-cache approach (Option 2), **not** a contract change: the three-specialist plan review found threading `IConfigurationSection` through the Core `ISectionBinder`/`BindingContext` contract to be a layering violation (Core must not reference `Microsoft.Extensions.Configuration`), and the optimization is single-implementer (env/cmdline/in-memory binders are flat lookups). Reload-safe: `GetSection` returns a live view, so the cached section re-reads providers on each access (locked by a test). Dropped the dead `?.` (`GetSection` never returns null); stripped a stray BOM. **Proof (`ConfigBinderBenchmark`, gated): BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)** — matched the perf review's predicted −40/−88 B deltas. 3 parity/live-view tests; also wired P4's `ConvertArrayBenchmark` into the CI filter (it was never gated). Reviewed clean (`/code-review`, after the `dotnet-claude-kit:code-reviewer` agent misfired 3×).
 
+### S1 · Redact secret values from exception messages  — Sev Med · Eff S · found in P5 security review
+`Resources.PropertySetterExceptionMessage` (`Resources.cs:34-36`) interpolates the raw bound value: `failed to to set the value [{value}] within the property [{property.Name}] for interface [{interfaceType.Name}]`. It backs `SettingsPropertyValueException`, thrown at **`ValuesPopulator.cs:124`** (per-populate convert failure — carries the real value) and `:103` (plan-build failure — passes `null`, so no leak there). A secret-bearing value that fails conversion lands in `Exception.Message` → logs. **Bigger than the one-liner:** the failing converter's **inner exception** *also* embeds the value — `Convert.ChangeType`/`Enum.Parse`/`DateTime.ParseExact`/`new Uri` all put the raw input in their own messages, and it's chained in (`: base(msg, exception)`) so loggers print it. **Realistic trigger:** a secret on a *typed* property (e.g. a credentialed URL failing `Uri` parse, or a secret mis-bound to `int`/`enum`); secret→`string` doesn't fail, so it won't hit. **Fix fork (decide in the plan review):** (a) redact our message only — quick, partial (inner exception still leaks); (b) also suppress/sanitize the value-bearing inner exception — full fix, costs diagnostic detail. Log type + length instead of the value. Both the exception and helper are `internal` → **non-breaking**. Fix the `failed to to set` double-word typo while here. Clean path for reference: `SettingsBindingException` (`Resources.cs:20-23`) already logs only binder + section + key, no value.
+
+**Resolved (this PR) — full fix, Rec 1.** The three-specialist plan review chose **Rec 1** (redact + drop the inner, no config knob). Rec 2 (an opt-in `SettingsOptions` flag to restore the value/inner for debugging) was **rejected** by the security review as insecure-by-configuration (a bool that re-enables secret logging gets flipped on and left on); Rec 3 (redact our message only) fails the goal because the inner still leaks. Shipped:
+- **`Resources.PropertySetterExceptionMessage`** rewritten — no `{value}`; reports property name, **target type** (`property.PropertyType.Name`), interface, and the failing converter's **exception type name** (a compile-time identifier, can't carry a secret). Typo fixed.
+- **`SettingsPropertyValueException`** — ctor drops the `object? value` param; **does not chain** the framework inner (its message embeds the raw value on modern .NET: `FormatException`/`ArgumentException` for int/enum/DateTime). Uniform invariant: *this type never carries a value and never chains an inner* → auditably leak-proof. Both `ValuesPopulator` throw sites (`:104` plan-build, `:133` convert) updated.
+- **New `SettingsPropertyNullException`** (internal) — the "AllowEmpty = false, no value" path (`PropertyConversion.cs:36`) is value-free, so it keeps its full message; `ConvertPropertyValue` rethrows it unredacted rather than folding it into the value exception (security-review required change — blanket redaction would have collapsed a useful message to `[Exception]`).
+- **`ISectionBinder`** doc note — `SettingsBindingException` chains the binder's inner (it's `public`), so custom binders must not throw value-bearing messages; built-in binders don't (all four verified).
+- **Tests (+5, suite 76 net10):** `Conversion/ExceptionRedactionTests.cs` binds a sentinel secret to `int`/`enum`/`DateTime`/`Uri`/a hostile custom converter and asserts the sentinel is absent from the whole `ex.ToString()` chain while property + target type remain; the existing null-path test updated to `SettingsPropertyNullException` + a message assertion. **Note (modern .NET):** `UriFormatException` is *generic* (no URI), so the Uri leak was via our message, not the inner — the Uri test targets our message accordingly. **Accepted tradeoff:** a non-secret misconfiguration now loses the framework message/stack (gets property + target + failure type instead) — flag in release notes.
+
 ---
 
 # Phase 6 — Architecture strategy