remediate: fix SAST scanner mappings and add validation test#608
remediate: fix SAST scanner mappings and add validation test#608Etoile-Bleu wants to merge 5 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR migrates the IDOR authentication service from Base64-encoded JSON tokens to cryptographically signed HS256 JWTs with per-service random secrets, adds comprehensive tampering and expiration validation tests, and introduces CSV integrity validation for the SAST expected issues file. ChangesIDOR Token JWT Migration
SAST Expected Issues CSV Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 |
Etoile-Bleu
force-pushed
the
fix-sast-mappings
branch
from
d05af58 to
29b04d7
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #608 +/- ##
============================================
+ Coverage 53.12% 57.52% +4.40%
- Complexity 510 638 +128
============================================
Files 72 84 +12
Lines 2850 3275 +425
Branches 296 366 +70
============================================
+ Hits 1514 1884 +370
- Misses 1201 1223 +22
- Partials 135 168 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/java/org/sasanlabs/service/vulnerability/idor/IDORLoginServiceTest.java (1)
100-112: ⚡ Quick winConsider renaming tests to reflect JWT validation.
The test names
WhenPayloadIsNotJsonandWhenBase64IsInvalidsuggest the old Base64-encoded JSON token format. With the JWT implementation, both tests now verify that malformed JWT strings triggerIllegalArgumentExceptionduring parsing. While the tests still provide value, renaming them would improve clarity.💡 Suggested test names
`@Test` -void decodeToken_ShouldThrowIllegalArgumentException_WhenPayloadIsNotJson() { +void decodeToken_ShouldThrowIllegalArgumentException_WhenTokenIsNotValidJWT() { String nonJsonPayload = java.util.Base64.getEncoder().encodeToString("not-json".getBytes()); assertThrows( IllegalArgumentException.class, () -> idorLoginService.decodeToken(nonJsonPayload)); } `@Test` -void decodeToken_ShouldThrowIllegalArgumentException_WhenBase64IsInvalid() { +void decodeToken_ShouldThrowIllegalArgumentException_WhenTokenIsMalformed() { assertThrows( IllegalArgumentException.class, () -> idorLoginService.decodeToken("@@not-base64@@")); }🤖 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/test/java/org/sasanlabs/service/vulnerability/idor/IDORLoginServiceTest.java` around lines 100 - 112, The test method names decodeToken_ShouldThrowIllegalArgumentException_WhenPayloadIsNotJson and decodeToken_ShouldThrowIllegalArgumentException_WhenBase64IsInvalid no longer match the JWT parsing behavior; rename them to reflect JWT validation (e.g., decodeToken_ShouldThrowIllegalArgumentException_ForMalformedJwt and decodeToken_ShouldThrowIllegalArgumentException_ForInvalidJwtSegment) and update any test annotations or references so idorLoginService.decodeToken is clearly tested for malformed/invalid JWT input rather than legacy Base64 JSON payloads.
🤖 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/test/java/org/sasanlabs/service/vulnerability/sast/ExpectedIssuesCSVTest.java`:
- Around line 34-35: The test uses new File(filePath) which treats relative file
paths against the process working directory; update ExpectedIssuesCSVTest to
resolve any relative CSV-referenced paths against the CSV file's directory
instead (use csvPath.getParent() or csvPath.toAbsolutePath().getParent() as the
stable base) and construct the target path via Paths.get(baseDir.toString(),
filePath) or baseDir.resolve(filePath).toFile(); apply the same change to both
places mentioned (lines 58–59) so all file lookups use the CSV's directory as
the base.
- Around line 16-21: Reformat the Javadoc comment on the ExpectedIssuesCSVTest
class (lines ~16-21) to satisfy Spotless wrapping rules (wrap lines to project
max line length and use standard Javadoc punctuation/spacing) and fix the
assertion formatting at lines ~62-63 in ExpectedIssuesCSVTest so it follows
project's assertion style (correct argument order and not splitting the
assertion across lines in a way that violates Spotless). Update the Javadoc and
the specific assertions in the ExpectedIssuesCSVTest class to be properly
wrapped and formatted so spotlessJavaCheck passes.
---
Nitpick comments:
In
`@src/test/java/org/sasanlabs/service/vulnerability/idor/IDORLoginServiceTest.java`:
- Around line 100-112: The test method names
decodeToken_ShouldThrowIllegalArgumentException_WhenPayloadIsNotJson and
decodeToken_ShouldThrowIllegalArgumentException_WhenBase64IsInvalid no longer
match the JWT parsing behavior; rename them to reflect JWT validation (e.g.,
decodeToken_ShouldThrowIllegalArgumentException_ForMalformedJwt and
decodeToken_ShouldThrowIllegalArgumentException_ForInvalidJwtSegment) and update
any test annotations or references so idorLoginService.decodeToken is clearly
tested for malformed/invalid JWT input rather than legacy Base64 JSON payloads.
🪄 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: 3bce97a5-c6c8-4afa-9b43-7f17fe2ce18c
⛔ Files ignored due to path filters (1)
scanner/sast/expectedIssues.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
src/main/java/org/sasanlabs/service/vulnerability/idor/IDORLoginService.javasrc/test/java/org/sasanlabs/service/vulnerability/idor/IDORLoginServiceTest.javasrc/test/java/org/sasanlabs/service/vulnerability/sast/ExpectedIssuesCSVTest.java
| assertTrue(Files.exists(csvPath), "expectedIssues.csv not found at " + csvPath.toAbsolutePath()); | ||
|
|
There was a problem hiding this comment.
Resolve CSV-referenced file paths from a stable base directory.
Line 58 currently uses new File(filePath), which is interpreted relative to the current working directory. That can make this test fail in environments where user.dir differs from repo root. Resolve relative paths against the resolved CSV location instead.
Proposed path-resolution fix
assertTrue(Files.exists(csvPath), "expectedIssues.csv not found at " + csvPath.toAbsolutePath());
+ Path repoRoot = csvPath.toAbsolutePath().normalize().getParent().getParent().getParent();
try (BufferedReader br = new BufferedReader(new FileReader(csvPath.toFile()))) {
@@
- File file = new File(filePath);
- assertTrue(file.exists(), "Source file not found: " + filePath + " (referenced at CSV line " + csvLineNumber + ")");
+ Path sourcePath = Paths.get(filePath);
+ if (!sourcePath.isAbsolute()) {
+ sourcePath = repoRoot.resolve(sourcePath).normalize();
+ }
+ File file = sourcePath.toFile();
+ assertTrue(
+ file.exists(),
+ "Source file not found: "
+ + sourcePath
+ + " (referenced at CSV line "
+ + csvLineNumber
+ + ")");Also applies to: 58-59
🤖 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/test/java/org/sasanlabs/service/vulnerability/sast/ExpectedIssuesCSVTest.java`
around lines 34 - 35, The test uses new File(filePath) which treats relative
file paths against the process working directory; update ExpectedIssuesCSVTest
to resolve any relative CSV-referenced paths against the CSV file's directory
instead (use csvPath.getParent() or csvPath.toAbsolutePath().getParent() as the
stable base) and construct the target path via Paths.get(baseDir.toString(),
filePath) or baseDir.resolve(filePath).toFile(); apply the same change to both
places mentioned (lines 58–59) so all file lookups use the CSV's directory as
the base.
There was a problem hiding this comment.
Yes good gatch :')
I'm moving this PR to Draft status for now. I've noted the feedback regarding the path resolution in ExpectedIssuesCSVTest.java.
Tomorrow, I will update the logic to ensure the test is environment-agnostic.
I think i will
- Use the absolute path of the CSV file as a stable anchor to resolve the project root.
- Construct all source file paths using
repoRoot.resolve(filePath).normalize().
There was a problem hiding this comment.
@Etoile-Bleu Sounds like a solid plan! Your two-step approach is exactly right. One small note: since the CSV lives at scanner/sast/expectedIssues.csv (three path segments deep), you'll need three .getParent() calls to reach the repo root from the resolved CSV path:
Path csvPath = Paths.get(CSV_PATH);
if (!Files.exists(csvPath)) {
csvPath = Paths.get(System.getProperty("user.dir"), CSV_PATH);
}
assertTrue(Files.exists(csvPath), "expectedIssues.csv not found at " + csvPath.toAbsolutePath());
// Derive repo root: scanner/sast/expectedIssues.csv → 3 levels up
Path repoRoot = csvPath.toAbsolutePath().normalize()
.getParent() // scanner/sast
.getParent() // scanner
.getParent(); // repo root
try (BufferedReader br = new BufferedReader(new FileReader(csvPath.toFile()))) {
// ...
Path sourcePath = Paths.get(filePath);
if (!sourcePath.isAbsolute()) {
sourcePath = repoRoot.resolve(sourcePath).normalize();
}
File file = sourcePath.toFile();
assertTrue(
file.exists(),
"Source file not found: " + sourcePath + " (referenced at CSV line " + csvLineNumber + ")");
// ...
}This way, no matter what user.dir is at test runtime (IDE, CI, Maven wrapper, etc.), paths are always resolved relative to the project root inferred from the CSV's own location. Ready to re-review whenever you push the update! 🚀
|
Hi Karan and the SasanLabs team, Following the discussion about the maintenance of expectedIssues.csv, I have performed a rigorous cleanup of the file to ensure it remains a high-quality ground truth for SAST scanners. You will notice that the file is now more concise. I think it's betterto improve the accuracy of the baseline. Here is a summary of the technical rationale behind this reduction:
My solution:To solve the maintenance struggle you mentioned, I have integrated a JUnit test (ExpectedIssuesCSVTest.java).
Question for the maintainersMy audit was based on the current CSV in the workspace. However, I noticed that files like BlindSQLInjectionVulnerability.java and UnionBasedSQLInjectionVulnerability.java were missing from the original list. How would you like me to proceed?
|
If you can add missing vulnerabilities that is always great and we want to have that. The CSV is outdated due to addition or updation of code. it would be great to update the CSV with newer line numbers. |
| if (csvLineNumber == 1) continue; | ||
| if (line.trim().isEmpty()) continue; | ||
|
|
||
| String[] parts = line.split(","); |
There was a problem hiding this comment.
I would suggest using library for this.
3 participants
Description
This PR cleans up and accurately re-maps the
scanner/sast/expectedIssues.csvfile by aligning vulnerability entries with actual actionable code sinks (SQL queries, reflection points, etc.) instead of boilerplate or incorrect line references.Changes
XSSInImgTagAttribute.java).ExpectedIssuesCSVTest.java, a cross-platform JUnit test that validates the integrity of the CSV (file existence and line bounds) during the test lifecycle.Future Recommendation: Dynamic Truth Generation
To eliminate manual maintenance drift, the repository could transition to a dynamic generation model:
// SAST_SINK) directly on vulnerable lines.@AttackVectorannotations and link them to the nearest marker.expectedIssues.csvas a build artifact, ensuring the SAST ground truth is always 100% synchronized with the source code.This approach would provide the "ultimate" source of truth while removing the need for manual CSV updates.
Summary by CodeRabbit
Bug Fixes
Tests