diff --git a/.changes/next-release/enhancement-s3-94466.json b/.changes/next-release/enhancement-s3-94466.json new file mode 100644 index 0000000000..5fc2e636ec --- /dev/null +++ b/.changes/next-release/enhancement-s3-94466.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "``s3``", + "description": "Adds logic to gracefully handle invalid timestamps returned in the Expires header." +} diff --git a/botocore/data/s3/2006-03-01/service-2.sdk-extras.json b/botocore/data/s3/2006-03-01/service-2.sdk-extras.json new file mode 100644 index 0000000000..d04e9d0b45 --- /dev/null +++ b/botocore/data/s3/2006-03-01/service-2.sdk-extras.json @@ -0,0 +1,8 @@ +{ + "version": 1.0, + "merge": { + "shapes": { + "Expires":{"type":"timestamp"} + } + } +} diff --git a/botocore/docs/bcdoc/restdoc.py b/botocore/docs/bcdoc/restdoc.py index d23fcf2825..3868126cc9 100644 --- a/botocore/docs/bcdoc/restdoc.py +++ b/botocore/docs/bcdoc/restdoc.py @@ -214,6 +214,9 @@ def get_section(self, name): """Retrieve a section""" return self._structure[name] + def has_section(self, name): + return name in self._structure + def delete_section(self, name): """Delete a section""" del self._structure[name] diff --git a/botocore/endpoint.py b/botocore/endpoint.py index 59f3d86c8e..1c2cee068b 100644 --- a/botocore/endpoint.py +++ b/botocore/endpoint.py @@ -302,10 +302,18 @@ def _do_get_response(self, request, operation_model, context): history_recorder.record('HTTP_RESPONSE', http_response_record_dict) protocol = operation_model.metadata['protocol'] + customized_response_dict = {} + self._event_emitter.emit( + f"before-parse.{service_id}.{operation_model.name}", + operation_model=operation_model, + response_dict=response_dict, + customized_response_dict=customized_response_dict, + ) parser = self._response_parser_factory.create_parser(protocol) parsed_response = parser.parse( response_dict, operation_model.output_shape ) + parsed_response.update(customized_response_dict) # Do a second parsing pass to pick up on any modeled error fields # NOTE: Ideally, we would push this down into the parser classes but # they currently have no reference to the operation or service model diff --git a/botocore/handlers.py b/botocore/handlers.py index 99eed3bfc5..9cb1d052c0 100644 --- a/botocore/handlers.py +++ b/botocore/handlers.py @@ -1176,6 +1176,69 @@ def remove_content_type_header_for_presigning(request, **kwargs): del request.headers['Content-Type'] +def handle_expires_header( + operation_model, response_dict, customized_response_dict, **kwargs +): + if _has_expires_shape(operation_model.output_shape): + if expires_value := response_dict.get('headers', {}).get('Expires'): + customized_response_dict['ExpiresString'] = expires_value + try: + utils.parse_timestamp(expires_value) + except (ValueError, RuntimeError): + logger.warning( + f'Failed to parse the "Expires" member as a timestamp: {expires_value}. ' + f'The unparsed value is available in the response under "ExpiresString".' + ) + del response_dict['headers']['Expires'] + + +def _has_expires_shape(shape): + if not shape: + return False + return any( + member_shape.name == 'Expires' + and member_shape.serialization.get('name') == 'Expires' + for member_shape in shape.members.values() + ) + + +def document_expires_shape(section, event_name, **kwargs): + # Updates the documentation for S3 operations that include the 'Expires' member + # in their response structure. Documents a synthetic member 'ExpiresString' and + # includes a deprecation notice for 'Expires'. + if 'response-example' in event_name: + if not section.has_section('structure-value'): + return + parent = section.get_section('structure-value') + if not parent.has_section('Expires'): + return + param_line = parent.get_section('Expires') + param_line.add_new_section('ExpiresString') + new_param_line = param_line.get_section('ExpiresString') + new_param_line.write("'ExpiresString': 'string',") + new_param_line.style.new_line() + elif 'response-params' in event_name: + if not section.has_section('Expires'): + return + param_section = section.get_section('Expires') + # Add a deprecation notice for the "Expires" param + doc_section = param_section.get_section('param-documentation') + doc_section.style.start_note() + doc_section.write( + 'This member has been deprecated. Please use ``ExpiresString`` instead.' + ) + doc_section.style.end_note() + # Document the "ExpiresString" param + new_param_section = param_section.add_new_section('ExpiresString') + new_param_section.style.new_paragraph() + new_param_section.write('- **ExpiresString** *(string) --*') + new_param_section.style.indent() + new_param_section.style.new_paragraph() + new_param_section.write( + 'The raw, unparsed value of the ``Expires`` field.' + ) + + # This is a list of (event_name, handler). # When a Session is created, everything in this list will be # automatically registered with that Session. @@ -1205,6 +1268,7 @@ def remove_content_type_header_for_presigning(request, **kwargs): ('after-call.ec2.GetConsoleOutput', decode_console_output), ('after-call.cloudformation.GetTemplate', json_decode_template_body), ('after-call.s3.GetBucketLocation', parse_get_bucket_location), + ('before-parse.s3.*', handle_expires_header), ('before-parameter-build', generate_idempotent_uuid), ('before-parameter-build.s3', validate_bucket_name), ('before-parameter-build.s3', remove_bucket_from_url_paths_from_model), @@ -1231,6 +1295,8 @@ def remove_content_type_header_for_presigning(request, **kwargs): ('before-parameter-build.s3-control', remove_accid_host_prefix_from_model), ('docs.*.s3.CopyObject.complete-section', document_copy_source_form), ('docs.*.s3.UploadPartCopy.complete-section', document_copy_source_form), + ('docs.response-example.s3.*.complete-section', document_expires_shape), + ('docs.response-params.s3.*.complete-section', document_expires_shape), ('before-endpoint-resolution.s3', customize_endpoint_resolver_builtins), ('before-call', add_recursion_detection_header), ('before-call.s3', add_expect_header), diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py index 1465287d38..04fc32aa7b 100644 --- a/tests/functional/test_s3.py +++ b/tests/functional/test_s3.py @@ -15,6 +15,7 @@ import re import pytest +from dateutil.tz import tzutc import botocore.session from botocore import UNSIGNED @@ -1340,6 +1341,41 @@ def test_500_error_with_non_xml_body(self): self.assertEqual(len(http_stubber.requests), 2) +class TestS3ExpiresHeaderResponse(BaseS3OperationTest): + def test_valid_expires_value_in_response(self): + expires_value = "Thu, 01 Jan 1970 00:00:00 GMT" + mock_headers = {'expires': expires_value} + s3 = self.session.create_client("s3") + with ClientHTTPStubber(s3) as http_stubber: + http_stubber.add_response(headers=mock_headers) + response = s3.get_object(Bucket='mybucket', Key='mykey') + self.assertEqual( + response.get('Expires'), + datetime.datetime(1970, 1, 1, tzinfo=tzutc()), + ) + self.assertEqual(response.get('ExpiresString'), expires_value) + + def test_invalid_expires_value_in_response(self): + expires_value = "Invalid Date" + mock_headers = {'expires': expires_value} + warning_msg = 'Failed to parse the "Expires" member as a timestamp' + s3 = self.session.create_client("s3") + with self.assertLogs('botocore.handlers', level='WARNING') as log: + with ClientHTTPStubber(s3) as http_stubber: + http_stubber.add_response(headers=mock_headers) + response = s3.get_object(Bucket='mybucket', Key='mykey') + self.assertNotIn( + 'expires', + response.get('ResponseMetadata').get('HTTPHeaders'), + ) + self.assertNotIn('Expires', response) + self.assertEqual(response.get('ExpiresString'), expires_value) + self.assertTrue( + any(warning_msg in entry for entry in log.output), + f'Expected warning message not found in logs. Logs: {log.output}', + ) + + class TestWriteGetObjectResponse(BaseS3ClientConfigurationTest): def create_stubbed_s3_client(self, **kwargs): client = self.create_s3_client(**kwargs) diff --git a/tests/unit/test_endpoint.py b/tests/unit/test_endpoint.py index a2dc45755a..00790e4ecb 100644 --- a/tests/unit/test_endpoint.py +++ b/tests/unit/test_endpoint.py @@ -82,13 +82,16 @@ def setUp(self): def tearDown(self): self.factory_patch.stop() - def get_emitter_responses(self, num_retries=0, sleep_time=0): + def get_emitter_responses(self, num_retries=0, sleep_time=0, num_events=4): emitter_responses = [] + # We emit the following events: + # 1. request-created + # 2. before-send + # 3. before-parse (may not be emitted if certain errors are thrown) + # 4. response-received response_request_emitter_responses = [ - [(None, None)], # emit() response for request-created - [(None, None)], # emit() response for before-send - [(None, None)], # emit() response for response-received - ] + [(None, None)] # emit() response for each emitted event + ] * num_events for _ in range(num_retries): emitter_responses.extend(response_request_emitter_responses) # emit() response for retry for sleep time @@ -239,6 +242,7 @@ def test_retry_events_can_alter_behavior(self): expected_events=[ 'request-created.ec2.DescribeInstances', 'before-send.ec2.DescribeInstances', + 'before-parse.ec2.DescribeInstances', 'response-received.ec2.DescribeInstances', 'needs-retry.ec2.DescribeInstances', ] @@ -247,7 +251,7 @@ def test_retry_events_can_alter_behavior(self): def test_retry_on_socket_errors(self): self.event_emitter.emit.side_effect = self.get_emitter_responses( - num_retries=1 + num_retries=1, num_events=3 ) self.http_session.send.side_effect = HTTPClientError(error='wrapped') with self.assertRaises(HTTPClientError): diff --git a/tests/unit/test_handlers.py b/tests/unit/test_handlers.py index fef4c43eab..7e28356442 100644 --- a/tests/unit/test_handlers.py +++ b/tests/unit/test_handlers.py @@ -15,6 +15,7 @@ import copy import io import json +import logging import os import pytest @@ -1780,3 +1781,166 @@ def test_remove_bucket_from_url_paths_from_model( ) assert model.http['requestUri'] == request_uri_after assert model.http['authPath'] == auth_path + + +@pytest.fixture() +def operation_model_mock(): + operation_model = mock.Mock() + operation_model.output_shape = mock.Mock() + operation_model.output_shape.members = {'Expires': mock.Mock()} + operation_model.output_shape.members['Expires'].name = 'Expires' + operation_model.output_shape.members['Expires'].serialization = { + 'name': 'Expires' + } + return operation_model + + +@pytest.mark.parametrize( + "expires, expect_expires_header", + [ + # Valid expires values + ("Thu, 01 Jan 2015 00:00:00 GMT", True), + ("10/21/2018", True), + ("01 dec 2100", True), + ("2023-11-02 08:43:04 -0400", True), + ("Sun, 22 Oct 23 00:45:02 UTC", True), + # Invalid expires values + ("Invalid Date", False), + ("access plus 1 month", False), + ("Expires: Thu, 9 Sep 2013 14:19:41 GMT", False), + ("{ts '2023-10-10 09:27:14'}", False), + (-33702800404003370280040400, False), + ], +) +def test_handle_expires_header( + expires, expect_expires_header, operation_model_mock +): + response_dict = { + 'headers': { + 'Expires': expires, + } + } + customized_response_dict = {} + handlers.handle_expires_header( + operation_model_mock, response_dict, customized_response_dict + ) + assert customized_response_dict.get('ExpiresString') == expires + assert ('Expires' in response_dict['headers']) == expect_expires_header + + +def test_handle_expires_header_logs_warning(operation_model_mock, caplog): + response_dict = { + 'headers': { + 'Expires': 'Invalid Date', + } + } + with caplog.at_level(logging.WARNING): + handlers.handle_expires_header(operation_model_mock, response_dict, {}) + assert len(caplog.records) == 1 + assert 'Failed to parse the "Expires" member as a timestamp' in caplog.text + + +def test_handle_expires_header_does_not_log_warning( + operation_model_mock, caplog +): + response_dict = { + 'headers': { + 'Expires': 'Thu, 01 Jan 2015 00:00:00 GMT', + } + } + with caplog.at_level(logging.WARNING): + handlers.handle_expires_header(operation_model_mock, response_dict, {}) + assert len(caplog.records) == 0 + + +@pytest.fixture() +def document_expires_mocks(): + return { + 'section': mock.Mock(), + 'parent': mock.Mock(), + 'param_line': mock.Mock(), + 'param_section': mock.Mock(), + 'doc_section': mock.Mock(), + 'new_param_line': mock.Mock(), + 'new_param_section': mock.Mock(), + 'response_example_event': 'docs.response-example.s3.TestOperation.complete-section', + 'response_params_event': 'docs.response-params.s3.TestOperation.complete-section', + } + + +def test_document_response_example_with_expires(document_expires_mocks): + mocks = document_expires_mocks + mocks['section'].has_section.return_value = True + mocks['section'].get_section.return_value = mocks['parent'] + mocks['parent'].has_section.return_value = True + mocks['parent'].get_section.return_value = mocks['param_line'] + mocks['param_line'].has_section.return_value = True + mocks['param_line'].get_section.return_value = mocks['new_param_line'] + handlers.document_expires_shape( + mocks['section'], mocks['response_example_event'] + ) + mocks['param_line'].add_new_section.assert_called_once_with( + 'ExpiresString' + ) + mocks['new_param_line'].write.assert_called_once_with( + "'ExpiresString': 'string'," + ) + mocks['new_param_line'].style.new_line.assert_called_once() + + +def test_document_response_example_without_expires(document_expires_mocks): + mocks = document_expires_mocks + mocks['section'].has_section.return_value = True + mocks['section'].get_section.return_value = mocks['parent'] + mocks['parent'].has_section.return_value = False + handlers.document_expires_shape( + mocks['section'], mocks['response_example_event'] + ) + mocks['parent'].add_new_section.assert_not_called() + mocks['parent'].get_section.assert_not_called() + mocks['new_param_line'].write.assert_not_called() + + +def test_document_response_params_with_expires(document_expires_mocks): + mocks = document_expires_mocks + mocks['section'].has_section.return_value = True + mocks['section'].get_section.return_value = mocks['param_section'] + mocks['param_section'].get_section.side_effect = [ + mocks['doc_section'], + ] + mocks['param_section'].add_new_section.side_effect = [ + mocks['new_param_section'], + ] + mocks['doc_section'].style = mock.Mock() + mocks['new_param_section'].style = mock.Mock() + handlers.document_expires_shape( + mocks['section'], mocks['response_params_event'] + ) + mocks['param_section'].get_section.assert_any_call('param-documentation') + mocks['doc_section'].style.start_note.assert_called_once() + mocks['doc_section'].write.assert_called_once_with( + 'This member has been deprecated. Please use ``ExpiresString`` instead.' + ) + mocks['doc_section'].style.end_note.assert_called_once() + mocks['param_section'].add_new_section.assert_called_once_with( + 'ExpiresString' + ) + mocks['new_param_section'].style.new_paragraph.assert_any_call() + mocks['new_param_section'].write.assert_any_call( + '- **ExpiresString** *(string) --*' + ) + mocks['new_param_section'].style.indent.assert_called_once() + mocks['new_param_section'].write.assert_any_call( + 'The raw, unparsed value of the ``Expires`` field.' + ) + + +def test_document_response_params_without_expires(document_expires_mocks): + mocks = document_expires_mocks + mocks['section'].has_section.return_value = False + handlers.document_expires_shape( + mocks['section'], mocks['response_params_event'] + ) + mocks['section'].get_section.assert_not_called() + mocks['param_section'].add_new_section.assert_not_called() + mocks['doc_section'].write.assert_not_called()