Enhance vulnerability descriptions with Source, Solution, and Reference (Fixes #89) - #735
Open
MixhizoR wants to merge 4 commits into
Open
Conversation
…eference Currently, vulnerability descriptions are a single block of text and do not separate where the vulnerable value was found (e.g., Cookie, URL) from the actual description. This implements a ZAP-style approach by introducing Source, Solution, and Reference fields for Attack Vectors. Changes: - Added `source`, `solution`, and `reference` to the `@AttackVector` annotation and `AttackVectorResponseBean` (with Builder pattern). - Implemented `getMetaInformation` in `EndPointsInformationProvider` to resolve i18n keys while gracefully handling `NOT_APPLICABLE` defaults. - Updated all 16 JWT vulnerability levels in `JWTVulnerability.java` to utilize the new fields with proper, actionable content. - Updated frontend (`vulnerableApp.js`) to render the new fields and implemented `escapeHtml` to prevent XSS via innerHTML injection. - Fixed reverse tabnabbing globally by adding `rel="noopener noreferrer"` to all `target="_blank"` links in `messages.properties`. - Aligned mock JSON (`mockAllEndPointJson.json`) with resolved backend formats. - Added unit tests for `EndPointsInformationProvider.getMetaInformation()`. Closes SasanLabs#89
Resolves SasanLabs#89 Previously, vulnerability descriptions mixed the location of the vulnerable value with the explanation. Following a ZAP-style approach, this separates the Source, Solution, and Reference into distinct fields. Changes: - Added `source`, `solution`, and `reference` fields to `@AttackVector` and `AttackVectorResponseBean`. - Updated all JWT vulnerability levels to utilize the new fields with actionable content. - Added `getMetaInformation` to resolve i18n keys gracefully. - Fixed reverse tabnabbing globally by adding `rel="noopener noreferrer"` to `target="_blank"` links. - Fixed broken 404 reference URLs (OWASP, IETF, CVE). - Updated frontend to render the new fields and safely wrap reference URLs in clickable links without introducing XSS. - Added unit tests for `EndPointsInformationProvider.getMetaInformation()`.
Contributor
📝 WalkthroughWalkthroughAttack-vector responses now include source, solution, and reference metadata. JWT annotations and localization catalogs provide the metadata. The endpoint provider resolves and exposes it. Browser rendering escapes dynamic values and adds secure external links. ChangesAttack-vector metadata
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AttackVector
participant EndPointsInformationProvider
participant MessageBundle
participant Browser
AttackVector->>EndPointsInformationProvider: provide metadata keys
EndPointsInformationProvider->>MessageBundle: resolve localized source, solution, and reference
MessageBundle-->>EndPointsInformationProvider: return localized metadata
EndPointsInformationProvider-->>Browser: return enriched attack-vector response
Browser->>Browser: escape dynamic metadata and render help
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #735 +/- ##
============================================
- Coverage 55.37% 55.04% -0.34%
- Complexity 760 771 +11
============================================
Files 105 105
Lines 4229 4284 +55
Branches 452 456 +4
============================================
+ Hits 2342 2358 +16
- Misses 1683 1722 +39
Partials 204 204 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/jwt/JWTVulnerability.java`:
- Around line 433-436: Update the source metadata in JWTVulnerability.java at
lines 433-436 to use JWT_SOURCE_HEADER because the endpoint reads
HttpHeaders.AUTHORIZATION; update the source metadata at lines 723-728 to use
JWT_SOURCE_COOKIE because that endpoint reads the cookie header.
In `@src/main/resources/i18n/messages_en_US.properties`:
- Line 60: Fix the RFC 2616 anchor in the messages resource by moving the
target="_blank" and rel="noopener noreferrer" attributes inside the opening <a>
tag, preserving the existing URL and link text.
In `@src/main/resources/static/mockAllEndPointJson.json`:
- Around line 20-23: Update the Reference field for all eight mock attack
vectors in the JSON fixture to contain only the URL, removing descriptive OWASP
text and separators while preserving each link’s destination.
🪄 Autofix
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: d9652535-d158-4552-8272-f78b05ca4f1a
📒 Files selected for processing (9)
src/main/java/org/sasanlabs/beans/AttackVectorResponseBean.javasrc/main/java/org/sasanlabs/internal/utility/annotations/AttackVector.javasrc/main/java/org/sasanlabs/service/impl/EndPointsInformationProvider.javasrc/main/java/org/sasanlabs/service/vulnerability/jwt/JWTVulnerability.javasrc/main/resources/i18n/messages.propertiessrc/main/resources/i18n/messages_en_US.propertiessrc/main/resources/static/mockAllEndPointJson.jsonsrc/main/resources/static/vulnerableApp.jssrc/test/java/org/sasanlabs/service/impl/EndPointsInformationProviderTest.java
Comment on lines
+433
to
+436
| description = "COOKIE_CONTAINING_JWT_TOKEN_SECURITY_ATTRIBUTES_MISSING", | ||
| source = "JWT_SOURCE_COOKIE", | ||
| solution = "COOKIE_CONTAINING_JWT_TOKEN_SECURITY_ATTRIBUTES_MISSING_SOLUTION", | ||
| reference = "COOKIE_CONTAINING_JWT_TOKEN_SECURITY_ATTRIBUTES_MISSING_REFERENCE") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align source metadata with the request location.
src/main/java/org/sasanlabs/service/vulnerability/jwt/JWTVulnerability.java#L433-L436: UseJWT_SOURCE_HEADER. The endpoint readsHttpHeaders.AUTHORIZATION.src/main/java/org/sasanlabs/service/vulnerability/jwt/JWTVulnerability.java#L723-L728: UseJWT_SOURCE_COOKIE. The endpoint reads thecookieheader.
📍 Affects 1 file
src/main/java/org/sasanlabs/service/vulnerability/jwt/JWTVulnerability.java#L433-L436(this comment)src/main/java/org/sasanlabs/service/vulnerability/jwt/JWTVulnerability.java#L723-L728
🤖 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/jwt/JWTVulnerability.java`
around lines 433 - 436, Update the source metadata in JWTVulnerability.java at
lines 433-436 to use JWT_SOURCE_HEADER because the endpoint reads
HttpHeaders.AUTHORIZATION; update the source metadata at lines 723-728 to use
JWT_SOURCE_COOKIE because that endpoint reads the cookie header.
| The ability to use an authentic application URL, targeting the correct domain and with a valid SSL certificate (if SSL is used), lends credibility to \ | ||
| the phishing attack because many users, even if they verify these features, will not notice the subsequent redirection to a different domain. \ | ||
| <br/><br/> <a href="https://www.w3.org/Protocols/rfc2616/rfc2616.html">RFC 2616 - "Hypertext Transfer Protocol - HTTP/1.1" target="_blank"</a> defines a variety \ | ||
| <br/><br/> <a href="https://www.w3.org/Protocols/rfc2616/rfc2616.html">RFC 2616 - "Hypertext Transfer Protocol - HTTP/1.1" target="_blank" rel="noopener noreferrer"</a> defines a variety \ |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the malformed RFC 2616 anchor.
Line 60 closes the opening <a> tag before target and rel. The reference opens in the current tab, and noopener noreferrer is not applied.
Proposed fix
-<br/><br/> <a href="https://www.w3.org/Protocols/rfc2616/rfc2616.html">RFC 2616 - "Hypertext Transfer Protocol - HTTP/1.1" target="_blank" rel="noopener noreferrer"</a> defines a variety \
+<br/><br/> <a href="https://www.w3.org/Protocols/rfc2616/rfc2616.html" target="_blank" rel="noopener noreferrer">RFC 2616 - "Hypertext Transfer Protocol - HTTP/1.1"</a> defines a variety \📝 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
| <br/><br/> <a href="https://www.w3.org/Protocols/rfc2616/rfc2616.html">RFC 2616 - "Hypertext Transfer Protocol - HTTP/1.1" target="_blank" rel="noopener noreferrer"</a> defines a variety \ | |
| <br/><br/> <a href="https://www.w3.org/Protocols/rfc2616/rfc2616.html" target="_blank" rel="noopener noreferrer">RFC 2616 - "Hypertext Transfer Protocol - HTTP/1.1"</a> defines a variety \ |
🤖 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/i18n/messages_en_US.properties` at line 60, Fix the RFC
2616 anchor in the messages resource by moving the target="_blank" and
rel="noopener noreferrer" attributes inside the opening <a> tag, preserving the
existing URL and link text.
Comment on lines
+20
to
+23
| "Description": "Mock attack vector for the Show Help (Scanner) view.", | ||
| "Source": "URL (query parameter)", | ||
| "Solution": "Validate the username server side before authenticating the session.", | ||
| "Reference": "OWASP A01 Broken Access Control - https://owasp.org/Top10/A01_2021-Broken_Access_Control/" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Store a URL in each Reference field.
The browser uses Reference as the anchor href. Values such as OWASP A01 Broken Access Control - https://... are relative URLs, so every mock reference link is broken. Keep only the URL in this field.
Proposed fix
- "Reference": "OWASP A01 Broken Access Control - https://owasp.org/Top10/A01_2021-Broken_Access_Control/"
+ "Reference": "https://owasp.org/Top10/A01_2021-Broken_Access_Control/"Apply this change to all eight mock attack vectors.
Also applies to: 64-67, 85-88, 106-109, 127-130, 148-151, 169-172, 190-193
🤖 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/mockAllEndPointJson.json` around lines 20 - 23,
Update the Reference field for all eight mock attack vectors in the JSON fixture
to contain only the URL, removing descriptive OWASP text and separators while
preserving each link’s destination.
Contributor
|
This PR has had no activity for 30 days, so it's been marked stale. If you're still working on it, push a commit or leave a comment and this label will clear. Otherwise it'll close automatically in 60 days — no hard feelings, and you (or anyone else) are welcome to reopen or resubmit against current main whenever you're ready. |
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.
Overview
This PR addresses issue #89 to improve vulnerability descriptions. Previously, descriptions were a single block of text that mixed the location of the vulnerable value with the explanation. Following the approach used by ZAP, this PR separates the
Source,Solution, andReferenceinto distinct fields.As noted in the issue, this was found while writing JWT Vulnerabilities, so this PR focuses on completely revamping the JWT vulnerability levels while providing the framework for other vulnerability classes to adopt later.
Key Changes
source(),solution(), andreference()to the@AttackVectorannotation. RefactoredAttackVectorResponseBeanto include the new fields with a Builder pattern.JWTVulnerability.javawith proper locations, actionable solutions, and reference links.escapeHtml()helper invulnerableApp.jsto sanitizecurlPayload,source, andsolutionbefore injecting them intoinnerHTML. TrusteddescriptionHTML is preserved.<a>tags withescapeHtml().rel="noopener noreferrer"totarget="_blank"links.EndPointsInformationProvider.getMetaInformation().Note to Reviewers
I didn't find any more links or articles for the references; any feedback or review is appreciated.
Also, while working on
JWTVulnerability.java, I noticed that Levels 11 and 12 are missing entirely (the code jumps from Level 10 to Level 13, with a commented-out block for 11). I left this untouched as it is out of scope for this issue, but it might be worth tracking in a separate issue if those levels are meant to exist.Closes #89
Summary by CodeRabbit
New Features
Bug Fixes
Tests