Implement Level 6 (UUID) and Level 7 (REST Update) for IDOR #512#628
Open
anatolii-kandiuk wants to merge 27 commits into
Open
Implement Level 6 (UUID) and Level 7 (REST Update) for IDOR #512#628anatolii-kandiuk wants to merge 27 commits into
anatolii-kandiuk wants to merge 27 commits into
Conversation
…erByUuid method Co-authored-by: Copilot <copilot@github.com>
…unctionality Co-authored-by: Copilot <copilot@github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a uuid field to User and DB schema/data, expands seed rows, and implements IDOR level6 (UUID fetch), level7 (PUT update by id), and level8 (PUT update by uuid) with SQL/JDBC, tests, payloads, and i18n updates. ChangesIDOR UUID-based levels and model update
Sequence DiagramsequenceDiagram
participant Client
participant IDORV as IDORVulnerability
participant Service as IDORService
participant DB as Database
Client->>IDORV: GET/PUT requests for level6/7/8 (cookie tokens)
IDORV->>Service: fetchUserByUuid / updateUserById / updateUserByUuid
Service->>DB: SQL_PROFILE_BY_UUID / SQL_UPDATE_PROFILE_BY_ID / SQL_UPDATE_PROFILE_BY_UUID
DB-->>Service: user row / update count
Service-->>IDORV: result
IDORV-->>Client: GenericVulnerabilityResponseBean
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/resources/scripts/IDOR/db/data.sql (1)
1-3: ⚡ Quick winUse explicit column lists in seed inserts.
Positional inserts are brittle when schema evolves. Explicit columns make the seed contract stable and self-documenting.
Proposed refactor
-INSERT INTO idor_users VALUES (1, '324bdf11-ef1a-48f0-b342-436779c24456', 'Alice', 'A1B2C3D4E5F60708:141efd4aa55f37300ad1c06d63854eca83a8d41953c5667b9d24eaff4a3edd1d', 50000, 'USER'); -INSERT INTO idor_users VALUES (2, '747e108e-10b1-463e-9328-c453070a1975', 'Bob', '6F1A9D3C7B2E4A10:8944bc8b1b5e87e1d2046a27c8e524c10cf6f6169c2a66cfb19b74513b679307', 60000, 'USER'); -INSERT INTO idor_users VALUES (3, '7b6b8212-66ff-4193-8915-e35399a37945', 'Charlie', 'C4E8B1D9A2F63750:ee5282141fdffb8adeb83550ac95436cdf2e5bee3be1353fcf2b284a03eba13e', 70000, 'ADMIN'); +INSERT INTO idor_users (id, uuid, username, password, salary, role) VALUES (1, '324bdf11-ef1a-48f0-b342-436779c24456', 'Alice', 'A1B2C3D4E5F60708:141efd4aa55f37300ad1c06d63854eca83a8d41953c5667b9d24eaff4a3edd1d', 50000, 'USER'); +INSERT INTO idor_users (id, uuid, username, password, salary, role) VALUES (2, '747e108e-10b1-463e-9328-c453070a1975', 'Bob', '6F1A9D3C7B2E4A10:8944bc8b1b5e87e1d2046a27c8e524c10cf6f6169c2a66cfb19b74513b679307', 60000, 'USER'); +INSERT INTO idor_users (id, uuid, username, password, salary, role) VALUES (3, '7b6b8212-66ff-4193-8915-e35399a37945', 'Charlie', 'C4E8B1D9A2F63750:ee5282141fdffb8adeb83550ac95436cdf2e5bee3be1353fcf2b284a03eba13e', 70000, 'ADMIN');🤖 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/scripts/IDOR/db/data.sql` around lines 1 - 3, The three INSERT statements into the idor_users table use positional inserts which are brittle; update each INSERT INTO idor_users VALUES (...) to specify the explicit column list (e.g., id, uuid, name, password_hash, salary, role) so the seed data remains stable if the schema changes and is self-documenting; modify the three INSERTs that currently insert Alice, Bob, and Charlie to include that column list and keep the same values in the same order.src/main/resources/scripts/IDOR/db/schema.sql (1)
5-5: ⚡ Quick winAdd a UNIQUE/NOT NULL constraint for idor_users.uuid (schema hardening).
Current Level 6 lookup already takes the first matching row, but the bundled seed data uses distinct, non-empty UUIDs and the app doesn’t write
uuid, so the “Level 6 breaks due to duplicates” risk isn’t evidenced here. Still, enforcingNOT NULL UNIQUEprevents invalid/duplicate data from ever being introduced.Proposed fix
- uuid VARCHAR(36), + uuid VARCHAR(36) NOT NULL UNIQUE,🤖 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/scripts/IDOR/db/schema.sql` at line 5, Update the idor_users.uuid column definition to enforce uniqueness and non-nullability by changing the column declaration for "uuid" in the idor_users table (the symbol: idor_users.uuid) from "uuid VARCHAR(36)" to a NOT NULL UNIQUE declaration (e.g., VARCHAR(36) NOT NULL UNIQUE or add an explicit UNIQUE CONSTRAINT on uuid) so the schema prevents duplicate or empty UUIDs from being inserted.
🤖 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.
Nitpick comments:
In `@src/main/resources/scripts/IDOR/db/data.sql`:
- Around line 1-3: The three INSERT statements into the idor_users table use
positional inserts which are brittle; update each INSERT INTO idor_users VALUES
(...) to specify the explicit column list (e.g., id, uuid, name, password_hash,
salary, role) so the seed data remains stable if the schema changes and is
self-documenting; modify the three INSERTs that currently insert Alice, Bob, and
Charlie to include that column list and keep the same values in the same order.
In `@src/main/resources/scripts/IDOR/db/schema.sql`:
- Line 5: Update the idor_users.uuid column definition to enforce uniqueness and
non-nullability by changing the column declaration for "uuid" in the idor_users
table (the symbol: idor_users.uuid) from "uuid VARCHAR(36)" to a NOT NULL UNIQUE
declaration (e.g., VARCHAR(36) NOT NULL UNIQUE or add an explicit UNIQUE
CONSTRAINT on uuid) so the schema prevents duplicate or empty UUIDs from being
inserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 82cd90ef-dfc4-4096-bc57-d3d366af5bef
📒 Files selected for processing (4)
src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.javasrc/main/java/org/sasanlabs/service/vulnerability/idor/User.javasrc/main/resources/scripts/IDOR/db/data.sqlsrc/main/resources/scripts/IDOR/db/schema.sql
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.java (3)
265-272:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRun Spotless on this block before merge.
CI is already red on the formatting here. Please run
./gradlew :spotlessApplysospotlessJavaCheckpasses.🤖 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/idor/IDORVulnerability.java` around lines 265 - 272, The annotation block in IDORVulnerability containing `@AttackVector` and `@VulnerableAppRequestMapping` (for LevelConstants.LEVEL_7 / IDOR_LEVEL_7_UPDATE) is misformatted; run the project formatter so Spotless passes (execute ./gradlew :spotlessApply) or apply the Java formatting rules to this annotation block and re-run spotless checks to ensure spotlessJavaCheck succeeds before merging.
273-282:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject incomplete update bodies before persisting them.
This writes
salaryandrolestraight from the deserialized body. If either field is absent, a PUT can overwrite the row with default/null values, and the fallback response currently saysPLEASE_LOGIN_FIRSTeven when the target user simply does not exist.Suggested fix
public ResponseEntity<GenericVulnerabilityResponseBean<Object>> level7( `@RequestBody` User userToUpdate) { - - int id = userToUpdate.getUserId(); - - if (fetchUserById(id) != null) { - updateUser(id, userToUpdate.getSalary(), userToUpdate.getRole()); - return response(USER_SUCCESSFULLY_UPDATED, true); - } - return response(PLEASE_LOGIN_FIRST, false); + + if (userToUpdate == null || userToUpdate.getRole() == null) { + return response("Provide userId, salary and role", false, HttpStatus.BAD_REQUEST); + } + + int id = userToUpdate.getUserId(); + if (fetchUserById(id) == null) { + return response(USER_NOT_FOUND, false, HttpStatus.NOT_FOUND); + } + + updateUser(id, userToUpdate.getSalary(), userToUpdate.getRole()); + return response(USER_SUCCESSFULLY_UPDATED, true, HttpStatus.OK); }
251-262:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn a UUID-specific validation error here.
LEVEL_6has no auth gate, soPLEASE_LOGIN_FIRSTon a missinguuidis misleading. A parameter-specific error keeps the demo behavior easier to understand.Suggested fix
public ResponseEntity<GenericVulnerabilityResponseBean<Object>> level6( `@RequestParam`(required = false) String uuid) { if (uuid != null) { User profile = fetchUserByUuid(uuid); @@ } return response(profile, true); } - return response(PLEASE_LOGIN_FIRST, false); + return response("Provide uuid", false, HttpStatus.BAD_REQUEST); }🤖 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/idor/IDORVulnerability.java` around lines 251 - 262, The controller method level6 currently returns PLEASE_LOGIN_FIRST when the uuid request param is missing which is misleading; change the null-uuid branch to return a UUID-specific validation error instead (e.g., create/use a UUID_REQUIRED or INVALID_UUID constant) by updating the level6 method to check uuid == null and call response(UUID_REQUIRED, false) (or an equivalent parameter-specific message), leaving the existing fetchUserByUuid and USER_NOT_FOUND handling intact.
🤖 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.
Outside diff comments:
In
`@src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.java`:
- Around line 265-272: The annotation block in IDORVulnerability containing
`@AttackVector` and `@VulnerableAppRequestMapping` (for LevelConstants.LEVEL_7 /
IDOR_LEVEL_7_UPDATE) is misformatted; run the project formatter so Spotless
passes (execute ./gradlew :spotlessApply) or apply the Java formatting rules to
this annotation block and re-run spotless checks to ensure spotlessJavaCheck
succeeds before merging.
- Around line 251-262: The controller method level6 currently returns
PLEASE_LOGIN_FIRST when the uuid request param is missing which is misleading;
change the null-uuid branch to return a UUID-specific validation error instead
(e.g., create/use a UUID_REQUIRED or INVALID_UUID constant) by updating the
level6 method to check uuid == null and call response(UUID_REQUIRED, false) (or
an equivalent parameter-specific message), leaving the existing fetchUserByUuid
and USER_NOT_FOUND handling intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2eb09f12-4b0a-4673-9a36-c54d84490bae
📒 Files selected for processing (1)
src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.java
…lii-kandiuk/VulnerableApp into feature/idor-vulnerability
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #628 +/- ##
============================================
- Coverage 54.78% 53.60% -1.18%
- Complexity 660 663 +3
============================================
Files 91 91
Lines 3565 3662 +97
Branches 395 407 +12
============================================
+ Hits 1953 1963 +10
- Misses 1435 1522 +87
Partials 177 177 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.java (2)
302-323: ⚡ Quick winExtract the
Userrow mapper and letupdateUserreturn its row count.
fetchUserById,fetchUserByUuid, andfetchAllUsersbuild the exact sameUserfrom the result set — three copies of the same 5-column mapping is easy to drift on the next schema change. A singleRowMapper<User>constant (or static helper) keeps them aligned. Likewise,updateUsercurrently swallows theintrow count returned byjdbcTemplate.update, which is exactly what would letlevel7distinguish "user not found" from "updated" without the extra SELECT (see thelevel7comment).📝 Suggested refactor
+ private static final org.springframework.jdbc.core.RowMapper<User> USER_ROW_MAPPER = + (rs, rowNum) -> + new User( + rs.getInt("id"), + rs.getString("uuid"), + rs.getString("username"), + rs.getInt("salary"), + rs.getString("role")); + private User fetchUserById(int id) { - List<User> users = - jdbcTemplate.query( - SQL_PROFILE_BY_ID, - new Object[] {id}, - (rs, rowNum) -> - new User( - rs.getInt("id"), - rs.getString("uuid"), - rs.getString("username"), - rs.getInt("salary"), - rs.getString("role"))); - - if (users.isEmpty()) { - return null; - } - return users.get(0); + List<User> users = jdbcTemplate.query(SQL_PROFILE_BY_ID, USER_ROW_MAPPER, id); + return users.isEmpty() ? null : users.get(0); } private User fetchUserByUuid(String uuid) { - List<User> users = - jdbcTemplate.query( - SQL_PROFILE_BY_UUID, - new Object[] {uuid}, - (rs, rowNum) -> - new User( - rs.getInt("id"), - rs.getString("uuid"), - rs.getString("username"), - rs.getInt("salary"), - rs.getString("role"))); - - if (users.isEmpty()) { - return null; - } - return users.get(0); + List<User> users = jdbcTemplate.query(SQL_PROFILE_BY_UUID, USER_ROW_MAPPER, uuid); + return users.isEmpty() ? null : users.get(0); } - private void updateUser(int id, int salary, String role) { - jdbcTemplate.update(SQL_UPDATE_PROFILE, new Object[] {salary, role, id}); - } + private int updateUser(int id, int salary, String role) { + return jdbcTemplate.update(SQL_UPDATE_PROFILE, salary, role, id); + }🤖 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/idor/IDORVulnerability.java` around lines 302 - 323, Extract a single RowMapper<User> (e.g., USER_ROW_MAPPER) and replace the duplicated inline mappings in fetchUserById, fetchUserByUuid, and fetchAllUsers with that constant to ensure a single source of truth for building User objects; also change updateUser to return the int row count from jdbcTemplate.update(SQL_UPDATE_PROFILE, new Object[] {salary, role, id}) instead of void so callers (e.g., level7 logic) can detect "not found" vs "updated", and update any call sites that expect void to handle the returned count.
249-281: ⚡ Quick winNo unit tests for
level6/level7.Codecov flags 0% patch coverage on this file (26 missing lines) and
IDORVulnerabilityTestdoesn't exercise either of the new endpoints. Given the new SQL constants (SQL_PROFILE_BY_UUID,SQL_UPDATE_PROFILE) and the JDBC helpers (fetchUserByUuid,updateUser), please add at least:
level6_ShouldReturnUserByUuid— happy path with a matching row.level6_ShouldReturnNotFoundForUnknownUuid— emptyJdbcTemplate#queryresult.level6_ShouldRejectMissingUuid—uuid == nullbranch.level7_ShouldUpdateExistingUser— verifies thejdbcTemplate.update(...)invocation with(salary, role, id).level7_ShouldReturnNotFoundForUnknownId—fetchUserByIdreturns empty.This also pins the SQL strings and argument order so a future refactor (see
updateUsercomment) doesn't silently regress.Want me to draft the test cases above using the existing fixtures pattern in
IDORVulnerabilityTest?🤖 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/idor/IDORVulnerability.java` around lines 249 - 281, Add unit tests in IDORVulnerabilityTest to cover level6 and level7: implement level6_ShouldReturnUserByUuid (mock fetchUserByUuid/JdbcTemplate.query to return a User row and assert level6 returns that profile), level6_ShouldReturnNotFoundForUnknownUuid (mock JdbcTemplate.query to return empty and assert NOT_FOUND response), and level6_ShouldRejectMissingUuid (call level6 with null and assert PLEASE_LOGIN_FIRST). For level7 add level7_ShouldUpdateExistingUser (mock fetchUserById to return a user, call level7 with a User containing salary/role/id and verify updateUser / jdbcTemplate.update is invoked with SQL_UPDATE_PROFILE and arguments in order (salary, role, id) and assert USER_SUCCESSFULLY_UPDATED) and level7_ShouldReturnNotFoundForUnknownId (mock fetchUserById to return null and assert NOT_FOUND). Use the existing fixtures/mocking pattern in IDORVulnerabilityTest, reference methods level6, level7, fetchUserByUuid, fetchUserById, updateUser and constants SQL_PROFILE_BY_UUID and SQL_UPDATE_PROFILE when verifying SQL and argument order.
🤖 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/idor/IDORVulnerability.java`:
- Around line 249-261: The method level6 currently returns PLEASE_LOGIN_FIRST
when the uuid query param is missing, which is misleading for an unauthenticated
UUID-only endpoint; update level6 to return a more appropriate message (either
reuse USER_NOT_FOUND or introduce a new constant like PROVIDE_UUID) when uuid ==
null, and replace the response(PLEASE_LOGIN_FIRST, false) call accordingly;
ensure the new constant is added to the same response/messages constants class
and used in level6 so responses accurately reflect the validation for
fetchUserByUuid.
- Around line 271-281: In level7 replace the redundant fetchUserById call and
primitive-id pitfalls by validating the incoming id (if id <= 0 return
USER_NOT_FOUND) then call updateUser and branch on the returned row-count: if
rowsUpdated == 0 return USER_NOT_FOUND else return USER_SUCCESSFULLY_UPDATED;
change updateUser to return int (rows affected) and accept boxed Integer salary
and String role (avoid unboxing NPE) and modify SQL_UPDATE_PROFILE to avoid
overwriting values with NULL (use SQL like: SET salary = COALESCE(?, salary),
role = COALESCE(?, role) WHERE id = ?) so omitted salary/role won’t write NULL;
remove fetchUserById usage from level7 and rely on updateUser’s row-count to
determine existence.
---
Nitpick comments:
In
`@src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.java`:
- Around line 302-323: Extract a single RowMapper<User> (e.g., USER_ROW_MAPPER)
and replace the duplicated inline mappings in fetchUserById, fetchUserByUuid,
and fetchAllUsers with that constant to ensure a single source of truth for
building User objects; also change updateUser to return the int row count from
jdbcTemplate.update(SQL_UPDATE_PROFILE, new Object[] {salary, role, id}) instead
of void so callers (e.g., level7 logic) can detect "not found" vs "updated", and
update any call sites that expect void to handle the returned count.
- Around line 249-281: Add unit tests in IDORVulnerabilityTest to cover level6
and level7: implement level6_ShouldReturnUserByUuid (mock
fetchUserByUuid/JdbcTemplate.query to return a User row and assert level6
returns that profile), level6_ShouldReturnNotFoundForUnknownUuid (mock
JdbcTemplate.query to return empty and assert NOT_FOUND response), and
level6_ShouldRejectMissingUuid (call level6 with null and assert
PLEASE_LOGIN_FIRST). For level7 add level7_ShouldUpdateExistingUser (mock
fetchUserById to return a user, call level7 with a User containing
salary/role/id and verify updateUser / jdbcTemplate.update is invoked with
SQL_UPDATE_PROFILE and arguments in order (salary, role, id) and assert
USER_SUCCESSFULLY_UPDATED) and level7_ShouldReturnNotFoundForUnknownId (mock
fetchUserById to return null and assert NOT_FOUND). Use the existing
fixtures/mocking pattern in IDORVulnerabilityTest, reference methods level6,
level7, fetchUserByUuid, fetchUserById, updateUser and constants
SQL_PROFILE_BY_UUID and SQL_UPDATE_PROFILE when verifying SQL and argument
order.
🪄 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: e863ff0c-1a91-4241-8770-6cc698cabd8b
📒 Files selected for processing (2)
src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.javasrc/test/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerabilityTest.java
| public ResponseEntity<GenericVulnerabilityResponseBean<Object>> level6( | ||
| @RequestParam(required = false) String uuid) { | ||
|
|
||
| if (uuid != null) { |
Member
There was a problem hiding this comment.
IDOR vulnerability links with User Login so add login setup as well and after login, user can guess and fetch other users.
|
|
||
| int rowsAffected = | ||
| updateUser( | ||
| userToUpdate.getUserId(), userToUpdate.getSalary(), userToUpdate.getRole()); |
Member
There was a problem hiding this comment.
Updating existing data is tricky as it will impact existing levels as well. best to not update existing users which are used in other levels. Can you please create 3 new users and update them from this level. Also if possible showcase update for both UUID and ID levels.
preetkaran20
requested changes
May 21, 2026
…date cookie token
…nhance payload descriptions
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/idor/IDORVulnerability.java`:
- Around line 258-260: Wrap calls to idorLoginService.decodeToken(cookieToken)
in a try-catch so token decode failures do not bubble up as 500s; catch
Exception (or the specific token decode exception), log if needed, set the
validation result to the controlled INVALID_TOKEN response (use the existing
INVALID_TOKEN constant/response handling used in this class) and return/exit the
handler early instead of proceeding (this applies to the decodeToken call near
the cookieToken/null check and the other two occurrences around the uuid checks
at the other noted locations).
- Around line 280-282: The level7 handler (public
ResponseEntity<GenericVulnerabilityResponseBean<Object>> level7(...,
`@RequestBody` User userToUpdate)) currently dereferences userToUpdate without
checking for null; add a defensive null-check at the start of level7 and return
a safe ResponseEntity/GenericVulnerabilityResponseBean indicating a bad request
(or no-op) when userToUpdate is null instead of continuing to access its fields.
Apply the same pattern to the other controller methods that accept `@RequestBody`
User (the other handlers referenced in the review) so each method validates the
request body before any dereference and returns an appropriate error
ResponseEntity when null.
🪄 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: b9295651-d0bd-4577-baec-a4ec31f0f46f
📒 Files selected for processing (4)
src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.javasrc/main/resources/attackvectors/IDORVulnerabilityPayload.propertiessrc/main/resources/i18n/messages.propertiessrc/main/resources/scripts/IDOR/db/data.sql
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/scripts/IDOR/db/data.sql
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.java (1)
296-300:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid writing
NULLon partial update payloads.Line 296 and Line 332 pass nullable
salary/rolestraight into the update SQL. A body like{"userId": 4}or{"uuid": "..."}will clear those columns or trip a DB constraint instead of behaving like a targeted update. Preserve existing values withCOALESCE(...)in the update queries, or reject requests where both mutable fields are absent.🛠️ Minimal fix
- private static final String SQL_UPDATE_PROFILE_BY_ID = - "UPDATE idor_users SET salary=?, role=? WHERE id=?"; - private static final String SQL_UPDATE_PROFILE_BY_UUID = - "UPDATE idor_users SET salary=?, role=? WHERE uuid=?"; + private static final String SQL_UPDATE_PROFILE_BY_ID = + "UPDATE idor_users SET salary=COALESCE(?, salary), role=COALESCE(?, role) WHERE id=?"; + private static final String SQL_UPDATE_PROFILE_BY_UUID = + "UPDATE idor_users SET salary=COALESCE(?, salary), role=COALESCE(?, role) WHERE uuid=?";Also applies to: 332-336
🤖 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/idor/IDORVulnerability.java` around lines 296 - 300, The update methods (e.g., updateUserById and updateUserByUuid) are currently passing nullable salary/role directly into the UPDATE SQL which will write NULLs when the payload omits those fields; change the SQL to preserve existing column values by wrapping parameters with COALESCE(column, ?) or equivalent (e.g., SET salary = COALESCE(?, salary), role = COALESCE(?, role)) so missing fields do not overwrite current values, and/or add input validation in the caller (where updateUserById/updateUserByUuid are invoked) to reject requests that provide neither mutable field before attempting the update.
🤖 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.
Duplicate comments:
In
`@src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.java`:
- Around line 296-300: The update methods (e.g., updateUserById and
updateUserByUuid) are currently passing nullable salary/role directly into the
UPDATE SQL which will write NULLs when the payload omits those fields; change
the SQL to preserve existing column values by wrapping parameters with
COALESCE(column, ?) or equivalent (e.g., SET salary = COALESCE(?, salary), role
= COALESCE(?, role)) so missing fields do not overwrite current values, and/or
add input validation in the caller (where updateUserById/updateUserByUuid are
invoked) to reject requests that provide neither mutable field before attempting
the update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c37e365-c6eb-4dec-bab7-29d30edc1fa4
📒 Files selected for processing (1)
src/main/java/org/sasanlabs/service/vulnerability/idor/IDORVulnerability.java
| return jdbcTemplate.update(SQL_UPDATE_PROFILE_BY_ID, new Object[] {salary, role, id}); | ||
| } | ||
|
|
||
| private int updateUserByUuid(String uuid, Integer salary, String role) { |
Member
There was a problem hiding this comment.
I think better way is introduce a field in database called Level and update based levels will have there own users which they can manipulate.
| } | ||
|
|
||
| private int updateUserByUuid(String uuid, Integer salary, String role) { | ||
| if ("747e108e-10b1-463e-9328-c453070a1975".equals(uuid) |
Member
There was a problem hiding this comment.
If checks in code are not good.
| payload = "IDOR_PAYLOAD_LEVEL_7") | ||
| @VulnerableAppRequestMapping( | ||
| value = LevelConstants.LEVEL_7, | ||
| htmlTemplate = "LEVEL_7/IDOR", |
Member
There was a problem hiding this comment.
I dont see these templates in the code. please add the html templates like it is done for other levels.
preetkaran20
requested changes
May 22, 2026
Member
preetkaran20
left a comment
preetkaran20
left a comment
There was a problem hiding this comment.
I would suggest testing these code changes as they are not working.
…vel restrictions and improve user data organization in the database
… logic for user login and profile updates
…e fetch request handling
… buttons to improve code clarity
…handling and response management
…Script logic for user login and profile fetching
…n, and enhance response handling for profile updates
…formatting for better readability
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
3 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.
Level 6 (UUID Based IDOR):
Added uuid column (VARCHAR(36)) to idor_users table in the database schema.
Created endpoint /LEVEL_6 that blindly trusts the provided UUID parameter, exposing the user profile without authorization checks.
Level 7 (REST API Updates / Privilege Escalation):
Granted UPDATE privileges to the DB user for the idor_users table.
Created PUT /LEVEL_7 endpoint that accepts a JSON Payload (@RequestBody).
The server processes the update (e.g., changing salary and role) based on the userId provided in the JSON body, leading to Mass Assignment / Privilege Escalation.
Summary by CodeRabbit
New Features
Chores