From 2458e308082ff29ada98b548ee2a69c4ee2e1bc3 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Mon, 23 Dec 2024 17:38:36 +0100 Subject: [PATCH 1/2] Explicitly handle redirects when doing POST requests We Explicitly redo `POST` requests if we get a 301, 302, 307 or 308 status code for the first call. We don't validate that the redirect location has the same domain as the original request, since we already unconditionally follow redirects with the bearer token for GET requests. Note that this wouldn't be necessary if all Lieutenant APIs would redirect us with a 308 for POST requests. --- commodore/helpers.py | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/commodore/helpers.py b/commodore/helpers.py index 1bc0e7dc..ed2d6bb1 100644 --- a/commodore/helpers.py +++ b/commodore/helpers.py @@ -146,7 +146,26 @@ def _lieutenant_request( headers=headers, params=params, timeout=timeout, + # don't let requests handle redirects (usually if the API URL is given without + # https://), since requests will rewrite the method to GET for the redirect which + # makes no sense when we're trying to POST to a POST-only endpoint. + allow_redirects=False, ) + + if r.status_code in [301, 302, 307, 308]: + # Explicitly redo the POST if we get a 301, 302, 307 or 308 status code for the + # first call. We don't validate that the redirect location has the same domain as + # the original request, since we already unconditionally follow redirects with the + # bearer token for GET requests. + # Note that this wouldn't be necessary if all Lieutenant APIs would redirect us with + # a 308 for POST requests. + r = requests.post( + r.headers["location"], + json.dumps(data), + headers=headers, + params=params, + timeout=timeout, + ) else: raise NotImplementedError(f"QueryType {method} not implemented") except ConnectionError as e: From 17243115e5999a7e7ccb6e901ae7b03fd6afb588 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Tue, 24 Dec 2024 11:20:40 +0100 Subject: [PATCH 2/2] Add test case for POST redirect --- tests/test_helpers.py | 44 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/test_helpers.py b/tests/test_helpers.py index 141e9446..ab2cced0 100644 --- a/tests/test_helpers.py +++ b/tests/test_helpers.py @@ -338,6 +338,50 @@ def test_lieutenant_post(request_data, response, expected): _verify_call_status(post_url, token=request_data["token"]) +@responses.activate +def test_lieutenant_post_redirect(): + base_url = "http://syn.example.com/" + redir_url = "https://syn.example.com/" + + post_url = url_normalize(f"{base_url}/clusters/c-cluster-1234/compileMeta") + real_post_url = url_normalize(f"{redir_url}/clusters/c-cluster-1234/compileMeta") + token = "token" + payload = {"some": "data", "other": "data"} + + # successful post response from Lieutenant API has no body + responses.add( + responses.POST, + real_post_url, + content_type="application/json", + status=204, + body=None, + match=[matchers.json_params_matcher(payload)], + ) + responses.add( + responses.POST, + post_url, + content_type="application/json", + headers={"location": real_post_url}, + status=302, + body=None, + match=[matchers.json_params_matcher(payload)], + ) + + helpers.lieutenant_post( + base_url, + token, + "clusters/c-cluster-1234", + "compileMeta", + post_data=payload, + ) + + assert len(responses.calls) == 2 + call_initial = responses.calls[0] + assert call_initial.request.url == post_url + call_redirected = responses.calls[1] + assert call_redirected.request.url == real_post_url + + def test_unimplemented_query_method(): with pytest.raises(NotImplementedError, match="QueryType PATCH not implemented"): helpers._lieutenant_request(