Joseph Doherty
9ec0da1b2c
ExtractUserName returned the first value of a possibly multi-valued UserNameAttribute (default cn), so jsmith could sign in as "John Smith" depending on server ordering. It now prefers the value equal to the typed name ignoring case (still canonicalising case) and falls back to the first value only when none matches. (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. 16 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 | 16 |
| Total | 16 |
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/3 | 3 | 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 (16)
| 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 |
| 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 |