feat: [channel auth]#189
Open
chenqwwq wants to merge 5 commits into
Open
Conversation
send authentication token as soon as the channel is available
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds JWT-based client authentication: route issues authToken, client includes authToken in login, server verifies the token via a new ClientAuthHandler, binds user info to channel attributes, and removes the handler on success; introduces JwtUtils, PayloadVO, ChannelAuthReq, channel attribute keys, and ServerConfig. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RouteServer as Route Server
participant JwtUtils as JWT Utils
participant CIMServer as CIM Server
Client->>RouteServer: request route/login (userId,userName)
RouteServer->>JwtUtils: generateToken(userId, payload{host,port})
JwtUtils-->>RouteServer: authToken
RouteServer-->>Client: CIMServerResVO(authToken,...)
Client->>CIMServer: connect + send ChannelAuthReq(userId,userName,authToken)
CIMServer->>CIMServer: ClientAuthHandler.channelRead(msg)
CIMServer->>JwtUtils: verifyToken(authToken)
JwtUtils-->>CIMServer: PayloadVO (userId,userName,host,port)
CIMServer-->>Client: success Response, set channel attrs, remove auth handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
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 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 |
Closed
![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: 13
🧹 Nitpick comments (1)
cim-common/src/test/java/com/crossoverjie/cim/common/auth/JwtAuthTest.java (1)
28-31: Consider adding assertions forhostandportfields.The test sets
hostandportin the payload but doesn't verify they are correctly preserved after token verification. These fields are critical for server-side authentication (used inClientAuthHandler).Proposed fix
final PayloadVO payloadVO = JwtUtils.verifyToken(token); Assert.assertEquals(userId, payloadVO.getUserId()); Assert.assertEquals(userName, payloadVO.getUserName()); + Assert.assertEquals(host, payloadVO.getHost()); + Assert.assertEquals(port, payloadVO.getPort()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cim-common/src/test/java/com/crossoverjie/cim/common/auth/JwtAuthTest.java` around lines 28 - 31, The test JwtAuthTest currently verifies userId and userName but misses host and port; update the JwtAuthTest test method that creates the PayloadVO and calls JwtUtils.verifyToken(token) to also assert payloadVO.getHost() equals the host value you set and payloadVO.getPort() equals the port value you set, ensuring the token round-trip preserves those fields used by ClientAuthHandler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java`:
- Around line 149-156: The current flow in ClientImpl calls doConnectServer(...)
and loginServer(...).sync() then immediately sets future.complete(true), which
only waits for the outbound write and can mark login as successful before the
server accepts the token; also the catch block calls conf.getEvent().fatal(this)
but doesn't complete the future exceptionally. Change the logic so the
CompletableFuture<Boolean> (future) is completed only when the server-side auth
result is received (i.e., from the server response handler/authorization
callback that processes the auth token result), completeExceptionally(failure)
when auth is rejected or on connection errors, and remove relying on
loginServer(...).sync() as the success signal; also ensure
conf.getEvent().fatal(this) is still invoked on fatal errors but does not
prevent future from being completed exceptionally so callers don't hang.
In `@cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java`:
- Line 8: Remove the unused import of Claim from JwtUtils (the import line
"import com.auth0.jwt.interfaces.Claim") to satisfy Checkstyle UnusedImports;
locate the JwtUtils class in
cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java and
delete the Claim import (or organize imports) so the file compiles and passes
maven-checkstyle-plugin checks.
- Around line 74-82: The catch blocks in JwtUtils (TokenExpiredException,
JWTDecodeException, and the generic Exception in the token verification path)
currently log the full JWT via the variable token; remove any logging of the raw
token value and replace it with a safe alternative (e.g., log a generic message
like "jwt token expired/failed to decode/verify" and optionally include a
non-secret identifier such as a token hash or the subject/claim extracted
securely), ensuring the catches still rethrow the original exceptions
(TokenExpiredException, JWTDecodeException) as before; update the log.warn calls
in the TokenExpiredException, JWTDecodeException, and generic Exception handlers
to omit the token parameter or substitute a non-sensitive fingerprint.
- Around line 30-33: The SECRET field in JwtUtils is hard-coded; replace it with
a value read from external configuration (e.g., System.getenv or application
config) and initialize the Algorithm from that runtime secret instead of a
compile-time constant; update the static ALGORITHM to be created in a static
initializer or via a getAlgorithm() method that reads the env/config value,
validate the secret is present (throw or fail-fast) and keep ISSUER unchanged.
- Around line 70-73: The verifier in JwtUtils.verifyToken (the block that builds
the JWT verifier and calls verify(token)) does not validate the issuer, so add
issuer validation to the verifier by configuring JWT.require(ALGORITHM) to
expect the same ISSUER used in generateToken (use withIssuer(ISSUER)) before
calling build().verify(token); update the logic that reads the "payload" claim
(getClaim("payload").as(PayloadVO.class)) to continue returning the payload
after the verifier enforces the issuer.
In
`@cim-common/src/main/java/com/crossoverjie/cim/common/enums/ChannelAttributeKeys.java`:
- Around line 32-34: The file's enum ChannelAttributeKeys has trailing blank
lines before its closing brace causing a checkstyle violation; open the enum
(ChannelAttributeKeys) and remove any extra blank lines so the final '}'
immediately follows the last enum/member or statement, then save and re-run
checks to ensure the trailing-whitespace/blank-line issue is resolved.
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java`:
- Around line 31-57: The current init() unconditionally probes network
interfaces via getLocalHostAddress(), which is brittle; change init() to prefer
an explicit configured host (e.g., a new config property/field like serverHost
or `@Value-injected` property) and only call getLocalHostAddress() as a fallback
when that configured host is empty/null; update getLocalHostAddress() to remain
the fallback probe for local IP discovery. Ensure references to host and
nettyPort remain, and update any field name you introduce (e.g., serverHost) in
init() and any Javadoc so tokens/validation use the explicit configured host
when provided.
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/handle/CIMServerHandle.java`:
- Around line 76-87: The method channelRead0 currently logs the full Request via
msg.toString() and msg.getReqMsg(), which now contains sensitive authToken;
remove those raw payload logs and instead log only non-sensitive identifiers.
Replace calls to log.info("received msg=[{}]", msg.toString()) and
log.info("client [{}] online success!!", msg.getReqMsg()) with safe messages
that reference Request identifiers (e.g., req.getUserId(), req.getUserName()) or
a redacted placeholder; ensure code paths using Request, ChannelAuthReq,
msg.getReqMsg(), and msg.toString() no longer emit full payloads to logs.
- Around line 80-86: The code currently binds the session using values from
ChannelAuthReq (req.getUserId()/req.getUserName()), which allows a client to
claim a different identity; instead, read the authenticated identity stored on
the Channel by the earlier ClientAuthHandler (the channel attributes for USER_ID
and USER_NAME via ctx.channel().attr(...).get()), validate they are present, and
call SessionSocketHolder.put(...) and SessionSocketHolder.saveSession(...) using
those channel attribute values (and throw a CIMException if the authenticated
attributes are missing) rather than using req.getUserId()/req.getUserName().
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java`:
- Around line 143-147: The current code in ClientAuthHandler lets any token with
payloadHost "127.0.0.1", "0.0.0.0" or "localhost" bypass host pinning; change
the logic so loopback claims are only accepted when the server itself is bound
to a loopback address. Concretely, in the host-check block that uses payloadHost
and serverConfig.getHost(), replace the unconditional loopback return with a
check that returns true only if payloadHost is loopback AND
serverConfig.getHost() is also a loopback (or both normalize to the same
InetAddress); otherwise require payloadHost.equals(serverConfig.getHost()). Use
payloadHost, serverConfig.getHost(), and ClientAuthHandler to locate and update
the code.
- Around line 95-107: ClientAuthHandler currently stores verified
userId/userName in ChannelAttributeKeys but forwards the original ChannelAuthReq
(msg) allowing spoofing; update ClientAuthHandler so that after setting
ctx.channel().attr(...) and before ctx.fireChannelRead(msg) you create a new
ChannelAuthReq (or copy the existing msg) with userId and userName replaced by
payload.getUserId()/payload.getUserName(), then call ctx.fireChannelRead(...)
with that rewritten request so downstream CIMServerHandle will receive the
verified identity; alternatively, if you prefer changing downstream, update
CIMServerHandle to read identities from ChannelAttributeKeys.USER_ID/USER_NAME
instead of req.getUserId()/getUserName().
- Around line 61-69: The null/non-Request branches in ClientAuthHandler
currently just return and leave the channel open; change them to close the
connection consistently by calling writeAndClose(...) as the other error paths
do: when msg is null or when !(msg instanceof Request) use the same
channel/context error handling path (invoke writeAndClose with an appropriate
error Response or message) so the connection is terminated instead of left
unauthenticated; locate the checks around msg, Request, and replace the plain
returns with calls to the existing writeAndClose(...) helper used elsewhere in
this class.
In `@pom.xml`:
- Around line 50-54: The dependency entry for com.auth0:java-jwt (artifactId
java-jwt) is pinned to 3.19.0 which pulls an insecure jackson-databind; update
the <version> for com.auth0:java-jwt to 4.5.1 in the POM to pick up the fixed
transitive dependency, then run mvn dependency:tree to confirm jackson-databind
is upgraded; if you cannot upgrade java-jwt for compatibility reasons,
explicitly override or exclude the transitive jackson-databind and add a safe
jackson-databind version (>= the patched release) as a direct dependency, and
run the test suite to verify there are no runtime incompatibilities with Spring
Boot 3.3.0.
---
Nitpick comments:
In `@cim-common/src/test/java/com/crossoverjie/cim/common/auth/JwtAuthTest.java`:
- Around line 28-31: The test JwtAuthTest currently verifies userId and userName
but misses host and port; update the JwtAuthTest test method that creates the
PayloadVO and calls JwtUtils.verifyToken(token) to also assert
payloadVO.getHost() equals the host value you set and payloadVO.getPort() equals
the port value you set, ensuring the token round-trip preserves those fields
used by ClientAuthHandler.
🪄 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: d55d6b7d-b72e-4f83-b83c-f50823e1f902
📒 Files selected for processing (18)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.javacim-client/src/main/java/com/crossoverjie/cim/client/service/MsgHandle.javacim-common/pom.xmlcim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.javacim-common/src/main/java/com/crossoverjie/cim/common/auth/jwt/dto/PayloadVO.javacim-common/src/main/java/com/crossoverjie/cim/common/enums/ChannelAttributeKeys.javacim-common/src/main/java/com/crossoverjie/cim/common/msg/ChannelAuthReq.javacim-common/src/test/java/com/crossoverjie/cim/common/auth/JwtAuthTest.javacim-forward-route/src/main/java/com/crossoverjie/cim/route/controller/RouteController.javacim-rout-api/src/main/java/com/crossoverjie/cim/route/api/vo/res/CIMServerResVO.javacim-server/src/main/java/com/crossoverjie/cim/server/CIMServerApplication.javacim-server/src/main/java/com/crossoverjie/cim/server/config/AppConfiguration.javacim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.javacim-server/src/main/java/com/crossoverjie/cim/server/handle/CIMServerHandle.javacim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.javacim-server/src/main/java/com/crossoverjie/cim/server/init/CIMServerInitializer.javacim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.javapom.xml
💤 Files with no reviewable changes (1)
- cim-client/src/main/java/com/crossoverjie/cim/client/service/MsgHandle.java
Comment on lines
+149
to
+156
| try { | ||
| this.doConnectServer(cimServer, future); | ||
| this.loginServer(cimServer.getAuthToken()); | ||
| this.serverInfo = cimServer; | ||
| future.complete(true); | ||
| } catch (Exception e) { | ||
| this.conf.getEvent().fatal(this); | ||
| } |
Contributor
There was a problem hiding this comment.
Drive the login future from a server auth result, not from the local write.
loginServer(...).sync() only waits for the outbound write to finish. future.complete(true) therefore runs before the server has accepted the token, and the catch block only calls fatal(this) without completing the future exceptionally. An auth failure can look like a successful login or leave the future unresolved.
Also applies to: 207-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java`
around lines 149 - 156, The current flow in ClientImpl calls
doConnectServer(...) and loginServer(...).sync() then immediately sets
future.complete(true), which only waits for the outbound write and can mark
login as successful before the server accepts the token; also the catch block
calls conf.getEvent().fatal(this) but doesn't complete the future exceptionally.
Change the logic so the CompletableFuture<Boolean> (future) is completed only
when the server-side auth result is received (i.e., from the server response
handler/authorization callback that processes the auth token result),
completeExceptionally(failure) when auth is rejected or on connection errors,
and remove relying on loginServer(...).sync() as the success signal; also ensure
conf.getEvent().fatal(this) is still invoked on fatal errors but does not
prevent future from being completed exceptionally so callers don't hang.
Comment on lines
+30
to
+33
| private static final String SECRET = "cim#crossoverjie"; | ||
|
|
||
| public static final Algorithm ALGORITHM = Algorithm.HMAC256(SECRET); | ||
| public static final String ISSUER = "cim"; |
Contributor
There was a problem hiding this comment.
Move the JWT secret out of source control.
Hard-coding the HMAC secret makes every environment share the same signing credential and exposes it to anyone with repository access.
🔑 Suggested fix
- private static final String SECRET = "cim#crossoverjie";
+ private static final String SECRET =
+ System.getProperty("cim.jwt.secret", System.getenv("CIM_JWT_SECRET"));
+
+ static {
+ Preconditions.checkArgument(StringUtils.isNotBlank(SECRET), "cim jwt secret is required");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static final String SECRET = "cim#crossoverjie"; | |
| public static final Algorithm ALGORITHM = Algorithm.HMAC256(SECRET); | |
| public static final String ISSUER = "cim"; | |
| private static final String SECRET; | |
| static { | |
| SECRET = System.getProperty("cim.jwt.secret", System.getenv("CIM_JWT_SECRET")); | |
| if (SECRET == null || SECRET.isBlank()) { | |
| throw new IllegalStateException("cim jwt secret is required"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java`
around lines 30 - 33, The SECRET field in JwtUtils is hard-coded; replace it
with a value read from external configuration (e.g., System.getenv or
application config) and initialize the Algorithm from that runtime secret
instead of a compile-time constant; update the static ALGORITHM to be created in
a static initializer or via a getAlgorithm() method that reads the env/config
value, validate the secret is present (throw or fail-fast) and keep ISSUER
unchanged.
Comment on lines
+70
to
+73
| final DecodedJWT decodedObj = JWT.require(ALGORITHM) | ||
| .build() | ||
| .verify(token); | ||
| return decodedObj.getClaim("payload").as(PayloadVO.class); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "JwtUtils.java" -type fRepository: crossoverJie/cim
Length of output: 133
🏁 Script executed:
cat -n ./cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.javaRepository: crossoverJie/cim
Length of output: 3769
Add issuer validation to the token verifier.
The generateToken() method stamps the ISSUER claim (withIssuer(ISSUER) at line 49), but verifyToken() does not validate it. The verifier accepts any token signed with the same secret regardless of issuer, creating a security gap.
Suggested fix
final DecodedJWT decodedObj = JWT.require(ALGORITHM)
+ .withIssuer(ISSUER)
.build()
.verify(token);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| final DecodedJWT decodedObj = JWT.require(ALGORITHM) | |
| .build() | |
| .verify(token); | |
| return decodedObj.getClaim("payload").as(PayloadVO.class); | |
| final DecodedJWT decodedObj = JWT.require(ALGORITHM) | |
| .withIssuer(ISSUER) | |
| .build() | |
| .verify(token); | |
| return decodedObj.getClaim("payload").as(PayloadVO.class); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java`
around lines 70 - 73, The verifier in JwtUtils.verifyToken (the block that
builds the JWT verifier and calls verify(token)) does not validate the issuer,
so add issuer validation to the verifier by configuring JWT.require(ALGORITHM)
to expect the same ISSUER used in generateToken (use withIssuer(ISSUER)) before
calling build().verify(token); update the logic that reads the "payload" claim
(getClaim("payload").as(PayloadVO.class)) to continue returning the payload
after the verifier enforces the issuer.
Comment on lines
+74
to
+82
| } catch (TokenExpiredException e) { | ||
| log.warn("jwt token has expired, token:{}", token); | ||
| throw e; | ||
| } catch (JWTDecodeException e) { | ||
| log.warn("jwt auth token decode failure, token:{}", token, e); | ||
| throw e; | ||
| } catch (Exception e) { | ||
| log.warn("jwt auth token verify failure, token:{}", token, e); | ||
| throw new JWTDecodeException("user client auth Token decoder failure"); |
Contributor
There was a problem hiding this comment.
Don't write bearer tokens to the logs.
These warnings print the full JWT on expiry/verification failures. That turns the log sink into a credential store for replayable tokens.
🧾 Suggested fix
- log.warn("jwt token has expired, token:{}", token);
+ log.warn("jwt token has expired");
throw e;
} catch (JWTDecodeException e) {
- log.warn("jwt auth token decode failure, token:{}", token, e);
+ log.warn("jwt auth token decode failure", e);
throw e;
} catch (Exception e) {
- log.warn("jwt auth token verify failure, token:{}", token, e);
+ log.warn("jwt auth token verify failure", e);
throw new JWTDecodeException("user client auth Token decoder failure");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java`
around lines 74 - 82, The catch blocks in JwtUtils (TokenExpiredException,
JWTDecodeException, and the generic Exception in the token verification path)
currently log the full JWT via the variable token; remove any logging of the raw
token value and replace it with a safe alternative (e.g., log a generic message
like "jwt token expired/failed to decode/verify" and optionally include a
non-secret identifier such as a token hash or the subject/claim extracted
securely), ensuring the catches still rethrow the original exceptions
(TokenExpiredException, JWTDecodeException) as before; update the log.warn calls
in the TokenExpiredException, JWTDecodeException, and generic Exception handlers
to omit the token parameter or substitute a non-sensitive fingerprint.
Comment on lines
+80
to
+86
| final ChannelAuthReq req = JSONObject.parseObject(msg.getReqMsg(), ChannelAuthReq.class); | ||
| if (Objects.isNull(req)) { | ||
| throw new CIMException("auth req body is empty"); | ||
| } | ||
| //保存客户端与 Channel 之间的关系 | ||
| SessionSocketHolder.put(msg.getRequestId(), (NioSocketChannel) ctx.channel()); | ||
| SessionSocketHolder.saveSession(msg.getRequestId(), msg.getReqMsg()); | ||
| SessionSocketHolder.put(req.getUserId(), (NioSocketChannel) ctx.channel()); | ||
| SessionSocketHolder.saveSession(req.getUserId(), req.getUserName()); |
Contributor
There was a problem hiding this comment.
Use the authenticated channel identity when binding the session.
ClientAuthHandler already writes the verified USER_ID / USER_NAME onto the channel before this handler runs. Rebinding from req.getUserId() / req.getUserName() lets a client authenticate with one token and then claim a different identity in the JSON body here.
🔐 Suggested fix
+import com.crossoverjie.cim.common.enums.ChannelAttributeKeys;
...
- SessionSocketHolder.put(req.getUserId(), (NioSocketChannel) ctx.channel());
- SessionSocketHolder.saveSession(req.getUserId(), req.getUserName());
+ Long userId = ctx.channel().attr(ChannelAttributeKeys.USER_ID).get();
+ String userName = ctx.channel().attr(ChannelAttributeKeys.USER_NAME).get();
+ if (userId == null || userName == null) {
+ throw new CIMException("channel auth context missing");
+ }
+ SessionSocketHolder.put(userId, (NioSocketChannel) ctx.channel());
+ SessionSocketHolder.saveSession(userId, userName);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| final ChannelAuthReq req = JSONObject.parseObject(msg.getReqMsg(), ChannelAuthReq.class); | |
| if (Objects.isNull(req)) { | |
| throw new CIMException("auth req body is empty"); | |
| } | |
| //保存客户端与 Channel 之间的关系 | |
| SessionSocketHolder.put(msg.getRequestId(), (NioSocketChannel) ctx.channel()); | |
| SessionSocketHolder.saveSession(msg.getRequestId(), msg.getReqMsg()); | |
| SessionSocketHolder.put(req.getUserId(), (NioSocketChannel) ctx.channel()); | |
| SessionSocketHolder.saveSession(req.getUserId(), req.getUserName()); | |
| final ChannelAuthReq req = JSONObject.parseObject(msg.getReqMsg(), ChannelAuthReq.class); | |
| if (Objects.isNull(req)) { | |
| throw new CIMException("auth req body is empty"); | |
| } | |
| //保存客户端与 Channel 之间的关系 | |
| Long userId = ctx.channel().attr(ChannelAttributeKeys.USER_ID).get(); | |
| String userName = ctx.channel().attr(ChannelAttributeKeys.USER_NAME).get(); | |
| if (userId == null || userName == null) { | |
| throw new CIMException("channel auth context missing"); | |
| } | |
| SessionSocketHolder.put(userId, (NioSocketChannel) ctx.channel()); | |
| SessionSocketHolder.saveSession(userId, userName); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/handle/CIMServerHandle.java`
around lines 80 - 86, The code currently binds the session using values from
ChannelAuthReq (req.getUserId()/req.getUserName()), which allows a client to
claim a different identity; instead, read the authenticated identity stored on
the Channel by the earlier ClientAuthHandler (the channel attributes for USER_ID
and USER_NAME via ctx.channel().attr(...).get()), validate they are present, and
call SessionSocketHolder.put(...) and SessionSocketHolder.saveSession(...) using
those channel attribute values (and throw a CIMException if the authenticated
attributes are missing) rather than using req.getUserId()/req.getUserName().
Comment on lines
+61
to
+69
| if (Objects.isNull(msg)) { | ||
| log.warn("Received null message during authentication"); | ||
| return; | ||
| } | ||
|
|
||
| if (!(msg instanceof Request)) { | ||
| log.warn("Received non-Request message during authentication: {}", msg.getClass()); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java | head -120Repository: crossoverJie/cim
Length of output: 5579
🏁 Script executed:
cat -n cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java | tail -50Repository: crossoverJie/cim
Length of output: 1890
🏁 Script executed:
# Search for ReferenceCountUtil usage in the codebase
rg "ReferenceCountUtil" --type java -B 2 -A 2Repository: crossoverJie/cim
Length of output: 42
🏁 Script executed:
# Search for patterns where messages are handled in other handlers
rg "instanceof Request" --type java -B 3 -A 3Repository: crossoverJie/cim
Length of output: 838
🏁 Script executed:
# Search for other inbound handler implementations
rg "channelRead" --type java -B 2 -A 5 | head -100Repository: crossoverJie/cim
Length of output: 5013
🏁 Script executed:
# Check if Request is a ReferenceCounted object by looking at the protocol definition
fd "Request.java" --type fRepository: crossoverJie/cim
Length of output: 210
🏁 Script executed:
cat -n cim-common/src/main/java/com/crossoverjie/cim/common/protocol/Request.java 2>/dev/null || cat -n cim-common/src/main/java/com/crossoverjie/cim/common/core/proxy/Request.java | head -50Repository: crossoverJie/cim
Length of output: 517
🏁 Script executed:
find . -path "*protocol*" -name "Request.java" -type fRepository: crossoverJie/cim
Length of output: 42
🏁 Script executed:
rg "class Request" --type java -B 2 -A 5Repository: crossoverJie/cim
Length of output: 42
🏁 Script executed:
# Check the import statement for Request in ClientAuthHandler
rg "import.*Request" cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.javaRepository: crossoverJie/cim
Length of output: 112
🏁 Script executed:
find . -path "*common/protocol*" -name "*.java" -type f | head -20Repository: crossoverJie/cim
Length of output: 42
🏁 Script executed:
rg "class Request\b" --type javaRepository: crossoverJie/cim
Length of output: 42
🏁 Script executed:
rg "interface Request|record Request|class Request" --type javaRepository: crossoverJie/cim
Length of output: 164
🏁 Script executed:
# Find all Java files in the common module
fd -e java "common" | grep -E "protocol|msg" | head -20Repository: crossoverJie/cim
Length of output: 42
🏁 Script executed:
# List files in common module
ls -la cim-common/src/main/java/com/crossoverjie/cim/common/Repository: crossoverJie/cim
Length of output: 938
🏁 Script executed:
# Search for where Request is actually defined
rg "import com.crossoverjie.cim.common.protocol" --type javaRepository: crossoverJie/cim
Length of output: 2695
Close connection on null or non-Request messages during authentication.
Lines 61-69 return without closing the connection, inconsistent with all other error paths in this handler (lines 75, 83, 92, 110) which call writeAndClose(). On an authentication-gated pipeline, this leaves the connection stuck unauthenticated.
♻️ Suggested fix
if (Objects.isNull(msg)) {
log.warn("Received null message during authentication");
+ writeAndClose("Require authentication first", ctx);
return;
}
- if (!(msg instanceof Request)) {
+ if (!(msg instanceof Request request)) {
log.warn("Received non-Request message during authentication: {}", msg.getClass());
+ writeAndClose("Require authentication first", ctx);
return;
}
// 处理 Token
- final String body = ((Request) msg).getReqMsg();
+ final String body = request.getReqMsg();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (Objects.isNull(msg)) { | |
| log.warn("Received null message during authentication"); | |
| return; | |
| } | |
| if (!(msg instanceof Request)) { | |
| log.warn("Received non-Request message during authentication: {}", msg.getClass()); | |
| return; | |
| } | |
| if (Objects.isNull(msg)) { | |
| log.warn("Received null message during authentication"); | |
| writeAndClose("Require authentication first", ctx); | |
| return; | |
| } | |
| if (!(msg instanceof Request request)) { | |
| log.warn("Received non-Request message during authentication: {}", msg.getClass()); | |
| writeAndClose("Require authentication first", ctx); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java`
around lines 61 - 69, The null/non-Request branches in ClientAuthHandler
currently just return and leave the channel open; change them to close the
connection consistently by calling writeAndClose(...) as the other error paths
do: when msg is null or when !(msg instanceof Request) use the same
channel/context error handling path (invoke writeAndClose with an appropriate
error Response or message) so the connection is terminated instead of left
unauthenticated; locate the checks around msg, Request, and replace the plain
returns with calls to the existing writeAndClose(...) helper used elsewhere in
this class.
Comment on lines
+95
to
+107
| ctx.channel().attr(ChannelAttributeKeys.USER_ID).set(payload.getUserId()); | ||
| ctx.channel().attr(ChannelAttributeKeys.USER_NAME).set(payload.getUserName()); | ||
| attr.set(Boolean.TRUE); | ||
| // 认证成功之后移除自身 | ||
| ctx.pipeline().remove(this); | ||
| // 发送欢迎信息 | ||
| final Response welcome = Response.newBuilder() | ||
| .setResponseId(-1) | ||
| .setResMsg("auth success,welcome to cim,You can speak freely") | ||
| .setCmd(BaseCommand.PING) | ||
| .build(); | ||
| ctx.writeAndFlush(welcome); | ||
| ctx.fireChannelRead(msg); // 继续向后传递 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ChannelAuthReq fields =="
fd 'ChannelAuthReq.java' | xargs -r sed -n '1,220p'
echo
echo "== Client auth request construction =="
fd 'ClientImpl.java' | xargs -r rg -n -C3 'loginServer|ChannelAuthReq|authToken'
echo
echo "== Downstream session binding =="
fd 'CIMServerHandle.java' | xargs -r rg -n -C3 'ChannelAuthReq|getUserId|getUserName|ChannelAttributeKeys'Repository: crossoverJie/cim
Length of output: 3570
🏁 Script executed:
fd 'ClientAuthHandler.java' | xargs -r wc -lRepository: crossoverJie/cim
Length of output: 146
🏁 Script executed:
fd 'ClientAuthHandler.java' | xargs -r cat -nRepository: crossoverJie/cim
Length of output: 7058
Rewrite the forwarded message with verified identity values.
Lines 95-96 extract and store verified userId and userName from the JWT payload, but line 107 forwards the original ChannelAuthReq unchanged. The downstream CIMServerHandle (lines 85-86) reads req.getUserId() and req.getUserName() directly from the request body instead of from the verified channel attributes. This allows a client to send a different userId/userName in the request than what the JWT actually authorizes.
Either rewrite the ChannelAuthReq in the forwarded message with the verified payload values before line 107, or update CIMServerHandle to read the identity from ChannelAttributeKeys.USER_ID / USER_NAME instead of the request body.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java`
around lines 95 - 107, ClientAuthHandler currently stores verified
userId/userName in ChannelAttributeKeys but forwards the original ChannelAuthReq
(msg) allowing spoofing; update ClientAuthHandler so that after setting
ctx.channel().attr(...) and before ctx.fireChannelRead(msg) you create a new
ChannelAuthReq (or copy the existing msg) with userId and userName replaced by
payload.getUserId()/payload.getUserName(), then call ctx.fireChannelRead(...)
with that rewritten request so downstream CIMServerHandle will receive the
verified identity; alternatively, if you prefer changing downstream, update
CIMServerHandle to read identities from ChannelAttributeKeys.USER_ID/USER_NAME
instead of req.getUserId()/getUserName().
Comment on lines
+143
to
+147
| // 回环地址直接放行 | ||
| if ("127.0.0.1".equals(payloadHost) || "0.0.0.0".equals(payloadHost) || "localhost".equals(payloadHost)) { | ||
| return true; | ||
| } | ||
| return payloadHost.equals(serverConfig.getHost()); |
Contributor
There was a problem hiding this comment.
Loopback claims currently bypass host pinning for every server.
A token with host=127.0.0.1, 0.0.0.0, or localhost is accepted here even when serverConfig.getHost() points to a different machine. That effectively turns the host claim into a wildcard and permits replay across any node sharing the same port.
🔐 Suggested hardening
- if ("127.0.0.1".equals(payloadHost) || "0.0.0.0".equals(payloadHost) || "localhost".equals(payloadHost)) {
- return true;
- }
- return payloadHost.equals(serverConfig.getHost());
+ if (isLoopback(payloadHost)) {
+ return isLoopback(serverConfig.getHost());
+ }
+ return payloadHost.equals(serverConfig.getHost());
}
+
+ private boolean isLoopback(String host) {
+ return "127.0.0.1".equals(host)
+ || "0.0.0.0".equals(host)
+ || "localhost".equalsIgnoreCase(host);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java`
around lines 143 - 147, The current code in ClientAuthHandler lets any token
with payloadHost "127.0.0.1", "0.0.0.0" or "localhost" bypass host pinning;
change the logic so loopback claims are only accepted when the server itself is
bound to a loopback address. Concretely, in the host-check block that uses
payloadHost and serverConfig.getHost(), replace the unconditional loopback
return with a check that returns true only if payloadHost is loopback AND
serverConfig.getHost() is also a loopback (or both normalize to the same
InetAddress); otherwise require payloadHost.equals(serverConfig.getHost()). Use
payloadHost, serverConfig.getHost(), and ClientAuthHandler to locate and update
the code.
Comment on lines
+50
to
+54
| <dependency> | ||
| <groupId>com.auth0</groupId> | ||
| <artifactId>java-jwt</artifactId> | ||
| <version>3.19.0</version> | ||
| </dependency> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for security advisories for java-jwt
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "com.auth0:java-jwt") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: crossoverJie/cim
Length of output: 220
🌐 Web query:
What is the latest version of com.auth0 java-jwt library?
💡 Result:
The latest version of com.auth0:java-jwt on Maven Central is 4.5.1 (published Feb 16, 2026). (repo1.maven.org)
Citations:
🌐 Web query:
java-jwt 3.19.0 security vulnerabilities CVE
💡 Result:
For com.auth0:java-jwt:3.19.0, I did not find CVEs that are assigned to java-jwt itself; however, Maven metadata flags known CVEs coming from its runtime dependency com.fasterxml.jackson.core:jackson-databind:2.13.2. (mvnrepository.com)
CVE impact you’ll see reported for java-jwt:3.19.0 (via jackson-databind):
- CVE-2022-42003 (jackson-databind) (nvd.nist.gov)
- CVE-2022-42004 (jackson-databind) (nvd.nist.gov)
- CVE-2020-36518 (jackson-databind) (nvd.nist.gov)
What to do
- If you must stay on java-jwt 3.19.0, mitigate by overriding/upgrading
jackson-databindto a non-vulnerable version for those CVEs (your build tool’s dependency management/BOM). (mvnrepository.com)
Citations:
- 1: https://mvnrepository.com/artifact/com.auth0/java-jwt/3.19.0
- 2: https://nvd.nist.gov/vuln/detail/cve-2022-42003?utm_source=openai
- 3: https://nvd.nist.gov/vuln/detail/cve-2022-42004?utm_source=openai
- 4: https://nvd.nist.gov/vuln/detail/cve-2020-36518?utm_source=openai
- 5: https://mvnrepository.com/artifact/com.auth0/java-jwt/3.19.0/dependencies
🌐 Web query:
java-jwt version history release dates
💡 Result:
Auth0 java-jwt (Maven artifact com.auth0:java-jwt) version history with release dates (per MvnRepository’s Maven Central index): [1]
| Version | Release date |
|---|---|
| 4.5.1 | Feb 16, 2026 [1] |
| 4.5.0 | Jan 29, 2025 [1] |
| 4.4.0 | Mar 31, 2023 [1] |
| 4.3.0 | Feb 13, 2023 [1] |
| 4.2.2 | Jan 11, 2023 [1] |
| 4.2.1 | Oct 25, 2022 [1] |
| 4.2.0 | Oct 20, 2022 [1] |
| 4.1.0 | Oct 12, 2022 [1] |
| 4.0.0 | Jun 24, 2022 [1] |
| 3.19.4 | Jan 11, 2023 [1] |
| … | … (list continues back to 0.1 on Mar 07, 2014) [1] |
If you want, tell me which range you care about (e.g., only 4.x, or since 3.0.0), and I’ll format the full list for that range.
Source: MvnRepository “Versions (60)” page for com.auth0:java-jwt. [1]
Upgrade java-jwt to version 4.5.1 or apply security patches for jackson-databind dependency.
Version 3.19.0 is outdated and carries known security vulnerabilities through its transitive dependency on jackson-databind:2.13.2, which is affected by CVE-2022-42003, CVE-2022-42004, and CVE-2020-36518. Upgrade to the latest version 4.5.1 (released February 2026) to resolve these vulnerabilities and gain compatibility with Spring Boot 3.3.0.
🧰 Tools
🪛 GitHub Actions: Java CI with Maven
[error] Command failed: mvn checkstyle:check --file pom.xml (maven-checkstyle-plugin:3.3.1:check default-cli) on project cim-common due to 1 Checkstyle violation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pom.xml` around lines 50 - 54, The dependency entry for com.auth0:java-jwt
(artifactId java-jwt) is pinned to 3.19.0 which pulls an insecure
jackson-databind; update the <version> for com.auth0:java-jwt to 4.5.1 in the
POM to pick up the fixed transitive dependency, then run mvn dependency:tree to
confirm jackson-databind is upgraded; if you cannot upgrade java-jwt for
compatibility reasons, explicitly override or exclude the transitive
jackson-databind and add a safe jackson-databind version (>= the patched
release) as a direct dependency, and run the test suite to verify there are no
runtime incompatibilities with Spring Boot 3.3.0.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java (3)
29-31:⚠️ Potential issue | 🔴 CriticalMove the JWT signing secret out of source control.
This still hard-codes the HMAC key that both the route service and the server trust for token issuance/verification. Anyone with repo access can mint valid auth tokens for every environment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java` around lines 29 - 31, The JwtUtils class currently hard-codes the HMAC secret via the SECRET constant and builds ALGORITHM from it; move secret management out of source by loading the signing key from configuration or environment at runtime (e.g., System.getenv or injected application properties) and initialize ALGORITHM from that value instead of the SECRET constant, and fail-fast or throw a clear error if the configured secret is missing/empty so no default secret remains in code; ensure references to SECRET and ALGORITHM are updated to use the runtime-loaded value.
69-71:⚠️ Potential issue | 🟠 MajorValidate the issuer during verification.
generateToken()stampsISSUER, butverifyToken()does not require it. Add.withIssuer(ISSUER)beforebuild()so the verifier rejects tokens signed with the same key but minted for a different issuer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java` around lines 69 - 71, The token verifier in JwtUtils (where DecodedJWT decodedObj is created via JWT.require(ALGORITHM).build().verify(token)) does not validate the issuer; update the verifier used in the verifyToken method to include .withIssuer(ISSUER) before .build() so tokens are rejected if their ISSUER claim does not match the one stamped by generateToken().
73-81:⚠️ Potential issue | 🟠 MajorDon't write bearer tokens to the logs.
These warning paths still log the full JWT. Keep the exception details, but remove
tokenfrom the log line or replace it with a non-secret fingerprint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java` around lines 73 - 81, In JwtUtils.java update the catch blocks (TokenExpiredException, JWTDecodeException, and the generic Exception handler) so log.warn no longer prints the full JWT token; instead log only the exception and a non-secret fingerprint (e.g., a deterministic hash like SHA-256 or first 8 chars of a secure HMAC) computed from the token. Locate the log.warn calls in the catch blocks around TokenExpiredException, JWTDecodeException and the generic Exception and replace the token interpolation with the fingerprint or omit the token entirely, keeping the thrown exceptions unchanged.cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java (1)
149-156:⚠️ Potential issue | 🟠 MajorDon't mark login successful from the outbound write.
loginServer(...).sync()only waits for the auth request to be written. The actual acceptance happens later incim-server/src/main/java/com/crossoverjie/cim/server/handle/ClientAuthHandler.java:80-91, so completingfutureand settingserverInfohere can still report a successful login for a token the server rejects. The catch path also still missesfuture.completeExceptionally(...), so callers can hang on failures.Also applies to: 207-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java` around lines 149 - 156, The code currently treats the outbound write as successful auth: do not call future.complete(true) or set serverInfo after loginServer(...) returns, because loginServer(...).sync() only waits for the request to be written; instead, move completion and serverInfo assignment to the actual auth-success callback (the handler that processes server acceptance), and ensure failures complete the future exceptionally; also update the catch to call future.completeExceptionally(e) in addition to conf.getEvent().fatal(this). Apply the same change to the other similar block referenced (the code around 207-230) so callers only observe success when the server's auth-accept event is received.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.java`:
- Around line 149-156: The code currently treats the outbound write as
successful auth: do not call future.complete(true) or set serverInfo after
loginServer(...) returns, because loginServer(...).sync() only waits for the
request to be written; instead, move completion and serverInfo assignment to the
actual auth-success callback (the handler that processes server acceptance), and
ensure failures complete the future exceptionally; also update the catch to call
future.completeExceptionally(e) in addition to conf.getEvent().fatal(this).
Apply the same change to the other similar block referenced (the code around
207-230) so callers only observe success when the server's auth-accept event is
received.
In `@cim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.java`:
- Around line 29-31: The JwtUtils class currently hard-codes the HMAC secret via
the SECRET constant and builds ALGORITHM from it; move secret management out of
source by loading the signing key from configuration or environment at runtime
(e.g., System.getenv or injected application properties) and initialize
ALGORITHM from that value instead of the SECRET constant, and fail-fast or throw
a clear error if the configured secret is missing/empty so no default secret
remains in code; ensure references to SECRET and ALGORITHM are updated to use
the runtime-loaded value.
- Around line 69-71: The token verifier in JwtUtils (where DecodedJWT decodedObj
is created via JWT.require(ALGORITHM).build().verify(token)) does not validate
the issuer; update the verifier used in the verifyToken method to include
.withIssuer(ISSUER) before .build() so tokens are rejected if their ISSUER claim
does not match the one stamped by generateToken().
- Around line 73-81: In JwtUtils.java update the catch blocks
(TokenExpiredException, JWTDecodeException, and the generic Exception handler)
so log.warn no longer prints the full JWT token; instead log only the exception
and a non-secret fingerprint (e.g., a deterministic hash like SHA-256 or first 8
chars of a secure HMAC) computed from the token. Locate the log.warn calls in
the catch blocks around TokenExpiredException, JWTDecodeException and the
generic Exception and replace the token interpolation with the fingerprint or
omit the token entirely, keeping the thrown exceptions unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7758c857-b9f0-485b-a1c4-b0d16b424451
📒 Files selected for processing (4)
cim-client-sdk/src/main/java/com/crossoverjie/cim/client/sdk/impl/ClientImpl.javacim-common/src/main/java/com/crossoverjie/cim/common/auth/JwtUtils.javacim-server/src/main/java/com/crossoverjie/cim/server/CIMServerApplication.javacim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java
🚧 Files skipped from review as they are similar to previous changes (2)
- cim-server/src/main/java/com/crossoverjie/cim/server/server/CIMServer.java
- cim-server/src/main/java/com/crossoverjie/cim/server/CIMServerApplication.java
added 2 commits
April 10, 2026 15:14
- 使用 serverConfig.getHost() 替代 InetAddress.getLocalHost().getHostAddress()
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java (1)
31-35:⚠️ Potential issue | 🟠 MajorPrefer configured host first; current auto-probing can break JWT auth.
Line 33 unconditionally probes interfaces, which is brittle in Docker/multi-NIC setups. Since auth does exact host matching, this can reject valid tokens at runtime.
Suggested fix
import org.springframework.beans.factory.annotation.Value; import org.springframework.context.annotation.Configuration; +import org.springframework.util.StringUtils; @@ `@Value`("${cim.server.port}") private int nettyPort; + + `@Value`("${cim.server.host:}") + private String configuredHost; @@ `@PostConstruct` public void init() throws Exception { - this.host = getLocalHostAddress(); + this.host = StringUtils.hasText(configuredHost) ? configuredHost : getLocalHostAddress(); log.info("当前服务配置, host={}, port={}", host, nettyPort); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java` around lines 31 - 35, init() currently always overwrites the configured host by calling getLocalHostAddress(), which breaks exact-host JWT matching in multi-NIC/Docker environments; change init() so it only probes and assigns this.host via getLocalHostAddress() when the configured host is null/empty (trimmed), otherwise keep the configured value; reference the init() method, the host field, and getLocalHostAddress() to locate where to add the conditional check and avoid overwriting an explicit configuration (ensure empty-string checks are handled).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java`:
- Around line 41-63: getLocalHostAddress() currently only skips loopback and
down interfaces, so it can return virtual/container/bridge addresses (e.g.,
docker0) and break host matching; update the method (in getLocalHostAddress) to
also skip NetworkInterface instances where networkInterface.isVirtual() is true
and to filter out common virtual/container interface name patterns (e.g., names
starting with "docker", "veth", "br-") or display names indicating virtual
adapters before iterating addresses, keeping the existing checks
(isLoopback/isUp) and logging behavior.
---
Duplicate comments:
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java`:
- Around line 31-35: init() currently always overwrites the configured host by
calling getLocalHostAddress(), which breaks exact-host JWT matching in
multi-NIC/Docker environments; change init() so it only probes and assigns
this.host via getLocalHostAddress() when the configured host is null/empty
(trimmed), otherwise keep the configured value; reference the init() method, the
host field, and getLocalHostAddress() to locate where to add the conditional
check and avoid overwriting an explicit configuration (ensure empty-string
checks are handled).
🪄 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: c7bde790-bac2-4c8a-9aff-3e0114a0810c
📒 Files selected for processing (1)
cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java
Comment on lines
+41
to
+63
| private String getLocalHostAddress() throws Exception { | ||
| try { | ||
| Enumeration<NetworkInterface> interfaces = NetworkInterface.getNetworkInterfaces(); | ||
| while (interfaces.hasMoreElements()) { | ||
| NetworkInterface networkInterface = interfaces.nextElement(); | ||
| // 只跳过回环接口和未启用的接口 | ||
| if (networkInterface.isLoopback() || !networkInterface.isUp()) { | ||
| continue; | ||
| } | ||
|
|
||
| Enumeration<InetAddress> addresses = networkInterface.getInetAddresses(); | ||
| while (addresses.hasMoreElements()) { | ||
| InetAddress address = addresses.nextElement(); | ||
| String hostAddress = address.getHostAddress(); | ||
|
|
||
| // 跳过IPv6地址 | ||
| if (hostAddress.contains(":")) { | ||
| continue; | ||
| } | ||
|
|
||
| // 跳过127.0.0.1 | ||
| if (!hostAddress.equals("127.0.0.1")) { | ||
| log.debug("找到网络接口: {}, 地址: {}", networkInterface.getName(), hostAddress); |
Contributor
There was a problem hiding this comment.
getLocalHostAddress() does not enforce its own “non-virtual” contract.
Line 39 states virtual interfaces should be excluded, but Lines 47-48 only skip loopback/down interfaces. This can pick docker0/bridge IPs and cause downstream host mismatch in auth/registry.
Suggested fix
- // 只跳过回环接口和未启用的接口
- if (networkInterface.isLoopback() || !networkInterface.isUp()) {
+ // 跳过回环、虚拟、未启用接口
+ if (networkInterface.isLoopback() || networkInterface.isVirtual() || !networkInterface.isUp()) {
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private String getLocalHostAddress() throws Exception { | |
| try { | |
| Enumeration<NetworkInterface> interfaces = NetworkInterface.getNetworkInterfaces(); | |
| while (interfaces.hasMoreElements()) { | |
| NetworkInterface networkInterface = interfaces.nextElement(); | |
| // 只跳过回环接口和未启用的接口 | |
| if (networkInterface.isLoopback() || !networkInterface.isUp()) { | |
| continue; | |
| } | |
| Enumeration<InetAddress> addresses = networkInterface.getInetAddresses(); | |
| while (addresses.hasMoreElements()) { | |
| InetAddress address = addresses.nextElement(); | |
| String hostAddress = address.getHostAddress(); | |
| // 跳过IPv6地址 | |
| if (hostAddress.contains(":")) { | |
| continue; | |
| } | |
| // 跳过127.0.0.1 | |
| if (!hostAddress.equals("127.0.0.1")) { | |
| log.debug("找到网络接口: {}, 地址: {}", networkInterface.getName(), hostAddress); | |
| private String getLocalHostAddress() throws Exception { | |
| try { | |
| Enumeration<NetworkInterface> interfaces = NetworkInterface.getNetworkInterfaces(); | |
| while (interfaces.hasMoreElements()) { | |
| NetworkInterface networkInterface = interfaces.nextElement(); | |
| // 跳过回环、虚拟、未启用接口 | |
| if (networkInterface.isLoopback() || networkInterface.isVirtual() || !networkInterface.isUp()) { | |
| continue; | |
| } | |
| Enumeration<InetAddress> addresses = networkInterface.getInetAddresses(); | |
| while (addresses.hasMoreElements()) { | |
| InetAddress address = addresses.nextElement(); | |
| String hostAddress = address.getHostAddress(); | |
| // 跳过IPv6地址 | |
| if (hostAddress.contains(":")) { | |
| continue; | |
| } | |
| // 跳过127.0.0.1 | |
| if (!hostAddress.equals("127.0.0.1")) { | |
| log.debug("找到网络接口: {}, 地址: {}", networkInterface.getName(), hostAddress); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java`
around lines 41 - 63, getLocalHostAddress() currently only skips loopback and
down interfaces, so it can return virtual/container/bridge addresses (e.g.,
docker0) and break host matching; update the method (in getLocalHostAddress) to
also skip NetworkInterface instances where networkInterface.isVirtual() is true
and to filter out common virtual/container interface name patterns (e.g., names
starting with "docker", "veth", "br-") or display names indicating virtual
adapters before iterating addresses, keeping the existing checks
(isLoopback/isUp) and logging behavior.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #189 +/- ##
============================================
+ Coverage 52.53% 52.84% +0.31%
- Complexity 342 353 +11
============================================
Files 79 83 +4
Lines 1738 1811 +73
Branches 134 134
============================================
+ Hits 913 957 +44
- Misses 783 811 +28
- Partials 42 43 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| private static final long serialVersionUID = 1L; | ||
|
|
||
| /** | ||
| * 用户id |
Owner
There was a problem hiding this comment.
Change all the comments to English
| SessionSocketHolder.put(msg.getRequestId(), (NioSocketChannel) ctx.channel()); | ||
| SessionSocketHolder.saveSession(msg.getRequestId(), msg.getReqMsg()); | ||
| SessionSocketHolder.put(req.getUserId(), (NioSocketChannel) ctx.channel()); | ||
| SessionSocketHolder.saveSession(req.getUserId(), req.getUserName()); |
Owner
There was a problem hiding this comment.
Why use the userId replace the requestId
Contributor
Author
There was a problem hiding this comment.
This userId is what truly represents "which user this channel belongs to", requestId is a protocol-layer request ID
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java (2)
32-32: Avoidthrows Exceptionon a@PostConstructmethod.
NetworkInterface.getNetworkInterfaces()throwsSocketException, and the body already swallows it in atry/catch. The only remaining checked throw isInetAddress.getLocalHost()in the fallback (UnknownHostException). Narrow the signature tothrows UnknownHostException(or catch it and log, returning a safe default) so startup failure modes are explicit rather than opaque.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java` at line 32, The `@PostConstruct` method init in ServerConfig currently declares throws Exception; narrow this to either throws UnknownHostException or, better, remove the checked exception entirely by catching UnknownHostException inside init (or the fallback call to InetAddress.getLocalHost()), log a clear warning and return a safe default instead of propagating Exception. Locate the init method in ServerConfig and replace the broad throws Exception with the specific UnknownHostException or add a try/catch around the fallback InetAddress.getLocalHost() call to handle/log the UnknownHostException and ensure init does not throw a generic Exception.
60-65: Redundant 127.0.0.1 guard.Loopback interfaces are already skipped at L47, so the
!hostAddress.equals("127.0.0.1")check here is effectively dead (any127.xaddress would have been onlo). The inverted condition also makes the control flow harder to read — simplify to a directreturn hostAddress;after the IPv6 check, or replace withaddress.isLoopbackAddress()/address.isAnyLocalAddress()if you want an address-level safety net.♻️ Proposed simplification
- // Skip 127.0.0.1 - if (!hostAddress.equals("127.0.0.1")) { - log.debug("Found network interface: {}, address: {}", networkInterface.getName(), hostAddress); - return hostAddress; - } + if (address.isLoopbackAddress() || address.isAnyLocalAddress()) { + continue; + } + log.debug("Found network interface: {}, address: {}", networkInterface.getName(), hostAddress); + return hostAddress;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java` around lines 60 - 65, The extra check !hostAddress.equals("127.0.0.1") is redundant because loopback interfaces are already skipped earlier; inside the loop after the IPv6 check simply return hostAddress (or, if you prefer an address-level guard, replace the string check with address.isLoopbackAddress() or address.isAnyLocalAddress()). Update the block around networkInterface / address / hostAddress in ServerConfig (the loop that inspects networkInterface.getInetAddresses()) to remove the inverted 127.0.0.1 condition and return hostAddress directly (or use address.isLoopbackAddress()/isAnyLocalAddress() for an explicit safety check).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java`:
- Line 32: The `@PostConstruct` method init in ServerConfig currently declares
throws Exception; narrow this to either throws UnknownHostException or, better,
remove the checked exception entirely by catching UnknownHostException inside
init (or the fallback call to InetAddress.getLocalHost()), log a clear warning
and return a safe default instead of propagating Exception. Locate the init
method in ServerConfig and replace the broad throws Exception with the specific
UnknownHostException or add a try/catch around the fallback
InetAddress.getLocalHost() call to handle/log the UnknownHostException and
ensure init does not throw a generic Exception.
- Around line 60-65: The extra check !hostAddress.equals("127.0.0.1") is
redundant because loopback interfaces are already skipped earlier; inside the
loop after the IPv6 check simply return hostAddress (or, if you prefer an
address-level guard, replace the string check with address.isLoopbackAddress()
or address.isAnyLocalAddress()). Update the block around networkInterface /
address / hostAddress in ServerConfig (the loop that inspects
networkInterface.getInetAddresses()) to remove the inverted 127.0.0.1 condition
and return hostAddress directly (or use
address.isLoopbackAddress()/isAnyLocalAddress() for an explicit safety check).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4780c9d9-87f0-44b7-bb84-91531c062ef0
📒 Files selected for processing (1)
cim-server/src/main/java/com/crossoverjie/cim/server/config/ServerConfig.java
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
2 participants
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.
send authentication token as soon as the channel is available
Summary by CodeRabbit