Joseph Doherty
b33f0f93c9
- Over-length key records (key_id > 64, prefix > 16, display name > 256) are refused with ArgumentException before connecting; an over-64 key-id lookup or mutation answers not-found without querying (SqlClient silently truncated the sized parameter, so a different id sharing the first 64 characters matched). - Every key_id predicate adds an exact VARBINARY comparison, so lookups are case- and trailing-space-exact like SQLite; the PK seek is unchanged and there is no schema change. - SqliteApiKeyStoreImporter reports case-only key-id collisions and over-length rows in new init lists (KeyIdCollisions, KeysRejected) instead of counting them as skips or importing them truncated. - Audit text truncation kept deliberately and documented. Offline refused-port tests and live SQL Server 2022 tests fail pre-fix; SqlServer suite 101/101 live. Auth findings now 0 open; CLAUDE.md records the #46/#50 work; code-reviews/README.md regenerated. (scadaproj#46) 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. 13 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 | 0 |
| Low | 13 |
| Total | 13 |
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/0/0 | 0 | 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/0/0 | 0 | 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 (0)
None open.
Low (13)
| ID | Library | Title |
|---|---|---|
| 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 |