Update recommendation in qldoc and make examples more comprehendible

Author: luchua-bc

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java

@@ -1,55 +1,11 @@
-public class UnsafeServletRequestDispatch extends HttpServlet {
-	private static final String BASE_PATH = "/pages";
-
-	@Override
-	protected void doGet(HttpServletRequest request, HttpServletResponse response)
-			throws ServletException, IOException {
-		{
-			ServletConfig cfg = getServletConfig();
-			ServletContext sc = cfg.getServletContext();
-
-			// GOOD: check for an explicitly permitted URI
-			String action = request.getParameter("action");
-			if (action.equals("Login")) {
-				RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp");
-				rd.forward(request, response);
-			}
-
-			// BAD: no URI validation
-			String returnURL = request.getParameter("returnURL");
-			RequestDispatcher rd = sc.getRequestDispatcher(returnURL);
-			rd.forward(request, response);
-
-			// A sample payload "/pages/welcome.jsp/../WEB-INF/web.xml" can bypass the `startsWith` check
-			// The payload "/pages/welcome.jsp/../../%57EB-INF/web.xml" can bypass the check as well since RequestDispatcher will decode `%57` as `W`
-			String path = request.getParameter("path");
-
-			// BAD: no check for path traversal
-			if (path.startsWith(BASE_PATH)) {
-				request.getServletContext().getRequestDispatcher(path).include(request, response);
-			}
-
-			// GOOD: To check there won't be unexpected path traversal, we can check for any `..` sequences and whether the URI starts with a given web root path.
-			if (path.startsWith(BASE_PATH) && !path.contains("..")) {
-				request.getServletContext().getRequestDispatcher(path).include(request, response);
-			}
-
-			// GOOD: Or alternatively we can use Path.normalize and check whether the URI starts with a given web root path.
-			Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize();
-			if (requestedPath.startsWith(BASE_PATH)) {
-				request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response);
-			}
-
-			// GOOD: Or alternatively ensure URL encoding is removed and then check for any `..` sequences.
-			boolean hasEncoding = path.contains("%");
-			while (hasEncoding) {
-				path = URLDecoder.decode(path, "UTF-8");
-				hasEncoding = path.contains("%");
-			}
-
-			if (!path.startsWith("/WEB-INF/") && !path.contains("..")) {
-				request.getServletContext().getRequestDispatcher(path).include(request, response);
-			}
-		}
-	}
-}
+// BAD: no URI validation
+String returnURL = request.getParameter("returnURL");
+RequestDispatcher rd = sc.getRequestDispatcher(returnURL);
+rd.forward(request, response);
+
+// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix:
+// (alternatively use `Path.normalize` instead of checking for `..`)
+if (!returnURL.contains("..") && returnURL.hasPrefix("/pages")) { ... }
+// Also GOOD: check for a forbidden prefix, ensuring URL-encoding is not used to evade the check:
+// (alternatively use `URLDecoder.decode` before `hasPrefix`)
+if (returnURL.hasPrefix("/internal") && !returnURL.contains("%")) { ... }

java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java

@@ -13,9 +13,9 @@
 
 <p>Unsanitized user provided data must not be used to construct the path for URL forwarding. In order to prevent
 untrusted URL forwarding, it is recommended to avoid concatenating user input directly into the forwarding URL.
-Instead, user input should be checked for path traversal using <code>..</code> sequences or the user input should
-be normalized using <code>Path.normalize</code>, any URL encoding should be removed, and user input should be
-checked against a permitted URI.
+Instead, user input should be checked against allowed (e.g., must come within <code>user_content/</code>) or disallowed
+(e.g. must not come within <code>/internal</code>) paths, ensuring that neither path traversal using <code>../</code>
+or URL encoding are used to evade these checks.
 </p>
 
 </recommendation>