fix: address critical security vulnerabilities#2
fix: address critical security vulnerabilities#2devin-ai-integration[bot] wants to merge 1 commit into
Conversation
- Remove unsafe JDK deserialization serializer (CWE-502, RCE risk) - Replace arbitrary Class.forName().newInstance() with allowlist validation (CWE-470) - Remove debug main() methods from production classes (MsgId, MsgIdGenerator, Score) - Remove debug print() methods from production ScriptHelper classes - Pin JUnit dependency to 4.13.2 (was RELEASE - non-reproducible builds) - Add null-safety check for @MessageHandlerBind annotation in ItQueueAutoConfig - Log swallowed SecurityException/Exception in ProcessID instead of silently ignoring - Update test files to use System.out.println after removing print() helpers Co-Authored-By: kenlin <kenlin8827@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (!ALLOWED_SERIALIZERS.contains(name)) { | ||
| throw new IllegalArgumentException( | ||
| "Unsupported serializer '" + name + "'. Allowed values: " + ALLOWED_SERIALIZERS); | ||
| } | ||
| throw new IllegalArgumentException("Unsupported serializer '" + name + "'"); |
There was a problem hiding this comment.
🟡 Unreachable code path due to ALLOWED_SERIALIZERS containing only already-handled values
In RedisSerializerFactory.getSerializer, after the if (name.equals("string")) and if (name.equals("json")) blocks return early, any remaining name is guaranteed to NOT be in ALLOWED_SERIALIZERS (which only contains "string" and "json"). This means !ALLOWED_SERIALIZERS.contains(name) on line 30 is always true, so line 32 always throws, making line 34 unreachable dead code. The behavior is functionally correct for all inputs, but the ALLOWED_SERIALIZERS guard is redundant and the final throw can never execute, indicating the code structure doesn't match the developer's likely intent.
| if (!ALLOWED_SERIALIZERS.contains(name)) { | |
| throw new IllegalArgumentException( | |
| "Unsupported serializer '" + name + "'. Allowed values: " + ALLOWED_SERIALIZERS); | |
| } | |
| throw new IllegalArgumentException("Unsupported serializer '" + name + "'"); | |
| throw new IllegalArgumentException( | |
| "Unsupported serializer '" + name + "'. Allowed values: " + ALLOWED_SERIALIZERS); |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Security audit of the itqueue codebase, fixing critical and medium-severity issues:
Critical fixes:
RedisSerializerFactory): The"jdk"serializer option usedJdkSerializationRedisSerializer, which is vulnerable to Remote Code Execution via Java deserialization gadget chains (CWE-502).RedisSerializerFactory):Class.forName(name).newInstance()allowed loading any class on the classpath. Replaced with a strict allowlist that only permits"string"and"json"serializers.Medium fixes:
main()debug methods from production classes (MsgId,MsgIdGenerator,Score).print()debug helpers fromRedisListScriptHelperandRedisZSetScriptHelper(leak message payloads to stdout).@MessageHandlerBindannotation lookup inItQueueAutoConfig.ProcessIDwith SLF4Jwarn-level logging.RELEASE(non-reproducible) to4.13.2.Test files updated to replace removed
print()calls withSystem.out.println.Review & Testing Checklist for Human
"jdk"serializer and customClass.forNameloading is intentionally breaking. Verify no existing deployments useserializer=jdkor a custom class name in their queue configuration. Consider whether a deprecation warning (rather than immediate removal) is more appropriate for your users.RedisSerializerFactorydead code: TheALLOWED_SERIALIZERSset and the check on line 30 are redundant —"string"and"json"are already handled by theifblocks above, so any name reaching the allowlist check is guaranteed to not be in it. The finalthrowon line 34 is unreachable dead code. Functionally correct but the logic should be simplified.ItQueueAutoConfignow silentlycontinues whengetHandlerAnnotation()returns null. Consider whether a warning log would be better to surface misconfiguredMessageHandlerbeans.mvn compilewas verified (tests require a live Redis connection).Notes
ConsumerManager.findConsumers, test filemain()methods) are documented there but not fixed in this PR.maven.compiler.source/target), but the build environment used Java 17. Compilation succeeded but runtime compatibility should be confirmed.Link to Devin session: https://app.devin.ai/sessions/c68aa64efbfd40779c90b764de3bcba1
Requested by: @kenlin8827