Skip to content

fix: validate redirects on integration requests to customer-supplied hosts - #1126

Merged
rohan-chaturvedi merged 4 commits into
mainfrom
fix/ph-004-integration-redirect-checks
Nov 12, 2026
Merged

rohan-chaturvedi merged 4 commits into
mainfrom
fix/ph-004-integration-redirect-checks

Conversation

@rohan-chaturvedi

@rohan-chaturvedi rohan-chaturvedi commented Nov 10, 2026 •

Copy link
Copy Markdown
Member

🔍 Overview

Hardening for outbound requests that integrations make to a host the customer supplies (self-hosted GitHub, Vault, Nomad). On Phase Cloud the host is already checked before a request is made; that check now also applies to every redirect a request follows, so all hops are held to the same rule as the first.

💡 Proposed Changes

  • Add api/utils/safe_requests.py: a small requests.Session subclass (plus get / put / delete / request helpers) that, on Phase Cloud, runs validate_url_is_safe on every request it sends, redirects included. Self-hosted deployments are unaffected.
  • Use it in the GitHub Actions, GitHub Dependabot, Nomad and Vault providers. Redirects to allowed hosts are still followed, so existing setups keep working.
  • The AWS IAM identity check no longer follows redirects from the configured STS endpoint (STS does not redirect).
  • A refused request raises a requests.RequestException subclass, so each provider's existing error handling reports it.
  • GitHub Enterprise: organisation listing now uses the API host stored with the credential, as repository and environment listing already do.

🖼️ Screenshots or Demo

N/A — backend change, no UI surface.

📝 Release Notes

Hardened outbound request handling for integrations that connect to customer-supplied hosts.

Fixed: organisation listing for GitHub Enterprise credentials now calls the Enterprise host.

❓ Open Questions

N/A

🧪 Testing

  • New tests run requests' real redirect handling over a fake transport and fake DNS: redirects are checked on every hop on Phase Cloud, allowed redirects are still followed, self-hosted behaviour is unchanged, and each provider (GitHub Actions, Dependabot, Nomad, Vault, AWS IAM identity) is covered end to end.
  • Existing GitHub provider tests updated for the new call path.
  • New test for the three GitHub listing calls (repositories, organisations, environments) with GitHub.com and GitHub Enterprise credentials. Organisation listing was also checked on a local instance against a stand-in Enterprise API.
  • Full backend test suite passing locally.

🎯 Reviewer Focus

backend/api/utils/safe_requests.py (SafeSession.send), then the call sites in api/utils/syncing/{github,nomad,vault} and api/views/identities/aws/iam.py. For the GitHub Enterprise fix: list_orgs in api/utils/syncing/github/actions.py.

➕ Additional Context

N/A

✨ How to Test the Changes Locally

docker compose -f dev-docker-compose.yml exec backend pytest \
  tests/api/utils/test_safe_requests.py \
  tests/utils/syncing/test_github_actions.py \
  tests/utils/syncing/test_github_dependabot.py \
  tests/api/views/identities/aws/test_iam_unit.py

💚 Did You...

  • Ensure linting passes (code style checks)?
  • Update dependencies and lockfiles (if required) — N/A
  • Update migrations (if required) — N/A
  • Regenerate graphql schema and types (if required) — N/A
  • Verify the app builds locally?
  • Manually test the changes on different browsers/devices? — N/A (backend-only)

Organisation listing looked for the API host under a different credential
key than repository and environment listing, so it always used the
GitHub.com API. Read the same key as the other two.

The new test runs the three listing calls with the credentials the OAuth
callback stores, for GitHub.com and for GitHub Enterprise.
@rohan-chaturvedi
rohan-chaturvedi merged commit 8ac31d8 into main Nov 12, 2026
15 checks passed
@rohan-chaturvedi
rohan-chaturvedi deleted the fix/ph-004-integration-redirect-checks branch November 12, 2026 09:01
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.

2 participants