Add GraphQL Authorization & Access Control vulnerability category (#530)#598
Conversation
…sanLabs#530) Introduces a new vulnerability category with five levels exercising the authorization failure family that is distinct from injection: LEVEL_1 IDOR in a query user(id:) returns any user LEVEL_2 BOLA order(id:) ignores ownership LEVEL_3 Privilege escalation mutation updateUserRole lets any USER self-promote LEVEL_4 Admin mutation exposure adminReport / deleteUser have no role check LEVEL_5 Secure reference owner + DB-verified role on every request Implementation notes: - Uses graphql-java 21.5 directly (not spring-boot-starter-graphql, which would auto-configure /graphql and conflict with the per-level pattern) - Each level builds its own GraphQL instance from its own SDL so resolver fixes do not leak across levels - Authentication reuses IDORLoginService (Base64-encoded JSON token); the decoded User is attached to GraphQLContext - Level 5 re-reads role from the DB on every admin op so a tampered token claiming role=ADMIN is rejected - Two new H2 tables (graphql_users, graphql_orders) seeded via DataSourceInitializer, kept separate from idor_users so mutations here don't corrupt IDOR state - Shared HTML/CSS/JS template under static/templates/GraphQLVulnerability/LEVEL_1 drives all five levels (JS branches on current level) Also adds: - 12-case JUnit 5 + Mockito test class covering all five levels and the Level 5 defenses (non-owner, non-admin, token tampering, admin allow) - README entry and docs/GraphQLVulnerability.md with per-level curl exploits and a QA walk-through Closes SasanLabs#530
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive GraphQL Authorization & Access Control vulnerability module, demonstrating five progressive security levels from unauthenticated access through fully secured implementations. It includes database schemas, multiple GraphQL endpoint levels, a login controller, UI templates, i18n strings, and a test suite. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant LoginCtrl as GraphQLLoginController
participant IDORSvc as IDORLoginService
participant GQLCtrl as GraphQLVulnerability
participant GQLEngine as GraphQL Engine
participant DB as Database
rect rgba(200, 150, 100, 0.5)
Note over Client,DB: Authentication Flow
Client->>LoginCtrl: POST /login (username, password)
LoginCtrl->>IDORSvc: loginWithToken(username, password)
IDORSvc->>DB: Query user credentials
DB-->>IDORSvc: User + token
IDORSvc-->>LoginCtrl: IDORLoginDetails
LoginCtrl-->>Client: 200 OK {token, userId, role}
end
rect rgba(100, 150, 200, 0.5)
Note over Client,DB: GraphQL Query Execution (Level 5: Secure)
Client->>GQLCtrl: POST /level5 (query, token)
GQLCtrl->>GQLCtrl: Decode token → User context
GQLCtrl->>GQLEngine: Execute GraphQL schema
alt Authenticated + Authorized
GQLEngine->>DB: Query with ownership check
DB-->>GQLEngine: Resource (verified owned)
GQLEngine-->>GQLCtrl: ExecutionResult {data}
else Unauthenticated OR Unauthorized
GQLEngine->>DB: Verify role from DB
DB-->>GQLEngine: Role mismatch
GQLEngine-->>GQLCtrl: ExecutionResult {errors}
end
GQLCtrl-->>Client: 200 OK {data/errors}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
⚔️ Resolve merge conflicts
Review rate limit: 2/3 reviews remaining, refill in 20 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.html (1)
24-24: ⚡ Quick winAdd ARIA live announcements for async login/response updates.
Screen readers won’t reliably announce content changes in these dynamic containers without live-region semantics.
Proposed change
- <div id="loginMessage" class="status-message hidden"></div> + <div id="loginMessage" class="status-message hidden" role="status" aria-live="polite"></div> ... - <pre id="gqlResponse" class="gql-response"><response will appear here></pre> + <pre id="gqlResponse" class="gql-response" aria-live="polite"><response will appear here></pre>Also applies to: 48-48
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.html` at line 24, The dynamic status container with id "loginMessage" (and the other similar dynamic status container referenced at lines 48-48) needs ARIA live-region attributes so screen readers announce its async updates; add aria-live="polite" (or "assertive" if immediate interruption is required), role="status" and aria-atomic="true" to the <div id="loginMessage" class="status-message hidden"> and to the other dynamic container so content changes are announced consistently, while preserving the existing "hidden" class behavior for visual hiding.src/main/resources/scripts/GraphQL/db/schema.sql (1)
12-17: ⚡ Quick winAdd a foreign key for
graphql_orders.user_idto preserve ownership integrity.Without an FK, orphaned orders can exist and undermine object-level ownership semantics in later levels.
Proposed change
CREATE TABLE graphql_orders ( id INT PRIMARY KEY, - user_id INT, + user_id INT NOT NULL, item VARCHAR(100), - amount INT + amount INT, + CONSTRAINT fk_graphql_orders_user + FOREIGN KEY (user_id) REFERENCES graphql_users(id) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/scripts/GraphQL/db/schema.sql` around lines 12 - 17, Add a foreign key constraint on graphql_orders.user_id to enforce ownership by referencing the users table: update the CREATE TABLE graphql_orders statement (or run an ALTER TABLE) to include "FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE" (or use ON DELETE RESTRICT if you prefer preventing user deletion while orders exist); ensure the referenced primary key symbol is users.id and adjust the referenced table name if your users table is named differently.docs/GraphQLVulnerability.md (1)
15-25: 💤 Low valueAdd a language identifier to the fenced code block.
The directory structure code block should specify a language (e.g.,
textorplaintext) to satisfy linting rules and improve rendering consistency.-``` +```text src/main/java/org/sasanlabs/service/vulnerability/graphql/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/GraphQLVulnerability.md` around lines 15 - 25, Update the fenced directory-structure code block in docs/GraphQLVulnerability.md to include a language identifier (e.g., change the opening ``` to ```text or ```plaintext) so the block renders and lints correctly; locate the block that lists src/main/java/org/sasanlabs/service/vulnerability/graphql/ and src/main/resources/scripts/GraphQL/ (which documents GraphQLVulnerability.java, GraphQLLoginController.java, GraphQLUser.java, GraphQLOrder.java and the schema/db files) and add the language token immediately after the backticks.src/test/java/org/sasanlabs/service/vulnerability/graphql/GraphQLVulnerabilityTest.java (1)
239-252: 💤 Low valueConsider making stub methods verify the expected ID parameter.
The stub helper methods use
any(Object[].class)which will match any ID. This means tests won't catch bugs where the wrong ID is passed to the database query. For more precise verification, you could use argument captors or specific matchers.However, since the tests already verify the returned data contains expected values, this provides indirect verification that the correct records are being fetched.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/sasanlabs/service/vulnerability/graphql/GraphQLVulnerabilityTest.java` around lines 239 - 252, The three stub helpers (stubUserLookup, stubOrderLookup, stubRoleLookup) use any(Object[].class) so they match any ID; change the matcher on the jdbcTemplate.query call to assert the expected id is passed (e.g., replace any(Object[].class) with an ArgumentMatcher that checks the Object[] contains the provided id or use argThat(arr -> arr != null && arr.length > 0 && arr[0].equals(id))) while keeping the same return value; reference the methods stubUserLookup/stubOrderLookup/stubRoleLookup and the SQL_* constants and jdbcTemplate.query when making this change.src/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.js (1)
89-111: 💤 Low valueConsider adding null checks for DOM element references.
The event listener attachments on lines 89 and 113 assume
loginBtnandexecuteBtnexist. If the HTML template changes or loads incorrectly, this will throw aTypeError. While this is unlikely given the tight coupling to the template, defensive checks would be more robust.🛡️ Suggested defensive approach
-document.getElementById("loginBtn").addEventListener("click", function () { +const loginBtn = document.getElementById("loginBtn"); +if (loginBtn) { + loginBtn.addEventListener("click", function () { // ... handler code ... -}); + }); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.js` around lines 89 - 111, The event listener attachments assume elements exist and can throw if null; update GraphQL.js to first query and store the DOM nodes (e.g., const loginBtn = document.getElementById("loginBtn"), const usernameEl = document.getElementById("username"), const passwordEl = document.getElementById("password")) and guard before calling addEventListener (if (loginBtn) { ... }) and likewise for executeBtn; inside the handler also ensure usernameEl and passwordEl are non-null before accessing .value and handle the missing-node case gracefully (no-op or show an error) so functions like the login handler and doPostAjaxCall are not invoked with undefined elements.src/main/java/org/sasanlabs/service/vulnerability/graphql/GraphQLVulnerability.java (1)
366-382: Consider updating to non-deprecatedJdbcTemplate.query()signature.The
JdbcTemplate.query(String, Object[], RowMapper)method is deprecated since Spring 5.3. Replace with the varargs signature:query(String, RowMapper, Object...). The same pattern appears infetchOrder(),fetchOrdersByUser(), andfetchRoleFromDb().♻️ Suggested update using varargs
private GraphQLUser fetchUser(Integer id) { if (id == null) { return null; } List<GraphQLUser> users = jdbcTemplate.query( SQL_USER_BY_ID, - new Object[] {id}, (rs, rowNum) -> new GraphQLUser( rs.getInt("id"), rs.getString("username"), rs.getString("email"), rs.getInt("salary"), - rs.getString("role"))); + rs.getString("role")), + id); return users.isEmpty() ? null : users.get(0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sasanlabs/service/vulnerability/graphql/GraphQLVulnerability.java` around lines 366 - 382, The JDBC calls use the deprecated JdbcTemplate.query(String, Object[], RowMapper) signature; update each to the varargs form query(String, RowMapper, Object...) to remove the deprecation: in fetchUser(Integer id) (and similarly in fetchOrder(...), fetchOrdersByUser(...), fetchRoleFromDb(...)) move the RowMapper (the lambda that maps ResultSet to GraphQLUser/Order/Role) to the second parameter and pass the query parameters (e.g., id) as following varargs instead of an Object[]; ensure the argument order matches query(sql, rowMapper, params...) and adjust any lambda/context if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.css`:
- Line 161: Replace the deprecated CSS declaration "word-break: break-word" with
the modern equivalent by updating the rule that currently sets word-break:
break-word to use overflow-wrap: anywhere (e.g., within the same selector where
"word-break: break-word" appears) so text wrapping behaviour remains the same
and stylelint no longer flags the property.
---
Nitpick comments:
In `@docs/GraphQLVulnerability.md`:
- Around line 15-25: Update the fenced directory-structure code block in
docs/GraphQLVulnerability.md to include a language identifier (e.g., change the
opening ``` to ```text or ```plaintext) so the block renders and lints
correctly; locate the block that lists
src/main/java/org/sasanlabs/service/vulnerability/graphql/ and
src/main/resources/scripts/GraphQL/ (which documents GraphQLVulnerability.java,
GraphQLLoginController.java, GraphQLUser.java, GraphQLOrder.java and the
schema/db files) and add the language token immediately after the backticks.
In
`@src/main/java/org/sasanlabs/service/vulnerability/graphql/GraphQLVulnerability.java`:
- Around line 366-382: The JDBC calls use the deprecated
JdbcTemplate.query(String, Object[], RowMapper) signature; update each to the
varargs form query(String, RowMapper, Object...) to remove the deprecation: in
fetchUser(Integer id) (and similarly in fetchOrder(...), fetchOrdersByUser(...),
fetchRoleFromDb(...)) move the RowMapper (the lambda that maps ResultSet to
GraphQLUser/Order/Role) to the second parameter and pass the query parameters
(e.g., id) as following varargs instead of an Object[]; ensure the argument
order matches query(sql, rowMapper, params...) and adjust any lambda/context if
needed.
In `@src/main/resources/scripts/GraphQL/db/schema.sql`:
- Around line 12-17: Add a foreign key constraint on graphql_orders.user_id to
enforce ownership by referencing the users table: update the CREATE TABLE
graphql_orders statement (or run an ALTER TABLE) to include "FOREIGN KEY
(user_id) REFERENCES users(id) ON DELETE CASCADE" (or use ON DELETE RESTRICT if
you prefer preventing user deletion while orders exist); ensure the referenced
primary key symbol is users.id and adjust the referenced table name if your
users table is named differently.
In
`@src/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.html`:
- Line 24: The dynamic status container with id "loginMessage" (and the other
similar dynamic status container referenced at lines 48-48) needs ARIA
live-region attributes so screen readers announce its async updates; add
aria-live="polite" (or "assertive" if immediate interruption is required),
role="status" and aria-atomic="true" to the <div id="loginMessage"
class="status-message hidden"> and to the other dynamic container so content
changes are announced consistently, while preserving the existing "hidden" class
behavior for visual hiding.
In `@src/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.js`:
- Around line 89-111: The event listener attachments assume elements exist and
can throw if null; update GraphQL.js to first query and store the DOM nodes
(e.g., const loginBtn = document.getElementById("loginBtn"), const usernameEl =
document.getElementById("username"), const passwordEl =
document.getElementById("password")) and guard before calling addEventListener
(if (loginBtn) { ... }) and likewise for executeBtn; inside the handler also
ensure usernameEl and passwordEl are non-null before accessing .value and handle
the missing-node case gracefully (no-op or show an error) so functions like the
login handler and doPostAjaxCall are not invoked with undefined elements.
In
`@src/test/java/org/sasanlabs/service/vulnerability/graphql/GraphQLVulnerabilityTest.java`:
- Around line 239-252: The three stub helpers (stubUserLookup, stubOrderLookup,
stubRoleLookup) use any(Object[].class) so they match any ID; change the matcher
on the jdbcTemplate.query call to assert the expected id is passed (e.g.,
replace any(Object[].class) with an ArgumentMatcher that checks the Object[]
contains the provided id or use argThat(arr -> arr != null && arr.length > 0 &&
arr[0].equals(id))) while keeping the same return value; reference the methods
stubUserLookup/stubOrderLookup/stubRoleLookup and the SQL_* constants and
jdbcTemplate.query when making this change.
🪄 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: 76b2459c-34c4-4662-8df7-62ddb95e9be8
📒 Files selected for processing (22)
README.mdbuild.gradledocs/GraphQLVulnerability.mdsrc/main/java/org/sasanlabs/configuration/VulnerableAppConfiguration.javasrc/main/java/org/sasanlabs/service/vulnerability/graphql/GraphQLLoginController.javasrc/main/java/org/sasanlabs/service/vulnerability/graphql/GraphQLOrder.javasrc/main/java/org/sasanlabs/service/vulnerability/graphql/GraphQLUser.javasrc/main/java/org/sasanlabs/service/vulnerability/graphql/GraphQLVulnerability.javasrc/main/java/org/sasanlabs/vulnerability/types/VulnerabilityType.javasrc/main/resources/attackvectors/GraphQLVulnerabilityPayload.propertiessrc/main/resources/i18n/messages.propertiessrc/main/resources/i18n/messages_en_US.propertiessrc/main/resources/scripts/GraphQL/db/data.sqlsrc/main/resources/scripts/GraphQL/db/schema.sqlsrc/main/resources/scripts/GraphQL/schema/level1.graphqlssrc/main/resources/scripts/GraphQL/schema/level2.graphqlssrc/main/resources/scripts/GraphQL/schema/level3.graphqlssrc/main/resources/scripts/GraphQL/schema/level4.graphqlssrc/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.csssrc/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.htmlsrc/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.jssrc/test/java/org/sasanlabs/service/vulnerability/graphql/GraphQLVulnerabilityTest.java
| text-align: left; | ||
| overflow: auto; | ||
| white-space: pre-wrap; | ||
| word-break: break-word; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify deprecated usage is removed from CSS files
rg -n --type=css 'word-break:\s*break-word'Repository: SasanLabs/VulnerableApp
Length of output: 562
🌐 Web query:
word-break break-word deprecated CSS spec
💡 Result:
Yes, word-break: break-word is deprecated in the CSS specification. It is retained only for backward compatibility with legacy content and has the same effect as word-break: normal combined with overflow-wrap: anywhere (ignoring the actual overflow-wrap value). The preferred modern replacement is overflow-wrap: break-word (or overflow-wrap: anywhere for cases involving min-content sizing), paired with word-break: normal if needed. Browsers still support it, but validators flag it, and future removal is possible.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/CSS/word-break
- 2: https://rocketvalidator.com/html-validation/css-the-value-break-word-is-deprecated
- 3: https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/word-break
- 4: https://w3.org/TR/css-text-3
- 5: https://drafts.csswg.org/css-text-4/
🌐 Web query:
overflow-wrap anywhere vs word-break break-word replacement
💡 Result:
overflow-wrap: anywhere is the modern replacement for the deprecated word-break: break-word. Both allow breaking unbreakable strings (like long words or URLs) at arbitrary points only when necessary to prevent overflow, unlike word-break: break-all which breaks more aggressively. Key points: - word-break: break-word (deprecated): Equivalent to word-break: normal + overflow-wrap: anywhere. Good browser support. - overflow-wrap: anywhere: Same breaking behavior, but considers soft wrap opportunities (from word-break) in min-content intrinsic size calculations. This prevents flex items or floats from expanding disproportionately due to long words. - Use overflow-wrap: anywhere for new code; it provides better layout behavior in flex/grid contexts. - For compatibility (e.g., older Safari), use: overflow-wrap: break-word; overflow-wrap: anywhere; (break-word falls back gracefully). - overflow-wrap: break-word is similar to anywhere but ignores those soft wraps in min-content sizing, potentially causing layout expansion. Examples from MDN: - Long word with overflow-wrap: anywhere breaks to fit without expanding container. - word-break: break-all breaks even if not strictly needed for overflow. As of 2026, overflow-wrap: anywhere has excellent support (Chrome 80+, Firefox 65+, Safari 15.4+), covering nearly all users.
Citations:
- 1: https://developer.mozilla.org/en/CSS/word-wrap
- 2: https://developer.mozilla.org/en-US/docs/Web/CSS/word-break
- 3: https://www.w3.org/TR/2021/CRD-css-text-3-20210422/
- 4: https://stackoverflow.com/questions/68593758/difference-between-overflow-wrap-anywhere-overflow-wrap-break-word
- 5: https://stackoverflow.com/questions/17259916/difference-between-overflow-wrap-and-word-break
- 6: https://caniuse.com/mdn-css_properties_overflow-wrap_anywhere
Replace deprecated word-break: break-word usage.
Line 161 uses a deprecated CSS value that can fail stylelint. Replace with overflow-wrap: anywhere for equivalent wrapping behavior and forward compatibility.
💡 Suggested patch
.gql-response {
- word-break: break-word;
+ overflow-wrap: anywhere;
+ word-break: normal;
}📝 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.
| word-break: break-word; | |
| .gql-response { | |
| overflow-wrap: anywhere; | |
| word-break: normal; | |
| } |
🧰 Tools
🪛 Stylelint (17.9.0)
[error] 161-161: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.css`
at line 161, Replace the deprecated CSS declaration "word-break: break-word"
with the modern equivalent by updating the rule that currently sets word-break:
break-word to use overflow-wrap: anywhere (e.g., within the same selector where
"word-break: break-word" appears) so text wrapping behaviour remains the same
and stylelint no longer flags the property.
|
Sorry for the delay. I will be reviewing this PR in next couple of days. thanks, |
| @PostMapping("login") | ||
| public ResponseEntity<GenericVulnerabilityResponseBean<Object>> login( | ||
| @RequestParam String username, @RequestParam String password) { | ||
| IDORLoginDetails loginDetails = idorLoginService.loginWithToken(username, password); |
There was a problem hiding this comment.
Thank you for using IDOR Login Service. I found a flaw in that service. I will try to fix it with JWT based setup where HS256 key is generated on server startup and will be used to sign the JWT which will be used to validate it in the later end.
There was a problem hiding this comment.
We did the change. please checkout.
| @@ -0,0 +1,8 @@ | |||
| INSERT INTO graphql_users VALUES (1, 'Alice', 'alice@example.com', 50000, 'USER'); | |||
There was a problem hiding this comment.
I think we are mutating the data so we should have per level data i.e. introduce level column into the table os one level doesn't update other level's data.
| </div> | ||
|
|
||
| <div class="gql-card"> | ||
| <h5>Query</h5> |
There was a problem hiding this comment.
I would suggest to hide this from user and let use find out that a graphql query is fired in the backgound and manipulate the query.
Create a simple button called say fetch profile or fetch order. In the hints section tell the user to fetch the profiule or other users order.
| LEVEL_1: `# IDOR - no authentication required.\n# Change the id argument to fetch any user's data.\n{\n user(id: 3) {\n id\n username\n email\n salary\n role\n }\n}`, | ||
| LEVEL_2: `# BOLA - login first, then fetch an order owned by another user.\n# Order 103 belongs to Bob. Login as Alice, then run this query.\n{\n order(id: 103) {\n id\n userId\n item\n amount\n }\n}`, | ||
| LEVEL_3: `# Privilege escalation - login as Alice (USER), then change her own role.\nmutation {\n updateUserRole(id: 1, role: "ADMIN") {\n id\n username\n role\n }\n}`, | ||
| LEVEL_4: `# Admin mutation exposure - login as any regular user, then call an admin-only op.\n{\n adminReport {\n id\n username\n email\n role\n }\n}`, |
There was a problem hiding this comment.
We should make the progression consistent across levels so the learning flow is clearer.
Level 1: vulnerable profile/order access where any user can fetch or update another user’s profile or order by changing the ID, and can also fetch their own order history.
Level 2: login is required, but a user can still fetch another user’s order and manipulate it.
Level 3: vulnerable to privilege escalation, where a regular user can modify their own role.
Level 4: vulnerable to function-level authorization issues, where a regular user can call an admin-only action. I am not sure how we can have consistent UX for this.
Level 5: the same actions should now be properly blocked by authorization checks.
This way each level follows the same UI mental model, and the security issue becomes easier to understand.
| private GraphQL buildLevel3() throws IOException { | ||
| // LEVEL 3 - Privilege Escalation: any authenticated user can change any user's role. | ||
| DataFetcher<GraphQLUser> meFetcher = env -> fetchUser(requireUser(env).getUserId()); | ||
| DataFetcher<GraphQLUser> userFetcher = |
There was a problem hiding this comment.
I think we can improve the design by extracting a core GraphQL service and placing a lightweight vulnerability/level layer on top of it.
This would keep schema + resolvers consistent across levels, and allow each level to only vary in authorization strategy (rather than duplicating GraphQL instances).
It will also make it easier to add future GraphQL attack scenarios like introspection abuse or query complexity attacks.
preetkaran20
left a comment
preetkaran20
left a comment
There was a problem hiding this comment.
great PR @zhouchristopher and this seems like an amazing introduction to the project. Can you please fix some of the comments.
Also, backend is not returning token when we login, can you start setting cookie for each level like level1_token or level2_token and then that cookie will be attached with the calls to backend from UI.
|
@zhouchristopher please let me know if you need any help with this PR. |
2 participants
Closes #530.
Adds a new top-level vulnerability category,
GraphQL Authorization & Access Control, with the four unsafe variants + one secure reference that matches the category-layout convention used by the rest of the app.Levels
user(id:)queryorder(id:)queryupdateUserRolemutationadminReportquery +deleteUsermutationFull per-level schemas, exploit
curlcommands, and a reproduction walk-through are indocs/GraphQLVulnerability.md.Implementation notes
graphql-java:21.5directly rather thanspring-boot-starter-graphql. The starter auto-configures a single/graphqlendpoint, which would conflict with VulnerableApp's per-level/<Category>/LEVEL_npattern.GraphQLinstance from its own SDL file so resolver fixes do not leak across levels.IDORLoginService(Base64-encoded JSON tokens); the decodedUseris attached toGraphQLContext. No new password-handling code.graphql_users,graphql_orders) seeded viaDataSourceInitializer. Kept separate fromidor_usersso mutations in this category don't corrupt IDOR state.VulnerabilityType.GRAPHQL_BROKEN_ACCESS_CONTROL(CWE-285, WASC-2)./scannerand the facade UI is automatic via the existing@VulnerableAppRestController/@AttackVectorannotations.static/templates/GraphQLVulnerability/LEVEL_1/drives all five levels; the JS pre-fills a level-appropriate default query.Tests
src/test/java/org/sasanlabs/service/vulnerability/graphql/GraphQLVulnerabilityTest.java— 12 JUnit 5 + Mockito cases covering:updateUserRoleanddeleteUserfor non-admins; blocks non-owneruser(id:); allows owner self-query; allows genuine admin; blocks a tampered token claimingrole=ADMINwhen the DB says the caller isUSERVerification
./gradlew clean build— green locally (Temurin JDK 17)./gradlew spotlessCheck— green (no style changes needed)./gradlew bootRun+curlsmoke of every level — all exploits behave as documented; all Level 5 defenses reject the same payloadscurl /VulnerableApp/scannerreturns five entries withvulnerabilityTypes: ["GRAPHQL_BROKEN_ACCESS_CONTROL"](UNSECURE × 4, SECURE × 1) — the new module auto-registers without manual catalog editsFiles
src/main/java/org/sasanlabs/service/vulnerability/graphql/src/main/resources/scripts/GraphQL/schema/level{1..4}.graphqlssrc/main/resources/scripts/GraphQL/db/{schema,data}.sqlsrc/main/resources/static/templates/GraphQLVulnerability/LEVEL_1/GraphQL.{html,css,js}src/main/resources/i18n/messages{,_en_US}.propertiessrc/main/resources/attackvectors/GraphQLVulnerabilityPayload.propertiessrc/test/java/org/sasanlabs/service/vulnerability/graphql/GraphQLVulnerabilityTest.javadocs/GraphQLVulnerability.md, plus entry #issues/13 #14 inREADME.mdbuild.gradle(graphql-java dep),VulnerableAppConfiguration(DB script registration),VulnerabilityType(enum)Summary by CodeRabbit
New Features
Documentation