From d0659fe8c6d21e14418f61f7ffbe14474313aeab Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger Date: Wed, 21 Oct 2026 15:20:14 -0300 Subject: [PATCH 1/3] fix(destination): do not forward secret headers across HTTP redirects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DestinationHttpClient pre-bakes destination credentials (auth tokens, API keys, x-sap-* headers) into a persistent requests.Session. Because requests followed 3xx redirects automatically, those headers were silently forwarded to the redirect target — potentially a different origin. Changes: - DestinationHttpClient.request() now defaults to RedirectMode.NONE, which returns the 3xx response as-is without following it. This is automatic: no changes are required in consuming code. - New RedirectMode.SAME_ORIGIN opt-in follows redirects only when the Location URL has the same scheme, host, and effective port. Cross-origin, port-change, HTTPS→HTTP, and userinfo redirects are rejected. Bounded at 5 hops. 303 See Other converts to GET and drops the body. - allow_redirects kwarg is silently stripped from request() — redirect behavior is controlled only through redirect_mode. - HttpClient._execute() (internal SAP service client) also sets allow_redirects=False to protect per-request Authorization headers. - RedirectMode exported from sap_cloud_sdk.destination. Tests: 22 new cases covering NONE/SAME_ORIGIN, all 3xx codes, credential non-leakage, chain limits, HTTPS→HTTP rejection, and 303 body drop. --- CHANGELOG.md | 25 +++ pyproject.toml | 2 +- .../core/protocol/http/client.py | 1 + src/sap_cloud_sdk/destination/__init__.py | 3 +- .../destination/_destination_http_client.py | 110 ++++++++++++- src/sap_cloud_sdk/destination/user-guide.md | 26 +++ tests/core/unit/test_http_client.py | 6 + .../unit/test_destination_http_client.py | 151 +++++++++++++++++- 8 files changed, 319 insertions(+), 5 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 00000000..677cf8ed --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,25 @@ +# Changelog + +All notable changes to this project will be documented in this file. +Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). + +## [0.63.0] — 2026-10-21 + +### Security + +- **`DestinationHttpClient` no longer follows HTTP redirects by default.** + Destination credentials (auth tokens, API keys, `x-sap-*` headers) pre-baked into the + session are now never forwarded to a redirect target implicitly. This prevents credential + leakage to a different origin. This is a **behavioral change**: callers that previously + relied on transparent redirect following will now receive the 3xx response directly. + + To opt in to same-origin redirect following: + ```python + http = DestinationHttpClient(dest, redirect_mode=RedirectMode.SAME_ORIGIN) + ``` + `RedirectMode` is now exported from `sap_cloud_sdk.destination`. + +- `HttpClient` (internal SAP service client used by Destination, AI Core, and others) also + stops following redirects. +- Passing `allow_redirects=True` in `**kwargs` to `DestinationHttpClient.request()` is now + silently ignored. Redirect behavior is controlled exclusively via `redirect_mode`. diff --git a/pyproject.toml b/pyproject.toml index d4835fa4..4669babf 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "sap-cloud-sdk" -version = "0.62.0" +version = "0.63.0" description = "SAP Cloud SDK for Python" readme = "README.md" license = "Apache-2.0" diff --git a/src/sap_cloud_sdk/core/protocol/http/client.py b/src/sap_cloud_sdk/core/protocol/http/client.py index c7a684aa..facfab23 100644 --- a/src/sap_cloud_sdk/core/protocol/http/client.py +++ b/src/sap_cloud_sdk/core/protocol/http/client.py @@ -95,6 +95,7 @@ def _execute( method_str, f"{base_url}{path}", timeout=self._timeout, + allow_redirects=False, **kwargs, ) diff --git a/src/sap_cloud_sdk/destination/__init__.py b/src/sap_cloud_sdk/destination/__init__.py index b8b3ff0f..3db9aaaf 100644 --- a/src/sap_cloud_sdk/destination/__init__.py +++ b/src/sap_cloud_sdk/destination/__init__.py @@ -52,7 +52,7 @@ _make_config_factory, ) from sap_cloud_sdk.core.protocol.http import HttpClient, XsuaaAuthProvider -from sap_cloud_sdk.destination._destination_http_client import DestinationHttpClient +from sap_cloud_sdk.destination._destination_http_client import DestinationHttpClient, RedirectMode from sap_cloud_sdk.destination.client import DestinationClient from sap_cloud_sdk.destination.fragment_client import FragmentClient from sap_cloud_sdk.destination.certificate_client import CertificateClient @@ -260,6 +260,7 @@ def create_certificate_client( "LocalDevFragmentClient", "LocalDevCertificateClient", "DestinationHttpClient", + "RedirectMode", # Exceptions "DestinationError", "ClientCreationError", diff --git a/src/sap_cloud_sdk/destination/_destination_http_client.py b/src/sap_cloud_sdk/destination/_destination_http_client.py index f97de1a4..922de811 100644 --- a/src/sap_cloud_sdk/destination/_destination_http_client.py +++ b/src/sap_cloud_sdk/destination/_destination_http_client.py @@ -3,7 +3,9 @@ from __future__ import annotations import ssl +from enum import Enum from typing import Any, Dict, Optional +from urllib.parse import urljoin, urlparse import requests from requests import Response @@ -12,6 +14,35 @@ from sap_cloud_sdk.destination._cert_loader import build_client_cert_context from sap_cloud_sdk.destination._models import Destination, DestinationType +_REDIRECT_STATUS_CODES = frozenset({301, 302, 303, 307, 308}) +_MAX_REDIRECT_HOPS = 5 + + +class RedirectMode(Enum): + """Controls whether ``DestinationHttpClient`` follows HTTP redirects. + + Attributes: + NONE: Never follow redirects (default). The 3xx response is returned + as-is. Destination credentials are never forwarded beyond the + configured origin. + SAME_ORIGIN: Follow redirects only when the ``Location`` URL resolves to + the same scheme, host, and port as the destination URL. Cross-origin + redirects, port changes, and HTTPS-to-HTTP downgrades are rejected + (the redirect response is returned). Hop count is bounded at 5. + """ + + NONE = "none" + SAME_ORIGIN = "same_origin" + + +def _parse_origin(url: str) -> tuple[str, str, int]: + """Return the normalized (scheme, host, effective_port) triple for *url*.""" + p = urlparse(url) + scheme = p.scheme.lower() + host = (p.hostname or "").lower() + port = p.port if p.port is not None else (443 if scheme == "https" else 80) + return (scheme, host, port) + class _ClientCertAdapter(HTTPAdapter): """requests HTTPAdapter that injects a stdlib SSLContext for mTLS.""" @@ -36,13 +67,23 @@ class DestinationHttpClient: sap-language), URL.headers.* properties, and auth tokens. Certificates from the destination's certificate list are mounted into the session. + Redirects are **not** followed by default (``RedirectMode.NONE``). This + prevents destination credentials pre-baked in the session from being + forwarded to a different origin. Use ``redirect_mode=RedirectMode.SAME_ORIGIN`` + to allow same-origin relative redirects. + Use as a context manager to ensure the underlying session is closed: with DestinationHttpClient(dest) as http: response = http.request("GET", "/api/resource") """ - def __init__(self, destination: Destination) -> None: + def __init__( + self, + destination: Destination, + *, + redirect_mode: RedirectMode = RedirectMode.NONE, + ) -> None: if destination.type != DestinationType.HTTP: raise ValueError( f"DestinationHttpClient only supports HTTP destinations, got: {destination.type}" @@ -51,6 +92,8 @@ def __init__(self, destination: Destination) -> None: self._session = requests.Session() self._session.headers.update(destination.get_headers()) self._base_url = destination.url.rstrip("/") if destination.url else "" + self._redirect_mode = redirect_mode + self._origin = _parse_origin(self._base_url) ssl_ctx = build_client_cert_context(destination) if ssl_ctx is not None: @@ -68,27 +111,90 @@ def request( ) -> Response: """Send an HTTP request to the target system. + Redirects are not followed by default. Construct with + ``redirect_mode=RedirectMode.SAME_ORIGIN`` to follow same-origin + relative redirects. Passing ``allow_redirects`` in ``**kwargs`` has no + effect — it is silently removed to prevent accidental credential leakage. + Args: method: HTTP verb (GET, POST, PUT, PATCH, DELETE). path: Path relative to the destination URL. params: Optional query parameters. json: Optional JSON body. headers: Optional additional headers merged on top of pre-baked ones. - **kwargs: Passed through to requests.Session.request. + **kwargs: Passed through to requests.Session.request (except + ``allow_redirects``, which is always controlled by the + ``redirect_mode`` set at construction time). Returns: requests.Response from the target system. """ url = f"{self._base_url}/{path.lstrip('/')}" if path else self._base_url + kwargs.pop("allow_redirects", None) + if self._redirect_mode is RedirectMode.SAME_ORIGIN: + return self._request_following_same_origin( + method.upper(), url, params=params, json=json, headers=headers, **kwargs + ) return self._session.request( method=method.upper(), url=url, params=params, json=json, headers=headers, + allow_redirects=False, **kwargs, ) + def _request_following_same_origin( + self, + method: str, + start_url: str, + *, + params: Optional[Dict[str, Any]], + json: Optional[Any], + headers: Optional[Dict[str, str]], + **kwargs: Any, + ) -> Response: + """Follow redirects only when the Location resolves to the same origin. + + Uses a manual hop loop so that ALL credential headers baked into the + session are protected — not only Authorization (which requests strips + natively). Every hop is validated before the next request is issued. + """ + current_url = start_url + for _ in range(_MAX_REDIRECT_HOPS): + resp = self._session.request( + method=method, + url=current_url, + params=params, + json=json, + headers=headers, + allow_redirects=False, + **kwargs, + ) + if resp.status_code not in _REDIRECT_STATUS_CODES: + return resp + location = resp.headers.get("Location") + if not location: + return resp + next_url = urljoin(current_url, location) + np = urlparse(next_url) + # Reject userinfo (e.g. http://user:pass@host/) or malformed authority + if "@" in np.netloc or not np.hostname: + return resp + # Reject HTTPS → HTTP downgrade (defense-in-depth on top of origin check) + if urlparse(current_url).scheme == "https" and np.scheme == "http": + return resp + # Reject any change in scheme, host, or effective port + if _parse_origin(next_url) != self._origin: + return resp + # 303 See Other — always convert to GET and drop the request body + if resp.status_code == 303: + method, json = "GET", None + params = None # Location already encodes the resolved query string + current_url = next_url + return resp # max hops exhausted — return the last redirect response + def __enter__(self) -> "DestinationHttpClient": return self diff --git a/src/sap_cloud_sdk/destination/user-guide.md b/src/sap_cloud_sdk/destination/user-guide.md index 264c9a12..b9bcb8fc 100644 --- a/src/sap_cloud_sdk/destination/user-guide.md +++ b/src/sap_cloud_sdk/destination/user-guide.md @@ -471,6 +471,32 @@ response = http.request("GET", "/api/resource", headers={"X-Correlation-ID": "ab Per-request headers are merged on top of the pre-baked session headers. +### Redirect behavior (security boundary) + +`DestinationHttpClient` **does not follow HTTP redirects by default.** If the target system +responds with a 3xx, the redirect response is returned as-is. This is intentional: credentials +pre-baked into the session (auth tokens, API keys, `x-sap-*` headers) must never be forwarded +to a different origin implicitly. + +To follow same-host relative redirects, construct the client with `redirect_mode=RedirectMode.SAME_ORIGIN`: + +```python +from sap_cloud_sdk.destination import create_client, DestinationHttpClient, RedirectMode + +dest = client.get_destination("my-erp") +http = DestinationHttpClient(dest, redirect_mode=RedirectMode.SAME_ORIGIN) +response = http.request("GET", "/api/resource") +``` + +`SAME_ORIGIN` follows a redirect only when the `Location` URL resolves to the same scheme, +host, and port as the destination URL. Cross-origin redirects, port changes, HTTPS-to-HTTP +downgrades, and `Location` values with userinfo are silently rejected (the redirect response is +returned). Redirect chains are bounded at 5 hops. + +If the target system has permanently moved to a new URL, update the destination in BTP rather +than relying on redirect following. Passing `allow_redirects=True` in `**kwargs` has no effect +— it is silently removed. + ### Using `get_headers()` directly If you manage your own HTTP client, use `dest.get_headers()` to get all derived headers as a plain dict: diff --git a/tests/core/unit/test_http_client.py b/tests/core/unit/test_http_client.py index 83d4c997..e5c748c3 100644 --- a/tests/core/unit/test_http_client.py +++ b/tests/core/unit/test_http_client.py @@ -64,6 +64,12 @@ def test_401_without_auth_provider_is_not_retried(self): assert result.status_code == 401 assert mock_req.call_count == 1 + def test_execute_sets_allow_redirects_false(self): + """Redirects must never be followed — credentials could leak to another origin.""" + client, auth, session = _make_client_with_mock_session() + client.request(HttpMethod.GET, "/path") + assert session.request.call_args.kwargs.get("allow_redirects") is False + def test_200_response_not_retried(self): client, auth, session = _make_client_with_mock_session() result = client.request(HttpMethod.GET, "/data") diff --git a/tests/destination/unit/test_destination_http_client.py b/tests/destination/unit/test_destination_http_client.py index 529e21e4..2e871467 100644 --- a/tests/destination/unit/test_destination_http_client.py +++ b/tests/destination/unit/test_destination_http_client.py @@ -1,13 +1,14 @@ """Unit tests for DestinationHttpClient.""" import ssl -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, call, patch import pytest from sap_cloud_sdk.destination._destination_http_client import ( _ClientCertAdapter, DestinationHttpClient, + RedirectMode, ) from sap_cloud_sdk.destination._models import AuthToken, Destination from sap_cloud_sdk.destination.exceptions import DestinationCertificateError @@ -152,3 +153,151 @@ def test_cert_load_error_propagates(self): with patch(self._PATCH_TARGET, side_effect=DestinationCertificateError("boom")): with pytest.raises(DestinationCertificateError): DestinationHttpClient(_dest()) + + +# --------------------------------------------------------------------------- +# Helpers shared by redirect tests +# --------------------------------------------------------------------------- + +def _redirect(status: int, location: str) -> MagicMock: + r = MagicMock() + r.status_code = status + r.headers = {"Location": location} + return r + + +def _ok() -> MagicMock: + r = MagicMock() + r.status_code = 200 + r.headers = {} + return r + + +class TestDestinationHttpClientRedirect: + """Verify redirect-suppression and same-origin enforcement.""" + + def setup_method(self): + self.dest = _dest() + self.dest.auth_tokens = [ + _auth_token("Authorization", "Bearer secret"), + _auth_token("x-sap-security-session", "sess-token"), + _auth_token("apiKey", "k3y"), + ] + + # ------------------------------------------------------------------ + # Default NONE mode + # ------------------------------------------------------------------ + + def test_default_mode_is_none(self): + client = DestinationHttpClient(self.dest) + assert client._redirect_mode is RedirectMode.NONE + + def test_none_does_not_follow_302(self): + client = DestinationHttpClient(self.dest) + with patch.object(client._session, "request", return_value=_redirect(302, "/new")) as mock_req: + resp = client.request("GET", "/old") + assert resp.status_code == 302 + assert mock_req.call_count == 1 + assert mock_req.call_args.kwargs["allow_redirects"] is False + + @pytest.mark.parametrize("status", [301, 302, 303, 307, 308]) + def test_none_blocks_all_redirect_codes(self, status): + client = DestinationHttpClient(self.dest) + with patch.object(client._session, "request", return_value=_redirect(status, "/new")) as mock_req: + resp = client.request("GET", "/path") + assert resp.status_code == status + assert mock_req.call_count == 1 + + def test_allow_redirects_kwarg_is_silently_stripped(self): + client = DestinationHttpClient(self.dest) + with patch.object(client._session, "request", return_value=_ok()) as mock_req: + client.request("GET", "/path", allow_redirects=True) + assert mock_req.call_args.kwargs["allow_redirects"] is False + + def test_credentials_not_forwarded_to_redirect_target(self): + """In NONE mode the session is only called once — credentials never reach a redirect target.""" + client = DestinationHttpClient(self.dest) + with patch.object(client._session, "request", return_value=_redirect(302, "https://evil.com/capture")) as mock_req: + client.request("GET", "/api") + assert mock_req.call_count == 1 # second origin never receives a call + + # ------------------------------------------------------------------ + # SAME_ORIGIN mode + # ------------------------------------------------------------------ + + def test_same_origin_follows_relative_redirect(self): + client = DestinationHttpClient(self.dest, redirect_mode=RedirectMode.SAME_ORIGIN) + with patch.object(client._session, "request", side_effect=[_redirect(302, "/new-path"), _ok()]) as mock_req: + resp = client.request("GET", "/old-path") + assert resp.status_code == 200 + assert mock_req.call_count == 2 + assert mock_req.call_args_list[1].kwargs["url"] == "https://example.com/new-path" + + @pytest.mark.parametrize("status", [301, 302, 303, 307, 308]) + def test_same_origin_follows_all_redirect_codes_on_same_host(self, status): + client = DestinationHttpClient(self.dest, redirect_mode=RedirectMode.SAME_ORIGIN) + with patch.object(client._session, "request", side_effect=[_redirect(status, "/dest"), _ok()]) as mock_req: + resp = client.request("GET", "/src") + assert resp.status_code == 200 + assert mock_req.call_count == 2 + + def test_same_origin_rejects_host_change(self): + client = DestinationHttpClient(self.dest, redirect_mode=RedirectMode.SAME_ORIGIN) + with patch.object(client._session, "request", side_effect=[_redirect(302, "https://other.com/x")]) as mock_req: + resp = client.request("GET", "/path") + assert resp.status_code == 302 + assert mock_req.call_count == 1 + + def test_same_origin_rejects_port_change(self): + client = DestinationHttpClient(self.dest, redirect_mode=RedirectMode.SAME_ORIGIN) + with patch.object(client._session, "request", side_effect=[_redirect(302, "https://example.com:9443/x")]) as mock_req: + resp = client.request("GET", "/path") + assert resp.status_code == 302 + assert mock_req.call_count == 1 + + def test_same_origin_rejects_https_to_http_downgrade(self): + client = DestinationHttpClient(self.dest, redirect_mode=RedirectMode.SAME_ORIGIN) + with patch.object(client._session, "request", side_effect=[_redirect(302, "http://example.com/x")]) as mock_req: + resp = client.request("GET", "/path") + assert resp.status_code == 302 + assert mock_req.call_count == 1 + + def test_same_origin_rejects_userinfo_in_location(self): + client = DestinationHttpClient(self.dest, redirect_mode=RedirectMode.SAME_ORIGIN) + with patch.object(client._session, "request", side_effect=[_redirect(302, "https://user:pass@example.com/x")]) as mock_req: + resp = client.request("GET", "/path") + assert resp.status_code == 302 + assert mock_req.call_count == 1 + + def test_303_converts_to_get_and_drops_body(self): + client = DestinationHttpClient(self.dest, redirect_mode=RedirectMode.SAME_ORIGIN) + with patch.object(client._session, "request", side_effect=[_redirect(303, "/result"), _ok()]) as mock_req: + resp = client.request("POST", "/submit", json={"data": "v"}) + assert resp.status_code == 200 + second = mock_req.call_args_list[1].kwargs + assert second["method"] == "GET" + assert second["json"] is None + + def test_chain_stops_at_cross_origin_hop(self): + """First two hops are same-origin; third is cross-origin — stops and returns third 302.""" + client = DestinationHttpClient(self.dest, redirect_mode=RedirectMode.SAME_ORIGIN) + chain = [ + _redirect(302, "/hop1"), + _redirect(302, "/hop2"), + _redirect(302, "https://other.com/evil"), + ] + with patch.object(client._session, "request", side_effect=chain) as mock_req: + resp = client.request("GET", "/start") + assert resp.status_code == 302 + assert mock_req.call_count == 3 # third call returned the cross-origin redirect + + def test_max_hops_returns_last_redirect(self): + """When the hop limit is exhausted, the last redirect response is returned.""" + from sap_cloud_sdk.destination._destination_http_client import _MAX_REDIRECT_HOPS + client = DestinationHttpClient(self.dest, redirect_mode=RedirectMode.SAME_ORIGIN) + # _MAX_REDIRECT_HOPS same-origin hops + one that never fires + chain = [_redirect(302, f"/hop{i}") for i in range(_MAX_REDIRECT_HOPS + 1)] + with patch.object(client._session, "request", side_effect=chain) as mock_req: + resp = client.request("GET", "/start") + assert resp.status_code == 302 + assert mock_req.call_count == _MAX_REDIRECT_HOPS From 881bb42d7179ef0d1ecff6837195305bd4bb7ba9 Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger Date: Wed, 21 Oct 2026 15:32:12 -0300 Subject: [PATCH 2/3] fix(destination): revert version bump and fix import formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Revert pyproject.toml version 0.63.0 → 0.62.0 (version managed by release pipeline) - Update CHANGELOG to [Unreleased] section (version assigned at release time) - Fix ruff-format: expand DestinationHttpClient/RedirectMode import to multi-line --- CHANGELOG.md | 2 +- pyproject.toml | 2 +- src/sap_cloud_sdk/destination/__init__.py | 5 ++++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 677cf8ed..d662b628 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ All notable changes to this project will be documented in this file. Format: [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). -## [0.63.0] — 2026-10-21 +## [Unreleased] ### Security diff --git a/pyproject.toml b/pyproject.toml index 4669babf..d4835fa4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "sap-cloud-sdk" -version = "0.63.0" +version = "0.62.0" description = "SAP Cloud SDK for Python" readme = "README.md" license = "Apache-2.0" diff --git a/src/sap_cloud_sdk/destination/__init__.py b/src/sap_cloud_sdk/destination/__init__.py index 3db9aaaf..3e1e4f6f 100644 --- a/src/sap_cloud_sdk/destination/__init__.py +++ b/src/sap_cloud_sdk/destination/__init__.py @@ -52,7 +52,10 @@ _make_config_factory, ) from sap_cloud_sdk.core.protocol.http import HttpClient, XsuaaAuthProvider -from sap_cloud_sdk.destination._destination_http_client import DestinationHttpClient, RedirectMode +from sap_cloud_sdk.destination._destination_http_client import ( + DestinationHttpClient, + RedirectMode, +) from sap_cloud_sdk.destination.client import DestinationClient from sap_cloud_sdk.destination.fragment_client import FragmentClient from sap_cloud_sdk.destination.certificate_client import CertificateClient From 677e16c6acfe68446c17b8c1bca5738becabaadd Mon Sep 17 00:00:00 2001 From: Tiago Kochenborger Date: Wed, 21 Oct 2026 15:48:57 -0300 Subject: [PATCH 3/3] test(destination): remove unused mock import MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove `call` from the redirect test imports — only `call_count` and `call_args_list` are used; `call` itself was never referenced. --- tests/destination/unit/test_destination_http_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/destination/unit/test_destination_http_client.py b/tests/destination/unit/test_destination_http_client.py index 2e871467..1ebf012b 100644 --- a/tests/destination/unit/test_destination_http_client.py +++ b/tests/destination/unit/test_destination_http_client.py @@ -1,7 +1,7 @@ """Unit tests for DestinationHttpClient.""" import ssl -from unittest.mock import MagicMock, call, patch +from unittest.mock import MagicMock, patch import pytest