[bugfix] Avoid storage corruption when copy/move Collection to a sub-…

…Collection of itself by raising an exception
eXist-db · Oct 7, 2016 · 35bbf57 · 35bbf57
1 parent b952884
commit 35bbf57
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 2 deletions.
diff --git a/src/org/exist/storage/NativeBroker.java b/src/org/exist/storage/NativeBroker.java
@@ -1056,10 +1056,13 @@ public void copyCollection(final Txn transaction, final Collection collection, f
         final XmldbURI dstURI = destination.getURI().append(newName);
 
         if(collection.getURI().equals(dstURI)) {
-            throw new PermissionDeniedException("Cannot move collection to itself '" + collection.getURI() + "'.");
+            throw new PermissionDeniedException("Cannot copy collection to itself '" + collection.getURI() + "'.");
         }
         if(collection.getId() == destination.getId()) {
-            throw new PermissionDeniedException("Cannot move collection to itself '" + collection.getURI() + "'.");
+            throw new PermissionDeniedException("Cannot copy collection to itself '" + collection.getURI() + "'.");
+        }
+        if(isSubCollection(collection, destination)) {
+            throw new PermissionDeniedException("Cannot copy collection '" + collection.getURI() + "' to it child collection '"+destination.getURI()+"'.");
         }
 
         final CollectionCache collectionsCache = pool.getCollectionsCache();
@@ -1069,6 +1072,11 @@ public void copyCollection(final Txn transaction, final Collection collection, f
                 pool.getProcessMonitor().startJob(ProcessMonitor.ACTION_COPY_COLLECTION, collection.getURI());
                 lock.acquire(Lock.WRITE_LOCK);
 
+                //recheck here because now under 'synchronized(collectionsCache)'
+                if(isSubCollection(collection, destination)) {
+                    throw new PermissionDeniedException("Cannot copy collection '" + collection.getURI() + "' to it child collection '"+destination.getURI()+"'.");
+                }
+
                 final XmldbURI parentName = collection.getParentURI();
                 final Collection parent = parentName == null ? collection : getCollection(parentName);
 
@@ -1205,6 +1213,10 @@ private void copyModeAndAcl(final Permission srcPermission, final Permission des
         }
     }
 
+    private boolean isSubCollection(final Collection col, final Collection sub) {
+        return sub.getURI().startsWith(col.getURI());
+    }
+
     @Override
     public void moveCollection(final Txn transaction, final Collection collection, final Collection destination, final XmldbURI newName) throws PermissionDeniedException, LockException, IOException, TriggerException {
 
@@ -1225,6 +1237,9 @@ public void moveCollection(final Txn transaction, final Collection collection, f
         if(collection.getURI().equals(XmldbURI.ROOT_COLLECTION_URI)) {
             throw new PermissionDeniedException("Cannot move the db root collection");
         }
+        if(isSubCollection(collection, destination)) {
+            throw new PermissionDeniedException("Cannot move collection '" + collection.getURI() + "' to it child collection '"+destination.getURI()+"'.");
+        }
 
         final XmldbURI parentName = collection.getParentURI();
         final Collection parent = parentName == null ? collection : getCollection(parentName);
@@ -1339,6 +1354,11 @@ private void moveCollectionRecursive(final Txn transaction, final CollectionTrig
             final XmldbURI srcURI = collection.getURI();
             final XmldbURI dstURI = destination.getURI().append(newName);
 
+            //recheck here because now under 'synchronized(collectionsCache)'
+            if(isSubCollection(collection, destination)) {
+                throw new PermissionDeniedException("Cannot move collection '" + srcURI + "' to it child collection '"+dstURI+"'.");
+            }
+
             if(fireTrigger) {
                 trigger.beforeMoveCollection(this, transaction, collection, dstURI);
             }

diff --git a/test/src/org/exist/storage/CopyCollectionTest.java b/test/src/org/exist/storage/CopyCollectionTest.java
@@ -75,6 +75,29 @@ public void storeAndReadXmldb() throws IllegalAccessException, DatabaseConfigura
         xmldbRead();
     }
 
+    @Test(expected = PermissionDeniedException.class)
+    public void copyToSubCollection() throws Exception {
+        final BrokerPool pool = startDB();
+
+        final TransactionManager transact = pool.getTransactionManager();
+
+        try(final DBBroker broker = pool.get(Optional.of(pool.getSecurityManager().getSystemSubject()));
+            final Txn transaction = transact.beginTransaction()) {
+
+            final Collection src = broker.getOrCreateCollection(transaction, TestConstants.TEST_COLLECTION_URI);
+            broker.saveCollection(transaction, src);
+
+            final Collection dst = broker.getOrCreateCollection(transaction, TestConstants.TEST_COLLECTION_URI2);
+            broker.saveCollection(transaction, dst);
+
+            broker.copyCollection(transaction, src, dst, src.getURI().lastSegment());
+
+            fail("expect PermissionDeniedException: Cannot copy collection '/db/test' to it child collection '/db/test/test2'");
+
+            transaction.commit();
+        }
+    }
+
     private void store() throws IllegalAccessException, DatabaseConfigurationException, InstantiationException, XMLDBException, EXistException, ClassNotFoundException, PermissionDeniedException, IOException, SAXException, LockException {
         BrokerPool.FORCE_CORRUPTION = true;
         final BrokerPool pool = startDB();

diff --git a/test/src/org/exist/storage/MoveCollectionTest.java b/test/src/org/exist/storage/MoveCollectionTest.java
@@ -51,9 +51,34 @@
 
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
 
 public class MoveCollectionTest {
 
+    @Test(expected = PermissionDeniedException.class)
+    public void moveToSubCollection() throws Exception {
+        final BrokerPool pool = startDB();
+        final TransactionManager transact = pool.getTransactionManager();
+
+        try(final DBBroker broker = pool.get(Optional.of(pool.getSecurityManager().getSystemSubject()));
+            final Txn transaction = transact.beginTransaction()) {
+
+            Collection src = broker.getOrCreateCollection(transaction, TestConstants.TEST_COLLECTION_URI);
+            assertNotNull(src);
+            broker.saveCollection(transaction, src);
+
+            Collection dst = broker.getOrCreateCollection(transaction, TestConstants.TEST_COLLECTION_URI2);
+            assertNotNull(dst);
+            broker.saveCollection(transaction, dst);
+
+            broker.moveCollection(transaction, src, dst, src.getURI().lastSegment());
+
+            fail("expect PermissionDeniedException: Cannot move collection '/db/test' to it child collection '/db/test/test2'");
+
+            transact.commit(transaction);
+        }
+    }
+
     @Test
     public void store() throws IllegalAccessException, DatabaseConfigurationException, InstantiationException, ClassNotFoundException, XMLDBException, EXistException, PermissionDeniedException, IOException, SAXException, LockException {
         BrokerPool.FORCE_CORRUPTION = true;