Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

need more leniency in handling invalid Response cookies (avoid FormatException) #42902

Open
2x2xplz opened this issue Jul 31, 2020 · 6 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io

Comments

@2x2xplz
Copy link

2x2xplz commented Jul 31, 2020

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 &#20;) 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:
cfug/dio#785
cfug/dio#412
cfug/dio#54
#37166
#33327
#33765

Thank you very much. 🎯 😊

@lrhn lrhn added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io labels Jul 31, 2020
@2x2xplz
Copy link
Author

2x2xplz commented Aug 1, 2020

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 dart-archive/http_multi_server#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
Copy link
Author

2x2xplz commented Aug 3, 2020

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).

@cutecutecat
Copy link

Thanks for @2x2xplz, I have worked around this distressing problem.
For that answer, I would like to provide a revised version for newer Dio and Dart, with some null check and arguments.

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

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

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

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

Hoping all of you watching at this have a nice day!(๑•̀ㅂ•́)و✧

@Theunodb
Copy link

Theunodb commented Jun 14, 2022

This is the complete code based on the answers above:

import 'dart:io';

import 'package:cookie_jar/cookie_jar.dart';
import 'package:dio/dio.dart';
import 'package:dio_cookie_manager/dio_cookie_manager.dart';

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

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

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

  _saveCookies(Response? response) {
    if (response != null) {
      List<String>? cookies = response.headers[HttpHeaders.setCookieHeader];
      if (cookies != null) {
        cookieJar.saveFromResponse(
          response.requestOptions.uri,
          cookies.map((str) => MyCookie.fromSetCookieValue(str)).toList(),
        );
      }
    }
  }
}

class MyCookie implements Cookie {
  String _name;
  String _value;
  DateTime? expires;
  int? maxAge;
  String? domain;
  String? _path;
  bool httpOnly = false;
  bool secure = false;

  MyCookie(String name, String value)
      : _name = _validateName(name),
        _value = _validateValue(value),
        httpOnly = true;

  String get name => _name;

  String get value => _value;

  String? get path => _path;

  set path(String? newPath) {
    _validatePath(newPath);
    _path = newPath;
  }

  set name(String newName) {
    _validateName(newName);
    _name = newName;
  }

  set value(String newValue) {
    _validateValue(newValue);
    _value = newValue;
  }

  MyCookie.fromSetCookieValue(String value)
      : _name = "",
        _value = "" {
    _parseSetCookieValue(value);
  }

  // Parse a 'set-cookie' header value according to the rules in RFC 6265.
  void _parseSetCookieValue(String s) {
    int index = 0;

    bool done() => index == s.length;

    String parseName() {
      int start = index;
      while (!done()) {
        if (s[index] == "=") break;
        index++;
      }
      return s.substring(start, index).trim();
    }

    String parseValue() {
      int start = index;
      while (!done()) {
        if (s[index] == ";") break;
        index++;
      }
      return s.substring(start, index).trim();
    }

    void parseAttributes() {
      String parseAttributeName() {
        int start = index;
        while (!done()) {
          if (s[index] == "=" || s[index] == ";") break;
          index++;
        }
        return s.substring(start, index).trim().toLowerCase();
      }

      String parseAttributeValue() {
        int start = index;
        while (!done()) {
          if (s[index] == ";") break;
          index++;
        }
        return s.substring(start, index).trim().toLowerCase();
      }

      while (!done()) {
        String name = parseAttributeName();
        String value = "";
        if (!done() && s[index] == "=") {
          index++; // Skip the = character.
          value = parseAttributeValue();
        }
        if (name == "expires") {
          expires = _parseCookieDate(value);
        } else if (name == "max-age") {
          maxAge = int.parse(value);
        } else if (name == "domain") {
          domain = value;
        } else if (name == "path") {
          path = value;
        } else if (name == "httponly") {
          httpOnly = true;
        } else if (name == "secure") {
          secure = true;
        }
        if (!done()) index++; // Skip the ; character
      }
    }

    _name = _validateName(parseName());
    if (done() || _name.isEmpty) {
      throw HttpException("Failed to parse header value [$s]");
    }
    index++; // Skip the = character.
    _value = _validateValue(parseValue());
    if (done()) return;
    index++; // Skip the ; character.
    parseAttributes();
  }

  String toString() {
    StringBuffer sb = StringBuffer();
    sb
      ..write(_name)
      ..write("=")
      ..write(_value);
    var expires = this.expires;
    if (expires != null) {
      sb
        ..write("; Expires=")
        ..write(HttpDate.format(expires));
    }
    if (maxAge != null) {
      sb
        ..write("; Max-Age=")
        ..write(maxAge);
    }
    if (domain != null) {
      sb
        ..write("; Domain=")
        ..write(domain);
    }
    if (path != null) {
      sb
        ..write("; Path=")
        ..write(path);
    }
    if (secure) sb.write("; Secure");
    if (httpOnly) sb.write("; HttpOnly");
    return sb.toString();
  }

  static String _validateName(String newName) {
    const separators = ["(", ")", "<", ">", "@", ",", ";", ":", "\\", '"', "/", "[", "]", "?", "=", "{", "}"];
    if (newName == null) throw ArgumentError.notNull("name");
    for (int i = 0; i < newName.length; i++) {
      int codeUnit = newName.codeUnitAt(i);
      if (codeUnit <= 32 || codeUnit >= 127 || separators.contains(newName[i])) {
        throw FormatException("Invalid character in cookie name, code unit: '$codeUnit'", newName, i);
      }
    }
    return newName;
  }

  static String _validateValue(String newValue) {
    if (newValue == null) throw 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 = 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 = newValue.codeUnits[i];
      if (!(codeUnit == 0x21 || codeUnit == 0x20 || (codeUnit >= 0x23 && codeUnit <= 0x2B) || (codeUnit >= 0x2D && codeUnit <= 0x3A) || (codeUnit >= 0x3C && codeUnit <= 0x5B) || (codeUnit >= 0x5D && codeUnit <= 0x7E))) {
        throw FormatException("Invalid character in cookie value, code unit: '$codeUnit'", newValue, i);
      }
    }
    return newValue;
  }

  static void _validatePath(String? path) {
    if (path == null) return;
    for (int i = 0; i < path.length; i++) {
      int codeUnit = path.codeUnitAt(i);
      // According to RFC 6265, semicolon and controls should not occur in the
      // path.
      // path-value = <any CHAR except CTLs or ";">
      // CTLs = %x00-1F / %x7F
      if (codeUnit < 0x20 || codeUnit >= 0x7f || codeUnit == 0x3b /*;*/) {
        throw FormatException("Invalid character in cookie path, code unit: '$codeUnit'");
      }
    }
  }

  static DateTime _parseCookieDate(String date) {
    const List monthsLowerCase = ["jan", "feb", "mar", "apr", "may", "jun", "jul", "aug", "sep", "oct", "nov", "dec"];

    int position = 0;

    Never error() {
      throw HttpException("Invalid cookie date $date");
    }

    bool isEnd() => position == date.length;

    bool isDelimiter(String s) {
      int char = s.codeUnitAt(0);
      if (char == 0x09) return true;
      if (char >= 0x20 && char <= 0x2F) return true;
      if (char >= 0x3B && char <= 0x40) return true;
      if (char >= 0x5B && char <= 0x60) return true;
      if (char >= 0x7B && char <= 0x7E) return true;
      return false;
    }

    bool isNonDelimiter(String s) {
      int char = s.codeUnitAt(0);
      if (char >= 0x00 && char <= 0x08) return true;
      if (char >= 0x0A && char <= 0x1F) return true;
      if (char >= 0x30 && char <= 0x39) return true; // Digit
      if (char == 0x3A) return true; // ':'
      if (char >= 0x41 && char <= 0x5A) return true; // Alpha
      if (char >= 0x61 && char <= 0x7A) return true; // Alpha
      if (char >= 0x7F && char <= 0xFF) return true; // Alpha
      return false;
    }

    bool isDigit(String s) {
      int char = s.codeUnitAt(0);
      if (char > 0x2F && char < 0x3A) return true;
      return false;
    }

    int getMonth(String month) {
      if (month.length < 3) return -1;
      return monthsLowerCase.indexOf(month.substring(0, 3));
    }

    int toInt(String s) {
      int index = 0;
      for (; index < s.length && isDigit(s[index]); index++);
      return int.parse(s.substring(0, index));
    }

    var tokens = <String>[];
    while (!isEnd()) {
      while (!isEnd() && isDelimiter(date[position])) position++;
      int start = position;
      while (!isEnd() && isNonDelimiter(date[position])) position++;
      tokens.add(date.substring(start, position).toLowerCase());
      while (!isEnd() && isDelimiter(date[position])) position++;
    }

    String? timeStr;
    String? dayOfMonthStr;
    String? monthStr;
    String? yearStr;

    for (var token in tokens) {
      if (token.isEmpty) continue;
      if (timeStr == null && token.length >= 5 && isDigit(token[0]) && (token[1] == ":" || (isDigit(token[1]) && token[2] == ":"))) {
        timeStr = token;
      } else if (dayOfMonthStr == null && isDigit(token[0])) {
        dayOfMonthStr = token;
      } else if (monthStr == null && getMonth(token) >= 0) {
        monthStr = token;
      } else if (yearStr == null && token.length >= 2 && isDigit(token[0]) && isDigit(token[1])) {
        yearStr = token;
      }
    }

    if (timeStr == null || dayOfMonthStr == null || monthStr == null || yearStr == null) {
      error();
    }

    int year = toInt(yearStr);
    if (year >= 70 && year <= 99)
      year += 1900;
    else if (year >= 0 && year <= 69) year += 2000;
    if (year < 1601) error();

    int dayOfMonth = toInt(dayOfMonthStr);
    if (dayOfMonth < 1 || dayOfMonth > 31) error();

    int month = getMonth(monthStr) + 1;

    var timeList = timeStr.split(":");
    if (timeList.length != 3) error();
    int hour = toInt(timeList[0]);
    int minute = toInt(timeList[1]);
    int second = toInt(timeList[2]);
    if (hour > 23) error();
    if (minute > 59) error();
    if (second > 59) error();

    return DateTime.utc(year, month, dayOfMonth, hour, minute, second, 0);
  }
}

@RicardoZap
Copy link

I have a same problem with Dio and Cookie manager, I implemented the final code and it does not give me any response, any solution?

@effusiveperiscope
Copy link

Still encountering this error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io
Projects
None yet
Development

No branches or pull requests

6 participants