Prevent double encoding problems when URI fragment contains special characters (#10769)

Change-Id: I9043a1f84140b441ab4b3e86eadb708359a29155

4 files changed, 160 insertions, 31 deletions

diff --git a/client/src/com/vaadin/client/ui/VUI.java b/client/src/com/vaadin/client/ui/VUI.java
index b07593896f..9a73aa5f8f 100644
--- a/client/src/com/vaadin/client/ui/VUI.java
+++ b/client/src/com/vaadin/client/ui/VUI.java
@@ -133,10 +133,16 @@ public class VUI extends SimplePanel implements ResizeHandler,
             // Send the location to the server if the fragment has changed
             // and flush active connectors in UI.
             if (!newFragment.equals(currentFragment) && connection != null) {
+
+                // Ensure the fragment is properly encoded in all browsers
+                // (#10769)
+                String location = Window.Location.createUrlBuilder()
+                        .buildString();
+
                 currentFragment = newFragment;
                 connection.flushActiveConnector();
                 connection.updateVariable(id, UIConstants.LOCATION_VARIABLE,
-                        Window.Location.getHref(), true);
+                        location, true);
             }
         }
     };
diff --git a/client/src/com/vaadin/client/ui/ui/UIConnector.java b/client/src/com/vaadin/client/ui/ui/UIConnector.java
index ac441fc625..593aa0d793 100644
--- a/client/src/com/vaadin/client/ui/ui/UIConnector.java
+++ b/client/src/com/vaadin/client/ui/ui/UIConnector.java
@@ -28,6 +28,7 @@ import com.google.gwt.event.dom.client.ScrollEvent;
 import com.google.gwt.event.dom.client.ScrollHandler;
 import com.google.gwt.event.logical.shared.ResizeEvent;
 import com.google.gwt.event.logical.shared.ResizeHandler;
+import com.google.gwt.http.client.URL;
 import com.google.gwt.user.client.Command;
 import com.google.gwt.user.client.DOM;
 import com.google.gwt.user.client.Element;
@@ -303,8 +304,9 @@ public class UIConnector extends AbstractSingleComponentContainerConnector
                     .getStringAttribute(UIConstants.LOCATION_VARIABLE);
             int fragmentIndex = location.indexOf('#');
             if (fragmentIndex >= 0) {
-                getWidget().currentFragment = location
-                        .substring(fragmentIndex + 1);
+                // Decode fragment to avoid double encoding (#10769)
+                getWidget().currentFragment = URL.decodePathSegment(location
+                        .substring(fragmentIndex + 1));
             }
             if (!getWidget().currentFragment.equals(History.getToken())) {
                 History.newItem(getWidget().currentFragment, true);
diff --git a/uitest/src/com/vaadin/tests/navigator/NavigatorTest.html b/uitest/src/com/vaadin/tests/navigator/NavigatorTest.html
index 030b30f37a..7eba02aa94 100644
--- a/uitest/src/com/vaadin/tests/navigator/NavigatorTest.html
+++ b/uitest/src/com/vaadin/tests/navigator/NavigatorTest.html
@@ -18,8 +18,13 @@
 </tr>
 <tr>
 	<td>assertText</td>
-	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/VOrderedLayout$Slot[4]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VLabel[0]</td>
-	<td>1. Navigated to DefaultView with params</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
+	<td>1. Navigated to DefaultView without params</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[6]/VVerticalLayout[0]/Slot[0]/VLabel[0]</td>
+	<td>Default view:</td>
 </tr>
 <tr>
 	<td>click</td>
@@ -28,8 +33,13 @@
 </tr>
 <tr>
 	<td>assertText</td>
-	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/VOrderedLayout$Slot[4]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VLabel[0]</td>
-	<td>2. Navigated to ListView with params</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
+	<td>2. Navigated to ListView without params</td>
+</tr>
+<tr>
+	<td>assertElementPresent</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[6]/VVerticalLayout[0]/Slot[0]/VScrollTable[0]</td>
+	<td></td>
 </tr>
 <tr>
 	<td>assertLocation</td>
@@ -43,8 +53,13 @@
 </tr>
 <tr>
 	<td>assertText</td>
-	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/VOrderedLayout$Slot[4]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VLabel[0]</td>
-	<td>3. Navigated to EditView with params</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
+	<td>3. Navigated to EditView without params</td>
+</tr>
+<tr>
+	<td>assertElementPresent</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[6]/VVerticalLayout[0]/Slot[0]/VRichTextArea[0]</td>
+	<td></td>
 </tr>
 <tr>
 	<td>assertLocation</td>
@@ -53,12 +68,12 @@
 </tr>
 <tr>
 	<td>mouseClick</td>
-	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/VOrderedLayout$Slot[3]/VTextField[0]</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[4]/VTextField[0]</td>
 	<td>56,6</td>
 </tr>
 <tr>
 	<td>enterCharacter</td>
-	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/VOrderedLayout$Slot[3]/VTextField[0]</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[4]/VTextField[0]</td>
 	<td>param=value</td>
 </tr>
 <tr>
@@ -68,10 +83,20 @@
 </tr>
 <tr>
 	<td>assertText</td>
-	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/VOrderedLayout$Slot[4]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VLabel[0]</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
 	<td>4. Navigated to ListView with params param=value</td>
 </tr>
 <tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[6]/VVerticalLayout[0]/Slot[0]/VScrollTable[0]/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[0]/domChild[0]/domChild[0]</td>
+	<td>param</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[6]/VVerticalLayout[0]/Slot[0]/VScrollTable[0]/domChild[1]/domChild[0]/domChild[1]/domChild[0]/domChild[0]/domChild[1]/domChild[0]</td>
+	<td>value</td>
+</tr>
+<tr>
 	<td>assertLocation</td>
 	<td>*#!list/param=value</td>
 	<td></td>
@@ -83,7 +108,7 @@
 </tr>
 <tr>
 	<td>assertText</td>
-	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/VOrderedLayout$Slot[4]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VLabel[0]</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
 	<td>5. Navigated to EditView with params param=value</td>
 </tr>
 <tr>
@@ -98,7 +123,7 @@
 </tr>
 <tr>
 	<td>assertText</td>
-	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/VOrderedLayout$Slot[4]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VLabel[0]</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
 	<td>6. Prevent navigation to ForbiddenView</td>
 </tr>
 <tr>
@@ -107,45 +132,123 @@
 	<td></td>
 </tr>
 <tr>
+	<td>click</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[3]/VButton[0]/domChild[0]/domChild[0]</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
+	<td>7. Navigated to SpecialCharsView: öääö !%&amp;/()=; fragment: !öääö !%&amp;/()=/param=value</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[6]/VVerticalLayout[0]/Slot[0]/VLabel[0]</td>
+	<td>öääö !%&amp;/()=</td>
+</tr>
+<tr>
+	<td>assertLocation</td>
+	<td>*#!%C3%B6%C3%A4%C3%A4%C3%B6%20!%25&amp;/()=/param=value</td>
+	<td></td>
+</tr>
+<tr>
+	<td>runScript</td>
+	<td>window.location.hash='!foo bar'</td>
+	<td></td>
+</tr>
+<tr>
+	<td>pause</td>
+	<td>3000</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
+	<td>8. View 'foo bar' not found!</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[6]/VVerticalLayout[0]/Slot[0]/VLabel[0]</td>
+	<td>Tried to navigate to foo bar but such a view could not be found :(</td>
+</tr>
+<tr>
+	<td>assertLocation</td>
+	<td>regex:.*#!foo( |%20)bar</td>
+	<td></td>
+</tr>
+<tr>
+	<td>runScript</td>
+	<td>window.location.hash='!/foo=bar'</td>
+	<td></td>
+</tr>
+<tr>
+	<td>pause</td>
+	<td>3000</td>
+	<td></td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
+	<td>9. Navigated to DefaultView with params foo=bar</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[6]/VVerticalLayout[0]/Slot[0]/VLabel[0]</td>
+	<td>Default view: foo=bar</td>
+</tr>
+<tr>
+	<td>assertLocation</td>
+	<td>*#!/foo=bar</td>
+	<td></td>
+</tr>
+<tr>
 	<td>runScript</td>
-	<td>window.location.hash='!foo'</td>
+	<td>window.location.hash='foo bar'</td>
 	<td></td>
 </tr>
 <tr>
 	<td>pause</td>
-	<td>1000</td>
+	<td>3000</td>
 	<td></td>
 </tr>
 <tr>
 	<td>assertText</td>
-	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/VOrderedLayout$Slot[4]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VLabel[0]</td>
-	<td>7. View 'foo' not found!</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
+	<td>10. Navigated to DefaultView without params</td>
 </tr>
 <tr>
 	<td>assertLocation</td>
-	<td>*#!foo</td>
+	<td>regex:.*#foo( |%20)bar</td>
 	<td></td>
 </tr>
+<!-- 
+This test is broken in Safari 5; doing the same manually works fine
 <tr>
 	<td>runScript</td>
-	<td>window.location.hash='foo'</td>
+	<td>window.location.hash='!öääö !%25&amp;/()='</td>
 	<td></td>
 </tr>
 <tr>
 	<td>pause</td>
-	<td>1000</td>
+	<td>3000</td>
 	<td></td>
 </tr>
 <tr>
 	<td>assertText</td>
-	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/VOrderedLayout$Slot[4]/VVerticalLayout[0]/VOrderedLayout$Slot[0]/VLabel[0]</td>
-	<td>8. Navigated to DefaultView with params</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::PID_SLog_row_0</td>
+	<td>10. Navigated to SpecialCharsView: öääö !%&amp;/()=; fragment: !öääö !%&amp;/()=</td>
+</tr>
+<tr>
+	<td>assertText</td>
+	<td>vaadin=runcomvaadintestsnavigatorNavigatorTest::/VVerticalLayout[0]/Slot[6]/VVerticalLayout[0]/Slot[0]/VLabel[0]</td>
+	<td>öääö !%&amp;/()=</td>
 </tr>
 <tr>
 	<td>assertLocation</td>
-	<td>*#foo</td>
+	<td>regex:.*#!(öääö !%|öääö !%25|%C3%B6%C3%A4%C3%A4%C3%B6%20!%25)&amp;/\(\)=</td>
 	<td></td>
 </tr>
+ -->
 
 </tbody></table>
 </body>
diff --git a/uitest/src/com/vaadin/tests/navigator/NavigatorTest.java b/uitest/src/com/vaadin/tests/navigator/NavigatorTest.java
index f35c8b876d..81944abd40 100644
--- a/uitest/src/com/vaadin/tests/navigator/NavigatorTest.java
+++ b/uitest/src/com/vaadin/tests/navigator/NavigatorTest.java
@@ -37,7 +37,9 @@ public class NavigatorTest extends UI {
         @Override
         public void enter(ViewChangeEvent event) {
             String params = event.getParameters();
-            log.log("Navigated to ListView with params " + params);
+            log.log("Navigated to ListView "
+                    + (params.isEmpty() ? "without params" : "with params "
+                            + params));
             removeAllItems();
             for (String arg : params.split(",")) {
                 addItem(arg.split("=|$", 2), arg);
@@ -49,19 +51,33 @@ public class NavigatorTest extends UI {
 
         @Override
         public void enter(ViewChangeEvent event) {
-            log.log("Navigated to EditView with params "
-                    + event.getParameters());
-            setValue("Displaying edit view with parameters "
-                    + event.getParameters());
+            String params = event.getParameters();
+            log.log("Navigated to EditView "
+                    + (params.isEmpty() ? "without params" : "with params "
+                            + params));
+            setValue("Displaying edit view with parameters " + params);
         }
     }
 
+    class SpecialCharsView extends Label implements View {
+
+        @Override
+        public void enter(ViewChangeEvent event) {
+            log.log("Navigated to SpecialCharsView: " + event.getViewName()
+                    + "; fragment: " + getPage().getUriFragment());
+            setValue(event.getViewName());
+        }
+
+    }
+
     class DefaultView extends Label implements View {
 
         @Override
         public void enter(ViewChangeEvent event) {
-            log.log("Navigated to DefaultView with params "
-                    + event.getParameters());
+            String params = event.getParameters();
+            log.log("Navigated to DefaultView "
+                    + (params.isEmpty() ? "without params" : "with params "
+                            + params));
             setValue("Default view: " + event.getParameters());
         }
     }
@@ -123,6 +139,7 @@ public class NavigatorTest extends UI {
 
             navi.addView("list", new ListView());
             navi.addView("edit", new EditView());
+            navi.addView("öääö !%&/()=", new SpecialCharsView());
             navi.addView("forbidden", new ForbiddenView());
 
             navi.addViewChangeListener(new NaviListener());
@@ -132,6 +149,7 @@ public class NavigatorTest extends UI {
             layout.addComponent(new NaviButton("list"));
             layout.addComponent(new NaviButton("edit"));
             layout.addComponent(new NaviButton("forbidden"));
+            layout.addComponent(new NaviButton("öääö !%&/()="));
 
             layout.addComponent(params);
             layout.addComponent(log);