From ff41556b9a6b12078f3fb7232a287bd89911e8e5 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 18 May 2026 21:31:46 -0400 Subject: [PATCH] Resolve Client.Java-001..005 code-review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Client.Java-001: redactApiKey echoed the last 4 secret characters. It now keeps only the non-secret mxgw__ prefix plus ***; non-gateway-shaped tokens return . Client.Java-002: a close() after a queue-overflow could wipe the enqueued overflow exception. Terminal transitions are now serialized through a single guarded terminate() — first terminal condition wins. Client.Java-003: openSession never read gateway_protocol_version. Both openSession paths now call ensureGatewayProtocolCompatible, rejecting a non-zero mismatch and accepting unset (0) for older gateways. Client.Java-004: register/addItem/addItem2 fell back to a return_value that silently yields 0 when unset. The fallback is now guarded by hasReturnValue() and throws on a protocol violation. Client.Java-005: close() in try-with-resources could mask the body exception when the CloseSession RPC failed. close() now catches and logs the close-time failure; closeRaw() still surfaces it for callers that want it. Co-Authored-By: Claude Opus 4.7 (1M context) --- clients/java/README.md | 12 + .../mxgateway/cli/MxGatewayCliTests.java | 4 +- .../mxgateway/client/MxEventStream.java | 52 ++- .../mxgateway/client/MxGatewayClient.java | 20 + .../mxgateway/client/MxGatewaySecrets.java | 28 +- .../mxgateway/client/MxGatewaySession.java | 38 +- .../client/MxGatewayMediumFindingsTests.java | 394 ++++++++++++++++++ code-reviews/Client.Java/findings.md | 22 +- 8 files changed, 537 insertions(+), 33 deletions(-) create mode 100644 clients/java/mxgateway-client/src/test/java/com/dohertylan/mxgateway/client/MxGatewayMediumFindingsTests.java diff --git a/clients/java/README.md b/clients/java/README.md index 7a9fe97..ad03020 100644 --- a/clients/java/README.md +++ b/clients/java/README.md @@ -62,6 +62,18 @@ underlying protobuf messages. `MxGatewayCommandException` and `MxAccessException` preserve the raw `MxCommandReply` when the gateway returns a data-bearing MXAccess failure. +`openSession` verifies the gateway's reported `gateway_protocol_version` against +the version this client was generated for and throws `MxGatewayException` on a +mismatch, so an incompatible client fails fast with a clear message instead of +issuing commands that fail downstream. A gateway that does not populate the +field is accepted unchanged. + +`MxGatewaySession` implements `AutoCloseable`. The try-with-resources `close()` +performs a `CloseSession` network RPC but swallows (and logs) any failure of +that RPC so a close-time error never replaces the exception a try-with-resources +body is already propagating. Call `closeRaw()` explicitly when you need to +observe the close result or handle a close-time failure. + `MxEventStream` implements `Iterator` and `AutoCloseable`. Closing it cancels the underlying gRPC stream. Canceling or timing out a Java client call only stops the client from waiting; it does not abort an in-flight MXAccess COM diff --git a/clients/java/mxgateway-cli/src/test/java/com/dohertylan/mxgateway/cli/MxGatewayCliTests.java b/clients/java/mxgateway-cli/src/test/java/com/dohertylan/mxgateway/cli/MxGatewayCliTests.java index 481bea7..a26bcb5 100644 --- a/clients/java/mxgateway-cli/src/test/java/com/dohertylan/mxgateway/cli/MxGatewayCliTests.java +++ b/clients/java/mxgateway-cli/src/test/java/com/dohertylan/mxgateway/cli/MxGatewayCliTests.java @@ -62,8 +62,10 @@ final class MxGatewayCliTests { assertEquals(0, run.exitCode()); assertTrue(run.output().contains("\"command\":\"open-session\"")); assertTrue(run.output().contains("\"sessionId\":\"session-cli\"")); - assertTrue(run.output().contains("mxgw***********cret")); + // Only the non-secret mxgw__ prefix survives; the secret is fully masked. + assertTrue(run.output().contains("mxgw_visible_***")); assertFalse(run.output().contains("visible_secret")); + assertFalse(run.output().contains("cret")); } @Test diff --git a/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxEventStream.java b/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxEventStream.java index 0f669cd..361b87b 100644 --- a/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxEventStream.java +++ b/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxEventStream.java @@ -21,13 +21,23 @@ import mxaccess_gateway.v1.MxaccessGateway.StreamEventsRequest; * stream cancels the underlying gRPC call. If the queue overflows the call is * cancelled and a follow-up call to {@link #next()} throws * {@link MxGatewayException}. + * + *

Threading: the iterator methods ({@link #hasNext()} and + * {@link #next()}) are not thread-safe and must be driven by a single + * consumer thread. {@link #close()} may be called from any thread. Terminal + * state transitions (queue overflow, server completion, and {@code close()}) + * are serialised so that the first terminal condition wins deterministically: + * once an overflow exception has been observed it is never silently replaced + * by an end-of-stream marker. */ public final class MxEventStream implements Iterator, AutoCloseable { private static final Object END = new Object(); private final BlockingQueue queue; + private final Object terminalLock = new Object(); private volatile ClientCallStreamObserver requestStream; private volatile boolean closed; + private boolean terminated; private Object next; MxEventStream(int capacity) { @@ -98,7 +108,7 @@ public final class MxEventStream implements Iterator, AutoCloseable { if (stream != null) { stream.cancel("client cancelled event stream", null); } - offer(END); + terminate(null); } private Object take() { @@ -115,10 +125,7 @@ public final class MxEventStream implements Iterator, AutoCloseable { private void offer(Object value) { Objects.requireNonNull(value, "value"); if (value == END) { - if (!queue.offer(value)) { - queue.clear(); - queue.offer(value); - } + terminate(null); return; } if (!queue.offer(value)) { @@ -126,9 +133,38 @@ public final class MxEventStream implements Iterator, AutoCloseable { if (stream != null) { stream.cancel("client event stream queue overflowed", null); } - queue.clear(); - queue.offer(new MxGatewayException("gateway stream events queue overflowed")); - queue.offer(END); + terminate(new MxGatewayException("gateway stream events queue overflowed")); + } + } + + /** + * Drives the single terminal transition. The first caller wins: a later + * end-of-stream or {@code close()} cannot overwrite or discard an overflow + * exception that has already been published to the consumer. + * + * @param fault the fault to surface to the consumer, or {@code null} for a + * clean end-of-stream + */ + private void terminate(MxGatewayException fault) { + synchronized (terminalLock) { + if (terminated) { + return; + } + terminated = true; + if (fault != null) { + // Make room for the fault marker; the consumer only needs the + // terminal signal, queued data events are no longer relevant. + queue.clear(); + queue.offer(fault); + queue.offer(END); + return; + } + // Clean end-of-stream: ensure the END marker is delivered even when + // the queue is currently full of undrained data events. + if (!queue.offer(END)) { + queue.clear(); + queue.offer(END); + } } } } diff --git a/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewayClient.java b/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewayClient.java index e3599fe..1ace8d4 100644 --- a/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewayClient.java +++ b/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewayClient.java @@ -150,6 +150,7 @@ public final class MxGatewayClient implements AutoCloseable { try { OpenSessionReply reply = rawBlockingStub().openSession(request); MxGatewayErrors.ensureProtocolSuccess("open session", reply.getProtocolStatus(), null); + ensureGatewayProtocolCompatible(reply); return reply; } catch (RuntimeException error) { if (error instanceof MxGatewayException) { @@ -159,6 +160,24 @@ public final class MxGatewayClient implements AutoCloseable { } } + /** + * Verifies that the gateway speaks the protocol version this client was + * generated against. A gateway that leaves {@code gateway_protocol_version} + * unset (value {@code 0}, e.g. an older gateway) is accepted unchanged. + * + * @param reply the {@code OpenSessionReply} returned by the gateway + * @throws MxGatewayException if the gateway reports an incompatible protocol version + */ + private static void ensureGatewayProtocolCompatible(OpenSessionReply reply) { + int gatewayVersion = reply.getGatewayProtocolVersion(); + int clientVersion = MxGatewayClientVersion.gatewayProtocolVersion(); + if (gatewayVersion != 0 && gatewayVersion != clientVersion) { + throw new MxGatewayException("gateway protocol version mismatch: gateway reports " + + gatewayVersion + " but this client was built for " + clientVersion + + "; upgrade the client or gateway so the protocol versions match"); + } + } + /** * Invokes {@code OpenSession} asynchronously. * @@ -170,6 +189,7 @@ public final class MxGatewayClient implements AutoCloseable { CompletableFuture future = toCompletable(rawFutureStub().openSession(request)); return future.thenApply(reply -> { MxGatewayErrors.ensureProtocolSuccess("open session", reply.getProtocolStatus(), null); + ensureGatewayProtocolCompatible(reply); return reply; }); } diff --git a/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySecrets.java b/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySecrets.java index be5deed..8060603 100644 --- a/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySecrets.java +++ b/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySecrets.java @@ -11,25 +11,35 @@ public final class MxGatewaySecrets { } /** - * Redacts the body of an API key, leaving only short prefix and suffix - * windows so it remains comparable in logs. + * Redacts the secret portion of an API key, leaving only the non-secret + * key identifier visible so the value remains comparable in logs. + * + *

A gateway API key has the form {@code mxgw__}. Only the + * {@code mxgw__} prefix is non-secret; everything after the second + * underscore is the secret and is masked entirely — no leading or + * trailing characters of the secret are echoed. Tokens that do not match + * the gateway shape are masked completely as {@code ""}. * * @param apiKey the API key to redact, may be {@code null} or empty * @return an empty string for {@code null}/empty input, {@code ""} - * for keys eight characters or shorter, or a masked form preserving - * the leading and trailing four characters + * for non-gateway-shaped tokens, or {@code mxgw__***} with the + * secret masked for gateway-shaped keys */ public static String redactApiKey(String apiKey) { if (apiKey == null || apiKey.isEmpty()) { return ""; } - if (apiKey.length() <= 8) { - return ""; + + // Gateway keys are mxgw__; keep only the non-secret prefix. + if (apiKey.startsWith("mxgw_")) { + int secretSeparator = apiKey.indexOf('_', "mxgw_".length()); + if (secretSeparator >= 0 && secretSeparator < apiKey.length() - 1) { + return apiKey.substring(0, secretSeparator + 1) + "***"; + } } - return apiKey.substring(0, 4) - + "*".repeat(apiKey.length() - 8) - + apiKey.substring(apiKey.length() - 4); + // Anything else is treated as wholly secret — reveal nothing. + return ""; } /** diff --git a/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySession.java b/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySession.java index 51b686a..35f68bd 100644 --- a/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySession.java +++ b/clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySession.java @@ -40,6 +40,7 @@ import mxaccess_gateway.v1.MxaccessGateway.WriteCommand; */ public final class MxGatewaySession implements AutoCloseable { private static final SecureRandom RANDOM = new SecureRandom(); + private static final System.Logger LOGGER = System.getLogger(MxGatewaySession.class.getName()); private final MxGatewayClient client; private final OpenSessionReply openReply; @@ -99,9 +100,26 @@ public final class MxGatewaySession implements AutoCloseable { return closeReply; } + /** + * Closes the session as part of try-with-resources. + * + *

This performs a {@code CloseSession} network RPC. Unlike + * {@link #closeRaw()}, any failure of that RPC is swallowed (and recorded + * as a suppressed exception when the JVM permits) rather than thrown: a + * close-time transport or protocol failure must not replace the exception + * that a try-with-resources body is already propagating. Callers that need + * to observe the close result should call {@link #closeRaw()} explicitly. + */ @Override public void close() { - closeRaw(); + try { + closeRaw(); + } catch (MxGatewayException error) { + LOGGER.log( + System.Logger.Level.WARNING, + () -> "ignoring close-time failure for session " + sessionId(), + error); + } } /** @@ -116,7 +134,11 @@ public final class MxGatewaySession implements AutoCloseable { if (reply.hasRegister()) { return reply.getRegister().getServerHandle(); } - return reply.getReturnValue().getInt32Value(); + if (reply.hasReturnValue()) { + return reply.getReturnValue().getInt32Value(); + } + throw new MxGatewayException( + "gateway register reply carried neither a register payload nor a return value"); } /** @@ -159,7 +181,11 @@ public final class MxGatewaySession implements AutoCloseable { if (reply.hasAddItem()) { return reply.getAddItem().getItemHandle(); } - return reply.getReturnValue().getInt32Value(); + if (reply.hasReturnValue()) { + return reply.getReturnValue().getInt32Value(); + } + throw new MxGatewayException( + "gateway addItem reply carried neither an add-item payload nor a return value"); } /** @@ -193,7 +219,11 @@ public final class MxGatewaySession implements AutoCloseable { if (reply.hasAddItem2()) { return reply.getAddItem2().getItemHandle(); } - return reply.getReturnValue().getInt32Value(); + if (reply.hasReturnValue()) { + return reply.getReturnValue().getInt32Value(); + } + throw new MxGatewayException( + "gateway addItem2 reply carried neither an add-item payload nor a return value"); } /** diff --git a/clients/java/mxgateway-client/src/test/java/com/dohertylan/mxgateway/client/MxGatewayMediumFindingsTests.java b/clients/java/mxgateway-client/src/test/java/com/dohertylan/mxgateway/client/MxGatewayMediumFindingsTests.java new file mode 100644 index 0000000..272e58e --- /dev/null +++ b/clients/java/mxgateway-client/src/test/java/com/dohertylan/mxgateway/client/MxGatewayMediumFindingsTests.java @@ -0,0 +1,394 @@ +package com.dohertylan.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; + +import io.grpc.ManagedChannel; +import io.grpc.Server; +import io.grpc.inprocess.InProcessChannelBuilder; +import io.grpc.inprocess.InProcessServerBuilder; +import io.grpc.stub.StreamObserver; +import java.time.Duration; +import java.util.UUID; +import mxaccess_gateway.v1.MxAccessGatewayGrpc; +import mxaccess_gateway.v1.MxaccessGateway.CloseSessionReply; +import mxaccess_gateway.v1.MxaccessGateway.CloseSessionRequest; +import mxaccess_gateway.v1.MxaccessGateway.MxCommandKind; +import mxaccess_gateway.v1.MxaccessGateway.MxCommandReply; +import mxaccess_gateway.v1.MxaccessGateway.MxCommandRequest; +import mxaccess_gateway.v1.MxaccessGateway.OpenSessionReply; +import mxaccess_gateway.v1.MxaccessGateway.OpenSessionRequest; +import mxaccess_gateway.v1.MxaccessGateway.ProtocolStatus; +import mxaccess_gateway.v1.MxaccessGateway.ProtocolStatusCode; +import org.junit.jupiter.api.Test; + +/** + * Regression tests for the Medium-severity Client.Java code-review findings + * (Client.Java-001 through Client.Java-005). + */ +final class MxGatewayMediumFindingsTests { + + // --- Client.Java-001: redactApiKey must not leak trailing secret chars --- + + @Test + void redactApiKeyDoesNotLeakAnyCharacterOfTheSecret() { + // mxgw__ — the secret is the segment after the second underscore. + String apiKey = "mxgw_keyid01_supersecretvalue"; + String redacted = MxGatewaySecrets.redactApiKey(apiKey); + + // None of the secret characters may appear in the redacted output. + assertFalse(redacted.contains("value"), () -> "redacted form leaked secret tail: " + redacted); + assertFalse(redacted.endsWith("alue"), () -> "redacted form leaked trailing secret chars: " + redacted); + assertFalse(redacted.contains("supersecret"), () -> "redacted form leaked secret: " + redacted); + // The non-secret key-id prefix may stay so the value is still comparable in logs. + assertTrue(redacted.startsWith("mxgw_keyid01_"), () -> "redacted form lost key-id prefix: " + redacted); + } + + @Test + void redactApiKeyForNonGatewayShapedKeyRevealsNothing() { + String redacted = MxGatewaySecrets.redactApiKey("plain-opaque-token-1234"); + assertFalse(redacted.contains("1234"), () -> "redacted form leaked trailing chars: " + redacted); + assertFalse(redacted.contains("plain-opaque-token"), () -> "redacted form leaked body: " + redacted); + } + + @Test + void redactApiKeyStillHandlesNullAndShortInput() { + assertEquals("", MxGatewaySecrets.redactApiKey(null)); + assertEquals("", MxGatewaySecrets.redactApiKey("")); + assertEquals("", MxGatewaySecrets.redactApiKey("short")); + } + + // --- Client.Java-002: terminal-state transition must be deterministic --- + + @Test + void eventStreamOverflowExceptionSurvivesASubsequentClose() { + // Deterministic reproduction of Client.Java-002: an overflow enqueues the + // overflow exception, then a later close() must NOT discard it. The first + // terminal condition (overflow) must win and stay observable by next(). + MxEventStream stream = new MxEventStream(2); + io.grpc.stub.ClientResponseObserver< + mxaccess_gateway.v1.MxaccessGateway.StreamEventsRequest, + mxaccess_gateway.v1.MxaccessGateway.MxEvent> + observer = stream.observer(); + observer.beforeStart(new NoopRequestStream()); + + // Force a queue overflow on a capacity-2 stream. + for (int i = 0; i < 8; i++) { + observer.onNext(testEvent(i)); + } + + // A close() arriving after the overflow must not erase the overflow signal. + stream.close(); + + MxGatewayException error = assertThrows(MxGatewayException.class, () -> { + while (stream.hasNext()) { + stream.next(); + } + }); + assertTrue(error.getMessage().contains("overflow"), error::getMessage); + } + + @Test + void eventStreamConcurrentOverflowAndCloseAlwaysTerminate() throws Exception { + // The terminal-state transition must be serialised: whatever the interleaving + // of overflow and close, hasNext() always reaches a terminal state. + for (int iteration = 0; iteration < 300; iteration++) { + MxEventStream stream = new MxEventStream(2); + io.grpc.stub.ClientResponseObserver< + mxaccess_gateway.v1.MxaccessGateway.StreamEventsRequest, + mxaccess_gateway.v1.MxaccessGateway.MxEvent> + observer = stream.observer(); + observer.beforeStart(new NoopRequestStream()); + + Thread filler = new Thread(() -> { + for (int i = 0; i < 8; i++) { + observer.onNext(testEvent(i)); + } + }); + Thread closer = new Thread(stream::close); + filler.start(); + closer.start(); + filler.join(); + closer.join(); + + try { + while (stream.hasNext()) { + stream.next(); + } + } catch (MxGatewayException expected) { + assertTrue(expected.getMessage().contains("overflow"), expected::getMessage); + } + assertFalse(stream.hasNext()); + } + } + + private static final class NoopRequestStream + extends io.grpc.stub.ClientCallStreamObserver { + @Override + public void cancel(String message, Throwable cause) { + } + + @Override + public boolean isReady() { + return true; + } + + @Override + public void setOnReadyHandler(Runnable onReadyHandler) { + } + + @Override + public void request(int count) { + } + + @Override + public void setMessageCompression(boolean enable) { + } + + @Override + public void disableAutoInboundFlowControl() { + } + + @Override + public void onNext(mxaccess_gateway.v1.MxaccessGateway.StreamEventsRequest value) { + } + + @Override + public void onError(Throwable t) { + } + + @Override + public void onCompleted() { + } + } + + // --- Client.Java-003: gateway protocol version mismatch must be rejected --- + + @Test + void openSessionRejectsIncompatibleGatewayProtocolVersion() throws Exception { + TestService service = new TestService() { + @Override + public void openSession(OpenSessionRequest request, StreamObserver responseObserver) { + responseObserver.onNext(OpenSessionReply.newBuilder() + .setSessionId("session-mismatch") + .setGatewayProtocolVersion(MxGatewayClientVersion.gatewayProtocolVersion() + 1) + .setProtocolStatus(ok()) + .build()); + responseObserver.onCompleted(); + } + }; + + try (Harness harness = Harness.start(service)) { + MxGatewayException error = assertThrows( + MxGatewayException.class, + () -> harness.client().openSession("junit-session")); + assertTrue(error.getMessage().contains("protocol version"), error::getMessage); + } + } + + @Test + void openSessionAcceptsMatchingOrUnsetGatewayProtocolVersion() throws Exception { + TestService matching = new TestService() { + @Override + public void openSession(OpenSessionRequest request, StreamObserver responseObserver) { + responseObserver.onNext(OpenSessionReply.newBuilder() + .setSessionId("session-ok") + .setGatewayProtocolVersion(MxGatewayClientVersion.gatewayProtocolVersion()) + .setProtocolStatus(ok()) + .build()); + responseObserver.onCompleted(); + } + }; + try (Harness harness = Harness.start(matching)) { + assertEquals("session-ok", harness.client().openSession("junit-session").sessionId()); + } + + // A gateway that leaves the field unset (0) must not be rejected — older gateways + // simply do not populate it. + TestService unset = new TestService(); + try (Harness harness = Harness.start(unset)) { + assertEquals("session-java", harness.client().openSession("junit-session").sessionId()); + } + } + + // --- Client.Java-004: missing typed payload AND missing return_value must throw --- + + @Test + void registerThrowsWhenReplyHasNeitherTypedPayloadNorReturnValue() throws Exception { + TestService service = new TestService() { + @Override + public void invoke(MxCommandRequest request, StreamObserver responseObserver) { + // Reply with neither register payload nor return_value set. + responseObserver.onNext(MxCommandReply.newBuilder() + .setSessionId(request.getSessionId()) + .setKind(request.getCommand().getKind()) + .setProtocolStatus(ok()) + .build()); + responseObserver.onCompleted(); + } + }; + + try (Harness harness = Harness.start(service)) { + MxGatewaySession session = MxGatewaySession.forSessionId(harness.client(), "s"); + MxGatewayException error = assertThrows( + MxGatewayException.class, () -> session.register("c")); + assertTrue(error.getMessage().contains("register"), error::getMessage); + } + } + + @Test + void addItemThrowsWhenReplyHasNeitherTypedPayloadNorReturnValue() throws Exception { + TestService service = new TestService() { + @Override + public void invoke(MxCommandRequest request, StreamObserver responseObserver) { + responseObserver.onNext(MxCommandReply.newBuilder() + .setSessionId(request.getSessionId()) + .setKind(request.getCommand().getKind()) + .setProtocolStatus(ok()) + .build()); + responseObserver.onCompleted(); + } + }; + + try (Harness harness = Harness.start(service)) { + MxGatewaySession session = MxGatewaySession.forSessionId(harness.client(), "s"); + assertThrows(MxGatewayException.class, () -> session.addItem(1, "Tag")); + assertThrows(MxGatewayException.class, () -> session.addItem2(1, "Tag", "ctx")); + } + } + + @Test + void addItemStillHonoursReturnValueFallback() throws Exception { + TestService service = new TestService() { + @Override + public void invoke(MxCommandRequest request, StreamObserver responseObserver) { + responseObserver.onNext(MxCommandReply.newBuilder() + .setSessionId(request.getSessionId()) + .setKind(request.getCommand().getKind()) + .setProtocolStatus(ok()) + .setReturnValue(mxaccess_gateway.v1.MxaccessGateway.MxValue.newBuilder() + .setInt32Value(99)) + .build()); + responseObserver.onCompleted(); + } + }; + + try (Harness harness = Harness.start(service)) { + MxGatewaySession session = MxGatewaySession.forSessionId(harness.client(), "s"); + assertEquals(99, session.addItem(1, "Tag")); + } + } + + // --- Client.Java-005: close() must not mask the primary try-with-resources error --- + + @Test + void closeSuppressesCloseTimeFailureInsteadOfMaskingBodyException() throws Exception { + TestService service = new TestService() { + @Override + public void closeSession(CloseSessionRequest request, StreamObserver responseObserver) { + responseObserver.onError(io.grpc.Status.UNAVAILABLE + .withDescription("WORKER_UNAVAILABLE") + .asRuntimeException()); + } + }; + + try (Harness harness = Harness.start(service)) { + IllegalStateException bodyError = assertThrows(IllegalStateException.class, () -> { + try (MxGatewaySession session = MxGatewaySession.forSessionId(harness.client(), "s")) { + throw new IllegalStateException("body failure"); + } + }); + // The body exception must propagate; the close-time RPC failure must not replace it. + assertEquals("body failure", bodyError.getMessage()); + } + } + + @Test + void closeRawStillSurfacesCloseTimeFailureForCallersWhoWantIt() throws Exception { + TestService service = new TestService() { + @Override + public void closeSession(CloseSessionRequest request, StreamObserver responseObserver) { + responseObserver.onError(io.grpc.Status.UNAVAILABLE + .withDescription("WORKER_UNAVAILABLE") + .asRuntimeException()); + } + }; + + try (Harness harness = Harness.start(service)) { + MxGatewaySession session = MxGatewaySession.forSessionId(harness.client(), "s"); + assertThrows(MxGatewayException.class, session::closeRaw); + } + } + + private static mxaccess_gateway.v1.MxaccessGateway.MxEvent testEvent(int sequence) { + return mxaccess_gateway.v1.MxaccessGateway.MxEvent.newBuilder() + .setWorkerSequence(sequence) + .build(); + } + + private static ProtocolStatus ok() { + return ProtocolStatus.newBuilder() + .setCode(ProtocolStatusCode.PROTOCOL_STATUS_CODE_OK) + .build(); + } + + private static class TestService extends MxAccessGatewayGrpc.MxAccessGatewayImplBase { + @Override + public void openSession(OpenSessionRequest request, StreamObserver responseObserver) { + responseObserver.onNext(OpenSessionReply.newBuilder() + .setSessionId("session-java") + .setProtocolStatus(ok()) + .build()); + responseObserver.onCompleted(); + } + + @Override + public void closeSession(CloseSessionRequest request, StreamObserver responseObserver) { + responseObserver.onNext(CloseSessionReply.newBuilder() + .setSessionId(request.getSessionId()) + .setProtocolStatus(ok()) + .build()); + responseObserver.onCompleted(); + } + + @Override + public void invoke(MxCommandRequest request, StreamObserver responseObserver) { + responseObserver.onNext(MxCommandReply.newBuilder() + .setSessionId(request.getSessionId()) + .setKind(MxCommandKind.MX_COMMAND_KIND_UNSPECIFIED) + .setProtocolStatus(ok()) + .build()); + responseObserver.onCompleted(); + } + } + + private record Harness(Server server, ManagedChannel channel, MxGatewayClient client) implements AutoCloseable { + static Harness start(MxAccessGatewayGrpc.MxAccessGatewayImplBase service) throws Exception { + String name = "mxgw-medium-" + UUID.randomUUID(); + Server server = InProcessServerBuilder.forName(name) + .directExecutor() + .addService(service) + .build() + .start(); + ManagedChannel channel = InProcessChannelBuilder.forName(name).directExecutor().build(); + MxGatewayClient client = new MxGatewayClient( + channel, + MxGatewayClientOptions.builder() + .endpoint("in-process") + .apiKey("") + .plaintext(true) + .callTimeout(Duration.ofSeconds(5)) + .build()); + return new Harness(server, channel, client); + } + + @Override + public void close() { + channel.shutdownNow(); + server.shutdownNow(); + } + } +} diff --git a/code-reviews/Client.Java/findings.md b/code-reviews/Client.Java/findings.md index c99afb5..a984d0c 100644 --- a/code-reviews/Client.Java/findings.md +++ b/code-reviews/Client.Java/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-18 | | Commit reviewed | `3cc53a8` | | Status | Reviewed | -| Open findings | 12 | +| Open findings | 7 | ## Checklist coverage @@ -33,13 +33,13 @@ | Severity | Medium | | Category | Security | | Location | `clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySecrets.java:30-32` | -| Status | Open | +| Status | Resolved | **Description:** `redactApiKey` preserves the leading and trailing four characters of the key. A gateway API key has the form `mxgw__`; the last four characters belong to the secret portion, so the "redacted" form leaks 4 characters of the actual secret into logs, CLI JSON output (`CommonOptions.redactedJsonMap`), and `MxGatewayClientOptions.toString()`. CLAUDE.md states API keys must never reach logs. **Recommendation:** Redact the secret entirely. Show only a stable non-secret prefix (e.g. the `mxgw__` portion) and mask everything after it, or emit a fixed `mxgw_***` form. Do not echo any trailing characters of the secret. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source: the old `substring(0,4) + stars + substring(len-4)` echoed the last four secret characters. `redactApiKey` now masks the secret entirely: for gateway-shaped keys it returns the non-secret `mxgw__` prefix followed by `***` (locating the secret separator as the first `_` after `mxgw_`); any non-gateway-shaped token returns ``. No leading/trailing secret characters are ever emitted. The pre-existing `MxGatewayCliTests.openSessionJsonRedactsApiKey` assertion that hardcoded the leaky `mxgw***********cret` form was corrected to assert the masked `mxgw_visible_***` form. Regression tests: `MxGatewayMediumFindingsTests.redactApiKeyDoesNotLeakAnyCharacterOfTheSecret`, `redactApiKeyForNonGatewayShapedKeyRevealsNothing`, `redactApiKeyStillHandlesNullAndShortInput`. ### Client.Java-002 @@ -48,13 +48,13 @@ | Severity | Medium | | Category | Concurrency & thread safety | | Location | `clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxEventStream.java:31,66-92` | -| Status | Open | +| Status | Resolved | **Description:** The `next` field is a plain (non-volatile) instance field, and `MxEventStream` exposes no thread-confinement guarantee. More concretely, a queue-overflow `offer()` and a `close()` `offer(END)` can interleave so the overflow exception is enqueued after `END` and never observed — the contract that "next() throws after overflow" is not guaranteed once `close()` has been called. **Recommendation:** Document single-consumer-thread usage explicitly in the Javadoc, and serialise terminal state transitions (overflow vs END vs close) behind a single guarded flag so the first terminal condition wins deterministically. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source: the old `offer()` END-branch did `queue.clear(); queue.offer(END)` when full, so a `close()` arriving after an overflow wiped the already-enqueued overflow exception, leaving the consumer with a clean end-of-stream and the overflow silently lost. Terminal transitions are now serialised through a single `terminate(MxGatewayException)` method guarded by a `terminated` flag and a `terminalLock`; the first terminal condition wins and a later `close()`/`END` cannot overwrite a published overflow fault. The Javadoc now explicitly documents that the iterator methods are single-consumer-only while `close()` is safe from any thread. Regression tests: `MxGatewayMediumFindingsTests.eventStreamOverflowExceptionSurvivesASubsequentClose` (deterministic) and `eventStreamConcurrentOverflowAndCloseAlwaysTerminate` (300-iteration race stress). ### Client.Java-003 @@ -63,13 +63,13 @@ | Severity | Medium | | Category | mxaccessgw conventions | | Location | `clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewayClient.java:119-140` | -| Status | Open | +| Status | Resolved | **Description:** `OpenSessionReply` carries `gateway_protocol_version` (proto field 8), and `MxGatewayClientVersion.GATEWAY_PROTOCOL_VERSION` exists so the client can reject incompatible generated-code inputs. The client never reads `reply.getGatewayProtocolVersion()` nor compares it against the compiled-in version. A client built against an older/newer contract issues commands blindly and fails with confusing downstream errors instead of a clear version-mismatch failure. **Recommendation:** In `openSession`/`openSessionRaw`, compare `reply.getGatewayProtocolVersion()` with `MxGatewayClientVersion.gatewayProtocolVersion()` and throw a typed `MxGatewayException` on mismatch. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source: neither `openSessionRaw` nor `openSessionAsync` read `getGatewayProtocolVersion()`. Added a private `ensureGatewayProtocolCompatible` helper, called from both `openSessionRaw` and `openSessionAsync`, that throws `MxGatewayException` with a clear mismatch message when the gateway reports a non-zero version differing from `MxGatewayClientVersion.gatewayProtocolVersion()`. A gateway that leaves the field unset (value 0, e.g. an older gateway) is accepted unchanged for backward compatibility. `clients/java/README.md` documents the new fail-fast check. Regression tests: `MxGatewayMediumFindingsTests.openSessionRejectsIncompatibleGatewayProtocolVersion` and `openSessionAcceptsMatchingOrUnsetGatewayProtocolVersion`. ### Client.Java-004 @@ -78,13 +78,13 @@ | Severity | Medium | | Category | Correctness & logic bugs | | Location | `clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySession.java:114-120,157-163,191-197` | -| Status | Open | +| Status | Resolved | **Description:** `register`, `addItem`, and `addItem2` check `reply.hasRegister()`/`hasAddItem()` and otherwise fall back to `reply.getReturnValue().getInt32Value()`. If the gateway returns a reply with neither the typed payload nor a `return_value` set, the method silently returns `0` — indistinguishable from a legitimate handle of 0. This masks a contract violation rather than surfacing it. **Recommendation:** If the expected typed payload is absent and no `return_value` is present, throw `MxGatewayException` (protocol violation) instead of returning `0`. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source: all three methods returned `reply.getReturnValue().getInt32Value()` (which yields `0` for an unset message field) when the typed payload was absent. Each method now guards the fallback with `reply.hasReturnValue()` and throws `MxGatewayException` describing the protocol violation when neither the typed payload nor a `return_value` is present. The legitimate `return_value` fallback is preserved. Regression tests: `MxGatewayMediumFindingsTests.registerThrowsWhenReplyHasNeitherTypedPayloadNorReturnValue`, `addItemThrowsWhenReplyHasNeitherTypedPayloadNorReturnValue`, and `addItemStillHonoursReturnValueFallback`. ### Client.Java-005 @@ -93,13 +93,13 @@ | Severity | Medium | | Category | Error handling & resilience | | Location | `clients/java/mxgateway-client/src/main/java/com/dohertylan/mxgateway/client/MxGatewaySession.java:92-105` | -| Status | Open | +| Status | Resolved | **Description:** `close()` delegates to `closeRaw()`, which performs a network RPC. When `MxGatewaySession` is used in try-with-resources and the body throws, a failure inside `closeSession` (e.g. `WORKER_UNAVAILABLE`) throws from `close()` and replaces the original exception as the propagated throwable (the body exception becomes a suppressed exception) — a known try-with-resources footgun for I/O-performing `close()`. **Recommendation:** Either make `close()` swallow/log close-time failures (keeping `closeRaw()` for callers who want the result), or document clearly that `close()` performs a network call that can throw. -**Resolution:** _(open)_ +**Resolution:** (2026-05-18) Confirmed against source: `close()` called `closeRaw()` directly, so a `CloseSession` RPC failure propagated out of try-with-resources and replaced the body exception. `close()` now catches `MxGatewayException` from `closeRaw()` and logs it at WARNING via `System.Logger` instead of rethrowing, so a close-time failure never masks the body exception. `closeRaw()` is unchanged and still throws for callers who want to observe the close result. The behavior change and the recommendation to use `closeRaw()` for explicit close handling are documented in `clients/java/README.md` and the `close()` Javadoc. Regression tests: `MxGatewayMediumFindingsTests.closeSuppressesCloseTimeFailureInsteadOfMaskingBodyException` and `closeRawStillSurfacesCloseTimeFailureForCallersWhoWantIt`. ### Client.Java-006