Sdk: need more leniency in handling invalid Response cookies (avoid FormatException)

Created on 31 Jul 2020 · 2Comments · Source: dart-lang/sdk

Dart 2.8.4 (Flutter 1.17.5) throws a `FormatException` when the cookie value in `Set-Cookie` header contains a space.

The following is from a Flutter app, but the underlying code throwing the error is in the Dart SDK.

I/flutter (24821): FormatException: Invalid character in cookie value, code unit: '32' (at character 16)
I/flutter (24821): cookievaluewith spaceinit
I/flutter (24821):                ^

https://github.com/dart-lang/sdk/blob/master/sdk/lib/_http/http_headers.dart

    for (int i = start; i < end; 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'",
            newValue,
            i);
      }
    }

Spaces in cookie values (or names) do violate RFC 6265. No argument there.

However most clients comply with RFC 2965 instead (or even 2109), which is more lenient. Then they add additional leniency. As a Client, we sometimes have to deal with malformed responses, therefore leniency is good. Chrome and Firefox don't crash, they just accept the cookie with spaces (or other illegal characters) and deal with it. But with Dart, the error is thrown so low in the stack that any upstream Dart/Flutter library (Dio, http) can't catch the error, at least not with access to the rest of the Response.

Http libraries in other languages are more accommodating with the characters they accept, and also by avoiding throwing an error if possible, in favor of returning NULL or something else:

OKHttp (Java):

      val cookieValue = setCookie.trimSubstring(pairEqualsSign + 1, cookiePairEnd)
      if (cookieValue.indexOfControlOrNonAscii() != -1) return null

and indexOfControlOrNonAscii contains:

      indexOfControlOrNonAscii:  if (c <= '\u001f' || c >= '\u007f')

(space is char \u0020 or 0x20, thus not rejected by okhttp)
source-1
source-2

urllib (Python) as of April 2020:

# These quoting routines conform to the RFC2109 specification, which in
# turn references the character definitions from RFC2068. They provide
# a two-way quoting algorithm. Any non-text character is translated
# into a 4 character sequence: a forward-slash followed by the
# three-digit octal equivalent of the character. Any '\' or '"' is
# quoted with a preceding '\' slash.
# Because of the way browsers really handle cookies (as opposed to what
# the RFC says) we also encode "," and ";".
#
# These are taken from RFC2068 and RFC2109.
# _LegalChars is the list of chars which don't require "'s
# _Translator hash-table for fast quoting
#

_LegalChars = string.ascii_letters + string.digits + "!#$%&'*+-.^_`|~:"
_UnescapedChars = _LegalChars + ' ()/<=>?@[]{}'

source

Potential resolutions:

Wrap cookie values with spaces (and some other illegal characters) in double-quotes. (what Python does)
HTML encode (%20 or ) spaces inside cookie values
edit the line if (!(codeUnit == 0x21 || to if (!(codeUnit == 0x21 || codeUnit == 0x20 ||
extract illegal characters from the cookie value and return the rest, i.e. a;b c => abc
Revert back to an earlier spec or clone the ruleset from another popular library/language
Pass along untouched so a client library can deal with it along with access to the full response. Maybe log a warning or non-fatal error
Other suggestions would be welcome. I think we all agree that if every web server would just follow the latest specs, everyone's lives could be made easier... unfortunately 20+ year old codebases are unlikely to change soon, since they work in popular browsers.

Similar issues have been raised before, and either partially fixed or not fixed:
https://github.com/flutterchina/dio/issues/785
https://github.com/flutterchina/dio/issues/412
https://github.com/flutterchina/dio/issues/54

37166

33327

33765

Thank you very much. 🎯 😊

area-library library-io

Source

2x2xplz

👍2

Most helpful comment

To follow up on this and offer a temporary workaround:

By default, validating cookies in Dart is handled by http_headers.dart, the implementation is the _Cookie class. The class is defined by the abstract Cookie class in http.dart. Because _Cookie is private, it cannot be extended except within its own module -- and of course, if you do extend or re-implement, any code that relies on default Cookie needs to be updated to point to your custom extension or implementation.

Side note: Some packages facilitate replacing parts of the default stack with a custom implementation. Dio, for example, makes it easy to use your own HttpClient simply by swapping out the DefaultHttpClientAdapter with your own. (the hard part, of course, is creating a custom HttpClient. Huge credit to @shaxxx who did exactly that with the "alt_http" module, without his example I could not have figured out how all of this work and create a solution)

In the case of cookies, at least with Dio, it is not the HttpClient that validates the cookies, but the cookie manager. Seeing that this is a single small file, creating an alternative implementation will be far easier than creating (and maintaining) a new HttpClient. Specifically, pointing the method Cookie.fromSetCookieValue to a custom implementation of Cookie will allow us to change how cookies are validated.

CookieManager class can be extended, rather than entirely re-implemented:

class MyCookieManager extends CookieManager {
  MyCookieManager(CookieJar cookieJar) : super(cookieJar);

  @override
  Future onResponse(Response response) async => _saveCookies(response);

  @override
  Future onError(DioError err) async => _saveCookies(err.response);

  _saveCookies(Response response) {
    if (response != null && response.headers != null) {
      List<String> cookies = response.headers[HttpHeaders.setCookieHeader];
      if (cookies != null) {
        cookieJar.saveFromResponse(
          response.request.uri,
          // we're changing this ↙ line only... to use a custom class for cookie parsing
          cookies.map((str) => MyCookie.fromSetCookieValue(str)).toList(),
        );
      }
    }
  }
}

And for MyCookie, as noted earlier it can't be extended, we must re-implement. Fortunately we can just copy the code of _Cookie from http_headers, with the changes you want to implement:

class MyCookie implements Cookie {
  ... copied code ...

  // make your changes wherever appropriate in the class
  if (!(codeUnit == 0x21 || codeUnit == 0x20 || // [allows spaces in cookie values]

  ... copied code ...
}

And because that copied code relies on a date parser from http_date.dart, we need to implement that too:

DateTime _parseCookieDate(String date) {
  ... copied code ...  // suggest not making any changes
}

You can do this all in a single file, or use part and part of to combine 3 physical files into one logical file to Dart.

The last step is to change your Dio interceptor from the default to our custom version:

Dio()..interceptors.add(MyCookieManager(cookieJar))

Hopefully this will help anyone else finding themselves communicating with a non-compliant web server. I stated earlier that @shaxxx's example were hugely helpful, perhaps someone will find this post useful as well. This example is based on Dio, but it should be possible to adapt to any package that leverages the built-in Dart SDK HttpClient or associated classes (like Cookie).

2x2xplz on 3 Aug 2020

👍4

All 2 comments

Dart has attempted to build a library that adheres to recent IETF specs.

@sortie wrote in 2019:

Dart implements a strict version of RFC 6265 (HTTP State Management Mechanism) which standardizes the cookie format.

Unfortunately, some web servers out in the wild aren't fully compliant (or compliant to an older spec), which leads to unwanted and unnecessary failures in real-world use. To their credit, the Dart devs have recognized the need to "bend the rules" on occasion to improve real-world compatibility. The most notable example is with case-sensitive header names:

Header fields of HttpHeaders are case-insensitive according to specification. Implementation class _HttpHeaders will convert all header fields into lowercases by default. This is expected behavior. However, some servers do rely on cased header fields.

From RFC-2616 (1999):

Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive.

And case-insensitivity is confirmed in RFC-7230:

Each header field consists of a case-insensitive field name

However, despite the clarity of the specs, the Dart developer community allowed for preservation of header casing.

[Breaking Change Request] HttpHeaders allows cased header fields #39657

Update packages with HttpHeaders' preserveHeaderCase change https://github.com/dart-lang/http_multi_server/pull/21

I believe that invalid cookie values, which are also out of the client's control, deserve similar treatment. Perhaps a strictCookieRules flag (on by default) would, just like preserveHeaderValues, allow the developer more control when communicating with a non-compliant server. Thank you for your consideration.

2x2xplz on 1 Aug 2020

To follow up on this and offer a temporary workaround:

CookieManager class can be extended, rather than entirely re-implemented:

class MyCookieManager extends CookieManager {
  MyCookieManager(CookieJar cookieJar) : super(cookieJar);

  @override
  Future onResponse(Response response) async => _saveCookies(response);

  @override
  Future onError(DioError err) async => _saveCookies(err.response);

  _saveCookies(Response response) {
    if (response != null && response.headers != null) {
      List<String> cookies = response.headers[HttpHeaders.setCookieHeader];
      if (cookies != null) {
        cookieJar.saveFromResponse(
          response.request.uri,
          // we're changing this ↙ line only... to use a custom class for cookie parsing
          cookies.map((str) => MyCookie.fromSetCookieValue(str)).toList(),
        );
      }
    }
  }
}

And for MyCookie, as noted earlier it can't be extended, we must re-implement. Fortunately we can just copy the code of _Cookie from http_headers, with the changes you want to implement:

class MyCookie implements Cookie {
  ... copied code ...

  // make your changes wherever appropriate in the class
  if (!(codeUnit == 0x21 || codeUnit == 0x20 || // [allows spaces in cookie values]

  ... copied code ...
}

And because that copied code relies on a date parser from http_date.dart, we need to implement that too:

DateTime _parseCookieDate(String date) {
  ... copied code ...  // suggest not making any changes
}

You can do this all in a single file, or use part and part of to combine 3 physical files into one logical file to Dart.

The last step is to change your Dio interceptor from the default to our custom version:

Dio()..interceptors.add(MyCookieManager(cookieJar))

2x2xplz on 3 Aug 2020

👍4

Was this page helpful?

0 / 5 - 0 ratings

Related issues

Stream.asBroadcastStream - Easy to cause leaks, what is the rationale?

matanlurey · 3Comments

Failed to create server socket (OS Error: An attempt was made to access a socket in a way forbidden by its access permissions. , errno = 10013), address = 127.0.0.1, port = 8080

DartBot · 3Comments

Tree-shaking and inlining

ranquild · 3Comments

"The getter 'name' was called on null" in analysis server.

gspencergoog · 3Comments

String.fromEnvironment() and environment variables

sgrekhov · 3Comments