Reject a ws:// server when the options carry an SSL context

[scottf]

A connection with an SSL context requires TLS to every server, but a ws:// server is a plain websocket that is never upgraded, so it would be reached without TLS. This PR closes the three ways such a server can reach the connection.

Configured in the options. Options.Builder.build() throws IllegalStateException when the options end up with a context, by any route (secure, opentls, a supplied context, keystore or truststore, a context factory, or a tls, wss or opentls server url), and the server list has a ws:// server. The message says to use wss:// for that server. nats:// servers stay allowed because they are upgraded like every other server of the connection.

Advertised by the cluster in connect_urls, or supplied by a custom ServerPool. When the pool hands such a server out, both connect loops skip it before anything is sent, go on to the next server, report it through the new ErrorListener.serverRejected(Connection, String server, String reason), and tell the pool through the new ServerPool.serverRejected(NatsUri). It is not a failed attempt: no exception, no connectFailed, no reconnect delay. Both new methods are defaults that do nothing; the logger and console listeners report it at WARNING. The default pool removes the server and remembers its exact uri, so a later connect_urls does not bring it back, while the same host and port under another scheme stays a candidate. A pool that keeps supplying a rejected server does not make the loops spin: when it hands one back with nothing usable handed out in between, the connect fails naming the server and the reconnect gives up as for an exhausted pool.

Also in this PR: the permanent README note for version 2.26.5, hostname verification on by default (#1640), under Recent Version Notes, and an Important section near the top of the README that links to it.

Tests: nine new assertions in OptionsTests.testSslContextIsProvided for the build-time rule; ServerPoolTests.testServerRejected for the pool; TLSConnectTests.testDiscoveredPlainWebsocketServerIsRejectedOnce and testPlainWebsocketServerFromThePoolIsSkipped for the runtime path end to end, testConnectEndsWhenThePoolKeepsOnlyARejectedServer and testReconnectEndsWhenThePoolKeepsOnlyARejectedServer for a pool that keeps a rejected server, on SniTestServer, which can now send connect_urls entries as given.

[synadia-claude-reviewer]

Claude finished @scottf's task in 1m 4s —— View job

---

Review of PR #1641

• Fetch PR diff and description
• Examine Options.java build() logic around SSL context resolution
• Check test coverage adequacy
• Post review

Summary

Correctness check out on src/main/java/io/nats/client/Options.java:2510-2518. The new check runs after sslContext is resolved from every route (direct sslContext(), factory, keystore/truststore, secure()/opentls(), or inferred from bootstrap server schemes), so it correctly catches all the paths the PR description claims. WEBSOCKET_PROTOCOL ("ws") is matched against NatsUri.getScheme(), which is lowercased at construction time (NatsUri.java:260-261), so case-variant schemes like WS:// are still caught.

Verified against existing tests: all current uses of sslContext()/opentls()/secure() in WebsocketConnectTests, ReconnectTests, and AuthTests pair with wss:// servers, and the one ws:// test (AuthTests.testWsJWTAuthWithCredsFile) sets no context — consistent with the PR's claim that no existing test regresses.

No bugs or security issues found. The new tests in OptionsTests.testSslContextIsProvided cover the main routes (secure, opentls, direct context) combined with mixed/pure ws:// server lists, plus the negative case (nats:// + ws:// with no TLS requested, which correctly stays allowed).

Minor, non-blocking observations:

• The exception message in Options.java:2515 is a bit long/repetitive (restates all the ways a context can be required); could be trimmed, but it's clear and matches the style of the adjacent useDefaultTls && useTrustAllTls error a few lines above.
• Only ws:// + context is rejected; the runtime case where a server advertises a ws:// entry via connect_urls after connect is explicitly out of scope per the PR description.

Looks good to merge.

[scottf]

@claude re-review

[synadia-claude-reviewer]

Claude finished @scottf's task in 2m 30s —— View job

---

Re-review of PR #1641

• Fetch PR diff and description
• Review Options.java build() rejection logic
• Review NatsServerPool.java / NatsConnection.java runtime skip logic (new since last review)
• Review ErrorListener/listener plumbing
• Review test coverage
• Post review

Summary

The second commit adds the runtime half that the PR description originally said was out of scope — a ws:// entry arriving via connect_urls (or from a custom ServerPool) is now skipped rather than dialed. Worth updating the PR description, since it currently still says that case "is not in this PR."

Build-time check (Options.java:2510-2518) — unchanged from before, still correct: it runs after sslContext is fully resolved from every route, so secure(), opentls(), a direct context, keystore/truststore, a factory, and bootstrap-scheme inference are all covered.

Runtime skip (NatsConnection.java:581-592, both connectImpl and reconnectImplConnect) — logic is sound:

• rejectionReason() is checked before a server is dialed, in both the initial-connect and reconnect loops.
• The pool is told via serverPool.serverRejected(nuri) so the entry is dropped and not resupplied; loops terminate correctly as rejected entries shrink entryList (confirmed no infinite-loop risk if every candidate is rejected — the pool empties and peekNextServer()/nextServer() returns null).
• ErrorListener.serverRejected callback is wired through both built-in listener impls and plumbed into the new tests.

Minor, non-blocking observation: NatsServerPool.serverRejected() (NatsServerPool.java:298-316) removes the exact entry from entryList using NatsUri.equals() (full URI match), but records it in the rejected list and checks future discoveries against it using findEquivalent(), i.e. NatsUri.equivalent() (host:port only, scheme-agnostic). This mirrors the existing scheme-agnostic matching already used for discovery pruning elsewhere in this class, so it's consistent with the file's existing model — but it does mean that if a host:port is ever rejected under ws://, that same host:port is permanently blacklisted even if later advertised under nats:// or wss://. Unlikely in practice (different schemes typically mean different ports), and untested either way; flagging only as an edge case worth a comment or a test if it's expected to matter.

Test coverage is strong: OptionsTests.testSslContextIsProvided covers the build-time paths, ServerPoolTests.testServerRejected covers pool bookkeeping, and the two new TLSConnectTests (testDiscoveredPlainWebsocketServerIsRejectedOnce, testPlainWebsocketServerFromThePoolIsSkipped) exercise the full runtime path end-to-end with real sockets, including the "not retaken from a later INFO" behavior and that the rejection isn't surfaced as an exception.

No bugs found. Looks good to merge.

[scottf]

Addressed both items from the re-review, in 8228b54 and in the description.

• Rejected memory: NatsServerPool.serverRejected and acceptDiscoveredUrls now match the remembered uri with NatsUri.equals, the full uri, instead of findEquivalent, which compares only host and port. The scheme is what made the server unusable, so the same host and port advertised under wss:// or nats:// stays a candidate, while a re-advertised ws:// entry parses to the same uri and is still left out. ServerPoolTests.testServerRejected covers both cases.
• Description: rewritten to cover the runtime half, which the second commit added; the sentence saying it was not in this PR is gone.

[scottf]

An independent review of 8228b54 found that a custom ServerPool which implements the interface directly, and so inherits the no-op default of serverRejected, keeps handing the rejected ws:// server back, and both connect loops would spin on it: the rejection check runs before first is set, so a rejection ended nothing, and the only exits were first coming around or the pool running empty. Ending the loop while first is still null is not enough, since first can be set and then leave the pool through connectFailed.

Addressed in 390ccd1. Each loop maps a rejected server to the count of usable servers handed out when the pool last supplied it: the first rejection is reported and skipped, a repeat with something usable in between is skipped without a report, and a repeat with nothing usable in between ends the loop, so the connect fails naming the server and the reconnect gives up as for an exhausted pool. ServerPool.serverRejected's javadoc states the behavior. testConnectEndsWhenThePoolKeepsOnlyARejectedServer and testReconnectEndsWhenThePoolKeepsOnlyARejectedServer cover both loops with a pool that ignores serverRejected; the description is updated.