opendev/gerrit
Code Issues Proposed changes

InlineEdit: Fix mangling of non-ASCII characters

Browse Source 
atob() does not handle Unicode well.  The decoding routine creates a
Unicode DOM string from the decoded bytes effectively treating the
stream as ISO-8859-1.  This breaks non-ASCII by decomposing characters
into several meaningless "characters".

Instead of decoding base64 in the browser, modify the server to return
base64() BinaryResult as a JSON string if the only value of the Accept
request header is application/json.  RestApi client code currently
always sets "Accept: application/json" in the request, as the browser
JS strongly prefers JSON in the response body.

Relying on the native JavaScript parser in the browser to read and
decode the JSON string is smaller and more efficient than a pure
JavaScript implementation of base64 decoding that supports UTF-8.

Fix the Content-Type used during POST and PUT to document charset is
UTF-8.  The XMLHttpRequest standard requires browsers to always encode
in UTF-8, however the server examines the request's Content-Type
header to extract the charset attribute. Without setting the charset
an arbitrary default encoding was chosen by the server, which may still
mangle a commit message or file during save.

Bug: issue 3093
Change-Id: I549037bfb82aff15bbffc8b4a01e8955ae0ce318
This commit is contained in:
Shawn Pearce 2015-02-05 12:18:20 -08:00
parent 28a21de8fa
commit 4cee8e9d16
3 changed files with 110 additions and 25 deletions
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi
BinaryResult.java
gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc
RestApi.java
gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi
RestApiServlet.java

78
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/restapi/BinaryResult.java

@ -14,11 +14,17 @@
package com.google.gerrit.extensions.restapi;
import java.io.ByteArrayOutputStream;
import java.io.Closeable;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.io.UnsupportedEncodingException;
import java.nio.ByteBuffer;
import java.nio.charset.CharacterCodingException;
import java.nio.charset.Charset;
import java.nio.charset.CodingErrorAction;
import java.nio.charset.StandardCharsets;
import java.nio.charset.UnsupportedCharsetException;
/**
* Wrapper around a non-JSON result from a {@link RestView}.
@ -34,13 +40,7 @@ public abstract class BinaryResult implements Closeable {
/** Produce a UTF-8 encoded result from a string. */
public static BinaryResult create(String data) {
try {
return create(data.getBytes("UTF-8"))
.setContentType("text/plain")
.setCharacterEncoding("UTF-8");
} catch (UnsupportedEncodingException e) {
throw new RuntimeException("JVM does not support UTF-8", e);
}
return new StringResult(data);
}
/** Produce an {@code application/octet-stream} result from a byte array. */
@ -144,6 +144,28 @@ public abstract class BinaryResult implements Closeable {
*/
public abstract void writeTo(OutputStream os) throws IOException;
/**
* Return a copy of the result as a String.
* <p>
* The default version of this method copies the result into a temporary byte
* array and then tries to decode it using the configured encoding.
*
* @return string version of the result.
* @throws IOException if the data cannot be produced or could not be
* decoded to a String.
*/
public String asString() throws IOException {
long len = getContentLength();
ByteArrayOutputStream buf;
if (0 < len) {
buf = new ByteArrayOutputStream((int) len);
} else {
buf = new ByteArrayOutputStream();
}
writeTo(buf);
return decode(buf.toByteArray(), getCharacterEncoding());
}
/** Close the result and release any resources it holds. */
@Override
public void close() throws IOException {
@ -161,6 +183,25 @@ public abstract class BinaryResult implements Closeable {
getContentType());
}
private static String decode(byte[] data, String enc) {
try {
Charset cs = enc != null
? Charset.forName(enc)
: StandardCharsets.UTF_8;
return cs.newDecoder()
.onMalformedInput(CodingErrorAction.REPORT)
.onUnmappableCharacter(CodingErrorAction.REPORT)
.decode(ByteBuffer.wrap(data))
.toString();
} catch (UnsupportedCharsetException | CharacterCodingException e) {
// Fallback to ISO-8850-1 style encoding.
StringBuilder r = new StringBuilder(data.length);
for (byte b : data)
r.append((char) (b & 0xff));
return r.toString();
}
}
private static class Array extends BinaryResult {
private final byte[] data;
@ -173,6 +214,27 @@ public abstract class BinaryResult implements Closeable {
public void writeTo(OutputStream os) throws IOException {
os.write(data);
}
@Override
public String asString() {
return decode(data, getCharacterEncoding());
}
}
private static class StringResult extends Array {
private final String str;
StringResult(String str) {
super(str.getBytes(StandardCharsets.UTF_8));
setContentType("text/plain");
setCharacterEncoding("UTF-8");
this.str = str;
}
@Override
public String asString() {
return str;
}
}
private static class Stream extends BinaryResult {

31
gerrit-gwtui/src/main/java/com/google/gerrit/client/rpc/RestApi.java

@ -45,6 +45,7 @@ public class RestApi {
private static final String JSON_TYPE = "application/json";
private static final String JSON_UTF8 = JSON_TYPE + "; charset=utf-8";
private static final String TEXT_TYPE = "text/plain";
private static final String TEXT_UTF8 = TEXT_TYPE + "; charset=utf-8";
/**
* Expected JSON content body prefix that prevents XSSI.
@ -129,9 +130,14 @@ public class RestApi {
final String type;
if (isJsonBody(res)) {
try {
// javac generics bug
data = RestApi.<T> cast(parseJson(res));
type = JSON_TYPE;
JSONValue val = parseJson(res);
if (isJsonEncoded(res) && val.isString() != null) {
data = NativeString.wrap(val.isString().stringValue()).cast();
type = simpleType(res.getHeader("X-FYI-Content-Type"));
} else {
data = RestApi.<T> cast(val);
type = JSON_TYPE;
}
} catch (JSONException e) {
if (!background) {
RpcStatus.INSTANCE.onRpcComplete();
@ -140,9 +146,6 @@ public class RestApi {
"Invalid JSON: " + e.getMessage()));
return;
}
} else if (isEncodedBase64(res)) {
data = NativeString.wrap(decodeBase64(res.getText())).cast();
type = simpleType(res.getHeader("X-FYI-Content-Type"));
} else if (isTextBody(res)) {
data = NativeString.wrap(res.getText()).cast();
type = TEXT_TYPE;
@ -371,7 +374,7 @@ public class RestApi {
public <T extends JavaScriptObject> void post(String content,
HttpCallback<T> cb) {
sendRaw(POST, content, cb);
sendText(POST, content, cb);
}
public <T extends JavaScriptObject> void put(AsyncCallback<T> cb) {
@ -389,7 +392,7 @@ public class RestApi {
public <T extends JavaScriptObject> void put(String content,
HttpCallback<T> cb) {
sendRaw(PUT, content, cb);
sendText(PUT, content, cb);
}
public <T extends JavaScriptObject> void put(
@ -423,10 +426,7 @@ public class RestApi {
private static native String str(JavaScriptObject jso)
/*-{ return JSON.stringify(jso) }-*/;
private static native String decodeBase64(String a)
/*-{ return $wnd.atob(a) }-*/;
private <T extends JavaScriptObject> void sendRaw(Method method, String body,
private <T extends JavaScriptObject> void sendText(Method method, String body,
HttpCallback<T> cb) {
HttpImpl<T> httpCallback = new HttpImpl<>(background, cb);
try {
@ -434,7 +434,7 @@ public class RestApi {
RpcStatus.INSTANCE.onRpcStart();
}
RequestBuilder req = request(method);
req.setHeader("Content-Type", TEXT_TYPE);
req.setHeader("Content-Type", TEXT_UTF8);
req.sendRequest(body, httpCallback);
} catch (RequestException e) {
httpCallback.onError(null, e);
@ -461,9 +461,8 @@ public class RestApi {
return isContentType(res, TEXT_TYPE);
}
private static boolean isEncodedBase64(Response res) {
return "base64".equals(res.getHeader("X-FYI-Content-Encoding"))
&& isTextBody(res);
private static boolean isJsonEncoded(Response res) {
return "json".equals(res.getHeader("X-FYI-Content-Encoding"));
}
private static boolean isContentType(Response res, String want) {

26
gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java

@ -91,6 +91,7 @@ import com.google.gson.JsonParseException;
import com.google.gson.JsonPrimitive;
import com.google.gson.stream.JsonReader;
import com.google.gson.stream.JsonToken;
import com.google.gson.stream.JsonWriter;
import com.google.gson.stream.MalformedJsonException;
import com.google.gwtexpui.server.CacheHeaders;
import com.google.inject.Inject;
@ -719,6 +720,7 @@ public class RestApiServlet extends HttpServlet {
}
}
@SuppressWarnings("resource")
static void replyBinaryResult(
@Nullable HttpServletRequest req,
HttpServletResponse res,
@ -731,7 +733,11 @@ public class RestApiServlet extends HttpServlet {
"attachment; filename=\"" + bin.getAttachmentName() + "\"");
}
if (bin.isBase64()) {
bin = stackBase64(res, bin);
if (req != null && JSON_TYPE.equals(req.getHeader(HttpHeaders.ACCEPT))) {
bin = stackJsonString(res, bin);
} else {
bin = stackBase64(res, bin);
}
}
if (bin.canGzip() && acceptsGzip(req)) {
bin = stackGzip(res, bin);
@ -758,6 +764,24 @@ public class RestApiServlet extends HttpServlet {
}
}
private static BinaryResult stackJsonString(HttpServletResponse res,
final BinaryResult src) throws IOException {
TemporaryBuffer.Heap buf = heap(Integer.MAX_VALUE);
buf.write(JSON_MAGIC);
try(Writer w = new BufferedWriter(new OutputStreamWriter(buf, UTF_8));
JsonWriter json = new JsonWriter(w)) {
json.setLenient(true);
json.setHtmlSafe(true);
json.value(src.asString());
w.write('\n');
}
res.setHeader("X-FYI-Content-Encoding", "json");
res.setHeader("X-FYI-Content-Type", src.getContentType());
return asBinaryResult(buf)
.setContentType(JSON_TYPE)
.setCharacterEncoding(UTF_8.name());
}
private static BinaryResult stackBase64(HttpServletResponse res,
final BinaryResult src) throws IOException {
BinaryResult b64;