Validate names of elements added via the API

Change-Id: I052f8ca6a8788665acb1bf87456f7cc67d64c365

2 files changed, 74 insertions, 0 deletions

diff --git a/configmgr/qa/unit/test.cxx b/configmgr/qa/unit/test.cxx
index fd004776105a..a6954ada4f55 100644
--- a/configmgr/qa/unit/test.cxx
+++ b/configmgr/qa/unit/test.cxx
@@ -28,11 +28,13 @@
 #include <com/sun/star/beans/XPropertyState.hpp>
 #include <com/sun/star/configuration/theDefaultProvider.hpp>
 #include <com/sun/star/container/XHierarchicalNameAccess.hpp>
+#include <com/sun/star/container/XNameContainer.hpp>
 #include <com/sun/star/container/XNameReplace.hpp>
 #include <com/sun/star/container/XNamed.hpp>
 #include <com/sun/star/lang/EventObject.hpp>
 #include <com/sun/star/lang/XComponent.hpp>
 #include <com/sun/star/lang/XMultiServiceFactory.hpp>
+#include <com/sun/star/lang/XSingleServiceFactory.hpp>
 #include <com/sun/star/uno/Any.hxx>
 #include <com/sun/star/uno/Reference.hxx>
 #include <com/sun/star/uno/RuntimeException.hpp>
@@ -66,6 +68,7 @@ public:
     void testKeySet();
     void testKeyReset();
     void testSetSetMemberName();
+    void testInsertSetMember();
     void testReadCommands();
 #if 0
     void testThreads();
@@ -93,6 +96,7 @@ public:
     CPPUNIT_TEST(testKeySet);
     CPPUNIT_TEST(testKeyReset);
     CPPUNIT_TEST(testSetSetMemberName);
+    CPPUNIT_TEST(testInsertSetMember);
     CPPUNIT_TEST(testReadCommands);
 #if 0
     CPPUNIT_TEST(testThreads);
@@ -286,6 +290,39 @@ void Test::testSetSetMemberName()
     CPPUNIT_ASSERT( s == "Fontwork Gallery..." );
 }
 
+void Test::testInsertSetMember() {
+    css::uno::Reference<css::container::XNameContainer> access(
+        createUpdateAccess(
+            "/org.openoffice.Office.UI.GenericCommands/UserInterface/Commands"),
+        css::uno::UNO_QUERY_THROW);
+    css::uno::Reference<css::uno::XInterface> member;
+    member.set(
+        css::uno::Reference<css::lang::XSingleServiceFactory>(
+            access, css::uno::UNO_QUERY_THROW)->createInstance());
+    CPPUNIT_ASSERT(member.is());
+    access->insertByName("A", css::uno::makeAny(member));
+    member.set(
+        css::uno::Reference<css::lang::XSingleServiceFactory>(
+            access, css::uno::UNO_QUERY_THROW)->createInstance());
+    CPPUNIT_ASSERT(member.is());
+    try {
+        access->insertByName("", css::uno::makeAny(member));
+        CPPUNIT_FAIL("expected IllegalArgumentException");
+    } catch (css::lang::IllegalArgumentException &) {}
+    try {
+        access->insertByName("\x01", css::uno::makeAny(member));
+        CPPUNIT_FAIL("expected IllegalArgumentException");
+    } catch (css::lang::IllegalArgumentException &) {}
+    try {
+        access->insertByName("a/b", css::uno::makeAny(member));
+        CPPUNIT_FAIL("expected IllegalArgumentException");
+    } catch (css::lang::IllegalArgumentException &) {}
+    css::uno::Reference<css::util::XChangesBatch>(
+        access, css::uno::UNO_QUERY_THROW)->commitChanges();
+    css::uno::Reference<css::lang::XComponent>(
+        access, css::uno::UNO_QUERY_THROW)->dispose();
+}
+
 void Test::testReadCommands()
 {
     css::uno::Reference< css::container::XNameAccess > access(
diff --git a/configmgr/source/access.cxx b/configmgr/source/access.cxx
index 41d86e78228b..b8fadbcc62f8 100644
--- a/configmgr/source/access.cxx
+++ b/configmgr/source/access.cxx
@@ -76,6 +76,7 @@
 #include <cppuhelper/weak.hxx>
 #include <osl/interlck.h>
 #include <osl/mutex.hxx>
+#include <rtl/character.hxx>
 #include <rtl/ref.hxx>
 #include <rtl/ustrbuf.hxx>
 #include <rtl/ustring.h>
@@ -103,6 +104,26 @@
 
 namespace configmgr {
 
+namespace {
+
+// Conservatively forbid what is either not an XML Char (including lone
+// surrogates, even though they should not appear in well-formed UNO OUString
+// instances anyway), or is a slash (as it causes problems in path syntax):
+bool isValidName(OUString const & name) {
+    for (sal_Int32 i = 0; i != name.getLength();) {
+        sal_uInt32 c = name.iterateCodePoints(&i);
+        if ((c < 0x20 && !(c == 0x09 || c == 0x0A || c == 0x0D))
+            || rtl::isHighSurrogate(c) || rtl::isLowSurrogate(c) || c == 0xFFFE
+            || c == 0xFFFF || c == '/')
+        {
+            return false;
+        }
+    }
+    return !name.isEmpty();
+}
+
+}
+
 oslInterlockedCount Access::acquireCounting() {
     return osl_atomic_increment(&m_refCount);
 }
@@ -648,6 +669,10 @@ void Access::setName(OUString const & aName)
                         if (node->getMandatory() == Data::NO_LAYER &&
                             !(other.is() && other->isFinalized()))
                         {
+                            if (!isValidName(aName)) {
+                                throw css::uno::RuntimeException(
+                                    "invalid element name " + aName);
+                            }
                             rtl::Reference< RootAccess > root(getRootAccess());
                             rtl::Reference< ChildAccess > childAccess(
                                 static_cast< ChildAccess * >(this));
@@ -1163,10 +1188,18 @@ void Access::insertByName(
         Modifications localMods;
         switch (getNode()->kind()) {
         case Node::KIND_LOCALIZED_PROPERTY:
+            if (!isValidName(aName)) {
+                throw css::lang::IllegalArgumentException(
+                    aName, static_cast<cppu::OWeakObject *>(this), 0);
+            }
             insertLocalizedValueChild(aName, aElement, &localMods);
             break;
         case Node::KIND_GROUP:
             {
+                if (!isValidName(aName)) {
+                    throw css::lang::IllegalArgumentException(
+                        aName, static_cast<cppu::OWeakObject *>(this), 0);
+                }
                 checkValue(aElement, TYPE_ANY, true);
                 rtl::Reference< ChildAccess > child(
                     new ChildAccess(
@@ -1179,6 +1212,10 @@ void Access::insertByName(
             break;
         case Node::KIND_SET:
             {
+                if (!isValidName(aName)) {
+                    throw css::lang::IllegalArgumentException(
+                        aName, static_cast<cppu::OWeakObject *>(this), 0);
+                }
                 rtl::Reference< ChildAccess > freeAcc(
                     getFreeSetMember(aElement));
                 freeAcc->bind(getRootAccess(), this, aName); // must not throw