Joseph Doherty
5f5d684c44
`secret set`/`rotate` took the plaintext as a positional argument (shell history, process list), and Program handed argv to Host.CreateApplicationBuilder, whose command-line config provider crashed on a value like "-p=Hunter2Secret" with the value in the unhandled FormatException on stderr. - SetCommandLine: `set|rotate <name> [-|--value-stdin]`; no value reads piped stdin or prompts twice (masked) at a TTY; one trailing newline dropped; empty refused. The positional <value> still works and warns on stderr. - host built from HostApplicationBuilderSettings without argv; startup failures print `secret: startup failed: <msg>` and exit 1 - unexpected trailing arguments reported by position, not echoed - tests: SetCommandLineTests incl. two child-process runs of the built CLI; the -p=Hunter2Secret probe fails (leaks) on the old Program.cs - docs: README, clustered-secrets runbook, CHANGELOG - code-reviews: Secrets-017 Resolved; README regenerated Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
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.pyfrom the per-libraryfindings.mdfiles. 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 (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.mdwith 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 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. 24 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 | 5 |
| Low | 19 |
| Total | 24 |
Library Status
| Library | Last reviewed | Commit | Open (C/H/M/L) | Open | Total |
|---|---|---|---|---|---|
| Audit | 2026-11-09 | 91fb2b4 |
0/0/0/0 | 0 | 5 |
| Auth | 2026-11-09 | 91fb2b4 |
0/0/1/3 | 4 | 10 |
| Configuration | 2026-11-09 | 91fb2b4 |
0/0/0/0 | 0 | 4 |
| GalaxyRepository | 2026-11-09 | 91fb2b4 |
0/0/0/0 | 0 | 8 |
| Health | 2026-11-09 | 91fb2b4 |
0/0/0/0 | 0 | 7 |
| LocalDb | 2026-11-09 | 91fb2b4 |
0/0/4/3 | 7 | 7 |
| Overview | 2026-11-09 | 91fb2b4 |
0/0/0/0 | 0 | 5 |
| Secrets | 2026-11-09 | 91fb2b4 |
0/0/0/13 | 13 | 19 |
| Telemetry | 2026-11-09 | 91fb2b4 |
0/0/0/0 | 0 | 11 |
| Theme | 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 §4–§5). Full detail —
description, location, recommendation — lives in the library's findings.md.
Critical (0)
None open.
High (0)
None open.
Medium (5)
| ID | Library | Title |
|---|---|---|
| Auth-007 | Auth | User-bind mode never checks that the entry whose groups it returns is the account that bound |
| LocalDb-001 | LocalDb | Sync endpoint is unauthenticated by default, and ApiKey is documented as "expected from the peer" |
| LocalDb-002 | LocalDb | 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 | INSERT OR REPLACE apply silently deletes a different row on a secondary UNIQUE conflict, so the pair diverges |
| LocalDb-004 | LocalDb | FailClosedOnDrift is bypassed on the snapshot path, so a future-stamped snapshot row moves the local HLC forward permanently |
Low (19)
| ID | Library | Title |
|---|---|---|
| Auth-008 | Auth | SQL Server store silently truncates over-length values and matches key ids case-insensitively, diverging from SQLite |
| Auth-009 | Auth | Canonical username is the first value of a possibly multi-valued attribute, not the value the user matched |
| Auth-010 | Auth | ApiKeyVerifier lets store exceptions escape despite its "only exception path is cancellation" contract |
| LocalDb-005 | LocalDb | Auto-assigned rowid / AUTOINCREMENT keys collide across nodes and LWW silently drops one row |
| LocalDb-006 | LocalDb | Column names containing ' are captured but break the apply and snapshot SQL |
| LocalDb-007 | LocalDb | README is stale on version, adoption, test count, Synchronous values and baseline scope |
| Secrets-004 | Secrets | Corrupt or wrong-length crypto fields escape Decrypt as ArgumentException: not audited, and breaks the cipher contract |
| Secrets-005 | Secrets | Store-path rules are lexical: drive-relative paths pass "rooted", and \\?\ / UNC / 8.3 / symlink spellings bypass "under content root" |
| Secrets-006 | Secrets | Resolver cache can re-cache a stale value after Invalidate, and expired plaintext is never evicted |
| Secrets-007 | Secrets | 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 | Hub push sweep is not isolated per row, so one rejected row stops every later push |
| Secrets-009 | Secrets | README wiring example uses a relative SqlitePath that the validator rejects; empty or unterminated tokens pass through as literals |
| Secrets-011 | Secrets | HandleAnnounceTick publishes from a thread-pool continuation: announces carry no sender under an async store, and faults are silently dropped |
| Secrets-012 | Secrets | Akka anti-entropy replies are unchunked; a large store exceeds Akka.Remote's frame size and never converges |
| Secrets-014 | Secrets | UI "Add secret" silently overwrites an existing (or tombstoned) secret, clobbers its metadata, and audits it as secret.add |
| Secrets-015 | Secrets | Reveal shows the plaintext even when its audit write fails, and swallows the failure with no log |
| Secrets-016 | Secrets | The public UI components enforce no authorization of their own; gating relies entirely on the host rendering them inside AuthorizeView |
| Secrets-018 | Secrets | Bundle import aborts mid-run on one bad entry, leaving earlier rows applied but reporting only "Import failed" |
| Secrets-019 | Secrets | 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 |