JSPWIKI-1323 Ignore a preferences cookie that is not a JSON object

ChangeLog.md

@@ -35,6 +35,7 @@ under the License.
 * [JSPWIKI-1263](https://issues.apache.org/jira/browse/JSPWIKI-1263): the `Admin` group can no longer be removed, so the wiki cannot be left without administrators
 * [JSPWIKI-529](https://issues.apache.org/jira/browse/JSPWIKI-529): markup handling defect when passing css properties containing a percent symbol
 * Fix for [JSPWIKI-1130](https://issues.apache.org/jira/browse/JSPWIKI-1130): the preview of the editor no longer runs the scripts of the page, which could replace the whole editor page ([PR#560](https://github.com/apache/jspwiki/pull/560))
+* Fix for [JSPWIKI-1323](https://issues.apache.org/jira/browse/JSPWIKI-1323): a `JSPWikiUserPrefs` cookie that is not a JSON object is ignored, so the page keeps its stylesheets and scripts ([PR#577](https://github.com/apache/jspwiki/pull/577))
 * Fix for [JSPWIKI-1324](https://issues.apache.org/jira/browse/JSPWIKI-1324): the time zone and time format lists of the user preferences are never wider than their form, so they stay within a narrow screen ([PR#578](https://github.com/apache/jspwiki/pull/578))
 * Fix for [JSPWIKI-1354](https://issues.apache.org/jira/browse/JSPWIKI-1354): a test of the search no longer fails when the index has not got to all the pages ([PR#585](https://github.com/apache/jspwiki/pull/585))
 

jspwiki-main/src/main/java/org/apache/wiki/preferences/Preferences.java

@@ -19,6 +19,7 @@ Licensed to the Apache Software Foundation (ASF) under one
 package org.apache.wiki.preferences;
 
 import com.google.gson.Gson;
+import com.google.gson.JsonParseException;
 import org.apache.commons.lang3.LocaleUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.logging.log4j.LogManager;
@@ -128,16 +129,33 @@ public static void reloadPreferences( final PageContext pageContext ) {
 
 
     /**
-     * Parses new-style preferences stored as JSON objects and stores them in the session.  Everything in the cookie is stored.
+     * Parses new-style preferences stored as JSON objects and stores them in the session.  Everything in the cookie is stored. The cookie
+     * comes from the client, so one that cannot be decoded or is not a JSON object is ignored and the default preferences stay.
      *
      * @param request
      * @param prefs The default hashmap of preferences
      */
-	static void parseJSONPreferences( final HttpServletRequest request, final Preferences prefs ) {
-        final String prefVal = TextUtil.urlDecodeUTF8( HttpUtil.retrieveCookieValue( request, COOKIE_USER_PREFS_NAME ) );
+    static void parseJSONPreferences( final HttpServletRequest request, final Preferences prefs ) {
+        final String prefVal;
+        try {
+            prefVal = TextUtil.urlDecodeUTF8( HttpUtil.retrieveCookieValue( request, COOKIE_USER_PREFS_NAME ) );
+        } catch( final IllegalArgumentException e ) {
+            LOG.warn( "Ignoring the {} cookie, it cannot be decoded: {}", COOKIE_USER_PREFS_NAME, e.getMessage() );
+            return;
+        }
         if( prefVal != null ) {
             // Convert prefVal JSON to a generic hashmap
-            @SuppressWarnings( "unchecked" ) final Map< String, String > map = new Gson().fromJson( prefVal, Map.class );
+            final Map< String, String > map;
+            try {
+                @SuppressWarnings( "unchecked" ) final Map< String, String > parsed = new Gson().fromJson( prefVal, Map.class );
+                map = parsed;
+            } catch( final JsonParseException e ) {
+                LOG.warn( "Ignoring the {} cookie, its value is not a JSON object: {}", COOKIE_USER_PREFS_NAME, e.getMessage() );
+                return;
+            }
+            if( map == null ) { // an empty value or the JSON literal null
+                return;
+            }
             for( String key : map.keySet() ) {
                 key = TextUtil.replaceEntities( key );
                 // Sometimes this is not a String as it comes from the Cookie set by Javascript

jspwiki-main/src/test/java/org/apache/wiki/preferences/PreferencesTest.java

@@ -27,8 +27,63 @@ Licensed to the Apache Software Foundation (ASF) under one
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
 
+public class PreferencesTest {
 
-class PreferencesTest {
+    private static Preferences parse( final String cookieValue ) {
+        final HttpServletRequest request = Mockito.mock( HttpServletRequest.class );
+        if( cookieValue != null ) {
+            Mockito.when( request.getCookies() ).thenReturn( new Cookie[] { new Cookie( Preferences.COOKIE_USER_PREFS_NAME, cookieValue ) } );
+        }
+        final Preferences prefs = new Preferences();
+        prefs.put( "SkinName", "PlainVanilla" );
+        Preferences.parseJSONPreferences( request, prefs );
+        return prefs;
+    }
+
+    /** https://issues.apache.org/jira/browse/JSPWIKI-1323 */
+    @Test
+    public void cookieOverridesTheDefaults() {
+        final Preferences prefs = parse( "%7B%22SkinName%22%3A%22Smart%22%2C%22Layout%22%3A%22fixed%22%7D" );
+        Assertions.assertEquals( "Smart", prefs.get( "SkinName" ) );
+        Assertions.assertEquals( "fixed", prefs.get( "Layout" ) );
+    }
+
+    /** https://issues.apache.org/jira/browse/JSPWIKI-1323 */
+    @Test
+    public void missingCookieKeepsTheDefaults() {
+        final Preferences prefs = parse( null );
+        Assertions.assertEquals( "PlainVanilla", prefs.get( "SkinName" ) );
+        Assertions.assertEquals( 1, prefs.size() );
+    }
+
+    /** https://issues.apache.org/jira/browse/JSPWIKI-1323 */
+    @Test
+    public void cookieThatIsNotAJsonObjectIsIgnored() {
+        for( final String value : new String[] { "%7Bnot-json", "%5B1%2C2%5D", "123", "text", "%22text%22", "null", "true", "%7B", "%7B%22a%22%3A" } ) {
+            final Preferences prefs = Assertions.assertDoesNotThrow( () -> parse( value ), value );
+            Assertions.assertEquals( "PlainVanilla", prefs.get( "SkinName" ), value );
+            Assertions.assertEquals( 1, prefs.size(), value );
+        }
+    }
+
+    /** https://issues.apache.org/jira/browse/JSPWIKI-1323 */
+    @Test
+    public void cookieWithBrokenPercentEncodingIsIgnored() {
+        for( final String value : new String[] { "%zz", "%7B%22a%22%3A%22b%22%7D%", "%E0%A4%A" } ) {
+            final Preferences prefs = Assertions.assertDoesNotThrow( () -> parse( value ), value );
+            Assertions.assertEquals( "PlainVanilla", prefs.get( "SkinName" ), value );
+        }
+    }
+
+    /** https://issues.apache.org/jira/browse/JSPWIKI-1323 */
+    @Test
+    public void valuesThatAreNotStringsAreKeptAsText() {
+        final Preferences prefs = parse( "%7B%22Sidebar%22%3Anull%2C%22Layout%22%3A%5B1%5D%2C%22Flag%22%3Atrue%7D" );
+        Assertions.assertEquals( "PlainVanilla", prefs.get( "SkinName" ) );
+        Assertions.assertFalse( prefs.containsKey( "Sidebar" ) );
+        Assertions.assertEquals( "[1.0]", prefs.get( "Layout" ) );
+        Assertions.assertEquals( "true", prefs.get( "Flag" ) );
+    }
 
     private HttpServletRequest requestWithCookie( final String json ) {
         final HttpServletRequest request = Mockito.mock( HttpServletRequest.class );