From 2eb2eda4bb2fe9db240dfa1abbc1df0787ea3667 Mon Sep 17 00:00:00 2001 From: Oleg Kalnichevski Date: Mon, 10 Jan 2011 10:57:29 +0000 Subject: [PATCH] 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 | 7 ++ .../http/impl/cookie/BestMatchSpec.java | 43 +++++++---- .../http/impl/cookie/BrowserCompatSpec.java | 33 ++++---- .../cookie/NetscapeDraftHeaderParser.java | 77 +++++++++++++++++-- .../http/impl/cookie/NetscapeDraftSpec.java | 10 +-- .../impl/cookie/TestBrowserCompatSpec.java | 6 +- .../impl/cookie/TestCookieBestMatchSpec.java | 12 +-- .../cookie/TestNetscapeDraftHeaderParser.java | 6 +- 8 files changed, 129 insertions(+), 65 deletions(-) diff --git a/RELEASE_NOTES.txt b/RELEASE_NOTES.txt index 80c7c3792..219c5d12d 100644 --- a/RELEASE_NOTES.txt +++ b/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 + * [HTTPCLIENT-1036] StringBody has incorrect default for characterset. (Default changed to US-ASCII) Contributed by Sebastian Bazley diff --git a/httpclient/src/main/java/org/apache/http/impl/cookie/BestMatchSpec.java b/httpclient/src/main/java/org/apache/http/impl/cookie/BestMatchSpec.java index 6399b00db..ce6211eef 100644 --- a/httpclient/src/main/java/org/apache/http/impl/cookie/BestMatchSpec.java +++ b/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 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 (versioned) { + if (netscape || !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); } } diff --git a/httpclient/src/main/java/org/apache/http/impl/cookie/BrowserCompatSpec.java b/httpclient/src/main/java/org/apache/http/impl/cookie/BrowserCompatSpec.java index 141031a43..018619fce 100644 --- a/httpclient/src/main/java/org/apache/http/impl/cookie/BrowserCompatSpec.java +++ b/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; - int i1 = headervalue.toLowerCase(Locale.ENGLISH).indexOf("expires="); - if (i1 != -1) { - i1 += "expires=".length(); - int i2 = headervalue.indexOf(';', i1); - if (i2 == -1) { - i2 = headervalue.length(); + HeaderElement[] helems = header.getElements(); + boolean versioned = false; + boolean netscape = false; + for (HeaderElement helem: helems) { + if (helem.getParameterByName("version") != null) { + versioned = true; } - try { - DateUtils.parseDate(headervalue.substring(i1, i2), this.datepatterns); - isNetscapeCookie = true; - } catch (DateParseException e) { - // Does not look like a valid expiry date + if (helem.getParameterByName("expires") != null) { + netscape = true; } } - HeaderElement[] elems = null; - if (isNetscapeCookie) { + if (netscape || !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; @@ -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) }; - } else { - elems = header.getElements(); + helems = new HeaderElement[] { parser.parseHeader(buffer, cursor) }; } - return parse(elems, origin); + return parse(helems, origin); } public List
formatCookies(final List cookies) { diff --git a/httpclient/src/main/java/org/apache/http/impl/cookie/NetscapeDraftHeaderParser.java b/httpclient/src/main/java/org/apache/http/impl/cookie/NetscapeDraftHeaderParser.java index 4b65e37df..0538a04b3 100644 --- a/httpclient/src/main/java/org/apache/http/impl/cookie/NetscapeDraftHeaderParser.java +++ b/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 params = new ArrayList(); 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); + } + } diff --git a/httpclient/src/main/java/org/apache/http/impl/cookie/NetscapeDraftSpec.java b/httpclient/src/main/java/org/apache/http/impl/cookie/NetscapeDraftSpec.java index d2fb0f752..9417d02fa 100644 --- a/httpclient/src/main/java/org/apache/http/impl/cookie/NetscapeDraftSpec.java +++ b/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 * * - *

Please note that Netscape draft specification does not fully - * conform to the HTTP header format. Netscape draft does not specify - * whether multiple cookies may be sent in one header. Hence, comma - * character may be present in unquoted cookie value or unquoted - * parameter value.

+ *

Please note that the Netscape draft specification does not fully conform to the HTTP + * header format. Comma character if present in Set-Cookie will not be treated + * as a header element separator

* - * @see + * @see * The Cookie Spec. * * @param header the Set-Cookie received from the server diff --git a/httpclient/src/test/java/org/apache/http/impl/cookie/TestBrowserCompatSpec.java b/httpclient/src/test/java/org/apache/http/impl/cookie/TestBrowserCompatSpec.java index 2597f36ea..2aadfce52 100644 --- a/httpclient/src/test/java/org/apache/http/impl/cookie/TestBrowserCompatSpec.java +++ b/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 cookies = cookiespec.parse(header, origin); diff --git a/httpclient/src/test/java/org/apache/http/impl/cookie/TestCookieBestMatchSpec.java b/httpclient/src/test/java/org/apache/http/impl/cookie/TestCookieBestMatchSpec.java index c57c7887c..619dd7693 100644 --- a/httpclient/src/test/java/org/apache/http/impl/cookie/TestCookieBestMatchSpec.java +++ b/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 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); - cookiespec.validate(cookies.get(0), origin); - Assert.fail("MalformedCookieException exception should have been thrown"); - } catch (MalformedCookieException e) { - // expected - } + cookies = cookiespec.parse(header, origin); + cookiespec.validate(cookies.get(0), origin); + Assert.assertEquals(1, cookies.size()); } @Test diff --git a/httpclient/src/test/java/org/apache/http/impl/cookie/TestNetscapeDraftHeaderParser.java b/httpclient/src/test/java/org/apache/http/impl/cookie/TestNetscapeDraftHeaderParser.java index f1cbf4251..0dfc47164 100644 --- a/httpclient/src/test/java/org/apache/http/impl/cookie/TestNetscapeDraftHeaderParser.java +++ b/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 = - "name = value; test; test1 = stuff,with,commas ; test2 = \"stuff; stuff\"; test3=\"stuff"; + String s = "name = value; test; test1 = stuff,with,commas ;" + + " 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());