You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "sha174n (via GitHub)" <gi...@apache.org> on 2026/04/12 12:07:17 UTC
[PR] fix(security): block SSRF in dataset import URL validation [superset]
sha174n opened a new pull request, #39301:
URL: https://github.com/apache/superset/pull/39301
### SUMMARY
Hardens dataset import URL validation against SSRF attacks:
1. **Default `DATASET_IMPORT_ALLOWED_DATA_URLS` to `[]`** — previously `[r".*"]` allowed any URL. Now non-file:// URIs are denied unless an admin explicitly allows patterns.
2. **Add `is_safe_host()` utility** (`superset/utils/network.py`) — resolves a hostname via `socket.getaddrinfo()` and rejects it if any resolved address falls within private, loopback, or link-local ranges (RFC-1918, 127/8, 169.254/16, ::1, fc00::/7, fe80::/10). Covers the `169.254.169.254` cloud metadata endpoint used in AWS/GCP/Azure IMDS attacks.
3. **Fix userinfo-injection bypass in `validate_data_uri()`** — a URL like `https://allowed.example.com@169.254.169.254/` passes a regex allowlist (the trusted domain appears first) but `urlparse().hostname` correctly resolves to the actual target IP. The fix checks `is_safe_host(urlparse(uri).hostname)` after the allowlist match.
4. **Add `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS: bool = False`** config flag — escape hatch for air-gapped or on-premises deployments where the data source lives on an internal network. Defaults to False (safe) in all internet-facing deployments.
`file://` URIs continue to work unconditionally — they are used by `normalize_example_data_url()` for bundled example data and make no network requests.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no UI changes.
### TESTING INSTRUCTIONS
```bash
# Run unit tests
pytest tests/unit_tests/utils/test_network.py -v
pytest tests/unit_tests/datasets/commands/importers/v1/import_test.py -v -k "validate_data_uri"
```
**Manual verification:**
- Set `DATASET_IMPORT_ALLOWED_DATA_URLS = [r".*"]` and attempt to import a dataset with `data: "https://trusted.example.com@169.254.169.254/"` — should raise `DatasetForbiddenDataURI`
- Set `DATASET_IMPORT_ALLOWED_DATA_URLS = []` — any `https://` URI should raise `DatasetForbiddenDataURI`
- `file://` URIs should always succeed regardless of config
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration
- [x] Introduces new feature or API — `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS` config flag, `is_safe_host()` utility
- [ ] Removes existing feature or API
> **Note:** `DATASET_IMPORT_ALLOWED_DATA_URLS` default changed from `[r".*"]` to `[]`. Deployments that rely on importing datasets from remote URLs will need to add explicit patterns to this config key.
🤖 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4367181951
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4594348326
Looks like CI just needs a little love, then we can merge this!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): block SSRF in dataset import URL validation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3069434278
##########
superset/utils/network.py:
##########
@@ -14,14 +14,55 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import ipaddress
import platform
import socket
import subprocess
+# Networks that must never be reached via user-supplied hostnames.
+# Includes loopback, RFC-1918 private ranges, link-local (covers cloud
+# metadata endpoints such as 169.254.169.254), and IPv6 equivalents.
+_SSRF_UNSAFE_NETWORKS = (
+ ipaddress.ip_network("0.0.0.0/8"),
+ ipaddress.ip_network("10.0.0.0/8"),
+ ipaddress.ip_network("127.0.0.0/8"),
+ ipaddress.ip_network("169.254.0.0/16"),
+ ipaddress.ip_network("172.16.0.0/12"),
+ ipaddress.ip_network("192.168.0.0/16"),
+ ipaddress.ip_network("::1/128"),
+ ipaddress.ip_network("fc00::/7"),
+ ipaddress.ip_network("fe80::/10"),
+)
+
PORT_TIMEOUT = 5
PING_TIMEOUT = 5
+def is_safe_host(host: str) -> bool:
+ """
+ Return True if ``host`` resolves exclusively to public, globally-routable
+ IP addresses.
+
+ Returns False if any resolved address falls within a private, loopback,
+ link-local, or otherwise non-routable range. An unresolvable host also
+ returns False.
+ """
+ try:
+ results = socket.getaddrinfo(host, None)
+ except socket.gaierror:
+ return False
+ if not results:
+ return False
+ for _, _, _, _, sockaddr in results:
+ try:
+ ip = ipaddress.ip_address(sockaddr[0])
+ except ValueError:
+ return False
+ if any(ip in net for net in _SSRF_UNSAFE_NETWORKS):
+ return False
Review Comment:
**🔴 Architect Review — CRITICAL**
is_safe_host() only blocks a hard-coded set of CIDR ranges and misses encodings like IPv4-mapped IPv6 (e.g. ::ffff:127.0.0.1), so a URL such as http://[::ffff:127.0.0.1]/... would be treated as "safe" and still reach loopback, leaving an SSRF bypass.
**Suggestion:** Instead of a manual CIDR blocklist, classify each resolved address using ipaddress' global-routability flags (treat any non-global address—including IPv4-mapped, translated, or otherwise special-use forms—as unsafe) so loopback/private targets are consistently rejected.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -82,12 +84,20 @@ def get_dtype(df: pd.DataFrame, dataset: SqlaTable) -> dict[str, VisitableType]:
def validate_data_uri(data_uri: str) -> None:
Review Comment:
**🟠 Architect Review — HIGH**
validate_data_uri() only validates the initially requested host, but load_data() then calls urllib.request.urlopen(), which will transparently follow HTTP redirects; an allowed public URL that 302-redirects to 169.254.169.254 or another internal/loopback address will bypass the one-time host safety check and still trigger SSRF.
**Suggestion:** Disable automatic redirects for this import path or install a custom redirect handler that re-runs the same host safety validation (including internal-IP checks) on every redirect target before following it.
##########
superset/utils/network.py:
##########
@@ -14,14 +14,55 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import ipaddress
import platform
import socket
import subprocess
+# Networks that must never be reached via user-supplied hostnames.
+# Includes loopback, RFC-1918 private ranges, link-local (covers cloud
+# metadata endpoints such as 169.254.169.254), and IPv6 equivalents.
+_SSRF_UNSAFE_NETWORKS = (
+ ipaddress.ip_network("0.0.0.0/8"),
+ ipaddress.ip_network("10.0.0.0/8"),
+ ipaddress.ip_network("127.0.0.0/8"),
Review Comment:
**Suggestion:** The unsafe network list omits `100.64.0.0/10` (shared address space), which includes cloud metadata endpoints such as `100.100.100.200` used in some environments. Because this range is currently treated as safe, SSRF protections can be bypassed to reach internal metadata services. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset import SSRF to shared-address-space internal endpoints.
- ❌ Cloud metadata on 100.64.0.0/10 potentially exposed.
```
</details>
```suggestion
ipaddress.ip_network("100.64.0.0/10"),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure dataset import to allow broad external URLs by setting
`DATASET_IMPORT_ALLOWED_DATA_URLS = [r\".*\"]` and
`DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS = False` (these configuration keys are consumed
by `validate_data_uri()` in `superset/commands/dataset/importers/v1/utils.py:32-44`).
2. In an environment where internal or metadata services are exposed in the shared address
space `100.64.0.0/10` (for example, a cloud metadata endpoint such as
`http://100.100.100.200/`), initiate a dataset import so that `load_data(data_uri, ...)`
in `superset/commands/dataset/importers/v1/utils.py:3-20` is called with `data_uri`
pointing to that IP.
3. `load_data()` normalizes the URL and calls `validate_data_uri(data_uri)` at
`utils.py:15`, which, for non-`file://` URIs, matches the URL against
`DATASET_IMPORT_ALLOWED_DATA_URLS` and then, because
`DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS` is False, extracts the hostname and calls
`is_safe_host(hostname)` at `utils.py:45-48` with `hostname="100.100.100.200"`.
4. In `is_safe_host()` (`superset/utils/network.py:41-63`), `socket.getaddrinfo()` returns
an address tuple with `resolved_ip="100.100.100.200"`, `ipaddress.ip_address(resolved_ip)`
at `network.py:56-58` produces an IPv4Address that is *not* contained in any of the
networks in `_SSRF_UNSAFE_NETWORKS` (`0.0.0.0/8`, `10.0.0.0/8`, `127.0.0.0/8`,
`169.254.0.0/16`, `172.16.0.0/12`, `192.168.0.0/16`, and IPv6 ranges at
`network.py:25-34`), so `is_safe_host()` returns True at `network.py:63`,
`validate_data_uri()` returns at `utils.py:49`, and `request.urlopen(data_uri)` at
`utils.py:17` issues an HTTP request to the internal shared-address-space endpoint,
enabling SSRF to metadata or other internal services within `100.64.0.0/10`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/network.py
**Line:** 28:28
**Comment:**
*Security: The unsafe network list omits `100.64.0.0/10` (shared address space), which includes cloud metadata endpoints such as `100.100.100.200` used in some environments. Because this range is currently treated as safe, SSRF protections can be bypassed to reach internal metadata services.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=f4b8b49e0595e28f081598ddac357282b998448b7207a64e6d42536428136298&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=f4b8b49e0595e28f081598ddac357282b998448b7207a64e6d42536428136298&reaction=dislike'>👎</a>
##########
superset/utils/network.py:
##########
@@ -14,14 +14,55 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import ipaddress
import platform
import socket
import subprocess
+# Networks that must never be reached via user-supplied hostnames.
+# Includes loopback, RFC-1918 private ranges, link-local (covers cloud
+# metadata endpoints such as 169.254.169.254), and IPv6 equivalents.
+_SSRF_UNSAFE_NETWORKS = (
+ ipaddress.ip_network("0.0.0.0/8"),
+ ipaddress.ip_network("10.0.0.0/8"),
+ ipaddress.ip_network("127.0.0.0/8"),
+ ipaddress.ip_network("169.254.0.0/16"),
+ ipaddress.ip_network("172.16.0.0/12"),
+ ipaddress.ip_network("192.168.0.0/16"),
+ ipaddress.ip_network("::1/128"),
+ ipaddress.ip_network("fc00::/7"),
+ ipaddress.ip_network("fe80::/10"),
+)
+
PORT_TIMEOUT = 5
PING_TIMEOUT = 5
+def is_safe_host(host: str) -> bool:
+ """
+ Return True if ``host`` resolves exclusively to public, globally-routable
+ IP addresses.
+
+ Returns False if any resolved address falls within a private, loopback,
+ link-local, or otherwise non-routable range. An unresolvable host also
+ returns False.
+ """
+ try:
+ results = socket.getaddrinfo(host, None)
+ except socket.gaierror:
+ return False
+ if not results:
+ return False
+ for _, _, _, _, sockaddr in results:
+ try:
+ ip = ipaddress.ip_address(sockaddr[0])
+ except ValueError:
Review Comment:
**Suggestion:** IPv4-mapped IPv6 addresses are not normalized before network checks, so an address like `::ffff:127.0.0.1` is treated as safe even though it routes to loopback. This leaves an SSRF bypass path to internal targets. Normalize mapped IPv6 addresses to their IPv4 equivalent before checking unsafe ranges. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset import SSRF to loopback via IPv4-mapped IPv6.
- ❌ Internal services on 127.0.0.1 reachable from `load_data()`.
```
</details>
```suggestion
if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped:
ip = ip.ipv4_mapped
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure dataset import to allow broad URLs by setting
`DATASET_IMPORT_ALLOWED_DATA_URLS = [r".*"]` and `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS
= False` (these config keys are read in `validate_data_uri()` at
`superset/commands/dataset/importers/v1/utils.py:32-44`).
2. Trigger dataset import using a data URL with an IPv4-mapped IPv6 literal pointing to
loopback, for example `http://[::ffff:127.0.0.1]/data.csv`, so that `load_data()` in
`superset/commands/dataset/importers/v1/utils.py:3-20` is called with this `data_uri`.
3. Inside `load_data()`, the function calls `validate_data_uri(data_uri)` at
`utils.py:15`, which for non-`file://` URIs matches the URL against the allowlist and,
because `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS` is False, resolves the hostname and
calls `is_safe_host(hostname)` at `utils.py:45-48` with `hostname="::ffff:127.0.0.1"`.
4. In `is_safe_host()` (`superset/utils/network.py:41-63`), `socket.getaddrinfo(host,
None)` returns an address tuple containing the IPv6 literal `::ffff:127.0.0.1`; this is
converted to an `IPv6Address` by `ipaddress.ip_address(sockaddr[0])` at
`network.py:56-58`, and the membership check `ip in net for net in _SSRF_UNSAFE_NETWORKS`
at `network.py:61-62` does not match any entry because `_SSRF_UNSAFE_NETWORKS` only
includes IPv4 loopback `127.0.0.0/8` and certain IPv6 ranges, not the IPv4-mapped form, so
`is_safe_host()` returns True at `network.py:63`, `validate_data_uri()` returns at
`utils.py:49`, and `request.urlopen(data_uri)` at `utils.py:17` successfully issues an
HTTP request to `127.0.0.1`, allowing SSRF to loopback/internal services.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/network.py
**Line:** 59:59
**Comment:**
*Security: IPv4-mapped IPv6 addresses are not normalized before network checks, so an address like `::ffff:127.0.0.1` is treated as safe even though it routes to loopback. This leaves an SSRF bypass path to internal targets. Normalize mapped IPv6 addresses to their IPv4 equivalent before checking unsafe ranges.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=eb35e058136c4650e954dcf383aef0943c5ebd0a528252d5dc3d0b774362c9c9&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=eb35e058136c4650e954dcf383aef0943c5ebd0a528252d5dc3d0b774362c9c9&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3405762800
##########
superset/utils/network.py:
##########
@@ -14,14 +14,61 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import ipaddress
import platform
import socket
import subprocess
+# Networks that must never be reached via user-supplied hostnames.
+# Includes loopback, RFC-1918 private ranges, link-local (covers cloud
+# metadata endpoints such as 169.254.169.254), shared address space
+# (RFC 6598, 100.64.0.0/10), and IPv6 equivalents.
+_SSRF_UNSAFE_NETWORKS = (
+ ipaddress.ip_network("0.0.0.0/8"),
+ ipaddress.ip_network("10.0.0.0/8"),
+ ipaddress.ip_network("100.64.0.0/10"),
+ ipaddress.ip_network("127.0.0.0/8"),
+ ipaddress.ip_network("169.254.0.0/16"),
+ ipaddress.ip_network("172.16.0.0/12"),
+ ipaddress.ip_network("192.168.0.0/16"),
+ ipaddress.ip_network("::1/128"),
+ ipaddress.ip_network("fc00::/7"),
+ ipaddress.ip_network("fe80::/10"),
+)
+
PORT_TIMEOUT = 5
PING_TIMEOUT = 5
+def is_safe_host(host: str) -> bool:
+ """
+ Return True if ``host`` resolves exclusively to public, globally-routable
+ IP addresses.
+
+ Returns False if any resolved address falls within a private, loopback,
+ link-local, or otherwise non-routable range. An unresolvable host also
+ returns False.
+ """
+ try:
+ results = socket.getaddrinfo(host, None)
+ except socket.gaierror:
+ return False
+ if not results:
+ return False
+ for _, _, _, _, sockaddr in results:
+ try:
+ ip = ipaddress.ip_address(sockaddr[0])
+ except ValueError:
+ return False
+ # Unwrap IPv4-mapped IPv6 addresses (e.g. ::ffff:127.0.0.1) so they
+ # are checked against the IPv4 unsafe networks rather than bypassing.
+ if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped:
+ ip = ip.ipv4_mapped
+ if any(ip in net for net in _SSRF_UNSAFE_NETWORKS):
Review Comment:
Addressed: `is_global` is the primary gate, backed by an explicit unsafe-network tuple (loopback, RFC-1918, link-local, 100.64.0.0/10, multicast) plus IPv4-mapped IPv6 unwrapping.
##########
superset/reports/notifications/webhook.py:
##########
@@ -93,6 +94,16 @@ def _get_files(self) -> list[tuple[str, tuple[str, bytes, str]]]:
)
return files
+ def _validate_webhook_url(self, url: str) -> None:
+ parsed = urlparse(url)
+ if current_app.config["ALERT_REPORTS_WEBHOOK_HTTPS_ONLY"]:
+ if parsed.scheme.lower() != "https":
+ raise NotificationParamException(
+ "Webhook failed: HTTPS is required by config for webhook URLs."
+ )
+ if parsed.hostname and not is_safe_host(parsed.hostname):
Review Comment:
Addressed: `_validate_webhook_url` rejects non-HTTP(S) schemes and missing hostnames before the host check.
##########
superset/db_engine_specs/impala.py:
##########
@@ -209,6 +210,8 @@ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
"""
try:
impala_host = query.database.url_object.host
+ if not impala_host or not is_safe_host(impala_host):
+ return False
url = f"http://{impala_host}:25000/cancel_query?query_id={cancel_query_id}"
response = requests.post(url, timeout=3)
Review Comment:
Addressed: gated behind `IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS` so internal Impala deployments can opt out (test hardened in 2e7e54d5ff).
##########
superset/db_engine_specs/couchbase.py:
##########
@@ -228,7 +228,7 @@ def validate_parameters(
if not host:
return errors
# host can be a connection string in case of couchbase cloud. So Connection Check is not required in that case. # noqa: E501
- if not is_hostname_valid(host):
+ if not is_safe_host(host):
Review Comment:
Reverted: couchbase keeps `is_hostname_valid`, so connection strings and internal hosts are unaffected.
##########
superset/db_engine_specs/databricks.py:
##########
@@ -393,7 +393,7 @@ def validate_parameters( # type: ignore
if not host:
return errors
- if not is_hostname_valid(host): # type: ignore
+ if not is_safe_host(host): # type: ignore
errors.append(
SupersetError(
message="The hostname provided can't be resolved.",
Review Comment:
Reverted: databricks keeps `is_hostname_valid`; the host-check change was dropped from this PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3408323739
##########
superset/db_engine_specs/impala.py:
##########
@@ -209,8 +210,25 @@ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
"""
try:
impala_host = query.database.url_object.host
+ # The cancel call issues an outbound HTTP request from the
+ # Superset backend to whatever host the DB connection was
+ # configured with; validate it before the call to keep this
+ # path consistent with the dataset-import and webhook URL
+ # checks. Operators with internal Impala targets can opt out
+ # via IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS.
+ if not impala_host:
+ return False
+ if not app.config[
+ "IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS"
+ ] and not is_safe_host(impala_host):
+ logger.warning(
+ "Impala cancel_query refused: target host is not allowed"
+ )
+ return False
url = f"http://{impala_host}:25000/cancel_query?query_id={cancel_query_id}"
- response = requests.post(url, timeout=3)
+ # Do not follow redirects: a validated host could otherwise 30x the
+ # request to an internal target, bypassing the is_safe_host check.
Review Comment:
**Suggestion:** This host validation is vulnerable to DNS rebinding because `is_safe_host()` resolves DNS first, but `requests.post()` performs a fresh DNS resolution when sending the request. An attacker controlling DNS for a hostname can return a public IP during validation and then switch to an internal IP before the HTTP call, bypassing the SSRF guard. Resolve once and pin the outbound connection to the validated IP (or use a transport that enforces resolved-address pinning) instead of validating and then reconnecting by hostname. [ssrf]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Impala cancel_query SSRF guard bypassable via DNS rebinding.
- ⚠️ Backend can issue HTTP requests to internal network hosts.
- ⚠️ Private services exposed whenever Impala hostname uses attacker DNS.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. A client calls the queries API stop endpoint (`@expose("/stop", methods=("POST",))`)
implemented in `superset/queries/api.py:232-271`, which deserializes the JSON body and
invokes `QueryDAO.stop_query(body["client_id"])` at `superset/queries/api.py:10-13`.
2. `QueryDAO.stop_query` (in `superset/daos/query.py:60-24`) loads the `Query` row for the
given `client_id` and, if the query is still running, calls `sql_lab.cancel_query(query)`
at `superset/daos/query.py:20-21`.
3. `sql_lab.cancel_query` (in `superset/sql_lab.py:5-39`) checks for an existing cancel
key in `query.extra` and then creates a new SQLAlchemy engine and cursor with
`query.database.get_sqla_engine(...)`, finally delegating to
`query.database.db_engine_spec.cancel_query(cursor, query, cancel_query_id)` at
`superset/sql_lab.py:32-39`. For an Impala database, this calls
`ImpalaEngineSpec.cancel_query` implemented in `superset/db_engine_specs/impala.py:23-56`.
4. Inside `ImpalaEngineSpec.cancel_query` (`superset/db_engine_specs/impala.py:33-48`),
the code reads `impala_host = query.database.url_object.host` and applies the host check:
`if not app.config["IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS"] and not
is_safe_host(impala_host): ...`. `is_safe_host` (defined in
`superset/utils/network.py:8-34`) calls `socket.getaddrinfo(host, None)` at line 18 and
verifies each returned IP is globally routable; if an attacker-controlled DNS server for
`impala_host` returns a public IP at this point, `is_safe_host` returns `True` and the
guard passes.
5. After validation, `cancel_query` constructs `url =
f"http://{impala_host}:25000/cancel_query?query_id={cancel_query_id}"` and issues
`requests.post(url, timeout=3, allow_redirects=False)` at
`superset/db_engine_specs/impala.py:49-52`. The `requests` library performs its own DNS
resolution for `impala_host` when opening the HTTP connection rather than reusing the
earlier `socket.getaddrinfo` result. An attacker who controls DNS for `impala_host` can
perform DNS rebinding: first returning a public IP during `is_safe_host` so the check
passes, then quickly changing the DNS record so that the subsequent resolution performed
by `requests.post` points to a private or loopback address. This causes the Superset
backend to send the HTTP cancel request to an internal host despite the SSRF guard,
because validation and connection use separate, unpinned DNS lookups and
`allow_redirects=False` only blocks redirect-based bypasses, not this rebinding window.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=889b60e7814241a08d48526f4cead6f8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=889b60e7814241a08d48526f4cead6f8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/impala.py
**Line:** 221:230
**Comment:**
*Ssrf: This host validation is vulnerable to DNS rebinding because `is_safe_host()` resolves DNS first, but `requests.post()` performs a fresh DNS resolution when sending the request. An attacker controlling DNS for a hostname can return a public IP during validation and then switch to an internal IP before the HTTP call, bypassing the SSRF guard. Resolve once and pin the outbound connection to the validated IP (or use a transport that enforces resolved-address pinning) instead of validating and then reconnecting by hostname.
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%2F39301&comment_hash=ccb9a5ba4b573dc5904f5cdc2bf41f4cfdf87c17421a24d26715ea4203efe371&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=ccb9a5ba4b573dc5904f5cdc2bf41f4cfdf87c17421a24d26715ea4203efe371&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4436988954
Looks like the bot comments are nearing the finish line... happy to review when you're done taming those beasts :D
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4383318069
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | dd11b334cbc33ae7baaee93e1b52a9143fba68c6 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69fa62070cb08d000858b6e4 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-39301--superset-docs-preview.netlify.app](https://deploy-preview-39301--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpbCoAgEEDRvcx30hBl1g5axvgiwRdmUUR7z7_L5byQSwq5wgrQgSwU1d7aurvXVOkwlbmQU6nsLJ5d5F3bLsWGtck-PZtunC-W-IAzKolCI6KYhORmhO8H) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-39301--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3082636402
##########
superset/utils/network.py:
##########
@@ -14,14 +14,61 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import ipaddress
import platform
import socket
import subprocess
+# Networks that must never be reached via user-supplied hostnames.
+# Includes loopback, RFC-1918 private ranges, link-local (covers cloud
+# metadata endpoints such as 169.254.169.254), shared address space
+# (RFC 6598, 100.64.0.0/10), and IPv6 equivalents.
+_SSRF_UNSAFE_NETWORKS = (
+ ipaddress.ip_network("0.0.0.0/8"),
+ ipaddress.ip_network("10.0.0.0/8"),
+ ipaddress.ip_network("100.64.0.0/10"),
+ ipaddress.ip_network("127.0.0.0/8"),
+ ipaddress.ip_network("169.254.0.0/16"),
+ ipaddress.ip_network("172.16.0.0/12"),
+ ipaddress.ip_network("192.168.0.0/16"),
+ ipaddress.ip_network("::1/128"),
+ ipaddress.ip_network("fc00::/7"),
+ ipaddress.ip_network("fe80::/10"),
+)
+
PORT_TIMEOUT = 5
PING_TIMEOUT = 5
+def is_safe_host(host: str) -> bool:
+ """
+ Return True if ``host`` resolves exclusively to public, globally-routable
+ IP addresses.
+
+ Returns False if any resolved address falls within a private, loopback,
+ link-local, or otherwise non-routable range. An unresolvable host also
+ returns False.
+ """
+ try:
+ results = socket.getaddrinfo(host, None)
+ except socket.gaierror:
+ return False
+ if not results:
+ return False
+ for _, _, _, _, sockaddr in results:
+ try:
+ ip = ipaddress.ip_address(sockaddr[0])
+ except ValueError:
+ return False
+ # Unwrap IPv4-mapped IPv6 addresses (e.g. ::ffff:127.0.0.1) so they
+ # are checked against the IPv4 unsafe networks rather than bypassing.
+ if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped:
+ ip = ip.ipv4_mapped
+ if any(ip in net for net in _SSRF_UNSAFE_NETWORKS):
Review Comment:
**Suggestion:** The host safety check only blocks a hardcoded subset of non-public ranges, so other non-globally-routable addresses (for example benchmark/test/documentation ranges) are incorrectly treated as safe. Use `ip.is_global` so all non-routable targets are rejected consistently. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset import can call hosts in documentation IP ranges.
- ❌ Connector host validation allows reserved or benchmark-only IP ranges.
- ⚠️ Webhook notifications may target non-global internal-only HTTP endpoints.
- ⚠️ SSRF guard weaker than `is_safe_host` documented behavior.
```
</details>
```suggestion
if not ip.is_global or any(ip in net for net in _SSRF_UNSAFE_NETWORKS):
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start Superset with this PR code and ensure dataset import is enabled. Confirm that
dataset imports use the v1 importer via the dispatcher in
`superset/commands/dataset/importers/dispatcher.py:32-38`, where `command_versions =
[v1.ImportDatasetsCommand, v0.ImportDatasetsCommand]`.
2. Configure dataset import to allow arbitrary HTTPS URLs but still enforce the "no
internal hosts" check: set `DATASET_IMPORT_ALLOWED_DATA_URLS = [r".*"]` and leave
`DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS = False` (default defined in
`superset/config.py:2283` and read in
`superset/commands/dataset/importers/v1/utils.py:122-135`).
3. Use the dataset import REST API implemented in `superset/datasets/api.py:1036-1047` by
POSTing a bundle to the import endpoint so that one dataset config contains a `data` field
pointing to `https://192.0.2.10/data.csv` (192.0.2.0/24 is an IANA documentation-only
range). The API constructs `ImportDatasetsCommand` (dispatcher), which instantiates
`v1.ImportDatasetsCommand` (`superset/commands/dataset/importers/v1/__init__.py:32-71`),
and that in turn calls `import_dataset(config, overwrite=overwrite)` with your config.
4. Inside `import_dataset` (`superset/commands/dataset/importers/v1/utils.py:143-230`),
when `config["data"]` is set, it eventually calls `load_data(data_uri, dataset,
dataset.database)` at line 224. `load_data` normalizes the URL then invokes
`validate_data_uri(data_uri)` at line 245, which resolves the hostname and calls
`is_safe_host(hostname)` at lines 136-138. For `hostname = "192.0.2.10"`,
`socket.getaddrinfo` returns that IP; `is_safe_host` (in
`superset/utils/network.py:43-69`) iterates the results and checks `if any(ip in net for
net in _SSRF_UNSAFE_NETWORKS): return False` (lines 67-68). Because 192.0.2.10 is not part
of any network in `_SSRF_UNSAFE_NETWORKS` (defined at lines 26-36), the condition never
triggers and the function returns `True`, treating this documentation-only,
non-globally-routable address as "safe". The dataset import proceeds to fetch from that
host, contradicting the docstring's promise to allow only "public, globally-routable IP
addresses". With the suggested change `if not ip.is_global or any(ip in net for net in
_SSRF_UNSAFE_NETWORKS): return False`, addresses like 192.0.2.10 (for which
`ipaddress.ip_address("192.0.2.10").is_global` is False) would be rejected and the import
would fail instead of reaching this non-global range.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/utils/network.py
**Line:** 67:67
**Comment:**
*Security: The host safety check only blocks a hardcoded subset of non-public ranges, so other non-globally-routable addresses (for example benchmark/test/documentation ranges) are incorrectly treated as safe. Use `ip.is_global` so all non-routable targets are rejected consistently.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=f8c263510fc01faf176e1de23300bcc92435048bade8ebf75c58a9570d207c1a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=f8c263510fc01faf176e1de23300bcc92435048bade8ebf75c58a9570d207c1a&reaction=dislike'>👎</a>
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -82,12 +106,19 @@ def get_dtype(df: pd.DataFrame, dataset: SqlaTable) -> dict[str, VisitableType]:
def validate_data_uri(data_uri: str) -> None:
"""
- Validate that the data URI is configured on DATASET_IMPORT_ALLOWED_URLS
- has a valid URL.
+ Validate that the data URI is permitted for dataset import.
+
+ Local ``file://`` URIs (used for bundled example data) are always allowed
+ since they do not make network requests. All other URIs must match a
+ pattern in ``DATASET_IMPORT_ALLOWED_DATA_URLS`` *and* resolve to a
+ publicly-routable host.
- :param data_uri:
- :return:
+ :param data_uri: the URI to validate
+ :raises DatasetForbiddenDataURI: if the URI is not permitted
"""
+ if data_uri.startswith("file://"):
+ return
Review Comment:
**Suggestion:** Allowing every `file://` URI unconditionally lets dataset imports read arbitrary local files, bypassing the configured URL allowlist. Restrict local file access to the examples directory (or otherwise enforce an explicit allowlist check) before returning. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset import API reads arbitrary local filesystem files.
- ❌ Attackers with import rights can exfiltrate sensitive host data.
- ⚠️ Violates expectation that file:// used only for examples.
```
</details>
```suggestion
from pathlib import Path
from superset.examples.helpers import get_examples_folder
file_path = Path(urlparse(data_uri).path).resolve()
examples_path = Path(get_examples_folder()).resolve()
if file_path.is_relative_to(examples_path):
return
raise DatasetForbiddenDataURI()
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. As an authenticated user with dataset import permission, call the dataset import
endpoint implemented in `superset/datasets/api.py:1000-1048` by sending `POST
/api/v1/dataset/import/` with a ZIP file payload.
2. Inside the ZIP, include a dataset config file under `datasets/` (handled by
`ImportDatasetsCommand` in `superset/commands/dataset/importers/v1/__init__.py:32-72`)
whose YAML, after loading via `ImportV1DatasetSchema` in
`superset/datasets/schemas.py:56-106`, sets `data` to a local file URL such as
`file:///etc/passwd`.
3. When `ImportDatasetsCommand._import()` calls `import_dataset(config,
overwrite=overwrite)` (`superset/commands/dataset/importers/v1/__init__.py:65-72`),
`import_dataset()` in `superset/commands/dataset/importers/v1/utils.py:143-230` copies
`config`, extracts `data_uri = config.get("data")` at line 193, and, because the target
table does not yet exist, invokes `load_data(data_uri, dataset, dataset.database)` at line
224.
4. In `load_data()` (`superset/commands/dataset/importers/v1/utils.py:233-288`), the URL
is first passed through `normalize_example_data_url()` which returns `file://` URLs
unchanged (`superset/examples/helpers.py:93-115`), then `validate_data_uri()` is called at
line 245; `validate_data_uri()` immediately returns for any `file://` URI without
allowlist or host checks at lines 107-120, and `load_data()` then executes
`opener.open(data_uri)` at line 248 and `pd.read_csv(data, encoding="utf-8")` at line 251,
causing the contents of the arbitrary local file (e.g. `/etc/passwd`) to be read into a
dataset and exposed through Superset.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 120:120
**Comment:**
*Security: Allowing every `file://` URI unconditionally lets dataset imports read arbitrary local files, bypassing the configured URL allowlist. Restrict local file access to the examples directory (or otherwise enforce an explicit allowlist check) before returning.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=d436a52805af2dc80d41762ed94e06a2d4a12cc75bbed81bfa1099465a24508b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=d436a52805af2dc80d41762ed94e06a2d4a12cc75bbed81bfa1099465a24508b&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3408325824
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -88,12 +117,33 @@ def get_dtype(df: pd.DataFrame, dataset: SqlaTable) -> dict[str, VisitableType]:
def validate_data_uri(data_uri: str) -> None:
"""
- Validate that the data URI is configured on DATASET_IMPORT_ALLOWED_URLS
- has a valid URL.
+ Validate that the data URI is permitted for dataset import.
+
+ Local ``file://`` URIs are allowed only when the path is confined to the
+ bundled examples folder. All other URIs must match a pattern in
+ ``DATASET_IMPORT_ALLOWED_DATA_URLS`` *and* resolve to a publicly-routable host.
- :param data_uri:
- :return:
+ :param data_uri: the URI to validate
+ :raises DatasetForbiddenDataURI: if the URI is not permitted
"""
+ if data_uri.startswith("file://"):
Review Comment:
**Suggestion:** The file-scheme gate is case-sensitive, so mixed-case variants like `FiLe://...` bypass the `file://` sandbox check. When `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS` is enabled and the allowlist regex matches broadly, this can allow arbitrary local file reads outside the examples directory. Parse the URI scheme via `urlparse(data_uri).scheme.lower()` and apply the examples-folder restriction to any `file` scheme regardless of input casing. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset import can read arbitrary local files via redirects.
- ❌ Alerts/reports using imported datasets may expose host files.
- ⚠️ Air-gapped deployments enabling internal URLs become exploitable.
- ⚠️ Validation logic contradicts file-URI safety comment in config.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In configuration, enable internal data URLs by setting
`DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS = True` in `superset/config.py:10-14` and keep
the default broad allowlist `DATASET_IMPORT_ALLOWED_DATA_URLS = [r".*"]` as defined in
`superset/config.py:1-8`.
2. Trigger a v1 dataset import that carries a `data` URL, e.g. via the dashboard/dataset
import flow where `import_dataset(config, overwrite=overwrite)` is invoked in
`superset/commands/dataset/importers/v1/__init__.py:10-18`; `import_dataset()` then
assigns `data_uri = config.get("data")` and calls `load_data(data_uri, dataset,
dataset.database)` when `data_uri` is present (see
`superset/commands/dataset/importers/v1/utils.py:248-286`).
3. In `load_data()` (`superset/commands/dataset/importers/v1/utils.py:327-343`), an
HTTP(S) source is opened with an `urllib` opener that installs
`_ValidatingRedirectHandler` (`utils.py:51-72`); when the remote server responds with a
redirect whose `Location` is a mixed-case file URI such as `FiLe:///etc/passwd`,
`_ValidatingRedirectHandler.redirect_request()` builds `absolute_url` and calls
`validate_data_uri(absolute_url)` at `utils.py:70-71`.
4. Inside `validate_data_uri()` (`utils.py:118-164`), the mixed-case `FiLe://` URI does
not satisfy the case-sensitive `if data_uri.startswith("file://"):` check at line 129, so
the examples-folder restriction and `DatasetForbiddenDataURI` checks for file URIs are
skipped; the URI instead matches the broad `DATASET_IMPORT_ALLOWED_DATA_URLS` regex at
`utils.py:147-156`, and because `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS` is `True` at
`utils.py:157-163`, no hostname validation is applied to the file URI, causing the
redirect target `FiLe:///etc/passwd` to be treated as allowed and subsequently opened by
`opener.open(data_uri)` in `load_data()` (line 342), enabling reads of arbitrary local
files outside the examples directory.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=81358197fbcd4b63ae3d0031c65a8f71&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=81358197fbcd4b63ae3d0031c65a8f71&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 129:129
**Comment:**
*Security: The file-scheme gate is case-sensitive, so mixed-case variants like `FiLe://...` bypass the `file://` sandbox check. When `DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS` is enabled and the allowlist regex matches broadly, this can allow arbitrary local file reads outside the examples directory. Parse the URI scheme via `urlparse(data_uri).scheme.lower()` and apply the examples-folder restriction to any `file` scheme regardless of input casing.
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%2F39301&comment_hash=2133958eaf2579ada421bd7e5bda4ea72e024c7c0745ef7865b1fdbed10b99da&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=2133958eaf2579ada421bd7e5bda4ea72e024c7c0745ef7865b1fdbed10b99da&reaction=dislike'>👎</a>
##########
superset/reports/notifications/webhook.py:
##########
@@ -121,9 +151,17 @@ def send(self) -> None:
else:
data[key] = value
- response = requests.post(wh_url, data=data, files=files, timeout=60)
+ response = requests.post(
+ wh_url,
+ data=data,
+ files=files,
+ timeout=60,
+ allow_redirects=False,
+ )
Review Comment:
**Suggestion:** Redirects are now disabled, but the response handling still treats 3xx statuses as success because only `>=400` is considered failure. This can silently drop notifications when webhook endpoints return 301/302/307, since no request is sent to the final target while `send()` logs success. Either add explicit 3xx failure handling (and validate `Location` safely) or restore safe redirect-follow behavior with host revalidation. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Webhook-based alerts can be silently dropped on HTTP 3xx.
- ⚠️ Report scheduler logs success for undelivered webhook notifications.
- ⚠️ Operators cannot reliably detect misconfigured webhook endpoints.
- ⚠️ Backoff/retry logic never triggers for persistent 3xx responses.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure and enable alerts/reports webhooks so that
`feature_flag_manager.is_feature_enabled("ALERT_REPORT_WEBHOOK")` returns True (checked in
`WebhookNotification.send()` at `superset/reports/notifications/webhook.py:135-139`), and
create a report schedule with a webhook recipient (type `ReportRecipientType.WEBHOOK`)
targeting an endpoint that responds with HTTP 302 or 307.
2. When the report executes, `send_notification_to_recipients()` in
`superset/commands/report/execute.py:1-23` iterates over `recipients`, calls
`create_notification(recipient, notification_content)` from
`superset/reports/notifications/__init__.py:25-34`, and for a webhook recipient
instantiates `WebhookNotification` and invokes `notification.send()` at
`execute.py:22-23`.
3. Inside `WebhookNotification.send()`
(`superset/reports/notifications/webhook.py:134-179`), after `_validate_webhook_url()`
passes, the method issues an HTTP request using `requests.post(wh_url, data=data,
files=files, timeout=60, allow_redirects=False)` for payloads with files (lines 154-160)
or `requests.post(wh_url, json=payload, timeout=60, allow_redirects=False)` otherwise
(lines 161-164); if the endpoint returns a redirect (e.g. 302), `requests` returns a
response with `status_code=302` and does not follow the redirect.
4. The subsequent response handling at `webhook.py:166-179` logs `"Webhook sent to %s,
status code: %s"` regardless of 3xx, then raises only on `response.status_code >= 500 or
== 429` (transient) and `response.status_code >= 400` (parameter error); because 3xx codes
are `< 400`, no exception is thrown, `send()` returns successfully to
`send_notification_to_recipients()`, and the scheduler treats the notification as
delivered even though no request was sent to the ultimate target URL and the webhook may
never be processed.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=96b0f3b71bd1488c9c81b9c98e577d0a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=96b0f3b71bd1488c9c81b9c98e577d0a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 154:160
**Comment:**
*Logic Error: Redirects are now disabled, but the response handling still treats 3xx statuses as success because only `>=400` is considered failure. This can silently drop notifications when webhook endpoints return 301/302/307, since no request is sent to the final target while `send()` logs success. Either add explicit 3xx failure handling (and validate `Location` safely) or restore safe redirect-follow behavior with host revalidation.
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%2F39301&comment_hash=9ff1953c37e85e73416b9d8e96df89df115b2b1f8d465d63a1275f5980614e1a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=9ff1953c37e85e73416b9d8e96df89df115b2b1f8d465d63a1275f5980614e1a&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3408456836
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -88,12 +117,33 @@ def get_dtype(df: pd.DataFrame, dataset: SqlaTable) -> dict[str, VisitableType]:
def validate_data_uri(data_uri: str) -> None:
"""
- Validate that the data URI is configured on DATASET_IMPORT_ALLOWED_URLS
- has a valid URL.
+ Validate that the data URI is permitted for dataset import.
+
+ Local ``file://`` URIs are allowed only when the path is confined to the
+ bundled examples folder. All other URIs must match a pattern in
+ ``DATASET_IMPORT_ALLOWED_DATA_URLS`` *and* resolve to a publicly-routable host.
- :param data_uri:
- :return:
+ :param data_uri: the URI to validate
+ :raises DatasetForbiddenDataURI: if the URI is not permitted
"""
+ if data_uri.startswith("file://"):
Review Comment:
Already addressed: the file-scheme check gates on `parsed.scheme == "file"`, and since `urlparse` lower-cases the scheme, mixed-case forms like `FiLe://` are handled identically (474294c).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3408384318
##########
superset/db_engine_specs/impala.py:
##########
@@ -209,8 +210,25 @@ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
"""
try:
impala_host = query.database.url_object.host
+ # The cancel call issues an outbound HTTP request from the
+ # Superset backend to whatever host the DB connection was
+ # configured with; validate it before the call to keep this
+ # path consistent with the dataset-import and webhook URL
+ # checks. Operators with internal Impala targets can opt out
+ # via IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS.
+ if not impala_host:
+ return False
+ if not app.config[
+ "IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS"
+ ] and not is_safe_host(impala_host):
+ logger.warning(
+ "Impala cancel_query refused: target host is not allowed"
+ )
+ return False
url = f"http://{impala_host}:25000/cancel_query?query_id={cancel_query_id}"
- response = requests.post(url, timeout=3)
+ # Do not follow redirects: a validated host could otherwise 30x the
+ # request to an internal target, bypassing the is_safe_host check.
Review Comment:
Good catch on the re-resolution window. The host check here is a defense-in-depth baseline that blocks the direct case. Fully closing the gap between validation and the request means pinning the resolved address across every outbound path (Impala cancel, webhook, dataset fetch), which is a larger cross-cutting change. Tracking that as a dedicated follow-up rather than folding it into this PR; the host validation stays as the baseline here.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4699688529
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset-import): validate hostname in import URL allowlist check [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4231577528
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3293224250
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -34,9 +37,32 @@
from superset.sql.parse import Table
from superset.utils import json
from superset.utils.core import get_user
+from superset.utils.network import is_safe_host
logger = logging.getLogger(__name__)
+
+class _ValidatingRedirectHandler(HTTPRedirectHandler):
+ """Re-validates the redirect target URL before following any HTTP redirect.
+
+ Prevents bypasses where an initial URL passes validation but a subsequent
+ redirect points to a disallowed destination.
+ """
+
+ def redirect_request(
+ self,
+ req: request.Request,
+ fp: Any,
+ code: int,
+ msg: str,
+ headers: Any,
+ newurl: str,
+ ) -> request.Request | None:
+ """Validate each redirect target before delegating to the parent handler."""
+ validate_data_uri(newurl)
+ return super().redirect_request(req, fp, code, msg, headers, newurl)
Review Comment:
Already addressed: the redirect handler resolves `newurl` against `req.full_url` via `urljoin` before validating.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -82,12 +108,27 @@ def get_dtype(df: pd.DataFrame, dataset: SqlaTable) -> dict[str, VisitableType]:
def validate_data_uri(data_uri: str) -> None:
"""
- Validate that the data URI is configured on DATASET_IMPORT_ALLOWED_URLS
- has a valid URL.
+ Validate that the data URI is permitted for dataset import.
+
+ Local ``file://`` URIs are allowed only when the path is confined to the
+ bundled examples folder. All other URIs must match a pattern in
+ ``DATASET_IMPORT_ALLOWED_DATA_URLS`` *and* resolve to a publicly-routable host.
- :param data_uri:
- :return:
+ :param data_uri: the URI to validate
+ :raises DatasetForbiddenDataURI: if the URI is not permitted
"""
+ if data_uri.startswith("file://"):
+ from superset.examples.helpers import get_examples_folder
+
+ # Strip the "file://" prefix to get the filesystem path.
+ file_path = data_uri[len("file://") :]
+ # Resolve symlinks and relative components before comparing.
+ real_path = os.path.realpath(file_path)
+ examples_folder = os.path.realpath(get_examples_folder())
+ if not real_path.startswith(examples_folder + os.sep):
+ raise DatasetForbiddenDataURI()
+ return
Review Comment:
Already addressed: `file://` URIs are now parsed via `urlparse` + `url2pathname`, and non-local netlocs (anything other than empty or `localhost`) are explicitly rejected.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): block SSRF in dataset import URL validation [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4231476186
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39301?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `25.00000%` with `21 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.30%. Comparing base ([`de40b58`](https://app.codecov.io/gh/apache/superset/commit/de40b58e1050c823167a8d98da5a49603bf4cfa1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f5d39c1`](https://app.codecov.io/gh/apache/superset/commit/f5d39c176224627f747c795dc6e670d6f31660ad?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39301?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/utils/network.py](https://app.codecov.io/gh/apache/superset/pull/39301?src=pr&el=tree&filepath=superset%2Futils%2Fnetwork.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvbmV0d29yay5weQ==) | 17.64% | [14 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/commands/dataset/importers/v1/utils.py](https://app.codecov.io/gh/apache/superset/pull/39301?src=pr&el=tree&filepath=superset%2Fcommands%2Fdataset%2Fimporters%2Fv1%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGF0YXNldC9pbXBvcnRlcnMvdjEvdXRpbHMucHk=) | 22.22% | [7 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39301?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
:x: Your project check has failed because the head coverage (99.81%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39301 +/- ##
==========================================
- Coverage 64.42% 55.30% -9.12%
==========================================
Files 2553 2553
Lines 132588 132615 +27
Branches 30758 30764 +6
==========================================
- Hits 85416 73339 -12077
- Misses 45686 58624 +12938
+ Partials 1486 652 -834
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39301/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/39301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/39301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/39301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/39301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.74% <25.00%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/39301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.74% <25.00%> (-20.53%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/39301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/39301/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39301?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4370186218
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3405763260
##########
superset/reports/notifications/webhook.py:
##########
@@ -93,6 +94,31 @@ def _get_files(self) -> list[tuple[str, tuple[str, bytes, str]]]:
)
return files
+ def _validate_webhook_url(self, url: str) -> None:
Review Comment:
**Suggestion:** Add an inline docstring to this newly introduced method describing what it validates and which exceptions it raises. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a newly introduced Python method and it has no docstring in the final file state.
The provided rule explicitly requires new functions and classes to be documented inline,
so this is a real violation.
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=aa6929ce409b493db7b81e480d9d8380&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=aa6929ce409b493db7b81e480d9d8380&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 97:97
**Comment:**
*Custom Rule: Add an inline docstring to this newly introduced method describing what it validates and which exceptions it raises.
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%2F39301&comment_hash=f9f22e6b21ca0170d40c78f47ddb7975e794108bc35c395330785c810c7e5ba2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=f9f22e6b21ca0170d40c78f47ddb7975e794108bc35c395330785c810c7e5ba2&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3408313722
##########
superset/reports/notifications/webhook.py:
##########
@@ -93,6 +94,31 @@ def _get_files(self) -> list[tuple[str, tuple[str, bytes, str]]]:
)
return files
+ def _validate_webhook_url(self, url: str) -> None:
Review Comment:
Added a docstring to `_validate_webhook_url` describing the checks it performs and the `NotificationParamException` it raises in cfe88e7728.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset-import): validate hostname in import URL allowlist check [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3069444137
##########
tests/unit_tests/utils/test_network.py:
##########
@@ -0,0 +1,99 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from unittest.mock import patch
+
+import pytest
+
+from superset.utils.network import is_safe_host
+
+
+@pytest.mark.parametrize(
+ "resolved_ip, expected",
+ [
+ # Public IPs → safe
+ ("93.184.216.34", True), # example.com
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Parametrize argument should be tuple not string</b></div>
<div id="fix">
Change the first argument of `pytest.mark.parametrize` from a string to a tuple: `("resolved_ip", "expected")` instead of `"resolved_ip, expected"`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
("resolved_ip", "expected"),
[
# Public IPs → safe
("93.184.216.34", True), # example.com
````
</div>
</details>
</div>
<small><i>Code Review Run #78c147</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4456130973
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3408384060
##########
superset/reports/notifications/webhook.py:
##########
@@ -121,9 +151,17 @@ def send(self) -> None:
else:
data[key] = value
- response = requests.post(wh_url, data=data, files=files, timeout=60)
+ response = requests.post(
+ wh_url,
+ data=data,
+ files=files,
+ timeout=60,
+ allow_redirects=False,
+ )
Review Comment:
Fixed in 474294c: a 3xx is now treated as a failure. Since redirects are not followed (allow_redirects=False), a 3xx means the request never reached the final target, so it no longer reports success. Added a regression test.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4616829990
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4231897448
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3294566540
##########
tests/unit_tests/utils/test_network.py:
##########
@@ -0,0 +1,99 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from unittest.mock import patch
+
+import pytest
+
+from superset.utils.network import is_safe_host
+
+
+@pytest.mark.parametrize(
+ "resolved_ip, expected",
+ [
+ # Public IPs → safe
+ ("93.184.216.34", True), # example.com
Review Comment:
<!-- Bito Reply -->
The suggestion to change the parametrize argument from a string to a tuple is valid and improves code clarity. The change aligns with Python best practices and ensures better readability and maintainability of the test function.
**tests/unit_tests/utils/test_network.py**
```
("resolved_ip", "expected"),
[
# Public IPs → safe
("93.184.216.34", True), # example.com
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n merged PR #39301:
URL: https://github.com/apache/superset/pull/39301
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4462963231
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3250798343
##########
superset-frontend/src/extensions/ExtensionsStartup.test.tsx:
##########
@@ -259,38 +251,36 @@ test('does not initialize ExtensionsLoader when EnableExtensions feature flag is
initializeSpy.mockRestore();
});
-test('logs error when ExtensionsLoader initialization fails', async () => {
+test('continues rendering children even when ExtensionsLoader initialization fails', async () => {
// Ensure feature flag is enabled
mockIsFeatureEnabled.mockReturnValue(true);
- const errorSpy = jest.spyOn(logging, 'error').mockImplementation();
-
- // Mock the initializeExtensions method to throw an error
+ // Mock the initializeExtensions method to reject — ExtensionsLoader handles
+ // its own error logging internally
const originalInitialize = ExtensionsLoader.prototype.initializeExtensions;
ExtensionsLoader.prototype.initializeExtensions = jest
.fn()
- .mockImplementation(() => {
- throw new Error('Test initialization error');
- });
+ .mockImplementation(() => Promise.resolve());
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Mock setup contradicts test name</b></div>
<div id="fix">
Test name says 'when ExtensionsLoader initialization fails' and comment (line 258-259) says 'Mock the initializeExtensions method to reject', yet the mock uses `Promise.resolve()` instead of `Promise.reject()`. This test does not actually test the failure scenario it claims to test. The mock must resolve to an error for the test assertions to be meaningful.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset-frontend/src/extensions/ExtensionsStartup.test.tsx
+++ b/superset-frontend/src/extensions/ExtensionsStartup.test.tsx
@@ -260,7 +260,7 @@ describe('ExtensionsStartup', () => {
ExtensionsLoader.prototype.initializeExtensions = jest
.fn()
- .mockImplementation(() => Promise.resolve());
+ .mockRejectedValue(new Error('Test initialization error'));
const { container } = render(
<ExtensionsStartup>
```
</div>
</details>
</div>
<small><i>Code Review Run #a63702</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
docs/versioned_docs/version-6.0.0/faq.mdx:
##########
@@ -51,11 +51,11 @@ multiple tables as long as your database account has access to the tables.
## How do I create my own visualization?
We recommend reading the instructions in
-[Creating Visualization Plugins](/docs/6.0.0/contributing/howtos#creating-visualization-plugins).
+[Creating Visualization Plugins](/user-docs/6.0.0/contributing/howtos#creating-visualization-plugins).
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broken link to wrong doc section</b></div>
<div id="fix">
The 'Creating Visualization Plugins' section is located in `developer_docs/contributing/howtos.md` (line 58), not in user-docs. Users clicking this link will encounter a 404 or incorrect page. The correct path is `/developer-docs/contributing/howtos#creating-visualization-plugins`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- docs/versioned_docs/version-6.0.0/faq.mdx
+++ docs/versioned_docs/version-6.0.0/faq.mdx
@@ -51,7 +51,7 @@
## How do I create my own visualization?
We recommend reading the instructions in
-[Creating Visualization Plugins](/user-docs/6.0.0/contributing/howtos#creating-visualization-plugins).
+[Creating Visualization Plugins](/developer-docs/contributing/howtos#creating-visualization-plugins).
## Can I upload and visualize CSV data?
```
</div>
</details>
</div>
<small><i>Code Review Run #a63702</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset/mcp_service/chart/tool/get_chart_sql.py:
##########
@@ -32,6 +32,13 @@
from superset.commands.explore.form_data.parameters import CommandParameters
from superset.exceptions import SupersetException, SupersetSecurityException
from superset.extensions import event_logger
+from superset.mcp_service.chart.chart_helpers import (
+ build_query_context_from_form_data,
+ extract_x_axis_col,
+ resolve_groupby,
+ resolve_metrics,
+ resolve_metrics_and_groupby,
+)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unused imports and wrappers</b></div>
<div id="fix">
Four imported functions from `chart_helpers` are unused: `extract_x_axis_col`, `resolve_groupby`, `resolve_metrics`, and `resolve_metrics_and_groupby`. Their wrapper functions are defined but never called elsewhere in the module. This dead code increases maintenance burden and could confuse future contributors.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- a/superset/mcp_service/chart/tool/get_chart_sql.py
+++ b/superset/mcp_service/chart/tool/get_chart_sql.py
@@ -32,13 +32,7 @@ from superset.commands.explore.form_data.parameters import CommandParameters
from superset.exceptions import SupersetException, SupersetSecurityException
from superset.extensions import event_logger
from superset.mcp_service.chart.chart_helpers import (
build_query_context_from_form_data,
- extract_x_axis_col,
- resolve_groupby,
- resolve_metrics,
- resolve_metrics_and_groupby,
)
from superset.mcp_service.chart.chart_utils import validate_chart_dataset
from superset.mcp_service.chart.schemas import (
@@ -77,23 +71,6 @@ def _sanitize_chart_sql_for_llm_context(chart_sql: ChartSql) -> ChartSql:
return None
-def _resolve_metrics(form_data: dict[str, Any], viz_type: str) -> list[Any]:
- """Extract metrics from form_data, handling chart-type-specific fields."""
- return resolve_metrics(form_data, viz_type)
-
-
-def _resolve_groupby(form_data: dict[str, Any]) -> list[Any]:
- """Extract groupby columns from form_data with fallback aliases."""
- return resolve_groupby(form_data)
-
-
def _resolve_metrics_and_groupby(
- form_data: dict[str, Any],
- chart: "Slice | None",
-) -> tuple[list[Any], list[Any]]:
- """Resolve metrics and groupby columns from form_data."""
- return resolve_metrics_and_groupby(form_data, chart)
-
-
def _extract_x_axis_col(form_data: dict[str, Any]) -> str | None:
- """Return the x_axis column name from form_data, or None if not set."""
- return extract_x_axis_col(form_data)
-
-
def _build_query_context_from_form_data(
```
</div>
</details>
</div>
<small><i>Code Review Run #a63702</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
docs/versioned_docs/version-6.0.0/contributing/guidelines.mdx:
##########
@@ -57,7 +57,7 @@ Finally, never submit a PR that will put master branch in broken state. If the P
in `requirements.txt` pinned to a specific version which ensures that the application
build is deterministic.
- For TypeScript/JavaScript, include new libraries in `package.json`
-- **Tests:** The pull request should include tests, either as doctests, unit tests, or both. Make sure to resolve all errors and test failures. See [Testing](/docs/6.0.0/contributing/howtos#testing) for how to run tests.
+- **Tests:** The pull request should include tests, either as doctests, unit tests, or both. Make sure to resolve all errors and test failures. See [Testing](/user-docs/6.0.0/contributing/howtos#testing) for how to run tests.
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Broken documentation link</b></div>
<div id="fix">
Update the test documentation link to use a relative path to the local howtos file. For example:
```md
See [Testing](howtos#testing) for how to run tests.
```
</div>
</div>
<small><i>Code Review Run #a63702</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3294565361
##########
superset/utils/network.py:
##########
@@ -14,14 +14,55 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import ipaddress
import platform
import socket
import subprocess
+# Networks that must never be reached via user-supplied hostnames.
+# Includes loopback, RFC-1918 private ranges, link-local (covers cloud
+# metadata endpoints such as 169.254.169.254), and IPv6 equivalents.
+_SSRF_UNSAFE_NETWORKS = (
+ ipaddress.ip_network("0.0.0.0/8"),
+ ipaddress.ip_network("10.0.0.0/8"),
+ ipaddress.ip_network("127.0.0.0/8"),
+ ipaddress.ip_network("169.254.0.0/16"),
+ ipaddress.ip_network("172.16.0.0/12"),
+ ipaddress.ip_network("192.168.0.0/16"),
+ ipaddress.ip_network("::1/128"),
+ ipaddress.ip_network("fc00::/7"),
+ ipaddress.ip_network("fe80::/10"),
+)
+
PORT_TIMEOUT = 5
PING_TIMEOUT = 5
+def is_safe_host(host: str) -> bool:
+ """
+ Return True if ``host`` resolves exclusively to public, globally-routable
+ IP addresses.
+
+ Returns False if any resolved address falls within a private, loopback,
+ link-local, or otherwise non-routable range. An unresolvable host also
+ returns False.
+ """
+ try:
+ results = socket.getaddrinfo(host, None)
+ except socket.gaierror:
+ return False
+ if not results:
+ return False
+ for _, _, _, _, sockaddr in results:
+ try:
+ ip = ipaddress.ip_address(sockaddr[0])
+ except ValueError:
+ return False
+ if any(ip in net for net in _SSRF_UNSAFE_NETWORKS):
+ return False
Review Comment:
Addressed in 9cd14699: is_safe_host now rejects any address where ip.is_global is False (covers IPv4-mapped IPv6, translated, and special-use forms) and keeps the explicit blocklist as defense-in-depth.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -82,12 +84,20 @@ def get_dtype(df: pd.DataFrame, dataset: SqlaTable) -> dict[str, VisitableType]:
def validate_data_uri(data_uri: str) -> None:
Review Comment:
Addressed in dfb304fa: load_data now uses a custom opener with _ValidatingRedirectHandler, which calls validate_data_uri() on every redirect target before following.
##########
superset/utils/network.py:
##########
@@ -14,14 +14,55 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import ipaddress
import platform
import socket
import subprocess
+# Networks that must never be reached via user-supplied hostnames.
+# Includes loopback, RFC-1918 private ranges, link-local (covers cloud
+# metadata endpoints such as 169.254.169.254), and IPv6 equivalents.
+_SSRF_UNSAFE_NETWORKS = (
+ ipaddress.ip_network("0.0.0.0/8"),
+ ipaddress.ip_network("10.0.0.0/8"),
+ ipaddress.ip_network("127.0.0.0/8"),
Review Comment:
Addressed in 9cd14699: 100.64.0.0/10 added to _SSRF_UNSAFE_NETWORKS.
##########
superset/utils/network.py:
##########
@@ -14,14 +14,55 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import ipaddress
import platform
import socket
import subprocess
+# Networks that must never be reached via user-supplied hostnames.
+# Includes loopback, RFC-1918 private ranges, link-local (covers cloud
+# metadata endpoints such as 169.254.169.254), and IPv6 equivalents.
+_SSRF_UNSAFE_NETWORKS = (
+ ipaddress.ip_network("0.0.0.0/8"),
+ ipaddress.ip_network("10.0.0.0/8"),
+ ipaddress.ip_network("127.0.0.0/8"),
+ ipaddress.ip_network("169.254.0.0/16"),
+ ipaddress.ip_network("172.16.0.0/12"),
+ ipaddress.ip_network("192.168.0.0/16"),
+ ipaddress.ip_network("::1/128"),
+ ipaddress.ip_network("fc00::/7"),
+ ipaddress.ip_network("fe80::/10"),
+)
+
PORT_TIMEOUT = 5
PING_TIMEOUT = 5
+def is_safe_host(host: str) -> bool:
+ """
+ Return True if ``host`` resolves exclusively to public, globally-routable
+ IP addresses.
+
+ Returns False if any resolved address falls within a private, loopback,
+ link-local, or otherwise non-routable range. An unresolvable host also
+ returns False.
+ """
+ try:
+ results = socket.getaddrinfo(host, None)
+ except socket.gaierror:
+ return False
+ if not results:
+ return False
+ for _, _, _, _, sockaddr in results:
+ try:
+ ip = ipaddress.ip_address(sockaddr[0])
+ except ValueError:
Review Comment:
Addressed in 04d042e5: IPv4-mapped IPv6 addresses are unwrapped to their IPv4 form before unsafe-network checks.
##########
tests/unit_tests/utils/test_network.py:
##########
@@ -0,0 +1,99 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from unittest.mock import patch
+
+import pytest
+
+from superset.utils.network import is_safe_host
+
+
+@pytest.mark.parametrize(
+ "resolved_ip, expected",
+ [
+ # Public IPs → safe
+ ("93.184.216.34", True), # example.com
Review Comment:
Addressed in de455257: parametrize argnames now use the tuple form.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -82,12 +106,19 @@ def get_dtype(df: pd.DataFrame, dataset: SqlaTable) -> dict[str, VisitableType]:
def validate_data_uri(data_uri: str) -> None:
"""
- Validate that the data URI is configured on DATASET_IMPORT_ALLOWED_URLS
- has a valid URL.
+ Validate that the data URI is permitted for dataset import.
+
+ Local ``file://`` URIs (used for bundled example data) are always allowed
+ since they do not make network requests. All other URIs must match a
+ pattern in ``DATASET_IMPORT_ALLOWED_DATA_URLS`` *and* resolve to a
+ publicly-routable host.
- :param data_uri:
- :return:
+ :param data_uri: the URI to validate
+ :raises DatasetForbiddenDataURI: if the URI is not permitted
"""
+ if data_uri.startswith("file://"):
+ return
Review Comment:
Addressed in 976afcf7 and 04d042e5: file:// URIs are now restricted to the bundled examples folder via os.path.realpath comparison, and non-localhost authority components are rejected.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4599991382
> Looks like CI just needs a little love, then we can merge this!
Fixed now, thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4232747408
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): block SSRF in dataset import URL validation [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3069431835
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -777,9 +777,60 @@ def test_import_dataset_without_owner_permission(
],
)
def test_validate_data_uri(allowed_urls, data_uri, expected, exception_class):
+ """Tests allowlist pattern matching. is_safe_host is stubbed out so that
+ fake/unresolvable test hostnames do not interfere with DNS-based checks
+ (those are covered by the dedicated is_safe_host tests below)."""
current_app.config["DATASET_IMPORT_ALLOWED_DATA_URLS"] = allowed_urls
- if expected:
- validate_data_uri(data_uri)
- else:
- with pytest.raises(exception_class):
+ current_app.config["DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS"] = False
+ with patch(
+ "superset.commands.dataset.importers.v1.utils.is_safe_host",
+ return_value=True,
+ ):
+ if expected:
validate_data_uri(data_uri)
+ else:
+ with pytest.raises(exception_class):
+ validate_data_uri(data_uri)
+
+
+def test_validate_data_uri_file_scheme_always_allowed():
+ """file:// URIs must always pass regardless of allowlist or SSRF guards."""
+ current_app.config["DATASET_IMPORT_ALLOWED_DATA_URLS"] = []
+ current_app.config["DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS"] = False
+ validate_data_uri("file:///tmp/data.csv")
+
+
+@pytest.mark.parametrize(
+ "data_uri",
+ [
+ # Userinfo-injection: allowlist matches the trusted hostname in the
+ # authority but urlparse().hostname resolves to the actual target.
+ "https://allowed.example.com@169.254.169.254/latest/meta-data/",
+ "https://allowed.example.com@10.0.0.1/internal",
+ "https://allowed.example.com@127.0.0.1/admin",
+ ],
+)
+def test_validate_data_uri_blocks_userinfo_ssrf_injection(data_uri):
Review Comment:
**Suggestion:** Add explicit parameter and return type annotations to this newly added test function. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
def test_validate_data_uri_blocks_userinfo_ssrf_injection(data_uri: str) -> None:
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This parametrized test is missing both a parameter annotation and a `-> None` return annotation, unlike the surrounding tests. Adding `data_uri: str` and `-> None` is valid Python, compatible with pytest parametrization, and does not alter runtime behavior.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/datasets/commands/importers/v1/import_test.py
**Line:** 813:813
**Comment:**
*Custom Rule: Add explicit parameter and return type annotations to this newly added test function.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=0733a89e5f963eb5505d4550c65679371dc42f96323eeff9edc8b622747d3ca5&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=0733a89e5f963eb5505d4550c65679371dc42f96323eeff9edc8b622747d3ca5&reaction=dislike'>👎</a>
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -777,9 +777,60 @@ def test_import_dataset_without_owner_permission(
],
)
def test_validate_data_uri(allowed_urls, data_uri, expected, exception_class):
+ """Tests allowlist pattern matching. is_safe_host is stubbed out so that
+ fake/unresolvable test hostnames do not interfere with DNS-based checks
+ (those are covered by the dedicated is_safe_host tests below)."""
current_app.config["DATASET_IMPORT_ALLOWED_DATA_URLS"] = allowed_urls
- if expected:
- validate_data_uri(data_uri)
- else:
- with pytest.raises(exception_class):
+ current_app.config["DATASET_IMPORT_ALLOW_INTERNAL_DATA_URLS"] = False
+ with patch(
+ "superset.commands.dataset.importers.v1.utils.is_safe_host",
+ return_value=True,
+ ):
+ if expected:
validate_data_uri(data_uri)
+ else:
+ with pytest.raises(exception_class):
+ validate_data_uri(data_uri)
+
+
+def test_validate_data_uri_file_scheme_always_allowed():
Review Comment:
**Suggestion:** Add an explicit return type annotation to this newly added test function. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
def test_validate_data_uri_file_scheme_always_allowed() -> None:
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new test function is missing the project's common `-> None` return annotation, while other tests in the file use it consistently. Adding the annotation is syntactically valid and does not change behavior.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/datasets/commands/importers/v1/import_test.py
**Line:** 796:796
**Comment:**
*Custom Rule: Add an explicit return type annotation to this newly added test function.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=19e169f408b7c743b6ecaecbd5d3a683b8d145f2e205516badadfcea14ac6843&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=19e169f408b7c743b6ecaecbd5d3a683b8d145f2e205516badadfcea14ac6843&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(security): block SSRF in dataset import URL validation [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4231470787
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4607982451
Sending some comments as a DM...
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4323152630
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4384821661
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "hainenber (via GitHub)" <gi...@apache.org>.
hainenber commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3275151923
##########
superset-frontend/packages/superset-ui-core/package.json:
##########
@@ -56,7 +56,7 @@
"react-js-cron": "^5.2.0",
"react-markdown": "^8.0.7",
"react-resize-detector": "^7.1.2",
- "react-syntax-highlighter": "^16.1.1",
+ "react-syntax-highlighter": "^16.1.0",
Review Comment:
Why is this getting downgraded? I don't see any frontend changes
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4695970224
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3082620941
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -34,9 +36,31 @@
from superset.sql.parse import Table
from superset.utils import json
from superset.utils.core import get_user
+from superset.utils.network import is_safe_host
logger = logging.getLogger(__name__)
+
+class _ValidatingRedirectHandler(HTTPRedirectHandler):
+ """Re-validates the redirect target URL before following any HTTP redirect.
+
+ Prevents bypasses where an initial URL passes validation but a subsequent
+ redirect points to a disallowed destination.
+ """
+
+ def redirect_request(
+ self,
+ req: request.Request,
+ fp: Any,
+ code: int,
+ msg: str,
+ headers: Any,
+ newurl: str,
+ ) -> request.Request | None:
+ validate_data_uri(newurl)
Review Comment:
**Suggestion:** Add a docstring to the newly introduced method so all new function definitions are documented. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
"""Validate each redirect target before delegating to the parent handler."""
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new method `redirect_request` is introduced without a docstring, which matches the stated documentation rule. The improved code adds a concise docstring without changing behavior, and the method remains syntactically valid and executable.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/dataset/importers/v1/utils.py
**Line:** 60:60
**Comment:**
*Custom Rule: Add a docstring to the newly introduced method so all new function definitions are documented.
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.
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=79eb0a1f1c467d113544a7074eb6c9d6b2084d3e21c7d591d3cd451a64569d58&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39301&comment_hash=79eb0a1f1c467d113544a7074eb6c9d6b2084d3e21c7d591d3cd451a64569d58&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4694570564
@rusackas the databend spec was reverted to `is_hostname_valid` (so internal/RFC-1918 Databend hosts save and test fine); couchbase and databricks were reverted too. The host check now only applies to the outbound-fetch paths (dataset import, webhook), and impala's cancel_query gained an `IMPALA_CANCEL_QUERY_ALLOW_INTERNAL_HOSTS` opt-out. Thanks for the review!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4231780011
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4384095046
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #39301:
URL: https://github.com/apache/superset/pull/39301#discussion_r3190475316
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -34,9 +37,32 @@
from superset.sql.parse import Table
from superset.utils import json
from superset.utils.core import get_user
+from superset.utils.network import is_safe_host
logger = logging.getLogger(__name__)
+
+class _ValidatingRedirectHandler(HTTPRedirectHandler):
+ """Re-validates the redirect target URL before following any HTTP redirect.
+
+ Prevents bypasses where an initial URL passes validation but a subsequent
+ redirect points to a disallowed destination.
+ """
+
+ def redirect_request(
+ self,
+ req: request.Request,
+ fp: Any,
+ code: int,
+ msg: str,
+ headers: Any,
+ newurl: str,
+ ) -> request.Request | None:
+ """Validate each redirect target before delegating to the parent handler."""
+ validate_data_uri(newurl)
+ return super().redirect_request(req, fp, code, msg, headers, newurl)
Review Comment:
`newurl` provided by `HTTPRedirectHandler` may be a relative URL (e.g. `"/download.csv"`). Validating the relative string directly will likely fail the allowlist and can break legitimate redirects; it also doesn’t validate the *actual* resolved target. Resolve the redirect against the request URL (e.g., via `urljoin(req.full_url, newurl)`) and validate the resulting absolute URL instead.
##########
superset/db_engine_specs/databricks.py:
##########
@@ -393,7 +393,7 @@ def validate_parameters( # type: ignore
if not host:
return errors
- if not is_hostname_valid(host): # type: ignore
+ if not is_safe_host(host): # type: ignore
errors.append(
SupersetError(
message="The hostname provided can't be resolved.",
Review Comment:
`is_safe_host()` can fail not only when a hostname can’t be resolved, but also when it resolves to a private/link-local/etc. address. This error message is now misleading; consider updating it to reflect both cases (e.g., “Host can’t be resolved or is not allowed”) or differentiating between resolution failure vs. disallowed address space.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -82,12 +108,27 @@ def get_dtype(df: pd.DataFrame, dataset: SqlaTable) -> dict[str, VisitableType]:
def validate_data_uri(data_uri: str) -> None:
"""
- Validate that the data URI is configured on DATASET_IMPORT_ALLOWED_URLS
- has a valid URL.
+ Validate that the data URI is permitted for dataset import.
+
+ Local ``file://`` URIs are allowed only when the path is confined to the
+ bundled examples folder. All other URIs must match a pattern in
+ ``DATASET_IMPORT_ALLOWED_DATA_URLS`` *and* resolve to a publicly-routable host.
- :param data_uri:
- :return:
+ :param data_uri: the URI to validate
+ :raises DatasetForbiddenDataURI: if the URI is not permitted
"""
+ if data_uri.startswith("file://"):
+ from superset.examples.helpers import get_examples_folder
+
+ # Strip the "file://" prefix to get the filesystem path.
+ file_path = data_uri[len("file://") :]
+ # Resolve symlinks and relative components before comparing.
+ real_path = os.path.realpath(file_path)
+ examples_folder = os.path.realpath(get_examples_folder())
+ if not real_path.startswith(examples_folder + os.sep):
+ raise DatasetForbiddenDataURI()
+ return
Review Comment:
Parsing `file://` URIs by string slicing is brittle and can mis-handle valid forms like `file://localhost/...` and URL-encoded paths; it also doesn’t explicitly reject non-local authorities (non-empty netloc). Use `urlparse()` to validate the scheme/netloc and convert the URL path to a filesystem path (e.g., via `urllib.request.url2pathname(parsed.path)`), and explicitly reject any non-empty netloc except possibly `localhost`.
##########
superset/db_engine_specs/couchbase.py:
##########
@@ -228,7 +228,7 @@ def validate_parameters(
if not host:
return errors
# host can be a connection string in case of couchbase cloud. So Connection Check is not required in that case. # noqa: E501
- if not is_hostname_valid(host):
+ if not is_safe_host(host):
Review Comment:
The adjacent comment states the Couchbase “host” can be a connection string for Couchbase Cloud, in which case a raw DNS resolution check on the entire string will fail (and `is_safe_host()` will reject). If connection strings are supported here, extract and validate only the hostname portion (or bypass this check for connection-string forms) so Cloud configs aren’t inadvertently blocked.
##########
superset/reports/notifications/webhook.py:
##########
@@ -93,6 +94,16 @@ def _get_files(self) -> list[tuple[str, tuple[str, bytes, str]]]:
)
return files
+ def _validate_webhook_url(self, url: str) -> None:
+ parsed = urlparse(url)
+ if current_app.config["ALERT_REPORTS_WEBHOOK_HTTPS_ONLY"]:
+ if parsed.scheme.lower() != "https":
+ raise NotificationParamException(
+ "Webhook failed: HTTPS is required by config for webhook URLs."
+ )
+ if parsed.hostname and not is_safe_host(parsed.hostname):
Review Comment:
`_validate_webhook_url` only validates `parsed.hostname` when it exists, which means URLs without a hostname (e.g. malformed URLs or non-HTTP(S) schemes like `file://...`) won’t be rejected here and will fail later in less controlled ways. Consider explicitly enforcing allowed schemes (http/https) and requiring a hostname/netloc so invalid or unsupported webhook URLs are rejected with a clear `NotificationParamException`.
##########
superset/db_engine_specs/impala.py:
##########
@@ -209,6 +210,8 @@ def cancel_query(cls, cursor: Any, query: Query, cancel_query_id: str) -> bool:
"""
try:
impala_host = query.database.url_object.host
+ if not impala_host or not is_safe_host(impala_host):
+ return False
url = f"http://{impala_host}:25000/cancel_query?query_id={cancel_query_id}"
response = requests.post(url, timeout=3)
Review Comment:
`is_safe_host()` rejects RFC1918/private/link-local addresses, which can disable `cancel_query()` for common Impala deployments running on internal networks. If the intent is SSRF-hardening for user-controlled inputs, consider scoping this to the highest-risk ranges (e.g. loopback/link-local/IMDS) or gating private-network blocking behind a configuration flag so on-prem/internal Impala setups can still cancel queries.
##########
superset/utils/network.py:
##########
@@ -14,14 +14,61 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
+import ipaddress
import platform
import socket
import subprocess
+# Networks that must never be reached via user-supplied hostnames.
+# Includes loopback, RFC-1918 private ranges, link-local (covers cloud
+# metadata endpoints such as 169.254.169.254), shared address space
+# (RFC 6598, 100.64.0.0/10), and IPv6 equivalents.
+_SSRF_UNSAFE_NETWORKS = (
+ ipaddress.ip_network("0.0.0.0/8"),
+ ipaddress.ip_network("10.0.0.0/8"),
+ ipaddress.ip_network("100.64.0.0/10"),
+ ipaddress.ip_network("127.0.0.0/8"),
+ ipaddress.ip_network("169.254.0.0/16"),
+ ipaddress.ip_network("172.16.0.0/12"),
+ ipaddress.ip_network("192.168.0.0/16"),
+ ipaddress.ip_network("::1/128"),
+ ipaddress.ip_network("fc00::/7"),
+ ipaddress.ip_network("fe80::/10"),
+)
+
PORT_TIMEOUT = 5
PING_TIMEOUT = 5
+def is_safe_host(host: str) -> bool:
+ """
+ Return True if ``host`` resolves exclusively to public, globally-routable
+ IP addresses.
+
+ Returns False if any resolved address falls within a private, loopback,
+ link-local, or otherwise non-routable range. An unresolvable host also
+ returns False.
+ """
+ try:
+ results = socket.getaddrinfo(host, None)
+ except socket.gaierror:
+ return False
+ if not results:
+ return False
+ for _, _, _, _, sockaddr in results:
+ try:
+ ip = ipaddress.ip_address(sockaddr[0])
+ except ValueError:
+ return False
+ # Unwrap IPv4-mapped IPv6 addresses (e.g. ::ffff:127.0.0.1) so they
+ # are checked against the IPv4 unsafe networks rather than bypassing.
+ if isinstance(ip, ipaddress.IPv6Address) and ip.ipv4_mapped:
+ ip = ip.ipv4_mapped
+ if any(ip in net for net in _SSRF_UNSAFE_NETWORKS):
Review Comment:
The docstring says “public, globally-routable” but the implementation only blocks a subset of non-global ranges. This currently allows other non-routable/special-use targets (e.g., multicast, reserved, documentation ranges, unspecified `0.0.0.0` / `::`, etc.) which weakens SSRF protection. Consider basing the decision on `ip.is_global` (fail closed if not global) and keeping the explicit blocklist (or expanding it) for clarity.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "sha174n (via GitHub)" <gi...@apache.org>.
sha174n commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4506831983
@hainenber Good catch! You're absolutely right. That dependency change was unrelated and shouldn't have been in this PR.
I've removed it in 1588f0b so the PR now focuses purely on the hostname validation logic.
Thanks for the careful review!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(network): validate target hostname in outbound requests [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #39301:
URL: https://github.com/apache/superset/pull/39301#issuecomment-4452492658
A few things before approving:
- The blocklist is fail-open — ip.is_global should be the primary gate; multicast, reserved, and 0.0.0.0 ranges currently slip through
- In validate_data_uri, a falsy hostname silently passes instead of raising
- Test coverage is ~21% on the security-critical paths — worth shoring up before merge
Happy to approve once the blocklist and coverage gaps are addressed.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org