Implement Level 6 (UUID) and Level 7 (REST Update) for IDOR #512 - #628
Implement Level 6 (UUID) and Level 7 (REST Update) for IDOR #512#628anatolii-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) { |
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()); |
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.
…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
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
| INSERT INTO idor_users VALUES (2, '747e108e-10b1-463e-9328-c453070a1975', 'Bob', '6F1A9D3C7B2E4A10:8944bc8b1b5e87e1d2046a27c8e524c10cf6f6169c2a66cfb19b74513b679307', 60000, 'USER', 1); | ||
| INSERT INTO idor_users VALUES (3, '7b6b8212-66ff-4193-8915-e35399a37945', 'Charlie', 'C4E8B1D9A2F63750:ee5282141fdffb8adeb83550ac95436cdf2e5bee3be1353fcf2b284a03eba13e', 70000, 'ADMIN', 1); | ||
| -- Users (LVL 7-8) | ||
| INSERT INTO idor_users VALUES (4, '44444444-4444-4444-4444-444444444444', 'David', 'A1B2C3D4E5F60708:141efd4aa55f37300ad1c06d63854eca83a8d41953c5667b9d24eaff4a3edd1d', 10000, 'USER', 7); |
There was a problem hiding this comment.
can you please use different passwords across levels?
| IDOR_LEVEL_3_JWT_TAMPERING=Secure login via token, but authorization is based on a client-side role_level3 cookie. Changing this cookie to ADMIN bypasses access controls. | ||
| IDOR_LEVEL_4_BROKEN_RBAC=Secure login with Base64-encoded role cookie, but cookie values can be tampered with client-side. Modify the role_level4 cookie to 'ADMIN' to bypass authorization. | ||
| IDOR_LEVEL_5_SECURE_RBAC=Secure login with proper server-side authorization. The server validates that the user owns the requested resource or has admin privileges before returning data. Cross-user access is blocked. | ||
| IDOR_LEVEL_6_UUID=Secure login, but objects are referenced by UUID instead of sequential IDs. Enumeration is prevented, but authorization is still missing. |
There was a problem hiding this comment.
we should not show the UUID in the UI. I am just thinking to show authorisation check is missing, we can have one user with uuid as say 0000000000000 or something like you have set 4444444444 but only one user not all users.
| <div class="idor-field" style="flex: 2;"> | ||
| <label for="profileUuid">Profile UUID</label> | ||
| <select id="profileUuid"> | ||
| <option value="324bdf11-ef1a-48f0-b342-436779c24456">Alice (324bdf11-ef1a-48f0-b342-436779c24456)</option> |
There was a problem hiding this comment.
i think we should not show UUID
| <div class="idor-field"> | ||
| <label for="username">User</label> | ||
| <select id="username"> | ||
| <option value="Alice">Alice</option> |
There was a problem hiding this comment.
for level 6,7,8 please use users of those levels not other levels.
| </div> | ||
| <div class="idor-field"> | ||
| <label for="password">Password</label> | ||
| <input type="password" id="password" value="P@ssw0rd!2026" autocomplete="current-password" /> |
There was a problem hiding this comment.
have a new password for David and make David selected for these levels.
| <div class="idor-field"> | ||
| <label for="targetUser">Target User</label> | ||
| <select id="targetUser"> | ||
| <option value="1" data-uuid="324bdf11-ef1a-48f0-b342-436779c24456">Alice (base)</option> |
There was a problem hiding this comment.
same as above. no need to show the UUID
| return; | ||
| } | ||
| if ( | ||
| lookupMethod === "UUID" && |
There was a problem hiding this comment.
I would suggest not showing those users at all. Have a check in backend instead of client side, client side checks can be bypassed. Also check should be on type of user.
| } | ||
| if ( | ||
| lookupMethod === "UUID" && | ||
| (uuid === "324bdf11-ef1a-48f0-b342-436779c24456" || |
There was a problem hiding this comment.
please remove UUID based checks. this is very difficult to maintain.
| salary INT, | ||
| role VARCHAR(20) | ||
| role VARCHAR(20), | ||
| level INT |
There was a problem hiding this comment.
have a field called Type.
2 types are there ID based and UUID based users.
There was a problem hiding this comment.
Schema / data layer
The level column on idor_users is heading in the right direction but the intent isn't clear enough. Replace it with a type column — values ID or UUID. Levels 1–5 query only type='ID' users, levels 6+ query only type='UUID' users. This makes the separation explicit in the data model and removes the fragile level-number comparisons scattered through the SQL (WHERE level=7 etc.).
No cross-type access — enforce it in the backend
The backend must filter by type on every query. Level 6's fetchUserByUuid should append AND type='UUID' to its SQL, and levels 1–5's fetchUserById should append AND type='ID'. That way it's physically impossible for a UUID-level request to touch an ID-level user and vice versa, regardless of what the client sends.
Remove all client-side protection checks
The JS in IDOR.js has guards like "Cannot modify protected base users" with hardcoded UUID comparisons. These need to go entirely. Client-side checks teach the wrong lesson in a security training app — the whole point is that they're bypassable. If the backend enforces type separation, you don't need any of this.
Don't show UUIDs in the UI for level 6
The dropdown in LEVEL_6/IDOR.html currently lists Alice's full UUID next to her name. The UUID should not appear in the UI at all before the user fetches the profile. The hint for how to obtain it should come from the attack vector description — e.g. "you can find other users' UUIDs by inspecting API responses or the database." That's the realistic attack scenario and it's a better learning moment than handing it to the student in a dropdown.
Level 6/7/8 should only show UUID-type users
Give the memorable UUID (44444444-4444-4444-4444-444444444444) to a user who is not in the login dropdown — so the student can never obtain it legitimately from their own session. Eve or Frank would work. David logs in, sees his own real UUID in the response, but Eve (or Frank) has the guessable one sitting there waiting to be found. The student has to notice the pattern and try it manually — that's the "aha" moment the level is trying to create. The remaining user (the third one) gets a realistic random UUID so there are only two targets: one guessable, one not.
|
@anatolii-kandiuk let me know if you need any help |
|
Hi! I've updated the backend validation as requested (removed client-side checks and added type validation). |
Great question. I think when the first 5 levels were implemented it was manually generated but we have a code which can be used to generate the passwords under database seeders class (https://github.com/aashpis/VulnerableApp/blob/master/src/main/java/org/sasanlabs/configuration/DatabaseSeeder.java). I am not sure if that is perfect for this case too but you can try to use it or enhance it for this use case. @antriksh-9 how did we generated other level passwords? Please correct me if I am wrong. |
@preetkaran |
|
@anatolii-kandiuk you can manually generate it for one time or use the tool to generate it for all the levels if easy. Upto you. |
|
@preetkaran20 Hi! Unfortunately, due to certain circumstances, I am currently unable to work on this task. I apologize for any inconvenience this may cause. Please feel free to reassign this to someone else so as not to block the progress. As soon as I have some free time, I would love to return and continue contributing to the project. Thank you for understanding! |
|
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. |
4 participants
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