Handle double quoted cookie values, master fix AsyncHttpClient#283

Author: Stephane Landelle

api/src/main/java/com/ning/http/client/Cookie.java

@@ -20,35 +20,83 @@
 import java.util.Set;
 import java.util.TreeSet;
 
-public class Cookie {
+public class Cookie implements Comparable<Cookie>{
     private final String domain;
     private final String name;
     private final String value;
     private final String path;
     private final int maxAge;
     private final boolean secure;
     private final int version;
+    private final boolean httpOnly;
+    private final boolean discard;
+    private final String comment;
+    private final String commentUrl;
+
     private Set<Integer> ports = Collections.emptySet();
     private Set<Integer> unmodifiablePorts = ports;
 
-    public Cookie(String domain, String name, String value, String path, int maxAge, boolean secure) {
-        this.domain = domain;
-        this.name = name;
-        this.value = value;
-        this.path = path;
-        this.maxAge = maxAge;
-        this.secure = secure;
-        this.version = 1;
-    }
+    public Cookie(String domain, String name, String value, String path, int maxAge, boolean secure, int version, boolean httpOnly, boolean discard, String comment, String commentUrl, Iterable<Integer> ports) {
+
+        if (name == null) {
+            throw new NullPointerException("name");
+        }
+        name = name.trim();
+        if (name.length() == 0) {
+            throw new IllegalArgumentException("empty name");
+        }
+
+        for (int i = 0; i < name.length(); i++) {
+            char c = name.charAt(i);
+            if (c > 127) {
+                throw new IllegalArgumentException("name contains non-ascii character: " + name);
+            }
+
+            // Check prohibited characters.
+            switch (c) {
+            case '\t':
+            case '\n':
+            case 0x0b:
+            case '\f':
+            case '\r':
+            case ' ':
+            case ',':
+            case ';':
+            case '=':
+                throw new IllegalArgumentException("name contains one of the following prohibited characters: " + "=,; \\t\\r\\n\\v\\f: " + name);
+            }
+        }
+
+        if (name.charAt(0) == '$') {
+            throw new IllegalArgumentException("name starting with '$' not allowed: " + name);
+        }
+
+        if (value == null) {
+            throw new NullPointerException("value");
+        }
 
-    public Cookie(String domain, String name, String value, String path, int maxAge, boolean secure, int version) {
-        this.domain = domain;
         this.name = name;
         this.value = value;
-        this.path = path;
+        this.domain = validateValue("domain", domain);
+        this.path = validateValue("path", path);
         this.maxAge = maxAge;
         this.secure = secure;
         this.version = version;
+        this.httpOnly = httpOnly;
+
+        if (version > 0) {
+            this.comment = validateValue("comment", comment);
+        } else {
+            this.comment = null;
+        }
+        if (version > 1) {
+            this.discard = discard;
+            this.commentUrl = validateValue("commentUrl", commentUrl);
+            setPorts(ports);
+        } else {
+            this.discard = false;
+            this.commentUrl = null;
+        }
     }
 
     public String getDomain() {
@@ -79,35 +127,30 @@ public int getVersion() {
         return version;
     }
 
+    public String getComment() {
+        return this.comment;
+    }
+
+    public String getCommentUrl() {
+        return this.commentUrl;
+    }
+
+    public boolean isHttpOnly() {
+        return httpOnly;
+    }
+
+    public boolean isDiscard() {
+        return discard;
+    }
+
     public Set<Integer> getPorts() {
         if (unmodifiablePorts == null) {
             unmodifiablePorts = Collections.unmodifiableSet(ports);
         }
         return unmodifiablePorts;
     }
 
-    public void setPorts(int... ports) {
-        if (ports == null) {
-            throw new NullPointerException("ports");
-        }
-
-        int[] portsCopy = ports.clone();
-        if (portsCopy.length == 0) {
-            unmodifiablePorts = this.ports = Collections.emptySet();
-        } else {
-            Set<Integer> newPorts = new TreeSet<Integer>();
-            for (int p : portsCopy) {
-                if (p <= 0 || p > 65535) {
-                    throw new IllegalArgumentException("port out of range: " + p);
-                }
-                newPorts.add(Integer.valueOf(p));
-            }
-            this.ports = newPorts;
-            unmodifiablePorts = null;
-        }
-    }
-
-    public void setPorts(Iterable<Integer> ports) {
+    private void setPorts(Iterable<Integer> ports) {
         Set<Integer> newPorts = new TreeSet<Integer>();
         for (int p : ports) {
             if (p <= 0 || p > 65535) {
@@ -125,7 +168,89 @@ public void setPorts(Iterable<Integer> ports) {
 
     @Override
     public String toString() {
-        return String.format("Cookie: domain=%s, name=%s, value=%s, path=%s, maxAge=%d, secure=%s",
-                domain, name, value, path, maxAge, secure);
+        StringBuilder buf = new StringBuilder();
+        buf.append(getName());
+        buf.append('=');
+        buf.append(getValue());
+        if (getDomain() != null) {
+            buf.append("; domain=");
+            buf.append(getDomain());
+        }
+        if (getPath() != null) {
+            buf.append("; path=");
+            buf.append(getPath());
+        }
+        if (getComment() != null) {
+            buf.append("; comment=");
+            buf.append(getComment());
+        }
+        if (getMaxAge() >= 0) {
+            buf.append("; maxAge=");
+            buf.append(getMaxAge());
+            buf.append('s');
+        }
+        if (isSecure()) {
+            buf.append("; secure");
+        }
+        if (isHttpOnly()) {
+            buf.append("; HTTPOnly");
+        }
+        return buf.toString();
+    }
+
+    private String validateValue(String name, String value) {
+        if (value == null) {
+            return null;
+        }
+        value = value.trim();
+        if (value.length() == 0) {
+            return null;
+        }
+        for (int i = 0; i < value.length(); i++) {
+            char c = value.charAt(i);
+            switch (c) {
+            case '\r':
+            case '\n':
+            case '\f':
+            case 0x0b:
+            case ';':
+                throw new IllegalArgumentException(name + " contains one of the following prohibited characters: " + ";\\r\\n\\f\\v (" + value + ')');
+            }
+        }
+        return value;
+    }
+
+    public int compareTo(Cookie c) {
+        int v;
+        v = getName().compareToIgnoreCase(c.getName());
+        if (v != 0) {
+            return v;
+        }
+
+        if (getPath() == null) {
+            if (c.getPath() != null) {
+                return -1;
+            }
+        } else if (c.getPath() == null) {
+            return 1;
+        } else {
+            v = getPath().compareTo(c.getPath());
+            if (v != 0) {
+                return v;
+            }
+        }
+
+        if (getDomain() == null) {
+            if (c.getDomain() != null) {
+                return -1;
+            }
+        } else if (c.getDomain() == null) {
+            return 1;
+        } else {
+            v = getDomain().compareToIgnoreCase(c.getDomain());
+            return v;
+        }
+
+        return 0;
     }
-}
+}

api/src/main/java/com/ning/http/client/providers/jdk/JDKResponse.java

@@ -12,6 +12,7 @@
  */
 package com.ning.http.client.providers.jdk;
 
+import com.ning.org.jboss.netty.handler.codec.http.CookieDecoder;
 import com.ning.http.client.Cookie;
 import com.ning.http.client.HttpResponseBodyPart;
 import com.ning.http.client.HttpResponseHeaders;
@@ -53,8 +54,7 @@ public List<Cookie> buildCookies() {
                 // TODO: ask for parsed header
                 List<String> v = header.getValue();
                 for (String value : v) {
-                    Cookie cookie = AsyncHttpProviderUtils.parseCookie(value);
-                    cookies.add(cookie);
+                    cookies.addAll(CookieDecoder.decode(value));
                 }
             }
         }

api/src/main/java/com/ning/http/util/AsyncHttpProviderUtils.java

@@ -508,69 +508,15 @@ public static String parseCharset(String contentType) {
         return null;
     }
 
-    public static Cookie parseCookie(String value) {
-        String[] fields = value.split(";\\s*");
-        String[] cookie = fields[0].split("=", 2);
-        String cookieName = cookie[0];
-        String cookieValue = (cookie.length == 1) ? null : cookie[1];
-
-        int maxAge = -1;
-        String path = null;
-        String domain = null;
-        boolean secure = false;
-
-        boolean maxAgeSet = false;
-        boolean expiresSet = false;
-
-        for (int j = 1; j < fields.length; j++) {
-            if ("secure".equalsIgnoreCase(fields[j])) {
-                secure = true;
-            } else if (fields[j].indexOf('=') > 0) {
-                String[] f = fields[j].split("=");
-                if (f.length == 1) continue; // Add protection against null field values
-
-                // favor 'max-age' field over 'expires'
-                if (!maxAgeSet && "max-age".equalsIgnoreCase(f[0])) {
-                    try {
-                        maxAge = Math.max(Integer.valueOf(removeQuote(f[1])), 0);
-                    } catch (NumberFormatException e1) {
-                        // ignore failure to parse -> treat as session cookie
-                        // invalidate a previously parsed expires-field
-                        maxAge = -1;
-                    }
-                    maxAgeSet = true;
-                } else if (!maxAgeSet && !expiresSet && "expires".equalsIgnoreCase(f[0])) {
-                    try {
-                        maxAge = Math.max(convertExpireField(f[1]), 0);
-                    } catch (Exception e) {
-                        // original behavior, is this correct at all (expires field with max-age semantics)?
-                        try {
-                            maxAge = Math.max(Integer.valueOf(f[1]), 0);
-                        } catch (NumberFormatException e1) {
-                            // ignore failure to parse -> treat as session cookie
-                        }
-                    }
-                    expiresSet = true;
-                } else if ("domain".equalsIgnoreCase(f[0])) {
-                    domain = f[1];
-                } else if ("path".equalsIgnoreCase(f[0])) {
-                    path = f[1];
-                }
-            }
-        }
-
-        return new Cookie(domain, cookieName, cookieValue, path, maxAge, secure);
-    }
-
-    public static int convertExpireField(String timestring) throws Exception {
+    public static int convertExpireField(String timestring) {
         String trimmedTimeString = removeQuote(timestring.trim());
 
         for (SimpleDateFormat sdf : simpleDateFormat.get()) {
             Date date = sdf.parse(trimmedTimeString, new ParsePosition(0));
             if (date != null) {
                 long now = System.currentTimeMillis();
-                long expire = date.getTime();
-                return (int) ((expire - now) / 1000);
+                long maxAgeMillis = date.getTime() - now;
+                return (int) (maxAgeMillis / 1000) + (maxAgeMillis % 1000 != 0 ? 1 : 0);
             }
         }