Harmonize response header behavior (dart-lang#993)

Author: brianquinlan

pkgs/cronet_http/CHANGELOG.md

@@ -1,6 +1,7 @@
 ## 0.2.2
 
 * Require Dart 3.0
+* Throw `ClientException` when the `'Content-Length'` header is invalid.
 
 ## 0.2.1
 

pkgs/cronet_http/lib/src/cronet_client.dart

@@ -21,6 +21,7 @@ import 'package:http/http.dart';
 import 'messages.dart' as messages;
 
 final _api = messages.HttpApi();
+final _digitRegex = RegExp(r'^\d+$');
 
 final Finalizer<String> _cronetEngineFinalizer = Finalizer(_api.freeEngine);
 
@@ -266,11 +267,20 @@ class CronetClient extends BaseClient {
         .cast<String, List<Object?>>()
         .map((key, value) => MapEntry(key.toLowerCase(), value.join(',')));
 
-    final contentLengthHeader = responseHeaders['content-length'];
+    int? contentLength;
+    switch (responseHeaders['content-length']) {
+      case final contentLengthHeader?
+          when !_digitRegex.hasMatch(contentLengthHeader):
+        throw ClientException(
+          'Invalid content-length header [$contentLengthHeader].',
+          request.url,
+        );
+      case final contentLengthHeader?:
+        contentLength = int.parse(contentLengthHeader);
+    }
+
     return StreamedResponse(responseDataController.stream, result.statusCode,
-        contentLength: contentLengthHeader == null
-            ? null
-            : int.tryParse(contentLengthHeader),
+        contentLength: contentLength,
         reasonPhrase: result.statusText,
         request: request,
         isRedirect: result.isRedirect,

pkgs/cronet_http/pubspec.yaml

@@ -1,7 +1,7 @@
 name: cronet_http
 description: >
   An Android Flutter plugin that provides access to the Cronet HTTP client.
-version: 0.2.1
+version: 0.2.2
 repository: https://github.com/dart-lang/http/tree/master/pkgs/cronet_http
 
 environment:

pkgs/cupertino_http/CHANGELOG.md

@@ -8,6 +8,7 @@
   any `List<int>`.
 * Disable additional analyses for generated Objective-C bindings to prevent
   errors from `dart analyze`.
+* Throw `ClientException` when the `'Content-Length'` header is invalid.
 
 ## 1.0.1
 

pkgs/cupertino_http/lib/src/cupertino_client.dart

@@ -15,6 +15,8 @@ import 'package:http/http.dart';
 
 import 'cupertino_api.dart';
 
+final _digitRegex = RegExp(r'^\d+$');
+
 class _TaskTracker {
   final responseCompleter = Completer<URLResponse>();
   final BaseRequest request;
@@ -279,6 +281,17 @@ class CupertinoClient extends BaseClient {
       throw ClientException('Redirect limit exceeded', request.url);
     }
 
+    final responseHeaders = response.allHeaderFields
+        .map((key, value) => MapEntry(key.toLowerCase(), value));
+
+    if (responseHeaders['content-length'] case final contentLengthHeader?
+        when !_digitRegex.hasMatch(contentLengthHeader)) {
+      throw ClientException(
+        'Invalid content-length header [$contentLengthHeader].',
+        request.url,
+      );
+    }
+
     return StreamedResponse(
       taskTracker.responseController.stream,
       response.statusCode,
@@ -288,8 +301,7 @@ class CupertinoClient extends BaseClient {
       reasonPhrase: _findReasonPhrase(response.statusCode),
       request: request,
       isRedirect: !request.followRedirects && taskTracker.numRedirects > 0,
-      headers: response.allHeaderFields
-          .map((key, value) => MapEntry(key.toLowerCase(), value)),
+      headers: responseHeaders,
     );
   }
 }

pkgs/http/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 1.1.1
+
+* `BrowserClient` throws `ClientException` when the `'Content-Length'` header
+  is invalid.
+* `IOClient` trims trailing whitespace on header values.
+
 ## 1.1.0
 
 * Add better error messages for `SocketException`s when using `IOClient`.

pkgs/http/lib/src/browser_client.dart

@@ -12,6 +12,8 @@ import 'byte_stream.dart';
 import 'exception.dart';
 import 'streamed_response.dart';
 
+final _digitRegex = RegExp(r'^\d+$');
+
 /// Create a [BrowserClient].
 ///
 /// Used from conditional imports, matches the definition in `client_stub.dart`.
@@ -64,6 +66,14 @@ class BrowserClient extends BaseClient {
     var completer = Completer<StreamedResponse>();
 
     unawaited(xhr.onLoad.first.then((_) {
+      if (xhr.responseHeaders['content-length'] case final contentLengthHeader?
+          when !_digitRegex.hasMatch(contentLengthHeader)) {
+        completer.completeError(ClientException(
+          'Invalid content-length header [$contentLengthHeader].',
+          request.url,
+        ));
+        return;
+      }
       var body = (xhr.response as ByteBuffer).asUint8List();
       completer.complete(StreamedResponse(
           ByteStream.fromBytes(body), xhr.status!,

pkgs/http/lib/src/io_client.dart

@@ -98,7 +98,10 @@ class IOClient extends BaseClient {
 
       var headers = <String, String>{};
       response.headers.forEach((key, values) {
-        headers[key] = values.join(',');
+        // TODO: Remove trimRight() when
+        // https://github.com/dart-lang/sdk/issues/53005 is resolved and the
+        // package:http SDK constraint requires that version or later.
+        headers[key] = values.map((value) => value.trimRight()).join(',');
       });
 
       return IOStreamedResponse(

pkgs/http/pubspec.yaml

@@ -1,5 +1,5 @@
 name: http
-version: 1.1.0
+version: 1.1.1-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_client_conformance_tests/lib/src/response_headers_server.dart

@@ -22,14 +22,21 @@ void hybridMain(StreamChannel<Object?> channel) async {
 
   server = (await HttpServer.bind('localhost', 0))
     ..listen((request) async {
-      request.response.headers.set('Access-Control-Allow-Origin', '*');
-      request.response.headers.set('Access-Control-Expose-Headers', '*');
+      await request.drain<void>();
+      final socket = await request.response.detachSocket(writeHeaders: false);
 
-      (await clientQueue.next as Map).forEach((key, value) => request
-          .response.headers
-          .set(key as String, value as String, preserveHeaderCase: true));
-
-      await request.response.close();
+      final headers = (await clientQueue.next) as String;
+      socket
+        ..writeAll([
+          'HTTP/1.1 200 OK',
+          'Access-Control-Allow-Origin: *',
+          'Access-Control-Expose-Headers: *',
+          'Content-Type: text/plain',
+          '', // Add \r\n at the end of this header section.
+        ], '\r\n')
+        ..write(headers)
+        ..write('Connection: Closed\r\n\r\n');
+      await socket.close();
       unawaited(server.close());
     });
 

pkgs/http_client_conformance_tests/lib/src/response_headers_tests.dart

@@ -24,14 +24,23 @@ void testResponseHeaders(Client client) async {
     });
 
     test('single header', () async {
-      httpServerChannel.sink.add({'foo': 'bar'});
+      httpServerChannel.sink.add('foo: bar\r\n');
 
       final response = await client.get(Uri.http(host, ''));
       expect(response.headers['foo'], 'bar');
     });
 
-    test('UPPERCASE header', () async {
-      httpServerChannel.sink.add({'foo': 'BAR'});
+    test('UPPERCASE header value', () async {
+      httpServerChannel.sink.add('foo: BAR\r\n');
+
+      final response = await client.get(Uri.http(host, ''));
+      // RFC 2616 14.44 states that header field names are case-insensitive.
+      // http.Client canonicalizes field names into lower case.
+      expect(response.headers['foo'], 'BAR');
+    });
+
+    test('space surrounding header value', () async {
+      httpServerChannel.sink.add('foo: \t BAR \t \r\n');
 
       final response = await client.get(Uri.http(host, ''));
       // RFC 2616 14.44 states that header field names are case-insensitive.
@@ -41,7 +50,7 @@ void testResponseHeaders(Client client) async {
 
     test('multiple headers', () async {
       httpServerChannel.sink
-          .add({'field1': 'value1', 'field2': 'value2', 'field3': 'value3'});
+          .add('field1: value1\r\n' 'field2: value2\r\n' 'field3: value3\r\n');
 
       final response = await client.get(Uri.http(host, ''));
       expect(response.headers['field1'], 'value1');
@@ -50,10 +59,76 @@ void testResponseHeaders(Client client) async {
     });
 
     test('multiple values per header', () async {
-      httpServerChannel.sink.add({'list': 'apple, orange, banana'});
+      // RFC-2616 4.2 says:
+      // "The field value MAY be preceded by any amount of LWS, though a single
+      // SP is preferred." and
+      // "The field-content does not include any leading or trailing LWS ..."
+      httpServerChannel.sink.add('list: apple, orange, banana\r\n');
 
       final response = await client.get(Uri.http(host, ''));
-      expect(response.headers['list'], 'apple, orange, banana');
+      expect(response.headers['list'],
+          matches(r'apple[ \t]*,[ \t]*orange[ \t]*,[ \t]*banana'));
+    });
+
+    test('multiple values per header surrounded with spaces', () async {
+      httpServerChannel.sink
+          .add('list: \t apple \t, \t orange \t , \t banana\t \t \r\n');
+
+      final response = await client.get(Uri.http(host, ''));
+      expect(response.headers['list'],
+          matches(r'apple[ \t]*,[ \t]*orange[ \t]*,[ \t]*banana'));
+    });
+
+    test('multiple headers with the same name', () async {
+      httpServerChannel.sink.add('list: apple\r\n'
+          'list: orange\r\n'
+          'list: banana\r\n');
+
+      final response = await client.get(Uri.http(host, ''));
+      expect(response.headers['list'],
+          matches(r'apple[ \t]*,[ \t]*orange[ \t]*,[ \t]*banana'));
+    });
+
+    test('multiple headers with the same name but different cases', () async {
+      httpServerChannel.sink.add('list: apple\r\n'
+          'LIST: orange\r\n'
+          'List: banana\r\n');
+
+      final response = await client.get(Uri.http(host, ''));
+      expect(response.headers['list'],
+          matches(r'apple[ \t]*,[ \t]*orange[ \t]*,[ \t]*banana'));
+    });
+
+    group('content length', () {
+      test('surrounded in spaces', () async {
+        // RFC-2616 4.2 says:
+        // "The field value MAY be preceded by any amount of LWS, though a
+        // single SP is preferred." and
+        // "The field-content does not include any leading or trailing LWS ..."
+        httpServerChannel.sink.add('content-length: \t 0 \t \r\n');
+        final response = await client.get(Uri.http(host, ''));
+        expect(response.contentLength, 0);
+      },
+          skip: 'Enable after https://github.com/dart-lang/sdk/issues/51532 '
+              'is fixed');
+
+      test('non-integer', () async {
+        httpServerChannel.sink.add('content-length: cat\r\n');
+        await expectLater(
+            client.get(Uri.http(host, '')), throwsA(isA<ClientException>()));
+      });
+
+      test('negative', () async {
+        httpServerChannel.sink.add('content-length: -5\r\n');
+        await expectLater(
+            client.get(Uri.http(host, '')), throwsA(isA<ClientException>()));
+      });
+
+      test('bigger than actual body', () async {
+        httpServerChannel.sink.add('content-length: 100\r\n');
+        await expectLater(
+            client.get(Uri.http(host, '')), throwsA(isA<ClientException>()));
+      });
     });
   });
 }

pkgs/java_http/lib/src/java_client.dart

@@ -14,6 +14,8 @@ import 'third_party/java/lang/System.dart';
 import 'third_party/java/net/HttpURLConnection.dart';
 import 'third_party/java/net/URL.dart';
 
+final _digitRegex = RegExp(r'^\d+$');
+
 // TODO: Add a description of the implementation.
 // Look at the description of cronet_client.dart and cupertino_client.dart for
 // examples.
@@ -69,7 +71,8 @@ class JavaClient extends BaseClient {
     });
 
     return StreamedResponse(Stream.value(responseBody), statusCode,
-        contentLength: _contentLengthHeader(request, responseHeaders),
+        contentLength:
+            _contentLengthHeader(request, responseHeaders, responseBody.length),
         request: request,
         headers: responseHeaders,
         reasonPhrase: reasonPhrase);
@@ -112,28 +115,28 @@ class JavaClient extends BaseClient {
       if (headerName.isNull) continue;
 
       headers
-          .putIfAbsent(headerName.toDartString(), () => [])
+          .putIfAbsent(headerName.toDartString().toLowerCase(), () => [])
           .add(headerValue.toDartString());
     }
 
-    return headers
-        .map((key, value) => MapEntry(key.toLowerCase(), value.join(',')));
+    return headers.map((key, value) => MapEntry(key, value.join(',')));
   }
 
-  int? _contentLengthHeader(BaseRequest request, Map<String, String> headers) {
-    final contentLengthHeader = headers['content-length'];
-
-    // Return null if the content length header is not set.
-    if (contentLengthHeader == null) return null;
-
-    // Throw ClientException if the content length header is not an integer.
-    final contentLength = int.tryParse(contentLengthHeader);
-    if (contentLength == null) {
-      throw ClientException(
-        'Invalid content-length header: $contentLengthHeader. '
-        'Content-length must be a non-negative integer.',
-        request.url,
-      );
+  int? _contentLengthHeader(
+      BaseRequest request, Map<String, String> headers, int bodyLength) {
+    int? contentLength;
+    switch (headers['content-length']) {
+      case final contentLengthHeader?
+          when !_digitRegex.hasMatch(contentLengthHeader):
+        throw ClientException(
+          'Invalid content-length header [$contentLengthHeader].',
+          request.url,
+        );
+      case final contentLengthHeader?:
+        contentLength = int.parse(contentLengthHeader);
+        if (bodyLength < contentLength) {
+          throw ClientException('Unexpected end of body', request.url);
+        }
     }
 
     return contentLength;