AbstractFlashMapManager needs to decode parameter names

Also includes general alignment with the 4.0.3 versions of AbstractFlashMapManager and FlashMapManagerTests.

Issue: SPR-11504

spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2014 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.
@@ -20,12 +20,12 @@ import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
-
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
+
 import org.springframework.util.Assert;
 import org.springframework.util.CollectionUtils;
 import org.springframework.util.MultiValueMap;
@@ -43,13 +43,14 @@ import org.springframework.web.util.UrlPathHelper;
  */
 public abstract class AbstractFlashMapManager implements FlashMapManager {
 
+	private static final Object writeLock = new Object();
+
 	protected final Log logger = LogFactory.getLog(getClass());
 
 	private int flashMapTimeout = 180;
 
 	private UrlPathHelper urlPathHelper = new UrlPathHelper();
 
-	private static final Object writeLock = new Object();
 
 	/**
 	 * Set the amount of time in seconds after a {@link FlashMap} is saved
@@ -82,16 +83,16 @@ public abstract class AbstractFlashMapManager implements FlashMapManager {
 		return this.urlPathHelper;
 	}
 
-	public final FlashMap retrieveAndUpdate(HttpServletRequest request, HttpServletResponse response) {
 
+	public final FlashMap retrieveAndUpdate(HttpServletRequest request, HttpServletResponse response) {
 		List<FlashMap> maps = retrieveFlashMaps(request);
 		if (CollectionUtils.isEmpty(maps)) {
 			return null;
 		}
+
 		if (logger.isDebugEnabled()) {
 			logger.debug("Retrieved FlashMap(s): " + maps);
 		}
-
 		List<FlashMap> mapsToRemove = getExpiredFlashMaps(maps);
 
 		FlashMap match = getMatchingFlashMap(maps, request);
@@ -156,21 +157,20 @@ public abstract class AbstractFlashMapManager implements FlashMapManager {
 
 	/**
 	 * Whether the given FlashMap matches the current request.
-	 * The default implementation uses the target request path and query
-	 * parameters saved in the FlashMap.
+	 * Uses the expected request path and query parameters saved in the FlashMap.
 	 */
 	protected boolean isFlashMapForRequest(FlashMap flashMap, HttpServletRequest request) {
-		if (flashMap.getTargetRequestPath() != null) {
+		String expectedPath = flashMap.getTargetRequestPath();
+		if (expectedPath != null) {
 			String requestUri = this.urlPathHelper.getOriginatingRequestUri(request);
-			if (!requestUri.equals(flashMap.getTargetRequestPath())
-					&& !requestUri.equals(flashMap.getTargetRequestPath() + "/")) {
+			if (!requestUri.equals(expectedPath) && !requestUri.equals(expectedPath + "/")) {
 				return false;
 			}
 		}
 		MultiValueMap<String, String> targetParams = flashMap.getTargetRequestParams();
-		for (String paramName : targetParams.keySet()) {
-			for (String targetValue : targetParams.get(paramName)) {
-				if (!ObjectUtils.containsElement(request.getParameterValues(paramName), targetValue)) {
+		for (String expectedName : targetParams.keySet()) {
+			for (String expectedValue : targetParams.get(expectedName)) {
+				if (!ObjectUtils.containsElement(request.getParameterValues(expectedName), expectedValue)) {
 					return false;
 				}
 			}
@@ -185,18 +185,16 @@ public abstract class AbstractFlashMapManager implements FlashMapManager {
 
 		String path = decodeAndNormalizePath(flashMap.getTargetRequestPath(), request);
 		flashMap.setTargetRequestPath(path);
-
 		decodeParameters(flashMap.getTargetRequestParams(), request);
 
 		if (logger.isDebugEnabled()) {
 			logger.debug("Saving FlashMap=" + flashMap);
 		}
-
 		flashMap.startExpirationPeriod(this.flashMapTimeout);
 
 		synchronized (writeLock) {
 			List<FlashMap> allMaps = retrieveFlashMaps(request);
-			allMaps = (allMaps == null) ? new CopyOnWriteArrayList<FlashMap>() : allMaps;
+			allMaps = (allMaps != null ? allMaps : new CopyOnWriteArrayList<FlashMap>());
 			allMaps.add(flashMap);
 			updateFlashMaps(allMaps, request, response);
 		}
@@ -217,7 +215,9 @@ public abstract class AbstractFlashMapManager implements FlashMapManager {
 	private void decodeParameters(MultiValueMap<String, String> params, HttpServletRequest request) {
 		for (String name : new ArrayList<String>(params.keySet())) {
 			for (String value : new ArrayList<String>(params.remove(name))) {
-				params.add(name, this.urlPathHelper.decodeRequestString(request, value));
+				name = this.urlPathHelper.decodeRequestString(request, name);
+				value = this.urlPathHelper.decodeRequestString(request, value);
+				params.add(name, value);
 			}
 		}
 	}
@@ -228,7 +228,7 @@ public abstract class AbstractFlashMapManager implements FlashMapManager {
 	 * @param request the current request
 	 * @param response the current response
 	 */
-	protected abstract void updateFlashMaps(List<FlashMap> flashMaps, HttpServletRequest request,
-			HttpServletResponse response);
+	protected abstract void updateFlashMaps(
+			List<FlashMap> flashMaps, HttpServletRequest request, HttpServletResponse response);
 
 }

spring-webmvc/src/test/java/org/springframework/web/servlet/support/FlashMapManagerTests.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2012 the original author or authors.
+ * Copyright 2002-2014 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.
@@ -16,34 +16,30 @@
 
 package org.springframework.web.servlet.support;
 
-
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertNull;
-import static org.junit.Assert.assertSame;
-import static org.junit.Assert.assertTrue;
-
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.CopyOnWriteArrayList;
-
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
 import org.junit.Before;
 import org.junit.Test;
+
 import org.springframework.mock.web.test.MockHttpServletRequest;
 import org.springframework.mock.web.test.MockHttpServletResponse;
+import org.springframework.util.MultiValueMap;
 import org.springframework.web.servlet.FlashMap;
 import org.springframework.web.util.WebUtils;
 
+import static org.junit.Assert.*;
+
 /**
  * Test fixture for testing {@link AbstractFlashMapManager} methods.
  *
  * @author Rossen Stoyanchev
  */
-public class AbstractFlashMapManagerTests {
+public class FlashMapManagerTests {
 
 	private TestFlashMapManager flashMapManager;
 
@@ -264,6 +260,8 @@ public class AbstractFlashMapManagerTests {
 		assertEquals("/once/only", flashMap.getTargetRequestPath());
 	}
 
+	// SPR-9657, SPR-11504
+
 	@Test
 	public void saveOutputFlashMapDecodeParameters() throws Exception {
 		this.request.setCharacterEncoding("UTF-8");
@@ -274,10 +272,12 @@ public class AbstractFlashMapManagerTests {
 		flashMap.addTargetRequestParam("key", "%D0%90%D0%90");
 		flashMap.addTargetRequestParam("key", "%D0%91%D0%91");
 		flashMap.addTargetRequestParam("key", "%D0%92%D0%92");
+		flashMap.addTargetRequestParam("%3A%2F%3F%23%5B%5D%40", "value");
 		this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response);
 
-		assertEquals(Arrays.asList("\u0410\u0410", "\u0411\u0411", "\u0412\u0412"),
-				flashMap.getTargetRequestParams().get("key"));
+		MultiValueMap<String,String> targetRequestParams = flashMap.getTargetRequestParams();
+		assertEquals(Arrays.asList("\u0410\u0410", "\u0411\u0411", "\u0412\u0412"), targetRequestParams.get("key"));
+		assertEquals(Arrays.asList("value"), targetRequestParams.get(":/?#[]@"));
 	}
 
 
@@ -303,8 +303,7 @@ public class AbstractFlashMapManagerTests {
 		}
 
 		@Override
-		protected void updateFlashMaps(List<FlashMap> flashMaps, HttpServletRequest request,
-				HttpServletResponse response) {
+		protected void updateFlashMaps(List<FlashMap> flashMaps, HttpServletRequest request, HttpServletResponse response) {
 			this.flashMaps = flashMaps;
 		}
 	}