Only set the charset when the body is set for certain mime types (#1798)

Author: brianquinlan

pkgs/cronet_http/example/pubspec.yaml

@@ -29,3 +29,9 @@ dev_dependencies:
 
 flutter:
   uses-material-design: true
+
+# Use the path version of `package:http` so that the behavior matches the
+# expectations in http_client_conformance_tests.
+dependency_overrides:
+  http:
+    path: ../../http/

pkgs/cupertino_http/example/pubspec.yaml

@@ -38,3 +38,9 @@ dev_dependencies:
 
 flutter:
   uses-material-design: true
+
+# Use the path version of `package:http` so that the behavior matches the
+# expectations in http_client_conformance_tests.
+dependency_overrides:
+  http:
+    path: ../../http/

pkgs/http/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 1.6.0-wip
+
+* **Breaking** Change the behavior of `Request.body` so that a charset
+  parameter is only added for text and XML media types. This brings the
+  behavior of `package:http` in line with RFC-8259.
+
 ## 1.5.0
 
 * Fixed a bug in `IOClient` where the `HttpClient`'s response stream was

pkgs/http/lib/src/request.dart

@@ -14,6 +14,22 @@ import 'utils.dart';
 
 /// An HTTP request where the entire request body is known in advance.
 class Request extends BaseRequest {
+  /// Whether the given MIME type should have a 'charset' parameter.
+  static bool _shouldHaveCharset(MediaType? contentType) =>
+      contentType != null &&
+      // RFC 8259, section 9 says that "charset" is not defined for JSON.
+      // Some uncommon non-text, non-xml types do specify charset
+      // (e.g. application/news-checkgroups) but the user will have to set the
+      // charset themselves for those types.
+      (contentType.type == 'text' ||
+          // XML media types defined by RFC 7303.
+          // Note that some media types (e.g. cda+xml) specify that the
+          // charset, when present, must be utf-8. We do not enforce this.
+          contentType.mimeType == 'application/xml' ||
+          contentType.mimeType == 'application/xml-external-parsed-entity' ||
+          contentType.mimeType == 'application/xml-dtd' ||
+          contentType.mimeType.endsWith('+xml'));
+
   /// The size of the request body, in bytes. This is calculated from
   /// [bodyBytes].
   ///
@@ -61,7 +77,9 @@ class Request extends BaseRequest {
     _checkFinalized();
     _defaultEncoding = value;
     var contentType = _contentType;
-    if (contentType == null) return;
+    if (contentType == null || !contentType.parameters.containsKey('charset')) {
+      return;
+    }
     _contentType = contentType.change(parameters: {'charset': value.name});
   }
 
@@ -93,20 +111,27 @@ class Request extends BaseRequest {
   /// This is converted to and from [bodyBytes] using [encoding].
   ///
   /// When this is set, if the request does not yet have a `Content-Type`
-  /// header, one will be added with the type `text/plain`. Then the `charset`
-  /// parameter of the `Content-Type` header (whether new or pre-existing) will
-  /// be set to [encoding] if it wasn't already set.
+  /// header, one will be added with the type `text/plain` and appropriate
+  /// `charset` parameter.
+  ///
+  /// If request has `Content-Type` header with MIME media type name `text` or
+  /// is an XML MIME type (e.g. `application/xml` or `image/svg+xml`) without
+  /// `charset` parameter, then the `charset` parameter will be set to
+  /// [encoding].
   ///
-  /// To set the body of the request, without setting the `Content-Type` header,
-  /// use [bodyBytes].
+  /// To set the body of the request, without changing the `Content-Type`
+  /// header, use [bodyBytes].
   String get body => encoding.decode(bodyBytes);
 
   set body(String value) {
+    // IANA defines known media types here:
+    // https://www.iana.org/assignments/media-types/media-types.xhtml
     bodyBytes = encoding.encode(value);
     var contentType = _contentType;
     if (contentType == null) {
       _contentType = MediaType('text', 'plain', {'charset': encoding.name});
-    } else if (!contentType.parameters.containsKey('charset')) {
+    } else if (_shouldHaveCharset(_contentType) &&
+        !contentType.parameters.containsKey('charset')) {
       _contentType = contentType.change(parameters: {'charset': encoding.name});
     }
   }

pkgs/http/pubspec.yaml

@@ -1,5 +1,5 @@
 name: http
-version: 1.5.0
+version: 1.6.0-wip
 description: A composable, multi-platform, Future-based API for HTTP requests.
 repository: https://github.com/dart-lang/http/tree/master/pkgs/http
 

pkgs/http/test/io/http_test.dart

@@ -158,9 +158,7 @@ void main() {
             'method': 'POST',
             'path': '/',
             'headers': {
-              'content-type': [
-                'application/x-www-form-urlencoded; charset=utf-8'
-              ],
+              'content-type': ['application/x-www-form-urlencoded'],
               'content-length': ['40'],
               'accept-encoding': ['gzip'],
               'user-agent': ['Dart'],
@@ -273,9 +271,7 @@ void main() {
             'method': 'PUT',
             'path': '/',
             'headers': {
-              'content-type': [
-                'application/x-www-form-urlencoded; charset=utf-8'
-              ],
+              'content-type': ['application/x-www-form-urlencoded'],
               'content-length': ['40'],
               'accept-encoding': ['gzip'],
               'user-agent': ['Dart'],
@@ -388,9 +384,7 @@ void main() {
             'method': 'PATCH',
             'path': '/',
             'headers': {
-              'content-type': [
-                'application/x-www-form-urlencoded; charset=utf-8'
-              ],
+              'content-type': ['application/x-www-form-urlencoded'],
               'content-length': ['40'],
               'accept-encoding': ['gzip'],
               'user-agent': ['Dart'],

pkgs/http/test/request_test.dart

@@ -189,23 +189,12 @@ void main() {
       expect(request.headers, containsPair('content-type', 'application/json'));
     });
 
-    test(
-        'is set to application/x-www-form-urlencoded with charset utf-8 if '
-        'bodyFields is set', () {
-      var request = http.Request('POST', dummyUrl)
-        ..bodyFields = {'hello': 'world'};
-      expect(request.headers['Content-Type'],
-          equals('application/x-www-form-urlencoded; charset=utf-8'));
-    });
-
-    test(
-        'is set to application/x-www-form-urlencoded with the given charset '
-        'if bodyFields and encoding are set', () {
+    test('is set to application/x-www-form-urlencoded if bodyFields is set',
+        () {
       var request = http.Request('POST', dummyUrl)
-        ..encoding = latin1
         ..bodyFields = {'hello': 'world'};
       expect(request.headers['Content-Type'],
-          equals('application/x-www-form-urlencoded; charset=iso-8859-1'));
+          equals('application/x-www-form-urlencoded'));
     });
 
     test(
@@ -218,20 +207,66 @@ void main() {
           equals('text/plain; charset=iso-8859-1'));
     });
 
-    test('is modified to include utf-8 if body is set', () {
+    test('is modified to include utf-8 if body is set and mime type is text',
+        () {
+      var request = http.Request('POST', dummyUrl);
+      request.headers['Content-Type'] = 'text/plain';
+      request.body = 'Hello World!';
+      expect(
+          request.headers['Content-Type'], equals('text/plain; charset=utf-8'));
+    });
+
+    test('is modified to include utf-8 if body is set and mime type is xml',
+        () {
+      var request = http.Request('POST', dummyUrl);
+      request.headers['Content-Type'] = 'application/xml';
+      request.body = '<?xml...';
+      expect(request.headers['Content-Type'],
+          equals('application/xml; charset=utf-8'));
+    });
+
+    test(
+        'is not modified to include utf-8 if body is set and mime type is json',
+        () {
       var request = http.Request('POST', dummyUrl);
       request.headers['Content-Type'] = 'application/json';
       request.body = '{"hello": "world"}';
+      expect(request.headers['Content-Type'], equals('application/json'));
+    });
+
+    test(
+        'is modified to include the given encoding if encoding is set and '
+        'mime type is text', () {
+      var request = http.Request('POST', dummyUrl);
+      request.headers['Content-Type'] = 'text/plain';
+      request
+        ..body = 'Hello World!'
+        ..encoding = latin1;
+      expect(request.headers['Content-Type'],
+          equals('text/plain; charset=iso-8859-1'));
+    });
+
+    test(
+        'is modified to include the given encoding if encoding is set and '
+        'mime type is xml', () {
+      var request = http.Request('POST', dummyUrl);
+      request.headers['Content-Type'] = 'application/xml';
+      request
+        ..body = '<?xml...'
+        ..encoding = latin1;
       expect(request.headers['Content-Type'],
-          equals('application/json; charset=utf-8'));
+          equals('application/xml; charset=iso-8859-1'));
     });
 
-    test('is modified to include the given encoding if encoding is set', () {
+    test(
+        'is not modified to include the given encoding if encoding is set mime '
+        'type is json', () {
       var request = http.Request('POST', dummyUrl);
       request.headers['Content-Type'] = 'application/json';
-      request.encoding = latin1;
-      expect(request.headers['Content-Type'],
-          equals('application/json; charset=iso-8859-1'));
+      request
+        ..body = '{"hello": "world"}'
+        ..encoding = latin1;
+      expect(request.headers['Content-Type'], equals('application/json'));
     });
 
     test('has its charset overridden by an explicit encoding', () {

pkgs/http/test/stub_server.dart

@@ -65,14 +65,20 @@ void hybridMain(StreamChannel<dynamic> channel) async {
     dynamic requestBody;
     if (requestBodyBytes.isEmpty) {
       requestBody = null;
-    } else if (request.headers.contentType?.charset != null) {
-      var encoding =
-          requiredEncodingForCharset(request.headers.contentType!.charset!);
-      requestBody = encoding.decode(requestBodyBytes);
     } else {
-      requestBody = requestBodyBytes;
+      requestBody = switch ((
+        request.headers.contentType?.mimeType,
+        request.headers.contentType?.charset
+      )) {
+        (_, var charset?) =>
+          requiredEncodingForCharset(charset).decode(requestBodyBytes),
+        // This is not a complete set of mime types that default to utf-8,
+        // just the ones found in the tests.
+        ('application/json' || 'application/x-www-form-urlencoded', null) =>
+          utf8.decode(requestBodyBytes),
+        _ => requestBodyBytes,
+      };
     }
-
     final headers = <String, List<String>>{};
 
     request.headers.forEach((name, values) {

pkgs/http_client_conformance_tests/lib/src/request_body_tests.dart

@@ -78,8 +78,7 @@ void testRequestBody(Client client) {
       final serverReceivedContentType = await httpServerQueue.next;
       final serverReceivedBody = await httpServerQueue.next;
 
-      expect(serverReceivedContentType,
-          ['application/x-www-form-urlencoded; charset=utf-8']);
+      expect(serverReceivedContentType, ['application/x-www-form-urlencoded']);
       expect(serverReceivedBody, 'key=value');
     });
 
@@ -90,8 +89,7 @@ void testRequestBody(Client client) {
       final serverReceivedContentType = await httpServerQueue.next;
       final serverReceivedBody = await httpServerQueue.next;
 
-      expect(serverReceivedContentType,
-          ['application/x-www-form-urlencoded; charset=plus2']);
+      expect(serverReceivedContentType, ['application/x-www-form-urlencoded']);
       expect(serverReceivedBody, 'gau;r]hqa'); // key=value
     });
 
@@ -149,7 +147,7 @@ void testRequestBody(Client client) {
       final serverReceivedContentType = await httpServerQueue.next;
       final serverReceivedBody = await httpServerQueue.next as String;
 
-      expect(serverReceivedContentType, ['image/png; charset=plus2']);
+      expect(serverReceivedContentType, ['image/png']);
       expect(serverReceivedBody.codeUnits, [1, 2, 3, 4, 5]);
     });
 

pkgs/http_client_conformance_tests/pubspec.yaml

@@ -15,7 +15,6 @@ dependencies:
   stream_channel: ^2.1.1
   test: ^1.21.2
 
-# TODO(brianquinlan): Remove dependency_overrides when package:http 1.5.0 is released.
 dependency_overrides:
   http:
     path: ../http