XWiki allows RCE from script right in configurable sections

@@ -32,6 +32,7 @@
   <packaging>pom</packaging>
   <description>Application for administrating wiki features like users, groups, rights etc.</description>
   <modules>
+    <module>xwiki-platform-administration-api</module>
     <module>xwiki-platform-administration-ui</module>
   </modules>
   <profiles>

@@ -0,0 +1,50 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+  <modelVersion>4.0.0</modelVersion>
+  <parent>
+    <groupId>org.xwiki.platform</groupId>
+    <artifactId>xwiki-platform-administration</artifactId>
+    <version>16.4.0-SNAPSHOT</version>
+  </parent>
+  <artifactId>xwiki-platform-administration-api</artifactId>
+  <name>XWiki Platform - Administration - API</name>
+  <packaging>jar</packaging>
+  <description>XWiki Platform - Administration - API</description>
+  <dependencies>
+    <dependency>
+      <groupId>org.xwiki.platform</groupId>
+      <artifactId>xwiki-platform-oldcore</artifactId>
+      <version>${project.version}</version>
+    </dependency>
+    <dependency>
+      <groupId>org.xwiki.commons</groupId>
+      <artifactId>xwiki-commons-component-api</artifactId>
+      <version>${commons.version}</version>
+    </dependency>
+    <dependency>
+      <groupId>org.xwiki.commons</groupId>
+      <artifactId>xwiki-commons-tool-test-component</artifactId>
+      <version>${commons.version}</version>
+      <scope>test</scope>
+    </dependency>
+  </dependencies>
+</project>

@@ -0,0 +1,62 @@
+/*
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+package org.xwiki.administration.api;
+
+import java.util.Map;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.xwiki.component.annotation.Component;
+import org.xwiki.evaluation.ObjectEvaluator;
+import org.xwiki.evaluation.ObjectEvaluatorException;
+import org.xwiki.evaluation.ObjectPropertyEvaluator;
+
+import com.xpn.xwiki.objects.BaseObject;
+
+/**
+ * Evaluator for objects of class {@code XWiki.ConfigurableClass}.
+ * Returns a Map storing the evaluated content for "heading" and "linkPrefix" properties.
+ *
+ * @version $Id$
+ * @since 15.10.9
+ * @since 16.3.0
+ */
+@Component
+@Singleton
+@Named(ConfigurableObjectEvaluator.ROLE_HINT)
+public class ConfigurableObjectEvaluator implements ObjectEvaluator
+{
+    /**
+     * The role hint of this component.
+     */
+    public static final String ROLE_HINT = "XWiki.ConfigurableClass";
+
+    @Inject
+    @Named("velocity")
+    private ObjectPropertyEvaluator velocityPropertyEvaluator;
+
+    @Override
+    public Map<String, String> evaluate(BaseObject object) throws ObjectEvaluatorException
+    {
+        return this.velocityPropertyEvaluator.evaluateProperties(object, "heading", "linkPrefix");
+    }
+}

@@ -0,0 +1 @@
+org.xwiki.administration.api.ConfigurableObjectEvaluator

@@ -0,0 +1,80 @@
+/*
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+package org.xwiki.administration.api;
+
+import java.util.Map;
+
+import javax.inject.Named;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mock;
+import org.xwiki.evaluation.ObjectEvaluatorException;
+import org.xwiki.evaluation.ObjectPropertyEvaluator;
+import org.xwiki.test.junit5.mockito.ComponentTest;
+import org.xwiki.test.junit5.mockito.InjectMockComponents;
+import org.xwiki.test.junit5.mockito.MockComponent;
+
+import com.xpn.xwiki.objects.BaseObject;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Validate {@link ConfigurableObjectEvaluator}.
+ *
+ * @version $Id$
+ */
+@ComponentTest
+class ConfigurableObjectEvaluatorTest
+{
+    @InjectMockComponents
+    private ConfigurableObjectEvaluator configurableObjectEvaluator;
+
+    @MockComponent
+    @Named("velocity")
+    private ObjectPropertyEvaluator velocityObjectPropertyEvaluator;
+
+    @Mock
+    private BaseObject baseObject;
+
+    @BeforeEach
+    void setUp() throws ObjectEvaluatorException
+    {
+        when(this.baseObject.getStringValue("heading")).thenReturn("unevaluated heading");
+        when(this.baseObject.getStringValue("linkPrefix")).thenReturn("unevaluated linkPrefix");
+
+        Map<String, String> evaluatedVelocityProperties =
+            Map.of("heading", "evaluated heading", "linkPrefix", "evaluated linkPrefix");
+
+        when(this.velocityObjectPropertyEvaluator.evaluateProperties(this.baseObject, "heading", "linkPrefix"))
+            .thenReturn(evaluatedVelocityProperties);
+    }
+
+    @Test
+    void checkEvaluationThroughPropertyEvaluator() throws ObjectEvaluatorException
+    {
+        Map<String, String> evaluationResults = this.configurableObjectEvaluator.evaluate(this.baseObject);
+        verify(this.velocityObjectPropertyEvaluator).evaluateProperties(this.baseObject, "heading", "linkPrefix");
+        assertEquals("evaluated heading", evaluationResults.get("heading"));
+        assertEquals("evaluated linkPrefix", evaluationResults.get("linkPrefix"));
+    }
+}

@@ -104,6 +104,12 @@
       <version>${project.version}</version>
       <scope>runtime</scope>
     </dependency>
+    <dependency>
+      <groupId>org.xwiki.platform</groupId>
+      <artifactId>xwiki-platform-administration-api</artifactId>
+      <version>${project.version}</version>
+    </dependency>
+
     <!-- Test dependencies. -->
     <dependency>
       <groupId>org.xwiki.platform</groupId>

@@ -339,8 +339,6 @@
       #if($propertiesToShow.size() > 0 && !$propertiesToShow.contains($propName))
         ## Silently skip over this property.
       #else
-        #set($prettyName = "#evaluate($app.displayPrettyName($propName, $obj))")
-        ##
         #set($hintKey = "${obj.xWikiClass.name}_${propName}.hint")
         #set($hint = $services.localization.render($hintKey))
         #if($hint == $hintKey)
@@ -364,11 +362,10 @@
           $out ##
           #set ($out = '')
         #end
-        $escapetool.xml($prettyName)
+        $escapetool.xml($app.displayPrettyName($propName, $obj))
         </label>
         #if($linkPrefix != '')
-          #set($linkScript = "$linkPrefix$propName")
-          <a href="$escapetool.xml("#evaluate($linkScript)")" class="xHelp" title="$services.localization.render('admin.documentation')">$services.localization.render('admin.documentation')</a>
+          <a href="$escapetool.xml($linkPrefix + $propName)" class="xHelp" title="$services.localization.render('admin.documentation')">$services.localization.render('admin.documentation')</a>
         #end
         #if ($hint)
           <span class="xHint">$hint</span>

@@ -195,32 +195,23 @@ $xwiki.jsfx.use('js/xwiki/actionbuttons/actionButtons.js', true)
         #set($formId = "${section.toLowerCase()}_${app.getFullName()}")
         #set($escapedAppName = $escapetool.xml($app.getFullName()))
         #foreach($configurableObj in $configurableObjs)
-          ## Execute the content code if any
-          ## FIXME: we have to do that before the title before of the dropPermissions
-          #set($codeToExecute = "$!app.getValue('codeToExecute', $configurableObj)")
-          #if($codeToExecute != '')
-            #set($codeToExecuteResult = $configurableObj.display('codeToExecute', 'view', false))
+          #set ($heading = $app.getValue('heading', $configurableObj))
+          #set ($codeToExecute = "$!app.getValue('codeToExecute', $configurableObj)")
+          ## If linkPrefix is set, then we will make each property label a link which starts with that prefix.
+          #set ($linkPrefix = "$!app.getValue('linkPrefix', $configurableObj)")
+          #if (!$app.restricted)
+            #set ($evaluatedConfigurableObj = $configurableObj.evaluate())
+            #set ($heading = $evaluatedConfigurableObj.heading)
+            #set ($linkPrefix = $evaluatedConfigurableObj.linkPrefix)
           #end
           ## Display the header if one exists.
-          #set($heading = $app.getValue('heading', $configurableObj))
           #if($heading && $heading != '')
-            ## This application should not run with programming rights because it evaluates code which may not be trustworthy.
-            ## Removing the next line will open a security hole.
-            ## Can't use $configurableObj.display('heading', 'view', false) to have proper heading id (because of the html macro)
-            ## FIXME: find a cleaner solution
-            #set($void = $doc.dropPermissions())
-            #if ($services.security.authorization.hasAccess("script",
-              $app.authors.effectiveMetadataAuthor, $app.documentReference) && !$app.restricted)
-              #set($evaluatedHeading = "#evaluate($heading)")
-            #else
-              #set($evaluatedHeading = $heading)
-            #end
-            == $services.rendering.escape($evaluatedHeading, 'xwiki/2.1') ==
+            == $services.rendering.escape($heading, 'xwiki/2.1') ==
           #end
           ## Display code to execute
-          #if($codeToExecute != '')
+          #if ($codeToExecute != '')
             (%class="codeToExecute"%)(((##
-              $codeToExecuteResult
+              $configurableObj.display('codeToExecute', 'view', false)
             )))
           #end
           ##
@@ -230,9 +221,6 @@ $xwiki.jsfx.use('js/xwiki/actionbuttons/actionButtons.js', true)
             #set($propertiesToShow = [])
           #end
           ##
-          ## If linkPrefix is set, then we will make each property label a link which starts with that prefix.
-          #set($linkPrefix = "$!app.getValue('linkPrefix', $configurableObj)")
-          ##
           ## If the Configurable object specifies a configuration class, use it,
           ## otherwise assume custom forms are used instead.
           #set($configClassName = "$!app.getValue('configurationClass', $configurableObj)")

@@ -24,6 +24,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.concurrent.Callable;
 
 import org.apache.http.client.utils.URLEncodedUtils;
 import org.jsoup.nodes.Document;
@@ -32,6 +33,9 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.Mock;
+import org.xwiki.administration.api.ConfigurableObjectEvaluator;
+import org.xwiki.evaluation.internal.DefaultObjectEvaluator;
+import org.xwiki.evaluation.internal.VelocityObjectPropertyEvaluator;
 import org.xwiki.localization.macro.internal.TranslationMacro;
 import org.xwiki.model.reference.DocumentReference;
 import org.xwiki.query.internal.ScriptQuery;
@@ -41,6 +45,7 @@
 import org.xwiki.rendering.internal.macro.message.ErrorMessageMacro;
 import org.xwiki.rendering.internal.macro.message.WarningMessageMacro;
 import org.xwiki.script.service.ScriptService;
+import org.xwiki.security.authorization.AuthorExecutor;
 import org.xwiki.security.authorization.Right;
 import org.xwiki.security.script.SecurityScriptServiceComponentList;
 import org.xwiki.test.annotation.ComponentList;
@@ -51,17 +56,23 @@
 import org.xwiki.user.UserReferenceComponentList;
 import org.xwiki.user.internal.converter.DocumentUserReferenceConverter;
 import org.xwiki.user.internal.document.DocumentUserReference;
+import org.xwiki.velocity.VelocityEngine;
+import org.xwiki.velocity.VelocityManager;
 
 import com.xpn.xwiki.doc.XWikiDocument;
 import com.xpn.xwiki.internal.model.reference.DocumentReferenceConverter;
 import com.xpn.xwiki.objects.BaseObject;
+import com.xpn.xwiki.objects.classes.BaseClass;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.fail;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 /**
@@ -81,7 +92,10 @@
     ErrorMessageMacro.class,
     WarningMessageMacro.class,
     DocumentUserReferenceConverter.class,
-    DocumentReferenceConverter.class
+    DocumentReferenceConverter.class,
+    DefaultObjectEvaluator.class,
+    VelocityObjectPropertyEvaluator.class,
+    ConfigurableObjectEvaluator.class
 })
 class ConfigurableClassPageTest extends PageTest
 {
@@ -100,6 +114,8 @@ class ConfigurableClassPageTest extends PageTest
 
     private static final String MY_SECTION_SERIALIZED = "XWiki.]],{{noscript /}}";
 
+    private static final String CONFIG_CLASS_NAME = "TestClass";
+
     private static final String WEB_HOME = "WebHome";
 
     @Mock
@@ -108,6 +124,10 @@ class ConfigurableClassPageTest extends PageTest
     @Mock
     private ScriptQuery query;
 
+    private AuthorExecutor authorExecutor;
+
+    private VelocityEngine velocityEngine;
+
     @BeforeEach
     void setUp() throws Exception
     {
@@ -122,6 +142,21 @@ void setUp() throws Exception
         when(this.query.setOffset(anyInt())).thenReturn(this.query);
         when(this.query.bindValues(any(Map.class))).thenReturn(this.query);
         when(this.query.bindValues(any(List.class))).thenReturn(this.query);
+
+        this.authorExecutor = this.componentManager.registerMockComponent(AuthorExecutor.class, true);
+
+        // Spy Velocity Engine.
+        VelocityManager velocityManager = this.componentManager.getInstance(VelocityManager.class);
+        this.velocityEngine = velocityManager.getVelocityEngine();
+        this.velocityEngine = spy(this.velocityEngine);
+        velocityManager = spy(velocityManager);
+        this.componentManager.registerComponent(VelocityManager.class, velocityManager);
+        when(velocityManager.getVelocityEngine()).thenReturn(this.velocityEngine);
+
+        when(this.authorExecutor.call(any(), any(), any())).thenAnswer(invocation -> {
+            Callable<?> callable = invocation.getArgument(0);
+            return callable.call();
+        });
     }
 
     @Test
@@ -167,12 +202,23 @@ void checkScriptRight(boolean hasScript) throws Exception
         when(this.oldcore.getMockRightService()
             .hasAccessLevel(eq("edit"), any(), any(), any())).thenReturn(true);
 
+        DocumentReference configClassRef = new DocumentReference("xwiki", SPACE_NAME, CONFIG_CLASS_NAME);
+        XWikiDocument configClassDoc = new XWikiDocument(configClassRef);
+        BaseClass configClass = new BaseClass();
+        configClass.addTextField("test", "Test", 10);
+        configClassDoc.setXClass(configClass);
+        this.xwiki.saveDocument(configClassDoc, this.context);
+
         XWikiDocument mySectionDoc = new XWikiDocument(MY_SECTION);
+        BaseObject configObject = mySectionDoc.newXObject(configClassRef, this.context);
         BaseObject object = mySectionDoc.newXObject(CONFIGURABLE_CLASS, this.context);
         object.setStringValue("displayInCategory", "other");
         object.setStringValue("displayInSection", "other");
+        object.setStringValue("configurationClass", SPACE_NAME + "." + CONFIG_CLASS_NAME);
         String originalHeading = "$appName {{noscript /}}";
         object.setStringValue("heading", originalHeading);
+        String originalLinkPrefix = "\"prefix/$appName/{{noscript /}}/";
+        object.setStringValue("linkPrefix", originalLinkPrefix);
         object.set("scope", "WIKI+ALL_SPACES", this.context);
         DocumentReference userReference = new DocumentReference(WIKI_NAME, SPACE_NAME, "Admin");
         mySectionDoc.getAuthors().setEffectiveMetadataAuthor(new DocumentUserReference(userReference, true));
@@ -181,13 +227,25 @@ void checkScriptRight(boolean hasScript) throws Exception
             userReference, mySectionDoc.getDocumentReference())).thenReturn(hasScript);
 
         Document htmlPage = renderHTMLPage(CONFIGURABLE_CLASS);
-        String expected;
+        verify(this.oldcore.getMockAuthorizationManager()).hasAccess(Right.SCRIPT, userReference, MY_SECTION);
+
+        String expectedHeading;
+        String expectedLink;
         if (hasScript) {
-            expected = String.format("%s {{noscript /}}", MY_SECTION_SERIALIZED);
+            expectedHeading = String.format("%s {{noscript /}}", MY_SECTION_SERIALIZED);
+            expectedLink = String.format("\"prefix/%s/{{noscript /}}/test", MY_SECTION_SERIALIZED);
+            verify(this.authorExecutor).call(any(), eq(userReference), eq(MY_SECTION));
+            verify(this.velocityEngine).evaluate(any(), any(), any(), eq(originalHeading));
+            verify(this.velocityEngine).evaluate(any(), any(), any(), eq(originalLinkPrefix));
         } else {
-            expected = originalHeading;
+            expectedHeading = originalHeading;
+            expectedLink = originalLinkPrefix + "test";
+            verify(this.velocityEngine, never()).evaluate(any(), any(), any(), eq(originalHeading));
+            verify(this.velocityEngine, never()).evaluate(any(), any(), any(), eq(originalLinkPrefix));
         }
-        assertEquals(expected, htmlPage.selectFirst("h2").text());
+
+        assertEquals(expectedHeading, Objects.requireNonNull(htmlPage.selectFirst("h2")).text());
+        assertEquals(expectedLink, Objects.requireNonNull(htmlPage.selectFirst("a")).attr("href"));
     }
 
     @Test