fix: Success if only Smith earn most salary (#1744)

* Update labels

* Update Java

* Update Test

---------

Co-authored-by: René Zubcevic <rene@zubcevic.com>

src/main/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson9.java

@@ -63,66 +63,74 @@ public class SqlInjectionLesson9 extends AssignmentEndpoint {
 
   protected AttackResult injectableQueryIntegrity(String name, String auth_tan) {
     StringBuilder output = new StringBuilder();
-    String query =
+    String queryInjection =
         "SELECT * FROM employees WHERE last_name = '"
             + name
             + "' AND auth_tan = '"
             + auth_tan
             + "'";
     try (Connection connection = dataSource.getConnection()) {
-      try {
+      // V2019_09_26_7__employees.sql
+      int oldMaxSalary = this.getMaxSalary(connection);
+      int oldSumSalariesOfOtherEmployees = this.getSumSalariesOfOtherEmployees(connection);
+      // begin transaction
+      connection.setAutoCommit(false);
+      // do injectable query
       Statement statement = connection.createStatement(TYPE_SCROLL_SENSITIVE, CONCUR_UPDATABLE);
-        SqlInjectionLesson8.log(connection, query);
+      SqlInjectionLesson8.log(connection, queryInjection);
-        ResultSet results = statement.executeQuery(query);
+      statement.execute(queryInjection);
-        var test = results.getRow() != 0;
+      // check new sum of salaries other employees and new salaries of John
-        if (results.getStatement() != null) {
+      int newJohnSalary = this.getJohnSalary(connection);
-          if (results.first()) {
+      int newSumSalariesOfOtherEmployees = this.getSumSalariesOfOtherEmployees(connection);
-            output.append(SqlInjectionLesson8.generateTable(results));
+      if (newJohnSalary > oldMaxSalary
-          } else {
+          && newSumSalariesOfOtherEmployees == oldSumSalariesOfOtherEmployees) {
-            // no results
+        // success commit
-            return failed(this).feedback("sql-injection.8.no.results").build();
+        connection.commit(); // need execute not executeQuery
-          }
+        connection.setAutoCommit(true);
+        output.append(
+            SqlInjectionLesson8.generateTable(this.getEmployeesDataOrderBySalaryDesc(connection)));
+        return success(this).feedback("sql-injection.9.success").output(output.toString()).build();
       }
+      // failed roolback
+      connection.rollback();
+      return failed(this)
+          .feedback("sql-injection.9.one")
+          .output(
+              SqlInjectionLesson8.generateTable(this.getEmployeesDataOrderBySalaryDesc(connection)))
+          .build();
     } catch (SQLException e) {
       System.err.println(e.getMessage());
       return failed(this)
           .output("<br><span class='feedback-negative'>" + e.getMessage() + "</span>")
           .build();
     }
-
-      return checkSalaryRanking(connection, output);
-
-    } catch (Exception e) {
-      System.err.println(e.getMessage());
-      return failed(this)
-          .output("<br><span class='feedback-negative'>" + e.getMessage() + "</span>")
-          .build();
-    }
   }
 
-  private AttackResult checkSalaryRanking(Connection connection, StringBuilder output) {
+  private int getSqlInt(Connection connection, String query) throws SQLException {
-    try {
+    Statement statement = connection.createStatement(TYPE_SCROLL_SENSITIVE, CONCUR_UPDATABLE);
-      String query = "SELECT * FROM employees ORDER BY salary DESC";
-      try (Statement statement =
-          connection.createStatement(TYPE_SCROLL_SENSITIVE, CONCUR_UPDATABLE); ) {
     ResultSet results = statement.executeQuery(query);
-
     results.first();
-        // user completes lesson if John Smith is the first in the list
+    return results.getInt(1);
-        if ((results.getString(2).equals("John")) && (results.getString(3).equals("Smith"))) {
+  }
-          output.append(SqlInjectionLesson8.generateTable(results));
+
-          return success(this)
+  private int getMaxSalary(Connection connection) throws SQLException {
-              .feedback("sql-injection.9.success")
+    String query = "SELECT max(salary) FROM employees";
-              .output(output.toString())
+    return this.getSqlInt(connection, query);
-              .build();
+  }
-        } else {
+
-          return failed(this).feedback("sql-injection.9.one").output(output.toString()).build();
+  private int getSumSalariesOfOtherEmployees(Connection connection) throws SQLException {
-        }
+    String query = "SELECT sum(salary) FROM employees WHERE auth_tan != '3SL99A'";
-      }
+    return this.getSqlInt(connection, query);
-    } catch (SQLException e) {
+  }
-      return failed(this)
+
-          .output("<br><span class='feedback-negative'>" + e.getMessage() + "</span>")
+  private int getJohnSalary(Connection connection) throws SQLException {
-          .build();
+    String query = "SELECT salary FROM employees WHERE auth_tan = '3SL99A'";
-    }
+    return this.getSqlInt(connection, query);
+  }
+
+  private ResultSet getEmployeesDataOrderBySalaryDesc(Connection connection) throws SQLException {
+    String query = "SELECT * FROM employees ORDER BY salary DESC";
+    Statement statement = connection.createStatement(TYPE_SCROLL_SENSITIVE, CONCUR_UPDATABLE);
+    return statement.executeQuery(query);
   }
 }

src/main/resources/lessons/sqlinjection/i18n/WebGoatLabels.properties

@@ -63,8 +63,8 @@ SqlStringInjectionHint.8.3=Try appending a SQL statement that always resolves to
 SqlStringInjectionHint.8.4=Make sure all quotes (" ' ") are opened and closed properly so the resulting SQL query is syntactically correct.
 SqlStringInjectionHint.8.5=Try extending the WHERE clause of the statement by adding something like: ' OR '1' = '1.
 
-sql-injection.9.success=Well done! Now you are earning the most money. And at the same time you successfully compromised the integrity of data by changing the salary!
+sql-injection.9.success=Well done! Now you are earning the most money. And at the same time you successfully compromised the integrity of data by changing your salary!
-sql-injection.9.one=Still not earning enough! Better try again and change that.
+sql-injection.9.one=Still not earning enough or only your own salary must be changed! Better try again and change that.
 SqlStringInjectionHint.9.1=Try to find a way, to chain another query to the end of the existing one.
 SqlStringInjectionHint.9.2=Use the ; metacharacter to do so.
 SqlStringInjectionHint.9.3=Make use of DML to change your salary.

src/test/java/org/owasp/webgoat/lessons/sqlinjection/introduction/SqlInjectionLesson9Test.java

@@ -37,123 +37,7 @@ import org.springframework.test.web.servlet.request.MockMvcRequestBuilders;
  */
 public class SqlInjectionLesson9Test extends SqlLessonTest {
 
-  private String completedError = "JSON path \"lessonCompleted\"";
+  private final String completedError = "JSON path \"lessonCompleted\"";
-
-  @Test
-  public void oneAccount() throws Exception {
-    try {
-      mockMvc
-          .perform(
-              MockMvcRequestBuilders.post("/SqlInjection/attack9")
-                  .param("name", "Smith")
-                  .param("auth_tan", "3SL99A"))
-          .andExpect(status().isOk())
-          .andExpect(jsonPath("lessonCompleted", is(false)))
-          .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.one"))))
-          .andExpect(jsonPath("$.output", containsString("<table><tr><th>")));
-    } catch (AssertionError e) {
-      if (!e.getMessage().contains(completedError)) throw e;
-
-      mockMvc
-          .perform(
-              MockMvcRequestBuilders.post("/SqlInjection/attack9")
-                  .param("name", "Smith")
-                  .param("auth_tan", "3SL99A"))
-          .andExpect(status().isOk())
-          .andExpect(jsonPath("lessonCompleted", is(true)))
-          .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.success"))))
-          .andExpect(jsonPath("$.output", containsString("<table><tr><th>")));
-    }
-  }
-
-  @Test
-  public void multipleAccounts() throws Exception {
-    try {
-      mockMvc
-          .perform(
-              MockMvcRequestBuilders.post("/SqlInjection/attack9")
-                  .param("name", "Smith")
-                  .param("auth_tan", "3SL99A' OR '1' = '1"))
-          .andExpect(status().isOk())
-          .andExpect(jsonPath("lessonCompleted", is(false)))
-          .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.one"))))
-          .andExpect(
-              jsonPath(
-                  "$.output",
-                  containsString(
-                      "<tr><td>96134<\\/td><td>Bob<\\/td><td>Franco<\\/td><td>Marketing<\\/td><td>83700<\\/td><td>LO9S2V<\\/td><\\/tr>")));
-    } catch (AssertionError e) {
-      if (!e.getMessage().contains(completedError)) throw e;
-
-      mockMvc
-          .perform(
-              MockMvcRequestBuilders.post("/SqlInjection/attack9")
-                  .param("name", "Smith")
-                  .param("auth_tan", "3SL99A' OR '1' = '1"))
-          .andExpect(status().isOk())
-          .andExpect(jsonPath("lessonCompleted", is(true)))
-          .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.success"))))
-          .andExpect(
-              jsonPath(
-                  "$.output",
-                  containsString(
-                      "<tr><td>96134<\\/td><td>Bob<\\/td><td>Franco<\\/td><td>Marketing<\\/td><td>83700<\\/td><td>LO9S2V<\\/td><\\/tr>")));
-    }
-  }
-
-  @Test
-  public void wrongNameReturnsNoAccounts() throws Exception {
-    try {
-      mockMvc
-          .perform(
-              MockMvcRequestBuilders.post("/SqlInjection/attack9")
-                  .param("name", "Smithh")
-                  .param("auth_tan", "3SL99A"))
-          .andExpect(status().isOk())
-          .andExpect(jsonPath("lessonCompleted", is(false)))
-          .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.8.no.results"))))
-          .andExpect(jsonPath("$.output").doesNotExist());
-    } catch (AssertionError e) {
-      if (!e.getMessage().contains(completedError)) throw e;
-
-      mockMvc
-          .perform(
-              MockMvcRequestBuilders.post("/SqlInjection/attack9")
-                  .param("name", "Smithh")
-                  .param("auth_tan", "3SL99A"))
-          .andExpect(status().isOk())
-          .andExpect(jsonPath("lessonCompleted", is(true)))
-          .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.8.no.success"))))
-          .andExpect(jsonPath("$.output").doesNotExist());
-    }
-  }
-
-  @Test
-  public void wrongTANReturnsNoAccounts() throws Exception {
-    try {
-      mockMvc
-          .perform(
-              MockMvcRequestBuilders.post("/SqlInjection/attack9")
-                  .param("name", "Smithh")
-                  .param("auth_tan", ""))
-          .andExpect(status().isOk())
-          .andExpect(jsonPath("lessonCompleted", is(false)))
-          .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.8.no.results"))))
-          .andExpect(jsonPath("$.output").doesNotExist());
-    } catch (AssertionError e) {
-      if (!e.getMessage().contains(completedError)) throw e;
-
-      mockMvc
-          .perform(
-              MockMvcRequestBuilders.post("/SqlInjection/attack9")
-                  .param("name", "Smithh")
-                  .param("auth_tan", ""))
-          .andExpect(status().isOk())
-          .andExpect(jsonPath("lessonCompleted", is(true)))
-          .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.success"))))
-          .andExpect(jsonPath("$.output").doesNotExist());
-    }
-  }
 
   @Test
   public void malformedQueryReturnsError() throws Exception {
@@ -181,6 +65,44 @@ public class SqlInjectionLesson9Test extends SqlLessonTest {
     }
   }
 
+  @Test
+  public void SmithIsNotMostEarning() throws Exception {
+    mockMvc
+        .perform(
+            MockMvcRequestBuilders.post("/SqlInjection/attack9")
+                .param("name", "Smith")
+                .param(
+                    "auth_tan",
+                    "3SL99A'; UPDATE employees SET salary = 9999 WHERE last_name = 'Smith"))
+        .andExpect(status().isOk())
+        .andExpect(jsonPath("lessonCompleted", is(false)))
+        .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.one"))));
+  }
+
+  @Test
+  public void OnlySmithSalaryMustBeUpdated() throws Exception {
+    mockMvc
+        .perform(
+            MockMvcRequestBuilders.post("/SqlInjection/attack9")
+                .param("name", "Smith")
+                .param("auth_tan", "3SL99A'; UPDATE employees SET salary = 9999 -- "))
+        .andExpect(status().isOk())
+        .andExpect(jsonPath("lessonCompleted", is(false)))
+        .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.one"))));
+  }
+
+  @Test
+  public void OnlySmithMustMostEarning() throws Exception {
+    mockMvc
+        .perform(
+            MockMvcRequestBuilders.post("/SqlInjection/attack9")
+                .param("name", "'; UPDATE employees SET salary = 999999 -- ")
+                .param("auth_tan", ""))
+        .andExpect(status().isOk())
+        .andExpect(jsonPath("lessonCompleted", is(false)))
+        .andExpect(jsonPath("$.feedback", is(messages.getMessage("sql-injection.9.one"))));
+  }
+
   @Test
   public void SmithIsMostEarningCompletesAssignment() throws Exception {
     mockMvc