Author: markt Date: Thu Jul 21 15:23:11 2011 New Revision: 1149220 URL: http://svn.apache.org/viewvc?rev=1149220&view=rev Log: Fix path parameter handling. Prevent the following URL failing with a 404: http://localhost:8080/examples/jsp/snp;x=y/snoop.jsp Patch by kkolinko
Modified: tomcat/tc6.0.x/trunk/STATUS.txt tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/LocalStrings.properties tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/Request.java tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Modified: tomcat/tc6.0.x/trunk/STATUS.txt URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?rev=1149220&r1=1149219&r2=1149220&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/STATUS.txt (original) +++ tomcat/tc6.0.x/trunk/STATUS.txt Thu Jul 21 15:23:11 2011 @@ -58,19 +58,6 @@ PATCHES PROPOSED TO BACKPORT: +1: rjung, funkman, kkolinko -1: -* Fix path parameter handling. Currently the following URL fails with a 404: - http://localhost:8080/examples/jsp/snp;x=y/snoop.jsp - http://people.apache.org/~kkolinko/patches/2010-11-17_tc6_path-params.patch - +1: kkolinko, markt, schultz - -1: - - kkolinko: The old parseSessionId() method calls "clearRequestedSessionURL(request);" - when there is no sessionid in the path. The new code does not do that. Was that - call needed? It looks that it was not needed, but I might miss something. - - kkolinko: Discussed in Re:r1005192 thread on dev@ - - * Backport JSP unloading patch (BZ48358). The patch has substantially changed since the original version. Original revisions are: 937787, 1028377, 1028389, 1028396, 1028861, 1028862, 1028863, Modified: tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java?rev=1149220&r1=1149219&r2=1149220&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java (original) +++ tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/CoyoteAdapter.java Thu Jul 21 15:23:11 2011 @@ -94,13 +94,6 @@ public class CoyoteAdapter implements Ad /** - * The match string for identifying a session ID parameter. - */ - private static final String match = - ";" + Globals.SESSION_PARAMETER_NAME + "="; - - - /** * The string manager for this package. */ protected StringManager sm = @@ -423,61 +416,43 @@ public class CoyoteAdapter implements Ad req.serverName().setString(proxyName); } - // Parse session Id - parseSessionId(req, request); - - // URI decoding + // Copy the raw URI to the decoded URI MessageBytes decodedURI = req.decodedURI(); decodedURI.duplicate(req.requestURI()); - if (decodedURI.getType() == MessageBytes.T_BYTES) { - // Remove any path parameters - ByteChunk uriBB = decodedURI.getByteChunk(); - int semicolon = uriBB.indexOf(';', 0); - if (semicolon > 0) { - decodedURI.setBytes - (uriBB.getBuffer(), uriBB.getStart(), semicolon); - } - // %xx decoding of the URL - try { - req.getURLDecoder().convert(decodedURI, false); - } catch (IOException ioe) { - res.setStatus(400); - res.setMessage("Invalid URI: " + ioe.getMessage()); - connector.getService().getContainer().logAccess( - request, response, 0, true); - return false; - } - // Normalization - if (!normalize(req.decodedURI())) { - res.setStatus(400); - res.setMessage("Invalid URI"); - connector.getService().getContainer().logAccess( - request, response, 0, true); - return false; - } - // Character decoding - convertURI(decodedURI, request); - // Check that the URI is still normalized - if (!checkNormalize(req.decodedURI())) { - res.setStatus(400); - res.setMessage("Invalid URI character encoding"); - connector.getService().getContainer().logAccess( - request, response, 0, true); - return false; - } - } else { - // The URL is chars or String, and has been sent using an in-memory - // protocol handler, we have to assume the URL has been properly - // decoded already - decodedURI.toChars(); - // Remove any path parameters - CharChunk uriCC = decodedURI.getCharChunk(); - int semicolon = uriCC.indexOf(';'); - if (semicolon > 0) { - decodedURI.setChars - (uriCC.getBuffer(), uriCC.getStart(), semicolon); - } + // Parse the path parameters. This will: + // - strip out the path parameters + // - convert the decodedURI to bytes + parsePathParameters(req, request); + + // URI decoding + // %xx decoding of the URL + try { + req.getURLDecoder().convert(decodedURI, false); + } catch (IOException ioe) { + res.setStatus(400); + res.setMessage("Invalid URI: " + ioe.getMessage()); + connector.getService().getContainer().logAccess( + request, response, 0, true); + return false; + } + // Normalization + if (!normalize(req.decodedURI())) { + res.setStatus(400); + res.setMessage("Invalid URI"); + connector.getService().getContainer().logAccess( + request, response, 0, true); + return false; + } + // Character decoding + convertURI(decodedURI, request); + // Check that the URI is still normalized + if (!checkNormalize(req.decodedURI())) { + res.setStatus(400); + res.setMessage("Invalid URI character encoding"); + connector.getService().getContainer().logAccess( + request, response, 0, true); + return false; } // Set the remote principal @@ -573,6 +548,12 @@ public class CoyoteAdapter implements Ad } // Parse session Id + String sessionID = + request.getPathParameter(Globals.SESSION_PARAMETER_NAME); + if (sessionID != null) { + request.setRequestedSessionId(sessionID); + request.setRequestedSessionURL(true); + } parseSessionCookiesId(req, request); return true; @@ -586,53 +567,131 @@ public class CoyoteAdapter implements Ad return (false); } + /** - * Parse session id in URL. + * Extract the path parameters from the request. This assumes parameters are + * of the form /path;name=value;name2=value2/ etc. Currently only really + * interested in the session ID that will be in this form. Other parameters + * can safely be ignored. + * + * @param req + * @param request */ - protected void parseSessionId(org.apache.coyote.Request req, Request request) { + protected void parsePathParameters(org.apache.coyote.Request req, + Request request) { - ByteChunk uriBC = req.requestURI().getByteChunk(); - int semicolon = uriBC.indexOf(match, 0, match.length(), 0); + // Process in bytes (this is default format so this is normally a NO-OP + req.decodedURI().toBytes(); - if (semicolon > 0) { - // What encoding to use? Some platforms, eg z/os, use a default - // encoding that doesn't give the expected result so be explicit - String enc = connector.getURIEncoding(); - if (enc == null) { - enc = "ISO-8859-1"; - } + ByteChunk uriBC = req.decodedURI().getByteChunk(); + int semicolon = uriBC.indexOf(';', 0); - // Parse session ID, and extract it from the decoded request URI + // What encoding to use? Some platforms, eg z/os, use a default + // encoding that doesn't give the expected result so be explicit + String enc = connector.getURIEncoding(); + if (enc == null) { + enc = "ISO-8859-1"; + } + + if (log.isDebugEnabled()) { + log.debug(sm.getString("coyoteAdapter.debug", "uriBC", + uriBC.toString())); + log.debug(sm.getString("coyoteAdapter.debug", "semicolon", + String.valueOf(semicolon))); + log.debug(sm.getString("coyoteAdapter.debug", "enc", enc)); + } + + boolean warnedEncoding = false; + + while (semicolon > -1) { + // Parse path param, and extract it from the decoded request URI int start = uriBC.getStart(); int end = uriBC.getEnd(); - int sessionIdStart = semicolon + match.length(); - int semicolon2 = uriBC.indexOf(';', sessionIdStart); - try { - if (semicolon2 >= 0) { - request.setRequestedSessionId - (new String(uriBC.getBuffer(), start + sessionIdStart, - semicolon2 - sessionIdStart, enc)); - // Extract session ID from request URI - byte[] buf = uriBC.getBuffer(); - for (int i = 0; i < end - start - semicolon2; i++) { - buf[start + semicolon + i] - = buf[start + i + semicolon2]; + int pathParamStart = semicolon + 1; + int pathParamEnd = ByteChunk.findBytes(uriBC.getBuffer(), + start + pathParamStart, end, + new byte[] {';', '/'}); + + String pv = null; + + if (pathParamEnd >= 0) { + try { + pv = (new String(uriBC.getBuffer(), start + pathParamStart, + pathParamEnd - pathParamStart, enc)); + } catch (UnsupportedEncodingException e) { + if (!warnedEncoding) { + log.warn(sm.getString("coyoteAdapter.parsePathParam", + enc)); + warnedEncoding = true; } - uriBC.setBytes(buf, start, - end - start - semicolon2 + semicolon); - } else { - request.setRequestedSessionId - (new String(uriBC.getBuffer(), start + sessionIdStart, - (end - start) - sessionIdStart, enc)); - uriBC.setEnd(start + semicolon); - } - request.setRequestedSessionURL(true); - } catch (UnsupportedEncodingException uee) { - // Make sure no session ID is returned - clearRequestedSessionURL(request); - log.warn(sm.getString("coyoteAdapter.parseSession", enc), uee); + } + // Extract path param from decoded request URI + byte[] buf = uriBC.getBuffer(); + for (int i = 0; i < end - start - pathParamEnd; i++) { + buf[start + semicolon + i] + = buf[start + i + pathParamEnd]; + } + uriBC.setBytes(buf, start, + end - start - pathParamEnd + semicolon); + } else { + try { + pv = (new String(uriBC.getBuffer(), start + pathParamStart, + (end - start) - pathParamStart, enc)); + } catch (UnsupportedEncodingException e) { + if (!warnedEncoding) { + log.warn(sm.getString("coyoteAdapter.parsePathParam", + enc)); + warnedEncoding = true; + } + } + uriBC.setEnd(start + semicolon); + } + + if (log.isDebugEnabled()) { + log.debug(sm.getString("coyoteAdapter.debug", "pathParamStart", + String.valueOf(pathParamStart))); + log.debug(sm.getString("coyoteAdapter.debug", "pathParamEnd", + String.valueOf(pathParamEnd))); + log.debug(sm.getString("coyoteAdapter.debug", "pv", pv)); + } + + if (pv != null) { + int equals = pv.indexOf('='); + if (equals > -1) { + String name = pv.substring(0, equals); + String value = pv.substring(equals + 1); + request.addPathParameter(name, value); + if (log.isDebugEnabled()) { + log.debug(sm.getString("coyoteAdapter.debug", "equals", + String.valueOf(equals))); + log.debug(sm.getString("coyoteAdapter.debug", "name", + name)); + log.debug(sm.getString("coyoteAdapter.debug", "value", + value)); + } + } } + + semicolon = uriBC.indexOf(';', semicolon); + } + } + + + /** + * Parse session id in URL. + * @deprecated Not used since 6.0.30 + */ + @Deprecated + protected void parseSessionId(org.apache.coyote.Request req, Request request) { + + parsePathParameters(req, request); + + String sessionID = + request.getPathParameter(Globals.SESSION_PARAMETER_NAME); + if (sessionID != null) { + request.setRequestedSessionId(sessionID); + request.setRequestedSessionURL(true); } else { clearRequestedSessionURL(request); } Modified: tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/LocalStrings.properties URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/LocalStrings.properties?rev=1149220&r1=1149219&r2=1149220&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/LocalStrings.properties (original) +++ tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/LocalStrings.properties Thu Jul 21 15:23:11 2011 @@ -37,6 +37,8 @@ coyoteConnector.protocolUnregistrationFa coyoteAdapter.service=An exception or error occurred in the container during the request processing coyoteAdapter.read=The servlet did not read all available bytes during the processing of the read event coyoteAdapter.parseSession=Unable to parse the session ID using encoding [{0}]. The session ID in the URL will be ignored. +coyoteAdapter.parsePathParam=Unable to parse the path parameters using encoding [{0}]. The path parameters in the URL will be ignored. +coyoteAdapter.debug=The variable [{0}] has value [{1}] # # CoyoteResponse Modified: tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/Request.java URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/Request.java?rev=1149220&r1=1149219&r2=1149220&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/Request.java (original) +++ tomcat/tc6.0.x/trunk/java/org/apache/catalina/connector/Request.java Thu Jul 21 15:23:11 2011 @@ -388,9 +388,23 @@ public class Request protected String localName = null; + /** + * Path parameters + */ + protected Map<String,String> pathParameters = new HashMap<String, String>(); + + // --------------------------------------------------------- Public Methods + protected void addPathParameter(String name, String value) { + pathParameters.put(name, value); + } + + protected String getPathParameter(String name) { + return pathParameters.get(name); + } + /** * Release all object references, and initialize instance variables, in * preparation for reuse of this object. @@ -470,6 +484,7 @@ public class Request } } + pathParameters.clear(); } Modified: tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java?rev=1149220&r1=1149219&r2=1149220&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java (original) +++ tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java Thu Jul 21 15:23:11 2011 @@ -762,6 +762,30 @@ public final class ByteChunk implements return -1; } + /** + * Returns the first instance of any of the given bytes in the byte array + * between the specified start and end. + * + * @param bytes The byte array to search + * @param start The point to start searching from in the byte array + * @param end The point to stop searching in the byte array + * @param b The array of bytes to search for + * @return The position of the first instance of the byte or -1 if the + * byte is not found. + */ + public static int findBytes(byte bytes[], int start, int end, byte b[]) { + int blen = b.length; + int offset = start; + while (offset < end) { + for (int i = 0; i < blen; i++) + if (bytes[offset] == b[i]) { + return offset; + } + offset++; + } + return -1; + } + /** Find a character, no side effects. * @return index of char if found, -1 if not */ Modified: tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml?rev=1149220&r1=1149219&r2=1149220&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml (original) +++ tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Thu Jul 21 15:23:11 2011 @@ -163,6 +163,10 @@ Log a failure if access log file cannot be opened. Improve i18n of messages. (kkolinko) </fix> + <fix> + Improve handling of URLs with path parameters and prevent incorrect 404 + responses that could occur when path parameters were present. (kkolinko) + </fix> </changelog> </subsection> <subsection name="Coyote"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org