0d874f91ee
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.
198 lines
7.3 KiB
Go
198 lines
7.3 KiB
Go
package mxgateway
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"os"
|
|
"path/filepath"
|
|
"strings"
|
|
"testing"
|
|
|
|
pb "gitea.dohertylan.com/dohertj2/mxaccessgw/clients/go/internal/generated"
|
|
"google.golang.org/protobuf/encoding/protojson"
|
|
)
|
|
|
|
// loadCommandReplyFixture parses a shared command-reply fixture into an
|
|
// MxCommandReply so the Go client can be driven through the same wire shapes the
|
|
// other language clients exercise.
|
|
func loadCommandReplyFixture(t *testing.T, name string) *pb.MxCommandReply {
|
|
t.Helper()
|
|
path := filepath.Join("..", "..", "proto", "fixtures", "behavior", "command-replies", name)
|
|
data, err := os.ReadFile(path)
|
|
if err != nil {
|
|
t.Fatalf("read fixture %s: %v", name, err)
|
|
}
|
|
var reply pb.MxCommandReply
|
|
if err := protojson.Unmarshal(data, &reply); err != nil {
|
|
t.Fatalf("parse fixture %s: %v", name, err)
|
|
}
|
|
return &reply
|
|
}
|
|
|
|
func TestAuthenticateUserMissingPayloadReturnsMalformedReplyError(t *testing.T) {
|
|
fake := &fakeGatewayServer{
|
|
invokeReply: loadCommandReplyFixture(t, "authenticate-user.missing-payload.reply.json"),
|
|
}
|
|
client, cleanup := newBufconnClient(t, fake)
|
|
defer cleanup()
|
|
session := NewSessionForID(client, "session-1")
|
|
|
|
_, err := session.AuthenticateUser(context.Background(), 12, "operator", "secret")
|
|
var malformed *MalformedReplyError
|
|
if !errors.As(err, &malformed) {
|
|
t.Fatalf("AuthenticateUser() error = %v (%T), want *MalformedReplyError", err, err)
|
|
}
|
|
if malformed.Op != "authenticate user" {
|
|
t.Fatalf("MalformedReplyError.Op = %q, want %q", malformed.Op, "authenticate user")
|
|
}
|
|
}
|
|
|
|
func TestAuthenticateUserReturnValueOnlyUsesInt32ReturnValue(t *testing.T) {
|
|
fake := &fakeGatewayServer{
|
|
invokeReply: loadCommandReplyFixture(t, "authenticate-user.return-value-only.reply.json"),
|
|
}
|
|
client, cleanup := newBufconnClient(t, fake)
|
|
defer cleanup()
|
|
session := NewSessionForID(client, "session-1")
|
|
|
|
userID, err := session.AuthenticateUser(context.Background(), 12, "operator", "secret")
|
|
if err != nil {
|
|
t.Fatalf("AuthenticateUser() error = %v", err)
|
|
}
|
|
if userID != 7 {
|
|
t.Fatalf("AuthenticateUser() = %d, want 7", userID)
|
|
}
|
|
}
|
|
|
|
// AddBufferedItem shares the prefer-payload / int32-return-value / malformed
|
|
// fallback code path; cover both branches for one of the siblings.
|
|
func TestAddBufferedItemFallbackHonoursReturnValueAndReportsMalformed(t *testing.T) {
|
|
t.Run("return-value-only", func(t *testing.T) {
|
|
fake := &fakeGatewayServer{
|
|
invokeReply: loadCommandReplyFixture(t, "authenticate-user.return-value-only.reply.json"),
|
|
}
|
|
client, cleanup := newBufconnClient(t, fake)
|
|
defer cleanup()
|
|
session := NewSessionForID(client, "session-1")
|
|
|
|
itemHandle, err := session.AddBufferedItem(context.Background(), 12, "Area001.Pump001.Speed", "runtime")
|
|
if err != nil {
|
|
t.Fatalf("AddBufferedItem() error = %v", err)
|
|
}
|
|
if itemHandle != 7 {
|
|
t.Fatalf("AddBufferedItem() = %d, want 7", itemHandle)
|
|
}
|
|
})
|
|
|
|
t.Run("missing-payload", func(t *testing.T) {
|
|
fake := &fakeGatewayServer{
|
|
invokeReply: loadCommandReplyFixture(t, "authenticate-user.missing-payload.reply.json"),
|
|
}
|
|
client, cleanup := newBufconnClient(t, fake)
|
|
defer cleanup()
|
|
session := NewSessionForID(client, "session-1")
|
|
|
|
_, err := session.AddBufferedItem(context.Background(), 12, "Area001.Pump001.Speed", "runtime")
|
|
var malformed *MalformedReplyError
|
|
if !errors.As(err, &malformed) {
|
|
t.Fatalf("AddBufferedItem() error = %v (%T), want *MalformedReplyError", err, err)
|
|
}
|
|
if malformed.Op != "add buffered item" {
|
|
t.Fatalf("MalformedReplyError.Op = %q, want %q", malformed.Op, "add buffered item")
|
|
}
|
|
})
|
|
}
|
|
|
|
// TestAuthenticateUserScrubsEchoedCredentialFromError is the CLI-40 regression:
|
|
// a gateway diagnostic that echoes the raw credential back must never reach the
|
|
// caller's surfaced error text.
|
|
func TestAuthenticateUserScrubsEchoedCredentialFromError(t *testing.T) {
|
|
const credential = "sup3rSecretVerify9f3a2b"
|
|
fake := &fakeGatewayServer{
|
|
invokeReply: loadCommandReplyFixture(t, "authenticate-user.echoed-credential.reply.json"),
|
|
}
|
|
client, cleanup := newBufconnClient(t, fake)
|
|
defer cleanup()
|
|
session := NewSessionForID(client, "session-1")
|
|
|
|
_, err := session.AuthenticateUser(context.Background(), 12, "operator", credential)
|
|
if err == nil {
|
|
t.Fatal("AuthenticateUser() error = nil, want an MXAccess failure")
|
|
}
|
|
message := err.Error()
|
|
if strings.Contains(message, credential) {
|
|
t.Fatalf("surfaced error leaked the credential: %q", message)
|
|
}
|
|
if !strings.Contains(message, "<redacted>") {
|
|
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)
|
|
}
|
|
})
|
|
}
|
|
}
|