From d05d7546911a7eb6af5aa2d353be6c67e0d2b274 Mon Sep 17 00:00:00 2001 From: evanweible-wf Date: Tue, 21 Jun 2016 12:52:29 -0600 Subject: [PATCH] Handle request with no encoding and no charset - Throw ArgumentError if a request's encoding is set to null - Null check for encoding before reading name in ResponseFormatException - HttpBody constructors now accept encoding param and will use if given - HttpBody constructors only parse content-type for charset if encoding not given - HttpBody constructors now default fallbackEncoding to UTF8 - Only catch ArgumentException when attempting to encode a body - Only catch FormatException when attempting to decode body bytes --- CHANGELOG.md | 14 ++++ lib/src/http/common/form_request.dart | 3 +- lib/src/http/common/request.dart | 1 + lib/src/http/http_body.dart | 58 ++++++++++++--- lib/src/http/response_format_exception.dart | 5 +- test/unit/http/form_request_test.dart | 16 +++++ test/unit/http/http_body_test.dart | 47 ++++++++++-- test/unit/http/json_request_test.dart | 16 +++++ test/unit/http/plain_text_request_test.dart | 16 +++++ .../http/response_format_exception_test.dart | 71 +++++++++++++++++++ test/unit/http/streamed_request_test.dart | 18 +++++ test/unit/platform_independent_unit_test.dart | 3 + 12 files changed, 250 insertions(+), 18 deletions(-) create mode 100644 test/unit/http/response_format_exception_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index cc6a62c8..bb454fa3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # Changelog +## [2.6.1](https://github.com/Workvia/w_transport/compare/2.6.0...2.6.1) +_TBD_ + +- **Bug Fix:** A request's `encoding` property can no longer be set to null. + This would have most likely caused an RTE when the request was sent, so now an + `ArgumentError` will be thrown immediately. + +- **Bug Fix:** As of 2.6.0, if you were to set a request's content-type manually + without a charset or with an unknown charset, it was possible to hit an RTE + due to a null `encoding`. The `HttpBody` class has been updated to be more + resilient to a missing encoding or charset. Additionally, all request classes + will now pass in the value of its `encoding` property, which should now always + be non-null. + ## [2.6.0](https://github.com/Workvia/w_transport/compare/2.5.1...2.6.0) _June 20, 2016_ diff --git a/lib/src/http/common/form_request.dart b/lib/src/http/common/form_request.dart index 57f5823f..1875154d 100644 --- a/lib/src/http/common/form_request.dart +++ b/lib/src/http/common/form_request.dart @@ -70,6 +70,7 @@ abstract class CommonFormRequest extends CommonRequest implements FormRequest { if (body != null) { this.fields = body; } - return new HttpBody.fromBytes(contentType, _encodedQuery); + return new HttpBody.fromBytes(contentType, _encodedQuery, + fallbackEncoding: encoding); } } diff --git a/lib/src/http/common/request.dart b/lib/src/http/common/request.dart index f2895c8a..da6cd75d 100644 --- a/lib/src/http/common/request.dart +++ b/lib/src/http/common/request.dart @@ -197,6 +197,7 @@ abstract class CommonRequest extends Object /// will update the [contentType] `charset` parameter. set encoding(Encoding encoding) { verifyUnsent(); + if (encoding == null) throw new ArgumentError.notNull('encoding'); _encoding = encoding; if (!_contentTypeSetManually) { updateContentType( diff --git a/lib/src/http/http_body.dart b/lib/src/http/http_body.dart index ca42f112..1d69818b 100644 --- a/lib/src/http/http_body.dart +++ b/lib/src/http/http_body.dart @@ -56,10 +56,31 @@ class HttpBody extends BaseHttpBody { Encoding _encoding; /// Construct the body to an HTTP request or an HTTP response from bytes. + /// + /// If [encoding] is given, it will be used to encode/decode the body. + /// + /// Otherwise, the `charset` parameter from [contentType] will be mapped to an + /// Encoding supported by Dart. + /// + /// If a `charset` parameter is not available or the value is unrecognized, + /// [fallbackEncoding] will be used. + /// + /// If [fallbackEncoding] is `null`, UTF8 will be the default encoding. + /// + /// If an encoding cannot be parsed from the content-type header (via the + /// `charset` param), then [fallbackEncoding] will be used (UTF8 by default). HttpBody.fromBytes(MediaType this.contentType, List bytes, - {Encoding fallbackEncoding}) { - _encoding = http_utils.parseEncodingFromContentType(contentType, - fallback: fallbackEncoding); + {Encoding encoding, Encoding fallbackEncoding}) { + if (fallbackEncoding == null) { + fallbackEncoding = UTF8; + } + if (encoding != null) { + _encoding = encoding; + } else { + _encoding = http_utils.parseEncodingFromContentType(contentType, + fallback: fallbackEncoding); + } + if (bytes == null) { bytes = []; } @@ -67,10 +88,31 @@ class HttpBody extends BaseHttpBody { } /// Construct the body to an HTTP request or an HTTP response from text. + /// + /// If [encoding] is given, it will be used to encode/decode the body. + /// + /// Otherwise, the `charset` parameter from [contentType] will be mapped to an + /// Encoding supported by Dart. + /// + /// If a `charset` parameter is not available or the value is unrecognized, + /// [fallbackEncoding] will be used. + /// + /// If [fallbackEncoding] is `null`, UTF8 will be the default encoding. + /// + /// If an encoding cannot be parsed from the content-type header (via the + /// `charset` param), then [fallbackEncoding] will be used (UTF8 by default). HttpBody.fromString(MediaType this.contentType, String body, - {Encoding fallbackEncoding}) { - _encoding = http_utils.parseEncodingFromContentType(contentType, - fallback: fallbackEncoding); + {Encoding encoding, Encoding fallbackEncoding: UTF8}) { + if (fallbackEncoding == null) { + fallbackEncoding = UTF8; + } + if (encoding != null) { + _encoding = encoding; + } else { + _encoding = http_utils.parseEncodingFromContentType(contentType, + fallback: fallbackEncoding); + } + if (body == null) { body = ''; } @@ -86,7 +128,7 @@ class HttpBody extends BaseHttpBody { var encoded; try { encoded = encoding.encode(_body); - } catch (e) { + } on ArgumentError { throw new ResponseFormatException(contentType, encoding, body: _body); } _bytes = new Uint8List.fromList(encoded); @@ -99,7 +141,7 @@ class HttpBody extends BaseHttpBody { if (_body == null) { try { _body = encoding.decode(_bytes); - } catch (e) { + } on FormatException { throw new ResponseFormatException(contentType, encoding, bytes: _bytes); } } diff --git a/lib/src/http/response_format_exception.dart b/lib/src/http/response_format_exception.dart index 3c2eb9df..f7e1e040 100644 --- a/lib/src/http/response_format_exception.dart +++ b/lib/src/http/response_format_exception.dart @@ -50,9 +50,10 @@ class ResponseFormatException implements Exception { bodyLine = 'Bytes: $bytes'; } - String msg = description; + var msg = description; + var encodingName = encoding != null ? encoding.name : 'null'; msg += '\n\tContent-Type: $contentType'; - msg += '\n\tEncoding: ${encoding.name}'; + msg += '\n\tEncoding: $encodingName'; msg += '\n\t$bodyLine'; return msg; diff --git a/test/unit/http/form_request_test.dart b/test/unit/http/form_request_test.dart index d9316062..5f9ff268 100644 --- a/test/unit/http/form_request_test.dart +++ b/test/unit/http/form_request_test.dart @@ -95,6 +95,13 @@ void main() { }, throwsUnsupportedError); }); + test('setting encoding to null should throw', () { + var request = new FormRequest(); + expect(() { + request.encoding = null; + }, throwsArgumentError); + }); + test('setting encoding should update content-type', () { FormRequest request = new FormRequest(); expect(request.contentType.parameters['charset'], equals(UTF8.name)); @@ -143,6 +150,15 @@ void main() { }, throwsStateError); }); + test('custom content-type without inferrable encoding', () async { + Uri uri = Uri.parse('/test'); + MockTransports.http.expect('POST', uri); + var request = new FormRequest() + ..contentType = new MediaType('application', 'x-custom') + ..fields['foo'] = 'bar'; + await request.post(uri: uri); + }); + test('clone()', () { var fields = {'f1': 'v1', 'f2': 'v2'}; FormRequest orig = new FormRequest()..fields = fields; diff --git a/test/unit/http/http_body_test.dart b/test/unit/http/http_body_test.dart index ed813e55..f914ab4d 100644 --- a/test/unit/http/http_body_test.dart +++ b/test/unit/http/http_body_test.dart @@ -36,18 +36,51 @@ void main() { expect(body.asBytes(), isEmpty); }); + test('should use encoding if one is explicitly given', () { + var contentType = new MediaType('text', 'plain'); + + var stringBody = + new HttpBody.fromString(contentType, 'body', encoding: ASCII); + expect(stringBody.encoding, equals(ASCII)); + + var bytesBody = new HttpBody.fromBytes(contentType, UTF8.encode('body'), + encoding: ASCII); + expect(bytesBody.encoding, equals(ASCII)); + }); + test('should parse encoding from content-type', () { - MediaType contentType = + var contentType = new MediaType('text', 'plain', {'charset': ASCII.name}); - HttpBody body = new HttpBody.fromString(contentType, 'body'); - expect(body.encoding.name, equals(ASCII.name)); + + var stringBody = new HttpBody.fromString(contentType, 'body'); + expect(stringBody.encoding, equals(ASCII)); + + var bytesBody = + new HttpBody.fromBytes(contentType, UTF8.encode('body')); + expect(bytesBody.encoding, equals(ASCII)); }); test('should allow a fallback encoding', () { - MediaType contentType = new MediaType('text', 'plain'); - HttpBody body = new HttpBody.fromString(contentType, 'body', - fallbackEncoding: LATIN1); - expect(body.encoding.name, equals(LATIN1.name)); + var contentType = new MediaType('text', 'plain'); + + var stringBody = new HttpBody.fromString(contentType, 'body', + fallbackEncoding: ASCII); + expect(stringBody.encoding, equals(ASCII)); + + var bytesBody = new HttpBody.fromBytes(contentType, UTF8.encode('body'), + fallbackEncoding: ASCII); + expect(bytesBody.encoding, equals(ASCII)); + }); + + test('should use UTF8 by default', () { + var contentType = new MediaType('text', 'plain'); + + var stringBody = new HttpBody.fromString(contentType, 'body'); + expect(stringBody.encoding, equals(UTF8)); + + var bytesBody = + new HttpBody.fromBytes(contentType, UTF8.encode('body')); + expect(bytesBody.encoding, equals(UTF8)); }); test('content-length should be calculated automaticlaly', () { diff --git a/test/unit/http/json_request_test.dart b/test/unit/http/json_request_test.dart index cd4dbdf0..3b0e23c7 100644 --- a/test/unit/http/json_request_test.dart +++ b/test/unit/http/json_request_test.dart @@ -121,6 +121,13 @@ void main() { }, throwsUnsupportedError); }); + test('setting encoding to null should throw', () { + var request = new JsonRequest(); + expect(() { + request.encoding = null; + }, throwsArgumentError); + }); + test('setting encoding should update content-type', () { JsonRequest request = new JsonRequest(); expect(request.contentType.parameters['charset'], equals(UTF8.name)); @@ -169,6 +176,15 @@ void main() { }, throwsStateError); }); + test('custom content-type without inferrable encoding', () async { + Uri uri = Uri.parse('/test'); + MockTransports.http.expect('POST', uri); + var request = new JsonRequest() + ..contentType = new MediaType('application', 'x-custom') + ..body = {'foo': 'bar'}; + await request.post(uri: uri); + }); + test('clone()', () { var body = [ {'f1': 'v1', 'f2': 'v2'} diff --git a/test/unit/http/plain_text_request_test.dart b/test/unit/http/plain_text_request_test.dart index 9404da92..6795f475 100644 --- a/test/unit/http/plain_text_request_test.dart +++ b/test/unit/http/plain_text_request_test.dart @@ -117,6 +117,13 @@ void main() { }, throwsUnsupportedError); }); + test('setting encoding to null should throw', () { + var request = new Request(); + expect(() { + request.encoding = null; + }, throwsArgumentError); + }); + test('setting encoding should update content-type', () { Request request = new Request(); expect(request.contentType.parameters['charset'], equals(UTF8.name)); @@ -165,6 +172,15 @@ void main() { }, throwsStateError); }); + test('custom content-type without inferrable encoding', () async { + Uri uri = Uri.parse('/test'); + MockTransports.http.expect('POST', uri); + var request = new Request() + ..contentType = new MediaType('application', 'x-custom') + ..body = 'body'; + await request.post(uri: uri); + }); + test('clone()', () { var body = 'body'; Request orig = new Request()..body = body; diff --git a/test/unit/http/response_format_exception_test.dart b/test/unit/http/response_format_exception_test.dart new file mode 100644 index 00000000..d9be1c3f --- /dev/null +++ b/test/unit/http/response_format_exception_test.dart @@ -0,0 +1,71 @@ +// Copyright 2015 Workiva Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +library w_transport.test.unit.http.response_format_exception_test; + +import 'dart:convert'; + +import 'package:http_parser/http_parser.dart' show MediaType; +import 'package:test/test.dart'; + +import 'package:w_transport/src/http/response_format_exception.dart'; + +import '../../naming.dart'; + +void main() { + Naming naming = new Naming() + ..testType = testTypeUnit + ..topic = topicHttp; + + group(naming.toString(), () { + group('ResponseFormatException', () { + test('should detail why bytes could not be decoded', () { + var bytes = UTF8.encode('bodyçå®'); + var contentType = + new MediaType('application', 'json', {'charset': ASCII.name}); + var exception = + new ResponseFormatException(contentType, ASCII, bytes: bytes); + expect(exception.toString(), contains('Bytes could not be decoded')); + expect(exception.toString(), contains('Content-Type: $contentType')); + expect(exception.toString(), contains('Encoding: ${ASCII.name}')); + expect( + exception.toString(), contains(UTF8.encode('bodyçå®').toString())); + }); + + test('should detail why string could not be encoded', () { + var body = 'bodyçå®'; + var contentType = + new MediaType('application', 'json', {'charset': ASCII.name}); + var exception = + new ResponseFormatException(contentType, ASCII, body: body); + expect(exception.toString(), contains('Body could not be encoded')); + expect(exception.toString(), contains('Content-Type: $contentType')); + expect(exception.toString(), contains('Encoding: ${ASCII.name}')); + expect(exception.toString(), contains('bodyçå®')); + }); + + test('should warn if encoding is null', () { + var body = 'bodyçå®'; + var contentType = + new MediaType('application', 'json', {'charset': ASCII.name}); + var exception = + new ResponseFormatException(contentType, null, body: body); + expect(exception.toString(), contains('Body could not be encoded')); + expect(exception.toString(), contains('Content-Type: $contentType')); + expect(exception.toString(), contains('Encoding: null')); + expect(exception.toString(), contains('bodyçå®')); + }); + }); + }); +} diff --git a/test/unit/http/streamed_request_test.dart b/test/unit/http/streamed_request_test.dart index 6a451b6d..1be9179d 100644 --- a/test/unit/http/streamed_request_test.dart +++ b/test/unit/http/streamed_request_test.dart @@ -104,6 +104,13 @@ void main() { }, throwsStateError); }); + test('setting encoding to null should throw', () { + var request = new StreamedRequest(); + expect(() { + request.encoding = null; + }, throwsArgumentError); + }); + test('setting encoding should update content-type', () { StreamedRequest request = new StreamedRequest(); expect(request.contentType.parameters['charset'], equals(UTF8.name)); @@ -152,6 +159,17 @@ void main() { }, throwsStateError); }); + test('custom content-type without inferrable encoding', () async { + Uri uri = Uri.parse('/test'); + MockTransports.http.expect('POST', uri); + var request = new StreamedRequest() + ..contentType = new MediaType('application', 'x-custom') + ..body = new Stream.fromIterable([ + [1, 2] + ]); + await request.post(uri: uri); + }); + test('clone()', () { StreamedRequest request = new StreamedRequest(); expect(request.clone, throwsUnsupportedError); diff --git a/test/unit/platform_independent_unit_test.dart b/test/unit/platform_independent_unit_test.dart index 4831f02d..5b46120e 100644 --- a/test/unit/platform_independent_unit_test.dart +++ b/test/unit/platform_independent_unit_test.dart @@ -29,6 +29,8 @@ import 'http/plain_text_request_test.dart' as http_plain_text_request_test; import 'http/request_exception_test.dart' as http_request_exception_test; import 'http/request_progress_test.dart' as http_request_progress_test; import 'http/request_test.dart' as http_request_test; +import 'http/response_format_exception_test.dart' + as http_response_format_exception_test; import 'http/response_test.dart' as http_response_test; import 'http/streamed_request_test.dart' as http_streamed_request_test; import 'http/utils_test.dart' as http_utils_test; @@ -54,6 +56,7 @@ void main() { http_request_exception_test.main(); http_request_progress_test.main(); http_request_test.main(); + http_response_format_exception_test.main(); http_response_test.main(); http_streamed_request_test.main(); http_utils_test.main();