jcr-server: WebDAV lock tokens violate specs due to path-based genera…

jackrabbit-jcr-server/src/main/java/org/apache/jackrabbit/server/jcr/JCRWebdavServer.java

@@ -227,6 +227,10 @@
                 }
             }
 
+            if (session != null) {
+                session = checkSameUser(session, request);
+            }
+
             // no cached session present -> create new one.
             if (session == null) {
                 Session repSession = getRepositorySession(request);
@@ -242,7 +246,34 @@
             return session;
         }
 
-        /**
+        private DavSession checkSameUser(DavSession cached, WebdavRequest request)
+                throws DavException {
+            Session repSession = getRepositorySession(request);
+            String userId = repSession.getUserID();
+            String cachedUserId = null;
+            try {
+                Session cachedSession = DavSessionImpl.getRepositorySession(cached);
+                if (cachedSession != null) {
+                    cachedUserId = cachedSession.getUserID();
+                }
+            } catch (DavException e) {
+                // should not occur, since only DavSessionImpl instances
+                // are put into the cache.
+                log.error("Unexpected error: {}", e.getMessage(), e.getCause());
+            }
+            if (cachedUserId != null && cachedUserId.equals(userId)) {
+                // same user: reuse the cached session and release the
+                // session that was only needed to authenticate the request.
+                sessionProvider.releaseSession(repSession);
+                return cached;
+            } else {
+                log.warn("Request of user '{}' presented a token referencing a session of user '{}' - token ignored.", userId, cachedUserId);
+                DavSession session = new DavSessionImpl(repSession);
+                sessionMap.put(session, new HashSet<>());
+                return session;
+            }
+        }
+            /**
          * Add a references to the specified <code>DavSession</code>.
          *
          * @param session

jackrabbit-jcr-server/src/main/java/org/apache/jackrabbit/webdav/jcr/DefaultItemCollection.java

@@ -62,6 +62,7 @@
 import org.apache.jackrabbit.webdav.DavResourceLocator;
 import org.apache.jackrabbit.webdav.DavServletResponse;
 import org.apache.jackrabbit.webdav.MultiStatusResponse;
+import org.apache.jackrabbit.webdav.jcr.lock.LockTokenMapper;
 import org.apache.jackrabbit.webdav.jcr.property.JcrDavPropertyNameSet;
 import org.apache.jackrabbit.webdav.jcr.security.JcrUserPrivilegesProperty;
 import org.apache.jackrabbit.webdav.jcr.security.JcrSupportedPrivilegesProperty;
@@ -695,6 +696,10 @@ public ActiveLock lock(LockInfo reqLockInfo) throws DavException {
                 javax.jcr.lock.LockManager lockMgr = getRepositorySession().getWorkspace().getLockManager();
                 Lock jcrLock = lockMgr.lock((item).getPath(), reqLockInfo.isDeep(),
                         sessionScoped, timeout, reqLockInfo.getOwner());
+                if (sessionScoped) {
+                    // discard any mapping of a previous lock on this node
+                    LockTokenMapper.releaseSessionScopedToken(jcrLock.getNode().getIdentifier());
+                }
                 ActiveLock lock = new JcrActiveLock(jcrLock);
                  // add reference to DAVSession for this lock
                 getSession().addReference(lock.getToken());
@@ -767,8 +772,12 @@ public void unlock(String lockToken) throws DavException {
         ActiveLock lock = getWriteLock();
         if (lock != null && lockToken.equals(lock.getToken())) {
             try {
-                ((Node) item).unlock();
-                getSession().removeReference(lock.getToken());                
+                Node n = (Node) item;
+                n.unlock();
+                getSession().removeReference(lockToken);
+                // the lock is gone: discard its DAV token secret so it can
+                // not be replayed against a future lock on this node.
+                LockTokenMapper.releaseSessionScopedToken(n.getIdentifier());
             } catch (RepositoryException e) {
                 throw new JcrDavException(e);
             }

jackrabbit-jcr-server/src/main/java/org/apache/jackrabbit/webdav/jcr/lock/LockTokenMapper.java

@@ -19,15 +19,20 @@
 import javax.jcr.RepositoryException;
 import javax.jcr.lock.Lock;
 
+import org.apache.commons.collections4.map.LRUMap;
 import org.apache.jackrabbit.util.Text;
 
+import java.util.Collections;
+import java.util.Map;
+import java.util.UUID;
+
 /**
  * Maps between WebDAV lock tokens and JCR lock tokens.
  * <p>
  * The following notations are used:
  * 
  * <pre>
- * opaquelocktoken:SESSIONSCOPED:<em>NODEIDENTIFIER</em>
+ * urn:uuid:<em>UUID of mapping</em>
  * opaquelocktoken:OPENSCOPED:<em>JCRLOCKTOKEN</em>
  * </pre>
  * 
@@ -44,22 +49,34 @@ public class LockTokenMapper {
 
     private static final String OL = "opaquelocktoken:";
 
-    private static final String SESSIONSCOPED = "4403ef44-4124-11e1-b965-00059a3c7a00";
     private static final String OPENSCOPED = "dccce564-412e-11e1-b969-00059a3c7a00";
 
-    private static final String SESSPREFIX = OL + SESSIONSCOPED + ":";
+    private static final String SESSPREFIX = "urn:uuid:";
     private static final String OPENPREFIX = OL + OPENSCOPED + ":";
 
+    // map node identifiers to randomized session locks, 4k mappings should be enough for everyone
+    private static final Map<String, String> mappings = Collections.synchronizedMap(new LRUMap<>(4196));
+
+    private LockTokenMapper() {
+        /* This utility class should not be instantiated */
+    }
+
+    /**
+     * Generate a WebDAV lock token from a JCR {@link Lock}.
+     */
     public static String getDavLocktoken(Lock lock) throws RepositoryException {
         String jcrLockToken = lock.getLockToken();
 
         if (jcrLockToken == null) {
-            return SESSPREFIX + Text.escape(lock.getNode().getIdentifier());
+            return SESSPREFIX + Text.escape(getMapping(lock.getNode().getIdentifier()));
         } else {
             return OPENPREFIX + Text.escape(jcrLockToken);
         }
     }
 
+    /**
+     * Map from a WebDAV lock token to JCR lock token (requires an open scoped lock).
+     */
     public static String getJcrLockToken(String token) throws RepositoryException {
         if (token.startsWith(OPENPREFIX)) {
             return Text.unescape(token.substring(OPENPREFIX.length()));
@@ -68,7 +85,26 @@ public static String getJcrLockToken(String token) throws RepositoryException {
         }
     }
 
+    /**
+     * Discards the DAV lock token mapping recorded for the given node, if
+     * present. This is called when a lock is removed or newly created, so
+     * that the token of an earlier lock on the same node can never be
+     * reused against a later one.
+     *
+     * @param nodeIdentifier the identifier of the lock holding node.
+     */
+    public static void releaseSessionScopedToken(String nodeIdentifier) {
+        mappings.remove(nodeIdentifier);
+    }
+
     public static boolean isForSessionScopedLock(String token) {
         return token.startsWith(SESSPREFIX);
     }
+
+    private static String getMapping(String nodeIdentifier) {
+        return mappings.computeIfAbsent(
+                nodeIdentifier,
+                k -> UUID.randomUUID().toString()
+        );
+    }
 }

jackrabbit-jcr-server/src/test/java/org/apache/jackrabbit/webdav/jcr/LockTokenMappingTest.java

@@ -16,14 +16,16 @@
  */
 package org.apache.jackrabbit.webdav.jcr;
 
+import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.Method;
+import java.lang.reflect.Proxy;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.util.UUID;
 
 import javax.jcr.Node;
 import javax.jcr.RepositoryException;
 import javax.jcr.lock.Lock;
-import javax.jcr.lock.LockException;
 
 import org.apache.jackrabbit.webdav.jcr.lock.LockTokenMapper;
 
@@ -44,6 +46,42 @@ public void testOpenScopedFancy() throws RepositoryException, URISyntaxException
         testRoundtrip("\n\u00c4 \u20ac");
     }
 
+    // the DAV token of a session-scoped lock must not be derivable from
+    // the (world-readable) node identifier
+    public void testSessionScopedTokenIsNotDerivedFromNodeId()
+            throws RepositoryException, URISyntaxException {
+        String nodeId = UUID.randomUUID().toString();
+        Lock l = new TestLock(nodeWithIdentifier(nodeId), true);
+        String davtoken = LockTokenMapper.getDavLocktoken(l);
+
+        assertNotNull(davtoken);
+        assertTrue(LockTokenMapper.isForSessionScopedLock(davtoken));
+        assertTrue("token must not contain the node identifier",
+                davtoken.indexOf(nodeId) < 0);
+
+        // valid URI?
+        URI u = new URI(davtoken);
+        assertTrue("lock token must be absolute URI", u.isAbsolute());
+        assertEquals("lock token URI must be all-ASCII", u.toASCIIString(), u.toString());
+
+        // stable for the same lock...
+        assertEquals(davtoken, LockTokenMapper.getDavLocktoken(l));
+
+        // ...but not reused once the lock is gone
+        LockTokenMapper.releaseSessionScopedToken(nodeId);
+        String next = LockTokenMapper.getDavLocktoken(l);
+        assertFalse("a new lock on the same node must get a fresh token",
+                davtoken.equals(next));
+    }
+
+    // two session-scoped locks on different nodes must get distinct tokens
+    public void testSessionScopedTokensAreDistinct() throws RepositoryException {
+        Lock l1 = new TestLock(nodeWithIdentifier(UUID.randomUUID().toString()), true);
+        Lock l2 = new TestLock(nodeWithIdentifier(UUID.randomUUID().toString()), true);
+        assertFalse(LockTokenMapper.getDavLocktoken(l1).equals(
+                LockTokenMapper.getDavLocktoken(l2)));
+    }
+
     private void testRoundtrip(String token) throws RepositoryException, URISyntaxException {
 
         Lock l = new TestLock(token);
@@ -58,15 +96,46 @@ private void testRoundtrip(String token) throws RepositoryException, URISyntaxEx
         assertEquals(jcrtoken, l.getLockToken());
     }
 
+    /**
+     * Returns a minimal {@link Node} that only supports
+     * {@link Node#getIdentifier()}.
+     */
+    private static Node nodeWithIdentifier(final String id) {
+        return (Node) Proxy.newProxyInstance(
+                LockTokenMappingTest.class.getClassLoader(),
+                new Class<?>[] {Node.class},
+                new InvocationHandler() {
+                    public Object invoke(Object proxy, Method method, Object[] args) {
+                        if ("getIdentifier".equals(method.getName())) {
+                            return id;
+                        }
+                        throw new UnsupportedOperationException(method.getName());
+                    }
+                });
+    }
+
     /**
      * Minimal Lock impl for tests above
      */
     private static class TestLock implements Lock {
 
         private final String token;
+        private final Node node;
+        private final boolean sessionScoped;
+        private final boolean lockOwningSession;
 
         public TestLock(String token) {
             this.token = token;
+            this.node = null;
+            this.sessionScoped = false;
+            this.lockOwningSession = false;
+        }
+
+        public TestLock(Node node, boolean lockOwningSession) {
+            this.token = null;
+            this.node = node;
+            this.sessionScoped = true;
+            this.lockOwningSession = lockOwningSession;
         }
 
         public String getLockOwner() {
@@ -78,30 +147,30 @@ public boolean isDeep() {
         }
 
         public Node getNode() {
-            return null;
+            return node;
         }
 
         public String getLockToken() {
             return token;
         }
 
-        public long getSecondsRemaining() throws RepositoryException {
+        public long getSecondsRemaining() {
             return 0;
         }
 
-        public boolean isLive() throws RepositoryException {
+        public boolean isLive() {
             return false;
         }
 
         public boolean isSessionScoped() {
-            return false;
+            return sessionScoped;
         }
 
         public boolean isLockOwningSession() {
-            return false;
+            return lockOwningSession;
         }
 
-        public void refresh() throws LockException, RepositoryException {
+        public void refresh() {
         }
     }
-}
+}