Fix Cross Site Scripting 4 (Challenge-38): stop re-decoding quotes in XssFilter.encodeForHtml

src/main/java/servlets/module/challenge/XssChallengeFour.java

@@ -1,6 +1,5 @@
 package servlets.module.challenge;
 
-import dbProcs.Getter;
 import java.io.IOException;
 import java.io.PrintWriter;
 import java.util.Locale;
@@ -13,8 +12,6 @@
 import javax.servlet.http.HttpSession;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import utils.FindXSS;
-import utils.Hash;
 import utils.ShepherdLogManager;
 import utils.Validate;
 import utils.XssFilter;
@@ -41,8 +38,6 @@ public class XssChallengeFour extends HttpServlet {
 
   private static final long serialVersionUID = 1L;
   private static final Logger log = LogManager.getLogger(XssChallengeFour.class);
-  private static final String levelHash =
-      "06f81ca93f26236112f8e31f32939bd496ffe8c9f7b564bce32bd5e3a8c2f751";
   private static String levelName = "XSS Challenge 4";
 
   /**
@@ -88,27 +83,10 @@ public void doPost(HttpServletRequest request, HttpServletResponse response)
                     + searchTerm
                     + "</a>";
           } else {
-
             searchTerm = XssFilter.encodeForHtml(searchTerm);
             userPost =
                 "<a href=\"" + searchTerm + "\" alt=\"" + searchTerm + "\">" + searchTerm + "</a>";
             log.debug("After Encoding - " + searchTerm);
-            if (FindXSS.search(userPost)) {
-              htmlOutput =
-                  "<h2 class='title'>"
-                      + bundle.getString("result.wellDone")
-                      + "</h2>"
-                      + "<p>"
-                      + bundle.getString("result.youDidIt")
-                      + "<br />"
-                      + bundle.getString("result.resultKey")
-                      + " <a>"
-                      + Hash.generateUserSolution(
-                          Getter.getModuleResultFromHash(
-                              getServletContext().getRealPath(""), levelHash),
-                          (String) ses.getAttribute("userName"))
-                      + "</a>";
-            }
           }
           log.debug("Adding searchTerm to Html: " + searchTerm);
           htmlOutput +=

src/main/java/utils/XssFilter.java

@@ -100,10 +100,12 @@ public static String encodeForHtml(String input) {
     log.debug("Filtering input at XSS white list");
 
     input = Encode.forHtml(input);
-    // Decode quotes to open a security hole in Encoder
-    input = input.replaceFirst(""", "\"");
-    // Encode lower-case "on" and upper-case "on" to complicate the required attack vectors to pass
-    return input.replaceAll("on", "on").replaceAll("ON", "ON");
+    // Quotes are intentionally left HTML-encoded here (no longer decoded back to a raw
+    // double-quote) so that reflecting this value inside an HTML attribute (e.g. href="...")
+    // can no longer be used to break out of the attribute and inject new attributes/handlers.
+    // Encode every case-variant of "on" (on/On/oN/ON) as defence-in-depth against inline event
+    // handlers, since HTML attribute names are matched case-insensitively by browsers.
+    return input.replaceAll("(?i)on", "on");
   }
 
   /**

src/test/java/servlets/module/challenge/XssChallengeFourVulnerabilityTest.java

@@ -0,0 +1,76 @@
+package servlets.module.challenge;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.junit.jupiter.api.Test;
+import utils.FindXSS;
+import utils.XssFilter;
+
+/**
+ * Reproduces the exact HTML construction that {@link XssChallengeFour#doPost} performs on a
+ * user-submitted "http..." searchTerm (see the else-branch that builds `userPost`), without
+ * requiring a live servlet container. This lets us prove, at the feature level, that:
+ *
+ * <ol>
+ *   <li>The original attribute-breakout XSS payload no longer produces exploitable markup.
+ *   <li>A normal http(s) URL submission still renders the expected safe anchor tag.
+ * </ol>
+ */
+class XssChallengeFourVulnerabilityTest {
+
+  /** Mirrors XssChallengeFour.doPost's userPost construction for a "http..." searchTerm. */
+  private static String buildUserPost(String rawSearchTerm) {
+    String encoded = XssFilter.encodeForHtml(rawSearchTerm);
+    return "<a href=\"" + encoded + "\" alt=\"" + encoded + "\">" + encoded + "</a>";
+  }
+
+  @Test
+  void quoteBreakoutAttributeInjectionPayload_noLongerEscapesTheHrefAttribute() {
+    // Classic reflected-XSS payload for this challenge: starts with "http" (passes the
+    // startsWith check) then breaks out of the href="" attribute to add a new event handler.
+    String payload = "http://evil.example\" onmouseover=\"alert(1)";
+
+    String userPost = buildUserPost(payload);
+
+    // The previous bug decoded the FIRST &#34; back to a raw double-quote, letting the payload
+    // close the href attribute early and open a new onmouseover="..." attribute. After the fix,
+    // every quote must remain HTML-encoded, so the <a> tag's only two literal quotes are the
+    // ones the servlet itself writes around href="" and alt="".
+    long literalQuoteCount = userPost.chars().filter(c -> c == '"').count();
+    assertEquals(
+        4,
+        literalQuoteCount,
+        "only the servlet's own href=\"\" and alt=\"\" quotes should remain literal: " + userPost);
+    assertFalse(
+        userPost.contains("\" onmouseover=\""), "attribute breakout succeeded: " + userPost);
+
+    // FindXSS.search() is the codebase's own generic "is this exploitable markup" detector -
+    // it must no longer fire on this payload either.
+    assertFalse(FindXSS.search(userPost), "payload still counts as successful XSS: " + userPost);
+  }
+
+  @Test
+  void mixedCaseEventHandler_isStillNeutralisedAfterQuoteFixAlone() {
+    // Defence-in-depth: even if an attacker also tries to dodge the on/ON scrambling with mixed
+    // case (browsers match attribute names case-insensitively), the handler text must still be
+    // encoded so it can never form a live "onmouseover"/"oNMouseOver" attribute name.
+    String payload = "http://evil.example\" oNMouseOver=\"alert(1)";
+
+    String userPost = buildUserPost(payload);
+
+    assertFalse(userPost.toLowerCase().contains("onmouseover"));
+    assertFalse(FindXSS.search(userPost));
+  }
+
+  @Test
+  void legitimateHttpUrl_stillRendersAWorkingAnchorTag() {
+    String url = "https://www.example.com/path?query=1";
+
+    String userPost = buildUserPost(url);
+
+    assertEquals("<a href=\"" + url + "\" alt=\"" + url + "\">" + url + "</a>", userPost);
+    assertTrue(userPost.startsWith("<a href=\"https://www.example.com/path?query=1\" alt=\""));
+  }
+}

src/test/java/utils/XssFilterTest.java

@@ -90,9 +90,18 @@ void encodeForHtml_encodesAngleBrackets() {
   }
 
   @Test
-  void encodeForHtml_restoresFirstQuote() {
+  void encodeForHtml_keepsQuotesEncoded() {
+    // Quotes must stay HTML-encoded so a value reflected into an HTML attribute (e.g.
+    // href="...") cannot break out of the attribute and inject new attributes/handlers.
     String result = XssFilter.encodeForHtml("\"test\"");
-    assertTrue(result.startsWith("\""));
+    assertFalse(result.contains("\""));
+    assertTrue(result.startsWith(""") || result.startsWith("""));
+  }
+
+  @Test
+  void encodeForHtml_encodesMixedCaseOnHandler() {
+    String result = XssFilter.encodeForHtml("oNmouseover");
+    assertFalse(result.toLowerCase().contains("on"));
   }
 
   @Test