[tabcmd] fix: preserve POST body across 3xx redirects (#1127, #1828) - #1848
Open
jacalata wants to merge 3 commits into
Open
[tabcmd] fix: preserve POST body across 3xx redirects (#1127, #1828)#1848jacalata wants to merge 3 commits into
jacalata wants to merge 3 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>
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
1 participant
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