From 11a7534025cf112b9588270e70af904255f3bc71 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 3 Aug 2026 20:19:13 -0700 Subject: [PATCH 1/3] 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 From b0e3e6240b95fa4291f0dd3b8f61087a3a15d708 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Thu, 6 Aug 2026 12:24:22 -0700 Subject: [PATCH 2/3] Address #1848 review: fix max_redirects=0, unify signin with base redirect handling, restructure tests Fixes from Claude review pass: 1. `_follow_redirect_if_any`: move the "not a redirect?" early-return outside the loop, so a 200 response returns immediately even when session.max_redirects=0 (previously fell straight to "Exceeded 0 redirect hops" error). Also switch to getattr(method, "__name__", "REQUEST") to survive functools.partial or other callable wrappers. 2. `auth_endpoint.sign_in`: replace the inline session.post + 301 handler with `_make_request`, so signin now inherits multi-hop chain support, the HTTPS -> HTTP scheme guard, the missing-Location diagnostic, and the hop limit. This resolves the divergent behavior between signin and every other endpoint (signin previously refused to follow 302 and had no security guards). 3. `test_redirect_handling.py`: rewrite all tests to drive real endpoint calls (`server.auth.sign_in`, `server.workbooks.get`) through `requests_mock`, exercising `_make_request` end-to-end rather than calling `_follow_redirect_if_any` in isolation. Add parametrized coverage for all 5 followed redirect codes (301/302/303/307/308) and the 4 non-followed ones (300/304/305/306). Add tests for header preservation (X-Tableau-Auth reaches the redirect target), HTTP->HTTPS upgrade allowed, cross-host redirect followed, second-hop HTTPS->HTTP downgrade caught, and max_redirects=1 error path. Document why max_redirects=0 isn't tested (`requests` refuses to complete any 3xx response when max_redirects=0, regardless of `allow_redirects`, so the response never reaches our code). Full test suite: 888 passed, 1 skipped. --- .../server/endpoint/auth_endpoint.py | 26 +- .../server/endpoint/endpoint.py | 15 +- test/test_redirect_handling.py | 307 +++++++++++------- 3 files changed, 216 insertions(+), 132 deletions(-) diff --git a/tableauserverclient/server/endpoint/auth_endpoint.py b/tableauserverclient/server/endpoint/auth_endpoint.py index fe0c9b3da..110d38a18 100644 --- a/tableauserverclient/server/endpoint/auth_endpoint.py +++ b/tableauserverclient/server/endpoint/auth_endpoint.py @@ -4,7 +4,7 @@ from defusedxml.ElementTree import fromstring -from tableauserverclient.server.endpoint.endpoint import Endpoint, api +from tableauserverclient.server.endpoint.endpoint import Endpoint, XML_CONTENT_TYPE, api from tableauserverclient.server.endpoint.exceptions import ServerResponseError from tableauserverclient.server.request_factory import RequestFactory @@ -68,20 +68,18 @@ def sign_in(self, auth_req: "Credentials") -> contextmgr: """ url = f"{self.baseurl}/signin" signin_req = RequestFactory.Auth.signin_req(auth_req) - server_response = self.parent_srv.session.post( - url, data=signin_req, **self.parent_srv.http_options, allow_redirects=False + # Route through _make_request so signin gets the same redirect handling + # (multi-hop, HTTPS->HTTP scheme guard, missing-Location diagnostic, + # hop limit) that every other endpoint uses. Explicit auth_token=None + # because we don't have one yet -- and self.parent_srv.auth_token + # raises NotSignedInError pre-signin, so post_request can't help here. + server_response = self._make_request( + self.parent_srv.session.post, + url, + content=signin_req, + auth_token=None, + content_type=XML_CONTENT_TYPE, ) - # manually handle a redirect so that we send the correct POST request instead of GET - # this will make e.g http://online.tableau.com work to redirect to http://east.online.tableau.com - if server_response.status_code == 301: - server_response = self.parent_srv.session.post( - server_response.headers["Location"], - data=signin_req, - **self.parent_srv.http_options, - allow_redirects=False, - ) - self.parent_srv._namespace.detect(server_response.content) - self._check_status(server_response, url) parsed_response = fromstring(server_response.content) site_id = parsed_response.find(".//t:site", namespaces=self.parent_srv.namespace).get("id", None) site_url = parsed_response.find(".//t:site", namespaces=self.parent_srv.namespace).get("contentUrl", None) diff --git a/tableauserverclient/server/endpoint/endpoint.py b/tableauserverclient/server/endpoint/endpoint.py index 58529dde0..9bf190125 100644 --- a/tableauserverclient/server/endpoint/endpoint.py +++ b/tableauserverclient/server/endpoint/endpoint.py @@ -190,13 +190,15 @@ def _follow_redirect_if_any( 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 + # Not a redirect? Return immediately regardless of max_hops (including 0). + if response.status_code not in Redirect_codes: + return response, current_url + method_name = getattr(method, "__name__", "REQUEST").upper() + for _ in range(max_hops): location = response.headers.get("Location") if not location: raise RedirectError( - f"{method.__name__.upper()} {current_url} returned HTTP {response.status_code} " + f"{method_name} {current_url} returned HTTP {response.status_code} " f"without a Location header; can't follow the redirect." ) # Support relative Locations per RFC 7231. @@ -212,8 +214,13 @@ def _follow_redirect_if_any( if next_response is None: raise RuntimeError(f"No response after redirect to {current_url}") if isinstance(next_response, Exception): + # _blocking_request already re-raises via except -> raise, so this + # branch is defensive; keep it to satisfy the Response|Exception|None + # return type. raise next_response response = next_response + if response.status_code not in Redirect_codes: + return response, current_url # 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}. " diff --git a/test/test_redirect_handling.py b/test/test_redirect_handling.py index 877682ae9..d1d826da0 100644 --- a/test/test_redirect_handling.py +++ b/test/test_redirect_handling.py @@ -1,15 +1,10 @@ """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 +We disable auto-redirect and re-issue the same method ourselves inside +`Endpoint._make_request`. These tests drive real endpoint calls (sign_in, +workbooks.get, etc.) through a `requests_mock` transport, so they exercise +the same code path production traffic takes -- not the helper in isolation. """ from pathlib import Path @@ -22,171 +17,255 @@ TEST_ASSET_DIR = Path(__file__).parent / "assets" SIGN_IN_XML = TEST_ASSET_DIR / "auth_sign_in.xml" +GET_XML = TEST_ASSET_DIR / "workbook_get.xml" @pytest.fixture def server() -> TSC.Server: - return TSC.Server("http://test", False) + s = TSC.Server("http://test", False) + return s @pytest.fixture def signed_in_server() -> TSC.Server: s = TSC.Server("http://test", False) + s.version = "3.10" 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") + return SIGN_IN_XML.read_text() + + +def _workbooks_get_xml() -> str: + return GET_XML.read_text() + +# --- Body / method preservation --------------------------------------------- -def test_post_body_preserved_across_redirect(signed_in_server: TSC.Server) -> None: + +def test_post_body_preserved_across_redirect(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. + # and dropped the body. Sign-in is the load-bearing POST path; drive it + # end-to-end and verify (a) the body reaches the final URL intact, and + # (b) sign_in still parses the response and sets auth state. + xml = _sign_in_xml() seen_bodies: list[bytes | None] = [] - def record(request, context): + def record_final(request, context): seen_bodies.append(request.body) context.status_code = 200 - return b"" + return xml 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, + m.post( + server.auth.baseurl + "/signin", + status_code=302, + headers={"Location": "http://test/api/3.6/auth/signin"}, ) + m.post("http://test/api/3.6/auth/signin", text=record_final) - assert final.status_code == 200 - assert url == "http://test/redirect-to" - assert seen_bodies == [b"payload=1"], seen_bodies + tableau_auth = TSC.TableauAuth("u", "p", site_id="Samples") + server.auth.sign_in(tableau_auth) + + assert server.auth_token is not None, "sign_in did not complete" + assert len(seen_bodies) == 1 + assert seen_bodies[0] is not None + assert b" None: +def test_post_body_preserved_across_multi_hop_chain(server: TSC.Server) -> None: + xml = _sign_in_xml() with requests_mock.mock() as m: - m.post("http://test/a", status_code=301, headers={"Location": "http://test/b"}) + m.post(server.auth.baseurl + "/signin", 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="") + m.post("http://test/c", status_code=303, headers={"Location": "http://test/d"}) + m.post("http://test/d", text=xml) - resp = signed_in_server.session.post("http://test/a", allow_redirects=False) - from tableauserverclient.server.endpoint.endpoint import Endpoint + server.auth.sign_in(TSC.TableauAuth("u", "p")) + + assert server.auth_token is not None - 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_headers_survive_redirect(signed_in_server: TSC.Server) -> None: + # Regression: the whole point of the PR is method+body+*headers* + # preservation. Verify X-Tableau-Auth reaches the redirect target. + seen_headers: list[dict] = [] + def capture(request, context): + seen_headers.append(dict(request.headers)) + context.status_code = 200 + return _workbooks_get_xml() -def test_relative_location_header(signed_in_server: TSC.Server) -> None: - # RFC 7231 allows relative Location values; join them against the request URL. + baseurl = signed_in_server.workbooks.baseurl 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="") + m.get(baseurl, status_code=302, headers={"Location": baseurl + "?redirected=1"}) + m.get(baseurl + "?redirected=1", text=capture) + signed_in_server.workbooks.get() - resp = signed_in_server.session.post("http://test/api/v1/thing", allow_redirects=False) - from tableauserverclient.server.endpoint.endpoint import Endpoint + # Last hop was the terminal 200 -- inspect its headers. + assert seen_headers, "final GET never fired" + final = seen_headers[-1] + assert final.get("x-tableau-auth") == "auth-token" or final.get("X-Tableau-Auth") == "auth-token", final - 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_get_redirect_still_works(signed_in_server: TSC.Server) -> None: + baseurl = signed_in_server.workbooks.baseurl + with requests_mock.mock() as m: + m.get(baseurl, status_code=301, headers={"Location": baseurl + "?v=2"}) + m.get(baseurl + "?v=2", text=_workbooks_get_xml()) + result = signed_in_server.workbooks.get() + assert result[0] is not None -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", "") +# --- Redirect codes --------------------------------------------------------- - 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 - ) +@pytest.mark.parametrize("code", [301, 302, 303, 307, 308]) +def test_all_supported_redirect_codes_preserve_post_body(server: TSC.Server, code: int) -> None: + xml = _sign_in_xml() + seen_bodies: list[bytes | None] = [] + def capture(request, context): + seen_bodies.append(request.body) + context.status_code = 200 + return xml -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 + m.post(server.auth.baseurl + "/signin", status_code=code, headers={"Location": "http://test/new"}) + m.post("http://test/new", text=capture) + server.auth.sign_in(TSC.TableauAuth("u", "p")) - 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 - ) + assert len(seen_bodies) == 1 + assert seen_bodies[0] is not None + assert b" None: - signed_in_server.session.max_redirects = 3 +@pytest.mark.parametrize("code", [300, 304, 305, 306]) +def test_non_followed_3xx_codes_pass_through(signed_in_server: TSC.Server, code: int) -> None: + # Only 301/302/303/307/308 are in Redirect_codes. Others should reach + # _check_status unchanged and surface as ServerResponseError or similar. + baseurl = signed_in_server.workbooks.baseurl 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 + m.get( + baseurl, + status_code=code, + text="xy", + ) + with pytest.raises((TSC.ServerResponseError, Exception)): + signed_in_server.workbooks.get() - 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 - ) +# --- Scheme handling -------------------------------------------------------- -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 +def test_https_to_http_downgrade_rejected() -> None: + s = TSC.Server("https://secure.test", False) + with requests_mock.mock() as m: + m.post( + s.auth.baseurl + "/signin", + status_code=301, + headers={"Location": "http://insecure.test/api/3.6/auth/signin"}, ) + with pytest.raises(RedirectError, match="HTTPS -> HTTP"): + s.auth.sign_in(TSC.TableauAuth("u", "p")) + - assert final.status_code == 200 - assert url == "http://test/ok" +def test_https_to_http_downgrade_rejected_on_later_hop() -> None: + # First hop is https->https (safe), second hop tries to downgrade. + # Regression coverage that the guard runs each iteration, not just once. + s = TSC.Server("https://a.test", False) + with requests_mock.mock() as m: + m.post(s.auth.baseurl + "/signin", status_code=301, headers={"Location": "https://b.test/signin"}) + m.post("https://b.test/signin", status_code=301, headers={"Location": "http://c.test/signin"}) + with pytest.raises(RedirectError, match="HTTPS -> HTTP"): + s.auth.sign_in(TSC.TableauAuth("u", "p")) -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. +def test_http_to_https_upgrade_allowed(server: TSC.Server) -> None: + # Not a security concern -- the whole point of #309 is that + # http://.../signin -> https://.../signin should work. 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"} + server.auth.baseurl + "/signin", status_code=301, headers={"Location": "https://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) + m.post("https://test/api/3.6/auth/signin", text=xml) + server.auth.sign_in(TSC.TableauAuth("u", "p")) + assert server.auth_token is not None + + +def test_cross_host_redirect_followed(server: TSC.Server) -> None: + # e.g. http://online.tableau.com -> http://east.online.tableau.com. + # This is the scenario in the original inline signin comment. + xml = _sign_in_xml() + with requests_mock.mock() as m: + m.post( + server.auth.baseurl + "/signin", + status_code=301, + headers={"Location": "http://east.test/api/3.6/auth/signin"}, + ) + m.post("http://east.test/api/3.6/auth/signin", text=xml) + server.auth.sign_in(TSC.TableauAuth("u", "p")) + assert server.auth_token is not None + + +# --- Location edge cases ---------------------------------------------------- + +def test_relative_location_header(server: TSC.Server) -> None: + # RFC 7231 allows relative Location values; urljoin against request URL. + xml = _sign_in_xml() + with requests_mock.mock() as m: + m.post(server.auth.baseurl + "/signin", status_code=302, headers={"Location": "/api/3.6/auth/signin"}) + m.post("http://test/api/3.6/auth/signin", text=xml) + server.auth.sign_in(TSC.TableauAuth("u", "p")) assert server.auth_token is not None + + +def test_missing_location_header_raises_redirecterror(server: TSC.Server) -> None: + # `requests`' internal resolve_redirects raises KeyError('location') with no + # context. We raise RedirectError with URL, method, status code. + with requests_mock.mock() as m: + m.post(server.auth.baseurl + "/signin", status_code=302) # no Location header + with pytest.raises(RedirectError, match="without a Location header"): + server.auth.sign_in(TSC.TableauAuth("u", "p")) + + +# --- Hop limits ------------------------------------------------------------- + + +def test_redirect_loop_hits_max_hops(server: TSC.Server) -> None: + server.session.max_redirects = 3 + with requests_mock.mock() as m: + m.post(server.auth.baseurl + "/signin", status_code=302, headers={"Location": "http://test/loop"}) + m.post("http://test/loop", status_code=302, headers={"Location": "http://test/loop"}) + with pytest.raises(RedirectError, match="Exceeded 3 redirect hops"): + server.auth.sign_in(TSC.TableauAuth("u", "p")) + + +def test_max_redirects_zero_passes_non_redirect_response(signed_in_server: TSC.Server) -> None: + # Regression for the review finding: with the loop bounded by + # `range(max_hops)`, max_redirects=0 previously fell straight into the + # "exceeded" error even for a 200 response. + signed_in_server.session.max_redirects = 0 + baseurl = signed_in_server.workbooks.baseurl + with requests_mock.mock() as m: + m.get(baseurl, text=_workbooks_get_xml()) + result = signed_in_server.workbooks.get() + assert result[0] is not None + + +def test_max_redirects_one_rejects_second_hop(signed_in_server: TSC.Server) -> None: + # max_redirects=1 allows one non-redirect response but errors on a second + # 3xx. (max_redirects=0 is not tested because `requests` itself refuses to + # complete any request that returns 3xx when max_redirects=0, regardless of + # allow_redirects; the response never reaches _make_request.) + signed_in_server.session.max_redirects = 1 + baseurl = signed_in_server.workbooks.baseurl + with requests_mock.mock() as m: + m.get(baseurl, status_code=302, headers={"Location": baseurl + "?v=2"}) + m.get(baseurl + "?v=2", status_code=302, headers={"Location": baseurl + "?v=3"}) + with pytest.raises(RedirectError, match="Exceeded 1 redirect hops"): + signed_in_server.workbooks.get() From 8ec8ed94ddc2effd6d354435f09e1125ae8550b6 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Fri, 14 Aug 2026 16:46:39 -0700 Subject: [PATCH 3/3] Promote server address to https on http->https redirect When the server redirects http://host to https://host on the same host, update `server._server_address` so subsequent requests skip the redirect round-trip. Recovers an older idea from the abandoned `jac/handle-https-better` branch, now that the manual-redirect handler from #1848 provides the right hook point. Only rewrites the stored address when: - current scheme is http, next scheme is https (upgrade, not downgrade which is already refused above) - current and next netloc match (same host, just scheme change) -- avoids the failure mode where a redirect to a completely unrelated https server silently repoints every future call at it. Two tests: one verifies the address is promoted on a same-host http->https redirect, the other verifies it is NOT promoted on a cross-host redirect. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../server/endpoint/endpoint.py | 17 +++++++++- test/test_redirect_handling.py | 31 +++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/tableauserverclient/server/endpoint/endpoint.py b/tableauserverclient/server/endpoint/endpoint.py index 9bf190125..e391941f4 100644 --- a/tableauserverclient/server/endpoint/endpoint.py +++ b/tableauserverclient/server/endpoint/endpoint.py @@ -203,11 +203,26 @@ def _follow_redirect_if_any( ) # Support relative Locations per RFC 7231. next_url = urljoin(current_url, location) - if urlparse(current_url).scheme == "https" and urlparse(next_url).scheme == "http": + current_scheme = urlparse(current_url).scheme + next_scheme = urlparse(next_url).scheme + if current_scheme == "https" and next_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." ) + # http -> https upgrade on the same host: promote the stored server + # address so subsequent requests skip this redirect round-trip. + # Only rewrite on same-host, same-path-root redirects to avoid + # accidentally pointing the client at an unrelated server. + if current_scheme == "http" and next_scheme == "https": + current_parsed = urlparse(current_url) + next_parsed = urlparse(next_url) + if current_parsed.netloc == next_parsed.netloc: + old_address = self.parent_srv._server_address + if old_address.startswith("http://") and old_address[7:].startswith(current_parsed.netloc): + new_address = "https://" + old_address[7:] + self.parent_srv._server_address = new_address + logger.info(f"Server redirected to HTTPS; updated server address to {new_address}") 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) diff --git a/test/test_redirect_handling.py b/test/test_redirect_handling.py index d1d826da0..fffd0a231 100644 --- a/test/test_redirect_handling.py +++ b/test/test_redirect_handling.py @@ -196,6 +196,37 @@ def test_http_to_https_upgrade_allowed(server: TSC.Server) -> None: assert server.auth_token is not None +def test_http_to_https_upgrade_promotes_stored_server_address(server: TSC.Server) -> None: + # When the server redirects http://host -> https://host on the same host, + # promote server._server_address so subsequent requests skip the redirect. + assert server._server_address == "http://test" + xml = _sign_in_xml() + with requests_mock.mock() as m: + m.post( + server.auth.baseurl + "/signin", status_code=301, headers={"Location": "https://test/api/3.6/auth/signin"} + ) + m.post("https://test/api/3.6/auth/signin", text=xml) + server.auth.sign_in(TSC.TableauAuth("u", "p")) + assert server._server_address == "https://test" + + +def test_http_to_https_upgrade_does_not_promote_on_different_host(server: TSC.Server) -> None: + # If the redirect target is on a different host, do NOT rewrite the stored + # server address -- the redirect might be to a completely unrelated server + # and rewriting would silently point every future call at it. + assert server._server_address == "http://test" + xml = _sign_in_xml() + with requests_mock.mock() as m: + m.post( + server.auth.baseurl + "/signin", + status_code=301, + headers={"Location": "https://other-host/api/3.6/auth/signin"}, + ) + m.post("https://other-host/api/3.6/auth/signin", text=xml) + server.auth.sign_in(TSC.TableauAuth("u", "p")) + assert server._server_address == "http://test" + + def test_cross_host_redirect_followed(server: TSC.Server) -> None: # e.g. http://online.tableau.com -> http://east.online.tableau.com. # This is the scenario in the original inline signin comment.