Author: remy.maucherat(a)jboss.com Date: 2007-10-20 19:36:09 -0400 (Sat, 20 Oct 2007) New Revision: 317 Modified: trunk/java/org/apache/tomcat/util/http/Cookies.java trunk/java/org/apache/tomcat/util/http/ServerCookie.java trunk/webapps/docs/changelog.xml Log: - Port cookie escaping improvements (just in case ...). Modified: trunk/java/org/apache/tomcat/util/http/Cookies.java =================================================================== --- trunk/java/org/apache/tomcat/util/http/Cookies.java 2007-10-20 00:50:54 UTC (rev 316) +++ trunk/java/org/apache/tomcat/util/http/Cookies.java 2007-10-20 23:36:09 UTC (rev 317) @@ -35,6 +35,9 @@ */ public final class Cookies { // extends MultiMap { + private static org.jboss.logging.Logger log= + org.jboss.logging.Logger.getLogger(Cookies.class ); + // expected average number of cookies per request public static final int INITIAL_SIZE=4; ServerCookie scookies[]=new ServerCookie[INITIAL_SIZE]; @@ -187,11 +190,13 @@ // Uncomment to test the new parsing code if( cookieValue.getType() == MessageBytes.T_BYTES ) { + if( dbg>0 ) log( "Parsing b[]: " + cookieValue.toString()); ByteChunk bc=cookieValue.getByteChunk(); processCookieHeader( bc.getBytes(), bc.getOffset(), bc.getLength()); } else { + if( dbg>0 ) log( "Parsing S: " + cookieValue.toString()); processCookieHeader( cookieValue.toString() ); } pos++;// search from the next position @@ -219,7 +224,7 @@ private void processCookieHeader( String cookieString ) { - + if( dbg>0 ) log( "Parsing cookie header " + cookieString ); // normal cookie, with a string value. // This is the original code, un-optimized - it shouldn't // happen in normal case @@ -243,7 +248,7 @@ cookie.getName().setString(name); cookie.getValue().setString(value); - + if( dbg > 0 ) log( "Add cookie " + name + "=" + value); } else { // we have a bad cookie.... just let it go } @@ -274,6 +279,14 @@ } + // log + static final int dbg=0; + public void log(String s ) { + if (log.isDebugEnabled()) + log.debug("Cookies: " + s); + } + + /** * Returns true if the byte is a separator character as * defined in RFC2619. Since this is called often, this @@ -332,9 +345,11 @@ int version = 0; ServerCookie sc=null; boolean isSpecial; + boolean isQuoted; while (pos < end) { isSpecial = false; + isQuoted = false; // Skip whitespace and non-token characters (separators) while (pos < end && @@ -376,6 +391,7 @@ // token, name-only with an '=', or other (bad) switch (bytes[pos]) { case '"':; // Quoted Value + isQuoted = true; valueStart=pos + 1; // strip " // getQuotedValue returns the position before // at the last qoute. This must be dealt with @@ -410,6 +426,7 @@ // INVALID COOKIE, advance to next delimiter // The starting character of the cookie value was // not valid. + log("Invalid cookie. Value not a token or quoted value"); while (pos < end && bytes[pos] != ';' && bytes[pos] != ',') {pos++; }; @@ -507,7 +524,8 @@ continue; } - // Unknown cookie + // Unknown cookie, complain + log("Unknown Special Cookie"); } else { // Normal Cookie sc = addCookie(); @@ -517,7 +535,12 @@ if (valueStart != -1) { // Normal AVPair sc.getValue().setBytes( bytes, valueStart, - valueEnd-valueStart); + valueEnd-valueStart); + if (isQuoted) { + // We know this is a byte value so this is safe + ServerCookie.unescapeDoubleQuotes( + sc.getValue().getByteChunk()); + } } else { // Name Only sc.getValue().setString(""); Modified: trunk/java/org/apache/tomcat/util/http/ServerCookie.java =================================================================== --- trunk/java/org/apache/tomcat/util/http/ServerCookie.java 2007-10-20 00:50:54 UTC (rev 316) +++ trunk/java/org/apache/tomcat/util/http/ServerCookie.java 2007-10-20 23:36:09 UTC (rev 317) @@ -21,13 +21,14 @@ import java.text.FieldPosition; import java.util.Date; +import org.apache.tomcat.util.buf.ByteChunk; import org.apache.tomcat.util.buf.DateTool; import org.apache.tomcat.util.buf.MessageBytes; /** * Server-side cookie representation. - * Allows recycling and uses MessageBytes as low-level + * Allows recycling and uses MessageBytes as low-level * representation ( and thus the byte-> char conversion can be delayed * until we know the charset ). * @@ -39,253 +40,265 @@ private static org.jboss.logging.Logger log= org.jboss.logging.Logger.getLogger(ServerCookie.class ); - + + // Version 0 (Netscape) attributes private MessageBytes name=MessageBytes.newInstance(); private MessageBytes value=MessageBytes.newInstance(); + // Expires - Not stored explicitly. Generated from Max-Age (see V1) + private MessageBytes path=MessageBytes.newInstance(); + private MessageBytes domain=MessageBytes.newInstance(); + private boolean secure; + + // Version 1 (RFC2109) attributes + private MessageBytes comment=MessageBytes.newInstance(); + private int maxAge = -1; + private int version = 0; - private MessageBytes comment=MessageBytes.newInstance(); // ;Comment=VALUE - private MessageBytes domain=MessageBytes.newInstance(); // ;Domain=VALUE ... + // Note: Servlet Spec =< 2.5 only refers to Netscape and RFC2109, + // not RFC2965 - private int maxAge = -1; // ;Max-Age=VALUE - // ;Discard ... implied by maxAge < 0 - // RFC2109: maxAge=0 will end a session - private MessageBytes path=MessageBytes.newInstance(); // ;Path=VALUE . - private boolean secure; // ;Secure - private int version = 0; // ;Version=1 + // Version 1 (RFC2965) attributes + // TODO Add support for CommentURL + // Discard - implied by maxAge <0 + // TODO Add support for Port - //XXX CommentURL, Port -> use notes ? - public ServerCookie() { - } public void recycle() { path.recycle(); - name.recycle(); - value.recycle(); - comment.recycle(); - maxAge=-1; - path.recycle(); + name.recycle(); + value.recycle(); + comment.recycle(); + maxAge=-1; + path.recycle(); domain.recycle(); - version=0; - secure=false; + version=0; + secure=false; } public MessageBytes getComment() { - return comment; + return comment; } public MessageBytes getDomain() { - return domain; + return domain; } public void setMaxAge(int expiry) { - maxAge = expiry; + maxAge = expiry; } public int getMaxAge() { - return maxAge; + return maxAge; } - public MessageBytes getPath() { - return path; + return path; } public void setSecure(boolean flag) { - secure = flag; + secure = flag; } public boolean getSecure() { - return secure; + return secure; } public MessageBytes getName() { - return name; + return name; } public MessageBytes getValue() { - return value; + return value; } public int getVersion() { - return version; + return version; } - public void setVersion(int v) { - version = v; + version = v; } // -------------------- utils -------------------- + public static void log(String s ) { + if (log.isDebugEnabled()) + log.debug("ServerCookie: " + s); + } + public String toString() { - return "Cookie " + getName() + "=" + getValue() + " ; " - + getVersion() + " " + getPath() + " " + getDomain(); + return "Cookie " + getName() + "=" + getValue() + " ; " + + getVersion() + " " + getPath() + " " + getDomain(); } - // Note -- disabled for now to allow full Netscape compatibility - // from RFC 2068, token special case characters - // - // private static final String tspecials = "()<>@,;:\\\"/[]?={} \t"; private static final String tspecials = ",; "; - private static final String tspecials2 = ",; \""; + private static final String tspecials2 = "()<>@,;:\\\"/[]?={} \t"; /* * Tests a string and returns true if the string counts as a * reserved token in the Java language. * - * @param value the <code>String</code> to be tested + * @param value the <code>String</code> to be tested * - * @return <code>true</code> if the <code>String</code> is - * a reserved token; <code>false</code> - * if it is not + * @return <code>true</code> if the <code>String</code> is a reserved + * token; <code>false</code> if it is not */ public static boolean isToken(String value) { - if( value==null) return true; - int len = value.length(); + if( value==null) return true; + int len = value.length(); - for (int i = 0; i < len; i++) { - char c = value.charAt(i); + for (int i = 0; i < len; i++) { + char c = value.charAt(i); - if (c < 0x20 || c >= 0x7f || tspecials.indexOf(c) != -1) - return false; - } - return true; + if (c < 0x20 || c >= 0x7f || tspecials.indexOf(c) != -1) + return false; + } + return true; } public static boolean isToken2(String value) { - if( value==null) return true; - int len = value.length(); + if( value==null) return true; + int len = value.length(); - for (int i = 0; i < len; i++) { - char c = value.charAt(i); + for (int i = 0; i < len; i++) { + char c = value.charAt(i); - if (c < 0x20 || c >= 0x7f || tspecials2.indexOf(c) != -1) - return false; - } - return true; + if (c < 0x20 || c >= 0x7f || tspecials2.indexOf(c) != -1) + return false; + } + return true; } + /** + * @deprecated - Not used + */ public static boolean checkName( String name ) { - if (!isToken(name) - || name.equalsIgnoreCase("Comment") // rfc2019 - || name.equalsIgnoreCase("Discard") // 2019++ - || name.equalsIgnoreCase("Domain") - || name.equalsIgnoreCase("Expires") // (old cookies) - || name.equalsIgnoreCase("Max-Age") // rfc2019 - || name.equalsIgnoreCase("Path") - || name.equalsIgnoreCase("Secure") - || name.equalsIgnoreCase("Version") - ) { - return false; - } - return true; + if (!isToken(name) + || name.equalsIgnoreCase("Comment") // rfc2019 + || name.equalsIgnoreCase("Discard") // rfc2965 + || name.equalsIgnoreCase("Domain") // rfc2019 + || name.equalsIgnoreCase("Expires") // Netscape + || name.equalsIgnoreCase("Max-Age") // rfc2019 + || name.equalsIgnoreCase("Path") // rfc2019 + || name.equalsIgnoreCase("Secure") // rfc2019 + || name.equalsIgnoreCase("Version") // rfc2019 + // TODO remaining RFC2965 attributes + ) { + return false; + } + return true; } // -------------------- Cookie parsing tools - /** Return the header name to set the cookie, based on cookie - * version + /** + * Return the header name to set the cookie, based on cookie version. */ public String getCookieHeaderName() { - return getCookieHeaderName(version); + return getCookieHeaderName(version); } - /** Return the header name to set the cookie, based on cookie - * version + /** + * Return the header name to set the cookie, based on cookie version. */ public static String getCookieHeaderName(int version) { - if( dbg>0 ) log( (version==1) ? "Set-Cookie2" : "Set-Cookie"); + // TODO Re-enable logging when RFC2965 is implemented + // log( (version==1) ? "Set-Cookie2" : "Set-Cookie"); if (version == 1) { - // RFC2109 - return "Set-Cookie"; - // XXX RFC2965 is not standard yet, and Set-Cookie2 - // is not supported by Netscape 4, 6, IE 3, 5 . - // It is supported by Lynx, and there is hope - // return "Set-Cookie2"; + // XXX RFC2965 not referenced in Servlet Spec + // Set-Cookie2 is not supported by Netscape 4, 6, IE 3, 5 + // Set-Cookie2 is supported by Lynx and Opera + // Need to check on later IE and FF releases but for now... + // RFC2109 + return "Set-Cookie"; + // return "Set-Cookie2"; } else { - // Old Netscape - return "Set-Cookie"; + // Old Netscape + return "Set-Cookie"; } } - private static final String ancientDate=DateTool.formatOldCookie(new Date(10000)); + private static final String ancientDate = + DateTool.formatOldCookie(new Date(10000)); + // TODO RFC2965 fields also need to be passed public static void appendCookieValue( StringBuffer buf, - int version, - String name, - String value, - String path, - String domain, - String comment, - int maxAge, - boolean isSecure ) + int version, + String name, + String value, + String path, + String domain, + String comment, + int maxAge, + boolean isSecure ) { - // this part is the same for all cookies - buf.append( name ); + // Servlet implementation checks name + buf.append( name ); buf.append("="); + // Servlet implementation does not check anything else + maybeQuote2(version, buf, value); - // XXX Netscape cookie: "; " - // add version 1 specific information - if (version == 1) { - // Version=1 ... required - buf.append ("; Version=1"); + // Add version 1 specific information + if (version == 1) { + // Version=1 ... required + buf.append ("; Version=1"); - // Comment=comment - if ( comment!=null ) { - buf.append ("; Comment="); - maybeQuote (version, buf, comment); - } - } - - // add domain information, if present + // Comment=comment + if ( comment!=null ) { + buf.append ("; Comment="); + maybeQuote2(version, buf, comment); + } + } + + // Add domain information, if present + if (domain!=null) { + buf.append("; Domain="); + maybeQuote2(version, buf, domain); + } - if (domain!=null) { - buf.append("; Domain="); - maybeQuote (version, buf, domain); - } - - // Max-Age=secs/Discard ... or use old "Expires" format - if (maxAge >= 0) { - if (version == 0) { - // XXX XXX XXX We need to send both, for - // interoperatibility (long word ) - buf.append ("; Expires="); - // Wdy, DD-Mon-YY HH:MM:SS GMT ( Expires netscape format ) - // To expire we need to set the time back in future - // ( pfrieden(a)dChain.com ) + // Max-Age=secs ... or use old "Expires" format + // TODO RFC2965 Discard + if (maxAge >= 0) { + if (version == 0) { + // Wdy, DD-Mon-YY HH:MM:SS GMT ( Expires Netscape format ) + buf.append ("; Expires="); + // To expire immediately we need to set the time in past if (maxAge == 0) - buf.append( ancientDate ); - else + buf.append( ancientDate ); + else DateTool.formatOldCookie (new Date( System.currentTimeMillis() + maxAge *1000L), buf, new FieldPosition(0)); - } else { - buf.append ("; Max-Age="); - buf.append (maxAge); - } - } + } else { + buf.append ("; Max-Age="); + buf.append (maxAge); + } + } - // Path=path - if (path!=null) { - buf.append ("; Path="); - maybeQuote (version, buf, path); - } + // Path=path + if (path!=null) { + buf.append ("; Path="); + maybeQuote2(version, buf, path); + } - // Secure - if (isSecure) { - buf.append ("; Secure"); - } - - + // Secure + if (isSecure) { + buf.append ("; Secure"); + } + + } + /** + * @deprecated - Not used + */ public static void maybeQuote (int version, StringBuffer buf, String value) { // special case - a \n or \r shouldn't happen in any case @@ -297,10 +310,17 @@ buf.append('"'); } } + + /** + * Quotes values using rules that vary depending on Cookie version. + * @param version + * @param buf + * @param value + */ public static void maybeQuote2 (int version, StringBuffer buf, String value) { // special case - a \n or \r shouldn't happen in any case - if (isToken2(value)) { + if (version == 0 && isToken(value) || version == 1 && isToken2(value)) { buf.append(value); } else { buf.append('"'); @@ -309,14 +329,7 @@ } } - // log - static final int dbg=1; - public static void log(String s ) { - if (log.isDebugEnabled()) - log.debug("ServerCookie: " + s); - } - /** * Escapes any double quotes in the given string. * @@ -331,18 +344,41 @@ } StringBuffer b = new StringBuffer(); - char p = s.charAt(0); for (int i = 0; i < s.length(); i++) { char c = s.charAt(i); - if (c == '"' && p != '\\') + if (c == '"') b.append('\\').append('"'); else b.append(c); - p = c; } return b.toString(); } + /** + * Unescapes any double quotes in the given cookie value. + * + * @param bc The cookie value to modify + */ + public static void unescapeDoubleQuotes(ByteChunk bc) { + + if (bc == null || bc.getLength() == 0 || bc.indexOf('"', 0) == -1) { + return; + } + + int src = bc.getStart(); + int end = bc.getEnd(); + int dest = src; + byte[] buffer = bc.getBuffer(); + + while (src < end) { + if (buffer[src] == '\\' && src < end && buffer[src+1] == '"') { + src++; + } + buffer[dest] = buffer[src]; + dest ++; + src ++; + } + bc.setEnd(dest); + } } - Modified: trunk/webapps/docs/changelog.xml =================================================================== --- trunk/webapps/docs/changelog.xml 2007-10-20 00:50:54 UTC (rev 316) +++ trunk/webapps/docs/changelog.xml 2007-10-20 23:36:09 UTC (rev 317) @@ -163,6 +163,9 @@ Cookie parser refactoring, submitted by John Kew. (remm) </update> <fix> + Make cookie escaping / unescaping consistent. (markt) + </fix> + <fix> <bug>43479</bug>: Memory leak cleaning up sendfile connections, submitted by Chris Elving. (remm) </fix> <fix>