Changed Browser-Compatibility and Best-Match cookie policies to emulate the behaviour of FireFox more closely when parsing Netscape style cookies. Comma will no longer be treated as a header element separator if Set-Cookie does not contain a Version attribute mandated by the RFC2109 / RFC 2965 cookie specifications

git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@1057148 13f79535-47bb-0310-9956-ffa450edef68

RELEASE_NOTES.txt

@@ -32,6 +32,13 @@ maintained and supported by Apache HttpComponents project.
 
 Changelog
 -------------------
+
+* Changed Browser-Compatibility and Best-Match cookie policies to emulate the behaviour of FireFox
+  more closely when parsing Netscape style cookies. Comma will no longer be treated as a header
+  element separator if Set-Cookie does not contain a Version attribute mandated by the 
+  RFC2109 / RFC 2965 cookie specifications.
+  Contributed by Oleg Kalnichevski <olegk at apache.org>
+
 * [HTTPCLIENT-1036] StringBody has incorrect default for characterset. (Default changed 
   to US-ASCII)
   Contributed by Sebastian Bazley <sebb at apache.org>

httpclient/src/main/java/org/apache/http/impl/cookie/BestMatchSpec.java

@@ -31,6 +31,7 @@ import java.util.List;
 
 import org.apache.http.annotation.NotThreadSafe;
 
+import org.apache.http.FormattedHeader;
 import org.apache.http.Header;
 import org.apache.http.HeaderElement;
 import org.apache.http.cookie.Cookie;
@@ -39,6 +40,8 @@ import org.apache.http.cookie.CookieSpec;
 import org.apache.http.cookie.MalformedCookieException;
 import org.apache.http.cookie.SM;
 import org.apache.http.cookie.SetCookie2;
+import org.apache.http.message.ParserCursor;
+import org.apache.http.util.CharArrayBuffer;
 
 /**
  * 'Meta' cookie specification that picks up a cookie policy based on
@@ -56,7 +59,6 @@ public class BestMatchSpec implements CookieSpec {
     private RFC2965Spec strict; // @NotThreadSafe
     private RFC2109Spec obsoleteStrict; // @NotThreadSafe
     private BrowserCompatSpec compat; // @NotThreadSafe
-    private NetscapeDraftSpec netscape; // @NotThreadSafe
 
     public BestMatchSpec(final String[] datepatterns, boolean oneHeader) {
         super();
@@ -89,13 +91,6 @@ public class BestMatchSpec implements CookieSpec {
         return compat;
     }
 
-    private NetscapeDraftSpec getNetscape() {
-        if (this.netscape == null) {
-            this.netscape = new NetscapeDraftSpec(this.datepatterns);
-        }
-        return netscape;
-    }
-
     public List<Cookie> parse(
             final Header header,
             final CookieOrigin origin) throws MalformedCookieException {
@@ -116,20 +111,34 @@ public class BestMatchSpec implements CookieSpec {
                netscape = true;
             }
         }
-        // Do we have a cookie with a version attribute?
+        if (netscape || !versioned) {
-        if (versioned) {
+            // Need to parse the header again, because Netscape style cookies do not correctly
+            // support multiple header elements (comma cannot be treated as an element separator)
+            NetscapeDraftHeaderParser parser = NetscapeDraftHeaderParser.DEFAULT;
+            CharArrayBuffer buffer;
+            ParserCursor cursor;
+            if (header instanceof FormattedHeader) {
+                buffer = ((FormattedHeader) header).getBuffer();
+                cursor = new ParserCursor(
+                        ((FormattedHeader) header).getValuePos(),
+                        buffer.length());
+            } else {
+                String s = header.getValue();
+                if (s == null) {
+                    throw new MalformedCookieException("Header value is null");
+                }
+                buffer = new CharArrayBuffer(s.length());
+                buffer.append(s);
+                cursor = new ParserCursor(0, buffer.length());
+            }
+            helems = new HeaderElement[] { parser.parseHeader(buffer, cursor) };
+            return getCompat().parse(helems, origin);
+        } else {
             if (SM.SET_COOKIE2.equals(header.getName())) {
                 return getStrict().parse(helems, origin);
             } else {
                 return getObsoleteStrict().parse(helems, origin);
             }
-        } else if (netscape) {
-            // Need to parse the header again,
-            // because Netscape draft cannot handle
-            // comma separators
-            return getNetscape().parse(header, origin);
-        } else {
-            return getCompat().parse(helems, origin);
         }
     }
 

httpclient/src/main/java/org/apache/http/impl/cookie/BrowserCompatSpec.java

@@ -29,7 +29,6 @@ package org.apache.http.impl.cookie;
 
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Locale;
 
 import org.apache.http.annotation.NotThreadSafe;
 
@@ -124,28 +123,24 @@ public class BrowserCompatSpec extends CookieSpecBase {
             throw new IllegalArgumentException("Cookie origin may not be null");
         }
         String headername = header.getName();
-        String headervalue = header.getValue();
         if (!headername.equalsIgnoreCase(SM.SET_COOKIE)) {
             throw new MalformedCookieException("Unrecognized cookie header '"
                     + header.toString() + "'");
         }
-        boolean isNetscapeCookie = false;
+        HeaderElement[] helems = header.getElements();
-        int i1 = headervalue.toLowerCase(Locale.ENGLISH).indexOf("expires=");
+        boolean versioned = false;
-        if (i1 != -1) {
+        boolean netscape = false;
-            i1 += "expires=".length();
+        for (HeaderElement helem: helems) {
-            int i2 = headervalue.indexOf(';', i1);
+            if (helem.getParameterByName("version") != null) {
-            if (i2 == -1) {
+                versioned = true;
-                i2 = headervalue.length();
             }
-            try {
+            if (helem.getParameterByName("expires") != null) {
-                DateUtils.parseDate(headervalue.substring(i1, i2), this.datepatterns);
+               netscape = true;
-                isNetscapeCookie = true;
-            } catch (DateParseException e) {
-                // Does not look like a valid expiry date
             }
         }
-        HeaderElement[] elems = null;
+        if (netscape || !versioned) {
-        if (isNetscapeCookie) {
+            // Need to parse the header again, because Netscape style cookies do not correctly
+            // support multiple header elements (comma cannot be treated as an element separator)
             NetscapeDraftHeaderParser parser = NetscapeDraftHeaderParser.DEFAULT;
             CharArrayBuffer buffer;
             ParserCursor cursor;
@@ -163,11 +158,9 @@ public class BrowserCompatSpec extends CookieSpecBase {
                 buffer.append(s);
                 cursor = new ParserCursor(0, buffer.length());
             }
-            elems = new HeaderElement[] { parser.parseHeader(buffer, cursor) };
+            helems = new HeaderElement[] { parser.parseHeader(buffer, cursor) };
-        } else {
-            elems = header.getElements();
         }
-        return parse(elems, origin);
+        return parse(helems, origin);
     }
 
     public List<Header> formatCookies(final List<Cookie> cookies) {

httpclient/src/main/java/org/apache/http/impl/cookie/NetscapeDraftHeaderParser.java

@@ -36,8 +36,9 @@ import org.apache.http.HeaderElement;
 import org.apache.http.NameValuePair;
 import org.apache.http.ParseException;
 import org.apache.http.message.BasicHeaderElement;
-import org.apache.http.message.BasicHeaderValueParser;
+import org.apache.http.message.BasicNameValuePair;
 import org.apache.http.message.ParserCursor;
+import org.apache.http.protocol.HTTP;
 import org.apache.http.util.CharArrayBuffer;
 
 /**
@@ -49,13 +50,8 @@ public class NetscapeDraftHeaderParser {
 
     public final static NetscapeDraftHeaderParser DEFAULT = new NetscapeDraftHeaderParser();
 
-    private final static char[] DELIMITERS = new char[] { ';' };
-
-    private final BasicHeaderValueParser nvpParser;
-
     public NetscapeDraftHeaderParser() {
         super();
-        this.nvpParser = BasicHeaderValueParser.DEFAULT;
     }
 
     public HeaderElement parseHeader(
@@ -67,10 +63,10 @@ public class NetscapeDraftHeaderParser {
         if (cursor == null) {
             throw new IllegalArgumentException("Parser cursor may not be null");
         }
-        NameValuePair nvp = this.nvpParser.parseNameValuePair(buffer, cursor, DELIMITERS);
+        NameValuePair nvp = parseNameValuePair(buffer, cursor);
         List<NameValuePair> params = new ArrayList<NameValuePair>();
         while (!cursor.atEnd()) {
-            NameValuePair param = this.nvpParser.parseNameValuePair(buffer, cursor, DELIMITERS);
+            NameValuePair param = parseNameValuePair(buffer, cursor);
             params.add(param);
         }
         return new BasicHeaderElement(
@@ -78,4 +74,69 @@ public class NetscapeDraftHeaderParser {
                 nvp.getValue(), params.toArray(new NameValuePair[params.size()]));
     }
 
+    private NameValuePair parseNameValuePair(
+            final CharArrayBuffer buffer, final ParserCursor cursor) {
+        boolean terminated = false;
+
+        int pos = cursor.getPos();
+        int indexFrom = cursor.getPos();
+        int indexTo = cursor.getUpperBound();
+
+        // Find name
+        String name = null;
+        while (pos < indexTo) {
+            char ch = buffer.charAt(pos);
+            if (ch == '=') {
+                break;
+            }
+            if (ch == ';') {
+                terminated = true;
+                break;
+            }
+            pos++;
+        }
+
+        if (pos == indexTo) {
+            terminated = true;
+            name = buffer.substringTrimmed(indexFrom, indexTo);
+        } else {
+            name = buffer.substringTrimmed(indexFrom, pos);
+            pos++;
+        }
+
+        if (terminated) {
+            cursor.updatePos(pos);
+            return new BasicNameValuePair(name, null);
+        }
+
+        // Find value
+        String value = null;
+        int i1 = pos;
+
+        while (pos < indexTo) {
+            char ch = buffer.charAt(pos);
+            if (ch == ';') {
+                terminated = true;
+                break;
+            }
+            pos++;
+        }
+
+        int i2 = pos;
+        // Trim leading white spaces
+        while (i1 < i2 && (HTTP.isWhitespace(buffer.charAt(i1)))) {
+            i1++;
+        }
+        // Trim trailing white spaces
+        while ((i2 > i1) && (HTTP.isWhitespace(buffer.charAt(i2 - 1)))) {
+            i2--;
+        }
+        value = buffer.substring(i1, i2);
+        if (terminated) {
+            pos++;
+        }
+        cursor.updatePos(pos);
+        return new BasicNameValuePair(name, value);
+    }
+
 }

httpclient/src/main/java/org/apache/http/impl/cookie/NetscapeDraftSpec.java

@@ -94,13 +94,11 @@ public class NetscapeDraftSpec extends CookieSpecBase {
       *  Set-Cookie: NAME=VALUE; expires=DATE; path=PATH; domain=DOMAIN_NAME; secure
       * </PRE>
       *
-      * <p>Please note that Netscape draft specification does not fully
+      * <p>Please note that the Netscape draft specification does not fully conform to the HTTP
-      * conform to the HTTP header format. Netscape draft does not specify
+      * header format. Comma character if present in <code>Set-Cookie</code> will not be treated
-      * whether multiple cookies may be sent in one header. Hence, comma
+      * as a header element separator</p>
-      * character may be present in unquoted cookie value or unquoted
-      * parameter value.</p>
       *
-      * @see <a href="http://wp.netscape.com/newsref/std/cookie_spec.html">
+      * @see <a href="http://web.archive.org/web/20020803110822/http://wp.netscape.com/newsref/std/cookie_spec.html">
       *  The Cookie Spec.</a>
       *
       * @param header the <tt>Set-Cookie</tt> received from the server

httpclient/src/test/java/org/apache/http/impl/cookie/TestBrowserCompatSpec.java

@@ -328,7 +328,7 @@ public class TestBrowserCompatSpec {
         }
         Assert.assertEquals("Found 1 cookie.",1,cookies.size());
         Assert.assertEquals("Name","cookie-name",cookies.get(0).getName());
-        Assert.assertEquals("Value"," cookie-value ",cookies.get(0).getValue());
+        Assert.assertEquals("Value","\" cookie-value \"",cookies.get(0).getValue());
         Assert.assertEquals("Domain","127.0.0.1",cookies.get(0).getDomain());
         Assert.assertEquals("Path","/",cookies.get(0).getPath());
         Assert.assertTrue("Secure",!cookies.get(0).isSecure());
@@ -414,7 +414,7 @@ public class TestBrowserCompatSpec {
         Assert.assertEquals("Path","/",cookies.get(0).getPath());
         Assert.assertTrue("Secure",!cookies.get(0).isSecure());
         Assert.assertTrue("ExpiryDate",null == cookies.get(0).getExpiryDate());
-        Assert.assertEquals("Comment","This is a comment.",cookies.get(0).getComment());
+        Assert.assertEquals("Comment","\"This is a comment.\"",cookies.get(0).getComment());
     }
 
     @Test
@@ -927,7 +927,7 @@ public class TestBrowserCompatSpec {
     @Test
     public void testFormatSeveralCookies() throws Exception {
         Header header = new BasicHeader("Set-Cookie",
-            "name1=value1; path=/; domain=.mydomain.com, name2 = value2 ; path=/; domain=.mydomain.com");
+            "name1=value1; path=/; domain=.mydomain.com, name2 = value2 ; path=/; domain=.mydomain.com; version=0");
         CookieSpec cookiespec = new BrowserCompatSpec();
         CookieOrigin origin = new CookieOrigin("myhost.mydomain.com", 80, "/", false);
         List<Cookie> cookies = cookiespec.parse(header, origin);

httpclient/src/test/java/org/apache/http/impl/cookie/TestCookieBestMatchSpec.java

@@ -70,16 +70,12 @@ public class TestCookieBestMatchSpec {
             "name=value; path=/; domain=.mydomain.com; expires=Thu, 01-Jan-2070 00:00:10 GMT; comment=no_comment");
         List<Cookie> cookies = cookiespec.parse(header, origin);
         cookiespec.validate(cookies.get(0), origin);
-
+        Assert.assertEquals(1, cookies.size());
         header = new BasicHeader("Set-Cookie",
             "name=value; path=/; domain=.mydomain.com; expires=Thu, 01-Jan-2070 00:00:10 GMT; version=1");
-        try {
+        cookies = cookiespec.parse(header, origin);
-            cookies = cookiespec.parse(header, origin);
+        cookiespec.validate(cookies.get(0), origin);
-            cookiespec.validate(cookies.get(0), origin);
+        Assert.assertEquals(1, cookies.size());
-            Assert.fail("MalformedCookieException exception should have been thrown");
-        } catch (MalformedCookieException e) {
-            // expected
-        }
     }
 
     @Test

httpclient/src/test/java/org/apache/http/impl/cookie/TestNetscapeDraftHeaderParser.java

@@ -43,8 +43,8 @@ public class TestNetscapeDraftHeaderParser {
     public void testNetscapeCookieParsing() throws Exception {
         NetscapeDraftHeaderParser parser = NetscapeDraftHeaderParser.DEFAULT;
 
-        String s =
+        String s = "name  = value; test; test1 =  stuff,with,commas   ;" +
-            "name  = value; test; test1 =  stuff,with,commas   ; test2 =  \"stuff; stuff\"; test3=\"stuff";
+                " test2 =  \"stuff, stuff\"; test3=\"stuff";
         CharArrayBuffer buffer = new CharArrayBuffer(16);
         buffer.append(s);
         ParserCursor cursor = new ParserCursor(0, s.length());
@@ -58,7 +58,7 @@ public class TestNetscapeDraftHeaderParser {
         Assert.assertEquals("test1", params[1].getName());
         Assert.assertEquals("stuff,with,commas", params[1].getValue());
         Assert.assertEquals("test2", params[2].getName());
-        Assert.assertEquals("stuff; stuff", params[2].getValue());
+        Assert.assertEquals("\"stuff, stuff\"", params[2].getValue());
         Assert.assertEquals("test3", params[3].getName());
         Assert.assertEquals("\"stuff", params[3].getValue());
         Assert.assertEquals(s.length(), cursor.getPos());