dohertj2/lmxopcua
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity
Files
e221371a0c7b1bff3f8d475bec79bf246a25d1f8
lmxopcua/code-reviews/Driver.AbCip.Cli/findings.md
Joseph Doherty 8568f5cd85 docs(code-reviews): comprehensive per-module review pass at 76d35d1 
Reviewed all 31 src/ production projects against the 10-category
checklist in REVIEW-PROCESS.md. Each module gets its own findings.md;
code-reviews/README.md is regenerated from them.

334 findings: 6 Critical, 46 High, 126 Medium, 156 Low.

Critical findings:
- Server-001: WriteNodeIdUnknown recurses unconditionally — a HistoryRead
  on an unresolvable node crashes the process (remote DoS).
- Admin-001/002: app-wide auth bypass (RouteView not AuthorizeRouteView)
  plus unauthenticated mutating routes.
- Core.Scripting-001: System.Environment reachable from operator scripts;
  Environment.Exit() terminates the server.
- Core.AlarmHistorian-001: rowIds/events parallel-list desync on a corrupt
  payload misapplies outcomes — silent alarm-event data loss.
- Driver.Galaxy-001: ReconnectSupervisor is built but never triggered, so
  a transient gateway drop permanently kills the event stream.

All findings are Status=Open; resolution is tracked per REVIEW-PROCESS.md
section 4. Review only — no source code changed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-22 05:20:27 -04:00

239 lines
10 KiB
Markdown
Raw Blame History

# Code Review — Driver.AbCip.Cli
| Field | Value |
|---|---|
| Module | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli` |
| Reviewer | Claude Code |
| Review date | 2026-05-22 |
| Commit reviewed | `76d35d1` |
| Status | Reviewed |
| Open findings | 8 |
## Checklist coverage
A comprehensive review completes every category, recording "No issues found" where
a category produced nothing rather than leaving it blank.
| # | Category | Result |
|---|---|---|
| 1 | Correctness & logic bugs | Driver.AbCip.Cli-001, Driver.AbCip.Cli-002 |
| 2 | OtOpcUa conventions | No issues found |
| 3 | Concurrency & thread safety | Driver.AbCip.Cli-003 |
| 4 | Error handling & resilience | Driver.AbCip.Cli-001, Driver.AbCip.Cli-004 |
| 5 | Security | No issues found |
| 6 | Performance & resource management | Driver.AbCip.Cli-005 |
| 7 | Design-document adherence | Driver.AbCip.Cli-006 |
| 8 | Code organization & conventions | No issues found |
| 9 | Testing coverage | Driver.AbCip.Cli-007 |
| 10 | Documentation & comments | Driver.AbCip.Cli-008 |
## Findings
<!-- One ### entry per finding. IDs are <Module>-NNN, sequential within the module,
never reused. Findings are never deleted — close them by changing Status and
completing Resolution. -->
### Driver.AbCip.Cli-001
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Error handling & resilience |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/WriteCommand.cs:70-85` |
| Status | Open |
**Description:** `ParseValue` parses every numeric Logix type with the BCL `*.Parse`
methods (`sbyte.Parse`, `short.Parse`, `int.Parse`, `float.Parse`, ...). These throw
the raw `FormatException` and `OverflowException` on bad operator input. The module's
own test `ParseValue_non_numeric_for_numeric_types_throws` confirms a raw
`FormatException` escapes for `DInt`. Meanwhile the `Bool` branch and the `_ =>`
default branch throw the CLI-friendly `CliFx.Exceptions.CommandException` with an
actionable message. The result is inconsistent operator UX: a typo in a boolean
value prints "Boolean value 'x' is not recognised...", but a typo in a numeric
value (`write -v 12x --type DInt`, or an out-of-range `write -v 99999999999 --type
Int`) escapes uncaught and CliFx renders a full .NET stack trace instead of a
one-line error. CliFx only formats `CommandException` cleanly.
**Recommendation:** Wrap the numeric `*.Parse` calls (or the whole `switch`) in a
`try`/`catch (Exception ex) when (ex is FormatException or OverflowException)` that
rethrows as a `CommandException` with the raw value, the target `--type`, and the
valid range — mirroring the `ParseBool` failure message.
**Resolution:** _(open)_
### Driver.AbCip.Cli-002
| Field | Value |
|---|---|
| Severity | Medium |
| Category | Correctness & logic bugs |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/ProbeCommand.cs:21-23`; `Commands/ReadCommand.cs:24-25`; `Commands/SubscribeCommand.cs:20-22` |
| Status | Open |
**Description:** `ProbeCommand`, `ReadCommand`, and `SubscribeCommand` expose
`--type` as a free `AbCipDataType` enum option with no exclusion of
`AbCipDataType.Structure`. Only `WriteCommand` rejects `Structure` (with an explicit
`CommandException`). Passing `probe/read/subscribe --type Structure` synthesises a
tag with `DataType = Structure` and no `Members` declared. The driver read path
treats a memberless Structure tag as a black box and routes it to the per-tag
fallback, where `runtime.DecodeValue(AbCipDataType.Structure, ...)` runs with no
declared layout — the operator gets either an opaque value or a confusing status
code rather than the clean "Structure writes need an explicit member layout"
guidance `write` gives. The `read` doc comment even claims "UDT / Structure reads
are out of scope here", but the code does not enforce it.
**Recommendation:** Reject `AbCipDataType.Structure` in `ProbeCommand`,
`ReadCommand`, and `SubscribeCommand` `ExecuteAsync` with the same `CommandException`
pattern `WriteCommand` uses, or factor a shared `RejectStructure(DataType)` guard
into `AbCipCommandBase`.
**Resolution:** _(open)_
### Driver.AbCip.Cli-003
| Field | Value |
|---|---|
| Severity | Low |
| Category | Concurrency & thread safety |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:50-56,60-61` |
| Status | Open |
**Description:** The `OnDataChange` handler writes change lines to `console.Output`
(a `TextWriter`) from the driver's poll-engine callback thread, while the command's
main flow concurrently writes the "Subscribed to ... Ctrl+C to stop." line on the
CLI thread. `TextWriter.WriteLine` is not guaranteed thread-safe; concurrent writes
from the poll thread and the main thread can interleave or, in the worst case,
corrupt buffered output. The window is small (one main-thread write right after
`SubscribeAsync`) but it exists, and any future addition of main-thread output
during the watch loop widens it.
**Recommendation:** Emit the "Subscribed..." banner before registering the
`OnDataChange` handler (or before `SubscribeAsync`), or guard all `console.Output`
writes during the subscription with a shared lock so poll-thread and main-thread
output cannot interleave.
**Resolution:** _(open)_
### Driver.AbCip.Cli-004
| Field | Value |
|---|---|
| Severity | Low |
| Category | Error handling & resilience |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/Commands/SubscribeCommand.cs:28,58`; `AbCipCommandBase.cs:26-34` |
| Status | Open |
**Description:** `--interval-ms` (`IntervalMs`) is taken verbatim and passed as
`TimeSpan.FromMilliseconds(IntervalMs)` to `SubscribeAsync` with no validation. A
zero or negative value produces a non-positive `TimeSpan`; the option description
claims "PollGroupEngine floors sub-250ms values" but says nothing about `0` or
negatives, and the flooring behaviour is the engine's, not the CLI's — relying on a
downstream component to sanitise operator input is fragile. `--timeout-ms` on
`AbCipCommandBase` has the same gap (a negative value yields a negative `TimeSpan`).
**Recommendation:** Validate `IntervalMs > 0` and `TimeoutMs > 0` at the top of
`ExecuteAsync` / in `AbCipCommandBase`, throwing a `CommandException` with the
accepted range when out of bounds.
**Resolution:** _(open)_
### Driver.AbCip.Cli-005
| Field | Value |
|---|---|
| Severity | Low |
| Category | Performance & resource management |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.Cli.Common/DriverCommandBase.cs:51-59` |
| Status | Open |
**Description:** `ConfigureLogging` assigns a freshly created Serilog logger to the
process-global `Log.Logger` but never calls `Log.CloseAndFlush()`. For a short-lived
one-shot command (`probe`, `read`, `write`) the process exit flushes the console
sink, so the practical impact is nil. For `subscribe` — a long-running command
terminated by Ctrl+C — buffered log lines emitted just before cancellation can be
lost on abrupt exit. (This lives in the shared `Driver.Cli.Common` base, so it is
noted here as it affects the AB CIP CLI; the canonical fix belongs in that shared
module's review.)
**Recommendation:** Register `Log.CloseAndFlush()` on process exit (e.g. via
`AppDomain.ProcessExit` or a `finally` in the command), or have the CLI use a
disposable logger scoped to `ExecuteAsync`.
**Resolution:** _(open)_
### Driver.AbCip.Cli-006
| Field | Value |
|---|---|
| Severity | Low |
| Category | Design-document adherence |
| Location | `src/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli/AbCipCommandBase.cs:29-34` |
| Status | Open |
**Description:** `AbCipCommandBase` overrides the abstract `DriverCommandBase.Timeout`
property with a getter derived from `TimeoutMs` and an empty `init` body
(`init { /* driven by TimeoutMs */ }`). Because the override has no
`[CommandOption]` attribute, CliFx never binds it, so the empty `init` is unreachable
in normal CLI use. However, an empty `init` accessor silently discards any
assignment — if a future caller or test constructs the command via an object
initializer (`new ReadCommand { Timeout = ... }`) the assignment is silently dropped
with no compiler warning. This is a latent correctness trap rather than a current
bug.
**Recommendation:** Either drop the `init` accessor entirely (make the override a
get-only expression-bodied property) or have the empty `init` throw
`NotSupportedException` to make the "driven by TimeoutMs" contract explicit and
fail-fast.
**Resolution:** _(open)_
### Driver.AbCip.Cli-007
| Field | Value |
|---|---|
| Severity | Low |
| Category | Testing coverage |
| Location | `tests/Drivers/Cli/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Cli.Tests/WriteCommandParseValueTests.cs` |
| Status | Open |
**Description:** The only test file covers `WriteCommand.ParseValue` and
`ReadCommand.SynthesiseTagName` — both pure static helpers. There is no coverage for
`AbCipCommandBase.BuildOptions` (the flag-to-`AbCipDriverOptions` mapping that all
four commands depend on) or `DriverInstanceId`. `BuildOptions` is pure and trivially
unit-testable yet untested: a regression that, say, flipped `EnableAlarmProjection`
back on or dropped `Probe.Enabled = false` would not be caught — and the comment
explicitly warns the probe loop "would race the operator's own reads", so that
mapping is behaviourally load-bearing. The `ExecuteAsync` bodies are reasonably left
untested (they need a fake `AbCipDriver` or hardware), consistent with the other
driver CLIs.
**Recommendation:** Add unit tests asserting `BuildOptions` produces
`Probe.Enabled == false`, `EnableControllerBrowse == false`,
`EnableAlarmProjection == false`, the expected single `AbCipDeviceOptions`
(`HostAddress`, `PlcFamily`, `DeviceName`), the supplied tag list, and the `Timeout`
derived from `TimeoutMs`.
**Resolution:** _(open)_
### Driver.AbCip.Cli-008
| Field | Value |
|---|---|
| Severity | Low |
| Category | Documentation & comments |
| Location | `docs/Driver.AbCip.Cli.md:8-9` |
| Status | Open |
**Description:** `docs/Driver.AbCip.Cli.md` opens with "Second of four driver
test-client CLIs (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT)." The count "four"
contradicts the chain that follows it (five names) and contradicts
`docs/DriverClis.md`, which documents six CLIs (Modbus, AB CIP, AB Legacy, S7,
TwinCAT, FOCAS). The FOCAS CLI shipped alongside the Tier-C work, so the AB CIP
doc's "four" and the truncated chain are both stale.
**Recommendation:** Update the sentence to "Second of six driver test-client CLIs"
and complete the chain (Modbus -> AB CIP -> AB Legacy -> S7 -> TwinCAT -> FOCAS), or
drop the explicit count and link `docs/DriverClis.md` as the authoritative roster.
**Resolution:** _(open)_
Reference in New Issue View Git Blame Copy Permalink