-
Notifications
You must be signed in to change notification settings - Fork 33
Fix the WASA Issue : IDOR Vulnerability #102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA systematic addition of JWT-based authorization to multiple Spring controller endpoints. UserId is extracted from JWT tokens in cookies and validated against provided userIDs in path variables or request payloads. Unauthorized requests return error responses. Also includes tm-api version upgrade to 3.6.1 and addition of slf4j logging dependencies. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant JwtUtil
participant CookieUtil
participant Service
Client->>Controller: HTTP Request (with Cookie containing JWT)
Controller->>CookieUtil: Extract JWT from Cookie
CookieUtil-->>Controller: JWT Token
Controller->>JwtUtil: getUserIdFromToken(token)
JwtUtil-->>Controller: userId from Token
alt userId matches path/payload userID
Controller->>Service: Call Business Logic
Service-->>Controller: Result
Controller-->>Client: 200 OK with Data
else userId mismatch
Controller-->>Client: 5000/5001 Error - "Unauthorized access!"
end
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touchesβ Failed checks (1 warning)
β Passed checks (2 passed)
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 review |
β Actions performedReview triggered.
|
src/main/java/com/iemr/tm/controller/common/main/WorklistController.java
Outdated
Show resolved
Hide resolved
pom.xml
Outdated
| <artifactId>lombok</artifactId> | ||
| <optional>true</optional> | ||
| </dependency> | ||
| <dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about adding these.
I think these were removed once for ELK integration if I remember
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I enabled this to get the console outputs. If it's meant for ELK integration, I'll remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (1)
src/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.java (1)
96-111: Add JWT validation to remaining unprotected endpoints.The PR fixes IDOR vulnerabilities in
getUserServicePointVanDetailsandgetUserVanSpDetailsby extracting the JWT token and comparing the requesteduserIDwith the token's user ID. However, two endpoints remain unprotected and vulnerable to IDOR:
getServicepointVillages(line 96) - acceptsservicePointIDwithout JWT validationgetUserSpokeDetails(line 149) - acceptspsmIdwithout JWT validationBoth endpoints bypass the Authorization requirement and directly process user-supplied parameters without verifying the requesting user has access to that resource. Add JWT validation consistent with the protected endpoints to prevent unauthorized access.
π§Ή Nitpick comments (1)
pom.xml (1)
9-9: Reconsider addingslf4j-simplealongside Spring Bootβs default logging and verify version alignment.With the commentedβout exclusion,
spring-boot-starter/spring-boot-starter-webwill again pull inspring-boot-starter-logging, which already providesslf4j-apiplus a logback binding. Adding:<dependency> <groupId>org.slf4j</groupId> <artifactId>slf4j-api</artifactId> <version>${slf4j.version}</version> </dependency> <dependency> <groupId>org.slf4j</groupId> <artifactId>slf4j-simple</artifactId> <version>${slf4j.version}</version> </dependency>means you may have multiple SLF4J bindings (logback + slf4jβsimple) on the classpath, which typically produces warnings and can lead to confusing logging behavior. Also, youβre explicitly pinning the SLF4J version rather than relying on Spring Bootβs dependency management.
Iβd suggest either:
- Relying solely on Spring Bootβs
spring-boot-starter-logging(removeslf4j-simple, and likely the explicitslf4j-api), or- Explicitly excluding logback and using only
slf4j-simpleif thatβs deliberate, but then ensuring all other logging expectations still hold.Please also confirm that
${slf4j.version}resolves to the version you expect under thespring-boot-starter-parentBOM and doesnβt conflict with Bootβs managed stack.Also applies to: 60-68, 131-140
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (6)
pom.xml(3 hunks)src/main/java/com/iemr/tm/controller/common/main/WorklistController.java(5 hunks)src/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.java(4 hunks)src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java(3 hunks)src/main/java/com/iemr/tm/controller/videoconsultationcontroller/VideoConsultationController.java(2 hunks)src/main/java/com/iemr/tm/utils/JwtUtil.java(1 hunks)
π§° Additional context used
π§ Learnings (1)
π Learning: 2025-10-01T20:25:37.804Z
Learnt from: abhijeetw035
Repo: PSMRI/TM-API PR: 95
File: package.json:15-23
Timestamp: 2025-10-01T20:25:37.804Z
Learning: In the TM-API repository (Java Spring Boot project), the lint-staged configuration uses echo statements as intentional placeholders rather than actual linting commands. This design decision prioritizes reliability and low setup friction for contributors, focusing on commit message linting (the primary goal) while avoiding complex toolchain dependencies for code linting. The infrastructure is ready for future enhancement if code linting becomes a requirement.
Applied to files:
pom.xml
π Additional comments (1)
src/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.java (1)
38-39: LGTM! JWT utilities correctly imported and injected.The necessary utilities for JWT-based authorization are properly imported and the
JwtUtildependency is correctly autowired.Also applies to: 44-44, 56-57
src/main/java/com/iemr/tm/controller/common/main/WorklistController.java
Show resolved
Hide resolved
src/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/tm/controller/login/IemrMmuLoginController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/iemr/tm/controller/teleconsultation/TeleConsultationController.java
Show resolved
Hide resolved
...ain/java/com/iemr/tm/controller/videoconsultationcontroller/VideoConsultationController.java
Show resolved
Hide resolved
| JSONObject obj = new JSONObject(comingRequest); | ||
| logger.info("getUserServicePointVanDetails request " + comingRequest); | ||
| if (!obj.has("userID") || !obj.get("userID").toString().equals(userId)) { | ||
| response.setError(5001, "Unauthorized access - userID does not match token"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to still accept userID from client side?
Instead of matching, get the userID directly from JWT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of change the FE flow, just accept the userID from client side and compare it with JWT UserId to authorize the user.
|
Mostly same comments as the other PR. |
|




π Description
JIRA ID:
AMM-1922
Extract the userId from the JWT token instead of getting it from payload, to prevent this IDOR Vulnerability.
β Type of Change
Summary by CodeRabbit
New Features
Dependencies