From 477245f5db3c29398bed82c56e99de90a2a250d6 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 9 Nov 2026 14:33:14 -0500 Subject: [PATCH] docs(code-reviews): review Secrets, LocalDb, GalaxyRepository, Overview; re-review changed libs (scadaproj#15) First reviews of Secrets, LocalDb, GalaxyRepository and Overview, and 2026-11-09 re-reviews of Auth, Health, Theme, Telemetry, Audit and Configuration, all at 91fb2b4. 49 new open findings, each with a P1/P2/P3 priority and a failure scenario: 1 P1, 13 P2, 35 P3. README regenerated; REVIEW-PROCESS, template and regen script now cover all ten libraries and the Priority field. Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV --- code-reviews/Audit/findings.md | 44 +- code-reviews/Auth/findings.md | 227 +++++++- code-reviews/Configuration/findings.md | 44 +- code-reviews/GalaxyRepository/findings.md | 409 +++++++++++++ code-reviews/Health/findings.md | 97 +++- code-reviews/LocalDb/findings.md | 399 +++++++++++++ code-reviews/Overview/findings.md | 301 ++++++++++ code-reviews/README.md | 95 ++- code-reviews/REVIEW-PROCESS.md | 10 +- code-reviews/Secrets/findings.md | 671 ++++++++++++++++++++++ code-reviews/Telemetry/findings.md | 183 +++++- code-reviews/Theme/findings.md | 161 +++++- code-reviews/_template/findings.md | 8 +- code-reviews/regen-readme.py | 5 +- 14 files changed, 2602 insertions(+), 52 deletions(-) create mode 100644 code-reviews/GalaxyRepository/findings.md create mode 100644 code-reviews/LocalDb/findings.md create mode 100644 code-reviews/Overview/findings.md create mode 100644 code-reviews/Secrets/findings.md diff --git a/code-reviews/Audit/findings.md b/code-reviews/Audit/findings.md index bb91f23..70bc0dd 100644 --- a/code-reviews/Audit/findings.md +++ b/code-reviews/Audit/findings.md @@ -7,14 +7,31 @@ | Component spec | `components/audit/spec/SPEC.md` | | Shared contract | `components/audit/shared-contract/ZB.MOM.WW.Audit.md` | | Status | Reviewed | -| Last reviewed | 2026-06-01 | -| Reviewer | Claude (automated baseline) | -| Commit reviewed | `5f75cd4` | +| Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) | +| Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) | +| Commit reviewed | `91fb2b4` | | Open findings | 0 | ## Summary -`ZB.MOM.WW.Audit` is a small, clean, well-scoped library: one canonical `AuditEvent` +**2026-11-09 re-review (`91fb2b4`, scadaproj#15).** Re-reviewed at `91fb2b4` (2026-11-09) against the 2026-06-01 baseline `5f75cd4`. The only source changes +since the baseline are the five baseline fixes from `544a6dd` and the repository-URL correction from +`01c6dc1`; the version is still `0.1.0`. The fixes are: + +- `CompositeAuditWriter` now swallows `OperationCanceledException` along with every other writer + fault, so the "must not throw" contract holds. +- `TruncatingAuditRedactor` over-redacts both `DetailsJson` and `Target` on internal failure, and + clamps a negative cap to 0. +- `TruncatingAuditRedactorOptions` is an immutable `record` with `init` accessors. A reflection check + of the locally cached `0.1.0` nupkg (2026-06-02) shows this shape, so the published package already + carries it. +- XML docs are generated. +- Edge-case tests were added. + +All five fixes are correct. `AddZbAudit` (`TryAdd` defaults) has a container-building test +(`AuditServiceCollectionExtensionsTests`). All 23 tests pass. No new findings. + +**2026-06-01 baseline (`5f75cd4`).** `ZB.MOM.WW.Audit` is a small, clean, well-scoped library: one canonical `AuditEvent` sealed record, a 3-value `AuditOutcome` enum, two narrow seams (`IAuditWriter`, `IAuditRedactor`), five shipped helpers, and a single `AddZbAudit` registration. The record matches `EVENT-MODEL.md` field-for-field (names, types, required/optional split, and the @@ -33,6 +50,23 @@ family-wide baseline pattern), and a couple of missing edge-case tests (Audit-00 ## Checklist coverage +2026-11-09 re-review at `91fb2b4`: + +| # | Category | Examined | Notes | +|---|----------|----------|-------| +| 1 | Correctness & logic bugs | ☑ | `Truncate` covers null, at-cap, a marker longer than the cap, cap 0 and a negative cap (clamped). Fan-out continues past a failing or cancelled writer. No issues found. | +| 2 | Public API surface & compatibility | ☑ | The class→record and `set`→`init` change on `TruncatingAuditRedactorOptions` is source- and binary-breaking for mutating callers. It shipped inside `0.1.0` (verified against the cached nupkg) and no consumer mutates the options. No issues found. | +| 3 | Concurrency & thread safety | ☑ | Options are now immutable; writers and redactors are stateless; the composite snapshots its writers with `ToArray()`. No issues found. | +| 4 | Error handling & resilience | ☑ | The composite swallows all inner faults, OCE included. The redactor never throws, and its catch path scrubs both owned fields. No issues found. | +| 5 | Security & secret handling | ☑ | `RedactingAuditWriter` still applies the redactor strictly before the inner writer. Over-redaction is now strictly safer. No issues found. | +| 6 | Performance & resource management | ☑ | One `with`-copy per redaction and nothing disposable. When cancelled, the composite still calls each remaining writer once with the cancelled token, which is cheap and intended. No issues found. | +| 7 | Spec & shared-contract adherence | ☑ | Writer hard rule ("never throws to caller") is now honoured; the options are the immutable record the contract describes. No issues found. | +| 8 | Packaging, dependencies & project layout | ☑ | `GenerateDocumentationFile` is on; repository URLs point at `scadaproj`; `push.sh` packs a named solution from the repo root. No issues found. | +| 9 | Testing coverage | ☑ | 23 tests, including never-throw/over-redact, negative caps, composite null/empty and OCE swallowing, plus a container-building `AddZbAudit` test. No issues found. | +| 10 | Documentation & XML docs | ☑ | XML remarks on the composite and redactor match the new behaviour, and docs are emitted. No issues found. | + +
2026-06-01 baseline checklist (5f75cd4) + | # | Category | Examined | Notes | |---|----------|----------|-------| | 1 | Correctness & logic bugs | ☑ | `Truncate` boundary logic correct for sane configs; marker-longer-than-max clips safely. Partial over-redaction edge under negative max → Audit-002. | @@ -46,6 +80,8 @@ family-wide baseline pattern), and a couple of missing edge-case tests (Audit-00 | 9 | Testing coverage | ☑ | 19 tests; public contract exercised (round-trip, UTC normalization, fan-out, cancellation, redaction ordering). Missing never-throw/over-redact and null-writer cases → Audit-005. | | 10 | Documentation & XML docs | ☑ | XML docs present and accurate on the whole public surface; README accurate. Docs not emitted to a file → Audit-004. | +
+ ## Findings ### Audit-001 — `CompositeAuditWriter` re-throws `OperationCanceledException` to the caller, contradicting the "must not throw" writer contract diff --git a/code-reviews/Auth/findings.md b/code-reviews/Auth/findings.md index da3ba5c..32dd55a 100644 --- a/code-reviews/Auth/findings.md +++ b/code-reviews/Auth/findings.md @@ -7,14 +7,48 @@ | Component spec | `components/auth/spec/SPEC.md` | | Shared contract | `components/auth/shared-contract/ZB.MOM.WW.Auth.md` | | Status | Reviewed | -| Last reviewed | 2026-06-01 | -| Reviewer | Claude (automated baseline) | -| Commit reviewed | `5f75cd4` | -| Open findings | 0 | +| Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) | +| Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) | +| Commit reviewed | `91fb2b4` | +| Open findings | 4 | ## Summary -The security-critical core of this library is sound: API-key secrets are 32-byte CSPRNG values, +**2026-11-09 re-review (`91fb2b4`, scadaproj#15).** Re-reviewed at `91fb2b4` (on-disk version `0.4.3`; the feed carries `0.4.1`), covering everything added +since the `5f75cd4` baseline: LDAP `FallbackServers` failover with a sticky preferred endpoint (0.2.0), the AD +continuation-referral skip (0.2.1), the new `ZB.MOM.WW.Auth.ApiKeys.SqlServer` package (stores, migrator that +copes with concurrent boots, SQLite importer, `AddZbApiKeyAuthSqlServer`) (0.3.0), opt-in LDAP user bind +(`BindMode=User` / `UserBindTemplate` / `AcceptDomainQualifiedNames`) (0.4.0), and the canonical username plus +empty-password refusal (0.4.1). Auth-001..006 remain correctly resolved, and the new code holds up on its main +security points: +- **One connection per attempt.** Every endpoint attempt gets a fresh `ILdapConnection`, disposed by `using` on + every path, including failover. +- **Failover is system-side only.** A `BadCredentials`/49 answer is final and is never replayed against another + DC. +- **Empty passwords are refused before any network call in both modes**, which closes the unauthenticated-bind + hole. +- **Filter values are still RFC 4515-escaped.** +- **The SQL Server store is parameterised throughout.** The schema name passes a strict ASCII allow-list + re-checked at the public DDL entry point. +- **Concurrent first-boot migration converges.** The retry on 1205/2714/1913/2627 with jitter works: the live + suite passes 92/92 against a throwaway SQL Server 2022, and the DBA script plus a row-only login migrate cleanly. +- **The SQL Server DI extension has container-building tests.** + +The main new issue is in user-bind mode. The account that binds and the entry whose groups and username are +returned are found independently: the first from `UserBindTemplate`, the second from a `UserNameAttribute` +search. Nothing checks that they are the same object, so a UPN template, which the validator's own error text +offers first, can sign one user in with another user's groups (Auth-007). The remaining findings are Low: +- **Silent truncation in the SQL Server store.** The store sets explicit `SqlParameter` sizes and uses + case-insensitive key ids, so it does not match SQLite's semantics (Auth-008). +- **Username from a multi-valued attribute.** The canonical username takes the first value of a possibly + multi-valued attribute (Auth-009). +- **Store faults escape the verifier.** `ApiKeyVerifier` still lets store exceptions out, which matters more now + that the store can be a network database (Auth-010). + +Offline suite: 331 passed / 76 skipped (live-gated). With `AUTH_SQLSERVER_CONNSTR` set, the SqlServer suite +runs 92/92. + +**2026-06-01 baseline (`5f75cd4`).** The security-critical core of this library is sound: API-key secrets are 32-byte CSPRNG values, hashing is HMAC-SHA256 keyed by an external pepper, verification uses `CryptographicOperations.FixedTimeEquals`, the SQLite stores use parameterized SQL throughout, LDAP filter values are RFC 4515-escaped before being interpolated into the search filter, group-DN @@ -31,6 +65,23 @@ hashing algorithm and the AspNetCore surface. The library is **not yet adopted** ## Checklist coverage +2026-11-09 re-review at `91fb2b4`: + +| # | Category | Examined | Notes | +|---|----------|----------|-------| +| 1 | Correctness & logic bugs | ☑ | Failover walk, sticky index, referral skip, `LdapUserName` normaliser and the expiry check are correct. See Auth-008 (silent truncation / case-insensitive key ids diverge from SQLite) and Auth-009 (canonical username = first value of a multi-valued attribute) | +| 2 | Public API surface & compatibility | ☑ | `IApiKeyStoreMigrator` seam is clean and the old `SqliteAuthStoreMigrator` ctor is kept. Two changes are 0.x source/binary breaks with no other effect: the new `IApiKeyAdminStore` members and the optional parameter on `ApiKeyRecord`. No issues found | +| 3 | Concurrency & thread safety | ☑ | The sticky `_preferredIndex` uses `Volatile`, so its race is benign. The SQL Server migrator's retry with jitter converged under the live 8-way test. Stores open one connection per operation. No issues found | +| 4 | Error handling & resilience | ☑ | `LdapAuthService` stays fail-closed on every stage and `MarkUsedAsync` is best-effort (Auth-002 holds). See Auth-010: store lookup faults (e.g. `SqlException` on a shared-DB outage) still escape `ApiKeyVerifier`, despite its "only cancellation" contract | +| 5 | Security & secret handling | ☑ | Empty passwords are refused in both modes, BadCredentials is never failed over, filter escaping is intact, SQL is parameterised and the schema identifier is allow-listed. See Auth-007 (user-bind mode reads a different entry from the one that bound). DN templates are not escaped, but README documents this | +| 6 | Performance & resource management | ☑ | `ILdapConnection` is disposed via `using` inside the per-attempt try (every path, incl. failover). SQL connections, commands and readers use `await using`, and `OpenConnectionAsync` disposes on a failed open. No issues found | +| 7 | Spec & shared-contract adherence | ☑ | SQL Server rows mirror SQLite column-for-column, apart from Auth-008. The `IApiKeyVerifier` "only cancellation" contract is violated (Auth-010) | +| 8 | Packaging, dependencies & project layout | ☑ | New SqlServer package is cleanly split and depends only on Abstractions-level `Microsoft.Extensions.*`. The Pkcs/SQLitePCLRaw transitive pins are present. No issues found | +| 9 | Testing coverage | ☑ | Strong: failover, user-bind, referral, normaliser, validator, live SQL Server suites, plus DI tests that build containers for `AddZbApiKeyAuthSqlServer`, `AddZbApiKeyAuth` and `AddZbLdapAuth`. Gaps: no test ties the bound identity to the returned entry (Auth-007), no over-length or case-variant key-id test (Auth-008), no multi-valued username attribute test (Auth-009), no store-throws verifier test (Auth-010) | +| 10 | Documentation & XML docs | ☑ | Thorough. `UserBindTemplate` docs warn about UPN mismatch, but the validator message offers the UPN form first (Auth-007). `ApiKeyVerifier` remark overstates its exception contract (Auth-010) | + +
2026-06-01 baseline checklist (5f75cd4) + | # | Category | Examined | Notes | |---|----------|----------|-------| | 1 | Correctness & logic bugs | ☑ | Core logic correct; see Auth-003 (JSON parse can throw on hot path), Auth-005 (KeyPrefix value inconsistency) | @@ -44,6 +95,8 @@ hashing algorithm and the AspNetCore surface. The library is **not yet adopted** | 9 | Testing coverage | ☑ | Broad and contract-level; gaps: no test for corrupt-`scopes` deserialization (Auth-003) or `MarkUsedAsync`-throws (Auth-002); `ValidateOnStart` behavior untested (Auth-001) | | 10 | Documentation & XML docs | ☑ | XML docs thorough and accurate; README is stale — see Auth-004 | +
+ ## Findings ### Auth-001 — LDAP options validator is registered but never runs at startup @@ -252,3 +305,167 @@ beyond the OS default, and document the default validation behavior. **Resolution** Resolved in `544a6dd`, 2026-06-01 — added an optional `LdapOptions.ServerCertificateValidationCallback` (BCL `RemoteCertificateValidationCallback`, no new dependency) threaded through the `ILdapConnection.Connect` seam; `NovellLdapConnection` now builds `LdapConnectionOptions.ConfigureRemoteCertificateValidationCallback(...)` (a supplied callback wins; otherwise the previously-dead `allowInsecure` gates a dev-only accept-any path; otherwise OS-trust-store default), and the default validation behaviour is documented in XML/README (tests: `LdapAuthServiceTests.Connect_ReceivesConfiguredCertValidationCallback`, `..._Connect_ReceivesAllowInsecureFlag_FromOptions`, `..._Connect_NoCertCallbackConfigured_PassesNull`). + + + +### Auth-007 — User-bind mode never checks that the entry whose groups it returns is the account that bound + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Security & secret handling | +| Status | Open | +| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs:329` | + +**Description** + +`CompleteAsUser` authenticates by binding as `UserBindTemplate.Replace("{0}", username)` (line 329–332). It then +runs a separate search, `(=)` (line 345 → `SearchUser`, line 361), and returns the +groups and username of whatever single entry that search finds (line 349 → `SucceedFromEntry`). Two different +directory lookups resolve the bind name and the search filter. Nothing checks that the entry found is the account +the directory just authenticated. There is no comparison of DNs, no RFC 4532 "Who am I?", and no search keyed on +the bind identity. The XML doc and README describe the UPN mismatch as guidance only ("a mismatch can … bind a +different account than the one whose groups are read"). The validator's failure message +(`LdapOptionsValidator.cs:77`) offers `"{0}@example.com"` as its first example template. A DN-form template is also +spliced in unescaped (documented), which is another way for the bind identity and the search key to diverge. + +**Failure scenario** + +`BindMode=User` with `UserBindTemplate="{0}@corp.com"` and `UserNameAttribute=sAMAccountName` on AD, a combination +the validator message suggests. Mallory's account has explicit UPN `jsmith@corp.com` and `sAMAccountName=jsmith2`, +which is common after a rename or when two people share a name. A separate admin account has `sAMAccountName=jsmith`. +Mallory signs in as `jsmith` with her own password: +1. AD resolves the UPN bind to Mallory's account and accepts it. +2. The search `(sAMAccountName=jsmith)` returns the admin's entry. +3. `LdapAuthResult.Success("jsmith", "Admin Smith", ["UG_Admins"])` is returned. + +That is privilege escalation with valid credentials of a different user. I reproduced this through the +`FakeLdapConnection` seam: the bind went to `jsmith@corp.test` and the result carried the other entry's groups. + +**Recommendation** + +After the bind, identify the entry by the bound identity, not the typed name: +- **UPN templates:** search on `userPrincipalName=`. +- **Any template:** use the RFC 4532 Who-am-I extended operation (AD returns `u:DOMAIN\sam`) and check it against + the found entry's `sAMAccountName`. +- **DN templates:** require the found entry's DN to equal the bind DN, and DN-escape `{0}` (RFC 4514). + +Fail with `GroupLookupFailed` on a mismatch. At minimum, reorder the validator example to lead with +`EXAMPLE\{0}`. Add a test where the bind name and the searched entry differ. + +### Auth-008 — SQL Server store silently truncates over-length values and matches key ids case-insensitively, diverging from SQLite + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAdminStore.cs:45` (also `:46`, `:48`; `SqlServerApiKeyStore.cs:56,77`; `SqlServerApiKeyAuditStore.cs:30-32`; `SqliteApiKeyStoreImporter.cs:258-261,292-294`; columns in `SqlServerAuthSchema.cs:79-96`) | + +**Description** + +Every string parameter is added with an explicit size: `NVarChar, 64` for `key_id`, `event_type` and +`remote_address`, `16` for `key_prefix`, and `256` for `display_name`. `SqlParameter` truncates an input longer +than `Size` without raising an error, so over-length values are cut down rather than rejected. The columns also use +the database's default collation, which is case-insensitive on a stock SQL Server. The SQLite store this one claims +to port "one-for-one" has `TEXT` columns, no length limits and BINARY (case-sensitive) comparison. `CreateKeyAsync` +validates only that `keyId` is non-blank and has no `_`. + +I verified this live against a SQL Server 2022 container: +- A 70-char `keyId` with a 20-char prefix and a 300-char display name was created without error, and the row was + stored as 64/16/256 characters. +- `FindByKeyIdAsync` with the full 70-char id, and with a different 70-char id sharing the first 64 characters, + both found the row. +- `FindByKeyIdAsync("LINE1")` found the row stored as `line1`. +- Creating `Line1` beside `line1` failed with a PK violation. +- An 80-char audit `event_type`/`remote_address` was stored as 64 characters. + +**Failure scenario** + +1. **Over-length key id.** An admin mints a key with a long, descriptive key id (more than 64 characters) on a host + using `AddZbApiKeyAuthSqlServer`. Creation succeeds and the returned token embeds the full id. Verification + succeeds only because the lookup parameter is truncated the same way. The authenticated `ApiKeyIdentity.KeyId` + is the 64-character stored value, not the id the admin assigned. Any host ACL or audit correlation keyed on the + assigned id never matches. +2. **Case-variant ids in the importer.** Two SQLite keys `Line1` and `line1` (legal on SQLite) migrate through + `SqliteApiKeyStoreImporter`. The second is counted as "skipped" because the `IF NOT EXISTS` check matches + case-insensitively. After cut-over its token resolves to the other key's row, fails `SecretMismatch`, and that + client is locked out with no error at import time. + +**Recommendation** + +- Pass `-1`/no explicit size, or validate lengths up front and throw `ArgumentException`, in both `CreateKeyAsync` + and the importer. +- Declare `key_id` with a binary or case-sensitive collation (e.g. `COLLATE Latin1_General_100_BIN2`) to match + SQLite. Alternatively, document and enforce a case-folded, `[A-Za-z0-9-]{1,64}` key-id alphabet in + `ApiKeyAdminCommands`. +- Make the importer report key-id collisions instead of counting them as plain skips. + +### Auth-009 — Canonical username is the first value of a possibly multi-valued attribute, not the value the user matched + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs:437` | + +**Description** + +0.4.1's `ExtractUserName` returns `values[0]` of `UserNameAttribute` whenever that value is non-blank (lines +437–441). The default `UserNameAttribute` is `cn` (`LdapContracts.cs:30`), which is multi-valued in standard LDAP +schemas: OpenLDAP and 389-DS entries routinely carry both `cn: John Smith` and `cn: jsmith`. The search matched on +one of those values, but the code returns whichever value the server lists first, which need not be the one the +user typed. This is the same person and the same entry, so there is no privilege effect. However, the identity +string handed to the host changes with the 0.4.1 upgrade and depends on server attribute ordering. + +**Failure scenario** + +A host on the default `UserNameAttribute=cn` runs against an OpenLDAP directory where users have two `cn` values. +Before 0.4.1 `jsmith` signed in as `jsmith`. After the upgrade the result's `Username` is `John Smith`. Per-user +records, audit trails and any username-keyed role overrides in the host split into two identities, and a username +containing a space can break downstream consumers that treat it as an account name. I reproduced this through the +fake connection: typed `jsmith` returned `Username == "John Smith"`. + +**Recommendation** + +Prefer the attribute value that equals the typed (normalised) name under `OrdinalIgnoreCase`, which still +canonicalises case. Fall back to `values[0]` only when no value matches. Add a multi-valued-attribute test. + +### Auth-010 — `ApiKeyVerifier` lets store exceptions escape despite its "only exception path is cancellation" contract + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Error handling & resilience | +| Status | Open | +| Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys/ApiKeyVerifier.cs:47` (contract stated at `:13`) | + +**Description** + +The type remark (line 13) promises "The only exception path is cancellation". Auth-002 fixed `MarkUsedAsync` to +honour that promise, but the lookup `await store.FindByKeyIdAsync(...)` at line 47 is unguarded. Its failures +propagate unchanged. With the new `SqlServerApiKeyStore` the store is a network database shared by a cluster, so +lookup failures are an expected operating condition, not a rare local-disk fault. A misconfigured +`SqlServerApiKeyOptions` also escapes: it is validated lazily in the `AuthSqlServerConnectionFactory` constructor. +With `RunMigrationsOnStartup=false` that throws `InvalidOperationException` out of the first verifier resolution or +call. + +**Failure scenario** + +The shared SQL Server is briefly unreachable during a failover, or the central pair loses its route to it. Every +inbound API request's `VerifyAsync` throws `Microsoft.Data.SqlClient.SqlException`; I verified this against a closed +port. Consumers wrote their middleware and interceptors against the documented contract and map +`Succeeded == false` to 401, so they do not catch this. The request surfaces as an unhandled 500, or a gRPC +`Unknown`, instead of a clean auth failure or 503, and the failure goes unaudited. + +**Recommendation** + +Wrap the lookup and map non-cancellation exceptions to a structured failure, for example a new +`ApiKeyFailure.StoreUnavailable`. Otherwise, correct the remark to say store exceptions propagate. Add a test +where the store throws. Optionally add `SqlServerApiKeyOptions` startup validation +(`AddOptions().Validate(...).ValidateOnStart()`), mirroring Auth-001. diff --git a/code-reviews/Configuration/findings.md b/code-reviews/Configuration/findings.md index 2698613..1513e45 100644 --- a/code-reviews/Configuration/findings.md +++ b/code-reviews/Configuration/findings.md @@ -7,14 +7,31 @@ | Component spec | `components/configuration/spec/SPEC.md` | | Shared contract | `components/configuration/shared-contract/ZB.MOM.WW.Configuration.md` | | Status | Reviewed | -| Last reviewed | 2026-06-01 | -| Reviewer | Claude (automated baseline) | -| Commit reviewed | `5f75cd4` | +| Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) | +| Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) | +| Commit reviewed | `91fb2b4` | | Open findings | 0 | ## Summary -The library is small (five source files, ~230 LOC) and in good health. Its core promise — +**2026-11-09 re-review (`91fb2b4`, scadaproj#15).** Re-reviewed at `91fb2b4` (2026-11-09) against the 2026-06-01 baseline `5f75cd4`. The only source changes +are the four baseline fixes from `544a6dd` and the repository-URL correction from `01c6dc1`; the version +is still `0.1.0`. The fixes are: + +- `AddValidatedOptions` registers its validator with `TryAddEnumerable`, so a double call runs it once. +- `PortValue` and `HostPort` parse ports with `NumberStyles.None`, invariant culture, so a sign and + surrounding whitespace are rejected. +- `PortValue` quotes the raw value in both the parse-failure and range-failure messages. +- The README and XML docs are packed. + +All four fixes are correct. `TryAddEnumerable` deduplicates on the (service, implementation) pair, so +one validator type serving two different options types is still registered for each. The DI helper has +container-building tests (`AddValidatedOptionsTests`, including the double-registration case), and +`ChecksWordingTests` pins the exact message strings. All 42 tests pass. No new findings. +`HostPort` still rejects bracketed IPv6 (`[::1]:443`). That behaviour is unchanged from the baseline and +documented as `host:port`, so it is not raised. + +**2026-06-01 baseline (`5f75cd4`).** The library is small (five source files, ~230 LOC) and in good health. Its core promise — **accumulate every failure, never short-circuit** — holds across all three entry points (`ValidationBuilder`, `OptionsValidatorBase`, `ConfigPreflight`): the override never returns early, each primitive appends to a `List` and returns `this`, and the `Success`/`Fail` @@ -37,6 +54,23 @@ is solid for the happy and primary-failure paths; a few edge cases noted below a ## Checklist coverage +2026-11-09 re-review at `91fb2b4`: + +| # | Category | Examined | Notes | +|---|----------|----------|-------| +| 1 | Correctness & logic bugs | ☑ | Strict invariant port parsing (`NumberStyles.None`) rejects a sign, leading or trailing whitespace and negatives, in both `PortValue` and `HostPort` (pinned by `ChecksWordingTests`); the range check is unchanged. Accumulation is unchanged. No issues found. | +| 2 | Public API surface & compatibility | ☑ | No public surface change. Stricter parsing is a deliberate tightening: values like "+5000" or " 5000" now fail at startup. No issues found. | +| 3 | Concurrency & thread safety | ☑ | Validators are stateless, and `TryAddEnumerable` happens at registration time only. No issues found. | +| 4 | Error handling & resilience | ☑ | `ValidateOnStart` with `OptionsValidationException` is unchanged, and guard clauses are present. No issues found. | +| 5 | Security & secret handling | ☑ | Messages echo non-sensitive config values only. No issues found. | +| 6 | Performance & resource management | ☑ | Startup-only. No issues found. | +| 7 | Spec & shared-contract adherence | ☑ | `PortValue` and `HostPort` wording is now consistent and pinned by `ChecksWordingTests`. No issues found. | +| 8 | Packaging, dependencies & project layout | ☑ | `PackageReadmeFile` plus the packed README; XML docs enabled; repository URLs corrected. No issues found. | +| 9 | Testing coverage | ☑ | 42 tests, including double-registration via a built `ServiceProvider`, null guards and exact wording. No issues found. | +| 10 | Documentation & XML docs | ☑ | `PortValue` XML doc matches the strict-parse behaviour. No issues found. | + +
2026-06-01 baseline checklist (5f75cd4) + | # | Category | Examined | Notes | |---|----------|----------|-------| | 1 | Correctness & logic bugs | ☑ | Accumulation never short-circuits; port/host:port/duration/min-count boundaries correct. Port parsing over-permissive (003). | @@ -50,6 +84,8 @@ is solid for the happy and primary-failure paths; a few edge cases noted below a | 9 | Testing coverage | ☑ | 27 tests cover the four public surfaces + accumulation + byte-format. Untested: `AddValidatedOptions` null guards, double-registration, exact wording strings. | | 10 | Documentation & XML docs | ☑ | Public XML docs present and accurate on every type/member. Not emitted into the nupkg (004). | +
+ ## Findings ### Configuration-001 — `AddValidatedOptions` uses `AddSingleton`, so a double call registers (and runs) the validator twice diff --git a/code-reviews/GalaxyRepository/findings.md b/code-reviews/GalaxyRepository/findings.md new file mode 100644 index 0000000..c921ed6 --- /dev/null +++ b/code-reviews/GalaxyRepository/findings.md @@ -0,0 +1,409 @@ +# Code Review — GalaxyRepository + +| Field | Value | +|-------|-------| +| Library | `ZB.MOM.WW.GalaxyRepository/` | +| Packages | `ZB.MOM.WW.GalaxyRepository` | +| Component spec | None under `components/`. The design of record is `docs/plans/2026-06-23-historian-gateway-design.md` §10 (component 1). | +| Shared contract | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/Protos/galaxy_repository.proto` (`galaxy_repository.v1`, wire-compatible with mxaccessgw's original) | +| Status | Reviewed | +| Last reviewed | 2026-11-09 | +| Reviewer | Claude | +| Commit reviewed | `91fb2b4` | +| Open findings | 8 | + +## Summary + +The library is in good shape overall. All SQL is constant text with no string-built input, so +there is no injection surface. Every `SqlConnection`, `SqlCommand` and `SqlDataReader` is disposed. +The cache publishes immutable entries through `Volatile` reference swaps behind a single refresh +gate. The refresh `BackgroundService` cannot fault the host. The snapshot store writes atomically +(temp file + rename) and fails soft. Page tokens are bound to the cache sequence and to a filter +signature. + +The two Medium findings are in the browse projectors: + +- **GalaxyRepository-001:** the per-entry filter memo has no size limit. Any gRPC client can grow it + to gigabytes until the next Galaxy deploy. A scratch test retained **1.0 GB from 2000 distinct + `tag_name_glob` values over 50k objects**. +- **GalaxyRepository-002:** browse-subtree scoping hides the *ancestors* of an allowed subtree. A + scoped API key that browses lazily from the root gets an empty tree. + +The remaining findings are Low: robustness, testing, packaging and docs. + +`AddZbGalaxyRepository` was checked against the family's DI-defect pattern. A scratch test built a +real `Host` with `AddGrpc()` + `AddZbGalaxyRepository`, resolved every service, started the host +against an unreachable SQL Server and observed the cache settle to `Unavailable`. No wiring defect +was found, but the repo has no such test (GalaxyRepository-006). + +The existing suite passes: 64/64 tests. The consumer side was spot-checked. mxaccessgw's +`GatewayBrowseScopeProvider` registers before the library's `TryAddSingleton` default and fails +closed when identity is absent, so the empty-means-unscoped contract is handled correctly there. + +## Checklist coverage + +| # | Category | Examined | Notes | +|---|----------|----------|-------| +| 1 | Correctness & logic bugs | ☑ | Hierarchy CTEs carry a depth-10 guard on template/package chains. Orphans and self-parents are re-rooted. See GalaxyRepository-002 (scoped ancestors hidden) and GalaxyRepository-003 (parent cycles vanish from browse). Deploy detection is gated on `time_of_last_deploy` and self-heals on the next tick after a mid-pull deploy. | +| 2 | Public API surface & compatibility | ☑ | Proto is unchanged and wire-compatible. `IGalaxyBrowseScopeProvider` treats null/empty as "no restriction" (fail-open by contract). The one consumer that implements it (mxaccessgw) handles this correctly. No issues found. | +| 3 | Concurrency & thread safety | ☑ | Immutable entries, a refresh `SemaphoreSlim`, and a bounded regex cache with locked eviction. See GalaxyRepository-005 (deploy-notifier bootstrap race reorders events). | +| 4 | Error handling & resilience | ☑ | Cache refresh catches every non-cancellation exception and degrades to Stale/Unavailable. The snapshot store fails soft. See GalaxyRepository-004 (hard-coded 5-min stale threshold). | +| 5 | Security & secret handling | ☑ | No string-built SQL. Regex has a 100 ms match timeout and a bounded compile cache. See GalaxyRepository-001 (unbounded memo is client-driven memory exhaustion). | +| 6 | Performance & resource management | ☑ | All ADO.NET objects are disposed. See GalaxyRepository-001 and GalaxyRepository-004 (the memo is defeated while the projected status is Stale). | +| 7 | Spec & shared-contract adherence | ☑ | Matches the design §10 split: SQL provider, cache, snapshot, deploy poll, projector and DI in the lib; scoping in the consumer. No issues found. | +| 8 | Packaging, dependencies & project layout | ☑ | See GalaxyRepository-007: the transitive `System.Security.Cryptography.Pkcs` 9.0.4 has a high-severity advisory (NU1903). | +| 9 | Testing coverage | ☑ | Projectors, cache, notifier, snapshot store and gRPC scoping are covered. No container-building test exists for the DI extension (GalaxyRepository-006). No test covers a scoped browse from the root (GalaxyRepository-002). | +| 10 | Documentation & XML docs | ☑ | See GalaxyRepository-008 (stale `SnapshotCachePath` doc, csproj comment and CLAUDE.md consumer matrix). | + +## Findings + +### GalaxyRepository-001 — Filter memo has no size limit: client-chosen filters retain unlimited memory until the next deploy + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Performance & resource management | +| Status | Open | +| Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs:28` (memo insert `:117`); same pattern in `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs:20` (insert `:121`) | + +**Description** + +`GalaxyHierarchyProjector` keeps a +`ConditionalWeakTable>>`. +Every distinct filter signature gets its own entry: root, max depth, category ids, template filters, +`tag_name_glob`, attribute/alarm/historized flags and browse globs. Each entry holds the full list of +matching views, so a broad filter costs one reference per Galaxy object. + +Nothing removes entries. The dictionary is released only when the cache publishes a new entry, +which happens only when `galaxy.time_of_last_deploy` advances. In production that can be days or +weeks apart. `GalaxyBrowseProjector.FilteredChildrenCache` has the same shape. + +`tag_name_glob` is fully client-controlled. `*`, `**`, `***` and so on are all distinct signatures +that match everything. The authors already treated client globs as hostile in `GalaxyGlobMatcher` +(its regex cache is deliberately capped at 256 for exactly this reason). The memo one layer up +received no such limit. + +**Failure scenario** + +Take a gateway (mxaccessgw or HistorianGateway) serving a 50k-object Galaxy. A client with a valid +API key, or a buggy client that builds a fresh glob per call, issues `DiscoverHierarchy` with 2000 +distinct `tag_name_glob` values and `page_size=1`. + +A scratch test of exactly this (`Project(entry, {TagNameGlob = new string('*', i)}, null, 0, 1)` for +i = 1..2000) retained **1010 MB**. Memory keeps growing with every new glob until the next Galaxy +deploy. The gateway process (the x64 host that also brokers MXAccess or Historian traffic) can be +driven out of memory without a single SQL query. + +**Recommendation** + +Bound the memo per entry. Options: + +- An LRU or FIFO cap (e.g. 64 signatures). +- Memoize only when the request carries a `page_token` or is likely to page, i.e. when + `TotalObjectCount` exceeds the page size. + +Apply the same cap to `GalaxyBrowseProjector`. Add a test that drives more than the cap of distinct +globs and asserts the memo size. + +**Resolution** + +_Unresolved._ + +### GalaxyRepository-002 — Browse-subtree scoping hides the ancestors of an allowed subtree, so scoped lazy browse from the root is empty + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyBrowseProjector.cs:134` (also `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyProjector.cs:233`) | + +**Description** + +`GetFilteredChildren` runs `MatchesBrowseSubtrees(view, globs)` first and `continue`s when a direct +child fails it. It never checks whether that child has a descendant that *is* inside the allowed +subtree. + +Right below, the request-filter branch does surface ancestors of matching descendants, with +`HasMatchingDescendant` and the comment "a matching descendant should still surface its ancestor". +The scope branch has no equivalent. + +So with a scope glob like `Plant/LineA/*`, neither `Plant` nor `Plant/LineA` matches. Both are +filtered out of their parent's child list. `DiscoverHierarchy` likewise omits the `Plant/LineA` area +object itself. + +**Failure scenario** + +An mxaccessgw API key is scoped to `BrowseSubtrees: ["Plant/LineA/*"]`. A lazy-browse client starts +at the root with `BrowseChildren{}` and expands nodes, as the OtOpcUa / ScadaBridge browse dialogs +do. Scratch-test result over `Plant → LineA → Pump01`, `Plant → LineB → Pump02`: + +- Root `BrowseChildren`: `TotalChildCount = 0`. +- `DiscoverHierarchy`: only `Pump01` (no `LineA` area). +- `BrowseChildren{ParentContainedPath="Plant/LineA"}`: 1 child. + +The scoped user sees an empty Galaxy unless they already know and type the exact parent path. + +Admins can work around it by also listing the ancestor paths as globs (`Plant`, `Plant/LineA`), but +that is undocumented. It also exposes those ancestor objects *with their attributes*, which is wider +than the intended scope. + +**Recommendation** + +In `GetFilteredChildren`, when a child fails the scope check, test whether it is an ancestor of an +allowed path. Either use `HasMatchingDescendant` with the scope applied, or do a cheap prefix test of +the child's `ContainedPath + "/"` against each glob's literal prefix. If it is an ancestor, include +it as a navigation-only node (`has_children=true`) with its attributes stripped. + +Document the semantics on `IGalaxyBrowseScopeProvider`. Add a test that browses from the root under +a deep scope glob. + +**Resolution** + +_Unresolved._ + +### GalaxyRepository-003 — Objects in a parent cycle disappear from `BrowseChildren` + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyIndex.cs:106` | + +**Description** + +`GalaxyHierarchyIndex.Build` re-roots two kinds of object under key 0: self-parented rows, and +orphans whose parent is absent. The comment there says it does so "so nothing disappears from +browse". + +A multi-node cycle (A→B→C→A, every parent present) is not re-rooted. Each member is bucketed under +another cycle member, and none is reachable from key 0. `GalaxyBrowseProjector.HasMatchingDescendant` +already defends against such cycles, which shows the authors expected them to occur, but only for +termination, not for reachability. + +**Failure scenario** + +Corrupt or mid-edit Galaxy data produces a containment cycle. A scratch test with +`Plant` (root) plus a cycle `A(10)→B(11)→C(12)→A` gave: + +- Root `BrowseChildren`: only `Plant`. +- `DiscoverHierarchy`: all 4 objects. + +The three cycle objects, and their attributes, are unreachable for every lazy-browse client while +still being present in flat discovery. + +**Recommendation** + +After bucketing, walk from key 0 and collect the reachable ids. Re-root any object that was not +reached (break each cycle at its lowest gobject id) and log a warning naming the cycle. + +**Resolution** + +_Unresolved._ + +### GalaxyRepository-004 — Stale threshold is hard-coded at 5 min regardless of the refresh interval, and stale projection defeats the memo + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Error handling & resilience | +| Status | Open | +| Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyHierarchyCache.cs:19` (projection at `:55-64`, `:357`) | + +**Description** + +`StaleThreshold` is a `static readonly` 5 minutes. It is independent of +`GalaxyRepositoryOptions.DashboardRefreshIntervalSeconds`, which is configurable with no upper bound. + +When `now - LastSuccessAt > 5 min`, the `Current` getter returns `snapshot with { Status = Stale }`. +That is a **new record instance on every read**. Both projectors key their memo on the entry +instance (`ConditionalWeakTable`), so every page of every request misses the memo and re-filters the +full object list (O(total) per page). Each read also attaches a throwaway memo dictionary. + +**Failure scenario** + +A consumer sets `DashboardRefreshIntervalSeconds: 600` to reduce SQL load. For half of every +interval, the healthy cache reports `Stale` to the dashboard and to `GetLastDeployTime` callers. +Every `DiscoverHierarchy` page meanwhile does full re-filtering. + +A scratch test confirmed it: after advancing the clock 6 min past a successful refresh, two +consecutive `cache.Current` reads returned `Status=Stale` and `ReferenceEquals == false`. The same +memo bypass happens in the legitimate stale case (SQL unreachable for more than 5 min). That is when +the gateway is already degraded. + +**Recommendation** + +Derive the threshold from the options, e.g. `max(5 min, 3 × refresh interval)`, or make it an +option. Cache the projected-stale record, or key the memo on `Sequence` rather than on the record +instance, so a status-only projection does not invalidate it. + +**Resolution** + +_Unresolved._ + +### GalaxyRepository-005 — Deploy-notifier bootstrap can deliver an older event after a newer one + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Concurrency & thread safety | +| Status | Open | +| Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyDeployNotifier.cs:53` | + +**Description** + +`SubscribeAsync` takes these steps in order: + +1. Registers the channel in `_subscribers` (`:53`). +2. Reads `_latest` (`:57`). +3. Writes that bootstrap value into the channel (`:60`). + +`Publish` can run between steps 2 and 3. It sets `_latest = new`, iterates `_subscribers` (which +already contains this channel) and `TryWrite(new)`. The subscriber then writes the stale `old` it +read in step 2. The channel holds `[new, old]`. + +**Failure scenario** + +A client opens `WatchDeployEvents` at the moment a refresh publishes a redeploy. It receives +sequence N+1 followed by sequence N. A client that treats the most recent event as current state ends +up on the older deploy time. If it sent `last_seen_deploy_time = new` it may also have the `new` +event suppressed and then receive `old` as if it were a change, triggering a spurious rebuild. + +The window is small (microseconds), but it is a real ordering violation, and the class doc tells +clients to use `sequence` to detect gaps. + +**Recommendation** + +Read `_latest` *before* registering the subscriber. Alternatively, have the reader skip any event +whose `Sequence` is not greater than the last one yielded. The second option also covers the +duplicate case, where the same event arrives both via `Publish` and via bootstrap. + +**Resolution** + +_Unresolved._ + +### GalaxyRepository-006 — No test builds the container from `AddZbGalaxyRepository` + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Testing coverage | +| Status | Open | +| Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/DependencyInjection/GalaxyRepositoryServiceCollectionExtensions.cs:31` | + +**Description** + +No test calls `AddZbGalaxyRepository` or `MapZbGalaxyRepository`. Every test constructs the types by +hand. This family has shipped three DI-extension defects of exactly this class: the Secrets +TryAdd-order no-op, the factory-lambda singleton cycle and the inert replicator. Each was invisible +without a test that builds the provider. + +The extension also carries a non-obvious ordering contract: the host must register its +`IGalaxyBrowseScopeProvider` *before* the call so that the `TryAddSingleton` default does not win. +mxaccessgw relies on this and documents it at `GatewayApplication.cs:152`. + +**Failure scenario** + +A future edit swaps a registration to `TryAdd`, introduces a factory cycle, or changes the scope +provider from `TryAdd` to `Add`. If the scope provider changes to `Add`, `NullGalaxyBrowseScopeProvider` +would override a host provider registered earlier and silently **un-scope every API key**. Nothing in +this repo's suite would fail. + +A scratch test confirms the current wiring is correct: it builds a `Host` with `AddGrpc()` + +`AddZbGalaxyRepository`, resolves all services, starts the host and sees the cache reach +`Unavailable`. So this finding is a missing guard, not a present bug. + +**Recommendation** + +Add container-building tests that: + +- Build the host and resolve `IGalaxyHierarchyCache`, `IGalaxyRepository`, the hosted service and + `GalaxyRepositoryGrpcService` (via `ActivatorUtilities`). +- Assert that a host-registered `IGalaxyBrowseScopeProvider` (registered before *and* after the + call) is the one resolved. +- Start and stop the host against an unreachable connection string. + +**Resolution** + +_Unresolved._ + +### GalaxyRepository-007 — Shipped package pulls a vulnerable `System.Security.Cryptography.Pkcs` 9.0.4 + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Packaging, dependencies & project layout | +| Status | Open | +| Location | `ZB.MOM.WW.GalaxyRepository/Directory.Packages.props:9` | + +**Description** + +`Microsoft.Data.SqlClient` 6.0.2 brings in `System.Security.Cryptography.Pkcs` 9.0.4 transitively. +Restore reports NU1902 (GHSA-2fmv-rv2c-59x7, moderate) and **NU1903 (GHSA-846f-f764-76mx, high)** +against both the library and anything that references it. This was observed while building a scratch +consumer. The library has no transitive pin, so every consumer inherits the advisory. + +**Failure scenario** + +A consumer that builds with `TreatWarningsAsErrors` (the family's zero-warnings gate) fails to +restore or build after a GalaxyRepository bump. It has to add its own pin, as already happened in +HistorianGateway ("crypto pin"). Consumers that do not gate ship the vulnerable assembly. + +**Recommendation** + +Pin `System.Security.Cryptography.Pkcs` to a patched version in `Directory.Packages.props` and +reference it from the library csproj, or move to a SqlClient release that depends on a patched +version. Publish this as a patch release. + +**Resolution** + +_Unresolved._ + +### GalaxyRepository-008 — Stale documentation: `SnapshotCachePath` default, csproj comment, consumer matrix + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Documentation & XML docs | +| Status | Open | +| Location | `ZB.MOM.WW.GalaxyRepository/src/ZB.MOM.WW.GalaxyRepository/GalaxyRepositoryOptions.cs:47` | + +**Description** + +Three places are out of date: + +- **`SnapshotCachePath` XML doc** (`GalaxyRepositoryOptions.cs:47-54`). It says that when the path + is empty and `PersistSnapshot` is enabled, "the snapshot store (a later task) decides where to + write". The shipped `GalaxyHierarchySnapshotStore` (`GalaxyHierarchySnapshotStore.cs:44-46`) + instead **disables persistence entirely**. +- **Library csproj** (`ZB.MOM.WW.GalaxyRepository.csproj:22`). It still says "Proto files are added + in Task 2; the empty glob is intentional". +- **`ZB.MOM.WW.GalaxyRepository/CLAUDE.md`**. It says "mxaccessgw adoption is a tracked follow-on", + but mxaccessgw has adopted the library: its `GatewayApplication.cs` calls `AddZbGalaxyRepository` + / `MapZbGalaxyRepository`, and the root index records adoption as of 2026-07-18. + +**Failure scenario** + +An operator reads the option doc and leaves `SnapshotCachePath` empty, expecting a default location. +They get no snapshot at all. The cold-start "browse last-known data while SQL is down" guarantee +silently does not exist on that host, and nothing logs that persistence is off. + +**Recommendation** + +- Correct the XML doc to "empty disables persistence", and log once at Information when the store + is disabled while `PersistSnapshot` is `true`. +- Drop the stale csproj comment. +- Update the CLAUDE.md consumer matrix (HistorianGateway + mxaccessgw). + +**Resolution** + +_Unresolved._ diff --git a/code-reviews/Health/findings.md b/code-reviews/Health/findings.md index cc6a6ba..6f78c97 100644 --- a/code-reviews/Health/findings.md +++ b/code-reviews/Health/findings.md @@ -7,14 +7,36 @@ | Component spec | `components/health/spec/SPEC.md` | | Shared contract | `components/health/shared-contract/ZB.MOM.WW.Health.md` | | Status | Reviewed | -| Last reviewed | 2026-06-01 | -| Reviewer | Claude (automated baseline) | -| Commit reviewed | `5f75cd4` | -| Open findings | 0 | +| Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) | +| Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) | +| Commit reviewed | `91fb2b4` | +| Open findings | 1 | ## Summary -The library is small, cohesive, and well-documented, and it tracks the normalized spec closely: +**2026-11-09 re-review (`91fb2b4`, scadaproj#15).** Re-reviewed at `91fb2b4` (2026-11-09) against the 2026-06-01 baseline `5f75cd4`; the library is now +`0.3.0`. The headline change, the active tier now meaning "oldest `Up` member" through the new +`ClusterActiveNode` helper instead of `Leader`/`RoleLeader`, is correct. Only `MemberStatus.Up` members +are eligible, so `Joining`, `WeaklyUp`, `Leaving`, `Exiting`, `Down` and `Removed` are all excluded. Ties +fall through to Akka's `Member.IsOlderThan`, which breaks an equal up-number on address ordering, so the +choice is deterministic. Self-identity is compared on `UniqueAddress`, so a restarted incarnation on the +same host and port is never mistaken for the old one. An `Up`-but-unreachable oldest member is still +named active, which matches singleton placement until the split-brain resolver downs it. A role-scoped +node that carries none of the preferred roles answers `NoActiveRoleStatus` (default Unhealthy, i.e. 503). +Both startup-safety paths (no `ActorSystem`, cluster not yet accessible) still return Degraded (200), and +a container-built `AddCheck` probe confirmed that `ActivatorUtilities` picks the +right constructor out of the three. The status mapping, the `data` JSON shape (emitted only when the +check supplies data, `description` always present) and the composite convention builder are correct. +All 76 tests pass (Health.Tests 25, Health.Akka.Tests 45, Health.EntityFrameworkCore.Tests 6). +`ClusterActiveNodeTests` runs a real two-node cluster whose age order and address order diverge, and +covers role scoping, role preference and the gate. The non-`Up` exclusions (`Leaving`/`WeaklyUp`/ +`Joining`) and the restart-incarnation `UniqueAddress` comparison are verified by reading the code only: +no test exercises them. That is a test gap, not a defect. There is one new Low finding: the `AkkaActiveNodeGate` can only be scoped to a +single role, while the probe takes an ordered role-preference list. So the documented "gate and probe +can never disagree" guarantee does not hold for the fused-node configuration that OtOpcUa actually runs +(Health-007). It is latent, because no family app uses `AkkaActiveNodeGate` today. + +**2026-06-01 baseline (`5f75cd4`).** The library is small, cohesive, and well-documented, and it tracks the normalized spec closely: the three-tier endpoint convention, the canonical JSON writer, the `IActiveNodeGate` seam, the configurable `AkkaClusterStatusPolicy` presets, and the role-filtered `ActiveNodeHealthCheck` all match the contract. The package split is clean — Akka and EF Core stay out of the core package, so @@ -36,6 +58,23 @@ and missing `GenerateDocumentationFile` so the (otherwise excellent) XML docs do ## Checklist coverage +2026-11-09 re-review at `91fb2b4`: + +| # | Category | Examined | Notes | +|---|----------|----------|-------| +| 1 | Correctness & logic bugs | ☑ | `OldestUpMember`/`SelfIsActive`/`ResolveActiveRole` are correct for ties, non-Up states, role scoping, the no-Up-member case (Unhealthy) and restart incarnations (`UniqueAddress`). Gate/probe scope mismatch → Health-007. | +| 2 | Public API surface & compatibility | ☑ | 0.3.0 is a deliberate, documented behaviour break (standby 503, role-lacking node Unhealthy). `ActiveNodeDecision` was removed but was internal. New public `ClusterActiveNode` and `ActiveNodeHealthCheckOptions` are sealed/static and nullable-correct. The gate offers a single `role` where the probe offers `RolePreference` → Health-007. | +| 3 | Concurrency & thread safety | ☑ | Checks and gate hold no mutable state. Membership is read from a single `ClusterReadView` snapshot per call. No issues found. | +| 4 | Error handling & resilience | ☑ | Health-001/002 fixes hold: cluster access is wrapped and maps to Degraded (OCE excluded); the gate swallows to `false`. No issues found. | +| 5 | Security & secret handling | ☑ | Endpoints are anonymous by design. The `data` payload carries node addresses, roles and counts only. No issues found. | +| 6 | Performance & resource management | ☑ | One linear member scan per probe. No allocations of note and nothing disposable. No issues found. | +| 7 | Spec & shared-contract adherence | ☑ | Three tiers, status strings and JSON envelope match SPEC §3. The oldest-Up rule matches both Akka apps (OtOpcUa `RedundancyStateActor` and ScadaBridge `ActiveNodeEvaluator` delegate to it). No issues found. | +| 8 | Packaging, dependencies & project layout | ☑ | Version 0.3.0; Akka stays out of the core package. The NU1510 warning (explicit `HealthChecks.Abstractions` reference is now prunable) is cosmetic. No issues found. | +| 9 | Testing coverage | ☑ | 76 tests pass, including real-cluster `ClusterActiveNodeTests` (age order diverging from address order), startup safety, role resolution and the `data` writer shape. Untested: the `Leaving`/`WeaklyUp`/`Joining` exclusion and same-address restart incarnations (correct by code reading). A container-built `AddCheck` was probed in the scratchpad and resolves correctly (Degraded with no ActorSystem). | +| 10 | Documentation & XML docs | ☑ | Thorough and accurate, apart from the gate's "can never disagree" claim (Health-007). | + +
2026-06-01 baseline checklist (5f75cd4) + | # | Category | Examined | Notes | |---|----------|----------|-------| | 1 | Correctness & logic bugs | ☑ | Tier predicates, policy presets, and `ActiveNodeDecision` matrices are correct and match the spec tables. No logic defects found. | @@ -49,6 +88,8 @@ and missing `GenerateDocumentationFile` so the (otherwise excellent) XML docs do | 9 | Testing coverage | ☑ | Strong: TestServer tier tests, real SQLite EF tests, table-driven policy/decision tests, gRPC probe + timeout + cancellation. Gaps noted inline in findings, not raised separately. | | 10 | Documentation & XML docs | ☑ | Health-004, Health-006. Public API XML docs are thorough and accurate; the issues are recommended-tag drift and docs not being packed. | +
+ ## Findings ### Health-001 — Akka health checks throw (instead of returning Degraded) when cluster state is inaccessible @@ -253,3 +294,49 @@ CS1591 warnings surface on currently-undocumented members. **Resolution** Resolved in `544a6dd`, 2026-06-01 — set `true` (plus `NoWarn=CS1591` for test/non-packed members) in `Directory.Build.props`; verified `dotnet pack -c Release` now ships `lib/net10.0/*.xml` in all three nupkgs. + + + +### Health-007 — `AkkaActiveNodeGate` cannot express the probe's `RolePreference`, so gate and `/health/active` can disagree despite the documented guarantee + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Public API surface & compatibility | +| Status | Open | +| Location | `ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/AkkaActiveNodeGate.cs:46` (ctor `string? role`), `:14-17` (doc claim), vs `ZB.MOM.WW.Health/src/ZB.MOM.WW.Health.Akka/ActiveNodeHealthCheck.cs:114-115` | + +**Description** + +In 0.3.0 `ActiveNodeHealthCheck` resolves its scope from an ordered `RolePreference` list through +`ClusterActiveNode.ResolveActiveRole(self.Roles, …)`, so a fused node answers for `admin` and a +driver-only node answers for `driver`. `AkkaActiveNodeGate` accepts only a single fixed `string? role` +and calls `ClusterActiveNode.SelfIsActive(cluster, _role)` directly. Its XML remarks still promise that +"an endpoint gated by this type and the `/health/active` probe an orchestrator routes by can never +disagree about which node is in charge". That holds only when the probe is configured with zero or one +role. For the multi-role preference (the exact configuration OtOpcUa's `HealthEndpoints` uses, +`[admin, driver]`) no gate configuration matches the probe on every node. + +**Failure scenario** + +An app wires `/health/active` with `RolePreference = ["admin","driver"]` and gates its +`RequireActiveNode()` endpoints with `new AkkaActiveNodeGate(sp, "admin")`. On a driver-only pair, the +oldest `driver` member answers `/health/active` 200, so the load balancer routes traffic to it. The gate +evaluates the `admin` scope: the node has no `admin` role, so `SelfIsActive` returns false and every +gated request gets 503 from `ActiveNodeGateEndpointFilter`. The pair serves nothing. Using +`new AkkaActiveNodeGate(sp)` (unscoped) instead makes the gate follow the oldest member of the whole +mesh, which on a mixed mesh can be a different node from the probe's role-scoped answer. Today this is +latent: no family app constructs `AkkaActiveNodeGate`, because ScadaBridge and OtOpcUa use their own +evaluators over `ClusterActiveNode`. + +**Recommendation** + +Give `AkkaActiveNodeGate` an `ActiveNodeHealthCheckOptions` constructor, or factor the "resolve role, +then `SelfIsActive`" step into one shared `ClusterActiveNode` method that both the probe and the gate +call, so they share one scope definition. Until then, qualify the remarks: the guarantee holds only when +both are configured with the same single role or both unscoped. + +**Resolution** + +_Unresolved._ diff --git a/code-reviews/LocalDb/findings.md b/code-reviews/LocalDb/findings.md new file mode 100644 index 0000000..0230345 --- /dev/null +++ b/code-reviews/LocalDb/findings.md @@ -0,0 +1,399 @@ +# Code Review — LocalDb + +| Field | Value | +|-------|-------| +| Library | `ZB.MOM.WW.LocalDb/` | +| Packages | `ZB.MOM.WW.LocalDb`, `ZB.MOM.WW.LocalDb.Replication`, `ZB.MOM.WW.LocalDb.Contracts` | +| Component spec | `docs/plans/2026-07-17-localdb-design.md` (design doc; there is no `components/localdb/` directory, so no SPEC.md exists) | +| Shared contract | None (no `components/localdb/shared-contract/`); the wire contract is `src/ZB.MOM.WW.LocalDb.Contracts/Protos/localdb_sync.proto` | +| Status | Reviewed | +| Last reviewed | 2026-11-09 | +| Reviewer | Claude | +| Commit reviewed | `91fb2b4` | +| Open findings | 7 | + +## Summary + +The sync engine is carefully built, and its watermark and session machinery holds up under a close +read. The parts checked and found sound: + +- Seq assignment matches commit order, because SQLite has a single writer and the oplog is AUTOINCREMENT. +- Watermarks are MAX-guarded, the applier runs one `BEGIN IMMEDIATE` transaction per batch with a + savepoint per entry, and the applying guard is a row in the same transaction. +- Prune gaps fault the live session, and a rebuilt peer is detected (both directions). +- Snapshot debt is cleared only on the peer's ack, the inbox is bounded and the writer has two lanes. +- Takeover is newest-wins, and the DI extensions are idempotent. + +The DI wiring is covered by tests that build the container: `DependencyInjectionTests`, +`GrpcAdapterTests.AddZbLocalDbReplication_*` and `MaintenanceTests.AddZbLocalDbReplication_RegistersMaintenance_BothRoles` +build a provider and resolve the hosted services. No TryAdd-ordering or factory-cycle defect was found. + +The findings fall into three themes: + +1. **Shapes the apply path cannot converge on.** The applier writes with `INSERT OR REPLACE` and + treats every `SqliteException` as per-entry poison. That holds for the narrow schemas the two + consumers use today, but registration accepts several shapes that diverge or lose rows silently: + - FK-linked tables during a snapshot (LocalDb-002); + - secondary `UNIQUE` constraints (LocalDb-003); + - rowid-alias keys assigned on both nodes (LocalDb-005); + - column names containing `'` (LocalDb-006). +2. **A fail-closed option the snapshot path skips.** `FailClosedOnDrift` is never checked on the + snapshot path (LocalDb-004). +3. **The endpoint is unauthenticated, and the option docs suggest otherwise.** The library maps an + unauthenticated sync endpoint, and `ApiKey`'s doc says the key is "expected from the peer" + (LocalDb-001). Both current consumers (ScadaBridge, OtOpcUa `LocalDbSyncAuthInterceptor`) close + the hole with their own interceptors. + +Every finding except LocalDb-001 and LocalDb-007 was reproduced with a throwaway test against a +scratch copy of the library (not committed). The suite at `91fb2b4` passes: 216/216 offline (223 +with the 7 probes added). The known open items #16 (tombstone resurrection past `TombstoneRetention`) +and #26 (Microsoft.Data.Sqlite pool bump) are excluded. + +## Checklist coverage + +| # | Category | Examined | Notes | +|---|----------|----------|-------| +| 1 | Correctness & logic bugs | ☑ | Watermarks, ack and prune, horizon gap, rebuilt-peer detection, LWW tie-break and per-entry savepoints are correct. Defects in how the apply path handles some schema shapes: LocalDb-002, LocalDb-003, LocalDb-005, LocalDb-006 | +| 2 | Public API surface & compatibility | ☑ | Proto evolution is additive, with capability negotiation (`supports_partial_snapshot`) and `lib_schema_version` left at 1. On-disk schema upgrades v1→v3 are idempotent. No issues found | +| 3 | Concurrency & thread safety | ☑ | One writer task over two lanes, `_master` guarded by a lock, Interlocked/Volatile on the status fields, takeover is cancel-then-await. HLC `Next`/`Observe` are locked. No issues found | +| 4 | Error handling & resilience | ☑ | Fault teardown flushes the control lane, keepalive pings, backoff with a fast path for schema mismatch, maintenance never dies. The poison catch is broad enough to turn FK-ordering failures into permanent dead-letters: LocalDb-002 | +| 5 | Security & secret handling | ☑ | Table names are regex-gated, all values are parameterized, and `Synchronous` is allow-listed. The sync endpoint is unauthenticated by default and `ApiKey` is documented misleadingly: LocalDb-001. Column-name quoting is incomplete (local schema only, not peer-controlled): LocalDb-006 | +| 6 | Performance & resource management | ☑ | Channel, CTS, call and reader-task disposal checked on every exit path. Statements are prepared once per batch, depth is O(1), batches are byte-budgeted, the inbox is bounded. No issues found | +| 7 | Spec & shared-contract adherence | ☑ | Design §Auth says "ZB.MOM.WW.Auth.ApiKeys keyId/Bearer on the channel", but the lib only sends a header (LocalDb-001). Design §Error handling ("Clock skew … optionally fail-closed") is not applied to snapshots (LocalDb-004) | +| 8 | Packaging, dependencies & project layout | ☑ | Three packages, central package management, TreatWarningsAsErrors, InternalsVisibleTo limited to Replication and Tests. Version 0.3.5 is unpublished (#26). No issues found | +| 9 | Testing coverage | ☑ | Broad, and DI is covered by tests that build the container. No test has a schema with a secondary `UNIQUE`, a non-cascading FK under snapshot, a quoted column name, or a drifted snapshot row. Those are the gaps behind LocalDb-002 through LocalDb-006 | +| 10 | Documentation & XML docs | ☑ | XML docs are thorough. The README is stale in places (LocalDb-007) and the `ApiKey` XML doc is wrong (LocalDb-001) | + +## Findings + +### LocalDb-001 — Sync endpoint is unauthenticated by default, and `ApiKey` is documented as "expected from the peer" + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Security & secret handling | +| Status | Open | +| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/LocalDbSyncService.cs:85`, `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/ReplicationOptions.cs:11`, `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/DependencyInjection/ReplicationServiceCollectionExtensions.cs:77` | + +**Description** + +`ReplicationOptions.ApiKey` is documented as "API key presented to / expected from the peer; null +disables key auth". Only the first half is true: + +- `SyncBackgroundService` attaches `Authorization: Bearer ` to the outbound call. +- `LocalDbSyncService.Sync` never reads request headers. +- `MapZbLocalDbSync` maps the service with no authorization metadata. +- The library ships no interceptor or validator. + +So setting `ApiKey` on both nodes, the natural reading of the option, leaves the passive endpoint +open. The README's Ops notes say this, but the XML doc does not. The design doc (§Auth: "`ZB.MOM.WW.Auth.ApiKeys` keyId/Bearer on +the channel") promises enforcement on the channel. Both current consumers wrote their own +fail-closed interceptor (ScadaBridge, and OtOpcUa's `LocalDbSyncAuthInterceptor`). This is the +duplication the shared library exists to remove, and every future consumer has to rediscover it. + +The handshake makes an open endpoint easy to exploit. `SyncSession.HandshakeAsync` sends the +server's own `Handshake`, including every registered table name and its schema digest +(`SyncSession.cs:180`), before validating the caller's. A caller can therefore echo the digests +back and pass the fail-closed schema check without knowing the schema. + +**Failure scenario** + +A third consumer (or a new OtOpcUa/ScadaBridge host profile) calls `AddZbLocalDbReplication` + +`MapZbLocalDbSync`. It sets `LocalDb:Replication:ApiKey` on both nodes and does not add an +interceptor. Any client that can reach the gRPC port can then: + +1. Open `Sync`, read the server's Handshake, and reply with the same digests and + `last_applied_remote_seq = 0`. +2. Receive a full snapshot of every replicated table, which is complete data exfiltration. +3. Push `DeltaBatch` entries with a high HLC that overwrite or tombstone any replicated row. They + win LWW and are then replicated onward by nothing, because the 2-node topology does not + re-forward. +4. Repeatedly displace the legitimate peer's stream, because the guard is newest-wins + (`LocalDbSyncService.cs:152`). + +A different `node_id` in step 1 also makes the server reset its inbound watermark +(`SyncSession.cs:199`). + +**Recommendation** + +Move the interceptor both consumers already wrote into the library. When `ApiKey` is set, the +service should verify `Bearer` with a constant-time compare (`CryptographicOperations.FixedTimeEquals`) +and refuse with `Unauthenticated` otherwise. Make the endpoint fail closed when no key is +configured, unless the host opts out explicitly, for example with `AllowUnauthenticated = true`. Fix +the `ApiKey` XML doc either way. Both consumers could then delete their copies. + +**Resolution** + +_Unresolved._ + +### LocalDb-002 — Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/SnapshotStreamer.cs:79` | + +**Description** + +`SnapshotStreamer.SendAsync` streams tables in ordinal name order +(`.OrderBy(t => t.Name, StringComparer.Ordinal)`, line 79–81). Within a table it pages +`ORDER BY rv.pk_json` (line 196), which is text order: `{"id":10}` sorts before `{"id":2}`. + +The receiver applies through `LwwApplier` on a connection with `PRAGMA foreign_keys=ON`. An +immediate FK violation is a `SqliteException`, and `LwwApplier.cs:116` treats it as per-entry +poison: dead-letter, keep going. `SnapshotApplier.OnCompleteAsync` then moves the watermark to +`as_of_seq`, and the sender clears its snapshot debt on the `SnapshotAck`. Nothing ever re-sends the +dead-lettered rows. + +Registration explicitly allows these schemas. `EnsureNoInboundActionOnDelete` rejects only +CASCADE / SET NULL / SET DEFAULT, and README §Limitations recommends `NO ACTION`/`RESTRICT`. So a +replicated parent/child pair with a default FK is a supported configuration that the delta path +handles (seq order matches commit order) but the snapshot path does not. + +**Failure scenario** + +Reproduced in a scratch test: + +1. Register `z_parent(id PK)` and `a_child(id PK, parent_id REFERENCES z_parent(id))` on both nodes. +2. Insert `(1)` into `z_parent` and `(10, 1)` into `a_child` on A. +3. Run a full snapshot from A to an empty B, the rebuilt-peer or cap-prune path. + +Result on B: `parents=1 children=0`, one dead-letter `a_child: SQLite Error 19: 'FOREIGN KEY +constraint failed'`, watermark advanced to 2. A self-referencing table (`node(id, parent_id +REFERENCES node(id))` with rows 2 and 10→2) loses row 10 the same way. The child rows are gone from +B permanently, and the pair reports itself converged. + +No current consumer has an FK on a replicated table (per the 0.3.3 notes), so this is latent. The +first consumer to add one loses data on every snapshot resync. + +**Recommendation** + +Choose one: + +- Apply snapshot batches with `PRAGMA defer_foreign_keys = ON` inside the apply transaction, which + checks FKs at commit. This only helps if parent and child land in the same transaction, which + they do not today across batches. More robustly, apply the whole snapshot with FK enforcement off + (`PRAGMA foreign_keys=OFF` on the snapshot apply connection, set outside a transaction) and run + `PRAGMA foreign_key_check` at `SnapshotComplete` to log or dead-letter any true orphans. +- Order the snapshot topologically by FK dependency, parents first. Even then a self-referencing + table still needs deferral. + +Also add a snapshot test with a non-cascading FK, and consider not dead-lettering +`SQLITE_CONSTRAINT_FOREIGNKEY` silently on the snapshot path. + +**Resolution** + +_Unresolved._ + +### LocalDb-003 — `INSERT OR REPLACE` apply silently deletes a different row on a secondary `UNIQUE` conflict, so the pair diverges + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/LwwApplier.cs:340` | + +**Description** + +`BuildUpsert` emits `INSERT OR REPLACE INTO (...)`. When the incoming row collides with a +local row on a non-PK `UNIQUE` constraint, REPLACE deletes that other row, which has a different +primary key. The deletion: + +- does not fire the capture triggers (they are guarded by the applying flag, and `recursive_triggers` is off anyway); +- updates no `__localdb_row_version` row; +- is never shipped to the peer. + +LWW only ever compares versions of the key it was handed, so nothing detects the change. +`RegisterReplicated` (`SqliteLocalDb.cs:170`) checks PKs, BLOBs and cascading FKs, but not `UNIQUE` +indexes. + +**Failure scenario** + +Latent today: the ScadaBridge site tables registered for replication declare no secondary `UNIQUE` +constraint (checked 2026-11-09). Reproduced in a scratch test: + +1. Create `t(id INTEGER PRIMARY KEY, code TEXT UNIQUE)` and register it on both nodes. +2. While disconnected, insert `(1,'X')` on A and `(2,'X')` on B, for example the same tag or + name created on each node during a partition. +3. Reconnect and exchange deltas. + +A applies `(2,'X')`, and REPLACE drops row 1. B applies `(1,'X')`, and REPLACE drops row 2. Final +state: A = `{2}`, B = `{1}`. Each row is applied (`applied=1, dl=0`) and A's ledger still says +`{"id":1}` is live. The pair is permanently diverged. Any later snapshot only logs "ledger/table +divergence" and skips the row (`SnapshotStreamer.cs:114`). + +**Recommendation** + +Reject, or at least warn on, registration of tables that have non-PK `UNIQUE` indexes or +constraints (`pragma_index_list` where `origin IN ('u')` and the index is not the PK). Alternatively, +apply with an explicit `INSERT … ON CONFLICT() DO UPDATE SET …`. A unique conflict on +another column then aborts and becomes a visible dead-letter instead of a silent delete. Document +that secondary unique keys must be globally unique across both nodes. + +**Resolution** + +_Unresolved._ + +### LocalDb-004 — `FailClosedOnDrift` is bypassed on the snapshot path, so a future-stamped snapshot row moves the local HLC forward permanently + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Spec & shared-contract adherence | +| Status | Open | +| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/SnapshotStreamer.cs:249` | + +**Description** + +The drift guard (`MaxHlcDriftAhead` + `FailClosedOnDrift`) lives only in +`SyncSession.ApplyDeltaEntriesAsync` (`SyncSession.cs:599-628`). `SnapshotApplier.OnBatchAsync` +passes every snapshot row straight to `LwwApplier.ApplyBatchAsync`. It takes no +`ReplicationOptions`, so it cannot check drift. The applier then calls +`db.Clock.Observe(maxHlc)` (`LwwApplier.cs:155`), and `OnCompleteAsync` persists `last_seen_hlc`. +That value feeds clock recovery on every later open (`SqliteLocalDb.cs:69-75`). + +This contradicts README §Semantics ("With `FailClosedOnDrift`, each over-drift entry is +dead-lettered") and design §Error handling ("configurable max-drift guard (warn, optionally +fail-closed)"). + +**Failure scenario** + +Reproduced in a scratch test with `FailClosedOnDrift = true`. A peer whose clock was a year ahead +when it wrote a row serves a snapshot after a cap prune or rebuild. The receiving node writes 0 +dead-letters, applies the row, and its HLC jumps about 365 days ahead. Every later local write on +that node is stamped a year in the future, and the stamp survives restarts through `last_seen_hlc`. +From then on those writes beat every genuine write on the peer under LWW until wall-clock time +catches up. The operator enabled `FailClosedOnDrift` specifically to prevent this. + +**Recommendation** + +Give `SnapshotApplier` the options and the clock, and run the same per-row drift check as the delta +path: dead-letter over-drift rows when `FailClosedOnDrift`, otherwise warn once. Better, move the +check into `LwwApplier` so both paths share it. Add a snapshot-drift test. + +**Resolution** + +_Unresolved._ + +### LocalDb-005 — Auto-assigned rowid / AUTOINCREMENT keys collide across nodes and LWW silently drops one row + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Documentation & XML docs | +| Status | Open | +| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb/Internal/SqliteLocalDb.cs:197` | + +**Description** + +Registration accepts any explicit PK, including an `INTEGER PRIMARY KEY [AUTOINCREMENT]` rowid +alias that each node assigns on its own. LWW keys on `pk_json`, so two distinct rows inserted with +`NULL` ids on the two nodes are treated as two versions of the same row, and one is discarded. This +is inherent to LWW with locally-assigned keys. However, neither the README §Limitations nor the +`RegisterReplicated` docs say that replicated keys must be globally unique (GUIDs or node-scoped). +The README quickstart even uses `id INTEGER PRIMARY KEY`. + +**Failure scenario** + +Reproduced in a scratch test: + +1. Create `ev(id INTEGER PRIMARY KEY AUTOINCREMENT, msg TEXT)` and register it on both nodes. +2. Insert `('from A')` on A and `('from B')` on B, for example when both nodes of a pair log events + during a split. +3. Exchange deltas. + +Both nodes end with `1:from A` only. B's row is lost on both nodes, with no dead-letter or metric. + +**Recommendation** + +Document in README §Limitations and the `RegisterReplicated` XML docs that replicated PKs must be +globally unique, and that a rowid alias assigned on both nodes loses rows. Optionally log a warning +at registration when the sole PK column is an `INTEGER PRIMARY KEY` rowid alias. + +**Resolution** + +_Unresolved._ + +### LocalDb-006 — Column names containing `'` are captured but break the apply and snapshot SQL + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.LocalDb/src/ZB.MOM.WW.LocalDb.Replication/Internal/LwwApplier.cs:349` | + +**Description** + +`TriggerSqlGenerator` escapes both quote styles (`Ident` doubles `"`, `Literal` doubles `'`), so +capture works for any column name. The replication side builds JSON paths with +`JsonPath(column) => "'$.\"" + column.Replace("\"", "\\\"") + "\"'"` (`LwwApplier.cs:349`, +`SnapshotStreamer.cs:205`). That code escapes `"` but not `'`, so a column name containing an +apostrophe ends the SQL string literal early. Only table names are regex-gated at registration +(`SqliteLocalDb.cs:172`); column names come from `PRAGMA table_info` unchecked. The input is the +local schema rather than the peer, so this is a robustness issue, not an injection vector. + +**Failure scenario** + +Reproduced in a scratch test. For a table with a column `"it's"`, capture on A succeeds. Applying on +B fails `Prepare` with `SQLite Error 1: near "s": syntax error`. That is a `SqliteException` inside +the per-entry try, so every row of the table is dead-lettered and acked (`applied=0 dl=1`). If the +column is part of the PK, the snapshot page query also throws and faults the session on every +reconnect. + +**Recommendation** + +Escape `'` in `JsonPath`, for example by building the path text and then passing it through the same +`Literal`-style doubling. Alternatively, validate column names at registration with the same +identifier regex used for table names, so registration fails loudly. Add a quoted-column round-trip +test. + +**Resolution** + +_Unresolved._ + +### LocalDb-007 — README is stale on version, adoption, test count, `Synchronous` values and baseline scope + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Documentation & XML docs | +| Status | Open | +| Location | `ZB.MOM.WW.LocalDb/README.md:13` | + +**Description** + +Several README statements no longer match the code or the library's own `CLAUDE.md`: + +- line 13: "Packages (all `0.3.4`)". `Directory.Build.props` is `0.3.5`. +- line 21: "adopted by … OtOpcUa (Phase 1)". OtOpcUa has adopted Phases 1+2. +- line 228: `Synchronous` lists `OFF` / `NORMAL` / `FULL`. The validator also accepts `EXTRA` + (`LocalDbOptionsValidator.cs:14`). +- line 373: "see Limitations: baselining flags a full, all-tables snapshot". This contradicts the + 0.3.0 per-table behaviour described in the same README at lines 264–266 and 315. +- line 390: "199 tests". There are 216 at this commit. + +**Failure scenario** + +An operator re-enabling a table follows line 373 and trims every high-volume table, expecting an +all-tables snapshot. That is unnecessary work, and it obscures the real rule: only the baselined +table, an old peer, or a cap prune streams in full. A reader checking which version is current gets +0.3.4 from the README and 0.3.5 from the build. + +**Recommendation** + +Refresh those five lines when 0.3.5 is published. + +**Resolution** + +_Unresolved._ diff --git a/code-reviews/Overview/findings.md b/code-reviews/Overview/findings.md new file mode 100644 index 0000000..3433f91 --- /dev/null +++ b/code-reviews/Overview/findings.md @@ -0,0 +1,301 @@ +# Code Review — Overview + +| Field | Value | +|-------|-------| +| Library | `ZB.MOM.WW.Overview/` (hosted application, not a packed library) | +| Packages | none (app; `IsPackable=false`) | +| Component spec | `docs/plans/2026-07-22-overview-dashboard-design.md` (+ `docs/plans/2026-07-22-overview-dashboard-impl-plan.md`) | +| Shared contract | Consumes the `ZB.MOM.WW.Health` `ZbHealthWriter` JSON body (`/health/ready`, `/health/active`); mirrored in `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReport.cs` | +| Status | Reviewed | +| Last reviewed | 2026-11-09 | +| Reviewer | Claude | +| Commit reviewed | `91fb2b4` | +| Open findings | 5 | + +## Summary + +The dashboard meets its core requirements: + +- **No authentication, no credentials, no mutating endpoint.** The only interaction is a per-circuit + pause toggle. +- **No SSRF surface.** Probe URLs come only from validated configuration (absolute http/https). +- **Health-check JSON cannot inject markup.** The one `MarkupString` (cluster evidence line) is built + from `WebUtility.HtmlEncode`-escaped values. Everything else renders through Razor encoding. +- **Blazor state is marshalled and cleaned up.** Store-change notifications go through `InvokeAsync`, + and the page unsubscribes and disposes its timer. +- **Sweeps cannot overlap.** There is a single writer and an immutable snapshot swap. +- **DI is covered.** `BootTests` boots the real host through `WebApplicationFactory`, so the DI + wiring has container-level coverage. + +The suite passes: 188/188 tests. + +The one serious defect is error handling at the probe boundary, Overview-001. `ZbHealthReportClient` +catches only `HttpRequestException` and `OperationCanceledException`. Any other exception from one +instance's response faults the poller `BackgroundService`, and the default `StopHost` policy then +takes down the whole dashboard. This was verified end-to-end: a single registered endpoint answering +with an unknown `charset` stopped the host within one sweep. + +The other findings cover: + +- leader-chip mis-attribution when two cluster groups of one application share hosts, which is the + QA ScadaBridge layout (Overview-002); +- a status-only active probe that reads any 200, including a followed login redirect, as Active + (Overview-003); +- one slow instance gating the whole sweep (Overview-004); +- stale docs (Overview-005). + +Known open item, not a code finding: scadaproj #14 (re-run the two OtOpcUa-blocked Phase 4 +live-acceptance checks). + +## Checklist coverage + +| # | Category | Examined | Notes | +|---|----------|----------|-------| +| 1 | Correctness & logic bugs | ☑ | Status derivation, damping and staleness are correct and table-tested. See Overview-002 (leader resolution scans the whole application) and Overview-003 (any 200 = Active). | +| 2 | Public API surface & compatibility | ☑ | App, not a library. The `ZbHealthReport` mirror is tolerant (case-insensitive, open `data` as `JsonElement`). No issues found. | +| 3 | Concurrency & thread safety | ☑ | Single-writer poller. `PollTarget.Last` is written only after `Task.WhenAll`. `Volatile` snapshot swap. Blazor updates go through `InvokeAsync`. The `_paused` read off the render thread is benign. No issues found. | +| 4 | Error handling & resilience | ☑ | See Overview-001 (one bad response kills the host) and Overview-004 (one slow instance stales every card). | +| 5 | Security & secret handling | ☑ | Anonymous and read-only by design. No credentials in registry or code. `MarkupString` input is HTML-encoded. Management links and probe URLs come from config only and are scheme-validated. No issues found. | +| 6 | Performance & resource management | ☑ | One pooled `IHttpClientFactory` client per sweep. Per-request linked CTS disposed. Page timer and event handler released in `Dispose`. No issues found beyond Overview-004. | +| 7 | Spec & shared-contract adherence | ☑ | Matches design §2 (anonymous, cache-first) and the Health 0.3.0 `activeNode`/`leader` data keys. See Overview-005: the design and options docs say `/health/active` is probed only when `HasActiveRole`, but the code probes it unconditionally. | +| 8 | Packaging, dependencies & project layout | ☑ | Inline pins (family app convention). No Auth/Audit/Secrets references, as required. Package source mapping restricts the Gitea feed to `ZB.MOM.WW.*`. No issues found. | +| 9 | Testing coverage | ☑ | Strong: poller, client, derivation, leader resolver, page and card via bUnit, boot. Gaps: no test for a non-`HttpRequestException` failure from a probe (Overview-001), no cross-group same-host leader test (Overview-002), no redirect test (Overview-003). | +| 10 | Documentation & XML docs | ☑ | See Overview-005. The `deployment.md` §7 claim that QA's registry resolves leaders correctly is undermined by Overview-002. | + +## Findings + +### Overview-001 — Any non-`HttpRequestException` from one instance faults the poller and stops the whole dashboard + +| | | +|--|--| +| Severity | High | +| Priority | P1 | +| Category | Error handling & resilience | +| Status | Open | +| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReportClient.cs:99` (catch list `:122-134`; unguarded loop `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/OverviewPollerService.cs:86-95`) | + +**Description** + +The method promises it "never throws for an unreachable endpoint". It catches only +`OperationCanceledException`, `HttpRequestException` and, around deserialization, `JsonException`. + +Other exceptions reachable from a remote response escape `ProbeAsync`. The confirmed case is +`response.Content.ReadAsStringAsync` (`:99`), which throws **`InvalidOperationException`** when the +`Content-Type` charset is not a known encoding. + +From there the exception propagates up through each layer, none of which catches it: + +1. The probe's `ProbeAsync` task faults. +2. `Task.WhenAll` in `SweepAsync` rethrows it. +3. `ExecuteAsync` catches only `OperationCanceledException` when stopping. +4. The `BackgroundService` faults. +5. The host applies the default `HostOptions.BackgroundServiceExceptionBehavior = StopHost`. + +A malformed answer from any *one* registered instance therefore terminates the entire dashboard. This +contradicts design §5, which classes a non-JSON or failed answer as an **Unreachable** instance, +not a fatal error. + +**Failure scenario** + +A registered `BaseUrl` points at something that is not a healthy family app: a port reused by another +service, a misconfigured reverse proxy, or an app that emits `Content-Type: application/json; +charset=`. Verified end-to-end with `WebApplicationFactory` and a local +`HttpListener` answering `charset=bogus-xyz` for one of two instances. Within the first sweep the log +showed: + +- `[ERR] BackgroundService failed — System.InvalidOperationException: The character set provided in + ContentType is invalid` +- `[FTL] ... BackgroundServiceExceptionBehavior is configured to StopHost ... the IHost instance is + stopping` + +`ApplicationStopping` fired within 256 ms. On wonder, NSSM restarts the service, and it dies again on +its first sweep: a crash loop. Operators lose the fleet view because of a single bad entry. + +**Recommendation** + +- In `ZbHealthReportClient.SendAsync`, after the `OperationCanceledException` filters, add a final + `catch (Exception ex)` that returns `Failed(startedAt, statusCode?, ex.Message)`. +- Alternatively, read the body as bytes and decode as UTF-8 yourself, which removes the charset + dependency. +- Defence in depth: wrap each per-target `ProbeAsync` in `SweepAsync` so a throwing probe becomes an + `Unreachable` result for that instance only, and catch-and-log per sweep in `ExecuteAsync` so one + bad sweep cannot end the loop. +- Add a test with a handler returning an invalid charset, and one whose handler throws an arbitrary + exception. + +**Resolution** + +_Unresolved._ + +### Overview-002 — Leader address is resolved against every instance of the application, not the group, so co-located groups steal each other's chip + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/LeaderResolver.cs:85` (scan at `:150-166`) | + +**Description** + +`ResolveGroup` passes `allInstances` (every instance in the application) to +`ResolveToInstanceName`. That method returns the **first** instance in registry order whose +`BaseUrl` host equals the Akka address host. + +A group's leader is by definition one of that group's members, but the members are never searched +first. Whenever two groups of the same application share hosts, the group listed later gets the +earlier group's instance name on its "Leader ·" chip. No card in the right group then gets the Leader +badge: `IsLeader` compares within the group, and the resolved name belongs to the other group. + +`ZB.MOM.WW.Overview/CLAUDE.md` and `deployment.md` §7 describe this as a single-box problem (worked +around on VD03 with `127.0.0.1` BaseUrls) and state that "QA's registry can use real FQDNs" because +the hosts are distinct. On QA, however, ScadaBridge **central-a and site-a both run on vq03, and +central-b and site-b on vq04** (`deployment.md:52-53`), all inside one `ScadaBridge` application. + +**Failure scenario** + +On the QA dashboard (`vq03:5320` / `vq04:5320`), ScadaBridge's central group is listed first and the +site cluster's leader is on vq03. A scratch test of that exact layout gave: + +- `central: leaderInstance=central-a` +- `site: leaderInstance=central-a` + +So the site group header reads "Leader · central-a" (a node in a different Akka cluster), and neither +site card shows the Leader chip. This is the only environment with a real HA pair, which is where +operators read leadership during failover. This is inferred from the documented QA layout; the QA +overlay is deployed-only and was not read. + +**Recommendation** + +In `ResolveGroup`, resolve against `members` first and fall back to `allInstances` only if no member +matches. Add a `LeaderResolverTests` case with two co-located groups. Then revisit the CLAUDE.md / +`deployment.md` guidance: with the fix, the VD03 `127.0.0.1` workaround also becomes unnecessary. + +**Resolution** + +_Unresolved._ + +### Overview-003 — Status-only active probe treats any 200, including a followed redirect to a login page, as Active + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/ZbHealthReportClient.cs:112` (client config `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/PollingServiceCollectionExtensions.cs:25`) | + +**Description** + +`ProbeStatusAsync` (`bodyRequired: false`) returns `Reachable = true` with the HTTP status even when +the body is not health JSON. `StatusDerivation.ObserveActive` then maps 200 to `Active`. + +The named `HttpClient` keeps the default `AllowAutoRedirect = true`. If `/health/active` 302s to a +login page, which is how ASP.NET cookie auth answers an unauthenticated request, the client follows +the redirect and the final 200 HTML page is reported as **Active**. + +The ready tier is protected, because its body must parse. The active tier is not. + +**Failure scenario** + +A family app is redeployed with a fallback authorization policy that accidentally covers +`/health/active`. This shape has happened before: OtOpcUa's LocalDb sync POST was once answered by +the login 302. Both nodes of the pair now show the **Active** chip, and the raw-signals footer reads +`active 200`, while their true role is unknown. + +Scratch test: a handler returning `200 text/html login` gives +`ObserveActive(true, probe) == Active`. + +**Recommendation** + +Configure the probe handler with `AllowAutoRedirect = false` +(`ConfigurePrimaryHttpMessageHandler(() => new SocketsHttpHandler { AllowAutoRedirect = false })`) +so a 3xx maps to `Unknown`. Optionally, for a non-empty body that is not health JSON, treat 200 as +`Unknown`: the comment's legitimate case is an *empty* body. + +**Resolution** + +_Unresolved._ + +### Overview-004 — One slow instance gates the single sweep, so every other instance goes Stale + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Error handling & resilience | +| Status | Open | +| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Polling/OverviewPollerService.cs:120` (sequential ready + active at `:155-171`; validator `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Registry/OverviewOptionsValidator.cs:69-76`) | + +**Description** + +All due targets are probed in one `Task.WhenAll`, and the next sweep cannot start until the slowest +target finishes. `PeriodicTimer` coalesces missed ticks. Each target makes up to two sequential +requests (ready, then active), so a sweep can last up to `2 × max(effective Timeout)`. + +The validator checks `StaleAfter > PollInterval` per instance. It does not check that any instance's +`StaleAfter` exceeds the worst-case sweep duration imposed by *other* instances' timeouts. Per-instance +`TimeoutSeconds` overrides are explicitly supported. + +**Failure scenario** + +An operator gives a slow WAN site a `TimeoutSeconds: 30` override. That site then hangs, accepting +TCP but not answering. Every sweep now takes about 60 s. Every other instance, with the default +`StaleAfterSeconds: 45`, is re-polled only every ~60 s, so the whole dashboard flips to **Stale** +between polls even though everything else is healthy. + +Scratch test: with one instance whose handler delays 4 s, `SweepAsync` took **8.0 s** while the other +instance answered instantly. + +**Recommendation** + +Decouple targets: run each target on its own loop or task, or let the sweep publish as results +arrive and not wait for laggards past their own interval. At minimum, extend the validator: reject a +configuration where any instance's effective `StaleAfter` is ≤ `2 × max effective Timeout` across the +registry, naming the offending override. + +**Resolution** + +_Unresolved._ + +### Overview-005 — Docs say `/health/active` is probed only when `HasActiveRole`; the code probes it for every reachable instance + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Documentation & XML docs | +| Status | Open | +| Location | `ZB.MOM.WW.Overview/src/ZB.MOM.WW.Overview/Registry/OverviewOptions.cs:9` (also `:76-80`) | + +**Description** + +Two places say the active tier is gated on the role: + +- The `OverviewOptions` remarks (`:8-10`) say "GET `{BaseUrl}/health/ready`, plus + `{BaseUrl}/health/active` when `HasActiveRole`". +- `InstanceEntry.HasActiveRole` (`:76-80`) says single-instance gateways "are never probed for + `/health/active`". + +Design doc §3 (line 81) and §5 ("only when `HasActiveRole`") say the same. `OverviewPollerService.ProbeAsync` (`:162-171`) deliberately +probes the active tier **unconditionally** whenever ready answered. Only the *interpretation* is gated +on `HasActiveRole`, so that MxAccessGateway's session counts on the active tier are visible. + +**Failure scenario** + +An operator or reviewer relies on the documented behaviour, for example when sizing the probe load, +or when deciding that a gateway's active-tier auth or rate limit is irrelevant because "Overview never +calls it". They are surprised by twice the requests per gateway, or by gateway access-log noise. A +future maintainer may also "fix" the code to match the doc and silently remove the MXAccess session +row. + +**Recommendation** + +Update the `OverviewOptions` remarks, the `HasActiveRole` XML doc and design §3: the active tier is +probed whenever ready answers, and `HasActiveRole` controls only whether an Active/Standby badge is +shown. + +**Resolution** + +_Unresolved._ diff --git a/code-reviews/README.md b/code-reviews/README.md index 4243011..5749126 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,29 +40,34 @@ code-reviews/ ## Summary -6 of 6 libraries reviewed. 0 pending findings across all libraries. +10 of 10 libraries reviewed. 49 pending findings across all libraries. -Coverage gap: Secrets, LocalDb, GalaxyRepository and Overview have never been reviewed here, -and Auth/Health/Theme have changed substantially since their 2026-06-01 review → #15. +Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, +Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also +carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | Severity | Open findings | |----------|---------------| | Critical | 0 | -| High | 0 | -| Medium | 0 | -| Low | 0 | -| **Total** | **0** | +| High | 1 | +| Medium | 15 | +| Low | 33 | +| **Total** | **49** | ## Library Status | Library | Last reviewed | Commit | Open (C/H/M/L) | Open | Total | |---------|---------------|--------|----------------|------|-------| -| [Audit](Audit/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 5 | -| [Auth](Auth/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 6 | -| [Configuration](Configuration/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 4 | -| [Health](Health/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 6 | -| [Telemetry](Telemetry/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 8 | -| [Theme](Theme/findings.md) | 2026-06-01 | `5f75cd4` | 0/0/0/0 | 0 | 6 | +| [Audit](Audit/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 | +| [Auth](Auth/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/3 | 4 | 10 | +| [Configuration](Configuration/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 4 | +| [GalaxyRepository](GalaxyRepository/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/2/6 | 8 | 8 | +| [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/1 | 1 | 7 | +| [LocalDb](LocalDb/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/4/3 | 7 | 7 | +| [Overview](Overview/findings.md) | 2026-11-09 | `91fb2b4` | 0/1/1/3 | 5 | 5 | +| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/6/13 | 19 | 19 | +| [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/3 | 3 | 11 | +| [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/1/1 | 2 | 8 | ## Pending Findings @@ -75,14 +80,66 @@ description, location, recommendation — lives in the library's `findings.md`. _None open._ -### High (0) +### High (1) -_None open._ +| ID | Library | Title | +|----|---------|-------| +| Overview-001 | [Overview](Overview/findings.md) | Any non-`HttpRequestException` from one instance faults the poller and stops the whole dashboard | -### Medium (0) +### Medium (15) -_None open._ +| ID | Library | Title | +|----|---------|-------| +| Auth-007 | [Auth](Auth/findings.md) | User-bind mode never checks that the entry whose groups it returns is the account that bound | +| GalaxyRepository-001 | [GalaxyRepository](GalaxyRepository/findings.md) | Filter memo has no size limit: client-chosen filters retain unlimited memory until the next deploy | +| GalaxyRepository-002 | [GalaxyRepository](GalaxyRepository/findings.md) | Browse-subtree scoping hides the ancestors of an allowed subtree, so scoped lazy browse from the root is empty | +| LocalDb-001 | [LocalDb](LocalDb/findings.md) | Sync endpoint is unauthenticated by default, and `ApiKey` is documented as "expected from the peer" | +| LocalDb-002 | [LocalDb](LocalDb/findings.md) | Snapshot streams tables by name and rows by `pk_json` text, so FK children land before parents and are dead-lettered for good | +| LocalDb-003 | [LocalDb](LocalDb/findings.md) | `INSERT OR REPLACE` apply silently deletes a different row on a secondary `UNIQUE` conflict, so the pair diverges | +| LocalDb-004 | [LocalDb](LocalDb/findings.md) | `FailClosedOnDrift` is bypassed on the snapshot path, so a future-stamped snapshot row moves the local HLC forward permanently | +| Overview-002 | [Overview](Overview/findings.md) | Leader address is resolved against every instance of the application, not the group, so co-located groups steal each other's chip | +| Secrets-001 | [Secrets](Secrets/findings.md) | Calling `AddZbSecrets` before a SQL-Server extension silently cancels the SQL-Server store / hub replicator | +| Secrets-002 | [Secrets](Secrets/findings.md) | The SQL-Server extensions cannot pass the content root, so the pre-host store-path hole reopens in hub mode | +| Secrets-003 | [Secrets](Secrets/findings.md) | `${secret:}` expansion is undone by configuration reload | +| Secrets-010 | [Secrets](Secrets/findings.md) | Akka replication actor is created only when `ISecretStore` is first resolved; a node that never resolves it never replicates | +| Secrets-013 | [Secrets](Secrets/findings.md) | UI add/rotate/delete: if the audit write throws after the store write succeeded, the change is reported and audited as a failure and the resolver cache is not evicted | +| Secrets-017 | [Secrets](Secrets/findings.md) | `secret set`/`rotate` take the plaintext on argv; the host builder re-parses argv as configuration and can crash with the value in the error | +| Theme-007 | [Theme](Theme/findings.md) | `TechButton` silently discards a caller-supplied `disabled` attribute | -### Low (0) +### Low (33) -_None open._ +| ID | Library | Title | +|----|---------|-------| +| Auth-008 | [Auth](Auth/findings.md) | SQL Server store silently truncates over-length values and matches key ids case-insensitively, diverging from SQLite | +| Auth-009 | [Auth](Auth/findings.md) | Canonical username is the first value of a possibly multi-valued attribute, not the value the user matched | +| Auth-010 | [Auth](Auth/findings.md) | `ApiKeyVerifier` lets store exceptions escape despite its "only exception path is cancellation" contract | +| GalaxyRepository-003 | [GalaxyRepository](GalaxyRepository/findings.md) | Objects in a parent cycle disappear from `BrowseChildren` | +| GalaxyRepository-004 | [GalaxyRepository](GalaxyRepository/findings.md) | Stale threshold is hard-coded at 5 min regardless of the refresh interval, and stale projection defeats the memo | +| GalaxyRepository-005 | [GalaxyRepository](GalaxyRepository/findings.md) | Deploy-notifier bootstrap can deliver an older event after a newer one | +| GalaxyRepository-006 | [GalaxyRepository](GalaxyRepository/findings.md) | No test builds the container from `AddZbGalaxyRepository` | +| GalaxyRepository-007 | [GalaxyRepository](GalaxyRepository/findings.md) | Shipped package pulls a vulnerable `System.Security.Cryptography.Pkcs` 9.0.4 | +| GalaxyRepository-008 | [GalaxyRepository](GalaxyRepository/findings.md) | Stale documentation: `SnapshotCachePath` default, csproj comment, consumer matrix | +| Health-007 | [Health](Health/findings.md) | `AkkaActiveNodeGate` cannot express the probe's `RolePreference`, so gate and `/health/active` can disagree despite the documented guarantee | +| LocalDb-005 | [LocalDb](LocalDb/findings.md) | Auto-assigned rowid / AUTOINCREMENT keys collide across nodes and LWW silently drops one row | +| LocalDb-006 | [LocalDb](LocalDb/findings.md) | Column names containing `'` are captured but break the apply and snapshot SQL | +| LocalDb-007 | [LocalDb](LocalDb/findings.md) | README is stale on version, adoption, test count, `Synchronous` values and baseline scope | +| Overview-003 | [Overview](Overview/findings.md) | Status-only active probe treats any 200, including a followed redirect to a login page, as Active | +| Overview-004 | [Overview](Overview/findings.md) | One slow instance gates the single sweep, so every other instance goes Stale | +| Overview-005 | [Overview](Overview/findings.md) | Docs say `/health/active` is probed only when `HasActiveRole`; the code probes it for every reachable instance | +| Secrets-004 | [Secrets](Secrets/findings.md) | Corrupt or wrong-length crypto fields escape `Decrypt` as `ArgumentException`: not audited, and breaks the cipher contract | +| Secrets-005 | [Secrets](Secrets/findings.md) | Store-path rules are lexical: drive-relative paths pass "rooted", and `\\?\` / UNC / 8.3 / symlink spellings bypass "under content root" | +| Secrets-006 | [Secrets](Secrets/findings.md) | Resolver cache can re-cache a stale value after `Invalidate`, and expired plaintext is never evicted | +| Secrets-007 | [Secrets](Secrets/findings.md) | KEK is re-resolved on every access: `KekId` and key are read non-atomically, the work is repeated per op, and key copies are never zeroed | +| Secrets-008 | [Secrets](Secrets/findings.md) | Hub push sweep is not isolated per row, so one rejected row stops every later push | +| Secrets-009 | [Secrets](Secrets/findings.md) | README wiring example uses a relative `SqlitePath` that the validator rejects; empty or unterminated tokens pass through as literals | +| Secrets-011 | [Secrets](Secrets/findings.md) | `HandleAnnounceTick` publishes from a thread-pool continuation: announces carry no sender under an async store, and faults are silently dropped | +| Secrets-012 | [Secrets](Secrets/findings.md) | Akka anti-entropy replies are unchunked; a large store exceeds Akka.Remote's frame size and never converges | +| Secrets-014 | [Secrets](Secrets/findings.md) | UI "Add secret" silently overwrites an existing (or tombstoned) secret, clobbers its metadata, and audits it as `secret.add` | +| Secrets-015 | [Secrets](Secrets/findings.md) | Reveal shows the plaintext even when its audit write fails, and swallows the failure with no log | +| Secrets-016 | [Secrets](Secrets/findings.md) | The public UI components enforce no authorization of their own; gating relies entirely on the host rendering them inside `AuthorizeView` | +| Secrets-018 | [Secrets](Secrets/findings.md) | Bundle import aborts mid-run on one bad entry, leaving earlier rows applied but reporting only "Import failed" | +| Secrets-019 | [Secrets](Secrets/findings.md) | Hub-client cancellation surfaces as `RpcException(Cancelled)`, so shutdown is recorded as a failed sweep and the failover reader rotates through every endpoint with a cancelled token | +| Telemetry-009 | [Telemetry](Telemetry/findings.md) | Enrichers added via `ZbSerilogOptions.ConfigureLogger` run after `RedactionEnricher`, so their properties are never redacted | +| Telemetry-010 | [Telemetry](Telemetry/findings.md) | Redaction projection keys logged dictionaries by `ToString()`, silently dropping entries whose keys stringify identically | +| Telemetry-011 | [Telemetry](Telemetry/findings.md) | OTLP fail-fast validation accepts scheme-less `host:port` and is skipped by the pre-built-options `AddZbTelemetry` overload | +| Theme-008 | [Theme](Theme/findings.md) | `nav-state.js` "wired" guard is a DOM attribute that Blazor enhanced-navigation DOM sync strips, so toggle listeners accumulate and the transient active-link reveal is persisted | diff --git a/code-reviews/REVIEW-PROCESS.md b/code-reviews/REVIEW-PROCESS.md index 625e78e..a45fdec 100644 --- a/code-reviews/REVIEW-PROCESS.md +++ b/code-reviews/REVIEW-PROCESS.md @@ -3,7 +3,7 @@ This document describes how to perform a comprehensive, per-library code review of the shared libraries hosted in **scadaproj** and how to track findings to resolution. -scadaproj is an umbrella workspace, but it hosts first-party source: the six +scadaproj is an umbrella workspace, but it hosts first-party source: the `ZB.MOM.WW.*` shared libraries (the realized output of the [component normalizations](../components/README.md)). Those libraries are the code that gets reviewed here. A **library** is one self-contained solution at the repo root (its own @@ -14,7 +14,7 @@ The review unit is the **library**, not the individual NuGet package. A library several packages (Auth → 4, Health → 3, Telemetry → 2) is reviewed as a whole; findings point at the specific package/file in their **Location**. -### The six libraries and their design context +### The libraries and their design context Each library is reviewed against its normalized component spec. The library folder name is the `ZB.MOM.WW.` prefix stripped; note the spec directory is **not** always the same word. @@ -27,6 +27,10 @@ the `ZB.MOM.WW.` prefix stripped; note the spec directory is **not** always the | `Telemetry` | `ZB.MOM.WW.Telemetry/` | `Telemetry`, `.Serilog` | [`components/observability/`](../components/observability/) | | `Configuration` | `ZB.MOM.WW.Configuration/` | single | [`components/configuration/`](../components/configuration/) | | `Audit` | `ZB.MOM.WW.Audit/` | single | [`components/audit/`](../components/audit/) | +| `Secrets` | `ZB.MOM.WW.Secrets/` | `Abstractions`, `Secrets`, `Replicator.SqlServer`, `Replicator.AkkaDotNet`, `Replicator.Grpc`, `Ui`, `Cli` | [`components/secrets/`](../components/secrets/) | +| `LocalDb` | `ZB.MOM.WW.LocalDb/` | see library README | none — [`docs/plans/2026-07-17-localdb-design.md`](../docs/plans/2026-07-17-localdb-design.md) | +| `GalaxyRepository` | `ZB.MOM.WW.GalaxyRepository/` | see library CLAUDE.md | none (design doc 2026-06-23) | +| `Overview` | `ZB.MOM.WW.Overview/` | application (not a package) | none — see `ZB.MOM.WW.Overview/CLAUDE.md` | ## 1. Before you start @@ -104,6 +108,8 @@ using the entry format in [`_template/findings.md`](_template/findings.md). public-API / back-compat break, with no safe workaround. - **Medium** — incorrect or risky behaviour with limited impact or a workaround. - **Low** — minor issues, style, maintainability, documentation. +- **Priority** (added 2026-11-09) — `P1` (Critical/High), `P2` (Medium), `P3` (Low); the + triage rank used when filing the finding as a Gitea issue. - **Category** — one of the 10 checklist categories above. - **Location** — `file:line` (clickable), or a list of locations. - **Description** — what is wrong and why it matters. diff --git a/code-reviews/Secrets/findings.md b/code-reviews/Secrets/findings.md new file mode 100644 index 0000000..cd674ae --- /dev/null +++ b/code-reviews/Secrets/findings.md @@ -0,0 +1,671 @@ +# Code Review — Secrets + +| Field | Value | +|-------|-------| +| Library | `ZB.MOM.WW.Secrets/` | +| Packages | `ZB.MOM.WW.Secrets.Abstractions`, `ZB.MOM.WW.Secrets`, `ZB.MOM.WW.Secrets.Replicator.SqlServer`, `ZB.MOM.WW.Secrets.Replicator.AkkaDotNet`, `ZB.MOM.WW.Secrets.Replicator.Grpc`, `ZB.MOM.WW.Secrets.Ui`, `ZB.MOM.WW.Secrets.Cli` (tool) | +| Component spec | `components/secrets/` | +| Shared contract | `components/secrets/` (see also `ZB.MOM.WW.Secrets/docs/`) | +| Status | Reviewed | +| Last reviewed | 2026-11-09 | +| Reviewer | Claude (scadaproj#15) | +| Commit reviewed | `91fb2b4` | +| Open findings | 19 | + +## Summary + +First review of the library (scadaproj#15). **The cryptographic core is sound:** +- AES-256-GCM envelope with a fresh random DEK and fresh 96-bit nonces for every seal and every wrap. +- The secret name is bound as AAD on the body, and the KEK id on the wrapped DEK. +- The DEK is zeroed after use, and KEK rewrap is compare-and-swap. +- SQL identifiers are allow-listed and all SQL is parameterized. +- No plaintext or token reaches a log or an exception message. +- The gRPC hub interceptor compares the token in constant time, fails closed, and gates every handler shape. +- Bundles carry ciphertext only. + +The defects are in composition, configuration and the operator surfaces, not in the cipher: +- **SQL-Server DI wiring depends on call order (Secrets-001, Secrets-002).** `AddZbSecretsSqlServerStore` / `AddZbSecretsSqlServerReplication` depend on `TryAdd` ordering, so calling `AddZbSecrets` first silently cancels them. They also cannot pass `contentRootPath`, which reopens the pre-host store-path hole that 0.6.1 closed. In hub mode no registration order gets both right. +- **Config reload undoes `${secret:}` expansion (Secrets-003).** Expansion is written into the providers' in-memory data, so the next `appsettings.json` reload reverts it to the placeholder text. +- **Akka replication starts lazily (Secrets-010).** The replication actor exists only once something resolves `ISecretStore`, so a node that only uses the pre-host expander never replicates. +- **UI mis-reports a committed write (Secrets-013).** If the audit write fails after the store write succeeded, the UI reports and audits the change as a failure, and it skips cache eviction. +- **CLI takes plaintext on the command line (Secrets-017).** `secret set`/`rotate` accept the value as an argument, and the host builder re-parses it as configuration. +- **The rest are P3:** robustness, latent and documentation issues. + +Known open items are not re-reported: the SQLite concurrent-init fix (#25), the Microsoft.Data.Sqlite pool bump (#26) and the fate of Replicator.Grpc (#8). + +**Build and test** (macOS, `91fb2b4`): +- `Secrets.Tests`: 224 passed, 1 skipped (DPAPI). +- `Replicator.SqlServer.Tests`: 49 passed, 16 skipped (live SQL). +- The Akka, Grpc and Ui suites are green. + +Every DI extension has a test that builds the container. The gaps are in which orderings and paths those tests exercise: +- No test mixes `AddZbSecrets` with a SQL-Server extension, in either order (Secrets-001/002). +- The Akka hosted-resolution test resolves the store explicitly, which hides Secrets-010. + +The following were confirmed with throwaway probes in the session scratchpad: Secrets-001, Secrets-002 (with a positive control), Secrets-003, Secrets-004, Secrets-009 (token half), Secrets-010, Secrets-011 and Secrets-017. + +## Checklist coverage + +| # | Category | Examined | Notes | +|---|----------|----------|-------| +| 1 | Correctness & logic bugs | ☑ | Secrets-003 (expansion reverted on reload), Secrets-006 (stale re-cache after `Invalidate`), Secrets-010 (Akka actor created lazily), Secrets-013 (committed write reported as failed), Secrets-014 (Add overwrites), Secrets-018 (partial bundle import) | +| 2 | Public API surface & compatibility | ☑ | Secrets-001/002: the SQL-Server extensions depend on registration order and have no `contentRootPath` parameter. Secrets-016: the public Razor components do no authorization themselves | +| 3 | Concurrency & thread safety | ☑ | Secrets-006 (`Invalidate` racing an in-flight resolve), Secrets-007 (KEK id and key read non-atomically), Secrets-011 (`HandleAnnounceTick` publishes from a continuation and loses its sender). The actor otherwise captures `Self`/`Sender` before every await, and the interceptor counters use Interlocked | +| 4 | Error handling & resilience | ☑ | Secrets-004 (`ArgumentException` escapes `Decrypt`), Secrets-008 (push sweep not isolated per row), Secrets-019 (cancellation surfaces as `RpcException`) | +| 5 | Security & secret handling | ☑ | Crypto core sound (see Summary). Secrets-005 (lexical path-rule bypasses), Secrets-006 (expired plaintext kept), Secrets-007 (KEK never zeroed), Secrets-015 (reveal fails open on audit), Secrets-016, Secrets-017 (plaintext on argv). The hub endpoint allows `http`, so the bearer token can travel in cleartext; this appears to be a deliberate design choice and is not raised | +| 6 | Performance & resource management | ☑ | Secrets-007 (KEK re-read twice per operation), Secrets-012 (Akka replies unchunked, so they exceed the frame size). Connections, channels and CTSs are disposed correctly | +| 7 | Spec & shared-contract adherence | ☑ | Secrets-004 (`ISecretCipher.Decrypt` contract), Secrets-011 (the actor's "slower store" claim) | +| 8 | Packaging, dependencies & project layout | ☑ | No issues found | +| 9 | Testing coverage | ☑ | Container tests exist for every extension, but they miss mixed call order (Secrets-001/002), the lazy actor (Secrets-010), audit-throws in the UI (Secrets-013) and the `AddSecretsAuthorization` policy matrix (Secrets-016) | +| 10 | Documentation & XML docs | ☑ | Secrets-009 (the README sample fails validation). The docs do not say that an Akka node must resolve `ISecretStore` (Secrets-010) | + +## Findings + +### Secrets-001 — Calling `AddZbSecrets` before a SQL-Server extension silently cancels the SQL-Server store / hub replicator + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Public API surface & compatibility | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/DependencyInjection/SqlServerSecretsServiceCollectionExtensions.cs:53` (also `:54`, `:90`) | + +**Description** + +Both SQL-Server modes put their registrations in with `TryAddSingleton`, and they only take effect because they run before the extension's own internal `AddZbSecrets` call (`:57`, `:94`). If the application has already called `AddZbSecrets`, the SQLite `ISecretStore`, the SQLite `ISecretsStoreMigrator` and `NoOpSecretReplicator` are already registered. Every `TryAdd` then does nothing: +- `AddZbSecretsSqlServerStore` changes nothing at all. +- `AddZbSecretsSqlServerReplication` still decorates the store, but with the no-op replicator. + +There is no exception, no log line and no documentation of the order requirement. Calling `AddZbSecrets` first is the natural order: it is the order the README teaches, and it is the order an application uses to pass `contentRootPath` (see Secrets-002). + +**Failure scenario** + +The application calls `services.AddZbSecrets(cfg, "Secrets", contentRoot)` and then `services.AddZbSecretsSqlServerStore(cfg)`. The probe resolved `ISecretStore = SqliteSecretStore` and `ISecretsStoreMigrator = SqliteSecretsStoreMigrator`. Each node of the "shared-store" central pair therefore writes to its own local SQLite file. A secret set on node A is never visible on node B, and the hub database is never even migrated. In hub mode the probe resolved `ISecretReplicator = NoOpSecretReplicator`, so live publish is silently disabled and propagation waits for the sync interval. + +**Recommendation** + +Make the SQL-Server extensions order-independent: use `Replace` / `RemoveAll` + `Add` for `ISecretStore`, `ISecretsStoreMigrator` and `ISecretReplicator`, rather than `TryAdd`. Alternatively, throw if a conflicting registration is already present. Add container tests for both orders. + +**Resolution** + +_Unresolved._ + +### Secrets-002 — The SQL-Server extensions cannot pass the content root, so the pre-host store-path hole reopens in hub mode + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Security & secret handling | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/DependencyInjection/SqlServerSecretsServiceCollectionExtensions.cs:94` (with `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DependencyInjection/SecretsServiceCollectionExtensions.cs:76`) | + +**Description** + +Both SQL-Server extensions call the 3-arg `AddZbSecrets(config, section)`, which uses `contentRootPath: null`. The validator registration is first-wins (`SecretsServiceCollectionExtensions.cs:76`), so: +- If the SQL-Server extension runs first, a later 4-arg `AddZbSecrets(..., contentRoot)` cannot supply the content root. +- If the 4-arg call runs first, Secrets-001 disables the replicator. + +In a pre-host container (no `IHostEnvironment`), the under-content-root rule is therefore skipped in hub-replication mode, where the local SQLite store *is* migrated. This is the exact `0.6.0` hole that the 4-arg overload exists to close. The family guidance ("use the 4-arg overload at every pre-host call site") cannot be followed with these extensions, and there is no registration order that gets both behaviours right. + +**Failure scenario** + +The probe used hub mode with `Secrets:SqlitePath = /data/s.db`. It called `AddZbSecretsSqlServerReplication(cfg)` and then `AddZbSecrets(cfg, "Secrets", contentRoot)`, then resolved `SqliteSecretsStoreMigrator`, and **no validation error** was raised. The pre-host expander migrates an empty store inside the content root. The next deploy renames the directory, and the store is lost (the 2026-08-09 incident shape). Positive control: the same configuration with only the 4-arg `AddZbSecrets` throws `OptionsValidationException`. + +**Recommendation** + +Add a `string? contentRootPath` parameter to both SQL-Server extensions and forward it. Also make the validator registration "last explicit content root wins" rather than first-wins, for example by storing the root in an options object that each call may set. + +**Resolution** + +_Unresolved._ + +### Secrets-003 — `${secret:}` expansion is undone by configuration reload + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs:115` | + +**Description** + +`ExpandConfigurationAsync` writes the expanded value through `config[key] = …`. That only sets the value in the providers' in-memory `Data`. A file provider's `Load()` replaces `Data` on reload, so every expanded key reverts to its literal `${secret:…}` placeholder. Reload happens on `IConfigurationRoot.Reload()`, or on any `appsettings*.json` change when `reloadOnChange` is on, which is the default for `WebApplication.CreateBuilder` / `Host.CreateDefaultBuilder`. The XML docs and the README do not mention this. + +**Failure scenario** + +The probe used a JSON file `{"Cs":"pw=${secret:x}"}`, expanded it to `pw=hunter2`, and then called `cfg.Reload()`. `cfg["Cs"]` became `pw=${secret:x}`. In production, an operator edits a log level in `appsettings.json` on a running service. From then on, `IConfiguration` readers and `IOptionsMonitor`-rebound options see the placeholder text as the credential. For example, a connection string read per use contains `Password=${secret:sql/…}`, and logins fail until the process restarts. + +**Recommendation** + +Expand through a dedicated configuration provider or source that wraps the others and re-expands on the change token. Alternatively, add an in-memory overlay provider last and re-run expansion on `GetReloadToken()`. At minimum, document that reload must be disabled for expanded keys. + +**Resolution** + +_Unresolved._ + +### Secrets-004 — Corrupt or wrong-length crypto fields escape `Decrypt` as `ArgumentException`: not audited, and breaks the cipher contract + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Spec & shared-contract adherence | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Crypto/AesGcmEnvelopeCipher.cs:141` (resolver: `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DefaultSecretResolver.cs:114`) | + +**Description** + +`ISecretCipher.Decrypt` documents `SecretDecryptionException` as its only failure. `Decrypt` catches only `CryptographicException` (`:141`, `:146`). `AesGcm.Decrypt` throws `ArgumentException` for a wrong-length nonce or tag, or for a ciphertext/plaintext length mismatch, for example a `WrappedDek` that is not 32 bytes. `Rewrap` already handles this (`:191`, "ArgumentException = a malformed … row"), but `Decrypt` does not. `DefaultSecretResolver` catches only `SecretDecryptionException`, so the `decryption-failed` audit event is skipped. + +**Failure scenario** + +A row is truncated by a buggy or hostile replication peer, or by a hand-edited DB. The probe used a row with `Nonce = byte[5]` or `WrappedDek = byte[31]`, and `Decrypt` threw `System.ArgumentException`. `ISecretResolver.GetAsync` then throws a raw `ArgumentException` with no audit record, and callers that classify `SecretDecryptionException` misreport the cause. + +**Recommendation** + +Mirror `Rewrap`: catch `ArgumentException` in `Decrypt` and wrap it as `SecretDecryptionException`. Add a corrupt-length test. + +**Resolution** + +_Unresolved._ + +### Secrets-005 — Store-path rules are lexical: drive-relative paths pass "rooted", and `\\?\` / UNC / 8.3 / symlink spellings bypass "under content root" + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Security & secret handling | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretsStorePathRules.cs:57` (rule 2: `:90`–`:115`) | + +**Description** + + +- **Rule 1.** It uses `Path.IsPathRooted`, which on Windows returns `true` for drive-relative `C:secrets.db` and for root-relative `\secrets.db`. Neither is fully qualified. `C:secrets.db` resolves against the process's current directory on C:, which for a service is `C:\Windows\System32`. That is exactly the incident this rule documents. +- **Rule 2.** It compares `Path.GetFullPath` strings. `GetFullPath` does not normalise `\\?\` paths, does not expand 8.3 short names, and does not resolve symlinks or junctions. `\\?\C:\app\secrets.db`, `\\localhost\C$\app\secrets.db`, `C:\PROGRA~1\App\secrets.db`, or a path through a symlink into the content root (or a content root reached through a "current" symlink) all pass the check while sitting inside the content root. + +**Failure scenario** + +A Windows service with content root `C:\Program Files\App` is configured with `SqlitePath = C:\PROGRA~1\App\data\secrets.db`. Validation passes, the store lives inside the install directory, and the next deploy that renames that directory takes the store with it. Similarly, `SqlitePath = C:secrets.db` passes rule 1 and the store lands in `System32`. + +**Recommendation** + + +- Rule 1: use `Path.IsPathFullyQualified`. +- Rule 2: reject `\\?\`, `\\.\` and UNC spellings outright, or canonicalise both sides before comparing (`GetFinalPathNameByHandle` / `FileSystemInfo.ResolveLinkTarget(returnFinalTarget: true)` on the existing ancestor directory). + +**Resolution** + +_Unresolved._ + +### Secrets-006 — Resolver cache can re-cache a stale value after `Invalidate`, and expired plaintext is never evicted + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Concurrency & thread safety | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/DefaultSecretResolver.cs:120` (also `:92`, `:132`) | + +**Description** + +There are two problems: +1. **Stale write-back.** `GetAsync` reads the row (`:99`), decrypts it, and then unconditionally writes `_cache[key]` (`:120`). If a rotation or delete plus `Invalidate` (`:132`) lands between the read and the write, the old plaintext is re-inserted with a fresh full TTL. The `Revision` stored in the entry is never compared. +2. **No eviction.** Nothing ever removes expired entries. An entry is only overwritten by a later resolve of the same name. Plaintext for any secret resolved once, such as at startup expansion, stays in the dictionary for the life of the process. The class remarks say it "lives only briefly". + +**Failure scenario** + + +1. A request thread misses the cache and loads revision 3. The operator rotates a compromised credential to revision 4, and the UI calls `Invalidate`. The request thread then stores revision 3, which is served for another 30 s, or for `ResolveCacheTtl` if longer. +2. A heap dump taken hours after startup contains every startup-resolved secret in `_cache`. + +**Recommendation** + + +1. Use `AddOrUpdate` with a guard that refuses to replace a newer `Revision`, or keep a per-key invalidation generation counter that is checked before insert. +2. Remove the entry on an expired hit (`TryRemove` with the observed value), and/or sweep periodically. + +**Resolution** + +_Unresolved._ + +### Secrets-007 — KEK is re-resolved on every access: `KekId` and key are read non-atomically, the work is repeated per op, and key copies are never zeroed + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Security & secret handling | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Crypto/AesGcmEnvelopeCipher.cs:52` (also `:53`, `:113`, `:119`; `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/MasterKey/MasterKeyProviderBase.cs:35`, `:48`, `:60`) | + +**Description** + +`MasterKeyProviderBase` re-reads the source on every `GetMasterKey()` and, when no explicit id is configured, also on every `KekId` get. The source is the env var, the key file, or a DPAPI unprotect. This causes three problems: +- **Non-atomic pair.** `Encrypt` reads `KekId` (`:52`) and the key (`:53`) as two separate resolutions. A key file swapped between the two reads produces a row stamped with the old derived KEK id but wrapped under the new key. That row then fails `Decrypt` (id mismatch) and also fails `Rewrap` with the old key (tag mismatch). It is unrecoverable without hand-building a provider with the new key and the old id. +- **Repeated work.** Each encrypt or decrypt does two file reads, or two DPAPI unprotects. +- **No zeroing.** None of the returned 32-byte KEK arrays, or the intermediate base64/blob buffers, are ever zeroed. A copy of the KEK is left on the GC heap for every operation. This sits awkwardly with the class's careful DEK zeroing. + +**Failure scenario** + +An operator replaces the key file on a running node while a set or rotate is in flight, which the provider docs explicitly support ("a rotated file takes effect immediately"). One row ends up permanently undecryptable. + +**Recommendation** + +Have the provider return a single snapshot (key + id) per call. Use it once per cipher operation and zero it in `finally`. Optionally cache the snapshot keyed on file mtime. + +**Resolution** + +_Unresolved._ + +### Secrets-008 — Hub push sweep is not isolated per row, so one rejected row stops every later push + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Error handling & resilience | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.SqlServer/SqlServerSecretSyncService.cs:109` | + +**Description** + +The pull side isolates failures per row (`SecretReplicationReconciler.ApplyAsync`), precisely to stop a poison row from blocking a batch forever. The push loop (`:109`–`:114`) calls `_hub.ApplyReplicatedAsync` without isolation. The first failure aborts the sweep, and because the push set is recomputed in the same name order every interval, every name sorting after it is never pushed. The SQLite store accepts rows the hub rejects: +- `SecretName` has no length limit, but the hub column is `NVARCHAR(450)` (`SqlServerSecretsSchema.cs:73`). +- An explicit `MasterKey:KekId` has no length limit, but the hub column is `NVARCHAR(256)` (`:82`). + +**Failure scenario** + +In hub mode a node stores a secret with a 500-character name. The live publish fails and the failure is swallowed. Every sweep then fails with "String or binary data would be truncated" on that row, and every secret written on that node whose name sorts after it never reaches the hub or the other nodes. + +**Recommendation** + +Wrap each push in a try/catch with a per-row log, as the pull side does. Optionally enforce a maximum `SecretName` length (≤ 450) in the abstraction. + +**Resolution** + +_Unresolved._ + +### Secrets-009 — README wiring example uses a relative `SqlitePath` that the validator rejects; empty or unterminated tokens pass through as literals + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Documentation & XML docs | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/README.md:83` (token: `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets/Configuration/SecretReferenceExpander.cs:18`) | + +**Description** + + +- The README's canonical `appsettings.json` sets `"SqlitePath": "secrets.db"`, which `SecretsOptionsValidator` rejects at startup (relative path). A consumer copying the sample gets a boot failure. +- Separately, the expander says it is fail-closed rather than "leaving the placeholder in place". However, `TokenPattern` requires `[^}]+`, so `${secret:}` and an unterminated `${secret:name` are not matched. `ExpandConfigurationAsync` then leaves them verbatim. The probe confirmed that `"pw=${secret:}"` is returned unchanged. + +**Failure scenario** + +A typo such as `Password=${secret:}`, or a missing closing brace, boots successfully and uses the literal placeholder text as the credential, instead of failing at startup. + +**Recommendation** + +Change the README sample to an absolute path, or drop the key so the default applies. In the expander, throw when a value contains `${secret:` but the regex does not consume every occurrence. + +**Resolution** + +_Unresolved._ + + +### Secrets-010 — Akka replication actor is created only when `ISecretStore` is first resolved; a node that never resolves it never replicates + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/DependencyInjection/AkkaSecretsServiceCollectionExtensions.cs:79` | + +**Description** + +The actor is created only when `SecretReplicationActorProvider.ActorRef` is read: +- `SecretReplicationActorProvider` wraps `system.ActorOf(...)` in a `Lazy` (lines 128–132). +- The only code that reads `.ActorRef` is the `ISecretReplicator` factory (line 91). +- That factory is resolved only by the `ReplicatingSecretStore` factory (lines 99–102), which is the `ISecretStore` registration. + +No hosted service resolves any of these. The only hosted service registered is `SecretsMigrationHostedService`, which takes `ISecretsStoreMigrator` only. So until application code resolves `ISecretStore` (or `ISecretResolver`, which depends on it), there is no actor. Without the actor, this node: +- has no topic subscription and receives none of its peers' live pushes; +- sends no manifest announces; +- does not answer pull requests. + +The node is invisible to replication, and nothing logs that. + +**Failure scenario** + +On a 2-node pair, node B's host uses the pre-host `${secret:}` expander, which reads its own store instance. At runtime B never touches `ISecretStore`: nobody opens `/admin/secrets` on B, and there is no runtime resolve. An operator rotates `db/central` on node A. +- A's live push and every announce reach nothing on B. +- B's SQLite keeps the old value indefinitely. +- On B's next restart, or a failover onto B, the pre-host expander resolves the stale credential, and login fails. + +Verification: a throwaway probe project (not committed) builds the real graph and starts the hosted services. It printed `actor exists = False` before resolving `ISecretStore` and `True` after. `HostedProcessResolutionTests` hides the gap because it resolves the store explicitly. Its own comment says OtOpcUa's startup hook happens to resolve the store, so the known consumer is likely covered by accident. The library contract does not guarantee this. + +**Recommendation** + +Register a small `IHostedService` in `AddZbSecretsAkkaReplication` that reads `SecretReplicationActorProvider.ActorRef` in `StartAsync`. Do this after the ActorSystem exists; resolving the provider does not re-enter the scadaproj#1 cycle, because the invalidator is still deferred. Add a test that starts the hosted services and asserts that `/user/` exists without anything resolving `ISecretStore`. Failing that, document that hosts must resolve `ISecretStore` at startup. + +**Resolution** + +_Unresolved._ + + +### Secrets-011 — `HandleAnnounceTick` publishes from a thread-pool continuation: announces carry no sender under an async store, and faults are silently dropped + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Concurrency & thread safety | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/SecretReplicationActor.cs:196` | + +**Description** + +`HandleAnnounceTick` (lines 194–211) calls `_store.GetManifestAsync(...).ContinueWith(..., ExecuteSynchronously, TaskScheduler.Default)` and calls `_mediator.Tell(new Publish(...))` inside the continuation (line 202), with no explicit sender. When the task has not already completed, the continuation runs on a pool thread with no actor context. The implicit sender is then `NoSender`, and receiving peers see `Sender == deadLetters`. + +`HandleManifestAnnounce` then captures `peer = Sender` (line 145). It sends its push rows and its `SecretPullRequest` to dead letters, so that half of the exchange is lost. + +This is the same class of bug that the class's own comment (lines 216–225) says was fixed in `ReconcileWithPeerAsync`. The tick path was missed. + +Separately, the continuation only checks `IsCompletedSuccessfully`. A faulted manifest read is never logged and its exception is never observed, so a node whose store cannot produce a manifest stops announcing and logs nothing. + +**Failure scenario** + +Verification: a throwaway probe project (not committed) subscribed a probe to the topic. +- With a synchronous store, the announce sender was `akka://s/user/sync-node`. +- With a store whose `GetManifestAsync` awaits a 5 ms delay, the sender was `akka://s/deadLetters`. + +With two genuinely async stores, anti-entropy never converges in either direction; only the best-effort live push works. Today the production store is `SqliteSecretStore`, whose Microsoft.Data.Sqlite async calls complete synchronously, and the existing async-store test still passes through the peer's own announce. That is why this is latent. `SecretReplicationActor.Props` is public and accepts any `ISecretStore`. + +**Recommendation** + +Capture `IActorRef self = Self` in `HandleAnnounceTick`. Then either `PipeTo(self)` a private "manifest ready" message and publish it from the mailbox, or call `_mediator.Tell(new Publish(...), self)`. Add an `OnlyOnFaulted` warning, as `HandleRows` already does. Extend the async-store test so that both nodes are async. + +**Resolution** + +_Unresolved._ + + +### Secrets-012 — Akka anti-entropy replies are unchunked; a large store exceeds Akka.Remote's frame size and never converges + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Performance & resource management | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.AkkaDotNet/SecretReplicationActor.cs:242` | + +**Description** + +`ReconcileWithPeerAsync` sends every row the peer is behind on as one `SecretRowsMessage` (line 242). `HandlePullRequest` answers an entire pull as one `SecretRowsMessage` (lines 184–186), and the pull itself is one `SecretPullRequest` listing every name (line 251). + +`SecretReplicationReconciler.ReconcileAsync` deliberately chunks at 500 names, and its comment says this is because a cold node's pull set is the peer's entire inventory and an unbounded message "breaks on real backends". The Akka transport bypasses that batching. + +The rows are STJ JSON with six base64 byte arrays, a `kek_id`, timestamps and names, so a row is a few hundred bytes. At the default Akka.Remote `maximum-frame-size` of 128000 bytes, that caps out at a few hundred rows. + +**Failure scenario** + +A node joins cold, or rejoins after its store was rebuilt, against a peer that holds a few hundred secrets. Every announce, the peer builds one oversized `SecretRowsMessage`. Akka.Remote discards it as an oversized payload. The same message is rebuilt on the next interval, so the cold node never receives any row. It recovers only for rows that are written again later. + +**Recommendation** + +Chunk the push and the pull-reply into messages of at most N rows (for example `SecretReplicationReconciler.FetchBatchSize`, or a smaller row-count constant). Optionally chunk `SecretPullRequest` too. Document the frame-size bound in `clustered-secrets.md`. + +**Resolution** + +_Unresolved._ + + +### Secrets-013 — UI add/rotate/delete: if the audit write throws after the store write succeeded, the change is reported and audited as a failure and the resolver cache is not evicted + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/SecretEditor.razor:138` (also `ConfirmDeleteModal.razor:91`) | + +**Description** + +In `SecretEditor.SubmitAsync`, `Store.UpsertAsync` (line 138) and the success `Audit.WriteAsync` (lines 140–151) sit inside the same `try`. Its `catch (Exception)` (lines 155–163) treats any throw as a failed mutation: +- it sets `_error = "Unable to add/rotate the secret."`; +- it writes a **Failure** audit event; +- it leaves `saved == false`, so the `ISecretCacheInvalidator.Invalidate` and `OnSaved` block (lines 170–178) is skipped. + +`ConfirmDeleteModal.ConfirmAsync` has the identical shape: `DeleteAsync` at line 91, audit at lines 93–104, catch at lines 108–114, and eviction skipped at lines 121–126. No UI test makes the audit writer throw; `CapturingAuditWriter` never fails. + +**Failure scenario** + +The audit sink (for example a SQL-backed `IAuditWriter`) is briefly unavailable. An operator rotates `db/central` to revoke a leaked credential. +- The new ciphertext is committed and, on a replicated host, already published. +- The UI says the rotation failed, and the audit trail records `secret.rotate` / **Failure**. +- The local resolver keeps serving the old plaintext for the rest of its cache TTL. + +The operator may retry, creating a second revision, or believe the secret is unchanged. A delete behaves the same way: the secret is tombstoned, yet it is reported as not deleted and keeps resolving from cache. + +**Recommendation** + +Move the success audit out of the mutation `try`. Once the store write returns, set `saved = true` and invalidate the cache. Then write the success audit in its own best-effort try/catch, and log when it fails. Add bUnit tests with a throwing `IAuditWriter` for both components. + +**Resolution** + +_Unresolved._ + + +### Secrets-014 — UI "Add secret" silently overwrites an existing (or tombstoned) secret, clobbers its metadata, and audits it as `secret.add` + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/SecretEditor.razor:115` | + +**Description** + +In add mode, `name = new SecretName(_name)` (line 115) is passed straight to `Cipher.Encrypt` and `Store.UpsertAsync` (lines 131–138). The store's upsert is `ON CONFLICT(name) DO UPDATE`, which overwrites the value and also clears any tombstone. Nothing checks whether the name already exists. + +The rotate path was specifically changed to preserve the existing description and content type (lines 94–104), because the upsert overwrites them. Add-on-existing-name bypasses that protection: the description becomes `null` and the content type becomes the form default `Text`. The event is then audited as `secret.add`. + +**Failure scenario** + +An operator types an existing name, such as `db/central`, into "Add secret", perhaps by copy-paste from another environment. The live credential is replaced without any prompt, and its description and content type are reset. A `ConnectionString` secret becomes `Text`, and consumers that branch on content type may now mis-handle it. The audit says "add" rather than "rotate", which misleads anyone reviewing it. + +**Recommendation** + +In add mode, call `Store.GetAsync(name)` first. If a live row exists, refuse with "already exists — use Rotate". If a tombstone exists, require an explicit confirmation to restore. + +**Resolution** + +_Unresolved._ + + +### Secrets-015 — Reveal shows the plaintext even when its audit write fails, and swallows the failure with no log + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Security & secret handling | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/RevealButton.razor:108` | + +**Description** + +`RevealAsync` decrypts and sets `_revealed = true` (lines 65 and 88) before writing the `secret.reveal` audit (lines 91–106). The audit is wrapped in a bare `catch { }` (lines 108–111) that neither logs nor hides the value. + +Reveal is the most sensitive operation in this UI: the policy doc calls it "strictly more privileged" and audited. If the audit fails, the plaintext is still displayed and nothing records that it happened. + +There is a second gap: if the component is disposed between the store read and the audit, `_cts.Token` (line 106) throws `ObjectDisposedException`, which the same bare catch swallows. That reveal was decrypted but never audited. + +**Failure scenario** + +The audit sink is down, or someone with Reveal rights deliberately makes it fail. A user with `secrets-reveal` reveals production credentials one after another. Every value is displayed and there is no audit row and no log line, so the "audited reveal" guarantee is silently fail-open. + +**Recommendation** + +Choose the policy explicitly. Either: +- fail closed: write the audit first, and show the value only if the write succeeds; or +- keep it fail-open, but log a Warning or Error naming the actor and the target (never the value) when the audit write fails. + +Add a test for a throwing audit writer. + +**Resolution** + +_Unresolved._ + + +### Secrets-016 — The public UI components enforce no authorization of their own; gating relies entirely on the host rendering them inside `AuthorizeView` + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Public API surface & compatibility | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Ui/Components/RevealButton.razor:50` (also `SecretEditor.razor:108`, `ConfirmDeleteModal.razor:80`) | + +**Description** + +`RevealButton`, `SecretEditor` and `ConfirmDeleteModal` are public Razor components in `ZB.MOM.WW.Secrets.Ui.Components`. Their handlers (`RevealAsync`, `SubmitAsync`, `ConfirmAsync`) decrypt, write or tombstone without evaluating `SecretsAuthorization.RevealPolicy` or `ManagePolicy`. The gating exists only in `SecretsList.razor`, which wraps them in `AuthorizeView` (lines 64 and 67), and on `SecretsPage` (`[Authorize(ManagePolicy)]`). + +Separately, the role matrix in `AddSecretsAuthorization` (`SecretsAuthorization.cs:66-79`) has no test. The bUnit tests use `auth.SetPolicies(...)`, which bypasses the real policy definitions. So nothing pins the security-critical property that `secrets-manager` does **not** satisfy `RevealPolicy`. + +**Failure scenario** + +A host composes `` into its own page, for example an equipment-config page under a lower-privilege policy, and does not wrap it in `AuthorizeView Policy=RevealPolicy`. Any user who can view that page can then reveal plaintext. + +A future edit that adds `ManageRole` to the reveal policy would also pass every existing test. + +**Recommendation** + +Inject `IAuthorizationService` into each component and re-check the policy at the top of each mutating or revealing handler, failing closed. Add an `AuthorizationOptions` test that builds the two policies and evaluates each role against each policy. + +**Resolution** + +_Unresolved._ + + +### Secrets-017 — `secret set`/`rotate` take the plaintext on argv; the host builder re-parses argv as configuration and can crash with the value in the error + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Security & secret handling | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Program.cs:21` (values consumed at `:57`, `:65`) | + +**Description** + +The headless verbs take the secret value as a positional argument: `set ` and `rotate ` (lines 57 and 65). There is no stdin or masked-prompt alternative. + +The `rewrap-all` path deliberately refuses literal key material, "which would be echoed/logged" (lines 91–92). Secret values get no such protection: they land in shell history and are visible in the process list (`ps`, or Windows process command lines in EDR telemetry) to any local user for the duration of the run. + +`Host.CreateApplicationBuilder(args)` (line 21) also feeds the same argv into the command-line configuration provider: +- A value containing `=` becomes a configuration key/value pair. +- A value starting with `-` and containing `=` throws an unhandled `FormatException`. That happens before the verb's `catch` is set up, and the exception message contains the whole argument. + +**Failure scenario** + +`secret set db/pw "-p=Hunter2Secret"`, run from a throwaway probe (not committed), prints: + +```text +Unhandled exception. System.FormatException: The short switch '-p=Hunter2Secret' is not defined in the switch mappings. + at Microsoft.Extensions.Configuration.CommandLine.CommandLineConfigurationProvider.Load() +``` + +The secret goes to stderr, which CI logs, deploy-script transcripts and the WinRM output capture all retain. The CLI exits with an unhandled crash rather than its usage code. + +Even without the crash, every `secret set` leaves the plaintext in `~/.zsh_history`, PSReadLine history, or the equivalent. + +**Recommendation** + +- Accept the value from stdin (`-` or `--value-stdin`) or a masked prompt when stdin is a TTY, and document that as the preferred form. +- Build the host with `Host.CreateApplicationBuilder(new HostApplicationBuilderSettings { Args = [] })`, or strip the verb arguments, so argv is never parsed as configuration. +- Wrap the host build so a configuration exception never echoes argv. + +**Resolution** + +_Unresolved._ + + +### Secrets-018 — Bundle import aborts mid-run on one bad entry, leaving earlier rows applied but reporting only "Import failed" + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Error handling & resilience | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Cli/Interactive/BundleService.cs:160` | + +**Description** + +`ImportAsync` applies rows one at a time (lines 147–211), outside any transaction. If entry #i fails, the method throws `InvalidOperationException` and loses its running tally: +- a malformed entry, such as bad base64 or an unknown content type, throws at line 160; +- a `SecretDecryptionException` (a subtype of `InvalidOperationException`) from `cipher.Rewrap` (line 169), for example a tampered wrap, also throws. + +Entries 0..i-1 have already been written with `ApplyReplicatedAsync`/`UpsertAsync`. `BundleImportFlow` catches the exception and prints only `Import failed: …` (`BundleImportFlow.cs:129-133`). It does not render a report, so the operator cannot tell that some rows were imported, or which. + +**Failure scenario** + +An operator imports a 40-row bundle whose entry #25 was hand-edited and corrupted. Rows 0–24 are written, which may include conflict winners that overwrote local values, and the console says "Import failed". The operator assumes nothing changed, fixes the bundle and re-imports with the per-conflict prompt. They are now asked about 25 "conflicts" that the first run created, with no record of which local values were overwritten. + +**Recommendation** + +Validate the whole bundle first: rehydrate every entry with `ToRow`, and check that every foreign-KEK row can be rewrapped, before any write. Alternatively, return a partial report: catch per entry, record the failure, and continue or stop, but always render what was written. + +**Resolution** + +_Unresolved._ + + +### Secrets-019 — Hub-client cancellation surfaces as `RpcException(Cancelled)`, so shutdown is recorded as a failed sweep and the failover reader rotates through every endpoint with a cancelled token + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Error handling & resilience | +| Status | Open | +| Location | `ZB.MOM.WW.Secrets/src/ZB.MOM.WW.Secrets.Replicator.Grpc/GrpcSecretSyncService.cs:176` (also `FailoverSecretsHubReader.cs:114`, `DependencyInjection/SecretsGrpcHubClientExtensions.cs:89`) | + +**Description** + +The hub channels are created with `GrpcChannel.ForAddress(endpoint)` and default options (`SecretsGrpcHubClientExtensions.cs:89-90` and `:115-117`). That leaves `GrpcChannelOptions.ThrowOperationCanceledOnCancellation` at its default of `false`, so a call cancelled through its `CancellationToken` throws `RpcException` with `StatusCode.Cancelled`, not `OperationCanceledException`. This contradicts two pieces of code: +- **`GrpcSecretSyncService.SweepAndRecordAsync`** (line 176) only treats `OperationCanceledException` as a shutdown. A cancelled in-flight sweep falls through to the catch-all (lines 180–189), which increments `ConsecutiveFailures`, sets `LastError`, and logs the "Secret hub sync failed" warning. +- **`FailoverSecretsHubReader`'s remarks** (lines 28–33) state that `OperationCanceledException` "is the host stopping — dialing another endpoint with a cancelled token would only delay shutdown" and propagates. In practice the `catch (RpcException)` (line 114) catches the cancellation, logs "did not answer (Cancelled); failing over" and tries every remaining endpoint with the same cancelled token. + +**Failure scenario** + +A site host is stopped while a sweep is in flight, which is likely on a cold start against a slow hub with `CallDeadline = 30s`. The log gets a spurious "failing over to …" warning per fallback endpoint plus a "sync failed" warning. Anything reading `ISecretsHubSyncStatus` in its last moments, such as a readiness probe or a shutdown health snapshot, reports a failure that did not happen. + +**Recommendation** + +Either: +- create the channels with `new GrpcChannelOptions { ThrowOperationCanceledOnCancellation = true }`; or +- in both places, treat `RpcException { StatusCode: Cancelled }` as cancellation when `ct.IsCancellationRequested`, rethrowing in the failover reader and returning quietly in the sweep. + +Add a test that cancels mid-call against the in-process harness. + +**Resolution** + +_Unresolved._ diff --git a/code-reviews/Telemetry/findings.md b/code-reviews/Telemetry/findings.md index 7d7311e..e9ccb17 100644 --- a/code-reviews/Telemetry/findings.md +++ b/code-reviews/Telemetry/findings.md @@ -7,14 +7,40 @@ | Component spec | `components/observability/spec/SPEC.md` | | Shared contract | `components/observability/shared-contract/ZB.MOM.WW.Telemetry.md` | | Status | Reviewed | -| Last reviewed | 2026-06-01 | -| Reviewer | Claude (automated baseline) | -| Commit reviewed | `5f75cd4` | -| Open findings | 0 | +| Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) | +| Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) | +| Commit reviewed | `91fb2b4` | +| Open findings | 3 | ## Summary -The library is small, focused, and well-structured: two packages with a clean Serilog/OTel +**2026-11-09 re-review (`91fb2b4`, scadaproj#15).** Re-reviewed at `91fb2b4` (2026-11-09) against the 2026-06-01 baseline `5f75cd4`. Source is at `0.2.0`, +which is built but not published; the feed still carries `0.1.0`. Changes since the baseline: + +- nested redaction (`05cc62a`): `RedactionEnricher` projects `StructureValue`/`SequenceValue`/ + `DictionaryValue` into mutable views, honours removals, and writes back only what changed; +- one canonical Resource-attribute map (`ZbResource.BuildAttributes`) shared by the OTel SDK and the + Serilog OTLP sink; +- an eager OTLP options validator; +- the 0.2.0 `ZbSerilogOptions` hooks (`MinimumLevelGovernor`, `ConfigureLogger`). + +The baseline redaction fixes (Telemetry-001/002/003) hold: removal and nested masking work, and are +tested. The Resource map is genuinely single-sourced. The DI extensions have container-building tests +(`AddZbTelemetryTests`, `MultiHostTests`, `MinimumLevelGovernanceTests`). All 42 tests pass (Serilog 30, +core 12). + +There are three new Low findings, each verified with a scratchpad probe: + +- **Telemetry-009:** enrichers added through the new `ConfigureLogger` hook run after + `RedactionEnricher`, so properties they add are never redacted. +- **Telemetry-010:** the redaction projection keys logged dictionaries by `ToString()`. Distinct keys + that stringify the same, such as `DateTime` keys differing only by milliseconds, collapse, and an + entry is silently dropped from the log even when the redactor changes nothing. +- **Telemetry-011:** the Telemetry-006 fail-fast validator accepts a scheme-less `host:port` endpoint + as an "absolute URI", and the pre-built-options `AddZbTelemetry(IServiceCollection, ZbTelemetryOptions)` + overload skips validation entirely. + +**2026-06-01 baseline (`5f75cd4`).** The library is small, focused, and well-structured: two packages with a clean Serilog/OTel boundary (the `Serilog.*` stack appears only in the `.Serilog` package; the core package is pure OTel + ASP.NET Core framework reference), correct argument validation, deliberate `sealed` types, thorough XML docs, and a deliberate no-process-global-state design for @@ -34,6 +60,23 @@ happy paths but have no coverage for redactor removal or structured-value redact ## Checklist coverage +2026-11-09 re-review at `91fb2b4`: + +| # | Category | Examined | Notes | +|---|----------|----------|-------| +| 1 | Correctness & logic bugs | ☑ | Nested projection/rebuild, removal reconciliation and change detection (`ValueEquals`) are correct, except that dictionary keys collide on `ToString()` → Telemetry-010. Governor ordering (after `ReadFrom.Configuration`) works as documented. | +| 2 | Public API surface & compatibility | ☑ | Additive: `ZbResource.BuildAttributes` (public), the `ZbSerilogOptions` overload, and `InternalsVisibleTo` for the Serilog package. The options-object `AddZbTelemetry` overload skips the new validation → Telemetry-011. | +| 3 | Concurrency & thread safety | ☑ | Enricher state is a `Lazy` with `ExecutionAndPublication`, and projection is per event. No issues found. | +| 4 | Error handling & resilience | ☑ | The validator gives named `ArgumentException`s, but its absolute-URI test is too permissive and is bypassed by one overload → Telemetry-011. | +| 5 | Security & secret handling | ☑ | Redaction reach is now top-level plus nested, with removal honoured. Properties added by `ConfigureLogger` enrichers bypass redaction → Telemetry-009. `Exception` text remains outside the redactor seam, by design and unchanged. | +| 6 | Performance & resource management | ☑ | The per-event projection allocates only when a redactor is registered and the event has properties. Untouched properties are not reallocated. No issues found. | +| 7 | Spec & shared-contract adherence | ☑ | Resource attributes (`service.*`, `host.name`, `site.id`, `node.role`) are identical across all three signals via `BuildAttributes`. No issues found. | +| 8 | Packaging, dependencies & project layout | ☑ | Version 0.2.0; repository URLs corrected to `scadaproj`; the Serilog stack is still confined to `.Serilog`. No issues found. | +| 9 | Testing coverage | ☑ | 42 tests pass, including removal and nested redaction and governor precedence. Untested: enricher ordering against `ConfigureLogger` (Telemetry-009), colliding dictionary keys (Telemetry-010), a scheme-less OTLP endpoint (Telemetry-011). | +| 10 | Documentation & XML docs | ☑ | The `ILogRedactor`/README redaction-reach text is accurate. `ConfigureLogger` docs advertise "extra enrichers" without saying they run after redaction (Telemetry-009). | + +
2026-06-01 baseline checklist (5f75cd4) + | # | Category | Examined | Notes | |---|----------|----------|-------| | 1 | Correctness & logic bugs | ☑ | Redactor "remove" path is a no-op (Telemetry-001); structured values opaque to redactor (Telemetry-002). | @@ -47,6 +90,8 @@ happy paths but have no coverage for redactor removal or structured-value redact | 9 | Testing coverage | ☑ | No tests for redactor removal or structured-value redaction (Telemetry-003). | | 10 | Documentation & XML docs | ☑ | `MapZbMetrics` doc-comment is stale: claims "only valid when Exporter = Prometheus" (Telemetry-008). | +
+ ## Findings ### Telemetry-001 — `RedactionEnricher` ignores property removal, leaving secrets in the event @@ -342,3 +387,131 @@ overlay). Align the shared-contract summary for `MapZbMetrics` to match. Resolved in `544a6dd`, 2026-06-01 — rewrote the `MapZbMetrics` XML doc to state it is valid under any `Exporter` value (Prometheus always-on; OTLP additive overlay) and aligned the matching shared-contract summary. + + + +### Telemetry-009 — Enrichers added via `ZbSerilogOptions.ConfigureLogger` run after `RedactionEnricher`, so their properties are never redacted + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Security & secret handling | +| Status | Open | +| Location | `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogExtensions.cs:118-120`, `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/ZbSerilogOptions.cs:41-46` | + +**Description** + +`AddZbSerilogCore` calls `ZbSerilogConfig.Apply(…)`, which registers `RedactionEnricher`, and only then +invokes `serilogOptions.ConfigureLogger`. Serilog runs enrichers in registration order, so any +`lc.Enrich.With(...)` or `Enrich.WithProperty(...)` added in the hook stamps its properties after +redaction has already run. They reach every sink unredacted. The `ConfigureLogger` XML doc and the +README advertise this hook as the place for "extra enrichers" and say it runs "after … the shared +enricher/redaction/export set". They never say this means its enrichers are outside the redaction +guarantee that `ILogRedactor` documents ("Called on every log event before it reaches any sink"). +Enrichers that come from `ReadFrom.Configuration` (for example `FromLogContext`) are unaffected, +because they are registered before redaction. + +**Failure scenario** + +MxGateway, which registers an `ILogRedactor` that masks `apiKey`, moves to the 0.2.0 overload and adds +`s.ConfigureLogger = (lc, sp) => lc.Enrich.With(new RequestHeadersEnricher(sp))`, which stamps a +`ClientApiKey` or `apiKey` property. Every log event now carries the raw key to the file and OTLP sinks. +A scratchpad probe reproduced it: a `LoggerConfiguration` with `RedactionEnricher` (redactor masks +`apiKey`) followed by `Enrich.WithProperty("apiKey","s3cret")` emitted `apiKey="s3cret"`. Today this +is latent: no family app uses the `ConfigureLogger` hook yet. + +**Recommendation** + +Register `RedactionEnricher` as the final stage. One way is to invoke `ConfigureLogger` before +`ZbSerilogConfig.Apply`'s redaction step. Another is to apply redaction through a wrapping sink or the +last enricher after the hook. A third is to keep the order but document plainly that hook-added +enrichers bypass `ILogRedactor`. Add a test that enriches a sensitive property in `ConfigureLogger` and +asserts it is redacted. + +**Resolution** + +_Unresolved._ + +### Telemetry-010 — Redaction projection keys logged dictionaries by `ToString()`, silently dropping entries whose keys stringify identically + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry.Serilog/RedactionEnricher.cs:134-143` (projection), `:163-173` (rebuild) | + +**Description** + +`Project` turns a `DictionaryValue` into a `ProjectedDictionary` keyed by +`pair.Key.Value?.ToString() ?? ""`. Two distinct keys with the same string form overwrite each other in +both the projected values and `OriginalKeys`. Examples are `DateTime`/`DateTimeOffset` keys that differ +below one second (the default `ToString()` has no fractional seconds), and a null key alongside `""`. +`Rebuild` then emits one entry where there were two. `ValueEquals` sees the element count differ and +writes the shrunken dictionary back onto the event. This happens for every event once any +`ILogRedactor` is registered, even one that does nothing. + +**Failure scenario** + +A scratchpad probe registered a no-op `ILogRedactor` and logged +`log.Information("{Map}", new Dictionary{ [12:00:00.100]="first", [12:00:00.200]="second" })`. +With the enricher the result was `[(11/9/2026 12:00:00 PM: "second")]`. Without it the result was +`[(…12:00:00: "first"), (…12:00:00: "second")]`. Diagnostic log data (for example per-timestamp +samples) is silently lost in MxGateway, the redaction-enabled consumer. Nothing about it is secret. + +**Recommendation** + +Keep the original `ScalarValue` keys alongside the string view, for example by projecting to a list of +key/value pairs or disambiguating colliding string keys. Alternatively, skip projection and pass a +dictionary through untouched whenever its stringified keys are not unique. Add a regression test with +colliding keys. + +**Resolution** + +_Unresolved._ + +### Telemetry-011 — OTLP fail-fast validation accepts scheme-less `host:port` and is skipped by the pre-built-options `AddZbTelemetry` overload + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Error handling & resilience | +| Status | Open | +| Location | `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryOptionsValidator.cs:36`, `ZB.MOM.WW.Telemetry/src/ZB.MOM.WW.Telemetry/ZbTelemetryExtensions.cs:40-46` | + +**Description** + +The Telemetry-006 resolution added `ZbTelemetryOptionsValidator.Validate`, which checks +`Uri.TryCreate(OtlpEndpoint, UriKind.Absolute, out _)`. A scheme-less `host:port`, which is the typo the +original finding named ("missing scheme"), parses as an absolute URI whose scheme is the host name, so +it passes. The checks it ran: + +- `"localhost:4317"` → `True`, scheme `localhost`; +- `"collector:4317"` → `True`, scheme `collector`; +- only a literal IP such as `10.0.0.5:4317` is rejected. + +Separately, the validator is called only from `BuildOptions` (the two lambda overloads) and +`AddZbSerilog`. The public `AddZbTelemetry(IServiceCollection, ZbTelemetryOptions)` overload validates +neither the OTLP endpoint nor the required `ServiceName`. + +**Failure scenario** + +An operator sets `Exporter=Otlp, OtlpEndpoint="otel-collector:4317"`, missing `http://`. Startup passes +validation, the exporter gets a URI with scheme `otel-collector`, and OTLP traces, metrics and logs fail +later inside the exporter rather than as the named startup error Telemetry-006 promised. Prometheus +`/metrics` keeps working, so the loss is easy to miss. Any caller that builds `ZbTelemetryOptions` +itself and uses the options overload gets no validation at all. No family app uses that overload today. + +**Recommendation** + +Require `uri.Scheme` to be `http` or `https` (the OTLP gRPC/HTTP schemes) and a non-empty +`uri.Host`. Call `BuildOptions`-equivalent validation, including `ServiceName`, from the +`AddZbTelemetry(IServiceCollection, ZbTelemetryOptions)` overload as well. Add a test for +`"localhost:4317"`. + +**Resolution** + +_Unresolved._ diff --git a/code-reviews/Theme/findings.md b/code-reviews/Theme/findings.md index bc3c27e..ec286b3 100644 --- a/code-reviews/Theme/findings.md +++ b/code-reviews/Theme/findings.md @@ -7,14 +7,45 @@ | Component spec | `components/ui-theme/spec/SPEC.md` | | Shared contract | `components/ui-theme/shared-contract/ZB.MOM.WW.Theme.md` | | Status | Reviewed | -| Last reviewed | 2026-06-01 | -| Reviewer | Claude (automated baseline) | -| Commit reviewed | `5f75cd4` | -| Open findings | 0 | +| Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) | +| Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) | +| Commit reviewed | `91fb2b4` | +| Open findings | 2 | ## Summary -`ZB.MOM.WW.Theme` is a small, clean, well-scoped Blazor Razor Class Library: ten flat-namespace +**2026-11-09 re-review (`91fb2b4`, scadaproj#15).** Re-reviewed at `91fb2b4` (2026-11-09) against the 2026-06-01 baseline `5f75cd4`; the kit is now `0.5.0`. +New since the baseline: + +- the CSS-only `
` app shell, with the `::details-content` fix and the mobile shell fix; +- `nav-state.js` (localStorage persistence of `NavRailSection`, active-link reveal, and a + `MutationObserver` backstop for interactive render modes); +- `ThemeScripts`; +- the `TechButton` `Size`/`Class` extensions; +- the stateless `TreeTable`/`ITreeRow`; +- the upstreamed `.btn` sizing block and the 0.5.0 presentation classes. + +The C# components hold no state, implement no `IDisposable` and hold no JS interop references. There is +no `MarkupString` anywhere, and every caller-supplied string is attribute- or text-encoded by Razor. +`LoginCard` still delegates `ReturnUrl` validation and antiforgery to the host, with explicit docs. +`TreeTable` documents its Bootstrap Icons dependency. + +The `MutationObserver` is never disconnected. That is fine: it is page-lifetime, created once per +document, and the observe→mutate loop is guarded. The `
` shell relies on author `display` +rules for browsers without `::details-content`. That could not be verified without those browsers, so it +is not raised. + +All 60 bUnit/static-asset tests pass. There are two new findings: + +- **Theme-007 (Medium):** `TechButton` silently discards a caller-supplied `disabled` attribute. The + explicit `disabled="@Busy"` is written after the splat, so a button the caller meant to disable + renders enabled. Verified with a bUnit probe. +- **Theme-008 (Low):** `nav-state.js` marks a section as wired with a DOM attribute. Blazor's + enhanced-navigation DOM sync strips that attribute, so toggle listeners pile up on each navigation, + and the extra listeners save the transient active-link reveal as the user's preference, which is the + issue #2 regression. Verified by reading the code and simulating it in jsdom, with a control. + +**2026-06-01 baseline (`5f75cd4`).** `ZB.MOM.WW.Theme` is a small, clean, well-scoped Blazor Razor Class Library: ten flat-namespace SSR components, two stylesheets (`theme.css` 379 lines, `layout.css` 191 lines), three vendored IBM Plex `.woff2` fonts, and two public enums. The component C# is correct and minimal — stateless render logic, no concurrency surface, no secret handling, sensible `[Parameter]`/`EditorRequired` @@ -31,6 +62,23 @@ absence of XML docs on the public parameter surface. ## Checklist coverage +2026-11-09 re-review at `91fb2b4`: + +| # | Category | Examined | Notes | +|---|----------|----------|-------| +| 1 | Correctness & logic bugs | ☑ | `TechButton` drops caller `disabled` → Theme-007. Duplicate toggle listeners after enhanced navigation persist the transient reveal → Theme-008. `TreeTable` (keyed rows, leaf ARIA omission, invariant-culture depth style) and the `NavRailSection` slug are correct. | +| 2 | Public API surface & compatibility | ☑ | Additive in 0.2.0–0.5.0: `ButtonSize`, `TechButton.Size`/`Class`, `TreeTable`/`ITreeRow`, `NavRailSection.Key`, `LoginCard.Heading`, `ThemeScripts`. No removals. The `CaptureUnmatchedValues` contract is not fully honoured for `disabled` (Theme-007). | +| 3 | Concurrency & thread safety | ☑ | Stateless components; the JS is single-threaded with idempotent `apply()`. No issues found. | +| 4 | Error handling & resilience | ☑ | `localStorage` access is wrapped in try/catch, and the script degrades when `window.Blazor` or `MutationObserver` is absent. No issues found. | +| 5 | Security & secret handling | ☑ | No `MarkupString` and no raw HTML. `Accent` goes into a `style` attribute but is developer-supplied. `LoginCard` `ReturnUrl` is echoed into an encoded hidden field, and the open-redirect check stays documented as the host's job. localStorage holds only open/closed flags. No issues found. | +| 6 | Performance & resource management | ☑ | The `MutationObserver` (whole-document `childList`/`subtree`) is page-lifetime and cheap per mutation. Toggle listeners accumulate one per enhanced navigation (Theme-008). No `IJSObjectReference` or `IDisposable` to leak. | +| 7 | Spec & shared-contract adherence | ☑ | No-JS shell holds: native `
`, no Bootstrap JS. Tokens and chips are unchanged apart from documented additions. No new drift found. | +| 8 | Packaging, dependencies & project layout | ☑ | 0.5.0 RCL, `FrameworkReference` only. `nav-state.js` is shipped and asserted by `StaticAssetsTests`. AngleSharp is pinned to 1.5.0 in the test project only. No issues found. | +| 9 | Testing coverage | ☑ | 60 tests pass. Gaps: no test passes `disabled` through `TechButton`'s splat (Theme-007), and nothing exercises `nav-state.js` under DOM sync (Theme-008). | +| 10 | Documentation & XML docs | ☑ | Theme-006 fix holds (XML docs on every component, parameter and enum). README documents `TreeTable`'s Bootstrap Icons and interactivity requirements. No issues found. | + +
2026-06-01 baseline checklist (5f75cd4) + | # | Category | Examined | Notes | |---|----------|----------|-------| | 1 | Correctness & logic bugs | ☑ | Component render logic correct; `open="@Expanded"` boolean-attribute binding behaves (tests confirm); chip/variant `switch` mappings complete. No issues found. | @@ -44,6 +92,8 @@ absence of XML docs on the public parameter surface. | 9 | Testing coverage | ☑ | 32 tests cover every component's render contract + corrected font path. CSS-class wiring (e.g. `.rail-ico`, orphan nav CSS) is not asserted, which is how Theme-004/005 slipped through. | | 10 | Documentation & XML docs | ☑ | README/contract accurate. XML docs present only on `LoginCard`; absent on the other 9 components, all their parameters, and both enums (Theme-006). | +
+ ## Findings ### Theme-001 — Mobile hamburger toggle silently depends on Bootstrap collapse JS @@ -240,3 +290,104 @@ minimum the non-obvious ones (`ThemeShell.Accent`, `NavRailItem.Match`, `TechBut **Resolution** Resolved in `544a6dd`, 2026-06-01 — added `///` summaries to all nine remaining components, every public `[Parameter]`, and both enums (`ButtonVariant`, `StatusState`); enabled `GenerateDocumentationFile` (CS1591 left off the error set for Razor-generated members) so docs build under `TreatWarningsAsErrors` with 0 warnings. + + + +### Theme-007 — `TechButton` silently discards a caller-supplied `disabled` attribute + +| | | +|--|--| +| Severity | Medium | +| Priority | P2 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/Components/TechButton.razor:3` | + +**Description** + +The button is rendered as +``, with no `disabled`, so the button is +clickable and fires `Delete`. A bUnit probe in the scratchpad confirmed this: +`Render(p => p.AddUnmatched("disabled", true))` produced +``. No current family call site passes +`disabled` (20 `.razor` files use `TechButton`), so this is latent. It is still a natural use of the +documented splat, and any server-side authorization check would be the only backstop. + +**Recommendation** + +Compute the effective state as `Busy || callerDisabled`, where `callerDisabled` is read from +`AdditionalAttributes["disabled"]` with the same case-insensitive lookup used for `class`. Better still, +add an explicit `[Parameter] public bool Disabled`. Then render `disabled="@(Busy || Disabled)"`. Add a +bUnit test for caller `disabled=true` with `Busy=false`. + +**Resolution** + +_Unresolved._ + +### Theme-008 — `nav-state.js` "wired" guard is a DOM attribute that Blazor enhanced-navigation DOM sync strips, so toggle listeners accumulate and the transient active-link reveal is persisted + +| | | +|--|--| +| Severity | Low | +| Priority | P3 | +| Category | Correctness & logic bugs | +| Status | Open | +| Location | `ZB.MOM.WW.Theme/src/ZB.MOM.WW.Theme/wwwroot/js/nav-state.js:7`, `:17-18`, `:39`, `:65-66` | + +**Description** + +`wire()` marks a section as initialised with the attribute `data-zbnav-initialized`. `apply()` uses +`hasAttribute(INIT_ATTR)` as its only guard against adding a second `toggle` listener (commit `9e4dedc`, +"guard nav-state.js against duplicate toggle listeners"). After an enhanced navigation, Blazor's DOM +sync (`blazor.web.js` 10.0.1, its attribute synchronisation step) keeps the existing `
` element +but removes every attribute that is not in the newly server-rendered HTML. That includes +`data-zbnav-initialized`, which the server never renders. Then `enhancedload` calls `apply()`, which +finds no marker and calls `wire()` again, adding another `toggle` listener to the same element. After N +navigations the element carries N+1 listeners. + +The transient-reveal protection assumes a single listener. The first listener consumes +`data-zbnav-transient` and returns. Every later listener sees no flag and writes +`localStorage[zbnav:]`. + +**Failure scenario** + +1. A user collapses the "Admin" rail section; `zbnav:admin` is saved as `"0"`. +2. They move around the app by enhanced (static-SSR) navigation. +3. They reach a page whose active link lives inside "Admin", for example from a link in the page body + or a redirect. +4. `apply()` reveals the section and flags it transient. + +The duplicate listeners then overwrite the saved preference with `"1"`, so the section stays expanded on +every later page. This is the issue #2 bug the transient flag exists to prevent. + +A jsdom simulation reproduced it, running the shipped `nav-state.js`, stripping the attribute the way +DOM sync does, and firing `enhancedload` 3 times: + +- one collapse produced 4 `setItem` writes; +- the reveal produced 3 writes of `"1"`, leaving `saved=1`; +- the control run, with the attribute not stripped, produced 1 collapse write and 0 reveal writes, + leaving `saved=0`. + +**Recommendation** + +Track wiring in a JS-side structure that DOM sync cannot touch, for example a module-level +`WeakSet` of wired elements or an expando property (`el.__zbnavWired = true`), instead of a DOM +attribute. Add a test that strips attributes, re-runs `apply()`, and asserts one write per toggle. + +**Resolution** + +_Unresolved._ diff --git a/code-reviews/_template/findings.md b/code-reviews/_template/findings.md index b77a42d..50b5ad2 100644 --- a/code-reviews/_template/findings.md +++ b/code-reviews/_template/findings.md @@ -4,7 +4,8 @@ Template for a shared-library review. Copy the structure below into code-reviews//findings.md and fill it in. is the ZB.MOM.WW.* library with the prefix stripped (Auth, Theme, - Health, Telemetry, Configuration, Audit). See ../REVIEW-PROCESS.md for the + Health, Telemetry, Configuration, Audit, Secrets, LocalDb, GalaxyRepository, + Overview). See ../REVIEW-PROCESS.md for the full process and the library → component-spec mapping. --> @@ -52,6 +53,7 @@ Confirm every category was examined. Record "No issues found" where applicable. | | | |--|--| | Severity | Critical \| High \| Medium \| Low | +| Priority | P1 \| P2 \| P3 | | Category | | | Status | Open \| In Progress \| Resolved \| Won't Fix \| Deferred | | Location | `ZB.MOM.WW./src//.cs:` | @@ -60,6 +62,10 @@ Confirm every category was examined. Record "No issues found" where applicable. What is wrong and why it matters. +**Failure scenario** + +Concrete inputs/sequence and the wrong outcome. + **Recommendation** Concrete suggested fix. diff --git a/code-reviews/regen-readme.py b/code-reviews/regen-readme.py index 49e00ff..da4cce6 100755 --- a/code-reviews/regen-readme.py +++ b/code-reviews/regen-readme.py @@ -132,8 +132,9 @@ def build_readme(libraries, per_library): add(f"{reviewed} of {len(libraries)} libraries reviewed. " f"{len(pending)} pending finding{'s' if len(pending) != 1 else ''} across all libraries.") add("") - add("Coverage gap: Secrets, LocalDb, GalaxyRepository and Overview have never been reviewed here,") - add("and Auth/Health/Theme have changed substantially since their 2026-06-01 review → #15.") + add("Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health,") + add("Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also") + add("carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low).") add("") add("| Severity | Open findings |") add("|----------|---------------|")