codeant-ai-for-open-source[bot] commented on code in PR #39646:
URL: https://github.com/apache/superset/pull/39646#discussion_r3206364292
##########
superset/db_engine_specs/base.py:
##########
@@ -810,6 +811,28 @@ def get_oauth2_token(
response.raise_for_status()
return response.json()
+ @classmethod
+ def _validate_oauth2_token_uri(cls, uri: str) -> None:
+ """Validate OAuth2 token URI to prevent SSRF attacks."""
+ import ipaddress
+ import socket as _socket
+ from urllib.parse import urlparse
+
+ parsed = urlparse(uri)
+ if parsed.scheme not in ("https", "http"):
+ raise ValueError(
+ f"OAuth2 token URI must use http or https scheme, got:
{parsed.scheme!r}"
+ )
+ hostname = parsed.hostname or ""
+ try:
+ ip = ipaddress.ip_address(_socket.gethostbyname(hostname))
Review Comment:
Yes — this review comment is **valid**.
`gethostbyname()` only checks a single resolved IPv4 address, so a hostname
with multiple DNS records can still slip through if `requests.post()` connects
to a different IP. That means the current validation does **not fully prevent
DNS-based SSRF**.
### Recommended fix
Validate **all resolved addresses** for the hostname and reject the URI if
**any** of them are private / loopback / link-local / reserved.
A concise improvement would be to switch from `gethostbyname()` to
`getaddrinfo()` and validate every returned IP:
```python
import ipaddress
import socket as _socket
from urllib.parse import urlparse
parsed = urlparse(uri)
hostname = parsed.hostname or ""
addresses = {
info[4][0]
for info in _socket.getaddrinfo(hostname, None)
}
for addr in addresses:
ip = ipaddress.ip_address(addr)
if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_reserved:
raise ValueError(
"OAuth2 token URI must not point to a private or reserved
network address"
)
```
### Why this is better
- Covers **multiple DNS results**
- Prevents the “one public IP passes, one private IP is used” bypass
- Keeps the change small and localized
So yes, I’d recommend updating this validation before merging. If you want,
I can also review the other comments on this PR and help apply the remaining
minimal fixes too.
--
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]
