Skip to content

Commit e29d81e

Browse files
jacalataclaude
andauthored
[tabcmd] fix: preserve POST body across 3xx redirects (#1127, #1828) (#1848)
* 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. * 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. * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * docs: justify uniform method preservation on 303 responses 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 47809c3 commit e29d81e

5 files changed

Lines changed: 505 additions & 14 deletions

File tree

CHANGELOG.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,14 @@
55
hierarchy path (e.g. `"Marketing/Q1 Reports"`). The walk is performed level by
66
level using the REST API name filter, so a path with *n* components issues *n*
77
requests. Returns the matching `ProjectItem` or `None` if no project is found.
8+
* Preserve HTTP method and body across 3xx redirects. Previously `requests`
9+
followed 301/302/303 by converting POST to GET and dropping the body, so
10+
endpoints like `users.add`, `workbooks.publish`, and any write hitting a
11+
server behind a redirect would 405. TSC now disables `requests`'s
12+
auto-redirect and walks the chain manually, up to `session.max_redirects`
13+
hops (default 30). Refuses HTTPS -> HTTP scheme downgrades and raises
14+
`RedirectError` with a clear message on missing `Location` headers or hop
15+
overflow. Fixes #1127 and #1828.
816
* `UserItem.CSVImport.create_user_from_line` no longer
917
lowercases the entire CSV line before parsing. Previously the whole line,
1018
including the username, display name, fullname, and email fields, was

tableauserverclient/server/endpoint/auth_endpoint.py

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
from defusedxml.ElementTree import fromstring
66

7-
from tableauserverclient.server.endpoint.endpoint import Endpoint, api
7+
from tableauserverclient.server.endpoint.endpoint import Endpoint, XML_CONTENT_TYPE, api
88
from tableauserverclient.server.endpoint.exceptions import ServerResponseError
99
from tableauserverclient.server.request_factory import RequestFactory
1010

@@ -68,20 +68,18 @@ def sign_in(self, auth_req: "Credentials") -> contextmgr:
6868
"""
6969
url = f"{self.baseurl}/signin"
7070
signin_req = RequestFactory.Auth.signin_req(auth_req)
71-
server_response = self.parent_srv.session.post(
72-
url, data=signin_req, **self.parent_srv.http_options, allow_redirects=False
71+
# Route through _make_request so signin gets the same redirect handling
72+
# (multi-hop, HTTPS->HTTP scheme guard, missing-Location diagnostic,
73+
# hop limit) that every other endpoint uses. Explicit auth_token=None
74+
# because we don't have one yet -- and self.parent_srv.auth_token
75+
# raises NotSignedInError pre-signin, so post_request can't help here.
76+
server_response = self._make_request(
77+
self.parent_srv.session.post,
78+
url,
79+
content=signin_req,
80+
auth_token=None,
81+
content_type=XML_CONTENT_TYPE,
7382
)
74-
# manually handle a redirect so that we send the correct POST request instead of GET
75-
# this will make e.g http://online.tableau.com work to redirect to http://east.online.tableau.com
76-
if server_response.status_code == 301:
77-
server_response = self.parent_srv.session.post(
78-
server_response.headers["Location"],
79-
data=signin_req,
80-
**self.parent_srv.http_options,
81-
allow_redirects=False,
82-
)
83-
self.parent_srv._namespace.detect(server_response.content)
84-
self._check_status(server_response, url)
8583
parsed_response = fromstring(server_response.content)
8684
site_id = parsed_response.find(".//t:site", namespaces=self.parent_srv.namespace).get("id", None)
8785
site_url = parsed_response.find(".//t:site", namespaces=self.parent_srv.namespace).get("contentUrl", None)

tableauserverclient/server/endpoint/endpoint.py

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import os
44
from contextlib import closing
55
from typing_extensions import Concatenate, ParamSpec
6+
from urllib.parse import urljoin, urlparse
67
from tableauserverclient import datetime_helpers as datetime
78

89
import abc
@@ -30,6 +31,7 @@
3031
InternalServerError,
3132
NonXMLResponseError,
3233
NotSignedInError,
34+
RedirectError,
3335
)
3436
from tableauserverclient.server.exceptions import EndpointUnavailableError
3537

@@ -45,6 +47,21 @@
4547

4648
Success_codes = [200, 201, 202, 204]
4749

50+
# 301/302/303/307/308 all indicate the caller should re-request at a new URL.
51+
# `requests`' default handler converts POST -> GET on 301/302/303, which drops
52+
# the POST body and breaks sign-in / addusers / publish / any write endpoint
53+
# whose target sits behind a redirect. We disable that and walk the chain
54+
# manually, keeping the original method and body across every hop.
55+
#
56+
# RFC 7231 §6.4.4 says 303 SHOULD change the method to GET on retry. We do NOT
57+
# follow that recommendation, deliberately: Tableau Server does not emit 303
58+
# for POST endpoints in normal operation (writes redirect via 301/302 in
59+
# proxy/HA setups), and preserving the method + body uniformly is the
60+
# behavior that fixes the reported bug (#1127). If a Tableau deployment ever
61+
# starts emitting 303 for writes, revisit; treating it identically today is
62+
# a conscious deviation, not an oversight.
63+
Redirect_codes = [301, 302, 303, 307, 308]
64+
4865
XML_CONTENT_TYPE = "text/xml"
4966
JSON_CONTENT_TYPE = "application/json"
5067

@@ -120,6 +137,16 @@ def _make_request(
120137
parameters = Endpoint.set_parameters(
121138
self.parent_srv.http_options, auth_token, content, content_type, parameters
122139
)
140+
# Manual redirect handling: see Redirect_codes comment. `requests`
141+
# follows 301/302/303 by converting POST to GET (RFC-conforming but
142+
# loses the body). We default it off here and re-issue the same
143+
# method ourselves in _follow_redirect_if_any. Use setdefault so a
144+
# caller who has a specific reason to override (e.g. a security
145+
# policy that says "fail loudly on any redirect, don't silently
146+
# follow it") can pass allow_redirects=True or =False on their
147+
# http_options and have it respected -- the manual redirect walk
148+
# is a default, not a mandate.
149+
parameters.setdefault("allow_redirects", False)
123150

124151
logger.debug(f"request method {method.__name__}, url: {url}")
125152
if content:
@@ -144,6 +171,7 @@ def _make_request(
144171
raise RuntimeError
145172
if isinstance(server_response, Exception):
146173
raise server_response
174+
server_response, url = self._follow_redirect_if_any(method, url, parameters, server_response)
147175
self._check_status(server_response, url)
148176

149177
loggable_response = self.log_response_safely(server_response)
@@ -157,6 +185,101 @@ def _make_request(
157185

158186
return server_response
159187

188+
def _follow_redirect_if_any(
189+
self,
190+
method: Callable[..., "Response"],
191+
url: str,
192+
parameters: dict[str, Any],
193+
server_response: "Response",
194+
) -> tuple["Response", str]:
195+
# Walk a 301/302/303/307/308 chain up to session.max_redirects hops,
196+
# preserving method and body. Rejects HTTPS -> HTTP scheme downgrades
197+
# (silent security regression). Raises RedirectError on a missing
198+
# Location header instead of the KeyError requests emits deep in its
199+
# internals, and on exceeding the session hop limit.
200+
try:
201+
max_hops = int(self.parent_srv.session.max_redirects)
202+
except (AttributeError, TypeError):
203+
max_hops = 30 # requests' library default
204+
current_url = url
205+
response = server_response
206+
# Not a redirect? Return immediately regardless of max_hops (including 0).
207+
if response.status_code not in Redirect_codes:
208+
return response, current_url
209+
# Preserve requests' `response.history` semantics: the intermediate 3xx
210+
# responses in receipt order, with the final non-3xx response as the
211+
# returned value. Callers doing forensic debugging on `.history` see
212+
# the same shape they would from requests' native follower.
213+
history: list["Response"] = []
214+
method_name = getattr(method, "__name__", "REQUEST").upper()
215+
for _ in range(max_hops):
216+
location = response.headers.get("Location")
217+
if not location:
218+
raise RedirectError(
219+
f"{method_name} {current_url} returned HTTP {response.status_code} "
220+
f"without a Location header; can't follow the redirect."
221+
)
222+
# Support relative Locations per RFC 7231.
223+
next_url = urljoin(current_url, location)
224+
current_scheme = urlparse(current_url).scheme
225+
next_scheme = urlparse(next_url).scheme
226+
if current_scheme == "https" and next_scheme == "http":
227+
raise RedirectError(
228+
f"Refusing to follow redirect from {current_url} to {next_url}: "
229+
f"HTTPS -> HTTP scheme downgrade would send request data over plaintext."
230+
)
231+
# http -> https upgrade on the same host: promote the stored server
232+
# address so subsequent requests skip this redirect round-trip.
233+
# Only rewrite when the stored address's netloc exactly matches
234+
# the redirected netloc to avoid pointing the client at an
235+
# unrelated server (prefix matching could match e.g. "test"
236+
# against a stored address of "test.other.example").
237+
if current_scheme == "http" and next_scheme == "https":
238+
current_parsed = urlparse(current_url)
239+
next_parsed = urlparse(next_url)
240+
if current_parsed.netloc == next_parsed.netloc:
241+
old_address = self.parent_srv._server_address
242+
old_parsed = urlparse(old_address)
243+
if old_parsed.scheme == "http" and old_parsed.netloc == current_parsed.netloc:
244+
new_address = "https://" + old_address[len("http://") :]
245+
self.parent_srv._server_address = new_address
246+
logger.info(f"Server redirected to HTTPS; updated server address to {new_address}")
247+
# Auth-material policy: the request `parameters` (including the
248+
# X-Tableau-Auth header and any session cookies) are forwarded
249+
# to the redirect target unchanged. This is intentional and
250+
# required. TSC is a client library for a specific server the
251+
# caller has already agreed to trust, and customers routinely
252+
# deploy Tableau Server behind reverse proxies, load balancers,
253+
# and SSO front-ends that redirect between hosts within their
254+
# own infrastructure (e.g. tableau.corp.example -> east.tableau.
255+
# corp.example, or an SSO IdP -> the auth-callback endpoint on
256+
# a different subdomain). Stripping X-Tableau-Auth on cross-
257+
# host redirects would break sign-in against every such
258+
# deployment. The HTTPS -> HTTP downgrade guard above (line 208)
259+
# is the boundary that keeps this from becoming a security
260+
# regression: once the caller connects over HTTPS, the token
261+
# never leaves TLS.
262+
logger.debug(f"Following {response.status_code} redirect: {current_url} -> {next_url}")
263+
history.append(response)
264+
current_url = next_url
265+
next_response = self._blocking_request(method, current_url, parameters)
266+
if next_response is None:
267+
raise RuntimeError(f"No response after redirect to {current_url}")
268+
if isinstance(next_response, Exception):
269+
# _blocking_request already re-raises via except -> raise, so this
270+
# branch is defensive; keep it to satisfy the Response|Exception|None
271+
# return type.
272+
raise next_response
273+
response = next_response
274+
if response.status_code not in Redirect_codes:
275+
response.history = history
276+
return response, current_url
277+
# Still a redirect after max_hops hops -> loop / misconfiguration.
278+
raise RedirectError(
279+
f"Exceeded {max_hops} redirect hops starting from {url}; last URL attempted was {current_url}. "
280+
f"Increase session.max_redirects if this is legitimate."
281+
)
282+
160283
def _check_status(self, server_response: "Response", url: str | None = None):
161284
logger.debug(f"Response status: {server_response}")
162285
if not hasattr(server_response, "status_code"):

tableauserverclient/server/endpoint/exceptions.py

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,3 +130,10 @@ class FlowRunCancelledException(FlowRunFailedException):
130130

131131
class UnsupportedAttributeError(TableauError):
132132
pass
133+
134+
135+
class RedirectError(TableauError):
136+
# Raised when a manual redirect can't be followed safely or at all.
137+
# Cases: missing Location header, HTTPS -> HTTP downgrade, redirect loop
138+
# exceeding session.max_redirects. See Endpoint._follow_redirect_if_any.
139+
pass

0 commit comments

Comments
 (0)