Skip to content

Commit

Permalink
1. ZK-5182: Prevent XSS issue in component attributes
Browse files Browse the repository at this point in the history
2. ZK-5162: emptyMessage is not escaped with HTML characters
  • Loading branch information
jumperchen committed Dec 21, 2023
1 parent 9653c09 commit 035f86e
Show file tree
Hide file tree
Showing 36 changed files with 277 additions and 62 deletions.
5 changes: 3 additions & 2 deletions zhtml/src/main/java/org/zkoss/zhtml/impl/AbstractTag.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.zkoss.zk.ui.Desktop;
import org.zkoss.zk.ui.Execution;
import org.zkoss.zk.ui.Executions;
import org.zkoss.zk.ui.SafeHtmlValue;
import org.zkoss.zk.ui.UiException;
import org.zkoss.zk.ui.WrongValueException;
import org.zkoss.zk.ui.event.Events;
Expand Down Expand Up @@ -520,8 +521,8 @@ protected void redrawChildrenDirectly(TagRenderContext rc, Execution exec, java.
protected void renderProperties(org.zkoss.zk.ui.sys.ContentRenderer renderer)
throws java.io.IOException {
super.renderProperties(renderer);
render(renderer, "prolog", getPrologHalf(false));
render(renderer, "epilog", getEpilogHalf());
render(renderer, "prolog", SafeHtmlValue.valueOf(getPrologHalf(false)));
render(renderer, "epilog", SafeHtmlValue.valueOf(getEpilogHalf()));
}

/**
Expand Down
6 changes: 3 additions & 3 deletions zhtml/src/main/resources/web/js/zhtml/Text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export class Text extends zhtml.Widget {
var n = this.$n();
if (n) {
var val = this._value;
n.innerHTML = this._encode ? zUtl.encodeXML(val) : val;
n.innerHTML = this._encode ? val : zUtl.decodeXML(val);
// See Bug 2871080 and ZK-294
}
}
Expand Down Expand Up @@ -71,7 +71,7 @@ export class Text extends zhtml.Widget {
var n = this.$n();
if (n) {
var val = this._value;
n.innerHTML = this._encode ? zUtl.encodeXML(val) : val;
n.innerHTML = this._encode ? val : zUtl.decodeXML(val);
// See Bug 2871080 and ZK-294
}
}
Expand Down Expand Up @@ -99,7 +99,7 @@ export class Text extends zhtml.Widget {
span = attrs || (this.idRequired && this._checkContentRequired(val));
// Bug 3245960: enclosed text was wrapped with <span>
if (span) out.push('<span', ' id="', this.uuid, '"', attrs, '>');
out.push(this._encode ? zUtl.encodeXML(val) : val);
out.push(this._encode ? val : zUtl.decodeXML(val));
// See Bug 2871080 and ZK-294
if (span) out.push('</span>');
}
Expand Down
10 changes: 9 additions & 1 deletion zk/src/main/java/org/zkoss/zk/ui/AbstractComponent.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import org.zkoss.lang.Strings;
import org.zkoss.util.CollectionsX;
import org.zkoss.util.Converter;
import org.zkoss.xml.XMLs;
import org.zkoss.zk.au.AuRequest;
import org.zkoss.zk.au.AuResponse;
import org.zkoss.zk.au.AuService;
Expand Down Expand Up @@ -1853,15 +1854,22 @@ protected void smartUpdate(String attr, Object value) {
* and <code>wgt.setAttr("value2")</code> will be invoked at the client
* accordingly.
*
* <p>Note: the String type value will be escaped by {@link Strings#escapeJavaScript} and {@link XMLs#escapeXML}.
* To allow the safe HTML, use {@link org.zkoss.zk.ui.SafeHtmlValue} to wrap the value. (since 10.0.0)
* @param append whether to append the updates of properties with the same
* name. If false, only the last value of the same property will be sent
* to the client.
* @since 5.0.0
* @see #smartUpdate(String, Object)
*/
protected void smartUpdate(String attr, Object value, boolean append) {
if (_page != null)
if (_page != null) {
if (value instanceof String) {
// no need to use Strings.escapeJavaScript() here.
value = XMLs.escapeXML((String) value);
}
getAttachedUiEngine().addSmartUpdate(this, attr, value, append);
}
}

/** A special smart update to update a value in int.
Expand Down
4 changes: 2 additions & 2 deletions zk/src/main/java/org/zkoss/zk/ui/HtmlNativeComponent.java
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,8 @@ private static boolean startsWith(StringBuffer sb, String tag, int start) {
protected void renderProperties(org.zkoss.zk.ui.sys.ContentRenderer renderer) throws java.io.IOException {
super.renderProperties(renderer);

render(renderer, "prolog", getPrologHalf());
render(renderer, "epilog", getEpilogHalf());
render(renderer, "prolog", SafeHtmlValue.valueOf(getPrologHalf()));
render(renderer, "epilog", SafeHtmlValue.valueOf(getEpilogHalf()));
}

private String getPrologHalf() {
Expand Down
78 changes: 78 additions & 0 deletions zk/src/main/java/org/zkoss/zk/ui/SafeHtmlValue.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/* SafeHtmlValue.java
Purpose:
Description:
History:
2:39 PM 2023/12/20, Created by jumperchen
Copyright (C) 2023 Potix Corporation. All Rights Reserved.
*/
package org.zkoss.zk.ui;

import java.util.Objects;

import org.zkoss.json.JSONValue;

/**
* A string-like value that is safe to be used as HTML content.
* @author jumperchen
* @since 10.0.0
*/
public class SafeHtmlValue implements org.zkoss.json.JSONAware, java.io.Serializable {
private static final long serialVersionUID = 202312201440L;
private final String _value;

/** An empty SafeHtmlValue instance. */
public static final SafeHtmlValue EMPTY = new SafeHtmlValue("");

/**
* Constructor.
* @param value the value to be wrapped.
*/
public SafeHtmlValue(String value) {
_value = value;
}

/**
* Returns the wrapped value.
*/
public String getValue() {
return _value;
}

@Override
public String toString() {
return toJSONString();
}

@Override
public String toJSONString() {
return JSONValue.toJSONString(_value);
}

@Override
public boolean equals(Object o) {
if (this == o)
return true;
if (o == null || getClass() != o.getClass())
return false;
SafeHtmlValue that = (SafeHtmlValue) o;
return Objects.equals(_value, that._value);
}

@Override
public int hashCode() {
return Objects.hash(_value);
}

/**
* Returns a SafeHtmlValue instance for the specified value.
* @param value the value to be wrapped.
* @since 10.0.0
*/
public static SafeHtmlValue valueOf(String value) {
return new SafeHtmlValue(value);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.zkoss.json.JSONs;
import org.zkoss.lang.Generics;
import org.zkoss.lang.Strings;
import org.zkoss.xml.XMLs;
import org.zkoss.zk.ui.Component;
import org.zkoss.zk.ui.UiException;

Expand Down Expand Up @@ -95,7 +96,7 @@ private String renderValue(String value) {
if (value == null)
return null;
else {
return Strings.escapeJavaScript(value);
return Strings.escapeJavaScript(XMLs.escapeXML(value));
}
}

Expand Down
3 changes: 2 additions & 1 deletion zk/src/main/java/org/zkoss/zk/ui/sys/JsContentRenderer.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.zkoss.json.JSONAware;
import org.zkoss.json.JSONs;
import org.zkoss.lang.Strings;
import org.zkoss.xml.XMLs;
import org.zkoss.zk.ui.Component;
import org.zkoss.zk.ui.UiException;

Expand Down Expand Up @@ -153,7 +154,7 @@ private void renderValue(String value) {
_buf.append((String) null);
else {
_buf.append('\'');
_buf.append(Strings.escapeJavaScript(value));
_buf.append(Strings.escapeJavaScript(XMLs.escapeXML(value)));
_buf.append('\'');
}
}
Expand Down
2 changes: 1 addition & 1 deletion zk/src/main/resources/web/js/zk/widget.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3160,7 +3160,7 @@ new zul.wnd.Window({
if ((s = this.domClass_(no)))
out += ' class="' + s + '"';
if ((s = this.domTooltiptext_()))
out += ' title="' + zUtl.encodeXML(s) + '"'; // ZK-676
out += ' title="' + s + '"'; // ZK-676
if ((tabIndex = this.getTabindex()) != undefined)
out += ' tabindex="' + tabIndex + '"';
} else {
Expand Down
6 changes: 6 additions & 0 deletions zkdoc/release-note
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ ZK 10.0.0
ZK-5048: MVVM DebuggerFactory should log via SLF4J
ZK-5595: Upgrade Servlet version from 2.4 to 3.1 aligned with Java EE 7
ZK-5596: Simplify the JavaScript url when enable embedded mode
ZK-5182: Prevent XSS issue in component attributes

* Bugs
ZK-5393: Update ZK jars to jakarta-friendly uploads
Expand All @@ -20,6 +21,7 @@ ZK 10.0.0
ZK-5569: Radiogroup onCheck event type mismatch
ZK-5161: page directive's attributes are not encoded before rendering into HTML
ZK-5598: CKEditor requests ExecutionImpl.encodeURL with null value, causes NPE in embedded mode
ZK-5162: emptyMessage is not escaped with HTML characters

* Upgrade Notes
+ Upgrade commons-fileupload to commons-fileupload2-javax 2.0.0-M1 and commons-io to 2.13.0 to support jakarta-friendly uploads
Expand All @@ -29,6 +31,10 @@ ZK 10.0.0
+ Remove all deprecated Java APIs and the legacy module "zkplus-legacy".
+ Use /zkEmbedded url (or specify yourself) to simply include embedded.js, instead of specify the real path to embedded.js,
simplifying the source to enable embedded mode.
+ To prevent XSS issue in component attributes, all component attributes will be
encoded before rendering into HTML. If you want to render HTML in component attributes,
please use the corresponding SafeHtmlValue object APIs instead of using HTML characters.
Such as Comboitem, Menu, Navitem, and HTML components.

--------
ZK 10.0.0-Beta
Expand Down
24 changes: 24 additions & 0 deletions zktest/src/main/webapp/test2/B100-ZK-5162.zul
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?xml version="1.0" encoding="UTF-8"?>

<!--
B100-ZK-5162.zul
Purpose:
Description:
History:
2023/12/20, Created by jumperchen
Copyright (C) 2023 Potix Corporation. All Rights Reserved.
-->
<zk>
<zscript><![CDATA[
String message = "</span><script>alert('XSS')</script>"; //might read from an external source
String script = "<script>alert('XSS')</script>";
]]></zscript>
<grid emptyMessage="${script}">
</grid>
<listbox emptyMessage="${script}"/>
</zk>
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/* B100_ZK_5162Test.java
Purpose:
Description:
History:
11:57 AM 2023/12/20, Created by jumperchen
Copyright (C) 2023 Potix Corporation. All Rights Reserved.
*/
package org.zkoss.zktest.zats.test2;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.fail;

import org.junit.jupiter.api.Test;
import org.openqa.selenium.NoAlertPresentException;

import org.zkoss.test.webdriver.WebDriverTestCase;

/**
* @author jumperchen
*/
public class B100_ZK_5162Test extends WebDriverTestCase {
@Test
public void test() {
connect();
try {
driver.switchTo().alert();
fail("Should not be here");
} catch (NoAlertPresentException ex) {
// ignore
}
assertEquals("<script>alert('XSS')</script>", jq(".z-grid-emptybody-content").text());
assertEquals("<script>alert('XSS')</script>", jq(".z-listbox-emptybody-content").text());
}
}
47 changes: 33 additions & 14 deletions zul/src/main/java/org/zkoss/zul/Comboitem.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
import java.io.Serializable;

import org.zkoss.lang.Objects;
import org.zkoss.lang.Strings;
import org.zkoss.xml.XMLs;
import org.zkoss.zk.ui.Component;
import org.zkoss.zk.ui.SafeHtmlValue;
import org.zkoss.zk.ui.UiException;
import org.zkoss.zul.impl.LabelImageElement;

Expand All @@ -36,7 +39,7 @@
public class Comboitem extends LabelImageElement implements org.zkoss.zk.ui.ext.Disable {
private String _desc = "";
private transient Object _value;
private String _content = "";
private SafeHtmlValue _content = SafeHtmlValue.EMPTY;
private boolean _disabled;
private transient int _index;

Expand Down Expand Up @@ -101,34 +104,50 @@ public void setDescription(String desc) {
* @since 3.0.0
*/
public String getContent() {
return _content;
return _content.toString();
}

/** Sets the embedded content (i.e., HTML tags) that is
/** Returns the embedded content (i.e., HTML tags) that is
* shown as part of the description.
*
* <p>It is useful to show the description in more versatile way.
*
* <p>Default: empty ("").
*
* <p>Deriving class can override it to return whatever it wants
* other than null.
* @see #getDescription
* @since 10.0.0
*/
public SafeHtmlValue getRawContent() {
return _content;
}

/** Sets the embedded content that is
* shown as part of the description.
*
* <h3>Security Note</h3>
* <p>Unlike other methods, the content assigned to this method
* is generated directly to the browser without escaping.
* Thus, it is better not to have something input by the user to avoid
* any <a href="http://books.zkoss.org/wiki/ZK_Developer%27s_Reference/Security_Tips/Cross-site_scripting">XSS</a>
* attach.
* <p>Since 10.0.0, the content assigned to this method will be escaped by default.
* To avoid escaping, use {@link #setContent(SafeHtmlValue)} instead.
* @see #setDescription
* @since 3.0.0
*/
public void setContent(String content) {
if (content == null)
content = "";
content = Strings.escapeJavaScript(XMLs.escapeXML(content));
if (!Objects.equals(_content, SafeHtmlValue.valueOf(content))) {
_content = SafeHtmlValue.valueOf(content);
smartUpdate("content", getRawContent());
}
}

/** Sets the embedded content (i.e., HTML tags) that is
* shown as part of the description.
* @since 10.0.0
*/
public void setContent(SafeHtmlValue content) {
if (content == null)
content = SafeHtmlValue.EMPTY;
if (!Objects.equals(_content, content)) {
_content = content;
smartUpdate("content", getContent()); //allow overriding getContent()
smartUpdate("content", getRawContent());
}
}

Expand Down Expand Up @@ -203,7 +222,7 @@ protected void renderProperties(org.zkoss.zk.ui.sys.ContentRenderer renderer) th

render(renderer, "disabled", _disabled);
render(renderer, "description", getDescription()); //allow overriding getDescription()
render(renderer, "content", getContent()); //allow overriding getContent()
render(renderer, "content", getRawContent()); //allow overriding getContent()

if (_value instanceof String) {
render(renderer, "value", _value);
Expand Down
Loading

0 comments on commit 035f86e

Please sign in to comment.