codeant-ai-for-open-source[bot] commented on code in PR #43395:
URL: https://github.com/apache/superset/pull/43395#discussion_r3831873378
##########
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:
**Suggestion:** The peer validation only replaces the direct `PoolManager`
scheme mappings. When Requests uses an HTTP or HTTPS proxy from its environment
or configuration, it creates a separate proxy manager with the standard urllib3
connection pools, so these validating connection classes are bypassed and the
checked peer is the proxy rather than the webhook destination. This leaves the
DNS-rebinding/private-target protection ineffective for proxied webhook
requests; configure the proxy pools to use the validating classes or explicitly
handle proxy resolution. [ssrf]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Proxied webhooks can reach rebinding-selected private addresses.
- ⚠️ SSRF protection depends on proxy configuration.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/notifications/webhook.py
**Line:** 106:114
**Comment:**
*Ssrf: The peer validation only replaces the direct `PoolManager`
scheme mappings. When Requests uses an HTTP or HTTPS proxy from its environment
or configuration, it creates a separate proxy manager with the standard urllib3
connection pools, so these validating connection classes are bypassed and the
checked peer is the proxy rather than the webhook destination. This leaves the
DNS-rebinding/private-target protection ineffective for proxied webhook
requests; configure the proxy pools to use the validating classes or explicitly
handle proxy resolution.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43395&comment_hash=c11b7a51657cbd1d8f7233cb1cf7066f922c5ee4c139cbdca28f13ec8f32c2b9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43395&comment_hash=c11b7a51657cbd1d8f7233cb1cf7066f922c5ee4c139cbdca28f13ec8f32c2b9&reaction=dislike'>👎</a>
##########
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:
**Suggestion:** The webhook controls the entire response body, including
newline and terminal-control characters, and the new warning logs it directly.
A malicious or compromised endpoint can therefore forge additional log records
or corrupt structured/line-oriented log ingestion. Sanitize or escape control
characters before logging response content, or omit the body from logs.
[security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Failed webhook responses can forge server-log lines.
- ⚠️ Structured or line-oriented log ingestion may be corrupted.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/reports/notifications/webhook.py
**Line:** 308:313
**Comment:**
*Security: The webhook controls the entire response body, including
newline and terminal-control characters, and the new warning logs it directly.
A malicious or compromised endpoint can therefore forge additional log records
or corrupt structured/line-oriented log ingestion. Sanitize or escape control
characters before logging response content, or omit the body from logs.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43395&comment_hash=b17ab88587522ef5ed489c6fd1f6b6fa2cea8d54f8fe774e682fcdac3cbe5fa4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43395&comment_hash=b17ab88587522ef5ed489c6fd1f6b6fa2cea8d54f8fe774e682fcdac3cbe5fa4&reaction=dislike'>👎</a>
--
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]
