feat: add WebSocket security vulnerability module (Issue #527)#597
feat: add WebSocket security vulnerability module (Issue #527)#597cisaula wants to merge 2 commits into
Conversation
WebSocket and Authentication entries
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #597 +/- ##
============================================
+ Coverage 51.21% 51.67% +0.46%
- Complexity 465 486 +21
============================================
Files 70 76 +6
Lines 2718 2775 +57
Branches 287 288 +1
============================================
+ Hits 1392 1434 +42
- Misses 1195 1209 +14
- Partials 131 132 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
preetkaran20
left a comment
preetkaran20
left a comment
There was a problem hiding this comment.
This is a strong functional addition and the level progression is well thought out.
One architectural concern: unlike REST vulnerabilities which are annotation-driven (@AttackVector, @VulnerableAppRequestMapping), WebSocket handlers are manually registered via WebSocketConfigurer. This creates an inconsistency in how vulnerability modules are defined and extended.
Suggestion:
- Introduce a @VulnerableWebSocketEndpoint annotation (similar to REST)
- Auto-register handlers by scanning annotated beans (you can look at existing code where we are scanning annotations)
- Align WebSocket handlers with @AttackVector metadata instead of duplicating payload/config in multiple places
This would make WebSocket vulnerabilities first-class citizens in the platform and improve extensibility and maintainability.
| vulnerabilityExposed = VulnerabilityType.WEBSOCKET_MISSING_AUTHENTICATION, | ||
| description = "WEBSOCKET_NO_AUTHENTICATION", | ||
| payload = "WEBSOCKET_PAYLOAD_LEVEL_1") | ||
| @VulnerableAppRequestMapping(value = LevelConstants.LEVEL_1, htmlTemplate = "LEVEL_1/WebSocket") |
There was a problem hiding this comment.
I think we can create annotations for Websocket as well similar to how rest is being handled.
| value = "WebSocketVulnerability") | ||
| public class WebSocketVulnerability { | ||
|
|
||
| private static final String WS_LEVEL1_PATH = "/ws/websocket-vulnerability/level1"; |
There was a problem hiding this comment.
These paths can be derived from the annotations.
| @Override | ||
| public void afterConnectionEstablished(WebSocketSession session) throws Exception { | ||
| String query = session.getUri() != null ? session.getUri().getQuery() : null; | ||
| if (query == null || !query.contains("token=" + VALID_TOKEN)) { |
There was a problem hiding this comment.
I think better way is to use spring utility to parse the query params. This is a bit fragile way to handle token.
| if (!config) return; | ||
|
|
||
| const wsScheme = window.location.protocol === "https:" ? "wss" : "ws"; | ||
| let wsUrl = |
There was a problem hiding this comment.
can you please look at VulnerableApp.js and see if any of the methods you can use to construct entire url?
|
|
||
| log("Connecting to: " + wsUrl, "info"); | ||
|
|
||
| try { |
There was a problem hiding this comment.
can you please abstract it out as a method.
| if (config.requiresToken) { | ||
| const token = document.getElementById("wsToken").value.trim(); | ||
| if (token) { | ||
| wsUrl += "?token=" + encodeURIComponent(token); |
There was a problem hiding this comment.
can we have this not as part of URL as this is security risk. may be create a level where it is not sent as query param?
Why not in query param? Query params are logged by various things like Load balancer, reverse proxy etc so it is not good to send sensitive information in the url. also this might get retained by browser too.
| } | ||
| } | ||
|
|
||
| log("Connecting to: " + wsUrl, "info"); |
| } | ||
|
|
||
| @Override | ||
| protected void handleTextMessage(WebSocketSession session, TextMessage message) |
There was a problem hiding this comment.
even if this is unauthenticated but it is not doing anything harmful. we should give user a way to exploit this level too.
| String payload = message.getPayload(); | ||
| LOGGER.info("Received message without validation: {}", payload); | ||
| // Vulnerability: user input reflected directly without sanitization | ||
| session.sendMessage(new TextMessage("Server processed: " + payload)); |
There was a problem hiding this comment.
how can we exploit it will XSS payload?
Similarly with Drop table payload? I think we should either tie it and make it exploitable.
|
@cisaula are you working on this? |
3 participants
Closes #527
Adds a new WebSocket Security Vulnerability module with 4 levels demonstrating common WebSocket security flaws and their secure fix.
Level 1 (CWE-306): Unauthenticated WebSocket — endpoint accepts all connections without any authentication
Level 2 (CWE-20): Message Injection — user input reflected back without sanitization, enabling XSS and injection attacks
Level 3 (CWE-346): No Origin Validation — server never checks the Origin header, enabling Cross-Site WebSocket Hijacking
Level 4 (Secure): Token-based authentication, origin restricted to localhost, HTML sanitization via StringEscapeUtils.escapeHtml4()
Testing
33 unit tests added (21 handler tests + 12 controller tests), all passing
Manually tested all 4 levels in browser against running app
Follows existing project patterns: @VulnerableAppRestController, @AttackVector, @VulnerableAppRequestMapping