rusackas commented on code in PR #43395:
URL: https://github.com/apache/superset/pull/43395#discussion_r3832329794
##########
superset/reports/notifications/webhook.py:
##########
@@ -32,10 +36,104 @@
)
from superset.utils import json
from superset.utils.decorators import statsd_gauge
-from superset.utils.network import is_safe_host
+from superset.utils.network import is_safe_host, is_safe_ip
logger = logging.getLogger(__name__)
+# Number of characters of a failing response body kept in the server-side log
+# line. Response bodies are never folded into the exception message raised
+# back to the caller -- that message is persisted verbatim as
+# ``ReportExecutionLog.error_message`` and readable via the execution log
+# API, which would otherwise turn the webhook target into a readback oracle
+# for whatever it chooses to return (including an internal host reached via
+# DNS rebinding).
+_LOGGED_RESPONSE_BODY_LIMIT = 500
+
+
+def _raise_for_unsafe_peer(conn: HTTPConnection) -> None:
+ """
+ Validate that a connection's actual peer is publicly routable.
+
+ ``_validate_webhook_url`` resolves and checks the hostname once, ahead of
+ time; the connection opened here is resolved independently and may reach
+ a different address (DNS rebinding via a low-TTL record), so the check
+ has to be repeated against the address actually connected to.
+ """
+ sock = conn.sock
+ if sock is None:
+ return
+ peer = sock.getpeername()[0]
+ if not is_safe_ip(ipaddress.ip_address(peer)):
+ raise NotificationParamException("Webhook URL target host is not
allowed.")
+
+
+class _PeerValidatingHTTPConnection(HTTPConnection):
+ """HTTP connection that validates the peer address on connect."""
+
+ def connect(self) -> None:
+ super().connect()
+ _raise_for_unsafe_peer(self)
+
+
+class _PeerValidatingHTTPSConnection(HTTPSConnection):
+ """HTTPS connection that validates the peer address after the handshake."""
+
+ def connect(self) -> None:
+ super().connect()
+ _raise_for_unsafe_peer(self)
+
+
+class _PeerValidatingHTTPConnectionPool(HTTPConnectionPool):
+ ConnectionCls = _PeerValidatingHTTPConnection
+
+
+class _PeerValidatingHTTPSConnectionPool(HTTPSConnectionPool):
+ ConnectionCls = _PeerValidatingHTTPSConnection
+
+
+class _PeerValidatingHTTPAdapter(HTTPAdapter):
+ """
+ Transport adapter that routes requests through connection classes which
+ validate the connected peer address, closing the TOCTOU window between
+ the hostname check in ``_validate_webhook_url`` and the connection that
+ ``send()`` actually opens.
+
+ Mirrors the peer-validation approach used for dataset-import data URIs
+ (``superset.commands.dataset.importers.v1.utils``), adapted to
+ ``requests``/``urllib3`` connection pooling instead of ``urllib``.
+ """
+
+ def init_poolmanager(self, *args: Any, **kwargs: Any) -> None:
+ super().init_poolmanager(*args, **kwargs)
+ # Assign a new dict rather than mutating the manager's dict in
+ # place -- the attribute otherwise aliases urllib3's module-global
+ # default scheme-to-pool-class mapping.
+ self.poolmanager.pool_classes_by_scheme = {
+ "http": _PeerValidatingHTTPConnectionPool,
+ "https": _PeerValidatingHTTPSConnectionPool,
+ }
Review Comment:
Looked into this closely. requests/urllib3's ProxyManager does route through
separate connection pools, but for a proxied connection the client's own socket
only ever connects to the proxy: for HTTPS-via-CONNECT urllib3's
HTTPSConnectionPool._new_conn() dials self.proxy.host/self.proxy.port (not the
destination), and for plain HTTP-via-proxy there's no separate destination
connection at all. So sock.getpeername() in the proxy pool would only ever see
the proxy's own address, never the destination's. Wiring the validating classes
into the proxy pool wouldn't validate the webhook target at all, it'd validate
the proxy, which commonly sits at a private address on purpose. Proxy selection
here is an operator-configured deployment decision (env/session config), and
DNS resolution/connection to the destination in that case is delegated to that
proxy, outside what this process can peer-validate.
##########
superset/reports/notifications/webhook.py:
##########
@@ -201,14 +300,29 @@ def send(self) -> None:
)
if response.status_code >= 500 or response.status_code == 429:
+ # The response body is logged server-side only (and
+ # truncated) -- it must not be folded into the exception
+ # message, which is persisted as the report execution log's
+ # error message and surfaced back to whoever can read that
+ # log, turning the webhook target into a readback oracle.
+ logger.warning(
+ "Webhook to %s failed with status code %s: %s",
+ wh_url,
+ response.status_code,
+ response.text[:_LOGGED_RESPONSE_BODY_LIMIT],
+ )
Review Comment:
Good catch, fixed. Control characters (newlines, CR, etc.) in the response
body are now escaped before being written to the log line.
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -4222,13 +4261,15 @@ def test_screenshot_invalid_download_format(
):
self.login(ADMIN_USERNAME)
mock_cache_task.return_value = None
- mock_get_from_cache_key.return_value = ScreenshotCachePayload(b"fake
png data")
dashboard = (
db.session.query(Dashboard)
.filter(Dashboard.dashboard_title == "dash with tag")
.first()
)
+ mock_get_from_cache_key.return_value = ScreenshotCachePayload(
+ b"fake png data", scope=f"dashboard:{dashboard.id}"
+ )
Review Comment:
Good catch, fixed. The test now restores a valid, correctly-scoped payload
before the GET with the invalid format, so it actually exercises the
download_format branch instead of failing the scope check first.
--
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]
