From 0d874f91ee38673ae89e0f4c29b37c6db8eb5d95 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Fri, 7 Aug 2026 07:04:56 -0400 Subject: [PATCH] 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).