From 11a7534025cf112b9588270e70af904255f3bc71 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 3 Aug 2026 20:19:13 -0700 Subject: [PATCH 1/8] 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/8] 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/8] 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. From 226c6e55ae91733976f9a0a008d11085080c5a41 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 17 Aug 2026 12:18:50 -0700 Subject: [PATCH 4/8] docs: justify auth-material forwarding across cross-host redirects Add a code comment in `_handle_redirects` explaining why the X-Tableau- Auth header and session cookies are intentionally preserved on cross- host redirects, rather than stripped as a generic library would. The concern was raised in a security-focused fresh-eyes review: same- scheme cross-host redirects normally leak bearer tokens to whoever controls the redirect target, and RFC 7235 recommends stripping auth on cross-origin hops for that reason. But TSC is a client for a specific server the caller has already trusted, and Tableau Server is routinely deployed behind reverse proxies, load balancers, and SSO front-ends that redirect between hosts within the same infrastructure (tableau.corp.example -> east.tableau.corp.example, SSO IdP -> auth callback endpoint on a different subdomain, etc.). Stripping auth material there would break sign-in against every such deployment. The HTTPS -> HTTP downgrade guard (line 208) is the load-bearing security boundary: once the caller connects over HTTPS, the token cannot leave TLS regardless of which host receives the redirect. Comment only. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) --- tableauserverclient/server/endpoint/endpoint.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tableauserverclient/server/endpoint/endpoint.py b/tableauserverclient/server/endpoint/endpoint.py index e391941f4..6ce240030 100644 --- a/tableauserverclient/server/endpoint/endpoint.py +++ b/tableauserverclient/server/endpoint/endpoint.py @@ -223,6 +223,21 @@ def _follow_redirect_if_any( 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}") + # Auth-material policy: the request `parameters` (including the + # X-Tableau-Auth header and any session cookies) are forwarded + # to the redirect target unchanged. This is intentional and + # required. TSC is a client library for a specific server the + # caller has already agreed to trust, and customers routinely + # deploy Tableau Server behind reverse proxies, load balancers, + # and SSO front-ends that redirect between hosts within their + # own infrastructure (e.g. tableau.corp.example -> east.tableau. + # corp.example, or an SSO IdP -> the auth-callback endpoint on + # a different subdomain). Stripping X-Tableau-Auth on cross- + # host redirects would break sign-in against every such + # deployment. The HTTPS -> HTTP downgrade guard above (line 208) + # is the boundary that keeps this from becoming a security + # regression: once the caller connects over HTTPS, the token + # never leaves TLS. 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) From 85351452ac24451f120000a1023f01f251d0e5df Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 17 Aug 2026 12:21:48 -0700 Subject: [PATCH 5/8] docs: justify uniform method preservation on 303 responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a code comment noting the deliberate deviation from RFC 7231 §6.4.4, which says 303 SHOULD change the method to GET on retry. TSC preserves the method and body on 303 the same as on the other redirect codes. Rationale: Tableau Server doesn't emit 303 for POST endpoints in normal operation, and PR #1848's goal is to preserve method+body across the common proxy/HA cases. If a deployment ever starts emitting 303 for writes, revisit. Marking it as a conscious deviation so a future reader doesn't submit a "fix" that reintroduces the bug we just fixed. Comment only. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) --- tableauserverclient/server/endpoint/endpoint.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tableauserverclient/server/endpoint/endpoint.py b/tableauserverclient/server/endpoint/endpoint.py index 6ce240030..b6df3a2c0 100644 --- a/tableauserverclient/server/endpoint/endpoint.py +++ b/tableauserverclient/server/endpoint/endpoint.py @@ -52,6 +52,14 @@ # 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. +# +# RFC 7231 §6.4.4 says 303 SHOULD change the method to GET on retry. We do NOT +# follow that recommendation, deliberately: Tableau Server does not emit 303 +# for POST endpoints in normal operation (writes redirect via 301/302 in +# proxy/HA setups), and preserving the method + body uniformly is the +# behavior that fixes the reported bug (#1127). If a Tableau deployment ever +# starts emitting 303 for writes, revisit; treating it identically today is +# a conscious deviation, not an oversight. Redirect_codes = [301, 302, 303, 307, 308] XML_CONTENT_TYPE = "text/xml" From 89602ccf0a05f3e89fd5403041caf83ff182b297 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 17 Aug 2026 12:26:22 -0700 Subject: [PATCH 6/8] respect explicit allow_redirects override from http_options Change `parameters["allow_redirects"] = False` to `parameters.setdefault("allow_redirects", False)`. Default behavior is unchanged: with no override, TSC walks the redirect chain itself and preserves method+body across every hop. If a caller has a specific reason to override -- a security policy that requires failing loudly on any redirect rather than silently following one, or a test harness that wants requests' default behavior -- they can pass allow_redirects=True or =False on the Server's http_options and have it respected. The manual redirect walker in _follow_redirect_if_any short-circuits on non-3xx responses, so requests handling the redirect first and returning a 200 is safe. The 24 existing redirect tests all pass unchanged; none of them override allow_redirects, and both the enforced-redirect and refused- redirect paths still exercise the correct code. Co-Authored-By: Claude Opus 4.7 (1M context) --- tableauserverclient/server/endpoint/endpoint.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/tableauserverclient/server/endpoint/endpoint.py b/tableauserverclient/server/endpoint/endpoint.py index b6df3a2c0..1976ea8a5 100644 --- a/tableauserverclient/server/endpoint/endpoint.py +++ b/tableauserverclient/server/endpoint/endpoint.py @@ -139,9 +139,14 @@ def _make_request( ) # 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 + # loses the body). We default it off here and re-issue the same + # method ourselves in _follow_redirect_if_any. Use setdefault so a + # caller who has a specific reason to override (e.g. a security + # policy that says "fail loudly on any redirect, don't silently + # follow it") can pass allow_redirects=True or =False on their + # http_options and have it respected -- the manual redirect walk + # is a default, not a mandate. + parameters.setdefault("allow_redirects", False) logger.debug(f"request method {method.__name__}, url: {url}") if content: From f9bf81a57af2bfd0a335c730ef3f01909185d355 Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 17 Aug 2026 12:32:51 -0700 Subject: [PATCH 7/8] populate response.history when walking the redirect chain manually `requests`' native follower populates `Response.history` with the intermediate 3xx responses in receipt order; the final non-3xx response is what's returned, not in history. PR #1848 short-circuited the native follower by setting `allow_redirects=False` and walking the chain in `_follow_redirect_if_any`, which meant `.history` came back as an empty list even after a multi-hop chain. Fine for internal callers (nothing in TSC reads .history), but a silent behavior change for external consumers who forensically inspect responses. Collect each intermediate 3xx response in a local list and assign it to `response.history` on the final non-3xx response before returning. Matches the shape callers get from requests' native follower. Two tests: - test_response_history_populated_across_multi_hop_chain confirms the intermediate 301 and 302 land in .history in order after a 3-hop chain terminating in 200. - test_response_history_empty_when_no_redirect confirms the no-redirect short-circuit still returns .history=[] as requests would have. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../server/endpoint/endpoint.py | 7 +++ test/test_redirect_handling.py | 53 +++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/tableauserverclient/server/endpoint/endpoint.py b/tableauserverclient/server/endpoint/endpoint.py index 1976ea8a5..ad4f104b7 100644 --- a/tableauserverclient/server/endpoint/endpoint.py +++ b/tableauserverclient/server/endpoint/endpoint.py @@ -206,6 +206,11 @@ def _follow_redirect_if_any( # Not a redirect? Return immediately regardless of max_hops (including 0). if response.status_code not in Redirect_codes: return response, current_url + # Preserve requests' `response.history` semantics: the intermediate 3xx + # responses in receipt order, with the final non-3xx response as the + # returned value. Callers doing forensic debugging on `.history` see + # the same shape they would from requests' native follower. + history: list["Response"] = [] method_name = getattr(method, "__name__", "REQUEST").upper() for _ in range(max_hops): location = response.headers.get("Location") @@ -252,6 +257,7 @@ def _follow_redirect_if_any( # regression: once the caller connects over HTTPS, the token # never leaves TLS. logger.debug(f"Following {response.status_code} redirect: {current_url} -> {next_url}") + history.append(response) current_url = next_url next_response = self._blocking_request(method, current_url, parameters) if next_response is None: @@ -263,6 +269,7 @@ def _follow_redirect_if_any( raise next_response response = next_response if response.status_code not in Redirect_codes: + response.history = history return response, current_url # Still a redirect after max_hops hops -> loop / misconfiguration. raise RedirectError( diff --git a/test/test_redirect_handling.py b/test/test_redirect_handling.py index fffd0a231..411d163d0 100644 --- a/test/test_redirect_handling.py +++ b/test/test_redirect_handling.py @@ -122,6 +122,59 @@ def test_get_redirect_still_works(signed_in_server: TSC.Server) -> None: # --- Redirect codes --------------------------------------------------------- +def test_response_history_populated_across_multi_hop_chain(server: TSC.Server) -> None: + # Regression: `requests` normally populates `response.history` with the + # intermediate 3xx responses when it follows a redirect chain. TSC's + # manual walker took over that job, so we have to populate .history + # ourselves for callers doing forensic debugging. + from unittest.mock import patch + + xml = _sign_in_xml() + captured: list = [] + original_check = TSC.server.endpoint.Endpoint._check_status # type: ignore[attr-defined] + + def capture(self, response, url=None): + captured.append(response) + return original_check(self, response, url) + + with requests_mock.mock() as m: + 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", text=xml) + with patch.object(TSC.server.endpoint.Endpoint, "_check_status", capture): # type: ignore[attr-defined] + server.auth.sign_in(TSC.TableauAuth("u", "p")) + + assert len(captured) == 1, f"Expected one final response, got {len(captured)}" + final = captured[0] + assert final.status_code == 200 + assert len(final.history) == 2, f"Expected 2 intermediate hops in history, got {len(final.history)}" + assert final.history[0].status_code == 301 + assert final.history[1].status_code == 302 + + +def test_response_history_empty_when_no_redirect(signed_in_server: TSC.Server) -> None: + # When there's no redirect, .history stays as whatever requests set it + # (empty list). We don't overwrite it in the no-redirect short-circuit. + from unittest.mock import patch + + captured: list = [] + original_check = TSC.server.endpoint.Endpoint._check_status # type: ignore[attr-defined] + + def capture(self, response, url=None): + captured.append(response) + return original_check(self, response, url) + + baseurl = signed_in_server.workbooks.baseurl + with requests_mock.mock() as m: + m.get(baseurl, text=_workbooks_get_xml()) + with patch.object(TSC.server.endpoint.Endpoint, "_check_status", capture): # type: ignore[attr-defined] + signed_in_server.workbooks.get() + + assert len(captured) == 1 + assert captured[0].status_code == 200 + assert captured[0].history == [] + + @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() From 1ce99e03279f061f82795830b258c1a3bcbf9afd Mon Sep 17 00:00:00 2001 From: Jac Fitzgerald Date: Mon, 17 Aug 2026 12:39:19 -0700 Subject: [PATCH 8/8] Address Copilot review findings on #1848 - endpoint.py: replace `old_address[7:].startswith(current_parsed.netloc)` with exact `old_parsed.netloc == current_parsed.netloc` comparison for the http->https address promotion. The startswith form was correct in the reviewed attacker scenarios but wrong for the common corporate case: an unqualified hostname like `TSC.Server("http://tableau")` where DNS search paths / split-horizon resolve `tableau` to different actual hosts. A redirect from `http://tableau/` to `https://tableau.other/` would previously promote `_server_address` to `https://tableau` even though the redirected netloc was `tableau.other`. Exact-netloc match kills that. - endpoint.py: change RedirectError message from "last Location was {current_url}" to "last URL attempted was {current_url}". current_url is the resolved URL of the last attempted hop, not the raw Location header value; the old phrasing was misleading during redirect-loop diagnostics. - test_redirect_handling.py: tighten test_non_followed_3xx_codes_pass_through to raise `ServerResponseError` specifically instead of `(ServerResponseError, Exception)`. The mocked XML error body deterministically produces ServerResponseError via _check_status; the broader assertion could mask an unrelated failure. - The fourth Copilot finding (namespace detection lost on the new signin path) is tracked separately as #1866; addressing there so it does not block this PR. Co-Authored-By: Claude Opus 4.7 (1M context) --- tableauserverclient/server/endpoint/endpoint.py | 13 ++++++++----- test/test_redirect_handling.py | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/tableauserverclient/server/endpoint/endpoint.py b/tableauserverclient/server/endpoint/endpoint.py index ad4f104b7..7fa34802d 100644 --- a/tableauserverclient/server/endpoint/endpoint.py +++ b/tableauserverclient/server/endpoint/endpoint.py @@ -230,15 +230,18 @@ def _follow_redirect_if_any( ) # 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. + # Only rewrite when the stored address's netloc exactly matches + # the redirected netloc to avoid pointing the client at an + # unrelated server (prefix matching could match e.g. "test" + # against a stored address of "test.other.example"). 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:] + old_parsed = urlparse(old_address) + if old_parsed.scheme == "http" and old_parsed.netloc == current_parsed.netloc: + new_address = "https://" + old_address[len("http://") :] self.parent_srv._server_address = new_address logger.info(f"Server redirected to HTTPS; updated server address to {new_address}") # Auth-material policy: the request `parameters` (including the @@ -273,7 +276,7 @@ def _follow_redirect_if_any( 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}. " + f"Exceeded {max_hops} redirect hops starting from {url}; last URL attempted was {current_url}. " f"Increase session.max_redirects if this is legitimate." ) diff --git a/test/test_redirect_handling.py b/test/test_redirect_handling.py index 411d163d0..37bca5c18 100644 --- a/test/test_redirect_handling.py +++ b/test/test_redirect_handling.py @@ -206,7 +206,7 @@ def test_non_followed_3xx_codes_pass_through(signed_in_server: TSC.Server, code: status_code=code, text="xy", ) - with pytest.raises((TSC.ServerResponseError, Exception)): + with pytest.raises(TSC.ServerResponseError): signed_in_server.workbooks.get()