Joseph Doherty
6112a9f447
The replication actor was created only when something resolved ISecretStore
(store -> replicator -> SecretReplicationActorProvider.ActorRef). A node that
expanded its ${secret:} references pre-host and never touched the store at
runtime had no actor: no topic subscription, no announces, no pull replies.
AddZbSecretsAkkaReplication now registers SecretReplicationActorStarter, which
creates the actor on IHostApplicationLifetime.ApplicationStarted (after every
hosted service, incl. an Akka.Hosting AkkaHostedService registered later), or
in StartAsync when no lifetime is registered.
- tests: EagerActorStartTests (generic host in both registration orders with a
no-actor-before-start control; bare container) - all fail without the fix
- docs: README, CHANGELOG
- code-reviews: Secrets-010 Resolved; README regenerated
Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
122 lines
8.1 KiB
Markdown
122 lines
8.1 KiB
Markdown
# 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. 26 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 | 7 |
|
|
| Low | 19 |
|
|
| **Total** | **26** |
|
|
|
|
## 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/0/0 | 0 | 8 |
|
|
| [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 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/0 | 0 | 5 |
|
|
| [Secrets](Secrets/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/2/13 | 15 | 19 |
|
|
| [Telemetry](Telemetry/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 11 |
|
|
| [Theme](Theme/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 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 (7)
|
|
|
|
| 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 |
|
|
| 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-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 (19)
|
|
|
|
| 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 |
|
|
| 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 |
|
|
| 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 |
|