Author: johnh
Date: Mon Aug 2 23:12:41 2010
New Revision: 981703
URL: http://svn.apache.org/viewvc?rev=981703&view=rev
Log:
This patch involves the following changes:
- ProxyHandler and AccelHandler use similar code for handling the headers,
status codes and rewrite-mime-types. This patch refactors ProxyHandler to use
the common code placed in UriUtils for all of the above. The
disallowed-headers-list has also been consolidated into UriUtils.
- As a side effect of the refactoring, the header names and values will be
validated (via the UriUtils method) for the ProxyHandler flow as well.
- A bug in the handling of the "rewrite-mime-type" logic has also been fixed
here. Earlier the response mime type was only checked for its prefix match with
the rewrite mime type upto the "/" marker and not including the "/" marker -
this has been fixed now.
- Tests have been added for the "rewrite-mime-type" handling.
Patch provided by Anupama Dutta.
Modified:
shindig/trunk/java/common/conf/shindig.properties
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriUtilsTest.java
Modified: shindig/trunk/java/common/conf/shindig.properties
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/common/conf/shindig.properties?rev=981703&r1=981702&r2=981703&view=diff
==============================================================================
--- shindig/trunk/java/common/conf/shindig.properties (original)
+++ shindig/trunk/java/common/conf/shindig.properties Mon Aug 2 23:12:41 2010
@@ -146,3 +146,4 @@ shindig.json-rpc.result-field=result
# "Bad Gateway error"s, so that it is clear to the user that the proxy server
is
# the one that threw the exception.
shindig.accelerate.remapInternalServerError=true
+shindig.proxy.remapInternalServerError=true
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java?rev=981703&r1=981702&r2=981703&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/AccelHandler.java
Mon Aug 2 23:12:41 2010
@@ -109,7 +109,7 @@ public class AccelHandler extends ProxyB
// Override the content type of the final http response if the input
request
// had the rewrite mime type header.
- rewriteContentType(req, response);
+ UriUtils.maybeRewriteContentType(req, response);
// Copy the content.
IOUtils.copy(results.getResponse(), response.getOutputStream());
@@ -195,23 +195,6 @@ public class AccelHandler extends ProxyB
}
/**
- * Rewrite the content type of the final http response if the request has the
- * rewrite-mime-type param.
- * @param req The http request.
- * @param response The final http response to be returned to user.
- */
- protected void rewriteContentType(HttpRequest req, HttpServletResponse
response) {
- String contentType = response.getContentType();
- String requiredType = req.getRewriteMimeType();
- if (!StringUtils.isEmpty(requiredType)) {
- if (requiredType.endsWith("/*") && !StringUtils.isEmpty(contentType)) {
- requiredType = requiredType.substring(0, requiredType.length() - 2);
- }
- response.setContentType(requiredType);
- }
- }
-
- /**
* Process errors when fetching uri using request pipeline and return the
* error response to be returned to the user if any.
* @param results The http response returned by request pipeline.
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java?rev=981703&r1=981702&r2=981703&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyBase.java
Mon Aug 2 23:12:41 2010
@@ -55,11 +55,6 @@ public abstract class ProxyBase {
public static final String REWRITE_MIME_TYPE_PARAM =
Param.REWRITE_MIME_TYPE.getKey();
public static final String SANITIZE_CONTENT_PARAM = Param.SANITIZE.getKey();
- protected static final Set<String> DISALLOWED_RESPONSE_HEADERS =
ImmutableSet.of(
- "set-cookie", "content-length", "content-encoding", "etag",
"last-modified" ,"accept-ranges",
- "vary", "expires", "date", "pragma", "cache-control",
"transfer-encoding", "www-authenticate"
- );
-
private static final Logger LOG =
Logger.getLogger(ProxyBase.class.getName());
/**
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java?rev=981703&r1=981702&r2=981703&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/ProxyHandler.java
Mon Aug 2 23:12:41 2010
@@ -20,6 +20,7 @@ package org.apache.shindig.gadgets.servl
import com.google.inject.Inject;
import com.google.inject.Singleton;
+import com.google.inject.name.Named;
import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.StringUtils;
@@ -34,6 +35,7 @@ import org.apache.shindig.gadgets.http.R
import org.apache.shindig.gadgets.rewrite.ResponseRewriterRegistry;
import org.apache.shindig.gadgets.rewrite.RewritingException;
import org.apache.shindig.gadgets.uri.ProxyUriManager;
+import org.apache.shindig.gadgets.uri.UriUtils;
import java.io.IOException;
import java.util.Map;
@@ -57,16 +59,20 @@ public class ProxyHandler extends ProxyB
private final LockedDomainService lockedDomainService;
private final ResponseRewriterRegistry contentRewriterRegistry;
private final ProxyUriManager proxyUriManager;
+ protected final boolean remapInternalServerError;
@Inject
public ProxyHandler(RequestPipeline requestPipeline,
LockedDomainService lockedDomainService,
ResponseRewriterRegistry contentRewriterRegistry,
- ProxyUriManager proxyUriManager) {
+ ProxyUriManager proxyUriManager,
+ @Named("shindig.proxy.remapInternalServerError")
+ Boolean remapInternalServerError) {
this.requestPipeline = requestPipeline;
this.lockedDomainService = lockedDomainService;
this.contentRewriterRegistry = contentRewriterRegistry;
this.proxyUriManager = proxyUriManager;
+ this.remapInternalServerError = remapInternalServerError;
}
/**
@@ -137,46 +143,20 @@ public class ProxyHandler extends ProxyB
}
}
- for (Map.Entry<String, String> entry : results.getHeaders().entries()) {
- String name = entry.getKey();
- if (!DISALLOWED_RESPONSE_HEADERS.contains(name.toLowerCase())) {
- try {
- response.addHeader(name, entry.getValue());
- } catch (IllegalArgumentException e) {
- // Skip illegal header
- LOG.info("Skipping illegal header: " + name + ":" +
entry.getValue());
- }
- }
- }
-
- String responseType = results.getHeader("Content-Type");
- if (!StringUtils.isEmpty(rcr.getRewriteMimeType())) {
- String requiredType = rcr.getRewriteMimeType();
- // Use a 'Vary' style check on the response
- if (requiredType.endsWith("/*") &&
- !StringUtils.isEmpty(responseType)) {
- requiredType = requiredType.substring(0, requiredType.length() - 2);
- if
(!responseType.toLowerCase().startsWith(requiredType.toLowerCase())) {
- response.setContentType(requiredType);
- responseType = requiredType;
- }
- } else {
- response.setContentType(requiredType);
- responseType = requiredType;
- }
- }
+ // Copy the response headers and status code to the final http servlet
+ // response.
+ UriUtils.copyResponseHeadersAndStatusCode(
+ results, response, remapInternalServerError, false,
+ UriUtils.DisallowedHeaders.OUTPUT_TRANSFER_DIRECTIVES,
+ UriUtils.DisallowedHeaders.CACHING_DIRECTIVES,
+ UriUtils.DisallowedHeaders.CLIENT_STATE_DIRECTIVES);
+
+ // Override the content type of the final http response if the input
request
+ // had the rewrite mime type header.
+ UriUtils.maybeRewriteContentType(rcr, response);
setResponseContentHeaders(response, results);
- if (results.isError()) {
- if (results.getHttpStatusCode() ==
HttpResponse.SC_INTERNAL_SERVER_ERROR) {
- // External "internal error" should be mapped to gateway error
- response.sendError(HttpResponse.SC_BAD_GATEWAY);
- } else {
- response.sendError(results.getHttpStatusCode());
- }
- } else {
- IOUtils.copy(results.getResponse(), response.getOutputStream());
- }
+ IOUtils.copy(results.getResponse(), response.getOutputStream());
}
}
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java?rev=981703&r1=981702&r2=981703&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/uri/UriUtils.java
Mon Aug 2 23:12:41 2010
@@ -22,10 +22,12 @@ import com.google.common.collect.Immutab
import org.apache.shindig.gadgets.GadgetException;
import org.apache.shindig.gadgets.http.HttpRequest;
import org.apache.shindig.gadgets.http.HttpResponse;
+import org.apache.commons.lang.StringUtils;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
+import java.util.logging.Logger;
import java.util.Enumeration;
import java.util.HashSet;
import java.util.Map;
@@ -35,6 +37,7 @@ import java.util.Set;
* Utility functions related to URI and Http servlet response management.
*/
public class UriUtils {
+ private static final Logger LOG = Logger.getLogger(UriUtils.class.getName());
/**
* Enum of disallowed response headers that should not be passed on as is to
* the user. The webserver serving out the response should be responsible
@@ -47,7 +50,7 @@ public class UriUtils {
"accept-ranges")),
CACHING_DIRECTIVES(ImmutableSet.of("vary", "expires", "date", "pragma",
- "cache-control")),
+ "cache-control", "etag",
"last-modified")),
CLIENT_STATE_DIRECTIVES(ImmutableSet.of("set-cookie", "www-authenticate")),
@@ -73,7 +76,9 @@ public class UriUtils {
/**
* Returns true if the header name is valid.
- * NOTE: RFC 822 section 3.1.2 describes the structure of header fields.
+ * NOTE: RFC 822 section 3.1.2 describes the structure of header fields.
+ * According to the RFC, a header name (or field-name) must be composed of
printable ASCII
+ * characters (i.e., characters that have values between 33. and 126.
decimal, except colon).
* @param name The header name.
* @return True if the header name is valid, false otherwise.
*/
@@ -95,11 +100,24 @@ public class UriUtils {
/**
* Returns true if the header value is valid.
* NOTE: RFC 822 section 3.1.2 describes the structure of header fields.
+ * According to the RFC, a header value (or field-body) may be composed of
any ASCII characters,
+ * except CR or LF.
* @param val The header value.
* @return True if the header value is valid, false otherwise.
*/
public static boolean isValidHeaderValue(String val) {
- // TODO: complete this.
+ char[] dst = new char[val.length()];
+ val.getChars(0, val.length(), dst, 0);
+
+ for (char c : dst) {
+ if (c == 13 || c == 10) {
+ // CR and LF.
+ return false;
+ }
+ if (c > 127) {
+ return false;
+ }
+ }
return true;
}
@@ -133,10 +151,15 @@ public class UriUtils {
for (Map.Entry<String, String> entry : data.getHeaders().entries()) {
if (isValidHeaderName(entry.getKey()) &&
isValidHeaderValue(entry.getValue()) &&
!allDisallowedHeaders.contains(entry.getKey().toLowerCase())) {
- if (setHeaders) {
- resp.setHeader(entry.getKey(), entry.getValue());
- } else {
- resp.addHeader(entry.getKey(), entry.getValue());
+ try {
+ if (setHeaders) {
+ resp.setHeader(entry.getKey(), entry.getValue());
+ } else {
+ resp.addHeader(entry.getKey(), entry.getValue());
+ }
+ } catch (IllegalArgumentException e) {
+ // Skip illegal header
+ LOG.warning("Skipping illegal header: " + entry.getKey() + ":" +
entry.getValue());
}
}
}
@@ -210,4 +233,28 @@ public class UriUtils {
throw new GadgetException(GadgetException.Code.INTERNAL_SERVER_ERROR, e);
}
}
+
+ /**
+ * Rewrite the content type of the final http response if the request has the
+ * rewrite-mime-type param.
+ * @param req The http request.
+ * @param response The final http response to be returned to user.
+ */
+ public static void maybeRewriteContentType(HttpRequest req,
HttpServletResponse response) {
+ String responseType = response.getContentType();
+ String requiredType = req.getRewriteMimeType();
+ if (!StringUtils.isEmpty(requiredType)) {
+ // Use a 'Vary' style check on the response
+ if (requiredType.endsWith("/*") && !StringUtils.isEmpty(responseType)) {
+ String requiredTypePrefix = requiredType.substring(0,
requiredType.length() - 1);
+ if
(!responseType.toLowerCase().startsWith(requiredTypePrefix.toLowerCase())) {
+ // TODO: We are currently setting the content type to something like
x/* (e.g. text/*)
+ // which is not a valid content type. Need to fix this.
+ response.setContentType(requiredType);
+ }
+ } else {
+ response.setContentType(requiredType);
+ }
+ }
+ }
}
Modified:
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java?rev=981703&r1=981702&r2=981703&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
(original)
+++
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
Mon Aug 2 23:12:41 2010
@@ -50,7 +50,7 @@ public class ProxyHandlerTest extends Se
private final ProxyUriManager passthruManager = new PassthruManager();
private final ProxyHandler proxyHandler
- = new ProxyHandler(pipeline, lockedDomainService, rewriterRegistry,
passthruManager);
+ = new ProxyHandler(pipeline, lockedDomainService, rewriterRegistry,
passthruManager, true);
private void expectGetAndReturnData(String url, byte[] data) throws
Exception {
HttpRequest req = new HttpRequest(Uri.parse(url));
@@ -243,6 +243,35 @@ public class ProxyHandlerTest extends Se
verify();
}
+ @Test
+ public void testInvalidHeaderDropped() throws Exception {
+ String url = "http://example.org/mypage.html";;
+ String domain = "example.org";
+
+
expect(lockedDomainService.isSafeForOpenProxy(domain)).andReturn(true).atLeastOnce();
+ setupProxyRequestMock(domain, url);
+
+ HttpRequest req = new HttpRequest(Uri.parse(url));
+ String contentType = "text/html; charset=UTF-8";
+ HttpResponse resp = new HttpResponseBuilder()
+ .setResponseString("Hello")
+ .addHeader("Content-Type", contentType)
+ .addHeader("Content-Length", "200") // Disallowed header.
+ .addHeader(":", "someDummyValue") // Invalid header name.
+ .create();
+
+ expect(pipeline.execute(req)).andReturn(resp);
+
+ replay();
+
+ proxyHandler.fetch(request, recorder);
+
+ verify();
+ assertNull(recorder.getHeader(":"));
+ assertNull(recorder.getHeader("Content-Length"));
+ assertEquals(recorder.getHeader("Content-Type"), contentType);
+ }
+
/**
* Override HttpRequest equals to check for cache control fields
*/
Modified:
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java?rev=981703&r1=981702&r2=981703&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java
(original)
+++
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyServletTest.java
Mon Aug 2 23:12:41 2010
@@ -47,7 +47,7 @@ public class ProxyServletTest extends Se
private final ProxyUriManager passthruManager = new PassthruManager();
private final ProxyHandler proxyHandler
- = new ProxyHandler(pipeline, lockedDomainService, null, passthruManager);
+ = new ProxyHandler(pipeline, lockedDomainService, null, passthruManager,
true);
private final ProxyServlet servlet = new ProxyServlet();
private final HttpRequest internalRequest = new HttpRequest(REQUEST_URL);
private final HttpResponse internalResponse = new
HttpResponse(RESPONSE_BODY);
Modified:
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriUtilsTest.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriUtilsTest.java?rev=981703&r1=981702&r2=981703&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriUtilsTest.java
(original)
+++
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/uri/UriUtilsTest.java
Mon Aug 2 23:12:41 2010
@@ -25,6 +25,7 @@ import org.apache.shindig.common.uri.Uri
import org.apache.shindig.gadgets.http.HttpRequest;
import org.apache.shindig.gadgets.http.HttpResponse;
import org.apache.shindig.gadgets.http.HttpResponseBuilder;
+import org.apache.shindig.gadgets.servlet.ServletTestFixture;
import org.easymock.EasyMock;
import org.easymock.IAnswer;
import org.junit.Before;
@@ -40,7 +41,7 @@ import static org.easymock.EasyMock.expe
/**
* Tests for UriUtils.
*/
-public class UriUtilsTest extends EasyMockTestCase {
+public class UriUtilsTest extends ServletTestFixture {
@Before
public void setUp() throws Exception {
}
@@ -53,6 +54,67 @@ public class UriUtilsTest extends EasyMo
return vector.elements();
}
+ private void verifyMime(String requestMime, String responseMime, String
expectedMime)
+ throws Exception {
+ String url = "http://example.org/foo";;
+ HttpRequest req = new HttpRequest(Uri.parse(url))
+ .setRewriteMimeType(requestMime);
+ recorder.setContentType(responseMime);
+
+ UriUtils.maybeRewriteContentType(req, recorder);
+ assertEquals(expectedMime, recorder.getContentType());
+ }
+
+ @Test
+ public void testMimeMatchPass() throws Exception {
+ verifyMime("text/css", "text/css", "text/css");
+ }
+
+ @Test
+ public void testMimeMatchPassWithAdditionalAttributes() throws Exception {
+ verifyMime("text/css", "text/css; charset=UTF-8", "text/css");
+ }
+
+ @Test
+ public void testNonMatchingMime() throws Exception {
+ verifyMime("text/css", "image/png; charset=UTF-8", "text/css");
+ }
+
+ @Test
+ public void testNonMatchingMimeWithSamePrefix() throws Exception {
+ verifyMime("text/html", "text/plain", "text/html");
+ }
+
+ @Test
+ public void testNonMatchingMimeWithWildCard() throws Exception {
+ verifyMime("text/*", "image/png", "text/*");
+ }
+
+ @Test
+ public void testNonMatchingMimeWithDifferentPrefix() throws Exception {
+ verifyMime("text/*", "text123/html", "text/*");
+ }
+
+ @Test
+ public void testMimeMatchVarySupport() throws Exception {
+ verifyMime("image/*", "image/gif", "image/gif");
+ }
+
+ @Test
+ public void testNullRequestMime() throws Exception {
+ verifyMime(null, "image/png; charset=UTF-8", "image/png; charset=UTF-8");
+ }
+
+ @Test
+ public void testEmptyRequestMime() throws Exception {
+ verifyMime("", "image/png; charset=UTF-8", "image/png; charset=UTF-8");
+ }
+
+ @Test
+ public void testNullResponseMime() throws Exception {
+ verifyMime("text/*", null, "text/*");
+ }
+
@Test
public void testCopyResponseHeadersAndStatusCode_AddHeader() throws
Exception {
HttpResponse resp = new HttpResponseBuilder()
