URI-escape cookies (addresses external issue 4365)

[kpr...@google.com]

Reviewers: Dan Rice,

Description:
Cookies weren't being escaped despite uriEscaping being set. This
addresses the problem and adds to the tests.

Please review this at http://gwt-code-reviews.appspot.com/128801

Affected files:
user/src/com/google/gwt/user/client/Cookies.java
user/test/com/google/gwt/user/client/CookieTest.java

Index: user/test/com/google/gwt/user/client/CookieTest.java
===================================================================
--- user/test/com/google/gwt/user/client/CookieTest.java (revision 7348)
+++ user/test/com/google/gwt/user/client/CookieTest.java (working copy)
@@ -151,7 +151,24 @@
Cookies.removeCookie("test1+test1");
cookies = Cookies.getCookieNames();
assertEquals(curCount, cookies.size());
+
+ // Make sure cookie names are URI encoded
+ Cookies.setUriEncode(true);
+ Cookies.setCookie("test1.,/?:@&=+$#", "value1");
+ assertEquals(curCount + 1, Cookies.getCookieNames().size());
+ Cookies.setUriEncode(false);
+ Cookies.removeCookie("test1.,/?:@&=+$#");
+ assertEquals(curCount + 1, Cookies.getCookieNames().size());
+ Cookies.setUriEncode(true);
+ Cookies.removeCookie("test1.,/?:@&=+$#");
+ assertEquals(curCount, Cookies.getCookieNames().size());

+ // Make sure cookie values are URI encoded
+ Cookies.setCookie("testencodedvalue", "value1,/?:@&=+$#");
+ Cookies.setUriEncode(false);
+ String encodedValue = Cookies.getCookie("testencodedvalue");
+
assertTrue(encodedValue.compareTo("value1%2C%2F%3F%3A%40%26%3D%2B%24%23")
== 0);
+
// Make sure unencoded cookies with bogus format are not added
try {
Cookies.setCookie("test1=test1", "value1");
Index: user/src/com/google/gwt/user/client/Cookies.java
===================================================================
--- user/src/com/google/gwt/user/client/Cookies.java (revision 7348)
+++ user/src/com/google/gwt/user/client/Cookies.java (working copy)
@@ -110,9 +110,11 @@
*/
public static void removeCookie(String name) {
if (uriEncoding) {
- uriEncode(name);
+ removeCookieNative(uriEncode(name));
}
- removeCookieNative(name);
+ else {
+ removeCookieNative(name);
+ }
}

/**
@@ -124,9 +126,11 @@
*/
public static void removeCookie(String name, String path) {
if (uriEncoding) {
- uriEncode(name);
+ removeCookieNative(uriEncode(name), path);
}
- removeCookieNative(name, path);
+ else {
+ removeCookieNative(name, path);
+ }
}

/**
@@ -174,8 +178,8 @@
public static void setCookie(String name, String value, Date expires,
String domain, String path, boolean secure) {
if (uriEncoding) {
- uriEncode(name);
- uriEncode(value);
+ name = uriEncode(name);
+ value = uriEncode(value);
} else if (!isValidCookieName(name) || !isValidCookieValue(value)) {
throw new IllegalArgumentException("Illegal cookie format.");
}

[ri...@google.com]

LGTM with minor nits.

http://gwt-code-reviews.appspot.com/128801/diff/1/3
File user/src/com/google/gwt/user/client/Cookies.java (right):

http://gwt-code-reviews.appspot.com/128801/diff/1/3#newcode112
Line 112: if (uriEncoding) {
Prettier and less duplicated code to do:

if (uriEncoding) {
name = uriEncode(name);
}
removeCookieNative(name);

http://gwt-code-reviews.appspot.com/128801/diff/1/3#newcode115
Line 115: else {
} else {

on same line

http://gwt-code-reviews.appspot.com/128801/diff/1/3#newcode128
Line 128: if (uriEncoding) {
Ditto

http://gwt-code-reviews.appspot.com/128801/diff/1/3#newcode184
Line 184: throw new IllegalArgumentException("Illegal cookie format.");
Include name and value in exception message, maybe distinguish between
bad name and bad value to make debugging easier.

http://gwt-code-reviews.appspot.com/128801/diff/1/2
File user/test/com/google/gwt/user/client/CookieTest.java (right):

http://gwt-code-reviews.appspot.com/128801/diff/1/2#newcode166
Line 166: // Make sure cookie values are URI encoded
Probably best to add 'Cookies.setUriEncode(true);' here (redundantly) to
avoid dependency between sections of this method.

http://gwt-code-reviews.appspot.com/128801