From dc7fd16dd59d2cc8b949ac576f9a69fb3e9e79ce Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 06:42:40 -0400 Subject: [PATCH 1/2] fix(CLI-40,CLI-41,CLI-44): exact-secret scrub, uniform malformed-reply contract, Go terminal-error mislabel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLI-40: port the exact-secret credential scrub to Rust/Java/.NET (Go/Python already did it). AuthenticateUser/WriteSecured(2) helpers now redact the exact caller-supplied secret from any surfaced error, as defense-in-depth on top of the by-construction guarantee. Rust hand-writes a redacting Debug (derived Debug would leak the reply); Java/.NET rebuild the same exception type with the redacted message and do not carry the secret-bearing original forward (so ToString/stack traces stay clean too). CLI-41: uniform malformed-reply contract for AuthenticateUser/ArchestrAUserToId/ AddBufferedItem across all five clients — typed payload, else a present int32 return_value, else a typed malformed-reply error. Fixes Go/Java silent-0, .NET NRE, and Rust's own internal inconsistency. CLI-44: the Go event goroutine's Recv-error path now uses a non-blocking sendTerminalEventResult on the reserved slot, so a genuine terminal stream error is reported as itself instead of being mislabeled ErrSlowConsumer under overflow. Riders from the CLI-37/38 review: (a) .NET ToDiagnosticSummary and Python _mxaccess_message surface the raw success member (diagnostics-only parity with Rust); (b) the status-conversion fixture carries an independent wantSuccess boolean and the Go/.NET fixture tests assert against it instead of recomputing the formula under test. Shared fixtures (authenticate-user.{echoed-credential,missing-payload, return-value-only}.reply.json) + manifest + ClientBehaviorFixtures.md + ClientLibrariesDesign.md updated in the same change. Tracking: CLI-40/41/44 -> Done. --- .../2026-07-12/remediation/00-tracking.md | 7 +- .../2026-07-12/remediation/50-clients.md | 6 +- .../MxGatewaySessionReplyContractTests.cs | 127 +++++++++++++ .../MxStatusProxyExtensionsTests.cs | 3 +- .../MxGatewayMalformedReplyException.cs | 46 +++++ .../MxGatewaySecretRedaction.cs | 84 +++++++++ .../MxGatewaySession.cs | 78 +++++++- .../MxStatusProxyExtensions.cs | 2 +- clients/go/mxgateway/client_session_test.go | 89 ++++++++- .../mxgateway/command_reply_fixtures_test.go | 129 +++++++++++++ clients/go/mxgateway/conversion_test.go | 10 +- clients/go/mxgateway/errors.go | 19 ++ clients/go/mxgateway/session.go | 66 +++++-- .../mxgateway/client/MxAccessException.java | 14 ++ .../client/MxGatewayCommandException.java | 17 ++ .../MxGatewayMalformedReplyException.java | 32 ++++ .../ww/mxgateway/client/MxGatewaySecrets.java | 28 +++ .../ww/mxgateway/client/MxGatewaySession.java | 122 ++++++++++--- .../client/MxGatewayCredentialReplyTests.java | 172 ++++++++++++++++++ ...enticate-user.echoed-credential.reply.json | 22 +++ ...thenticate-user.missing-payload.reply.json | 10 + ...enticate-user.return-value-only.reply.json | 15 ++ clients/proto/fixtures/behavior/manifest.json | 21 +++ .../statuses/status-conversion-cases.json | 3 + .../src/zb_mom_ww_mxgateway/__init__.py | 2 + .../python/src/zb_mom_ww_mxgateway/errors.py | 16 +- .../python/src/zb_mom_ww_mxgateway/session.py | 32 +++- clients/python/tests/test_malformed_reply.py | 96 ++++++++++ clients/rust/src/error.rs | 80 +++++++- clients/rust/src/session.rs | 58 +++++- clients/rust/tests/client_behavior.rs | 106 +++++++++++ docs/ClientBehaviorFixtures.md | 35 ++++ docs/ClientLibrariesDesign.md | 15 +- 33 files changed, 1465 insertions(+), 97 deletions(-) create mode 100644 clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySessionReplyContractTests.cs create mode 100644 clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewayMalformedReplyException.cs create mode 100644 clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySecretRedaction.cs create mode 100644 clients/go/mxgateway/command_reply_fixtures_test.go create mode 100644 clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayMalformedReplyException.java create mode 100644 clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewayCredentialReplyTests.java create mode 100644 clients/proto/fixtures/behavior/command-replies/authenticate-user.echoed-credential.reply.json create mode 100644 clients/proto/fixtures/behavior/command-replies/authenticate-user.missing-payload.reply.json create mode 100644 clients/proto/fixtures/behavior/command-replies/authenticate-user.return-value-only.reply.json create mode 100644 clients/python/tests/test_malformed_reply.py 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). From 0d874f91ee38673ae89e0f4c29b37c6db8eb5d95 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 07:04:56 -0400 Subject: [PATCH 2/2] fix(CLI-40): scrub the credential from the redacted error's structured reply, route MXACCESS_FAILURE to MxAccess (Rust), fix Go Subscribe terminal-error drop Code-review follow-up on the CLI-40/41/44 branch. ISSUE 1 (all five, critical): the message-only scrub still leaked the server-echoed credential through the redacted error's structured reply accessor (.NET Reply/Statuses, Java reply()/protocolStatus(), Go MxAccessError.Reply via errors.As, Rust reply()/into_reply(), Python raw_reply). The redacted error now carries a scrubbed clone of the reply (protocol_status.message, diagnostic_message, statuses[].diagnostic_text), with per-language tests asserting the reply accessor no longer contains the credential. ISSUE 2 (Rust, critical): ensure_command_success routed MXACCESS_FAILURE to Error::Command (unlike the other four clients), bypassing attach_secrets and leaking via derived Debug/Display. MXACCESS_FAILURE now routes to Error::MxAccess, fixing the cross-client inconsistency. ISSUE 3 (Go, important): the CLI-44 terminal send was unconditionally non-blocking, dropping a genuine terminal error under a full buffer on the never-drop SubscribeEvents path. It is now reserved-slot-non-blocking only for the cancel-on-overflow path and blocking for the never-drop path. New shared fixture authenticate-user.echoed-credential-mxaccess-failure.reply.json wired into all five suites. Minors: whitespace-secret guard on .NET/Java redact helpers; Java preserves exception subtype on redaction; redaction-helper unit tests (Go/Java/.NET). Docs (ClientBehaviorFixtures.md, ClientLibrariesDesign.md) updated to make the structured-field claim true. --- .../2026-07-12/remediation/00-tracking.md | 2 +- .../MxGatewaySecretRedactionTests.cs | 96 ++++++++++ .../MxGatewaySessionReplyContractTests.cs | 38 ++++ .../MxGatewaySecretRedaction.cs | 171 ++++++++++++++++-- clients/go/mxgateway/client_session_test.go | 68 +++++++ .../mxgateway/command_reply_fixtures_test.go | 68 +++++++ clients/go/mxgateway/errors.go | 101 ++++++++++- clients/go/mxgateway/errors_redaction_test.go | 115 ++++++++++++ clients/go/mxgateway/session.go | 38 ++-- .../ww/mxgateway/client/MxGatewaySecrets.java | 12 +- .../ww/mxgateway/client/MxGatewaySession.java | 136 ++++++++++++-- .../client/MxGatewaySessionException.java | 14 ++ .../client/MxGatewayWorkerException.java | 14 ++ .../client/MxGatewayCredentialReplyTests.java | 33 +++- .../client/MxGatewaySecretsTests.java | 50 +++++ ...oed-credential-mxaccess-failure.reply.json | 22 +++ clients/proto/fixtures/behavior/manifest.json | 9 +- .../python/src/zb_mom_ww_mxgateway/session.py | 36 +++- clients/python/tests/test_malformed_reply.py | 22 ++- .../tests/test_typed_command_helpers.py | 15 +- clients/rust/src/error.rs | 31 +++- clients/rust/src/session.rs | 40 +++- clients/rust/tests/client_behavior.rs | 114 +++++++++++- docs/ClientBehaviorFixtures.md | 21 ++- docs/ClientLibrariesDesign.md | 12 +- 25 files changed, 1190 insertions(+), 88 deletions(-) create mode 100644 clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySecretRedactionTests.cs create mode 100644 clients/go/mxgateway/errors_redaction_test.go create mode 100644 clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewaySecretsTests.java create mode 100644 clients/proto/fixtures/behavior/command-replies/authenticate-user.echoed-credential-mxaccess-failure.reply.json diff --git a/archreview/2026-07-12/remediation/00-tracking.md b/archreview/2026-07-12/remediation/00-tracking.md index 10631bd..203aa6e 100644 --- a/archreview/2026-07-12/remediation/00-tracking.md +++ b/archreview/2026-07-12/remediation/00-tracking.md @@ -169,5 +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 | **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). **Code review of the branch found three defects, all fixed before merge:** (1, all five, critical) the message-only scrub left the server-echoed credential exposed on the redacted error's **structured reply accessor** (`.NET MxAccessException.Reply`/`Statuses`, Java `reply()`/`protocolStatus()`, Go `MxAccessError.Reply` via `errors.As`, Rust `reply()`/`into_reply()`, Python `raw_reply`) — the redacted error now carries a **scrubbed clone** of the reply (`protocol_status.message`, `diagnostic_message`, `statuses[].diagnostic_text` all redacted), with per-language tests asserting the reply accessor is clean; docs/ClientLibrariesDesign.md updated to make the "never reaches exception text" claim true for structured fields too. (2, Rust, critical) `ensure_command_success` routed `PROTOCOL_STATUS_CODE_MXACCESS_FAILURE` to `Error::Command` (unlike the other four clients), where `attach_secrets` did not patch it and its derived `Debug`/`Display` leaked the secret — now `MxaccessFailure` routes to `Error::MxAccess` (fixing a real cross-client inconsistency; an existing test flipped from `Error::Command` to `Error::MxAccess`). (3, Go, important) the CLI-44 `sendTerminalEventResult` was unconditionally non-blocking, so on the never-drop `SubscribeEvents`/`SubscribeEventsAfter` path (`cancelWhenResultBufferFull=false`) a genuine terminal error under a full buffer hit the `default:` and was silently dropped — now the terminal send is reserved-slot-non-blocking only for the cancel-on-overflow path and **blocking** for the never-drop path. A new shared fixture `authenticate-user.echoed-credential-mxaccess-failure.reply.json` (the echo under `MXACCESS_FAILURE`) is wired into all five suites. Minors also landed: whitespace-only-secret guard on the .NET/Java redact helpers; Java `invokeCommandRedacted` now preserves the exception subtype (mirroring .NET's type switch) instead of collapsing to the base type; dedicated redaction-helper unit tests (multiple occurrences, substring-overlap, empty/blank secrets) in Go/Java/.NET. Re-verified all five green (Go `go test ./...` ok + gofmt clean; Python 163 passed/1 skipped; .NET build 0 warnings + 128 passed/1 skipped; Rust fmt/check/`test --workspace`/clippy `-D warnings` all clean; Java gradle BUILD SUCCESSFUL, no generated churn). | | 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/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySecretRedactionTests.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySecretRedactionTests.cs new file mode 100644 index 0000000..ba04c89 --- /dev/null +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySecretRedactionTests.cs @@ -0,0 +1,96 @@ +using ZB.MOM.WW.MxGateway.Contracts.Proto; + +namespace ZB.MOM.WW.MxGateway.Client.Tests; + +///

+/// Unit tests for — the exact-substring scrub applied to +/// diagnostic text and rebuilt exceptions before they leave the client on a failure path. +/// +public sealed class MxGatewaySecretRedactionTests +{ + [Fact] + public void Redact_ReplacesEveryOccurrenceOfSecret() + { + string result = MxGatewaySecretRedaction.Redact( + "pw=hunter2 retry pw=hunter2 again hunter2", + "hunter2"); + + Assert.DoesNotContain("hunter2", result, StringComparison.Ordinal); + Assert.Equal("pw= retry pw= again ", result); + } + + [Fact] + public void Redact_ScrubsBothSecretsWhenOneIsSubstringOfTheOther() + { + // "secret" is a substring of "secretPassword"; both must be fully scrubbed regardless of + // supplied order — no residual leak of either verbatim value. + string result = MxGatewaySecretRedaction.Redact( + "a=secretPassword b=secret", + "secret", + "secretPassword"); + + Assert.DoesNotContain("secretPassword", result, StringComparison.Ordinal); + Assert.DoesNotContain("secret", result, StringComparison.Ordinal); + } + + [Fact] + public void Redact_WithNullSecretsArray_ReturnsMessageUnchanged() + { + const string message = "nothing to scrub here"; + + string result = MxGatewaySecretRedaction.Redact(message, null!); + + Assert.Equal(message, result); + } + + [Fact] + public void Redact_WithEmptySecretsArray_ReturnsMessageUnchanged() + { + const string message = "nothing to scrub here"; + + string result = MxGatewaySecretRedaction.Redact(message); + + Assert.Equal(message, result); + } + + [Fact] + public void Redact_IgnoresWhitespaceOnlySecret() + { + // A whitespace-only secret must not over-redact the internal spaces of the message. + const string message = "user operator logged in"; + + string result = MxGatewaySecretRedaction.Redact(message, " "); + + Assert.Equal(message, result); + } + + [Fact] + public void Redacted_PreservesConcreteSubtypeAndDoesNotChainSecretBearingOriginal() + { + const string secret = "hunter2"; + Exception transportCause = new InvalidOperationException("transport reset"); + MxGatewaySessionException original = new( + $"session rejected credential '{secret}'", + "session-1", + "correlation-1", + new ProtocolStatus { Code = ProtocolStatusCode.SessionNotReady, Message = $"echoed '{secret}'" }, + hResult: -1, + statuses: [new MxStatusProxy { DiagnosticText = $"denied '{secret}'" }], + innerException: transportCause); + + MxGatewayException redacted = MxGatewaySecretRedaction.Redacted(original, secret); + + // Concrete runtime type is preserved. + Assert.IsType(redacted); + // The secret is gone from the message and every structured accessor. + Assert.DoesNotContain(secret, redacted.Message, StringComparison.Ordinal); + Assert.DoesNotContain(secret, redacted.ToString(), StringComparison.Ordinal); + Assert.DoesNotContain(secret, redacted.ProtocolStatus!.Message, StringComparison.Ordinal); + Assert.All(redacted.Statuses, status => + Assert.DoesNotContain(secret, status.DiagnosticText, StringComparison.Ordinal)); + Assert.Contains("", redacted.Message, StringComparison.Ordinal); + // The secret-bearing original is NOT chained; the original's transport cause is carried. + Assert.NotSame(original, redacted.InnerException); + Assert.Same(transportCause, redacted.InnerException); + } +} diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySessionReplyContractTests.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySessionReplyContractTests.cs index 8b9a77e..f9c71d0 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySessionReplyContractTests.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client.Tests/MxGatewaySessionReplyContractTests.cs @@ -32,6 +32,44 @@ public sealed class MxGatewaySessionReplyContractTests Assert.DoesNotContain(password, exception.ToString(), StringComparison.Ordinal); } + /// + /// CLI-40: the redacted exception must not leak the echoed credential through any structured + /// accessor either — (protocol message, diagnostic + /// message, and each MXSTATUS_PROXY diagnostic text) and + /// all carry the server-echoed credential verbatim before the fix. Both the OK+negative-HRESULT + /// and the MXACCESS_FAILURE reply route to , so both must scrub. + /// + /// The echoed-credential reply fixture to drive. + [Theory] + [InlineData("authenticate-user.echoed-credential.reply.json")] + [InlineData("authenticate-user.echoed-credential-mxaccess-failure.reply.json")] + public async Task AuthenticateUserAsync_RedactsEchoedCredentialInStructuredAccessors(string fixture) + { + const string password = "sup3rSecretVerify9f3a2b"; + FakeGatewayTransport transport = CreateTransport(); + transport.AddInvokeReply(ReadReplyFixture(fixture)); + 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); + Assert.DoesNotContain(password, exception.ToString(), StringComparison.Ordinal); + Assert.DoesNotContain(password, exception.Reply.ProtocolStatus.Message, StringComparison.Ordinal); + Assert.DoesNotContain(password, exception.Reply.DiagnosticMessage, StringComparison.Ordinal); + foreach (MxStatusProxy status in exception.Reply.Statuses) + { + Assert.DoesNotContain(password, status.DiagnosticText, StringComparison.Ordinal); + } + + foreach (MxStatusProxy status in exception.Statuses) + { + Assert.DoesNotContain(password, status.DiagnosticText, 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. diff --git a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySecretRedaction.cs b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySecretRedaction.cs index 7cbad16..dddd7eb 100644 --- a/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySecretRedaction.cs +++ b/clients/dotnet/ZB.MOM.WW.MxGateway.Client/MxGatewaySecretRedaction.cs @@ -1,3 +1,5 @@ +using ZB.MOM.WW.MxGateway.Contracts.Proto; + namespace ZB.MOM.WW.MxGateway.Client; /// @@ -13,9 +15,10 @@ 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. + /// Replaces every usable 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. A secret that is null, empty, or whitespace-only is ignored so + /// it cannot over-redact ordinary separator characters in the message. /// /// The diagnostic message to scrub. /// The secret values to remove from the message. @@ -30,7 +33,7 @@ internal static class MxGatewaySecretRedaction string result = message; foreach (string? secret in secrets) { - if (!string.IsNullOrEmpty(secret)) + if (!string.IsNullOrWhiteSpace(secret)) { result = result.Replace(secret, Marker, StringComparison.Ordinal); } @@ -39,6 +42,81 @@ internal static class MxGatewaySecretRedaction return result; } + /// + /// Returns a scrubbed clone of : the protocol-status message, the + /// reply-level diagnostic message, and each MXSTATUS_PROXY diagnostic text have every verbatim + /// secret replaced with the redaction marker. The original is left untouched. MXAccess can echo + /// a submitted credential into any of these fields, so a redacted exception must carry the + /// scrubbed reply rather than the secret-bearing original. + /// + /// The reply to clone and scrub. + /// The secret values to remove. + /// A scrubbed clone of the reply. + internal static MxCommandReply RedactReply(MxCommandReply reply, params string?[] secrets) + { + ArgumentNullException.ThrowIfNull(reply); + + MxCommandReply clone = reply.Clone(); + if (clone.ProtocolStatus is not null) + { + clone.ProtocolStatus.Message = Redact(clone.ProtocolStatus.Message, secrets); + } + + clone.DiagnosticMessage = Redact(clone.DiagnosticMessage, secrets); + foreach (MxStatusProxy status in clone.Statuses) + { + status.DiagnosticText = Redact(status.DiagnosticText, secrets); + } + + return clone; + } + + /// + /// Returns a scrubbed clone of (its message with every verbatim + /// secret removed), or when the input is null. + /// + /// The protocol status to clone and scrub. + /// The secret values to remove. + /// A scrubbed clone, or . + internal static ProtocolStatus? RedactStatus(ProtocolStatus? status, params string?[] secrets) + { + if (status is null) + { + return null; + } + + ProtocolStatus clone = status.Clone(); + clone.Message = Redact(clone.Message, secrets); + return clone; + } + + /// + /// Returns a list of scrubbed clones of — each MXSTATUS_PROXY's + /// diagnostic text has every verbatim secret removed. The originals are left untouched. + /// + /// The statuses to clone and scrub. + /// The secret values to remove. + /// A list of scrubbed clones. + internal static IReadOnlyList RedactStatuses( + IReadOnlyList statuses, + params string?[] secrets) + { + if (statuses is null || statuses.Count is 0) + { + return statuses ?? []; + } + + MxStatusProxy[] result = new MxStatusProxy[statuses.Count]; + for (int i = 0; i < statuses.Count; i++) + { + MxStatusProxy clone = statuses[i].Clone(); + clone.DiagnosticText = Redact(clone.DiagnosticText, secrets); + result[i] = clone; + } + + return result; + } + /// /// Returns an exception equivalent to but with any verbatim secret /// scrubbed from its message. When nothing changes, the original exception is returned @@ -57,28 +135,95 @@ internal static class MxGatewaySecretRedaction ArgumentNullException.ThrowIfNull(ex); string redacted = Redact(ex.Message, secrets); - if (string.Equals(redacted, ex.Message, StringComparison.Ordinal)) + bool messageChanged = !string.Equals(redacted, ex.Message, StringComparison.Ordinal); + Exception? cause = ex.InnerException; + + // MxAccessException derives its structured fields from the raw reply, so scrubbing must + // clone and redact that reply — the message alone changing is not enough, because the reply + // can carry the echoed secret even when the message does not. + if (ex is MxAccessException access) + { + if (!messageChanged && !ReplyContainsSecret(access.Reply, secrets)) + { + return ex; + } + + return new MxAccessException(redacted, RedactReply(access.Reply, secrets), cause); + } + + // Other subtypes carry the secret through ProtocolStatus.Message and Statuses[].DiagnosticText. + if (!messageChanged + && !ContainsSecret(ex.ProtocolStatus?.Message, secrets) + && !StatusesContainSecret(ex.Statuses, secrets)) { return ex; } - Exception? cause = ex.InnerException; + ProtocolStatus? status = RedactStatus(ex.ProtocolStatus, secrets); + IReadOnlyList statuses = RedactStatuses(ex.Statuses, secrets); 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), + redacted, ex.SessionId, ex.CorrelationId, status, ex.HResultCode, statuses, cause), MxGatewayWorkerException => new MxGatewayWorkerException( - redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + redacted, ex.SessionId, ex.CorrelationId, status, ex.HResultCode, statuses, cause), MxGatewayAuthenticationException => new MxGatewayAuthenticationException( - redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + redacted, ex.SessionId, ex.CorrelationId, status, ex.HResultCode, statuses, cause), MxGatewayAuthorizationException => new MxGatewayAuthorizationException( - redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + redacted, ex.SessionId, ex.CorrelationId, status, ex.HResultCode, statuses, cause), MxGatewayMalformedReplyException => new MxGatewayMalformedReplyException( - redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + redacted, ex.SessionId, ex.CorrelationId, status, ex.HResultCode, statuses, cause), MxGatewayCommandException => new MxGatewayCommandException( - redacted, ex.SessionId, ex.CorrelationId, ex.ProtocolStatus, ex.HResultCode, ex.Statuses, cause), + redacted, ex.SessionId, ex.CorrelationId, status, ex.HResultCode, statuses, cause), _ => new MxGatewayException(redacted, cause), }; } + + private static bool ContainsSecret(string? text, string?[] secrets) + { + if (string.IsNullOrEmpty(text) || secrets is null) + { + return false; + } + + foreach (string? secret in secrets) + { + if (!string.IsNullOrWhiteSpace(secret) && text.Contains(secret, StringComparison.Ordinal)) + { + return true; + } + } + + return false; + } + + private static bool StatusesContainSecret(IReadOnlyList statuses, string?[] secrets) + { + if (statuses is null) + { + return false; + } + + foreach (MxStatusProxy status in statuses) + { + if (ContainsSecret(status.DiagnosticText, secrets)) + { + return true; + } + } + + return false; + } + + private static bool ReplyContainsSecret(MxCommandReply reply, string?[] secrets) + { + if (reply is null) + { + return false; + } + + return ContainsSecret(reply.ProtocolStatus?.Message, secrets) + || ContainsSecret(reply.DiagnosticMessage, secrets) + || StatusesContainSecret(reply.Statuses, secrets); + } } diff --git a/clients/go/mxgateway/client_session_test.go b/clients/go/mxgateway/client_session_test.go index ad6522a..7088294 100644 --- a/clients/go/mxgateway/client_session_test.go +++ b/clients/go/mxgateway/client_session_test.go @@ -264,6 +264,74 @@ func TestEventsFullBufferTerminalErrorKeepsRootCause(t *testing.T) { } } +// TestSubscribeEventsFullBufferDeliversTerminalError is the CLI-44 regression for +// the never-drop Subscribe path. SubscribeEvents/SubscribeEventsAfter use +// cancelWhenResultBufferFull=false, so ordinary sends are blocking and uncapped and +// can fill every slot in the results channel — including the reserved terminal slot. +// A genuine terminal Recv error must still be delivered as the final result, never +// silently dropped. The server sends eventBufferSize+eventBufferReservedSlots events +// (filling every slot) and then returns a genuine gRPC error; with an unconditional +// non-blocking terminal send the error is dropped, so this fails red until the send +// path blocks for the never-drop mode. +func TestSubscribeEventsFullBufferDeliversTerminalError(t *testing.T) { + fake := &fakeGatewayServer{ + streamStarted: make(chan struct{}), + streamDone: make(chan struct{}), + streamEventCount: eventBufferSize + eventBufferReservedSlots, + streamTerminalErr: status.Error(codes.Internal, "boom"), + } + client, cleanup := newBufconnClient(t, fake) + defer cleanup() + session := NewSessionForID(client, "session-1") + + subscription, err := session.SubscribeEvents(context.Background()) + if err != nil { + t.Fatalf("SubscribeEvents() error = %v", err) + } + defer subscription.Close() + <-fake.streamStarted + + // Wait for the server to finish sending every event and return the terminal + // error, so the producer goroutine has filled every buffered slot before the + // terminal result is processed. That is what makes the dropped-terminal bug + // observable: with the buffer full, an unconditional non-blocking send discards + // the terminal error. + select { + case <-fake.streamDone: + case <-time.After(2 * time.Second): + t.Fatal("event stream did not stop after terminal error") + } + time.Sleep(250 * time.Millisecond) + + // Drain fully. Every data event, then the terminal gRPC error as the final + // result, must arrive; the channel must not close without yielding it. + events := subscription.Events() + 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 (%v), want the terminal *GatewayError; it was dropped", last.Err, last.Err) + } + if code := status.Code(last.Err); code != codes.Internal { + t.Fatalf("final event result gRPC code = %s, want %s", code, codes.Internal) + } + 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{}), diff --git a/clients/go/mxgateway/command_reply_fixtures_test.go b/clients/go/mxgateway/command_reply_fixtures_test.go index 2561f83..02567b1 100644 --- a/clients/go/mxgateway/command_reply_fixtures_test.go +++ b/clients/go/mxgateway/command_reply_fixtures_test.go @@ -127,3 +127,71 @@ func TestAuthenticateUserScrubsEchoedCredentialFromError(t *testing.T) { t.Fatalf("surfaced error missing redaction marker: %q", message) } } + +// TestAuthenticateUserScrubsEchoedCredentialFromStructuredReply is the CLI-40 +// follow-up: redacting only the rendered Error() string is not enough. The typed +// *MxAccessError still carries the raw command reply, whose ProtocolStatus.Message, +// DiagnosticMessage, and Statuses[].DiagnosticText echo the credential verbatim. A +// logger dumping structured fields would reintroduce the leak, so the reply the +// typed error carries must be a scrubbed clone. Both the OK+negative-HRESULT and the +// MXACCESS_FAILURE fixtures route to *MxAccessError (via EnsureProtocolSuccess), so +// both must be scrubbed identically. +func TestAuthenticateUserScrubsEchoedCredentialFromStructuredReply(t *testing.T) { + const credential = "sup3rSecretVerify9f3a2b" + fixtures := []string{ + "authenticate-user.echoed-credential.reply.json", + "authenticate-user.echoed-credential-mxaccess-failure.reply.json", + } + for _, fixture := range fixtures { + t.Run(fixture, func(t *testing.T) { + fake := &fakeGatewayServer{ + invokeReply: loadCommandReplyFixture(t, fixture), + } + 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") + } + + var mxErr *MxAccessError + if !errors.As(err, &mxErr) { + t.Fatalf("AuthenticateUser() error = %v (%T), want *MxAccessError", err, err) + } + + reply := mxErr.Reply + if reply == nil { + t.Fatal("MxAccessError.Reply is nil, want the scrubbed command reply") + } + if got := reply.GetProtocolStatus().GetMessage(); strings.Contains(got, credential) { + t.Fatalf("MxAccessError.Reply.ProtocolStatus.Message leaked the credential: %q", got) + } + if got := reply.GetDiagnosticMessage(); strings.Contains(got, credential) { + t.Fatalf("MxAccessError.Reply.DiagnosticMessage leaked the credential: %q", got) + } + for i, status := range reply.GetStatuses() { + if got := status.GetDiagnosticText(); strings.Contains(got, credential) { + t.Fatalf("MxAccessError.Reply.Statuses[%d].DiagnosticText leaked the credential: %q", i, got) + } + } + + // The wrapped CommandError's status/reply must be scrubbed too. + if mxErr.Command != nil { + if got := mxErr.Command.Status.GetMessage(); strings.Contains(got, credential) { + t.Fatalf("MxAccessError.Command.Status.Message leaked the credential: %q", got) + } + if cmdReply := mxErr.Command.Reply; cmdReply != nil { + if got := cmdReply.GetDiagnosticMessage(); strings.Contains(got, credential) { + t.Fatalf("MxAccessError.Command.Reply.DiagnosticMessage leaked the credential: %q", got) + } + } + } + + if got := err.Error(); strings.Contains(got, credential) { + t.Fatalf("rendered error leaked the credential: %q", got) + } + }) + } +} diff --git a/clients/go/mxgateway/errors.go b/clients/go/mxgateway/errors.go index 54c918a..318cd5d 100644 --- a/clients/go/mxgateway/errors.go +++ b/clients/go/mxgateway/errors.go @@ -6,6 +6,7 @@ import ( "strings" pb "gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/internal/generated" + "google.golang.org/protobuf/proto" ) // redactedSecretMarker is the placeholder substituted for credential material in @@ -49,20 +50,108 @@ func (e *secretRedactingError) Unwrap() error { return e.err } -// redactSecrets wraps err so any occurrence of a non-empty secret in the surfaced -// message is redacted, while errors.As / errors.Is still reach the wrapped typed -// error. It returns nil unchanged and skips wrapping when no non-empty secret is -// supplied, so non-secret-bearing calls keep their original error verbatim. +// scrubReplyStrings returns a clone of reply with every non-empty secret replaced +// by redactedSecretMarker in the free-text fields a gateway diagnostic could echo a +// credential into: ProtocolStatus.Message, DiagnosticMessage, and each +// Statuses[].DiagnosticText. It clones with proto.Clone so the caller's original +// reply is never mutated. A nil reply, or an empty/whitespace-only secret set, is a +// no-op (nil in, nil out; a clone otherwise). +func scrubReplyStrings(reply *pb.MxCommandReply, secrets []string) *pb.MxCommandReply { + if reply == nil { + return nil + } + clone, ok := proto.Clone(reply).(*pb.MxCommandReply) + if !ok { + return reply + } + for _, secret := range secrets { + if secret == "" { + continue + } + if clone.GetProtocolStatus() != nil { + clone.ProtocolStatus.Message = strings.ReplaceAll(clone.GetProtocolStatus().GetMessage(), secret, redactedSecretMarker) + } + clone.DiagnosticMessage = strings.ReplaceAll(clone.GetDiagnosticMessage(), secret, redactedSecretMarker) + for _, status := range clone.GetStatuses() { + status.DiagnosticText = strings.ReplaceAll(status.GetDiagnosticText(), secret, redactedSecretMarker) + } + } + return clone +} + +// scrubProtocolStatusMessage returns a clone of status with every non-empty secret +// redacted from its Message, leaving the original untouched. +func scrubProtocolStatusMessage(status *ProtocolStatus, secrets []string) *ProtocolStatus { + if status == nil { + return nil + } + clone, ok := proto.Clone(status).(*ProtocolStatus) + if !ok { + return status + } + for _, secret := range secrets { + if secret != "" { + clone.Message = strings.ReplaceAll(clone.GetMessage(), secret, redactedSecretMarker) + } + } + return clone +} + +// redactSecrets scrubs a non-empty secret set from the error it surfaces. When the +// wrapped error is a typed *MxAccessError or *CommandError it is rebuilt carrying +// scrubbed clones of its reply and protocol status, so a caller logging the typed +// error's structured fields cannot reintroduce the credential the rendered message +// hides. The rebuilt (or original, for other error types) value is then wrapped in +// secretRedactingError as a belt-and-suspenders scrub of any remaining rendered +// text. errors.As / errors.Is still reach the typed error through the wrapper. It +// returns nil unchanged and skips all work when no non-empty secret is supplied, so +// non-secret-bearing calls keep their original error verbatim. func redactSecrets(err error, secrets ...string) error { if err == nil { return nil } + hasSecret := false for _, secret := range secrets { if secret != "" { - return &secretRedactingError{err: err, secrets: secrets} + hasSecret = true + break } } - return err + if !hasSecret { + return err + } + + rebuilt := rebuildScrubbedError(err, secrets) + return &secretRedactingError{err: rebuilt, secrets: secrets} +} + +// rebuildScrubbedError rebuilds the typed error carrying scrubbed clones of any +// command reply / protocol status it holds, so credential text never survives in the +// error's structured fields. Non-reply-bearing error types are returned unchanged. +func rebuildScrubbedError(err error, secrets []string) error { + switch typed := err.(type) { + case *MxAccessError: + return &MxAccessError{ + Command: scrubCommandError(typed.Command, secrets), + Reply: scrubReplyStrings(typed.Reply, secrets), + } + case *CommandError: + return scrubCommandError(typed, secrets) + default: + return err + } +} + +// scrubCommandError rebuilds a CommandError with a scrubbed Status and Reply. +func scrubCommandError(cmd *CommandError, secrets []string) *CommandError { + if cmd == nil { + return nil + } + return &CommandError{ + Op: cmd.Op, + Status: scrubProtocolStatusMessage(cmd.Status, secrets), + Reply: scrubReplyStrings(cmd.Reply, secrets), + } } // ErrSlowConsumer is the terminal error sent on the Events/EventsAfter diff --git a/clients/go/mxgateway/errors_redaction_test.go b/clients/go/mxgateway/errors_redaction_test.go new file mode 100644 index 0000000..0b04433 --- /dev/null +++ b/clients/go/mxgateway/errors_redaction_test.go @@ -0,0 +1,115 @@ +package mxgateway + +import ( + "errors" + "strings" + "testing" + + pb "gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/internal/generated" +) + +// TestScrubReplyStringsRedactsEveryOccurrence covers the multi-occurrence case: +// one secret appearing across ProtocolStatus.Message, DiagnosticMessage, and every +// Statuses[].DiagnosticText must be fully redacted with no residue. +func TestScrubReplyStringsRedactsEveryOccurrence(t *testing.T) { + const secret = "hunter2" + reply := &pb.MxCommandReply{ + ProtocolStatus: &pb.ProtocolStatus{Message: "rejected hunter2 then hunter2 again"}, + DiagnosticMessage: "echoed hunter2 back", + Statuses: []*pb.MxStatusProxy{ + {DiagnosticText: "first hunter2"}, + {DiagnosticText: "second hunter2 and hunter2"}, + }, + } + + scrubbed := scrubReplyStrings(reply, []string{secret}) + + if strings.Contains(scrubbed.GetProtocolStatus().GetMessage(), secret) { + t.Fatalf("ProtocolStatus.Message still contains the secret: %q", scrubbed.GetProtocolStatus().GetMessage()) + } + if strings.Contains(scrubbed.GetDiagnosticMessage(), secret) { + t.Fatalf("DiagnosticMessage still contains the secret: %q", scrubbed.GetDiagnosticMessage()) + } + for i, status := range scrubbed.GetStatuses() { + if strings.Contains(status.GetDiagnosticText(), secret) { + t.Fatalf("Statuses[%d].DiagnosticText still contains the secret: %q", i, status.GetDiagnosticText()) + } + } + if !strings.Contains(scrubbed.GetProtocolStatus().GetMessage(), redactedSecretMarker) { + t.Fatalf("ProtocolStatus.Message missing redaction marker: %q", scrubbed.GetProtocolStatus().GetMessage()) + } + + // The original reply must be untouched (scrubReplyStrings clones). + if !strings.Contains(reply.GetDiagnosticMessage(), secret) { + t.Fatal("scrubReplyStrings mutated the original reply instead of cloning it") + } +} + +// TestScrubReplyStringsRedactsOverlappingSecrets covers two secrets where one is a +// substring of the other: both must be fully redacted, with no partial leak of the +// longer secret's non-shared remainder. +func TestScrubReplyStringsRedactsOverlappingSecrets(t *testing.T) { + const shortSecret = "pass" + const longSecret = "password123" + reply := &pb.MxCommandReply{ + DiagnosticMessage: "value was password123 and also pass", + } + + scrubbed := scrubReplyStrings(reply, []string{longSecret, shortSecret}) + + got := scrubbed.GetDiagnosticMessage() + if strings.Contains(got, shortSecret) { + t.Fatalf("scrubbed message still contains a secret substring %q: %q", shortSecret, got) + } + if strings.Contains(got, longSecret) { + t.Fatalf("scrubbed message still contains %q: %q", longSecret, got) + } + // "123" is the longer secret's remainder past the shared "pass" prefix; it must + // not survive as a partial leak. + if strings.Contains(got, "123") { + t.Fatalf("scrubbed message leaked the longer secret's remainder: %q", got) + } +} + +// TestRedactSecretsEmptyOrNilLeavesErrorUnchanged confirms the no-secret paths keep +// the original typed error verbatim (no wrapping, no scrubbed clone). +func TestRedactSecretsEmptyOrNilLeavesErrorUnchanged(t *testing.T) { + base := &MxAccessError{Reply: &pb.MxCommandReply{DiagnosticMessage: "boom"}} + + if got := redactSecrets(base); got != error(base) { + t.Fatalf("redactSecrets with no secrets = %v, want the original error unchanged", got) + } + if got := redactSecrets(base, ""); got != error(base) { + t.Fatalf("redactSecrets with only an empty secret = %v, want the original error unchanged", got) + } + if got := redactSecrets(nil, "secret"); got != nil { + t.Fatalf("redactSecrets(nil, ...) = %v, want nil", got) + } +} + +// TestRedactSecretsRebuildsTypedCommandError confirms a *CommandError (non-MXAccess +// path) is rebuilt with a scrubbed Status and Reply, and errors.As still reaches it. +func TestRedactSecretsRebuildsTypedCommandError(t *testing.T) { + const secret = "topSecretValue" + base := &CommandError{ + Op: "write secured", + Status: &pb.ProtocolStatus{Message: "rejected topSecretValue"}, + Reply: &pb.MxCommandReply{DiagnosticMessage: "echoed topSecretValue"}, + } + + redacted := redactSecrets(base, secret) + + var cmdErr *CommandError + if !errors.As(redacted, &cmdErr) { + t.Fatalf("redactSecrets result %T does not unwrap to *CommandError", redacted) + } + if strings.Contains(cmdErr.Status.GetMessage(), secret) { + t.Fatalf("CommandError.Status.Message leaked the secret: %q", cmdErr.Status.GetMessage()) + } + if strings.Contains(cmdErr.Reply.GetDiagnosticMessage(), secret) { + t.Fatalf("CommandError.Reply.DiagnosticMessage leaked the secret: %q", cmdErr.Reply.GetDiagnosticMessage()) + } + if strings.Contains(redacted.Error(), secret) { + t.Fatalf("rendered error leaked the secret: %q", redacted.Error()) + } +} diff --git a/clients/go/mxgateway/session.go b/clients/go/mxgateway/session.go index 74d0fce..d20caaf 100644 --- a/clients/go/mxgateway/session.go +++ b/clients/go/mxgateway/session.go @@ -1073,8 +1073,8 @@ func (s *Session) subscribeEventsAfter(ctx context.Context, afterWorkerSequence // 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}}) + // the real gRPC status, so send it directly, bypassing that branch. + sendTerminalEventResult(streamCtx, results, EventResult{Err: &GatewayError{Op: "stream events", Err: err}}, cancelWhenResultBufferFull) return } }() @@ -1093,20 +1093,32 @@ 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. +// sendTerminalEventResult enqueues a terminal EventResult, bypassing +// sendEventResult's overflow branch so a genuine stream error is reported verbatim +// rather than relabeled as ErrSlowConsumer. How it sends depends on the mode: // -// 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) { +// - cancelWhenBufferFull=true (Events/EventsAfter): ordinary data sends are capped +// at eventBufferSize, leaving eventBufferReservedSlots free, so a non-blocking +// send always lands the terminal result. 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. +// - cancelWhenBufferFull=false (SubscribeEvents/SubscribeEventsAfter, never-drop): +// ordinary data sends are uncapped and blocking, so every slot including the +// reserve can hold data. A non-blocking send would then hit the full buffer and +// silently drop the terminal error, breaking the never-drop contract; instead +// block until the consumer drains a slot (or the stream context is cancelled). +func sendTerminalEventResult(ctx context.Context, results chan<- EventResult, result EventResult, cancelWhenBufferFull bool) { + if cancelWhenBufferFull { + select { + case results <- result: + default: + } + return + } select { case results <- result: - default: + case <-ctx.Done(): } } 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 49b2a2e..a1d221e 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 @@ -63,9 +63,9 @@ public final class MxGatewaySecrets { * 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 + * @param secrets the exact secret substrings to remove; {@code null}, empty, + * and blank (whitespace-only) entries and a {@code null} array are ignored + * @return {@code message} unchanged when it is {@code null} or no non-blank * secret is supplied, otherwise the message with every secret occurrence * replaced by {@code ""} */ @@ -76,9 +76,11 @@ public final class MxGatewaySecrets { String result = message; for (String secret : secrets) { - if (secret != null && !secret.isEmpty()) { - result = result.replace(secret, ""); + if (secret == null || secret.isBlank()) { + // A blank "secret" would over-redact real whitespace; skip it. + continue; } + 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 bb21891..485ba5b 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 @@ -31,6 +31,7 @@ import mxaccess_gateway.v1.MxaccessGateway.MxSparseElement; import mxaccess_gateway.v1.MxaccessGateway.MxStatusProxy; import mxaccess_gateway.v1.MxaccessGateway.MxValue; import mxaccess_gateway.v1.MxaccessGateway.OpenSessionReply; +import mxaccess_gateway.v1.MxaccessGateway.ProtocolStatus; import mxaccess_gateway.v1.MxaccessGateway.ReadBulkCommand; import mxaccess_gateway.v1.MxaccessGateway.RegisterCommand; import mxaccess_gateway.v1.MxaccessGateway.RemoveItemBulkCommand; @@ -1055,29 +1056,142 @@ public final class MxGatewaySession implements AutoCloseable { * 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 + *

On failure both the exception's message and its structured + * context (the {@link ProtocolStatus} and {@link MxCommandReply} a caller can + * inspect and log) are scrubbed with {@link MxGatewaySecrets#redactExact}: the + * gateway echoes the credential into {@code protocolStatus.message}, + * {@code reply.diagnosticMessage}, and each {@code statuses[i].diagnosticText}. + * If nothing carried the secret (the common case) 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. + * carrying the redacted message and scrubbed context; 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)) { + String redactedMessage = MxGatewaySecrets.redactExact(original, secrets); + boolean messageChanged = redactedMessage != null && !redactedMessage.equals(original); + + ProtocolStatus status = protocolStatusOf(ex); + ProtocolStatus scrubbedStatus = scrubProtocolStatus(status, secrets); + boolean statusChanged = status != null && !status.equals(scrubbedStatus); + + MxCommandReply reply = replyOf(ex); + MxCommandReply scrubbedReply = scrubReply(reply, secrets); + boolean replyChanged = reply != null && !reply.equals(scrubbedReply); + + if (!messageChanged && !statusChanged && !replyChanged) { throw ex; } - if (ex instanceof MxAccessException mx) { - throw new MxAccessException(redacted, mx.protocolStatus(), mx.reply(), null); - } - throw new MxGatewayException(redacted); + + String message = messageChanged ? redactedMessage : original; + throw rebuildRedacted(ex, message, scrubbedStatus, scrubbedReply); } } + /** + * Extracts the {@link ProtocolStatus} an exception carries, if any, so it can + * be scrubbed and re-attached to the rebuilt exception. + */ + private static ProtocolStatus protocolStatusOf(MxGatewayException ex) { + if (ex instanceof MxGatewayCommandException command) { + return command.protocolStatus(); + } + if (ex instanceof MxGatewaySessionException session) { + return session.protocolStatus(); + } + if (ex instanceof MxGatewayWorkerException worker) { + return worker.protocolStatus(); + } + return null; + } + + /** + * Extracts the raw {@link MxCommandReply} an exception carries, if any. + */ + private static MxCommandReply replyOf(MxGatewayException ex) { + if (ex instanceof MxGatewayCommandException command) { + return command.reply(); + } + return null; + } + + /** + * Rebuilds a gateway exception of the same concrete type with a redacted + * message and already-scrubbed context, mirroring the .NET client's + * type-switch. Only a truly-unknown subtype collapses to the base + * {@link MxGatewayException}. The original (secret-bearing) exception is + * deliberately not chained as a cause. + */ + private static MxGatewayException rebuildRedacted( + MxGatewayException ex, String message, ProtocolStatus status, MxCommandReply reply) { + if (ex instanceof MxAccessException) { + return new MxAccessException(message, status, reply, null); + } + if (ex instanceof MxGatewayCommandException) { + return new MxGatewayCommandException(message, status, reply, null); + } + if (ex instanceof MxGatewaySessionException) { + return new MxGatewaySessionException(message, status, null); + } + if (ex instanceof MxGatewayWorkerException) { + return new MxGatewayWorkerException(message, status, null); + } + if (ex instanceof MxGatewayMalformedReplyException) { + return new MxGatewayMalformedReplyException(message); + } + if (ex instanceof MxGatewayAuthenticationException) { + return new MxGatewayAuthenticationException(message, null); + } + if (ex instanceof MxGatewayAuthorizationException) { + return new MxGatewayAuthorizationException(message, null); + } + return new MxGatewayException(message); + } + + /** + * Produces a scrubbed clone of a command reply, removing any exact secret the + * gateway echoed into {@code protocolStatus.message}, + * {@code diagnosticMessage}, or a status's {@code diagnosticText}. + * + * @param reply the reply to scrub, or {@code null} + * @param secrets the exact secrets to strip + * @return {@code null} when {@code reply} is {@code null}, otherwise a clone + * with every echoed secret replaced by the redaction marker + */ + private static MxCommandReply scrubReply(MxCommandReply reply, String... secrets) { + if (reply == null) { + return null; + } + MxCommandReply.Builder builder = reply.toBuilder(); + if (builder.hasProtocolStatus()) { + builder.setProtocolStatus(scrubProtocolStatus(builder.getProtocolStatus(), secrets)); + } + builder.setDiagnosticMessage(MxGatewaySecrets.redactExact(builder.getDiagnosticMessage(), secrets)); + for (int index = 0; index < builder.getStatusesCount(); index++) { + MxStatusProxy.Builder status = builder.getStatuses(index).toBuilder(); + status.setDiagnosticText(MxGatewaySecrets.redactExact(status.getDiagnosticText(), secrets)); + builder.setStatuses(index, status); + } + return builder.build(); + } + + /** + * Produces a scrubbed clone of a protocol status, removing any exact secret + * the gateway echoed into its free-form {@code message}. + */ + private static ProtocolStatus scrubProtocolStatus(ProtocolStatus status, String... secrets) { + if (status == null) { + return null; + } + return status.toBuilder() + .setMessage(MxGatewaySecrets.redactExact(status.getMessage(), secrets)) + .build(); + } + /** * 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 diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySessionException.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySessionException.java index e4080dd..ae61144 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySessionException.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewaySessionException.java @@ -20,6 +20,20 @@ public final class MxGatewaySessionException extends MxGatewayException { this.protocolStatus = protocolStatus; } + /** + * Creates a new session exception with an already-built, verbatim message. + * Used to re-surface a failure with a redacted message while preserving the + * (already scrubbed) protocol status. + * + * @param message the exact message to surface (already formatted/redacted) + * @param protocolStatus protocol status returned by the gateway + * @param cause underlying error, or {@code null} + */ + protected MxGatewaySessionException(String message, ProtocolStatus protocolStatus, Throwable cause) { + super(message, cause); + this.protocolStatus = protocolStatus; + } + /** * 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/MxGatewayWorkerException.java b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayWorkerException.java index 932ea62..ed1d083 100644 --- a/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayWorkerException.java +++ b/clients/java/zb-mom-ww-mxgateway-client/src/main/java/com/zb/mom/ww/mxgateway/client/MxGatewayWorkerException.java @@ -20,6 +20,20 @@ public final class MxGatewayWorkerException extends MxGatewayException { this.protocolStatus = protocolStatus; } + /** + * Creates a new worker exception with an already-built, verbatim message. + * Used to re-surface a failure with a redacted message while preserving the + * (already scrubbed) protocol status. + * + * @param message the exact message to surface (already formatted/redacted) + * @param protocolStatus protocol status returned by the gateway + * @param cause underlying error, or {@code null} + */ + protected MxGatewayWorkerException(String message, ProtocolStatus protocolStatus, Throwable cause) { + super(message, cause); + this.protocolStatus = protocolStatus; + } + /** * Returns the gateway protocol status that triggered this exception. * 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 index 9b805a9..1657a84 100644 --- 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 @@ -2,6 +2,7 @@ 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.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -28,11 +29,23 @@ final class MxGatewayCredentialReplyTests { @Test void authenticateUserRedactsEchoedCredentialFromReplyDrivenError() throws Exception { - MxCommandReply reply = loadReply("authenticate-user.echoed-credential.reply.json"); + assertCredentialFullyRedacted( + "authenticate-user.echoed-credential.reply.json", "auth-echo-session"); + } + + @Test + void authenticateUserRedactsEchoedCredentialFromMxAccessFailureReply() throws Exception { + assertCredentialFullyRedacted( + "authenticate-user.echoed-credential-mxaccess-failure.reply.json", + "auth-echo-failure-session"); + } + + private static void assertCredentialFullyRedacted(String fixture, String sessionId) throws Exception { + MxCommandReply reply = loadReply(fixture); try (InProcessGateway gateway = InProcessGateway.startReturning(reply); MxGatewayClient client = gateway.client()) { - MxGatewaySession session = MxGatewaySession.forSessionId(client, "auth-echo-session"); + MxGatewaySession session = MxGatewaySession.forSessionId(client, sessionId); MxAccessException error = assertThrows( MxAccessException.class, @@ -42,6 +55,22 @@ final class MxGatewayCredentialReplyTests { "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"); + + // The rebuilt exception must not re-expose the credential through the + // structured reply/protocolStatus a caller can inspect and log. + MxCommandReply surfaced = error.reply(); + assertNotNull(surfaced, "the redacted exception must preserve a reply for inspection"); + assertFalse(surfaced.getProtocolStatus().getMessage().contains(CREDENTIAL), + "reply protocol status message must not leak the echoed credential"); + assertFalse(surfaced.getDiagnosticMessage().contains(CREDENTIAL), + "reply diagnostic message must not leak the echoed credential"); + for (int index = 0; index < surfaced.getStatusesCount(); index++) { + assertFalse(surfaced.getStatusesList().get(index).getDiagnosticText().contains(CREDENTIAL), + "reply status diagnostic text must not leak the echoed credential"); + } + assertNotNull(error.protocolStatus(), "the redacted exception must preserve a protocol status"); + assertFalse(error.protocolStatus().getMessage().contains(CREDENTIAL), + "exception protocol status must not leak the echoed credential"); } } diff --git a/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewaySecretsTests.java b/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewaySecretsTests.java new file mode 100644 index 0000000..9c72646 --- /dev/null +++ b/clients/java/zb-mom-ww-mxgateway-client/src/test/java/com/zb/mom/ww/mxgateway/client/MxGatewaySecretsTests.java @@ -0,0 +1,50 @@ +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.assertNull; + +import org.junit.jupiter.api.Test; + +final class MxGatewaySecretsTests { + @Test + void redactExactReplacesEveryOccurrenceOfASecret() { + String message = "verify s3cr3t, retry s3cr3t, done s3cr3t"; + + String result = MxGatewaySecrets.redactExact(message, "s3cr3t"); + + assertFalse(result.contains("s3cr3t"), "no occurrence of the secret may survive"); + assertEquals("verify , retry , done ", result); + } + + @Test + void redactExactFullyRedactsOverlappingSecretsWhenOneIsASubstringOfTheOther() { + String message = "password=hunter2 token=hunter2extra"; + + String result = MxGatewaySecrets.redactExact(message, "hunter2extra", "hunter2"); + + assertFalse(result.contains("hunter2"), "both the secret and its superstring must be fully redacted"); + assertEquals("password= token=", result); + } + + @Test + void redactExactWithNoSecretsReturnsMessageUnchanged() { + String message = "nothing to scrub here"; + + assertEquals(message, MxGatewaySecrets.redactExact(message)); + } + + @Test + void redactExactToleratesNullMessage() { + assertNull(MxGatewaySecrets.redactExact(null, "secret")); + } + + @Test + void redactExactIgnoresBlankSecretSoRealSpacesAreNotOverRedacted() { + String message = "keep these spaces intact"; + + String result = MxGatewaySecrets.redactExact(message, " ", ""); + + assertEquals(message, result); + } +} diff --git a/clients/proto/fixtures/behavior/command-replies/authenticate-user.echoed-credential-mxaccess-failure.reply.json b/clients/proto/fixtures/behavior/command-replies/authenticate-user.echoed-credential-mxaccess-failure.reply.json new file mode 100644 index 0000000..16f4e7c --- /dev/null +++ b/clients/proto/fixtures/behavior/command-replies/authenticate-user.echoed-credential-mxaccess-failure.reply.json @@ -0,0 +1,22 @@ +{ + "sessionId": "session-fixture", + "correlationId": "gateway-correlation-authenticate-echoed-mxaccess-failure", + "kind": "MX_COMMAND_KIND_AUTHENTICATE_USER", + "protocolStatus": { + "code": "PROTOCOL_STATUS_CODE_MXACCESS_FAILURE", + "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/manifest.json b/clients/proto/fixtures/behavior/manifest.json index 1b56674..01ab577 100644 --- a/clients/proto/fixtures/behavior/manifest.json +++ b/clients/proto/fixtures/behavior/manifest.json @@ -53,7 +53,14 @@ "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." + "expectation": "When a gateway/MXAccess diagnostic echoes the caller's credential back (OK envelope, negative HRESULT), the surfaced error redacts the exact secret from both the rendered message and the structured reply accessors." + }, + { + "id": "command-reply.authenticate-user.echoed-credential-mxaccess-failure", + "category": "command_replies", + "messageType": "mxaccess_gateway.v1.MxCommandReply", + "path": "command-replies/authenticate-user.echoed-credential-mxaccess-failure.reply.json", + "expectation": "The same echoed-credential redaction holds when the reply is coded PROTOCOL_STATUS_CODE_MXACCESS_FAILURE, which every client routes to its MXAccess error type." }, { "id": "command-reply.authenticate-user.missing-payload", diff --git a/clients/python/src/zb_mom_ww_mxgateway/session.py b/clients/python/src/zb_mom_ww_mxgateway/session.py index 724d619..78f75aa 100644 --- a/clients/python/src/zb_mom_ww_mxgateway/session.py +++ b/clients/python/src/zb_mom_ww_mxgateway/session.py @@ -919,19 +919,47 @@ def _value_secrets(value: MxValueInput) -> list[str]: def _redact_error(error: MxGatewayError, secrets: Sequence[str | None]) -> None: - """Scrub secret substrings from a raised error's message in place. + """Scrub secret substrings from a raised error's message and reply in place. Rewrites ``error.args[0]`` (the message returned by ``str(error)``) through the shared :func:`~zb_mom_ww_mxgateway.auth.redact_secret` seam so credential - text can never reach logs or be re-raised to a caller. The - ``protocol_status`` / ``raw_reply`` context is left untouched — those hold the - gateway's own fields, which never echo the client-supplied secret. + text can never reach logs or be re-raised to a caller. + + A misbehaving MXAccess provider can echo the client-supplied credential back + verbatim in its failure diagnostics, so ``error.raw_reply`` (the protobuf + reply) can carry the secret in ``protocol_status.message``, + ``diagnostic_message``, and each ``statuses[].diagnostic_text``. A logger + dumping those structured fields would reintroduce the leak the message scrub + closes. When there is a secret to scrub and a reply is attached, this rebinds + ``error.raw_reply`` to a scrubbed deep copy so the raised exception carries no + credential text on any surface. The clone leaves the original reply untouched. """ scrubbed = [secret for secret in secrets if secret] if not scrubbed: return if error.args and isinstance(error.args[0], str): error.args = (redact_secret(error.args[0], scrubbed), *error.args[1:]) + if error.raw_reply is not None: + error.raw_reply = _redact_reply(error.raw_reply, scrubbed) + + +def _redact_reply(reply: pb.MxCommandReply, secrets: Sequence[str]) -> pb.MxCommandReply: + """Return a deep copy of *reply* with credential text scrubbed from diagnostics. + + Operates on a clone so the caller's original reply object is never mutated. + Only the free-text diagnostic fields that can echo a client-supplied secret + are scrubbed; the structured/enum fields the gateway itself sets are left as-is. + """ + clone = type(reply)() + clone.CopyFrom(reply) + if clone.protocol_status.message: + clone.protocol_status.message = redact_secret(clone.protocol_status.message, secrets) + if clone.diagnostic_message: + clone.diagnostic_message = redact_secret(clone.diagnostic_message, secrets) + for status in clone.statuses: + if status.diagnostic_text: + status.diagnostic_text = redact_secret(status.diagnostic_text, secrets) + return clone from .client import GatewayClient # noqa: E402 diff --git a/clients/python/tests/test_malformed_reply.py b/clients/python/tests/test_malformed_reply.py index bc1c634..fe8b8a3 100644 --- a/clients/python/tests/test_malformed_reply.py +++ b/clients/python/tests/test_malformed_reply.py @@ -82,15 +82,31 @@ async def test_add_buffered_item_missing_payload_raises_malformed_reply() -> Non assert captured.value.raw_reply is reply +@pytest.mark.parametrize( + "fixture", + [ + "command-replies/authenticate-user.echoed-credential.reply.json", + "command-replies/authenticate-user.echoed-credential-mxaccess-failure.reply.json", + ], +) @pytest.mark.asyncio -async def test_authenticate_user_echoed_credential_is_scrubbed() -> None: +async def test_authenticate_user_echoed_credential_is_scrubbed(fixture: str) -> None: credential = "sup3rSecretVerify9f3a2b" - reply = _load_reply("command-replies/authenticate-user.echoed-credential.reply.json") + reply = _load_reply(fixture) session, _ = await _session_with([reply]) with pytest.raises(MxAccessError) as captured: await session.authenticate_user(12, "operator", credential) - message = str(captured.value) + exc = captured.value + message = str(exc) assert credential not in message assert "[redacted]" in message + + # The credential must not survive in the structured protobuf context either: + # a logger dumping raw_reply's fields would otherwise reintroduce the leak. + assert exc.raw_reply is not None + assert credential not in exc.raw_reply.protocol_status.message + assert credential not in exc.raw_reply.diagnostic_message + for status in exc.raw_reply.statuses: + assert credential not in status.diagnostic_text diff --git a/clients/python/tests/test_typed_command_helpers.py b/clients/python/tests/test_typed_command_helpers.py index c0f7124..3e25686 100644 --- a/clients/python/tests/test_typed_command_helpers.py +++ b/clients/python/tests/test_typed_command_helpers.py @@ -140,10 +140,19 @@ async def test_write_secured_surfaces_native_failure_without_prior_authenticate( with pytest.raises(MxAccessError) as captured: await session.write_secured(12, 34, secret_value, current_user_id=5, verifier_user_id=6) - # Native failure is surfaced (not "fixed") and the raw reply is preserved... - assert captured.value.raw_reply is failure - # ...but the credential-sensitive value is scrubbed from the surfaced message. + # Native failure is surfaced (not "fixed"): the raw reply's structure is + # preserved so callers still see the native verdict... + raw = captured.value.raw_reply + assert raw is not None + assert raw.kind == pb.MX_COMMAND_KIND_WRITE_SECURED + assert raw.hresult == -2147217407 + assert raw.protocol_status.code == pb.PROTOCOL_STATUS_CODE_MXACCESS_FAILURE + # ...but the credential-sensitive value is scrubbed from the surfaced message + # AND from the reply's echoed diagnostics, so a logger dumping raw_reply's + # structured fields cannot reintroduce the leak. assert secret_value not in str(captured.value) + assert secret_value not in raw.protocol_status.message + assert "[redacted]" in raw.protocol_status.message command = stub.invoke.requests[0].command assert command.kind == pb.MX_COMMAND_KIND_WRITE_SECURED assert command.write_secured.current_user_id == 5 diff --git a/clients/rust/src/error.rs b/clients/rust/src/error.rs index 90b22a6..80654c0 100644 --- a/clients/rust/src/error.rs +++ b/clients/rust/src/error.rs @@ -346,10 +346,17 @@ impl From for Error { /// Promote a non-OK protocol status carried inside an [`MxCommandReply`] /// to an [`Error::Command`]. /// +/// [`ProtocolStatusCode::MxaccessFailure`] is deliberately **not** a +/// command-level failure here: it signals an MXAccess-level rejection, so it +/// falls through to [`ensure_mxaccess_success`] and surfaces as +/// [`Error::MxAccess`] — matching the .NET, Java, Go, and Python clients. Every +/// other non-`Ok` code stays [`Error::Command`]. +/// /// # Errors /// /// Returns [`Error::Command`] when `reply.protocol_status` is missing or -/// reports any code other than [`ProtocolStatusCode::Ok`]. +/// reports any code other than [`ProtocolStatusCode::Ok`] or +/// [`ProtocolStatusCode::MxaccessFailure`]. pub fn ensure_command_success(reply: MxCommandReply) -> Result { let code = reply .protocol_status @@ -357,7 +364,7 @@ pub fn ensure_command_success(reply: MxCommandReply) -> Result Result Result Result { + let protocol_code = reply + .protocol_status + .as_ref() + .and_then(|status| ProtocolStatusCode::try_from(status.code).ok()) + .unwrap_or(ProtocolStatusCode::Unspecified); + let mxaccess_failure = protocol_code == ProtocolStatusCode::MxaccessFailure; let hresult_failure = reply.hresult.is_some_and(|hresult| hresult < 0); let status_failure = reply .statuses .iter() .any(|status| status.category != MxStatusCategory::Ok as i32); - if hresult_failure || status_failure { + if mxaccess_failure || hresult_failure || status_failure { Err(Box::new(MxAccessError::new(reply)).into()) } else { Ok(reply) diff --git a/clients/rust/src/session.rs b/clients/rust/src/session.rs index fd6768e..7a7dd02 100644 --- a/clients/rust/src/session.rs +++ b/clients/rust/src/session.rs @@ -11,7 +11,7 @@ use std::sync::atomic::{AtomicU64, Ordering}; use crate::client::{EventStream, GatewayClient}; -use crate::error::{ensure_protocol_success, Error}; +use crate::error::{ensure_protocol_success, Error, MxAccessError}; use crate::generated::mxaccess_gateway::v1::mx_command::Payload; use crate::generated::mxaccess_gateway::v1::mx_command_reply; use crate::generated::mxaccess_gateway::v1::{ @@ -1117,16 +1117,46 @@ fn string_secret(value: &MxValue) -> Vec { } /// 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. +/// propagates. This both scrubs the stored reply's caller-readable string +/// fields (so `reply()`/`into_reply()` cannot recover a credential MXAccess +/// echoed back verbatim) and keeps the secrets on the error as a +/// belt-and-suspenders for `Display`/`Debug`. 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))), + Error::MxAccess(boxed) => { + let mut reply = boxed.into_reply(); + scrub_reply_strings(&mut reply, &secrets); + Error::MxAccess(Box::new(MxAccessError::new(reply).with_secrets(secrets))) + } other => other, } } +/// Replace every non-empty secret occurrence with `` in the reply's +/// caller-readable string fields — `protocol_status.message`, +/// `diagnostic_message`, and each `statuses[i].diagnostic_text`. A caller +/// reading the structured reply back off an [`Error::MxAccess`] would otherwise +/// reintroduce the leak that `Display`/`Debug` already close. +fn scrub_reply_strings(reply: &mut MxCommandReply, secrets: &[String]) { + for secret in secrets { + if secret.is_empty() { + continue; + } + if let Some(status) = reply.protocol_status.as_mut() { + status.message = status.message.replace(secret.as_str(), ""); + } + reply.diagnostic_message = reply + .diagnostic_message + .replace(secret.as_str(), ""); + for status in &mut reply.statuses { + status.diagnostic_text = status + .diagnostic_text + .replace(secret.as_str(), ""); + } + } +} + fn suspend_status(reply: MxCommandReply) -> Result { match reply.payload { Some(mx_command_reply::Payload::Suspend(suspend)) => suspend diff --git a/clients/rust/tests/client_behavior.rs b/clients/rust/tests/client_behavior.rs index 84323ca..4b0dba3 100644 --- a/clients/rust/tests/client_behavior.rs +++ b/clients/rust/tests/client_behavior.rs @@ -84,8 +84,10 @@ async fn session_helpers_build_commands_and_preserve_command_errors() { .write(12, 34, ClientMxValue::int32(123), 0) .await .unwrap_err(); - let Error::Command(error) = error else { - panic!("write failure should preserve the raw command reply: {error:?}"); + // A MXACCESS_FAILURE-coded reply is an MXAccess-level failure, routed to + // Error::MxAccess (matching .NET/Java/Go/Python) rather than Error::Command. + let Error::MxAccess(error) = error else { + panic!("MXACCESS_FAILURE reply should route to Error::MxAccess: {error:?}"); }; assert_eq!( error.reply().protocol_status.as_ref().unwrap().code, @@ -841,6 +843,92 @@ async fn authenticate_user_scrubs_exact_caller_credential_echoed_in_diagnostic() ); } +/// Drive `authenticate_user` against a canned reply that echoes the caller's +/// credential in every string field, then assert the surfaced +/// [`Error::MxAccess`] leaks it nowhere — neither through the structured reply a +/// caller can read back (`reply().protocol_status.message`, +/// `reply().diagnostic_message`, `reply().statuses[i].diagnostic_text`) nor +/// through `Display`/`Debug`. +async fn assert_authenticate_user_scrubs_structured_reply(fixture: &str) { + let credential = "sup3rSecretVerify9f3a2b"; + let state = Arc::new(FakeState::default()); + *state.invoke_override.lock().await = Some(InvokeOverride::CannedReply(Box::new( + command_reply_fixture(fixture), + ))); + 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(); + + let Error::MxAccess(mx_access) = &error else { + panic!("{fixture}: credential-echoed reply must route to Error::MxAccess, got {error:?}"); + }; + + // The structured reply a caller can read back must be scrubbed too — the raw + // MxCommandReply otherwise reintroduces the leak Display/Debug already close. + let reply = mx_access.reply(); + if let Some(status) = reply.protocol_status.as_ref() { + assert!( + !status.message.contains(credential), + "{fixture}: credential leaked via reply().protocol_status.message: {}", + status.message + ); + } + assert!( + !reply.diagnostic_message.contains(credential), + "{fixture}: credential leaked via reply().diagnostic_message: {}", + reply.diagnostic_message + ); + for (index, status) in reply.statuses.iter().enumerate() { + assert!( + !status.diagnostic_text.contains(credential), + "{fixture}: credential leaked via reply().statuses[{index}].diagnostic_text: {}", + status.diagnostic_text + ); + } + + let display = error.to_string(); + let debug = format!("{error:?}"); + assert!( + !display.contains(credential), + "{fixture}: credential leaked into Display: {display}" + ); + assert!( + !debug.contains(credential), + "{fixture}: credential leaked into Debug: {debug}" + ); + assert!( + display.contains(""), + "{fixture}: Display must mark the redaction: {display}" + ); +} + +#[tokio::test] +async fn authenticate_user_scrubs_credential_from_structured_reply_ok_protocol_variant() { + // OK protocol envelope + negative hresult: already Error::MxAccess before + // ISSUE 2, but the stored reply's string fields still leaked the credential. + assert_authenticate_user_scrubs_structured_reply( + "authenticate-user.echoed-credential.reply.json", + ) + .await; +} + +#[tokio::test] +async fn authenticate_user_scrubs_credential_from_structured_reply_mxaccess_failure_variant() { + // PROTOCOL_STATUS_CODE_MXACCESS_FAILURE: before ISSUE 2 this landed in + // Error::Command (unscrubbed, raw Display/Debug) — the red-first case. + assert_authenticate_user_scrubs_structured_reply( + "authenticate-user.echoed-credential-mxaccess-failure.reply.json", + ) + .await; +} + #[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 @@ -1548,15 +1636,35 @@ fn command_reply_fixture(file_name: &str) -> MxCommandReply { }) }); + // Honor the fixture's real protocol status (code + message) so a canned + // reply can drive the MXACCESS_FAILURE routing path, not just an OK + // envelope. Falls back to an OK envelope when the fixture omits it. + let protocol_status = fixture.get("protocolStatus").map_or_else( + || ok_status("command ok"), + |status| { + let code_name = status["code"].as_str().unwrap_or("PROTOCOL_STATUS_CODE_OK"); + ProtocolStatus { + code: ProtocolStatusCode::from_str_name(code_name) + .unwrap_or_else(|| panic!("unknown protocol status code {code_name}")) + as i32, + message: status["message"].as_str().unwrap_or_default().to_owned(), + } + }, + ); + MxCommandReply { session_id: fixture["sessionId"].as_str().unwrap_or_default().to_owned(), correlation_id: fixture["correlationId"] .as_str() .unwrap_or_default() .to_owned(), - protocol_status: Some(ok_status("command ok")), + protocol_status: Some(protocol_status), hresult: fixture["hresult"].as_i64().map(|hresult| hresult as i32), statuses, + diagnostic_message: fixture["diagnosticMessage"] + .as_str() + .unwrap_or_default() + .to_owned(), return_value, ..MxCommandReply::default() } diff --git a/docs/ClientBehaviorFixtures.md b/docs/ClientBehaviorFixtures.md index 0d0f709..3d7f195 100644 --- a/docs/ClientBehaviorFixtures.md +++ b/docs/ClientBehaviorFixtures.md @@ -77,7 +77,8 @@ 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.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 from **both** the rendered message and the structured reply accessors (never leaks the verbatim value) | +| `authenticate-user.echoed-credential-mxaccess-failure.reply.json` | the same echo, but coded `PROTOCOL_STATUS_CODE_MXACCESS_FAILURE` | identical redaction; confirms every client routes the MXAccess-failure protocol code to its MXAccess error type and scrubs it | | `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 | @@ -91,13 +92,17 @@ The rules those fixtures lock in are: 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. + values they were called with from any surfaced error — both the rendered + message text **and** the structured reply the error still exposes (a + server-echoed credential lives in `protocolStatus.message` and + `statuses[].diagnosticText`, which the error's raw-reply accessor would + otherwise re-expose to a logger dumping structured fields). The redacted error + therefore carries a scrubbed clone of the reply. 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; each suite asserts that + neither the surfaced message nor the exposed reply still contains the + credential, and that the message contains the client's marker. ## Event Streams diff --git a/docs/ClientLibrariesDesign.md b/docs/ClientLibrariesDesign.md index 0050138..44775ee 100644 --- a/docs/ClientLibrariesDesign.md +++ b/docs/ClientLibrariesDesign.md @@ -138,10 +138,16 @@ 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. 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 +values it was called with from any surfaced error 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. +(CLI-40). The scrub covers **both** the rendered message and the structured reply +the error still exposes (`protocolStatus.message`, `statuses[].diagnosticText`, +`diagnosticMessage`): the redacted error carries a scrubbed clone of the reply so +a logger dumping the exception's structured fields cannot reintroduce the leak. +This holds regardless of whether the reply is coded `OK` (with a failing HRESULT) +or `MXACCESS_FAILURE` — every client routes both to its MXAccess error type. Each +client's test suite asserts the distinctive credential is absent from both the +surfaced message and the exposed reply, and that the redaction marker is present. Shipped in all five clients (.NET / Go / Rust / Python / Java).