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.
This commit is contained in:
@@ -346,10 +346,17 @@ impl From<tonic::Status> 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<MxCommandReply, Error> {
|
||||
let code = reply
|
||||
.protocol_status
|
||||
@@ -357,7 +364,7 @@ pub fn ensure_command_success(reply: MxCommandReply) -> Result<MxCommandReply, E
|
||||
.and_then(|status| ProtocolStatusCode::try_from(status.code).ok())
|
||||
.unwrap_or(ProtocolStatusCode::Unspecified);
|
||||
|
||||
if code == ProtocolStatusCode::Ok {
|
||||
if code == ProtocolStatusCode::Ok || code == ProtocolStatusCode::MxaccessFailure {
|
||||
Ok(reply)
|
||||
} else {
|
||||
Err(Box::new(CommandError::new(reply)).into())
|
||||
@@ -368,9 +375,12 @@ pub fn ensure_command_success(reply: MxCommandReply) -> Result<MxCommandReply, E
|
||||
/// [`MxCommandReply`] to an [`Error::MxAccess`].
|
||||
///
|
||||
/// This is the second reply check applied to the typed command path, after
|
||||
/// [`ensure_command_success`] confirms the protocol envelope is `Ok`. It
|
||||
/// enforces MXAccess parity: a reply can carry an `Ok` protocol envelope while
|
||||
/// MXAccess itself rejected the operation. Following COM semantics, only a
|
||||
/// [`ensure_command_success`] confirms the protocol envelope is `Ok` (or a
|
||||
/// [`ProtocolStatusCode::MxaccessFailure`] the first check lets fall through).
|
||||
/// It enforces MXAccess parity: a reply can carry an `Ok` protocol envelope
|
||||
/// while MXAccess itself rejected the operation, and a
|
||||
/// [`ProtocolStatusCode::MxaccessFailure`] envelope is itself an MXAccess-level
|
||||
/// failure regardless of `hresult`. Following COM semantics, only a
|
||||
/// **negative** `hresult` is a failure — positive codes such as `S_FALSE = 1`
|
||||
/// are success. A `MXSTATUS_PROXY` entry is treated as a failure when its
|
||||
/// `category` is not [`MxStatusCategory::Ok`]; the `success` member mirrors the
|
||||
@@ -383,17 +393,24 @@ pub fn ensure_command_success(reply: MxCommandReply) -> Result<MxCommandReply, E
|
||||
///
|
||||
/// # Errors
|
||||
///
|
||||
/// Returns [`Error::MxAccess`] when `reply.hresult` is negative or any
|
||||
/// Returns [`Error::MxAccess`] when the reply's protocol code is
|
||||
/// [`ProtocolStatusCode::MxaccessFailure`], `reply.hresult` is negative, or any
|
||||
/// `reply.statuses` entry reports a category other than
|
||||
/// [`MxStatusCategory::Ok`].
|
||||
pub fn ensure_mxaccess_success(reply: MxCommandReply) -> Result<MxCommandReply, Error> {
|
||||
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)
|
||||
|
||||
@@ -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<String> {
|
||||
}
|
||||
|
||||
/// 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<String>) -> 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 `<redacted>` 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(), "<redacted>");
|
||||
}
|
||||
reply.diagnostic_message = reply
|
||||
.diagnostic_message
|
||||
.replace(secret.as_str(), "<redacted>");
|
||||
for status in &mut reply.statuses {
|
||||
status.diagnostic_text = status
|
||||
.diagnostic_text
|
||||
.replace(secret.as_str(), "<redacted>");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn suspend_status(reply: MxCommandReply) -> Result<MxStatus, Error> {
|
||||
match reply.payload {
|
||||
Some(mx_command_reply::Payload::Suspend(suspend)) => suspend
|
||||
|
||||
@@ -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("<redacted>"),
|
||||
"{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()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user