fix: preserve POST body across 3xx redirects (#1127, #1828)

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

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"):

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

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"<tsResponse/>"
+
+    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="<tsResponse/>")
+
+        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="<tsResponse/>")
+
+        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="<tsResponse/>")
+        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