Wicket 9.x security fixes

wicket-core/src/main/java/org/apache/wicket/core/util/string/JavaScriptUtils.java

@@ -104,6 +104,26 @@ public static CharSequence escapeQuotes(final CharSequence input)
 		return s;
 	}
 
+	/**
+	 * Escape single and double quotes so that they can be part of e.g. an alert call.
+	 *
+	 * Note: JSON values need to escape only the double quote, so this method wont help.
+	 *
+	 * @param input
+	 *            the JavaScript which needs to be escaped
+	 * @return Escaped version of the input
+	 */
+	public static CharSequence escapeQuotesAndBackslash(final CharSequence input)
+	{
+		CharSequence s = input;
+		if (s != null)
+		{
+			s = Strings.replaceAll(s, "\\", "\\\\");
+			s = escapeQuotes(s);
+		}
+		return s;
+	}
+
 	/**
 	 * Write a reference to a javascript file to the response object
 	 * 

wicket-core/src/main/java/org/apache/wicket/markup/html/form/upload/resource/FolderUploadsFileManager.java

@@ -18,7 +18,10 @@
 
 import java.io.FileOutputStream;
 import java.io.IOException;
+import java.io.InputStream;
 import java.nio.file.Files;
+import java.nio.file.NoSuchFileException;
+import java.nio.file.Path;
 import org.apache.wicket.WicketRuntimeException;
 import org.apache.wicket.markup.html.form.upload.FileUpload;
 import org.apache.wicket.util.file.File;
@@ -60,14 +63,62 @@ public File getFolder() {
         return folder;
     }
 
+    /**
+     * Returns the canonical path for a directory for use in path traversal checks.
+     * Uses {@code toRealPath()} when the directory exists; falls back to
+     * {@code toAbsolutePath().normalize()} only when the directory does not yet exist
+     * (e.g. an upload sub-folder that will be created during {@link #save}).
+     * Other {@link IOException} subtypes (e.g. permission errors) are intentionally
+     * re-thrown so they are not silently swallowed.
+     */
+    private static Path getPathForComparison(java.io.File dir) throws IOException
+    {
+        try
+        {
+            return dir.toPath().toRealPath();
+        }
+        catch (NoSuchFileException e)
+        {
+            return dir.toPath().toAbsolutePath().normalize();
+        }
+    }
+
+    /**
+     * Validates {@code uploadFieldId} and {@code clientFileName} against the base folder to
+     * prevent path traversal, and returns the fully resolved, canonical target file.
+     *
+     * @throws SecurityException if either component would escape the base folder
+     */
+    private java.io.File resolveTargetFile(String uploadFieldId, String clientFileName)
+            throws IOException
+    {
+        Path baseFolderPath = getFolder().toPath().toRealPath();
+        java.io.File uploadFieldFolder = new File(getFolder(), uploadFieldId).getCanonicalFile();
+        if (!uploadFieldFolder.toPath().startsWith(baseFolderPath))
+        {
+            throw new SecurityException("Path traversal detected in uploadFieldId");
+        }
+        java.io.File target = new File(uploadFieldFolder, clientFileName).getCanonicalFile();
+        Path uploadFieldFolderPath = getPathForComparison(uploadFieldFolder);
+        if (!target.toPath().startsWith(uploadFieldFolderPath))
+        {
+            throw new SecurityException("Path traversal detected in client filename");
+        }
+        return target;
+    }
+
     @Override
     public void save(FileUpload fileItem, String uploadFieldId)
     {
-        File uploadFieldFolder = new File(getFolder(), uploadFieldId);
-        uploadFieldFolder.mkdirs();
         try
         {
-            IOUtils.copy(fileItem.getInputStream(),  new FileOutputStream(new File(uploadFieldFolder, fileItem.getClientFileName())));
+            java.io.File target = resolveTargetFile(uploadFieldId, fileItem.getClientFileName());
+            Files.createDirectories(target.toPath().getParent());
+            try (InputStream in = fileItem.getInputStream();
+                 FileOutputStream out = new FileOutputStream(target))
+            {
+                IOUtils.copy(in, out);
+            }
         }
         catch (IOException e)
         {
@@ -78,6 +129,13 @@ public void save(FileUpload fileItem, String uploadFieldId)
     @Override
     public File getFile(String uploadFieldId, String clientFileName)
     {
-        return new File(new File(getFolder(), uploadFieldId), clientFileName);
+        try
+        {
+            return new File(resolveTargetFile(uploadFieldId, clientFileName));
+        }
+        catch (IOException e)
+        {
+            throw new WicketRuntimeException(e);
+        }
     }
 }

wicket-core/src/main/java/org/apache/wicket/markup/html/link/ExternalLink.java

@@ -16,6 +16,7 @@
  */
 package org.apache.wicket.markup.html.link;
 
+import org.apache.wicket.core.util.string.JavaScriptUtils;
 import org.apache.wicket.markup.ComponentTag;
 import org.apache.wicket.markup.head.IHeaderResponse;
 import org.apache.wicket.markup.head.OnEventHeaderItem;
@@ -188,7 +189,7 @@ public void renderHead(IHeaderResponse response)
 		{
 			if (popupSettings != null)
 			{
-				popupSettings.setTarget("'" + url + "'");
+				popupSettings.setTarget(url);
 				response.render(OnEventHeaderItem.forComponent(this, "click",
 					popupSettings.getPopupJavaScript()));
 				return;
@@ -205,8 +206,9 @@ public void renderHead(IHeaderResponse response)
 				// in firefox when the element is quickly clicked 3 times a second request is
 				// generated during page load. This check ensures that the click is ignored
 				response.render(OnEventHeaderItem.forComponent(this, "click",
-					"var win = this.ownerDocument.defaultView || this.ownerDocument.parentWindow; "
-						+ "if (win == window) { window.location.href='" + url
+					"var win = this.ownerDocument.defaultView || this.ownerDocument.parentWindow; " //
+						+ "if (win == window) { window.location.href='" //
+						+ JavaScriptUtils.escapeQuotes(url) //
 						+ "'; } ;return false"));
 				return;
 			}

wicket-core/src/main/java/org/apache/wicket/markup/html/link/Link.java

@@ -407,7 +407,7 @@ public void renderHead(IHeaderResponse response)
 			// next check for popup settings
 			if (popupSettings != null)
 			{
-				popupSettings.setTarget("'" + url + "'");
+				popupSettings.setTarget(url.toString());
 				response.render(OnEventHeaderItem.forComponent(this, "click",
 					popupSettings.getPopupJavaScript()));
 				return;

wicket-core/src/main/java/org/apache/wicket/markup/html/link/PopupSettings.java

@@ -16,6 +16,7 @@
  */
 package org.apache.wicket.markup.html.link;
 
+import org.apache.wicket.core.util.string.JavaScriptUtils;
 import org.apache.wicket.util.io.IClusterable;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -155,8 +156,10 @@ public String getPopupJavaScript()
 			windowTitle = windowTitle.replaceAll("\\W", "_");
 		}
 
-	    StringBuilder script = new StringBuilder("var w = window.open(" + target + ", '").append(
-			windowTitle).append("', '");
+		StringBuilder script = new StringBuilder(//
+			"var w = window.open('"//
+				+ JavaScriptUtils.escapeQuotes(target) //
+				+ "', '").append(windowTitle).append("', '");
 
 		script.append("scrollbars=").append(flagToString(SCROLLBARS));
 		script.append(",location=").append(flagToString(LOCATION_BAR));

wicket-core/src/main/java/org/apache/wicket/request/resource/PackageResource.java

@@ -16,16 +16,6 @@
  */
 package org.apache.wicket.request.resource;
 
-import java.io.ByteArrayInputStream;
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.Serializable;
-import java.nio.charset.Charset;
-import java.nio.charset.StandardCharsets;
-import java.time.Instant;
-import java.util.Locale;
-import java.util.Objects;
-import javax.servlet.http.HttpServletResponse;
 import org.apache.wicket.Application;
 import org.apache.wicket.IWicketInternalException;
 import org.apache.wicket.Session;
@@ -45,14 +35,24 @@
 import org.apache.wicket.util.io.IOUtils;
 import org.apache.wicket.util.lang.Classes;
 import org.apache.wicket.util.lang.Packages;
-import org.apache.wicket.util.resource.IFixedLocationResourceStream;
 import org.apache.wicket.util.resource.IResourceStream;
 import org.apache.wicket.util.resource.ResourceStreamNotFoundException;
 import org.apache.wicket.util.resource.ResourceStreamWrapper;
 import org.apache.wicket.util.string.Strings;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import javax.servlet.http.HttpServletResponse;
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.Serializable;
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
+import java.time.Instant;
+import java.util.Locale;
+import java.util.Objects;
+
 /**
  * Represents a localizable static resource.
  * <p>
@@ -555,38 +555,18 @@ public void setCompress(boolean compress)
 
 	private IResourceStream internalGetResourceStream(final String style, final Locale locale)
 	{
+		if (!accept(absolutePath))
+		{
+			throw new PackageResourceBlockedException(
+				"Access denied to (static) package resource " + absolutePath + ". See IPackageResourceGuard");
+		}
+
 		IResourceStreamLocator resourceStreamLocator = Application.get()
 			.getResourceSettings()
 			.getResourceStreamLocator();
 		IResourceStream resourceStream = resourceStreamLocator.locate(getScope(), absolutePath,
 			style, variation, locale, null, false);
 
-		String realPath = absolutePath;
-		if (resourceStream instanceof IFixedLocationResourceStream)
-		{
-			realPath = ((IFixedLocationResourceStream)resourceStream).locationAsString();
-			if (realPath != null)
-			{
-				int index = realPath.indexOf(absolutePath);
-				if (index != -1)
-				{
-					realPath = realPath.substring(index);
-				}
-			}
-			else
-			{
-				realPath = absolutePath;
-			}
-
-		}
-
-		if (accept(realPath) == false)
-		{
-			throw new PackageResourceBlockedException(
-				"Access denied to (static) package resource " + absolutePath +
-					". See IPackageResourceGuard");
-		}
-
 		if (resourceStream != null)
 		{
 			resourceStream = new ProcessingResourceStream(resourceStream);

wicket-core/src/test/java/org/apache/wicket/core/util/string/JavaScriptUtilsTest.java

@@ -16,10 +16,12 @@
  */
 package org.apache.wicket.core.util.string;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import org.apache.wicket.response.StringResponse;
 import org.apache.wicket.util.value.AttributeMap;
+import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.Test;
 
 /**
@@ -97,4 +99,9 @@ public void scriptTag()
 			JavaScriptUtils.SCRIPT_OPEN_TAG);
 		assertEquals("\n/*]]>*/\n</script>\n", JavaScriptUtils.SCRIPT_CLOSE_TAG);
 	}
+
+	@Test
+	void escapeQuotesAndBackslash(){
+		assertThat(JavaScriptUtils.escapeQuotesAndBackslash("alert('foo\\tbar')")).isEqualTo("alert(\\'foo\\\\tbar\\')");
+	}
 }

wicket-core/src/test/java/org/apache/wicket/markup/html/PackageResourceTest.java

@@ -16,16 +16,11 @@
  */
 package org.apache.wicket.markup.html;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertNotNull;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
-import java.util.Locale;
-
 import org.apache.wicket.Application;
 import org.apache.wicket.SharedResources;
+import org.apache.wicket.markup.html.snake_case.TestPageInsideSnakeCasePackage;
 import org.apache.wicket.protocol.http.WebApplication;
+import org.apache.wicket.protocol.https.HttpPage;
 import org.apache.wicket.request.resource.JavaScriptPackageResource;
 import org.apache.wicket.request.resource.PackageResource;
 import org.apache.wicket.request.resource.PackageResourceReference;
@@ -35,6 +30,12 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
+import java.util.Locale;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.junit.jupiter.api.Assertions.*;
+
 /**
  * Tests for package resources.
  * 
@@ -193,4 +194,59 @@ void javascriptFileWithEncoding()
 		final String contentType = tester.getLastResponse().getContentType();
 		assertEquals("text/javascript; charset=" + encoding, contentType);
 	}
+
+	@Test
+	void getResourceStream()
+	{
+		PackageResource resource = new PackageResourceReference(PackageResourceTest.class,
+			"packaged1.txt").getResource();
+		assertThat(resource.getResourceStream()).isNotNull();
+	}
+
+	@Test
+	void dontGetResourceStream()
+	{
+		PackageResource resource = new PackageResourceReference(HttpPage.class,
+			"HttpPage.html").getResource();
+		assertThatThrownBy(resource::getResourceStream).isInstanceOf(
+			PackageResource.PackageResourceBlockedException.class);
+	}
+
+	@Test
+	void dontGetResourceStreamIfNameHasSuffix()
+	{
+		PackageResource resource = new PackageResourceReference(HttpPage.class,
+			"HttpPage_en.html").getResource();
+		assertThatThrownBy(resource::getResourceStream).isInstanceOf(
+			PackageResource.PackageResourceBlockedException.class);
+	}
+
+	@Test
+	void getResourceStreamInSnakeCasePackage()
+	{
+		PackageResource resource = new PackageResourceReference(
+			TestPageInsideSnakeCasePackage.class, "style.css").getResource();
+		assertThat(resource.getResourceStream()).isNotNull();
+	}
+
+	@Test
+	void dontGetResourceStreamInSnakeCasePackage()
+	{
+		PackageResource resource = new PackageResourceReference(
+			TestPageInsideSnakeCasePackage.class,
+			"TestPageInsideSnakeCasePackage.html").getResource();
+		assertThatThrownBy(resource::getResourceStream).isInstanceOf(
+			PackageResource.PackageResourceBlockedException.class);
+	}
+
+	@Test
+	void dontGetResourceStreamInSnakeCasePackageIfNameHasSuffix()
+	{
+		PackageResource resource = new PackageResourceReference(
+			TestPageInsideSnakeCasePackage.class,
+			"TestPageInsideSnakeCasePackage_en.html").getResource();
+		assertThatThrownBy(resource::getResourceStream).isInstanceOf(
+			PackageResource.PackageResourceBlockedException.class);
+	}
+
 }