Re: [xwiki-devs] [xwiki-notifications] r29494 - in platform/core/trunk/xwiki-rendering/xwiki-rendering-macros: xwiki-rendering-macro-html/src/main/java/org/xwiki/rendering/internal/macro/html xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/internal/macro/script xwiki-rendering-macro-script/src/main/java/org/xwiki/rendering/macro/script xwiki-rendering-macro-script/src/main/resources/META-INF xwiki-rendering-macro-script/src/test/java/org/xwiki/rendering/macro/script xwiki-rendering-macro-scri
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! :) Thanks -Vincent
ComponentManagerTestSetup testSetup = new ComponentManagerTestSetup(suite); new ScriptMockSetup(testSetup.getComponentManager());
Deleted: 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/macroscript3.test 2010-06-16 09:35:24 UTC (rev 29493) +++ platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript3.test 2010-06-16 10:03:02 UTC (rev 29494) @@ -1,27 +0,0 @@ -.#----------------------------------------------------- -.input|xwiki/2.0 -.# Validate that inner macro is transformed also -.#----------------------------------------------------- -{{script}} -{{script}} -script here -{{/script}} -{{/script}} -.#----------------------------------------------------- -.expect|event/1.0 -.#----------------------------------------------------- -beginDocument -beginMacroMarkerStandalone [script] [] [{{script}} -script here -{{/script}}] -beginMacroMarkerStandalone [script] [] [script here] -beginParagraph -onWord [script] -onSpace -onWord [here] -endParagraph -endMacroMarkerStandalone [script] [] [script here] -endMacroMarkerStandalone [script] [] [{{script}} -script here -{{/script}}] -endDocument \ No newline at end of file
Deleted: platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript5.test =================================================================== --- platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript5.test 2010-06-16 09:35:24 UTC (rev 29493) +++ platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript5.test 2010-06-16 10:03:02 UTC (rev 29494) @@ -1,23 +0,0 @@ -.#----------------------------------------------------- -.input|xwiki/2.0 -.# Validate that inner macro is inline too -.#----------------------------------------------------- -text {{script}}{{script}}script here{{/script}}{{/script}} text -.#----------------------------------------------------- -.expect|event/1.0 -.#----------------------------------------------------- -beginDocument -beginParagraph -onWord [text] -onSpace -beginMacroMarkerInline [script] [] [{{script}}script here{{/script}}] -beginMacroMarkerInline [script] [] [script here] -onWord [script] -onSpace -onWord [here] -endMacroMarkerInline [script] [] [script here] -endMacroMarkerInline [script] [] [{{script}}script here{{/script}}] -onSpace -onWord [text] -endParagraph -endDocument
Thanks -Vincent
On 06/16/2010 01:18 PM, Vincent Massol wrote:
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. Alex
Thanks -Vincent
ComponentManagerTestSetup testSetup = new ComponentManagerTestSetup(suite); new ScriptMockSetup(testSetup.getComponentManager());
Deleted: 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/macroscript3.test 2010-06-16 09:35:24 UTC (rev 29493) +++ platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript3.test 2010-06-16 10:03:02 UTC (rev 29494) @@ -1,27 +0,0 @@ -.#----------------------------------------------------- -.input|xwiki/2.0 -.# Validate that inner macro is transformed also -.#----------------------------------------------------- -{{script}} -{{script}} -script here -{{/script}} -{{/script}} -.#----------------------------------------------------- -.expect|event/1.0 -.#----------------------------------------------------- -beginDocument -beginMacroMarkerStandalone [script] [] [{{script}} -script here -{{/script}}] -beginMacroMarkerStandalone [script] [] [script here] -beginParagraph -onWord [script] -onSpace -onWord [here] -endParagraph -endMacroMarkerStandalone [script] [] [script here] -endMacroMarkerStandalone [script] [] [{{script}} -script here -{{/script}}] -endDocument \ No newline at end of file
Deleted: platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript5.test =================================================================== --- platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript5.test 2010-06-16 09:35:24 UTC (rev 29493) +++ platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript5.test 2010-06-16 10:03:02 UTC (rev 29494) @@ -1,23 +0,0 @@ -.#----------------------------------------------------- -.input|xwiki/2.0 -.# Validate that inner macro is inline too -.#----------------------------------------------------- -text {{script}}{{script}}script here{{/script}}{{/script}} text -.#----------------------------------------------------- -.expect|event/1.0 -.#----------------------------------------------------- -beginDocument -beginParagraph -onWord [text] -onSpace -beginMacroMarkerInline [script] [] [{{script}}script here{{/script}}] -beginMacroMarkerInline [script] [] [script here] -onWord [script] -onSpace -onWord [here] -endMacroMarkerInline [script] [] [script here] -endMacroMarkerInline [script] [] [{{script}}script here{{/script}}] -onSpace -onWord [text] -endParagraph -endDocument
Thanks -Vincent _______________________________________________ devs mailing list [email protected] http://lists.xwiki.org/mailman/listinfo/devs
On Jun 16, 2010, at 2:22 PM, Alex Busenius wrote:
On 06/16/2010 01:18 PM, Vincent Massol wrote:
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.
ok but there should absolutely be unit tests for the committed code. No code should be committed without unit tests especially when it's easy to write unit tests which is the case here. I don't see why we would need functional tests in this case. Thanks -Vincent
Alex
Thanks -Vincent
ComponentManagerTestSetup testSetup = new ComponentManagerTestSetup(suite); new ScriptMockSetup(testSetup.getComponentManager());
Deleted: 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/macroscript3.test 2010-06-16 09:35:24 UTC (rev 29493) +++ platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript3.test 2010-06-16 10:03:02 UTC (rev 29494) @@ -1,27 +0,0 @@ -.#----------------------------------------------------- -.input|xwiki/2.0 -.# Validate that inner macro is transformed also -.#----------------------------------------------------- -{{script}} -{{script}} -script here -{{/script}} -{{/script}} -.#----------------------------------------------------- -.expect|event/1.0 -.#----------------------------------------------------- -beginDocument -beginMacroMarkerStandalone [script] [] [{{script}} -script here -{{/script}}] -beginMacroMarkerStandalone [script] [] [script here] -beginParagraph -onWord [script] -onSpace -onWord [here] -endParagraph -endMacroMarkerStandalone [script] [] [script here] -endMacroMarkerStandalone [script] [] [{{script}} -script here -{{/script}}] -endDocument \ No newline at end of file
Deleted: platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript5.test =================================================================== --- platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript5.test 2010-06-16 09:35:24 UTC (rev 29493) +++ platform/core/trunk/xwiki-rendering/xwiki-rendering-macros/xwiki-rendering-macro-script/src/test/resources/macroscript5.test 2010-06-16 10:03:02 UTC (rev 29494) @@ -1,23 +0,0 @@ -.#----------------------------------------------------- -.input|xwiki/2.0 -.# Validate that inner macro is inline too -.#----------------------------------------------------- -text {{script}}{{script}}script here{{/script}}{{/script}} text -.#----------------------------------------------------- -.expect|event/1.0 -.#----------------------------------------------------- -beginDocument -beginParagraph -onWord [text] -onSpace -beginMacroMarkerInline [script] [] [{{script}}script here{{/script}}] -beginMacroMarkerInline [script] [] [script here] -onWord [script] -onSpace -onWord [here] -endMacroMarkerInline [script] [] [script here] -endMacroMarkerInline [script] [] [{{script}}script here{{/script}}] -onSpace -onWord [text] -endParagraph -endDocument
Thanks -Vincent
participants (2)
-
Alex Busenius -
Vincent Massol