Hi Sergiu,
I don't like several of the changes here since I believe they're increasing our
technical debt so I'd like we find a better way. Here's some feedback/ideas:
1) com.xpn.util.Util and com.xpn.xwiki.web.Utils are deprecated and shouldn't
be used. On the contrary we should work towards removing stuff from them, not
adding to them :) I see you've added a new method which isn't too good IMO.
2) The EscapeTool you've added to the velocity module doesn't belong there IMO
since I don't see why it would be restricted to Velocity. It's not about a
language limitation that would be only needed for Velocity. I believe other
scripts and even other java part of XWiki might need to escape XML. In addition
you haven't commented why there's a need to write a new class when an escape
tool already exists and is provided by the Velocity Tools project (in a few
days/months we'll wonder why).
For 2) what I think is best is to add the escape code to the xwiki-xml module
and have a ScriptService to make it available to all scripts. BTW there's
already a XMLUtils in there and which already does xml escaping (and which I
don't like and it could a good time to extract part of its stuff into an
Escaper/Encoder component).
So XML and HTML escaping should go in xwiki-xml IMO and URL escaping should go
in the xwiki-URL module (should we need them).
WDYT?
Thanks
-Vincent
On Dec 8, 2010, at 4:47 AM, sdumitriu (SVN) wrote:
> Author: sdumitriu
> Date: 2010-12-08 04:47:08 +0100 (Wed, 08 Dec 2010)
> New Revision: 33301
>
> Added:
>
> platform/core/trunk/xwiki-velocity/src/main/java/org/xwiki/velocity/tools/EscapeTool.java
>
> platform/core/trunk/xwiki-velocity/src/test/java/org/xwiki/velocity/tools/EscapeToolTest.java
> Modified:
> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/XWiki.java
> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/util/Util.java
> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/Utils.java
>
> platform/core/trunk/xwiki-rendering/xwiki-rendering-api/src/main/java/org/xwiki/rendering/internal/renderer/printer/XHTMLWriter.java
>
> platform/core/trunk/xwiki-velocity/src/main/java/org/xwiki/velocity/internal/DefaultVelocityConfiguration.java
> Log:
> XWIKI-5514: "apostrophe" character badly displayed in Internet Explorer
> XWIKI-5763: Remove entity encoding from UTF-8 text in XHTML
> Fixed.
>
> Modified:
> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/XWiki.java
> ===================================================================
> --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/XWiki.java
> 2010-12-07 16:01:32 UTC (rev 33300)
> +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/XWiki.java
> 2010-12-08 03:47:08 UTC (rev 33301)
> @@ -5806,12 +5806,12 @@
> try {
> userdoc = getDocument(user, context);
> if (userdoc == null) {
> - return StringEscapeUtils.escapeXml(user);
> + return Utils.escapeXml(user);
> }
>
> BaseObject userobj = userdoc.getObject("XWiki.XWikiUsers");
> if (userobj == null) {
> - return StringEscapeUtils.escapeXml(userdoc.getName());
> + return Utils.escapeXml(userdoc.getName());
> }
>
> Set<String> proplist = userobj.getPropertyList();
> @@ -5832,7 +5832,7 @@
> + context.getDoc().getPrefixedFullName() + ">",
> vcontext, context);
> }
>
> - text = StringEscapeUtils.escapeXml(text.trim());
> + text = Utils.escapeXml(text.trim());
>
> if (link) {
> text =
>
> Modified:
> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/util/Util.java
> ===================================================================
> --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/util/Util.java
> 2010-12-07 16:01:32 UTC (rev 33300)
> +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/util/Util.java
> 2010-12-08 03:47:08 UTC (rev 33301)
> @@ -51,7 +51,6 @@
> import org.apache.commons.io.FileUtils;
> import org.apache.commons.io.IOUtils;
> import org.apache.commons.lang.ArrayUtils;
> -import org.apache.commons.lang.StringEscapeUtils;
> import org.apache.commons.lang.StringUtils;
> import org.apache.commons.logging.Log;
> import org.apache.commons.logging.LogFactory;
> @@ -623,8 +622,8 @@
>
> public static String getHTMLExceptionMessage(XWikiException xe,
> XWikiContext context)
> {
> - String title = StringEscapeUtils.escapeXml(xe.getMessage());
> - String text = StringEscapeUtils.escapeXml(xe.getFullMessage());
> + String title = Utils.escapeXml(xe.getMessage());
> + String text = Utils.escapeXml(xe.getFullMessage());
> String id = (String) context.get("xwikierrorid");
> if (id == null) {
> id = "1";
>
> Modified:
> platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/Utils.java
> ===================================================================
> --- platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/Utils.java
> 2010-12-07 16:01:32 UTC (rev 33300)
> +++ platform/core/trunk/xwiki-core/src/main/java/com/xpn/xwiki/web/Utils.java
> 2010-12-08 03:47:08 UTC (rev 33301)
> @@ -40,8 +40,6 @@
> import org.apache.commons.lang.StringUtils;
> import org.apache.commons.logging.Log;
> import org.apache.commons.logging.LogFactory;
> -import org.apache.ecs.Filter;
> -import org.apache.ecs.filter.CharacterFilter;
> import org.apache.log4j.MDC;
> import org.apache.struts.upload.MultipartRequestWrapper;
> import org.xwiki.component.manager.ComponentLookupException;
> @@ -512,14 +510,58 @@
> map.put(name, newValues);
> }
>
> + /**
> + * Escapes the XML special characters in a <code>String</code> using
> numerical XML entities.
> + *
> + * @param value the text to escape, may be null
> + * @return a new escaped <code>String</code>, <code>null</code> if null
> input
> + * @deprecated starting with 2.7 use {...@link #escapeXml(String)}
> + */
> + @Deprecated
> public static String formEncode(String value)
> {
> - Filter filter = new CharacterFilter();
> - filter.removeAttribute("'");
> - String svalue = filter.process(value);
> - return svalue;
> + return escapeXml(value);
> }
>
> + /**
> + * Escapes the XML special characters in a <code>String</code> using
> numerical XML entities.
> + *
> + * @param value the text to escape, may be {...@code null}
> + * @return a new escaped {...@code String}, {...@code null} if {...@code
> null} input
> + */
> + public static String escapeXml(String value)
> + {
> + if (value == null) {
> + return null;
> + }
> + StringBuilder result = new StringBuilder((int) (value.length() *
> 1.1));
> + int length = value.length();
> + char c;
> + for (int i = 0; i < length; ++i) {
> + c = value.charAt(i);
> + switch (c) {
> + case '&':
> + result.append("&");
> + break;
> + case '<':
> + result.append("<");
> + break;
> + case '>':
> + result.append(">");
> + break;
> + case '"':
> + result.append(""");
> + break;
> + case '\'':
> + result.append("'");
> + break;
> + default:
> + result.append(c);
> + }
> + }
> + return result.toString();
> + }
> +
> public static String SQLFilter(String text)
> {
> try {
>
> Modified:
> platform/core/trunk/xwiki-rendering/xwiki-rendering-api/src/main/java/org/xwiki/rendering/internal/renderer/printer/XHTMLWriter.java
> ===================================================================
> ---
> platform/core/trunk/xwiki-rendering/xwiki-rendering-api/src/main/java/org/xwiki/rendering/internal/renderer/printer/XHTMLWriter.java
> 2010-12-07 16:01:32 UTC (rev 33300)
> +++
> platform/core/trunk/xwiki-rendering/xwiki-rendering-api/src/main/java/org/xwiki/rendering/internal/renderer/printer/XHTMLWriter.java
> 2010-12-08 03:47:08 UTC (rev 33301)
> @@ -44,6 +44,19 @@
> super(writer, DEFAULT_XHTML_FORMAT);
>
> // escape all non US-ASCII to have as less encoding problems as
> possible
> - setMaximumAllowedCharacter(127);
> + setMaximumAllowedCharacter(-1);
> }
> +
> + /**
> + * Escapes a string to be used as an attribute value. Unlike the
> original method in {...@link XMLWriter}, apostrophes
> + * are replaced by a numerical entity &#38;, since &apos; is not
> valid in HTML documents.
> + *
> + * @param text the attribute value to escape
> + * @return the text with all occurrences of special XML characters
> replaced by entity references.
> + */
> + @Override
> + protected String escapeAttributeEntities(String text)
> + {
> + return super.escapeAttributeEntities(text).replace("'",
> "&");
> + }
> }
>
> Modified:
> platform/core/trunk/xwiki-velocity/src/main/java/org/xwiki/velocity/internal/DefaultVelocityConfiguration.java
> ===================================================================
> ---
> platform/core/trunk/xwiki-velocity/src/main/java/org/xwiki/velocity/internal/DefaultVelocityConfiguration.java
> 2010-12-07 16:01:32 UTC (rev 33300)
> +++
> platform/core/trunk/xwiki-velocity/src/main/java/org/xwiki/velocity/internal/DefaultVelocityConfiguration.java
> 2010-12-08 03:47:08 UTC (rev 33301)
> @@ -22,7 +22,6 @@
> import java.util.Properties;
>
> import org.apache.velocity.tools.generic.ComparisonDateTool;
> -import org.apache.velocity.tools.generic.EscapeTool;
> import org.apache.velocity.tools.generic.ListTool;
> import org.apache.velocity.tools.generic.MathTool;
> import org.apache.velocity.tools.generic.NumberTool;
> @@ -37,6 +36,7 @@
> import org.xwiki.velocity.VelocityConfiguration;
> import org.xwiki.velocity.introspection.ChainingUberspector;
> import org.xwiki.velocity.introspection.DeprecatedCheckUberspector;
> +import org.xwiki.velocity.tools.EscapeTool;
> import org.xwiki.velocity.tools.RegexTool;
>
> /**
>
> Added:
> platform/core/trunk/xwiki-velocity/src/main/java/org/xwiki/velocity/tools/EscapeTool.java
> ===================================================================
> ---
> platform/core/trunk/xwiki-velocity/src/main/java/org/xwiki/velocity/tools/EscapeTool.java
> (rev 0)
> +++
> platform/core/trunk/xwiki-velocity/src/main/java/org/xwiki/velocity/tools/EscapeTool.java
> 2010-12-08 03:47:08 UTC (rev 33301)
> @@ -0,0 +1,72 @@
> +/*
> + * 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.velocity.tools;
> +
> +/**
> + * Tool for working with escaping in Velocity templates. It provides methods
> to escape outputs for Velocity, Java,
> + * JavaScript, HTML, XML and SQL.
> + *
> + * @version $Id$
> + * @since 2.7RC1
> + */
> +public class EscapeTool extends org.apache.velocity.tools.generic.EscapeTool
> +{
> + /**
> + * Escapes the XML special characters in a <code>String</code> using
> numerical XML entities.
> + *
> + * @param value the text to escape, may be {...@code null}
> + * @return a new escaped {...@code String}, {...@code null} if {...@code
> null} input
> + */
> + @Override
> + public String xml(Object source)
> + {
> + if (source == null) {
> + return null;
> + }
> + String str = String.valueOf(source);
> + StringBuilder result = new StringBuilder((int) (str.length() * 1.1));
> + int length = str.length();
> + char c;
> + for (int i = 0; i < length; ++i) {
> + c = str.charAt(i);
> + switch (c) {
> + case '&':
> + result.append("&");
> + break;
> + case '<':
> + result.append("<");
> + break;
> + case '>':
> + result.append(">");
> + break;
> + case '"':
> + result.append(""");
> + break;
> + case '\'':
> + result.append("'");
> + break;
> + default:
> + result.append(c);
> + }
> + }
> + return result.toString();
> + }
> +}
>
>
> Property changes on:
> platform/core/trunk/xwiki-velocity/src/main/java/org/xwiki/velocity/tools/EscapeTool.java
> ___________________________________________________________________
> Added: svn:keywords
> + Author Id Revision HeadURL
> Added: svn:eol-style
> + native
>
> Added:
> platform/core/trunk/xwiki-velocity/src/test/java/org/xwiki/velocity/tools/EscapeToolTest.java
> ===================================================================
> ---
> platform/core/trunk/xwiki-velocity/src/test/java/org/xwiki/velocity/tools/EscapeToolTest.java
> (rev 0)
> +++
> platform/core/trunk/xwiki-velocity/src/test/java/org/xwiki/velocity/tools/EscapeToolTest.java
> 2010-12-08 03:47:08 UTC (rev 33301)
> @@ -0,0 +1,70 @@
> +/*
> + * 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.velocity.tools;
> +
> +import org.junit.Assert;
> +import org.junit.Test;
> +
> +/**
> + * Unit tests for {...@link EscapeTool}.
> + *
> + * @version $Id$
> + * @since 2.7RC1
> + */
> +public class EscapeToolTest
> +{
> + @Test
> + public void testEscapeSimpleXML()
> + {
> + EscapeTool tool = new EscapeTool();
> + String escapedText = tool.xml("a < a' && a' < a\" => a < a\"");
> +
> + Assert.assertFalse("Failed to escape <", escapedText.contains("<"));
> + Assert.assertFalse("Failed to escape >", escapedText.contains(">"));
> + Assert.assertFalse("Failed to escape '", escapedText.contains("'"));
> + Assert.assertFalse("Failed to escape \"",
> escapedText.contains("\""));
> + Assert.assertFalse("Failed to escape &", escapedText.contains("&&"));
> + }
> +
> + @Test
> + public void testEscapeXMLApos()
> + {
> + EscapeTool tool = new EscapeTool();
> +
> + Assert.assertFalse("' wrongly escaped to non-HTML '",
> tool.xml("'").equals("'"));
> + }
> +
> + @Test
> + public void testEscapeXMLWithNull()
> + {
> + EscapeTool tool = new EscapeTool();
> +
> + Assert.assertNull("null should be null", tool.xml(null));
> + }
> +
> + @Test
> + public void testEscapeXMLNonAscii()
> + {
> + EscapeTool tool = new EscapeTool();
> +
> + Assert.assertTrue("Non-ASCII characters shouldn't be escaped",
> tool.xml("\u0123").equals("\u0123"));
> + }
> +}
>
>
> Property changes on:
> platform/core/trunk/xwiki-velocity/src/test/java/org/xwiki/velocity/tools/EscapeToolTest.java
> ___________________________________________________________________
> Added: svn:keywords
> + Author Id Revision HeadURL
> Added: svn:eol-style
> + native
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
