Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1. ZK-5182: Prevent XSS issue in component attributes #3099

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
9 changes: 5 additions & 4 deletions zk/src/main/java/org/zkoss/zk/au/out/AuAlert.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.zkoss.zk.au.out;

import org.zkoss.xml.XMLs;
import org.zkoss.zk.au.AuResponse;

/**
Expand All @@ -28,7 +29,7 @@
*/
public class AuAlert extends AuResponse {
public AuAlert(String message) {
super("alert", message); //component-independent
super("alert", XMLs.encodeText(message)); //component-independent
}

/**
Expand All @@ -39,7 +40,7 @@ public AuAlert(String message) {
* @since 7.0.0
*/
public AuAlert(String message, boolean disabledAuRequest) {
super("alert", new Object[] { message, null, null, disabledAuRequest }); //component-independent
super("alert", new Object[] { XMLs.encodeText(message), null, null, disabledAuRequest }); //component-independent
}

/**
Expand All @@ -48,7 +49,7 @@ public AuAlert(String message, boolean disabledAuRequest) {
* @since 5.0.3
*/
public AuAlert(String message, String title) {
super("alert", new String[] { message, title }); //component-independent
super("alert", new String[] { XMLs.encodeText(message), title }); //component-independent
}

/**
Expand All @@ -60,6 +61,6 @@ public AuAlert(String message, String title) {
* @since 5.0.3
*/
public AuAlert(String message, String title, String icon) {
super("alert", new String[] { message, title, icon }); //component-independent
super("alert", new String[] { XMLs.encodeText(message), title, icon }); //component-independent
}
}
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 _value;
}

@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
9 changes: 6 additions & 3 deletions zk/src/main/resources/web/js/zk/utl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ export interface EncodeXmlOptions {
pre?: boolean;
multiline?: boolean;
maxlength?: number;
encode?: boolean;
}

export interface ProgressboxOptions {
Expand Down Expand Up @@ -241,6 +242,7 @@ export namespace utl_global {
* <li>pre - whether to replace whitespace with `&nbsp;`</li>
* <li>multiline - whether to replace linefeed with `<br>`</li>
* <li>maxlength - the maximal allowed length of the text</li>
* <li>encode - false to not encode the text</li>
* </ul>
* @returns the encoded text.
*/
Expand All @@ -254,7 +256,8 @@ export namespace utl_global {
var tl = txt.length,
pre = opts && opts.pre,
multiline = pre || (opts && opts.multiline),
maxlength = opts ? opts.maxlength : 0;
maxlength = opts ? opts.maxlength : 0,
encode = opts ? opts.encode : true;

if (!multiline && maxlength && tl > maxlength) {
var j = maxlength;
Expand All @@ -265,7 +268,7 @@ export namespace utl_global {
}

var out = '', k = 0, enc;
if (multiline || pre) {
if (encode !== false && (multiline || pre)) {
for (let j = 0; j < tl; ++j) {
var cc = txt.charAt(j);
if (enc = _encs[cc] as undefined | string) {
Expand All @@ -283,7 +286,7 @@ export namespace utl_global {
}
} else {
// fixed B65-ZK-1836 that opt may be an empty object.
return _encodeXML0(txt);
return encode !== false ? _encodeXML0(txt) : txt;
}

if (!k) return txt;
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());
}
}
Loading
Loading