[dart:_http] Require Cookie name and value to be valid.

This is a breaking change. #37192

This change makes the name and value positional optional parameters in the
Cookie class constructor mandatory by changing the signature from

  Cookie([String name, String value])

to

  Cookie(String name, String value)

The parameters were already effectively mandatory as a bug introduced in
Dart 1.3.0 (2014) meant the name and value parameters could not be null, and
any such uses already threw a noSuchMethod exception because null did not
have a length getter. As such, this is not a breaking change but adopts the
current behavior as a null name and value was already of questionable use.

Breaking change: This change adds validation to the String name and String
value setters, which had not been validating the fields at all, unlike the
constructor. This also forbids the name and value from being set to null.
That meant potentially invalid cookies could be sent to servers if the
cookie was modified after construction. This change adds the validation to
follow the rule of least surprise.

The documentation has been updated accordingly and improved a bit.

Closes #37192
Closes #29463

Change-Id: Iffed3dc265ca9c68142c4372522913f9d1ff4d51
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/103840
Commit-Queue: Jonas Termansen <sortie@google.com>
Reviewed-by: Lasse R.H. Nielsen <lrn@google.com>

Author: sortie

CHANGELOG.md

@@ -50,6 +50,32 @@
 
   [29456]: https://github.com/dart-lang/sdk/issues/29456
 
+#### `dart:io`
+
+* **Breaking Change:** The `Cookie` class's constructor's `name` and `value`
+  optional positional parameters are now mandatory (Issue [37192][]). The
+  signature changes from:
+
+      Cookie([String name, String value])
+
+  to
+
+      Cookie(String name, String value)
+
+  However, it has not been possible to set `name` and `value` to null since Dart
+  1.3.0 (2014) where a bug made it impossible. Any code not using both
+  parameters or setting any to null would necessarily get a noSuchMethod
+  exception at runtime. This change catches such erroneous uses at compile time.
+  Since code could not previously correctly omit the parameters, this is not
+  really a breaking change.
+
+* **Breaking Change:** The `Cookie` class's `name` and `value` setters now
+  validates that the strings are made from the allowed character set and are not
+  null (Issue [37192][]). The constructor already made these checks and this
+  fixes the loophole where the setters didn't also validate.
+
+[37192]: https://github.com/dart-lang/sdk/issues/37192
+
 ### Dart VM
 
 ### Tools

sdk/lib/_http/http.dart

@@ -912,60 +912,78 @@ abstract class ContentType implements HeaderValue {
 }
 
 /**
- * Representation of a cookie. For cookies received by the server as
- * Cookie header values only [:name:] and [:value:] fields will be
- * set. When building a cookie for the 'set-cookie' header in the server
- * and when receiving cookies in the client as 'set-cookie' headers all
- * fields can be used.
+ * Representation of a cookie. For cookies received by the server as Cookie
+ * header values only [name] and [value] properties will be set. When building a
+ * cookie for the 'set-cookie' header in the server and when receiving cookies
+ * in the client as 'set-cookie' headers all fields can be used.
  */
 abstract class Cookie {
   /**
-   * Gets and sets the name.
+   * The name of the cookie.
+   *
+   * Must be a `token` as specified in RFC 6265.
+   *
+   * The allowed characters in a `token` are the visible ASCII characters,
+   * U+0021 (`!`) through U+007E (`~`), except the separator characters:
+   * `(`, `)`, `<`, `>`, `@`, `,`, `;`, `:`, `\`, `"`, `/`, `[`, `]`, `?`, `=`,
+   * `{`, and `}`.
    */
   String name;
 
   /**
-   * Gets and sets the value.
+   * The value of the cookie.
+   *
+   * Must be a `cookie-value` as specified in RFC 6265.
+   *
+   * The allowed characters in a cookie value are the visible ASCII characters,
+   * U+0021 (`!`) through U+007E (`~`) except the characters:
+   * `"`, `,`, `;` and `\`.
+   * Cookie values may be wrapped in a single pair of double quotes
+   * (U+0022, `"`).
    */
   String value;
 
   /**
-   * Gets and sets the expiry date.
+   * The time at which the cookie expires.
    */
   DateTime expires;
 
   /**
-   * Gets and sets the max age. A value of [:0:] means delete cookie
-   * now.
+   * The number of seconds until the cookie expires. A zero or negative value
+   * means the cookie has expired.
    */
   int maxAge;
 
   /**
-   * Gets and sets the domain.
+   * The domain the cookie applies to.
    */
   String domain;
 
   /**
-   * Gets and sets the path.
+   * The path within the [domain] the cookie applies to.
    */
   String path;
 
   /**
-   * Gets and sets whether this cookie is secure.
+   * Whether to only send this cookie on secure connections.
    */
   bool secure;
 
   /**
-   * Gets and sets whether this cookie is HTTP only.
+   * Whether the cookie is only sent in the HTTP request and is not made
+   * available to client side scripts.
    */
   bool httpOnly;
 
   /**
-   * Creates a new cookie optionally setting the name and value.
+   * Creates a new cookie setting the name and value.
+   *
+   * [name] and [value] must be composed of valid characters according to RFC
+   * 6265.
    *
    * By default the value of `httpOnly` will be set to `true`.
    */
-  factory Cookie([String name, String value]) => new _Cookie(name, value);
+  factory Cookie(String name, String value) => new _Cookie(name, value);
 
   /**
    * Creates a new cookie by parsing a header value from a 'set-cookie'

sdk/lib/_http/http_headers.dart

@@ -829,19 +829,31 @@ class _ContentType extends _HeaderValue implements ContentType {
 }
 
 class _Cookie implements Cookie {
-  String name;
-  String value;
+  String _name;
+  String _value;
   DateTime expires;
   int maxAge;
   String domain;
   String path;
   bool httpOnly = false;
   bool secure = false;
 
-  _Cookie([this.name, this.value]) {
-    // Default value of httponly is true.
-    httpOnly = true;
-    _validate();
+  _Cookie(String name, String value)
+      : _name = _validateName(name),
+        _value = _validateValue(value),
+        httpOnly = true;
+
+  String get name => _name;
+  String get value => _value;
+
+  set name(String newName) {
+    _validateName(newName);
+    _name = newName;
+  }
+
+  set value(String newValue) {
+    _validateValue(newValue);
+    _value = newValue;
   }
 
   _Cookie.fromSetCookieValue(String value) {
@@ -924,21 +936,20 @@ class _Cookie implements Cookie {
       }
     }
 
-    name = parseName();
-    if (done() || name.length == 0) {
+    _name = _validateName(parseName());
+    if (done() || _name.length == 0) {
       throw new HttpException("Failed to parse header value [$s]");
     }
     index++; // Skip the = character.
-    value = parseValue();
-    _validate();
+    _value = _validateValue(parseValue());
     if (done()) return;
     index++; // Skip the ; character.
     parseAttributes();
   }
 
   String toString() {
     StringBuffer sb = new StringBuffer();
-    sb..write(name)..write("=")..write(value);
+    sb..write(_name)..write("=")..write(_value);
     if (expires != null) {
       sb..write("; Expires=")..write(HttpDate.format(expires));
     }
@@ -956,7 +967,7 @@ class _Cookie implements Cookie {
     return sb.toString();
   }
 
-  void _validate() {
+  static String _validateName(String newName) {
     const separators = const [
       "(",
       ")",
@@ -976,41 +987,47 @@ class _Cookie implements Cookie {
       "{",
       "}"
     ];
-    for (int i = 0; i < name.length; i++) {
-      int codeUnit = name.codeUnits[i];
+    if (newName == null) throw new ArgumentError.notNull("name");
+    for (int i = 0; i < newName.length; i++) {
+      int codeUnit = newName.codeUnits[i];
       if (codeUnit <= 32 ||
           codeUnit >= 127 ||
-          separators.indexOf(name[i]) >= 0) {
+          separators.indexOf(newName[i]) >= 0) {
         throw new FormatException(
             "Invalid character in cookie name, code unit: '$codeUnit'",
-            name,
+            newName,
             i);
       }
     }
+    return newName;
+  }
 
+  static String _validateValue(String newValue) {
+    if (newValue == null) throw new ArgumentError.notNull("value");
     // Per RFC 6265, consider surrounding "" as part of the value, but otherwise
     // double quotes are not allowed.
     int start = 0;
-    int end = value.length;
-    if (2 <= value.length &&
-        value.codeUnits[start] == 0x22 &&
-        value.codeUnits[end - 1] == 0x22) {
+    int end = newValue.length;
+    if (2 <= newValue.length &&
+        newValue.codeUnits[start] == 0x22 &&
+        newValue.codeUnits[end - 1] == 0x22) {
       start++;
       end--;
     }
 
     for (int i = start; i < end; i++) {
-      int codeUnit = value.codeUnits[i];
+      int codeUnit = newValue.codeUnits[i];
       if (!(codeUnit == 0x21 ||
           (codeUnit >= 0x23 && codeUnit <= 0x2B) ||
           (codeUnit >= 0x2D && codeUnit <= 0x3A) ||
           (codeUnit >= 0x3C && codeUnit <= 0x5B) ||
           (codeUnit >= 0x5D && codeUnit <= 0x7E))) {
         throw new FormatException(
             "Invalid character in cookie value, code unit: '$codeUnit'",
-            value,
+            newValue,
             i);
       }
     }
+    return newValue;
   }
 }