Added Session Management Vulnerability#653
Open
CHI7440 wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR implements a complete session management vulnerability module for VulnerableApp, adding five progressive security levels that demonstrate common session handling flaws: session fixation, predictable token generation, missing logout invalidation, absence of rate limiting, and secure (correct) session practices. The implementation spans backend service logic with in-memory session storage, REST endpoints, frontend UI templates for each level, database support, and comprehensive tests. ChangesSession Management Vulnerabilities
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/sasanlabs/service/vulnerability/sessionManagement/SessionManagementService.java`:
- Around line 167-179: The profile method currently calls
sessions.containsKey(sessionId) then sessions.get(sessionId), which can race
with logout and yield a null SessionUser; change profile to read the map once
into a local (SessionUser user = sessions.get(sessionId)), validate sessionId
(null/blank) and that user != null, and only then access
user.getId()/getUsername()/getRole(); update the method to return the same error
response when the retrieved user is null to make the lookup atomic and avoid
NPEs.
- Around line 126-145: level5Login currently accesses level5FailedAttempts with
the raw username which throws if username is null; add a null/empty guard at the
start of level5Login (before any getOrDefault or merge) and treat a missing
username the same as invalid credentials (return the same response structure:
message "Invalid credentials", failedAttempts omitted or zero,
rateLimitingApplied true) instead of touching level5FailedAttempts; ensure
subsequent calls to level5FailedAttempts.merge(username, ...) only run when
username is non-null (or use a non-null local key after the guard) so
SessionManagementVulnerability.level5SecureSessionManagement's optional username
won't cause a 500.
- Around line 149-151: The code currently removes an entry from the shared
sessions map using the untrusted incomingSessionId
(sessions.remove(incomingSessionId)), which allows a client to revoke other
users' sessions; in SessionManagementService locate the method that handles
login/session regeneration where incomingSessionId is used and remove the
unconditional sessions.remove(incomingSessionId) call, instead always generate
and store a fresh session id for the new login (and return the new token), and
only revoke or replace an existing session after verifying the incomingSessionId
belongs to the authenticated user (e.g., compare the stored session's user
identifier to the authenticated principal) before deleting from the sessions
map.
In
`@src/main/resources/static/templates/SessionManagementVulnerability/LEVEL_1/SessionManagement.css`:
- Around line 97-99: Replace the deprecated property "word-break: break-word" in
SessionManagement.css with a combination of a non-deprecated word-break value
and overflow-wrap to preserve wrapping: remove "word-break: break-word", add
"overflow-wrap: anywhere;" and set "word-break: normal;" (or another appropriate
non-deprecated value) in the same CSS rule where "white-space: pre-wrap;" was
defined so stylelint passes and wrapping behavior is preserved.
In
`@src/main/resources/static/templates/SessionManagementVulnerability/LEVEL_2/PredictableSessionId.css`:
- Around line 97-99: Replace the deprecated CSS property by removing
"word-break: break-word" and use "overflow-wrap: break-word" in the same rule
where "white-space: pre-wrap" is declared (look for the rule containing
white-space: pre-wrap; and the string "word-break: break-word"); update that
rule to include overflow-wrap: break-word to preserve wrapping behavior and
satisfy linters.
In
`@src/main/resources/static/templates/SessionManagementVulnerability/LEVEL_3/MissingLogoutInvalidation.css`:
- Line 98: The CSS rule using the deprecated declaration "word-break:
break-word;" in MissingLogoutInvalidation.css should be replaced with modern
wrapping properties; remove the deprecated line and add "overflow-wrap:
anywhere;" (and, if you need more aggressive breaking for long strings, consider
also adding "word-break: break-word" is deprecated so prefer "word-break:
break-all" only when appropriate), ensuring the selector that contained
"word-break: break-word;" now uses "overflow-wrap: anywhere;" to satisfy
stylelint and current spec.
In
`@src/main/resources/static/templates/SessionManagementVulnerability/LEVEL_4/NoLoginRateLimiting.css`:
- Line 98: Replace the deprecated CSS rule "word-break: break-word" in
NoLoginRateLimiting.css with a standards-compliant wrapping rule: remove the
"word-break: break-word" declaration and add "overflow-wrap: anywhere" (and
optionally "word-wrap: break-word" for legacy support) in the same selector so
text wrapping behavior is preserved without using the deprecated property.
In
`@src/main/resources/static/templates/SessionManagementVulnerability/LEVEL_5/SecureSessionManagement.css`:
- Line 98: The CSS contains the deprecated rule "word-break: break-word;" —
replace it with the standards-aligned property "overflow-wrap: anywhere;" (and
remove or set "word-break" to a normal value if present) so stylelint stops
flagging the rule and wrapping behavior remains the same; update the occurrence
of the "word-break: break-word;" declaration in SecureSessionManagement.css
accordingly.
🪄 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 Plus
Run ID: 410b828d-9adb-4288-9bd4-9c2afb9c12a8
📒 Files selected for processing (26)
src/main/java/org/sasanlabs/configuration/VulnerableAppConfiguration.javasrc/main/java/org/sasanlabs/service/vulnerability/sessionManagement/SessionManagementService.javasrc/main/java/org/sasanlabs/service/vulnerability/sessionManagement/SessionManagementVulnerability.javasrc/main/java/org/sasanlabs/service/vulnerability/sessionManagement/SessionUser.javasrc/main/java/org/sasanlabs/service/vulnerability/sessionManagement/SessionUserRepository.javasrc/main/java/org/sasanlabs/vulnerability/types/VulnerabilityType.javasrc/main/resources/attackvectors/SessionManagementVulnerabilityPayload.propertiessrc/main/resources/i18n/messages.propertiessrc/main/resources/scripts/SessionManagement/db/data.sqlsrc/main/resources/scripts/SessionManagement/db/schema.sqlsrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_1/SessionManagement.csssrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_1/SessionManagement.htmlsrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_1/SessionManagement.jssrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_2/PredictableSessionId.csssrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_2/PredictableSessionId.htmlsrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_2/PredictableSessionId.jssrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_3/MissingLogoutInvalidation.csssrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_3/MissingLogoutInvalidation.htmlsrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_3/MissingLogoutInvalidation.jssrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_4/NoLoginRateLimiting.csssrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_4/NoLoginRateLimiting.htmlsrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_4/NoLoginRateLimiting.jssrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_5/SecureSessionManagement.csssrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_5/SecureSessionManagement.htmlsrc/main/resources/static/templates/SessionManagementVulnerability/LEVEL_5/SecureSessionManagement.jssrc/test/java/org/sasanlabs/service/vulnerability/sessionManagement/SessionManagementServiceTest.java
Comment on lines
+126
to
+145
| public ResponseEntity<GenericVulnerabilityResponseBean<Object>> level5Login( | ||
| String username, String password, String incomingSessionId) { | ||
| if (level5FailedAttempts.getOrDefault(username, 0) >= 3) { | ||
| Map<String, Object> content = new LinkedHashMap<>(); | ||
| content.put("message", "Too many failed attempts"); | ||
| content.put("rateLimitingApplied", true); | ||
| content.put("loginBlocked", true); | ||
| return response(content, false); | ||
| } | ||
|
|
||
| Optional<SessionUser> user = | ||
| sessionUserRepository.findByUsernameAndPassword(username, password); | ||
| if (user.isEmpty()) { | ||
| level5FailedAttempts.merge(username, 1, Integer::sum); | ||
| Map<String, Object> content = new LinkedHashMap<>(); | ||
| content.put("message", "Invalid credentials"); | ||
| content.put("failedAttempts", level5FailedAttempts.get(username)); | ||
| content.put("rateLimitingApplied", true); | ||
| return response(content, false); | ||
| } |
There was a problem hiding this comment.
Guard LEVEL_5 login before touching level5FailedAttempts.
SessionManagementVulnerability.level5SecureSessionManagement passes username as an optional request param, but level5FailedAttempts.getOrDefault(username, 0) and merge(username, ...) will throw when username is null because ConcurrentHashMap does not accept null keys. A request like LEVEL_5?action=login with a missing username currently returns 500 instead of a normal invalid-credentials response.
Proposed fix
public ResponseEntity<GenericVulnerabilityResponseBean<Object>> level5Login(
String username, String password, String incomingSessionId) {
+ if (username == null || username.isBlank() || password == null || password.isBlank()) {
+ return response("Invalid credentials", false);
+ }
+
if (level5FailedAttempts.getOrDefault(username, 0) >= 3) {
Map<String, Object> content = new LinkedHashMap<>();
content.put("message", "Too many failed attempts");📝 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
| public ResponseEntity<GenericVulnerabilityResponseBean<Object>> level5Login( | |
| String username, String password, String incomingSessionId) { | |
| if (level5FailedAttempts.getOrDefault(username, 0) >= 3) { | |
| Map<String, Object> content = new LinkedHashMap<>(); | |
| content.put("message", "Too many failed attempts"); | |
| content.put("rateLimitingApplied", true); | |
| content.put("loginBlocked", true); | |
| return response(content, false); | |
| } | |
| Optional<SessionUser> user = | |
| sessionUserRepository.findByUsernameAndPassword(username, password); | |
| if (user.isEmpty()) { | |
| level5FailedAttempts.merge(username, 1, Integer::sum); | |
| Map<String, Object> content = new LinkedHashMap<>(); | |
| content.put("message", "Invalid credentials"); | |
| content.put("failedAttempts", level5FailedAttempts.get(username)); | |
| content.put("rateLimitingApplied", true); | |
| return response(content, false); | |
| } | |
| public ResponseEntity<GenericVulnerabilityResponseBean<Object>> level5Login( | |
| String username, String password, String incomingSessionId) { | |
| if (username == null || username.isBlank() || password == null || password.isBlank()) { | |
| return response("Invalid credentials", false); | |
| } | |
| if (level5FailedAttempts.getOrDefault(username, 0) >= 3) { | |
| Map<String, Object> content = new LinkedHashMap<>(); | |
| content.put("message", "Too many failed attempts"); | |
| content.put("rateLimitingApplied", true); | |
| content.put("loginBlocked", true); | |
| return response(content, false); | |
| } | |
| Optional<SessionUser> user = | |
| sessionUserRepository.findByUsernameAndPassword(username, password); | |
| if (user.isEmpty()) { | |
| level5FailedAttempts.merge(username, 1, Integer::sum); | |
| Map<String, Object> content = new LinkedHashMap<>(); | |
| content.put("message", "Invalid credentials"); | |
| content.put("failedAttempts", level5FailedAttempts.get(username)); | |
| content.put("rateLimitingApplied", true); | |
| return response(content, false); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/sasanlabs/service/vulnerability/sessionManagement/SessionManagementService.java`
around lines 126 - 145, level5Login currently accesses level5FailedAttempts with
the raw username which throws if username is null; add a null/empty guard at the
start of level5Login (before any getOrDefault or merge) and treat a missing
username the same as invalid credentials (return the same response structure:
message "Invalid credentials", failedAttempts omitted or zero,
rateLimitingApplied true) instead of touching level5FailedAttempts; ensure
subsequent calls to level5FailedAttempts.merge(username, ...) only run when
username is non-null (or use a non-null local key after the guard) so
SessionManagementVulnerability.level5SecureSessionManagement's optional username
won't cause a 500.
Comment on lines
+149
to
+151
| if (incomingSessionId != null && !incomingSessionId.isBlank()) { | ||
| sessions.remove(incomingSessionId); | ||
| } |
There was a problem hiding this comment.
Don’t revoke server sessions based on an untrusted incoming token.
In the secure flow, any incomingSessionId is removed from the shared sessions map before the new session is created. If a client submits another user’s valid token here, this login path force-logs out that unrelated session. Regeneration should issue a fresh token for the new login, not treat the presented cookie as authorization to delete existing server-side state.
Safer direction
level5FailedAttempts.remove(username);
String sessionId = UUID.randomUUID().toString();
- if (incomingSessionId != null && !incomingSessionId.isBlank()) {
- sessions.remove(incomingSessionId);
- }
sessions.put(sessionId, user.get());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/sasanlabs/service/vulnerability/sessionManagement/SessionManagementService.java`
around lines 149 - 151, The code currently removes an entry from the shared
sessions map using the untrusted incomingSessionId
(sessions.remove(incomingSessionId)), which allows a client to revoke other
users' sessions; in SessionManagementService locate the method that handles
login/session regeneration where incomingSessionId is used and remove the
unconditional sessions.remove(incomingSessionId) call, instead always generate
and store a fresh session id for the new login (and return the new token), and
only revoke or replace an existing session after verifying the incomingSessionId
belongs to the authenticated user (e.g., compare the stored session's user
identifier to the authenticated principal) before deleting from the sessions
map.
Comment on lines
+167
to
+179
| public ResponseEntity<GenericVulnerabilityResponseBean<Object>> profile(String sessionId) { | ||
| if (sessionId == null || sessionId.isBlank() || !sessions.containsKey(sessionId)) { | ||
| return response("No valid session found", false); | ||
| } | ||
|
|
||
| SessionUser user = sessions.get(sessionId); | ||
| Map<String, Object> content = new LinkedHashMap<>(); | ||
| content.put("userId", user.getId()); | ||
| content.put("username", user.getUsername()); | ||
| content.put("role", user.getRole()); | ||
| content.put("sessionId", sessionId); | ||
|
|
||
| return response(content, true); |
There was a problem hiding this comment.
Make the session lookup in profile atomic.
profile checks sessions.containsKey(sessionId) and then does a second lookup with sessions.get(sessionId). If logout removes the entry between those calls, user becomes null and Line 174 dereferences it. Read once and validate the retrieved SessionUser instead.
Proposed fix
public ResponseEntity<GenericVulnerabilityResponseBean<Object>> profile(String sessionId) {
- if (sessionId == null || sessionId.isBlank() || !sessions.containsKey(sessionId)) {
+ if (sessionId == null || sessionId.isBlank()) {
return response("No valid session found", false);
}
SessionUser user = sessions.get(sessionId);
+ if (user == null) {
+ return response("No valid session found", false);
+ }
Map<String, Object> content = new LinkedHashMap<>();
content.put("userId", user.getId());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/org/sasanlabs/service/vulnerability/sessionManagement/SessionManagementService.java`
around lines 167 - 179, The profile method currently calls
sessions.containsKey(sessionId) then sessions.get(sessionId), which can race
with logout and yield a null SessionUser; change profile to read the map once
into a local (SessionUser user = sessions.get(sessionId)), validate sessionId
(null/blank) and that user != null, and only then access
user.getId()/getUsername()/getRole(); update the method to return the same error
response when the retrieved user is null to make the lookup atomic and avoid
NPEs.
Comment on lines
+97
to
+99
| white-space: pre-wrap; | ||
| word-break: break-word; | ||
| } |
There was a problem hiding this comment.
Replace deprecated word-break: break-word usage.
Use overflow-wrap + a non-deprecated word-break value to keep wrapping behavior and satisfy stylelint.
Suggested patch
.session-response {
margin: 0;
padding: 8px;
border: 1px solid black;
border-radius: 4px;
background: `#fff`;
color: black;
text-align: left;
white-space: pre-wrap;
- word-break: break-word;
+ overflow-wrap: anywhere;
+ word-break: normal;
}📝 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
| white-space: pre-wrap; | |
| word-break: break-word; | |
| } | |
| white-space: pre-wrap; | |
| overflow-wrap: anywhere; | |
| word-break: normal; | |
| } |
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 98-98: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/resources/static/templates/SessionManagementVulnerability/LEVEL_1/SessionManagement.css`
around lines 97 - 99, Replace the deprecated property "word-break: break-word"
in SessionManagement.css with a combination of a non-deprecated word-break value
and overflow-wrap to preserve wrapping: remove "word-break: break-word", add
"overflow-wrap: anywhere;" and set "word-break: normal;" (or another appropriate
non-deprecated value) in the same CSS rule where "white-space: pre-wrap;" was
defined so stylelint passes and wrapping behavior is preserved.
Source: Linters/SAST tools
Comment on lines
+97
to
+99
| white-space: pre-wrap; | ||
| word-break: break-word; | ||
| } |
There was a problem hiding this comment.
Use non-deprecated text wrapping properties.
word-break: break-word is deprecated; replace it with overflow-wrap to preserve behavior and pass linting.
Suggested patch
.session-response {
margin: 0;
padding: 8px;
border: 1px solid black;
border-radius: 4px;
background: `#fff`;
color: black;
text-align: left;
white-space: pre-wrap;
- word-break: break-word;
+ overflow-wrap: anywhere;
+ word-break: normal;
}📝 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
| white-space: pre-wrap; | |
| word-break: break-word; | |
| } | |
| .session-response { | |
| margin: 0; | |
| padding: 8px; | |
| border: 1px solid black; | |
| border-radius: 4px; | |
| background: `#fff`; | |
| color: black; | |
| text-align: left; | |
| white-space: pre-wrap; | |
| overflow-wrap: anywhere; | |
| word-break: normal; | |
| } |
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 98-98: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/resources/static/templates/SessionManagementVulnerability/LEVEL_2/PredictableSessionId.css`
around lines 97 - 99, Replace the deprecated CSS property by removing
"word-break: break-word" and use "overflow-wrap: break-word" in the same rule
where "white-space: pre-wrap" is declared (look for the rule containing
white-space: pre-wrap; and the string "word-break: break-word"); update that
rule to include overflow-wrap: break-word to preserve wrapping behavior and
satisfy linters.
Source: Linters/SAST tools
| color: black; | ||
| text-align: left; | ||
| white-space: pre-wrap; | ||
| word-break: break-word; |
There was a problem hiding this comment.
Replace deprecated word-break: break-word.
This value is deprecated and already flagged by stylelint; update wrapping rules to avoid lint failure and future CSS behavior drift.
Suggested fix
.session-response {
@@
- word-break: break-word;
+ word-break: normal;
+ overflow-wrap: anywhere;
}📝 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
| word-break: break-word; | |
| word-break: normal; | |
| overflow-wrap: anywhere; |
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 98-98: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/resources/static/templates/SessionManagementVulnerability/LEVEL_3/MissingLogoutInvalidation.css`
at line 98, The CSS rule using the deprecated declaration "word-break:
break-word;" in MissingLogoutInvalidation.css should be replaced with modern
wrapping properties; remove the deprecated line and add "overflow-wrap:
anywhere;" (and, if you need more aggressive breaking for long strings, consider
also adding "word-break: break-word" is deprecated so prefer "word-break:
break-all" only when appropriate), ensuring the selector that contained
"word-break: break-word;" now uses "overflow-wrap: anywhere;" to satisfy
stylelint and current spec.
Source: Linters/SAST tools
| color: black; | ||
| text-align: left; | ||
| white-space: pre-wrap; | ||
| word-break: break-word; |
There was a problem hiding this comment.
Use non-deprecated text wrapping rule.
word-break: break-word is deprecated and may fail linting.
Suggested fix
.session-response {
@@
- word-break: break-word;
+ word-break: normal;
+ overflow-wrap: anywhere;
}📝 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
| word-break: break-word; | |
| .session-response { | |
| word-break: normal; | |
| overflow-wrap: anywhere; | |
| } |
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 98-98: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/resources/static/templates/SessionManagementVulnerability/LEVEL_4/NoLoginRateLimiting.css`
at line 98, Replace the deprecated CSS rule "word-break: break-word" in
NoLoginRateLimiting.css with a standards-compliant wrapping rule: remove the
"word-break: break-word" declaration and add "overflow-wrap: anywhere" (and
optionally "word-wrap: break-word" for legacy support) in the same selector so
text wrapping behavior is preserved without using the deprecated property.
Source: Linters/SAST tools
| color: black; | ||
| text-align: left; | ||
| white-space: pre-wrap; | ||
| word-break: break-word; |
There was a problem hiding this comment.
Fix deprecated CSS keyword in response wrapping.
Replace deprecated word-break: break-word to keep stylelint clean and standards-aligned.
Suggested fix
.session-response {
@@
- word-break: break-word;
+ word-break: normal;
+ overflow-wrap: anywhere;
}📝 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
| word-break: break-word; | |
| word-break: normal; | |
| overflow-wrap: anywhere; |
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 98-98: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/resources/static/templates/SessionManagementVulnerability/LEVEL_5/SecureSessionManagement.css`
at line 98, The CSS contains the deprecated rule "word-break: break-word;" —
replace it with the standards-aligned property "overflow-wrap: anywhere;" (and
remove or set "word-break" to a normal value if present) so stylelint stops
flagging the rule and wrapping behavior remains the same; update the occurrence
of the "word-break: break-word;" declaration in SecureSessionManagement.css
accordingly.
Source: Linters/SAST tools
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #653 +/- ##
============================================
+ Coverage 54.69% 55.11% +0.42%
- Complexity 663 686 +23
============================================
Files 91 95 +4
Lines 3587 3781 +194
Branches 397 414 +17
============================================
+ Hits 1962 2084 +122
- Misses 1448 1509 +61
- Partials 177 188 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Resolves #647
This introduces a new Session Management Vulnerability module to VulnerableApp. The goal is to add session-related security scenarios that were previously missing, while keeping the implementation aligned with the existing VulnerableApp pattern of vulnerable earlier levels and a secure later level.
The new module is exposed as:
It uses annotation-driven discovery through
@VulnerableAppRestController,@VulnerableAppRequestMapping, and@AttackVector, so the new levels appear in the legacy UI, scanner metadata, sitemap, and facade definitions.Implemented Levels
LEVEL_1 - Session FixationDemonstrates CWE-384. The application accepts a client-supplied
VAPP_SESSION_IDbefore login and keeps the same session token after authentication. The response shows the pre-login token, post-login token, and whether session fixation was confirmed.LEVEL_2 - Predictable Session IDsDemonstrates CWE-330. The application generates session IDs using a predictable counter such as
SESSION-1001,SESSION-1002, etc. Users can log in multiple times and check profiles by manually supplied session tokens to demonstrate guessable session IDs.LEVEL_3 - Missing Logout InvalidationDemonstrates CWE-613. Logout clears the client-side cookie but intentionally does not destroy the server-side session. Reusing the old session token after logout still allows profile access.
LEVEL_4 - No Login Rate LimitingDemonstrates CWE-307. The application allows repeated failed login attempts without throttling, lockout, delay, or CAPTCHA. Responses explicitly show that attempts remain allowed and rate limiting is not applied.
LEVEL_5 - Secure Session ManagementAdds a secure comparison level marked with
Variant.SECURE. This level regenerates the session token after login, uses random UUID session IDs, invalidates server-side sessions on logout, and applies a simple username-based failed-login limit.Database Support
Added a new
session_userstable for this module instead of reusingauth_users, keeping session-management scenarios independent from authentication vulnerabilities.Seeded users:
Frontend Templates
Added separate legacy UI templates for each level under:
Each level has focused HTML/CSS/JS to keep the UI simple and avoid level-specific conditionals.
Scanner Metadata
Added new vulnerability types:
Added corresponding attack-vector payloads and i18n descriptions.
Tests
Added focused service tests covering all implemented levels:
The tests verify vulnerable and secure behavior for session fixation, predictable IDs, logout invalidation, missing rate limiting, secure token regeneration, secure logout invalidation, and failed-login blocking.
Password Reset And SMTP Scope
This PR focuses only on session-management vulnerabilities. Password reset vulnerabilities and SMTP-based reset flows are intentionally left out of scope because they represent a separate vulnerability area and require different user flows, backend logic, and possibly SMTP infrastructure.
Those scenarios should be handled in a follow-up issue/PR.
Summary by CodeRabbit