Skip to content

Commit

Permalink
JBIDE-15317 - LiveReload script injection corrupted html encoding
Browse files Browse the repository at this point in the history
Using Charset configured in Eclipse when reading local files.
Fallback to UTF-8 if charset is unknown
Fixed the "content-length" response header value
Also, the returned content is encoded with the charset provided in the request's
"accept" header.
  • Loading branch information
xcoulon committed Sep 4, 2013
1 parent 6b0c126 commit 57b8445
Show file tree
Hide file tree
Showing 17 changed files with 268 additions and 44 deletions.
Expand Up @@ -68,6 +68,9 @@ public static JBossLiveReloadCoreActivator getDefault() {

public InputStream getResourceContent(String path) throws IOException {
final URL resource = getBundle().getResource(path);
if(resource == null) {
return null;
}
return resource.openStream();
}
}
Expand Up @@ -36,8 +36,12 @@ public class LiveReloadProxyServer extends Server {
/**
* Constructor
*
* @param config
* the LiveReload configuration to use.
* @param proxyPort
* @param targetHost
* @param targetPort
* @param liveReloadPort
* @param allowRemoteConnections
* @param enableScriptInjection
* @throws UnknownHostException
*/
public LiveReloadProxyServer(final int proxyPort, final String targetHost, final int targetPort, final int liveReloadPort, final boolean allowRemoteConnections,
Expand Down
Expand Up @@ -13,6 +13,7 @@

import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.Charset;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
Expand All @@ -21,6 +22,7 @@

import org.apache.commons.io.IOUtils;
import org.jboss.tools.livereload.core.internal.JBossLiveReloadCoreActivator;
import org.jboss.tools.livereload.core.internal.util.HttpUtils;
import org.jboss.tools.livereload.core.internal.util.Logger;

/**
Expand All @@ -33,13 +35,26 @@ public class LiveReloadScriptFileServlet extends HttpServlet {
private static final long serialVersionUID = 163695311668462503L;

@Override
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
Logger.info("Serving embedded /livereload.js");
HttpServletResponse httpServletResponse = (HttpServletResponse) response;
final InputStream scriptContent = JBossLiveReloadCoreActivator.getDefault().getResourceContent("/script/livereload.js");
httpServletResponse.getOutputStream().write(IOUtils.toByteArray(scriptContent));
httpServletResponse.setStatus(200);
httpServletResponse.setContentType("text/javascript");
protected void doGet(final HttpServletRequest request, final HttpServletResponse response) throws ServletException, IOException {
InputStream scriptContent = null;
try {
Logger.info("Serving embedded /livereload.js");
final HttpServletResponse httpServletResponse = (HttpServletResponse) response;
scriptContent = JBossLiveReloadCoreActivator.getDefault().getResourceContent("/script/livereload.js");
if(scriptContent == null) {
httpServletResponse.setStatus(404);
} else {
final Charset charset = HttpUtils.getContentCharSet(request.getHeader("accept"), "UTF-8");

This comment has been minimized.

Copy link
@tan9

tan9 Sep 4, 2013

Can we set the default encoding form hard-coded "UTF-8" to the encoding set in eclipse project Properties / Resource / Text file encoding or eclipse Preferences / General / Workspace / Text file encoding?

This comment has been minimized.

Copy link
@xcoulon

xcoulon Sep 4, 2013

Author Member

@tan9,

What's wrong with having UTF-8 in that particular case ? UTF-8 would be the charset to use if the browser did not specify one in the "accept" header of the request.
Do you have cases where this would cause an issue ?

This comment has been minimized.

Copy link
@tan9

tan9 Sep 5, 2013

My fault, it is ok for seving lirereload.js in any character.
What I want to talk is about the sentence in your commit log:

Using Charset configured in Eclipse when reading local files. Fallback to UTF-8 if charset is unknown

If charset is unknown, can we fallback to user's preference in project or workspace? It is the business for user to set their encoding correctly, indeed.

This comment has been minimized.

Copy link
@xcoulon

xcoulon Sep 5, 2013

Author Member

@tan9

I think my commit message deserves some clarification. Here's how the charset is going to be set and used in the HTTP response:

  • if the request contains an "Accept" header, that charset will be used
  • otherwise, the LiveReload plugin will retrieve the charset for the file to read in the user's workspace
  • if this charset is unknown, then only it will fallback to UTF-8

I really doubt the charset could be unknown, to be honest. So the fallback to UTF-8 should barely happen. But since I wanted to provide a charset in any case, using UTF-8 in the last case makes sense, IMO.

We could add another layer of fallback (project or workspace level), but honestly I don't think this is necessary here.
Given the explanations above, do you agree with me ?

This comment has been minimized.

Copy link
@tan9

tan9 Sep 5, 2013

@xcoulon

Thanks for your detailed explaining. I agree with your point, and current states is 100% suit my use cases.
Please push this version into public, make users worldwide examine if there really exist the third case 👻

This comment has been minimized.

Copy link
@xcoulon

xcoulon Sep 5, 2013

Author Member

@tan9

Thanks for the confirmation. We're code-freezing JBT 4.1.1.Alpha1 today, so I expected there'll be a public release in the 2 next weeks.
Again, thanks for your contribution !

httpServletResponse.setContentType("text/javascript; charset=" + charset.name());
// output content will use the charset defined in the content type above.
httpServletResponse.getOutputStream().write(IOUtils.toByteArray(scriptContent));
httpServletResponse.setStatus(200);
}
} finally {
if(scriptContent != null) {
scriptContent.close();
}
}
}

}
Expand Up @@ -14,6 +14,9 @@
import java.io.IOException;
import java.io.InputStream;
import java.net.UnknownHostException;
import java.nio.ByteBuffer;
import java.nio.CharBuffer;
import java.nio.charset.Charset;

import javax.servlet.Filter;
import javax.servlet.FilterChain;
Expand Down Expand Up @@ -122,7 +125,8 @@ private void terminate(final HttpServletRequest httpRequest,
returnedContentType);
final InputStream responseStream = responseWrapper.getResponseAsStream();
final char[] modifiedResponseContent = ScriptInjectionUtils.injectContent(responseStream, scriptContent);
responseWrapper.terminate(modifiedResponseContent);
final Charset charset = HttpUtils.getContentCharSet(returnedContentType, "UTF-8");
responseWrapper.terminate(modifiedResponseContent, charset);
}
// finalize the responseWrapper by copying the wrapper's
// outputstream into the response outputstream that will be returned
Expand Down Expand Up @@ -184,24 +188,29 @@ public ServletOutputStream getOutputStream() throws IOException {
public InputStream getResponseAsStream() throws IOException {
final byte[] byteArray = responseOutputStream.toByteArray();
responseOutputStream.close();
return IOUtils.toInputStream(new String(byteArray), getCharacterEncoding());
final String characterEncoding = getCharacterEncoding();
return IOUtils.toInputStream(new String(byteArray, characterEncoding), characterEncoding);
}

/**
* Writes the given content into the wrapped {@link HttpServletResponse}
* and adjust the 'content-length' response header as well (in case
* content modification occurred in the response entity).
*
* @param responseContent the content of the response.
* @param encoding the ecnoding to use when writing the char[] content into the response's outputstream.
*
* @throws IOException
*/
public void terminate(final char[] responseContent) throws IOException {
((HttpServletResponse) getResponse()).setHeader("Content-length", Integer.toString(responseContent.length));
IOUtils.write(responseContent, getResponse().getOutputStream());
// getResponse().getOutputStream().flush();
// getResponse().getOutputStream().close();
public void terminate(final char[] responseContent, final Charset charset) throws IOException {
// see http://mark.koli.ch/2009/09/remember-kids-an-http-content-length-is-the-number-of-bytes-not-the-number-of-characters.html
final CharBuffer charBuffer = CharBuffer.wrap(responseContent);
final ByteBuffer byteBuffer = charset.encode(charBuffer);
((HttpServletResponse) getResponse()).setHeader("Content-length", Integer.toString(byteBuffer.array().length));
IOUtils.write(responseContent, getResponse().getOutputStream(), charset.name());

This comment has been minimized.

Copy link
@tan9

tan9 Sep 4, 2013

Here is a Chrome regression which I corrected in tan9@84d6e68 .
I have no idea why the length of byteBuffer,array() is differ than the byte array converted by IOUtils.write(), but since the charset.encode(charBuffer) has done the char[] to byte[] conversion, it should be outperforming to dump byteBuffer.array() to response.getOutputStream() :)

This comment has been minimized.

Copy link
@xcoulon

xcoulon Sep 5, 2013

Author Member

@tan9 I fixed that problem in 94af703
thanks for your help !

responseOutputStream.close();
}

/**
* Writes the content of the internal and temporary
* {@link ByteArrayOutputStream} into the wrapped
Expand Down
Expand Up @@ -11,7 +11,6 @@

package org.jboss.tools.livereload.core.internal.server.jetty;

import java.net.UnknownHostException;
import java.util.EventObject;

import org.eclipse.jetty.server.Server;
Expand Down Expand Up @@ -49,10 +48,11 @@ public class LiveReloadServer extends Server implements Subscriber {

/**
* Constructor
*
* @param config
* the LiveReload configuration to use.
* @throws UnknownHostException
* @param name the server name (appears in the Servers Views)
* @param websocketPort the websocket port
* @param enableProxyServer flag to enable the proxy server
* @param allowRemoteConnections flag to allow remote connections
* @param enableScriptInjection flag to enable script injection
*/
public LiveReloadServer(final String name, final int websocketPort, final boolean enableProxyServer,
final boolean allowRemoteConnections, final boolean enableScriptInjection) {
Expand Down
Expand Up @@ -13,6 +13,7 @@

import java.io.IOException;
import java.net.URLConnection;
import java.nio.charset.Charset;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServlet;
Expand All @@ -23,6 +24,7 @@
import org.eclipse.core.resources.IFile;
import org.eclipse.core.resources.IResource;
import org.eclipse.core.runtime.CoreException;
import org.jboss.tools.livereload.core.internal.util.HttpUtils;
import org.jboss.tools.livereload.core.internal.util.Logger;
import org.jboss.tools.livereload.core.internal.util.ResourceUtils;

Expand All @@ -36,7 +38,7 @@ public class WorkspaceFileServlet extends HttpServlet {
private static final long serialVersionUID = 163695311668462503L;

@Override
protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
protected void doGet(final HttpServletRequest request, final HttpServletResponse response) throws ServletException, IOException {
final HttpServletResponse httpServletResponse = (HttpServletResponse) response;
final String requestURI = request.getRequestURI();
Logger.info("Serving " + requestURI);
Expand All @@ -46,10 +48,17 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t
final IResource resource = ResourceUtils.retrieveResource(requestURI);
if (resource != null && resource.getType() == IResource.FILE) {
try {
final byte[] scriptContent = IOUtils.toByteArray(((IFile) resource).getContents());
final IFile workspaceFile = (IFile) resource;
final byte[] scriptContent = IOUtils.toByteArray(workspaceFile.getContents());
httpServletResponse.getOutputStream().write(scriptContent);
httpServletResponse.setStatus(200);
httpServletResponse.setContentType(URLConnection.guessContentTypeFromName(resource.getName()));

final Charset charset = HttpUtils.getContentCharSet(request.getHeader("accept"), workspaceFile.getCharset());
String guessedContentType = URLConnection.guessContentTypeFromName(resource.getName());
if(guessedContentType != null && !guessedContentType.contains("charset")) {
guessedContentType = guessedContentType.concat("; charset=").concat(charset.name());
}
httpServletResponse.setContentType(guessedContentType);
// httpServletResponse.setContentType("text/javascript");
} catch (CoreException e) {
Logger.error("Error occurred while returning content at location: " + requestURI, e);
Expand Down
Expand Up @@ -40,6 +40,7 @@ public class LiveReloadLaunchConfiguration implements ILaunchConfigurationDelega

public static final int DEFAULT_WEBSOCKET_PORT = 35729;


@Override
public void launch(ILaunchConfiguration configuration, String mode, ILaunch launch, IProgressMonitor monitor)
throws CoreException {
Expand Down
Expand Up @@ -125,7 +125,6 @@ public void startServer() throws CoreException {
final IServer server = getServer();
// retrieve the websocket port, use the default value if it was missing
websocketPort = server.getAttribute(LiveReloadLaunchConfiguration.WEBSOCKET_PORT, LiveReloadLaunchConfiguration.DEFAULT_WEBSOCKET_PORT);

// fix the new default behaviour: proxy is now always enabled
if(!isProxyEnabled()) {
setProxyEnabled(true);
Expand Down
Expand Up @@ -11,6 +11,8 @@

package org.jboss.tools.livereload.core.internal.util;

import java.nio.charset.Charset;
import java.nio.charset.UnsupportedCharsetException;
import java.util.StringTokenizer;

/**
Expand Down Expand Up @@ -45,7 +47,7 @@ public static boolean isHtmlContentType(final String acceptedContentTypes) {
if (acceptedContentTypes == null) {
return false;
}
// first, let's remove everything after the coma character
// first, let's remove everything behind the comma character
int location = acceptedContentTypes.indexOf(";");
final String contentTypes = (location != -1) ? acceptedContentTypes.substring(0, location):acceptedContentTypes;
// now, let's analyze each type
Expand All @@ -59,5 +61,37 @@ public static boolean isHtmlContentType(final String acceptedContentTypes) {
}
return false;
}

/**
* Returns the {@link Charset} from the given content-type.
*
* @param contentType
* the given content type that may contain some ";charset=...".
* @param defaultCharsetName
* the name of the default charset to return in case when the given
* contentType would be null or would not contain any specific
* charset. If this name cannot be resolved into a valid charset, then "UTF-8" is used.
* @return the value of the "charset" token in the given contentType, or
* the given defaultCharsetName, or "UTF-8" as a last resort.
*/
public static Charset getContentCharSet(final String contentType, final String defaultCharsetName) {
if(contentType != null) {
final StringTokenizer stk = new StringTokenizer(contentType, ";");
while(stk.hasMoreTokens()) {
final String token = stk.nextToken().toLowerCase().replace(" ", "");
if(token.startsWith("charset=")) {
final StringTokenizer tokenSplitter = new StringTokenizer(token, "=");
tokenSplitter.nextToken(); // skip the 'charset' part as we already know it
final String value = tokenSplitter.hasMoreTokens()? tokenSplitter.nextToken() : null;
return Charset.forName(value.toUpperCase());
}
}
}
try {
return Charset.forName(defaultCharsetName);
} catch(UnsupportedCharsetException e) {
return Charset.forName("UTF-8");
}
}

}
Expand Up @@ -19,7 +19,7 @@ private LiveReloadServerConfigurationMessages() {

public static String WEBSOCKET_SERVER_PORT_LABEL;
public static String WEBSOCKET_SERVER_PORT_COMMAND;

public static String PROXY_SERVER_CONFIGURATION_TITLE;
public static String PROXY_CONFIGURATION_DESCRIPTION;

Expand Down
@@ -0,0 +1,3 @@
eclipse.preferences.version=1
encoding//projects/sample-static-site/WebContent/chinese.html=UTF-8
encoding//src/org/jboss/tools/livereload/internal/server/jetty/LiveReloadServerTestCase.java=UTF-8
@@ -0,0 +1,9 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
</head>
<body>
中文
</body>
</html>
9 changes: 9 additions & 0 deletions tests/org.jboss.tools.livereload.test/src/chinese.html
@@ -0,0 +1,9 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="UTF-8">
</head>
<body>
中文
</body>
</html>

0 comments on commit 57b8445

Please sign in to comment.