[vharseko]

Closes code scanning alerts #36–#39 (java/dereferenced-value-may-be-null, ServerEndPoint), #34 (java/input-resource-leak, SeparatedValuesFile) and, in a follow-up commit, #23 / #24 (java/missing-override-annotation, the two onMessage handlers in ServerEndPoint).

ServerEndPoint — a real bug behind the warnings

echoTextMessage / echoBinaryMessage wait up to ~5 s for session_upstream and then dereference it without a check, relying on the NullPointerException to reach the catch (Throwable) that closes the client. Writing a test for that path showed the close never happens: jakarta.websocket.CloseReason throws IllegalArgumentException for reason phrases longer than 123 UTF-8 bytes (checked in jakarta.websocket-api-2.0.0), and the helpful NPE message of Java 14+ (Cannot invoke "jakarta.websocket.Session.getBasicRemote()" because "this.session_upstream" is null) is longer than that. new CloseReason(...) is evaluated as the argument of close(), the exception is swallowed by the inner catch (Throwable e1) {}, and the client connection is left open with the message silently dropped. The same applies to any exception with a long message in all seven new CloseReason(..., "…: " + e) sites in the class.

This holds wherever CloseReason enforces the limit: the Eclipse jakarta.websocket-api (the unit-test classpath), Jetty 11+, Tyrus. Tomcat's own CloseReason has no length check and WsSession truncates the phrase itself, so on the openig-docker image (tomcat:11-jre25) the client was already closed and only the reason phrase is affected — except for a phrase over 123 bytes with supplementary characters that is short in UTF-16 units, where WsSession.appendCloseReasonWithTruncation throws StringIndexOutOfBoundsException and no close frame goes out.

Changes:

• upstream(): the wait loop moved into a method that reads the volatile field once per iteration and fails with IOException("upstream not connected") when the upstream is still missing/closed.
• closeReason(code, phrase): a phrase over the 123-byte limit is cut to 120 bytes and ends with … (U+2026), the marker Tomcat's WsSession puts on its own cut, so a truncated reason reads the same on every container. The cut never splits a multi-byte character or a surrogate pair. Every CloseReason in the class is now built through it.
• closeUpstream(reason) / closeAbnormally(client, phrase): null-safe close and reset of the upstream, shared by both @OnMessage handlers, @OnClose and @OnError. The upstream-side MessageHandlers still close their own onOpen session parameter directly: it is never null, and the field is assigned only after connectToServer() returns, while the handlers are registered in onOpen.
• @OnMessage handlers call authorize() before waiting for the upstream: a revoked client is closed with access revoked, as before, and no longer waits ~5 s for a missing upstream first.
• @OnError closes the upstream before the client. On Tomcat close() runs @OnClose on the calling thread, so end() used to close the upstream with NORMAL_CLOSURE first and the client error: … reason never reached it. Now the upstream gets CLOSED_ABNORMALLY (sent by Tomcat as 1002), and its onClose closes the client with the same reason.
• session_upstream is volatile (written in @OnOpen, read from @OnMessage on other threads).

Tests (ServerEndPointTest, new):

• text and binary message without an upstream → client closed with CLOSED_ABNORMALLY / "upstream not connected". Before the fix both failed with Wanted but not invoked: session.close(...) — zero interactions with the client session.
• shouldCloseStaleUpstreamWhenItIsNotOpen: an upstream that is set but no longer open → client closed with "upstream not connected", the stale session closed and the field cleared.
• shouldTruncateLongUpstreamErrorWhenClosingClient: the upstream fails with a 200-char IOException → the client close phrase is cut to 123 bytes ending in …, and the same CloseReason closes the upstream (pins the closeAbnormally() → closeReason() routing; no wait).
• shouldCloseRevokedClientWithoutWaitingForUpstreamOnTextMessage / …OnBinaryMessage: authorize() returns 403 and there is no upstream → client closed with exactly "client message error: access revoked" (no wait).
• shouldCloseUpstreamWithClientErrorOnError: client.close() runs end() as Tomcat does → the upstream is closed once, with CLOSED_ABNORMALLY / "client error: java.io.IOException: broken pipe".
• shouldTruncateCloseReasonPhraseToProtocolLimit: ASCII 123 untouched, 500 → 120 + …; Cyrillic 200 → 60 + … (123 bytes); "x" + U+1F600 ×100 → "x" + U+1F600 ×29 + … and "a" ×119 + U+1F600 ×5 → "a" ×119 + … (no lone high surrogate at the cut); a lone \uD83D and "" → ""; short phrases untouched.

The three tests that go through the wait (no upstream ×2, stale upstream) each take the real ~5 s (about +18 s on the openig-core run); I preferred that over adding a test-only hook to shorten the timeout in production code.

ServerEndPoint — #23 / #24

@Override added to onMessage of the two anonymous MessageHandler.Whole classes (second commit, 1a122ed1). Folded in here rather than into #180 to keep the file in a single open PR.

SeparatedValuesFile (#34)

SeparatedValuesReader now implements Closeable (it already had close()), and getRecord() opens it in try-with-resources instead of try/finally, with the underlying FileInputStream as a resource of its own so that it is closed even if the InputStreamReader constructor throws. Behavior-preserving; covered by SeparatedValuesFileTest (new), which reads records from a real CSV file. FileAttributesFilterTest mocks SeparatedValuesFile and does not run this code.

[maximthomas]

praise: The fix sits where the bug is, and the new tests prove it.

• upstream() turns the silent NPE into IOException("upstream not connected"); both integration tests fail at BASE (the 152-byte NPE phrase makes the Eclipse CloseReason throw, so client.close is never called) and pass at HEAD.
• Every CloseReason in the class goes through closeReason(), including TRY_AGAIN_LATER in start(), where at BASE an IllegalArgumentException would have escaped catch (IOException e1).
• SeparatedValuesReader implements Closeable is enough for CodeQL: alert #34 has no instance on the PR merge ref.

---

issue (non-blocking): closeReason() can split a surrogate pair and return a phrase that ends in a lone high surrogate.

openig-core/src/main/java/org/openidentityplatform/openig/websocket/ServerEndPoint.java:56-59

Line 56 cuts at 123 chars and the loop drops one UTF-16 unit at a time. getBytes(UTF_8) counts a lone surrogate as one byte ('?'), so the loop can stop with the high half still attached. Measured: "x" + U+1F600 x100 gives 62 chars / 122 bytes ending in \uD83D, and "a" x122 + U+1F600 x5 splits at line 56 alone. The close still goes out (Tomcat encodes the phrase with String.getBytes, so the client sees a trailing ?), but this contradicts the Javadoc, the test comment "never split a character" and the description.

static CloseReason closeReason(CloseReason.CloseCode code, String phrase) {
	String reason = phrase.length() > MAX_REASON_PHRASE_BYTES ? phrase.substring(0, MAX_REASON_PHRASE_BYTES) : phrase;
	while (reason.getBytes(StandardCharsets.UTF_8).length > MAX_REASON_PHRASE_BYTES
			|| (!reason.isEmpty() && Character.isHighSurrogate(reason.charAt(reason.length() - 1)))) {
		reason = reason.substring(0, reason.length() - 1);
	}
	return new CloseReason(code, reason);
}

Pin, in shouldTruncateCloseReasonPhraseToProtocolLimit: closeReason(CLOSED_ABNORMALLY, "x" + "😀".repeat(100)).getReasonPhrase() is "x" + "😀".repeat(30). It is red at HEAD, which returns the same string plus a lone \uD83D.

---

suggestion (non-blocking): No test sends a phrase over 123 bytes through closeAbnormally(), so the closeReason() routing has no pin.

openig-core/src/test/java/org/openidentityplatform/openig/websocket/ServerEndPointTest.java:51-71, openig-core/src/main/java/org/openidentityplatform/openig/websocket/ServerEndPoint.java:89

Both integration tests close with "client message error: java.io.IOException: upstream not connected" (65 bytes), which closeReason() returns unchanged. If closeReason(...) at line 89 is replaced by new CloseReason(...), every test stays green, and that change brings back the long-message failure this PR fixes. The case below covers it and needs no 5 s wait. With that mutant, closeAbnormally throws IllegalArgumentException out of the catch block and the test fails. Deleting closeUpstream(reason) from closeAbnormally also turns it red.

@Test
public void shouldTruncateLongUpstreamErrorWhenClosingClient() throws Exception {
    Session upstream = mock(Session.class);
    RemoteEndpoint.Basic remote = mock(RemoteEndpoint.Basic.class);
    when(upstream.isOpen()).thenReturn(true);
    when(upstream.getBasicRemote()).thenReturn(remote);
    doThrow(new IOException("x".repeat(200))).when(remote).sendText("hello", true);
    endpoint.session_upstream = upstream;

    endpoint.echoTextMessage(client, "hello", true);

    CloseReason reason = closeReasonOf(client);
    assertThat(reason.getReasonPhrase().getBytes(StandardCharsets.UTF_8).length).isLessThanOrEqualTo(123);
    verify(upstream).close(reason);
}

---

suggestion (non-blocking): Every test runs with session_upstream == null, so the one changed outcome for a non-null upstream has no test.

openig-core/src/test/java/org/openidentityplatform/openig/websocket/ServerEndPointTest.java:48

Take an upstream that is set but already closed. BASE calls getBasicRemote() on it, gets an NPE and leaves the client open. HEAD throws "upstream not connected", closes the stale session and clears the field. start(), end() and onError() are not called by any test either; for end()/onError() with a null upstream, BASE behaves the same, so they need no pin. The case below is red at BASE and takes the same ~5 s wait as the existing two.

@Test
public void shouldCloseStaleUpstreamWhenItIsNotOpen() throws Exception {
    Session upstream = mock(Session.class);
    when(upstream.isOpen()).thenReturn(false);
    endpoint.session_upstream = upstream;

    endpoint.echoTextMessage(client, "hello", true);

    assertThat(closeReasonOf(client).getReasonPhrase()).contains("upstream not connected");
    verify(upstream).close(any(CloseReason.class));
    assertThat(endpoint.session_upstream).isNull();
}

---

suggestion (non-blocking): The wait loop in upstream() still reads the volatile field twice.

openig-core/src/main/java/org/openidentityplatform/openig/websocket/ServerEndPoint.java:66-69

session_upstream==null || !session_upstream.isOpen() can throw an NPE if closeUpstream() runs between the two reads. Nothing observable follows on Tomcat, because every path that clears the field closes the client first. Still, this is the java/dereferenced-value-may-be-null pattern the PR removes elsewhere, and the snapshot is already there one line later.

int ct=0;
Session upstream=session_upstream;
while ((ct++<5000)&&(upstream==null || !upstream.isOpen())) {
	Thread.sleep(1);
	upstream=session_upstream;
}
if (upstream==null || !upstream.isOpen()) {
	throw new IOException("upstream not connected");
}
return upstream;

---

suggestion (non-blocking): getRecord() still creates the FileInputStream inside the reader argument, so it is not closed if the InputStreamReader constructor throws.

openig-core/src/main/java/org/forgerock/openig/text/SeparatedValuesFile.java:152-154

A null charset passed through the public SeparatedValuesFile(File, Charset, ...) constructors makes new InputStreamReader(stream, null) throw after the file is open. The same happens at BASE, and the only in-repo caller (FileAttributesFilter.java:134) never passes null.

try (FileInputStream stream = new FileInputStream(file);
        SeparatedValuesReader reader = new SeparatedValuesReader(new InputStreamReader(stream, charset), separator)) {

---

nitpick (non-blocking): The description says the client is always left open at BASE; that holds only where CloseReason enforces the 123-byte limit.

openig-core/src/main/java/org/openidentityplatform/openig/websocket/ServerEndPoint.java:51

The websocket API is provided. Tomcat 11's CloseReason(CloseCode, String) has no length check, and WsSession truncates the phrase itself, so on the openig-docker image (tomcat:11-jre25) BASE already closed the client and only the phrase changes. The bug reproduces with the Eclipse API: the unit-test classpath, Jetty 11+ and Tyrus. The fix is right everywhere; only the stated production impact needs that qualifier.

---

nitpick (non-blocking): The upstream MessageHandler catches still close the upstream inline, not through closeUpstream().

openig-core/src/main/java/org/openidentityplatform/openig/websocket/ServerEndPoint.java:155-157, :177-179

The description says upstream sessions are closed "null-safely in one place". These two sites close the onOpen parameter that shadows the field. The outcome is the same: the parameter is never null, and session_client.close(reason) runs end() -> closeUpstream() first on Tomcat. Either reword the description or mention these two sites.

---

note (non-blocking): Description vs code, no reason stated in the PR.

• ServerEndPoint.java: "without splitting multi-byte characters" (see the closeReason() issue); "null-safely in one place" (upstream handler catches, double read in upstream()); "the client connection is left open" (Eclipse CloseReason only).

[vharseko]

All points taken; the round is in 1ffac86 (the branch was rebased onto the current master first).

• closeReason() surrogate split — fixed with the loop you proposed, plus a comment on why the byte check alone misses a lone high surrogate. Both of your cases are pinned in shouldTruncateCloseReasonPhraseToProtocolLimit: "x" + U+1F600 ×100 → "x" + U+1F600 ×30, and "a" ×122 + U+1F600 ×5 → "a" ×122.
• closeAbnormally() routing — added shouldTruncateLongUpstreamErrorWhenClosingClient as proposed, with an extra assert that the field is cleared.
• Stale upstream — added shouldCloseStaleUpstreamWhenItIsNotOpen as proposed.
• Double volatile read in upstream() — the loop now works on a local snapshot, re-read once per iteration.
• FileInputStream in the reader argument — now a resource of its own in the same try-with-resources.
• Tomcat vs Eclipse CloseReason — confirmed against the bytecode: Tomcat 11.0.20 CloseReason just stores the phrase and WsSession.appendCloseReasonWithTruncation cuts it. The description now says where the client was left open and that on tomcat:11-jre25 only the phrase was affected.
• Upstream handler catches — kept as they are; the description now says why: they close their own onOpen session parameter, which is never null, while the field is assigned only after connectToServer() returns.

Each new check was confirmed red against its mutant (no surrogate check in closeReason(); new CloseReason(...) in closeAbnormally(); upstream() ignoring isOpen()). ServerEndPointTest + FileAttributesFilterTest: 7/7 green.

[maximthomas]

praise: Every round-1 point is in, and each fix comes with a test.

• closeReason() now stops on a trailing high surrogate (ServerEndPoint.java:61-63), and both emoji cases are pinned in shouldTruncateCloseReasonPhraseToProtocolLimit. shouldTruncateLongUpstreamErrorWhenClosingClient pins the closeAbnormally() → closeReason() routing.
• upstream() re-reads a local snapshot (ServerEndPoint.java:71-75), and getRecord() holds the FileInputStream as its own resource (SeparatedValuesFile.java:152-155).
• The pre-cut also fixes Tomcat. At BASE, Tomcat's WsSession.appendCloseReasonWithTruncation throws StringIndexOutOfBoundsException for a phrase over 123 bytes that has supplementary characters and at most 120 UTF-16 units, so no close frame went out (reflection probe on Tomcat 11.0.7/11.0.20). HEAD sends valid UTF-8 of 123 bytes or less.

---

issue (non-blocking): No test runs the rewritten getRecord(). FileAttributesFilterTest mocks SeparatedValuesFile.

openig-core/src/main/java/org/forgerock/openig/text/SeparatedValuesFile.java:150-187, openig-core/src/test/java/org/forgerock/openig/filter/FileAttributesFilterTest.java:40-41, :51, :66

The description says the change is "covered by FileAttributesFilterTest". That test declares @Mock private SeparatedValuesFile file and stubs getRecord(). No other test or route config references SeparatedValuesFile, SeparatedValuesReader or FileAttributesFilter. So nothing in CI runs the new try-with-resources, the header read or the key lookup: break the lookup and every test stays green.

package org.forgerock.openig.text;

import static java.nio.charset.StandardCharsets.UTF_8;
import static org.assertj.core.api.Assertions.assertThat;

import java.io.File;
import java.nio.file.Files;

import org.testng.annotations.Test;

@SuppressWarnings("javadoc")
public class SeparatedValuesFileTest {

    @Test
    public void shouldReadRecordFromFile() throws Exception {
        File csv = File.createTempFile("users", ".csv");
        csv.deleteOnExit();
        Files.write(csv.toPath(), "username,password\njoe,secret\nbob,hunter2\n".getBytes(UTF_8));

        SeparatedValuesFile file = new SeparatedValuesFile(csv);

        assertThat(file.getRecord("username", "bob")).containsEntry("password", "hunter2");
        assertThat(file.getRecord("username", "nobody")).isNull();
        assertThat(file.getRecord("missing", "joe")).isNull();
    }
}

Pin: this runs the code inside the new try block. It cannot see whether the FileInputStream gets closed. Or: drop "covered by FileAttributesFilterTest" from the description.

---

question (non-blocking): Is it intended that a cut close reason no longer ends with Tomcat's … marker on the shipped image?

openig-core/src/main/java/org/openidentityplatform/openig/websocket/ServerEndPoint.java:58-66, used at :95, :157, :179, :205, :222

Tomcat 11's WsSession.appendCloseReasonWithTruncation writes a phrase of 123 bytes or less as it is. It cuts only longer phrases, to 120 bytes plus U+2026 (javap, 11.0.7). closeReason() now cuts to 123 bytes first, so Tomcat never adds the marker. Example: upstream down: jakarta.websocket.DeploymentException: The HTTP request to initiate the WebSocket connection to [ws://backend:8080/ws] failed (140 B) used to go out as 120 B + … on tomcat:11-jre25, and now goes out as a bare 123-byte cut. A client or log reader can no longer tell a cut reason from a complete one. Close codes are unchanged and nothing breaks. If the bare cut is intended, nothing changes. If the marker should stay, here is a version that keeps it on every container:

static CloseReason closeReason(CloseReason.CloseCode code, String phrase) {
	if (phrase.getBytes(StandardCharsets.UTF_8).length <= MAX_REASON_PHRASE_BYTES) {
		return new CloseReason(code, truncate(phrase, MAX_REASON_PHRASE_BYTES));
	}
	// mark the cut as Tomcat does: at most 120 bytes, then U+2026 (3 bytes in UTF-8)
	return new CloseReason(code, truncate(phrase, MAX_REASON_PHRASE_BYTES - 3) + "…");
}

private static String truncate(String phrase, int maxBytes) {
	String reason = phrase.length() > maxBytes ? phrase.substring(0, maxBytes) : phrase;
	// a lone high surrogate is encoded as a single '?' byte, so the length check alone does not catch it
	while (reason.getBytes(StandardCharsets.UTF_8).length > maxBytes
			|| (!reason.isEmpty() && Character.isHighSurrogate(reason.charAt(reason.length() - 1)))) {
		reason = reason.substring(0, reason.length() - 1);
	}
	return reason;
}

Pin: in shouldTruncateCloseReasonPhraseToProtocolLimit, the expected results become "x"×120 + …, Cyrillic ×60 + … (123 bytes), "x" + U+1F600 ×29 + …, and "a"×120 + …. "short" stays as it is. In shouldTruncateLongUpstreamErrorWhenClosingClient, startsWith(...) and hasSize(123) still hold.

---

question (non-blocking): Is it intended that the echo handlers no longer call authorize() when upstream() gives up?

openig-core/src/main/java/org/openidentityplatform/openig/websocket/ServerEndPoint.java:231-232, :245-246

At BASE the wait loop ended, authorize() ran, and then came the NPE. At HEAD, upstream() throws before authorize(). Take a client whose upstream is gone and whose access was revoked: Principal.authorize() re-runs the gateway route once its TTL (180 s by default) has expired. At BASE that client was closed with client message error: access revoked. Now it gets … upstream not connected, and the route call is skipped. Both versions close with the same code and forward nothing. No test pins either order, because setUp stubs authorize() and nothing verifies it.

// if the HEAD order is intended — in shouldCloseClientWithClearReasonWhenUpstreamIsNotConnectedOnTextMessage
// (import static org.mockito.Mockito.never;)
verify(endpoint.principal, never()).authorize();

Pin: this assertion fails if authorize() moves back before the upstream check. Or, if you want BASE's order back, put authorize(); before final Session upstream=upstream(); at both lines. A revoked client is then closed with access revoked and skips the 5 s wait. The pin then becomes verify(endpoint.principal).authorize();.

---

suggestion (non-blocking): onError() closes the client before the upstream, so on Tomcat the upstream never gets the client error: … reason.

openig-core/src/main/java/org/openidentityplatform/openig/websocket/ServerEndPoint.java:216-222

Tomcat's WsSession.doClose fires @OnClose on the calling thread. So session_client.close() first runs end(), which closes the upstream with NORMAL_CLOSURE and clears the field. The closeUpstream(closeReason(..., "client error: " + t)) at :222 then finds the field null. This was the same at BASE. If the upstream should learn that the client failed:

@OnError
public void onError(Session session_client,Throwable t) throws Throwable {
	closeUpstream(closeReason(CloseReason.CloseCodes.CLOSED_ABNORMALLY,"client error: "+t.toString()));
	try {
		session_client.close();
	} catch (Throwable e1) {}
}

On the wire, Tomcat then sends 1002 (PROTOCOL_ERROR) to the upstream instead of 1000, because sendCloseMessage rewrites 1006 to 1002. If 1000 is the signal you want, leave it.

---

nitpick (non-blocking): No test reaches the !reason.isEmpty() guard in closeReason().

openig-core/src/main/java/org/openidentityplatform/openig/websocket/ServerEndPoint.java:62

None of the test phrases is empty or ends up empty, so dropping the guard leaves every test green. Without the guard, a phrase made only of a high surrogate would throw StringIndexOutOfBoundsException from charAt(-1).

assertThat(ServerEndPoint.closeReason(CloseReason.CloseCodes.CLOSED_ABNORMALLY, "\uD83D").getReasonPhrase()).isEmpty();

[vharseko]

All five points taken; the round is in d1a4ade (the branch was rebased onto the current master first).

• getRecord() has no test — added SeparatedValuesFileTest as proposed, on Files.createTempFile and deleted in finally. The description now names it instead of FileAttributesFilterTest. As you say, it runs the code inside the try block but cannot see whether the FileInputStream gets closed.
• Tomcat's … marker — kept on every container: closeReason() cuts a phrase over 123 bytes to 120 bytes and appends U+2026, with the cut moved into truncate() as proposed. The expectations in shouldTruncateCloseReasonPhraseToProtocolLimit are updated, plus the 123-byte boundary ("x" ×123 untouched). With the 120-byte cut, "a" ×122 + U+1F600 ×5 no longer reaches the pair, so the cut-through-a-pair case is now "a" ×119 + U+1F600 ×5 → "a" ×119 + …. shouldTruncateLongUpstreamErrorWhenClosingClient also checks the trailing ….
• authorize() order — authorize() now runs before upstream() in both handlers: a revoked client is closed with access revoked, as at BASE, and no longer waits ~5 s for a missing upstream. Pinned with shouldCloseRevokedClientWithoutWaitingForUpstreamOnTextMessage / …OnBinaryMessage: authorize() returns 403, there is no upstream, and the phrase must be exactly client message error: access revoked. I used that instead of verify(principal).authorize(), which passes in either order.
• onError() order — the upstream is closed first. shouldCloseUpstreamWithClientErrorOnError mimics Tomcat by running end() from inside client.close() and expects the upstream to be closed once, with CLOSED_ABNORMALLY / client error: java.io.IOException: broken pipe. One more effect on Tomcat: closing the upstream fires its onClose, which closes the client with the same reason, so the client now gets client error: … as well (1002 on the wire) instead of 1000; the explicit close() that follows is then a no-op.
• !reason.isEmpty() guard — pinned with a lone \uD83D → "" and "" → ""; both throw StringIndexOutOfBoundsException without the guard.

Each new check was confirmed red against its mutant: the key lookup in getRecord() inverted; no surrogate check in truncate(); no !reason.isEmpty() guard; no … on the cut; upstream() before authorize() in the text and, separately, the binary handler; onError() closing the client first. ServerEndPointTest + SeparatedValuesFileTest + FileAttributesFilterTest: 11/11 green.

[maximthomas]

praise: Every round-2 point is in the code, and the ones about behaviour have tests.

• closeReason() now cuts at 120 bytes and appends U+2026, the same way Tomcat's WsSession does (ServerEndPoint.java:58-64). The empty-phrase guard is pinned (ServerEndPointTest.java:171-172).
• authorize() runs before upstream(), pinned by shouldCloseRevokedClientWithoutWaitingForUpstreamOnTextMessage and …OnBinaryMessage. onError() closes the upstream first (ServerEndPoint.java:229), and shouldCloseUpstreamWithClientErrorOnError fails if that order is reverted.
• SeparatedValuesFileTest runs the real getRecord() against a CSV file.