diff --git a/packages/aws-durable-execution-sdk-python-extras/README.md b/packages/aws-durable-execution-sdk-python-extras/README.md index 491f95895..587668ab0 100644 --- a/packages/aws-durable-execution-sdk-python-extras/README.md +++ b/packages/aws-durable-execution-sdk-python-extras/README.md @@ -136,7 +136,7 @@ A step retry costs one durable operation and one more Lambda invocation. Most fa 1. **Inside one step attempt.** The boto3 client retries throttling and server errors of `RunMicrovm`, `CreateMicrovmAuthToken`, and `TerminateMicrovm`. The request step retries the POST itself: connection errors, timeouts, 429, and 5xx, with a delay from 250 milliseconds to 4 seconds. It retries one 401 or 403 with a new auth token. The endpoint answers this way until the MicroVM's `run` hook returns. This tier ends after `retry_window`, or 10 seconds before the invocation times out, whichever comes first. 2. **The step retry strategy.** When the first tier gives up, the step retries after a longer delay, outside the invocation. `default_microvm_retry_strategy` is the default, unless the config sets `retry_strategy`. It retries throttling, server errors, connection failures, timeouts, and the end of the first tier, for at most 5 attempts, with a delay of 2, 4, 8, and 16 seconds. Every other error, such as `ValidationException`, `AccessDeniedException`, or `InsufficientCapacityException`, fails the step at once. -A route that answers another 4xx status fails the operation without a retry, with `MicrovmDeliveryError`. So does a redirect, 3xx. The operation does not follow redirects, and the message names the `Location`. Set the path that the route serves, for example with its trailing slash. The JavaScript operation also fails every 3xx at once, with `MicrovmDeliveryError`. +A route that answers another 4xx status fails the operation without a retry, with `MicrovmDeliveryError`. So does a redirect, 3xx. The operation does not follow redirects. The execution history stores the error message. A query in the `Location` can hold a secret. So the message names the path, and the status of a 3xx answer. It omits the `Location`, and the URL that a followed redirect reached. Set the path that the route serves, for example with its trailing slash. The JavaScript operation also fails every 3xx at once, with `MicrovmDeliveryError`. The POST ignores the proxy settings in the environment, as the JavaScript SDK's `fetch` does. boto3 reads `HTTPS_PROXY`. So in a network that reaches the MicroVM endpoint only through a proxy, the launch succeeds and every POST fails. Pass an `opener` in the config for that network, for example `urllib.request.build_opener(urllib.request.ProxyHandler({"https": "http://proxy.internal:3128"}))`. When an opener follows a redirect, the delivery still fails at once with `MicrovmDeliveryError`. The followed request carries no auth token, because the operation adds its headers as unredirected headers. diff --git a/packages/aws-durable-execution-sdk-python-extras/src/aws_durable_execution_sdk_python_extras/microvm/request.py b/packages/aws-durable-execution-sdk-python-extras/src/aws_durable_execution_sdk_python_extras/microvm/request.py index e562fb387..9a109b9c9 100644 --- a/packages/aws-durable-execution-sdk-python-extras/src/aws_durable_execution_sdk_python_extras/microvm/request.py +++ b/packages/aws-durable-execution-sdk-python-extras/src/aws_durable_execution_sdk_python_extras/microvm/request.py @@ -283,10 +283,14 @@ def send_job(options: SendJobOptions) -> int: The function does not follow a redirect, and neither does the JavaScript operation. A followed 301, 302, or 303 would turn the POST into a GET without the job. A followed 307 or 308 to another host would receive the - auth token. So a redirect fails the delivery at once, with the - ``Location`` in the message. The headers are added as unredirected - headers, so an opener that follows a redirect does not send the auth - token on either. + auth token. So a redirect fails the delivery at once. The headers are + added as unredirected headers, so an opener that follows a redirect does + not send the auth token on either. + + The execution history stores the message. A query in the ``Location`` + can hold a secret, for example a presigned signature. So the message + names the path, and the status of a 3xx answer. It omits the + ``Location``, and the URL that a followed redirect reached. The delay starts at 250 milliseconds and doubles up to 4 seconds. The tier ends at the retry window, or 10 seconds before the invocation times @@ -396,9 +400,9 @@ def new_token() -> Mapping[str, str]: # the job: a 301, 302, or 303 turns the POST into a GET. msg = ( f"The MicroVM answered the job at {options.path} with a " - f"redirect to {answer.location}, and the opener followed it. " - "The operation does not follow redirects, so set the path that " - "the route serves." + "redirect, and the opener followed it. The operation does not " + "follow redirects, so set the path that the route serves, and " + "pass an opener that follows no redirect." ) raise MicrovmRequestRejectedError(msg, status or 0) @@ -429,11 +433,13 @@ def new_token() -> Mapping[str, str]: continue if 300 <= status < 400: # noqa: PLR2004 msg = ( - f"The MicroVM answered the job at {options.path} with HTTP " - f"{status}, a redirect to {answer.location or 'no location'}. " - "The operation does not follow redirects, so set the path " - "that the route serves." + f"The MicroVM answered the job at {options.path} with HTTP {status}" ) + if answer.has_location: + msg += ( + ", a redirect. The operation does not follow redirects, " + "so set the path that the route serves." + ) raise MicrovmRequestRejectedError(msg, status) if 400 <= status < 500 and status != 429: # noqa: PLR2004 msg = ( @@ -535,9 +541,8 @@ class _Answer: """The HTTP status. ``None`` when no answer arrived.""" failure: str = "" """Why no answer arrived.""" - location: str | None = None - """The ``Location`` header of a redirect, or the URL that a followed - redirect reached.""" + has_location: bool = False + """Whether the answer has a ``Location`` header.""" followed_redirect: bool = False """Whether the opener followed a redirect.""" @@ -587,14 +592,12 @@ def _post( with opener.open(request, timeout=timeout) as response: # An opener that follows redirects reports the URL it reached. if response.geturl() != url: - return _Answer( - response.status, location=response.geturl(), followed_redirect=True - ) + return _Answer(response.status, followed_redirect=True) return _Answer(response.status) except urllib.error.HTTPError as error: - location = error.headers.get("Location") if error.headers else None + has_location = bool(error.headers and error.headers.get("Location")) error.close() - return _Answer(error.code, location=location) + return _Answer(error.code, has_location=has_location) except (OSError, http.client.HTTPException, ValueError) as error: # URLError, a refused connection, and a timeout are OSError. An # invalid response from the endpoint raises an HTTPException. diff --git a/packages/aws-durable-execution-sdk-python-extras/tests/conftest.py b/packages/aws-durable-execution-sdk-python-extras/tests/conftest.py index 92f1fb029..480fbbbb2 100644 --- a/packages/aws-durable-execution-sdk-python-extras/tests/conftest.py +++ b/packages/aws-durable-execution-sdk-python-extras/tests/conftest.py @@ -141,11 +141,13 @@ class FakeEndpoint: It records each request, and answers from ``statuses`` in order. After the list ends, it answers 202. A status of ``None`` closes the connection without an answer. A redirect status gets ``Location: ``, - ``/moved`` by default. + ``/moved`` by default. A ``location`` of ``None`` sends no ``Location``. """ def __init__( - self, statuses: list[int | None] | None = None, location: str = "/moved" + self, + statuses: list[int | None] | None = None, + location: str | None = "/moved", ) -> None: self.requests: list[dict[str, Any]] = [] self._statuses = list(statuses or []) @@ -185,7 +187,7 @@ def do_POST(self) -> None: # noqa: N802 - the http.server name self.connection.shutdown(socket.SHUT_RDWR) return self.send_response(status) - if 300 <= status < 400: # noqa: PLR2004 + if 300 <= status < 400 and location is not None: # noqa: PLR2004 self.send_header("location", location) self.send_header("content-length", "0") self.end_headers() @@ -211,7 +213,7 @@ def fake_endpoint() -> Iterator[Callable[..., FakeEndpoint]]: endpoints: list[FakeEndpoint] = [] def _create( - statuses: list[int | None] | None = None, location: str = "/moved" + statuses: list[int | None] | None = None, location: str | None = "/moved" ) -> FakeEndpoint: endpoint = FakeEndpoint(statuses, location) endpoints.append(endpoint) diff --git a/packages/aws-durable-execution-sdk-python-extras/tests/microvm/request_test.py b/packages/aws-durable-execution-sdk-python-extras/tests/microvm/request_test.py index 638d651d0..6efeb8e62 100644 --- a/packages/aws-durable-execution-sdk-python-extras/tests/microvm/request_test.py +++ b/packages/aws-durable-execution-sdk-python-extras/tests/microvm/request_test.py @@ -383,13 +383,57 @@ def test_rejects_a_redirect_without_following_it( assert raised.value.status == status assert str(raised.value) == ( f"The MicroVM answered the job at /durable-execution/v1/job with HTTP " - f"{status}, a redirect to /moved. The operation does not follow " - "redirects, so set the path that the route serves." + f"{status}, a redirect. The operation does not follow redirects, so set " + "the path that the route serves." ) # A followed 301, 302, or 303 would arrive as a GET without the job. assert [r["path"] for r in endpoint.requests] == ["/durable-execution/v1/job"] +@pytest.mark.parametrize("status", [302, 304]) +def test_rejects_a_3xx_without_a_location( + fake_client: Any, fake_endpoint: Any, status: int +) -> None: + endpoint = fake_endpoint([status], location=None) + + with pytest.raises(MicrovmRequestRejectedError) as raised: + send_job(options(fake_client(), endpoint.url, [])) + + assert raised.value.status == status + assert str(raised.value) == ( + f"The MicroVM answered the job at /durable-execution/v1/job with HTTP {status}" + ) + + +def test_rejects_a_3xx_with_an_empty_location( + fake_client: Any, fake_endpoint: Any +) -> None: + # An empty Location names no target. So it counts as no Location. + endpoint = fake_endpoint([302], location="") + + with pytest.raises(MicrovmRequestRejectedError) as raised: + send_job(options(fake_client(), endpoint.url, [])) + + assert raised.value.status == 302 + assert str(raised.value) == ( + "The MicroVM answered the job at /durable-execution/v1/job with HTTP 302" + ) + + +def test_omits_the_location_of_a_redirect_from_the_message( + fake_client: Any, fake_endpoint: Any +) -> None: + # The execution history stores the message. A query in the Location can + # hold a secret. So the message must not contain any part of it. + endpoint = fake_endpoint([302], location="https://example.com/x?token=secret") + + with pytest.raises(MicrovmRequestRejectedError) as raised: + send_job(options(fake_client(), endpoint.url, [])) + + assert "example.com" not in str(raised.value) + assert "secret" not in str(raised.value) + + @pytest.mark.parametrize( "path", ["/clone%20build", "/caf%C3%A9", "/a/b-c_d.e~f!$&'()*+,;=:@", "/a/"] ) @@ -439,9 +483,9 @@ def test_rejects_a_redirect_that_the_callers_opener_followed( assert str(raised.value) == ( "The MicroVM answered the job at /durable-execution/v1/job with a " - f"redirect to {endpoint.url}/moved, and the opener followed it. The " - "operation does not follow redirects, so set the path that the route " - "serves." + "redirect, and the opener followed it. The operation does not follow " + "redirects, so set the path that the route serves, and pass an opener " + "that follows no redirect." ) assert [r["path"] for r in endpoint.requests] == [ "/durable-execution/v1/job", @@ -449,6 +493,25 @@ def test_rejects_a_redirect_that_the_callers_opener_followed( ] +def test_omits_the_followed_url_from_the_message( + fake_client: Any, fake_endpoint: Any +) -> None: + # The opener follows the 302 to the URL with the query. The message must + # not contain that URL. + endpoint = fake_endpoint([302], location="/x?token=secret") + + with pytest.raises(MicrovmRequestRejectedError) as raised: + send_job( + options( + fake_client(), endpoint.url, [], opener=urllib.request.build_opener() + ) + ) + + assert endpoint.requests[-1]["path"] == "/x?token=secret" + assert "?token=" not in str(raised.value) + assert "secret" not in str(raised.value) + + def test_builds_the_default_opener_once() -> None: assert request.default_opener() is request.default_opener()