JSPWIKI-1286 see jira for details

jspwiki-main/src/main/java/org/apache/wiki/auth/permissions/PermissionFactory.java

@@ -38,10 +38,10 @@ public final class PermissionFactory
     private PermissionFactory() {}
 
     /**
-     *  This is a WeakHashMap<Integer,PagePermission>, which stores the
+     *  This is a WeakHashMap<String,PagePermission>, which stores the
      *  cached page permissions.
      */
-    private static final WeakHashMap<Integer, PagePermission> c_cache = new WeakHashMap<>();
+    private static final WeakHashMap<String, PagePermission> c_cache = new WeakHashMap<>();
 
     /**
      *  Get a permission object for a WikiPage and a set of actions.
@@ -79,15 +79,13 @@ private static PagePermission getPagePermission( final String wiki, String page,
     {
         PagePermission perm;
         //
-        //  Since this is pretty speed-critical, we try to avoid the StringBuffer creation
-        //  overhead by XORring the hashcodes.  However, if page name length > 32 characters,
-        //  this might result in two same hashCodes.
-        //  FIXME: Make this work for page-name lengths > 32 characters (use the alt implementation
-        //         if page.length() > 32?)
-        // Alternative implementation below, but it does create an extra StringBuffer.
-        //String         key = wiki+":"+page+":"+actions;
-
-        final Integer key = wiki.hashCode() ^ page.hashCode() ^ actions.hashCode();
+        //  The cache key must uniquely identify the (wiki, page, actions) triple.  The previous
+        //  XOR-of-hashCodes key allowed two different pages to collide onto the same 32-bit
+        //  value, so an access check for one page could silently be evaluated against another
+        //  page's cached permission (and hence the wrong ACL).  The concatenated string key is
+        //  collision-free for any attacker-choosable page name.
+        //
+        final String key = wiki + ":" + page + ":" + actions;
 
         //
         //  It's fine if two threads update the cache, since the objects mean the same

jspwiki-main/src/test/java/org/apache/wiki/auth/permissions/PermissionFactoryTest.java

@@ -0,0 +1,43 @@
+/*
+    Licensed to the Apache Software Foundation (ASF) under one
+    or more contributor license agreements.  See the NOTICE file
+    distributed with this work for additional information
+    regarding copyright ownership.  The ASF licenses this file
+    to you 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.apache.wiki.auth.permissions;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class PermissionFactoryTest
+{
+
+    @Test
+    public void testHashCollidingPageNamesGetDistinctPermissions()
+    {
+        // "Aa" and "BB" have identical String hashCodes; the old XOR-of-hashCodes cache key
+        // returned the first page's cached permission for the second page, so ACL checks
+        // could be evaluated against the wrong page.
+        Assertions.assertEquals( "Aa".hashCode(), "BB".hashCode() );
+
+        final PagePermission p1 = PermissionFactory.getPagePermission( "Aa", "view" );
+        final PagePermission p2 = PermissionFactory.getPagePermission( "BB", "view" );
+
+        Assertions.assertEquals( "Aa", p1.getPage() );
+        Assertions.assertEquals( "BB", p2.getPage() );
+        Assertions.assertNotSame( p1, p2 );
+    }
+
+}