Skip to content

Commit

Permalink
#2985 Prevent XSS for REST API by escape String content:
Browse files Browse the repository at this point in the history
- Using class XssProtectUtils (use Spring) instead of Security class from dwr;
_ Rename classes: XssProtectHtmlUtils to XssProtectUtils, XssUtils to XssValidatorUtils;
- Changed private on public no args constructor for class XssProtectUtils;
- Added Data Point information to console log in ScriptExecutor.convertContext;
  • Loading branch information
Limraj committed Nov 25, 2024
1 parent a89321f commit def8af2
Show file tree
Hide file tree
Showing 22 changed files with 68 additions and 70 deletions.
10 changes: 5 additions & 5 deletions src/br/org/scadabr/view/component/ChartComparatorComponent.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import org.scada_lts.dao.model.ScadaObjectIdentifier;
import org.scada_lts.permissions.service.GetDataPointsWithAccess;

import static org.scada_lts.web.security.XssProtectHtmlUtils.escape;
import static org.scada_lts.web.security.XssProtectUtils.escapeHtml;


@JsonRemoteEntity
Expand Down Expand Up @@ -116,7 +116,7 @@ private String createDataPointsSelectComponent(String idPrefix, List<ScadaObject
sb.append("<option value='0'> &nbsp; </option>");

for (ScadaObjectIdentifier dp : dataPoints) {
sb.append("<option value='" + dp.getId() + "'> " + escape(dp.getName())
sb.append("<option value='" + dp.getId() + "'> " + escapeHtml(dp.getName())
+ "</option>");
}
sb.append("</select>");
Expand All @@ -138,10 +138,10 @@ private String createDateRangeComponent(String idPrefix, String fromDateId,
sb.append("<table>");
sb.append("<tr> <td> De </td> <td> A </td> </tr>");
sb.append("<tr> <td><input type='text' class='formField' id='"
+ escape(fromDateId) + "' value='" + escape(defaultFromDateString)
+ escapeHtml(fromDateId) + "' value='" + escapeHtml(defaultFromDateString)
+ "'/> </td> "
+ "<td> <input type='text' class='formField' id='" + escape(toDateId)
+ "' value='" + escape(defaultToDateString) + "'/> </td> </tr>");
+ "<td> <input type='text' class='formField' id='" + escapeHtml(toDateId)
+ "' value='" + escapeHtml(defaultToDateString) + "'/> </td> </tr>");
sb.append("</table>");
return sb.toString();
}
Expand Down
6 changes: 3 additions & 3 deletions src/br/org/scadabr/view/component/LinkComponent.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import com.serotonin.mango.view.component.ViewComponent;
import com.serotonin.util.SerializationHelper;

import static org.scada_lts.web.security.XssProtectHtmlUtils.escape;
import static org.scada_lts.web.security.XssProtectUtils.escapeHtml;

@JsonRemoteEntity
public class LinkComponent extends HtmlComponent {
Expand Down Expand Up @@ -50,8 +50,8 @@ private void createLink() {

public String createLinkContent() {
StringBuilder sb = new StringBuilder();
sb.append("<a href='" + escape(link) + "'>");
sb.append(escape(text));
sb.append("<a href='" + escapeHtml(link) + "'>");
sb.append(escapeHtml(text));
sb.append("</a>");
return sb.toString();
}
Expand Down
6 changes: 3 additions & 3 deletions src/br/org/scadabr/view/component/ScriptButtonComponent.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import com.serotonin.mango.view.component.ViewComponent;
import com.serotonin.util.SerializationHelper;

import static org.scada_lts.web.security.XssProtectHtmlUtils.escape;
import static org.scada_lts.web.security.XssProtectUtils.escapeHtml;

@JsonRemoteEntity
public class ScriptButtonComponent extends HtmlComponent {
Expand Down Expand Up @@ -50,9 +50,9 @@ private void createScriptButton() {

public String createScriptButtonContent() {
StringBuilder sb = new StringBuilder();
sb.append("<button class='viewComponent' onclick='mango.view.executeScript(\"" + escape(scriptXid)
sb.append("<button class='viewComponent' onclick='mango.view.executeScript(\"" + escapeHtml(scriptXid)
+ "\");'>");
sb.append(escape(text));
sb.append(escapeHtml(text));
sb.append("</button>");
return sb.toString();
}
Expand Down
14 changes: 7 additions & 7 deletions src/br/org/scadabr/vo/scripting/ContextualizedScriptVO.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import com.serotonin.web.i18n.LocalizableMessage;

import static org.scada_lts.utils.ValidationDwrUtils.validateVarNameScript;
import static org.scada_lts.web.security.XssProtectHtmlUtils.escape;
import static org.scada_lts.web.security.XssProtectUtils.escapeHtml;

@JsonRemoteEntity
public class ContextualizedScriptVO extends ScriptVO<ContextualizedScriptVO>
Expand All @@ -55,16 +55,16 @@ public void validate(DwrResponseI18n response) {
}

if (!validateVarNameScript(varName)) {
response.addContextualMessage("context", "validate.invalidVarName", escape(varName));
response.addContextualMessage("context", "validate.invalidVarName", escapeHtml(varName));
break;
}

if (varNameSpace.contains(varName)) {
response.addContextualMessage("context", "validate.duplicateVarName", escape(varName));
response.addContextualMessage("context", "validate.duplicateVarName", escapeHtml(varName));
break;
}

varNameSpace.add(escape(varName));
varNameSpace.add(escapeHtml(varName));
}

for (IntValuePair point : objectsOnContext) {
Expand All @@ -76,16 +76,16 @@ public void validate(DwrResponseI18n response) {
}

if (!validateVarNameScript(varName)) {
response.addContextualMessage("context", "validate.invalidVarName", escape(varName));
response.addContextualMessage("context", "validate.invalidVarName", escapeHtml(varName));
break;
}

if (varNameSpace.contains(varName)) {
response.addContextualMessage("context", "validate.duplicateVarName", escape(varName));
response.addContextualMessage("context", "validate.duplicateVarName", escapeHtml(varName));
break;
}

varNameSpace.add(escape(varName));
varNameSpace.add(escapeHtml(varName));
}

super.validate(response);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public Map<String, IDataPoint> convertContext(List<IntValuePair> context, DataPo
if (point == null) {
LOG.error("Error DataPointRT null "
+ new Exception("key:" + contextEntry.getKey()
+ " value:" + contextEntry.getValue()));
+ " value:" + contextEntry.getValue()) + " from:" + LoggingUtils.dataPointInfo(dataPoint));
DataPointStateException dataPointStateException = createPointUnavailableException(contextEntry);
if(dataPoint != null && metaDataSource != null) {
metaDataSource.raiseContextError(System.currentTimeMillis(), dataPoint, dataPointStateException.getLocalizableMessage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
import com.serotonin.web.i18n.LocalizableMessage;

import static org.scada_lts.utils.ValidationDwrUtils.validateVarNameScript;
import static org.scada_lts.web.security.XssProtectHtmlUtils.escape;
import static org.scada_lts.web.security.XssProtectUtils.escapeHtml;

/**
* @author Matthew Lohbihler
Expand Down Expand Up @@ -193,7 +193,7 @@ public void validate(DwrResponseI18n response, int dataPointId) {
int pointId = point.getKey();

if(pointId != Common.NEW_ID && pointId == dataPointId) {
response.addContextualMessage("context", "validate.invalidVariable", escape(varName));
response.addContextualMessage("context", "validate.invalidVariable", escapeHtml(varName));
break;
}

Expand All @@ -203,12 +203,12 @@ public void validate(DwrResponseI18n response, int dataPointId) {
}

if (!validateVarNameScript(varName)) {
response.addContextualMessage("context", "validate.invalidVarName", escape(varName));
response.addContextualMessage("context", "validate.invalidVarName", escapeHtml(varName));
break;
}

if (varNameSpace.contains(varName)) {
response.addContextualMessage("context", "validate.duplicateVarName", escape(varName));
response.addContextualMessage("context", "validate.duplicateVarName", escapeHtml(varName));
break;
}

Expand Down
4 changes: 2 additions & 2 deletions src/com/serotonin/mango/vo/report/ReportChartCreator.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@
import freemarker.template.Template;
import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.directwebremoting.Security;
import org.jfree.data.time.TimeSeries;
import org.scada_lts.mango.service.SystemSettingsService;
import org.scada_lts.utils.ColorUtils;
import com.serotonin.mango.util.DateUtils;
import org.scada_lts.web.security.XssProtectUtils;

import java.awt.*;
import java.io.*;
Expand Down Expand Up @@ -124,7 +124,7 @@ public void createContent(ReportInstance reportInstance, ReportDao reportDao, St
model.put("instance", reportInstance);
model.put("points", pointStatistics);
model.put("inline", inlinePrefix == null ? "" : "cid:");
model.put("security", new Security());
model.put("security", new XssProtectUtils());

model.put("ALPHANUMERIC", DataTypes.ALPHANUMERIC);
model.put("BINARY", DataTypes.BINARY);
Expand Down
4 changes: 1 addition & 3 deletions src/com/serotonin/mango/vo/report/SeriesIdentifier.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.serotonin.mango.vo.report;

import org.scada_lts.web.security.XssProtectHtmlUtils;

import java.util.Objects;

public class SeriesIdentifier implements Comparable<SeriesIdentifier> {
Expand Down Expand Up @@ -38,6 +36,6 @@ public int hashCode() {

@Override
public String toString() {
return XssProtectHtmlUtils.escape(name);
return name;
}
}
6 changes: 3 additions & 3 deletions src/com/serotonin/mango/web/dwr/ViewDwr.java
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
import static com.serotonin.mango.web.dwr.util.AnonymousUserUtils.getRequest;
import static com.serotonin.mango.web.dwr.util.AnonymousUserUtils.getResponse;
import static com.serotonin.mango.web.dwr.util.AnonymousUserUtils.authenticateAnonymousUser;
import static org.scada_lts.web.security.XssProtectHtmlUtils.escape;
import static org.scada_lts.web.security.XssProtectUtils.escapeHtml;

/**
* This class is so not threadsafe. Do not use class fields except for the
Expand Down Expand Up @@ -212,9 +212,9 @@ public List<ViewComponentState> getViewPointData(User user, View view, boolean e
if (point != null) {
Map<String, Object> map = new HashMap<String, Object>();
if (imageChart)
map.put("name", escape(point.getName()));
map.put("name", escapeHtml(point.getName()));
else
map.put("name", escape(getMessage(child.getDescription())));
map.put("name", escapeHtml(getMessage(child.getDescription())));
map.put("point", point);
map.put("pointValue", point.lastValue());
childData.add(map);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@
import org.directwebremoting.extend.OutboundContext;
import org.directwebremoting.extend.OutboundVariable;

import static org.scada_lts.web.security.XssProtectHtmlUtils.escape;
import static org.scada_lts.web.security.XssProtectUtils.escapeHtml;

public class XssDataPointBeanConverter extends BeanConverter {

@Override
public OutboundVariable convertOutbound(Object data, OutboundContext outctx) throws MarshallException {
DataPointBean dataPointBean = (DataPointBean)data;
dataPointBean.setName(escape(dataPointBean.getName()));
dataPointBean.setXid(escape(dataPointBean.getXid()));
dataPointBean.setName(escapeHtml(dataPointBean.getName()));
dataPointBean.setXid(escapeHtml(dataPointBean.getXid()));
return super.convertOutbound(dataPointBean, outctx);
}
}
16 changes: 8 additions & 8 deletions src/com/serotonin/mango/web/dwr/XssDataPointVoConverter.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,20 @@
import org.directwebremoting.convert.BeanConverter;
import org.directwebremoting.extend.*;

import static org.scada_lts.web.security.XssProtectHtmlUtils.escape;
import static org.scada_lts.web.security.XssProtectUtils.escapeHtml;

public class XssDataPointVoConverter extends BeanConverter {

@Override
public OutboundVariable convertOutbound(Object data, OutboundContext outctx) throws MarshallException {
DataPointVO dataPointVo = (DataPointVO)data;
dataPointVo.setName(escape(dataPointVo.getName()));
dataPointVo.setXid(escape(dataPointVo.getXid()));
dataPointVo.setDataSourceXid(escape(dataPointVo.getDataSourceXid()));
dataPointVo.setChartColour(escape(dataPointVo.getChartColour()));
dataPointVo.setDataSourceName(escape(dataPointVo.getDataSourceName()));
dataPointVo.setDescription(escape(dataPointVo.getDescription()));
dataPointVo.setDeviceName(escape(dataPointVo.getDeviceName()));
dataPointVo.setName(escapeHtml(dataPointVo.getName()));
dataPointVo.setXid(escapeHtml(dataPointVo.getXid()));
dataPointVo.setDataSourceXid(escapeHtml(dataPointVo.getDataSourceXid()));
dataPointVo.setChartColour(escapeHtml(dataPointVo.getChartColour()));
dataPointVo.setDataSourceName(escapeHtml(dataPointVo.getDataSourceName()));
dataPointVo.setDescription(escapeHtml(dataPointVo.getDescription()));
dataPointVo.setDeviceName(escapeHtml(dataPointVo.getDeviceName()));
return super.convertOutbound(dataPointVo, outctx);
}
}
12 changes: 6 additions & 6 deletions src/com/serotonin/mango/web/taglib/Functions.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
import com.serotonin.util.StringUtils;
import com.serotonin.web.taglib.DateFunctions;

import static org.scada_lts.web.security.XssProtectHtmlUtils.escape;
import static org.scada_lts.web.security.XssProtectUtils.escapeHtml;

public class Functions {
public static String getHtmlText(DataPointVO point, PointValueTime pointValue) {
Expand Down Expand Up @@ -81,16 +81,16 @@ private static String getHtml(String colour, String text, boolean detectOverflow

if (text != null && detectOverflow && text.length() > 30) {
if (StringUtils.isEmpty(colour))
result = "<input type='text' readonly='readonly' class='ovrflw' value=\"" + escape(text) + "\"/>";
result = "<input type='text' readonly='readonly' class='ovrflw' value=\"" + escapeHtml(text) + "\"/>";
else
result = "<input type='text' readonly='readonly' class='ovrflw' style='color:" + escape(colour) + ";' value=\""
+ escape(text) + "\"/>";
result = "<input type='text' readonly='readonly' class='ovrflw' style='color:" + escapeHtml(colour) + ";' value=\""
+ escapeHtml(text) + "\"/>";
}
else {
if (StringUtils.isEmpty(colour))
result = "<span>" + escape(text) + "</span>";
result = "<span>" + escapeHtml(text) + "</span>";
else
result = "<span style='color:" + escape(colour) + ";'>" + escape(text) + "</span>";
result = "<span style='color:" + escapeHtml(colour) + ";'>" + escapeHtml(text) + "</span>";
}

return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import org.scada_lts.web.beans.validation.ScadaValidator;

import static org.scada_lts.web.security.XssUtils.validateHttpBody;
import static org.scada_lts.web.security.XssValidatorUtils.validateHttpBody;

public class XssValidator implements ScadaValidator<String> {

Expand Down
2 changes: 1 addition & 1 deletion src/org/scada_lts/web/security/XssFilter.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
throws ServletException, IOException {

String queryString = request.getQueryString();
if (queryString != null && !XssUtils.validateHttpQuery(queryString)) {
if (queryString != null && !XssValidatorUtils.validateHttpQuery(queryString)) {
LOG.warn("Potential XSS detected in request. Request URI: {}, Query: {}",
request.getRequestURI(), queryString);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public XssProtectCssStyleSerializer() {

@Override
public void serialize(CssStyle value, JsonGenerator jgen, SerializerProvider provider) throws IOException {
String content = XssProtectHtmlUtils.escape(value.getContent());
String content = XssProtectUtils.escapeHtml(value.getContent());
jgen.writeStartObject();
jgen.writeStringField("content", content);
jgen.writeEndObject();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public XssProtectStringSerializer() {

@Override
public void serialize(String value, JsonGenerator jgen, SerializerProvider provider) throws IOException {
String content = XssProtectHtmlUtils.escape(value);
String content = XssProtectUtils.escapeHtml(value);
jgen.writeString(content);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

import org.springframework.web.util.HtmlUtils;

public final class XssProtectHtmlUtils {
public final class XssProtectUtils {

private XssProtectHtmlUtils() {}
public XssProtectUtils() {}

public static String escape(String value) {
public static String escapeHtml(String value) {
if(value == null)
return "";
String content = HtmlUtils.htmlEscape(value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@
import java.util.function.Predicate;
import java.util.regex.Pattern;

public final class XssUtils {
public final class XssValidatorUtils {

private XssUtils() {}
private XssValidatorUtils() {}

private static final Pattern SECURITY_HTTP_ACCESS_DENIED_QUERY_REGEX = init(SystemSettingsUtils.getSecurityHttpQueryAccessDeniedRegex());
private static final Pattern SECURITY_HTTP_ACCESS_GRANTED_QUERY_REGEX = init(SystemSettingsUtils.getSecurityHttpQueryAccessGrantedRegex());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@
import static org.junit.Assert.assertEquals;

@RunWith(Parameterized.class)
public class BodyXssUtilsTest {
public class BodyXssValidatorUtilsTest {

private final String input;
private final boolean expectedResult;

public BodyXssUtilsTest(String input, boolean expectedResult) {
public BodyXssValidatorUtilsTest(String input, boolean expectedResult) {
this.input = input;
this.expectedResult = expectedResult;
}
Expand Down Expand Up @@ -152,7 +152,7 @@ public static Collection<Object[]> data() {
public void testValidate() {

//when:
boolean result = XssUtils.validateHttpBody(input);
boolean result = XssValidatorUtils.validateHttpBody(input);

//then:
assertEquals("Validation Body failed for input: " + input, expectedResult, result);
Expand Down
Loading

0 comments on commit def8af2

Please sign in to comment.