Skip to content

Commit

Permalink
[SECURITY-1204] XSS escape beforce outputting the value in json writer
Browse files Browse the repository at this point in the history
For extra safety, cdata the scripts so no rogue tags can break out
  • Loading branch information
Gavin Mogan authored and halkeye committed Jan 16, 2019
1 parent 26958d6 commit 74d511f
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,20 @@ public class Export {
*/
@Nonnull
public static String toJson(@Nonnull Object object) throws IOException {
return toJson(object, false);
}

/**
* Serialize the supplied object to JSON and return as a {@link String}.
* @param object The object to serialize.
* @param boolean enable html encoding so its safe to output to html
* @return The JSON as a {@link String}.
* @throws IOException Error serializing model object.
*/
@Nonnull
public static String toJson(@Nonnull Object object, boolean htmlEncoded) throws IOException {
try (StringWriter writer = new StringWriter()) {
toJson(object, writer);
toJson(object, writer, htmlEncoded);
return writer.toString();
}
}
Expand All @@ -57,8 +69,24 @@ public static String toJson(@Nonnull Object object) throws IOException {
*/
@SuppressWarnings("unchecked")
public static void toJson(@Nonnull Object object, @Nonnull Writer writer) throws IOException {
toJson(object, writer, false);
}


/**
* Serialize the supplied object to JSON and write to the supplied {@link Writer}.
* @param object The object to serialize.
* @param writer The writer to output to.
* @throws IOException Error serializing model object.
*/
@SuppressWarnings("unchecked")
public static void toJson(@Nonnull Object object, @Nonnull Writer writer, boolean htmlEncoded) throws IOException {
Model model = new ModelBuilder().get(object.getClass());
model.writeTo(object, Flavor.JSON.createDataWriter(object, writer, createExportConfig()));
ExportConfig exportConfig = createExportConfig();
if (htmlEncoded) {
exportConfig.withHtmlEncode(true);
}
model.writeTo(object, Flavor.JSON.createDataWriter(object, writer, exportConfig));
writer.flush();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ public class ExportConfig {

private Flavor flavor = Flavor.JSON;

private boolean htmlEncode = false;

/**
* If true, output will be indented to make it easier for humans to understand.
*/
Expand Down Expand Up @@ -83,4 +85,19 @@ public ExportConfig withFlavor(Flavor flavor){
this.flavor = flavor;
return this;
}

/**
* If true, output will escaped to be embedded into html
*/
public boolean isHtmlEncode() {
return htmlEncode;
}

/**
* If true, output will escaped to be embedded into html
*/
public ExportConfig withHtmlEncode(boolean htmlEncode) {
this.htmlEncode = htmlEncode;
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
package io.jenkins.blueocean.commons.stapler.export;

import com.fasterxml.jackson.core.io.JsonStringEncoder;
import org.apache.commons.lang.StringEscapeUtils;

import javax.annotation.Nonnull;
import java.io.IOException;
Expand Down Expand Up @@ -109,7 +110,11 @@ public void value(String v) throws IOException {
StringBuilder buf = new StringBuilder(v.length());
buf.append('\"');
// TODO: remove when JENKINS-45099 has been fixed correctly in upstream stapler
jsonEncoder.quoteAsString(v, buf);
if (config.isHtmlEncode()) {
jsonEncoder.quoteAsString(StringEscapeUtils.escapeHtml(v), buf);
} else {
jsonEncoder.quoteAsString(v, buf);
}
buf.append('\"');
data(buf.toString());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,13 @@

public class JSONDataWriterTest {
private ExportConfig config = new ExportConfig().withFlavor(Flavor.JSON).withClassAttribute(ClassAttributeBehaviour.IF_NEEDED.simple());
private ExportConfig configWithHtmlEncode = new ExportConfig().withFlavor(Flavor.JSON).withClassAttribute(ClassAttributeBehaviour.IF_NEEDED.simple()).withHtmlEncode(true);

private <T> String serialize(T bean, Class<T> clazz) throws IOException {
return serialize(bean, clazz, config);
}

private <T> String serialize(T bean, Class<T> clazz, ExportConfig config) throws IOException {
StringWriter w = new StringWriter();
Model<T> model = new ModelBuilder().get(clazz);
model.writeTo(bean, Flavor.JSON.createDataWriter(bean, w, config));
Expand Down Expand Up @@ -78,4 +83,14 @@ public Supers(Super... elements) {
}
}

@ExportedBean public static class Escaping {
@Exported public String specific() {return "<script>alert('hi')</script>";}
}

@Test
public void testEscaping() throws Exception {
assertEquals("{\"_class\":\"Escaping\",\"specific\":\"&lt;script&gt;alert('hi')&lt;/script&gt;\"}",
serialize(new Escaping(), Escaping.class, configWithHtmlEncode));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public String getStateJson() {
try {
User currentUser = User.current();
if (currentUser != null && organization != null) {
return Export.toJson(new UserImpl(organization, currentUser));
return Export.toJson(new UserImpl(organization, currentUser), true);
} else {
return ANONYMOUS;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?jelly escape-by-default='false'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler">
<script>
<script>//&lt;![CDATA[
// construct the state object parent path inside window.$blueocean.
var stateRoot = window.$blueocean = (window.$blueocean || {});
(function () {
Expand Down Expand Up @@ -37,5 +37,6 @@
</j:if>
</j:forEach>
})();
//]]&gt;
</script>
</j:jelly>

0 comments on commit 74d511f

Please sign in to comment.