Joseph Doherty
c207573595
The drift guard lived only in the delta path; SnapshotApplier applied every row unchecked, so a future-stamped snapshot row was applied, observed by the clock and persisted as last_seen_hlc - moving the receiver's HLC ahead across restarts despite FailClosedOnDrift (LocalDb-004). SnapshotApplier now takes ReplicationOptions and runs the same per-row check (warn once per snapshot; dead-letter under FailClosedOnDrift via the shared HlcDriftGuard, with the row's HLC kept out of last_seen_hlc). Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
114 lines
6.8 KiB
Markdown
114 lines
6.8 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. 19 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 | 19 |
|
|
| **Total** | **19** |
|
|
|
|
## 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/0/3 | 3 | 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/0/3 | 3 | 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/0/13 | 13 | 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 (0)
|
|
|
|
_None open._
|
|
|
|
### 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 |
|