Author: markt Date: Wed Jun 24 13:26:06 2015 New Revision: 1687261 URL: http://svn.apache.org/r1687261 Log: Make the (first) reason parameter parsing failed available as a request attribute and then use it to provide a better status code via the FailedRequstFilter (if configured).
Modified: tomcat/trunk/java/org/apache/catalina/Globals.java tomcat/trunk/java/org/apache/catalina/connector/Request.java tomcat/trunk/java/org/apache/catalina/filters/FailedRequestFilter.java tomcat/trunk/java/org/apache/tomcat/util/http/Parameters.java tomcat/trunk/test/org/apache/catalina/connector/TestRequest.java Modified: tomcat/trunk/java/org/apache/catalina/Globals.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/Globals.java?rev=1687261&r1=1687260&r2=1687261&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/Globals.java (original) +++ tomcat/trunk/java/org/apache/catalina/Globals.java Wed Jun 24 13:26:06 2015 @@ -213,6 +213,13 @@ public final class Globals { /** + * The reason that the parameter parsing failed. + */ + public static final String PARAMETER_PARSE_FAILED_REASON_ATTR = + "org.apache.catalina.parameter_parse_failed_reason"; + + + /** * The master flag which controls strict servlet specification * compliance. */ Modified: tomcat/trunk/java/org/apache/catalina/connector/Request.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/connector/Request.java?rev=1687261&r1=1687260&r2=1687261&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/connector/Request.java (original) +++ tomcat/trunk/java/org/apache/catalina/connector/Request.java Wed Jun 24 13:26:06 2015 @@ -88,6 +88,7 @@ import org.apache.tomcat.util.buf.UDecod import org.apache.tomcat.util.http.CookieProcessor; import org.apache.tomcat.util.http.FastHttpDateFormat; import org.apache.tomcat.util.http.Parameters; +import org.apache.tomcat.util.http.Parameters.FailReason; import org.apache.tomcat.util.http.ServerCookie; import org.apache.tomcat.util.http.ServerCookies; import org.apache.tomcat.util.http.fileupload.FileItem; @@ -2636,6 +2637,7 @@ public class Request } if (!location.isDirectory()) { + parameters.setParseFailedReason(FailReason.MULTIPART_CONFIG_INVALID); partsParseException = new IOException( sm.getString("coyoteRequest.uploadLocationInvalid", location)); @@ -2648,6 +2650,7 @@ public class Request try { factory.setRepository(location.getCanonicalFile()); } catch (IOException ioe) { + parameters.setParseFailedReason(FailReason.IO_ERROR); partsParseException = ioe; return; } @@ -2714,6 +2717,7 @@ public class Request // Value separator postSize++; if (postSize > maxPostSize) { + parameters.setParseFailedReason(FailReason.POST_TOO_LARGE); throw new IllegalStateException(sm.getString( "coyoteRequest.maxPostSizeExceeded")); } @@ -2724,19 +2728,23 @@ public class Request success = true; } catch (InvalidContentTypeException e) { + parameters.setParseFailedReason(FailReason.INVALID_CONTENT_TYPE); partsParseException = new ServletException(e); } catch (FileUploadBase.SizeException e) { + parameters.setParseFailedReason(FailReason.POST_TOO_LARGE); checkSwallowInput(); partsParseException = new IllegalStateException(e); } catch (FileUploadException e) { + parameters.setParseFailedReason(FailReason.IO_ERROR); partsParseException = new IOException(e); } catch (IllegalStateException e) { + // addParameters() will set parseFailedReason checkSwallowInput(); partsParseException = e; } } finally { if (partsParseException != null || !success) { - parameters.setParseFailed(true); + parameters.setParseFailedReason(FailReason.UNKNOWN); } } } @@ -3015,6 +3023,7 @@ public class Request sm.getString("coyoteRequest.postTooLarge")); } checkSwallowInput(); + parameters.setParseFailedReason(FailReason.POST_TOO_LARGE); return; } byte[] formData = null; @@ -3028,6 +3037,7 @@ public class Request } try { if (readPostBody(formData, len) != len) { + parameters.setParseFailedReason(FailReason.REQUEST_BODY_INCOMPLETE); return; } } catch (IOException e) { @@ -3038,6 +3048,7 @@ public class Request sm.getString("coyoteRequest.parseParameters"), e); } + parameters.setParseFailedReason(FailReason.CLIENT_DISCONNECT); return; } parameters.processParameters(formData, 0, len); @@ -3048,6 +3059,7 @@ public class Request formData = readChunkedPostBody(); } catch (IllegalStateException ise) { // chunkedPostTooLarge error + parameters.setParseFailedReason(FailReason.POST_TOO_LARGE); Context context = getContext(); if (context != null && context.getLogger().isDebugEnabled()) { context.getLogger().debug( @@ -3057,6 +3069,7 @@ public class Request return; } catch (IOException e) { // Client disconnect + parameters.setParseFailedReason(FailReason.CLIENT_DISCONNECT); Context context = getContext(); if (context != null && context.getLogger().isDebugEnabled()) { context.getLogger().debug( @@ -3072,7 +3085,7 @@ public class Request success = true; } finally { if (!success) { - parameters.setParseFailed(true); + parameters.setParseFailedReason(FailReason.UNKNOWN); } } @@ -3275,6 +3288,18 @@ public class Request } @Override + public void set(Request request, String name, Object value) { + // NO-OP + } + }); + specialAttributes.put(Globals.PARAMETER_PARSE_FAILED_REASON_ATTR, + new SpecialAttributeAdapter() { + @Override + public Object get(Request request, String name) { + return request.getCoyoteRequest().getParameters().getParseFailedReason(); + } + + @Override public void set(Request request, String name, Object value) { // NO-OP } Modified: tomcat/trunk/java/org/apache/catalina/filters/FailedRequestFilter.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/filters/FailedRequestFilter.java?rev=1687261&r1=1687260&r2=1687261&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/catalina/filters/FailedRequestFilter.java (original) +++ tomcat/trunk/java/org/apache/catalina/filters/FailedRequestFilter.java Wed Jun 24 13:26:06 2015 @@ -27,6 +27,7 @@ import javax.servlet.http.HttpServletRes import org.apache.catalina.Globals; import org.apache.juli.logging.Log; import org.apache.juli.logging.LogFactory; +import org.apache.tomcat.util.http.Parameters.FailReason; /** * Filter that will reject requests if there was a failure during parameter @@ -53,8 +54,41 @@ public class FailedRequestFilter extends public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { if (!isGoodRequest(request)) { - ((HttpServletResponse) response) - .sendError(HttpServletResponse.SC_BAD_REQUEST); + FailReason reason = (FailReason) request.getAttribute( + Globals.PARAMETER_PARSE_FAILED_REASON_ATTR); + + int status; + + switch (reason) { + case IO_ERROR: + // Not the client's fault + status = HttpServletResponse.SC_INTERNAL_SERVER_ERROR; + break; + case POST_TOO_LARGE: + status = HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE; + break; + case TOO_MANY_PARAMETERS: + // 413/414 aren't really correct here since the request body + // and/or URI could be well below any limits set. Use the + // default. + case UNKNOWN: // Assume the client is at fault + // Various things that the client can get wrong that don't have + // a specific status code so use the default. + case INVALID_CONTENT_TYPE: + case MULTIPART_CONFIG_INVALID: + case NO_NAME: + case REQUEST_BODY_INCOMPLETE: + case URL_DECODING: + case CLIENT_DISCONNECT: + // Client is never going to see this so this is really just + // for the access logs. The default is fine. + default: + // 400 + status = HttpServletResponse.SC_BAD_REQUEST; + break; + } + + ((HttpServletResponse) response).sendError(status); return; } chain.doFilter(request, response); Modified: tomcat/trunk/java/org/apache/tomcat/util/http/Parameters.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/http/Parameters.java?rev=1687261&r1=1687260&r2=1687261&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/tomcat/util/http/Parameters.java (original) +++ tomcat/trunk/java/org/apache/tomcat/util/http/Parameters.java Wed Jun 24 13:26:06 2015 @@ -66,10 +66,10 @@ public final class Parameters { private int parameterCount = 0; /** - * Is set to <code>true</code> if there were failures during parameter - * parsing. + * Set to the reason for the failure (the first failure if there is more + * than one) if there were failures during parameter parsing. */ - private boolean parseFailed = false; + private FailReason parseFailedReason = null; public Parameters() { // NO-OP @@ -101,23 +101,34 @@ public final class Parameters { } } + public boolean isParseFailed() { - return parseFailed; + return parseFailedReason != null; + } + + + public FailReason getParseFailedReason() { + return parseFailedReason; } - public void setParseFailed(boolean parseFailed) { - this.parseFailed = parseFailed; + + public void setParseFailedReason(FailReason failReason) { + if (this.parseFailedReason == null) { + this.parseFailedReason = failReason; + } } + public void recycle() { parameterCount = 0; paramHashValues.clear(); didQueryParameters=false; encoding=null; decodedQuery.recycle(); - parseFailed = false; + parseFailedReason = null; } + // -------------------- Data access -------------------- // Access to the current name/values, no side effect ( processing ). // You must explicitly call handleQueryParameters and the post methods. @@ -189,7 +200,7 @@ public final class Parameters { if (limit > -1 && parameterCount > limit) { // Processing this parameter will push us over the limit. ISE is // what Request.parseParts() uses for requests that are too big - parseFailed = true; + setParseFailedReason(FailReason.TOO_MANY_PARAMETERS); throw new IllegalStateException(sm.getString( "parameters.maxCountFail", Integer.valueOf(limit))); } @@ -334,7 +345,7 @@ public final class Parameters { log.debug(message); } } - parseFailed = true; + setParseFailedReason(FailReason.NO_NAME); continue; // invalid chunk - it's better to ignore } @@ -388,7 +399,6 @@ public final class Parameters { } catch (IllegalStateException ise) { // Hitting limit stops processing further params but does // not cause request to fail. - parseFailed = true; UserDataHelper.Mode logMode = maxParamCountLog.getNextMode(); if (logMode != null) { String message = ise.getMessage(); @@ -407,7 +417,7 @@ public final class Parameters { break; } } catch (IOException e) { - parseFailed = true; + setParseFailedReason(FailReason.URL_DECODING); decodeFailCount++; if (decodeFailCount == 1 || log.isDebugEnabled()) { if (log.isDebugEnabled()) { @@ -511,4 +521,18 @@ public final class Parameters { } return sb.toString(); } + + + public enum FailReason { + CLIENT_DISCONNECT, + MULTIPART_CONFIG_INVALID, + INVALID_CONTENT_TYPE, + IO_ERROR, + NO_NAME, + POST_TOO_LARGE, + REQUEST_BODY_INCOMPLETE, + TOO_MANY_PARAMETERS, + UNKNOWN, + URL_DECODING + } } Modified: tomcat/trunk/test/org/apache/catalina/connector/TestRequest.java URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/connector/TestRequest.java?rev=1687261&r1=1687260&r2=1687261&view=diff ============================================================================== --- tomcat/trunk/test/org/apache/catalina/connector/TestRequest.java (original) +++ tomcat/trunk/test/org/apache/catalina/connector/TestRequest.java Wed Jun 24 13:26:06 2015 @@ -84,17 +84,17 @@ public class TestRequest extends TomcatB assertTrue(client.isResponseBodyOK()); client.reset(); client.doRequest(0, false); // 0 bytes - too small should fail - assertTrue(client.isResponse400()); + assertTrue(client.isResponse413()); client.reset(); client.doRequest(1, false); // 1 byte - too small should fail - assertTrue(client.isResponse400()); + assertTrue(client.isResponse413()); client.reset(); // Edge cases around actual content length client.reset(); client.doRequest(6, false); // Too small should fail - assertTrue(client.isResponse400()); + assertTrue(client.isResponse413()); client.reset(); client.doRequest(7, false); // Just enough should pass assertTrue(client.isResponse200()); --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org