fix(connect): resolve relative Location headers on redirect#216
fix(connect): resolve relative Location headers on redirect#216ian-flores merged 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes ConnectClient.fetch_content failing on redirects when Posit Connect returns a relative Location header by resolving redirect targets to absolute URLs before performing the safety check and follow-up request.
Changes:
- Resolve relative redirect
Locationvalues usingurljoin(str(resp.url), location)before parsing and fetching. - Add selftests covering relative redirects (followed) and cross-origin redirects (blocked).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/vip/clients/connect.py |
Resolves relative redirect locations to absolute URLs before same-origin gating and follow-up GET. |
selftests/test_clients_connect.py |
Adds regression tests for relative redirect handling and cross-origin redirect blocking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| target = urlparse(absolute_location) | ||
| # Only follow redirects to the same origin. | ||
| if target.hostname and target.hostname != origin.hostname: | ||
| break | ||
| resp = httpx.get( | ||
| location, | ||
| absolute_location, |
There was a problem hiding this comment.
The redirect “same-origin” check only compares target.hostname to origin.hostname. This still allows redirects that change scheme (e.g. https → http) or port on the same host, which can undermine the stated goal of avoiding API key leakage (e.g., key sent over plain HTTP or to a different service bound to another port). Consider enforcing full origin equality (scheme + hostname + port/netloc), and optionally restricting redirects to http/https schemes only before issuing the follow-up GET.
There was a problem hiding this comment.
Good catch — addressed in e692c2f. The origin check now requires full (scheme, hostname, port) equality with default-port normalization, and rejects non-http(s) schemes. Added selftests for scheme downgrade, port change, file:// scheme, and explicit :443 normalization.
|
Summary
ConnectClient.fetch_contentraisedhttpcore.UnsupportedProtocolwhen Connect returned a relativeLocationheader (e.g./content/{guid}/notebook.html) because the raw path was handed tohttpx.get.Locationvalue againststr(resp.url)withurljoinbefore the same-origin check and the follow-up GET.Test plan
uv run ruff check src/ selftests/uv run ruff format --check src/ selftests/uv run pytest selftests/test_clients_connect.py -v— 2 new tests pass (relative redirect followed; cross-origin redirect blocked)uv run pytest selftests/ -q— 312 passed, 1 known timing flake (test_1k_users)vip verify --connect-url https://pub.ganso.lab.staging.posit.team --categories connect --filter "deploy and not rmarkdown"against ganso01-staging — 6 deploy tests pass (jupyter, quarto, plumber, fastapi, dash, gitbacked, all of which callfetch_content). Shiny timed out during packrat-restore, unrelated to this change.Fixes #213