From 11a7534025cf112b9588270e70af904255f3bc71 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 3 Aug 2026 20:19:13 -0700 Subject: [PATCH] fix: preserve POST body across 3xx redirects (#1127, #1828) `requests` follows 301/302/303 by converting POST to GET and dropping the request body. Any TSC write hitting a server behind a redirect (users.add, workbooks.publish, addusers, etc.) returned 405 Method Not Allowed because the server saw a GET where it expected a POST. Disable requests' auto-redirect and walk the chain manually in Endpoint._make_request, keeping the original method and body across every hop. Hop count bounded by session.max_redirects (default 30, same as requests). Also close two nearby gaps: - Refuse HTTPS -> HTTP scheme downgrades. Silently following them would send auth material over plaintext; no legitimate server behaviour requires this. Raises RedirectError with the original and target URLs. - Raise RedirectError (with URL, method, status code) when a 3xx response has no Location header, replacing the bare KeyError('location') that requests emits deep in its internals. Sign-in retains its own single-hop 301 handler in auth_endpoint.py for backwards compatibility; the new path is additive. Test coverage: 8 new tests in test_redirect_handling.py covering POST body preservation, multi-hop chains, relative Location headers, scheme downgrade refusal, missing Location, and hop-cap enforcement. Existing 866-test suite unchanged. Fixes #1127. Fixes #1828. --- CHANGELOG.md | 8 + .../server/endpoint/endpoint.py | 63 ++++++ .../server/endpoint/exceptions.py | 7 + test/test_redirect_handling.py | 192 ++++++++++++++++++ 4 files changed, 270 insertions(+) create mode 100644 test/test_redirect_handling.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 943436b27..1a9173680 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ hierarchy path (e.g. `"Marketing/Q1 Reports"`). The walk is performed level by level using the REST API name filter, so a path with *n* components issues *n* requests. Returns the matching `ProjectItem` or `None` if no project is found. +* Preserve HTTP method and body across 3xx redirects. Previously `requests` + followed 301/302/303 by converting POST to GET and dropping the body, so + endpoints like `users.add`, `workbooks.publish`, and any write hitting a + server behind a redirect would 405. TSC now disables `requests`'s + auto-redirect and walks the chain manually, up to `session.max_redirects` + hops (default 30). Refuses HTTPS -> HTTP scheme downgrades and raises + `RedirectError` with a clear message on missing `Location` headers or hop + overflow. Fixes #1127 and #1828. ## 0.18.0 (6 April 2022) * Switched to using defused_xml for xml attack protection diff --git a/tableauserverclient/server/endpoint/endpoint.py b/tableauserverclient/server/endpoint/endpoint.py index 31a0806dc..58529dde0 100644 --- a/tableauserverclient/server/endpoint/endpoint.py +++ b/tableauserverclient/server/endpoint/endpoint.py @@ -3,6 +3,7 @@ import os from contextlib import closing from typing_extensions import Concatenate, ParamSpec +from urllib.parse import urljoin, urlparse from tableauserverclient import datetime_helpers as datetime import abc @@ -30,6 +31,7 @@ InternalServerError, NonXMLResponseError, NotSignedInError, + RedirectError, ) from tableauserverclient.server.exceptions import EndpointUnavailableError @@ -45,6 +47,13 @@ Success_codes = [200, 201, 202, 204] +# 301/302/303/307/308 all indicate the caller should re-request at a new URL. +# `requests`' default handler converts POST -> GET on 301/302/303, which drops +# the POST body and breaks sign-in / addusers / publish / any write endpoint +# whose target sits behind a redirect. We disable that and walk the chain +# manually, keeping the original method and body across every hop. +Redirect_codes = [301, 302, 303, 307, 308] + XML_CONTENT_TYPE = "text/xml" JSON_CONTENT_TYPE = "application/json" @@ -120,6 +129,11 @@ def _make_request( parameters = Endpoint.set_parameters( self.parent_srv.http_options, auth_token, content, content_type, parameters ) + # Manual redirect handling: see Redirect_codes comment. `requests` + # follows 301/302/303 by converting POST to GET (RFC-conforming but + # loses the body). We disable it here and re-issue the same method + # ourselves in _follow_redirect_if_any. + parameters["allow_redirects"] = False logger.debug(f"request method {method.__name__}, url: {url}") if content: @@ -144,6 +158,7 @@ def _make_request( raise RuntimeError if isinstance(server_response, Exception): raise server_response + server_response, url = self._follow_redirect_if_any(method, url, parameters, server_response) self._check_status(server_response, url) loggable_response = self.log_response_safely(server_response) @@ -157,6 +172,54 @@ def _make_request( return server_response + def _follow_redirect_if_any( + self, + method: Callable[..., "Response"], + url: str, + parameters: dict[str, Any], + server_response: "Response", + ) -> tuple["Response", str]: + # Walk a 301/302/303/307/308 chain up to session.max_redirects hops, + # preserving method and body. Rejects HTTPS -> HTTP scheme downgrades + # (silent security regression). Raises RedirectError on a missing + # Location header instead of the KeyError requests emits deep in its + # internals, and on exceeding the session hop limit. + try: + max_hops = int(self.parent_srv.session.max_redirects) + except (AttributeError, TypeError): + max_hops = 30 # requests' library default + current_url = url + response = server_response + for hop in range(max_hops): + if response.status_code not in Redirect_codes: + return response, current_url + location = response.headers.get("Location") + if not location: + raise RedirectError( + f"{method.__name__.upper()} {current_url} returned HTTP {response.status_code} " + f"without a Location header; can't follow the redirect." + ) + # Support relative Locations per RFC 7231. + next_url = urljoin(current_url, location) + if urlparse(current_url).scheme == "https" and urlparse(next_url).scheme == "http": + raise RedirectError( + f"Refusing to follow redirect from {current_url} to {next_url}: " + f"HTTPS -> HTTP scheme downgrade would send request data over plaintext." + ) + logger.debug(f"Following {response.status_code} redirect: {current_url} -> {next_url}") + current_url = next_url + next_response = self._blocking_request(method, current_url, parameters) + if next_response is None: + raise RuntimeError(f"No response after redirect to {current_url}") + if isinstance(next_response, Exception): + raise next_response + response = next_response + # Still a redirect after max_hops hops -> loop / misconfiguration. + raise RedirectError( + f"Exceeded {max_hops} redirect hops starting from {url}; last Location was {current_url}. " + f"Increase session.max_redirects if this is legitimate." + ) + def _check_status(self, server_response: "Response", url: str | None = None): logger.debug(f"Response status: {server_response}") if not hasattr(server_response, "status_code"): diff --git a/tableauserverclient/server/endpoint/exceptions.py b/tableauserverclient/server/endpoint/exceptions.py index 49e065ed3..2a94a2969 100644 --- a/tableauserverclient/server/endpoint/exceptions.py +++ b/tableauserverclient/server/endpoint/exceptions.py @@ -130,3 +130,10 @@ class FlowRunCancelledException(FlowRunFailedException): class UnsupportedAttributeError(TableauError): pass + + +class RedirectError(TableauError): + # Raised when a manual redirect can't be followed safely or at all. + # Cases: missing Location header, HTTPS -> HTTP downgrade, redirect loop + # exceeding session.max_redirects. See Endpoint._follow_redirect_if_any. + pass diff --git a/test/test_redirect_handling.py b/test/test_redirect_handling.py new file mode 100644 index 000000000..877682ae9 --- /dev/null +++ b/test/test_redirect_handling.py @@ -0,0 +1,192 @@ +"""Tests for manual redirect handling in Endpoint._make_request. + +`requests` follows 301/302/303 by converting POST to GET (dropping the body). +We disable auto-redirect and re-issue the same method ourselves in +Endpoint._follow_redirect_if_any. These tests cover the resulting behavior: + +- POST body preserved across a redirect +- multi-hop chains +- HTTPS -> HTTP scheme downgrade refused +- missing Location header raises RedirectError +- exceeding session.max_redirects raises RedirectError +- GET redirects still work +""" + +from pathlib import Path + +import pytest +import requests_mock + +import tableauserverclient as TSC +from tableauserverclient.server.endpoint.exceptions import RedirectError + +TEST_ASSET_DIR = Path(__file__).parent / "assets" +SIGN_IN_XML = TEST_ASSET_DIR / "auth_sign_in.xml" + + +@pytest.fixture +def server() -> TSC.Server: + return TSC.Server("http://test", False) + + +@pytest.fixture +def signed_in_server() -> TSC.Server: + s = TSC.Server("http://test", False) + s._set_auth("site-id", "user-id", "auth-token", "") + return s + + +def _sign_in_xml() -> str: + with open(SIGN_IN_XML, "rb") as f: + return f.read().decode("utf-8") + + +def test_post_body_preserved_across_redirect(signed_in_server: TSC.Server) -> None: + # Regression for tableau/tabcmd#309: POST -> 302 previously turned into GET + # and dropped the request body. Verify the body reaches the final URL intact. + seen_bodies: list[bytes | None] = [] + + def record(request, context): + seen_bodies.append(request.body) + context.status_code = 200 + return b"" + + with requests_mock.mock() as m: + m.post("http://test/redirect-from", status_code=302, headers={"Location": "http://test/redirect-to"}) + m.post("http://test/redirect-to", content=record) + + resp = signed_in_server.session.post( + "http://test/redirect-from", + data=b"payload=1", + allow_redirects=False, + ) + # The Endpoint layer, not the raw session, is what re-issues. Route + # through _make_request so we exercise the code under test. + from tableauserverclient.server.endpoint.endpoint import Endpoint + + endpoint = Endpoint(signed_in_server) + final, url = endpoint._follow_redirect_if_any( + signed_in_server.session.post, + "http://test/redirect-from", + {"data": b"payload=1", "allow_redirects": False}, + resp, + ) + + assert final.status_code == 200 + assert url == "http://test/redirect-to" + assert seen_bodies == [b"payload=1"], seen_bodies + + +def test_multi_hop_redirect_chain(signed_in_server: TSC.Server) -> None: + with requests_mock.mock() as m: + m.post("http://test/a", status_code=301, headers={"Location": "http://test/b"}) + m.post("http://test/b", status_code=302, headers={"Location": "http://test/c"}) + m.post("http://test/c", status_code=200, text="") + + resp = signed_in_server.session.post("http://test/a", allow_redirects=False) + from tableauserverclient.server.endpoint.endpoint import Endpoint + + endpoint = Endpoint(signed_in_server) + final, url = endpoint._follow_redirect_if_any( + signed_in_server.session.post, "http://test/a", {"allow_redirects": False}, resp + ) + + assert final.status_code == 200 + assert url == "http://test/c" + + +def test_relative_location_header(signed_in_server: TSC.Server) -> None: + # RFC 7231 allows relative Location values; join them against the request URL. + with requests_mock.mock() as m: + m.post("http://test/api/v1/thing", status_code=302, headers={"Location": "/api/v2/thing"}) + m.post("http://test/api/v2/thing", status_code=200, text="") + + resp = signed_in_server.session.post("http://test/api/v1/thing", allow_redirects=False) + from tableauserverclient.server.endpoint.endpoint import Endpoint + + endpoint = Endpoint(signed_in_server) + final, url = endpoint._follow_redirect_if_any( + signed_in_server.session.post, "http://test/api/v1/thing", {"allow_redirects": False}, resp + ) + + assert final.status_code == 200 + assert url == "http://test/api/v2/thing" + + +def test_https_to_http_downgrade_rejected() -> None: + # HTTPS -> HTTP redirect is never legitimate: quietly following it would + # send auth material over plaintext. Refuse and surface a clear error. + s = TSC.Server("https://secure.test", False) + s._set_auth("site-id", "user-id", "auth-token", "") + + with requests_mock.mock() as m: + m.post("https://secure.test/signin", status_code=301, headers={"Location": "http://insecure.test/signin"}) + resp = s.session.post("https://secure.test/signin", allow_redirects=False) + from tableauserverclient.server.endpoint.endpoint import Endpoint + + endpoint = Endpoint(s) + with pytest.raises(RedirectError, match="HTTPS -> HTTP"): + endpoint._follow_redirect_if_any( + s.session.post, "https://secure.test/signin", {"allow_redirects": False}, resp + ) + + +def test_missing_location_header_raises_redirecterror(signed_in_server: TSC.Server) -> None: + # `requests`' internal resolve_redirects raises KeyError('location') with no + # context. We raise RedirectError with the URL, method, and status code. + with requests_mock.mock() as m: + m.post("http://test/broken", status_code=302) # no Location header + resp = signed_in_server.session.post("http://test/broken", allow_redirects=False) + from tableauserverclient.server.endpoint.endpoint import Endpoint + + endpoint = Endpoint(signed_in_server) + with pytest.raises(RedirectError, match="without a Location header"): + endpoint._follow_redirect_if_any( + signed_in_server.session.post, "http://test/broken", {"allow_redirects": False}, resp + ) + + +def test_redirect_loop_hits_max_hops(signed_in_server: TSC.Server) -> None: + signed_in_server.session.max_redirects = 3 + with requests_mock.mock() as m: + m.post("http://test/loop", status_code=302, headers={"Location": "http://test/loop"}) + resp = signed_in_server.session.post("http://test/loop", allow_redirects=False) + from tableauserverclient.server.endpoint.endpoint import Endpoint + + endpoint = Endpoint(signed_in_server) + with pytest.raises(RedirectError, match="Exceeded 3 redirect hops"): + endpoint._follow_redirect_if_any( + signed_in_server.session.post, "http://test/loop", {"allow_redirects": False}, resp + ) + + +def test_non_redirect_response_passes_through(signed_in_server: TSC.Server) -> None: + # 200 stays 200; the helper is a no-op for non-3xx. + with requests_mock.mock() as m: + m.post("http://test/ok", status_code=200, text="") + resp = signed_in_server.session.post("http://test/ok", allow_redirects=False) + from tableauserverclient.server.endpoint.endpoint import Endpoint + + endpoint = Endpoint(signed_in_server) + final, url = endpoint._follow_redirect_if_any( + signed_in_server.session.post, "http://test/ok", {"allow_redirects": False}, resp + ) + + assert final.status_code == 200 + assert url == "http://test/ok" + + +def test_sign_in_after_redirect(server: TSC.Server) -> None: + # Integration-style: real sign-in flow across a redirect. Verifies that + # auth_endpoint's existing manual-redirect-of-signin still works alongside + # the generic _make_request redirect handling. + xml = _sign_in_xml() + with requests_mock.mock() as m: + m.post( + server.auth.baseurl + "/signin", status_code=301, headers={"Location": "http://test/api/3.6/auth/signin"} + ) + m.post("http://test/api/3.6/auth/signin", text=xml) + tableau_auth = TSC.TableauAuth("u", "p", site_id="Samples") + server.auth.sign_in(tableau_auth) + + assert server.auth_token is not None