[tabcmd] 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/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)

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,76 @@ 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
+        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 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)
+            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}. "
+            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