diff --git a/CHANGELOG.md b/CHANGELOG.md index fc1430d38..aa995f952 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. * Added `JobItem.status_notes` for the structured `` block documented on the Query Job REST endpoint. Populated for UserImport and other multi-row jobs where 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 31a0806dc..7fa34802d 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,21 @@ 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. +# +# 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" JSON_CONTENT_TYPE = "application/json" @@ -120,6 +137,16 @@ 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 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: @@ -144,6 +171,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 +185,101 @@ 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 + # 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") + if not location: + raise RedirectError( + 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. + next_url = urljoin(current_url, location) + 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 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 + 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 + # 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}") + history.append(response) + 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): + # _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: + response.history = history + 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 URL attempted 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..37bca5c18 --- /dev/null +++ b/test/test_redirect_handling.py @@ -0,0 +1,355 @@ +"""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 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 + +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" +GET_XML = TEST_ASSET_DIR / "workbook_get.xml" + + +@pytest.fixture +def server() -> TSC.Server: + 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: + 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(server: TSC.Server) -> None: + # Regression for tableau/tabcmd#309: POST -> 302 previously turned into GET + # 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_final(request, context): + seen_bodies.append(request.body) + context.status_code = 200 + return xml + + with requests_mock.mock() as m: + 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) + + 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: + xml = _sign_in_xml() + 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", status_code=303, headers={"Location": "http://test/d"}) + m.post("http://test/d", text=xml) + + server.auth.sign_in(TSC.TableauAuth("u", "p")) + + assert server.auth_token is not None + + +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() + + baseurl = signed_in_server.workbooks.baseurl + with requests_mock.mock() as m: + m.get(baseurl, status_code=302, headers={"Location": baseurl + "?redirected=1"}) + m.get(baseurl + "?redirected=1", text=capture) + signed_in_server.workbooks.get() + + # 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 + + +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 + + +# --- 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() + seen_bodies: list[bytes | None] = [] + + def capture(request, context): + seen_bodies.append(request.body) + context.status_code = 200 + return xml + + with requests_mock.mock() as m: + 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")) + + assert len(seen_bodies) == 1 + assert seen_bodies[0] is not None + assert b" 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.get( + baseurl, + status_code=code, + text="xy", + ) + with pytest.raises(TSC.ServerResponseError): + signed_in_server.workbooks.get() + + +# --- Scheme handling -------------------------------------------------------- + + +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")) + + +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_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": "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.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. + 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()