dohertj2/ScadaBridge
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

docs(src): fix CommentChecker findings across src/ — 1,032 → 5 (all false positives)

Browse Source 
Comment/XML-doc changes only, no code logic:
- strip project-tracking references (WP*, P*-*, row-NN, #NN, Task N) from
  comments, keeping the explanatory rationale
- add missing summaries, <param>, <returns>, <typeparam> and <inheritdoc/>
- replace misused <inheritdoc/> with real docs; where <inheritdoc/> was
  combined with useful remarks, move the rationale into // comments
- drop stale replicationActor <param> on DeploymentManagerActor ctor

Remaining 5 are false positives (CWE-1236 x2, failures-1, year-0001, M365).
Build 0 errors / 0 warnings.
This commit is contained in:
Joseph Doherty
2026-10-07 14:12:25 -04:00
parent 3a217b9484
commit 0ffd4bdbea
217 changed files with 1224 additions and 695 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ZB.MOM.WW.ScadaBridge.ManagementService/AuditEndpoints.cs
+3 -3
View File
@@ -763,7 +763,7 @@ public static class AuditEndpoints
// ─────────────────────────────────────────────────────────────────────
// CSV helpers — 24 columns, same order/naming as the JSONL export
// (AuditExportRow property declaration order; arch-review 2026-08-19 P6-7)
// (AuditExportRow property declaration order)
// ─────────────────────────────────────────────────────────────────────
public const string CsvHeader =
@@ -783,7 +783,7 @@ public static class AuditEndpoints
/// <see cref="AuditExportRow.ResponseSummary"/>, <see cref="AuditExportRow.Extra"/>) are run
/// through <see cref="NeedsFormulaNeutralization"/> before RFC 4180 quoting so a leading
/// <c>=</c>/<c>+</c>/<c>-</c>/<c>@</c>/TAB/CR can't be interpreted as a formula or DDE
/// trigger by a spreadsheet application (CWE-1236). GUID/enum/numeric/date/bool columns the
/// trigger by a spreadsheet application (CSV formula injection). GUID/enum/numeric/date/bool columns the
/// exporter formats itself are never neutralized — none of them can start with a trigger
/// character, and neutralizing a signed numeric column would corrupt it.
/// </summary>
@@ -833,7 +833,7 @@ public static class AuditEndpoints
return;
}
// OWASP CSV-injection neutralization (CWE-1236): a free-text cell whose
// OWASP CSV-injection neutralization (CSV formula injection): a free-text cell whose
// first character would be interpreted by a spreadsheet application as a
// formula/DDE trigger is prefixed with a literal single quote. Applied
// BEFORE RFC 4180 quoting, and only to free-text columns — never to the
src/ZB.MOM.WW.ScadaBridge.ManagementService/ConfigSecretScrubber.cs
+3
View File
@@ -18,6 +18,9 @@ internal static class ConfigSecretScrubber
private static readonly string[] SecretNameFragments =
["password", "secret", "token", "apikey", "credential", "passphrase"];
/// <summary>Determines whether a property name looks like it holds a secret.</summary>
/// <param name="propertyName">The property name to test.</param>
/// <returns>True when the name contains a secret-like fragment.</returns>
internal static bool IsSecretName(string propertyName)
{
foreach (var fragment in SecretNameFragments)
src/ZB.MOM.WW.ScadaBridge.ManagementService/DebugStreamHub.cs
+3 -3
View File
@@ -193,7 +193,7 @@ public class DebugStreamHub : Hub
CancellationToken.None, TaskContinuationOptions.OnlyOnFaulted, TaskScheduler.Default);
});
// arch-review 2026-08-19 P6-6: the client may have disconnected while
// The client may have disconnected while
// StartStreamAsync was in flight (up to 30s). OnDisconnectedAsync already ran
// and found no session id to release, so publishing it here would leave the
// session registered in the singleton DebugStreamService — whose keepalive
@@ -255,7 +255,7 @@ public class DebugStreamHub : Hub
/// <summary>
/// Atomically removes and returns the session id registered for this connection, so a
/// release path and the late publish in <see cref="SubscribeInstance"/> cannot both
/// claim (or both miss) the same session — see P6-6 there.
/// claim (or both miss) the same session — see there.
/// </summary>
/// <returns>The session id that was registered, or <see langword="null"/> if none was.</returns>
private string? TakeSessionId()
@@ -274,7 +274,7 @@ public class DebugStreamHub : Hub
/// <inheritdoc />
public override async Task OnDisconnectedAsync(Exception? exception)
{
// P6-6: mark the connection gone BEFORE releasing, so a SubscribeInstance still
// Mark the connection gone BEFORE releasing, so a SubscribeInstance still
// awaiting StartStreamAsync sees the flag and stops its own session rather than
// registering it against a connection nothing will ever unsubscribe.
lock (Context.Items)
src/ZB.MOM.WW.ScadaBridge.ManagementService/FlowDebug/FlowDebugRunner.cs
+27
View File
@@ -96,12 +96,16 @@ public sealed class FlowDebugRunner
private readonly Func<DateTimeOffset> _now;
/// <summary>Creates the runner.</summary>
/// <param name="logger">Logger for run diagnostics.</param>
public FlowDebugRunner(ILogger<FlowDebugRunner> logger)
: this(logger, Task.Delay, () => DateTimeOffset.UtcNow)
{
}
/// <summary>Creates the runner with an injectable delay and clock (tests).</summary>
/// <param name="logger">Logger for run diagnostics.</param>
/// <param name="delay">Delay function used for polling waits.</param>
/// <param name="now">Clock returning the current UTC time.</param>
public FlowDebugRunner(
ILogger<FlowDebugRunner> logger,
Func<TimeSpan, CancellationToken, Task> delay,
@@ -125,6 +129,11 @@ public sealed class FlowDebugRunner
/// registered none (a test composition root) collects raw but is still capped.
/// </para>
/// </summary>
/// <param name="sp">The request-scoped service provider.</param>
/// <param name="cmd">The command to execute.</param>
/// <param name="user">The authenticated operator.</param>
/// <param name="ct">Cancellation token.</param>
/// <returns>The run result with the collected trace rows.</returns>
public Task<FlowDebugRunResult> RunMethodAsync(
IServiceProvider sp, FlowDebugRunMethodCommand cmd, AuthenticatedUser user, CancellationToken ct)
=> WithinCommandBudgetAsync(sp, (deadline, token) => RunMethodCoreAsync(sp, cmd, user, deadline, token), ct);
@@ -233,6 +242,11 @@ public sealed class FlowDebugRunner
/// Live run of a value-change trigger (design §5.3/§5.4): traced routed write through the
/// site's normal path, then poll the site's trace rows for the spawned run(s).
/// </summary>
/// <param name="sp">The request-scoped service provider.</param>
/// <param name="cmd">The command to execute.</param>
/// <param name="user">The authenticated operator.</param>
/// <param name="ct">Cancellation token.</param>
/// <returns>The run result with the collected trace rows.</returns>
public Task<FlowDebugRunResult> RunValueChangeAsync(
IServiceProvider sp, FlowDebugRunValueChangeCommand cmd, AuthenticatedUser user, CancellationToken ct)
=> WithinCommandBudgetAsync(sp, (deadline, token) => RunValueChangeCoreAsync(sp, cmd, user, deadline, token), ct);
@@ -269,6 +283,11 @@ public sealed class FlowDebugRunner
/// rather than through <see cref="RouteHelper"/> is deliberate: the helper emits its own
/// always-on RouteCall row with no operator actor, which would give the graph two roots.
/// </summary>
/// <param name="sp">The request-scoped service provider.</param>
/// <param name="cmd">The command to execute.</param>
/// <param name="user">The authenticated operator.</param>
/// <param name="ct">Cancellation token.</param>
/// <returns>The run result with the collected trace rows.</returns>
public Task<FlowDebugRunResult> RunScriptAsync(
IServiceProvider sp, FlowDebugRunScriptCommand cmd, AuthenticatedUser user, CancellationToken ct)
=> WithinCommandBudgetAsync(sp, (deadline, token) => RunScriptCoreAsync(sp, cmd, user, deadline, token), ct);
@@ -570,6 +589,11 @@ public sealed class FlowDebugRunner
}
/// <summary>Replay (design §7.1): the stored ScriptTrace chain for any execution id.</summary>
/// <param name="sp">The request-scoped service provider.</param>
/// <param name="cmd">The replay command naming the execution.</param>
/// <param name="ct">Cancellation token.</param>
/// <param name="user">The authenticated operator.</param>
/// <returns>The reconstructed run result read from the stored trace rows.</returns>
public async Task<FlowDebugRunResult> ReplayAsync(
IServiceProvider sp, FlowDebugReplayCommand cmd, CancellationToken ct, AuthenticatedUser user)
{
@@ -627,8 +651,11 @@ public sealed class FlowDebugRunner
private sealed class TraceReadBatch(Guid[] executionIds, DateTime sinceUtc)
{
/// <summary>Execution ids whose trace rows are being read.</summary>
public Guid[] ExecutionIds { get; } = executionIds;
/// <summary>Lower bound (UTC) for the next read.</summary>
public DateTime SinceUtc { get; set; } = sinceUtc;
/// <summary>Keyset cursor: the last event id already consumed, or null.</summary>
public string? AfterEventId { get; set; }
}
src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs
+14 -10
View File
@@ -190,6 +190,11 @@ public class ManagementActor : ReceiveActor
}
/// <summary>Audited database-only application boundary shared by UI and management envelopes.</summary>
/// <param name="services">Scoped service provider used to resolve handlers and repositories.</param>
/// <param name="command">The configuration command to execute.</param>
/// <param name="user">The authenticated user the command runs as.</param>
/// <param name="clientIp">Optional client IP address recorded in the audit entry.</param>
/// <returns>The command's result object, or <c>null</c> when the command produces none.</returns>
public static async Task<object?> ExecuteConfigurationCommandAsync(IServiceProvider services, object command, AuthenticatedUser user, string? clientIp = null)
{
if (!IsConfigurationCommand(command)) throw new ArgumentException("Not a configuration command.", nameof(command));
@@ -1496,8 +1501,8 @@ public class ManagementActor : ReceiveActor
// submit device writes to their permitted sites — same rule C2 applies to
// DeployArtifacts, on a higher-consequence path. Checked before the row exists.
//
// The unresolvable case is fail-CLOSED and answered BEFORE the "not found"
// (arch-review 2026-08-19 P6-5): resolving first and reporting not-found made
// The unresolvable case is fail-CLOSED and answered BEFORE the "not found":
// resolving first and reporting not-found made
// this handler a site-identifier existence oracle for an out-of-scope caller,
// who now gets the same scope denial whether or not the site exists.
var siteRepo = sp.GetRequiredService<ISiteRepository>();
@@ -1560,7 +1565,7 @@ public class ManagementActor : ReceiveActor
// the row's target site — checked BEFORE the TTL/self-approval/CAS chain so an
// out-of-scope verifier can neither consume the Pending transition nor relay.
//
// It is also checked BEFORE the status test (arch-review 2026-08-19 P6-5): the
// It is also checked BEFORE the status test: the
// "is 'Executed', not Pending" message told an out-of-scope caller both that the
// id existed and what had happened to it. An out-of-scope caller now gets the
// same scope denial for every id, existing or not.
@@ -1701,7 +1706,7 @@ public class ManagementActor : ReceiveActor
row.ExecutionError = error;
// UpdateAsync overwrites all columns -> pass the fully-populated entity.
// Safe as a blind write (arch-review 2026-08-19 P6-1): winning the
// Safe as a blind write: winning the
// Pending->Approved CAS above gives this leg EXCLUSIVE ownership of the row from
// that point on. Every other writer transitions only out of Pending —
// TryMarkApprovedAsync, TryMarkRejectedAsync and TryMarkExpiredAsync (both the
@@ -1738,8 +1743,7 @@ public class ManagementActor : ReceiveActor
// Site scope (arch-review R2 N3): the rejecting Verifier must be permitted on
// the row's target site — checked BEFORE the TTL/self-approval chain, and (as on
// approve) BEFORE the status test, so the not-found/status errors stop being an
// existence-and-status oracle for an out-of-scope caller (arch-review 2026-08-19
// P6-5). The CAS below is unchanged.
// existence-and-status oracle for an out-of-scope caller. The CAS below is unchanged.
await EnforceSiteScopeForIdentifier(sp, user, entity.SiteId);
if (!string.Equals(entity.Status, "Pending", StringComparison.Ordinal))
@@ -1759,7 +1763,7 @@ public class ManagementActor : ReceiveActor
"A secured write cannot be verified by its submitter.");
// Compare-and-swap: guards the approve-vs-reject race exactly as approve's own
// CAS guards the two-verifier race (arch-review 2026-08-19 P6-1). The status
// CAS guards the two-verifier race. The status
// check above is a stale read — commands run concurrently, each on its own DI
// scope and across both central nodes on one shared MS SQL — so a concurrent
// approve can win its CAS and enter its multi-second WriteTagAsync relay in the
@@ -2003,8 +2007,8 @@ public class ManagementActor : ReceiveActor
/// Lists data connections, constrained to the caller's permitted sites.
/// </summary>
/// <remarks>
/// Arch-review 2026-08-19 P6-4. The list used to take no user at all and return
/// EVERY site's connections, which nullified the row-98 fail-closed gate on
/// The list used to take no user at all and return
/// EVERY site's connections, which nullified the fail-closed gate on
/// <see cref="HandleGetDataConnection"/> — Get denied exactly what List handed
/// over, and the secret-elided <see cref="DataConnectionPublicShape"/> still
/// leaked site topology plus an id-existence oracle. Scoping now mirrors
@@ -3872,7 +3876,7 @@ public class ManagementActor : ReceiveActor
private static async Task<object?> HandleDebugSnapshot(IServiceProvider sp, DebugSnapshotCommand cmd, AuthenticatedUser user)
{
// Fail-CLOSED before the not-found (arch-review 2026-08-19 P6-5): reporting
// Fail-CLOSED before the not-found: reporting
// "Instance {id} not found" ahead of the scope check made this handler an
// instance-id existence oracle for an out-of-scope caller. The scope denial is
// now the same answer whether or not the instance exists.
src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementEndpoints.cs
+1 -1
View File
@@ -19,7 +19,7 @@ public static class ManagementEndpoints
/// <summary>
/// Shared options for the per-request command payload JSON parse in
/// <see cref="ParseCommand"/> (perf remediation, arch-review WP1.5) — avoids
/// <see cref="ParseCommand"/> — avoids
/// allocating a new <see cref="JsonSerializerOptions"/> per inbound management
/// request. Settings preserved exactly.
/// </summary>