rusackas opened a new pull request, #45766: URL: https://github.com/apache/superset/pull/45766
### SUMMARY `superset/utils/network.py` exposes `get_ssrf_safe_requester(allow_unsafe_hosts=False)`, a `requests`-compatible object that re-validates the actually-connected peer IP on every request, closing the DNS-rebinding/TOCTOU gap that an upfront `is_safe_host` check alone leaves open. Its docstring explicitly calls out that it exists so other outbound-request call sites can reuse it instead of reimplementing the same peer-validation machinery (`superset/db_engine_specs/base.py` already does this for the OAuth2 token/authorization endpoints). Two call sites had their own copies of this machinery instead of reusing it: - **`superset/reports/notifications/webhook.py`**: carried a private, near-identical copy of the peer-validation classes (`_raise_for_unsafe_peer`, `_PeerValidatingHTTPConnection`/`_PeerValidatingHTTPSConnection`, `_PeerValidatingHTTPConnectionPool`/`_PeerValidatingHTTPSConnectionPool`, `_PeerValidatingHTTPAdapter`) and a `_get_requester()` helper. This PR deletes the duplicate and the now-unused imports it pulled in (`ipaddress`, `HTTPAdapter`, `HTTPConnection`/`HTTPSConnection`, `HTTPConnectionPool`/`HTTPSConnectionPool`, `is_safe_ip`), and replaces the `_get_requester()` call site with `get_ssrf_safe_requester(allow_unsafe_hosts=...)`, threading through the same `ALERT_REPORTS_WEBHOOK_ALLOW_INTERNAL_HOSTS` config flag `_get_requester()` already used — no behavior change, same opt-in. - **`superset/db_engine_specs/impala.py`**: `cancel_query` validated the configured Impala host up front via `is_safe_host` (kept, since it's a deliberate "refuse to even try" early exit gated by `IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS`) but then issued the actual cancel request with a raw `requests.post(...)`, missing the peer-repin-on-connect step. This PR replaces that call with `get_ssrf_safe_requester(allow_unsafe_hosts=...)` using the same `IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS` flag, keeping the same `timeout=3, allow_redirects=False` arguments. Both call sites are reached with operator/admin-configured destinations (a configured webhook URL; a database's configured host), consistent with this repo's `SECURITY.md` threat model where operator-configured destinations are a trust boundary the codebase already decided deserves peer-revalidation for this class of outbound request — this closes an internal-consistency gap in that existing protection mechanism across all three call sites, rather than introducing new protection. Tests for both files were updated only where they directly asserted against now-removed implementation details: - `tests/unit_tests/reports/notifications/webhook_tests.py`: removed 4 tests that exercised the webhook module's now-deleted private `_PeerValidatingHTTPConnection`/`_PeerValidatingHTTPAdapter`/`_get_requester` symbols directly — equivalent coverage already exists in `tests/unit_tests/utils/test_network.py` against the shared `superset.utils.network` implementation. One test that mocked `_get_requester` now mocks `get_ssrf_safe_requester` instead. All other webhook tests (including the DNS-rebinding end-to-end regression test) pass unchanged, since they already work at the `requests.post`/`current_app.config` level. - `tests/unit_tests/db_engine_specs/test_impala.py`: the three `cancel_query` tests that previously patched `requests.post` directly now patch `superset.db_engine_specs.impala.get_ssrf_safe_requester`, since the default (non-opt-out) path now returns a `Session` rather than the `requests` module. The opt-out test still patches `requests.post` directly, since `allow_unsafe_hosts=True` returns the plain `requests` module unchanged. No new SSRF protection is being introduced here; this is closing an inconsistency so the existing, already-reviewed peer-validation approach applies uniformly. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A -- backend-only refactor, no UI change. ### TESTING INSTRUCTIONS ``` python -m pytest tests/unit_tests/reports/notifications/webhook_tests.py -q python -m pytest tests/unit_tests/db_engine_specs/test_impala.py -q ``` Both suites pass. `pre-commit run` (staged files only) passes clean (ruff-format, ruff, mypy, pylint). ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
