Hi Caleb,
Some reviews below.
On Jun 16, 2010, at 12:03 PM, cjdelisle (SVN) wrote:
Author: cjdelisle
Date: 2010-06-16 12:03:02 +0200 (Wed, 16 Jun 2010)
New Revision: 29494
Added:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/DefaultScriptMacroConfiguration.java
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/DefaultScriptMacroUtils.java
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/ScriptMacroUtils.java
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/ScriptMacro.java
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/ScriptMacroConfiguration.java
Removed:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript3.test
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript5.test
Modified:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-html/src/main/java/org/xwiki/rendering/internal/macro/html/HTMLMacro.java
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/AbstractScriptMacro.java
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/resources/META-INF/components.txt
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/java/org/xwiki/rendering/macro/script/RenderingTests.java
Log:
XWIKI-5275: Prevent nested script macros.
When script runs, looks up the dom tree to see if it's inside of the output from
another script and stops.
Thanks Alex Busenius for the patch.
Reviewed and applied with some revisions.
[snip]
Added:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/DefaultScriptMacroUtils.java
===================================================================
---
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/DefaultScriptMacroUtils.java
(rev 0)
+++
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/DefaultScriptMacroUtils.java 2010-06-16
10:03:02 UTC (rev 29494)
@@ -0,0 +1,76 @@
+/*
+ * 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.rendering.internal.macro.script;
+
+import org.xwiki.component.annotation.Component;
+import org.xwiki.component.annotation.Requirement;
+import org.xwiki.rendering.block.MacroBlock;
+import org.xwiki.rendering.block.MacroMarkerBlock;
+import org.xwiki.rendering.macro.MacroId;
+import org.xwiki.rendering.macro.MacroLookupException;
+import org.xwiki.rendering.macro.MacroManager;
+import org.xwiki.rendering.macro.script.ScriptMacro;
+import org.xwiki.rendering.macro.script.ScriptMacroConfiguration;
+
+/**
+ * A collection of utility methods for script macros.
+ *
+ * @version $Id$
+ * @since 2.4M2
+ */
+@Component
+public class DefaultScriptMacroUtils implements ScriptMacroUtils
I really don' t like having a component with such a bad name. Utils are the opposite
to good OO design IMO.
And the fact that isScriptNested will return false even if the script is nested is even
worse (when the config param is set to true)...
+{
+ /** Used to get a macro by id. */
+ @Requirement
+ private MacroManager macroManager;
+
+ /** Script macro configuration. */
+ @Requirement
+ private ScriptMacroConfiguration configuration;
+
+ /**
+ * {@inheritDoc}
+ * @see
org.xwiki.rendering.internal.macro.script.ScriptMacroUtils#isScriptNested(org.xwiki.rendering.block.MacroBlock)
+ */
+ public boolean isScriptNested(MacroBlock currentBlock)
+ {
+ if (configuration.isNestedScriptsEnabled()) {
+ return false;
+ }
+ // traverse the XDOM tree up to the root
+ MacroMarkerBlock parent =
currentBlock.getParentBlockByType(MacroMarkerBlock.class);
+ while (parent != null) {
+ String parentId = parent.getId();
+ try {
+ if (macroManager.getMacro(new MacroId(parentId)) instanceof ScriptMacro)
{
+ return true;
+ } else if ("include".equals(parentId)) {
+ // included documents intercept the chain of nested script macros
with XWiki syntax
+ return false;
+ }
+ } catch (MacroLookupException exception) {
+ // shouldn't happen, the parent macro was already successfully
executed earlier
+ }
+ parent = parent.getParentBlockByType(MacroMarkerBlock.class);
+ }
+ return false;
+ }
+}
Property changes on:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/DefaultScriptMacroUtils.java
___________________________________________________________________
Name: svn:keywords
+ Author Id Revision HeadURL
Name: svn:eol-style
+ native
Added:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/ScriptMacroUtils.java
===================================================================
---
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/ScriptMacroUtils.java
(rev 0)
+++
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/ScriptMacroUtils.java 2010-06-16
10:03:02 UTC (rev 29494)
@@ -0,0 +1,41 @@
+/*
+ * 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.rendering.internal.macro.script;
+
+import org.xwiki.component.annotation.ComponentRole;
+import org.xwiki.rendering.block.MacroBlock;
+
+/**
+ * An internal component for accessing script macro-related utility methods.
+ *
+ * @version $Id$
+ * @since 2.4M2
+ */
+@ComponentRole
+public interface ScriptMacroUtils
+{
+ /**
+ * Check if the given macro block is nested inside a script macro.
+ *
+ * @param currentBlock the block to check
+ * @return true if the block is a ancestor of a script macro block
The implementation doesn't do what the javadoc says since it will return false
independently of the passed blocks if the config is set to true.
+ */
+ boolean isScriptNested(MacroBlock currentBlock);
+}
Property changes on:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script/ScriptMacroUtils.java
___________________________________________________________________
Name: svn:keywords
+ Author Id Revision HeadURL
Name: svn:eol-style
+ native
Modified:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/AbstractScriptMacro.java
===================================================================
---
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/AbstractScriptMacro.java 2010-06-16
09:35:24 UTC (rev 29493)
+++
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/AbstractScriptMacro.java 2010-06-16
10:03:02 UTC (rev 29494)
@@ -20,7 +20,6 @@
package org.xwiki.rendering.macro.script;
import java.io.StringReader;
-import java.net.URLClassLoader;
import java.util.Collections;
import java.util.List;
@@ -35,6 +34,7 @@
import org.xwiki.rendering.block.MacroBlock;
import org.xwiki.rendering.block.XDOM;
import org.xwiki.rendering.internal.macro.script.AttachmentClassLoaderFactory;
+import org.xwiki.rendering.internal.macro.script.ScriptMacroUtils;
import org.xwiki.rendering.macro.AbstractMacro;
import org.xwiki.rendering.macro.MacroExecutionException;
import org.xwiki.rendering.macro.descriptor.ContentDescriptor;
@@ -49,8 +49,10 @@
* @param <P> the type of macro parameters bean.
* @version $Id$
* @since 1.7M3
+ * @todo This class is on the edge of a fanout violation and needs to be refactored and
split up.
*/
public abstract class AbstractScriptMacro<P extends ScriptMacroParameters> extends
AbstractMacro<P>
+ implements ScriptMacro
{
/**
* The default description of the script macro content.
@@ -106,6 +108,12 @@
private ParserUtils parserUtils = new ParserUtils();
/**
+ * Used to check for nested scripts.
+ */
+ @Requirement
+ private ScriptMacroUtils scriptUtils;
This looks very artificial and I have the feeling it was introduced artifically because
of a fanout violation.
As I 've said on irc it's much better for example to introduce a generic notion
of filter and implement a filter based on the logic found in DefaultScriptMacroUtils.
+
+ /**
* @param macroName the name of the macro (eg "groovy")
*/
public AbstractScriptMacro(String macroName)
@@ -196,6 +204,11 @@
try {
Thread.currentThread().setContextClassLoader(newClassLoader);
+ // 0) Abort execution if the script is nested
+ if (scriptUtils.isScriptNested(context.getCurrentMacroBlock())) {
+ throw new MacroExecutionException("Nested scripts are not
allowed");
+ }
This part can be replaced by ScriptValidator components:
try {
for (ScriptValidator validator : componentManager.lookupList(ScriptValidator.class)) {
validator.validate(context); <-- will throw ScriptValidationException
}
} catch (ScriptValidationException e) {
throw new MacroExecutionException("Script failed to pass validations. Reason =
[" + e.getMessage() + "]", e);
}
+
// 1) Run script engine on macro block content
String scriptResult = evaluate(parameters, content, context);
@@ -238,7 +251,7 @@
* @return the class loader to use for executing the script
* @throws Exception in case of an error in building the class loader
*/
- private URLClassLoader findClassLoader(String jarsParameterValue, ClassLoader
parent) throws Exception
+ private ClassLoader findClassLoader(String jarsParameterValue, ClassLoader parent)
throws Exception
{
// We cache the Class Loader for improved performances and we check if the saved
class loader had the same
// jar parameters value as the current execution. If not, we compute a new class
loader.
Added:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/ScriptMacro.java
===================================================================
---
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/ScriptMacro.java
(rev 0)
+++
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/ScriptMacro.java 2010-06-16
10:03:02 UTC (rev 29494)
@@ -0,0 +1,34 @@
+/*
+ * 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.rendering.macro.script;
+
+
+/**
+ * An interface used to distinguish script macros from the rest. The most important
difference
+ * is that script macros cannot be nested.
That's not true. It depends on the configuration param.
+ *
+ * @version $Id$
+ * @since 2.4M2
+ */
+public interface ScriptMacro
+{
+
+}
+
Property changes on:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/ScriptMacro.java
___________________________________________________________________
Name: svn:keywords
+ Author Id Revision HeadURL
Name: svn:eol-style
+ native
Added:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/ScriptMacroConfiguration.java
===================================================================
---
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/ScriptMacroConfiguration.java
(rev 0)
+++
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/ScriptMacroConfiguration.java 2010-06-16
10:03:02 UTC (rev 29494)
@@ -0,0 +1,46 @@
+/*
+ * 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.rendering.macro.script;
+
+import org.xwiki.component.annotation.ComponentRole;
+
+/**
+ * Configuration properties for the Script macro.
+ *
+ * You can override the default values for each of the configuration properties below by
defining them in XWiki's global
+ * configuration file using a prefix of "rendering.macro.script" followed by
the property name. For example:
+ * <code>rendering.macro.script.nestedscripts.enabled = 1</code>
+ *
+ * @version $Id$
+ * @since 2.4M2
+ */
+@ComponentRole
+public interface ScriptMacroConfiguration
+{
+ /**
+ * Allowing nested scripting macros, for example
{{velocity}}{{groovy}}...{{/groovy}}{{/velocity}} is potentially
+ * insecure, because an insufficiently escaped user data might allow script
injection attacks.
+ *
+ * Default: 0 (disabled)
What does "Default: 0 (disabled)" means?
+ *
+ * @return true if nested scripts are allowed
+ */
+ boolean isNestedScriptsEnabled();
+}
Property changes on:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script/ScriptMacroConfiguration.java
___________________________________________________________________
Name: svn:keywords
+ Author Id Revision HeadURL
Name: svn:eol-style
+ native
Modified:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/resources/META-INF/components.txt
===================================================================
---
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/resources/META-INF/components.txt 2010-06-16
09:35:24 UTC (rev 29493)
+++
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/main/resources/META-INF/components.txt 2010-06-16
10:03:02 UTC (rev 29494)
@@ -1,2 +1,4 @@
org.xwiki.rendering.internal.macro.script.DefaultScriptMacro
-org.xwiki.rendering.internal.macro.script.DefaultAttachmentClassLoaderFactory
\ No newline at end of file
+org.xwiki.rendering.internal.macro.script.DefaultAttachmentClassLoaderFactory
+org.xwiki.rendering.internal.macro.script.DefaultScriptMacroUtils
+org.xwiki.rendering.internal.macro.script.DefaultScriptMacroConfiguration
Modified:
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/java/org/xwiki/rendering/macro/script/RenderingTests.java
===================================================================
---
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/java/org/xwiki/rendering/macro/script/RenderingTests.java 2010-06-16
09:35:24 UTC (rev 29493)
+++
platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/java/org/xwiki/rendering/macro/script/RenderingTests.java 2010-06-16
10:03:02 UTC (rev 29494)
@@ -39,9 +39,7 @@
suite.addTestsFromResource("macroscript1", true);
suite.addTestsFromResource("macroscript2", true);
- suite.addTestsFromResource("macroscript3", true);
suite.addTestsFromResource("macroscript4", true);
- suite.addTestsFromResource("macroscript5", true);
There's a whole now. Other tests should be renamed or a new test added? Why don't
we test the code btw? I didn't see any tests, have I missed something? We should never
commit without tests especially not in the rendering module that is well tested! :)
There are functional tests that replace the removed unit tests in
another patch attached to XWIKI-5223. There was also a small discussion
on IRC about ways to unit test those changes too.