tdf#173160 Maintain order of arguments to macro

The arguments for a macro when invoked by the dispatch provider get
converted from a set of PropertyValues to a sequence of Anys containing
just the values. Since the commit below, the TransformItems step was
changed to store the PropertyValues in an unordered_map which makes the
order of the values unpredictable. If the dispatch provider is used to
execute a macro then the order of the arguments becomes important
because the macro has no other way to determine which argument
represents which property. In order to maintain the previous behaviour,
the dispatch method now reorders the arguments so that they have the
same order as before. The previous order was determined by the order the
properties were handled in TransformItems.

commit e8d73d5307
Author: Mike Kaganski <mike.kaganski@collabora.com>
Date:   Wed Dec 3 10:45:35 2025 +0500

    Let TransformItems return comphelper::SequenceAsHashMap

    Simplifies its usage in all places where the result needs to be
    modified.

    Change-Id: I37f8d99827e0f1cb2b6e4f4f7735e26fb87f6703
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/195162

Change-Id: Iacf64650842eb62c0a2bd266784aa9c7accf45bd
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/209231
Reviewed-by: Neil Roberts <nroberts@libreoffice.org>
Tested-by: Jenkins

scripting/CppunitTest_scripting_scripthandler.mk

@@ -0,0 +1,43 @@
+# -*- Mode: makefile-gmake; tab-width: 4; indent-tabs-mode: t; fill-column: 100 -*-
+#
+# This file is part of the LibreOffice project.
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+
+$(eval $(call gb_CppunitTest_CppunitTest,scripting_scripthandler))
+
+$(eval $(call gb_CppunitTest_add_exception_objects,scripting_scripthandler, \
+    scripting/qa/cppunit/scripthandler/ScriptHandlerTest \
+))
+
+$(eval $(call gb_CppunitTest_use_libraries,scripting_scripthandler, \
+    cppu \
+    cppuhelper \
+    sal \
+    subsequenttest \
+    test \
+    unotest \
+    utl \
+    tl \
+    vcl \
+))
+
+$(eval $(call gb_CppunitTest_use_externals,scripting_scripthandler,\
+    boost_headers \
+    libxml2 \
+))
+
+$(eval $(call gb_CppunitTest_use_api,scripting_scripthandler,\
+    udkapi \
+    offapi \
+))
+
+$(eval $(call gb_CppunitTest_use_ure,scripting_scripthandler))
+$(eval $(call gb_CppunitTest_use_vcl,scripting_scripthandler))
+$(eval $(call gb_CppunitTest_use_rdb,scripting_scripthandler,services))
+$(eval $(call gb_CppunitTest_use_configuration,scripting_scripthandler))
+
+# vim: set noet sw=4 ts=4:

scripting/Module_scripting.mk

@@ -51,6 +51,7 @@ $(eval $(call gb_Module_add_check_targets,scripting,\
     $(if $(ENABLE_QUICKJS),CppunitTest_scripting_jsprov) \
     $(if $(ENABLE_SCRIPTING_BEANSHELL),CppunitTest_scripting_beanshell) \
     $(if $(DISABLE_PYTHON),,CppunitTest_scripting_pythonscript) \
+    CppunitTest_scripting_scripthandler \
 ))
 
 $(eval $(call gb_Module_add_subsequentcheck_targets,scripting,\

scripting/qa/cppunit/scripthandler/ScriptHandlerTest.cxx

@@ -0,0 +1,217 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+#include <sal/config.h>
+
+#include <comphelper/propertyvalue.hxx>
+#include <comphelper/scopeguard.hxx>
+#include <cppuhelper/implbase.hxx>
+#include <test/unoapi_test.hxx>
+#include <vcl/scheduler.hxx>
+
+#include <com/sun/star/container/XSet.hpp>
+#include <com/sun/star/document/MacroExecMode.hpp>
+#include <com/sun/star/document/UpdateDocMode.hpp>
+#include <com/sun/star/lang/XServiceInfo.hpp>
+#include <com/sun/star/lang/XSingleComponentFactory.hpp>
+#include <com/sun/star/script/provider/XScriptProvider.hpp>
+#include <com/sun/star/util/URLTransformer.hpp>
+
+namespace
+{
+constexpr OUString DUMMY_PROVIDER_SERVICE
+    = u"com.sun.star.script.provider.ScriptProviderForDummy"_ustr;
+constexpr OUString LANGUAGE_PROVIDER_SERVICE
+    = u"com.sun.star.script.provider.LanguageScriptProvider"_ustr;
+constexpr OUString SCRIPT_URL
+    = u"vnd.sun.star.script:Whatever?language=Dummy&location=document"_ustr;
+
+// A Dummy script provider so that we can execute our own function instead of running a script
+class DummyProvider
+    : public cppu::WeakImplHelper<css::script::provider::XScriptProvider,
+                                  css::script::provider::XScript, css::lang::XServiceInfo,
+                                  css::lang::XSingleComponentFactory>
+{
+public:
+    // XScriptProvider
+    css::uno::Reference<css::script::provider::XScript>
+        SAL_CALL getScript(const OUString& sScriptURI) override;
+
+    // XScript
+    css::uno::Any SAL_CALL invoke(const css::uno::Sequence<css::uno::Any>& aParams,
+                                  css::uno::Sequence<sal_Int16>& aOutParamIndex,
+                                  css::uno::Sequence<css::uno::Any>& aOutParam) override;
+
+    // XServiceInfo
+    OUString SAL_CALL getImplementationName() override;
+    sal_Bool SAL_CALL supportsService(const OUString& sServiceName) override;
+    css::uno::Sequence<OUString> SAL_CALL getSupportedServiceNames() override;
+
+    // XSingleComponentFactory
+    css::uno::Reference<css::uno::XInterface> SAL_CALL createInstanceWithContext(
+        const css::uno::Reference<css::uno::XComponentContext>& xContext) override;
+    css::uno::Reference<css::uno::XInterface> SAL_CALL createInstanceWithArgumentsAndContext(
+        const css::uno::Sequence<css::uno::Any>& rArguments,
+        const css::uno::Reference<css::uno::XComponentContext>& xContext) override;
+
+    bool m_bInvoked = false;
+};
+
+css::uno::Reference<css::script::provider::XScript>
+    SAL_CALL DummyProvider::getScript(const OUString&)
+{
+    return this;
+}
+
+css::uno::Any SAL_CALL DummyProvider::invoke(const css::uno::Sequence<css::uno::Any>& aParams,
+                                             css::uno::Sequence<sal_Int16>&,
+                                             css::uno::Sequence<css::uno::Any>&)
+{
+    CPPUNIT_ASSERT_GREATEREQUAL(sal_Int32(6), aParams.getLength());
+
+    // First argument should be the URL
+    OUString sUrl;
+    CPPUNIT_ASSERT(aParams[0] >>= sUrl);
+    CPPUNIT_ASSERT_EQUAL(SCRIPT_URL, sUrl);
+
+    // Second argument should be an XInteractionHandler
+    css::uno::Reference<css::task::XInteractionHandler> xInteractionHandler;
+    CPPUNIT_ASSERT(aParams[1] >>= xInteractionHandler);
+
+    // Third argument is the OpenNewView value
+    bool bOpenNewView = true;
+    CPPUNIT_ASSERT(aParams[2] >>= bOpenNewView);
+    CPPUNIT_ASSERT(!bOpenNewView);
+
+    // Fourth argument is the frame name
+    OUString sFrameName;
+    CPPUNIT_ASSERT(aParams[3] >>= sFrameName);
+    CPPUNIT_ASSERT_EQUAL(u"_self"_ustr, sFrameName);
+
+    // Fifth argument is the macro execution mode
+    sal_Int16 nMacroExecutionMode = 0;
+    CPPUNIT_ASSERT(aParams[4] >>= nMacroExecutionMode);
+    CPPUNIT_ASSERT_EQUAL(css::document::MacroExecMode::USE_CONFIG, nMacroExecutionMode);
+
+    // Sixth argument is the update doc mode
+    sal_Int16 nUpdateDocMode = 0;
+    CPPUNIT_ASSERT(aParams[5] >>= nUpdateDocMode);
+    CPPUNIT_ASSERT_EQUAL(css::document::UpdateDocMode::ACCORDING_TO_CONFIG, nUpdateDocMode);
+
+    m_bInvoked = true;
+
+    return css::uno::Any();
+}
+
+OUString SAL_CALL DummyProvider::getImplementationName()
+{
+    return u"com.sun.star.test.ScriptProviderForDummy"_ustr;
+}
+
+sal_Bool SAL_CALL DummyProvider::supportsService(const OUString& sServiceName)
+{
+    return sServiceName == DUMMY_PROVIDER_SERVICE || sServiceName == LANGUAGE_PROVIDER_SERVICE;
+}
+
+css::uno::Sequence<OUString> SAL_CALL DummyProvider::getSupportedServiceNames()
+{
+    return css::uno::Sequence<OUString>{ DUMMY_PROVIDER_SERVICE, LANGUAGE_PROVIDER_SERVICE };
+}
+
+css::uno::Reference<css::uno::XInterface> SAL_CALL
+DummyProvider::createInstanceWithContext(const css::uno::Reference<css::uno::XComponentContext>&)
+{
+    return static_cast<css::script::provider::XScriptProvider*>(this);
+}
+
+css::uno::Reference<css::uno::XInterface>
+    SAL_CALL DummyProvider::createInstanceWithArgumentsAndContext(
+        const css::uno::Sequence<css::uno::Any>&,
+        const css::uno::Reference<css::uno::XComponentContext>&)
+{
+    return static_cast<css::script::provider::XScriptProvider*>(this);
+}
+
+class ScriptHandlerTest : public UnoApiTest
+{
+public:
+    ScriptHandlerTest()
+        : UnoApiTest(u"/scripting/qa/extras"_ustr)
+    {
+    }
+
+private:
+    void testDispatchArguments();
+
+    CPPUNIT_TEST_SUITE(ScriptHandlerTest);
+    CPPUNIT_TEST(testDispatchArguments);
+    CPPUNIT_TEST_SUITE_END();
+};
+
+void ScriptHandlerTest::testDispatchArguments()
+{
+    // Tests that the first argument to a macro when invoked via a dispatch URL is the URL itself.
+    // See tdf#173160.
+    rtl::Reference<DummyProvider> xDummyProvider = new DummyProvider;
+
+    // Temporarily register our dummy provider with the service manager so that the master script
+    // provider can find it
+    css::uno::Reference<css::container::XSet> xServiceSet(m_xFactory, css::uno::UNO_QUERY_THROW);
+    css::uno::Reference<css::lang::XServiceInfo> xService(xDummyProvider);
+    xServiceSet->insert(css::uno::Any(xService));
+    comphelper::ScopeGuard g(
+        [&xServiceSet, &xService] { xServiceSet->remove(css::uno::Any(xService)); });
+
+    css::uno::Sequence<css::beans::PropertyValue> aPropertyValues = {
+        comphelper::makePropertyValue(u"FileName"_ustr, SCRIPT_URL),
+        comphelper::makePropertyValue(u"InteractionHandler"_ustr,
+                                      css::uno::Reference<css::task::XInteractionHandler>()),
+        comphelper::makePropertyValue(u"OpenNewView"_ustr, false),
+        comphelper::makePropertyValue(u"FrameName"_ustr, u"_self"_ustr),
+        comphelper::makePropertyValue(u"MacroExecutionMode"_ustr,
+                                      css::document::MacroExecMode::USE_CONFIG),
+        comphelper::makePropertyValue(u"UpdateDocMode"_ustr,
+                                      css::document::UpdateDocMode::ACCORDING_TO_CONFIG),
+        comphelper::makePropertyValue(u"SynchronMode"_ustr, false),
+    };
+
+    loadFromURL(u"private:factory/swriter"_ustr);
+
+    // Dispatch the .uno:Open command. We can’t use XDispatchHelper for this because it will set
+    // SynchronMode to true and there isn’t a synchronous handler for it.
+
+    css::uno::Reference<css::frame::XModel> xModel(mxComponent, css::uno::UNO_QUERY_THROW);
+    css::uno::Reference<css::frame::XDispatchProvider> xDispatchProvider(
+        xModel->getCurrentController(), css::uno::UNO_QUERY_THROW);
+
+    css::uno::Reference<css::util::XURLTransformer> xParser
+        = css::util::URLTransformer::create(m_xContext);
+    css::util::URL aURL;
+    aURL.Complete = ".uno:Open";
+    xParser->parseStrict(aURL);
+
+    css::uno::Reference<css::frame::XDispatch> xDispatch
+        = xDispatchProvider->queryDispatch(aURL, OUString(), 0);
+    CPPUNIT_ASSERT(xDispatch.is());
+
+    xDispatch->dispatch(aURL, aPropertyValues);
+
+    Scheduler::ProcessEventsToIdle();
+
+    // Make sure the script was actually invoked
+    CPPUNIT_ASSERT(xDummyProvider->m_bInvoked);
+}
+
+CPPUNIT_TEST_SUITE_REGISTRATION(ScriptHandlerTest);
+
+} // namespace
+
+CPPUNIT_PLUGIN_IMPLEMENT();
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */

scripting/source/protocolhandler/scripthandler.cxx

@@ -84,6 +84,71 @@ void SAL_CALL ScriptProtocolHandler::initialize(
     m_bInitialised = true;
 }
 
+namespace
+{
+Sequence<Any> lcl_filterAndOrderScriptArgs(const Sequence<PropertyValue>& rArgs)
+{
+    // Copy the Sequence to a local array so we can modify the order
+    Sequence<PropertyValue> aArgs = rArgs;
+    sal_Int32 nSize = aArgs.getLength();
+    PropertyValue* pArgs = aArgs.getArray();
+
+    // For some reason the script gets passed only the values from the PropertyValue array and not
+    // the names so the only way for it to distinguish them is by the order. That means this order
+    // is set in stone. See tdf#173160.
+    static const std::array<OUString, 6> aPropertyOrder = {
+        u"URL"_ustr,       u"InteractionHandler"_ustr, u"OpenNewView"_ustr,
+        u"FrameName"_ustr, u"MacroExecutionMode"_ustr, u"UpdateDocMode"_ustr,
+    };
+    sal_Int32 nUsedArgs = 0;
+
+    // Reorder the property values according to the preset order
+    for (const auto& sPropertyName : aPropertyOrder)
+    {
+        for (sal_Int32 i = nUsedArgs; i < nSize; ++i)
+        {
+            if (sPropertyName == pArgs[i].Name)
+            {
+                // Swap this property with the one in the place we want it to be
+                if (i != nUsedArgs)
+                    std::swap(pArgs[i], pArgs[nUsedArgs]);
+                ++nUsedArgs;
+                break;
+            }
+        }
+    }
+
+    // Sometimes we get a propertyval with name = "Referer" or "SynchronMode". These are not actual
+    // arguments to be passed to script, but flags describing the call, so ignore. Who thought that
+    // passing such "meta-arguments" mixed in with real arguments was a good idea?
+    while (nUsedArgs < nSize)
+    {
+        if (pArgs[nUsedArgs].Name == "Referer" || pArgs[nUsedArgs].Name == "SynchronMode")
+        {
+            // Move the last argument over the place of this one and then decrease the number of
+            // arguments that we’ll use so that the moved one will get ignored
+            if (nUsedArgs + 1 < nSize)
+                pArgs[nUsedArgs] = std::move(pArgs[nSize - 1]);
+            --nSize;
+        }
+        else
+        {
+            // Otherwise keep the argument
+            ++nUsedArgs;
+        }
+    }
+
+    Sequence<Any> aValues(nUsedArgs);
+    Any* pValues = aValues.getArray();
+
+    // Move only the values into the final array
+    for (sal_Int32 i = 0; i < nUsedArgs; ++i)
+        pValues[i] = std::move(pArgs[i].Value);
+
+    return aValues;
+}
+}
+
 Reference< XDispatch > SAL_CALL ScriptProtocolHandler::queryDispatch(
     const URL& aURL, const OUString&, sal_Int32 )
 {
@@ -178,29 +243,10 @@ void SAL_CALL ScriptProtocolHandler::dispatchWithNotification(
                 "ScriptProtocolHandler::dispatchWithNotification: validate xFunc - unable to obtain XScript interface" );
 
 
-            Sequence< Any > inArgs;
+            Sequence< Any > inArgs = lcl_filterAndOrderScriptArgs(lArgs);
             Sequence< Any > outArgs;
             Sequence< sal_Int16 > outIndex;
 
-            if ( lArgs.hasElements() )
-            {
-               int argCount = 0;
-               for ( const auto& rArg : lArgs )
-               {
-                   // Sometimes we get a propertyval with name = "Referer" or "SynchronMode". These
-                   // are not actual arguments to be passed to script, but flags describing the
-                   // call, so ignore. Who thought that passing such "meta-arguments" mixed in with
-                   // real arguments was a good idea?
-                   if ( (rArg.Name != "Referer" &&
-                         rArg.Name != "SynchronMode") ||
-                        rArg.Name.isEmpty() ) //TODO:???
-                   {
-                       inArgs.realloc( ++argCount );
-                       inArgs.getArray()[ argCount - 1 ] = rArg.Value;
-                   }
-               }
-            }
-
             // attempt to protect the document against the script tampering with its Undo Context
             std::unique_ptr< ::framework::DocumentUndoGuard > pUndoGuard;
             if ( bIsDocumentScript )