SPR-6291 - UrlPathHelper is too aggressive decoding URLs

org.springframework.web.servlet/src/main/java/org/springframework/web/servlet/tags/UrlTag.java

@@ -33,6 +33,7 @@ import org.springframework.web.util.ExpressionEvaluationUtils;
 import org.springframework.web.util.HtmlUtils;
 import org.springframework.web.util.JavaScriptUtils;
 import org.springframework.web.util.TagUtils;
+import org.springframework.web.util.UriUtils;
 
 /**
  * JSP tag for creating URLs. Modeled after the JSTL c:url tag with backwards
@@ -296,41 +297,13 @@ public class UrlTag extends HtmlEscapingAwareTag implements ParamAware {
 		}
 		try {
 			String encoding = pageContext.getResponse().getCharacterEncoding();
-			String formUrlEncodedValue = URLEncoder.encode(value, encoding);
-			if (!formUrlEncodedValue.contains("+")) {
-				return formUrlEncodedValue;
-			}
-			String spaceEncoding = this.urlEncode(' ', encoding);
-			return formUrlEncodedValue.replace("+", spaceEncoding);
+			return UriUtils.encode(value, encoding);
 		}
 		catch (UnsupportedEncodingException ex) {
 			throw new JspException(ex);
 		}
 	}
 
-	/*
-	 * based on URLCodec from Apache Commons Codec
-	 */
-	protected String urlEncode(Character c, String enc) throws UnsupportedEncodingException {
-		if (c == null) {
-			return null;
-		}
-		byte[] bytes = c.toString().getBytes(enc);
-		StringBuilder builder = new StringBuilder();
-		for (int i = 0; i < bytes.length; i++) {
-			int b = bytes[i];
-			if (b < 0) {
-				b = 256 + b;
-			}
-			char hex1 = Character.toUpperCase(Character.forDigit((b >> 4) & 0xF, 16));
-			char hex2 = Character.toUpperCase(Character.forDigit(b & 0xF, 16));
-			builder.append('%');
-			builder.append(hex1);
-			builder.append(hex2);
-		}
-		return builder.toString();
-	}
-
 	/**
 	 * Internal enum that classifies URLs by type.
 	 */

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/handler/PathMatchingUrlHandlerMappingTests.java

@@ -26,11 +26,15 @@ import org.springframework.web.servlet.HandlerExecutionChain;
 import org.springframework.web.servlet.HandlerInterceptor;
 import org.springframework.web.servlet.HandlerMapping;
 
+import org.junit.Before;
+import org.junit.Test;
+import static org.junit.Assert.*;
+
 /**
  * @author Alef Arendsen
  * @author Juergen Hoeller
  */
-public class PathMatchingUrlHandlerMappingTests extends TestCase {
+public class PathMatchingUrlHandlerMappingTests {
 
 	public static final String CONF = "/org/springframework/web/servlet/handler/map3.xml";
 
@@ -38,6 +42,7 @@ public class PathMatchingUrlHandlerMappingTests extends TestCase {
 
 	private ConfigurableWebApplicationContext wac;
 
+	@Before
 	public void setUp() throws Exception {
 		MockServletContext sc = new MockServletContext("");
 		wac = new XmlWebApplicationContext();
@@ -47,7 +52,8 @@ public class PathMatchingUrlHandlerMappingTests extends TestCase {
 		hm = (HandlerMapping) wac.getBean("urlMapping");
 	}
 
-	public void testRequestsWithHandlers() throws Exception {
+	@Test
+	public void requestsWithHandlers() throws Exception {
 		Object bean = wac.getBean("mainController");
 
 		MockHttpServletRequest req = new MockHttpServletRequest("GET", "/welcome.html");
@@ -63,7 +69,8 @@ public class PathMatchingUrlHandlerMappingTests extends TestCase {
 		assertTrue("Handler is correct bean", hec != null && hec.getHandler() == bean);
 	}
 
-	public void testActualPathMatching() throws Exception {
+	@Test
+	public void actualPathMatching() throws Exception {
 		// there a couple of mappings defined with which we can test the
 		// path matching, let's do that...
 
@@ -222,14 +229,16 @@ public class PathMatchingUrlHandlerMappingTests extends TestCase {
 		assertTrue("Handler is correct bean", hec != null && hec.getHandler() == defaultBean);
 	}
 
-	public void testDefaultMapping() throws Exception {
+	@Test
+	public void defaultMapping() throws Exception {
 		Object bean = wac.getBean("starController");
 		MockHttpServletRequest req = new MockHttpServletRequest("GET", "/goggog.html");
 		HandlerExecutionChain hec = getHandler(req);
 		assertTrue("Handler is correct bean", hec != null && hec.getHandler() == bean);
 	}
 
-	public void testMappingExposedInRequest() throws Exception {
+	@Test
+	public void mappingExposedInRequest() throws Exception {
 		Object bean = wac.getBean("mainController");
 		MockHttpServletRequest req = new MockHttpServletRequest("GET", "/show.html");
 		HandlerExecutionChain hec = getHandler(req);
@@ -241,8 +250,8 @@ public class PathMatchingUrlHandlerMappingTests extends TestCase {
 		HandlerExecutionChain hec = hm.getHandler(req);
 		HandlerInterceptor[] interceptors = hec.getInterceptors();
 		if (interceptors != null) {
-			for (int i = 0; i < interceptors.length; i++) {
-				interceptors[i].preHandle(req, null, hec.getHandler());
+			for (HandlerInterceptor interceptor : interceptors) {
+				interceptor.preHandle(req, null, hec.getHandler());
 			}
 		}
 		return hec;

org.springframework.web.servlet/src/test/java/org/springframework/web/servlet/tags/UrlTagTests.java

@@ -554,13 +554,6 @@ public class UrlTagTests extends AbstractTagTests {
 		assertEquals("my%20name%2Bis", tag.urlEncode("my name+is"));
 	}
 	
-	public void testUrlEncode_character() throws UnsupportedEncodingException {
-		assertEquals("%20", tag.urlEncode(' ', "UTF-8"));
-		assertEquals(" ", URLDecoder.decode("%20", "UTF-8"));
-		assertEquals("%FE%FF%00%20", tag.urlEncode(' ', "UTF-16"));
-		assertEquals("%40", tag.urlEncode(' ', "IBM-Thai"));
-	}
-
 	public void testUrlEncodeNull() throws JspException {
 		assertNull(tag.urlEncode(null));
 	}

org.springframework.web/src/main/java/org/springframework/web/util/UriUtils.java

+/*
+ * Copyright 2002-2009 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.web.util;
+
+import java.io.ByteArrayOutputStream;
+import java.io.UnsupportedEncodingException;
+import java.util.BitSet;
+
+import org.springframework.util.Assert;
+
+/**
+ * Utility class for URI encoding. Based on RFC 2396.
+ *
+ * <p>Effectively, the encoding and decoding methods in this class
+ * are similar to those found in {@link java.net.URLEncoder} and
+ * {@link java.net.URLDecoder}, except that the space character
+ * is encoded as {@code %20}, not {@code +}.
+ *
+ * @author Arjen Poutsma
+ * @since 3.0
+ * @see <a href="http://www.ietf.org/rfc/rfc2396.txt">RFC 2396</a>
+ */
+public abstract class UriUtils {
+
+	private static final BitSet notEncoded = new BitSet(256);
+
+	static {
+		for (int i = 'a'; i <= 'z'; i++) {
+			notEncoded.set(i);
+		}
+		for (int i = 'A'; i <= 'Z'; i++) {
+			notEncoded.set(i);
+		}
+		for (int i = '0'; i <= '9'; i++) {
+			notEncoded.set(i);
+		}
+		notEncoded.set('-');
+		notEncoded.set('_');
+		notEncoded.set('.');
+		notEncoded.set('*');
+
+	}
+
+	/**
+	 * Encodes the given source URI into an encoded String. Based on the following
+	 * rules:
+	 * <ul>
+	 * <li>Alphanumeric characters {@code "a"} through {@code "z"},
+	 * {@code "A"} through {@code "Z"}, and {@code "0"} through {@code "9"}
+	 * stay the same.
+	 * <li>Special characters {@code "-"}, {@code "_"}, {@code "."}, and
+	 * {@code "*"} stay the same.
+	 * <li>All other characters are converted into one or more bytes using the
+	 * given encoding scheme. Each of the resulting bytes is written as a
+	 * hexadecimal string in the "<code>%<i>xy</i></code>" format.
+	 * </ul>
+	 *
+	 * @param source the String to be encoded
+	 * @param encoding the encoding to use
+	 * @return the encoded string
+	 * @throws UnsupportedEncodingException when the given encoding parameter is not supported
+	 * @see java.net.URLEncoder#encode(String, String)
+	 */
+	public static String encode(String source, String encoding) throws UnsupportedEncodingException {
+		Assert.notNull(source, "'source' must not be null");
+		Assert.hasLength(encoding, "'encoding' must not be empty");
+		ByteArrayOutputStream bos = new ByteArrayOutputStream(source.length() * 2);
+
+		for (int i = 0; i < source.length(); i++) {
+			int ch = source.charAt(i);
+			if (notEncoded.get(ch)) {
+				bos.write(ch);
+			}
+			else {
+				bos.write('%');
+				char hex1 = Character.toUpperCase(Character.forDigit((ch >> 4) & 0xF, 16));
+				char hex2 = Character.toUpperCase(Character.forDigit(ch & 0xF, 16));
+				bos.write(hex1);
+				bos.write(hex2);
+			}
+		}
+		return new String(bos.toByteArray(), encoding);
+	}
+
+	/**
+	 * Decodes the given encoded source String into an URI. Based on the following
+	 * rules:
+	 * <ul>
+	 * <li>Alphanumeric characters {@code "a"} through {@code "z"},
+	 * {@code "A"} through {@code "Z"}, and {@code "0"} through {@code "9"}
+	 * stay the same.
+	 * <li>Special characters {@code "-"}, {@code "_"}, {@code "."}, and
+	 * {@code "*"} stay the same.
+	 * <li>All other characters are converted into one or more bytes using the
+	 * given encoding scheme. Each of the resulting bytes is written as a
+	 * hexadecimal string in the {@code %xy} format.
+	 * <li>A sequence "<code>%<i>xy</i></code>" is interpreted as a hexadecimal
+	 * representation of the character.
+	 * </ul>
+	 * @param source
+	 * @param encoding
+	 * @return the decoded URI
+	 * @throws UnsupportedEncodingException when the given encoding parameter is not supported
+	 * @see java.net.URLDecoder#decode(String, String)
+	 */
+	public static String decode(String source, String encoding) throws UnsupportedEncodingException {
+		Assert.notNull(source, "'source' must not be null");
+		Assert.hasLength(encoding, "'encoding' must not be empty");
+		int length = source.length();
+		ByteArrayOutputStream bos = new ByteArrayOutputStream(length);
+		for (int i = 0; i < length; i++) {
+			int ch = source.charAt(i);
+			if (ch == '%') {
+				if ((i + 2) < length) {
+					char hex1 = source.charAt(i + 1);
+					char hex2 = source.charAt(i + 2);
+					int u = Character.digit(hex1, 16);
+					int l = Character.digit(hex2, 16);
+					bos.write((char) ((u << 4) + l));
+					i += 2;
+				}
+				else {
+					throw new IllegalArgumentException("Invalid encoded sequence \"" + source.substring(i) + "\"");
+				}
+			}
+			else {
+				bos.write(ch);
+			}
+		}
+		return new String(bos.toByteArray(), encoding);
+	}
+
+}

org.springframework.web/src/main/java/org/springframework/web/util/UrlPathHelper.java

@@ -304,7 +304,7 @@ public class UrlPathHelper {
 		if (this.urlDecode) {
 			String enc = determineEncoding(request);
 			try {
-				return URLDecoder.decode(source, enc);
+				return UriUtils.decode(source, enc);
 			}
 			catch (UnsupportedEncodingException ex) {
 				if (logger.isWarnEnabled()) {

org.springframework.web/src/test/java/org/springframework/web/util/UriUtilsTest.java

+/*
+ * Copyright 2002-2009 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.web.util;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.regex.Pattern;
+import java.util.regex.Matcher;
+import java.io.UnsupportedEncodingException;
+
+import org.junit.Test;
+import static org.junit.Assert.assertEquals;
+
+/** @author Arjen Poutsma */
+public class UriUtilsTest {
+
+	@Test
+	public void encode() throws UnsupportedEncodingException {
+		assertEquals("Invalid encoded URI", "foobar", UriUtils.encode("foobar", "UTF-8"));
+		assertEquals("Invalid encoded URI", "foo%20bar", UriUtils.encode("foo bar", "UTF-8"));
+		assertEquals("Invalid encoded URI", "foo%2Bbar", UriUtils.encode("foo+bar", "UTF-8"));
+	}
+
+	@Test
+	public void decode() throws UnsupportedEncodingException {
+		assertEquals("Invalid encoded URI", "foobar", UriUtils.decode("foobar", "UTF-8"));
+		assertEquals("Invalid encoded URI", "foo bar", UriUtils.decode("foo%20bar", "UTF-8"));
+		assertEquals("Invalid encoded URI", "foo+bar", UriUtils.decode("foo%2bbar", "UTF-8"));
+	}
+
+	@Test(expected = IllegalArgumentException.class)
+	public void decodeInvalidSequence() throws UnsupportedEncodingException {
+		UriUtils.decode("foo%2", "UTF-8");
+	}
+
+}

org.springframework.web/src/test/java/org/springframework/web/util/UrlPathHelperTests.java

@@ -16,44 +16,68 @@
 
 package org.springframework.web.util;
 
+import java.net.URI;
+import java.net.URISyntaxException;
+
 import junit.framework.TestCase;
 
 import org.springframework.mock.web.MockHttpServletRequest;
 
+import org.junit.Test;
+import org.junit.Before;
+import static org.junit.Assert.assertEquals;
+
 /**
  * @author Rob Harrop
  * @author Juergen Hoeller
  */
-public class UrlPathHelperTests extends TestCase {
+public class UrlPathHelperTests {
 
-	public void testGetPathWithinApplication() {
-		UrlPathHelper helper = new UrlPathHelper();
+	private UrlPathHelper helper;
 
-		MockHttpServletRequest request = new MockHttpServletRequest();
+	private MockHttpServletRequest request;
+
+	@Before
+	public void setUp() {
+		helper = new UrlPathHelper();
+		request = new MockHttpServletRequest();
+	}
+
+	@Test
+	public void getPathWithinApplication() {
 		request.setContextPath("/petclinic");
 		request.setRequestURI("/petclinic/welcome.html");
 
 		assertEquals("Incorrect path returned", "/welcome.html", helper.getPathWithinApplication(request));
 	}
 
-	public void testGetPathWithinApplicationForRootWithNoLeadingSlash() {
-		UrlPathHelper helper = new UrlPathHelper();
-
-		MockHttpServletRequest request = new MockHttpServletRequest();
+	@Test
+	public void getPathWithinApplicationForRootWithNoLeadingSlash() {
 		request.setContextPath("/petclinic");
 		request.setRequestURI("/petclinic");
 
 		assertEquals("Incorrect root path returned", "/", helper.getPathWithinApplication(request));
 	}
 
-	public void testGetPathWithinApplicationForSlashContextPath() {
-		UrlPathHelper helper = new UrlPathHelper();
-
-		MockHttpServletRequest request = new MockHttpServletRequest();
+	@Test
+	public void getPathWithinApplicationForSlashContextPath() {
 		request.setContextPath("/");
 		request.setRequestURI("/welcome.html");
 
 		assertEquals("Incorrect path returned", "/welcome.html", helper.getPathWithinApplication(request));
 	}
 
+	@Test
+	public void getRequestUri() {
+		request.setRequestURI("/welcome.html");
+		assertEquals("Incorrect path returned", "/welcome.html", helper.getRequestUri(request));
+
+		request.setRequestURI("/foo%20bar");
+		assertEquals("Incorrect path returned", "/foo bar", helper.getRequestUri(request));
+
+		request.setRequestURI("/foo+bar");
+		assertEquals("Incorrect path returned", "/foo+bar", helper.getRequestUri(request));
+
+	}
+
 }