Skip to content

fix: improve url injection safeguards - #373

Open
betinacosta wants to merge 5 commits into
mainfrom
fix/tenant-subdomain-token-url-injection
Open

betinacosta wants to merge 5 commits into
mainfrom
fix/tenant-subdomain-token-url-injection

Conversation

@betinacosta

Copy link
Copy Markdown
Member

Disclaimer: Do not include SAP-internal or customer-specific information in this PR (e.g. internal system URLs, customer names, tenant IDs, or confidential configurations). This is a public repository.

Description

Replaces the unconstrained str.replace used to derive per-tenant OAuth2 token URLs in XsuaaAuthProvider with a URL-aware substitution that only replaces the first DNS label of the configured token URL hostname.

Background: When a subscriber tenant subdomain is provided for Destination Service access, XsuaaAuthProvider._fetch_token previously derived the tenant-scoped token URL by calling str(token_url).replace(str(identityzone), tenant_subdomain). This is URL-unaware: if the identityzone value also appears in the path or query portion of the token URL, all occurrences are replaced, not just the hostname label. Although a pre-existing _validate_tenant_subdomain call already blocks authority-injection payloads (slashes, dots, etc.), the str.replace approach is semantically incorrect and a source of latent correctness bugs.

Fix: A new helper _derive_tenant_token_url in sap_cloud_sdk.core._tenant uses urllib.parse.urlparse / urlunparse to precisely swap only the leading hostname label when it matches identityzone. Every other URL component — scheme, port, path, query, fragment — is preserved verbatim. If the first label does not match identityzone, the original URL is returned unchanged.

Related Issue

Closes #

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Code refactoring
  • Dependency update

How to Test

  1. Run the unit tests added in this PR:
    uv run pytest tests/core/unit/test_tenant.py tests/core/unit/test_http_client.py -v
  2. Verify all 66 tests pass.
  3. To confirm the path-collision fix specifically, check test_identityzone_in_path_is_not_replaced in tests/core/unit/test_http_client.py — this test fails against the old str.replace implementation and passes with _derive_tenant_token_url.

Checklist

  • I have read the Contributing Guidelines
  • I have verified that my changes solve the issue
  • I have added/updated automated tests to cover my changes
  • All tests pass locally
  • I have verified that my code follows the Code Guidelines
  • I have updated documentation (if applicable)
  • I have added type hints for all public APIs
  • My code does not contain sensitive information (credentials, tokens, etc.)
  • I have followed Conventional Commits for commit messages

Additional Notes

_derive_tenant_token_url is intentionally internal (prefixed _). It is co-located with _validate_tenant_subdomain in sap_cloud_sdk.core._tenant so all tenant-subdomain safety logic lives in one place.

The fix has no effect on the normal happy path: a valid identityzone that is the first hostname label produces the same derived URL as before. Only the edge case where identityzone also appears elsewhere in the URL is handled differently (correctly).

@betinacosta
betinacosta marked this pull request as ready for review October 5, 2026 18:22
@betinacosta
betinacosta requested a review from a team as a code owner October 5, 2026 18:22

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant