fix #156 Instead of pipeline.fireChannelRead(...), forward PacketsMessage from the context of the HTTP/WebSocket codec#158
Open
NeatGuyCoding wants to merge 5 commits intomainfrom
Open
fix #156 Instead of pipeline.fireChannelRead(...), forward PacketsMessage from the context of the HTTP/WebSocket codec#158NeatGuyCoding wants to merge 5 commits intomainfrom
NeatGuyCoding wants to merge 5 commits intomainfrom
Conversation
…age from the context of the HTTP/WebSocket codec. Signed-off-by: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com>
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughMigrates TLS from JDK SSLContext to Netty SslContext with provider selection and optional tcnative; routes PacketsMessage via discovered Netty codec contexts in polling/websocket transports; adds end-to-end Java WSS/WebSocket tests and bumps example Netty version and dependency management for netty-tcnative-boringssl-static. Changes
Sequence DiagramssequenceDiagram
participant App as Application
participant CI as SocketIOChannelInitializer
participant Builder as SslContextBuilder
participant OpenSSL as OpenSSL Provider
participant JDK as JDK Provider
participant SslC as SslContext
App->>CI: createSSLContext(kmf, socketSslConfig)
CI->>Builder: SslContextBuilder.forServer(kmf)
alt OpenSSL available
Builder->>OpenSSL: sslProvider(OPENSSL)
OpenSSL-->>Builder: configured
else OpenSSL not available
Builder->>JDK: sslProvider(JDK)
JDK-->>Builder: configured
end
Builder->>Builder: apply protocols & trustManager(if any)
Builder-->>CI: build() -> SslContext
CI-->>App: sslContext set on pipeline (newEngine(...))
sequenceDiagram
participant Transport as Polling/WebSocket Transport
participant Pipeline as Netty Pipeline
participant Codec as Codec Handler
participant PacketH as Packet Handler
Transport->>Pipeline: create PacketsMessage
Transport->>Pipeline: lookup codec handler context (HttpRequestDecoder / WebSocket13FrameDecoder / HttpServerCodec)
alt codec context found
Transport->>Codec: codecCtx.fireChannelRead(packetsMessage)
Codec->>PacketH: deliver packetsMessage
else not found
Transport->>Pipeline: pipeline.fireChannelRead(packetsMessage)
Pipeline->>PacketH: deliver packetsMessage
end
PacketH-->>Transport: processed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can disable poems in the walkthrough.Disable the |
Collaborator
Author
|
@sanjomo kindly review please |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...socketio-core/src/test/java/com/socketio4j/socketio/transport/SocketIoJavaClientSslTest.java
Dismissed
Show dismissed
Hide dismissed
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses #156 by changing how PacketsMessage is forwarded within the Netty pipeline to avoid re-entering from the pipeline head (and re-traversing SSL / HTTP / WebSocket codec handlers), which can break mixed polling+websocket transport flows.
Changes:
- Forward
PacketsMessagevia the codec handler’sChannelHandlerContext(with fallbacks) instead ofpipeline.fireChannelRead(...)from the pipeline head. - Update SSL handling to build a Netty
SslContext(optionally using OpenSSL via tcnative) and add an end-to-end Java socket.io-client SSL test + test keystore. - Add additional WebSocket frame handling (ping/pong) and align handshake extension enablement with
configuration.isWebsocketCompression().
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
pom.xml |
Adds managed Netty tcnative version + dependency management entry for netty-tcnative-boringssl-static. |
netty-socketio-core/pom.xml |
Adds optional dependency on netty-tcnative-boringssl-static for core. |
netty-socketio-examples/netty-socketio-examples-spring-boot-base/pom.xml |
Aligns example module Netty version with the parent. |
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java |
Switches from JDK SSLContext to Netty SslContext + OpenSSL selection and truststore integration. |
netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java |
Fires PacketsMessage from codec context, adds ping/pong handling, and tweaks WebSocket handshake extension flag. |
netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/PollingTransport.java |
Fires PacketsMessage from codec context in POST handling (mirroring the websocket path). |
netty-socketio-core/src/test/java/com/socketio4j/socketio/transport/SocketIoJavaClientSslTest.java |
Adds an end-to-end Java socket.io-client test over WS and WSS (with ack). |
netty-socketio-core/src/test/resources/ssl/test-socketio.p12 |
Adds a PKCS12 keystore used by the new SSL integration test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java
Show resolved
Hide resolved
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java
Show resolved
Hide resolved
netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java
Show resolved
Hide resolved
netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/PollingTransport.java
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java (1)
176-189: Extract the codec-context lookup into one helper.The
HttpRequestDecoder→WebSocket13FrameDecoder→HttpServerCodecresolution now exists here and inPollingTransport.onPost(...); centralizing it would keep future pipeline fixes in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java` around lines 176 - 189, Extract the repeated pipeline codec-context lookup into a single helper method (e.g., findCodecContext) and replace the inline resolution in firePacketsMessageToPacketHandler and PollingTransport.onPost with a call to that helper; the helper should accept a ChannelHandlerContext, attempt ctx.pipeline().context(HttpRequestDecoder.class) then WebSocket13FrameDecoder.class then HttpServerCodec.class, and return the found ChannelHandlerContext (or null) so callers can fireChannelRead on the returned context or fall back to ctx.pipeline().fireChannelRead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@netty-socketio-core/src/test/java/com/socketio4j/socketio/transport/SocketIoJavaClientSslTest.java`:
- Around line 227-231: The test's allocatePort() creates a race by
opening/closing a ServerSocket on port 0; instead have the server bind to port 0
directly and read back the assigned port after startup. Replace uses of
allocatePort() with creating the SocketIOServer/Configuration using port 0, call
server.start() (or equivalent) and then read the actual bound port via the
server's accessor (e.g., SocketIOServer.getPort() /
getConfiguration().getPort()) to use in client connections; if server API
doesn't expose the bound port, keep the ServerSocket open until the
SocketIOServer binds to avoid the race.
- Around line 98-137: Current tests force opts.transports = {"websocket"} so
they never exercise the polling→websocket upgrade or the PollingTransport.onPost
forwarding path (regression `#156`); add a mixed-transport test (or modify an
existing test in SocketIoJavaClientSslTest) that sets opts.transports = new
String[] { "polling", "websocket" } (or leaves it default), then have the server
send a packet immediately after connect (server-initiated send) to trigger the
polling path and upgrade sequence, and assert the client does not disconnect
(reuse existing latches like serverReceivedHello/clientReceivedAck or add a
latch) to verify the polling->websocket upgrade and PollingTransport.onPost
behavior are exercised.
- Around line 75-95: The test currently creates a blind X509TrustManager and
disables hostname verification in SocketIoJavaClientSslTest, which bypasses real
TLS validation; replace that by loading the test truststore (the fixed keystore
resource already used elsewhere), create a TrustManagerFactory from that
KeyStore to obtain the proper X509TrustManager, initialize SSLContext with those
trust managers (use SSLContext.init(null, trustManagers, new SecureRandom())),
pass the real X509TrustManager to
OkHttpClient.Builder.sslSocketFactory(sslContext.getSocketFactory(),
realTrustManager), and remove the hostnameVerifier override so the client
actually verifies the server certificate and hostname.
---
Nitpick comments:
In
`@netty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.java`:
- Around line 176-189: Extract the repeated pipeline codec-context lookup into a
single helper method (e.g., findCodecContext) and replace the inline resolution
in firePacketsMessageToPacketHandler and PollingTransport.onPost with a call to
that helper; the helper should accept a ChannelHandlerContext, attempt
ctx.pipeline().context(HttpRequestDecoder.class) then
WebSocket13FrameDecoder.class then HttpServerCodec.class, and return the found
ChannelHandlerContext (or null) so callers can fireChannelRead on the returned
context or fall back to ctx.pipeline().fireChannelRead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5e35b761-a7c7-49aa-9836-24a719cee537
📒 Files selected for processing (8)
netty-socketio-core/pom.xmlnetty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/transport/PollingTransport.javanetty-socketio-core/src/main/java/com/socketio4j/socketio/transport/WebSocketTransport.javanetty-socketio-core/src/test/java/com/socketio4j/socketio/transport/SocketIoJavaClientSslTest.javanetty-socketio-core/src/test/resources/ssl/test-socketio.p12netty-socketio-examples/netty-socketio-examples-spring-boot-base/pom.xmlpom.xml
...socketio-core/src/test/java/com/socketio4j/socketio/transport/SocketIoJavaClientSslTest.java
Show resolved
Hide resolved
...socketio-core/src/test/java/com/socketio4j/socketio/transport/SocketIoJavaClientSslTest.java
Show resolved
Hide resolved
...socketio-core/src/test/java/com/socketio4j/socketio/transport/SocketIoJavaClientSslTest.java
Outdated
Show resolved
Hide resolved
Signed-off-by: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com>
Signed-off-by: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java (1)
219-225: Consider warning about deprecated TLS versions (TLSv1, TLSv1.1).The validation accepts "TLSv1" and "TLSv1.1" which are deprecated per RFC 8996. Given that
SocketSslConfigdefaults to"TLSv1", users relying on defaults may encounter connection failures with modern clients.♻️ Proposed enhancement to warn about deprecated protocols
if (sslProtocol != null) { // SocketSslConfig historically accepted SSLContext algorithm names like "TLS". // SslContextBuilder.protocols(...) expects concrete enabled protocol versions. if (isTlsProtocolVersion(sslProtocol)) { + if ("TLSv1".equals(sslProtocol) || "TLSv1.1".equals(sslProtocol)) { + log.warn("SocketSslConfig.sslProtocol='{}' is deprecated (RFC 8996). " + + "Consider upgrading to 'TLSv1.2' or 'TLSv1.3'.", sslProtocol); + } builder.protocols(sslProtocol); } else {Consider also updating the default in
SocketSslConfigfrom"TLSv1"to"TLSv1.2"in a separate change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java` around lines 219 - 225, The current protocol validation in isTlsProtocolVersion accepts deprecated values like "TLSv1" and "TLSv1.1" but does not warn users; update the branch in SocketIOChannelInitializer (where you call builder.protocols(sslProtocol)) to detect these deprecated strings and emit a warning (e.g., via log.warn) that the chosen protocol is deprecated per RFC 8996 and may cause modern client failures, while still allowing the value; reference the isTlsProtocolVersion helper and SocketSslConfig.sslProtocol to locate the check, and note in the log that changing the default to "TLSv1.2" is recommended (implement the default change in a separate PR).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java`:
- Around line 237-244: The isTlsProtocolVersion method's regex currently accepts
"TLSv1.0"; update the pattern used in isTlsProtocolVersion to only allow "TLSv1"
or "TLSv1.1", "TLSv1.2", "TLSv1.3" (i.e., disallow a ".0" suffix). Replace the
existing regex in isTlsProtocolVersion with one that permits either no minor
version or a minor of 1, 2, or 3 (for example using a grouped alternative for
1|2|3).
---
Nitpick comments:
In
`@netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java`:
- Around line 219-225: The current protocol validation in isTlsProtocolVersion
accepts deprecated values like "TLSv1" and "TLSv1.1" but does not warn users;
update the branch in SocketIOChannelInitializer (where you call
builder.protocols(sslProtocol)) to detect these deprecated strings and emit a
warning (e.g., via log.warn) that the chosen protocol is deprecated per RFC 8996
and may cause modern client failures, while still allowing the value; reference
the isTlsProtocolVersion helper and SocketSslConfig.sslProtocol to locate the
check, and note in the log that changing the default to "TLSv1.2" is recommended
(implement the default change in a separate PR).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: caa3996c-b214-4246-b168-6ca0fecbd770
📒 Files selected for processing (1)
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java
netty-socketio-core/src/main/java/com/socketio4j/socketio/SocketIOChannelInitializer.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
pom.xml:86
- PR title/description focus on forwarding
PacketsMessage, but this PR also introduces a Netty-native TLS provider selection (OpenSSL/JDK) and adds a newnetty-tcnative-boringssl-staticdependency. Consider updating the PR description (and/or splitting into a separate PR) so reviewers and release notes clearly capture the TLS/runtime dependency changes.
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
<testcontainers.version>2.0.3</testcontainers.version>
<netty.version>4.2.9.Final</netty.version>
<netty.tcnative.version>2.0.74.Final</netty.tcnative.version>
<jmockit.version>1.50</jmockit.version>
<byte-buddy.version>1.18.4</byte-buddy.version>
<junit.version>6.0.2</junit.version>
<junit-platform-launcher.version>6.0.2</junit-platform-launcher.version>
<slf4j.version>2.0.17</slf4j.version>
<jackson.version>2.20.1</jackson.version>
<redisson.version>4.1.0</redisson.version>
<hazelcast.version>5.2.5</hazelcast.version>
<awaitility.version>4.3.0</awaitility.version>
<assertj.version>3.27.6</assertj.version>
<mockito.version>5.21.0</mockito.version>
<logback.version>1.5.25</logback.version>
<socketio.version>2.1.2</socketio.version>
<javafaker.version>1.0.2</javafaker.version>
<kafka.version>4.1.1</kafka.version>
<commons-lang3.version>3.20.0</commons-lang3.version>
<nats.version>2.25.1</nats.version>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+206
to
+240
| private SslContext createSSLContext(SocketSslConfig socketSslConfig) throws Exception { | ||
| KeyStore ks = KeyStore.getInstance(socketSslConfig.getKeyStoreFormat()); | ||
| try (InputStream keyStoreStream = socketSslConfig.getKeyStore()) { | ||
| ks.load(keyStoreStream, socketSslConfig.getKeyStorePassword().toCharArray()); | ||
| } | ||
|
|
||
| KeyManagerFactory kmf = KeyManagerFactory.getInstance(socketSslConfig.getKeyManagerFactoryAlgorithm()); | ||
| kmf.init(ks, socketSslConfig.getKeyStorePassword().toCharArray()); | ||
|
|
||
| SslProvider sslProvider = OpenSsl.isAvailable() ? SslProvider.OPENSSL : SslProvider.JDK; | ||
|
|
||
| SslContextBuilder builder = SslContextBuilder.forServer(kmf).sslProvider(sslProvider); | ||
| String sslProtocol = socketSslConfig.getSSLProtocol(); | ||
| if (sslProtocol != null) { | ||
| // SocketSslConfig historically accepted SSLContext algorithm names like "TLS". | ||
| // SslContextBuilder.protocols(...) expects concrete enabled protocol versions. | ||
| if (isTlsProtocolVersion(sslProtocol)) { | ||
| builder.protocols(sslProtocol); | ||
| } else { | ||
| log.warn("Ignoring SocketSslConfig.sslProtocol='{}' because it is not a concrete TLS protocol " + | ||
| "version (expected values like 'TLSv1.2' or 'TLSv1.3'). Using provider defaults instead.", | ||
| sslProtocol); | ||
| } | ||
| } | ||
| if (socketSslConfig.getTrustStore() != null) { | ||
| KeyStore ts = KeyStore.getInstance(socketSslConfig.getTrustStoreFormat()); | ||
| ts.load(socketSslConfig.getTrustStore(), socketSslConfig.getTrustStorePassword().toCharArray()); | ||
| TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); | ||
| KeyStore ts = KeyStore.getInstance(socketSslConfig.getTrustStoreFormat()); | ||
| try (InputStream trustStoreStream = socketSslConfig.getTrustStore()) { | ||
| ts.load(trustStoreStream, socketSslConfig.getTrustStorePassword().toCharArray()); | ||
| } | ||
| tmf.init(ts); | ||
| managers = tmf.getTrustManagers(); | ||
| builder.trustManager(tmf); | ||
| } | ||
| return builder.build(); | ||
| } |
There was a problem hiding this comment.
createSSLContext now wraps socketSslConfig.getKeyStore() / getTrustStore() in try-with-resources, which closes the provided InputStreams. Configuration(Configuration) performs a shallow copy of SocketSslConfig (same instance), and SocketIOServer.stop() explicitly documents that the server may be started again. Closing the streams on first start makes subsequent restarts fail even if the caller provided a resettable stream. Consider either (a) documenting/contracting that SocketSslConfig must provide a fresh InputStream per start (e.g., via Supplier/Path), or (b) buffering the keystore/truststore bytes so restarts don’t depend on reusing the original stream.
Signed-off-by: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com>
Signed-off-by: NeatGuyCoding <15627489+NeatGuyCoding@users.noreply.github.com>
| } | ||
| }; | ||
| SSLContext sslContext = SSLContext.getInstance("TLS"); | ||
| sslContext.init(null, new TrustManager[] { trustAll }, new SecureRandom()); |
Check failure
Code scanning / CodeQL
`TrustManager` that accepts all certificates High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 minutes ago
General approach: Replace the custom “trust all” X509TrustManager with the platform-default trust managers derived from TrustManagerFactory. This way, normal certificate validation occurs and we no longer have an implementation that accepts all certificates. Additionally, remove the custom hostnameVerifier that unconditionally returns true, so hostname verification uses the default secure behavior.
Concrete fix in this file:
- Remove the anonymous
X509TrustManager trustAlldefinition. - Instead of
sslContext.init(null, new TrustManager[] { trustAll }, new SecureRandom());, initialize theSSLContextwith default trust managers:- Obtain a
TrustManagerFactoryviaTrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()). - Initialize it with
nullkey store (tmf.init((java.security.KeyStore) null);) to use the default system trust store. - Use
tmf.getTrustManagers()as the second argument tosslContext.init(...).
- Obtain a
- Change the
OkHttpClientconstruction:- Use the first
X509TrustManagerfrom the default trust managers when calling.sslSocketFactory. - Remove
.hostnameVerifier((hostname, session) -> true)so default hostname verification applies.
- Use the first
- Add imports as needed:
javax.net.ssl.TrustManagerFactoryandjava.security.KeyStore(explicit type reference; we already have otherjava.securityimports).
These changes are localized to the shouldPollUpgradeToWebSocketWithServerPushAndClientHelloAckOverWss test method in SocketIoJavaClientSslTest.java and preserve the overall test logic and structure while eliminating the “trust all certificates” behavior.
Suggested changeset
1
netty-socketio-core/src/test/java/com/socketio4j/socketio/transport/SocketIoJavaClientSslTest.java
| @@ -27,6 +27,7 @@ | ||
|
|
||
| import javax.net.ssl.SSLContext; | ||
| import javax.net.ssl.TrustManager; | ||
| import javax.net.ssl.TrustManagerFactory; | ||
| import javax.net.ssl.X509TrustManager; | ||
|
|
||
| import org.json.JSONObject; | ||
| @@ -159,26 +160,25 @@ | ||
| int port = awaitBoundPort(server); | ||
| assertTrue(port > 0, "server did not bind an ephemeral port"); | ||
|
|
||
| X509TrustManager trustAll = new X509TrustManager() { | ||
| @Override | ||
| public void checkClientTrusted(X509Certificate[] chain, String authType) { | ||
| TrustManagerFactory tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); | ||
| tmf.init((java.security.KeyStore) null); | ||
| TrustManager[] trustManagers = tmf.getTrustManagers(); | ||
| X509TrustManager trustManager = null; | ||
| for (TrustManager tm : trustManagers) { | ||
| if (tm instanceof X509TrustManager) { | ||
| trustManager = (X509TrustManager) tm; | ||
| break; | ||
| } | ||
| } | ||
| if (trustManager == null) { | ||
| throw new IllegalStateException("No X509TrustManager found"); | ||
| } | ||
|
|
||
| @Override | ||
| public void checkServerTrusted(X509Certificate[] chain, String authType) { | ||
| } | ||
|
|
||
| @Override | ||
| public X509Certificate[] getAcceptedIssuers() { | ||
| return new X509Certificate[0]; | ||
| } | ||
| }; | ||
| SSLContext sslContext = SSLContext.getInstance("TLS"); | ||
| sslContext.init(null, new TrustManager[] { trustAll }, new SecureRandom()); | ||
| sslContext.init(null, trustManagers, new SecureRandom()); | ||
|
|
||
| OkHttpClient okHttp = new OkHttpClient.Builder() | ||
| .sslSocketFactory(sslContext.getSocketFactory(), trustAll) | ||
| .hostnameVerifier((hostname, session) -> true) | ||
| .sslSocketFactory(sslContext.getSocketFactory(), trustManager) | ||
| .readTimeout(1, TimeUnit.MINUTES) | ||
| .build(); | ||
|
|
Copilot is powered by AI and may make mistakes. Always verify output.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+81
to
+94
| byte[] resolveKeyStoreBytes() throws IOException { | ||
| synchronized (sslMaterialLock) { | ||
| if (cachedKeyStoreBytes != null) { | ||
| return cachedKeyStoreBytes; | ||
| } | ||
| if (keyStore == null) { | ||
| return null; | ||
| } | ||
| try (InputStream in = keyStore) { | ||
| cachedKeyStoreBytes = in.readAllBytes(); | ||
| } | ||
| keyStore = null; | ||
| return cachedKeyStoreBytes; | ||
| } |
There was a problem hiding this comment.
resolveKeyStoreBytes() always returns cachedKeyStoreBytes when present, so calling setKeyStore(...) after the first TLS context build will never take effect. This also risks leaking the newly provided stream because it may never be read/closed. If runtime reconfiguration is expected, consider clearing the cache when setKeyStore(...) is called or adding an explicit API to reset the cached SSL material.
Comment on lines
+141
to
+154
| byte[] resolveTrustStoreBytes() throws IOException { | ||
| synchronized (sslMaterialLock) { | ||
| if (cachedTrustStoreBytes != null) { | ||
| return cachedTrustStoreBytes; | ||
| } | ||
| if (trustStore == null) { | ||
| return null; | ||
| } | ||
| try (InputStream in = trustStore) { | ||
| cachedTrustStoreBytes = in.readAllBytes(); | ||
| } | ||
| trustStore = null; | ||
| return cachedTrustStoreBytes; | ||
| } |
There was a problem hiding this comment.
resolveTrustStoreBytes() always prefers cachedTrustStoreBytes when present, so a later setTrustStore(...) call won’t be observed and the new stream may never be consumed/closed. If the intent is to support updating trust material between restarts, consider clearing the cache on setTrustStore(...) or providing a dedicated reset method.
Comment on lines
+252
to
+257
| // Common enabled-protocol tokens used by JSSE/Netty. | ||
| // Accept "TLSv1", "TLSv1.1", "TLSv1.2", "TLSv1.3"; reject "TLSv1.0" and other dotted minors. | ||
| if (value == null) { | ||
| return false; | ||
| } | ||
| return value.matches("^TLSv1(\\.(1|2|3))?$"); |
There was a problem hiding this comment.
isTlsProtocolVersion currently accepts "TLSv1", but in JSSE/Netty this token corresponds to TLS 1.0. The comment says TLSv1.0 should be rejected, so either the regex/comment is inconsistent or TLS 1.0 is being re-enabled unintentionally (which can be a security and interoperability issue because many clients/JDKs disable it). Consider tightening the accepted set (e.g., only TLSv1.2+/TLSv1.3) or updating the comment/logging to match the intended behavior.
Suggested change
| // Common enabled-protocol tokens used by JSSE/Netty. | |
| // Accept "TLSv1", "TLSv1.1", "TLSv1.2", "TLSv1.3"; reject "TLSv1.0" and other dotted minors. | |
| if (value == null) { | |
| return false; | |
| } | |
| return value.matches("^TLSv1(\\.(1|2|3))?$"); | |
| // Accept only concrete, modern TLS protocol versions supported by JSSE/Netty. | |
| // Specifically allow "TLSv1.2" and "TLSv1.3" and reject TLS 1.0/1.1 and other variants. | |
| if (value == null) { | |
| return false; | |
| } | |
| return value.matches("^TLSv1\\.(2|3)$"); |
Comment on lines
+601
to
612
| if (configCopy.getPort() == 0) { | ||
| try { | ||
| InetSocketAddress local = (InetSocketAddress) cf.channel().localAddress(); | ||
| int actualPort = local.getPort(); | ||
| configCopy.setPort(actualPort); | ||
| configuration.setPort(actualPort); | ||
| } catch (Exception ignore) { | ||
| // keep configured port if localAddress is not InetSocketAddress | ||
| } | ||
| } | ||
| serverStatus.set(ServerStatus.STARTED); | ||
| log.info("SocketIO server started on port {}", configCopy.getPort()); |
There was a problem hiding this comment.
When the configured port is 0 (ephemeral), this code mutates both configCopy and the original configuration to the bound port. That changes the semantics of “port 0” for subsequent restarts (a second start() will attempt to bind the previous port instead of a new ephemeral port) and can introduce flakiness or port-collision surprises. Consider keeping the configured port as-is (0) and exposing the actual bound port via serverChannel.localAddress()/a dedicated getter instead of persisting it back into the configuration object.
Suggested change
| if (configCopy.getPort() == 0) { | |
| try { | |
| InetSocketAddress local = (InetSocketAddress) cf.channel().localAddress(); | |
| int actualPort = local.getPort(); | |
| configCopy.setPort(actualPort); | |
| configuration.setPort(actualPort); | |
| } catch (Exception ignore) { | |
| // keep configured port if localAddress is not InetSocketAddress | |
| } | |
| } | |
| serverStatus.set(ServerStatus.STARTED); | |
| log.info("SocketIO server started on port {}", configCopy.getPort()); | |
| int loggedPort = configCopy.getPort(); | |
| try { | |
| if (cf.channel().localAddress() instanceof InetSocketAddress) { | |
| InetSocketAddress local = (InetSocketAddress) cf.channel().localAddress(); | |
| loggedPort = local.getPort(); | |
| } | |
| } catch (Exception ignore) { | |
| // keep configured port if localAddress is not InetSocketAddress | |
| } | |
| serverStatus.set(ServerStatus.STARTED); | |
| log.info("SocketIO server started on port {}", loggedPort); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Instead of pipeline.fireChannelRead(...), forward PacketsMessage from the context of the HTTP/WebSocket codec
Type of Change
Related Issue
Closes #156
Changes Made
Only if none exist, fall back to pipeline.fireChannelRead(...)
Testing
mvn testChecklist
Additional Notes
Any additional information, screenshots, or context that reviewers should know.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores