414101 Do not escape special characters in cookies

jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java

@@ -18,6 +18,9 @@
 
 package org.eclipse.jetty.http;
 
+import static org.eclipse.jetty.util.QuotedStringTokenizer.isQuoted;
+import static org.eclipse.jetty.util.QuotedStringTokenizer.quoteOnly;
+
 import java.io.IOException;
 import java.nio.ByteBuffer;
 import java.text.SimpleDateFormat;
@@ -54,17 +57,21 @@ import org.eclipse.jetty.util.log.Logger;
 /**
  * HTTP Fields. A collection of HTTP header and or Trailer fields.
  *
- * <p>This class is not synchronized as it is expected that modifications will only be performed by a
+ * <p>This class is not synchronised as it is expected that modifications will only be performed by a
  * single thread.
  * 
+ * <p>The cookie handling provided by this class is guided by the Servlet specification and RFC6265.
+ *
  */
 public class HttpFields implements Iterable<HttpField>
 {
     private static final Logger LOG = Log.getLogger(HttpFields.class);
-    public static final String __COOKIE_DELIM="\"\\\n\r\t\f\b%+ ;=";
     public static final TimeZone __GMT = TimeZone.getTimeZone("GMT");
     public static final DateCache __dateCache = new DateCache("EEE, dd MMM yyyy HH:mm:ss 'GMT'", Locale.US);
 
+    public static final String __COOKIE_DELIM_PATH="\"\\\t%+ :;,@?=()<>{}[]";
+    public static final String __COOKIE_DELIM=__COOKIE_DELIM_PATH+"/";
+    
     static
     {
         __GMT.setID("GMT");
@@ -808,69 +815,87 @@ public class HttpFields implements Iterable<HttpField>
             final boolean isHttpOnly,
             int version)
     {
-        String delim=__COOKIE_DELIM;
-
         // Check arguments
         if (name == null || name.length() == 0)
             throw new IllegalArgumentException("Bad cookie name");
 
         // Format value and params
         StringBuilder buf = new StringBuilder(128);
-        String name_value_params;
+        
-        QuotedStringTokenizer.quoteIfNeeded(buf, name, delim);
+        // Name is checked by servlet spec, but can also be passed directly so check again
+        boolean quote_name=isQuoteNeededForCookie(name);
+        quoteOnlyOrAppend(buf,name,quote_name);
+        
         buf.append('=');
-        String start=buf.toString();
-        boolean hasDomain = false;
-        boolean hasPath = false;
         
-        if (value != null && value.length() > 0)
+        // Remember name= part to look for other matching set-cookie
-            QuotedStringTokenizer.quoteIfNeeded(buf, value, delim);
+        String name_equals=buf.toString();
 
+        // Append the value
+        boolean quote_value=isQuoteNeededForCookie(value);
+        quoteOnlyOrAppend(buf,value,quote_value);
 
-        if (path != null && path.length() > 0)
+        // Look for domain and path fields and check if they need to be quoted
+        boolean has_domain = domain!=null && domain.length()>0;
+        boolean quote_domain = has_domain && isQuoteNeededForCookie(domain);
+        boolean has_path = path!=null && path.length()>0;
+        boolean quote_path = has_path && isQuoteNeededForCookiePath(path);
+        
+        // Upgrade the version if we have a comment or we need to quote value/path/domain or if they were already quoted
+        if (version==0 && ( comment!=null || quote_name || quote_value || quote_domain || quote_path || isQuoted(name) || isQuoted(value) || isQuoted(path) || isQuoted(domain)))
+            version=1;
+
+        // Append version
+        if (version==1)
+            buf.append (";Version=1");
+        else if (version>1)
+            buf.append (";Version=").append(version);
+        
+        // Append path
+        if (has_path)
         {
-            hasPath = true;
             buf.append(";Path=");
-            if (path.trim().startsWith("\""))
+            quoteOnlyOrAppend(buf,path,quote_path);
-                buf.append(path);
-            else
-                QuotedStringTokenizer.quoteIfNeeded(buf,path,delim);
-        }
-        if (domain != null && domain.length() > 0)
-        {
-            hasDomain = true;
-            buf.append(";Domain=");
-            QuotedStringTokenizer.quoteIfNeeded(buf,domain.toLowerCase(Locale.ENGLISH),delim);
         }
         
+        // Append domain
+        if (has_domain)
+        {
+            buf.append(";Domain=");
+            quoteOnlyOrAppend(buf,domain,quote_domain);
+        }
+
+        // Handle max-age and/or expires
         if (maxAge >= 0)
         {
-            // Always add the expires param as some browsers still don't handle max-age
+            // Always use expires
+            // This is required as some browser (M$ this means you!) don't handle max-age even with v1 cookies
             buf.append(";Expires=");
             if (maxAge == 0)
                 buf.append(__01Jan1970_COOKIE);
             else
                 formatCookieDate(buf, System.currentTimeMillis() + 1000L * maxAge);
             
+            // for v1 cookies, also send max-age
+            if (version>=1)
+            {
                 buf.append(";Max-Age=");
                 buf.append(maxAge);
             }
+        }
 
+        // add the other fields
         if (isSecure)
             buf.append(";Secure");
         if (isHttpOnly)
             buf.append(";HttpOnly");
-
+        if (comment != null)
-        if (comment != null && comment.length() > 0)
         {
             buf.append(";Comment=");
-            QuotedStringTokenizer.quoteIfNeeded(buf, comment, delim);
+            quoteOnlyOrAppend(buf,comment,isQuoteNeededForCookie(comment));
         }
 
-        name_value_params = buf.toString();
+        // remove any existing set-cookie fields of same name
-
-        // remove existing set-cookie of same name
-
         Iterator<HttpField> i=_fields.iterator();
         while (i.hasNext())
         {
@@ -878,26 +903,26 @@ public class HttpFields implements Iterable<HttpField>
             if (field.getHeader()==HttpHeader.SET_COOKIE)
             {
                 String val = (field.getValue() == null ? null : field.getValue().toString());
-                if (val!=null && val.startsWith(start))
+                if (val!=null && val.startsWith(name_equals))
                 {
                     //existing cookie has same name, does it also match domain and path?
-                    if (((!hasDomain && !val.contains("Domain")) || (hasDomain && val.contains("Domain="+domain))) &&
+                    if (((!has_domain && !val.contains("Domain")) || (has_domain && val.contains(domain))) &&
-                        ((!hasPath && !val.contains("Path")) || (hasPath && val.contains("Path="+path))))
+                        ((!has_path && !val.contains("Path")) || (has_path && val.contains(path))))
                     {
                         i.remove();
                     }
                 }
-                
             }
         }
         
-        add(HttpHeader.SET_COOKIE.toString(), name_value_params);
+        // add the set cookie
+        add(HttpHeader.SET_COOKIE.toString(), buf.toString());
 
         // Expire responses with set-cookie headers so they do not get cached.
         put(HttpHeader.EXPIRES.toString(), __01Jan1970);
     }
 
-    public void putTo(ByteBuffer bufferInFillMode) throws IOException
+    public void putTo(ByteBuffer bufferInFillMode) 
     {
         for (HttpField field : _fields)
         {
@@ -1095,19 +1120,20 @@ public class HttpFields implements Iterable<HttpField>
             }
         }
 
-        List vl = LazyList.getList(list, false);
+        List<String> vl = LazyList.getList(list, false);
-        if (vl.size() < 2) return vl;
+        if (vl.size() < 2) 
+            return vl;
 
-        List ql = LazyList.getList(qual, false);
+        List<Float> ql = LazyList.getList(qual, false);
 
         // sort list with swaps
         Float last = __zero;
         for (int i = vl.size(); i-- > 0;)
         {
-            Float q = (Float) ql.get(i);
+            Float q = ql.get(i);
             if (last.compareTo(q) > 0)
             {
-                Object tmp = vl.get(i);
+                String tmp = vl.get(i);
                 vl.set(i, vl.get(i + 1));
                 vl.set(i + 1, tmp);
                 ql.set(i, ql.get(i + 1));
@@ -1123,4 +1149,66 @@ public class HttpFields implements Iterable<HttpField>
     }
 
 
+
+    /* ------------------------------------------------------------ */
+    /** Does a cookie value need to be quoted?
+     * @param s value string
+     * @return true if quoted;
+     * @throws IllegalArgumentException If there a control characters in the string
+     */
+    public static boolean isQuoteNeededForCookie(String s)
+    {
+        if (s==null || s.length()==0)
+            return true;
+        
+        if (QuotedStringTokenizer.isQuoted(s))
+            return false;
+
+        for (int i=0;i<s.length();i++)
+        {
+            char c = s.charAt(i);
+            if (__COOKIE_DELIM.indexOf(c)>=0)
+                return true;
+            
+            if (c<0x20 || c>=0x7f)
+                throw new IllegalArgumentException("Illegal character in cookie value");
+        }
+
+        return false;
+    }
+    
+    /* ------------------------------------------------------------ */
+    /** Does a cookie path need to be quoted?
+     * @param s value string
+     * @return true if quoted;
+     * @throws IllegalArgumentException If there a control characters in the string
+     */
+    public static boolean isQuoteNeededForCookiePath(String s)
+    {
+        if (s==null || s.length()==0)
+            return true;
+
+        if (QuotedStringTokenizer.isQuoted(s))
+            return false;
+        
+        for (int i=0;i<s.length();i++)
+        {
+            char c = s.charAt(i);
+            if (__COOKIE_DELIM_PATH.indexOf(c)>=0)
+                return true;
+            
+            if (c<0x20 || c>=0x7f)
+                throw new IllegalArgumentException("Illegal character in cookie value");
+        }
+
+        return false;
+    }
+    
+    private static void quoteOnlyOrAppend(StringBuilder buf, String s, boolean quote)
+    {
+        if (quote)
+            QuotedStringTokenizer.quoteOnly(buf,s);
+        else
+            buf.append(s);
+    }
 }

jetty-http/src/test/java/org/eclipse/jetty/http/HttpFieldsTest.java

@@ -279,10 +279,10 @@ public class HttpFieldsTest
         //test cookies with same name, domain and path, only 1 allowed
         fields.addSetCookie("everything","wrong","domain","path",0,"to be replaced",true,true,0);
         fields.addSetCookie("everything","value","domain","path",0,"comment",true,true,0);
-        assertEquals("everything=value;Path=path;Domain=domain;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",fields.getStringField("Set-Cookie"));
+        assertEquals("everything=value;Version=1;Path=path;Domain=domain;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",fields.getStringField("Set-Cookie"));
         Enumeration<String> e =fields.getValues("Set-Cookie");
         assertTrue(e.hasMoreElements());
-        assertEquals("everything=value;Path=path;Domain=domain;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
+        assertEquals("everything=value;Version=1;Path=path;Domain=domain;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
         assertFalse(e.hasMoreElements());
         assertEquals("Thu, 01 Jan 1970 00:00:00 GMT",fields.getStringField("Expires"));
         assertFalse(e.hasMoreElements());
@@ -293,9 +293,9 @@ public class HttpFieldsTest
         fields.addSetCookie("everything","value","domain2","path",0,"comment",true,true,0);
         e =fields.getValues("Set-Cookie");
         assertTrue(e.hasMoreElements());
-        assertEquals("everything=other;Path=path;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement());
+        assertEquals("everything=other;Version=1;Path=path;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement());
         assertTrue(e.hasMoreElements());
-        assertEquals("everything=value;Path=path;Domain=domain2;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
+        assertEquals("everything=value;Version=1;Path=path;Domain=domain2;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
         assertFalse(e.hasMoreElements());
         
         //test cookies with same name, same path, one with domain, one without
@@ -304,9 +304,9 @@ public class HttpFieldsTest
         fields.addSetCookie("everything","value","","path",0,"comment",true,true,0);
         e =fields.getValues("Set-Cookie");
         assertTrue(e.hasMoreElements());
-        assertEquals("everything=other;Path=path;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement());
+        assertEquals("everything=other;Version=1;Path=path;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement());
         assertTrue(e.hasMoreElements());
-        assertEquals("everything=value;Path=path;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
+        assertEquals("everything=value;Version=1;Path=path;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
         assertFalse(e.hasMoreElements());
         
         
@@ -316,9 +316,9 @@ public class HttpFieldsTest
         fields.addSetCookie("everything","value","domain1","path2",0,"comment",true,true,0);
         e =fields.getValues("Set-Cookie");
         assertTrue(e.hasMoreElements());
-        assertEquals("everything=other;Path=path1;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement());
+        assertEquals("everything=other;Version=1;Path=path1;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement());
         assertTrue(e.hasMoreElements());
-        assertEquals("everything=value;Path=path2;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
+        assertEquals("everything=value;Version=1;Path=path2;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
         assertFalse(e.hasMoreElements());
         
         //test cookies with same name, same domain, one with path, one without
@@ -327,9 +327,9 @@ public class HttpFieldsTest
         fields.addSetCookie("everything","value","domain1","",0,"comment",true,true,0);
         e =fields.getValues("Set-Cookie");
         assertTrue(e.hasMoreElements());
-        assertEquals("everything=other;Path=path1;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement());
+        assertEquals("everything=other;Version=1;Path=path1;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=blah",e.nextElement());
         assertTrue(e.hasMoreElements());
-        assertEquals("everything=value;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
+        assertEquals("everything=value;Version=1;Domain=domain1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
         assertFalse(e.hasMoreElements());
         
         //test cookies same name only, no path, no domain
@@ -338,13 +338,13 @@ public class HttpFieldsTest
         fields.addSetCookie("everything","value","","",0,"comment",true,true,0);
         e =fields.getValues("Set-Cookie");
         assertTrue(e.hasMoreElements());
-        assertEquals("everything=value;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
+        assertEquals("everything=value;Version=1;Expires=Thu, 01-Jan-1970 00:00:00 GMT;Max-Age=0;Secure;HttpOnly;Comment=comment",e.nextElement());
         assertFalse(e.hasMoreElements());
 
         fields.clear();
-        fields.addSetCookie("ev erything","va lue","do main","pa th",1,"co mment",true,true,2);
+        fields.addSetCookie("ev erything","va lue","do main","pa th",1,"co mment",true,true,1);
         String setCookie=fields.getStringField("Set-Cookie");
-        assertThat(setCookie,Matchers.startsWith("\"ev erything\"=\"va lue\";Path=\"pa th\";Domain=\"do main\";Expires="));
+        assertThat(setCookie,Matchers.startsWith("\"ev erything\"=\"va lue\";Version=1;Path=\"pa th\";Domain=\"do main\";Expires="));
         assertThat(setCookie,Matchers.endsWith(" GMT;Max-Age=1;Secure;HttpOnly;Comment=\"co mment\""));
 
         fields.clear();
@@ -376,7 +376,7 @@ public class HttpFieldsTest
         fields=new HttpFields();
         fields.addSetCookie("name","value==",null,null,-1,null,false,false,0);
         setCookie=fields.getStringField("Set-Cookie");
-        assertEquals("name=\"value==\"",setCookie);
+        assertEquals("name=\"value==\";Version=1",setCookie);
 
     }
 

jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java

@@ -601,7 +601,7 @@ public class ResponseTest
 
         String set = response.getHttpFields().getStringField("Set-Cookie");
 
-        assertEquals("name=value;Path=/path;Domain=domain;Secure;HttpOnly;Comment=comment", set);
+        assertEquals("name=value;Version=1;Path=/path;Domain=domain;Secure;HttpOnly;Comment=comment", set);
     }
 
 
@@ -630,7 +630,7 @@ public class ResponseTest
         assertNotNull(set);
         ArrayList<String> list = Collections.list(set);
         assertEquals(2, list.size());
-        assertTrue(list.contains("name=value;Path=/path;Domain=domain;Secure;HttpOnly;Comment=comment"));
+        assertTrue(list.contains("name=value;Version=1;Path=/path;Domain=domain;Secure;HttpOnly;Comment=comment"));
         assertTrue(list.contains("name2=value2;Path=/path;Domain=domain"));
 
         //get rid of the cookies

jetty-spdy/spdy-http-server/src/test/java/org/eclipse/jetty/spdy/server/http/ServerHTTPSPDYTest.java

@@ -189,9 +189,9 @@ public class ServerHTTPSPDYTest extends AbstractHTTPSPDYTest
                 assertThat("response code is 200 OK", replyHeaders.get(HTTPSPDYHeader.STATUS.name(version)).value()
                         .contains("200"), is(true));
                 assertThat(replyInfo.getHeaders().get("Set-Cookie").values()[0], is(cookie1 + "=\"" + cookie1Value +
-                        "\""));
+                        "\";Version=1"));
                 assertThat(replyInfo.getHeaders().get("Set-Cookie").values()[1], is(cookie2 + "=\"" + cookie2Value +
-                        "\""));
+                        "\";Version=1"));
                 replyLatch.countDown();
             }
         });

jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java

@@ -332,6 +332,32 @@ public class QuotedStringTokenizer
         escapes['\r'] = 'r';
     }
 
+    /* ------------------------------------------------------------ */
+    /** Quote a string into an Appendable.
+     * Only quotes and backslash are escaped. 
+     * @param buffer The Appendable
+     * @param input The String to quote.
+     */
+    public static void quoteOnly(Appendable buffer, String input)
+    {
+        try
+        {
+            buffer.append('"');
+            for (int i = 0; i < input.length(); ++i)
+            {
+                char c = input.charAt(i);
+                if (c == '"' || c == '\\')
+                    buffer.append('\\');
+                buffer.append(c);
+            }
+            buffer.append('"');
+        }
+        catch (IOException x)
+        {
+            throw new RuntimeException(x);
+        }
+    }
+
     /* ------------------------------------------------------------ */
     /** Quote a string into an Appendable.
      * The characters ", \, \n, \r, \t, \f and \b are escaped
@@ -377,38 +403,6 @@ public class QuotedStringTokenizer
         }
     }
 
-    /* ------------------------------------------------------------ */
-    /** Quote a string into a StringBuffer only if needed.
-     * Quotes are forced if any delim characters are present.
-     *
-     * @param buf The StringBuffer
-     * @param s The String to quote.
-     * @param delim String of characters that must be quoted.
-     * @return true if quoted;
-     */
-    public static boolean quoteIfNeeded(Appendable buf, String s,String delim)
-    {
-        for (int i=0;i<s.length();i++)
-        {
-            char c = s.charAt(i);
-            if (delim.indexOf(c)>=0)
-            {
-            	quote(buf,s);
-            	return true;
-            }
-        }
-
-        try
-        {
-            buf.append(s);
-            return false;
-        }
-        catch(IOException e)
-        {
-            throw new RuntimeException(e);
-        }
-    }
-    
     
     /* ------------------------------------------------------------ */
     public static String unquoteOnly(String s)
@@ -565,6 +559,12 @@ public class QuotedStringTokenizer
                  (c == '/') || (c == '"') || (c == 'u'));
     }
 
+    /* ------------------------------------------------------------ */
+    public static boolean isQuoted(String s)
+    {
+        return s!=null && s.length()>0 && s.charAt(0)=='"' && s.charAt(s.length()-1)=='"';
+    }
+    
     /* ------------------------------------------------------------ */
     /**
      * @return handle double quotes if true

jetty-util/src/test/java/org/eclipse/jetty/util/QuotedStringTokenizerTest.java

@@ -112,14 +112,6 @@ public class QuotedStringTokenizerTest
         QuotedStringTokenizer.quote(buf,"abcefg\"");
         assertEquals("\"abcefg\\\"\"",buf.toString());
 
-        buf.setLength(0);
-        QuotedStringTokenizer.quoteIfNeeded(buf,"abc \n efg","\"\\\n\r\t\f\b%+ ;=");
-        assertEquals("\"abc \\n efg\"",buf.toString());
-
-        buf.setLength(0);
-        QuotedStringTokenizer.quoteIfNeeded(buf,"abcefg","\"\\\n\r\t\f\b%+ ;=");
-        assertEquals("abcefg",buf.toString());
-
     }
 
     /*