From de9c9febc385fe07809f634cef7933b632e7141f Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Thu, 19 Mar 2015 16:57:26 -0400 Subject: [PATCH] Compare encoded params in AbstractFlashMapManager AbstractFlashMapManager no longer decodes the target query parameters it needs to use to match to the request after the redirect. Instead it stores query parameters as-is adn then relies on parsing the encoded query string after the redirect. Issue: SPR-12569 --- .../support/AbstractFlashMapManager.java | 33 ++++------ ...nnotationControllerHandlerMethodTests.java | 9 +-- .../servlet/support/FlashMapManagerTests.java | 64 ++++++++++++++----- 3 files changed, 68 insertions(+), 38 deletions(-) diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java index 503662876f6..c859a2301a8 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/support/AbstractFlashMapManager.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2015 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,11 +16,11 @@ package org.springframework.web.servlet.support; -import java.util.ArrayList; import java.util.Collections; import java.util.LinkedList; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; + import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -30,12 +30,13 @@ import org.apache.commons.logging.LogFactory; import org.springframework.util.Assert; import org.springframework.util.CollectionUtils; import org.springframework.util.MultiValueMap; -import org.springframework.util.ObjectUtils; import org.springframework.util.StringUtils; import org.springframework.web.servlet.FlashMap; import org.springframework.web.servlet.FlashMapManager; +import org.springframework.web.util.UriComponents; import org.springframework.web.util.UrlPathHelper; + /** * A base class for {@link FlashMapManager} implementations. * @@ -172,10 +173,16 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { return false; } } - MultiValueMap targetParams = flashMap.getTargetRequestParams(); - for (String expectedName : targetParams.keySet()) { - for (String expectedValue : targetParams.get(expectedName)) { - if (!ObjectUtils.containsElement(request.getParameterValues(expectedName), expectedValue)) { + UriComponents uriComponents = ServletUriComponentsBuilder.fromRequest(request).build(); + MultiValueMap actualParams = uriComponents.getQueryParams(); + MultiValueMap expectedParams = flashMap.getTargetRequestParams(); + for (String expectedName : expectedParams.keySet()) { + List actualValues = actualParams.get(expectedName); + if (actualValues == null) { + return false; + } + for (String expectedValue : expectedParams.get(expectedName)) { + if (!actualValues.contains(expectedValue)) { return false; } } @@ -191,7 +198,6 @@ 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); @@ -227,17 +233,6 @@ public abstract class AbstractFlashMapManager implements FlashMapManager { return path; } - private void decodeParameters(MultiValueMap params, HttpServletRequest request) { - for (String name : new ArrayList(params.keySet())) { - for (String value : new ArrayList(params.remove(name))) { - name = getUrlPathHelper().decodeRequestString(request, name); - value = getUrlPathHelper().decodeRequestString(request, value); - params.add(name, value); - } - } - } - - /** * Retrieve saved FlashMap instances from the underlying storage. * @param request the current request diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java index 737408a083e..e7d7e1abc08 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/ServletAnnotationControllerHandlerMethodTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2015 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,6 +16,8 @@ package org.springframework.web.servlet.mvc.method.annotation; +import static org.junit.Assert.*; + import java.beans.PropertyEditorSupport; import java.io.IOException; import java.io.Serializable; @@ -42,6 +44,7 @@ import java.util.List; import java.util.Locale; import java.util.Map; import java.util.Set; + import javax.servlet.ServletConfig; import javax.servlet.ServletContext; import javax.servlet.ServletException; @@ -137,8 +140,6 @@ import org.springframework.web.servlet.mvc.support.RedirectAttributes; import org.springframework.web.servlet.support.RequestContextUtils; import org.springframework.web.servlet.view.InternalResourceViewResolver; -import static org.junit.Assert.*; - /** * The origin of this test class is {@link ServletAnnotationControllerHandlerMethodTests}. * @@ -1532,7 +1533,7 @@ public class ServletAnnotationControllerHandlerMethodTests extends AbstractServl // GET after POST request = new MockHttpServletRequest("GET", "/messages/1"); - request.addParameter("name", "value"); + request.setQueryString("name=value"); request.setSession(session); response = new MockHttpServletResponse(); getServlet().service(request, response); diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/support/FlashMapManagerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/support/FlashMapManagerTests.java index 6b5fd2b3917..9a5e3cb7731 100644 --- a/spring-webmvc/src/test/java/org/springframework/web/servlet/support/FlashMapManagerTests.java +++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/support/FlashMapManagerTests.java @@ -18,6 +18,7 @@ package org.springframework.web.servlet.support; import static org.junit.Assert.*; +import java.net.URLEncoder; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -31,7 +32,6 @@ 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; @@ -113,19 +113,19 @@ public class FlashMapManagerTests { this.flashMapManager.setFlashMaps(Arrays.asList(flashMap)); - this.request.setParameter("number", (String) null); + this.request.setQueryString("number="); FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertNull(inputFlashMap); assertEquals("FlashMap should not have been removed", 1, this.flashMapManager.getFlashMaps().size()); - this.request.setParameter("number", "two"); + this.request.setQueryString("number=two"); inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertNull(inputFlashMap); assertEquals("FlashMap should not have been removed", 1, this.flashMapManager.getFlashMaps().size()); - this.request.setParameter("number", "one"); + this.request.setQueryString("number=one"); inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertEquals(flashMap, inputFlashMap); @@ -143,13 +143,13 @@ public class FlashMapManagerTests { this.flashMapManager.setFlashMaps(Arrays.asList(flashMap)); - this.request.setParameter("id", "1"); + this.request.setQueryString("id=1"); FlashMap inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertNull(inputFlashMap); assertEquals("FlashMap should not have been removed", 1, this.flashMapManager.getFlashMaps().size()); - this.request.addParameter("id", "2"); + this.request.setQueryString("id=1&id=2"); inputFlashMap = this.flashMapManager.retrieveAndUpdate(this.request, this.response); assertEquals(flashMap, inputFlashMap); @@ -268,20 +268,54 @@ public class FlashMapManagerTests { @Test public void saveOutputFlashMapDecodeParameters() throws Exception { - this.request.setCharacterEncoding("UTF-8"); FlashMap flashMap = new FlashMap(); - flashMap.put("anyKey", "anyValue"); - - flashMap.addTargetRequestParam("key", "%D0%90%D0%90"); - flashMap.addTargetRequestParam("key", "%D0%91%D0%91"); - flashMap.addTargetRequestParam("key", "%D0%92%D0%92"); + flashMap.put("key", "value"); + flashMap.setTargetRequestPath("/path"); + flashMap.addTargetRequestParam("param", "%D0%90%D0%90"); + flashMap.addTargetRequestParam("param", "%D0%91%D0%91"); + flashMap.addTargetRequestParam("param", "%D0%92%D0%92"); flashMap.addTargetRequestParam("%3A%2F%3F%23%5B%5D%40", "value"); + + this.request.setCharacterEncoding("UTF-8"); this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); - MultiValueMap targetRequestParams = flashMap.getTargetRequestParams(); - assertEquals(Arrays.asList("\u0410\u0410", "\u0411\u0411", "\u0412\u0412"), targetRequestParams.get("key")); - assertEquals(Arrays.asList("value"), targetRequestParams.get(":/?#[]@")); + MockHttpServletRequest requestAfterRedirect = new MockHttpServletRequest("GET", "/path"); + requestAfterRedirect.setQueryString("param=%D0%90%D0%90¶m=%D0%91%D0%91¶m=%D0%92%D0%92&%3A%2F%3F%23%5B%5D%40=value"); + requestAfterRedirect.addParameter("param", "\u0410\u0410"); + requestAfterRedirect.addParameter("param", "\u0411\u0411"); + requestAfterRedirect.addParameter("param", "\u0412\u0412"); + requestAfterRedirect.addParameter(":/?#[]@", "value"); + + flashMap = this.flashMapManager.retrieveAndUpdate(requestAfterRedirect, new MockHttpServletResponse()); + assertNotNull(flashMap); + assertEquals(1, flashMap.size()); + assertEquals("value", flashMap.get("key")); + } + + // SPR-12569 + + @Test + public void flashAttributesWithQueryParamsWithSpace() throws Exception { + + String encodedValue = URLEncoder.encode("1 2", "UTF-8"); + + FlashMap flashMap = new FlashMap(); + flashMap.put("key", "value"); + flashMap.setTargetRequestPath("/path"); + flashMap.addTargetRequestParam("param", encodedValue); + + this.request.setCharacterEncoding("UTF-8"); + this.flashMapManager.saveOutputFlashMap(flashMap, this.request, this.response); + + MockHttpServletRequest requestAfterRedirect = new MockHttpServletRequest("GET", "/path"); + requestAfterRedirect.setQueryString("param=" + encodedValue); + requestAfterRedirect.addParameter("param", "1 2"); + + flashMap = this.flashMapManager.retrieveAndUpdate(requestAfterRedirect, new MockHttpServletResponse()); + assertNotNull(flashMap); + assertEquals(1, flashMap.size()); + assertEquals("value", flashMap.get("key")); }