scadaproj/code-reviews/README.md

# Code Reviews

Comprehensive, per-library code reviews of the `ZB.MOM.WW.*` shared libraries hosted
in this repo. Each library (one self-contained `.slnx` at the repo root) has its own
folder containing a `findings.md`. This README is the aggregated index — the single
place to see all outstanding work.

> Generated by `regen-readme.py` from the per-library `findings.md` files. Do not
> edit by hand — edit the findings files and re-run the script.

> **Status lives in Gitea.** Open items from this file are tracked as issues on
> [gitea.dohertylan.com/dohertj2/scadaproj](https://gitea.dohertylan.com/dohertj2/scadaproj/issues)
> (label `migrated`, 2026-11-06). This file is a historical/design record — do not track status
> here; file new work (including new review findings) as an issue.

## How it works

- Reviews are performed one library at a time against a fixed checklist.
- Each library is reviewed against its normalized component spec under `components/`.
- Every finding is recorded in the library's `findings.md` with a severity and status.
- Findings are **never deleted** — they are closed by changing their status, keeping
  a full audit trail.
- This README aggregates every **pending** finding (`Open` / `In Progress`) across all
  libraries.

See **[REVIEW-PROCESS.md](REVIEW-PROCESS.md)** for the full procedure: the review
checklist, severity definitions, finding format, the library → component-spec mapping,
and how to mark items resolved.

## Layout

```
code-reviews/
├── README.md            # this file — process overview + pending findings
├── REVIEW-PROCESS.md     # how to perform a review and track findings
├── regen-readme.py       # regenerates this README from the findings files
├── _template/findings.md # copy-this template for a library review
└── <Library>/findings.md # one folder per ZB.MOM.WW.* shared library
```

## Summary

10 of 10 libraries reviewed. 46 pending findings across all libraries.

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 | 13 |
| Low | 33 |
| **Total** | **46** |

## Library Status

| Library | Last reviewed | Commit | Open (C/H/M/L) | Open | Total |
|---------|---------------|--------|----------------|------|-------|
| [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/0/0/3 | 3 | 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/0/1 | 1 | 8 |

## Pending Findings

Every `Open` / `In Progress` finding across all libraries, highest severity first.
Resolved findings drop off this list but remain recorded in their library's
`findings.md` (see [REVIEW-PROCESS.md](REVIEW-PROCESS.md) §4–§5). Full detail —
description, location, recommendation — lives in the library's `findings.md`.

### Critical (0)

_None open._

### High (0)

_None open._

### Medium (13)

| 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 |
| 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 |

### Low (33)

| 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 |