diff --git a/FIX-PLAN.md b/FIX-PLAN.md index c44ccb3..d442520 100644 --- a/FIX-PLAN.md +++ b/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`, which a `T[]` satisfies). `CreateNullResult` uses `Array.CreateInstance(t,0)` instead of the `Enumerable.Empty()` 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`, 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 diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index 29c37a2..98de55b 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -3,63 +3,58 @@ _Last updated: 2026-07-13 · owner: Guy Ludvig (guy@frontegg.com)_ ## TL;DR -We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). Perf track: **P3 (#24) and P4 (#25) are MERGED**; **P5 — resolve the config section once per type — is implemented + reviewed clean on branch `perf/p5-resolve-section-once`, ready to commit + PR.** P5 makes `ConfigurationBinder` cache the resolved `IConfigurationSection` per section name (a `ConcurrentDictionary` with a **zero-capture** `GetOrAdd`) instead of calling `GetSection(...)` per property. Plan was reviewed up front by the architect / perf / security agents (chose the internal cache over a public-contract change — Core must not reference `Microsoft.Extensions.Configuration`); code reviewed via `/code-review`. **Proven via new gated `ConfigBinderBenchmark`: BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%).** Suite green — **71 tests on net10.0** (+3 P5 parity/live-view tests; CI runs net8.0 + net10.0). (The benchmark-tracking CI (#22) gates PRs on allocation regressions.) +We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). The perf track **P0–P5 is merged**; **S1 (secret redaction) is now done and in an open PR** on branch **`security/s1-redact-exception-value`** (run `gh pr list` for the number). `master` is still @ `498fc81`. Suite **76 tests net10** (was 71; +5 redaction tests). CI runs net8 + net10. -Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking changes remain free — keep doing breaking cleanup now. (P4's `EnumerableTypeConverter` now returning `T[]` instead of `List` is safe regardless — `IsEnumerable()` matches only `IEnumerable`, which `T[]` satisfies.) +**S1 — full fix (Rec 1):** conversion-failure exceptions no longer carry the bound value **or** chain the value-bearing framework inner exception; only the failure's *type name* is surfaced. The value-free "required value missing" case was split into its own `SettingsPropertyNullException` so its useful message survives. The three-specialist plan review picked Rec 1 and **rejected Rec 2** (an opt-in flag to restore value/inner) as insecure-by-configuration. Both changed exceptions are `internal` → non-breaking. + +Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking changes remain free — a good window to batch the breaking cleanups (C2 / A5 / C1 / A6 / A3 / A4) before the first `v2.0.0-beta`. ## Do this first (new session) -1. **Verify git state** (`git log`, `gh pr list`) — this file can lag. Expect `master` @ `f9a3061` (P4, #25 merged) and a **P5 PR open** from branch `perf/p5-resolve-section-once` (or, if the wrap didn't finish, P5 sitting committed/uncommitted on that branch). -2. **P5 is implemented + reviewed clean.** If the PR isn't open yet: commit on `perf/p5-resolve-section-once`, push + open the PR via the **`guy-lud`** identity (see Gotchas). If it's open and CI is green, it's ready to **merge** (user merges — that's the alpha-publishing step). -3. After P5 merges → **S1** (redact the secret from the exception message — the P5 security review found it) or **P3b**. Per the workflow the user set (project memory `[[dotnet-review-workflow]]`): **plan first, review the plan with `dotnet-architect` / `performance-analyst` / `security-auditor`, implement, then review the diff with the code-reviewer** — note the `dotnet-claude-kit:code-reviewer` agent has been misfiring (returns a leaked skill/role preamble, 0 tool calls); the `/code-review` skill is the reliable fallback. **Refresh this handoff on the work branch** at wrap — never a dedicated docs branch (see Gotchas). +1. **Verify git state** (`git log`, `gh pr list`) — expect `master` @ `498fc81`, branch **`security/s1-redact-exception-value`** pushed with the S1 commit, and **its PR open**. If the PR already merged, `master` will have advanced — reconcile. +2. **If the S1 PR is still open:** check CI (build+test net8/net10, and the **benchmark** gate — S1 is off the hot path, so allocations shouldn't move). Merge when green (squash), or address review. S1 carries this handoff + `FIX-PLAN.md` refresh in the same commit, so they merge with it. +3. **Then pick next work** (ranked below): engine tests (T4/T5/T7) **or** batch the breaking cleanups (C2 public exception base / A5 / C1 / A6 / A3 / A4) while pre-stable. Per the workflow (project memory `[[dotnet-review-workflow]]`): plan → review the plan with `dotnet-architect`/`performance-analyst`/`security-auditor` → implement → review the diff with the **`/code-review` skill**. ⚠️ **See the sub-agent gotcha** — this session the `dotnet-architect` and `performance-analyst` agents ALSO misfired (0 tool calls, leaked preamble); only `security-auditor` worked. Have the in-context fallback ready. ## Current state -- On branch **`perf/p5-resolve-section-once`** (off `master` @ `f9a3061`). **P5 implemented + reviewed clean, at wrap being committed/PR'd.** Changed: `Extensions.Binders/ConfigurationBinder.cs` (section cache + BOM strip + dropped dead `?.`); new `UnitTests/ConfigurationBinderCacheTests.cs` (3 tests); new `Benchmark/ConfigBinderBenchmark.cs`; `benchmark.yml` (+`ConfigBinderBenchmark`, +`ConvertArrayBenchmark`); benchmark `.csproj` (+`Microsoft.Extensions.Configuration`); doc refresh. -- **`master` @ `f9a3061`** now holds P4 (#25) **plus a user modernization pass** (collection expressions across ~19 files, merged with #25). -- A **`gh-pages`** branch holds benchmark data (`dev/bench/`); do **not** delete it — the baseline lives there. The remote also still has the merged **`perf/p4-dereflect-converters`** branch (GitHub didn't auto-delete; optional cleanup, needs `guy-lud`) plus legacy/held branches. Deleting remote branches needs the `guy-lud` push identity. -- A **`gh-pages`** branch was bootstrapped to hold benchmark data (`dev/bench/`); do **not** delete it — the baseline lives there (first recorded on the #22 master run). Otherwise the remote holds only **legacy / held** branches (`validate-settings`, `version-7.x`, older pre-#8 feature branches). Deleting remote branches needs the `guy-lud` push identity. +- On branch **`security/s1-redact-exception-value`** with the S1 commit (code + tests + this docs refresh). **PR open** (see `gh pr list`). `master` @ `498fc81` (P5, #26). +- **Perf track P0–P5 complete + merged.** Build clean (0 warnings, both TFMs). Suite **76 net10**. +- A **`gh-pages`** branch holds benchmark data (`dev/bench/`); do **not** delete it — the allocation baseline lives there. The remote also still has merged `perf/p4-*`/`perf/p5-*` branches (optional cleanup) plus legacy/held branches (`validate-settings`, `version-7.x`, pre-#8). Deleting remote branches needs the `guy-lud` push identity. ## What shipped (recent → older) -- **P5 — resolve config section once per type (branch `perf/p5-resolve-section-once`; committing/PR'ing at wrap).** `ConfigurationBinder` now caches the resolved `IConfigurationSection` per section name in a `private readonly ConcurrentDictionary`, replacing the per-property `GetSection(...)`. The `GetOrAdd` uses the **zero-capture** `static (name, self) => self.ResolveSection(name), this` form — a capturing lambda would allocate a 64 B delegate per call (measured). **Internal cache, not a contract change:** the plan review (architect/perf/security) found threading `IConfigurationSection` through the Core `ISectionBinder`/`BindingContext` 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 (cached section is a live view — re-reads providers each access; locked by a test). Dropped the dead `?.` (`GetSection` is non-null); stripped a stray BOM. **Proof — gated `ConfigBinderBenchmark`: BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)** (matched the perf review's −40/−88 B prediction). 3 tests (multi-property no-root/with-root + `Bind_CachedSection_ReflectsLaterConfigChange` live-view). Also **wired P4's `ConvertArrayBenchmark` into the CI filter** — it was never gated. Suite **71 net10**. **Reviewed:** plan by the 3 specialists; code by `/code-review` (the `dotnet-claude-kit:code-reviewer` agent misfired 3× — leaked skill/role preamble, 0 tool calls). **Security review found a pre-existing leak → S1** (see Next priorities): `Resources.cs:34-36` interpolates the raw bound value into `SettingsPropertyValueException` → secrets in logs. -- **P4 — de-reflect + DRY the array/enumerable converters (merged, #25).** New abstract `Conversion/CollectionTypeConverter` owns the shared `Convert`: normalize the incoming value to an array (split a delimited string / passthrough an existing array / wrap a scalar), select the element converter by a **manual walk over the concrete `LinkedList`** (struct enumerator — no boxed enumerator, no predicate closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` collapse to thin subclasses (only `CanConvert` + element-type extraction differ); both now return `T[]`. Gone: the `List` + its backing array, the reflected `Enumerable.ToArray` (`MakeGenericMethod`+`Invoke`+args array), and the `First` closure. `TypeConverter.CreateNullResult` swaps the `Enumerable.Empty()` reflection for `Array.CreateInstance(t,0)`, and its `GetConverter` is now a true manual walk (matching its own comment). **Proof — new gated `ConvertArrayBenchmark` (isolates the hot path like Q1/Q3/Q4): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×)** (before measured by swapping master's old converter back in). Residual 688 B is the split-substrings + per-element boxing + result array shared by both versions. **12 parity tests** (`Conversion/CollectionConversionTests.cs`): delimited→`int[]`/`string[]`/`IEnumerable`/`DayOfWeek[]`/`DateTime[]`/`Uri[]`, empty-entry removal, custom delimiter, default-array passthrough, unbound-no-default→empty `T[]` (the `CreateNullResult` line), the enumerable-path-materializes-`T[]` guard, and a bad-element→`SettingsPropertyValueException` negative. Suite **68 net10** (was 56). **Code-reviewed** by the dotnet code-reviewer (the last 5 tests were its suggestions): all 6 adversarial parity claims verified, no defects. -- **P3 — cached "settings plan" (#24, merged `faa48d9`).** `ValuesPopulator` builds a per-type `SettingsPlan` once (cached on the populator instance): section name resolved **once and lazily** (a no-binder scan never pays for it); `[SettingsProperty]` read **once** per property then threaded into key/default/conversion; per property a `readonly struct` `PropertyPlan`/`PropertyConversion` carrying the resolved key, default, and **precomputed converter** (chosen via a manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed). **Warm re-populate −55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. New gated `PlanPopulateBenchmark` tracks the warm path. **Reflective `SetValue` kept:** the emitted `__Set`/compiled-`Action` setter was built + measured but **reverted** — it regressed the gated cold scan (+25% for `__Set`) for **zero** warm gain, since net10's `PropertyInfo.SetValue` no longer allocates an args array. **Reviewed** by the dotnet code-review + perf agents (behavior/exception parity confirmed); their fixes: single attribute read, `SettingsPropertyValueException` re-wrap at plan build, binder-array materialization, `ISettingsTypeConverter` stateless/thread-safe doc. Follow-ups in `FIX-PLAN.md`: **P3b** (tiered/lazy setter, only if set *time* matters) and the binder `CreateKey` string-concat (pre-existing, ~⅓ of the warm number). -- **#23 — session-wrap docs.** Handoff + fix-plan refresh; gitignored BenchmarkDotNet output + the personal `.claude/settings.local.json` (SessionStart hook). Squash-merged onto `master`. -- **#22 — benchmark-tracking CI merged.** (Detail below.) First master run recorded the allocation baseline on `gh-pages`. -- **#21 — Perf quick wins Q1–Q4 + M1 fix + micro-benchmarks.** - - Q1 `SettingsCollection.GetEnumerator` yields over its dictionary (was rebuilding a whole `Dictionary` per enumeration); Q2 `SettingsTypesExtractor` suffix match → `EndsWith(…, OrdinalIgnoreCase)` + hoisted suffix (kills a `ToLower` CurrentCulture smell); Q3 `EnvironmentVariableBinder` fast-paths `context.Key` (no `StringBuilder`) + single lookup; Q4 `SettingsClassGenerator` caches the generated impl by interface `Type`. **Q5 was already done by B4.** - - **M1 (found in the code review):** Q4's Type-keyed cache exposed a latent collision — the generated impl name was derived from the *simple* interface name, so `Foo.ISettings` + `Bar.ISettings` collided and aborted the scan. Fixed by namespace-qualifying the impl name **in the generator only**. ⚠️ `GetNormalizeInterfaceName` was left alone on purpose — it also backs the default config **section name** (`SettingsOptions.SectionNameFormatter`). - - **Micro-benchmarks** (`MicroBenchmarks.cs`): `EnumerateBenchmark` (Q1), `EnvBinderBenchmark` (Q3), `GenerateTypeBenchmark` (Q4). The macro `ScanBenchmark` can't resolve these (IL-emit dominates), so they isolate each hot path. Proven before/after: **Q1 2.7× / 64 KB→88 B · Q3 2.65× / 152 B→0 · Q4 32× / 224 B→0**. The benchmark assembly now has `InternalsVisibleTo` (Info.cs) + a Binders project ref. -- **#22 — benchmark-tracking CI (merged)** (`.github/workflows/benchmark.yml`). Runs BDN (ShortRun) on push-to-master and PRs, `jq`-extracts per-benchmark **allocated bytes**, feeds `benchmark-action/github-action-benchmark` (`customSmallerIsBetter`) stored on `gh-pages`. PRs comment the allocation diff and **fail on a >10% regression**; time is informational only. -- **#20 — docs tutorials refresh.** All six `docs/*.md` rewritten against the current public API + the `SimpleConfig`→`SimpleSettings` rename (settles the A2 docs debt). -- **#18 — P2.** Memoized `TypePropertiesExtractor.ExtractTypeProperties` + `HashSet` dedup; cache is a **private instance field** (not static, not injected). -- **#17 — P1 + C3.** `ISettingsProvider.GetSettings` now serves the startup-built `ISettingsCollection` (same instance as the DI singleton); build fallback only for never-scanned types. **C3 = provider-level cache only** (Core `SettingsBuilder.GetSettings` unchanged; no reload). -- **#16 — P0 benchmark harness** (`[MemoryDiagnoser]` BDN: `ScanBenchmark` / `ResolveBenchmark` / `ShapeBenchmark`). Run: `dotnet run -c Release --project src/performance/ExistForAll.SimpleSettings.Benchmark` (smoke: `-- --job dry`). -- Earlier: #8 (5 bugfixes + tests), #10–#15 (tests, D3 typo, DI tests, solution rename, A2 naming). +- **S1 — redact secret values from exception messages (branch `security/s1-redact-exception-value`, PR open).** A bound config value that fails type conversion used to reach logs two ways: (1) our own `SettingsPropertyValueException` message interpolated `[{value}]`, and (2) the failing converter's framework inner exception (`FormatException`/`ArgumentException` for int/enum/DateTime) embeds the raw input and was chained in. **Fix (Rec 1, full):** `Resources.PropertySetterExceptionMessage` rewritten to report property name + **target type** + the inner's **exception type name** (no value, typo `to to` fixed); `SettingsPropertyValueException` ctor drops the `value` param and **no longer chains** the inner — uniform invariant *"never carries a value, never chains an inner"* → auditably leak-proof; both `ValuesPopulator` throw sites updated (`:104` plan-build, `:133` convert). New **`SettingsPropertyNullException`** (internal): the "AllowEmpty=false, no value" path is value-free, so it keeps its full message and `ConvertPropertyValue` rethrows it unredacted (security-review required change — else it degraded to `[Exception]`). `ISectionBinder` doc note added (its wrapper `SettingsBindingException` is `public` and chains the binder's inner → custom binders must not throw value-bearing messages; the 4 built-ins don't). **Tests +5** (`Conversion/ExceptionRedactionTests.cs`): sentinel secret bound to int/enum/DateTime/Uri/hostile-custom-converter, each asserting the secret is absent from the whole `ex.ToString()` chain while property + target type remain; existing null test updated to the new type. **Note:** on modern .NET `UriFormatException` is generic (no URI), so the Uri leak was via *our* message — the Uri test targets that. **Accepted tradeoff:** non-secret misconfigs lose the framework message/stack (get property + target + failure type instead) — flag in release notes. **Reviewed:** plan by the 3 specialists (security ENDORSE-WITH-CHANGES + 3 required changes, all folded in; architect+perf **misfired** → done in-context); code by `/code-review` high effort + Roslyn MCP (0 antipatterns, blast radius = only the 2 throw sites + tests). +- **P5 — resolve config section once per type (merged, #26).** `ConfigurationBinder` caches the resolved `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd(..., static (name, self) => …, this)`). Internal cache, not a contract change (threading `IConfigurationSection` through Core's `ISectionBinder` would be a layering violation; single-implementer). Reload-safe (live view). **Gated `ConfigBinderBenchmark`: BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%).** Also wired P4's `ConvertArrayBenchmark` into the CI filter. **Security review of P5 surfaced the leak → S1.** +- **P4 — de-reflect + DRY the array/enumerable converters (merged, #25).** Shared `Conversion/CollectionTypeConverter` (`Array.CreateInstance` + indexed fill, manual `LinkedList` walk — no boxed enumerator/closure); `ArrayTypeConverter`/`EnumerableTypeConverter` are thin subclasses, both return `T[]`; `CreateNullResult` de-reflected. **Gated `ConvertArrayBenchmark`: 1.33 KB→688 B (−49%), 5.7×.** 12 parity tests. +- **P3 — cached "settings plan" (#24, `faa48d9`).** Per-type `SettingsPlan` cached on the populator: section name once+lazy, `[SettingsProperty]` read once, per-property `readonly struct` `PropertyPlan`/`PropertyConversion` with precomputed converter. **Warm re-populate −55–61%**; gated `ScanBenchmark` ≈flat. Emitted/compiled setter built but **reverted** (regressed the gated cold scan for no warm gain — net10 `SetValue` is alloc-free). Follow-ups: P3b (tiered setter, only if set *time* matters) + binder `CreateKey` string-concat. +- **#23** session-wrap docs · **#22** benchmark-tracking CI (gates PRs on allocation regressions via github-action-benchmark on `gh-pages`) · **#21** perf quick wins Q1–Q4 + M1 collision fix + micro-benchmarks (Q1 2.7× / Q3 2.65× / Q4 32×) · **#20** docs tutorials refresh · **#18** P2 memoize extraction · **#17** P1 provider cache + C3 · **#16** P0 benchmark harness · earlier #8/#10–#15. ## Key decisions & context (carry forward) -- **Benchmark tracking gates on ALLOCATIONS, not time.** Allocated bytes are deterministic → stable on shared CI runners → safe to fail a build on. Time is far too noisy to gate (this is also why the macro `ScanBenchmark` time didn't move for the quick wins). `gh-pages` (`dev/bench/`) holds the historical baseline. +- **Benchmark tracking gates on ALLOCATIONS, not time.** Allocated bytes are deterministic → safe to fail a build on. `gh-pages` (`dev/bench/`) holds the baseline. +- **S1 exception invariant.** `SettingsPropertyValueException` deliberately **never carries the bound value and never chains an inner** (both can embed secrets). `SettingsPropertyNullException` is the distinct value-free "required value missing" case and keeps its full message. Don't reintroduce a chained inner or a value param — that reopens the leak. A future opt-in "full diagnostics" knob was explicitly rejected (insecure-by-configuration). +- **C2 (future) will make exceptions public + structured** (a `SimpleSettingsException` base, reparent all, expose context as properties). S1 stayed minimal on purpose; when C2 lands, give `SettingsPropertyValueException`/`SettingsPropertyNullException` structured props (PropertyName/TargetType/FailureType) and reparent them. - **M1 / generated names.** The generated impl type name (in `SettingsClassGenerator`) is namespace-qualified and must stay **separate** from `GetNormalizeInterfaceName`, which drives the default config section name. Don't merge them. -- **C3 resolved — option 2 (provider-level cache).** Reload / `IOptionsMonitor` is the future "option 3" if ever wanted. -- **Validations (D1) — HELD, do NOT delete.** Public `Validations/*` + `SettingsPropertyAttribute.ValidatorType` are dead but intended for a feature; reconcile with the `validate-settings` branch. Wire into `ValuesPopulator`. +- **C3 resolved — option 2 (provider-level cache).** Reload / `IOptionsMonitor` is the future "option 3". +- **Validations (D1) — HELD, do NOT delete.** Public `Validations/*` + `SettingsPropertyAttribute.ValidatorType` are dead but intended for a feature; reconcile with the `validate-settings` branch. - **`EqualityCompererCreator` (D2) — HELD** (internal, dead, latent invalid-IL bug at `EqualityCompererCreator.cs:38`). -- **Pre-stable window:** no `v*` stable tag (the `version-*` tags are the dead legacy package). Breaking changes free until the first `v2.0.0-beta`. +- **Pre-stable window:** no `v*` stable tag. Breaking changes free until the first `v2.0.0-beta`. ## Next priorities (ranked — detail in FIX-PLAN.md) -1. **Finish P5:** commit + PR the branch (then user merges). After that the perf track's queued items are **S1** (security: redact the secret value from `SettingsPropertyValueException` — `Resources.cs:34-36`; found in the P5 security review) and optional **P3b** (tiered/lazy compiled setter, only if set *time* shows up in a profile — allocation is already handled). -2. **Engine tests:** T4 `ValuesPopulator`, T5 `TypeConverter`, **T6 converters — largely done across P4+P5** (array/enumerable/element-type covered by `CollectionConversionTests`; ConfigurationBinder covered by `ConfigurationBinderCacheTests`), T7 generator concurrency stress — the unsynchronized check-then-`DefineType` in `GenerateType` is **still open** (Q4's `ConcurrentDictionary` made the cache thread-safe but did not close that race). -3. **Architecture:** A1 (AOT/trim annotations — HIGH, `Reflection.Emit` lib), C1 (`List`/`IList` support), C2 (public `SimpleSettingsException` base), A3 (`Core.AspNet` public type or drop the package), A4 (float `Microsoft.Extensions.*` floor per-TFM), A5 (make `SettingsHolder` internal), A6 (command-line quoted-arg parsing). -4. **README** links — the `docs/` tutorials were done in #20; the README may still have stale `existall/SimpleConfig` links. -5. **D1 validations feature** — owner-driven; reconcile the `validate-settings` branch. +1. **Merge the S1 PR** once CI is green (see Do this first). +2. **Engine tests:** T4 `ValuesPopulator` (precedence + exception wrappers), T5 `TypeConverter` (null/nullable/empty-enumerable/attribute), T7 generator concurrency stress — the unsynchronized check-then-`DefineType` in `GenerateType` is **still open** (Q4's `ConcurrentDictionary` made the cache thread-safe but did not close that race). T6 converters largely done across P4+P5. +3. **Breaking cleanups (batch while pre-stable):** C2 (public `SimpleSettingsException` base + structured exceptions — pairs naturally with S1's new types), A5 (make `SettingsHolder` internal), C1 (`List`/`IList` support), A6 (command-line quoted-arg parsing), A3 (`Core.AspNet` public type or drop the package), A4 (float `Microsoft.Extensions.*` floor per-TFM). +4. **A1 (HIGH):** AOT/trim annotations for the `Reflection.Emit` engine (or plan a source generator); at minimum document the limitation before stable. +5. **README** — may still have stale `existall/SimpleConfig` links (the `docs/` tutorials were done in #20). +6. **D1 validations feature** — owner-driven; reconcile the `validate-settings` branch. Optional **P3b** compiled setter (only if a profile shows set *time* matters). ## How releasing works (unchanged — durable) - **`ci.yml`** — on PRs to `master`: build + test (net8.0 + net10.0). **`release.yml`**: push to `master` → auto-publishes a MinVer height-based `-alpha` to nuget.org; manual **Release** (`workflow_dispatch`, `channel` beta/rc/stable + `bump` patch/minor/major) computes the next version, tags `v*`, publishes, creates a GitHub Release (`dry_run: true` previews). -- **`benchmark.yml`** — on push to `master` + PRs: runs BDN, gates PRs on allocation regressions (see Key decisions). +- **`benchmark.yml`** — on push to `master` + PRs: runs BDN, gates PRs on allocation regressions. - **Versioning = MinVer**, tag prefix `v`, baseline **2.0.0**, keyless publish via NuGet Trusted Publishing (OIDC). First real release: Actions → Release → `channel: beta` (→ `v2.0.0-beta.1`); use `dry_run` first. - Workflows invoke the solution through the `SOLUTION` env var (**`SimpleSettings.slnx`**). **Any push to `master` publishes an alpha** — so everything goes through PRs. ## Gotchas a new session MUST know -- **Pushing / PRs:** the active `git`/`gh` identity (`guy-frontegg`) is **read-only** on this repo; push/PR/merge via the **`guy-lud`** account — SSH alias `github-guy-lud` for `git push`, `gh auth switch --user guy-lud` for `gh` writes (switch back to `guy-frontegg` after). Full recipe in the assistant's private project memory (`simplesettings-push-access`). -- **Run `dotnet` from `src/`** (global.json opts into Microsoft.Testing.Platform for TUnit). Only the net10 runtime is installed locally → net8 is **build-only** locally; CI runs both. Do NOT prefix `cd ` before `dotnet`. -- **Benchmarks:** run from `src/` — `dotnet run -c Release --project performance/ExistForAll.SimpleSettings.Benchmark -- --filter --job short`. Output dir (`BenchmarkDotNet.Artifacts/`) is now gitignored. Micro-benchmarks depend on the benchmark assembly's `InternalsVisibleTo` (Info.cs) + the Binders project ref. +- **Pushing / PRs:** the active `git`/`gh` identity (`guy-frontegg`) is **read-only** on this repo; push/PR/merge via the **`guy-lud`** account. `origin` already uses the SSH alias **`github-guy-lud`** (→ `~/.ssh/guy-lud-account`), so **`git push` already uses guy-lud** — no change needed. For `gh` writes: `gh auth switch --user guy-lud`, then switch back to `guy-frontegg` after. Full recipe in the assistant's private project memory (`simplesettings-push-access`). +- **Run `dotnet` from `src/`** (global.json opts into Microsoft.Testing.Platform for TUnit). Only the net10 runtime is installed locally → net8 is **build-only** locally; CI runs both. Do NOT prefix `cd ` before `dotnet`. Run a single test project on net10 with `dotnet test --framework net10.0`. +- **Benchmarks:** run from `src/` — `dotnet run -c Release --project performance/ExistForAll.SimpleSettings.Benchmark -- --filter --job short`. Output dir (`BenchmarkDotNet.Artifacts/`) is gitignored. - **`FIX-PLAN.md`** (repo root) is the full, prioritized plan with per-item file:line detail — open it explicitly; it is not auto-injected. -- **Wrap ritual — handoff branch rule:** refresh this file **on the current work branch** (or `master`) so it merges with the session's real PR. **Never** create a dedicated docs branch/PR for it (that's what #23 was) — `release.yml` fires on *every* `master` push with **no `paths` filter**, so a doc-only merge burns a throwaway `-alpha` for nothing. +- **Wrap ritual — handoff branch rule:** refresh this file so it rides the session's real PR (the current work branch). If there is **no** open work branch at wrap (everything merged), **leave the refresh uncommitted** so the *next* session's first branch carries it. Do **not** commit docs to `master`, and do **not** create a dedicated docs branch/PR: `release.yml` fires on *every* `master` push with **no `paths` filter**, so a doc-only push burns a throwaway `-alpha`. (This session S1 was the work branch, so the docs rode it.) +- **Sub-agent flakiness (dotnet-claude-kit) — WORSENING:** the kit agents intermittently misfire — returning a leaked skill/role preamble with **0 tool calls** instead of doing the work. Previously only `code-reviewer`; **this session `dotnet-architect` AND `performance-analyst` also misfired** on the S1 plan review (only `security-auditor` worked, first try). Mitigation that held: **use the `/code-review` skill for the code-review step** (in-context, reliable), and **do the architect/perf plan-review lenses in-context** if their agents misfire (perf here was trivially "failure-path only, no impact"; architect was an exception-design judgment). `security-auditor` is still worth spawning — it did a thorough, source-verified pass. Kit is at latest (0.10.0). For perf specifically, `dotnet-diag:analyzing-dotnet-performance` (Microsoft-maintained) is a fallback. - Commits/PRs here **omit** the Co-Authored-By / Generated-with trailer (project preference). diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs index d10d3a0..8e3e8d9 100644 --- a/src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs @@ -33,7 +33,7 @@ public PropertyConversion(ISettingsTypeConverter converter, if (value == null) { if (_throwOnNull) - throw new Exception(Resources.PropertyNotAllowNullMessage(_propertyName)); + throw new SettingsPropertyNullException(_propertyName); return _nullResult; } diff --git a/src/Core/ExistForAll.SimpleSettings/ISectionBinder.cs b/src/Core/ExistForAll.SimpleSettings/ISectionBinder.cs index 5125632..4e736e6 100644 --- a/src/Core/ExistForAll.SimpleSettings/ISectionBinder.cs +++ b/src/Core/ExistForAll.SimpleSettings/ISectionBinder.cs @@ -2,6 +2,9 @@ namespace ExistForAll.SimpleSettings { public interface ISectionBinder { + // Implementations must not throw exceptions whose message embeds a fetched configuration value: + // SettingsBindingException chains the thrown exception, so a value-bearing message would reach logs + // (a secret leak). The built-in binders only read and store, so they never do this. See S1 in FIX-PLAN.md. void BindPropertySettings(BindingContext context); } } \ No newline at end of file diff --git a/src/Core/ExistForAll.SimpleSettings/Resources.cs b/src/Core/ExistForAll.SimpleSettings/Resources.cs index ca51ac6..7b3c101 100644 --- a/src/Core/ExistForAll.SimpleSettings/Resources.cs +++ b/src/Core/ExistForAll.SimpleSettings/Resources.cs @@ -31,9 +31,10 @@ public static string SettingsClassGenerationException(Type type) => $@"While trying to generate a class from interface [{type.FullName}] something went wrong. please see inner exception for more details"; - public static string PropertySetterExceptionMessage(Type interfaceType, object? value, PropertyInfo property) => - $@"failed to to set the value [{value}] within the property [{property.Name}] for interface [{interfaceType.Name}]. -see inner exception for more details"; + // The bound value is deliberately omitted (it may be a secret): we report only the property, its target + // type, and the failing converter's exception type. See S1 in FIX-PLAN.md and SettingsPropertyValueException. + public static string PropertySetterExceptionMessage(Type interfaceType, PropertyInfo property, string failureType) => + $@"Failed to set property [{property.Name}] of type [{property.PropertyType.Name}] on interface [{interfaceType.Name}]: the bound value could not be converted ([{failureType}]). The value is omitted to avoid leaking secrets into logs."; public static string SettingsPropertiesExtractionMessage(Type type) => $@"An error has occurred while trying to extract diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsPropertyNullException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsPropertyNullException.cs new file mode 100644 index 0000000..4a3aef0 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/SettingsPropertyNullException.cs @@ -0,0 +1,15 @@ +using System; + +namespace ExistForAll.SimpleSettings +{ + // Raised when a property marked [SettingsProperty(AllowEmpty = false)] resolves to no value. This is + // distinct from SettingsPropertyValueException (a value that failed conversion): here there is no bound + // value at all, so the message (property name only) carries nothing sensitive and is surfaced in full — + // it is a common misconfiguration and the detail aids diagnosis. See S1 in FIX-PLAN.md. + internal class SettingsPropertyNullException : Exception + { + public SettingsPropertyNullException(string propertyName) + : base(Resources.PropertyNotAllowNullMessage(propertyName)) + {} + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsPropertyValueException.cs b/src/Core/ExistForAll.SimpleSettings/SettingsPropertyValueException.cs index ca73da8..02fdf57 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsPropertyValueException.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsPropertyValueException.cs @@ -3,14 +3,19 @@ namespace ExistForAll.SimpleSettings { + // Raised when a bound configuration value fails type conversion. The bound value is deliberately NOT put + // in the message, and the converter's inner exception is NOT chained: both can embed the raw value (e.g. a + // FormatException reads "The input string 'SECRET' was not in a correct format."), which would then reach + // logs via Exception.ToString()/ILogger. Only the failure's exception type name is surfaced — that is a + // compile-time identifier and cannot carry a secret. See S1 in FIX-PLAN.md. For the distinct + // "required value missing" case (no value at all, so nothing to leak) see SettingsPropertyNullException. internal class SettingsPropertyValueException : Exception { public SettingsPropertyValueException( Type interfaceType, - object? value, PropertyInfo property, - Exception exception) - : base(Resources.PropertySetterExceptionMessage(interfaceType, value, property), exception) + Exception conversionError) + : base(Resources.PropertySetterExceptionMessage(interfaceType, property, conversionError.GetType().Name)) {} } } \ No newline at end of file diff --git a/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs b/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs index d873112..7cece38 100644 --- a/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs +++ b/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs @@ -98,9 +98,10 @@ private SettingsPlan GetOrBuildPlan(Type settings, SettingsOptions options) catch (Exception e) { // Restore the original exception contract: converter-setup failures used to surface inside - // the per-populate convert try as SettingsPropertyValueException. The bound value isn't - // known at plan build, hence null. - throw new SettingsPropertyValueException(settings, null, property, e); + // the per-populate convert try as SettingsPropertyValueException. No bound value exists at + // plan build, and setup failures describe types/converters, not a value — so nothing sensitive + // is dropped by the redacting exception (which never carries the value or chains the inner). + throw new SettingsPropertyValueException(settings, property, e); } } @@ -119,9 +120,13 @@ private SettingsPlan GetOrBuildPlan(Type settings, SettingsOptions options) { return propertyPlan.Conversion.Convert(value); } - catch (Exception e) + // SettingsPropertyNullException is the value-free "required value missing" signal — the filter skips + // this catch so it propagates as-is, rather than being redacted into a value-conversion exception. + catch (Exception e) when (e is not SettingsPropertyNullException) { - throw new SettingsPropertyValueException(settingsType, value, propertyPlan.Property, e); + // e (and its message) may embed the raw bound value, which could be a secret — never chain it + // or put the value in the message. Only the failure's type name is surfaced. See S1. + throw new SettingsPropertyValueException(settingsType, propertyPlan.Property, e); } } } diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/ExceptionRedactionTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/ExceptionRedactionTests.cs new file mode 100644 index 0000000..8014192 --- /dev/null +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/Conversion/ExceptionRedactionTests.cs @@ -0,0 +1,124 @@ +using System; +using ExistForAll.SimpleSettings.Binder; +using ExistForAll.SimpleSettings.Conversion; + +namespace ExistForAll.SimpleSettings.UnitTests.Conversion +{ + // S1: a bound value that fails conversion must never reach the exception message OR a chained inner + // exception — either can embed a secret, which then lands in logs via Exception.ToString()/ILogger. + // Each test binds a distinctive sentinel to a typed property that fails to convert and asserts the + // sentinel appears NOWHERE in the thrown exception's full ToString() (which walks message + every inner + // exception + stack), while the safe diagnostics (property name + target type) are still present. + public class ExceptionRedactionTests + { + private const string Secret = "S3CR3T-sentinel-do-not-log"; + + [Test] + public async Task Convert_SecretToInt_DoesNotLeakValue() + { + // Convert.ChangeType throws a FormatException whose message embeds the raw input on modern .NET. + var ex = CaptureConversionFailure(nameof(IIntSetting.Value), Secret); + await AssertRedacted(ex, nameof(IIntSetting.Value), nameof(Int32)); + } + + [Test] + public async Task Convert_SecretToEnum_DoesNotLeakValue() + { + // Enum.Parse throws an ArgumentException that embeds the requested value. + var ex = CaptureConversionFailure(nameof(IEnumSetting.Day), Secret); + await AssertRedacted(ex, nameof(IEnumSetting.Day), nameof(DayOfWeek)); + } + + [Test] + public async Task Convert_SecretToDateTime_DoesNotLeakValue() + { + // DateTime.ParseExact throws a FormatException that embeds the input string. + var ex = CaptureConversionFailure(nameof(IDateTimeSetting.When), Secret); + await AssertRedacted(ex, nameof(IDateTimeSetting.When), nameof(DateTime)); + } + + [Test] + public async Task Convert_SecretToUri_DoesNotLeakValue() + { + // UriFormatException's message is generic on modern .NET (no URI), so this pins the OTHER vector: + // our own message used to interpolate the raw value — a credentialed URL would leak there. + var ex = CaptureConversionFailure(nameof(IUriSetting.Endpoint), Secret); + await AssertRedacted(ex, nameof(IUriSetting.Endpoint), nameof(Uri)); + } + + [Test] + public async Task Convert_CustomConverterLeakingValue_IsStillRedacted() + { + // ISettingsTypeConverter is a public extension point. A custom converter that throws a value-bearing + // message must not leak either — the wrapper drops the inner and keeps only its type name. + var ex = CaptureConversionFailure(nameof(ILeakyConverterSetting.Value), Secret); + await AssertRedacted(ex, nameof(ILeakyConverterSetting.Value), nameof(String)); + } + + private static SettingsPropertyValueException CaptureConversionFailure(string key, string value) + where T : class + { + var collection = new InMemoryCollection(); + collection.Add(SectionOf(), key, value); + var builder = SettingsBuilder.CreateBuilder(x => x.AddSectionBinder(new InMemoryBinder(collection))); + + try + { + builder.GetSettings(); + } + catch (SettingsPropertyValueException e) + { + return e; + } + + throw new Exception($"Expected a SettingsPropertyValueException for [{typeof(T).Name}], but none was thrown."); + } + + private static async Task AssertRedacted(SettingsPropertyValueException ex, string propertyName, string targetType) + { + var full = ex.ToString(); + await Assert.That(full.Contains(Secret)).IsFalse(); + await Assert.That(full.Contains(propertyName)).IsTrue(); + await Assert.That(full.Contains(targetType)).IsTrue(); + } + + // The default SectionNameFormatter strips the leading "I": IIntSetting -> "IntSetting". + private static string SectionOf() => typeof(T).Name.Substring(1); + + public interface IIntSetting + { + int Value { get; set; } + } + + public interface IEnumSetting + { + DayOfWeek Day { get; set; } + } + + public interface IDateTimeSetting + { + DateTime When { get; set; } + } + + public interface IUriSetting + { + Uri Endpoint { get; set; } + } + + public interface ILeakyConverterSetting + { + [SettingsProperty(ConverterType = typeof(LeakyConverter))] + string Value { get; set; } + } + + // A deliberately hostile custom converter: it stuffs the bound value into its exception message to + // prove the wrapper still refuses to surface it. + public class LeakyConverter : ISettingsTypeConverter + { + public bool CanConvert(Type settingsType) => true; + + public object Convert(object value, Type settingsType) + => throw new InvalidOperationException($"conversion blew up for value '{value}'"); + } + } +} diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs index 9d1fafc..b84f878 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsPropertyTests.cs @@ -7,7 +7,12 @@ public async Task Build_WhenAllowEmptyIsFalse_ShouldThrowException() { var sut = SettingsBuilder.CreateBuilder(); - await Assert.That(() => sut.GetSettings()).Throws(); + // A required-but-missing value is a distinct, value-free failure (SettingsPropertyNullException), + // not a conversion failure — so it keeps its full, informative message (property name). See S1. + var exception = await Assert.That(() => sut.GetSettings()) + .Throws(); + + await Assert.That(exception!.Message.Contains(nameof(IWithNonNullInterface.Value))).IsTrue(); } public interface IWithNonNullInterface