Add 5 vulnerability modules: GraphQL injections, SSTI, Criteria QB#623
Open
djm726 wants to merge 6 commits into
Open
Add 5 vulnerability modules: GraphQL injections, SSTI, Criteria QB#623djm726 wants to merge 6 commits into
djm726 wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds five comprehensive security vulnerability demonstrations to the VulnerableApp framework: Criteria Query Builder injection via JPA, command injection through GraphQL mutations, SQL injection through GraphQL resolvers, NoSQL authentication bypass via operator injection, and Server-Side Template Injection via Thymeleaf. Each includes backend logic, frontend UI, tests, and documentation. ChangesVulnerability Demonstrations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #623 +/- ##
============================================
+ Coverage 56.24% 57.12% +0.88%
- Complexity 639 715 +76
============================================
Files 84 93 +9
Lines 3357 3690 +333
Branches 378 408 +30
============================================
+ Hits 1888 2108 +220
- Misses 1295 1387 +92
- Partials 174 195 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/main/resources/static/templates/ServerSideTemplateInjection/LEVEL_1/SSTI_Level1.html (1)
5-7: ⚡ Quick winAssociate the name field with an explicit
<label>for accessibility.This improves screen-reader context and click-target behavior with a minimal change.
♿ Suggested update
- <div id="input">please enter your name: - <input type="text" id="name"/> - <button id="greetBtn">Greet me</button> - </div> + <div id="input"> + <label for="name">Please enter your name:</label> + <input type="text" id="name"/> + <button id="greetBtn">Greet me</button> + </div>🤖 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/static/templates/ServerSideTemplateInjection/LEVEL_1/SSTI_Level1.html` around lines 5 - 7, Add an explicit accessible label for the name input: create a <label> element (e.g., for="name") with readable text such as "Name:" and associate it with the input that has id="name" (the <input id="name"/> inside the div with id="input"); keep the existing button id="greetBtn" unchanged and ensure the label is placed before the input so screen readers and click-to-focus work correctly.src/main/resources/static/templates/NoSQLAuthBypass/LEVEL_1/NoSQLAuthBypass.html (1)
4-6: ⚡ Quick winAdd explicit labels (or
aria-label) for form controls.Lines 4–6 currently depend on placeholders; adding labels improves keyboard/screen-reader usability with minimal change.
Proposed update
+ <label for="usernameInput">Username</label> <input type="text" id="usernameInput" placeholder='username (or JSON, e.g. {"$ne": null})' /> + <label for="passwordInput">Password</label> <input type="text" id="passwordInput" placeholder='password (or JSON, e.g. {"$ne": null})' />🤖 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/static/templates/NoSQLAuthBypass/LEVEL_1/NoSQLAuthBypass.html` around lines 4 - 6, Add accessible labels for the form controls by either adding <label> elements tied to the inputs (use for="usernameInput" and for="passwordInput") or by adding explicit aria-label attributes on the existing input elements (usernameInput and passwordInput); ensure the labels/aria-labels provide clear text like "Username" and "Password" and leave the loginButton as-is (or add aria-label if the button needs clearer context) so screen readers and keyboard users can identify each control.
🤖 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/configuration/MongoConfiguration.java`:
- Around line 15-22: The seedUsers CommandLineRunner runs unconditionally and
drops the User collection; scope it to the public profile by adding
`@Profile`("public") so it only runs in the non-production/testing "public"
environment. Annotate the MongoConfiguration class or the seedUsers(`@Bean`)
method with `@Profile`("public") (import
org.springframework.context.annotation.Profile) to prevent this destructive
seeding from executing against production MongoDB; keep the bean signature
seedUsers(MongoTemplate mongoTemplate) and existing logic unchanged.
In
`@src/main/java/org/sasanlabs/service/vulnerability/commandInjectionViaGraphql/CommandInjectionViaGraphQLVulnerability.java`:
- Line 124: The ping invocation in CommandInjectionViaGraphQLVulnerability uses
a hard-coded Unix flag ("-c 2") which fails on Windows; update both occurrences
where shellRunner.run("ping -c 2 " + ipaddress) is called to choose the correct
flag depending on the OS (use System.getProperty("os.name") or similar to detect
Windows and use "-n 2", otherwise use "-c 2"), build the command string
accordingly (e.g., flag = isWindows ? "-n 2" : "-c 2"; command = "ping " + flag
+ " " + ipaddress) and pass that command to shellRunner.run so ping behavior is
correct on Windows and Unix-like systems.
In
`@src/main/java/org/sasanlabs/service/vulnerability/graphqlInjection/SQLInjectionViaGraphQLVulnerability.java`:
- Around line 36-38: The controller mapping currently uses generic GraphQL names
and must be updated to the SQL-specific GraphQL assets: change the
`@VulnerableAppRestController` value from "GraphQLVulnerability" to the SQL
GraphQL identifier used by the new module (e.g., "SQLGraphQLVulnerability"), and
update any template/view references that use "SampleVulnerability" or the
generic GraphQL template to the new SQL GraphQL template path/name (e.g.,
"sql-graphql-vulnerability"); apply the same replacement to the other
occurrences in this file (the other `@VulnerableAppRestController` annotations and
any methods returning the template name) so the controller and template names
match the new SQL GraphQL module assets.
In
`@src/main/resources/static/templates/CommandInjectionViaGraphQL/LEVEL_1/CommandInjectionViaGraphQL.html`:
- Line 3: Add an explicit accessible label for the IP input element (id
"ipAddressInput") instead of relying solely on the placeholder: create a <label>
element that references the input via for="ipAddressInput" and contains
descriptive text (e.g., "IP address"), or if a visible label is not desired, add
an aria-label or aria-labelledby that points to a hidden descriptive element;
ensure the label text clearly matches the input purpose and that the input
retains id="ipAddressInput" for the association.
In
`@src/main/resources/static/templates/NoSQLAuthBypass/LEVEL_1/NoSQLAuthBypass.js`:
- Around line 44-45: Replace unsafe innerHTML usage when rendering the login
message: do not inject login.username into
document.getElementById("loginResult").innerHTML; instead create elements or set
the element's textContent/insert a text node and build the DOM using safe APIs
(e.g., getElementById("loginResult"), createElement("div"), appendChild with a
text node containing login.username) for both the occurrences that reference
login.username (the lines using innerHTML around "loginResult"); this prevents
HTML/script injection while preserving the original output.
In
`@src/main/resources/static/templates/SqlInjectionViaGraphQL/LEVEL_1/SqlInjactionViaGraphQL.html`:
- Around line 4-11: The car selector <select id="carId"> is unlabeled; add an
accessible label tied to that element (e.g., a <label for="carId">Car</label> or
a wrapping <label>Car<select id="carId">...</select></label>), or provide an
appropriate aria-labelledby/aria-label referencing "carId", so screen readers
and keyboard users can identify the control.
- Line 7: Update the user-facing option text for the dropdown option element
with value "6" by correcting the typo: change the visible label "Hundai" to
"Hyundai" in the <option value="6"> element so the car brand is spelled
correctly for users.
In
`@src/main/resources/static/templates/SqlInjectionViaGraphQL/LEVEL_1/SqlInjctionViaGraphQL.js`:
- Around line 18-29: The callback fetchCarInfoCallBack is using an undefined
variable car and a non-existent property car.isDataPresent; update the function
to parse the incoming payload (the data parameter) to extract the car object
from the GraphQL response (e.g., data.data.car or data.car depending on your
resolver), then check existence with a truthy check like if (car) before using
car.name and car.imagePath, and fall back to the "not present" branch when car
is null/undefined; also defensively handle missing name/image fields when
updating the carInformation element.
- Around line 20-26: Avoid using innerHTML with untrusted values: replace the
direct assignment to document.getElementById("carInformation").innerHTML that
injects car.name and car.imagePath with safe DOM APIs — create elements (e.g.,
div, img) and set their text via element.textContent for car.name and set the
image source via element.setAttribute('src', car.imagePath) or img.src (not via
string concatenation), then append those elements to the element returned by
getElementById("carInformation"); this prevents DOM XSS from car.name and
car.imagePath.
---
Nitpick comments:
In
`@src/main/resources/static/templates/NoSQLAuthBypass/LEVEL_1/NoSQLAuthBypass.html`:
- Around line 4-6: Add accessible labels for the form controls by either adding
<label> elements tied to the inputs (use for="usernameInput" and
for="passwordInput") or by adding explicit aria-label attributes on the existing
input elements (usernameInput and passwordInput); ensure the labels/aria-labels
provide clear text like "Username" and "Password" and leave the loginButton
as-is (or add aria-label if the button needs clearer context) so screen readers
and keyboard users can identify each control.
In
`@src/main/resources/static/templates/ServerSideTemplateInjection/LEVEL_1/SSTI_Level1.html`:
- Around line 5-7: Add an explicit accessible label for the name input: create a
<label> element (e.g., for="name") with readable text such as "Name:" and
associate it with the input that has id="name" (the <input id="name"/> inside
the div with id="input"); keep the existing button id="greetBtn" unchanged and
ensure the label is placed before the input so screen readers and click-to-focus
work correctly.
🪄 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: b2fc37e8-307e-4b80-98dd-6f3a95918c14
📒 Files selected for processing (41)
build.gradlesrc/main/java/org/sasanlabs/configuration/MongoConfiguration.javasrc/main/java/org/sasanlabs/configuration/VulnerableAppConfiguration.javasrc/main/java/org/sasanlabs/service/vulnerability/commandInjectionViaGraphql/CommandInjectionViaGraphQLVulnerability.javasrc/main/java/org/sasanlabs/service/vulnerability/criteriaQueryBuilderInjection/CriteriaQueryBuilderInjection.javasrc/main/java/org/sasanlabs/service/vulnerability/criteriaQueryBuilderInjection/Employee.javasrc/main/java/org/sasanlabs/service/vulnerability/criteriaQueryBuilderInjection/EmployeeRepository.javasrc/main/java/org/sasanlabs/service/vulnerability/criteriaQueryBuilderInjection/EmployeeView.javasrc/main/java/org/sasanlabs/service/vulnerability/graphqlInjection/SQLInjectionViaGraphQLVulnerability.javasrc/main/java/org/sasanlabs/service/vulnerability/nosqlInjection/NoSQLAuthBypassVulnerability.javasrc/main/java/org/sasanlabs/service/vulnerability/nosqlInjection/User.javasrc/main/java/org/sasanlabs/service/vulnerability/nosqlInjection/UserRepository.javasrc/main/java/org/sasanlabs/service/vulnerability/ssti/ServerSideTemplateInjection.javasrc/main/java/org/sasanlabs/vulnerability/types/VulnerabilityType.javasrc/main/resources/application.propertiessrc/main/resources/attackvectors/CommandInjectionViaGraphQLPayload.propertiessrc/main/resources/attackvectors/CriteriaQueryBuilderInjectionPayload.propertiessrc/main/resources/attackvectors/NoSQLInjectionPayload.propertiessrc/main/resources/i18n/messages.propertiessrc/main/resources/scripts/CriteriaQueryBuilderInjection/db/data.sqlsrc/main/resources/scripts/CriteriaQueryBuilderInjection/db/schema.sqlsrc/main/resources/static/templates/CommandInjectionViaGraphQL/LEVEL_1/CommandInjectionViaGraphQL.csssrc/main/resources/static/templates/CommandInjectionViaGraphQL/LEVEL_1/CommandInjectionViaGraphQL.htmlsrc/main/resources/static/templates/CommandInjectionViaGraphQL/LEVEL_1/CommandInjectionViaGraphQL.jssrc/main/resources/static/templates/CriteriaQueryBuilderInjection/LEVEL_1/CriteriaQB.csssrc/main/resources/static/templates/CriteriaQueryBuilderInjection/LEVEL_1/CriteriaQB.htmlsrc/main/resources/static/templates/CriteriaQueryBuilderInjection/LEVEL_1/CriteriaQB.jssrc/main/resources/static/templates/NoSQLAuthBypass/LEVEL_1/NoSQLAuthBypass.csssrc/main/resources/static/templates/NoSQLAuthBypass/LEVEL_1/NoSQLAuthBypass.htmlsrc/main/resources/static/templates/NoSQLAuthBypass/LEVEL_1/NoSQLAuthBypass.jssrc/main/resources/static/templates/ServerSideTemplateInjection/LEVEL_1/SSTI_Level1.csssrc/main/resources/static/templates/ServerSideTemplateInjection/LEVEL_1/SSTI_Level1.htmlsrc/main/resources/static/templates/ServerSideTemplateInjection/LEVEL_1/SSTI_Level1.jssrc/main/resources/static/templates/SqlInjectionViaGraphQL/LEVEL_1/SqlInjactionViaGraphQL.htmlsrc/main/resources/static/templates/SqlInjectionViaGraphQL/LEVEL_1/SqlInjctionViaGraphQL.jssrc/main/resources/static/templates/SqlInjectionViaGraphQL/LEVEL_1/SqlInjectionViaGraphQL.csssrc/test/java/org/sasanlabs/service/vulnerability/commandInjectionViaGraphql/CommandInjectionViaGraphQLVulnerabilityTest.javasrc/test/java/org/sasanlabs/service/vulnerability/criteriaQueryBuilderInjection/CriteriaQueryBuilderInjectionTest.javasrc/test/java/org/sasanlabs/service/vulnerability/graphqlInjection/SQLInjectionViaGraphQLVulnerabilityTest.javasrc/test/java/org/sasanlabs/service/vulnerability/nosqlInjection/NoSQLAuthBypassVulnerabilityTest.javasrc/test/java/org/sasanlabs/service/vulnerability/ssti/ServerSideTemplateInjectionTest.java
| private DataFetcher<Map<String, Object>> unsafePingFetcher() { | ||
| return env -> { | ||
| String ipaddress = env.getArgument("ipaddress"); | ||
| String output = shellRunner.run("ping -c 2 " + ipaddress); |
There was a problem hiding this comment.
Use OS-appropriate ping flags to keep behavior correct on Windows.
The command always uses ping -c 2, but Windows expects -n. The current implementation can fail or produce misleading output on Windows even for valid inputs.
Suggested fix
+ private static String buildPingCommand(String ipaddress) {
+ boolean isWindows = System.getProperty("os.name").toLowerCase().startsWith("windows");
+ return isWindows ? "ping -n 2 " + ipaddress : "ping -c 2 " + ipaddress;
+ }
+
private DataFetcher<Map<String, Object>> unsafePingFetcher() {
return env -> {
String ipaddress = env.getArgument("ipaddress");
- String output = shellRunner.run("ping -c 2 " + ipaddress);
+ String output = shellRunner.run(buildPingCommand(ipaddress));
return pingResult(ipaddress, output);
};
}
@@
boolean valid =
ipaddress != null
&& (IP_ADDRESS_PATTERN.matcher(ipaddress).matches()
|| "localhost".equals(ipaddress));
- String output = valid ? shellRunner.run("ping -c 2 " + ipaddress) : "";
+ String output = valid ? shellRunner.run(buildPingCommand(ipaddress)) : "";
return pingResult(ipaddress, output);
};
}Also applies to: 136-136
🤖 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/commandInjectionViaGraphql/CommandInjectionViaGraphQLVulnerability.java`
at line 124, The ping invocation in CommandInjectionViaGraphQLVulnerability uses
a hard-coded Unix flag ("-c 2") which fails on Windows; update both occurrences
where shellRunner.run("ping -c 2 " + ipaddress) is called to choose the correct
flag depending on the OS (use System.getProperty("os.name") or similar to detect
Windows and use "-n 2", otherwise use "-c 2"), build the command string
accordingly (e.g., flag = isWindows ? "-n 2" : "-c 2"; command = "ping " + flag
+ " " + ipaddress) and pass that command to shellRunner.run so ping behavior is
correct on Windows and Unix-like systems.
Comment on lines
+36
to
+38
| @VulnerableAppRestController( | ||
| descriptionLabel = "SQL_INJECTION_VIA_GRAPHQL_VULNERABILITY", | ||
| value = "GraphQLVulnerability") |
There was a problem hiding this comment.
Align controller/template mapping with the new SQL GraphQL module assets.
The mapping currently points to generic GraphQLVulnerability + SampleVulnerability, which does not match the new SQL GraphQL template path. This can route users to the wrong page or miss the intended UI.
Suggested fix
`@VulnerableAppRestController`(
descriptionLabel = "SQL_INJECTION_VIA_GRAPHQL_VULNERABILITY",
- value = "GraphQLVulnerability")
+ value = "SqlInjectionViaGraphQL")
public class SQLInjectionViaGraphQLVulnerability {
@@
`@VulnerableAppRequestMapping`(
value = LevelConstants.LEVEL_1,
- htmlTemplate = "LEVEL_1/SampleVulnerability",
+ htmlTemplate = "LEVEL_1/SqlInjactionViaGraphQL",
requestMethod = RequestMethod.POST)
@@
`@VulnerableAppRequestMapping`(
value = LevelConstants.LEVEL_2,
- htmlTemplate = "LEVEL_1/SampleVulnerability",
+ htmlTemplate = "LEVEL_1/SqlInjactionViaGraphQL",
requestMethod = RequestMethod.POST,
variant = Variant.SECURE)Also applies to: 143-145, 153-155
🤖 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/graphqlInjection/SQLInjectionViaGraphQLVulnerability.java`
around lines 36 - 38, The controller mapping currently uses generic GraphQL
names and must be updated to the SQL-specific GraphQL assets: change the
`@VulnerableAppRestController` value from "GraphQLVulnerability" to the SQL
GraphQL identifier used by the new module (e.g., "SQLGraphQLVulnerability"), and
update any template/view references that use "SampleVulnerability" or the
generic GraphQL template to the new SQL GraphQL template path/name (e.g.,
"sql-graphql-vulnerability"); apply the same replacement to the other
occurrences in this file (the other `@VulnerableAppRestController` annotations and
any methods returning the template name) so the controller and template names
match the new SQL GraphQL module assets.
| @@ -0,0 +1,8 @@ | |||
| <div id="ci_graphql"> | |||
| <div> | |||
| <input type="text" id="ipAddressInput" placeholder='ipaddress (try: 127.0.0.1; cat /etc/passwd)' /> | |||
There was a problem hiding this comment.
Add an explicit label for the IP input.
Relying on placeholder text alone hurts screen-reader usability and form accessibility.
Suggested fix
- <input type="text" id="ipAddressInput" placeholder='ipaddress (try: 127.0.0.1; cat /etc/passwd)' />
+ <label for="ipAddressInput">IP address</label>
+ <input type="text" id="ipAddressInput" placeholder='ipaddress (try: 127.0.0.1; cat /etc/passwd)' />📝 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.
Suggested change
| <input type="text" id="ipAddressInput" placeholder='ipaddress (try: 127.0.0.1; cat /etc/passwd)' /> | |
| <label for="ipAddressInput">IP address</label> | |
| <input type="text" id="ipAddressInput" placeholder='ipaddress (try: 127.0.0.1; cat /etc/passwd)' /> |
🤖 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/static/templates/CommandInjectionViaGraphQL/LEVEL_1/CommandInjectionViaGraphQL.html`
at line 3, Add an explicit accessible label for the IP input element (id
"ipAddressInput") instead of relying solely on the placeholder: create a <label>
element that references the input via for="ipAddressInput" and contains
descriptive text (e.g., "IP address"), or if a visible label is not desired, add
an aria-label or aria-labelledby that points to a hidden descriptive element;
ensure the label text clearly matches the input purpose and that the input
retains id="ipAddressInput" for the association.
Comment on lines
+44
to
+45
| document.getElementById("loginResult").innerHTML = | ||
| "<div>Logged in as " + login.username + "</div>"; |
There was a problem hiding this comment.
Avoid HTML injection when rendering login messages.
Line [44] and Line [47] use innerHTML with dynamic data. If login.username is tainted, this enables script/HTML injection in the page.
🔧 Suggested fix
function loginCallBack(response) {
let login = response && response.data && response.data.login;
+ const loginResult = document.getElementById("loginResult");
if (login && login.loggedIn) {
- document.getElementById("loginResult").innerHTML =
- "<div>Logged in as " + login.username + "</div>";
+ loginResult.textContent = "Logged in as " + login.username;
} else {
- document.getElementById("loginResult").innerHTML =
- "<div>Invalid credentials</div>";
+ loginResult.textContent = "Invalid credentials";
}
}Also applies to: 47-48
🤖 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/static/templates/NoSQLAuthBypass/LEVEL_1/NoSQLAuthBypass.js`
around lines 44 - 45, Replace unsafe innerHTML usage when rendering the login
message: do not inject login.username into
document.getElementById("loginResult").innerHTML; instead create elements or set
the element's textContent/insert a text node and build the DOM using safe APIs
(e.g., getElementById("loginResult"), createElement("div"), appendChild with a
text node containing login.username) for both the occurrences that reference
login.username (the lines using innerHTML around "loginResult"); this prevents
HTML/script injection while preserving the original output.
Comment on lines
+4
to
+11
| <select id="carId"> | ||
| <option value="1">Audi</option> | ||
| <option value="2">BMW</option> | ||
| <option value="6">Hundai</option> | ||
| <option value="3">Mercedes-Benz</option> | ||
| <option value="4">Skoda</option> | ||
| <option value="5">Tata</option> | ||
| </select> |
There was a problem hiding this comment.
Add a label for the car selector.
The <select> is currently unlabeled, which hurts accessibility and keyboard/screen-reader navigation.
Suggested fix
- <select id="carId">
+ <label for="carId">Select car</label>
+ <select id="carId">📝 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.
Suggested change
| <select id="carId"> | |
| <option value="1">Audi</option> | |
| <option value="2">BMW</option> | |
| <option value="6">Hundai</option> | |
| <option value="3">Mercedes-Benz</option> | |
| <option value="4">Skoda</option> | |
| <option value="5">Tata</option> | |
| </select> | |
| <label for="carId">Select car</label> | |
| <select id="carId"> | |
| <option value="1">Audi</option> | |
| <option value="2">BMW</option> | |
| <option value="6">Hundai</option> | |
| <option value="3">Mercedes-Benz</option> | |
| <option value="4">Skoda</option> | |
| <option value="5">Tata</option> | |
| </select> |
🤖 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/static/templates/SqlInjectionViaGraphQL/LEVEL_1/SqlInjactionViaGraphQL.html`
around lines 4 - 11, The car selector <select id="carId"> is unlabeled; add an
accessible label tied to that element (e.g., a <label for="carId">Car</label> or
a wrapping <label>Car<select id="carId">...</select></label>), or provide an
appropriate aria-labelledby/aria-label referencing "carId", so screen readers
and keyboard users can identify the control.
| <select id="carId"> | ||
| <option value="1">Audi</option> | ||
| <option value="2">BMW</option> | ||
| <option value="6">Hundai</option> |
There was a problem hiding this comment.
Fix the car brand typo in option text.
Hundai should be Hyundai for user-facing correctness.
Suggested fix
- <option value="6">Hundai</option>
+ <option value="6">Hyundai</option>📝 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.
Suggested change
| <option value="6">Hundai</option> | |
| <option value="6">Hyundai</option> |
🤖 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/static/templates/SqlInjectionViaGraphQL/LEVEL_1/SqlInjactionViaGraphQL.html`
at line 7, Update the user-facing option text for the dropdown option element
with value "6" by correcting the typo: change the visible label "Hundai" to
"Hyundai" in the <option value="6"> element so the car brand is spelled
correctly for users.
Comment on lines
+18
to
+29
| function fetchCarInfoCallBack(data) { | ||
| if (car.isDataPresent) { | ||
| document.getElementById("carInformation").innerHTML = | ||
| "<div>Car is Present: " + | ||
| car.name + | ||
| "</div>" + | ||
| "<img src='" + | ||
| car.imagePath + | ||
| "' width='900'/>"; | ||
| } else { | ||
| document.getElementById("carInformation").innerHTML = | ||
| "<div>Car is not Present</div>"; |
There was a problem hiding this comment.
Fix callback payload parsing and existence check.
Line 19 uses car without defining it, and car.isDataPresent is not part of the queried GraphQL fields. This will break the callback path at runtime.
Proposed fix
function fetchCarInfoCallBack(data) {
- if (car.isDataPresent) {
+ const car = data?.data?.car;
+ if (car) {
document.getElementById("carInformation").innerHTML =
"<div>Car is Present: " +
car.name +
"</div>" +
"<img src='" +
car.imagePath +
"' width='900'/>";
} else {
document.getElementById("carInformation").innerHTML =
"<div>Car is not Present</div>";
}
}📝 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.
Suggested change
| function fetchCarInfoCallBack(data) { | |
| if (car.isDataPresent) { | |
| document.getElementById("carInformation").innerHTML = | |
| "<div>Car is Present: " + | |
| car.name + | |
| "</div>" + | |
| "<img src='" + | |
| car.imagePath + | |
| "' width='900'/>"; | |
| } else { | |
| document.getElementById("carInformation").innerHTML = | |
| "<div>Car is not Present</div>"; | |
| function fetchCarInfoCallBack(data) { | |
| const car = data?.data?.car; | |
| if (car) { | |
| document.getElementById("carInformation").innerHTML = | |
| "<div>Car is Present: " + | |
| car.name + | |
| "</div>" + | |
| "<img src='" + | |
| car.imagePath + | |
| "' width='900'/>"; | |
| } else { | |
| document.getElementById("carInformation").innerHTML = | |
| "<div>Car is not Present</div>"; | |
| } | |
| } |
🤖 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/static/templates/SqlInjectionViaGraphQL/LEVEL_1/SqlInjctionViaGraphQL.js`
around lines 18 - 29, The callback fetchCarInfoCallBack is using an undefined
variable car and a non-existent property car.isDataPresent; update the function
to parse the incoming payload (the data parameter) to extract the car object
from the GraphQL response (e.g., data.data.car or data.car depending on your
resolver), then check existence with a truthy check like if (car) before using
car.name and car.imagePath, and fall back to the "not present" branch when car
is null/undefined; also defensively handle missing name/image fields when
updating the carInformation element.
Comment on lines
+20
to
+26
| document.getElementById("carInformation").innerHTML = | ||
| "<div>Car is Present: " + | ||
| car.name + | ||
| "</div>" + | ||
| "<img src='" + | ||
| car.imagePath + | ||
| "' width='900'/>"; |
There was a problem hiding this comment.
Avoid rendering server values via innerHTML.
Lines 20–26 inject car.name/car.imagePath directly into HTML. With this module’s SQLi behavior, attacker-controlled values can become DOM XSS.
Safer rendering approach
- document.getElementById("carInformation").innerHTML =
- "<div>Car is Present: " +
- car.name +
- "</div>" +
- "<img src='" +
- car.imagePath +
- "' width='900'/>";
+ const container = document.getElementById("carInformation");
+ container.textContent = "";
+ const title = document.createElement("div");
+ title.textContent = `Car is Present: ${car.name ?? ""}`;
+ const img = document.createElement("img");
+ img.setAttribute("src", car.imagePath ?? "");
+ img.setAttribute("width", "900");
+ container.appendChild(title);
+ container.appendChild(img);📝 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.
Suggested change
| document.getElementById("carInformation").innerHTML = | |
| "<div>Car is Present: " + | |
| car.name + | |
| "</div>" + | |
| "<img src='" + | |
| car.imagePath + | |
| "' width='900'/>"; | |
| const container = document.getElementById("carInformation"); | |
| container.textContent = ""; | |
| const title = document.createElement("div"); | |
| title.textContent = `Car is Present: ${car.name ?? ""}`; | |
| const img = document.createElement("img"); | |
| img.setAttribute("src", car.imagePath ?? ""); | |
| img.setAttribute("width", "900"); | |
| container.appendChild(title); | |
| container.appendChild(img); |
🤖 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/static/templates/SqlInjectionViaGraphQL/LEVEL_1/SqlInjctionViaGraphQL.js`
around lines 20 - 26, Avoid using innerHTML with untrusted values: replace the
direct assignment to document.getElementById("carInformation").innerHTML that
injects car.name and car.imagePath with safe DOM APIs — create elements (e.g.,
div, img) and set their text via element.textContent for car.name and set the
image source via element.setAttribute('src', car.imagePath) or img.src (not via
string concatenation), then append those elements to the element returned by
getElementById("carInformation"); this prevents DOM XSS from car.name and
car.imagePath.
Member
|
@djm726 thanks a lot for the PR. This PR contains too much of a code change. I would suggest breaking into multiple PR's for easier review and debugging. |
Comment on lines
+1
to
+183
| package org.sasanlabs.service.vulnerability.commandInjectionViaGraphql; | ||
|
|
||
| import graphql.ExecutionInput; | ||
| import graphql.ExecutionResult; | ||
| import graphql.GraphQL; | ||
| import graphql.schema.DataFetcher; | ||
| import graphql.schema.GraphQLSchema; | ||
| import graphql.schema.idl.RuntimeWiring; | ||
| import graphql.schema.idl.SchemaGenerator; | ||
| import graphql.schema.idl.SchemaParser; | ||
| import graphql.schema.idl.TypeDefinitionRegistry; | ||
| import java.io.BufferedReader; | ||
| import java.io.IOException; | ||
| import java.io.InputStreamReader; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.Map; | ||
| import java.util.regex.Pattern; | ||
| import org.sasanlabs.internal.utility.LevelConstants; | ||
| import org.sasanlabs.internal.utility.Variant; | ||
| import org.sasanlabs.internal.utility.annotations.AttackVector; | ||
| import org.sasanlabs.internal.utility.annotations.VulnerableAppRequestMapping; | ||
| import org.sasanlabs.internal.utility.annotations.VulnerableAppRestController; | ||
| import org.sasanlabs.vulnerability.types.VulnerabilityType; | ||
| import org.springframework.context.annotation.Profile; | ||
| import org.springframework.http.HttpStatus; | ||
| import org.springframework.http.ResponseEntity; | ||
| import org.springframework.web.bind.annotation.RequestBody; | ||
| import org.springframework.web.bind.annotation.RequestMethod; | ||
|
|
||
| /** | ||
| * Command injection exposed through a GraphQL mutation. The schema declares {@code | ||
| * runPing(ipaddress: String!)} and the unsafe resolver concatenates the argument straight into a | ||
| * {@code sh -c} string, so any shell metacharacter ({@code ;}, {@code |}, {@code &&}, {@code $()}, | ||
| * backtick, {@code %0A}) flows into the OS shell. Beyond the standard sink, GraphQL adds an | ||
| * amplification primitive: a single request can fan the mutation out via aliases so the injected | ||
| * command runs N times in parallel from one HTTP call. | ||
| */ | ||
| @Profile("public") | ||
| @VulnerableAppRestController( | ||
| descriptionLabel = "COMMAND_INJECTION_VIA_GRAPHQL_VULNERABILITY", | ||
| value = "CommandInjectionViaGraphQL") | ||
| public class CommandInjectionViaGraphQLVulnerability { | ||
|
|
||
| private static final String SCHEMA = | ||
| "type PingResult { output: String!, ipaddress: String! }\n" | ||
| + "type Mutation { runPing(ipaddress: String!): PingResult }\n" | ||
| + "type Query { _empty: String }\n"; | ||
|
|
||
| private static final Pattern IP_ADDRESS_PATTERN = | ||
| Pattern.compile("\\b((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\\.|$)){4}\\b"); | ||
|
|
||
| /** Boundary that the test replaces; production wiring shells out via {@link ProcessBuilder}. */ | ||
| @FunctionalInterface | ||
| public interface ShellRunner { | ||
| String run(String shellCommand) throws IOException; | ||
| } | ||
|
|
||
| public static class GraphQLRequest { | ||
| private String query; | ||
| private Map<String, Object> variables; | ||
|
|
||
| public String getQuery() { | ||
| return this.query; | ||
| } | ||
|
|
||
| public void setQuery(String query) { | ||
| this.query = query; | ||
| } | ||
|
|
||
| public Map<String, Object> getVariables() { | ||
| return this.variables; | ||
| } | ||
|
|
||
| public void setVariables(Map<String, Object> variables) { | ||
| this.variables = variables; | ||
| } | ||
| } | ||
|
|
||
| private final ShellRunner shellRunner; | ||
| private final GraphQL unsafeGraphQL; | ||
| private final GraphQL safeGraphQL; | ||
|
|
||
| public CommandInjectionViaGraphQLVulnerability() { | ||
| this(defaultShellRunner()); | ||
| } | ||
|
|
||
| CommandInjectionViaGraphQLVulnerability(ShellRunner shellRunner) { | ||
| this.shellRunner = shellRunner; | ||
| this.unsafeGraphQL = buildGraphQL(unsafePingFetcher()); | ||
| this.safeGraphQL = buildGraphQL(safePingFetcher()); | ||
| } | ||
|
|
||
| private static ShellRunner defaultShellRunner() { | ||
| return command -> { | ||
| boolean isWindows = System.getProperty("os.name").toLowerCase().startsWith("windows"); | ||
| String[] argv = | ||
| isWindows | ||
| ? new String[] {"cmd", "/c", command} | ||
| : new String[] {"sh", "-c", command}; | ||
| Process process = new ProcessBuilder(argv).redirectErrorStream(true).start(); | ||
| StringBuilder out = new StringBuilder(); | ||
| try (BufferedReader br = | ||
| new BufferedReader(new InputStreamReader(process.getInputStream()))) { | ||
| br.lines().forEach(l -> out.append(l).append("\n")); | ||
| } | ||
| return out.toString(); | ||
| }; | ||
| } | ||
|
|
||
| private GraphQL buildGraphQL(DataFetcher<Map<String, Object>> pingFetcher) { | ||
| TypeDefinitionRegistry tdr = new SchemaParser().parse(SCHEMA); | ||
| RuntimeWiring wiring = | ||
| RuntimeWiring.newRuntimeWiring() | ||
| .type("Mutation", b -> b.dataFetcher("runPing", pingFetcher)) | ||
| .build(); | ||
| GraphQLSchema schema = new SchemaGenerator().makeExecutableSchema(tdr, wiring); | ||
| return GraphQL.newGraphQL(schema).build(); | ||
| } | ||
|
|
||
| private DataFetcher<Map<String, Object>> unsafePingFetcher() { | ||
| return env -> { | ||
| String ipaddress = env.getArgument("ipaddress"); | ||
| String output = shellRunner.run("ping -c 2 " + ipaddress); | ||
| return pingResult(ipaddress, output); | ||
| }; | ||
| } | ||
|
|
||
| private DataFetcher<Map<String, Object>> safePingFetcher() { | ||
| return env -> { | ||
| String ipaddress = env.getArgument("ipaddress"); | ||
| boolean valid = | ||
| ipaddress != null | ||
| && (IP_ADDRESS_PATTERN.matcher(ipaddress).matches() | ||
| || "localhost".equals(ipaddress)); | ||
| String output = valid ? shellRunner.run("ping -c 2 " + ipaddress) : ""; | ||
| return pingResult(ipaddress, output); | ||
| }; | ||
| } | ||
|
|
||
| private static Map<String, Object> pingResult(String ipaddress, String output) { | ||
| Map<String, Object> result = new HashMap<>(); | ||
| result.put("ipaddress", ipaddress == null ? "" : ipaddress); | ||
| result.put("output", output); | ||
| return result; | ||
| } | ||
|
|
||
| private ResponseEntity<Map<String, Object>> execute(GraphQL graphQL, GraphQLRequest request) { | ||
| if (request == null || request.getQuery() == null) { | ||
| return new ResponseEntity<>(HttpStatus.BAD_REQUEST); | ||
| } | ||
| ExecutionInput.Builder input = ExecutionInput.newExecutionInput().query(request.getQuery()); | ||
| input.variables( | ||
| request.getVariables() != null ? request.getVariables() : Collections.emptyMap()); | ||
| ExecutionResult result = graphQL.execute(input.build()); | ||
| return new ResponseEntity<>(result.toSpecification(), HttpStatus.OK); | ||
| } | ||
|
|
||
| @AttackVector( | ||
| vulnerabilityExposed = VulnerabilityType.COMMAND_INJECTION, | ||
| description = "COMMAND_INJECTION_GRAPHQL_MUTATION_ARG_DIRECTLY_EXECUTED", | ||
| payload = "COMMAND_INJECTION_GRAPHQL_SHELL_METACHAR") | ||
| @VulnerableAppRequestMapping( | ||
| value = LevelConstants.LEVEL_1, | ||
| htmlTemplate = "LEVEL_1/CommandInjectionViaGraphQL", | ||
| requestMethod = RequestMethod.POST) | ||
| public ResponseEntity<Map<String, Object>> runPingLevel1(@RequestBody GraphQLRequest request) { | ||
| return execute(unsafeGraphQL, request); | ||
| } | ||
|
|
||
| /** | ||
| * Secure level: the resolver validates the {@code ipaddress} argument against an IPv4 regex (or | ||
| * {@code localhost}) before invoking the shell, so metacharacters never reach {@code sh}. | ||
| */ | ||
| @VulnerableAppRequestMapping( | ||
| value = LevelConstants.LEVEL_2, | ||
| htmlTemplate = "LEVEL_1/CommandInjectionViaGraphQL", | ||
| requestMethod = RequestMethod.POST, | ||
| variant = Variant.SECURE) | ||
| public ResponseEntity<Map<String, Object>> runPingLevel2(@RequestBody GraphQLRequest request) { | ||
| return execute(safeGraphQL, request); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
2 suggestions:
- we have already implemented command injection by giving ping utility. I think it is better to think more about usecases before implementation.
- we want to teach/scanner test atleast 4-5 levels.
Can you please think what levels and usecases we should add to make this Vulnerability complete?
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.
Summary
Adds five new vulnerability modules covering modern injection classes that weren't previously represented. Each module shipsvwith one or more vulnerable levels and a secure-baseline level, plus per-level i18n descriptions, attack-vector payload hints, and a Legacy UI page. All five new controllers are annotated
@Profile("public")consistent with #621.Modules
SQLInjectionViaGraphQLVulnerabilitycar(id: String!)flows through unchanged when the resolver concatenates the arg into JDBC. Secure level binds via prepared statement.NoSQLAuthBypassVulnerabilityAnyscalar onlogin(username, password)lets{"$ne": null}reach Spring Data Mongo as a BSON operator document. Secure level pins the args toString!.CommandInjectionViaGraphQLVulnerabilityrunPingmutation concatenatesipaddressintosh -c. Beyond the standard sink, demonstrates GraphQL aliasing as an amplification primitive: one HTTP call fans the unsafe mutation out N times in parallel. Secure level validates against an IPv4 regex.ServerSideTemplateInjectionStringTemplateResolver. Level 1 is the naive sink; Levels 2 & 3 progressively tighten a blacklist that's then bypassed via alternative SpEL constructs (T(...),[[...]]).CriteriaQueryBuilderInjectionfield/sortflows intoRoot.get(String), so the attacker can pivot WHERE/ORDER BY onto sensitive columns (salary,social_security_number) that are never returned in the response — using row presence and ordering as a side-channel oracle. Four levels: direct, sort-order leak, blacklist bypass, whitelist (secure).Highlights
VulnerabilityType.java— five new enum entries (one isCRITERIA_QUERY_BUILDER_INJECTION, the others reuse existing CWEs where applicable)messages.properties— full descriptions and per-level attack-vector explanations for each new moduleattackvectors/*.properties— payload hints exposed in the UIEmployeefor the Criteria-QB module, with both public columns (name,department) and sensitive columns (salary,socialSecurityNumber), seeded viascripts/CriteriaQueryBuilderInjection/db/{schema,data}.sqlTests
*Test.javanext to its source.CriteriaQueryBuilderInjectionTest(12 tests) uses Mockito withArgumentCaptorto verify thatattacker-controlled field paths reach
Root.get(String)— mirroring the mocking pattern used insqlInjection/UnionBasedSQLInjectionVulnerabilityTest.Verification
./gradlew spotlessCheckCriteriaQueryBuilderInjectionTest(12 tests) uses Mockito withArgumentCaptorto verify that attacker-controlled field paths reachRoot.get(String)— mirroring the mocking pattern used insqlInjection/UnionBasedSQLInjectionVulnerabilityTest.Verification
./gradlew spotlessCheck./gradlew test(full suite, including new tests)./gradlew bootRunstarts cleanly; all five modules appear in the vuln list athttp://localhost:9090/VulnerableAppTest plan
./gradlew spotlessCheck test/VulnerableApp/h2shows the newEMPLOYEEStable seededNotes for reviewers
already logically separated (one per module, plus a final commit adding
the
@Profileannotation and the Criteria-QB test).Criteria API ultimately renders to SQL, but I'm open to a different
classification if the project has a convention here.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores