From 1a42b4a16636e86b5fed60666b0de91219c5110c Mon Sep 17 00:00:00 2001 From: Brian Quinlan Date: Fri, 21 Jul 2023 16:31:41 -0700 Subject: [PATCH] Harmonize response header behavior (#993) --- pkgs/cronet_http/CHANGELOG.md | 1 + pkgs/cronet_http/lib/src/cronet_client.dart | 18 +++- pkgs/cronet_http/pubspec.yaml | 2 +- pkgs/cupertino_http/CHANGELOG.md | 1 + .../lib/src/cupertino_client.dart | 16 +++- pkgs/http/CHANGELOG.md | 6 ++ pkgs/http/lib/src/browser_client.dart | 10 +++ pkgs/http/lib/src/io_client.dart | 5 +- pkgs/http/pubspec.yaml | 2 +- .../lib/src/response_headers_server.dart | 21 +++-- .../lib/src/response_headers_tests.dart | 87 +++++++++++++++++-- pkgs/java_http/lib/src/java_client.dart | 39 +++++---- 12 files changed, 168 insertions(+), 40 deletions(-) diff --git a/pkgs/cronet_http/CHANGELOG.md b/pkgs/cronet_http/CHANGELOG.md index 8126700d45..fe0c35faca 100644 --- a/pkgs/cronet_http/CHANGELOG.md +++ b/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 diff --git a/pkgs/cronet_http/lib/src/cronet_client.dart b/pkgs/cronet_http/lib/src/cronet_client.dart index 8799d43166..3e4007eb27 100644 --- a/pkgs/cronet_http/lib/src/cronet_client.dart +++ b/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 _cronetEngineFinalizer = Finalizer(_api.freeEngine); @@ -266,11 +267,20 @@ class CronetClient extends BaseClient { .cast>() .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, diff --git a/pkgs/cronet_http/pubspec.yaml b/pkgs/cronet_http/pubspec.yaml index e90c9b0c90..f1517a6957 100644 --- a/pkgs/cronet_http/pubspec.yaml +++ b/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: diff --git a/pkgs/cupertino_http/CHANGELOG.md b/pkgs/cupertino_http/CHANGELOG.md index de027abfb7..8f5ed3d145 100644 --- a/pkgs/cupertino_http/CHANGELOG.md +++ b/pkgs/cupertino_http/CHANGELOG.md @@ -8,6 +8,7 @@ any `List`. * 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 diff --git a/pkgs/cupertino_http/lib/src/cupertino_client.dart b/pkgs/cupertino_http/lib/src/cupertino_client.dart index a716b49418..a4c7715de5 100644 --- a/pkgs/cupertino_http/lib/src/cupertino_client.dart +++ b/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(); 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, ); } } diff --git a/pkgs/http/CHANGELOG.md b/pkgs/http/CHANGELOG.md index 86f9805011..6d39b104df 100644 --- a/pkgs/http/CHANGELOG.md +++ b/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`. diff --git a/pkgs/http/lib/src/browser_client.dart b/pkgs/http/lib/src/browser_client.dart index ddaa43feae..9345be0ce1 100644 --- a/pkgs/http/lib/src/browser_client.dart +++ b/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(); 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!, diff --git a/pkgs/http/lib/src/io_client.dart b/pkgs/http/lib/src/io_client.dart index 88d44113e8..247cc8cad6 100644 --- a/pkgs/http/lib/src/io_client.dart +++ b/pkgs/http/lib/src/io_client.dart @@ -98,7 +98,10 @@ class IOClient extends BaseClient { var headers = {}; 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( diff --git a/pkgs/http/pubspec.yaml b/pkgs/http/pubspec.yaml index 990ca6ad37..ec23e2d608 100644 --- a/pkgs/http/pubspec.yaml +++ b/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 diff --git a/pkgs/http_client_conformance_tests/lib/src/response_headers_server.dart b/pkgs/http_client_conformance_tests/lib/src/response_headers_server.dart index 3f2d4d3f0f..54431edd74 100644 --- a/pkgs/http_client_conformance_tests/lib/src/response_headers_server.dart +++ b/pkgs/http_client_conformance_tests/lib/src/response_headers_server.dart @@ -22,14 +22,21 @@ void hybridMain(StreamChannel 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(); + 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()); }); diff --git a/pkgs/http_client_conformance_tests/lib/src/response_headers_tests.dart b/pkgs/http_client_conformance_tests/lib/src/response_headers_tests.dart index 9b2c262c3d..012fa63943 100644 --- a/pkgs/http_client_conformance_tests/lib/src/response_headers_tests.dart +++ b/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())); + }); + + test('negative', () async { + httpServerChannel.sink.add('content-length: -5\r\n'); + await expectLater( + client.get(Uri.http(host, '')), throwsA(isA())); + }); + + test('bigger than actual body', () async { + httpServerChannel.sink.add('content-length: 100\r\n'); + await expectLater( + client.get(Uri.http(host, '')), throwsA(isA())); + }); }); }); } diff --git a/pkgs/java_http/lib/src/java_client.dart b/pkgs/java_http/lib/src/java_client.dart index 283fce5ff1..f652084507 100644 --- a/pkgs/java_http/lib/src/java_client.dart +++ b/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 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 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;