[tabcmd] fix: preserve POST body across 3xx redirects (#1127, #1828) - #1848
Open
jacalata wants to merge 9 commits into
Open
[tabcmd] fix: preserve POST body across 3xx redirects (#1127, #1828)#1848jacalata wants to merge 9 commits into
jacalata wants to merge 9 commits into
Conversation
`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.
…irect 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.
jacalata
changed the title
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>
There was a problem hiding this comment.
Pull request overview
This PR updates the HTTP request path used by tableauserverclient endpoints to manually follow 3xx redirects (instead of relying on requests), preserving the original HTTP method/body across redirect hops and adding safety checks (notably refusing HTTPS→HTTP downgrades) plus clearer redirect-related errors.
Changes:
- Disable
requestsauto-redirects and manually follow 301/302/303/307/308 inEndpoint._make_request, preserving method/body across multiple hops. - Introduce
RedirectErrorfor redirect-following failures (missingLocation, hop limit exceeded, HTTPS→HTTP downgrade). - Add a dedicated redirect-handling test suite and document the behavior in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tableauserverclient/server/endpoint/endpoint.py |
Adds manual redirect-walking logic and security guardrails; updates request setup to disable auto-redirects. |
tableauserverclient/server/endpoint/auth_endpoint.py |
Routes sign_in through _make_request to pick up the new redirect behavior. |
tableauserverclient/server/endpoint/exceptions.py |
Adds RedirectError for redirect-following failures. |
test/test_redirect_handling.py |
Adds tests for POST body preservation, multi-hop redirects, scheme downgrade refusal, missing Location, and hop limits. |
CHANGELOG.md |
Documents the redirect handling behavior change and new error conditions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+76
to
83
| 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) |
Contributor
Author
There was a problem hiding this comment.
Good catch. Filed separately as #1866 -- same bug was flagged by a fresh-eyes pass this morning. Not addressing on this PR because PR #1863 proposes removing the whole Namespace.detect subsystem entirely (dead code since TSC min supported server is 10.0, 2016). If #1863 lands, #1866 is moot; if not, we widen the guard there.
Comment on lines
+217
to
+225
| 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}") |
Contributor
Author
There was a problem hiding this comment.
Fixed in 1ce99e0. Replaced the startswith with an exact netloc comparison after parsing the stored address. Corporate networks with DNS search paths / split-horizon (e.g. TSC.Server("http://tableau") resolving via search suffix) can legitimately hit this in practice, so the tighter check is warranted regardless of adversarial framing.
Comment on lines
+240
to
+243
| 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." | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
Fixed in 1ce99e0 -- error message now reads "last URL attempted was {current_url}".
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>
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>
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>
`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>
…ost-body # Conflicts: # CHANGELOG.md
- 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1127. Closes #1828.
Motivation
requestsfollows 301/302/303 by converting POST to GET and droppingthe request body. Any TSC write hitting a server behind a redirect
(
users.add,workbooks.publish,addusers, etc.) returned 405Method Not Allowed because the server saw a GET where it expected a
POST. Reported in #1127 in 2022; the underlying
requestsbehaviorpredates that.
Also picked up three nearby gaps in the same code region:
auth material over plaintext (TSC security: refuse HTTPS→HTTP scheme downgrade in sign-in redirect #1828, filed by me while working
on this).
Locationheaders on 3xx responses surfaced as bareKeyError('location')from deep insiderequests.server._server_addresswas never updated when the serverredirected http:// to https:// on the same host, so every subsequent
request paid the redirect round-trip. Recovers an older idea from
an abandoned branch (
jac/handle-https-better, 2026-04) now thatthe manual-redirect handler here provides the right hook point.
Behavior change
For users:
Locationsame schemeLocation; caller sees the eventual 2xx/errorsession.max_redirectshops (default 30); if exceeded, raisesRedirectErrorhttp://...RedirectErrorLocationKeyError('location')from insiderequestsRedirectErrornaming URL, method, status coderequestsEndpoint._make_requestdisables requests' auto-redirect and walks thechain manually, keeping the original method and body across every hop.
Sign-in retains its own single-hop 301 handler in
auth_endpoint.pyfor backwards compatibility; the new path is additive.
The http->https address promotion only fires when the redirect target
netloc matches the current netloc (same host, just scheme change), so
a cross-host redirect never rewrites the stored address.
Test plan
test/test_redirect_handling.pycovering POST bodypreservation, multi-hop chains, relative
Locationheaders, schemedowngrade refusal, missing
Location, hop-cap enforcement, http->https address promotion, and same-host guard on the promotion
🤖 Generated with Claude Code