XSS lesson completion fixes (WebGoat#669)

* XSS lesson completion fixes

* removed log all

* lesson progress capable of deprecated assignments in the database

* fixed unit test for lesson progress

webgoat-container/src/main/java/org/owasp/webgoat/lessons/Assignment.java

@@ -45,6 +45,7 @@ public class Assignment {
     private Long id;
     private String name;
     private String path;
+
     @Transient
     private List<String> hints;
 
@@ -64,4 +65,14 @@ public Assignment(String name, String path, List<String> hints) {
         this.path = path;
         this.hints = hints;
     }
+
+    /**
+     * Set path is here to overwrite stored paths.
+     * Since a stored path can no longer be used in a lesson while
+     * the lesson (name) itself is still part of the lesson.
+     * @param pathName
+     */
+    public void setPath(String pathName) {
+    	this.path = pathName;
+    }
 }

webgoat-container/src/main/java/org/owasp/webgoat/service/LessonMenuService.java

@@ -30,6 +30,7 @@
 
 import lombok.AllArgsConstructor;
 import org.owasp.webgoat.lessons.Lesson;
+import org.owasp.webgoat.lessons.Assignment;
 import org.owasp.webgoat.lessons.Category;
 import org.owasp.webgoat.lessons.LessonMenuItem;
 import org.owasp.webgoat.lessons.LessonMenuItemType;
@@ -45,6 +46,7 @@
 import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.List;
+import java.util.Map;
 import java.util.stream.Collectors;
 
 /**
@@ -88,7 +90,8 @@ List<LessonMenuItem> showLeftNav() {
                 lessonItem.setLink(lesson.getLink());
                 lessonItem.setType(LessonMenuItemType.LESSON);
                 LessonTracker lessonTracker = userTracker.getLessonTracker(lesson);
-                lessonItem.setComplete(lessonTracker.isLessonSolved());
+                boolean lessonSolved = lessonCompleted(lessonTracker.getLessonOverview(), lesson);
+                lessonItem.setComplete(lessonSolved);
                 categoryItem.addChild(lessonItem);
             }
             categoryItem.getChildren().sort((o1, o2) -> o1.getRanking() - o2.getRanking());
@@ -97,4 +100,27 @@ List<LessonMenuItem> showLeftNav() {
         return menu;
 
     }
+
+    /**
+     * This determines if the lesson is complete based on data in the database 
+     * and the list of assignments actually linked to the existing current lesson.
+     * This way older removed assignments will not prevent a lesson from being completed.
+     * @param map
+     * @param currentLesson
+     * @return
+     */
+    private boolean lessonCompleted(Map<Assignment, Boolean> map, Lesson currentLesson) {
+        boolean result = true;
+        for (Map.Entry<Assignment, Boolean> entry : map.entrySet()) {
+        	    Assignment storedAssignment = entry.getKey();
+            	for (Assignment lessonAssignment: currentLesson.getAssignments()) {
+            		if (lessonAssignment.getName().equals(storedAssignment.getName())) {
+            			result = result && entry.getValue();
+            			break;
+            		}
+            	}
+
+        }
+        return result;
+    }
 }

webgoat-container/src/main/java/org/owasp/webgoat/service/LessonProgressService.java

@@ -47,7 +47,7 @@ public Map getLessonInfo() {
             String successMessage = "";
             boolean lessonCompleted = false;
             if (lessonTracker != null) {
-                lessonCompleted = lessonTracker.isLessonSolved();
+                lessonCompleted = lessonCompleted(lessonTracker.getLessonOverview(),webSession.getCurrentLesson());
                 successMessage = "LessonCompleted"; //@todo we still use this??
             }
             json.put("lessonCompleted", lessonCompleted);
@@ -70,19 +70,56 @@ public List<LessonOverview> lessonOverview() {
         List<LessonOverview> result = Lists.newArrayList();
         if ( currentLesson != null ) {
             LessonTracker lessonTracker = userTracker.getLessonTracker(currentLesson);
-            result = toJson(lessonTracker.getLessonOverview());
+            result = toJson(lessonTracker.getLessonOverview(), currentLesson);
         }
         return result;
     }
 
-    private List<LessonOverview> toJson(Map<Assignment, Boolean> map) {
+    private List<LessonOverview> toJson(Map<Assignment, Boolean> map, Lesson currentLesson) {
         List<LessonOverview> result = new ArrayList();
         for (Map.Entry<Assignment, Boolean> entry : map.entrySet()) {
-            result.add(new LessonOverview(entry.getKey(), entry.getValue()));
+        	    Assignment storedAssignment = entry.getKey();
+            	for (Assignment lessonAssignment: currentLesson.getAssignments()) {
+            		if (lessonAssignment.getName().equals(storedAssignment.getName())
+            				&& !lessonAssignment.getPath().equals(storedAssignment.getPath())) {
+            			//here a stored path in the assignments table will be corrected for the JSON output
+            			//with the value of the actual expected path
+            			storedAssignment.setPath(lessonAssignment.getPath());
+            			result.add(new LessonOverview(storedAssignment, entry.getValue()));
+            			break;
+
+            		} else if (lessonAssignment.getName().equals(storedAssignment.getName())) {
+            			result.add(new LessonOverview(storedAssignment, entry.getValue()));
+            			break;
+            		}
+            	}
+            	//assignments not in the list will not be put in the lesson progress JSON output
+
         }
         return result;
     }
 
+    /**
+     * Get the lesson completed based on Assignment data from the database
+     * while ignoring assignments no longer in the application.
+     * @param map
+     * @param currentLesson
+     * @return
+     */
+    private boolean lessonCompleted(Map<Assignment, Boolean> map, Lesson currentLesson) {
+        boolean result = true;
+        for (Map.Entry<Assignment, Boolean> entry : map.entrySet()) {
+        	    Assignment storedAssignment = entry.getKey();
+            	for (Assignment lessonAssignment: currentLesson.getAssignments()) {
+            		if (lessonAssignment.getName().equals(storedAssignment.getName())) {
+            			result = result && entry.getValue();
+            			break;
+            		}
+            	}
+
+        }
+        return result;
+    }
 
     @AllArgsConstructor
     @Getter

webgoat-container/src/main/java/org/owasp/webgoat/session/WebSession.java

@@ -1,10 +1,10 @@
 package org.owasp.webgoat.session;
 
-import lombok.extern.slf4j.Slf4j;
 import org.owasp.webgoat.lessons.Lesson;
 import org.owasp.webgoat.users.WebGoatUser;
 import org.springframework.security.core.context.SecurityContextHolder;
 
+import java.io.Serializable;
 import java.sql.Connection;
 import java.sql.SQLException;
 
@@ -37,10 +37,10 @@
  * @version $Id: $Id
  * @since October 28, 2003
  */
-@Slf4j
-public class WebSession {
+public class WebSession implements Serializable {
 
-    private final WebGoatUser currentUser;
+	private static final long serialVersionUID = -4270066103101711560L;
+	private final WebGoatUser currentUser;
     private final WebgoatContext webgoatContext;
     private Lesson currentLesson;
 

webgoat-container/src/test/java/org/owasp/webgoat/service/LessonMenuServiceTest.java

@@ -50,7 +50,7 @@
 @RunWith(MockitoJUnitRunner.class)
 public class LessonMenuServiceTest {
 
-    @Mock
+    @Mock(lenient=true)
     private LessonTracker lessonTracker;
     @Mock
     private Course course;

webgoat-container/src/test/java/org/owasp/webgoat/service/LessonProgressServiceTest.java

@@ -76,6 +76,7 @@ public void setup() {
         when(userTrackerRepository.findByUser(any())).thenReturn(userTracker);
         when(userTracker.getLessonTracker(any(Lesson.class))).thenReturn(lessonTracker);
         when(websession.getCurrentLesson()).thenReturn(lesson);
+        when(lesson.getAssignments()).thenReturn(List.of(assignment));
         when(lessonTracker.getLessonOverview()).thenReturn(Maps.newHashMap(assignment, true));
         this.mockMvc = MockMvcBuilders.standaloneSetup(new LessonProgressService(userTrackerRepository, websession)).build();
     }

webgoat-integration-tests/src/test/java/org/owasp/webgoat/GeneralLessonTest.java

@@ -123,4 +123,20 @@ public void chrome() {
 
         checkResults("/ChromeDevTools/");
     }
+
+    @Test
+    public void authByPass() {
+        startLesson("AuthBypass");
+        Map<String, Object> params = new HashMap<>();
+        params.clear();
+        params.put("secQuestion2", "John");
+        params.put("secQuestion3", "Main");
+        params.put("jsEnabled", "1");
+        params.put("verifyMethod", "SEC_QUESTIONS");
+        params.put("userId", "12309746");
+        checkAssignment(url("/auth-bypass/verify-account"), params, true);
+        checkResults("/auth-bypass/");
+
+    }
+
 }

webgoat-integration-tests/src/test/java/org/owasp/webgoat/IntegrationTest.java

@@ -211,7 +211,7 @@ public void checkResults(String prefix) {
                 .config(restConfig)
                 .cookie("JSESSIONID", getWebGoatCookie())
                 .get(url("service/lessonoverview.mvc"))
-                .then()                
+                .then()     
                 .statusCode(200).extract().jsonPath().getList("solved"), CoreMatchers.everyItem(CoreMatchers.is(true)));
 
         Assert.assertThat(RestAssured.given()
@@ -238,5 +238,18 @@ public void checkAssignment(String url, ContentType contentType, String body, bo
                         .extract().path("lessonCompleted"), CoreMatchers.is(expectedResult));
     }
 
+    public void checkAssignmentWithGet(String url, Map<String, ?> params, boolean expectedResult) {
+        Assert.assertThat(
+                RestAssured.given()
+                        .when()
+                        .config(restConfig)
+                        .cookie("JSESSIONID", getWebGoatCookie())   
+                        .queryParams(params)
+                        .get(url)                        
+                        .then()
+                        .statusCode(200)
+                        .extract().path("lessonCompleted"), CoreMatchers.is(expectedResult));
+    }
+
 }
 

webgoat-integration-tests/src/test/java/org/owasp/webgoat/XSSTest.java

@@ -0,0 +1,68 @@
+package org.owasp.webgoat;
+
+import org.junit.Test;
+
+import io.restassured.RestAssured;
+
+import java.util.HashMap;
+import java.util.Map;
+
+public class XSSTest extends IntegrationTest {
+
+
+    @Test
+    public void crossSiteScriptingAssignments() {
+        startLesson("CrossSiteScripting");
+
+        Map<String, Object> params = new HashMap<>();
+        params.clear();
+        params.put("answer_xss_1", "yes");
+        checkAssignment(url("/CrossSiteScripting/attack1"), params, true);
+
+        params.clear();
+        params.put("QTY1", "1");
+        params.put("QTY2", "1");
+        params.put("QTY3", "1");
+        params.put("QTY4", "1");
+        params.put("field1", "<script>alert('XSS+Test')</script>");
+        params.put("field2", "111");
+        checkAssignmentWithGet(url("/CrossSiteScripting/attack5a"), params, true);
+
+        params.clear();
+        params.put("DOMTestRoute", "start.mvc#test");
+        checkAssignment(url("/CrossSiteScripting/attack6a"), params, true);
+
+        params.clear();
+        params.put("param1", "42");
+        params.put("param2", "24");
+
+        String result =
+                RestAssured.given()
+                        .when()
+                        .config(restConfig)
+                        .cookie("JSESSIONID", getWebGoatCookie())
+                        .header("webgoat-requested-by", "dom-xss-vuln")
+                        .header("X-Requested-With", "XMLHttpRequest")
+                        .formParams(params)
+                        .post(url("/CrossSiteScripting/phone-home-xss"))
+                        .then()
+                        .statusCode(200)
+                        .extract().path("output");
+        String secretNumber = result.substring("phoneHome Response is ".length());
+
+        params.clear();
+        params.put("successMessage", secretNumber);
+        checkAssignment(url("/CrossSiteScripting/dom-follow-up"), params, true);
+
+        params.clear();
+        params.put("question_0_solution", "Solution 4: No because the browser trusts the website if it is acknowledged trusted, then the browser does not know that the script is malicious.");
+        params.put("question_1_solution", "Solution 3: The data is included in dynamic content that is sent to a web user without being validated for malicious content.");
+        params.put("question_2_solution", "Solution 1: The script is permanently stored on the server and the victim gets the malicious script when requesting information from the server.");
+        params.put("question_3_solution", "Solution 2: They reflect the injected script off the web server. That occurs when input sent to the web server is part of the request.");
+        params.put("question_4_solution", "Solution 4: No there are many other ways. Like HTML, Flash or any other type of code that the browser executes.");
+        checkAssignment(url("/CrossSiteScripting/quiz"), params, true);
+
+        checkResults("/CrossSiteScripting/");
+
+    }
+}

webgoat-lessons/cross-site-scripting/src/main/java/org/owasp/webgoat/xss/CrossSiteScriptingLesson3.java

@@ -27,15 +27,18 @@
 import org.jsoup.nodes.Document;
 import org.owasp.webgoat.assignments.AssignmentEndpoint;
 import org.owasp.webgoat.assignments.AssignmentHints;
-import org.owasp.webgoat.assignments.AssignmentPath;
 import org.owasp.webgoat.assignments.AttackResult;
 import org.springframework.web.bind.annotation.*;
 
-@RestController
+//@RestController
+@Deprecated
+//TODO This assignment seems not to be in use in the UI
+// it is there to make sure the lesson can be marked complete
+// in order to restore it, make it accessible through the UI and uncomment RestController
 @AssignmentHints(value = {"xss-mitigation-3-hint1", "xss-mitigation-3-hint2", "xss-mitigation-3-hint3", "xss-mitigation-3-hint4"})
 public class CrossSiteScriptingLesson3 extends AssignmentEndpoint {
 
-    @PostMapping("CrossSiteScripting/attack3")
+    @PostMapping("/CrossSiteScripting/attack3")
     @ResponseBody
     public AttackResult completed(@RequestParam String editor) {
         String unescapedString = org.jsoup.parser.Parser.unescapeEntities(editor, true);

webgoat-lessons/cross-site-scripting/src/main/java/org/owasp/webgoat/xss/CrossSiteScriptingLesson4.java

@@ -24,39 +24,37 @@
 
 import org.owasp.webgoat.assignments.AssignmentEndpoint;
 import org.owasp.webgoat.assignments.AssignmentHints;
-import org.owasp.webgoat.assignments.AssignmentPath;
 import org.owasp.webgoat.assignments.AttackResult;
 import org.springframework.web.bind.annotation.*;
 
-import javax.tools.*;
-import java.io.IOException;
-import java.net.URI;
-import java.util.Arrays;
-import java.util.List;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
+import lombok.extern.slf4j.Slf4j;
 
-@RestController
+//@RestController
+@Deprecated
+//TODO This assignment seems not to be in use in the UI
+//it is there to make sure the lesson can be marked complete
+//in order to restore it, make it accessible through the UI and uncomment RestController@Slf4j
+@Slf4j
 @AssignmentHints(value = {"xss-mitigation-4-hint1"})
 public class CrossSiteScriptingLesson4 extends AssignmentEndpoint {
 
-    @PostMapping("CrossSiteScripting/attack4")
+    @PostMapping("/CrossSiteScripting/attack4")
     @ResponseBody
     public AttackResult completed(@RequestParam String editor2) {
 
         String editor = editor2.replaceAll("\\<.*?>", "");
-        System.out.println(editor);
+        log.debug(editor);
 
         if ((editor.contains("Policy.getInstance(\"antisamy-slashdot.xml\"") || editor.contains(".scan(newComment, \"antisamy-slashdot.xml\"") || editor.contains(".scan(newComment, new File(\"antisamy-slashdot.xml\")")) &&
                 editor.contains("new AntiSamy();") &&
                 editor.contains(".scan(newComment,") &&
                 editor.contains("CleanResults") &&
                 editor.contains("MyCommentDAO.addComment(threadID, userID") &&
                 editor.contains(".getCleanHTML());")) {
-            System.out.println("true");
+            log.debug("true");
             return trackProgress(success().feedback("xss-mitigation-4-success").build());
         } else {
-            System.out.println("false");
+            log.debug("false");
             return trackProgress(failed().feedback("xss-mitigation-4-failed").build());
         }
     }

webgoat-lessons/cross-site-scripting/src/main/java/org/owasp/webgoat/xss/CrossSiteScriptingLesson5a.java

@@ -25,15 +25,11 @@
 
 import org.owasp.webgoat.assignments.AssignmentEndpoint;
 import org.owasp.webgoat.assignments.AssignmentHints;
-import org.owasp.webgoat.assignments.AssignmentPath;
 import org.owasp.webgoat.assignments.AttackResult;
 import org.owasp.webgoat.session.UserSessionData;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.web.bind.annotation.*;
 
-import javax.servlet.http.HttpServletRequest;
-import java.io.IOException;
-
 
 @RestController
 @AssignmentHints(value = {"xss-reflected-5a-hint-1", "xss-reflected-5a-hint-2", "xss-reflected-5a-hint-3", "xss-reflected-5a-hint-4"})