diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index 5353b77..10631bd 100644 --- a/archreview/2026-07-12/remediation/00-tracking.md +++ b/archreview/2026-07-12/remediation/00-tracking.md @@ -115,11 +115,11 @@ Full design + implementation for each row lives in the linked domain doc under i | CLI-37 | Medium | P1 | M | CLI-38 (co-land) | Done | Status-array validation must branch on `category` per the proto contract (4-vs-1 divergence) | | CLI-38 | Medium | P1 | S | — | Done | Align .NET/Go/Java on `hresult < 0` — lands old CLI-08, cures design-doc drift | | CLI-39 | Medium | P1 | S | CLI-35..38, CLI-45 (land last) | Not started | Bump client versions off published 0.1.2 (converge on 0.2.0); registry-collision guard in pack-clients.ps1 | -| CLI-40 | Low | — | M | — | Not started | Port the exact-secret credential scrub to Rust/Java/.NET | -| CLI-41 | Low | — | M | — | Not started | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem | +| CLI-40 | Low | — | M | — | Done | Port the exact-secret credential scrub to Rust/Java/.NET | +| CLI-41 | Low | — | M | — | Done | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem | | CLI-42 | Low | P1 | S | — | Not started | Document the vendored Rust proto layout (CLI-02's missing doc half) | | CLI-43 | Low | — | S | — | Not started | Java style guide still prescribes "Java 21 preferred" | -| CLI-44 | Low | — | S | — | Not started | Go event goroutine can mislabel a genuine terminal error as `ErrSlowConsumer` | +| CLI-44 | Low | — | S | — | Done | Go event goroutine can mislabel a genuine terminal error as `ErrSlowConsumer` | | CLI-45 | Low | P1 | M | — | Done | Standardize CLI credential env-var names; fail fast on missing/empty passwords | ### Testing, docs & gaps — [60-testing-docs-gaps.md](60-testing-docs-gaps.md) @@ -169,4 +169,5 @@ Sequence these together rather than piecemeal — several are one change set spa | 2026-08-07 | **CLI-37 + CLI-38 -> `Done`** (branch `fix/cli-37-38-conformance`), one cross-client conformance commit; **closes old-tracker CLI-08**. Canonical rules landed everywhere: an `MxStatusProxy` entry fails iff `category != MX_STATUS_CATEGORY_OK` (`success` is the raw COM member, diagnostics only; absent entry = success, present entry with `UNSPECIFIED` = failure), and a reply fails on HRESULT iff `hresult` is present and `< 0` (so `S_FALSE = 1` passes). Edits: .NET `MxStatusProxyExtensions.IsSuccess` (drop the `Success != 0` conjunct) + `MxCommandReplyExtensions` (`!= 0` -> `< 0`); Go `StatusSucceeded` (category) + `errors.go` (`< 0`); Java `MxStatuses.succeeded` (category, Javadoc corrected) + `MxGatewayErrors` (`< 0`); Python `errors.py` (category); Rust `ensure_mxaccess_success` (category, doc comment corrected). Four shared fixtures added under `clients/proto/fixtures/behavior/command-replies/` (`write.status-category-{error-success-set,ok-success-zero}.reply.json`, `write.hresult-{s-false,e-fail}.reply.json`) + manifest + `docs/ClientBehaviorFixtures.md`; each of the five suites now runs all four fixture-driven, plus a per-language table test for the two edges fixtures cannot express (nil/null entry, `UNSPECIFIED` category). Docs same commit: `ClientLibrariesDesign.md` per-item rule sentence (its existing HRESULT `< 0` claim is now true), .NET/Go/Java README error sections. Also fixed a Java test fake that built a status with a bare `setSuccess(1)` and no category. Verification: dotnet build 0 warnings + 110 passed/1 skipped; `gofmt -l` clean, `go build ./...`, `go test ./...` all ok; `gradle test` BUILD SUCCESSFUL with **no** generated-file churn to revert this time (no `.proto` changed and `generateProto` stayed up to date); `python -m pytest` 155 passed/1 skipped; `cargo fmt` (no unrelated reformat), `cargo check`, `cargo test --workspace` 100 passed, `cargo clippy --all-targets -- -D warnings` clean. Gateway-side `ClientBehaviorFixtureTests` 8/8 re-run because the new fixtures are validated there. | | 2026-08-07 | **CLI-45 → `Done`** on `fix/cli-45-credential-envvar`. All five CLIs now share one credential contract for `authenticate-user`: flags `--password` / `--password-env` (Go: `-password` / `-password-env`) defaulting to env `MXGATEWAY_VERIFY_PASSWORD`, resolution flag-then-env, and a resolved credential that is missing **or empty** is a usage error naming the flag and the variable — never the value, never sent to the wire. Go and Java previously authenticated with an empty password: Go now returns the guard error before dialing, Java throws a picocli `ParameterException` instead of falling back to `""`. Python's `--password-env` gained the canonical default (its `UsageError` was already conformant) and its message now names the resolved variable. Rust treats an empty `--password` or empty env value as missing (resolution extracted into a testable `resolve_verify_user_password`). .NET adopted the canonical flags and keeps its pre-existing names as **deprecated aliases for one release** — order: `--password`, `--verify-user-password`, the variable named by `--password-env` (or the deprecated `--verify-user-password-env`; default `MXGATEWAY_VERIFY_PASSWORD`), then `MXGATEWAY_VERIFY_USER_PASSWORD`. Tests: `TestRunAuthenticateUser{RejectsEmptyPassword,ReadsPasswordFromCanonicalEnv}` (Go), 3 picocli cases (Java), 3 click cases (Python), 2 clap/resolver cases (Rust), 4 xUnit cases covering the canonical flag, both env-name paths, the deprecated flag+env aliases, and the missing/empty failure (.NET). Docs same commit: `docs/CrossLanguageSmokeMatrix.md` gained a "Credential contract for `authenticate-user`" section **and** the per-CLI subcommand-coverage table — the half of this finding that is documented rather than fixed (.NET exposes all nine single-item session commands; Rust `unregister` + the credential pair; Go/Python/Java the credential pair only; verified against each dispatch table, and every gap is CLI surface only since all five *libraries* implement all nine helpers). All five client READMEs name the canonical variable and the fail-fast rule; the .NET README gained an `authenticate-user` credentials section carrying the deprecation note. **Deviation:** Java keeps `isBlank()` (per this design's "null or blank" wording for Java) where the other four test emptiness, so a whitespace-only credential is additionally rejected there. Verification (all five, on macOS): Go `gofmt -l .` clean, `go build ./...` clean, `go test ./...` ok; Java `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` BUILD SUCCESSFUL, CLI suite 51 tests / 0 failures — **no generated-tree churn appeared this run**, `git status` for `clients/java/**/generated` clean with no revert needed (no `.proto` changed); Python `python -m pytest` 148 passed / 1 skipped (TLS opt-in); .NET `dotnet build …Client.slnx` 0 warnings / 0 errors and client tests 108 passed / 1 skipped (live-gateway opt-in); Rust `cargo fmt` (diff confined to the new code), `cargo check --workspace`, `cargo test --workspace` 100 tests across 6 targets all green, `cargo clippy --all-targets -- -D warnings` clean. | | 2026-08-07 | **SEC-31 + SEC-32 → `Done`** (branch `fix/sec-31-32-limiter`, one change set as planned). `ApiKeyFailureLimiter` reworked from `IsBlocked/RecordFailure/Reset(string peer)` to a partition-pair API (`Check/RecordFailure/Reset(ApiKeyThrottlePartition)` returning `ApiKeyThrottleDecision`): layer 1 is the composite `(transport peer, key id)` partition, layer 2 a per-key-id aggregate across peers (`ApiKeyFailureAggregateLimit`, default 30), and an over-limit state now admits one probe per `ApiKeyFailureProbeIntervalSeconds` (default 5) instead of blocking absolutely — so a success can reset the state while throttled, killing the 10-packets-per-minute lockout. SEC-32 rides along: the interceptor validates token shape (`mxgw` prefix, ≥3 non-empty `_` segments, key id ≤ 64 chars) before minting a key-id partition, each peer may mint at most 32 of them (overflow collapses to its fallback partition), and eviction prefers expired windows, never dropping an over-limit partition below a 2× transient overshoot ceiling. New counter `mxgateway.auth.throttled` tagged `stage=peer\|aggregate` only (no key material — `/metrics` is still unauthenticated per open SEC-14). Docs updated in the same commit (`docs/GatewayConfiguration.md` limiter rows + two new keys, `docs/Authentication.md` hot-path paragraph, `docs/Authorization.md` SEC-11 section, limiter/`SecurityOptions` XML remarks). Evidence: `dotnet build …Server` clean; `--filter ~ApiKeyFailureLimiter` 11/11 passed (new `ApiKeyFailureLimiterTests`), `--filter ~GatewayGrpcAuthorizationInterceptor` 20/20 passed (incl. the four SEC-31 contract tests and `NonMxgwToken_FallsBackToTransportPeerPartition`), `--filter ~GatewayOptionsValidator` 66/66 passed. Full suite on macOS: 804 passed / 44 failed — all 44 are the pre-existing named-pipe fake-worker classes (`WorkerClientTests`, `FakeWorkerHarnessTests`, `SessionWorkerClientFactoryFakeWorkerTests`, `GatewayEndToEnd*`), verified identical (44) on the unmodified tree. Follow-up unchanged: the new `MxGateway:Security` keys belong in old **SEC-24**'s effective-config projection when that is picked up. **Code review of the branch found two defects in the first pass, both fixed before merge:** (1) probe admission was check-then-act across two lock scopes, so a burst arriving at an interval boundary could all observe "due" and all be admitted — the claim is now a single critical section (`TryConsumeProbe`), and because the two layers are claimed one at a time, a slot claimed on the partition is compensated (`ReleaseProbe`) when the aggregate then refuses; (2) `Reset` on a success whose key id had been collapsed into the address's shared fallback partition removed that shared partition, letting one authentication wipe an in-progress spray from the same address — it is now left to decay by window expiry, while the key's aggregate is still cleared. Tests added: `ProbeAdmission_UnderConcurrentArrivals_GrantsExactlyOneSlot`, `ProbeAdmission_WhenAggregateRefuses_ReturnsTheClaimedPeerSlot`, `Reset_WithOverCapKeyId_DoesNotClearSharedFallbackPartition`. **A second review pass found a residual defect in that compensation path:** the release identified its own reservation by comparing `NextProbeAtTicks` to `now + interval`, the identical expression a failure re-arm writes — so a concurrent `RecordFailure` on the same state sharing a clock tick (routine at ~1 ms resolution) was mistaken for the caller's own claim and stomped back to the stale, already-due value, prematurely reopening the probe slot. Replaced with a monotonic per-state `ProbeVersion` bumped by every writer of `NextProbeAtTicks` (claim and re-arm alike); the release restores only when the version still matches the one its claim stamped, and bumps it again on restore so no other stale release can match. Covered by `ProbeSlotRestore_DoesNotStompConcurrentRearmAtSameTick`, made deterministic by a new `internal ProbeReleaseInterleaveHook` test seam (null in production, one null check on the refused path) because the claim-to-release window is nanoseconds wide and racing threads cannot hit it reliably — verified as a genuine red against the timestamp guard (`Expected: ThrottledByPeer / Actual: ProbeAdmitted`). Limiter suite 11 → 15. | +| 2026-08-07 | **CLI-40 + CLI-41 + CLI-44 → `Done`** (branch `fix/cli-40-41-44`), one change set; two fast-follow riders from the CLI-37/38 review landed alongside. **CLI-40** (exact-secret credential scrub, ported to Rust/Java/.NET; Go/Python already did it): every credential-bearing helper (`AuthenticateUser` password, `WriteSecured`/`WriteSecured2` string payloads) now scrubs the **exact** caller-supplied secret from any surfaced error text, on top of the by-construction guarantee — Rust `MxAccessError` gained a `secrets: Vec` field whose `Display` scrubs exact-then-pattern (and a **hand-written redacting `Debug`**, since the derived `Debug` would have leaked the reply verbatim — caught by the existing Debug regression test); Java added `MxGatewaySecrets.redactExact` + a private `invokeCommandRedacted(command, secrets…)` that rebuilds the same exception type with the redacted message and **does not chain the secret-bearing original as cause**; .NET added an internal `MxGatewaySecretRedaction` (rebuilds the same concrete `MxGateway*Exception` type via a type switch) wired into the three credential helpers — and it carries the original's **inner** cause forward rather than the secret-bearing original, so `ToString()` (what loggers emit) is scrubbed too, not just `Message` (locked by a `ToString()` assertion). **CLI-41** (uniform malformed-reply contract for `AuthenticateUser`/`ArchestrAUserToId`/`AddBufferedItem` across all five): typed payload → present `return_value` with the int32 variant → else a typed malformed-reply error (`MalformedReplyError` Go/Python, `MxGatewayMalformedReplyException` Java/.NET, existing `Error::MalformedReply` Rust) — never a proto3 default `0`, never an NRE (fixes the Go/Java silent-`0`, .NET NRE, and Rust's own internal inconsistency by giving `authenticate_user_id`/`archestra_user_id` the same `return_value` fallback `add_buffered_item_handle` already had). **CLI-44** (Go): the event goroutine's Recv-error path now uses a new non-blocking `sendTerminalEventResult` on the reserved slot instead of `sendEventResult`, so a genuine terminal gRPC error is reported as itself even when the 16 data slots are full, rather than being mislabeled `ErrSlowConsumer`; test `TestEventsFullBufferTerminalErrorKeepsRootCause` was confirmed red-first (a 250 ms settle after `streamDone` is required to make the buffer genuinely full at error time). Three shared fixtures added under `clients/proto/fixtures/behavior/command-replies/` (`authenticate-user.{echoed-credential,missing-payload,return-value-only}.reply.json`; the echoed-credential reply uses an OK envelope + negative HRESULT + the credential in `protocolStatus.message` / `statuses[0].diagnosticText` / `diagnosticMessage` so all five clients route it to their MXAccess error uniformly) + manifest + `docs/ClientBehaviorFixtures.md` + `docs/ClientLibrariesDesign.md`. **Rider (a):** .NET `ToDiagnosticSummary` and Python `_mxaccess_message` now surface the raw `success` member (Rust already did), for diagnostics-only parity. **Rider (b):** the status-conversion fixture gained an independent `wantSuccess` boolean per case; the Go `TestStatusConversionFixtures` and .NET `FixtureStatuses_ProjectSuccessAndPreserveRawFields` now assert against it instead of recomputing `category == OK` (the formula under test). **Deviation:** the `` marker is not universal — Go/Rust/Java use ``, Python and the .NET CLI use `[redacted]`; each suite asserts its own client's marker plus the exact-secret absence (marker unification was out of scope). Verification (all five, on macOS): Go `gofmt -l` clean + `go build ./...` + `go test ./...` ok; Python `python -m pytest` 162 passed / 1 skipped; .NET `dotnet build …Client.slnx` 0 warnings + client tests 120 passed / 1 skipped; Rust `cargo fmt` + `cargo check --workspace` + `cargo test --workspace` (all targets pass) + `cargo clippy --all-targets -- -D warnings` clean; Java `JAVA_HOME=/opt/homebrew/opt/openjdk@17 gradle test` BUILD SUCCESSFUL with **no** generated-tree churn to revert (no `.proto` changed). | | 2026-08-07 | **GWC-28, GWC-29, GWC-30, TST-28 → `Done`** (branch `fix/gwc-28-29-30-polish`). GWC-28: `WorkerClient.WriteLoopAsync` now stamps `envelope.Sequence = unchecked(++_nextSequence)` immediately before `_writer.WriteAsync`, and `CreateEnvelope` leaves it unset; `_nextSequence` dropped from `long` + `Interlocked` to a plain `ulong` touched only by the write loop (the channel's single consumer, `SingleReader = true`), so wire order and sequence order are the same thing by construction. Mirrors the worker's WRK-04 stamping, which the gateway half had never received; `gateway.md`'s envelope-sequence rule now states that both sides stamp at write inside their single write path and that inbound enforcement (still open, old **GWC-10**) would rely on it. New `WorkerClientTests.ConcurrentInvokesEmitStrictlyIncreasingSequencesOnTheWire` (32 parallel invokes, sequences asserted strictly increasing in wire order) failed 3/3 pre-fix. GWC-29: added `MxAccessGrpcMapper.MapCommand(MxCommand)`; `Invoke` no longer deep-clones the whole `MxCommandRequest` just to overwrite and discard its command. The one clone inside `MapCommand` stays and is documented as required — `commandToInvoke` may be the gRPC-owned `request.Command` and is read again after dispatch by `TrackCommandReply`, so it is what keeps `CreateCommandEnvelope`'s no-aliasing invariant true. New `MxAccessGrpcMapperTests.MapCommandFromCommandClonesPayload` (isolation + both overloads equal under a `FakeTimeProvider`). GWC-30: `WorkerFrameReader` reuses a per-instance `_lengthPrefix` scratch buffer instead of allocating 4 bytes per frame, with a class remark that `ReadAsync` is not reentrant (single read loop per `WorkerClient`; handshake reads complete before the loop starts); guarded by new `WorkerFrameProtocolTests.ReadAsync_WithMultipleFramesOnOneReader_ParsesEveryFrame` (5 frames, varying payload lengths, one reader). TST-28: new `[Theory] WorkerClientTests.StartAsync_SendsGatewayHelloWithConfiguredMaxFrameBytes` over the default and a 2 MiB override via `FakeWorkerHarness.CreateConnectedPairAsync(maxMessageBytes:)` — test-only, and the mutation check (hard-code `MaxFrameBytes = 0`) failed both cases before being reverted. Verification: `NonWindows.slnx` 0 warnings/0 errors; `WorkerClientTests` 25 passed, `WorkerFrameProtocolTests` 11 passed, `MxAccessGrpcMapperTests` 6 passed, `MxAccessGatewayService*` 29 passed, full gateway suite 844 passed / 0 failed (`TMPDIR=/tmp` on macOS). | diff --git a/archreview/2026-07-12/remediation/50-clients.md b/archreview/2026-07-12/remediation/50-clients.md index c7543a5..db30076 100644 --- a/archreview/2026-07-12/remediation/50-clients.md +++ b/archreview/2026-07-12/remediation/50-clients.md @@ -21,11 +21,11 @@ Operating constraints carried from prior work: | CLI-37 | Medium | P1 | M | CLI-38 | Done | Status-array validation must branch on `category` per the proto contract (4-vs-1 divergence) | | CLI-38 | Medium | P1 | S | — | Done | Align .NET/Go/Java on `hresult < 0` — lands prior CLI-08 and cures the design-doc drift | | CLI-39 | Medium | P1 | S | CLI-35..38, CLI-45 | Not started | Bump client versions off the already-published 0.1.2 before the next publish; add registry-collision guard | -| CLI-40 | Low | — | M | — | Not started | Port the exact-secret credential scrub to Rust/Java/.NET | -| CLI-41 | Low | — | M | — | Not started | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem | +| CLI-40 | Low | — | M | — | Done | Port the exact-secret credential scrub to Rust/Java/.NET | +| CLI-41 | Low | — | M | — | Done | Uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/AddBufferedItem | | CLI-42 | Low | P1 | S | — | Not started | Document the vendored Rust proto layout (CLI-02's missing doc half) | | CLI-43 | Low | — | S | — | Not started | Java style guide still prescribes "Java 21 preferred" | -| CLI-44 | Low | — | S | — | Not started | Go event goroutine can mislabel a genuine terminal error as `ErrSlowConsumer` | +| CLI-44 | Low | — | S | — | Done | Go event goroutine can mislabel a genuine terminal error as `ErrSlowConsumer` | | CLI-45 | Low | P1 | M | — | Done | Standardize CLI credential env-var name and fail fast on missing/empty passwords | Cross-domain dependencies: **CLI-35/CLI-36 pair with GWC-25** (gateway emits `oldest_available_sequence = 0` on an empty replay ring — the server-side half of the same reconnect story; the CLI fixes here are independently landable but the end-to-end resume walk in the smoke matrix needs both). **CLI-39 pairs with the publishing process** (`scripts/pack-clients.ps1`, `scripts/tag-go-module.ps1`, Gitea package registry). diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySessionReplyContractTests.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySessionReplyContractTests.cs new file mode 100644 index 0000000..8b9a77e --- /dev/null +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySessionReplyContractTests.cs @@ -0,0 +1,127 @@ +using Google.Protobuf; +using ZB.MOM.WW.MxGateway.Contracts.Proto; + +namespace ZB.MOM.WW.MxGateway.Client.Tests; + +/// +/// Tests for the credential-scrub (CLI-40) and malformed-reply (CLI-41) contracts on the +/// credential and id-returning session helpers, driven from shared behavior fixtures. +/// +public sealed class MxGatewaySessionReplyContractTests +{ + /// + /// CLI-40: when MXAccess echoes the submitted credential back in its failure diagnostic, + /// the surfaced exception message must scrub it to the library redaction marker. + /// + [Fact] + public async Task AuthenticateUserAsync_RedactsEchoedCredentialInFailureMessage() + { + const string password = "sup3rSecretVerify9f3a2b"; + FakeGatewayTransport transport = CreateTransport(); + transport.AddInvokeReply(ReadReplyFixture("authenticate-user.echoed-credential.reply.json")); + await using MxGatewayClient client = CreateClient(transport); + MxGatewaySession session = await client.OpenSessionAsync(); + + MxAccessException exception = await Assert.ThrowsAsync( + async () => await session.AuthenticateUserAsync(12, "operator", password)); + + Assert.DoesNotContain(password, exception.Message, StringComparison.Ordinal); + Assert.Contains("", exception.Message, StringComparison.Ordinal); + // ToString() is what logging frameworks emit; the secret-bearing original must not be + // chained as an inner exception where it would re-surface the credential verbatim. + Assert.DoesNotContain(password, exception.ToString(), StringComparison.Ordinal); + } + + /// + /// CLI-41: an OK reply that carries neither the typed AuthenticateUser payload nor an + /// int32 return_value is a malformed reply, surfaced as a typed exception rather than an NRE. + /// + [Fact] + public async Task AuthenticateUserAsync_MissingPayloadAndReturnValue_ThrowsMalformedReply() + { + FakeGatewayTransport transport = CreateTransport(); + transport.AddInvokeReply(ReadReplyFixture("authenticate-user.missing-payload.reply.json")); + await using MxGatewayClient client = CreateClient(transport); + MxGatewaySession session = await client.OpenSessionAsync(); + + await Assert.ThrowsAsync( + async () => await session.AuthenticateUserAsync(12, "operator", "pw")); + } + + /// + /// CLI-41: an OK reply that omits the typed payload but carries an int32 return_value + /// resolves to that return value. + /// + [Fact] + public async Task AuthenticateUserAsync_ReturnValueOnly_ResolvesReturnValue() + { + FakeGatewayTransport transport = CreateTransport(); + transport.AddInvokeReply(ReadReplyFixture("authenticate-user.return-value-only.reply.json")); + await using MxGatewayClient client = CreateClient(transport); + MxGatewaySession session = await client.OpenSessionAsync(); + + int userId = await session.AuthenticateUserAsync(12, "operator", "pw"); + + Assert.Equal(7, userId); + } + + /// + /// CLI-41: the AddBufferedItem fallback shares the malformed-reply contract — an OK reply + /// with neither a typed item handle nor an int32 return_value throws the typed exception. + /// + [Fact] + public async Task AddBufferedItemAsync_MissingPayloadAndReturnValue_ThrowsMalformedReply() + { + FakeGatewayTransport transport = CreateTransport(); + transport.AddInvokeReply(new MxCommandReply + { + SessionId = "session-fixture", + Kind = MxCommandKind.AddBufferedItem, + ProtocolStatus = new ProtocolStatus { Code = ProtocolStatusCode.Ok }, + }); + await using MxGatewayClient client = CreateClient(transport); + MxGatewaySession session = await client.OpenSessionAsync(); + + await Assert.ThrowsAsync( + async () => await session.AddBufferedItemAsync(12, "Area001.Pump001.Speed", "runtime")); + } + + private static MxGatewayClient CreateClient(FakeGatewayTransport transport) + { + return new MxGatewayClient(transport.Options, transport); + } + + private static FakeGatewayTransport CreateTransport() + { + return new FakeGatewayTransport(new MxGatewayClientOptions + { + Endpoint = new Uri("http://localhost:5000"), + ApiKey = "test-api-key", + }); + } + + private static MxCommandReply ReadReplyFixture(string fileName) + { + DirectoryInfo directory = new(AppContext.BaseDirectory); + while (directory is not null) + { + string path = Path.Combine( + directory.FullName, + "clients", + "proto", + "fixtures", + "behavior", + "command-replies", + fileName); + + if (File.Exists(path)) + { + return JsonParser.Default.Parse(File.ReadAllText(path)); + } + + directory = directory.Parent!; + } + + throw new FileNotFoundException(fileName); + } +} diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxStatusProxyExtensionsTests.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxStatusProxyExtensionsTests.cs index cd417a2..5c54950 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxStatusProxyExtensionsTests.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxStatusProxyExtensionsTests.cs @@ -20,7 +20,8 @@ public sealed class MxStatusProxyExtensionsTests MxStatusProxy status = JsonParser.Default.Parse( testCase.GetProperty("status").GetRawText()); - Assert.Equal(status.Category is MxStatusCategory.Ok, status.IsSuccess()); + bool wantSuccess = testCase.GetProperty("wantSuccess").GetBoolean(); + Assert.Equal(wantSuccess, status.IsSuccess()); Assert.Equal( testCase.GetProperty("status").GetProperty("rawCategory").GetInt32(), status.RawCategory); diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayMalformedReplyException.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayMalformedReplyException.cs new file mode 100644 index 0000000..4b5f03a --- /dev/null +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayMalformedReplyException.cs @@ -0,0 +1,46 @@ +using ZB.MOM.WW.MxGateway.Contracts.Proto; + +namespace ZB.MOM.WW.MxGateway.Client; + +/// +/// Exception thrown when the gateway returns a protocol-OK reply that carries neither the +/// expected typed payload nor an int32 return_value, so the client cannot resolve the +/// operation result. This replaces the historical that a +/// blind reply.ReturnValue.Int32Value fallback would throw. +/// +public sealed class MxGatewayMalformedReplyException : MxGatewayException +{ + /// Initializes a new instance with the given message. + /// The error message describing the malformed reply. + public MxGatewayMalformedReplyException(string message) + : base(message) + { + } + + /// Initializes a new instance with full diagnostic context. + /// The error message describing the malformed reply. + /// The session ID, if available. + /// The correlation ID for tracing, if available. + /// The protocol status details, if available. + /// The HResult code, if available. + /// The MXAccess statuses, if available. + /// The underlying exception, if any. + public MxGatewayMalformedReplyException( + string message, + string? sessionId = null, + string? correlationId = null, + ProtocolStatus? protocolStatus = null, + int? hResult = null, + IReadOnlyList? statuses = null, + Exception? innerException = null) + : base( + message, + sessionId, + correlationId, + protocolStatus, + hResult, + statuses ?? [], + innerException) + { + } +} diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySecretRedaction.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySecretRedaction.cs new file mode 100644 index 0000000..7cbad16 --- /dev/null +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySecretRedaction.cs @@ -0,0 +1,84 @@ +namespace ZB.MOM.WW.MxGateway.Client; + +/// +/// Scrubs exact secret substrings out of diagnostic text before it leaves the client on an +/// exception path. MXAccess can echo a submitted credential or secured value back inside a +/// failure diagnostic (protocol message, MXSTATUS_PROXY diagnostic text, HRESULT description); +/// this helper replaces any such verbatim occurrence with <redacted> so the raw +/// request payload never reaches a caught exception's message. The marker matches the Go, Rust, +/// and Java clients. +/// +internal static class MxGatewaySecretRedaction +{ + private const string Marker = ""; + + /// + /// Replaces every non-null, non-empty secret in with the + /// redaction marker (ordinal comparison). Returns the message unchanged when it is null or + /// empty, or when no usable secret is supplied. + /// + /// The diagnostic message to scrub. + /// The secret values to remove from the message. + /// The scrubbed message. + internal static string Redact(string message, params string?[] secrets) + { + if (string.IsNullOrEmpty(message) || secrets is null) + { + return message; + } + + string result = message; + foreach (string? secret in secrets) + { + if (!string.IsNullOrEmpty(secret)) + { + result = result.Replace(secret, Marker, StringComparison.Ordinal); + } + } + + return result; + } + + /// + /// Returns an exception equivalent to but with any verbatim secret + /// scrubbed from its message. When nothing changes, the original exception is returned + /// unchanged; otherwise a new exception of the same concrete runtime type is built and the + /// original reply/status context is preserved. The secret-bearing original is deliberately + /// not chained as the inner exception — doing so would let its unredacted message + /// re-surface through (which logging frameworks call). The + /// original's own inner cause (a transport error, never the request payload) is carried + /// forward instead. + /// + /// The exception to redact. + /// The secret values to remove from the message. + /// The redacted exception, or the original when no change was needed. + internal static MxGatewayException Redacted(MxGatewayException ex, params string?[] secrets) + { + ArgumentNullException.ThrowIfNull(ex); + + string redacted = Redact(ex.Message, secrets); + if (string.Equals(redacted, ex.Message, StringComparison.Ordinal)) + { + return ex; + } + + Exception? cause = ex.InnerException; + return ex switch + { + MxAccessException access => new MxAccessException(redacted, access.Reply, cause), + MxGatewaySessionException => new MxGatewaySessionException( + redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + MxGatewayWorkerException => new MxGatewayWorkerException( + redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + MxGatewayAuthenticationException => new MxGatewayAuthenticationException( + redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + MxGatewayAuthorizationException => new MxGatewayAuthorizationException( + redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + MxGatewayMalformedReplyException => new MxGatewayMalformedReplyException( + redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + MxGatewayCommandException => new MxGatewayCommandException( + redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + _ => new MxGatewayException(redacted, cause), + }; + } +} diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySession.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySession.cs index 094cf50..b24512c 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySession.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySession.cs @@ -945,7 +945,7 @@ public sealed class MxGatewaySession : IAsyncDisposable cancellationToken) .ConfigureAwait(false); reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); - return reply.AddBufferedItem?.ItemHandle ?? reply.ReturnValue.Int32Value; + return ResolveInt32Result(reply.AddBufferedItem?.ItemHandle, reply, "AddBufferedItem"); } /// @@ -1141,7 +1141,14 @@ public sealed class MxGatewaySession : IAsyncDisposable verifierUserId, cancellationToken) .ConfigureAwait(false); - reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); + try + { + reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); + } + catch (MxGatewayException ex) + { + throw MxGatewaySecretRedaction.Redacted(ex, ExtractSecretString(value)); + } } /// @@ -1215,7 +1222,14 @@ public sealed class MxGatewaySession : IAsyncDisposable verifierUserId, cancellationToken) .ConfigureAwait(false); - reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); + try + { + reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); + } + catch (MxGatewayException ex) + { + throw MxGatewaySecretRedaction.Redacted(ex, ExtractSecretString(value)); + } } /// @@ -1285,8 +1299,15 @@ public sealed class MxGatewaySession : IAsyncDisposable verifyUserPassword, cancellationToken) .ConfigureAwait(false); - reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); - return reply.AuthenticateUser?.UserId ?? reply.ReturnValue.Int32Value; + try + { + reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); + return ResolveInt32Result(reply.AuthenticateUser?.UserId, reply, "AuthenticateUser"); + } + catch (MxGatewayException ex) + { + throw MxGatewaySecretRedaction.Redacted(ex, verifyUserPassword); + } } /// @@ -1337,7 +1358,7 @@ public sealed class MxGatewaySession : IAsyncDisposable MxCommandReply reply = await ArchestraUserToIdRawAsync(serverHandle, userIdGuid, cancellationToken) .ConfigureAwait(false); reply.EnsureProtocolSuccess().EnsureMxAccessSuccess(); - return reply.ArchestraUserToId?.UserId ?? reply.ReturnValue.Int32Value; + return ResolveInt32Result(reply.ArchestraUserToId?.UserId, reply, "ArchestrAUserToId"); } /// @@ -1367,6 +1388,51 @@ public sealed class MxGatewaySession : IAsyncDisposable cancellationToken); } + /// + /// Resolves the int32 result of an OK command reply: the typed payload value when present, + /// otherwise an int32 return_value when the reply carries one. A reply that provides + /// neither is malformed and surfaces as + /// rather than the historical . + /// + /// The typed payload value, or when absent. + /// The OK command reply. + /// The MXAccess operation name, for the diagnostic message. + /// The resolved int32 result. + private static int ResolveInt32Result(int? typedValue, MxCommandReply reply, string operation) + { + if (typedValue.HasValue) + { + return typedValue.Value; + } + + if (reply.ReturnValue is not null + && reply.ReturnValue.KindCase == MxValue.KindOneofCase.Int32Value) + { + return reply.ReturnValue.Int32Value; + } + + throw new MxGatewayMalformedReplyException( + $"{operation} returned a malformed reply: OK reply carried neither the typed payload nor an int32 return_value", + reply.SessionId, + reply.CorrelationId, + reply.ProtocolStatus, + reply.HasHresult ? reply.Hresult : null, + reply.Statuses.ToArray()); + } + + /// + /// Extracts the raw string form of a credential-bearing for redaction, + /// or when the value does not carry a string. + /// + /// The value written by a secured write. + /// The string payload, or . + private static string? ExtractSecretString(MxValue value) + { + return value.KindCase == MxValue.KindOneofCase.StringValue + ? value.StringValue + : null; + } + /// /// Invokes an MXAccess command on this session. /// diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxStatusProxyExtensions.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxStatusProxyExtensions.cs index 5223b1b..5b2916d 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxStatusProxyExtensions.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxStatusProxyExtensions.cs @@ -30,6 +30,6 @@ public static class MxStatusProxyExtensions ? "no diagnostic text" : status.DiagnosticText; - return $"{status.Category} by {status.DetectedBy}; detail={status.Detail}; {diagnosticText}"; + return $"success={status.Success}; {status.Category} by {status.DetectedBy}; detail={status.Detail}; {diagnosticText}"; } } diff --git a/clients/go/mxgateway/client_session_test.go b/clients/go/mxgateway/client_session_test.go index 172d268..ad6522a 100644 --- a/clients/go/mxgateway/client_session_test.go +++ b/clients/go/mxgateway/client_session_test.go @@ -10,7 +10,9 @@ import ( pb "gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/internal/generated" "google.golang.org/grpc" + "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" + "google.golang.org/grpc/status" "google.golang.org/grpc/test/bufconn" ) @@ -200,6 +202,68 @@ func TestEventsSlowConsumerYieldsErrSlowConsumerBeforeClose(t *testing.T) { } } +func TestEventsFullBufferTerminalErrorKeepsRootCause(t *testing.T) { + fake := &fakeGatewayServer{ + streamStarted: make(chan struct{}), + streamDone: make(chan struct{}), + streamEventCount: eventBufferSize, + streamTerminalErr: status.Error(codes.Internal, "boom"), + } + client, cleanup := newBufconnClient(t, fake) + defer cleanup() + session := NewSessionForID(client, "session-1") + + events, err := session.EventsAfter(context.Background(), 0) + if err != nil { + t.Fatalf("EventsAfter() error = %v", err) + } + <-fake.streamStarted + + // Do not drain until the stream has fully ended: the server sends exactly + // eventBufferSize events (filling the data slots) and then returns a genuine + // terminal gRPC error. The client must report that error as itself, using the + // reserved slot, rather than mislabeling it as ErrSlowConsumer. + select { + case <-fake.streamDone: + case <-time.After(2 * time.Second): + t.Fatal("event stream did not stop after terminal error") + } + // streamDone fires when the server returns; the client's producer goroutine + // still needs a moment to drain the gRPC stream, fill all data slots, and + // enqueue the terminal result. Let it settle before draining so the buffer is + // genuinely full when the terminal error is processed (which is what makes the + // mislabel bug observable). + time.Sleep(250 * time.Millisecond) + + var last EventResult + gotResult := false + for { + select { + case res, ok := <-events: + if !ok { + if !gotResult { + t.Fatal("events channel closed without yielding any result") + } + var gwErr *GatewayError + if !errors.As(last.Err, &gwErr) { + t.Fatalf("final event result err is %T, want *GatewayError", last.Err) + } + if code := status.Code(last.Err); code != codes.Internal { + t.Fatalf("final event result gRPC code = %s, want %s", code, codes.Internal) + } + if errors.Is(last.Err, ErrSlowConsumer) { + t.Fatalf("final event result err = %v, must not be mislabeled as ErrSlowConsumer", last.Err) + } + return + } + last = res + gotResult = true + case <-time.After(2 * time.Second): + t.Fatal("events channel did not close after terminal error") + } + } +} + func TestEventsSurfacesReplayGapSentinelAsTypedSignal(t *testing.T) { fake := &fakeGatewayServer{ streamStarted: make(chan struct{}), @@ -694,15 +758,16 @@ func newBufconnClient(t *testing.T, fake *fakeGatewayServer) (*Client, func()) { type fakeGatewayServer struct { pb.UnimplementedMxAccessGatewayServer - openReply *pb.OpenSessionReply - openAuth string - streamAuth string - streamStarted chan struct{} - streamDone chan struct{} - streamEventCount int - streamReplayGap *pb.ReplayGap - invokeReply *pb.MxCommandReply - invokeRequest *pb.MxCommandRequest + openReply *pb.OpenSessionReply + openAuth string + streamAuth string + streamStarted chan struct{} + streamDone chan struct{} + streamEventCount int + streamReplayGap *pb.ReplayGap + streamTerminalErr error + invokeReply *pb.MxCommandReply + invokeRequest *pb.MxCommandRequest } func (s *fakeGatewayServer) OpenSession(ctx context.Context, req *pb.OpenSessionRequest) (*pb.OpenSessionReply, error) { @@ -772,6 +837,12 @@ func (s *fakeGatewayServer) StreamEvents(req *pb.StreamEventsRequest, stream grp return err } } + if s.streamTerminalErr != nil { + // Return a genuine terminal stream error immediately after sending the + // events, without waiting on the client to cancel. This exercises the + // Recv-error path while the client's result buffer is still full. + return s.streamTerminalErr + } <-stream.Context().Done() return io.EOF } diff --git a/clients/go/mxgateway/command_reply_fixtures_test.go b/clients/go/mxgateway/command_reply_fixtures_test.go new file mode 100644 index 0000000..2561f83 --- /dev/null +++ b/clients/go/mxgateway/command_reply_fixtures_test.go @@ -0,0 +1,129 @@ +package mxgateway + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + pb "gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/internal/generated" + "google.golang.org/protobuf/encoding/protojson" +) + +// loadCommandReplyFixture parses a shared command-reply fixture into an +// MxCommandReply so the Go client can be driven through the same wire shapes the +// other language clients exercise. +func loadCommandReplyFixture(t *testing.T, name string) *pb.MxCommandReply { + t.Helper() + path := filepath.Join("..", "..", "proto", "fixtures", "behavior", "command-replies", name) + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read fixture %s: %v", name, err) + } + var reply pb.MxCommandReply + if err := protojson.Unmarshal(data, &reply); err != nil { + t.Fatalf("parse fixture %s: %v", name, err) + } + return &reply +} + +func TestAuthenticateUserMissingPayloadReturnsMalformedReplyError(t *testing.T) { + fake := &fakeGatewayServer{ + invokeReply: loadCommandReplyFixture(t, "authenticate-user.missing-payload.reply.json"), + } + client, cleanup := newBufconnClient(t, fake) + defer cleanup() + session := NewSessionForID(client, "session-1") + + _, err := session.AuthenticateUser(context.Background(), 12, "operator", "secret") + var malformed *MalformedReplyError + if !errors.As(err, &malformed) { + t.Fatalf("AuthenticateUser() error = %v (%T), want *MalformedReplyError", err, err) + } + if malformed.Op != "authenticate user" { + t.Fatalf("MalformedReplyError.Op = %q, want %q", malformed.Op, "authenticate user") + } +} + +func TestAuthenticateUserReturnValueOnlyUsesInt32ReturnValue(t *testing.T) { + fake := &fakeGatewayServer{ + invokeReply: loadCommandReplyFixture(t, "authenticate-user.return-value-only.reply.json"), + } + client, cleanup := newBufconnClient(t, fake) + defer cleanup() + session := NewSessionForID(client, "session-1") + + userID, err := session.AuthenticateUser(context.Background(), 12, "operator", "secret") + if err != nil { + t.Fatalf("AuthenticateUser() error = %v", err) + } + if userID != 7 { + t.Fatalf("AuthenticateUser() = %d, want 7", userID) + } +} + +// AddBufferedItem shares the prefer-payload / int32-return-value / malformed +// fallback code path; cover both branches for one of the siblings. +func TestAddBufferedItemFallbackHonoursReturnValueAndReportsMalformed(t *testing.T) { + t.Run("return-value-only", func(t *testing.T) { + fake := &fakeGatewayServer{ + invokeReply: loadCommandReplyFixture(t, "authenticate-user.return-value-only.reply.json"), + } + client, cleanup := newBufconnClient(t, fake) + defer cleanup() + session := NewSessionForID(client, "session-1") + + itemHandle, err := session.AddBufferedItem(context.Background(), 12, "Area001.Pump001.Speed", "runtime") + if err != nil { + t.Fatalf("AddBufferedItem() error = %v", err) + } + if itemHandle != 7 { + t.Fatalf("AddBufferedItem() = %d, want 7", itemHandle) + } + }) + + t.Run("missing-payload", func(t *testing.T) { + fake := &fakeGatewayServer{ + invokeReply: loadCommandReplyFixture(t, "authenticate-user.missing-payload.reply.json"), + } + client, cleanup := newBufconnClient(t, fake) + defer cleanup() + session := NewSessionForID(client, "session-1") + + _, err := session.AddBufferedItem(context.Background(), 12, "Area001.Pump001.Speed", "runtime") + var malformed *MalformedReplyError + if !errors.As(err, &malformed) { + t.Fatalf("AddBufferedItem() error = %v (%T), want *MalformedReplyError", err, err) + } + if malformed.Op != "add buffered item" { + t.Fatalf("MalformedReplyError.Op = %q, want %q", malformed.Op, "add buffered item") + } + }) +} + +// TestAuthenticateUserScrubsEchoedCredentialFromError is the CLI-40 regression: +// a gateway diagnostic that echoes the raw credential back must never reach the +// caller's surfaced error text. +func TestAuthenticateUserScrubsEchoedCredentialFromError(t *testing.T) { + const credential = "sup3rSecretVerify9f3a2b" + fake := &fakeGatewayServer{ + invokeReply: loadCommandReplyFixture(t, "authenticate-user.echoed-credential.reply.json"), + } + client, cleanup := newBufconnClient(t, fake) + defer cleanup() + session := NewSessionForID(client, "session-1") + + _, err := session.AuthenticateUser(context.Background(), 12, "operator", credential) + if err == nil { + t.Fatal("AuthenticateUser() error = nil, want an MXAccess failure") + } + message := err.Error() + if strings.Contains(message, credential) { + t.Fatalf("surfaced error leaked the credential: %q", message) + } + if !strings.Contains(message, "") { + t.Fatalf("surfaced error missing redaction marker: %q", message) + } +} diff --git a/clients/go/mxgateway/conversion_test.go b/clients/go/mxgateway/conversion_test.go index a259d28..11bbecc 100644 --- a/clients/go/mxgateway/conversion_test.go +++ b/clients/go/mxgateway/conversion_test.go @@ -51,8 +51,9 @@ func TestStatusConversionFixtures(t *testing.T) { var fixture struct { Cases []struct { - ID string `json:"id"` - Status json.RawMessage `json:"status"` + ID string `json:"id"` + WantSuccess bool `json:"wantSuccess"` + Status json.RawMessage `json:"status"` } `json:"cases"` } if err := json.Unmarshal(data, &fixture); err != nil { @@ -65,9 +66,8 @@ func TestStatusConversionFixtures(t *testing.T) { if err := protojson.Unmarshal(tc.Status, &status); err != nil { t.Fatalf("parse status: %v", err) } - want := status.GetCategory() == pb.MxStatusCategory_MX_STATUS_CATEGORY_OK - if got := StatusSucceeded(&status); got != want { - t.Fatalf("StatusSucceeded() = %v, want %v", got, want) + if got := StatusSucceeded(&status); got != tc.WantSuccess { + t.Fatalf("StatusSucceeded() = %v, want %v", got, tc.WantSuccess) } }) } diff --git a/clients/go/mxgateway/errors.go b/clients/go/mxgateway/errors.go index 70c9f59..54c918a 100644 --- a/clients/go/mxgateway/errors.go +++ b/clients/go/mxgateway/errors.go @@ -72,6 +72,25 @@ func redactSecrets(err error, secrets ...string) error { // dropping events. Match it with errors.Is. var ErrSlowConsumer = errors.New("mxgateway: event consumer fell behind; stream terminated") +// MalformedReplyError reports an OK command reply that carried neither the +// typed payload the operation expected nor a usable int32 return_value, so the +// client cannot produce a result. It gives every affected helper one uniform, +// inspectable failure instead of silently returning a zero value. +type MalformedReplyError struct { + // Op names the operation whose reply was malformed. + Op string + // Detail explains what the reply was missing. + Detail string +} + +// Error returns the formatted malformed-reply message. +func (e *MalformedReplyError) Error() string { + if e == nil { + return "" + } + return fmt.Sprintf("mxgateway: %s returned a malformed reply: %s", e.Op, e.Detail) +} + // GatewayError wraps transport-level gRPC failures. type GatewayError struct { // Op names the operation that failed (for example "dial" or "invoke"). diff --git a/clients/go/mxgateway/session.go b/clients/go/mxgateway/session.go index 7e920aa..74d0fce 100644 --- a/clients/go/mxgateway/session.go +++ b/clients/go/mxgateway/session.go @@ -812,7 +812,13 @@ func (s *Session) AuthenticateUser(ctx context.Context, serverHandle int32, veri if reply.GetAuthenticateUser() != nil { return reply.GetAuthenticateUser().GetUserId(), nil } - return reply.GetReturnValue().GetInt32Value(), nil + if x, ok := reply.GetReturnValue().GetKind().(*pb.MxValue_Int32Value); ok { + return x.Int32Value, nil + } + return 0, &MalformedReplyError{ + Op: "authenticate user", + Detail: "reply carried neither an AuthenticateUser payload nor an int32 return_value", + } } // AuthenticateUserRaw invokes MXAccess AuthenticateUser and returns the raw @@ -847,7 +853,13 @@ func (s *Session) ArchestrAUserToId(ctx context.Context, serverHandle int32, use if reply.GetArchestraUserToId() != nil { return reply.GetArchestraUserToId().GetUserId(), nil } - return reply.GetReturnValue().GetInt32Value(), nil + if x, ok := reply.GetReturnValue().GetKind().(*pb.MxValue_Int32Value); ok { + return x.Int32Value, nil + } + return 0, &MalformedReplyError{ + Op: "archestra user to id", + Detail: "reply carried neither an ArchestrAUserToId payload nor an int32 return_value", + } } // ArchestrAUserToIdRaw invokes MXAccess ArchestrAUserToId and returns the raw reply. @@ -876,7 +888,13 @@ func (s *Session) AddBufferedItem(ctx context.Context, serverHandle int32, itemD if reply.GetAddBufferedItem() != nil { return reply.GetAddBufferedItem().GetItemHandle(), nil } - return reply.GetReturnValue().GetInt32Value(), nil + if x, ok := reply.GetReturnValue().GetKind().(*pb.MxValue_Int32Value); ok { + return x.Int32Value, nil + } + return 0, &MalformedReplyError{ + Op: "add buffered item", + Detail: "reply carried neither an AddBufferedItem payload nor an int32 return_value", + } } // AddBufferedItemRaw invokes MXAccess AddBufferedItem and returns the raw reply. @@ -981,10 +999,12 @@ func stringSecrets(values ...*MxValue) []string { // context cancellation stops Recv, or a terminal error is sent. // // The returned channel is buffered. If the consumer falls behind and the buffer -// overflows, the stream is terminated and a final EventResult carrying a -// GatewayError that wraps ErrSlowConsumer is delivered before the channel -// closes. Callers must match it with errors.Is(res.Err, ErrSlowConsumer) to -// distinguish a slow-consumer drop from a graceful server end. Use +// overflows with data, the stream is terminated and a final EventResult carrying +// a GatewayError that wraps ErrSlowConsumer is delivered before the channel +// closes; match it with errors.Is(res.Err, ErrSlowConsumer) to distinguish a +// slow-consumer drop from a graceful server end. A genuine stream error is +// reported as itself even under overflow — it is never relabeled as +// ErrSlowConsumer, so the underlying gRPC status stays inspectable. Use // SubscribeEvents for a blocking, backpressured stream that never drops. func (s *Session) Events(ctx context.Context) (<-chan EventResult, error) { return s.EventsAfter(ctx, 0) @@ -994,7 +1014,9 @@ func (s *Session) Events(ctx context.Context) (<-chan EventResult, error) { // // Like Events, the returned channel is buffered and terminates with a final // EventResult wrapping ErrSlowConsumer (matchable via errors.Is) if the consumer -// falls behind and the buffer overflows, rather than silently closing. +// falls behind and the buffer overflows with data, rather than silently closing. +// A genuine stream error is reported as itself even under overflow, never +// relabeled as ErrSlowConsumer. func (s *Session) EventsAfter(ctx context.Context, afterWorkerSequence uint64) (<-chan EventResult, error) { subscription, err := s.subscribeEventsAfter(ctx, afterWorkerSequence, true) if err != nil { @@ -1048,12 +1070,11 @@ func (s *Session) subscribeEventsAfter(ctx context.Context, afterWorkerSequence if err == io.EOF || status.Code(err) == codes.Canceled || streamCtx.Err() != nil { return } - sendEventResult( - streamCtx, - results, - EventResult{Err: &GatewayError{Op: "stream events", Err: err}}, - cancelWhenResultBufferFull, - cancel) + // A genuine terminal stream error must be reported as itself, even + // when the data slots are full. Routing it through sendEventResult + // would let the overflow branch substitute ErrSlowConsumer and lose + // the real gRPC status, so send it directly on the reserved slot. + sendTerminalEventResult(results, EventResult{Err: &GatewayError{Op: "stream events", Err: err}}) return } }() @@ -1072,6 +1093,23 @@ func ensureBulkSize(name string, length int) error { return nil } +// sendTerminalEventResult enqueues a terminal EventResult with a non-blocking +// send. The eventBufferReservedSlots reserve (beyond the eventBufferSize data +// slots) guarantees the send lands unless a terminal result was already +// enqueued; because this goroutine is the sole producer, at most one terminal +// send ever races for the reserved slot, so the select default only fires when +// the reserve is already spent — never dropping a first terminal error. +// +// Unlike sendEventResult, this bypasses the overflow branch: a genuine terminal +// stream error is reported verbatim even when the data slots are full, rather +// than being relabeled as ErrSlowConsumer. +func sendTerminalEventResult(results chan<- EventResult, result EventResult) { + select { + case results <- result: + default: + } +} + func sendEventResult( ctx context.Context, results chan<- EventResult, diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxAccessException.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxAccessException.java index 3622939..a81ab94 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxAccessException.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxAccessException.java @@ -29,4 +29,18 @@ public final class MxAccessException extends MxGatewayCommandException { public MxAccessException(String operation, MxCommandReply reply) { super(operation, reply == null ? null : reply.getProtocolStatus(), reply); } + + /** + * Creates a new MXAccess exception with an already-built, verbatim message. + * Used to re-surface an MXAccess failure with a redacted message while + * preserving the original protocol status and reply. + * + * @param message the exact message to surface (already formatted/redacted) + * @param protocolStatus protocol status reported by the gateway + * @param reply raw command reply containing the MXAccess failure detail + * @param cause underlying error, or {@code null} + */ + public MxAccessException(String message, ProtocolStatus protocolStatus, MxCommandReply reply, Throwable cause) { + super(message, protocolStatus, reply, cause); + } } diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayCommandException.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayCommandException.java index 1dee974..c6b4aa2 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayCommandException.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayCommandException.java @@ -25,6 +25,23 @@ public class MxGatewayCommandException extends MxGatewayException { this.reply = reply; } + /** + * Creates a new command exception with an already-built, verbatim message. + * Used to re-surface a failure with a redacted message while preserving the + * original protocol status and reply. + * + * @param message the exact message to surface (already formatted/redacted) + * @param protocolStatus protocol status returned by the gateway + * @param reply raw command reply, or {@code null} when none was produced + * @param cause underlying error, or {@code null} + */ + protected MxGatewayCommandException( + String message, ProtocolStatus protocolStatus, MxCommandReply reply, Throwable cause) { + super(message, cause); + this.protocolStatus = protocolStatus; + this.reply = reply; + } + /** * Returns the gateway protocol status that triggered this exception. * diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayMalformedReplyException.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayMalformedReplyException.java new file mode 100644 index 0000000..230a3e8 --- /dev/null +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayMalformedReplyException.java @@ -0,0 +1,32 @@ +package com.zb.mom.ww.mxgateway.client; + +/** + * Thrown when the gateway returns a protocol-OK command reply that carries + * neither the expected typed payload nor a usable {@code return_value}. + * + *

A successful reply for a value-returning command (for example + * {@code AuthenticateUser}, {@code ArchestrAUserToId}, or {@code AddBufferedItem}) + * must supply either the command's typed payload or an int32 {@code return_value}. + * A reply that satisfies neither is malformed, and the client surfaces this + * distinct failure rather than silently returning a default {@code 0}. + */ +public final class MxGatewayMalformedReplyException extends MxGatewayException { + /** + * Creates a new malformed-reply exception with the supplied message. + * + * @param message human-readable description of the malformed reply + */ + public MxGatewayMalformedReplyException(String message) { + super(message); + } + + /** + * Creates a new malformed-reply exception with the supplied message and cause. + * + * @param message human-readable description of the malformed reply + * @param cause underlying error that triggered the failure + */ + public MxGatewayMalformedReplyException(String message, Throwable cause) { + super(message, cause); + } +} diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySecrets.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySecrets.java index 943b0b5..49b2a2e 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySecrets.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySecrets.java @@ -54,4 +54,32 @@ public final class MxGatewaySecrets { } return String.join(" ", parts); } + + /** + * Replaces every occurrence of each supplied secret with the redaction + * marker {@code ""}. Unlike {@link #redactCredentials(String)}, + * which scrubs by pattern, this performs an exact-substring scrub of the + * caller-known secrets — used to strip a credential the gateway echoed back + * into a free-form failure message. + * + * @param message the message to scrub, may be {@code null} + * @param secrets the exact secret substrings to remove; {@code null}/empty + * entries and a {@code null} array are ignored + * @return {@code message} unchanged when it is {@code null} or no non-empty + * secret is supplied, otherwise the message with every secret occurrence + * replaced by {@code ""} + */ + public static String redactExact(String message, String... secrets) { + if (message == null || secrets == null) { + return message; + } + + String result = message; + for (String secret : secrets) { + if (secret != null && !secret.isEmpty()) { + result = result.replace(secret, ""); + } + } + return result; + } } diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySession.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySession.java index 68c3843..bb21891 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySession.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySession.java @@ -782,15 +782,17 @@ public final class MxGatewaySession implements AutoCloseable { */ public MxCommandReply writeSecuredRaw( int serverHandle, int itemHandle, int currentUserId, int verifierUserId, MxValue value) { - return invokeCommand(MxCommand.newBuilder() - .setKind(MxCommandKind.MX_COMMAND_KIND_WRITE_SECURED) - .setWriteSecured(WriteSecuredCommand.newBuilder() - .setServerHandle(serverHandle) - .setItemHandle(itemHandle) - .setCurrentUserId(currentUserId) - .setVerifierUserId(verifierUserId) - .setValue(value)) - .build()); + return invokeCommandRedacted( + MxCommand.newBuilder() + .setKind(MxCommandKind.MX_COMMAND_KIND_WRITE_SECURED) + .setWriteSecured(WriteSecuredCommand.newBuilder() + .setServerHandle(serverHandle) + .setItemHandle(itemHandle) + .setCurrentUserId(currentUserId) + .setVerifierUserId(verifierUserId) + .setValue(value)) + .build(), + secretStringOf(value)); } /** @@ -837,16 +839,18 @@ public final class MxGatewaySession implements AutoCloseable { int verifierUserId, MxValue value, MxValue timestampValue) { - return invokeCommand(MxCommand.newBuilder() - .setKind(MxCommandKind.MX_COMMAND_KIND_WRITE_SECURED2) - .setWriteSecured2(WriteSecured2Command.newBuilder() - .setServerHandle(serverHandle) - .setItemHandle(itemHandle) - .setCurrentUserId(currentUserId) - .setVerifierUserId(verifierUserId) - .setValue(value) - .setTimestampValue(timestampValue)) - .build()); + return invokeCommandRedacted( + MxCommand.newBuilder() + .setKind(MxCommandKind.MX_COMMAND_KIND_WRITE_SECURED2) + .setWriteSecured2(WriteSecured2Command.newBuilder() + .setServerHandle(serverHandle) + .setItemHandle(itemHandle) + .setCurrentUserId(currentUserId) + .setVerifierUserId(verifierUserId) + .setValue(value) + .setTimestampValue(timestampValue)) + .build(), + secretStringOf(value)); } /** @@ -866,17 +870,24 @@ public final class MxGatewaySession implements AutoCloseable { * @throws MxAccessException when MXAccess rejects the credential */ public int authenticateUser(int serverHandle, String verifyUser, String verifyUserPassword) { - MxCommandReply reply = invokeCommand(MxCommand.newBuilder() - .setKind(MxCommandKind.MX_COMMAND_KIND_AUTHENTICATE_USER) - .setAuthenticateUser(AuthenticateUserCommand.newBuilder() - .setServerHandle(serverHandle) - .setVerifyUser(verifyUser) - .setVerifyUserPassword(verifyUserPassword)) - .build()); + MxCommandReply reply = invokeCommandRedacted( + MxCommand.newBuilder() + .setKind(MxCommandKind.MX_COMMAND_KIND_AUTHENTICATE_USER) + .setAuthenticateUser(AuthenticateUserCommand.newBuilder() + .setServerHandle(serverHandle) + .setVerifyUser(verifyUser) + .setVerifyUserPassword(verifyUserPassword)) + .build(), + verifyUserPassword); if (reply.hasAuthenticateUser()) { return reply.getAuthenticateUser().getUserId(); } - return reply.getReturnValue().getInt32Value(); + if (reply.hasReturnValue() && reply.getReturnValue().getKindCase() == MxValue.KindCase.INT32_VALUE) { + return reply.getReturnValue().getInt32Value(); + } + throw new MxGatewayMalformedReplyException( + "AuthenticateUser returned a malformed reply: OK reply carried neither " + + "the typed payload nor an int32 return_value"); } /** @@ -899,7 +910,12 @@ public final class MxGatewaySession implements AutoCloseable { if (reply.hasArchestraUserToId()) { return reply.getArchestraUserToId().getUserId(); } - return reply.getReturnValue().getInt32Value(); + if (reply.hasReturnValue() && reply.getReturnValue().getKindCase() == MxValue.KindCase.INT32_VALUE) { + return reply.getReturnValue().getInt32Value(); + } + throw new MxGatewayMalformedReplyException( + "ArchestrAUserToId returned a malformed reply: OK reply carried neither " + + "the typed payload nor an int32 return_value"); } /** @@ -925,7 +941,12 @@ public final class MxGatewaySession implements AutoCloseable { if (reply.hasAddBufferedItem()) { return reply.getAddBufferedItem().getItemHandle(); } - return reply.getReturnValue().getInt32Value(); + if (reply.hasReturnValue() && reply.getReturnValue().getKindCase() == MxValue.KindCase.INT32_VALUE) { + return reply.getReturnValue().getInt32Value(); + } + throw new MxGatewayMalformedReplyException( + "AddBufferedItem returned a malformed reply: OK reply carried neither " + + "the typed payload nor an int32 return_value"); } /** @@ -1027,6 +1048,49 @@ public final class MxGatewaySession implements AutoCloseable { .build()); } + /** + * Invokes a credential-bearing command, scrubbing any exact secret the + * gateway may have echoed back into a surfaced failure message. The secret + * lives only in the request, but a non-parity gateway or provider can copy + * it into a diagnostic; this guarantees it never survives in the exception + * text a caller might log. + * + *

On failure the original exception's message is scrubbed with + * {@link MxGatewaySecrets#redactExact}. If nothing changed (the common case + * where the message never carried the secret), the original exception is + * rethrown untouched. Otherwise it is re-thrown as the same concrete type + * carrying the redacted message; the secret-bearing original is not chained + * as a cause, so it cannot leak through a printed stack trace. + */ + private MxCommandReply invokeCommandRedacted(MxCommand command, String... secrets) { + try { + return invokeCommand(command); + } catch (MxGatewayException ex) { + String original = ex.getMessage(); + String redacted = MxGatewaySecrets.redactExact(original, secrets); + if (redacted == null || redacted.equals(original)) { + throw ex; + } + if (ex instanceof MxAccessException mx) { + throw new MxAccessException(redacted, mx.protocolStatus(), mx.reply(), null); + } + throw new MxGatewayException(redacted); + } + } + + /** + * Extracts the string payload of a secured-write value so it can be scrubbed + * from an echoed failure message. Only string-kind values carry a + * credential-shaped secret worth redacting; other kinds return {@code null} + * (ignored by {@link MxGatewaySecrets#redactExact}). + */ + private static String secretStringOf(MxValue value) { + if (value != null && value.getKindCase() == MxValue.KindCase.STRING_VALUE) { + return value.getStringValue(); + } + return null; + } + private static String newCorrelationId() { byte[] bytes = new byte[16]; RANDOM.nextBytes(bytes); diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayCredentialReplyTests.java b/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayCredentialReplyTests.java new file mode 100644 index 0000000..9b805a9 --- /dev/null +++ b/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayCredentialReplyTests.java @@ -0,0 +1,172 @@ +package com.zb.mom.ww.mxgateway.client; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.google.protobuf.util.JsonFormat; +import io.grpc.ManagedChannel; +import io.grpc.Server; +import io.grpc.inprocess.InProcessChannelBuilder; +import io.grpc.inprocess.InProcessServerBuilder; +import io.grpc.stub.StreamObserver; +import java.nio.file.Files; +import java.nio.file.Path; +import java.time.Duration; +import java.util.UUID; +import mxaccess_gateway.v1.MxAccessGatewayGrpc; +import mxaccess_gateway.v1.MxaccessGateway.MxCommandReply; +import mxaccess_gateway.v1.MxaccessGateway.MxCommandRequest; +import mxaccess_gateway.v1.MxaccessGateway.MxValue; +import mxaccess_gateway.v1.MxaccessGateway.ProtocolStatus; +import mxaccess_gateway.v1.MxaccessGateway.ProtocolStatusCode; +import org.junit.jupiter.api.Test; + +final class MxGatewayCredentialReplyTests { + private static final String CREDENTIAL = "sup3rSecretVerify9f3a2b"; + + @Test + void authenticateUserRedactsEchoedCredentialFromReplyDrivenError() throws Exception { + MxCommandReply reply = loadReply("authenticate-user.echoed-credential.reply.json"); + + try (InProcessGateway gateway = InProcessGateway.startReturning(reply); + MxGatewayClient client = gateway.client()) { + MxGatewaySession session = MxGatewaySession.forSessionId(client, "auth-echo-session"); + + MxAccessException error = assertThrows( + MxAccessException.class, + () -> session.authenticateUser(12, "operator", CREDENTIAL)); + + assertFalse(error.getMessage().contains(CREDENTIAL), + "credential echoed by the gateway must not survive in the surfaced message"); + assertTrue(error.getMessage().contains(""), + "the echoed credential must be replaced with the redaction marker"); + } + } + + @Test + void authenticateUserMissingPayloadThrowsMalformedReply() throws Exception { + MxCommandReply reply = loadReply("authenticate-user.missing-payload.reply.json"); + + try (InProcessGateway gateway = InProcessGateway.startReturning(reply); + MxGatewayClient client = gateway.client()) { + MxGatewaySession session = MxGatewaySession.forSessionId(client, "auth-missing-session"); + + assertThrows( + MxGatewayMalformedReplyException.class, + () -> session.authenticateUser(3, "operator", "pw")); + } + } + + @Test + void authenticateUserReturnValueOnlyReplyReturnsInt32Fallback() throws Exception { + MxCommandReply reply = loadReply("authenticate-user.return-value-only.reply.json"); + + try (InProcessGateway gateway = InProcessGateway.startReturning(reply); + MxGatewayClient client = gateway.client()) { + MxGatewaySession session = MxGatewaySession.forSessionId(client, "auth-return-session"); + + assertEquals(7, session.authenticateUser(3, "operator", "pw")); + } + } + + @Test + void addBufferedItemReturnValueOnlyReplyReturnsInt32Fallback() throws Exception { + MxCommandReply reply = MxCommandReply.newBuilder() + .setProtocolStatus(ok()) + .setReturnValue(MxValue.newBuilder().setInt32Value(55)) + .build(); + + try (InProcessGateway gateway = InProcessGateway.startReturning(reply); + MxGatewayClient client = gateway.client()) { + MxGatewaySession session = MxGatewaySession.forSessionId(client, "buffered-return-session"); + + assertEquals(55, session.addBufferedItem(3, "Tank01.Level", "galaxy")); + } + } + + @Test + void addBufferedItemMissingPayloadThrowsMalformedReply() throws Exception { + MxCommandReply reply = MxCommandReply.newBuilder().setProtocolStatus(ok()).build(); + + try (InProcessGateway gateway = InProcessGateway.startReturning(reply); + MxGatewayClient client = gateway.client()) { + MxGatewaySession session = MxGatewaySession.forSessionId(client, "buffered-malformed-session"); + + assertThrows( + MxGatewayMalformedReplyException.class, + () -> session.addBufferedItem(3, "Tank01.Level", "galaxy")); + } + } + + private static ProtocolStatus ok() { + return ProtocolStatus.newBuilder() + .setCode(ProtocolStatusCode.PROTOCOL_STATUS_CODE_OK) + .build(); + } + + private static MxCommandReply loadReply(String fixture) throws Exception { + MxCommandReply.Builder builder = MxCommandReply.newBuilder(); + JsonFormat.parser().merge( + Files.readString(fixtureRoot().resolve("command-replies/" + fixture)), + builder); + return builder.build(); + } + + private static Path fixtureRoot() { + Path current = Path.of(System.getProperty("user.dir")).toAbsolutePath(); + for (Path path = current; path != null; path = path.getParent()) { + Path candidate = path.resolve("clients/proto/fixtures/behavior"); + if (Files.exists(candidate)) { + return candidate; + } + candidate = path.resolve("../proto/fixtures/behavior").normalize(); + if (Files.exists(candidate)) { + return candidate; + } + } + throw new IllegalStateException("could not locate behavior fixtures from " + current); + } + + private record InProcessGateway(Server server, ManagedChannel channel) implements AutoCloseable { + static InProcessGateway startReturning(MxCommandReply reply) throws Exception { + String serverName = "mxgw-java-cred-" + UUID.randomUUID(); + MxAccessGatewayGrpc.MxAccessGatewayImplBase service = + new MxAccessGatewayGrpc.MxAccessGatewayImplBase() { + @Override + public void invoke( + MxCommandRequest request, StreamObserver responseObserver) { + responseObserver.onNext(reply); + responseObserver.onCompleted(); + } + }; + Server server = InProcessServerBuilder.forName(serverName) + .directExecutor() + .addService(service) + .build() + .start(); + ManagedChannel channel = InProcessChannelBuilder.forName(serverName) + .directExecutor() + .build(); + return new InProcessGateway(server, channel); + } + + MxGatewayClient client() { + return new MxGatewayClient( + channel, + MxGatewayClientOptions.builder() + .endpoint("in-process") + .apiKey("") + .plaintext(true) + .callTimeout(Duration.ofSeconds(5)) + .build()); + } + + @Override + public void close() { + channel.shutdownNow(); + server.shutdownNow(); + } + } +} diff --git a/clients/proto/fixtures/behavior/command-replies/authenticate-user.echoed-credential.reply.json b/clients/proto/fixtures/behavior/command-replies/authenticate-user.echoed-credential.reply.json new file mode 100644 index 0000000..e7ebd0a --- /dev/null +++ b/clients/proto/fixtures/behavior/command-replies/authenticate-user.echoed-credential.reply.json @@ -0,0 +1,22 @@ +{ + "sessionId": "session-fixture", + "correlationId": "gateway-correlation-authenticate-echoed", + "kind": "MX_COMMAND_KIND_AUTHENTICATE_USER", + "protocolStatus": { + "code": "PROTOCOL_STATUS_CODE_OK", + "message": "MXAccess AuthenticateUser rejected credential 'sup3rSecretVerify9f3a2b'." + }, + "hresult": -2147024891, + "statuses": [ + { + "success": 0, + "category": "MX_STATUS_CATEGORY_SECURITY_ERROR", + "detectedBy": "MX_STATUS_SOURCE_RESPONDING_NMX", + "detail": 5, + "rawCategory": 8, + "rawDetectedBy": 5, + "diagnosticText": "Authentication failed for password 'sup3rSecretVerify9f3a2b'." + } + ], + "diagnosticMessage": "MXAccess echoed the credential 'sup3rSecretVerify9f3a2b' back in its failure diagnostic." +} diff --git a/clients/proto/fixtures/behavior/command-replies/authenticate-user.missing-payload.reply.json b/clients/proto/fixtures/behavior/command-replies/authenticate-user.missing-payload.reply.json new file mode 100644 index 0000000..cd9b543 --- /dev/null +++ b/clients/proto/fixtures/behavior/command-replies/authenticate-user.missing-payload.reply.json @@ -0,0 +1,10 @@ +{ + "sessionId": "session-fixture", + "correlationId": "gateway-correlation-authenticate-missing-payload", + "kind": "MX_COMMAND_KIND_AUTHENTICATE_USER", + "protocolStatus": { + "code": "PROTOCOL_STATUS_CODE_OK", + "message": "AuthenticateUser reached MXAccess." + }, + "diagnosticMessage": "Malformed: the OK reply carried neither an AuthenticateUser payload nor a return_value." +} diff --git a/clients/proto/fixtures/behavior/command-replies/authenticate-user.return-value-only.reply.json b/clients/proto/fixtures/behavior/command-replies/authenticate-user.return-value-only.reply.json new file mode 100644 index 0000000..c0ed830 --- /dev/null +++ b/clients/proto/fixtures/behavior/command-replies/authenticate-user.return-value-only.reply.json @@ -0,0 +1,15 @@ +{ + "sessionId": "session-fixture", + "correlationId": "gateway-correlation-authenticate-return-value-only", + "kind": "MX_COMMAND_KIND_AUTHENTICATE_USER", + "protocolStatus": { + "code": "PROTOCOL_STATUS_CODE_OK", + "message": "AuthenticateUser reached MXAccess." + }, + "returnValue": { + "dataType": "MX_DATA_TYPE_INTEGER", + "variantType": "VT_I4", + "int32Value": 7 + }, + "diagnosticMessage": "Legacy worker populated only return_value; the typed AuthenticateUser payload is absent." +} diff --git a/clients/proto/fixtures/behavior/manifest.json b/clients/proto/fixtures/behavior/manifest.json index 13fd66f..1b56674 100644 --- a/clients/proto/fixtures/behavior/manifest.json +++ b/clients/proto/fixtures/behavior/manifest.json @@ -48,6 +48,27 @@ "path": "command-replies/write.hresult-e-fail.reply.json", "expectation": "A negative HRESULT fails the reply even when every status entry reports MX_STATUS_CATEGORY_OK." }, + { + "id": "command-reply.authenticate-user.echoed-credential", + "category": "command_replies", + "messageType": "mxaccess_gateway.v1.MxCommandReply", + "path": "command-replies/authenticate-user.echoed-credential.reply.json", + "expectation": "When a gateway/MXAccess diagnostic echoes the caller's credential back, the surfaced error redacts the exact secret and never leaks the verbatim value." + }, + { + "id": "command-reply.authenticate-user.missing-payload", + "category": "command_replies", + "messageType": "mxaccess_gateway.v1.MxCommandReply", + "path": "command-replies/authenticate-user.missing-payload.reply.json", + "expectation": "An OK reply with neither the typed AuthenticateUser payload nor a return_value raises a typed malformed-reply error, never a proto3 default 0 and never an NRE." + }, + { + "id": "command-reply.authenticate-user.return-value-only", + "category": "command_replies", + "messageType": "mxaccess_gateway.v1.MxCommandReply", + "path": "command-replies/authenticate-user.return-value-only.reply.json", + "expectation": "An OK reply missing the typed AuthenticateUser payload but carrying an int32 return_value falls back to the return_value (legacy-worker compatibility)." + }, { "id": "event-stream.session-ordered", "category": "event_streams", diff --git a/clients/proto/fixtures/behavior/statuses/status-conversion-cases.json b/clients/proto/fixtures/behavior/statuses/status-conversion-cases.json index 4219463..7f309e7 100644 --- a/clients/proto/fixtures/behavior/statuses/status-conversion-cases.json +++ b/clients/proto/fixtures/behavior/statuses/status-conversion-cases.json @@ -3,6 +3,7 @@ "cases": [ { "id": "ok.responding-lmx", + "wantSuccess": true, "status": { "success": 1, "category": "MX_STATUS_CATEGORY_OK", @@ -15,6 +16,7 @@ }, { "id": "security-error.requesting-lmx", + "wantSuccess": false, "status": { "success": 0, "category": "MX_STATUS_CATEGORY_SECURITY_ERROR", @@ -27,6 +29,7 @@ }, { "id": "raw-unknown-category", + "wantSuccess": false, "status": { "success": 0, "category": "MX_STATUS_CATEGORY_UNKNOWN", diff --git a/clients/python/src/zb_mom_ww_mxgateway/__init__.py b/clients/python/src/zb_mom_ww_mxgateway/__init__.py index 0d9943d..1f8f31b 100644 --- a/clients/python/src/zb_mom_ww_mxgateway/__init__.py +++ b/clients/python/src/zb_mom_ww_mxgateway/__init__.py @@ -11,6 +11,7 @@ from .generated.galaxy_repository_pb2 import ( ) from .events import ReplayGap from .errors import ( + MalformedReplyError, MxAccessError, MxGatewayAuthenticationError, MxGatewayAuthorizationError, @@ -35,6 +36,7 @@ __all__ = [ "GalaxyRepositoryClient", "GatewayClient", "LazyBrowseNode", + "MalformedReplyError", "MxAccessError", "MxGatewayAuthenticationError", "MxGatewayAuthorizationError", diff --git a/clients/python/src/zb_mom_ww_mxgateway/errors.py b/clients/python/src/zb_mom_ww_mxgateway/errors.py index d7f65db..ca2770c 100644 --- a/clients/python/src/zb_mom_ww_mxgateway/errors.py +++ b/clients/python/src/zb_mom_ww_mxgateway/errors.py @@ -53,6 +53,10 @@ class MxAccessError(MxGatewayCommandError): """MXAccess HRESULT or status failure.""" +class MalformedReplyError(MxGatewayError): + """Raised when an OK reply lacks the expected typed payload and any usable return_value fallback.""" + + def map_rpc_error(operation: str, error: grpc.RpcError) -> MxGatewayTransportError: """Map a generated gRPC exception to the client exception hierarchy.""" @@ -153,8 +157,18 @@ def ensure_mxaccess_success(operation: str, reply: pb.MxCommandReply) -> pb.MxCo def _mxaccess_message(operation: str, reply: pb.MxCommandReply) -> str: status_text = reply.protocol_status.message or "MXAccess command failed" hresult = reply.hresult if reply.HasField("hresult") else None - return ( + message = ( f"{operation} failed: {status_text}; " f"session={reply.session_id}; correlation={reply.correlation_id}; " f"hresult={hresult}; statuses={len(reply.statuses)}" ) + # Append a per-status breakdown that carries the raw `success` COM member + # verbatim for diagnostic parity with the other clients. `category` remains + # the authoritative verdict; `success` is diagnostics only. + for status in reply.statuses: + category = pb.MxStatusCategory.Name(status.category) + message += ( + f" [success={status.success}, category={category}, " + f"detail={status.detail}, {status.diagnostic_text}]" + ) + return message diff --git a/clients/python/src/zb_mom_ww_mxgateway/session.py b/clients/python/src/zb_mom_ww_mxgateway/session.py index 057d37f..724d619 100644 --- a/clients/python/src/zb_mom_ww_mxgateway/session.py +++ b/clients/python/src/zb_mom_ww_mxgateway/session.py @@ -5,7 +5,7 @@ from __future__ import annotations from collections.abc import AsyncIterator, Sequence from .auth import redact_secret -from .errors import MxGatewayError, ensure_mxaccess_success +from .errors import MalformedReplyError, MxGatewayError, ensure_mxaccess_success from .events import ReplayGap from .generated import mxaccess_gateway_pb2 as pb from .values import MxValueInput, to_mx_value @@ -710,7 +710,15 @@ class Session: correlation_id=correlation_id, secrets=[verify_user_password], ) - return reply.authenticate_user.user_id + if reply.HasField("authenticate_user"): + return reply.authenticate_user.user_id + if reply.HasField("return_value") and reply.return_value.WhichOneof("kind") == "int32_value": + return reply.return_value.int32_value + raise MalformedReplyError( + "authenticate_user returned a malformed reply: OK reply carried " + "neither the typed payload nor an int32 return_value", + raw_reply=reply, + ) async def archestra_user_to_id( self, @@ -730,7 +738,15 @@ class Session: ), correlation_id=correlation_id, ) - return reply.archestra_user_to_id.user_id + if reply.HasField("archestra_user_to_id"): + return reply.archestra_user_to_id.user_id + if reply.HasField("return_value") and reply.return_value.WhichOneof("kind") == "int32_value": + return reply.return_value.int32_value + raise MalformedReplyError( + "archestra_user_to_id returned a malformed reply: OK reply carried " + "neither the typed payload nor an int32 return_value", + raw_reply=reply, + ) async def add_buffered_item( self, @@ -752,7 +768,15 @@ class Session: ), correlation_id=correlation_id, ) - return reply.add_buffered_item.item_handle + if reply.HasField("add_buffered_item"): + return reply.add_buffered_item.item_handle + if reply.HasField("return_value") and reply.return_value.WhichOneof("kind") == "int32_value": + return reply.return_value.int32_value + raise MalformedReplyError( + "add_buffered_item returned a malformed reply: OK reply carried " + "neither the typed payload nor an int32 return_value", + raw_reply=reply, + ) async def set_buffered_update_interval( self, diff --git a/clients/python/tests/test_malformed_reply.py b/clients/python/tests/test_malformed_reply.py new file mode 100644 index 0000000..bc1c634 --- /dev/null +++ b/clients/python/tests/test_malformed_reply.py @@ -0,0 +1,96 @@ +"""Tests for the uniform malformed-reply contract (CLI-41) and the CLI-40 +credential-redaction regression, driven through the shared fixtures. + +CLI-41: an OK reply that carries neither the expected typed payload nor a usable +``return_value`` int32 fallback raises :class:`MalformedReplyError`; a legacy +reply that populates only ``return_value`` falls back to that int32. + +CLI-40: an OK reply whose diagnostics echo the caller's credential must never +surface that credential in the raised error message. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest +from google.protobuf.json_format import ParseDict + +from zb_mom_ww_mxgateway import MalformedReplyError, MxAccessError +from zb_mom_ww_mxgateway.generated import mxaccess_gateway_pb2 as pb + +from test_typed_command_helpers import _session_with + +FIXTURE_ROOT = Path(__file__).resolve().parents[2] / "proto" / "fixtures" / "behavior" + + +def _load_reply(relative: str) -> pb.MxCommandReply: + path = FIXTURE_ROOT / relative + return ParseDict(json.loads(path.read_text()), pb.MxCommandReply()) + + +@pytest.mark.asyncio +async def test_authenticate_user_missing_payload_raises_malformed_reply() -> None: + reply = _load_reply("command-replies/authenticate-user.missing-payload.reply.json") + session, _ = await _session_with([reply]) + + with pytest.raises(MalformedReplyError) as captured: + await session.authenticate_user(12, "operator", "any-password") + + assert captured.value.raw_reply is reply + assert "malformed reply" in str(captured.value) + + +@pytest.mark.asyncio +async def test_authenticate_user_return_value_only_falls_back_to_int32() -> None: + reply = _load_reply("command-replies/authenticate-user.return-value-only.reply.json") + session, _ = await _session_with([reply]) + + user_id = await session.authenticate_user(12, "operator", "any-password") + + assert user_id == 7 + + +@pytest.mark.asyncio +async def test_add_buffered_item_falls_back_to_return_value_int32() -> None: + reply = pb.MxCommandReply( + session_id="session-1", + kind=pb.MX_COMMAND_KIND_ADD_BUFFERED_ITEM, + protocol_status=pb.ProtocolStatus(code=pb.PROTOCOL_STATUS_CODE_OK), + return_value=pb.MxValue(int32_value=99), + ) + session, _ = await _session_with([reply]) + + item_handle = await session.add_buffered_item(12, "Object.Attribute", "ctx") + + assert item_handle == 99 + + +@pytest.mark.asyncio +async def test_add_buffered_item_missing_payload_raises_malformed_reply() -> None: + reply = pb.MxCommandReply( + session_id="session-1", + kind=pb.MX_COMMAND_KIND_ADD_BUFFERED_ITEM, + protocol_status=pb.ProtocolStatus(code=pb.PROTOCOL_STATUS_CODE_OK), + ) + session, _ = await _session_with([reply]) + + with pytest.raises(MalformedReplyError) as captured: + await session.add_buffered_item(12, "Object.Attribute", "ctx") + + assert captured.value.raw_reply is reply + + +@pytest.mark.asyncio +async def test_authenticate_user_echoed_credential_is_scrubbed() -> None: + credential = "sup3rSecretVerify9f3a2b" + reply = _load_reply("command-replies/authenticate-user.echoed-credential.reply.json") + session, _ = await _session_with([reply]) + + with pytest.raises(MxAccessError) as captured: + await session.authenticate_user(12, "operator", credential) + + message = str(captured.value) + assert credential not in message + assert "[redacted]" in message diff --git a/clients/rust/src/error.rs b/clients/rust/src/error.rs index 0a0bf74..90b22a6 100644 --- a/clients/rust/src/error.rs +++ b/clients/rust/src/error.rs @@ -193,17 +193,43 @@ impl std::error::Error for CommandError {} /// The wrapper is heap-allocated inside [`Error::MxAccess`] to keep the /// containing enum small. Callers can recover the reply with /// [`MxAccessError::reply`] or [`MxAccessError::into_reply`]. Its `Display` -/// summarizes the `hresult` and status entries and scrubs any credential-like -/// tokens from diagnostic text before it reaches a caller. -#[derive(Clone, Debug)] +/// summarizes the `hresult` and status entries and scrubs credentials from the +/// rendered text before it reaches a caller: credential-*shaped* tokens +/// (`mxgw_...`, `bearer`) via a pattern scrub, plus any exact caller-supplied +/// secrets registered with [`MxAccessError::with_secrets`] — the latter catches +/// a password MXAccess echoed back verbatim even though it has no token shape. +/// +/// `Debug` is hand-written (not derived) so the attached exact secrets never +/// reach `{:?}` output either: it scrubs them from the reply rendering and +/// prints only the count of attached secrets, never their values. +#[derive(Clone)] pub struct MxAccessError { reply: MxCommandReply, + /// Exact caller-supplied secrets (e.g. an `AuthenticateUser` password or a + /// `WriteSecured` string value) scrubbed from the rendered message. Empty + /// unless a helper attaches them via [`Self::with_secrets`]. + secrets: Vec, } impl MxAccessError { /// Wrap a reply whose MXAccess-level result reported a failure. pub fn new(reply: MxCommandReply) -> Self { - Self { reply } + Self { + reply, + secrets: Vec::new(), + } + } + + /// Register exact caller-supplied secrets to scrub from the rendered + /// message, returning the updated error. + /// + /// A credential MXAccess echoes back into its diagnostic text has no + /// `mxgw_`/`bearer` shape, so the pattern scrub cannot catch it. Attaching + /// the exact secret lets `Display` replace every occurrence with + /// ``. + pub fn with_secrets(mut self, secrets: Vec) -> Self { + self.secrets = secrets; + self } /// Borrow the underlying reply (correlation id, hresult, statuses). @@ -217,15 +243,43 @@ impl MxAccessError { } } +impl std::fmt::Debug for MxAccessError { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + // Render the reply, scrub any exact caller secret from it, and never + // print the raw secrets themselves — only how many are attached. + let mut reply = format!("{:?}", self.reply); + for secret in &self.secrets { + if !secret.is_empty() { + reply = reply.replace(secret.as_str(), ""); + } + } + + formatter + .debug_struct("MxAccessError") + .field("reply", &format_args!("{reply}")) + .field( + "secrets", + &format_args!("[{} redacted]", self.secrets.len()), + ) + .finish() + } +} + impl std::fmt::Display for MxAccessError { fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + use std::fmt::Write as _; + let hresult = match self.reply.hresult { Some(value) => value.to_string(), None => "none".to_owned(), }; + // Render the whole body first so the exact-secret scrub can sweep every + // field — including diagnostic text that already went through the + // credential-shape scrub — before any of it reaches the caller. + let mut body = String::new(); write!( - formatter, + body, "hresult={hresult}, {} status entr{}", self.reply.statuses.len(), if self.reply.statuses.len() == 1 { @@ -233,20 +287,28 @@ impl std::fmt::Display for MxAccessError { } else { "ies" } - )?; + ) + .expect("writing to a String is infallible"); for status in &self.reply.statuses { let category = MxStatusCategory::try_from(status.category) .unwrap_or(MxStatusCategory::Unspecified); let diagnostic = redact_credentials(&status.diagnostic_text); write!( - formatter, + body, "; [success={}, category={category:?}, detail={}, {}]", status.success, status.detail, diagnostic - )?; + ) + .expect("writing to a String is infallible"); } - Ok(()) + for secret in &self.secrets { + if !secret.is_empty() { + body = body.replace(secret.as_str(), ""); + } + } + + formatter.write_str(&body) } } diff --git a/clients/rust/src/session.rs b/clients/rust/src/session.rs index 621474a..fd6768e 100644 --- a/clients/rust/src/session.rs +++ b/clients/rust/src/session.rs @@ -27,7 +27,7 @@ use crate::generated::mxaccess_gateway::v1::{ WriteSecured2BulkCommand, WriteSecured2BulkEntry, WriteSecured2Command, WriteSecuredBulkCommand, WriteSecuredBulkEntry, WriteSecuredCommand, }; -use crate::value::{MxStatus, MxValue}; +use crate::value::{MxStatus, MxValue, MxValueProjection}; const MAX_BULK_ITEMS: usize = 1_000; @@ -801,6 +801,7 @@ impl Session { verifier_user_id: i32, value: MxValue, ) -> Result<(), Error> { + let secrets = string_secret(&value); self.invoke( MxCommandKind::WriteSecured, Payload::WriteSecured(WriteSecuredCommand { @@ -811,7 +812,8 @@ impl Session { value: Some(value.into_proto()), }), ) - .await?; + .await + .map_err(|error| attach_secrets(error, secrets))?; Ok(()) } @@ -831,6 +833,7 @@ impl Session { value: MxValue, timestamp_value: MxValue, ) -> Result<(), Error> { + let secrets = string_secret(&value); self.invoke( MxCommandKind::WriteSecured2, Payload::WriteSecured2(WriteSecured2Command { @@ -842,7 +845,8 @@ impl Session { timestamp_value: Some(timestamp_value.into_proto()), }), ) - .await?; + .await + .map_err(|error| attach_secrets(error, secrets))?; Ok(()) } @@ -882,7 +886,8 @@ impl Session { verify_user_password: verify_user_password.to_owned(), }), ) - .await?; + .await + .map_err(|error| attach_secrets(error, vec![verify_user_password.to_owned()]))?; authenticate_user_id(&reply) } @@ -1074,18 +1079,51 @@ fn add_buffered_item_handle(reply: &MxCommandReply) -> Result { fn authenticate_user_id(reply: &MxCommandReply) -> Result { match reply.payload.as_ref() { Some(mx_command_reply::Payload::AuthenticateUser(authenticate)) => Ok(authenticate.user_id), - _ => Err(Error::MalformedReply { - detail: "authenticate_user reply lacked an AuthenticateUser payload".to_owned(), - }), + _ => reply + .return_value + .as_ref() + .and_then(int32_reply_value) + .ok_or_else(|| Error::MalformedReply { + detail: + "authenticate_user reply lacked an AuthenticateUser payload or int32 return_value" + .to_owned(), + }), } } fn archestra_user_id(reply: &MxCommandReply) -> Result { match reply.payload.as_ref() { Some(mx_command_reply::Payload::ArchestraUserToId(archestra)) => Ok(archestra.user_id), - _ => Err(Error::MalformedReply { - detail: "archestra_user_to_id reply lacked an ArchestraUserToId payload".to_owned(), - }), + _ => reply + .return_value + .as_ref() + .and_then(int32_reply_value) + .ok_or_else(|| Error::MalformedReply { + detail: + "archestra_user_to_id reply lacked an ArchestraUserToId payload or int32 return_value" + .to_owned(), + }), + } +} + +/// Extract an exact string secret from a credential-sensitive [`MxValue`] so a +/// failing `WriteSecured`/`WriteSecured2` can scrub it from the surfaced error. +/// Non-string values carry no scrubbable secret and yield an empty vector. +fn string_secret(value: &MxValue) -> Vec { + match value.projection() { + MxValueProjection::String(text) if !text.is_empty() => vec![text.clone()], + _ => Vec::new(), + } +} + +/// Attach caller-supplied exact secrets to an [`Error::MxAccess`] before it +/// propagates, so its `Display` scrubs any occurrence of the credential (e.g. a +/// password MXAccess echoed back verbatim). Any other error variant is returned +/// unchanged. +fn attach_secrets(error: Error, secrets: Vec) -> Error { + match error { + Error::MxAccess(boxed) => Error::MxAccess(Box::new(boxed.with_secrets(secrets))), + other => other, } } diff --git a/clients/rust/tests/client_behavior.rs b/clients/rust/tests/client_behavior.rs index 28f4a58..84323ca 100644 --- a/clients/rust/tests/client_behavior.rs +++ b/clients/rust/tests/client_behavior.rs @@ -804,6 +804,93 @@ async fn authenticate_user_keeps_credentials_out_of_surfaced_errors() { ); } +#[tokio::test] +async fn authenticate_user_scrubs_exact_caller_credential_echoed_in_diagnostic() { + // CLI-40: MXAccess can echo the supplied credential back inside its failure + // diagnostic (here in statuses[0].diagnostic_text). The token has no + // mxgw_/bearer shape, so the pattern scrub alone cannot catch it — the + // exact-secret scrub must replace the caller's password with . + let credential = "sup3rSecretVerify9f3a2b"; + let state = Arc::new(FakeState::default()); + *state.invoke_override.lock().await = Some(InvokeOverride::CannedReply(Box::new( + command_reply_fixture("authenticate-user.echoed-credential.reply.json"), + ))); + let endpoint = spawn_fake_gateway(state.clone()).await; + let client = GatewayClient::connect(ClientOptions::new(endpoint)) + .await + .unwrap(); + let session = client.session("session-fixture"); + + let error = session + .authenticate_user(7, "verifier", credential) + .await + .unwrap_err(); + + assert!( + matches!(error, Error::MxAccess(_)), + "OK protocol + negative hresult must route to Error::MxAccess: {error:?}" + ); + let rendered = error.to_string(); + assert!( + !rendered.contains(credential), + "exact caller credential leaked into the surfaced error: {rendered}" + ); + assert!( + rendered.contains(""), + "credential occurrence must be replaced with : {rendered}" + ); +} + +#[tokio::test] +async fn authenticate_user_maps_missing_payload_reply_to_malformed_reply() { + // CLI-41: an OK reply with neither a typed AuthenticateUser payload nor a + // return_value is malformed. + let state = Arc::new(FakeState::default()); + *state.invoke_override.lock().await = Some(InvokeOverride::CannedReply(Box::new( + command_reply_fixture("authenticate-user.missing-payload.reply.json"), + ))); + let endpoint = spawn_fake_gateway(state.clone()).await; + let client = GatewayClient::connect(ClientOptions::new(endpoint)) + .await + .unwrap(); + let session = client.session("session-fixture"); + + let error = session + .authenticate_user(7, "verifier", "pw") + .await + .unwrap_err(); + + assert!( + matches!(error, Error::MalformedReply { .. }), + "missing payload + missing return_value must be MalformedReply, got {error:?}" + ); +} + +#[tokio::test] +async fn authenticate_user_falls_back_to_return_value_when_typed_payload_absent() { + // CLI-41: an OK reply that carries only a return_value (legacy worker) must + // resolve the user id from it, mirroring add_buffered_item's fallback. + let state = Arc::new(FakeState::default()); + *state.invoke_override.lock().await = Some(InvokeOverride::CannedReply(Box::new( + command_reply_fixture("authenticate-user.return-value-only.reply.json"), + ))); + let endpoint = spawn_fake_gateway(state.clone()).await; + let client = GatewayClient::connect(ClientOptions::new(endpoint)) + .await + .unwrap(); + let session = client.session("session-fixture"); + + let user_id = session + .authenticate_user(7, "verifier", "pw") + .await + .unwrap(); + + assert_eq!( + user_id, 7, + "user id must resolve from the int32 return_value" + ); +} + #[tokio::test] async fn stream_alarms_emits_snapshot_then_complete_then_transition_in_order() { let state = Arc::new(FakeState::default()); @@ -955,6 +1042,11 @@ enum InvokeOverride { /// `AuthenticateUser` rejected by MXAccess) so the client's /// `ensure_mxaccess_success` check is exercised on the typed helper path. MxAccessFailure, + /// Reply with a caller-supplied canned [`MxCommandReply`]. Lets a test + /// drive a helper with a shared behavior fixture (e.g. the + /// echoed-credential / missing-payload / return-value-only + /// authenticate-user replies). Boxed to keep the enum small. + CannedReply(Box), } #[derive(Clone)] @@ -1057,6 +1149,7 @@ impl MxAccessGateway for FakeGateway { payload: None, ..MxCommandReply::default() })), + InvokeOverride::CannedReply(reply) => Ok(Response::new(*reply)), InvokeOverride::WriteOk => { // Extract and capture the WriteCommand payload so the test // can assert on server_handle, item_handle, user_id, and value. @@ -1443,6 +1536,18 @@ fn command_reply_fixture(file_name: &str) -> MxCommandReply { }) .collect(); + // The fixtures that exercise the return_value fallback path carry a typed + // `returnValue` (VT_I4). Project it so a canned reply can drive the + // helper's payload -> return_value -> MalformedReply precedence. + let return_value = fixture.get("returnValue").and_then(|value| { + value["int32Value"].as_i64().map(|int32| MxValue { + data_type: MxDataType::Integer as i32, + variant_type: value["variantType"].as_str().unwrap_or("VT_I4").to_owned(), + kind: Some(Kind::Int32Value(int32 as i32)), + ..MxValue::default() + }) + }); + MxCommandReply { session_id: fixture["sessionId"].as_str().unwrap_or_default().to_owned(), correlation_id: fixture["correlationId"] @@ -1452,6 +1557,7 @@ fn command_reply_fixture(file_name: &str) -> MxCommandReply { protocol_status: Some(ok_status("command ok")), hresult: fixture["hresult"].as_i64().map(|hresult| hresult as i32), statuses, + return_value, ..MxCommandReply::default() } } diff --git a/docs/ClientBehaviorFixtures.md b/docs/ClientBehaviorFixtures.md index b8d058b..0d0f709 100644 --- a/docs/ClientBehaviorFixtures.md +++ b/docs/ClientBehaviorFixtures.md @@ -70,6 +70,35 @@ The rules those fixtures lock in are: negative. Positive COM success codes such as `S_FALSE` pass, matching COM semantics. +### Malformed-Reply And Credential-Redaction Conformance + +Three further command reply fixtures pin the id/handle-extraction and +credential-redaction contracts for the credential-bearing helpers: + +| Fixture | Reply | Expected behavior | +|---|---|---| +| `authenticate-user.echoed-credential.reply.json` | OK envelope, negative `hresult`, and the caller's credential echoed into `protocolStatus.message`, `statuses[0].diagnosticText`, and `diagnosticMessage` | the surfaced error redacts the exact secret (never leaks the verbatim value) | +| `authenticate-user.missing-payload.reply.json` | OK envelope, no `AuthenticateUser` payload, no `return_value` | a typed malformed-reply error, never a proto3 default `0` and never an NRE | +| `authenticate-user.return-value-only.reply.json` | OK envelope, `return_value.int32_value = 7`, no typed payload | the id resolves to `7` via the legacy `return_value` compatibility path | + +The rules those fixtures lock in are: + +- **Malformed-reply extraction (CLI-41).** Every helper that extracts a scalar + id/handle (`AuthenticateUser`, `ArchestrAUserToId`, `AddBufferedItem`, and the + handle extractors) prefers the typed payload; when it is absent it falls back + to `return_value` **only** when `return_value` is present with the expected + int32 variant; when neither is present it raises a typed malformed-reply error. + It never surfaces a proto3 default `0` and never throws a null-reference. +- **Credential redaction (CLI-40).** The credential-bearing helpers + (`AuthenticateUser`, `WriteSecured`/`WriteSecured2`) scrub the exact secret + values they were called with from any surfaced error text, replacing each + occurrence with the client's redaction marker. This is defense-in-depth on top + of the by-construction guarantee that exceptions carry reply-derived text, not + the request. The marker is `` in the Go, Rust, and Java clients and + `[redacted]` in the Python client and the .NET CLI; the assertion each suite + makes is that the surfaced message no longer contains the credential and does + contain the client's marker. + ## Event Streams Event stream fixtures live in @@ -100,6 +129,12 @@ behavior. A language helper may expose native booleans, integers, strings, arrays, and timestamps, but it must keep `rawDiagnostic`, raw data type fields, and raw byte payloads accessible when conversion is incomplete. +Each status case also carries an independent `wantSuccess` boolean alongside its +`status` object. The success/failure conformance tests assert the helper's +verdict against this fixture-declared expectation rather than recomputing it from +`category` (the same formula under test), so a regression in the verdict rule +cannot hide behind a self-consistent computation. + ## Auth, Timeout, And Cancel Behavior Authentication fixtures live in `clients/proto/fixtures/behavior/auth/`. They diff --git a/docs/ClientLibrariesDesign.md b/docs/ClientLibrariesDesign.md index 9da39e4..0050138 100644 --- a/docs/ClientLibrariesDesign.md +++ b/docs/ClientLibrariesDesign.md @@ -126,11 +126,22 @@ rules across all five clients (see failing before a prior `AuthenticateUser` + `AdviseSupervisory` surfaces the native failure unchanged — the helper does not pre-validate or reorder it. +**Malformed-reply extraction:** the id/handle-returning helpers +(`AuthenticateUser`, `ArchestrAUserToId`, `AddBufferedItem`, and the handle +extractors) follow one contract across all five clients — prefer the typed +payload; fall back to `return_value` only when it is present with the expected +int32 variant; when neither is present, raise a typed malformed-reply error. +They never surface a proto3 default `0` and never throw a null-reference (CLI-41). + **Credential handling:** `AuthenticateUser` credentials and `WriteSecured` secured payloads route through each client's secret-redaction seam so they never reach logs, exception text, or `ToString`/`Debug`/`Display` — the value is carried -only on the wire. Each client's test suite asserts a distinctive credential is -absent from any surfaced error. +only on the wire. In addition to that by-construction guarantee (exceptions carry +reply-derived text, not the request), every client scrubs the **exact** secret +values it was called with from any surfaced error text as defense-in-depth, so a +gateway or MXAccess diagnostic that echoes a credential back cannot leak it +(CLI-40). Each client's test suite asserts a distinctive credential is absent +from any surfaced error and that the redaction marker is present. Shipped in all five clients (.NET / Go / Rust / Python / Java).