From 738e67acc5a278a3aa626d2dd8ae2562ef3fdbd2 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 16 May 2026 20:58:03 -0400 Subject: [PATCH] =?UTF-8?q?fix(cli):=20resolve=20CLI-002..007=20=E2=80=94?= =?UTF-8?q?=20robust=20response=20rendering,=20URL/JSON=20arg=20validation?= =?UTF-8?q?,=20credential=20env-vars,=20doc=20refresh?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- code-reviews/CLI/findings.md | 69 ++++-- docs/requirements/Component-CLI.md | 198 ++++++++++-------- src/ScadaLink.CLI/CliConfig.cs | 23 ++ src/ScadaLink.CLI/Commands/CommandHelpers.cs | 129 ++++++++---- src/ScadaLink.CLI/Commands/DebugCommands.cs | 14 +- .../Commands/InstanceCommands.cs | 108 +++++++++- src/ScadaLink.CLI/README.md | 2 + tests/ScadaLink.CLI.Tests/CliConfigTests.cs | 1 + .../CommandHelpersTests.cs | 1 + .../CredentialResolutionTests.cs | 71 +++++++ .../InstanceArgumentParsingTests.cs | 98 +++++++++ .../OutputFormatterTests.cs | 1 + .../ResponseRenderingTests.cs | 73 +++++++ tests/ScadaLink.CLI.Tests/TestCollections.cs | 16 ++ .../ScadaLink.CLI.Tests/UrlValidationTests.cs | 31 +++ 15 files changed, 685 insertions(+), 150 deletions(-) create mode 100644 tests/ScadaLink.CLI.Tests/CredentialResolutionTests.cs create mode 100644 tests/ScadaLink.CLI.Tests/InstanceArgumentParsingTests.cs create mode 100644 tests/ScadaLink.CLI.Tests/ResponseRenderingTests.cs create mode 100644 tests/ScadaLink.CLI.Tests/TestCollections.cs create mode 100644 tests/ScadaLink.CLI.Tests/UrlValidationTests.cs diff --git a/code-reviews/CLI/findings.md b/code-reviews/CLI/findings.md index 4e4e8fd..c0570e8 100644 --- a/code-reviews/CLI/findings.md +++ b/code-reviews/CLI/findings.md @@ -8,7 +8,7 @@ | Last reviewed | 2026-05-16 | | Reviewer | claude-agent | | Commit reviewed | `9c60592` | -| Open findings | 12 | +| Open findings | 6 | ## Summary @@ -92,7 +92,7 @@ now call `ResolveFormat`. Regression tests added in `FormatResolutionTests`. |--|--| | Severity | Medium | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/CommandHelpers.cs:59-68`, `src/ScadaLink.CLI/Commands/CommandHelpers.cs:78-80` | **Description** @@ -112,7 +112,13 @@ output" case (print nothing or `(ok)`), and return 0 before attempting to parse. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed against source — +`HandleResponse` tested `JsonData != null`, so an empty success body fell through to +`WriteAsTable` → `JsonDocument.Parse("")` and threw an uncaught `JsonException`. +`HandleResponse` now treats a null-or-whitespace `JsonData` as a "succeeded, no output" +case, prints `(ok)`, and returns 0 before any parse. Regression tests added in +`ResponseRenderingTests` (`HandleResponse_EmptyBody_TableFormat_DoesNotThrow_ReturnsZero`, +`HandleResponse_EmptyBody_JsonFormat_DoesNotThrow_ReturnsZero`). ### CLI-003 — Non-JSON success body crashes table rendering @@ -120,7 +126,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/CommandHelpers.cs:80` | **Description** @@ -139,7 +145,11 @@ printing the raw body verbatim (as the JSON path already does at line 66). **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed — `WriteAsTable` parsed the +body with no `try/catch`. The `JsonDocument.Parse` call is now wrapped in a +`try/catch (JsonException)` that prints the raw body verbatim on failure, mirroring the +raw-body fallback on the JSON path. Regression test +`ResponseRenderingTests.HandleResponse_NonJsonBody_TableFormat_FallsBackToRaw_ReturnsZero`. ### CLI-004 — Malformed `--url` throws an unhandled `UriFormatException` @@ -147,7 +157,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/ManagementHttpClient.cs:13` | **Description** @@ -166,7 +176,12 @@ clean `INVALID_URL` error with exit code 1 on failure. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed — the +`ManagementHttpClient` constructor's `new Uri(...)` ran outside the `SendCommandAsync` +`try/catch`. Added `CommandHelpers.IsValidManagementUrl`, which checks for an absolute +http/https URL via `Uri.TryCreate`. Both `CommandHelpers.ExecuteCommandAsync` and +`DebugCommands.BuildStream` now validate the resolved URL up front and emit a clean +`INVALID_URL` error with exit code 1. Regression tests in `UrlValidationTests`. ### CLI-005 — Malformed `--bindings` / `--overrides` JSON throws unhandled exceptions @@ -174,7 +189,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Error handling & resilience | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Commands/InstanceCommands.cs:55-58`, `src/ScadaLink.CLI/Commands/InstanceCommands.cs:181-182` | **Description** @@ -195,7 +210,15 @@ code and return 1. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed — both `set-bindings` and +`set-overrides` deserialized and indexed JSON inline with no `try/catch`. Extracted the +parsing into testable `InstanceCommands.TryParseBindings` / `TryParseOverrides` helpers +that catch `JsonException`, guard against null results, and (for bindings) validate pair +arity and element kinds (`JsonValueKind`) instead of letting `ArgumentOutOfRangeException` +/ `InvalidOperationException` escape. The command actions now emit a clean +`INVALID_ARGUMENT` error and return 1 on failure. Regression tests in +`InstanceArgumentParsingTests` (8 tests covering valid input, malformed JSON, short pairs, +wrong element types, and JSON null). ### CLI-006 — Password is passed as a command-line argument with no safer alternative @@ -203,7 +226,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Security | -| Status | Open | +| Status | Resolved | | Location | `src/ScadaLink.CLI/Program.cs:9`, `src/ScadaLink.CLI/Commands/CommandHelpers.cs:36-44` | **Description** @@ -225,7 +248,17 @@ supplied. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Root cause confirmed — credentials had no +non-command-line source. Added `SCADALINK_USERNAME` / `SCADALINK_PASSWORD` environment +fallbacks: `CliConfig.Load` now reads them into new `CliConfig.Username` / `Password` +properties (credentials are sourced from environment variables only, never the config +file, so they are not persisted). `CommandHelpers.ResolveCredential` resolves precedence +(explicit `--username`/`--password` → env var); both `ExecuteCommandAsync` and +`DebugCommands.BuildStream` use it. The design doc and the in-repo `README.md` now +document that `--password` on the command line is discouraged. The `--password-stdin` +option / interactive prompt was not added — the env-var fallback fully satisfies the +CI/CD safe-credential need; a stdin/prompt variant can be a follow-up if interactive use +demands it. Regression tests in `CredentialResolutionTests`. ### CLI-007 — `Component-CLI.md` command surface is substantially stale @@ -233,7 +266,7 @@ _Unresolved._ |--|--| | Severity | Medium | | Category | Design-document adherence | -| Status | Open | +| Status | Resolved | | Location | `docs/requirements/Component-CLI.md:51-211` (vs. all files under `src/ScadaLink.CLI/Commands/`) | **Description** @@ -267,7 +300,17 @@ authoritative. **Resolution** -_Unresolved._ +Resolved 2026-05-16 (commit pending). Drift confirmed against every file under +`src/ScadaLink.CLI/Commands/`. Regenerated the entire "Command Structure" section of +`Component-CLI.md` from the actual command tree: all entities are now keyed by integer +`--id`; the non-existent `--file` option is removed; create/update commands list their +real individual flags; non-existent commands (`template diff`, `instance +bind-connections`/`assign-area`, `data-connection assign/unassign`, `security api-key +enable/disable`) are removed; previously-omitted commands (`instance alarm-override +set/delete/list`, `external-system method` subgroup, `site deploy-artifacts`) are added. +A note now points to `src/ScadaLink.CLI/README.md` as the authoritative reference. The +Configuration section also documents the new `SCADALINK_USERNAME`/`SCADALINK_PASSWORD` +env vars (see CLI-006). ### CLI-008 — `--format` value is not validated diff --git a/docs/requirements/Component-CLI.md b/docs/requirements/Component-CLI.md index f80f46b..01614bf 100644 --- a/docs/requirements/Component-CLI.md +++ b/docs/requirements/Component-CLI.md @@ -48,127 +48,142 @@ The CLI uses a hierarchical subcommand structure mirroring the Management Servic scadalink [options] ``` +All entities are identified by their integer **ID** (via `--id`, `--template-id`, +`--site-id`, etc.), not by name. Create/update commands take individual flags — there +is no `--file` option. The authoritative, always-current reference is the in-repo +`src/ScadaLink.CLI/README.md`; the command lists below mirror the implemented command +tree at the time of writing. + ### Template Commands ``` -scadalink template list [--format json|table] -scadalink template get [--format json|table] -scadalink template create --name [--parent ] --file -scadalink template update --file -scadalink template delete -scadalink template validate -scadalink template diff -scadalink template attribute add --template-id --name --data-type [--default-value ] [--tag-path ] -scadalink template attribute update --template-id --name [--data-type ] [--default-value ] [--tag-path ] -scadalink template attribute delete --template-id --name -scadalink template alarm add --template-id --name --trigger-attribute --condition --setpoint [--severity ] [--notification-list ] -scadalink template alarm update --template-id --name [--condition ] [--setpoint ] [--severity ] [--notification-list ] -scadalink template alarm delete --template-id --name -scadalink template script add --template-id --name --trigger-type [--trigger-attribute ] [--interval ] --code -scadalink template script update --template-id --name [--trigger-type ] [--trigger-attribute ] [--interval ] [--code ] -scadalink template script delete --template-id --name -scadalink template composition add --template-id --module-template-id --instance-name +scadalink template list +scadalink template get --id +scadalink template create --name [--description ] [--parent-id ] +scadalink template update --id [--name ] [--description ] [--parent-id ] +scadalink template validate --id +scadalink template delete --id +scadalink template attribute add --template-id --name --data-type [--value ] [--description ] [--data-source ] [--locked ] +scadalink template attribute update --id [--name ] [--data-type ] [--value ] [--description ] [--data-source ] [--locked ] +scadalink template attribute delete --id +scadalink template alarm add --template-id --name --trigger-type --priority [--description ] [--trigger-config ] [--locked ] +scadalink template alarm update --id [--name ] [--trigger-type ] [--priority ] [--description ] [--trigger-config ] [--locked ] +scadalink template alarm delete --id +scadalink template script add --template-id --name --code --trigger-type [--trigger-config ] [--locked ] [--parameters ] [--return-def ] +scadalink template script update --id [--name ] [--code ] [--trigger-type ] [--trigger-config ] [--locked ] [--parameters ] [--return-def ] +scadalink template script delete --id +scadalink template composition add --template-id --instance-name --composed-template-id scadalink template composition delete --template-id --instance-name ``` ### Instance Commands ``` -scadalink instance list [--site ] [--area ] [--format json|table] -scadalink instance get [--format json|table] -scadalink instance create --template --site --code [--area ] -scadalink instance set-overrides --file -scadalink instance set-bindings --bindings -scadalink instance bind-connections --file -scadalink instance assign-area --area -scadalink instance enable -scadalink instance disable -scadalink instance delete +scadalink instance list [--site-id ] [--template-id ] [--search ] +scadalink instance get --id +scadalink instance create --name --template-id --site-id [--area-id ] +scadalink instance set-bindings --id --bindings +scadalink instance set-overrides --id --overrides +scadalink instance alarm-override set --instance-id --alarm [--trigger-config ] [--priority ] +scadalink instance alarm-override delete --instance-id --alarm +scadalink instance alarm-override list --instance-id +scadalink instance set-area --id [--area-id ] +scadalink instance diff --id +scadalink instance deploy --id +scadalink instance enable --id +scadalink instance disable --id +scadalink instance delete --id ``` +`--bindings` is a JSON array of `[attributeName, dataConnectionId]` pairs, e.g. +`[["Speed", 5], ["Mode", 7]]`. `--overrides` is a JSON object of attribute name to +value, e.g. `{"Speed": "100", "Mode": null}`. + ### Site Commands ``` -scadalink site list [--format json|table] -scadalink site get [--format json|table] -scadalink site create --name --id [--node-a-address ] [--node-b-address ] [--grpc-node-a-address ] [--grpc-node-b-address ] -scadalink site update --file -scadalink site delete -scadalink site area list -scadalink site area create --name [--parent ] -scadalink site area update --name [--new-name ] [--parent ] -scadalink site area delete --name +scadalink site list +scadalink site get --id +scadalink site create --identifier --name [--description ] [--node-a-address ] [--node-b-address ] [--grpc-node-a-address ] [--grpc-node-b-address ] +scadalink site update --id [--name ] [--description ] [--node-a-address ] [--node-b-address ] [--grpc-node-a-address ] [--grpc-node-b-address ] +scadalink site delete --id +scadalink site area list --site-id +scadalink site area create --site-id --name [--parent-id ] +scadalink site area update --id --name +scadalink site area delete --id +scadalink site deploy-artifacts [--site-id ] ``` ### Deployment Commands ``` -scadalink deploy instance -scadalink deploy artifacts [--site ] [--type ] -scadalink deploy status [--format json|table] +scadalink deploy instance --id +scadalink deploy artifacts [--site-id ] +scadalink deploy status [--instance-id ] [--status ] [--page ] [--page-size ] ``` ### Data Connection Commands ``` -scadalink data-connection list [--format json|table] -scadalink data-connection get [--format json|table] -scadalink data-connection create --file -scadalink data-connection update --file -scadalink data-connection delete -scadalink data-connection assign --site -scadalink data-connection unassign --site +scadalink data-connection list [--site-id ] +scadalink data-connection get --id +scadalink data-connection create --site-id --name --protocol [--backup-config ] [--failover-retry-count ] +scadalink data-connection update --id [--name ] [--protocol ] [--backup-config ] [--failover-retry-count ] +scadalink data-connection delete --id ``` ### External System Commands ``` -scadalink external-system list [--format json|table] -scadalink external-system get [--format json|table] -scadalink external-system create --file -scadalink external-system update --file -scadalink external-system delete +scadalink external-system list +scadalink external-system get --id +scadalink external-system create --name --endpoint-url --auth-type [--auth-config ] +scadalink external-system update --id [--name ] [--endpoint-url ] [--auth-type ] [--auth-config ] +scadalink external-system delete --id +scadalink external-system method list --external-system-id +scadalink external-system method get --id +scadalink external-system method create --external-system-id --name --http-method --path [--params ] [--return ] +scadalink external-system method update --id [--name ] [--http-method ] [--path ] [--params ] [--return ] +scadalink external-system method delete --id ``` ### Notification Commands ``` -scadalink notification list [--format json|table] -scadalink notification get [--format json|table] -scadalink notification create --file -scadalink notification update --file -scadalink notification delete -scadalink notification smtp list [--format json|table] -scadalink notification smtp update --file +scadalink notification list +scadalink notification get --id +scadalink notification create --name --emails +scadalink notification update --id [--name ] [--emails ] +scadalink notification delete --id +scadalink notification smtp list +scadalink notification smtp update --id --server --port --auth-mode --from-address ``` ### Security Commands ``` -scadalink security api-key list [--format json|table] +scadalink security api-key list scadalink security api-key create --name -scadalink security api-key update [--name ] [--enabled ] -scadalink security api-key enable -scadalink security api-key disable -scadalink security api-key delete -scadalink security role-mapping list [--format json|table] -scadalink security role-mapping create --group --role [--site ] -scadalink security role-mapping update --id [--group ] [--role ] -scadalink security role-mapping delete --group --role -scadalink security scope-rule list [--role-mapping-id ] [--format json|table] -scadalink security scope-rule add --role-mapping-id --site-id +scadalink security api-key update --id --enabled +scadalink security api-key delete --id +scadalink security role-mapping list +scadalink security role-mapping create --ldap-group --role +scadalink security role-mapping update --id [--ldap-group ] [--role ] +scadalink security role-mapping delete --id +scadalink security scope-rule list [--mapping-id ] +scadalink security scope-rule add --mapping-id --site-id scadalink security scope-rule delete --id ``` ### Audit Log Commands ``` -scadalink audit-log query [--user ] [--entity-type ] [--from ] [--to ] [--format json|table] +scadalink audit-log query [--user ] [--entity-type ] [--action ] [--from ] [--to ] [--page ] [--page-size ] ``` ### Health Commands ``` -scadalink health summary [--format json|table] -scadalink health site [--format json|table] -scadalink health event-log --site-identifier [--from ] [--to ] [--search ] [--page ] [--page-size ] [--format json|table] -scadalink health parked-messages --site-identifier [--page ] [--page-size ] [--format json|table] +scadalink health summary +scadalink health site --identifier +scadalink health event-log --site [--event-type ] [--severity ] [--keyword ] [--from ] [--to ] [--page ] [--page-size ] [--instance-name ] +scadalink health parked-messages --site [--page ] [--page-size ] ``` ### Debug Commands ``` -scadalink debug snapshot --id [--format json|table] -scadalink debug stream --id [--url ...] [--username ...] [--password ...] +scadalink debug snapshot --id +scadalink debug stream --id ``` The `debug snapshot` command retrieves a point-in-time snapshot via the HTTP Management API. @@ -185,31 +200,33 @@ Unlike `debug snapshot` (which uses the HTTP Management API), `debug stream` use ### Shared Script Commands ``` -scadalink shared-script list [--format json|table] -scadalink shared-script get --id [--format json|table] -scadalink shared-script create --name --code -scadalink shared-script update --id [--name ] [--code ] +scadalink shared-script list +scadalink shared-script get --id +scadalink shared-script create --name --code [--parameters ] [--return-def ] +scadalink shared-script update --id [--name ] [--code ] [--parameters ] [--return-def ] scadalink shared-script delete --id ``` ### Database Connection Commands ``` -scadalink db-connection list [--format json|table] -scadalink db-connection get --id [--format json|table] -scadalink db-connection create --name --connection-string [--provider ] -scadalink db-connection update --id [--name ] [--connection-string ] [--provider ] +scadalink db-connection list +scadalink db-connection get --id +scadalink db-connection create --name --connection-string +scadalink db-connection update --id [--name ] [--connection-string ] scadalink db-connection delete --id ``` ### Inbound API Method Commands ``` -scadalink api-method list [--format json|table] -scadalink api-method get --id [--format json|table] -scadalink api-method create --name --code [--description ] -scadalink api-method update --id [--name ] [--code ] [--description ] +scadalink api-method list +scadalink api-method get --id +scadalink api-method create --name --script [--timeout ] [--parameters ] [--return-def ] +scadalink api-method update --id [--script ] [--timeout ] [--parameters ] [--return-def ] scadalink api-method delete --id ``` +The `--format json|table` option is recursive and accepted on every command above. + ## Configuration Configuration is resolved in the following priority order (highest wins): @@ -218,7 +235,10 @@ Configuration is resolved in the following priority order (highest wins): 2. **Environment variables**: - `SCADALINK_MANAGEMENT_URL` — Management API URL (e.g., `http://central-host:5000`). - `SCADALINK_FORMAT` — Default output format (`json` or `table`). -3. **Configuration file**: `~/.scadalink/config.json` — Persistent defaults for management URL and output format. + - `SCADALINK_USERNAME` / `SCADALINK_PASSWORD` — LDAP credentials. Preferred over + `--password` on the command line, which is visible in process listings and shell + history. Credentials are never read from the config file. +3. **Configuration file**: `~/.scadalink/config.json` — Persistent defaults for management URL and output format only (never credentials). ### Configuration File Format diff --git a/src/ScadaLink.CLI/CliConfig.cs b/src/ScadaLink.CLI/CliConfig.cs index 80d775e..91fff5c 100644 --- a/src/ScadaLink.CLI/CliConfig.cs +++ b/src/ScadaLink.CLI/CliConfig.cs @@ -7,6 +7,20 @@ public class CliConfig public string? ManagementUrl { get; set; } public string DefaultFormat { get; set; } = "json"; + /// + /// LDAP username from the SCADALINK_USERNAME environment variable, if set. + /// Credentials are intentionally only sourced from environment variables (or the + /// command line) — never from the config file — so they are not persisted to disk. + /// + public string? Username { get; set; } + + /// + /// LDAP password from the SCADALINK_PASSWORD environment variable, if set. + /// Provides a safer alternative to --password, which leaks into process + /// listings and shell history. + /// + public string? Password { get; set; } + public static CliConfig Load() { var config = new CliConfig(); @@ -38,6 +52,15 @@ public class CliConfig if (!string.IsNullOrEmpty(envFormat)) config.DefaultFormat = envFormat; + // Credentials from environment variables only (never the config file). + var envUsername = Environment.GetEnvironmentVariable("SCADALINK_USERNAME"); + if (!string.IsNullOrEmpty(envUsername)) + config.Username = envUsername; + + var envPassword = Environment.GetEnvironmentVariable("SCADALINK_PASSWORD"); + if (!string.IsNullOrEmpty(envPassword)) + config.Password = envPassword; + return config; } diff --git a/src/ScadaLink.CLI/Commands/CommandHelpers.cs b/src/ScadaLink.CLI/Commands/CommandHelpers.cs index f2f5f20..eb393c7 100644 --- a/src/ScadaLink.CLI/Commands/CommandHelpers.cs +++ b/src/ScadaLink.CLI/Commands/CommandHelpers.cs @@ -31,14 +31,23 @@ internal static class CommandHelpers return 1; } - // Validate credentials - var username = result.GetValue(usernameOption); - var password = result.GetValue(passwordOption); + if (!IsValidManagementUrl(url)) + { + OutputFormatter.WriteError( + $"Invalid management URL '{url}'. Expected an absolute http/https URL (e.g. http://localhost:9001).", + "INVALID_URL"); + return 1; + } + + // Resolve credentials: command-line options take precedence, then the + // SCADALINK_USERNAME / SCADALINK_PASSWORD environment variables. + var username = ResolveCredential(result.GetValue(usernameOption), config.Username); + var password = ResolveCredential(result.GetValue(passwordOption), config.Password); if (string.IsNullOrWhiteSpace(username) || string.IsNullOrWhiteSpace(password)) { OutputFormatter.WriteError( - "Credentials required. Use --username and --password options.", + "Credentials required. Use --username/--password or set SCADALINK_USERNAME/SCADALINK_PASSWORD.", "NO_CREDENTIALS"); return 1; } @@ -74,10 +83,40 @@ internal static class CommandHelpers return string.IsNullOrWhiteSpace(config.DefaultFormat) ? "json" : config.DefaultFormat; } + /// + /// Resolves a single credential: an explicit command-line value wins, otherwise the + /// environment-variable fallback (from ) is used. + /// + internal static string? ResolveCredential(string? commandLineValue, string? envValue) + => string.IsNullOrWhiteSpace(commandLineValue) ? envValue : commandLineValue; + + /// + /// Validates that a management URL is an absolute http/https URL. A malformed URL + /// (missing scheme, empty, or a non-http scheme) would otherwise reach + /// new Uri(...) in the constructor and throw + /// an unhandled . + /// + internal static bool IsValidManagementUrl(string? url) + { + if (string.IsNullOrWhiteSpace(url)) + return false; + + return Uri.TryCreate(url, UriKind.Absolute, out var uri) + && (uri.Scheme == Uri.UriSchemeHttp || uri.Scheme == Uri.UriSchemeHttps); + } + internal static int HandleResponse(ManagementResponse response, string format) { if (response.JsonData != null) { + // A success status with an empty/whitespace body (e.g. a 204 from a delete) + // is a "command succeeded, no output" case — do not attempt to parse it. + if (string.IsNullOrWhiteSpace(response.JsonData)) + { + Console.WriteLine("(ok)"); + return 0; + } + if (string.Equals(format, "table", StringComparison.OrdinalIgnoreCase)) { WriteAsTable(response.JsonData); @@ -98,46 +137,62 @@ internal static class CommandHelpers private static void WriteAsTable(string json) { - using var doc = JsonDocument.Parse(json); - var root = doc.RootElement; - - if (root.ValueKind == JsonValueKind.Array) + JsonDocument doc; + try { - var items = root.EnumerateArray().ToList(); - if (items.Count == 0) - { - Console.WriteLine("(no results)"); - return; - } + doc = JsonDocument.Parse(json); + } + catch (JsonException) + { + // The server returned a success status but a non-JSON body (e.g. a proxy + // HTML error page, or a plain-text message). Print it verbatim rather than + // crashing — mirrors the raw-body fallback on the JSON path. + Console.WriteLine(json); + return; + } - var headers = items[0].ValueKind == JsonValueKind.Object - ? items[0].EnumerateObject().Select(p => p.Name).ToArray() - : new[] { "Value" }; + using (doc) + { + var root = doc.RootElement; - var rows = items.Select(item => + if (root.ValueKind == JsonValueKind.Array) { - if (item.ValueKind == JsonValueKind.Object) + var items = root.EnumerateArray().ToList(); + if (items.Count == 0) { - return headers.Select(h => - item.TryGetProperty(h, out var val) - ? val.ValueKind == JsonValueKind.Null ? "" : val.ToString() - : "").ToArray(); + Console.WriteLine("(no results)"); + return; } - return new[] { item.ToString() }; - }); - OutputFormatter.WriteTable(rows, headers); - } - else if (root.ValueKind == JsonValueKind.Object) - { - var headers = new[] { "Property", "Value" }; - var rows = root.EnumerateObject().Select(p => - new[] { p.Name, p.Value.ValueKind == JsonValueKind.Null ? "" : p.Value.ToString() }); - OutputFormatter.WriteTable(rows, headers); - } - else - { - Console.WriteLine(root.ToString()); + var headers = items[0].ValueKind == JsonValueKind.Object + ? items[0].EnumerateObject().Select(p => p.Name).ToArray() + : new[] { "Value" }; + + var rows = items.Select(item => + { + if (item.ValueKind == JsonValueKind.Object) + { + return headers.Select(h => + item.TryGetProperty(h, out var val) + ? val.ValueKind == JsonValueKind.Null ? "" : val.ToString() + : "").ToArray(); + } + return new[] { item.ToString() }; + }); + + OutputFormatter.WriteTable(rows, headers); + } + else if (root.ValueKind == JsonValueKind.Object) + { + var headers = new[] { "Property", "Value" }; + var rows = root.EnumerateObject().Select(p => + new[] { p.Name, p.Value.ValueKind == JsonValueKind.Null ? "" : p.Value.ToString() }); + OutputFormatter.WriteTable(rows, headers); + } + else + { + Console.WriteLine(root.ToString()); + } } } } diff --git a/src/ScadaLink.CLI/Commands/DebugCommands.cs b/src/ScadaLink.CLI/Commands/DebugCommands.cs index 3dc558d..b6bcc70 100644 --- a/src/ScadaLink.CLI/Commands/DebugCommands.cs +++ b/src/ScadaLink.CLI/Commands/DebugCommands.cs @@ -57,13 +57,21 @@ public static class DebugCommands return 1; } - var username = result.GetValue(usernameOption); - var password = result.GetValue(passwordOption); + if (!CommandHelpers.IsValidManagementUrl(url)) + { + OutputFormatter.WriteError( + $"Invalid management URL '{url}'. Expected an absolute http/https URL (e.g. http://localhost:9001).", + "INVALID_URL"); + return 1; + } + + var username = CommandHelpers.ResolveCredential(result.GetValue(usernameOption), config.Username); + var password = CommandHelpers.ResolveCredential(result.GetValue(passwordOption), config.Password); if (string.IsNullOrWhiteSpace(username) || string.IsNullOrWhiteSpace(password)) { OutputFormatter.WriteError( - "Credentials required. Use --username and --password options.", + "Credentials required. Use --username/--password or set SCADALINK_USERNAME/SCADALINK_PASSWORD.", "NO_CREDENTIALS"); return 1; } diff --git a/src/ScadaLink.CLI/Commands/InstanceCommands.cs b/src/ScadaLink.CLI/Commands/InstanceCommands.cs index 691c973..efdf3ad 100644 --- a/src/ScadaLink.CLI/Commands/InstanceCommands.cs +++ b/src/ScadaLink.CLI/Commands/InstanceCommands.cs @@ -52,17 +52,106 @@ public static class InstanceCommands { var id = result.GetValue(idOption); var bindingsJson = result.GetValue(bindingsOption)!; - var pairs = System.Text.Json.JsonSerializer.Deserialize>>(bindingsJson) - ?? throw new InvalidOperationException("Invalid bindings JSON"); - var bindings = pairs.Select(p => - (p[0].GetString()!, p[1].GetInt32())).ToList(); + if (!TryParseBindings(bindingsJson, out var bindings, out var error)) + { + OutputFormatter.WriteError(error!, "INVALID_ARGUMENT"); + return 1; + } return await CommandHelpers.ExecuteCommandAsync( result, urlOption, formatOption, usernameOption, passwordOption, - new SetConnectionBindingsCommand(id, bindings)); + new SetConnectionBindingsCommand(id, bindings!)); }); return cmd; } + /// + /// Parses the --bindings argument — a JSON array of + /// [attributeName, dataConnectionId] pairs — into a typed list. + /// Returns false with a descriptive instead of + /// throwing when the JSON is malformed, a pair has the wrong arity, or an element + /// has the wrong type. + /// + internal static bool TryParseBindings( + string json, + out List<(string, int)>? bindings, + out string? error) + { + bindings = null; + error = null; + try + { + var pairs = System.Text.Json.JsonSerializer + .Deserialize>>(json); + if (pairs == null) + { + error = "Bindings JSON must be a non-null array of [attributeName, dataConnectionId] pairs."; + return false; + } + + var result = new List<(string, int)>(pairs.Count); + foreach (var pair in pairs) + { + if (pair.Count != 2) + { + error = "Each binding must be a [attributeName, dataConnectionId] pair of exactly two elements."; + return false; + } + if (pair[0].ValueKind != System.Text.Json.JsonValueKind.String) + { + error = "The first element of each binding (attributeName) must be a string."; + return false; + } + if (pair[1].ValueKind != System.Text.Json.JsonValueKind.Number + || !pair[1].TryGetInt32(out var connectionId)) + { + error = "The second element of each binding (dataConnectionId) must be an integer."; + return false; + } + result.Add((pair[0].GetString()!, connectionId)); + } + + bindings = result; + return true; + } + catch (System.Text.Json.JsonException ex) + { + error = $"Invalid bindings JSON: {ex.Message}"; + return false; + } + } + + /// + /// Parses the --overrides argument — a JSON object of + /// attributeName -> value pairs — into a typed dictionary. Returns + /// false with a descriptive instead of throwing + /// when the JSON is malformed or null. + /// + internal static bool TryParseOverrides( + string json, + out Dictionary? overrides, + out string? error) + { + overrides = null; + error = null; + try + { + var parsed = System.Text.Json.JsonSerializer + .Deserialize>(json); + if (parsed == null) + { + error = "Overrides JSON must be a non-null object of attribute name -> value pairs."; + return false; + } + overrides = parsed; + return true; + } + catch (System.Text.Json.JsonException ex) + { + error = $"Invalid overrides JSON: {ex.Message}"; + return false; + } + } + private static Command BuildList(Option urlOption, Option formatOption, Option usernameOption, Option passwordOption) { var siteIdOption = new Option("--site-id") { Description = "Filter by site ID" }; @@ -178,11 +267,14 @@ public static class InstanceCommands { var id = result.GetValue(idOption); var overridesJson = result.GetValue(overridesOption)!; - var overrides = System.Text.Json.JsonSerializer.Deserialize>(overridesJson) - ?? throw new InvalidOperationException("Invalid overrides JSON"); + if (!TryParseOverrides(overridesJson, out var overrides, out var error)) + { + OutputFormatter.WriteError(error!, "INVALID_ARGUMENT"); + return 1; + } return await CommandHelpers.ExecuteCommandAsync( result, urlOption, formatOption, usernameOption, passwordOption, - new SetInstanceOverridesCommand(id, overrides)); + new SetInstanceOverridesCommand(id, overrides!)); }); return cmd; } diff --git a/src/ScadaLink.CLI/README.md b/src/ScadaLink.CLI/README.md index 7f31fa8..d6b7ade 100644 --- a/src/ScadaLink.CLI/README.md +++ b/src/ScadaLink.CLI/README.md @@ -59,6 +59,8 @@ For the Docker test environment, see `docker/README.md` for a ready-to-use confi |----------|-------------| | `SCADALINK_MANAGEMENT_URL` | Management API URL (overrides config file) | | `SCADALINK_FORMAT` | Default output format (overrides config file) | +| `SCADALINK_USERNAME` | LDAP username (fallback when `--username` is not supplied) | +| `SCADALINK_PASSWORD` | LDAP password (fallback when `--password` is not supplied). Preferred over `--password` on the command line, which leaks into process listings and shell history. | ## Output diff --git a/tests/ScadaLink.CLI.Tests/CliConfigTests.cs b/tests/ScadaLink.CLI.Tests/CliConfigTests.cs index 0c179e4..0185a06 100644 --- a/tests/ScadaLink.CLI.Tests/CliConfigTests.cs +++ b/tests/ScadaLink.CLI.Tests/CliConfigTests.cs @@ -2,6 +2,7 @@ using ScadaLink.CLI; namespace ScadaLink.CLI.Tests; +[Collection("Environment")] public class CliConfigTests { [Fact] diff --git a/tests/ScadaLink.CLI.Tests/CommandHelpersTests.cs b/tests/ScadaLink.CLI.Tests/CommandHelpersTests.cs index 51d84da..7df8381 100644 --- a/tests/ScadaLink.CLI.Tests/CommandHelpersTests.cs +++ b/tests/ScadaLink.CLI.Tests/CommandHelpersTests.cs @@ -3,6 +3,7 @@ using ScadaLink.CLI.Commands; namespace ScadaLink.CLI.Tests; +[Collection("Console")] public class CommandHelpersTests { [Fact] diff --git a/tests/ScadaLink.CLI.Tests/CredentialResolutionTests.cs b/tests/ScadaLink.CLI.Tests/CredentialResolutionTests.cs new file mode 100644 index 0000000..2249b97 --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/CredentialResolutionTests.cs @@ -0,0 +1,71 @@ +using ScadaLink.CLI; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for CLI-006 — credentials could only be supplied via the +/// --password command-line option, which leaks into process listings and +/// shell history. A SCADALINK_PASSWORD / SCADALINK_USERNAME environment +/// fallback gives CI/CD a safer alternative. +/// +[Collection("Environment")] +public class CredentialResolutionTests +{ + [Fact] + public void Load_Password_FromEnvironment() + { + var orig = Environment.GetEnvironmentVariable("SCADALINK_PASSWORD"); + try + { + Environment.SetEnvironmentVariable("SCADALINK_PASSWORD", "s3cret"); + + var config = CliConfig.Load(); + + Assert.Equal("s3cret", config.Password); + } + finally + { + Environment.SetEnvironmentVariable("SCADALINK_PASSWORD", orig); + } + } + + [Fact] + public void Load_Username_FromEnvironment() + { + var orig = Environment.GetEnvironmentVariable("SCADALINK_USERNAME"); + try + { + Environment.SetEnvironmentVariable("SCADALINK_USERNAME", "ci-user"); + + var config = CliConfig.Load(); + + Assert.Equal("ci-user", config.Username); + } + finally + { + Environment.SetEnvironmentVariable("SCADALINK_USERNAME", orig); + } + } + + [Fact] + public void Load_NoCredentialEnvVars_LeavesCredentialsNull() + { + var origUser = Environment.GetEnvironmentVariable("SCADALINK_USERNAME"); + var origPass = Environment.GetEnvironmentVariable("SCADALINK_PASSWORD"); + try + { + Environment.SetEnvironmentVariable("SCADALINK_USERNAME", null); + Environment.SetEnvironmentVariable("SCADALINK_PASSWORD", null); + + var config = CliConfig.Load(); + + Assert.Null(config.Username); + Assert.Null(config.Password); + } + finally + { + Environment.SetEnvironmentVariable("SCADALINK_USERNAME", origUser); + Environment.SetEnvironmentVariable("SCADALINK_PASSWORD", origPass); + } + } +} diff --git a/tests/ScadaLink.CLI.Tests/InstanceArgumentParsingTests.cs b/tests/ScadaLink.CLI.Tests/InstanceArgumentParsingTests.cs new file mode 100644 index 0000000..b9d3c9d --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/InstanceArgumentParsingTests.cs @@ -0,0 +1,98 @@ +using ScadaLink.CLI.Commands; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for CLI-005 — malformed --bindings / --overrides JSON +/// previously threw unhandled exceptions instead of producing a clean validation error. +/// +public class InstanceArgumentParsingTests +{ + [Fact] + public void ParseBindings_ValidJson_ReturnsPairs() + { + var ok = InstanceCommands.TryParseBindings( + "[[\"Speed\", 5], [\"Mode\", 7]]", out var bindings, out var error); + + Assert.True(ok); + Assert.Null(error); + Assert.Equal(2, bindings!.Count); + Assert.Equal(("Speed", 5), bindings[0]); + Assert.Equal(("Mode", 7), bindings[1]); + } + + [Fact] + public void ParseBindings_MalformedJson_ReturnsErrorNotException() + { + var ok = InstanceCommands.TryParseBindings("not json", out var bindings, out var error); + + Assert.False(ok); + Assert.Null(bindings); + Assert.NotNull(error); + } + + [Fact] + public void ParseBindings_ShortPair_ReturnsErrorNotException() + { + // A pair with fewer than two elements previously threw ArgumentOutOfRangeException. + var ok = InstanceCommands.TryParseBindings("[[\"Speed\"]]", out var bindings, out var error); + + Assert.False(ok); + Assert.Null(bindings); + Assert.NotNull(error); + } + + [Fact] + public void ParseBindings_WrongElementTypes_ReturnsErrorNotException() + { + // A non-string name / non-int id previously threw InvalidOperationException. + var ok = InstanceCommands.TryParseBindings("[[5, \"Speed\"]]", out var bindings, out var error); + + Assert.False(ok); + Assert.Null(bindings); + Assert.NotNull(error); + } + + [Fact] + public void ParseBindings_JsonNull_ReturnsErrorNotException() + { + var ok = InstanceCommands.TryParseBindings("null", out var bindings, out var error); + + Assert.False(ok); + Assert.Null(bindings); + Assert.NotNull(error); + } + + [Fact] + public void ParseOverrides_ValidJson_ReturnsDictionary() + { + var ok = InstanceCommands.TryParseOverrides( + "{\"Speed\": \"100\", \"Mode\": null}", out var overrides, out var error); + + Assert.True(ok); + Assert.Null(error); + Assert.Equal(2, overrides!.Count); + Assert.Equal("100", overrides["Speed"]); + Assert.Null(overrides["Mode"]); + } + + [Fact] + public void ParseOverrides_MalformedJson_ReturnsErrorNotException() + { + var ok = InstanceCommands.TryParseOverrides("{bad json", out var overrides, out var error); + + Assert.False(ok); + Assert.Null(overrides); + Assert.NotNull(error); + } + + [Fact] + public void ParseOverrides_JsonNull_ReturnsErrorNotException() + { + var ok = InstanceCommands.TryParseOverrides("null", out var overrides, out var error); + + Assert.False(ok); + Assert.Null(overrides); + Assert.NotNull(error); + } +} diff --git a/tests/ScadaLink.CLI.Tests/OutputFormatterTests.cs b/tests/ScadaLink.CLI.Tests/OutputFormatterTests.cs index aaf42f9..3c5a89a 100644 --- a/tests/ScadaLink.CLI.Tests/OutputFormatterTests.cs +++ b/tests/ScadaLink.CLI.Tests/OutputFormatterTests.cs @@ -2,6 +2,7 @@ using ScadaLink.CLI; namespace ScadaLink.CLI.Tests; +[Collection("Console")] public class OutputFormatterTests { [Fact] diff --git a/tests/ScadaLink.CLI.Tests/ResponseRenderingTests.cs b/tests/ScadaLink.CLI.Tests/ResponseRenderingTests.cs new file mode 100644 index 0000000..ed0401b --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/ResponseRenderingTests.cs @@ -0,0 +1,73 @@ +using ScadaLink.CLI; +using ScadaLink.CLI.Commands; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for CLI-002 (empty success body) and CLI-003 (non-JSON success +/// body) — both previously crashed table rendering with an unhandled exception. +/// +[Collection("Console")] +public class ResponseRenderingTests +{ + [Fact] + public void HandleResponse_EmptyBody_TableFormat_DoesNotThrow_ReturnsZero() + { + // CLI-002: a 200/204 with an empty body must be treated as "succeeded, no output". + var writer = new StringWriter(); + Console.SetOut(writer); + + try + { + var response = new ManagementResponse(204, "", null, null); + var exitCode = CommandHelpers.HandleResponse(response, "table"); + + Assert.Equal(0, exitCode); + } + finally + { + Console.SetOut(new StreamWriter(Console.OpenStandardOutput()) { AutoFlush = true }); + } + } + + [Fact] + public void HandleResponse_EmptyBody_JsonFormat_DoesNotThrow_ReturnsZero() + { + var writer = new StringWriter(); + Console.SetOut(writer); + + try + { + var response = new ManagementResponse(200, " ", null, null); + var exitCode = CommandHelpers.HandleResponse(response, "json"); + + Assert.Equal(0, exitCode); + } + finally + { + Console.SetOut(new StreamWriter(Console.OpenStandardOutput()) { AutoFlush = true }); + } + } + + [Fact] + public void HandleResponse_NonJsonBody_TableFormat_FallsBackToRaw_ReturnsZero() + { + // CLI-003: a success status with a non-JSON body (e.g. proxy HTML error page) + // must not crash; it should print the raw body verbatim. + var writer = new StringWriter(); + Console.SetOut(writer); + + try + { + var response = new ManagementResponse(200, "Service Unavailable", null, null); + var exitCode = CommandHelpers.HandleResponse(response, "table"); + + Assert.Equal(0, exitCode); + Assert.Contains("Service Unavailable", writer.ToString()); + } + finally + { + Console.SetOut(new StreamWriter(Console.OpenStandardOutput()) { AutoFlush = true }); + } + } +} diff --git a/tests/ScadaLink.CLI.Tests/TestCollections.cs b/tests/ScadaLink.CLI.Tests/TestCollections.cs new file mode 100644 index 0000000..e47a874 --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/TestCollections.cs @@ -0,0 +1,16 @@ +namespace ScadaLink.CLI.Tests; + +/// +/// xUnit runs test classes in parallel by default. Several CLI test classes redirect +/// the process-global / , +/// which races when they run concurrently. Tests in this collection run serially. +/// +[CollectionDefinition("Console")] +public sealed class ConsoleCollection { } + +/// +/// Test classes that mutate process-global environment variables run serially so they +/// do not observe each other's changes. +/// +[CollectionDefinition("Environment")] +public sealed class EnvironmentCollection { } diff --git a/tests/ScadaLink.CLI.Tests/UrlValidationTests.cs b/tests/ScadaLink.CLI.Tests/UrlValidationTests.cs new file mode 100644 index 0000000..0f9ec88 --- /dev/null +++ b/tests/ScadaLink.CLI.Tests/UrlValidationTests.cs @@ -0,0 +1,31 @@ +using ScadaLink.CLI.Commands; + +namespace ScadaLink.CLI.Tests; + +/// +/// Regression tests for CLI-004 — a malformed --url previously reached +/// new Uri(...) in the constructor +/// and threw an unhandled . +/// +public class UrlValidationTests +{ + [Theory] + [InlineData("http://localhost:9001")] + [InlineData("https://central-host:5000/")] + [InlineData("http://central")] + public void IsValidManagementUrl_AcceptsAbsoluteHttpUrls(string url) + { + Assert.True(CommandHelpers.IsValidManagementUrl(url)); + } + + [Theory] + [InlineData("localhost:9001")] // no scheme + [InlineData("")] // empty + [InlineData(" ")] // whitespace + [InlineData("not a url")] + [InlineData("ftp://central:21")] // non-http scheme + public void IsValidManagementUrl_RejectsMalformedOrNonHttpUrls(string url) + { + Assert.False(CommandHelpers.IsValidManagementUrl(url)); + } +}